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 <[email protected]>
---
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};
+
/* 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_LIGHT_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;
--
2.43.0
On Sun, 4 Feb 2024 05:03:29 -0800
Srinivas Pandruvada <[email protected]> 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 <[email protected]>
Hi Srinivas,
Logic looks fine, but not that global variable...
> ---
> 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_LIGHT_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;
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 <[email protected]> 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
> > <[email protected]>
> 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;
>