Re: [PATCH] iio: light: hid-sensor-als: Avoid failure for chromaticity support

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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@linux.intel.com> 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@t-8ch.de>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218223
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  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;
>