Re: [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing

[Hans de Goede <hansg@kernel.org>]

Hi,

On 29-May-26 14:16, 谢致邦 (XIE Zhibang) wrote:
> Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
> separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
> devices and hid-over-i2c OF devices. After the split, devices with _HID
> "PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
> i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
> for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
> descriptor address only through the _DSM method. Fall back to the _DSM
> call when the property is absent.
> 
> Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>

Thank you for the new patch, this is an interesting approach and better
then the modalias magic from the previous version.

Note I'm not the i2c-hid maintainer, with that said I think this should
be acceptable. But currently it duplicates the _DSM handling code and
that should be fixed.

I think this should be changed to a series of 3 patches:

1. Move the i2c_hid_acpi_blacklist handling out of
   i2c_hid_acpi_get_descriptor() into i2c_hid_acpi_probe()
   to above the devm_kzalloc() call.

2. Move i2c_hid_acpi_get_descriptor() to a generic
   int i2c_hid_acpi_get_descriptor(struct device *dev)
   helper in drivers/hid/i2c-hid/i2c-hid-core.c .
   Wrapped in #ifdef CONFIG_ACPI and with a static inline
   stub in drivers/hid/i2c-hid/i2c-hid.h when CONFIG_ACPI
   is not set, e.g. in drivers/hid/i2c-hid/i2c-hid.h add:

#ifdef CONFIG_ACPI
	int i2c_hid_acpi_get_descriptor_address(struct device *dev);
#else
	static inline int i2c_hid_acpi_get_descriptor_address(struct device *dev)
	{
		return -ENODEV;
	}
#endif

3. Modify i2c-hid-of.c to try i2c_hid_acpi_get_descriptor_address() as
fallback for the missing "hid-descr-addr" property.  Please also add
a comment in the code explaining that this fallback is about ACPI I2C-hid
devices which use a "PRP0001" ACPI _HID with an "hid-over-i2c" compatible.

Regards,

Hans



> ---
>  drivers/hid/i2c-hid/i2c-hid-of.c | 44 ++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> index 57379b77e977..62c089a6455a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -19,6 +19,7 @@
>   * more details.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/gpio/consumer.h>
> @@ -74,6 +75,39 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
>  			       ihid_of->supplies);
>  }
>  
> +#ifdef CONFIG_ACPI
> +/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
> +static guid_t i2c_hid_of_acpi_guid =
> +	GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> +		  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> +
> +/*
> + * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
> + * declare their I2C HID touchpad with _HID "PRP0001" and _DSD compatible
> + * "hid-over-i2c" but lack the "hid-descr-addr" property. Use the _DSM
> + * method to obtain the HID descriptor address.
> + */
> +static int i2c_hid_of_acpi_get_descriptor(struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	union acpi_object *obj;
> +	u16 addr;
> +
> +	if (!adev)
> +		return -ENODEV;
> +
> +	obj = acpi_evaluate_dsm_typed(acpi_device_handle(adev),
> +				      &i2c_hid_of_acpi_guid, 1, 1, NULL,
> +				      ACPI_TYPE_INTEGER);
> +	if (!obj)
> +		return -ENODEV;
> +
> +	addr = obj->integer.value;
> +	ACPI_FREE(obj);
> +	return addr;
> +}
> +#endif
> +
>  static int i2c_hid_of_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -92,6 +126,16 @@ static int i2c_hid_of_probe(struct i2c_client *client)
>  	ihid_of->ops.power_down = i2c_hid_of_power_down;
>  
>  	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
> +#ifdef CONFIG_ACPI
> +	if (ret) {
> +		int dsm_ret = i2c_hid_of_acpi_get_descriptor(dev);
> +
> +		if (dsm_ret >= 0) {
> +			val = dsm_ret;
> +			ret = 0;
> +		}
> +	}
> +#endif
>  	if (ret) {
>  		dev_err(dev, "HID register address not provided\n");
>  		return -ENODEV;