Re: [PATCH v4 1/4] iio: hid-sensor-als: Assign channels dynamically

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jontahan,

On Sun, 2024-02-04 at 15:58 +0000, Jonathan Cameron wrote:
> On Sun,  4 Feb 2024 05:03:29 -0800
> Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> 
> > Instead of assuming that every channel defined statically by
> > als_channels[] is present, assign dynamically based on presence of
> > the
> > respective usage id in the descriptor. This will allow to register
> > ALS
> > with limited channel support. Append the timestamp as the last
> > channel.
> > 
> > Update available_scan_mask to specify all channels which are
> > present.
> > 
> > There is no intentional function changes done.
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Hi Srinivas,
> 
> Logic looks fine, but not that global variable...
This will be a problem with multiple instances with different
combination. Although not seen in current devices, but this is
possible.
So will send an update.

Thanks,
Srinivas

> 

> > ---
> > v4:
> > Addressed comments from Jonthan:
> >         - Use available_scan_masks
> >         - timestamp location is fixed and left gaps in sample data
> > for absent channels
> >         - Use CHANNEL_SCAN_INDEX_MAX as limit to check presence of
> > usage ids, otherwise
> >           it will miss newer channels added in subsequent patches.
> > v3:
> > Addressed comments from Jonthan:
> >         - Remove channel allocation and move to iio_priv()
> >         - Parse all usage IDs in a single loop and continue
> >         for failure. This way the temperature and chromaticity
> >         will not need any special processing to parse usage ids.
> >         - Don't leave empty channel indexes
> > 
> > v2:
> > New change
> > 
> >  drivers/iio/light/hid-sensor-als.c | 52 +++++++++++++++++++++-----
> > ----
> >  1 file changed, 36 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/iio/light/hid-sensor-als.c
> > b/drivers/iio/light/hid-sensor-als.c
> > index b6c4bef2a7bb..d3b892c0e307 100644
> > --- a/drivers/iio/light/hid-sensor-als.c
> > +++ b/drivers/iio/light/hid-sensor-als.c
> > @@ -25,6 +25,7 @@ struct als_state {
> >         struct hid_sensor_hub_callbacks callbacks;
> >         struct hid_sensor_common common_attributes;
> >         struct hid_sensor_hub_attribute_info
> > als[CHANNEL_SCAN_INDEX_MAX];
> > +       struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
> >         struct {
> >                 u32 illum[CHANNEL_SCAN_INDEX_MAX];
> >                 u64 timestamp __aligned(8);
> > @@ -33,9 +34,16 @@ struct als_state {
> >         int scale_post_decml;
> >         int scale_precision;
> >         int value_offset;
> > +       int num_channels;
> >         s64 timestamp;
> >  };
> >  
> > +/* The order of usage ids must match scan index starting from
> > CHANNEL_SCAN_INDEX_INTENSITY */
> > +static const u32 als_usage_ids[] = {
> > +       HID_USAGE_SENSOR_LIGHT_ILLUM,
> > +       HID_USAGE_SENSOR_LIGHT_ILLUM,
> > +};
> > +
> >  static const u32 als_sensitivity_addresses[] = {
> >         HID_USAGE_SENSOR_DATA_LIGHT,
> >         HID_USAGE_SENSOR_LIGHT_ILLUM,
> > @@ -68,6 +76,8 @@ static const struct iio_chan_spec als_channels[]
> > = {
> >         IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> >  };
> >  
> > +static unsigned long als_scan_mask[] = {0, 0};
> 
> Global? No. Could be multiple instances with different sensors
> supported.
> Needs to be embedded in the als_state structure so it is per
> instance.
> 
> > +
> >  /* Adjust channel real bits based on report descriptor */
> >  static void als_adjust_channel_bit_mask(struct iio_chan_spec
> > *channels,
> >                                         int channel, int size)
> > @@ -238,27 +248,38 @@ static int als_capture_sample(struct
> > hid_sensor_hub_device *hsdev,
> >  /* Parse report which is specific to an usage id*/
> >  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)
> >  {
> > -       int ret;
> > +       struct iio_chan_spec *channels;
> > +       int ret, index = 0;
> >         int i;
> >  
> > -       for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> > +       channels = st->channels;
> > +
> > +       for (i = 0; i < CHANNEL_SCAN_INDEX_MAX; ++i) {
> >                 ret = sensor_hub_input_get_attribute_info(hsdev,
> >                                                 HID_INPUT_REPORT,
> >                                                 usage_id,
> > -
> >                                                HID_USAGE_SENSOR_LIGH
> > T_ILLUM,
> > +                                               als_usage_ids[i],
> >                                                 &st->als[i]);
> >                 if (ret < 0)
> > -                       return ret;
> > -               als_adjust_channel_bit_mask(channels, i, st-
> > >als[i].size);
> > +                       continue;
> > +
> > +               channels[index] = als_channels[i];
> > +               als_scan_mask[0] |= BIT(i);
> > +               als_adjust_channel_bit_mask(channels, index, st-
> > >als[i].size);
> > +               ++index;
> >  
> >                 dev_dbg(&pdev->dev, "als %x:%x\n", st-
> > >als[i].index,
> >                         st->als[i].report_id);
> >         }
> >  
> > +       st->num_channels = index;
> > +       /* Return success even if one usage id is present */
> > +       if (index)
> > +               ret = 0;
> > +
> >         st->scale_precision = hid_sensor_format_scale(usage_id,
> >                                 &st-
> > >als[CHANNEL_SCAN_INDEX_INTENSITY],
> >                                 &st->scale_pre_decml, &st-
> > >scale_post_decml);
> > @@ -294,15 +315,7 @@ static int hid_als_probe(struct
> > platform_device *pdev)
> >                 return ret;
> >         }
> >  
> > -       indio_dev->channels = devm_kmemdup(&pdev->dev,
> > als_channels,
> > -                                          sizeof(als_channels),
> > GFP_KERNEL);
> > -       if (!indio_dev->channels) {
> > -               dev_err(&pdev->dev, "failed to duplicate
> > channels\n");
> > -               return -ENOMEM;
> > -       }
> > -
> >         ret = als_parse_report(pdev, hsdev,
> > -                              (struct iio_chan_spec *)indio_dev-
> > >channels,
> >                                hsdev->usage,
> >                                als_state);
> >         if (ret) {
> > @@ -310,8 +323,15 @@ static int hid_als_probe(struct
> > platform_device *pdev)
> >                 return ret;
> >         }
> >  
> > -       indio_dev->num_channels =
> > -                               ARRAY_SIZE(als_channels);
> > +       /* Add timestamp channel */
> > +       als_state->channels[als_state->num_channels] =
> > als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
> > +
> > +       /* +1 for adding timestamp channel */
> > +       indio_dev->num_channels = als_state->num_channels + 1;
> > +
> > +       indio_dev->channels = als_state->channels;
> > +       indio_dev->available_scan_masks = als_scan_mask;
> > +
> >         indio_dev->info = &als_info;
> >         indio_dev->name = name;
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> 





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux