diff mbox series

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

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

Commit Message

Philipp Jungkamp Nov. 17, 2022, 11:48 p.m. UTC
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(-)

--
2.38.1

Comments

Jonathan Cameron Nov. 23, 2022, 5:08 p.m. UTC | #1
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) {
Jonathan Cameron Nov. 23, 2022, 5:10 p.m. UTC | #2
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.
diff mbox series

Patch

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