Re: [PATCH 1/8] iio: hid-sensor-als: Use channel index to support more hub attributes

From: Jonathan Cameron <jic23@kernel.org>
To: Basavaraj Natikar <Basavaraj.Natikar@amd.com>,
	<linux-iio@vger.kernel.org>
Cc: <jikos@kernel.org>, <benjamin.tissoires@redhat.com>,
	<lars@metafoo.de>, <srinivas.pandruvada@linux.intel.com>,
	<linux-input@vger.kernel.org>
Subject: Re: [PATCH 1/8] iio: hid-sensor-als: Use channel index to support more hub attributes
Date: Sun, 17 Sep 2023 11:56:18 +0100	[thread overview]
Message-ID: <20230917115618.1659a36c@jic23-huawei> (raw)
In-Reply-To: <20230915051703.1689578-2-Basavaraj.Natikar@amd.com>

On Fri, 15 Sep 2023 10:46:56 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote:

> Sensor hub attributes can be extended to support more channels. So in
> order to support more sensor hub attributes for ALS use channel index to
> get specific sensor hub attributes.
> 
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>

Hi Basavaraj,

> ---
>  drivers/iio/light/hid-sensor-als.c | 38 +++++++++++++++---------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index eb1aedad7edc..48879e233aec 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -24,7 +24,7 @@ enum {
>  struct als_state {
>  	struct hid_sensor_hub_callbacks callbacks;
>  	struct hid_sensor_common common_attributes;
> -	struct hid_sensor_hub_attribute_info als_illum;
> +	struct hid_sensor_hub_attribute_info als_illum[CHANNEL_SCAN_INDEX_MAX];

If being used for other channels, probably want to rename it as no longer
als_*illum*
>  	struct {
>  		u32 illum[CHANNEL_SCAN_INDEX_MAX];
>  		u64 timestamp __aligned(8);
> @@ -99,8 +99,8 @@ static int als_read_raw(struct iio_dev *indio_dev,
>  		switch (chan->scan_index) {
>  		case  CHANNEL_SCAN_INDEX_INTENSITY:
>  		case  CHANNEL_SCAN_INDEX_ILLUM:
> -			report_id = als_state->als_illum.report_id;
> -			min = als_state->als_illum.logical_minimum;
> +			report_id = als_state->als_illum[chan->scan_index].report_id;
> +			min = als_state->als_illum[chan->scan_index].logical_minimum;
>  			address = HID_USAGE_SENSOR_LIGHT_ILLUM;
>  			break;
>  		default:
> @@ -242,23 +242,23 @@ static int als_parse_report(struct platform_device *pdev,
>  				struct als_state *st)
>  {
>  	int ret;
> +	int i;
> +
> +	for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {

> +		ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT, usage_id,
> +							  HID_USAGE_SENSOR_LIGHT_ILLUM,
> +							  &st->als_illum[i]);

I would call out either as a comment here or in the patch description that we repeat
the same reading for the two existing channels and hence whilst we store them in
separate entries things continue to work as before where there was just one entry.

> +		if (ret < 0)
> +			return ret;
> +		als_adjust_channel_bit_mask(channels, i, st->als_illum[i].size);
> +
> +		dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum[i].index,
> +			st->als_illum[i].report_id);
> +	}
>  
> -	ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
> -			usage_id,
> -			HID_USAGE_SENSOR_LIGHT_ILLUM,
> -			&st->als_illum);
> -	if (ret < 0)
> -		return ret;
> -	als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_INTENSITY,
> -				    st->als_illum.size);
> -	als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_ILLUM,
> -					st->als_illum.size);
> -
> -	dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
> -			st->als_illum.report_id);
> -
> -	st->scale_precision = hid_sensor_format_scale(usage_id, &st->als_illum,
> -				&st->scale_pre_decml, &st->scale_post_decml);
> +	st->scale_precision = hid_sensor_format_scale(usage_id,
> +						      &st->als_illum[CHANNEL_SCAN_INDEX_INTENSITY],
> +						      &st->scale_pre_decml, &st->scale_post_decml);

Keep line lengths shorter anywhere it doesn't hurt readability.  Whilst the hard limit has
gone up we still prefer to keep under 80chars where it is easy to do.
Sometimes that means relaxing the alignment with opening bracket.

>  
>  	return ret;
>  }