| Message ID | 20231127212726.77707-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v6,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand |
On Mon, 27 Nov 2023 22:26:53 +0100 Marek Vasut <marex@denx.de> wrote: > The ISL76682 is very basic ALS which only supports ALS or IR mode > in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > other fancy functionality. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > Signed-off-by: Marek Vasut <marex@denx.de> Hi Marek, Discussion around available on v5 made me look closer at that aspect. You are providing all the available entries in the callback but they shouldn't be exposed to actually read unless the *_available bitmap bits corresponding to them are set. If you like I can just rip the unused code out whilst applying? Or if you'd prefer to send a v7 that's great too. Otherwise everything looks good to me. Jonathan > +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > + > +static int isl76682_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, > + int *length, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_LIGHT: > + *vals = illuminance_scale_available; > + *length = ARRAY_SIZE(illuminance_scale_available); > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + case IIO_INTENSITY: > + *vals = intensity_scale_available; > + *length = ARRAY_SIZE(intensity_scale_available); > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_INT_TIME: Never used. So can just drop this case which tidies up the question I h ad earlier on what the single entry array was conveying. > + *vals = integration_time_available; > + *length = ARRAY_SIZE(integration_time_available); > + *type = IIO_VAL_INT_PLUS_MICRO; > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_chan_spec isl76682_channels[] = { > + { > + .type = IIO_LIGHT, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), Without setting .info_mask_shared_by_all_available (unless we have a bug) you won't see the available attributes for INT_TIME. > + }, { > + .type = IIO_INTENSITY, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > + } > +}; > + > +static const struct iio_info isl76682_info = { > + .read_avail = isl76682_read_avail, > + .read_raw = isl76682_read_raw, > + .write_raw = isl76682_write_raw, > +}; > +static const struct i2c_device_id isl76682_id[] = { > + { "isl76682" }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, isl76682_id); > + > +static const struct of_device_id isl76682_of_match[] = { > + { .compatible = "isil,isl76682" }, > + { } Completely trivial but why { } here and {} in the similar case above? Pick one! > +}; > +MODULE_DEVICE_TABLE(of, isl76682_of_match);
On 12/4/23 12:20, Jonathan Cameron wrote: > On Mon, 27 Nov 2023 22:26:53 +0100 > Marek Vasut <marex@denx.de> wrote: > >> The ISL76682 is very basic ALS which only supports ALS or IR mode >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any >> other fancy functionality. >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Signed-off-by: Marek Vasut <marex@denx.de> > Hi Marek, > > Discussion around available on v5 made me look closer at that aspect. > You are providing all the available entries in the callback but they > shouldn't be exposed to actually read unless the *_available bitmap > bits corresponding to them are set. > > If you like I can just rip the unused code out whilst applying? > Or if you'd prefer to send a v7 that's great too. > > Otherwise everything looks good to me. Maybe just do that while applying and I'll test it right after to see whether something broke, that's probably fastest. Just let me know where this got applied. I have the device on my desk .
On Mon, 27 Nov 2023 22:26:53 +0100 Marek Vasut <marex@denx.de> wrote: > The ISL76682 is very basic ALS which only supports ALS or IR mode > in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > other fancy functionality. > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > Signed-off-by: Marek Vasut <marex@denx.de> There is a additional question on what scale means for an IR channel. There aren't any well defined units (as it depends on the sensitivity curve) so in general we avoid providing scale for intensity readings. The datasheet has a vague go at working around this problem by using counts relative to IR measurement in daylight which would measure 210 LUX. (via faking that with a 850nm green led??) That's far from a standard.. We do have precedence of scale applied to intensity channels so I guess the ship has sailed. Perhaps we should just add an explicit not to the ABI docs to cover that changing the scale in these sensors will result in the counts changing, but multiplying raw by scale isn't going to give any sensible units. So probably not a thing to fix in this series, but to address separately. Jonathan
On Mon, 4 Dec 2023 12:23:06 +0100 Marek Vasut <marex@denx.de> wrote: > On 12/4/23 12:20, Jonathan Cameron wrote: > > On Mon, 27 Nov 2023 22:26:53 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> The ISL76682 is very basic ALS which only supports ALS or IR mode > >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > >> other fancy functionality. > >> > >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> Signed-off-by: Marek Vasut <marex@denx.de> > > Hi Marek, > > > > Discussion around available on v5 made me look closer at that aspect. > > You are providing all the available entries in the callback but they > > shouldn't be exposed to actually read unless the *_available bitmap > > bits corresponding to them are set. > > > > If you like I can just rip the unused code out whilst applying? > > Or if you'd prefer to send a v7 that's great too. > > > > Otherwise everything looks good to me. > > Maybe just do that while applying and I'll test it right after to see > whether something broke, that's probably fastest. Just let me know where > this got applied. I have the device on my desk . Diff is below. Applied to the togreg branch of iio.git and initially pushed out as testing for normal reasons + for you to test. Thanks, Jonathan diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c index 15a68609985b..8605187bfb62 100644 --- a/drivers/iio/light/isl76682.c +++ b/drivers/iio/light/isl76682.c @@ -184,8 +184,6 @@ static int intensity_scale_available[] = { 0, 673000, }; -static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; - static int isl76682_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, const int **vals, int *type, @@ -207,11 +205,6 @@ static int isl76682_read_avail(struct iio_dev *indio_dev, default: return -EINVAL; } - case IIO_CHAN_INFO_INT_TIME: - *vals = integration_time_available; - *length = ARRAY_SIZE(integration_time_available); - *type = IIO_VAL_INT_PLUS_MICRO; - return IIO_AVAIL_LIST; default: return -EINVAL; }
On 12/4/23 15:35, Jonathan Cameron wrote: > On Mon, 4 Dec 2023 12:23:06 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 12/4/23 12:20, Jonathan Cameron wrote: >>> On Mon, 27 Nov 2023 22:26:53 +0100 >>> Marek Vasut <marex@denx.de> wrote: >>> >>>> The ISL76682 is very basic ALS which only supports ALS or IR mode >>>> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any >>>> other fancy functionality. >>>> >>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>> Hi Marek, >>> >>> Discussion around available on v5 made me look closer at that aspect. >>> You are providing all the available entries in the callback but they >>> shouldn't be exposed to actually read unless the *_available bitmap >>> bits corresponding to them are set. >>> >>> If you like I can just rip the unused code out whilst applying? >>> Or if you'd prefer to send a v7 that's great too. >>> >>> Otherwise everything looks good to me. >> >> Maybe just do that while applying and I'll test it right after to see >> whether something broke, that's probably fastest. Just let me know where >> this got applied. I have the device on my desk . > > Diff is below. Applied to the togreg I only found the commit in 'testing' branch , so I used that one . > branch of iio.git and initially pushed out > as testing for normal reasons + for you to test. > > Thanks, > > Jonathan > > > diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c > index 15a68609985b..8605187bfb62 100644 > --- a/drivers/iio/light/isl76682.c > +++ b/drivers/iio/light/isl76682.c > @@ -184,8 +184,6 @@ static int intensity_scale_available[] = { > 0, 673000, > }; > > -static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > - > static int isl76682_read_avail(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > const int **vals, int *type, > @@ -207,11 +205,6 @@ static int isl76682_read_avail(struct iio_dev *indio_dev, > default: > return -EINVAL; > } > - case IIO_CHAN_INFO_INT_TIME: > - *vals = integration_time_available; > - *length = ARRAY_SIZE(integration_time_available); > - *type = IIO_VAL_INT_PLUS_MICRO; > - return IIO_AVAIL_LIST; > default: > return -EINVAL; > } Ah, looking at the attrs before and after, now I get it: $ grep -H . /sys/bus/iio/devices/iio\:device0/* /sys/bus/iio/devices/iio:device0/in_illuminance_raw:21 /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000 /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015 0.06 0.24 0.96 /sys/bus/iio/devices/iio:device0/in_intensity_raw:33 /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500 /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.0105 0.042 0.168 0.673 /sys/bus/iio/devices/iio:device0/integration_time_available:0.090 /sys/bus/iio/devices/iio:device0/name:isl76682 /sys/bus/iio/devices/iio:device0/in_illuminance_raw:22 /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000 /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015000 0.060000 0.240000 0.960000 /sys/bus/iio/devices/iio:device0/in_intensity_raw:24 /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500 /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.010500 0.042000 0.168000 0.673000 /sys/bus/iio/devices/iio:device0/integration_time:0.090000 /sys/bus/iio/devices/iio:device0/name:isl76682 Thanks !
On 12/4/23 12:20, Jonathan Cameron wrote: > On Mon, 27 Nov 2023 22:26:53 +0100 > Marek Vasut <marex@denx.de> wrote: > >> The ISL76682 is very basic ALS which only supports ALS or IR mode >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any >> other fancy functionality. >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Signed-off-by: Marek Vasut <marex@denx.de> > Hi Marek, > > Discussion around available on v5 made me look closer at that aspect. > You are providing all the available entries in the callback but they > shouldn't be exposed to actually read unless the *_available bitmap > bits corresponding to them are set. > > If you like I can just rip the unused code out whilst applying? > Or if you'd prefer to send a v7 that's great too. > > Otherwise everything looks good to me. > > Jonathan > >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; >> + >> +static int isl76682_read_avail(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + const int **vals, int *type, >> + int *length, long mask) >> +{ >> + switch (mask) { >> + case IIO_CHAN_INFO_SCALE: >> + switch (chan->type) { >> + case IIO_LIGHT: >> + *vals = illuminance_scale_available; >> + *length = ARRAY_SIZE(illuminance_scale_available); >> + *type = IIO_VAL_INT_PLUS_MICRO; >> + return IIO_AVAIL_LIST; >> + case IIO_INTENSITY: >> + *vals = intensity_scale_available; >> + *length = ARRAY_SIZE(intensity_scale_available); >> + *type = IIO_VAL_INT_PLUS_MICRO; >> + return IIO_AVAIL_LIST; >> + default: >> + return -EINVAL; >> + } >> + case IIO_CHAN_INFO_INT_TIME: > > Never used. So can just drop this case which tidies up the question > I h ad earlier on what the single entry array was conveying. Seeing the diff, that now totally makes sense, thanks ! >> + *vals = integration_time_available; >> + *length = ARRAY_SIZE(integration_time_available); >> + *type = IIO_VAL_INT_PLUS_MICRO; >> + return IIO_AVAIL_LIST; >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static const struct iio_chan_spec isl76682_channels[] = { >> + { >> + .type = IIO_LIGHT, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> + BIT(IIO_CHAN_INFO_SCALE), >> + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), > > Without setting .info_mask_shared_by_all_available (unless we have a bug) > you won't see the available attributes for INT_TIME. > >> + }, { >> + .type = IIO_INTENSITY, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> + BIT(IIO_CHAN_INFO_SCALE), >> + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), >> + } >> +}; >> + >> +static const struct iio_info isl76682_info = { >> + .read_avail = isl76682_read_avail, >> + .read_raw = isl76682_read_raw, >> + .write_raw = isl76682_write_raw, >> +}; > > >> +static const struct i2c_device_id isl76682_id[] = { >> + { "isl76682" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(i2c, isl76682_id); >> + >> +static const struct of_device_id isl76682_of_match[] = { >> + { .compatible = "isil,isl76682" }, >> + { } > > Completely trivial but why { } here and {} in the similar > case above? Pick one! I sent a one-liner for this , it might make sense to squash it if possible .
On 12/4/23 12:29, Jonathan Cameron wrote: > On Mon, 27 Nov 2023 22:26:53 +0100 > Marek Vasut <marex@denx.de> wrote: > >> The ISL76682 is very basic ALS which only supports ALS or IR mode >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any >> other fancy functionality. >> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >> Signed-off-by: Marek Vasut <marex@denx.de> > > There is a additional question on what scale means for > an IR channel. There aren't any well defined units (as it depends > on the sensitivity curve) so in general we avoid providing scale > for intensity readings. > > The datasheet has a vague go at working around this problem by > using counts relative to IR measurement in daylight which would > measure 210 LUX. (via faking that with a 850nm green led??) > > That's far from a standard.. > > We do have precedence of scale applied to intensity channels > so I guess the ship has sailed. > > Perhaps we should just add an explicit not to the ABI docs > to cover that changing the scale in these sensors will result > in the counts changing, but multiplying raw by scale isn't going > to give any sensible units. > > So probably not a thing to fix in this series, but to address > separately. Is something like this what you have in mind ? diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio index 0eadc08c3a139..584607f560d02 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio +++ b/Documentation/ABI/testing/sysfs-bus-iio @@ -618,7 +618,9 @@ KernelVersion: 2.6.35 Contact: linux-iio@vger.kernel.org Description: If a discrete set of scale values is available, they - are listed in this attribute. + are listed in this attribute. Unlike illumination, + multiplying intensity by intensity_scale does not + yield value with any standardized unit . What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_hardwaregain What: /sys/bus/iio/devices/iio:deviceX/in_intensity_hardwaregain
On Tue, Dec 05, 2023 at 02:43:30AM +0100, Marek Vasut wrote: > On 12/4/23 12:29, Jonathan Cameron wrote: > > On Mon, 27 Nov 2023 22:26:53 +0100 > > Marek Vasut <marex@denx.de> wrote: ... > If a discrete set of scale values is available, they > - are listed in this attribute. > + are listed in this attribute. Unlike illumination, > + multiplying intensity by intensity_scale does not > + yield value with any standardized unit . (Do not forget to drop extra space)
On 12/5/23 16:54, Andy Shevchenko wrote: > On Tue, Dec 05, 2023 at 02:43:30AM +0100, Marek Vasut wrote: >> On 12/4/23 12:29, Jonathan Cameron wrote: >>> On Mon, 27 Nov 2023 22:26:53 +0100 >>> Marek Vasut <marex@denx.de> wrote: > > ... > >> If a discrete set of scale values is available, they >> - are listed in this attribute. >> + are listed in this attribute. Unlike illumination, >> + multiplying intensity by intensity_scale does not >> + yield value with any standardized unit . > > (Do not forget to drop extra space) Which extra space ?
On Tue, Dec 05, 2023 at 10:02:32PM +0100, Marek Vasut wrote: > On 12/5/23 16:54, Andy Shevchenko wrote: > > On Tue, Dec 05, 2023 at 02:43:30AM +0100, Marek Vasut wrote: ... > > ...unit . > > > > (Do not forget to drop extra space) > > Which extra space ? Like in your question :-) (I left the only relevant context, easy to notice.)
On 12/5/23 22:57, Andy Shevchenko wrote: > On Tue, Dec 05, 2023 at 10:02:32PM +0100, Marek Vasut wrote: >> On 12/5/23 16:54, Andy Shevchenko wrote: >>> On Tue, Dec 05, 2023 at 02:43:30AM +0100, Marek Vasut wrote: > > ... > >>> ...unit . >>> >>> (Do not forget to drop extra space) >> >> Which extra space ? > > Like in your question :-) > (I left the only relevant context, easy to notice.) Added, thanks.
On Tue, 5 Dec 2023 02:24:51 +0100 Marek Vasut <marex@denx.de> wrote: > On 12/4/23 15:35, Jonathan Cameron wrote: > > On Mon, 4 Dec 2023 12:23:06 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 12/4/23 12:20, Jonathan Cameron wrote: > >>> On Mon, 27 Nov 2023 22:26:53 +0100 > >>> Marek Vasut <marex@denx.de> wrote: > >>> > >>>> The ISL76682 is very basic ALS which only supports ALS or IR mode > >>>> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > >>>> other fancy functionality. > >>>> > >>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >>>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > >>>> Signed-off-by: Marek Vasut <marex@denx.de> > >>> Hi Marek, > >>> > >>> Discussion around available on v5 made me look closer at that aspect. > >>> You are providing all the available entries in the callback but they > >>> shouldn't be exposed to actually read unless the *_available bitmap > >>> bits corresponding to them are set. > >>> > >>> If you like I can just rip the unused code out whilst applying? > >>> Or if you'd prefer to send a v7 that's great too. > >>> > >>> Otherwise everything looks good to me. > >> > >> Maybe just do that while applying and I'll test it right after to see > >> whether something broke, that's probably fastest. Just let me know where > >> this got applied. I have the device on my desk . > > > > Diff is below. Applied to the togreg > > I only found the commit in 'testing' branch , so I used that one . I messed up it seems and didn't actually push the updated version. Done so now along with squashing in the bracket tidy up. > > > branch of iio.git and initially pushed out > > as testing for normal reasons + for you to test. > > > > Thanks, > > > > Jonathan > > > > > > diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c > > index 15a68609985b..8605187bfb62 100644 > > --- a/drivers/iio/light/isl76682.c > > +++ b/drivers/iio/light/isl76682.c > > @@ -184,8 +184,6 @@ static int intensity_scale_available[] = { > > 0, 673000, > > }; > > > > -static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > > - > > static int isl76682_read_avail(struct iio_dev *indio_dev, > > struct iio_chan_spec const *chan, > > const int **vals, int *type, > > @@ -207,11 +205,6 @@ static int isl76682_read_avail(struct iio_dev *indio_dev, > > default: > > return -EINVAL; > > } > > - case IIO_CHAN_INFO_INT_TIME: > > - *vals = integration_time_available; > > - *length = ARRAY_SIZE(integration_time_available); > > - *type = IIO_VAL_INT_PLUS_MICRO; > > - return IIO_AVAIL_LIST; > > default: > > return -EINVAL; > > } > > Ah, looking at the attrs before and after, now I get it: > > $ grep -H . /sys/bus/iio/devices/iio\:device0/* > /sys/bus/iio/devices/iio:device0/in_illuminance_raw:21 > /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000 > /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015 > 0.06 0.24 0.96 > /sys/bus/iio/devices/iio:device0/in_intensity_raw:33 > /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500 > /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.0105 > 0.042 0.168 0.673 > /sys/bus/iio/devices/iio:device0/integration_time_available:0.090 > /sys/bus/iio/devices/iio:device0/name:isl76682 > > /sys/bus/iio/devices/iio:device0/in_illuminance_raw:22 > /sys/bus/iio/devices/iio:device0/in_illuminance_scale:0.015000 > /sys/bus/iio/devices/iio:device0/in_illuminance_scale_available:0.015000 > 0.060000 0.240000 0.960000 > /sys/bus/iio/devices/iio:device0/in_intensity_raw:24 > /sys/bus/iio/devices/iio:device0/in_intensity_scale:0.010500 > /sys/bus/iio/devices/iio:device0/in_intensity_scale_available:0.010500 > 0.042000 0.168000 0.673000 > /sys/bus/iio/devices/iio:device0/integration_time:0.090000 > /sys/bus/iio/devices/iio:device0/name:isl76682 > > Thanks ! >
On Tue, 5 Dec 2023 02:43:30 +0100 Marek Vasut <marex@denx.de> wrote: > On 12/4/23 12:29, Jonathan Cameron wrote: > > On Mon, 27 Nov 2023 22:26:53 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> The ISL76682 is very basic ALS which only supports ALS or IR mode > >> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any > >> other fancy functionality. > >> > >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > >> Signed-off-by: Marek Vasut <marex@denx.de> > > > > There is a additional question on what scale means for > > an IR channel. There aren't any well defined units (as it depends > > on the sensitivity curve) so in general we avoid providing scale > > for intensity readings. > > > > The datasheet has a vague go at working around this problem by > > using counts relative to IR measurement in daylight which would > > measure 210 LUX. (via faking that with a 850nm green led??) > > > > That's far from a standard.. > > > > We do have precedence of scale applied to intensity channels > > so I guess the ship has sailed. > > > > Perhaps we should just add an explicit not to the ABI docs > > to cover that changing the scale in these sensors will result > > in the counts changing, but multiplying raw by scale isn't going > > to give any sensible units. > > > > So probably not a thing to fix in this series, but to address > > separately. > > Is something like this what you have in mind ? Yes, something along those lines. We could add more detail on why but perhaps that would just confuse things more than just stating it is the case. > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio > b/Documentation/ABI/testing/sysfs-bus-iio > index 0eadc08c3a139..584607f560d02 100644 > --- a/Documentation/ABI/testing/sysfs-bus-iio > +++ b/Documentation/ABI/testing/sysfs-bus-iio > @@ -618,7 +618,9 @@ KernelVersion: 2.6.35 > Contact: linux-iio@vger.kernel.org > Description: > If a discrete set of scale values is available, they > - are listed in this attribute. > + are listed in this attribute. Unlike illumination, > + multiplying intensity by intensity_scale does not > + yield value with any standardized unit . > > What: /sys/bus/iio/devices/iio:deviceX/out_voltageY_hardwaregain > What: /sys/bus/iio/devices/iio:deviceX/in_intensity_hardwaregain >
On 12/6/23 18:20, Jonathan Cameron wrote: > On Tue, 5 Dec 2023 02:24:51 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 12/4/23 15:35, Jonathan Cameron wrote: >>> On Mon, 4 Dec 2023 12:23:06 +0100 >>> Marek Vasut <marex@denx.de> wrote: >>> >>>> On 12/4/23 12:20, Jonathan Cameron wrote: >>>>> On Mon, 27 Nov 2023 22:26:53 +0100 >>>>> Marek Vasut <marex@denx.de> wrote: >>>>> >>>>>> The ISL76682 is very basic ALS which only supports ALS or IR mode >>>>>> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any >>>>>> other fancy functionality. >>>>>> >>>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>> Hi Marek, >>>>> >>>>> Discussion around available on v5 made me look closer at that aspect. >>>>> You are providing all the available entries in the callback but they >>>>> shouldn't be exposed to actually read unless the *_available bitmap >>>>> bits corresponding to them are set. >>>>> >>>>> If you like I can just rip the unused code out whilst applying? >>>>> Or if you'd prefer to send a v7 that's great too. >>>>> >>>>> Otherwise everything looks good to me. >>>> >>>> Maybe just do that while applying and I'll test it right after to see >>>> whether something broke, that's probably fastest. Just let me know where >>>> this got applied. I have the device on my desk . >>> >>> Diff is below. Applied to the togreg >> >> I only found the commit in 'testing' branch , so I used that one . > I messed up it seems and didn't actually push the updated version. > Done so now along with squashing in the bracket tidy up. Driver works just fine, still, thank you.
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 45edba797e4c7..9e8cdc091556d 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -252,6 +252,21 @@ config ISL29125 To compile this driver as a module, choose M here: the module will be called isl29125. +config ISL76682 + tristate "Intersil ISL76682 Light Sensor" + depends on I2C + select REGMAP_I2C + help + Say Y here if you want to build a driver for the Intersil ISL76682 + Ambient Light Sensor and IR Intensity sensor. This driver provides + the readouts via standard IIO sysfs and device interface. Both ALS + illuminance and IR illuminance are provided raw with separate scale + setting which can be configured via sysfs, the default scale is 1000 + lux, other options are 4000/16000/64000 lux. + + To compile this driver as a module, choose M here: the module will be + called isl76682. + config HID_SENSOR_ALS depends on HID_SENSOR_HUB select IIO_BUFFER diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index c0db4c4c36ec9..09fa585f3109f 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -28,6 +28,7 @@ obj-$(CONFIG_IQS621_ALS) += iqs621-als.o obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o obj-$(CONFIG_ISL29125) += isl29125.o +obj-$(CONFIG_ISL76682) += isl76682.o obj-$(CONFIG_JSA1212) += jsa1212.o obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o obj-$(CONFIG_LTR501) += ltr501.o diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c new file mode 100644 index 0000000000000..15a68609985b6 --- /dev/null +++ b/drivers/iio/light/isl76682.c @@ -0,0 +1,353 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * IIO driver for the light sensor ISL76682. + * ISL76682 is Ambient Light Sensor + * + * Copyright (c) 2023 Marek Vasut <marex@denx.de> + */ + +#include <linux/array_size.h> +#include <linux/bits.h> +#include <linux/cleanup.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/types.h> + +#include <linux/iio/iio.h> + +#define ISL76682_REG_COMMAND 0x00 + +#define ISL76682_COMMAND_EN BIT(7) +#define ISL76682_COMMAND_MODE_CONTINUOUS BIT(6) +#define ISL76682_COMMAND_LIGHT_IR BIT(5) + +#define ISL76682_COMMAND_RANGE_LUX_1K 0x0 +#define ISL76682_COMMAND_RANGE_LUX_4K 0x1 +#define ISL76682_COMMAND_RANGE_LUX_16K 0x2 +#define ISL76682_COMMAND_RANGE_LUX_64K 0x3 +#define ISL76682_COMMAND_RANGE_LUX_MASK GENMASK(1, 0) + +#define ISL76682_REG_ALSIR_L 0x01 + +#define ISL76682_REG_ALSIR_U 0x02 + +#define ISL76682_NUM_REGS (ISL76682_REG_ALSIR_U + 1) + +#define ISL76682_CONV_TIME_MS 100 +#define ISL76682_INT_TIME_US 90000 + +#define ISL76682_ADC_MAX (BIT(16) - 1) + +struct isl76682_chip { + /* + * Lock to synchronize access to device command register + * and the content of range variable below. + */ + struct mutex lock; + struct regmap *regmap; + u8 range; + u8 command; +}; + +struct isl76682_range { + u8 range; + u32 als; + u32 ir; +}; + +static struct isl76682_range isl76682_range_table[] = { + { ISL76682_COMMAND_RANGE_LUX_1K, 15000, 10500 }, + { ISL76682_COMMAND_RANGE_LUX_4K, 60000, 42000 }, + { ISL76682_COMMAND_RANGE_LUX_16K, 240000, 168000 }, + { ISL76682_COMMAND_RANGE_LUX_64K, 960000, 673000 } +}; + +static int isl76682_get(struct isl76682_chip *chip, bool mode_ir, int *data) +{ + u8 command; + int ret; + + command = ISL76682_COMMAND_EN | ISL76682_COMMAND_MODE_CONTINUOUS | + chip->range; + + if (mode_ir) + command |= ISL76682_COMMAND_LIGHT_IR; + + if (command != chip->command) { + ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, command); + if (ret) + return ret; + + /* Need to wait for conversion time if ALS/IR mode enabled */ + msleep(ISL76682_CONV_TIME_MS); + + chip->command = command; + } + + ret = regmap_bulk_read(chip->regmap, ISL76682_REG_ALSIR_L, data, 2); + *data &= ISL76682_ADC_MAX; + return ret; +} + +static int isl76682_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct isl76682_chip *chip = iio_priv(indio_dev); + int i; + + if (mask != IIO_CHAN_INFO_SCALE) + return -EINVAL; + + if (val != 0) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) { + if (chan->type == IIO_LIGHT && val2 != isl76682_range_table[i].als) + continue; + if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir) + continue; + + scoped_guard(mutex, &chip->lock) + chip->range = isl76682_range_table[i].range; + return 0; + } + + return -EINVAL; +} + +static int isl76682_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct isl76682_chip *chip = iio_priv(indio_dev); + int ret; + int i; + + guard(mutex)(&chip->lock); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_LIGHT: + ret = isl76682_get(chip, false, val); + return (ret < 0) ? ret : IIO_VAL_INT; + case IIO_INTENSITY: + ret = isl76682_get(chip, true, val); + return (ret < 0) ? ret : IIO_VAL_INT; + default: + return -EINVAL; + } + return -EINVAL; + case IIO_CHAN_INFO_SCALE: + for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) { + if (chip->range != isl76682_range_table[i].range) + continue; + + *val = 0; + switch (chan->type) { + case IIO_LIGHT: + *val2 = isl76682_range_table[i].als; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_INTENSITY: + *val2 = isl76682_range_table[i].ir; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } + } + return -EINVAL; + case IIO_CHAN_INFO_INT_TIME: + *val = 0; + *val2 = ISL76682_INT_TIME_US; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static int illuminance_scale_available[] = { + 0, 15000, + 0, 60000, + 0, 240000, + 0, 960000, +}; + +static int intensity_scale_available[] = { + 0, 10500, + 0, 42000, + 0, 168000, + 0, 673000, +}; + +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; + +static int isl76682_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, + int *length, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_SCALE: + switch (chan->type) { + case IIO_LIGHT: + *vals = illuminance_scale_available; + *length = ARRAY_SIZE(illuminance_scale_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + case IIO_INTENSITY: + *vals = intensity_scale_available; + *length = ARRAY_SIZE(intensity_scale_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_INT_TIME: + *vals = integration_time_available; + *length = ARRAY_SIZE(integration_time_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + +static const struct iio_chan_spec isl76682_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + }, { + .type = IIO_INTENSITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + } +}; + +static const struct iio_info isl76682_info = { + .read_avail = isl76682_read_avail, + .read_raw = isl76682_read_raw, + .write_raw = isl76682_write_raw, +}; + +static int isl76682_clear_configure_reg(struct isl76682_chip *chip) +{ + struct device *dev = regmap_get_device(chip->regmap); + int ret; + + ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, 0x0); + if (ret < 0) + dev_err(dev, "Error %d clearing the CONFIGURE register\n", ret); + + /* + * In the success case, the command register was zeroed out. + * + * In the error case, we do not know in which state the command + * register is, so we assume it is zeroed out, so that it would + * be reprogrammed at the next data read out, and at that time + * we hope it would be reprogrammed successfully. That is very + * much a best effort approach. + */ + chip->command = 0; + + return ret; +} + +static void isl76682_reset_action(void *chip) +{ + isl76682_clear_configure_reg(chip); +} + +static bool isl76682_is_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case ISL76682_REG_ALSIR_L: + case ISL76682_REG_ALSIR_U: + return true; + default: + return false; + } +} + +static const struct regmap_config isl76682_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .volatile_reg = isl76682_is_volatile_reg, + .max_register = ISL76682_NUM_REGS - 1, + .num_reg_defaults_raw = ISL76682_NUM_REGS, + .cache_type = REGCACHE_FLAT, +}; + +static int isl76682_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct isl76682_chip *chip; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); + if (!indio_dev) + return -ENOMEM; + + chip = iio_priv(indio_dev); + + mutex_init(&chip->lock); + + chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config); + ret = PTR_ERR_OR_ZERO(chip->regmap); + if (ret) + return dev_err_probe(dev, ret, "Error initializing regmap\n"); + + chip->range = ISL76682_COMMAND_RANGE_LUX_1K; + + ret = isl76682_clear_configure_reg(chip); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(dev, isl76682_reset_action, chip); + if (ret) + return ret; + + indio_dev->info = &isl76682_info; + indio_dev->channels = isl76682_channels; + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); + indio_dev->name = "isl76682"; + indio_dev->modes = INDIO_DIRECT_MODE; + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct i2c_device_id isl76682_id[] = { + { "isl76682" }, + {} +}; +MODULE_DEVICE_TABLE(i2c, isl76682_id); + +static const struct of_device_id isl76682_of_match[] = { + { .compatible = "isil,isl76682" }, + { } +}; +MODULE_DEVICE_TABLE(of, isl76682_of_match); + +static struct i2c_driver isl76682_driver = { + .driver = { + .name = "isl76682", + .of_match_table = isl76682_of_match, + }, + .probe = isl76682_probe, + .id_table = isl76682_id, +}; +module_i2c_driver(isl76682_driver); + +MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");