Hi Srinivas,
On 1/9/2024 11:30 PM, Srinivas Pandruvada 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.
>
> When not all usage ids are present, the scan index is adjusted to
> exclude unsupported channels.
>
> There is no intentional function changes done.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
> 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 | 56 +++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 5cd27f04b45e..72a7c01c97f8 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -25,17 +25,26 @@ 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];
> + u32 scan_index[CHANNEL_SCAN_INDEX_MAX];
> u64 timestamp __aligned(8);
> } scan;
> int scale_pre_decml;
> 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,
> @@ -216,11 +225,14 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
> struct als_state *als_state = iio_priv(indio_dev);
> int ret = -EINVAL;
> u32 sample_data = *(u32 *)raw_data;
> + int scan_index;
>
> switch (usage_id) {
> case HID_USAGE_SENSOR_LIGHT_ILLUM:
> - als_state->scan.illum[CHANNEL_SCAN_INDEX_INTENSITY] = sample_data;
> - als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_INTENSITY];
> + als_state->scan.illum[scan_index] = sample_data;
> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_ILLUM];
> + als_state->scan.illum[scan_index] = sample_data;
Could you please use original above change which works fine for some reason if
we use this change als_state->scan.scan_index we are getting garbage value.
> ret = 0;
> break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> @@ -237,27 +249,39 @@ 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;
>
> + channels = st->channels;
> +
> for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
Could you please change CHANNEL_SCAN_INDEX_ILLUM to CHANNEL_SCAN_INDEX_MAX
which helps remaining 2 patches in the series.
> 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];
> + st->scan.scan_index[i] = index;
> +
> + 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);
> @@ -293,15 +317,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) {
> @@ -309,8 +325,14 @@ 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];
> + als_state->channels[als_state->num_channels].scan_index = als_state->num_channels;
> +
> + /* +1 for adding timestamp channel */
> + indio_dev->num_channels = als_state->num_channels + 1;
> +
> + indio_dev->channels = als_state->channels;
> indio_dev->info = &als_info;
> indio_dev->name = name;
> indio_dev->modes = INDIO_DIRECT_MODE;
