On Mon, 18 Dec 2023 12:30:24 -0800
Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> Instead of assuming that every channel defined statically by
> als_channels[] is present, allocate 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.
>
> There is no intentional function changes done.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
Hi Srinivas,
No huge rush on this as I'll not have the revert in my upstream now
until after the merge window + may not have a chance for a second pull
request anyway.
A few comments inline,
Jonathan
> ---
> v2:
> New change
>
> drivers/iio/light/hid-sensor-als.c | 57 ++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 5cd27f04b45e..e57ad1946b56 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -236,14 +236,21 @@ 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)
> + struct hid_sensor_hub_device *hsdev,
> + struct iio_chan_spec **channels_out,
> + int *size_channels_out,
> + unsigned int usage_id,
> + struct als_state *st)
Align with opening bracket. Also, try in general to avoid white space changes
when changing anything else. It would be easier to see what actually changed
here without that.
> {
> - int ret;
> + struct iio_chan_spec *channels;
> + int ret, index = 0;
> int i;
>
> + channels = devm_kcalloc(&pdev->dev, ARRAY_SIZE(als_channels),
> + sizeof(als_channels), GFP_KERNEL);
Given it's a fixed size (which is fine even though you might not use it
all), can you just make it part of the iio_priv() structure
and avoid need for handling the allocation here?
> + if (!channels)
> + return -ENOMEM;
> +
> for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> @@ -251,9 +258,11 @@ static int als_parse_report(struct platform_device *pdev,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> &st->als[i]);
> if (ret < 0)
> - return ret;
> - als_adjust_channel_bit_mask(channels, i, st->als[i].size);
> + break;
>
> + channels[i] = als_channels[i];
channels[index] = als_channels[i]? Might be gaps. What you currently have
only works if the 'present channels' are all at the start.
> + als_adjust_channel_bit_mask(channels, i, st->als[i].size);
Would use index not i.
> + ++index;
> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> st->als[i].report_id);
> }
> @@ -262,17 +271,24 @@ static int als_parse_report(struct platform_device *pdev,
> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st->scale_post_decml);
>
> - return ret;
> + *channels_out = channels;
> + *size_channels_out = index;
> +
> + if (!index)
> + ret = -ENODEV;
> +
> + return 0;
> }
>
> /* Function to initialize the processing for usage id */
> static int hid_als_probe(struct platform_device *pdev)
> {
> - int ret = 0;
> + int ret = 0, max_channel_index;
> static const char *name = "als";
> struct iio_dev *indio_dev;
> struct als_state *als_state;
> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
> + struct iio_chan_spec *channels;
>
> indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct als_state));
> if (!indio_dev)
> @@ -293,24 +309,21 @@ 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);
> + ret = als_parse_report(pdev, hsdev, &channels, &max_channel_index,
> + hsdev->usage, als_state);
> if (ret) {
> dev_err(&pdev->dev, "failed to setup attributes\n");
> return ret;
> }
>
> - indio_dev->num_channels =
> - ARRAY_SIZE(als_channels);
> + /* Add timestamp channel */
> + channels[max_channel_index] = als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
> + channels[max_channel_index].scan_index = max_channel_index;
> +
> + /* +1 for adding timestamp channel */
> + indio_dev->num_channels = max_channel_index + 1;
> +
> + indio_dev->channels = channels;
> indio_dev->info = &als_info;
> indio_dev->name = name;
> indio_dev->modes = INDIO_DIRECT_MODE;
