LinuxTV Patchwork [02/13] si2157: Check error status bit on cmd execute

login
register
mail settings
Submitter Brad Love
Date Dec. 29, 2018, 5:51 p.m.
Message ID <1546105882-15693-3-git-send-email-brad@nextdimension.cc>
Download mbox | patch
Permalink /patch/53673/
State Superseded
Headers show

Comments

Brad Love - Dec. 29, 2018, 5:51 p.m.
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(-)
Antti Palosaari - Jan. 9, 2019, 6:01 p.m.
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
Brad Love - Jan. 15, 2019, 5:28 p.m.
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
>

Patch

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));

Privacy Policy