Re: [PATCH 5/7] iio: light: veml6030: update sensor resolution

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 15 Sep 2024 10:31:11 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> On 14/09/2024 16:57, Jonathan Cameron wrote:
> > On Fri, 13 Sep 2024 15:19:00 +0200
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >   
> >> The driver still uses the sensor resolution provided in the datasheet
> >> until Rev. 1.6, 28-Apr-2022, which was updated with Rev 1.7,
> >> 28-Nov-2023. The original ambient light resolution has been updated from
> >> 0.0036 lx/ct to 0.0042 lx/ct, which is the value that can be found in
> >> the current device datasheet.
> >>
> >> Update the default resolution for IT = 100 ms and GAIN = 1/8 from the
> >> original 4608 mlux/cnt to the current value from the "Resolution and
> >> maximum detection range" table (Application Note 84367, page 5), 5376
> >> mlux/cnt.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > Interesting.  So does the datasheet say this was fixing an error, or
> > is there any chance there are different versions of the chip out there?
> > 
> > Also, should we treat this as a fix?  I think we probably should given
> > we don't really want stable kernels to have wrong data being reported.
> > If so, please reply with a fixes tag.
> > 
> > Jonathan
> >  
> 
> According to the Product Information Notification (link in the cover
> letter):
> 
> "Reason for Change: Adjusted resolution as this was wrongly stated in
> the current datasheet."
> 
> "If resolution is defined in the particular application by the customer,
> no changes in the system should be made. In the case resolution was
> taken from the datasheet or app note, this has to be adjusted accordingly."
> 
> Which means that stable kernels are using the wrong resolution. I don't
> know what IIO usually does in such cases, because a fix could
> potentially make existing applications return "wrong data". If that is
> alright, and applications are meant to be adjusted after the kernel
> update, I have no problems to make this patch as a fix and add the
> stable tag.

It's unfortunate, but fixing a bug is a valid reason for ABI change
(which this is - sort of) so existing applications will need to be
fixed if anyone notices.

So please send this as a fix with appropriate tags and
that datasheet change log included in the patch description.

Thanks,

Jonathan

> 
> Best regards,
> Javier Carrasco
> 
> >> ---
> >>  drivers/iio/light/veml6030.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iio/light/veml6030.c b/drivers/iio/light/veml6030.c
> >> index 5d4c2e35b987..d5add040d0b3 100644
> >> --- a/drivers/iio/light/veml6030.c
> >> +++ b/drivers/iio/light/veml6030.c
> >> @@ -779,7 +779,7 @@ static int veml6030_hw_init(struct iio_dev *indio_dev)
> >>  
> >>  	/* Cache currently active measurement parameters */
> >>  	data->cur_gain = 3;
> >> -	data->cur_resolution = 4608;
> >> +	data->cur_resolution = 5376;
> >>  	data->cur_integration_time = 3;
> >>  
> >>  	return ret;
> >>  
> >   
>