diff mbox series

[v3,3/4] IIO: hid-sensor-als: Use generic usage

Message ID 20221117234824.6227-3-p.jungkamp@gmx.net (mailing list archive)
State Changes Requested
Headers show
Series [v3,1/4] HID: hid-sensor-custom: Allow more custom iio sensors | expand

Commit Message

Philipp Jungkamp Nov. 17, 2022, 11:48 p.m. UTC 
Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS to
allow this driver to drive the Lenovo custom ambient light sensor,
which is registered under a 'custom' usage and not HID_USAGE_SENSOR_ALS.

Add the Lenovo Intelligent Sensing Solution (LISS) ambient light sensor
to the platform device ids.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Less unnecessary line breaks in function calls.

 drivers/iio/light/hid-sensor-als.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

--
2.38.1

Comments

srinivas pandruvada Nov. 18, 2022, 8:23 p.m. UTC | #1 
On Fri, 2022-11-18 at 00:48 +0100, Philipp Jungkamp wrote:
> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS to
> allow this driver to drive the Lenovo custom ambient light sensor,
> which is registered under a 'custom' usage and not
> HID_USAGE_SENSOR_ALS.
> 
> Add the Lenovo Intelligent Sensing Solution (LISS) ambient light sensor
> to the platform device ids.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
> Less unnecessary line breaks in function calls.
> 
>  drivers/iio/light/hid-sensor-als.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index 5a1a625d8d16..eb1aedad7edc 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -86,6 +86,7 @@ static int als_read_raw(struct iio_dev *indio_dev,
>                               long mask)
>  {
>         struct als_state *als_state = iio_priv(indio_dev);
> +       struct hid_sensor_hub_device *hsdev = als_state-
> >common_attributes.hsdev;
>         int report_id = -1;
>         u32 address;
>         int ret_type;
> @@ -110,11 +111,8 @@ static int als_read_raw(struct iio_dev *indio_dev,
>                         hid_sensor_power_state(&als_state-
> >common_attributes,
>                                                 true);
>                         *val = sensor_hub_input_attr_get_raw_value(
> -                                       als_state-
> >common_attributes.hsdev,
> -                                       HID_USAGE_SENSOR_ALS, address,
> -                                       report_id,
> -                                       SENSOR_HUB_SYNC,
> -                                       min < 0);
> +                                       hsdev, hsdev->usage, address,
> report_id,
> +                                       SENSOR_HUB_SYNC, min < 0);
>                         hid_sensor_power_state(&als_state-
> >common_attributes,
>                                                 false);
>                 } else {
> @@ -259,9 +257,7 @@ static int als_parse_report(struct platform_device
> *pdev,
>         dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
>                         st->als_illum.report_id);
> 
> -       st->scale_precision = hid_sensor_format_scale(
> -                               HID_USAGE_SENSOR_ALS,
> -                               &st->als_illum,
> +       st->scale_precision = hid_sensor_format_scale(usage_id, &st-
> >als_illum,
>                                 &st->scale_pre_decml, &st-
> >scale_post_decml);
> 
>         return ret;
> @@ -285,7 +281,8 @@ static int hid_als_probe(struct platform_device
> *pdev)
>         als_state->common_attributes.hsdev = hsdev;
>         als_state->common_attributes.pdev = pdev;
> 
> -       ret = hid_sensor_parse_common_attributes(hsdev,
> HID_USAGE_SENSOR_ALS,
> +       ret = hid_sensor_parse_common_attributes(hsdev,
> +                                       hsdev->usage,
>                                         &als_state->common_attributes,
>                                         als_sensitivity_addresses,
>                                         ARRAY_SIZE(als_sensitivity_addr
> esses));
> @@ -303,7 +300,8 @@ static int hid_als_probe(struct platform_device
> *pdev)
> 
>         ret = als_parse_report(pdev, hsdev,
>                                (struct iio_chan_spec *)indio_dev-
> >channels,
> -                              HID_USAGE_SENSOR_ALS, als_state);
> +                              hsdev->usage,
> +                              als_state);
>         if (ret) {
>                 dev_err(&pdev->dev, "failed to setup attributes\n");
>                 return ret;
> @@ -333,8 +331,7 @@ static int hid_als_probe(struct platform_device
> *pdev)
>         als_state->callbacks.send_event = als_proc_event;
>         als_state->callbacks.capture_sample = als_capture_sample;
>         als_state->callbacks.pdev = pdev;
> -       ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_ALS,
> -                                       &als_state->callbacks);
> +       ret = sensor_hub_register_callback(hsdev, hsdev->usage,
> &als_state->callbacks);
>         if (ret < 0) {
>                 dev_err(&pdev->dev, "callback reg failed\n");
>                 goto error_iio_unreg;
> @@ -356,7 +353,7 @@ static int hid_als_remove(struct platform_device
> *pdev)
>         struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>         struct als_state *als_state = iio_priv(indio_dev);
> 
> -       sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_ALS);
> +       sensor_hub_remove_callback(hsdev, hsdev->usage);
>         iio_device_unregister(indio_dev);
>         hid_sensor_remove_trigger(indio_dev, &als_state-
> >common_attributes);
> 
> @@ -368,6 +365,10 @@ static const struct platform_device_id
> hid_als_ids[] = {
>                 /* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>                 .name = "HID-SENSOR-200041",
>         },
> +       {
> +               /* Format: HID-SENSOR-custom_sensor_tag-
> usage_id_in_hex_lowercase */
> +               .name = "HID-SENSOR-LISS-0041",
> +       },
>         { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, hid_als_ids);
> --
> 2.38.1
>
Jiri Kosina Nov. 21, 2022, 5:59 p.m. UTC | #2 
On Fri, 18 Nov 2022, srinivas pandruvada wrote:

> > Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS to
> > allow this driver to drive the Lenovo custom ambient light sensor,
> > which is registered under a 'custom' usage and not
> > HID_USAGE_SENSOR_ALS.
> > 
> > Add the Lenovo Intelligent Sensing Solution (LISS) ambient light sensor
> > to the platform device ids.
> > 
> > Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Is my understanding correct that this Ack is valid for patches 1,2 and 3 
(and not for 4) from the series?

Thanks,
srinivas pandruvada Nov. 21, 2022, 7:55 p.m. UTC | #3 
On Mon, 2022-11-21 at 18:59 +0100, Jiri Kosina wrote:
> On Fri, 18 Nov 2022, srinivas pandruvada wrote:
> 
> > > Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS
> > > to
> > > allow this driver to drive the Lenovo custom ambient light
> > > sensor,
> > > which is registered under a 'custom' usage and not
> > > HID_USAGE_SENSOR_ALS.
> > > 
> > > Add the Lenovo Intelligent Sensing Solution (LISS) ambient light
> > > sensor
> > > to the platform device ids.
> > > 
> > > Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> 
> Is my understanding correct that this Ack is valid for patches 1,2
> and 3 
> (and not for 4) from the series?

Not for the series, I have some comments on others except 2.
So waiting for response.

Thanks,
Srinivas

> 
> Thanks,
>
Jonathan Cameron Nov. 23, 2022, 5:16 p.m. UTC | #4 
On Fri, 18 Nov 2022 00:48:23 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS to
> allow this driver to drive the Lenovo custom ambient light sensor,
> which is registered under a 'custom' usage and not HID_USAGE_SENSOR_ALS.
> 
> Add the Lenovo Intelligent Sensing Solution (LISS) ambient light sensor
> to the platform device ids.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>

One comment on the existing code inline. Not something that needs cleaning
up in this series, but would be nice if someone has time to deal with
it separately.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Less unnecessary line breaks in function calls.
> 
>  drivers/iio/light/hid-sensor-als.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 5a1a625d8d16..eb1aedad7edc 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -86,6 +86,7 @@ static int als_read_raw(struct iio_dev *indio_dev,
>  			      long mask)
>  {
>  	struct als_state *als_state = iio_priv(indio_dev);
> +	struct hid_sensor_hub_device *hsdev = als_state->common_attributes.hsdev;
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> @@ -110,11 +111,8 @@ static int als_read_raw(struct iio_dev *indio_dev,
>  			hid_sensor_power_state(&als_state->common_attributes,
>  						true);
>  			*val = sensor_hub_input_attr_get_raw_value(
> -					als_state->common_attributes.hsdev,
> -					HID_USAGE_SENSOR_ALS, address,
> -					report_id,
> -					SENSOR_HUB_SYNC,
> -					min < 0);
> +					hsdev, hsdev->usage, address, report_id,
> +					SENSOR_HUB_SYNC, min < 0);
>  			hid_sensor_power_state(&als_state->common_attributes,
>  						false);
>  		} else {
> @@ -259,9 +257,7 @@ static int als_parse_report(struct platform_device *pdev,
>  	dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
>  			st->als_illum.report_id);
> 
> -	st->scale_precision = hid_sensor_format_scale(
> -				HID_USAGE_SENSOR_ALS,
> -				&st->als_illum,
> +	st->scale_precision = hid_sensor_format_scale(usage_id, &st->als_illum,
>  				&st->scale_pre_decml, &st->scale_post_decml);
> 
>  	return ret;
> @@ -285,7 +281,8 @@ static int hid_als_probe(struct platform_device *pdev)
>  	als_state->common_attributes.hsdev = hsdev;
>  	als_state->common_attributes.pdev = pdev;
> 
> -	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_ALS,
> +	ret = hid_sensor_parse_common_attributes(hsdev,
> +					hsdev->usage,
>  					&als_state->common_attributes,
>  					als_sensitivity_addresses,
>  					ARRAY_SIZE(als_sensitivity_addresses));
> @@ -303,7 +300,8 @@ static int hid_als_probe(struct platform_device *pdev)
> 
>  	ret = als_parse_report(pdev, hsdev,
>  			       (struct iio_chan_spec *)indio_dev->channels,

Side comment so shouldn't affect this series, but this is nasty.

Channels should not be modified by casting away the const (type is
struct iio_chan_spec const *)

Much better to use a local copy of the pointer, modify the content and only assign
it to indio_dev once it is const.

> -			       HID_USAGE_SENSOR_ALS, als_state);
> +			       hsdev->usage,
> +			       als_state);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to setup attributes\n");
>  		return ret;
> @@ -333,8 +331,7 @@ static int hid_als_probe(struct platform_device *pdev)
>  	als_state->callbacks.send_event = als_proc_event;
>  	als_state->callbacks.capture_sample = als_capture_sample;
>  	als_state->callbacks.pdev = pdev;
> -	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_ALS,
> -					&als_state->callbacks);
> +	ret = sensor_hub_register_callback(hsdev, hsdev->usage, &als_state->callbacks);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
>  		goto error_iio_unreg;
> @@ -356,7 +353,7 @@ static int hid_als_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct als_state *als_state = iio_priv(indio_dev);
> 
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_ALS);
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  	iio_device_unregister(indio_dev);
>  	hid_sensor_remove_trigger(indio_dev, &als_state->common_attributes);
> 
> @@ -368,6 +365,10 @@ static const struct platform_device_id hid_als_ids[] = {
>  		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>  		.name = "HID-SENSOR-200041",
>  	},
> +	{
> +		/* Format: HID-SENSOR-custom_sensor_tag-usage_id_in_hex_lowercase */
> +		.name = "HID-SENSOR-LISS-0041",
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, hid_als_ids);
> --
> 2.38.1
>
diff mbox series

Patch

diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
index 5a1a625d8d16..eb1aedad7edc 100644
--- a/drivers/iio/light/hid-sensor-als.c
+++ b/drivers/iio/light/hid-sensor-als.c
@@ -86,6 +86,7 @@  static int als_read_raw(struct iio_dev *indio_dev,
 			      long mask)
 {
 	struct als_state *als_state = iio_priv(indio_dev);
+	struct hid_sensor_hub_device *hsdev = als_state->common_attributes.hsdev;
 	int report_id = -1;
 	u32 address;
 	int ret_type;
@@ -110,11 +111,8 @@  static int als_read_raw(struct iio_dev *indio_dev,
 			hid_sensor_power_state(&als_state->common_attributes,
 						true);
 			*val = sensor_hub_input_attr_get_raw_value(
-					als_state->common_attributes.hsdev,
-					HID_USAGE_SENSOR_ALS, address,
-					report_id,
-					SENSOR_HUB_SYNC,
-					min < 0);
+					hsdev, hsdev->usage, address, report_id,
+					SENSOR_HUB_SYNC, min < 0);
 			hid_sensor_power_state(&als_state->common_attributes,
 						false);
 		} else {
@@ -259,9 +257,7 @@  static int als_parse_report(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
 			st->als_illum.report_id);

-	st->scale_precision = hid_sensor_format_scale(
-				HID_USAGE_SENSOR_ALS,
-				&st->als_illum,
+	st->scale_precision = hid_sensor_format_scale(usage_id, &st->als_illum,
 				&st->scale_pre_decml, &st->scale_post_decml);

 	return ret;
@@ -285,7 +281,8 @@  static int hid_als_probe(struct platform_device *pdev)
 	als_state->common_attributes.hsdev = hsdev;
 	als_state->common_attributes.pdev = pdev;

-	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_ALS,
+	ret = hid_sensor_parse_common_attributes(hsdev,
+					hsdev->usage,
 					&als_state->common_attributes,
 					als_sensitivity_addresses,
 					ARRAY_SIZE(als_sensitivity_addresses));
@@ -303,7 +300,8 @@  static int hid_als_probe(struct platform_device *pdev)

 	ret = als_parse_report(pdev, hsdev,
 			       (struct iio_chan_spec *)indio_dev->channels,
-			       HID_USAGE_SENSOR_ALS, als_state);
+			       hsdev->usage,
+			       als_state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to setup attributes\n");
 		return ret;
@@ -333,8 +331,7 @@  static int hid_als_probe(struct platform_device *pdev)
 	als_state->callbacks.send_event = als_proc_event;
 	als_state->callbacks.capture_sample = als_capture_sample;
 	als_state->callbacks.pdev = pdev;
-	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_ALS,
-					&als_state->callbacks);
+	ret = sensor_hub_register_callback(hsdev, hsdev->usage, &als_state->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
 		goto error_iio_unreg;
@@ -356,7 +353,7 @@  static int hid_als_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct als_state *als_state = iio_priv(indio_dev);

-	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_ALS);
+	sensor_hub_remove_callback(hsdev, hsdev->usage);
 	iio_device_unregister(indio_dev);
 	hid_sensor_remove_trigger(indio_dev, &als_state->common_attributes);

@@ -368,6 +365,10 @@  static const struct platform_device_id hid_als_ids[] = {
 		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
 		.name = "HID-SENSOR-200041",
 	},
+	{
+		/* Format: HID-SENSOR-custom_sensor_tag-usage_id_in_hex_lowercase */
+		.name = "HID-SENSOR-LISS-0041",
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, hid_als_ids);