Received-SPF: pass (google.com: domain of linux-kernel+bounces-7153-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 20 Dec 2023 15:24:46 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: jikos@kernel.org, lars@metafoo.de, Basavaraj.Natikar@amd.com,
 linux-input@vger.kernel.org, linux-iio@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] iio: hid-sensor-als: Allocate channels
 dynamically
Message-ID: <20231220152446.18d220d2@jic23-huawei>
In-Reply-To: <20231218203026.1156375-2-srinivas.pandruvada@linux.intel.com>
References: <20231218203026.1156375-1-srinivas.pandruvada@linux.intel.com>
	<20231218203026.1156375-2-srinivas.pandruvada@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 18 Dec 2023 12:30:24 -0800
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> 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@linux.intel.com>
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;