| Message ID | 20231125222738.97875-1-marex@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v5,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Sat, 25 Nov 2023 23:26:23 +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, One last question + a comment in general. Act on that if you like. Thanks, Jonathan > +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; Why have an available attribute for a single value. Is it useful for anything? > +static int isl76682_probe(struct i2c_client *client) > +{ ... > + indio_dev->info = &isl76682_info; > + indio_dev->channels = isl76682_channels; > + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); > + indio_dev->name = ISL76682_DRIVER_NAME; Trivial but I'm not a fan of using defines in cases like this. It just makes me go find the define when I could see the string directly here. In cases where matching or similar strictly requires the naming to be the same in various places a define is useful. In this case less so. Anyhow, it's a very minor comment so never mind if you prefer to leave it as it stands. > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + return devm_iio_device_register(dev, indio_dev); > +}
On 11/26/23 19:16, Jonathan Cameron wrote: > On Sat, 25 Nov 2023 23:26:23 +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, > > One last question + a comment in general. Act on that if you like. > > Thanks, > > Jonathan > > >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > > Why have an available attribute for a single value. Is it useful for anything? To report it to userspace, iio-sensor-proxy uses that to control the ALS poll interval . >> +static int isl76682_probe(struct i2c_client *client) >> +{ > > ... > >> + indio_dev->info = &isl76682_info; >> + indio_dev->channels = isl76682_channels; >> + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); >> + indio_dev->name = ISL76682_DRIVER_NAME; > Trivial but I'm not a fan of using defines in cases like this. It just makes > me go find the define when I could see the string directly here. > > In cases where matching or similar strictly requires the naming to be the same > in various places a define is useful. In this case less so. > > Anyhow, it's a very minor comment so never mind if you prefer to leave it > as it stands. I added it to V6 . I'll wait for the integration time reply above and then send V6 .
On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote: [...] > +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 (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY) > + return -EINVAL; > > In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify > the channels available at all. So there will be no other channel types created by the IIO environment, I think. That's a good point, I think we can drop it in this case and the next one. Jonathan , is there any reason you can think of why this shouldn't be dropped ? I'll add it into V6, thanks.
On Sun, Nov 26, 2023 at 08:33:26PM +0100, Dr.-Ing. Andre Werner wrote: > Dear Marek, > > see remarks below. Do not top-post! Do not overquote hundreds of lines just for a single comment!
On Sat, Nov 25, 2023 at 11:26:23PM +0100, Marek Vasut 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. ... > + 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; You forgot to drop indentation level for 'continue' lines. > + scoped_guard(mutex, &chip->lock) > + chip->range = isl76682_range_table[i].range; > + return 0; > + }
On 11/27/23 17:13, Andy Shevchenko wrote: > On Sat, Nov 25, 2023 at 11:26:23PM +0100, Marek Vasut 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. > > ... > >> + 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; > > You forgot to drop indentation level for 'continue' lines. I noticed that too and already fixed it in v6 .
On Sun, 26 Nov 2023 23:09:36 +0100 Marek Vasut <marex@denx.de> wrote: > On 11/26/23 19:16, Jonathan Cameron wrote: > > On Sat, 25 Nov 2023 23:26:23 +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, > > > > One last question + a comment in general. Act on that if you like. > > > > Thanks, > > > > Jonathan > > > > > >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > > > > Why have an available attribute for a single value. Is it useful for anything? > > To report it to userspace, iio-sensor-proxy uses that to control the ALS > poll interval . It should use integration_time, not the associated available attribute. > > >> +static int isl76682_probe(struct i2c_client *client) > >> +{ > > > > ... > > > >> + indio_dev->info = &isl76682_info; > >> + indio_dev->channels = isl76682_channels; > >> + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); > >> + indio_dev->name = ISL76682_DRIVER_NAME; > > Trivial but I'm not a fan of using defines in cases like this. It just makes > > me go find the define when I could see the string directly here. > > > > In cases where matching or similar strictly requires the naming to be the same > > in various places a define is useful. In this case less so. > > > > Anyhow, it's a very minor comment so never mind if you prefer to leave it > > as it stands. > > I added it to V6 . > > I'll wait for the integration time reply above and then send V6 . >
On Sun, 26 Nov 2023 23:17:55 +0100 Marek Vasut <marex@denx.de> wrote: > On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote: > > [...] > > > +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 (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY) > > + return -EINVAL; > > > > In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify > > the channels available at all. So there will be no other channel types created by the IIO environment, I think. > > That's a good point, I think we can drop it in this case and the next one. > > Jonathan , is there any reason you can think of why this shouldn't be > dropped ? I don't mind a little paranoia, but fine with dropping it as well. The reason paranoia is fine is that if other channels are added in future we might not remember to fix this up if they happen to have no writeable attributes. I leave that assessment up to driver authors. Jonathan > > I'll add it into V6, thanks. >
On 12/1/23 19:20, Jonathan Cameron wrote: > On Sun, 26 Nov 2023 23:17:55 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote: >> >> [...] >> >>> +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 (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY) >>> + return -EINVAL; >>> >>> In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify >>> the channels available at all. So there will be no other channel types created by the IIO environment, I think. >> >> That's a good point, I think we can drop it in this case and the next one. >> >> Jonathan , is there any reason you can think of why this shouldn't be >> dropped ? > > I don't mind a little paranoia, but fine with dropping it as well. > > The reason paranoia is fine is that if other channels are added in future > we might not remember to fix this up if they happen to have no writeable attributes. > I leave that assessment up to driver authors. So, dropped, since this chip seems to be very much legacy hardware.
On 12/1/23 19:17, Jonathan Cameron wrote: > On Sun, 26 Nov 2023 23:09:36 +0100 > Marek Vasut <marex@denx.de> wrote: > >> On 11/26/23 19:16, Jonathan Cameron wrote: >>> On Sat, 25 Nov 2023 23:26:23 +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, >>> >>> One last question + a comment in general. Act on that if you like. >>> >>> Thanks, >>> >>> Jonathan >>> >>> >>>> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; >>> >>> Why have an available attribute for a single value. Is it useful for anything? >> >> To report it to userspace, iio-sensor-proxy uses that to control the ALS >> poll interval . > > It should use integration_time, not the associated available attribute. So, what should I do with this feedback ? I did send the V6 already, so shall I change anything in V7 ?
On Sat, 2 Dec 2023 07:48:12 +0100 Marek Vasut <marex@denx.de> wrote: > On 12/1/23 19:17, Jonathan Cameron wrote: > > On Sun, 26 Nov 2023 23:09:36 +0100 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 11/26/23 19:16, Jonathan Cameron wrote: > >>> On Sat, 25 Nov 2023 23:26:23 +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, > >>> > >>> One last question + a comment in general. Act on that if you like. > >>> > >>> Thanks, > >>> > >>> Jonathan > >>> > >>> > >>>> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; > >>> > >>> Why have an available attribute for a single value. Is it useful for anything? > >> > >> To report it to userspace, iio-sensor-proxy uses that to control the ALS > >> poll interval . > > > > It should use integration_time, not the associated available attribute. > > So, what should I do with this feedback ? > > I did send the V6 already, so shall I change anything in V7 ? I'll reply there. Just noticed some other oddities around available.
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