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;
>
