| Message ID | 20231127212726.77707-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | 8c82e9e3766b8a7e6a4c633dab5b652333ada78f |
| Headers | show |
| Series | [v6,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand |
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 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 ! >
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index 441b55723675a..d7ffecc0e9bf5 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -181,6 +181,8 @@ properties: - isil,isl29030 # Intersil ISL68137 Digital Output Configurable PWM Controller - isil,isl68137 + # Intersil ISL76682 Ambient Light Sensor + - isil,isl76682 # Linear Technology LTC2488 - lineartechnology,ltc2488 # 5 Bit Programmable, Pulse-Width Modulator