Received-SPF: pass (google.com: domain of linux-kernel+bounces-2647-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 17 Dec 2023 14:35:55 +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, Thomas =?UTF-8?B?V2Vpw59zY2h1aA==?=
 <thomas@t-8ch.de>, stable@vger.kernel.org
Subject: Re: [PATCH] iio: light: hid-sensor-als: Avoid failure for
 chromaticity support
Message-ID: <20231217143555.1f89ddaa@jic23-huawei>
In-Reply-To: <20231215160159.648963-1-srinivas.pandruvada@linux.intel.com>
References: <20231215160159.648963-1-srinivas.pandruvada@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, 15 Dec 2023 08:01:59 -0800
Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote:

> With the commit ee3710f39f9d ("iio: hid-sensor-als: Add light chromaticity
> support"), there is an assumption that the every HID ALS descriptor has
> support of usage ids for chromaticity support. If they are not present,
> probe fails for the driver . This breaks ALS functionality on majority of
> platforms.
>=20
> It is possible that chromaticity usage ids are not present. When not
> present, restrict number of IIO channels to not include support for
> chromaticity and continue.
>=20
> Fixes: ee3710f39f9d ("iio: hid-sensor-als: Add light chromaticity support=
")
> Reported-by: Thomas Wei=C3=9Fschuh <thomas@t-8ch.de>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=3D218223
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/iio/light/hid-sensor-als.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>=20
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-s=
ensor-als.c
> index f17304b54468..9941b0b927c7 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -303,11 +303,14 @@ 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 als_state *st,
> +				int *max_channels)
>  {
>  	int ret;
>  	int i;
> =20
> +	*max_channels =3D CHANNEL_SCAN_INDEX_MAX;
> +
>  	for (i =3D 0; i <=3D CHANNEL_SCAN_INDEX_ILLUM; ++i) {
>  		ret =3D sensor_hub_input_get_attribute_info(hsdev,
>  						HID_INPUT_REPORT,
> @@ -326,8 +329,12 @@ static int als_parse_report(struct platform_device *=
pdev,
>  				usage_id,
>  				HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE,
>  				&st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
> -	if (ret < 0)
> -		return ret;
> +	if (ret < 0) {
> +		*max_channels =3D CHANNEL_SCAN_INDEX_ILLUM;
> +		ret =3D 0;
> +		goto skip_color_chromaticity;
> +	}
> +
>  	als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
>  				st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> =20
> @@ -354,6 +361,7 @@ static int als_parse_report(struct platform_device *p=
dev,
>  			st->als[next_scan_index].report_id);
>  	}
> =20
> +skip_color_chromaticity:
>  	st->scale_precision =3D hid_sensor_format_scale(usage_id,
>  				&st->als[CHANNEL_SCAN_INDEX_INTENSITY],
>  				&st->scale_pre_decml, &st->scale_post_decml);
> @@ -364,7 +372,7 @@ static int als_parse_report(struct platform_device *p=
dev,
>  /* Function to initialize the processing for usage id */
>  static int hid_als_probe(struct platform_device *pdev)
>  {
> -	int ret =3D 0;
> +	int ret =3D 0, max_channels;
>  	static const char *name =3D "als";
>  	struct iio_dev *indio_dev;
>  	struct als_state *als_state;
> @@ -398,15 +406,15 @@ static int hid_als_probe(struct platform_device *pd=
ev)
> =20
>  	ret =3D als_parse_report(pdev, hsdev,
>  			       (struct iio_chan_spec *)indio_dev->channels,
> -			       hsdev->usage,
> -			       als_state);
> +			       hsdev->usage, als_state, &max_channels);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to setup attributes\n");
>  		return ret;
>  	}
> =20
> -	indio_dev->num_channels =3D
> -				ARRAY_SIZE(als_channels);
> +	/* +1 to include time stamp */
> +	indio_dev->num_channels =3D max_channels + 1;

In the current array the timestamp channel isn't the next one, so how does =
this work?

I think we either have to form the channel array dynamically or pick between
one that does have the colour info and one that doesn't for the original ca=
se.

Given timing we may just need to revert the broken patch and revisit this n=
ext
cycle.

Jonathan


> +
>  	indio_dev->info =3D &als_info;
>  	indio_dev->name =3D name;
>  	indio_dev->modes =3D INDIO_DIRECT_MODE;