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

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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
>