PROBLEM: Lenovo ALS sensor disguised under custom usage

PROBLEM: Lenovo ALS sensor disguised under custom usage

[Philipp Jungkamp]

Hello,

my Lenovo Yoga 9 14IAP7 includes several sensors above the screen which
are connected to the Intel ISH of this device. The report descriptor of
the sensor hub categorizes them in a custom usage. But fields under
that usage are clearly those of a sensor.

I went ahead and activated the sensor I believed to be for ambient
light sensing and checked the values reported. The sensor behaves like
a virtual ISH sensor. It does not report values on a synchronous
request. But, when in buffered mode, reports the light-level as
expected.

A modified version of hid-sensor-custom.c allowed for other known
sensors beside the intel-custom-hinge-sensor and exposed it under a
modified name as an hid sensor. I slightly modified the
hid-sensor-als.c to allow it to bind to this device and it works
without problems.

But, maybe a little late, I'm considering whether a fixup for the ish
report descriptor would be the better way to go.

I'll post the patches I used in reply to this message for discussion.
And I'd really like to see at least the ambient light sensor working on
mainline as an iio-sensor.

Lenovo seems to use the sensors for some proprietary features of their
Lenovo Vantage application under the name Lenovo Intelligent Sensing
Solution.

Regards,
Philipp Jungkamp


I used this fish script to query some information on the sensors:
https://raw.githubusercontent.com/PJungkamp/yoga9-linux/main/intel-ish/find_sensors.fish

Which yields this output on my device:
https://raw.githubusercontent.com/PJungkamp/yoga9-linux/main/intel-ish/sensors.txt

Here is the report descriptor for the relevant ISH device (already
decoded text file):
https://raw.githubusercontent.com/PJungkamp/yoga9-linux/main/intel-ish/ish_report_descriptor

[PATCH 1/3] HID: hid-sensor-custom: More custom iio sensors

[Philipp Jungkamp]

The known LUID table for established/known custom HID sensors was
limited to sensors with "INTEL" as manufacturer. But some vendors such
as Lenovo also include fairly standard iio sensors (e.g. ambient light)
in their custom sensors.

Expand the known custom sensors table by a tag used for the platform
device name and match sensors based on the LUID as well as optionally
on model and manufacturer properties.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
 drivers/hid/hid-sensor-custom.c | 225 +++++++++++++++++++++++---------
 include/linux/hid-sensor-ids.h  |   1 +
 2 files changed, 161 insertions(+), 65 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 32c2306e240d..cb21f9178830 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -5,6 +5,7 @@
  */

 #include <linux/ctype.h>
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -750,114 +751,208 @@ static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom

 }

-/* luid defined in FW (e.g. ISH).  Maybe used to identify sensor. */
-static const char *const known_sensor_luid[] = { "020B000000000000" };
+/*
+ * Match a known custom sensor.
+ * tag and luid is mandatory.
+ */
+struct hid_sensor_custom_match {
+	const char *tag;
+	const char *luid;
+	const char *model;
+	const char *manufacturer;
+	bool check_dmi;
+	struct dmi_system_id dmi;
+};

-static int get_luid_table_index(unsigned char *usage_str)
-{
-	int i;
+/*
+ * Custom sensor properties used for matching.
+ */
+struct hid_sensor_custom_properties {
+	u16 serial_num[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 model[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 manufacturer[HID_CUSTOM_MAX_FEATURE_BYTES];
+};

-	for (i = 0; i < ARRAY_SIZE(known_sensor_luid); i++) {
-		if (!strncmp(usage_str, known_sensor_luid[i],
-			     strlen(known_sensor_luid[i])))
-			return i;
+static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
+	/*
+	 * Intel Integrated Sensor Hub (ISH)
+	 */
+	{	/* Intel ISH hinge */
+		.tag = "INT",
+		.luid = "020B000000000000",
+		.manufacturer = "INTEL",
+	},
+	{}
+};
+
+static int hid_sensor_custom_prop_match_str(const u16 *prop, const char *match,
+					    size_t max_count)
+{
+	while (max_count-- && *prop && *match) {
+		if (*prop & 0xFF00 ||
+		    *match != (char) *prop)
+			return 0;
+		prop++;
+		match++;
 	}

-	return -ENODEV;
+	return 1;
 }

-static int get_known_custom_sensor_index(struct hid_sensor_hub_device *hsdev)
+static int hid_sensor_custom_get_prop(struct hid_sensor_hub_device *hsdev,
+				      u32 prop_usage_id, size_t prop_size,
+				      u16 *prop)
 {
-	struct hid_sensor_hub_attribute_info sensor_manufacturer = { 0 };
-	struct hid_sensor_hub_attribute_info sensor_luid_info = { 0 };
+	struct hid_sensor_hub_attribute_info prop_attr = { 0 };
 	int report_size;
 	int ret;
-	static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	int i;

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	memset(prop, 0, prop_size);

-	/* get manufacturer info */
+	/* get property info */
 	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_MANUFACTURER, &sensor_manufacturer);
+						  HID_FEATURE_REPORT,
+						  hsdev->usage,
+						  prop_usage_id,
+						  &prop_attr);
+	/* property does not exist */
 	if (ret < 0)
-		return ret;
+		return -ENODATA;

 	report_size =
-		sensor_hub_get_feature(hsdev, sensor_manufacturer.report_id,
-				       sensor_manufacturer.index, sizeof(w_buf),
-				       w_buf);
+		sensor_hub_get_feature(hsdev, prop_attr.report_id,
+				       prop_attr.index, prop_size,
+				       prop);
 	if (report_size <= 0) {
 		hid_err(hsdev->hdev,
-			"Failed to get sensor manufacturer info %d\n",
+			"Failed to get sensor property %08x %d\n",
+			prop_usage_id,
 			report_size);
 		return -ENODEV;
 	}

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	return 0;
+}

-	/* ensure it's ISH sensor */
-	if (strncmp(buf, "INTEL", strlen("INTEL")))
-		return -ENODEV;
+static int
+hid_sensor_custom_do_match(struct hid_sensor_hub_device *hsdev,
+			   const struct hid_sensor_custom_match *match,
+			   const struct hid_sensor_custom_properties *prop)
+{
+	struct dmi_system_id dmi[] = { match->dmi, { 0 } };

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	/*
+	 * Match the LUID property.
+	 */
+	if (!hid_sensor_custom_prop_match_str(prop->serial_num, "LUID:", 5) ||
+	    !hid_sensor_custom_prop_match_str(prop->serial_num + 5,
+					      match->luid,
+					      HID_CUSTOM_MAX_FEATURE_BYTES - 5))
+		return 0;

-	/* get real usage id */
-	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_SERIAL_NUM, &sensor_luid_info);
+	/*
+	 * Match the model property. (optional)
+	 */
+	if (match->model &&
+	    !hid_sensor_custom_prop_match_str(prop->model,
+					      match->model,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return 0;
+
+	/*
+	 * Match the manufacturer property. (optional)
+	 */
+	if (match->manufacturer &&
+	    !hid_sensor_custom_prop_match_str(prop->manufacturer,
+					      match->manufacturer,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return 0;
+
+	/*
+	 * Match DMI. (optional)
+	 */
+	if (match->check_dmi && !dmi_check_system(dmi))
+		return 0;
+
+	return 1;
+}
+
+static int
+hid_sensor_custom_properties_get(struct hid_sensor_hub_device *hsdev,
+				 struct hid_sensor_custom_properties *prop)
+{
+	int ret;
+
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_SERIAL_NUM,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->serial_num);
 	if (ret < 0)
 		return ret;

-	report_size = sensor_hub_get_feature(hsdev, sensor_luid_info.report_id,
-					     sensor_luid_info.index, sizeof(w_buf),
-					     w_buf);
-	if (report_size <= 0) {
-		hid_err(hsdev->hdev, "Failed to get real usage info %d\n",
-			report_size);
-		return -ENODEV;
-	}
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MODEL,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->model);
+	if (ret < 0 && ret != -ENODATA)
+		return ret;

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MANUFACTURER,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->manufacturer);
+	if (ret < 0 && ret != -ENODATA)
+		return ret;

-	if (strlen(buf) != strlen(known_sensor_luid[0]) + 5) {
-		hid_err(hsdev->hdev,
-			"%s luid length not match %zu != (%zu + 5)\n", __func__,
-			strlen(buf), strlen(known_sensor_luid[0]));
+	return 0;
+}
+
+
+static int
+hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
+			    const struct hid_sensor_custom_match **known)
+{
+	int ret;
+	const struct hid_sensor_custom_match *match =
+		hid_sensor_custom_known_table;
+	struct hid_sensor_custom_properties prop;
+
+	ret = hid_sensor_custom_properties_get(hsdev, &prop);
+	if (ret < 0)
 		return -ENODEV;
+
+	while (match->tag) {
+		if (hid_sensor_custom_do_match(hsdev, match, &prop)) {
+			*known = match;
+			return 0;
+		}
+		match++;
 	}

-	/* get table index with luid (not matching 'LUID: ' in luid) */
-	return get_luid_table_index(&buf[5]);
+	return -ENODEV;
 }

 static struct platform_device *
 hid_sensor_register_platform_device(struct platform_device *pdev,
 				    struct hid_sensor_hub_device *hsdev,
-				    int index)
+				    const struct hid_sensor_custom_match *match)
 {
-	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
+	char real_usage[HID_SENSOR_USAGE_LENGTH];
 	struct platform_device *custom_pdev;
 	const char *dev_name;
 	char *c;

 	/* copy real usage id */
-	memcpy(real_usage, known_sensor_luid[index], 4);
+	memcpy(real_usage, match->luid, 4);
+	real_usage[4] = '\0';

-	/* usage id are all lowcase */
+	/* usage ids are all lowcase */
 	for (c = real_usage; *c != '\0'; c++)
 		*c = tolower(*c);

-	/* HID-SENSOR-INT-REAL_USAGE_ID */
-	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
+	/* HID-SENSOR-tag-real_usage */
+	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
+			     match->tag, real_usage);
 	if (!dev_name)
 		return ERR_PTR(-ENOMEM);

@@ -873,7 +968,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	struct hid_sensor_custom *sensor_inst;
 	struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
 	int ret;
-	int index;
+	const struct hid_sensor_custom_match *match = NULL;

 	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
 				   GFP_KERNEL);
@@ -888,10 +983,10 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	mutex_init(&sensor_inst->mutex);
 	platform_set_drvdata(pdev, sensor_inst);

-	index = get_known_custom_sensor_index(hsdev);
-	if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
+	ret = hid_sensor_custom_get_known(hsdev, &match);
+	if (!ret && match) {
 		sensor_inst->custom_pdev =
-			hid_sensor_register_platform_device(pdev, hsdev, index);
+			hid_sensor_register_platform_device(pdev, hsdev, match);

 		ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
 		if (ret) {
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index ac631159403a..13b1e65fbdcc 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -132,6 +132,7 @@
 #define HID_USAGE_SENSOR_PROP_FRIENDLY_NAME			0x200301
 #define HID_USAGE_SENSOR_PROP_SERIAL_NUM			0x200307
 #define HID_USAGE_SENSOR_PROP_MANUFACTURER			0x200305
+#define HID_USAGE_SENSOR_PROP_MODEL				0x200306
 #define HID_USAGE_SENSOR_PROP_REPORT_INTERVAL			0x20030E
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS			0x20030F
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_RANGE_PCT		0x200310
--
2.38.1

[PATCH 2/3] IIO: hid-sensor-als: Use generic usage

[Philipp Jungkamp]

Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_ALS to
allow this driver to drive the Lenvo 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>
---
 drivers/hid/hid-sensor-custom.c    | 13 +++++++++++++
 drivers/iio/light/hid-sensor-als.c | 24 +++++++++++++++++-------
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index cb21f9178830..124493b8abaf 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -782,6 +782,19 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
 		.luid = "020B000000000000",
 		.manufacturer = "INTEL",
 	},
+	/*
+	 * Lenovo Intelligent Sensing Solution (LISS)
+	 */
+	{	/* ambient light */
+		.tag = "LISS",
+		.luid = "0041010200000082",
+		.model = "STK3X3X Sensor",
+		.manufacturer = "Vendor 258",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
 	{}
 };

diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
index 5a1a625d8d16..0ada158582a9 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,8 +111,9 @@ 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,
+					hsdev,
+					hsdev->usage,
+					address,
 					report_id,
 					SENSOR_HUB_SYNC,
 					min < 0);
@@ -260,7 +262,7 @@ static int als_parse_report(struct platform_device *pdev,
 			st->als_illum.report_id);

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

@@ -285,7 +287,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 +306,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,7 +337,8 @@ 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,
+	ret = sensor_hub_register_callback(hsdev,
+					hsdev->usage,
 					&als_state->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
@@ -356,7 +361,8 @@ 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 +374,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

Re: [PATCH 2/3] IIO: hid-sensor-als: Use generic usage

[Jonathan Cameron]

On Thu, 17 Nov 2022 00:19:46 +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 Lenvo custom ambient light sensor,

Lenovo

> 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>

It's a small patch, but in the ideal world, first patch should be a noop
patch that makes the hid-sensor-als stuff able to use a provided
usage ID.  Second patch then adds the new one.

> ---
>  drivers/hid/hid-sensor-custom.c    | 13 +++++++++++++
>  drivers/iio/light/hid-sensor-als.c | 24 +++++++++++++++++-------
>  2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index cb21f9178830..124493b8abaf 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -782,6 +782,19 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
>  		.luid = "020B000000000000",
>  		.manufacturer = "INTEL",
>  	},
> +	/*
> +	 * Lenovo Intelligent Sensing Solution (LISS)
> +	 */
> +	{	/* ambient light */
> +		.tag = "LISS",
> +		.luid = "0041010200000082",
> +		.model = "STK3X3X Sensor",
> +		.manufacturer = "Vendor 258",

They should definitely rebrand their laptops as this :)


> +		.check_dmi = true,
> +		.dmi.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		}
> +	},
>  	{}
>  };
> 
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 5a1a625d8d16..0ada158582a9 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,8 +111,9 @@ 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,
> +					hsdev,
> +					hsdev->usage,
> +					address,
>  					report_id,
>  					SENSOR_HUB_SYNC,
>  					min < 0);
> @@ -260,7 +262,7 @@ static int als_parse_report(struct platform_device *pdev,
>  			st->als_illum.report_id);
> 
>  	st->scale_precision = hid_sensor_format_scale(
> -				HID_USAGE_SENSOR_ALS,
> +				usage_id,
>  				&st->als_illum,
>  				&st->scale_pre_decml, &st->scale_post_decml);
> 
> @@ -285,7 +287,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 +306,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,7 +337,8 @@ 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,
> +	ret = sensor_hub_register_callback(hsdev,
> +					hsdev->usage,
>  					&als_state->callbacks);

Where possible, put multiple params on a line up to the 80 chars limit.
If it improves readability for some reason to go a bit beyond 80 chars,
that is fine too.

>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
> @@ -356,7 +361,8 @@ 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);

Oddly short line wrap.

>  	iio_device_unregister(indio_dev);
>  	hid_sensor_remove_trigger(indio_dev, &als_state->common_attributes);
> 
> @@ -368,6 +374,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
> 

[PATCH 3/3] IIO: hid-sensor-prox: Use generic usage

[Philipp Jungkamp]

Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
allow this driver to drive the Lenvo custom proximity sensor, which is
registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.

Check for raw_len to accomodate the ReportSize(8) field used on the
Lenovo Yoga 9 14IAP7.

Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
to the platform device ids.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
 drivers/hid/hid-sensor-custom.c     | 10 ++++++++
 drivers/iio/light/hid-sensor-prox.c | 39 +++++++++++++++++++++--------
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 124493b8abaf..ec49dc80f074 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -795,6 +795,16 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
 			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
 		}
 	},
+	{	/* human presence */
+		.tag = "LISS",
+		.luid = "0226000171AC0081",
+		.model = "VL53L1_HOD Sensor",
+		.manufacturer = "ST_MICRO",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
 	{}
 };

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index f10fa2abfe72..cca6b75c44d5 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			      long mask)
 {
 	struct prox_state *prox_state = iio_priv(indio_dev);
+	struct hid_sensor_hub_device *hsdev;
 	int report_id = -1;
 	u32 address;
 	int ret_type;
@@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			report_id = prox_state->prox_attr.report_id;
 			min = prox_state->prox_attr.logical_minimum;
 			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
+			hsdev = prox_state->common_attributes.hsdev;
 			break;
 		default:
 			report_id = -1;
@@ -84,8 +86,9 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			hid_sensor_power_state(&prox_state->common_attributes,
 						true);
 			*val = sensor_hub_input_attr_get_raw_value(
-				prox_state->common_attributes.hsdev,
-				HID_USAGE_SENSOR_PROX, address,
+				hsdev,
+				hsdev->usage,
+				address,
 				report_id,
 				SENSOR_HUB_SYNC,
 				min < 0);
@@ -191,10 +194,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,

 	switch (usage_id) {
 	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
-		prox_state->human_presence = *(u32 *)raw_data;
-		ret = 0;
-		break;
-	default:
+		switch (raw_len) {
+		case 1:
+			prox_state->human_presence = *(u8 *)raw_data;
+			ret = 0;
+			break;
+		case 4:
+			prox_state->human_presence = *(u32 *)raw_data;
+			ret = 0;
+			break;
+		}
 		break;
 	}

@@ -244,7 +253,8 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->common_attributes.hsdev = hsdev;
 	prox_state->common_attributes.pdev = pdev;

-	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
+	ret = hid_sensor_parse_common_attributes(hsdev,
+					hsdev->usage,
 					&prox_state->common_attributes,
 					prox_sensitivity_addresses,
 					ARRAY_SIZE(prox_sensitivity_addresses));
@@ -262,7 +272,8 @@ static int hid_prox_probe(struct platform_device *pdev)

 	ret = prox_parse_report(pdev, hsdev,
 				(struct iio_chan_spec *)indio_dev->channels,
-				HID_USAGE_SENSOR_PROX, prox_state);
+				hsdev->usage,
+				prox_state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to setup attributes\n");
 		return ret;
@@ -291,8 +302,9 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->callbacks.send_event = prox_proc_event;
 	prox_state->callbacks.capture_sample = prox_capture_sample;
 	prox_state->callbacks.pdev = pdev;
-	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
-					&prox_state->callbacks);
+	ret = sensor_hub_register_callback(hsdev,
+					   hsdev->usage,
+					   &prox_state->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
 		goto error_iio_unreg;
@@ -314,7 +326,8 @@ static int hid_prox_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct prox_state *prox_state = iio_priv(indio_dev);

-	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
+	sensor_hub_remove_callback(hsdev,
+				   hsdev->usage);
 	iio_device_unregister(indio_dev);
 	hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);

@@ -326,6 +339,10 @@ static const struct platform_device_id hid_prox_ids[] = {
 		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
 		.name = "HID-SENSOR-200011",
 	},
+	{
+		/* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
+		.name = "HID-SENSOR-LISS-0226",
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, hid_prox_ids);
--
2.38.1

Re: [PATCH 3/3] IIO: hid-sensor-prox: Use generic usage

[Jonathan Cameron]

On Thu, 17 Nov 2022 00:19:47 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
> allow this driver to drive the Lenvo custom proximity sensor, which is
> registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.
> 
> Check for raw_len to accomodate the ReportSize(8) field used on the
> Lenovo Yoga 9 14IAP7.
> 
> Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
> to the platform device ids.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
As with previous - prefer this split into a noop patch then one that
adds the new use case.

It's small enough I'm not that bothered though if you'd rather not.

A few comments inline.

Thanks,

Jonathan

> ---
>  drivers/hid/hid-sensor-custom.c     | 10 ++++++++
>  drivers/iio/light/hid-sensor-prox.c | 39 +++++++++++++++++++++--------
>  2 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 124493b8abaf..ec49dc80f074 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -795,6 +795,16 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>  		}
>  	},
> +	{	/* human presence */
> +		.tag = "LISS",
> +		.luid = "0226000171AC0081",
> +		.model = "VL53L1_HOD Sensor",
> +		.manufacturer = "ST_MICRO",
> +		.check_dmi = true,
> +		.dmi.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +		}
> +	},
>  	{}
>  };
> 
> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index f10fa2abfe72..cca6b75c44d5 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			      long mask)
>  {
>  	struct prox_state *prox_state = iio_priv(indio_dev);
> +	struct hid_sensor_hub_device *hsdev;
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> @@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			report_id = prox_state->prox_attr.report_id;
>  			min = prox_state->prox_attr.logical_minimum;
>  			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
> +			hsdev = prox_state->common_attributes.hsdev;
>  			break;
>  		default:
>  			report_id = -1;
> @@ -84,8 +86,9 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			hid_sensor_power_state(&prox_state->common_attributes,
>  						true);
>  			*val = sensor_hub_input_attr_get_raw_value(
> -				prox_state->common_attributes.hsdev,
> -				HID_USAGE_SENSOR_PROX, address,
> +				hsdev,
> +				hsdev->usage,
> +				address,
Wrapping is already odd here but this patch can avoid making it worse
by trying to get multiple params on one line < 80 chars.

>  				report_id,
>  				SENSOR_HUB_SYNC,
>  				min < 0);
> @@ -191,10 +194,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,
> 
>  	switch (usage_id) {
>  	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
> -		prox_state->human_presence = *(u32 *)raw_data;
> -		ret = 0;
> -		break;
> -	default:
> +		switch (raw_len) {
> +		case 1:
> +			prox_state->human_presence = *(u8 *)raw_data;
> +			ret = 0;

No obvious reason not to use direct returns here as they will be slightly
more readable as well as shortening the code a little.

			return 0;

> +			break;
> +		case 4:
> +			prox_state->human_presence = *(u32 *)raw_data;
> +			ret = 0;
> +			break;

Likely to generate warnings if a compiler can't see far enough to tell this
only ever gets 1 or 4 in raw_len.

Avoid that by adding a default.

> +		}
>  		break;
>  	}
> 
> @@ -244,7 +253,8 @@ static int hid_prox_probe(struct platform_device *pdev)
>  	prox_state->common_attributes.hsdev = hsdev;
>  	prox_state->common_attributes.pdev = pdev;
> 
> -	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
> +	ret = hid_sensor_parse_common_attributes(hsdev,
> +					hsdev->usage,
>  					&prox_state->common_attributes,
>  					prox_sensitivity_addresses,
>  					ARRAY_SIZE(prox_sensitivity_addresses));
> @@ -262,7 +272,8 @@ static int hid_prox_probe(struct platform_device *pdev)
> 
>  	ret = prox_parse_report(pdev, hsdev,
>  				(struct iio_chan_spec *)indio_dev->channels,
> -				HID_USAGE_SENSOR_PROX, prox_state);
> +				hsdev->usage,
> +				prox_state);
As below.

>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to setup attributes\n");
>  		return ret;
> @@ -291,8 +302,9 @@ static int hid_prox_probe(struct platform_device *pdev)
>  	prox_state->callbacks.send_event = prox_proc_event;
>  	prox_state->callbacks.capture_sample = prox_capture_sample;
>  	prox_state->callbacks.pdev = pdev;
> -	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
> -					&prox_state->callbacks);
> +	ret = sensor_hub_register_callback(hsdev,
> +					   hsdev->usage,
> +					   &prox_state->callbacks);
As below.

>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
>  		goto error_iio_unreg;
> @@ -314,7 +326,8 @@ static int hid_prox_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct prox_state *prox_state = iio_priv(indio_dev);
> 
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
> +	sensor_hub_remove_callback(hsdev,
> +				   hsdev->usage);

Similar to previous patch.  Line wrap is too short.

>  	iio_device_unregister(indio_dev);
>  	hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);
> 
> @@ -326,6 +339,10 @@ static const struct platform_device_id hid_prox_ids[] = {
>  		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>  		.name = "HID-SENSOR-200011",
>  	},
> +	{
> +		/* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
> +		.name = "HID-SENSOR-LISS-0226",
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, hid_prox_ids);
> --
> 2.38.1
> 

Re: [PATCH 1/3] HID: hid-sensor-custom: More custom iio sensors

[Jonathan Cameron]

On Thu, 17 Nov 2022 00:19:45 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> The known LUID table for established/known custom HID sensors was
> limited to sensors with "INTEL" as manufacturer. But some vendors such
> as Lenovo also include fairly standard iio sensors (e.g. ambient light)
> in their custom sensors.
> 
> Expand the known custom sensors table by a tag used for the platform
> device name and match sensors based on the LUID as well as optionally
> on model and manufacturer properties.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>

Hi Philipp,

A few trivial things inline. This looks good to me in general.
I dread to think how many of these we will eventually end up with
but such is life...

Jonathan

> ---
>  drivers/hid/hid-sensor-custom.c | 225 +++++++++++++++++++++++---------
>  include/linux/hid-sensor-ids.h  |   1 +
>  2 files changed, 161 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
> index 32c2306e240d..cb21f9178830 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -5,6 +5,7 @@
>   */
> 
>  #include <linux/ctype.h>
> +#include <linux/dmi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -750,114 +751,208 @@ static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom
> 
>  }
> 
> -/* luid defined in FW (e.g. ISH).  Maybe used to identify sensor. */
> -static const char *const known_sensor_luid[] = { "020B000000000000" };
> +/*
> + * Match a known custom sensor.
> + * tag and luid is mandatory.
> + */
> +struct hid_sensor_custom_match {
> +	const char *tag;
> +	const char *luid;
> +	const char *model;
> +	const char *manufacturer;
> +	bool check_dmi;
> +	struct dmi_system_id dmi;
> +};
> 
> -static int get_luid_table_index(unsigned char *usage_str)
> -{
> -	int i;
> +/*
> + * Custom sensor properties used for matching.
> + */
> +struct hid_sensor_custom_properties {
> +	u16 serial_num[HID_CUSTOM_MAX_FEATURE_BYTES];
> +	u16 model[HID_CUSTOM_MAX_FEATURE_BYTES];
> +	u16 manufacturer[HID_CUSTOM_MAX_FEATURE_BYTES];
> +};
> 
> -	for (i = 0; i < ARRAY_SIZE(known_sensor_luid); i++) {
> -		if (!strncmp(usage_str, known_sensor_luid[i],
> -			     strlen(known_sensor_luid[i])))
> -			return i;
> +static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
> +	/*
> +	 * Intel Integrated Sensor Hub (ISH)
> +	 */
> +	{	/* Intel ISH hinge */
> +		.tag = "INT",
> +		.luid = "020B000000000000",
> +		.manufacturer = "INTEL",
> +	},
> +	{}
> +};
> +
> +static int hid_sensor_custom_prop_match_str(const u16 *prop, const char *match,
> +					    size_t max_count)
> +{
> +	while (max_count-- && *prop && *match) {
> +		if (*prop & 0xFF00 ||
> +		    *match != (char) *prop)
> +			return 0;
> +		prop++;
> +		match++;
>  	}
> 
> -	return -ENODEV;
> +	return 1;

As you are handling it like a bool, perhaps just use a bool for the return type.

>  }
> 
> -static int get_known_custom_sensor_index(struct hid_sensor_hub_device *hsdev)
> +static int hid_sensor_custom_get_prop(struct hid_sensor_hub_device *hsdev,
> +				      u32 prop_usage_id, size_t prop_size,
> +				      u16 *prop)
>  {
> -	struct hid_sensor_hub_attribute_info sensor_manufacturer = { 0 };
> -	struct hid_sensor_hub_attribute_info sensor_luid_info = { 0 };
> +	struct hid_sensor_hub_attribute_info prop_attr = { 0 };
>  	int report_size;
>  	int ret;
> -	static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -	static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -	int i;
> 
> -	memset(w_buf, 0, sizeof(w_buf));
> -	memset(buf, 0, sizeof(buf));
> +	memset(prop, 0, prop_size);
> 
> -	/* get manufacturer info */
> +	/* get property info */
>  	ret = sensor_hub_input_get_attribute_info(hsdev,
> -			HID_FEATURE_REPORT, hsdev->usage,
> -			HID_USAGE_SENSOR_PROP_MANUFACTURER, &sensor_manufacturer);
> +						  HID_FEATURE_REPORT,
> +						  hsdev->usage,
> +						  prop_usage_id,
> +						  &prop_attr);
> +	/* property does not exist */
>  	if (ret < 0)
> -		return ret;
> +		return -ENODATA;

Hmm. Doing this to indicate a reason for this function returning is
prone to changes in the errors returned by sensor_hub_input_get_attribute_info()
in the future.  Perhaps use an extra parameter to convey this info instead.
Unless we are sure that ENODATA will always mean the same thing.

Whilst we are here I note that sensor_hub_input_get_attribute_info()
returns 0 or -1 so isn't returning a kernel error code anyway..
So this will return -EPERM right now which is probably not what is desired.

We should fix that independent of this set.

> 
>  	report_size =
> -		sensor_hub_get_feature(hsdev, sensor_manufacturer.report_id,
> -				       sensor_manufacturer.index, sizeof(w_buf),
> -				       w_buf);
> +		sensor_hub_get_feature(hsdev, prop_attr.report_id,
> +				       prop_attr.index, prop_size,
> +				       prop);
>  	if (report_size <= 0) {
>  		hid_err(hsdev->hdev,
> -			"Failed to get sensor manufacturer info %d\n",
> +			"Failed to get sensor property %08x %d\n",
> +			prop_usage_id,
>  			report_size);
>  		return -ENODEV;
>  	}
> 
> -	/* convert from wide char to char */
> -	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
> -		buf[i] = (char)w_buf[i];
> +	return 0;
> +}
> 
> -	/* ensure it's ISH sensor */
> -	if (strncmp(buf, "INTEL", strlen("INTEL")))
> -		return -ENODEV;
> +static int
> +hid_sensor_custom_do_match(struct hid_sensor_hub_device *hsdev,
> +			   const struct hid_sensor_custom_match *match,
> +			   const struct hid_sensor_custom_properties *prop)
> +{
> +	struct dmi_system_id dmi[] = { match->dmi, { 0 } };
> 
> -	memset(w_buf, 0, sizeof(w_buf));
> -	memset(buf, 0, sizeof(buf));
> +	/*
> +	 * Match the LUID property.
> +	 */

Comment seems unnecessary given the code makes this clear. So in interests
of not having comments that might get out of sync I would drop it
and any similar ones as not adding sufficient value.

> +	if (!hid_sensor_custom_prop_match_str(prop->serial_num, "LUID:", 5) ||
> +	    !hid_sensor_custom_prop_match_str(prop->serial_num + 5,
> +					      match->luid,
> +					      HID_CUSTOM_MAX_FEATURE_BYTES - 5))
> +		return 0;
> 
> -	/* get real usage id */
> -	ret = sensor_hub_input_get_attribute_info(hsdev,
> -			HID_FEATURE_REPORT, hsdev->usage,
> -			HID_USAGE_SENSOR_PROP_SERIAL_NUM, &sensor_luid_info);
> +	/*
> +	 * Match the model property. (optional)
> +	 */
> +	if (match->model &&
> +	    !hid_sensor_custom_prop_match_str(prop->model,
> +					      match->model,
> +					      HID_CUSTOM_MAX_FEATURE_BYTES))
> +		return 0;
> +
> +	/*
> +	 * Match the manufacturer property. (optional)
> +	 */
> +	if (match->manufacturer &&
> +	    !hid_sensor_custom_prop_match_str(prop->manufacturer,
> +					      match->manufacturer,
> +					      HID_CUSTOM_MAX_FEATURE_BYTES))
> +		return 0;
> +
> +	/*
> +	 * Match DMI. (optional)
> +	 */
> +	if (match->check_dmi && !dmi_check_system(dmi))
> +		return 0;
> +
> +	return 1;
> +}
> +
> +static int
> +hid_sensor_custom_properties_get(struct hid_sensor_hub_device *hsdev,
> +				 struct hid_sensor_custom_properties *prop)
> +{
> +	int ret;
> +
> +	ret = hid_sensor_custom_get_prop(hsdev,
> +					 HID_USAGE_SENSOR_PROP_SERIAL_NUM,
> +					 HID_CUSTOM_MAX_FEATURE_BYTES,
> +					 prop->serial_num);
>  	if (ret < 0)
>  		return ret;
> 
> -	report_size = sensor_hub_get_feature(hsdev, sensor_luid_info.report_id,
> -					     sensor_luid_info.index, sizeof(w_buf),
> -					     w_buf);
> -	if (report_size <= 0) {
> -		hid_err(hsdev->hdev, "Failed to get real usage info %d\n",
> -			report_size);
> -		return -ENODEV;
> -	}
> +	ret = hid_sensor_custom_get_prop(hsdev,
> +					 HID_USAGE_SENSOR_PROP_MODEL,
> +					 HID_CUSTOM_MAX_FEATURE_BYTES,
> +					 prop->model);
> +	if (ret < 0 && ret != -ENODATA)
> +		return ret;
> 
> -	/* convert from wide char to char */
> -	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
> -		buf[i] = (char)w_buf[i];
> +	ret = hid_sensor_custom_get_prop(hsdev,
> +					 HID_USAGE_SENSOR_PROP_MANUFACTURER,
> +					 HID_CUSTOM_MAX_FEATURE_BYTES,
> +					 prop->manufacturer);
> +	if (ret < 0 && ret != -ENODATA)
> +		return ret;
> 
> -	if (strlen(buf) != strlen(known_sensor_luid[0]) + 5) {
> -		hid_err(hsdev->hdev,
> -			"%s luid length not match %zu != (%zu + 5)\n", __func__,
> -			strlen(buf), strlen(known_sensor_luid[0]));
> +	return 0;
> +}
> +

I haven't checked local style, but in general one blank line is enough
between functions...

> +
> +static int
> +hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
> +			    const struct hid_sensor_custom_match **known)
> +{
> +	int ret;
> +	const struct hid_sensor_custom_match *match =
> +		hid_sensor_custom_known_table;
> +	struct hid_sensor_custom_properties prop;
> +
> +	ret = hid_sensor_custom_properties_get(hsdev, &prop);
> +	if (ret < 0)
>  		return -ENODEV;
> +
> +	while (match->tag) {
> +		if (hid_sensor_custom_do_match(hsdev, match, &prop)) {
> +			*known = match;
> +			return 0;
> +		}
> +		match++;
>  	}
> 
> -	/* get table index with luid (not matching 'LUID: ' in luid) */
> -	return get_luid_table_index(&buf[5]);
> +	return -ENODEV;
>  }
> 
>  static struct platform_device *
>  hid_sensor_register_platform_device(struct platform_device *pdev,
>  				    struct hid_sensor_hub_device *hsdev,
> -				    int index)
> +				    const struct hid_sensor_custom_match *match)
>  {
> -	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> +	char real_usage[HID_SENSOR_USAGE_LENGTH];
>  	struct platform_device *custom_pdev;
>  	const char *dev_name;
>  	char *c;
> 
>  	/* copy real usage id */
> -	memcpy(real_usage, known_sensor_luid[index], 4);
> +	memcpy(real_usage, match->luid, 4);
> +	real_usage[4] = '\0';
> 
> -	/* usage id are all lowcase */
> +	/* usage ids are all lowcase */
>  	for (c = real_usage; *c != '\0'; c++)
>  		*c = tolower(*c);
> 
> -	/* HID-SENSOR-INT-REAL_USAGE_ID */
> -	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
> +	/* HID-SENSOR-tag-real_usage */
> +	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> +			     match->tag, real_usage);
>  	if (!dev_name)
>  		return ERR_PTR(-ENOMEM);
> 
> @@ -873,7 +968,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	struct hid_sensor_custom *sensor_inst;
>  	struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>  	int ret;
> -	int index;
> +	const struct hid_sensor_custom_match *match = NULL;
> 
>  	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
>  				   GFP_KERNEL);
> @@ -888,10 +983,10 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	mutex_init(&sensor_inst->mutex);
>  	platform_set_drvdata(pdev, sensor_inst);
> 
> -	index = get_known_custom_sensor_index(hsdev);
> -	if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
> +	ret = hid_sensor_custom_get_known(hsdev, &match);
> +	if (!ret && match) {
>  		sensor_inst->custom_pdev =
> -			hid_sensor_register_platform_device(pdev, hsdev, index);
> +			hid_sensor_register_platform_device(pdev, hsdev, match);
> 
>  		ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
>  		if (ret) {
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index ac631159403a..13b1e65fbdcc 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -132,6 +132,7 @@
>  #define HID_USAGE_SENSOR_PROP_FRIENDLY_NAME			0x200301
>  #define HID_USAGE_SENSOR_PROP_SERIAL_NUM			0x200307
>  #define HID_USAGE_SENSOR_PROP_MANUFACTURER			0x200305
> +#define HID_USAGE_SENSOR_PROP_MODEL				0x200306
>  #define HID_USAGE_SENSOR_PROP_REPORT_INTERVAL			0x20030E
>  #define HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS			0x20030F
>  #define HID_USAGE_SENSOR_PROP_SENSITIVITY_RANGE_PCT		0x200310
> --
> 2.38.1
> 

[PATCH v2 1/4] HID: hid-sensor-custom: Allow more custom iio sensors

[Philipp Jungkamp]

The known LUID table for established/known custom HID sensors was
limited to sensors with "INTEL" as manufacturer. But some vendors such
as Lenovo also include fairly standard iio sensors (e.g. ambient light)
in their custom sensors.

Expand the known custom sensors table by a tag used for the platform
device name and match sensors based on the LUID as well as optionally
on model and manufacturer properties.
Introduce sensors from Lenovo's "Intelligent Sensing Solution" on the
Lenovo Yoga 9 14IAP7 as an example.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Hello,

here's a short summary of changes compared to v1:

 Added an additional commit which adds the LISS entries to the known
 custom sensors.

 Removed all unnecessary linebreaks from modified function calls and stayed
 below the 80 columns.

 Revisited the matching logic and removed the explicit error codes
 in favor of bools and forwarding the inner error.

 I noticed that the strange ENODATA return code was already redundant with
 the **known out parameter. So I removed it.

Regards,
Philipp Jungkamp

 drivers/hid/hid-sensor-custom.c | 211 +++++++++++++++++++++-----------
 include/linux/hid-sensor-ids.h  |   1 +
 2 files changed, 142 insertions(+), 70 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 32c2306e240d..89e56913c92e 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -5,6 +5,7 @@
  */

 #include <linux/ctype.h>
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -750,114 +751,184 @@ static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom

 }

-/* luid defined in FW (e.g. ISH).  Maybe used to identify sensor. */
-static const char *const known_sensor_luid[] = { "020B000000000000" };
+/*
+ * Match a known custom sensor.
+ * tag and luid is mandatory.
+ */
+struct hid_sensor_custom_match {
+	const char *tag;
+	const char *luid;
+	const char *model;
+	const char *manufacturer;
+	bool check_dmi;
+	struct dmi_system_id dmi;
+};

-static int get_luid_table_index(unsigned char *usage_str)
-{
-	int i;
+/*
+ * Custom sensor properties used for matching.
+ */
+struct hid_sensor_custom_properties {
+	u16 serial_num[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 model[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 manufacturer[HID_CUSTOM_MAX_FEATURE_BYTES];
+};
+
+static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
+	/*
+	 * Intel Integrated Sensor Hub (ISH)
+	 */
+	{	/* Intel ISH hinge */
+		.tag = "INT",
+		.luid = "020B000000000000",
+		.manufacturer = "INTEL",
+	},
+	{}
+};

-	for (i = 0; i < ARRAY_SIZE(known_sensor_luid); i++) {
-		if (!strncmp(usage_str, known_sensor_luid[i],
-			     strlen(known_sensor_luid[i])))
-			return i;
+static bool hid_sensor_custom_prop_match_str(const u16 *prop, const char *match,
+					     size_t count)
+{
+	while (count-- && *prop && *match) {
+		if (*prop & 0xFF00 ||
+		    *match != (char) *prop)
+			return false;
+		prop++;
+		match++;
 	}

-	return -ENODEV;
+	return (count == -1) || *prop == (u16)*match;
 }

-static int get_known_custom_sensor_index(struct hid_sensor_hub_device *hsdev)
+static bool hid_sensor_custom_get_prop(struct hid_sensor_hub_device *hsdev,
+				      u32 prop_usage_id, size_t prop_size,
+				      u16 *prop)
 {
-	struct hid_sensor_hub_attribute_info sensor_manufacturer = { 0 };
-	struct hid_sensor_hub_attribute_info sensor_luid_info = { 0 };
+	struct hid_sensor_hub_attribute_info prop_attr = { 0 };
 	int report_size;
 	int ret;
-	static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	int i;

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	memset(prop, 0, prop_size);

-	/* get manufacturer info */
-	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_MANUFACTURER, &sensor_manufacturer);
+	ret = sensor_hub_input_get_attribute_info(hsdev, HID_FEATURE_REPORT,
+						  hsdev->usage, prop_usage_id,
+						  &prop_attr);
 	if (ret < 0)
-		return ret;
+		return 0;

-	report_size =
-		sensor_hub_get_feature(hsdev, sensor_manufacturer.report_id,
-				       sensor_manufacturer.index, sizeof(w_buf),
-				       w_buf);
+	report_size = sensor_hub_get_feature(hsdev, prop_attr.report_id,
+					     prop_attr.index, prop_size, prop);
 	if (report_size <= 0) {
 		hid_err(hsdev->hdev,
-			"Failed to get sensor manufacturer info %d\n",
+			"Failed to get sensor property %08x %d\n",
+			prop_usage_id,
 			report_size);
-		return -ENODEV;
+		return report_size;
 	}

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	return 0;
+}
+
+static bool
+hid_sensor_custom_do_match(struct hid_sensor_hub_device *hsdev,
+			   const struct hid_sensor_custom_match *match,
+			   const struct hid_sensor_custom_properties *prop)
+{
+	struct dmi_system_id dmi[] = { match->dmi, { 0 } };
+
+	if (!hid_sensor_custom_prop_match_str(prop->serial_num, "LUID:", 5) ||
+	    !hid_sensor_custom_prop_match_str(prop->serial_num + 5, match->luid,
+					      HID_CUSTOM_MAX_FEATURE_BYTES - 5))
+		return false;
+
+	if (match->model &&
+	    !hid_sensor_custom_prop_match_str(prop->model, match->model,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return false;

-	/* ensure it's ISH sensor */
-	if (strncmp(buf, "INTEL", strlen("INTEL")))
-		return -ENODEV;
+	if (match->manufacturer &&
+	    !hid_sensor_custom_prop_match_str(prop->manufacturer, match->manufacturer,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return false;

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	if (match->check_dmi && !dmi_check_system(dmi))
+		return false;

-	/* get real usage id */
-	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_SERIAL_NUM, &sensor_luid_info);
+	return true;
+}
+
+static int
+hid_sensor_custom_properties_get(struct hid_sensor_hub_device *hsdev,
+				 struct hid_sensor_custom_properties *prop)
+{
+	int ret;
+
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_SERIAL_NUM,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->serial_num);
 	if (ret < 0)
 		return ret;

-	report_size = sensor_hub_get_feature(hsdev, sensor_luid_info.report_id,
-					     sensor_luid_info.index, sizeof(w_buf),
-					     w_buf);
-	if (report_size <= 0) {
-		hid_err(hsdev->hdev, "Failed to get real usage info %d\n",
-			report_size);
-		return -ENODEV;
-	}
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MODEL,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->model);
+	if (ret < 0)
+		return ret;

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MANUFACTURER,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->manufacturer);
+	if (ret < 0)
+		return ret;

-	if (strlen(buf) != strlen(known_sensor_luid[0]) + 5) {
-		hid_err(hsdev->hdev,
-			"%s luid length not match %zu != (%zu + 5)\n", __func__,
-			strlen(buf), strlen(known_sensor_luid[0]));
-		return -ENODEV;
+	return 0;
+}
+
+static int
+hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
+			    const struct hid_sensor_custom_match **known)
+{
+	int ret;
+	const struct hid_sensor_custom_match *match =
+		hid_sensor_custom_known_table;
+	struct hid_sensor_custom_properties prop;
+
+	ret = hid_sensor_custom_properties_get(hsdev, &prop);
+	if (ret < 0)
+		return ret;
+
+	while (match->tag) {
+		if (hid_sensor_custom_do_match(hsdev, match, &prop)) {
+			*known = match;
+			return 0;
+		}
+		match++;
 	}

-	/* get table index with luid (not matching 'LUID: ' in luid) */
-	return get_luid_table_index(&buf[5]);
+	*known = NULL;
+	return 0;
 }

 static struct platform_device *
 hid_sensor_register_platform_device(struct platform_device *pdev,
 				    struct hid_sensor_hub_device *hsdev,
-				    int index)
+				    const struct hid_sensor_custom_match *match)
 {
-	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
+	char real_usage[HID_SENSOR_USAGE_LENGTH];
 	struct platform_device *custom_pdev;
 	const char *dev_name;
 	char *c;

-	/* copy real usage id */
-	memcpy(real_usage, known_sensor_luid[index], 4);
+	memcpy(real_usage, match->luid, 4);
+	real_usage[4] = '\0';

-	/* usage id are all lowcase */
 	for (c = real_usage; *c != '\0'; c++)
 		*c = tolower(*c);

-	/* HID-SENSOR-INT-REAL_USAGE_ID */
-	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
+	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
+			     match->tag, real_usage);
 	if (!dev_name)
 		return ERR_PTR(-ENOMEM);

@@ -873,7 +944,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	struct hid_sensor_custom *sensor_inst;
 	struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
 	int ret;
-	int index;
+	const struct hid_sensor_custom_match *match = NULL;

 	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
 				   GFP_KERNEL);
@@ -888,10 +959,10 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	mutex_init(&sensor_inst->mutex);
 	platform_set_drvdata(pdev, sensor_inst);

-	index = get_known_custom_sensor_index(hsdev);
-	if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
+	ret = hid_sensor_custom_get_known(hsdev, &match);
+	if (!ret && match) {
 		sensor_inst->custom_pdev =
-			hid_sensor_register_platform_device(pdev, hsdev, index);
+			hid_sensor_register_platform_device(pdev, hsdev, match);

 		ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
 		if (ret) {
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index ac631159403a..13b1e65fbdcc 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -132,6 +132,7 @@
 #define HID_USAGE_SENSOR_PROP_FRIENDLY_NAME			0x200301
 #define HID_USAGE_SENSOR_PROP_SERIAL_NUM			0x200307
 #define HID_USAGE_SENSOR_PROP_MANUFACTURER			0x200305
+#define HID_USAGE_SENSOR_PROP_MODEL				0x200306
 #define HID_USAGE_SENSOR_PROP_REPORT_INTERVAL			0x20030E
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS			0x20030F
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_RANGE_PCT		0x200310
--
2.38.1

[PATCH v2 2/4] HID: hid-sensor-custom: Add LISS custom sensors

[Philipp Jungkamp]

Add the Lenovo Intelligent Sensing Solution (LISS) custom sensors to the
known custom sensors.
---
Here is the requested noop commit that changes the device ids for the known
LISS sensors.

 drivers/hid/hid-sensor-custom.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 89e56913c92e..85db14406b23 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -782,6 +782,29 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
 		.luid = "020B000000000000",
 		.manufacturer = "INTEL",
 	},
+	/*
+	 * Lenovo Intelligent Sensing Solution (LISS)
+	 */
+	{	/* ambient light */
+		.tag = "LISS",
+		.luid = "0041010200000082",
+		.model = "STK3X3X Sensor",
+		.manufacturer = "Vendor 258",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
+	{	/* human presence */
+		.tag = "LISS",
+		.luid = "0226000171AC0081",
+		.model = "VL53L1_HOD Sensor",
+		.manufacturer = "ST_MICRO",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
 	{}
 };

--
2.38.1

Re: [PATCH v2 2/4] HID: hid-sensor-custom: Add LISS custom sensors

[srinivas pandruvada]

On Fri, 2022-11-18 at 00:43 +0100, Philipp Jungkamp wrote:
> Add the Lenovo Intelligent Sensing Solution (LISS) custom sensors to
> the
> known custom sensors.
> ---
Generally you need write a version history:

v2
..

if no change just write
- No change.


> Here is the requested noop commit that changes the device ids for the
> known
> LISS sensors.
> 
>  drivers/hid/hid-sensor-custom.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 89e56913c92e..85db14406b23 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -782,6 +782,29 @@ static const struct hid_sensor_custom_match
> hid_sensor_custom_known_table[] = {
>                 .luid = "020B000000000000",
>                 .manufacturer = "INTEL",
>         },
> +       /*
> +        * Lenovo Intelligent Sensing Solution (LISS)
> +        */
> +       {       /* ambient light */
> +               .tag = "LISS",
> +               .luid = "0041010200000082",
> +               .model = "STK3X3X Sensor",
> +               .manufacturer = "Vendor 258",
> +               .check_dmi = true,
> +               .dmi.matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +               }
> +       },
> +       {       /* human presence */
> +               .tag = "LISS",
> +               .luid = "0226000171AC0081",
> +               .model = "VL53L1_HOD Sensor",
> +               .manufacturer = "ST_MICRO",
> +               .check_dmi = true,
> +               .dmi.matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +               }
> +       },
>         {}
>  };
> 
> --
> 2.38.1
> 

[PATCH v2 3/4] IIO: hid-sensor-als: Use generic usage

[Philipp Jungkamp]

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(-)

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);
--
2.38.1

[PATCH v2 4/4] IIO: hid-sensor-prox: Use generic usage

[Philipp Jungkamp]

Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
allow this driver to drive the Lenvo custom proximity sensor, which is
registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.

Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
to the platform device ids.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Lines are shorter and the switch statement for raw_len is cleaner.

 drivers/iio/light/hid-sensor-prox.c | 37 ++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index f10fa2abfe72..a47591e1bad9 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			      long mask)
 {
 	struct prox_state *prox_state = iio_priv(indio_dev);
+	struct hid_sensor_hub_device *hsdev;
 	int report_id = -1;
 	u32 address;
 	int ret_type;
@@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			report_id = prox_state->prox_attr.report_id;
 			min = prox_state->prox_attr.logical_minimum;
 			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
+			hsdev = prox_state->common_attributes.hsdev;
 			break;
 		default:
 			report_id = -1;
@@ -84,11 +86,8 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			hid_sensor_power_state(&prox_state->common_attributes,
 						true);
 			*val = sensor_hub_input_attr_get_raw_value(
-				prox_state->common_attributes.hsdev,
-				HID_USAGE_SENSOR_PROX, address,
-				report_id,
-				SENSOR_HUB_SYNC,
-				min < 0);
+				hsdev, hsdev->usage, address, report_id,
+				SENSOR_HUB_SYNC, min < 0);
 			hid_sensor_power_state(&prox_state->common_attributes,
 						false);
 		} else {
@@ -191,10 +190,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,

 	switch (usage_id) {
 	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
-		prox_state->human_presence = *(u32 *)raw_data;
-		ret = 0;
-		break;
-	default:
+		switch (raw_len) {
+		case 1:
+			prox_state->human_presence = *(u8 *)raw_data;
+			return 0;
+		case 4:
+			prox_state->human_presence = *(u32 *)raw_data;
+			return 0;
+		default:
+			break;
+		}
 		break;
 	}

@@ -244,7 +249,7 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->common_attributes.hsdev = hsdev;
 	prox_state->common_attributes.pdev = pdev;

-	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
+	ret = hid_sensor_parse_common_attributes(hsdev, hsdev->usage,
 					&prox_state->common_attributes,
 					prox_sensitivity_addresses,
 					ARRAY_SIZE(prox_sensitivity_addresses));
@@ -262,7 +267,7 @@ static int hid_prox_probe(struct platform_device *pdev)

 	ret = prox_parse_report(pdev, hsdev,
 				(struct iio_chan_spec *)indio_dev->channels,
-				HID_USAGE_SENSOR_PROX, prox_state);
+				hsdev->usage, prox_state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to setup attributes\n");
 		return ret;
@@ -291,8 +296,8 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->callbacks.send_event = prox_proc_event;
 	prox_state->callbacks.capture_sample = prox_capture_sample;
 	prox_state->callbacks.pdev = pdev;
-	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
-					&prox_state->callbacks);
+	ret = sensor_hub_register_callback(hsdev, hsdev->usage,
+					   &prox_state->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
 		goto error_iio_unreg;
@@ -314,7 +319,7 @@ static int hid_prox_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct prox_state *prox_state = iio_priv(indio_dev);

-	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
+	sensor_hub_remove_callback(hsdev, hsdev->usage);
 	iio_device_unregister(indio_dev);
 	hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);

@@ -326,6 +331,10 @@ static const struct platform_device_id hid_prox_ids[] = {
 		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
 		.name = "HID-SENSOR-200011",
 	},
+	{
+		/* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
+		.name = "HID-SENSOR-LISS-0226",
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, hid_prox_ids);
--
2.38.1

Re: [PATCH v2 1/4] HID: hid-sensor-custom: Allow more custom iio sensors

[srinivas pandruvada]

On Fri, 2022-11-18 at 00:42 +0100, Philipp Jungkamp wrote:
> The known LUID table for established/known custom HID sensors was
> limited to sensors with "INTEL" as manufacturer. But some vendors
> such
> as Lenovo also include fairly standard iio sensors (e.g. ambient
> light)
> in their custom sensors.
> 
> Expand the known custom sensors table by a tag used for the platform
> device name and match sensors based on the LUID as well as optionally
> on model and manufacturer properties.
> Introduce sensors from Lenovo's "Intelligent Sensing Solution" on the
> Lenovo Yoga 9 14IAP7 as an example.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Hello,
> 
> here's a short summary of changes compared to v1:
> 
>  Added an additional commit which adds the LISS entries to the known
>  custom sensors.
> 
>  Removed all unnecessary linebreaks from modified function calls and
> stayed
>  below the 80 columns.
> 
>  Revisited the matching logic and removed the explicit error codes
>  in favor of bools and forwarding the inner error.
> 
>  I noticed that the strange ENODATA return code was already redundant
> with
>  the **known out parameter. So I removed it.
> 
> Regards,
> Philipp Jungkamp
> 
>  drivers/hid/hid-sensor-custom.c | 211 +++++++++++++++++++++---------
> --
>  include/linux/hid-sensor-ids.h  |   1 +
>  2 files changed, 142 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 32c2306e240d..89e56913c92e 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -5,6 +5,7 @@
>   */
> 
>  #include <linux/ctype.h>
> +#include <linux/dmi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -750,114 +751,184 @@ static void
> hid_sensor_custom_dev_if_remove(struct hid_sensor_custom
> 
>  }
> 
> -/* luid defined in FW (e.g. ISH).  Maybe used to identify sensor. */
> -static const char *const known_sensor_luid[] = { "020B000000000000"
> };
> +/*
> + * Match a known custom sensor.
> + * tag and luid is mandatory.
> + */
> +struct hid_sensor_custom_match {
> +       const char *tag;
> +       const char *luid;
> +       const char *model;
> +       const char *manufacturer;
> +       bool check_dmi;
> +       struct dmi_system_id dmi;
> +};
> 
> -static int get_luid_table_index(unsigned char *usage_str)
> -{
> -       int i;
> +/*
> + * Custom sensor properties used for matching.
> + */
> +struct hid_sensor_custom_properties {
> +       u16 serial_num[HID_CUSTOM_MAX_FEATURE_BYTES];
> +       u16 model[HID_CUSTOM_MAX_FEATURE_BYTES];
> +       u16 manufacturer[HID_CUSTOM_MAX_FEATURE_BYTES];
> +};
> +
> +static const struct hid_sensor_custom_match
> hid_sensor_custom_known_table[] = {
> +       /*
> +        * Intel Integrated Sensor Hub (ISH)
> +        */
> +       {       /* Intel ISH hinge */
> +               .tag = "INT",
> +               .luid = "020B000000000000",
> +               .manufacturer = "INTEL",
> +       },
> +       {}
> +};
> 
> -       for (i = 0; i < ARRAY_SIZE(known_sensor_luid); i++) {
> -               if (!strncmp(usage_str, known_sensor_luid[i],
> -                            strlen(known_sensor_luid[i])))
> -                       return i;
> +static bool hid_sensor_custom_prop_match_str(const u16 *prop, const
> char *match,
> +                                            size_t count)
> +{
> +       while (count-- && *prop && *match) {
> +               if (*prop & 0xFF00 ||
> +                   *match != (char) *prop)
> +                       return false;
> +               prop++;
> +               match++;
>         }
> 
> -       return -ENODEV;
> +       return (count == -1) || *prop == (u16)*match;
>  }
> 
> -static int get_known_custom_sensor_index(struct
> hid_sensor_hub_device *hsdev)
> +static bool hid_sensor_custom_get_prop(struct hid_sensor_hub_device
> *hsdev,
> +                                     u32 prop_usage_id, size_t
> prop_size,
> +                                     u16 *prop)
>  {
> -       struct hid_sensor_hub_attribute_info sensor_manufacturer = {
> 0 };
> -       struct hid_sensor_hub_attribute_info sensor_luid_info = { 0
> };
> +       struct hid_sensor_hub_attribute_info prop_attr = { 0 };
>         int report_size;
>         int ret;
> -       static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -       static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -       int i;
> 
> -       memset(w_buf, 0, sizeof(w_buf));
> -       memset(buf, 0, sizeof(buf));
> +       memset(prop, 0, prop_size);
> 
> -       /* get manufacturer info */
> -       ret = sensor_hub_input_get_attribute_info(hsdev,
> -                       HID_FEATURE_REPORT, hsdev->usage,
> -                       HID_USAGE_SENSOR_PROP_MANUFACTURER,
> &sensor_manufacturer);
> +       ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_FEATURE_REPORT,
> +                                                 hsdev->usage,
> prop_usage_id,
> +                                                 &prop_attr);
>         if (ret < 0)
> -               return ret;
> +               return 0;
> 
> -       report_size =
> -               sensor_hub_get_feature(hsdev,
> sensor_manufacturer.report_id,
> -                                      sensor_manufacturer.index,
> sizeof(w_buf),
> -                                      w_buf);
> +       report_size = sensor_hub_get_feature(hsdev,
> prop_attr.report_id,
> +                                            prop_attr.index,
> prop_size, prop);
>         if (report_size <= 0) {
>                 hid_err(hsdev->hdev,
> -                       "Failed to get sensor manufacturer info
> %d\n",
> +                       "Failed to get sensor property %08x %d\n",
> +                       prop_usage_id,
>                         report_size);
> -               return -ENODEV;
> +               return report_size;
>         }
> 
> -       /* convert from wide char to char */
> -       for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
> -               buf[i] = (char)w_buf[i];
> +       return 0;
> +}
> +
> +static bool
> +hid_sensor_custom_do_match(struct hid_sensor_hub_device *hsdev,
> +                          const struct hid_sensor_custom_match
> *match,
> +                          const struct hid_sensor_custom_properties
> *prop)
> +{
> +       struct dmi_system_id dmi[] = { match->dmi, { 0 } };
> +
> +       if (!hid_sensor_custom_prop_match_str(prop->serial_num,
> "LUID:", 5) ||
> +           !hid_sensor_custom_prop_match_str(prop->serial_num + 5,
> match->luid,
> +                                            
> HID_CUSTOM_MAX_FEATURE_BYTES - 5))
> +               return false;
> +
> +       if (match->model &&
> +           !hid_sensor_custom_prop_match_str(prop->model, match-
> >model,
> +                                            
> HID_CUSTOM_MAX_FEATURE_BYTES))
> +               return false;
> 
> -       /* ensure it's ISH sensor */
> -       if (strncmp(buf, "INTEL", strlen("INTEL")))
> -               return -ENODEV;
> +       if (match->manufacturer &&
> +           !hid_sensor_custom_prop_match_str(prop->manufacturer,
> match->manufacturer,
> +                                            
> HID_CUSTOM_MAX_FEATURE_BYTES))
> +               return false;
> 
> -       memset(w_buf, 0, sizeof(w_buf));
> -       memset(buf, 0, sizeof(buf));
> +       if (match->check_dmi && !dmi_check_system(dmi))
> +               return false;
> 
> -       /* get real usage id */
> -       ret = sensor_hub_input_get_attribute_info(hsdev,
> -                       HID_FEATURE_REPORT, hsdev->usage,
> -                       HID_USAGE_SENSOR_PROP_SERIAL_NUM,
> &sensor_luid_info);
> +       return true;
> +}
> +
> +static int
> +hid_sensor_custom_properties_get(struct hid_sensor_hub_device
> *hsdev,
> +                                struct hid_sensor_custom_properties
> *prop)
> +{
> +       int ret;
> +
> +       ret = hid_sensor_custom_get_prop(hsdev,
> +                                       
> HID_USAGE_SENSOR_PROP_SERIAL_NUM,
> +                                       
> HID_CUSTOM_MAX_FEATURE_BYTES,
> +                                        prop->serial_num);
>         if (ret < 0)
>                 return ret;
> 
> -       report_size = sensor_hub_get_feature(hsdev,
> sensor_luid_info.report_id,
> -                                            sensor_luid_info.index,
> sizeof(w_buf),
> -                                            w_buf);
> -       if (report_size <= 0) {
> -               hid_err(hsdev->hdev, "Failed to get real usage info
> %d\n",
> -                       report_size);
> -               return -ENODEV;
> -       }
> +       ret = hid_sensor_custom_get_prop(hsdev,
> +                                        HID_USAGE_SENSOR_PROP_MODEL,
> +                                       
> HID_CUSTOM_MAX_FEATURE_BYTES,
> +                                        prop->model);
> +       if (ret < 0)
> +               return ret;
> 
> -       /* convert from wide char to char */
> -       for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
> -               buf[i] = (char)w_buf[i];
> +       ret = hid_sensor_custom_get_prop(hsdev,
> +                                       
> HID_USAGE_SENSOR_PROP_MANUFACTURER,
> +                                       
> HID_CUSTOM_MAX_FEATURE_BYTES,
> +                                        prop->manufacturer);
> +       if (ret < 0)
> +               return ret;
> 
> -       if (strlen(buf) != strlen(known_sensor_luid[0]) + 5) {
> -               hid_err(hsdev->hdev,
> -                       "%s luid length not match %zu != (%zu +
> 5)\n", __func__,
> -                       strlen(buf), strlen(known_sensor_luid[0]));
> -               return -ENODEV;
> +       return 0;
> +}
> +
> +static int
> +hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
> +                           const struct hid_sensor_custom_match
> **known)
> +{
> +       int ret;
> +       const struct hid_sensor_custom_match *match =
> +               hid_sensor_custom_known_table;
> +       struct hid_sensor_custom_properties prop;
> +
> +       ret = hid_sensor_custom_properties_get(hsdev, &prop);
> +       if (ret < 0)
> +               return ret;
> +
> +       while (match->tag) {
> +               if (hid_sensor_custom_do_match(hsdev, match, &prop))
> {
> +                       *known = match;
> +                       return 0;
> +               }
> +               match++;
>         }
> 
> -       /* get table index with luid (not matching 'LUID: ' in luid)
> */
> -       return get_luid_table_index(&buf[5]);
> +       *known = NULL;
> +       return 0;

The function returns 0, means success. The caller needn't look at some
other param to check if there was some known sensor exists.

return -ENODATA;

 The caller will be simplified also. He doesn't have to check "match"
address.

>  }
> 
>  static struct platform_device *
>  hid_sensor_register_platform_device(struct platform_device *pdev,
>                                     struct hid_sensor_hub_device
> *hsdev,
> -                                   int index)
> +                                   const struct
> hid_sensor_custom_match *match)
>  {
> -       char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> +       char real_usage[HID_SENSOR_USAGE_LENGTH];
>         struct platform_device *custom_pdev;
>         const char *dev_name;
>         char *c;
> 
> -       /* copy real usage id */
> -       memcpy(real_usage, known_sensor_luid[index], 4);
> +       memcpy(real_usage, match->luid, 4);
> +       real_usage[4] = '\0';
You already did init whole array above. Do you need this?

> 
> -       /* usage id are all lowcase */
>         for (c = real_usage; *c != '\0'; c++)
>                 *c = tolower(*c);
> 
> -       /* HID-SENSOR-INT-REAL_USAGE_ID */
> -       dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
> real_usage);
> +       dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> +                            match->tag, real_usage);
>         if (!dev_name)
>                 return ERR_PTR(-ENOMEM);
> 
> @@ -873,7 +944,7 @@ static int hid_sensor_custom_probe(struct
> platform_device *pdev)
>         struct hid_sensor_custom *sensor_inst;
>         struct hid_sensor_hub_device *hsdev = pdev-
> >dev.platform_data;
>         int ret;
> -       int index;
> +       const struct hid_sensor_custom_match *match = NULL;
Not necessary to initialize, if you follow above advise to return error
for hid_sensor_custom_get_known()

> 
>         sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
>                                    GFP_KERNEL);
> @@ -888,10 +959,10 @@ static int hid_sensor_custom_probe(struct
> platform_device *pdev)
>         mutex_init(&sensor_inst->mutex);
>         platform_set_drvdata(pdev, sensor_inst);
> 
> -       index = get_known_custom_sensor_index(hsdev);
> -       if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
> +       ret = hid_sensor_custom_get_known(hsdev, &match);
If hid_sensor_custom_get_known() returns error you don't need to check
validity of "match" in below if.

Thanks,
Srinivas

> +       if (!ret && match) {
>                 sensor_inst->custom_pdev =
> -                       hid_sensor_register_platform_device(pdev,
> hsdev, index);
> +                       hid_sensor_register_platform_device(pdev,
> hsdev, match);
> 
>                 ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
>                 if (ret) {
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> sensor-ids.h
> index ac631159403a..13b1e65fbdcc 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -132,6 +132,7 @@
>  #define
> HID_USAGE_SENSOR_PROP_FRIENDLY_NAME                    0x200301
>  #define
> HID_USAGE_SENSOR_PROP_SERIAL_NUM                       0x200307
>  #define
> HID_USAGE_SENSOR_PROP_MANUFACTURER                     0x200305
> +#define
> HID_USAGE_SENSOR_PROP_MODEL                            0x200306
>  #define
> HID_USAGE_SENSOR_PROP_REPORT_INTERVAL                  0x20030E
>  #define
> HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS                  0x20030F
>  #define
> HID_USAGE_SENSOR_PROP_SENSITIVITY_RANGE_PCT            0x200310
> --
> 2.38.1
> 

[PATCH v3 1/4] HID: hid-sensor-custom: Allow more custom iio sensors

[Philipp Jungkamp]

The known LUID table for established/known custom HID sensors was
limited to sensors with "INTEL" as manufacturer. But some vendors such
as Lenovo also include fairly standard iio sensors (e.g. ambient light)
in their custom sensors.

Expand the known custom sensors table by a tag used for the platform
device name and match sensors based on the LUID as well as optionally
on model and manufacturer properties.
Introduce sensors from Lenovo's "Intelligent Sensing Solution" on the
Lenovo Yoga 9 14IAP7 as an example.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Hello,

here's a short summary of changes compared to v1:

 Added an additional commit which adds the LISS entries to the known
 custom sensors.

 Removed all unnecessary linebreaks from modified function calls and stayed
 below the 80 columns.

 Revisited the matching logic and removed the explicit error codes
 in favor of bools and forwarding the inner error.

 I noticed that the strange ENODATA return code was already redundant with
 the **known out parameter. So I removed it.

Changes compared to v2:
 I forgot to sign off on the new commit I introduced.
 I'm not really too comfortable with signoffs and mailing lists yet.

Regards,
Philipp Jungkamp

 drivers/hid/hid-sensor-custom.c | 211 +++++++++++++++++++++-----------
 include/linux/hid-sensor-ids.h  |   1 +
 2 files changed, 142 insertions(+), 70 deletions(-)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 32c2306e240d..89e56913c92e 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -5,6 +5,7 @@
  */

 #include <linux/ctype.h>
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -750,114 +751,184 @@ static void hid_sensor_custom_dev_if_remove(struct hid_sensor_custom

 }

-/* luid defined in FW (e.g. ISH).  Maybe used to identify sensor. */
-static const char *const known_sensor_luid[] = { "020B000000000000" };
+/*
+ * Match a known custom sensor.
+ * tag and luid is mandatory.
+ */
+struct hid_sensor_custom_match {
+	const char *tag;
+	const char *luid;
+	const char *model;
+	const char *manufacturer;
+	bool check_dmi;
+	struct dmi_system_id dmi;
+};

-static int get_luid_table_index(unsigned char *usage_str)
-{
-	int i;
+/*
+ * Custom sensor properties used for matching.
+ */
+struct hid_sensor_custom_properties {
+	u16 serial_num[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 model[HID_CUSTOM_MAX_FEATURE_BYTES];
+	u16 manufacturer[HID_CUSTOM_MAX_FEATURE_BYTES];
+};
+
+static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
+	/*
+	 * Intel Integrated Sensor Hub (ISH)
+	 */
+	{	/* Intel ISH hinge */
+		.tag = "INT",
+		.luid = "020B000000000000",
+		.manufacturer = "INTEL",
+	},
+	{}
+};

-	for (i = 0; i < ARRAY_SIZE(known_sensor_luid); i++) {
-		if (!strncmp(usage_str, known_sensor_luid[i],
-			     strlen(known_sensor_luid[i])))
-			return i;
+static bool hid_sensor_custom_prop_match_str(const u16 *prop, const char *match,
+					     size_t count)
+{
+	while (count-- && *prop && *match) {
+		if (*prop & 0xFF00 ||
+		    *match != (char) *prop)
+			return false;
+		prop++;
+		match++;
 	}

-	return -ENODEV;
+	return (count == -1) || *prop == (u16)*match;
 }

-static int get_known_custom_sensor_index(struct hid_sensor_hub_device *hsdev)
+static bool hid_sensor_custom_get_prop(struct hid_sensor_hub_device *hsdev,
+				      u32 prop_usage_id, size_t prop_size,
+				      u16 *prop)
 {
-	struct hid_sensor_hub_attribute_info sensor_manufacturer = { 0 };
-	struct hid_sensor_hub_attribute_info sensor_luid_info = { 0 };
+	struct hid_sensor_hub_attribute_info prop_attr = { 0 };
 	int report_size;
 	int ret;
-	static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
-	int i;

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	memset(prop, 0, prop_size);

-	/* get manufacturer info */
-	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_MANUFACTURER, &sensor_manufacturer);
+	ret = sensor_hub_input_get_attribute_info(hsdev, HID_FEATURE_REPORT,
+						  hsdev->usage, prop_usage_id,
+						  &prop_attr);
 	if (ret < 0)
-		return ret;
+		return 0;

-	report_size =
-		sensor_hub_get_feature(hsdev, sensor_manufacturer.report_id,
-				       sensor_manufacturer.index, sizeof(w_buf),
-				       w_buf);
+	report_size = sensor_hub_get_feature(hsdev, prop_attr.report_id,
+					     prop_attr.index, prop_size, prop);
 	if (report_size <= 0) {
 		hid_err(hsdev->hdev,
-			"Failed to get sensor manufacturer info %d\n",
+			"Failed to get sensor property %08x %d\n",
+			prop_usage_id,
 			report_size);
-		return -ENODEV;
+		return report_size;
 	}

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	return 0;
+}
+
+static bool
+hid_sensor_custom_do_match(struct hid_sensor_hub_device *hsdev,
+			   const struct hid_sensor_custom_match *match,
+			   const struct hid_sensor_custom_properties *prop)
+{
+	struct dmi_system_id dmi[] = { match->dmi, { 0 } };
+
+	if (!hid_sensor_custom_prop_match_str(prop->serial_num, "LUID:", 5) ||
+	    !hid_sensor_custom_prop_match_str(prop->serial_num + 5, match->luid,
+					      HID_CUSTOM_MAX_FEATURE_BYTES - 5))
+		return false;
+
+	if (match->model &&
+	    !hid_sensor_custom_prop_match_str(prop->model, match->model,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return false;

-	/* ensure it's ISH sensor */
-	if (strncmp(buf, "INTEL", strlen("INTEL")))
-		return -ENODEV;
+	if (match->manufacturer &&
+	    !hid_sensor_custom_prop_match_str(prop->manufacturer, match->manufacturer,
+					      HID_CUSTOM_MAX_FEATURE_BYTES))
+		return false;

-	memset(w_buf, 0, sizeof(w_buf));
-	memset(buf, 0, sizeof(buf));
+	if (match->check_dmi && !dmi_check_system(dmi))
+		return false;

-	/* get real usage id */
-	ret = sensor_hub_input_get_attribute_info(hsdev,
-			HID_FEATURE_REPORT, hsdev->usage,
-			HID_USAGE_SENSOR_PROP_SERIAL_NUM, &sensor_luid_info);
+	return true;
+}
+
+static int
+hid_sensor_custom_properties_get(struct hid_sensor_hub_device *hsdev,
+				 struct hid_sensor_custom_properties *prop)
+{
+	int ret;
+
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_SERIAL_NUM,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->serial_num);
 	if (ret < 0)
 		return ret;

-	report_size = sensor_hub_get_feature(hsdev, sensor_luid_info.report_id,
-					     sensor_luid_info.index, sizeof(w_buf),
-					     w_buf);
-	if (report_size <= 0) {
-		hid_err(hsdev->hdev, "Failed to get real usage info %d\n",
-			report_size);
-		return -ENODEV;
-	}
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MODEL,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->model);
+	if (ret < 0)
+		return ret;

-	/* convert from wide char to char */
-	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
-		buf[i] = (char)w_buf[i];
+	ret = hid_sensor_custom_get_prop(hsdev,
+					 HID_USAGE_SENSOR_PROP_MANUFACTURER,
+					 HID_CUSTOM_MAX_FEATURE_BYTES,
+					 prop->manufacturer);
+	if (ret < 0)
+		return ret;

-	if (strlen(buf) != strlen(known_sensor_luid[0]) + 5) {
-		hid_err(hsdev->hdev,
-			"%s luid length not match %zu != (%zu + 5)\n", __func__,
-			strlen(buf), strlen(known_sensor_luid[0]));
-		return -ENODEV;
+	return 0;
+}
+
+static int
+hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
+			    const struct hid_sensor_custom_match **known)
+{
+	int ret;
+	const struct hid_sensor_custom_match *match =
+		hid_sensor_custom_known_table;
+	struct hid_sensor_custom_properties prop;
+
+	ret = hid_sensor_custom_properties_get(hsdev, &prop);
+	if (ret < 0)
+		return ret;
+
+	while (match->tag) {
+		if (hid_sensor_custom_do_match(hsdev, match, &prop)) {
+			*known = match;
+			return 0;
+		}
+		match++;
 	}

-	/* get table index with luid (not matching 'LUID: ' in luid) */
-	return get_luid_table_index(&buf[5]);
+	*known = NULL;
+	return 0;
 }

 static struct platform_device *
 hid_sensor_register_platform_device(struct platform_device *pdev,
 				    struct hid_sensor_hub_device *hsdev,
-				    int index)
+				    const struct hid_sensor_custom_match *match)
 {
-	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
+	char real_usage[HID_SENSOR_USAGE_LENGTH];
 	struct platform_device *custom_pdev;
 	const char *dev_name;
 	char *c;

-	/* copy real usage id */
-	memcpy(real_usage, known_sensor_luid[index], 4);
+	memcpy(real_usage, match->luid, 4);
+	real_usage[4] = '\0';

-	/* usage id are all lowcase */
 	for (c = real_usage; *c != '\0'; c++)
 		*c = tolower(*c);

-	/* HID-SENSOR-INT-REAL_USAGE_ID */
-	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
+	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
+			     match->tag, real_usage);
 	if (!dev_name)
 		return ERR_PTR(-ENOMEM);

@@ -873,7 +944,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	struct hid_sensor_custom *sensor_inst;
 	struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
 	int ret;
-	int index;
+	const struct hid_sensor_custom_match *match = NULL;

 	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
 				   GFP_KERNEL);
@@ -888,10 +959,10 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
 	mutex_init(&sensor_inst->mutex);
 	platform_set_drvdata(pdev, sensor_inst);

-	index = get_known_custom_sensor_index(hsdev);
-	if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
+	ret = hid_sensor_custom_get_known(hsdev, &match);
+	if (!ret && match) {
 		sensor_inst->custom_pdev =
-			hid_sensor_register_platform_device(pdev, hsdev, index);
+			hid_sensor_register_platform_device(pdev, hsdev, match);

 		ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
 		if (ret) {
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index ac631159403a..13b1e65fbdcc 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -132,6 +132,7 @@
 #define HID_USAGE_SENSOR_PROP_FRIENDLY_NAME			0x200301
 #define HID_USAGE_SENSOR_PROP_SERIAL_NUM			0x200307
 #define HID_USAGE_SENSOR_PROP_MANUFACTURER			0x200305
+#define HID_USAGE_SENSOR_PROP_MODEL				0x200306
 #define HID_USAGE_SENSOR_PROP_REPORT_INTERVAL			0x20030E
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS			0x20030F
 #define HID_USAGE_SENSOR_PROP_SENSITIVITY_RANGE_PCT		0x200310
--
2.38.1

[PATCH v3 2/4] HID: hid-sensor-custom: Add LISS custom sensors

[Philipp Jungkamp]

Add the Lenovo Intelligent Sensing Solution (LISS) custom sensors to the
known custom sensors.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Here is the requested noop commit that changes the device ids for the known
LISS sensors.

 drivers/hid/hid-sensor-custom.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 89e56913c92e..85db14406b23 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -782,6 +782,29 @@ static const struct hid_sensor_custom_match hid_sensor_custom_known_table[] = {
 		.luid = "020B000000000000",
 		.manufacturer = "INTEL",
 	},
+	/*
+	 * Lenovo Intelligent Sensing Solution (LISS)
+	 */
+	{	/* ambient light */
+		.tag = "LISS",
+		.luid = "0041010200000082",
+		.model = "STK3X3X Sensor",
+		.manufacturer = "Vendor 258",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
+	{	/* human presence */
+		.tag = "LISS",
+		.luid = "0226000171AC0081",
+		.model = "VL53L1_HOD Sensor",
+		.manufacturer = "ST_MICRO",
+		.check_dmi = true,
+		.dmi.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+		}
+	},
 	{}
 };

--
2.38.1

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

[Philipp Jungkamp]

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(-)

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);
--
2.38.1

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

[srinivas pandruvada]

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
> 

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

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs

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

[srinivas pandruvada]

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,
> 

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

[Jonathan Cameron]

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
> 

[PATCH v3 4/4] IIO: hid-sensor-prox: Use generic usage

[Philipp Jungkamp]

Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
allow this driver to drive the Lenvo custom proximity sensor, which is
registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.

Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
to the platform device ids.

Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
---
Lines are shorter and the switch statement for raw_len is cleaner.

 drivers/iio/light/hid-sensor-prox.c | 37 ++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
index f10fa2abfe72..a47591e1bad9 100644
--- a/drivers/iio/light/hid-sensor-prox.c
+++ b/drivers/iio/light/hid-sensor-prox.c
@@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			      long mask)
 {
 	struct prox_state *prox_state = iio_priv(indio_dev);
+	struct hid_sensor_hub_device *hsdev;
 	int report_id = -1;
 	u32 address;
 	int ret_type;
@@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			report_id = prox_state->prox_attr.report_id;
 			min = prox_state->prox_attr.logical_minimum;
 			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
+			hsdev = prox_state->common_attributes.hsdev;
 			break;
 		default:
 			report_id = -1;
@@ -84,11 +86,8 @@ static int prox_read_raw(struct iio_dev *indio_dev,
 			hid_sensor_power_state(&prox_state->common_attributes,
 						true);
 			*val = sensor_hub_input_attr_get_raw_value(
-				prox_state->common_attributes.hsdev,
-				HID_USAGE_SENSOR_PROX, address,
-				report_id,
-				SENSOR_HUB_SYNC,
-				min < 0);
+				hsdev, hsdev->usage, address, report_id,
+				SENSOR_HUB_SYNC, min < 0);
 			hid_sensor_power_state(&prox_state->common_attributes,
 						false);
 		} else {
@@ -191,10 +190,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,

 	switch (usage_id) {
 	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
-		prox_state->human_presence = *(u32 *)raw_data;
-		ret = 0;
-		break;
-	default:
+		switch (raw_len) {
+		case 1:
+			prox_state->human_presence = *(u8 *)raw_data;
+			return 0;
+		case 4:
+			prox_state->human_presence = *(u32 *)raw_data;
+			return 0;
+		default:
+			break;
+		}
 		break;
 	}

@@ -244,7 +249,7 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->common_attributes.hsdev = hsdev;
 	prox_state->common_attributes.pdev = pdev;

-	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
+	ret = hid_sensor_parse_common_attributes(hsdev, hsdev->usage,
 					&prox_state->common_attributes,
 					prox_sensitivity_addresses,
 					ARRAY_SIZE(prox_sensitivity_addresses));
@@ -262,7 +267,7 @@ static int hid_prox_probe(struct platform_device *pdev)

 	ret = prox_parse_report(pdev, hsdev,
 				(struct iio_chan_spec *)indio_dev->channels,
-				HID_USAGE_SENSOR_PROX, prox_state);
+				hsdev->usage, prox_state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to setup attributes\n");
 		return ret;
@@ -291,8 +296,8 @@ static int hid_prox_probe(struct platform_device *pdev)
 	prox_state->callbacks.send_event = prox_proc_event;
 	prox_state->callbacks.capture_sample = prox_capture_sample;
 	prox_state->callbacks.pdev = pdev;
-	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
-					&prox_state->callbacks);
+	ret = sensor_hub_register_callback(hsdev, hsdev->usage,
+					   &prox_state->callbacks);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "callback reg failed\n");
 		goto error_iio_unreg;
@@ -314,7 +319,7 @@ static int hid_prox_remove(struct platform_device *pdev)
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct prox_state *prox_state = iio_priv(indio_dev);

-	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
+	sensor_hub_remove_callback(hsdev, hsdev->usage);
 	iio_device_unregister(indio_dev);
 	hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);

@@ -326,6 +331,10 @@ static const struct platform_device_id hid_prox_ids[] = {
 		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
 		.name = "HID-SENSOR-200011",
 	},
+	{
+		/* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
+		.name = "HID-SENSOR-LISS-0226",
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, hid_prox_ids);
--
2.38.1

Re: [PATCH v3 4/4] IIO: hid-sensor-prox: Use generic usage

[srinivas pandruvada]

On Fri, 2022-11-18 at 00:48 +0100, Philipp Jungkamp wrote:
> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
> allow this driver to drive the Lenvo custom proximity sensor, which
> is
> registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.
> 
> Add the Lenovo Intelligent Sensing Solution (LISS) human presence
> sensor
> to the platform device ids.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> ---
> Lines are shorter and the switch statement for raw_len is cleaner.
Didn't understand why this change is required.

Thanks,
Srinivas

> 
>  drivers/iio/light/hid-sensor-prox.c | 37 ++++++++++++++++++---------
> --
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-prox.c
> b/drivers/iio/light/hid-sensor-prox.c
> index f10fa2abfe72..a47591e1bad9 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>                               long mask)
>  {
>         struct prox_state *prox_state = iio_priv(indio_dev);
> +       struct hid_sensor_hub_device *hsdev;
>         int report_id = -1;
>         u32 address;
>         int ret_type;
> @@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>                         report_id = prox_state->prox_attr.report_id;
>                         min = prox_state->prox_attr.logical_minimum;
>                         address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
> +                       hsdev = prox_state->common_attributes.hsdev;
>                         break;
>                 default:
>                         report_id = -1;
> @@ -84,11 +86,8 @@ static int prox_read_raw(struct iio_dev
> *indio_dev,
>                         hid_sensor_power_state(&prox_state-
> >common_attributes,
>                                                 true);
>                         *val = sensor_hub_input_attr_get_raw_value(
> -                               prox_state->common_attributes.hsdev,
> -                               HID_USAGE_SENSOR_PROX, address,
> -                               report_id,
> -                               SENSOR_HUB_SYNC,
> -                               min < 0);
> +                               hsdev, hsdev->usage, address,
> report_id,
> +                               SENSOR_HUB_SYNC, min < 0);
>                         hid_sensor_power_state(&prox_state-
> >common_attributes,
>                                                 false);
>                 } else {
> @@ -191,10 +190,16 @@ static int prox_capture_sample(struct
> hid_sensor_hub_device *hsdev,
> 
>         switch (usage_id) {
>         case HID_USAGE_SENSOR_HUMAN_PRESENCE:
> -               prox_state->human_presence = *(u32 *)raw_data;
> -               ret = 0;
> -               break;
> -       default:
> +               switch (raw_len) {
> +               case 1:
> +                       prox_state->human_presence = *(u8 *)raw_data;
> +                       return 0;
> +               case 4:
> +                       prox_state->human_presence = *(u32
> *)raw_data;
> +                       return 0;
> +               default:
> +                       break;
> +               }
>                 break;
>         }
> 
> @@ -244,7 +249,7 @@ static int hid_prox_probe(struct platform_device
> *pdev)
>         prox_state->common_attributes.hsdev = hsdev;
>         prox_state->common_attributes.pdev = pdev;
> 
> -       ret = hid_sensor_parse_common_attributes(hsdev,
> HID_USAGE_SENSOR_PROX,
> +       ret = hid_sensor_parse_common_attributes(hsdev, hsdev->usage,
>                                         &prox_state-
> >common_attributes,
>                                         prox_sensitivity_addresses,
>                                         ARRAY_SIZE(prox_sensitivity_a
> ddresses));
> @@ -262,7 +267,7 @@ static int hid_prox_probe(struct platform_device
> *pdev)
> 
>         ret = prox_parse_report(pdev, hsdev,
>                                 (struct iio_chan_spec *)indio_dev-
> >channels,
> -                               HID_USAGE_SENSOR_PROX, prox_state);
> +                               hsdev->usage, prox_state);
>         if (ret) {
>                 dev_err(&pdev->dev, "failed to setup attributes\n");
>                 return ret;
> @@ -291,8 +296,8 @@ static int hid_prox_probe(struct platform_device
> *pdev)
>         prox_state->callbacks.send_event = prox_proc_event;
>         prox_state->callbacks.capture_sample = prox_capture_sample;
>         prox_state->callbacks.pdev = pdev;
> -       ret = sensor_hub_register_callback(hsdev,
> HID_USAGE_SENSOR_PROX,
> -                                       &prox_state->callbacks);
> +       ret = sensor_hub_register_callback(hsdev, hsdev->usage,
> +                                          &prox_state->callbacks);
>         if (ret < 0) {
>                 dev_err(&pdev->dev, "callback reg failed\n");
>                 goto error_iio_unreg;
> @@ -314,7 +319,7 @@ static int hid_prox_remove(struct platform_device
> *pdev)
>         struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>         struct prox_state *prox_state = iio_priv(indio_dev);
> 
> -       sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
> +       sensor_hub_remove_callback(hsdev, hsdev->usage);
>         iio_device_unregister(indio_dev);
>         hid_sensor_remove_trigger(indio_dev, &prox_state-
> >common_attributes);
> 
> @@ -326,6 +331,10 @@ static const struct platform_device_id
> hid_prox_ids[] = {
>                 /* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>                 .name = "HID-SENSOR-200011",
>         },
> +       {
> +               /* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase
> */
> +               .name = "HID-SENSOR-LISS-0226",
> +       },
>         { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, hid_prox_ids);
> --
> 2.38.1
> 

Re: [PATCH v3 4/4] IIO: hid-sensor-prox: Use generic usage

[Jonathan Cameron]

On Fri, 18 Nov 2022 00:48:24 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> Use a generic 'hsdev->usage' instead of the HID_USAGE_SENSOR_PROX to
> allow this driver to drive the Lenvo custom proximity sensor, which is
> registered under a 'custom' usage and not HID_USAGE_SENSOR_PROX.
> 
> Add the Lenovo Intelligent Sensing Solution (LISS) human presence sensor
> to the platform device ids.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Not sure if this is going via HID or IIO trees, so for now

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> Lines are shorter and the switch statement for raw_len is cleaner.
> 
>  drivers/iio/light/hid-sensor-prox.c | 37 ++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/hid-sensor-prox.c b/drivers/iio/light/hid-sensor-prox.c
> index f10fa2abfe72..a47591e1bad9 100644
> --- a/drivers/iio/light/hid-sensor-prox.c
> +++ b/drivers/iio/light/hid-sensor-prox.c
> @@ -61,6 +61,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			      long mask)
>  {
>  	struct prox_state *prox_state = iio_priv(indio_dev);
> +	struct hid_sensor_hub_device *hsdev;
>  	int report_id = -1;
>  	u32 address;
>  	int ret_type;
> @@ -75,6 +76,7 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			report_id = prox_state->prox_attr.report_id;
>  			min = prox_state->prox_attr.logical_minimum;
>  			address = HID_USAGE_SENSOR_HUMAN_PRESENCE;
> +			hsdev = prox_state->common_attributes.hsdev;
>  			break;
>  		default:
>  			report_id = -1;
> @@ -84,11 +86,8 @@ static int prox_read_raw(struct iio_dev *indio_dev,
>  			hid_sensor_power_state(&prox_state->common_attributes,
>  						true);
>  			*val = sensor_hub_input_attr_get_raw_value(
> -				prox_state->common_attributes.hsdev,
> -				HID_USAGE_SENSOR_PROX, address,
> -				report_id,
> -				SENSOR_HUB_SYNC,
> -				min < 0);
> +				hsdev, hsdev->usage, address, report_id,
> +				SENSOR_HUB_SYNC, min < 0);
>  			hid_sensor_power_state(&prox_state->common_attributes,
>  						false);
>  		} else {
> @@ -191,10 +190,16 @@ static int prox_capture_sample(struct hid_sensor_hub_device *hsdev,
> 
>  	switch (usage_id) {
>  	case HID_USAGE_SENSOR_HUMAN_PRESENCE:
> -		prox_state->human_presence = *(u32 *)raw_data;
> -		ret = 0;
> -		break;
> -	default:
> +		switch (raw_len) {
> +		case 1:
> +			prox_state->human_presence = *(u8 *)raw_data;
> +			return 0;
> +		case 4:
> +			prox_state->human_presence = *(u32 *)raw_data;
> +			return 0;
> +		default:
> +			break;
> +		}
>  		break;
>  	}
> 
> @@ -244,7 +249,7 @@ static int hid_prox_probe(struct platform_device *pdev)
>  	prox_state->common_attributes.hsdev = hsdev;
>  	prox_state->common_attributes.pdev = pdev;
> 
> -	ret = hid_sensor_parse_common_attributes(hsdev, HID_USAGE_SENSOR_PROX,
> +	ret = hid_sensor_parse_common_attributes(hsdev, hsdev->usage,
>  					&prox_state->common_attributes,
>  					prox_sensitivity_addresses,
>  					ARRAY_SIZE(prox_sensitivity_addresses));
> @@ -262,7 +267,7 @@ static int hid_prox_probe(struct platform_device *pdev)
> 
>  	ret = prox_parse_report(pdev, hsdev,
>  				(struct iio_chan_spec *)indio_dev->channels,
> -				HID_USAGE_SENSOR_PROX, prox_state);
> +				hsdev->usage, prox_state);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to setup attributes\n");
>  		return ret;
> @@ -291,8 +296,8 @@ static int hid_prox_probe(struct platform_device *pdev)
>  	prox_state->callbacks.send_event = prox_proc_event;
>  	prox_state->callbacks.capture_sample = prox_capture_sample;
>  	prox_state->callbacks.pdev = pdev;
> -	ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_PROX,
> -					&prox_state->callbacks);
> +	ret = sensor_hub_register_callback(hsdev, hsdev->usage,
> +					   &prox_state->callbacks);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "callback reg failed\n");
>  		goto error_iio_unreg;
> @@ -314,7 +319,7 @@ static int hid_prox_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct prox_state *prox_state = iio_priv(indio_dev);
> 
> -	sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_PROX);
> +	sensor_hub_remove_callback(hsdev, hsdev->usage);
>  	iio_device_unregister(indio_dev);
>  	hid_sensor_remove_trigger(indio_dev, &prox_state->common_attributes);
> 
> @@ -326,6 +331,10 @@ static const struct platform_device_id hid_prox_ids[] = {
>  		/* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>  		.name = "HID-SENSOR-200011",
>  	},
> +	{
> +		/* Format: HID-SENSOR-tag-usage_id_in_hex_lowercase */
> +		.name = "HID-SENSOR-LISS-0226",
> +	},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, hid_prox_ids);
> --
> 2.38.1
> 

Re: [PATCH v3 1/4] HID: hid-sensor-custom: Allow more custom iio sensors

[Jonathan Cameron]

On Fri, 18 Nov 2022 00:48:21 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> The known LUID table for established/known custom HID sensors was
> limited to sensors with "INTEL" as manufacturer. But some vendors such
> as Lenovo also include fairly standard iio sensors (e.g. ambient light)
> in their custom sensors.
> 
> Expand the known custom sensors table by a tag used for the platform
> device name and match sensors based on the LUID as well as optionally
> on model and manufacturer properties.
> Introduce sensors from Lenovo's "Intelligent Sensing Solution" on the
> Lenovo Yoga 9 14IAP7 as an example.
> 
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>

Hi Philipp,

A few process things.
1) Don't send a new version in reply to an old one.  After a while the threads
get really confused (it's definitely happening on this one).
2) Put a over letter on any non trivial series
  git format-patch --cover-letter ... 
  Provides a useful place for general comments / background info etc and
  helpfully somewhere for series wide comments / tags.


A few additional comments inline from reading this version.

Jonathan


> -static int get_known_custom_sensor_index(struct hid_sensor_hub_device *hsdev)
> +static bool hid_sensor_custom_get_prop(struct hid_sensor_hub_device *hsdev,
> +				      u32 prop_usage_id, size_t prop_size,
> +				      u16 *prop)
>  {
> -	struct hid_sensor_hub_attribute_info sensor_manufacturer = { 0 };
> -	struct hid_sensor_hub_attribute_info sensor_luid_info = { 0 };
> +	struct hid_sensor_hub_attribute_info prop_attr = { 0 };
>  	int report_size;
>  	int ret;
> -	static u16 w_buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -	static char buf[HID_CUSTOM_MAX_FEATURE_BYTES];
> -	int i;
> 
> -	memset(w_buf, 0, sizeof(w_buf));
> -	memset(buf, 0, sizeof(buf));
> +	memset(prop, 0, prop_size);
> 
> -	/* get manufacturer info */
> -	ret = sensor_hub_input_get_attribute_info(hsdev,
> -			HID_FEATURE_REPORT, hsdev->usage,
> -			HID_USAGE_SENSOR_PROP_MANUFACTURER, &sensor_manufacturer);
> +	ret = sensor_hub_input_get_attribute_info(hsdev, HID_FEATURE_REPORT,
> +						  hsdev->usage, prop_usage_id,
> +						  &prop_attr);
>  	if (ret < 0)
> -		return ret;
> +		return 0;

If eating an error and returning, always good to add a comment on why.
For a 'get' function' like this I'd normally expect the function to return an
error code and the higher level code to decide to ignore it or not.

> 
> -	report_size =
> -		sensor_hub_get_feature(hsdev, sensor_manufacturer.report_id,
> -				       sensor_manufacturer.index, sizeof(w_buf),
> -				       w_buf);
> +	report_size = sensor_hub_get_feature(hsdev, prop_attr.report_id,
> +					     prop_attr.index, prop_size, prop);
>  	if (report_size <= 0) {
>  		hid_err(hsdev->hdev,
> -			"Failed to get sensor manufacturer info %d\n",
> +			"Failed to get sensor property %08x %d\n",
> +			prop_usage_id,
>  			report_size);
> -		return -ENODEV;
> +		return report_size;
>  	}
> 
> -	/* convert from wide char to char */
> -	for (i = 0; i < ARRAY_SIZE(buf) - 1 && w_buf[i]; i++)
> -		buf[i] = (char)w_buf[i];
> +	return 0;
> +}
> +


> +
> +static int
> +hid_sensor_custom_get_known(struct hid_sensor_hub_device *hsdev,
> +			    const struct hid_sensor_custom_match **known)
> +{
> +	int ret;
> +	const struct hid_sensor_custom_match *match =
> +		hid_sensor_custom_known_table;
> +	struct hid_sensor_custom_properties prop;
> +
> +	ret = hid_sensor_custom_properties_get(hsdev, &prop);
> +	if (ret < 0)
> +		return ret;
> +
> +	while (match->tag) {
> +		if (hid_sensor_custom_do_match(hsdev, match, &prop)) {
> +			*known = match;
> +			return 0;
> +		}
> +		match++;
>  	}
> 
> -	/* get table index with luid (not matching 'LUID: ' in luid) */
> -	return get_luid_table_index(&buf[5]);
> +	*known = NULL;

I'd expect this to be side effect free. That is, if nothing found it leaves 
*known untouched.

> +	return 0;

returning -EINVAL or similar from here probably makes sense (and then ignore the returned
value as suggested below.  The only reason to return anything at all from here
is that it's a generic function that might later get used for cases where we
do care about the error.

>  }
> 
>  static struct platform_device *
>  hid_sensor_register_platform_device(struct platform_device *pdev,
>  				    struct hid_sensor_hub_device *hsdev,
> -				    int index)
> +				    const struct hid_sensor_custom_match *match)
>  {
> -	char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> +	char real_usage[HID_SENSOR_USAGE_LENGTH];
>  	struct platform_device *custom_pdev;
>  	const char *dev_name;
>  	char *c;
> 
> -	/* copy real usage id */
> -	memcpy(real_usage, known_sensor_luid[index], 4);
> +	memcpy(real_usage, match->luid, 4);
> +	real_usage[4] = '\0';

Why the change in approach for setting the NULL character?
Doesn't seem relevant to main purpose of this patch.

> 
> -	/* usage id are all lowcase */

Why drop this comment. If it's wrong, then I'd prefer to see that
as a separate patch with explanation of why.

>  	for (c = real_usage; *c != '\0'; c++)
>  		*c = tolower(*c);
> 
> -	/* HID-SENSOR-INT-REAL_USAGE_ID */
> -	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s", real_usage);
> +	dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> +			     match->tag, real_usage);
>  	if (!dev_name)
>  		return ERR_PTR(-ENOMEM);
> 
> @@ -873,7 +944,7 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	struct hid_sensor_custom *sensor_inst;
>  	struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>  	int ret;
> -	int index;
> +	const struct hid_sensor_custom_match *match = NULL;
> 
>  	sensor_inst = devm_kzalloc(&pdev->dev, sizeof(*sensor_inst),
>  				   GFP_KERNEL);
> @@ -888,10 +959,10 @@ static int hid_sensor_custom_probe(struct platform_device *pdev)
>  	mutex_init(&sensor_inst->mutex);
>  	platform_set_drvdata(pdev, sensor_inst);
> 
> -	index = get_known_custom_sensor_index(hsdev);
> -	if (index >= 0 && index < ARRAY_SIZE(known_sensor_luid)) {
> +	ret = hid_sensor_custom_get_known(hsdev, &match);
> +	if (!ret && match) {

Match must be left NULL on error (if not it should be)
and we are otherwise ignoring ret, so why not just have the following?

	hid_sensor_custom_get_known(hsdev, &match);
	if (match) {
...


>  		sensor_inst->custom_pdev =
> -			hid_sensor_register_platform_device(pdev, hsdev, index);
> +			hid_sensor_register_platform_device(pdev, hsdev, match);
> 
>  		ret = PTR_ERR_OR_ZERO(sensor_inst->custom_pdev);
>  		if (ret) {

Re: [PATCH v3 1/4] HID: hid-sensor-custom: Allow more custom iio sensors

[Jonathan Cameron]

On Fri, 18 Nov 2022 00:48:21 +0100
Philipp Jungkamp <p.jungkamp@gmx.net> wrote:

> The known LUID table for established/known custom HID sensors was
> limited to sensors with "INTEL" as manufacturer. But some vendors such
> as Lenovo also include fairly standard iio sensors (e.g. ambient light)
> in their custom sensors.
> 
> Expand the known custom sensors table by a tag used for the platform
> device name and match sensors based on the LUID as well as optionally
> on model and manufacturer properties.
> Introduce sensors from Lenovo's "Intelligent Sensing Solution" on the
> Lenovo Yoga 9 14IAP7 as an example.
Description needs an update.  This is the No Operational Changes
patch, not the next one (which adds those IDs).
Point being that after this patch the code should function exactly as
before.  After the next patch, more devices will be recognized due to the
new IDs.