Re: [PATCH 09/12] tda10071: use jiffies when poll firmware status

[Antti Palosaari <crope@iki.fi>]

On 08/11/2015 01:20 PM, Mauro Carvalho Chehab wrote:
> Em Thu,  9 Jul 2015 07:06:29 +0300
> Antti Palosaari <crope@iki.fi> escreveu:
>
>> Use jiffies to set timeout for firmware command status polling.
>> It is more elegant solution than poll X times with sleep.

>>   	/* wait cmd execution terminate */
>> -	for (i = 1000, uitmp = 1; i && uitmp; i--) {
>> +	#define CMD_EXECUTE_TIMEOUT 30
>> +	timeout = jiffies + msecs_to_jiffies(CMD_EXECUTE_TIMEOUT);
>> +	for (uitmp = 1; !time_after(jiffies, timeout) && uitmp;) {
>>   		ret = regmap_read(dev->regmap, 0x1f, &uitmp);
>>   		if (ret)
>>   			goto error;
>> -
>> -		usleep_range(200, 5000);
>
> Hmm... removing the usleep() doesn't sound a good idea. You'll be
> flooding the I2C bus with read commands and spending CPU cycles
> for 30ms spending more power than the previous code. That doesn't
> sound more "elegant solution than poll X times with sleep" for me.
>
> So, I would keep the usleep_range() here and add a better
> comment on the patch description.

First of all, polling firmware ready status is very common for chips 
having firmware. And there is 2 ways to implement it:
1) poll N times in a loop using X sleep, timeout = N * X
2) poll in a loop using jiffies as a timeout

IMHO 2 is more elegant solution and I have started using it recently.

What you now propose is add some throttle in order to slow down polling 
interval to reduce I2C I/O. Yes sure less I/O is better, but downside is 
that it makes some unneeded extra delay to code path. Usually these sort 
firmware ready polling ends rather quickly, in a loop or two.

Sure it eats some extra CPU cycles, but I think extra control messages 
are about nothing compared to I/O used for data streaming.

Which kind of throttle delay you think is suitable for polling command 
status over I2C bus?

regards
Antti

-- 
http://palosaari.fi/