Re: [PATCH] TSL2550 driver bugfix

["Michele De Candia (VT)" <michele.decandia-EZxuzQJkuwwybS5Ee8rs3A@public.gmane.org>]

Hi Jean,

Jean Delvare wrote:
> On Sat, 11 Jul 2009 20:20:30 +0200, Jean Delvare wrote:
>   
>> For another, I do hit the case c1 > c0 from times to times, in
>> particular when I move the sensor in dark areas. Do you hit this as
>> well? This might be specific to the serial evaluation module I'm using,
>> it is very slow and this might cause c0 and c1 to be sampled at
>> significantly different times (say 200 ms apart.) If the light
>> conditions change meanwhile, this can explain why c1 > c0.
>>     
>
> Scratch this theory. Reading the TSL2550 datasheet again, I understand
> that sampling happens continuously and is unrelated to I2C reads.
> Sampling of each channel takes 400 ms in standard mode, so c0 and c1
> samplings are _always_ 400 ms apart, regardless of how fast the I2C
> interface is. So you should be able do see the problem too.
>
> This also means that the code in tsl2550_get_adc_value() is more
> complex than it needs to be. The only case where it makes sense to wait
> 400 ms to get a reading is right after power-up. 400 ms after power-up,
> c0 will always have a valid value, and after 800 ms c1 will always have
> a valid value as well. So I propose to simplify tsl2550_get_adc_value()
> and just return -EAGAIN if there is no valid reading. In practice I
> doubt we'll ever hit it.
>
>   
>> I am not
>> sure what to do in this case. We can return -EAGAIN and let user-space
>> retry. But we could also restart the computation automatically in this
>> case. Of course if the problem only affects the evaluation module then
>> we don't really care.
>>     
>
> Thinking some more about this: if we want to retry then we will have to
> wait at least 400 ms to read c0 again, and if it's not enough 400 more
> ms to read c1 again. And even then, there's no guarantee the new
> readings will be OK if the light conditions are still changing. This is
> an intrinsic weakness of the TSL2550 that both channels are sampled
> sequentially. I doubt we want to let the user wait for the lux value
> for several seconds, so returning -EAGAIN seems OK to me.
>   
I think that returning -EAGAIN should be fine in this case,
giving to the user application the responsibility to check the returned 
value.

> But I am not using the light sensor a lot myself, so my practical
> experience is rather limited, thus I would welcome comments and
> opinions on this.
>
>   
 From my experience, the case of c1 > c0 happens when light conditions 
are still changing and, in this case, returning -EAGAIN it's the correct 
way. This behaviour is due to the sequentially read of c1 and c0 
channel: we can do nothing to improve it through the driver.

I suggest, like you said, just to replace the -1 return value with -EAGAIN.

If you are in accord with this, I can pass you a new patch.