Re: [PATCH v2 2/3] HID: i2c-hid: Move common ACPI _DSM helper into core

[Benjamin Tissoires <bentiss@kernel.org>]

On Jun 01 2026, 谢致邦 (XIE Zhibang) wrote:
> Move the _DSM call that gets the HID descriptor address from
> i2c-hid-acpi.c to a shared helper in i2c-hid-core.c so both
> i2c-hid-acpi.c and i2c-hid-of.c can use it.
> 
> Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-acpi.c | 32 ++++-----------------------
>  drivers/hid/i2c-hid/i2c-hid-core.c | 35 ++++++++++++++++++++++++++++++

I'm not a big fan of removing 25% of the i2c-hid-acpi.c code and inject
it in i2c-hid-core.c.

There's something I can't really understand in the problem:
- we are talking about non arm architecture, which should not have OF in
	them
- the current compatible hid-over-i2c loads the OF version?
- how can you make the device working without the couple of ACPI
	functions that are in the i2c-hid-acpi.c driver for powering up and
	down the device?

If the platform is indeed ACPI + x86(_64), then shouldn't we tackle the
problem in the i2c-hid-acpi driver, or add a secondary leaf driver for
this particular platform/device? Kind of what we have with
i2c-hid-of-elan.c

Cheers,
Benjamin

>  drivers/hid/i2c-hid/i2c-hid.h      | 11 ++++++++++
>  3 files changed, 50 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> index f65fb6396b69..234789a07047 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -25,12 +25,12 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pm.h>
> -#include <linux/uuid.h>
>  
>  #include "i2c-hid.h"
>  
>  struct i2c_hid_acpi {
>  	struct i2chid_ops ops;
> +	struct i2c_client *client;
>  	struct acpi_device *adev;
>  };
>  
> @@ -48,36 +48,11 @@ static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
>  	{ }
>  };
>  
> -/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
> -static guid_t i2c_hid_guid =
> -	GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> -		  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> -
> -static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
> -{
> -	struct acpi_device *adev = ihid_acpi->adev;
> -	acpi_handle handle = acpi_device_handle(adev);
> -	union acpi_object *obj;
> -	u16 hid_descriptor_address;
> -
> -	obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
> -				      ACPI_TYPE_INTEGER);
> -	if (!obj) {
> -		acpi_handle_err(handle, "Error _DSM call to get HID descriptor address failed\n");
> -		return -ENODEV;
> -	}
> -
> -	hid_descriptor_address = obj->integer.value;
> -	ACPI_FREE(obj);
> -
> -	return hid_descriptor_address;
> -}
> -
>  static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
>  {
>  	struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
>  
> -	i2c_hid_acpi_get_descriptor(ihid_acpi);
> +	i2c_hid_core_acpi_get_descriptor(&ihid_acpi->client->dev);
>  }
>  
>  static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
> @@ -102,11 +77,12 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>  	if (!ihid_acpi)
>  		return -ENOMEM;
>  
> +	ihid_acpi->client = client;
>  	ihid_acpi->adev = adev;
>  	ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
>  	ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
>  
> -	ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
> +	ret = i2c_hid_core_acpi_get_descriptor(dev);
>  	if (ret < 0)
>  		return ret;
>  	hid_descriptor_address = ret;
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 3adb16366e93..1e1a8df5686d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1405,6 +1405,41 @@ const struct dev_pm_ops i2c_hid_core_pm = {
>  };
>  EXPORT_SYMBOL_GPL(i2c_hid_core_pm);
>  
> +#ifdef CONFIG_ACPI
> +#include <linux/acpi.h>
> +
> +/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
> +static guid_t i2c_hid_guid =
> +	GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> +		  0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> +
> +int i2c_hid_core_acpi_get_descriptor(struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	acpi_handle handle;
> +	union acpi_object *obj;
> +	u16 hid_descriptor_address;
> +
> +	if (!adev)
> +		return -ENODEV;
> +
> +	handle = acpi_device_handle(adev);
> +	obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
> +				      ACPI_TYPE_INTEGER);
> +	if (!obj) {
> +		acpi_handle_err(handle,
> +				"Error _DSM call to get HID descriptor address failed\n");
> +		return -ENODEV;
> +	}
> +
> +	hid_descriptor_address = obj->integer.value;
> +	ACPI_FREE(obj);
> +
> +	return hid_descriptor_address;
> +}
> +EXPORT_SYMBOL_GPL(i2c_hid_core_acpi_get_descriptor);
> +#endif
> +
>  MODULE_DESCRIPTION("HID over I2C core driver");
>  MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/i2c-hid/i2c-hid.h b/drivers/hid/i2c-hid/i2c-hid.h
> index 1724a435c783..bc8661c65b1a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.h
> +++ b/drivers/hid/i2c-hid/i2c-hid.h
> @@ -44,4 +44,15 @@ void i2c_hid_core_shutdown(struct i2c_client *client);
>  
>  extern const struct dev_pm_ops i2c_hid_core_pm;
>  
> +#ifdef CONFIG_ACPI
> +struct device;
> +int i2c_hid_core_acpi_get_descriptor(struct device *dev);
> +#else
> +struct device;
> +static inline int i2c_hid_core_acpi_get_descriptor(struct device *dev)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  #endif
> -- 
> 2.54.0
> 
>