Re: [PATCH] TSL2550 driver bugfix

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

Hi Michele,

On Mon, 13 Jul 2009 09:56:10 +0200, Michele De Candia (VT) wrote:
> 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.

Yes I agree, please send an updated patch and I'll apply it.

In the meantime I've improved the performance of the i2c-taos-evm
driver I'm using for the TSL5220, I'll post it later today, once I am
fully done with testing. I'm not sure if there are other users of this
driver though.

Thanks,
-- 
Jean Delvare