| Message ID | 1546105882-15693-3-git-send-email-brad@nextdimension.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | si2157: Analog tuning and optimizations | expand |
On 12/29/18 7:51 PM, Brad Love wrote: > Check error status bit on command execute, if error bit is > set return -EAGAIN. Ignore -EAGAIN in probe during device check. > > Signed-off-by: Brad Love <brad@nextdimension.cc> > --- > drivers/media/tuners/si2157.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c > index 4855448..3924c42 100644 > --- a/drivers/media/tuners/si2157.c > +++ b/drivers/media/tuners/si2157.c > @@ -56,14 +56,20 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) > break; > } > > - dev_dbg(&client->dev, "cmd execution took %d ms\n", > + dev_dbg(&client->dev, "cmd execution took %d ms, status=%x\n", > jiffies_to_msecs(jiffies) - > - (jiffies_to_msecs(timeout) - TIMEOUT)); > + (jiffies_to_msecs(timeout) - TIMEOUT), > + cmd->args[0]); > > if (!((cmd->args[0] >> 7) & 0x01)) { > ret = -ETIMEDOUT; > goto err_mutex_unlock; > } > + /* check error status bit */ > + if (cmd->args[0] & 0x40) { > + ret = -EAGAIN; > + goto err_mutex_unlock; > + } > } > > mutex_unlock(&dev->i2c_mutex); > @@ -477,7 +483,7 @@ static int si2157_probe(struct i2c_client *client, > cmd.wlen = 0; > cmd.rlen = 1; > ret = si2157_cmd_execute(client, &cmd); > - if (ret) > + if (ret && (ret != -EAGAIN)) > goto err_kfree; > > memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct dvb_tuner_ops)); > So you added check if firmware returns error during command execution, but that error is still skipped during probe, which does not feel correct. Chip should work during probe and ideally driver should ensure it is correct chip. At least you should read some property value or execute some other command without failure. regards Antti
Hi Antti, On 09/01/2019 12.01, Antti Palosaari wrote: > On 12/29/18 7:51 PM, Brad Love wrote: >> Check error status bit on command execute, if error bit is >> set return -EAGAIN. Ignore -EAGAIN in probe during device check. >> >> Signed-off-by: Brad Love <brad@nextdimension.cc> >> --- >> drivers/media/tuners/si2157.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/tuners/si2157.c >> b/drivers/media/tuners/si2157.c >> index 4855448..3924c42 100644 >> --- a/drivers/media/tuners/si2157.c >> +++ b/drivers/media/tuners/si2157.c >> @@ -56,14 +56,20 @@ static int si2157_cmd_execute(struct i2c_client >> *client, struct si2157_cmd *cmd) >> break; >> } >> - dev_dbg(&client->dev, "cmd execution took %d ms\n", >> + dev_dbg(&client->dev, "cmd execution took %d ms, status=%x\n", >> jiffies_to_msecs(jiffies) - >> - (jiffies_to_msecs(timeout) - TIMEOUT)); >> + (jiffies_to_msecs(timeout) - TIMEOUT), >> + cmd->args[0]); >> if (!((cmd->args[0] >> 7) & 0x01)) { >> ret = -ETIMEDOUT; >> goto err_mutex_unlock; >> } >> + /* check error status bit */ >> + if (cmd->args[0] & 0x40) { >> + ret = -EAGAIN; >> + goto err_mutex_unlock; >> + } >> } >> mutex_unlock(&dev->i2c_mutex); >> @@ -477,7 +483,7 @@ static int si2157_probe(struct i2c_client *client, >> cmd.wlen = 0; >> cmd.rlen = 1; >> ret = si2157_cmd_execute(client, &cmd); >> - if (ret) >> + if (ret && (ret != -EAGAIN)) >> goto err_kfree; >> memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct >> dvb_tuner_ops)); >> > > So you added check if firmware returns error during command execution, > but that error is still skipped during probe, which does not feel > correct. Chip should work during probe and ideally driver should > ensure it is correct chip. At least you should read some property > value or execute some other command without failure. > The error status flags have not been enabled yet. The bits will be set and the cmd_execute will return -EAGAIN even if the chip is there, until the status flags are on. Probe currently only contains a sanity check. The chip identification happens in si2157_init, some code could (and I think should) be moved from init into probe, but that is reorganization unrelated to this series. I'll think about that and probably submit a patch to address it. Regards, Brad > regards > Antti >
diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c index 4855448..3924c42 100644 --- a/drivers/media/tuners/si2157.c +++ b/drivers/media/tuners/si2157.c @@ -56,14 +56,20 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd) break; } - dev_dbg(&client->dev, "cmd execution took %d ms\n", + dev_dbg(&client->dev, "cmd execution took %d ms, status=%x\n", jiffies_to_msecs(jiffies) - - (jiffies_to_msecs(timeout) - TIMEOUT)); + (jiffies_to_msecs(timeout) - TIMEOUT), + cmd->args[0]); if (!((cmd->args[0] >> 7) & 0x01)) { ret = -ETIMEDOUT; goto err_mutex_unlock; } + /* check error status bit */ + if (cmd->args[0] & 0x40) { + ret = -EAGAIN; + goto err_mutex_unlock; + } } mutex_unlock(&dev->i2c_mutex); @@ -477,7 +483,7 @@ static int si2157_probe(struct i2c_client *client, cmd.wlen = 0; cmd.rlen = 1; ret = si2157_cmd_execute(client, &cmd); - if (ret) + if (ret && (ret != -EAGAIN)) goto err_kfree; memcpy(&fe->ops.tuner_ops, &si2157_ops, sizeof(struct dvb_tuner_ops));
Check error status bit on command execute, if error bit is set return -EAGAIN. Ignore -EAGAIN in probe during device check. Signed-off-by: Brad Love <brad@nextdimension.cc> --- drivers/media/tuners/si2157.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)