Re: [PATCH v2 0/9] Support light color temperature and chromaticity

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 Dec 2023 00:39:28 +0100
Thomas Weißschuh <thomas@t-8ch.de> wrote:

> Hi everybody,
> 
> On 2023-09-19 13:40:45+0530, Basavaraj Natikar wrote:
> > This series adds support for light color temperature and chromaticity.
> > 
> > v1->v2:
> > *Rename the series.
> > *Rename als_illum to als channel as it supports other channels.
> > *Update patch description to include same reading for the two existing
> >  channels to use channel index to support more hub attributes.
> > *Keep line length under 80chars in hid-sensor-als.
> > *Add new channel type IIO_COLORTEMP.
> > *Update patch description and its subject to add channel type for 
> >  chromaticity. 
> > 
> > Basavaraj Natikar (9):
> >   iio: hid-sensor-als: Use channel index to support more hub attributes
> >   iio: Add channel type light color temperature
> >   iio: hid-sensor-als: Add light color temperature support
> >   HID: amd_sfh: Add support for light color temperature
> >   HID: amd_sfh: Add support for SFH1.1 light color temperature
> >   iio: Add channel type for chromaticity
> >   iio: hid-sensor-als: Add light chromaticity support
> >   HID: amd_sfh: Add light chromaticity support
> >   HID: amd_sfh: Add light chromaticity for SFH1.1  
> 
> This series is breaking probing of hid-sensor-als on Framework 13 AMD
> laptops [0].
> The problem is that the patches require hid-sensors-als sensors to also
> report chromaticity and color temparature which they don't.
Gah.  Missed that in review.  Sorry about that and thanks for digging into
this.
> 
> When I remove the 'if (ret < 0) return ret;' checks in
> als_parse_report() probing works and the illuminance/intensity channels
> that show up behave as expected.
> Unfortunately this still leaves behind a bunch of unusable channels.
> A nice fix would be to have something like sysfs/hwmon .is_visible()
> callback but that's not supported by IIO.

It's tricky to do because there is no simple association between
what is registered as channels and the resulting attribute.  We could probably
make it work, but not a simple thing to do.

> 
> One aproach would be to detect the usable channels in als_parse_report()
> and then adapt the indio_dev->channels based on that information.
> 
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=218223

Agreed that adapting the channels is the way to go.
Easiest option probably to set the relevant masks to 0 if the chromacity and
temp channels aren't there + set their scan index values to -1.
That 'should' suppress any attributes being created.
Having a gap in scan indexes is common anyway so any userspace should cope
with the timestamp being after a gap.

Alternatives would be to rebuild the chan_spec array to not have the entries,
or pass in and fill in two copies of the array, picking the relevant one only
on discovering if the temp and chromacity channels are present.

Jonathan

> 
> #regzbot introduced: 5f05285df691b1e82108eead7165feae238c95ef
> #regzbot monitor: https://bugzilla.kernel.org/show_bug.cgi?id=218223
>