iio: hid-sensor-als: Don't stop probing at non-supported attribute
Commit Message
Some ambient light sensors don't support color temperature and
chromaticity attributes. The driver stops probing if it finds this.
To support sensors without of color temperature and chromaticity
attributes, just skip them at probing if they weren't found.
Tested at Lenovo Yogabook YB1-X91L tablet.
Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
---
drivers/iio/light/hid-sensor-als.c | 39 ++++++++++++++++++------------
1 file changed, 23 insertions(+), 16 deletions(-)
Comments
On Sat, Dec 16, 2023 at 01:42:29PM +0200, Yauhen Kharuzhy wrote:
> Some ambient light sensors don't support color temperature and
> chromaticity attributes. The driver stops probing if it finds this.
>
> To support sensors without of color temperature and chromaticity
> attributes, just skip them at probing if they weren't found.
>
> Tested at Lenovo Yogabook YB1-X91L tablet.
Hi, It seems that Srinivas Pandruvada has posted another patch fixing
the same issue. So, drop my patch in favor of his one.
>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 39 ++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index f17304b54468..b711bac3bb2b 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -314,8 +314,11 @@ static int als_parse_report(struct platform_device *pdev,
> usage_id,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> &st->als[i]);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "Failed to setup Illuminance attribute\n");
> return ret;
> + }
> als_adjust_channel_bit_mask(channels, i, st->als[i].size);
>
> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> @@ -326,14 +329,16 @@ 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;
> - als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> + if (!ret) {
> + dev_info(&pdev->dev, "Color temperature is supported\n");
> + als_adjust_channel_bit_mask(channels,
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
>
> - dev_dbg(&pdev->dev, "als %x:%x\n",
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> + }
>
> for (i = 0; i < 2; i++) {
> int next_scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X + i;
> @@ -342,23 +347,25 @@ static int als_parse_report(struct platform_device *pdev,
> HID_INPUT_REPORT, usage_id,
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X + i,
> &st->als[next_scan_index]);
> - if (ret < 0)
> - return ret;
> -
> - als_adjust_channel_bit_mask(channels,
> + if (!ret) {
> + dev_info(&pdev->dev,
> + "Light chromaticity %c is supported\n",
> + i ? 'Y' : 'X');
> + als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_CHROMATICITY_X + i,
> st->als[next_scan_index].size);
>
> - dev_dbg(&pdev->dev, "als %x:%x\n",
> - st->als[next_scan_index].index,
> - st->als[next_scan_index].report_id);
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[next_scan_index].index,
> + st->als[next_scan_index].report_id);
> + }
> }
>
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st->scale_post_decml);
>
> - return ret;
> + return 0;
> }
>
> /* Function to initialize the processing for usage id */
> --
> 2.43.0
>
On Sat, 16 Dec 2023 13:42:29 +0200
Yauhen Kharuzhy <jekhor@gmail.com> wrote:
> Some ambient light sensors don't support color temperature and
> chromaticity attributes. The driver stops probing if it finds this.
>
> To support sensors without of color temperature and chromaticity
> attributes, just skip them at probing if they weren't found.
>
> Tested at Lenovo Yogabook YB1-X91L tablet.
>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
i reviewed this one as as well as Srinivas' as that had issues that need solving.
This one just seems to half paper over the problem It won't update the channels
etc but the set of channels provided to userspace are still garbage.
So better than before, but not fixing the issue fully.
Jonathan
> ---
> drivers/iio/light/hid-sensor-als.c | 39 ++++++++++++++++++------------
> 1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index f17304b54468..b711bac3bb2b 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -314,8 +314,11 @@ static int als_parse_report(struct platform_device *pdev,
> usage_id,
> HID_USAGE_SENSOR_LIGHT_ILLUM,
> &st->als[i]);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "Failed to setup Illuminance attribute\n");
> return ret;
> + }
Unrelated change. For a fix we should look to keep things minimal.
> als_adjust_channel_bit_mask(channels, i, st->als[i].size);
>
> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> @@ -326,14 +329,16 @@ 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;
> - als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> + if (!ret) {
> + dev_info(&pdev->dev, "Color temperature is supported\n");
I'd argue we shouldn't print a message on this.
Use the availability of channels after driver is probed to figure this out if
needed.
> + als_adjust_channel_bit_mask(channels,
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
>
> - dev_dbg(&pdev->dev, "als %x:%x\n",
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> - st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> + }
>
> for (i = 0; i < 2; i++) {
> int next_scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X + i;
> @@ -342,23 +347,25 @@ static int als_parse_report(struct platform_device *pdev,
> HID_INPUT_REPORT, usage_id,
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X + i,
> &st->als[next_scan_index]);
> - if (ret < 0)
> - return ret;
> -
> - als_adjust_channel_bit_mask(channels,
> + if (!ret) {
> + dev_info(&pdev->dev,
> + "Light chromaticity %c is supported\n",
> + i ? 'Y' : 'X');
> + als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_CHROMATICITY_X + i,
> st->als[next_scan_index].size);
>
> - dev_dbg(&pdev->dev, "als %x:%x\n",
> - st->als[next_scan_index].index,
> - st->als[next_scan_index].report_id);
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[next_scan_index].index,
> + st->als[next_scan_index].report_id);
> + }
> }
>
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st->scale_post_decml);
>
> - return ret;
> + return 0;
> }
>
> /* Function to initialize the processing for usage id */
@@ -314,8 +314,11 @@ static int als_parse_report(struct platform_device *pdev,
usage_id,
HID_USAGE_SENSOR_LIGHT_ILLUM,
&st->als[i]);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "Failed to setup Illuminance attribute\n");
return ret;
+ }
als_adjust_channel_bit_mask(channels, i, st->als[i].size);
dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
@@ -326,14 +329,16 @@ 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;
- als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
- st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
+ if (!ret) {
+ dev_info(&pdev->dev, "Color temperature is supported\n");
+ als_adjust_channel_bit_mask(channels,
+ CHANNEL_SCAN_INDEX_COLOR_TEMP,
+ st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
- dev_dbg(&pdev->dev, "als %x:%x\n",
- st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
- st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
+ dev_dbg(&pdev->dev, "als %x:%x\n",
+ st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
+ st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
+ }
for (i = 0; i < 2; i++) {
int next_scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X + i;
@@ -342,23 +347,25 @@ static int als_parse_report(struct platform_device *pdev,
HID_INPUT_REPORT, usage_id,
HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X + i,
&st->als[next_scan_index]);
- if (ret < 0)
- return ret;
-
- als_adjust_channel_bit_mask(channels,
+ if (!ret) {
+ dev_info(&pdev->dev,
+ "Light chromaticity %c is supported\n",
+ i ? 'Y' : 'X');
+ als_adjust_channel_bit_mask(channels,
CHANNEL_SCAN_INDEX_CHROMATICITY_X + i,
st->als[next_scan_index].size);
- dev_dbg(&pdev->dev, "als %x:%x\n",
- st->als[next_scan_index].index,
- st->als[next_scan_index].report_id);
+ dev_dbg(&pdev->dev, "als %x:%x\n",
+ st->als[next_scan_index].index,
+ st->als[next_scan_index].report_id);
+ }
}
st->scale_precision = hid_sensor_format_scale(usage_id,
&st->als[CHANNEL_SCAN_INDEX_INTENSITY],
&st->scale_pre_decml, &st->scale_post_decml);
- return ret;
+ return 0;
}
/* Function to initialize the processing for usage id */