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@xxxxxxxxxxxxxxxx>
>> ---
>> 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
>
