diff mbox series

[v6,1/2] dt-bindings: iio: light: isl76682: Document ISL76682

Message ID 20231127212726.77707-1-marex@denx.de
State Not Applicable
Headers show
Series [v6,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Marek Vasut Nov. 27, 2023, 9:26 p.m. UTC
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. Document it as trivial device.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Andre Werner <andre.werner@systec-electronic.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Naresh Solanki <naresh.solanki@9elements.com>
Cc: Patrick Rudolph <patrick.rudolph@9elements.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
Cc: Vincent Tremblay <vincent@vtremblay.dev>
Cc: devicetree@vger.kernel.org
Cc: linux-iio@vger.kernel.org
---
V2: Add AB from Conor
V3: No change
V4: No change
V5: No change
V6: No change
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jonathan Cameron Dec. 4, 2023, 11:20 a.m. UTC | #1
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);
Marek Vasut Dec. 4, 2023, 11:23 a.m. UTC | #2
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 .
Jonathan Cameron Dec. 4, 2023, 11:29 a.m. UTC | #3
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
Marek Vasut Dec. 5, 2023, 1:24 a.m. UTC | #4
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 !
Marek Vasut Dec. 5, 2023, 1:26 a.m. UTC | #5
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 .
Andy Shevchenko Dec. 5, 2023, 3:54 p.m. UTC | #6
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)
Marek Vasut Dec. 5, 2023, 9:02 p.m. UTC | #7
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 ?
Andy Shevchenko Dec. 5, 2023, 9:57 p.m. UTC | #8
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.)
Marek Vasut Dec. 5, 2023, 11:11 p.m. UTC | #9
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.
Jonathan Cameron Dec. 6, 2023, 5:20 p.m. UTC | #10
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 !
>
Jonathan Cameron Dec. 6, 2023, 5:23 p.m. UTC | #11
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
>
Marek Vasut Dec. 7, 2023, 1:28 p.m. UTC | #12
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 mbox series

Patch

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