Re: [PATCH] HID: i2c-hid-acpi: Add PRP0001 to match table and OF alias

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Adding `PRP0001` directly to the ACPI match table incorrectly bypasses `compatible` string matching, causing the driver to intercept all PRP0001 I2C devices and spam the kernel log.
--

commit 7d0ee8c935eaadd4f5e66e197d5712a960fd31f5
Author: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>

HID: i2c-hid-acpi: Add PRP0001 to match table and OF alias

This commit adds the PRP0001 ACPI ID to the i2c-hid-acpi driver's match
table and manually defines an OF module alias. This is intended to allow the
driver to probe devices that declare themselves as PRP0001 with a "hid-over-i2c"
compatible string in their _DSD.

> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> index abd700a101f46..515ced22c978d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -119,10 +119,30 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>  static const struct acpi_device_id i2c_hid_acpi_match[] = {
>  	{ "ACPI0C50" },
>  	{ "PNP0C50" },
> +	/*
> +	 * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
> +	 * declare their I2C HID ACPI touchpad in the DSDT as _HID "PRP0001"
> +	 * with _DSD compatible "hid-over-i2c" instead of the standard
> +	 * "PNP0C50". This worked before i2c-hid was split into i2c-hid-acpi
> +	 * and i2c-hid-of, but PRP0001 devices on the ACPI bus are no longer
> +	 * probed after the split. The _DSM call in probe() naturally rejects
> +	 * PRP0001 devices that are not actually I2C HID, so matching PRP0001
> +	 * here is safe.
> +	 */
> +	{ "PRP0001" },

[Severity: Medium]
Does adding PRP0001 directly to the ACPI match table bypass the OF compatible
string check in __acpi_match_device()?

PRP0001 is a special identifier used by the ACPI subsystem to defer matching
to the OF compatible string within the device's _DSD. When PRP0001 is
explicitly added here, the match returns true immediately without checking the
compatible string.

If a system has other unrelated I2C devices using PRP0001 (like temperature
sensors or RTCs utilizing compatible strings), won't the driver core attempt
to bind i2c-hid-acpi to all of them?

Because i2c_hid_acpi_probe() unconditionally evaluates the HID _DSM which will
fail for these unrelated devices, does this introduce boot delays and pollute
the kernel log with false-positive "Error _DSM call to get HID descriptor
address failed" messages for every non-HID PRP0001 device?

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
>  
> +	/*
> +	 * When an ACPI device has a _DSD "compatible" property, the uevent
> +	 * modalias uses the OF format (of:N<name>T<compatible>) instead of
> +	 * the ACPI format (acpi:<HID>). Add an OF alias so udev can autoload
> +	 * this module for such devices. probe() will reject pure DT devices
> +	 * via the _DSM HID descriptor call.
> +	 */
> +MODULE_ALIAS("of:N*TChid-over-i2c");

[Severity: Medium]
Instead of manually defining this MODULE_ALIAS, could we define an
of_device_id match table containing { .compatible = "hid-over-i2c" }?

Using an of_device_id table would naturally solve the matching logic by
allowing the ACPI core to correctly evaluate the _DSD compatible string.
This prevents intercepting unrelated PRP0001 devices, and the driver core
would automatically generate the correct, robust module alias for autoloading.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_61698D7A14AC9BFE6C9F3FDB797FCA02E309@qq.com?part=1