| Message ID | 20240913-veml6035-v1-5-0b09c0c90418@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: light: veml6030: fix issues and add support for veml6035 | expand |
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 > --- > 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; >
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. 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; >> >
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; > >> > > >
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;
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> --- drivers/iio/light/veml6030.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)