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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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