On Sun, 2023-12-17 at 14:35 +0000, Jonathan Cameron wrote:
> On Fri, 15 Dec 2023 08:01:59 -0800
> Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
>
> > With the commit ee3710f39f9d ("iio: hid-sensor-als: Add light
> > chromaticity
> > support"), there is an assumption that the every HID ALS descriptor
> > has
> > support of usage ids for chromaticity support. If they are not
> > present,
> > probe fails for the driver . This breaks ALS functionality on
> > majority of
> > platforms.
> >
> > It is possible that chromaticity usage ids are not present. When
> > not
> > present, restrict number of IIO channels to not include support for
> > chromaticity and continue.
> >
> > Fixes: ee3710f39f9d ("iio: hid-sensor-als: Add light chromaticity
> > support")
> > Reported-by: Thomas Weißschuh <thomas@xxxxxxxx>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218223
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > drivers/iio/light/hid-sensor-als.c | 24 ++++++++++++++++--------
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/iio/light/hid-sensor-als.c
> > b/drivers/iio/light/hid-sensor-als.c
> > index f17304b54468..9941b0b927c7 100644
> > --- a/drivers/iio/light/hid-sensor-als.c
> > +++ b/drivers/iio/light/hid-sensor-als.c
> > @@ -303,11 +303,14 @@ static int als_parse_report(struct
> > platform_device *pdev,
> > struct hid_sensor_hub_device
> > *hsdev,
> > struct iio_chan_spec *channels,
> > unsigned usage_id,
> > - struct als_state *st)
> > + struct als_state *st,
> > + int *max_channels)
> > {
> > int ret;
> > int i;
> >
> > + *max_channels = CHANNEL_SCAN_INDEX_MAX;
> > +
> > for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> > ret = sensor_hub_input_get_attribute_info(hsdev,
> > HID_INPUT_REPORT,
> > @@ -326,8 +329,12 @@ static int als_parse_report(struct
> > platform_device *pdev,
> > usage_id,
> > HID_USAGE_SENSOR_LIGHT_COLOR_TEMPER
> > ATURE,
> > &st-
> > >als[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
> > - if (ret < 0)
> > - return ret;
> > + if (ret < 0) {
> > + *max_channels = CHANNEL_SCAN_INDEX_ILLUM;
> > + ret = 0;
> > + goto skip_color_chromaticity;
> > + }
> > +
> > als_adjust_channel_bit_mask(channels,
> > CHANNEL_SCAN_INDEX_COLOR_TEMP,
> > st-
> > >als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> >
> > @@ -354,6 +361,7 @@ static int als_parse_report(struct
> > platform_device *pdev,
> > st->als[next_scan_index].report_id);
> > }
> >
> > +skip_color_chromaticity:
> > st->scale_precision = hid_sensor_format_scale(usage_id,
> > &st-
> > >als[CHANNEL_SCAN_INDEX_INTENSITY],
> > &st->scale_pre_decml, &st-
> > >scale_post_decml);
> > @@ -364,7 +372,7 @@ static int als_parse_report(struct
> > platform_device *pdev,
> > /* Function to initialize the processing for usage id */
> > static int hid_als_probe(struct platform_device *pdev)
> > {
> > - int ret = 0;
> > + int ret = 0, max_channels;
> > static const char *name = "als";
> > struct iio_dev *indio_dev;
> > struct als_state *als_state;
> > @@ -398,15 +406,15 @@ static int hid_als_probe(struct
> > platform_device *pdev)
> >
> > ret = als_parse_report(pdev, hsdev,
> > (struct iio_chan_spec *)indio_dev-
> > >channels,
> > - hsdev->usage,
> > - als_state);
> > + hsdev->usage, als_state,
> > &max_channels);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to setup
> > attributes\n");
> > return ret;
> > }
> >
> > - indio_dev->num_channels =
> > - ARRAY_SIZE(als_channels);
> > + /* +1 to include time stamp */
> > + indio_dev->num_channels = max_channels + 1;
>
> In the current array the timestamp channel isn't the next one, so how
> does this work?
>
> I think we either have to form the channel array dynamically or pick
> between
> one that does have the colour info and one that doesn't for the
> original case.
>
You are right, let me resubmit.
> Given timing we may just need to revert the broken patch and revisit
> this next
> cycle.
This is better. I will send a revert.
Thanks,
Srinivas
>
> Jonathan
>
>
> > +
> > indio_dev->info = &als_info;
> > indio_dev->name = name;
> > indio_dev->modes = INDIO_DIRECT_MODE;
>
