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

[Hans de Goede]

Hi,

On 3-Jun-26 1:59 PM, Benjamin Tissoires wrote:
> On Jun 03 2026, Hans de Goede wrote:
>> Hi Benjamin,
>>
>> On 3-Jun-26 11:43 AM, Benjamin Tissoires wrote:
>>> 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@xxxxxxxxx>
>>>> ---
>>>>  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.
>>
>> It is only 35 new lines in i2c-hid-core.c, so it is not that big / much code.
> 
> My concern is not so much about these 35 lines of code, but the fact
> that i2c-hid-acpi.c is now down to only 3 functions:
> - i2c_hid_acpi_probe()
> - i2c_hid_acpi_shutdown_tail()
> - i2c_hid_acpi_restore_sequence()
> 
> So then, what's the point of keeping it as a separate driver? (more
> rethorical question than anything).
> 
>>
>>> 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
>>
>> As part of the work to use ACPI on ARM systems, it is possible now a days
>> to inject DT snippets into ACPI tables.
>>
>> The problem on these Loongson laptops is that they have created this
>> very ugly mixed-mode where they put a PRP0001 ACPI HID on the ACPI
>> node for the touchscreen, which means it contains an embedded DT
>> snippet and in that snipped out "compatible=hid-over-i2c" but NOT
>> also "i2c-hid-descr-addr=x". To get the i2c-hid descriptor address
>> the implemented the ACPI _DSM on the same ACPI device/fwnode.
>>
>> The ACPI core sees HID=PRP0001 so it creates an of-compatible 
>> modalias / match and not an ACPI one, so the i2c-hid-of driver will
>> bind.
> 
> But if this fix is for just one platform, why not keeping the design
> clean and have a dmi_match instead in a separate driver, like the
> i2c-hid-of-elan does for Elan touchpads/touchscreens?
> i2c-hid-acpi-loongson.c for instance.

Ok, so let me see if I've got this right:

1. We create a i2c-hid-acpi-loongson.c with a DMI
MODULE_DEVICE_TABLE() for auto-module-loading

2. This will have a struct of_device_id table with
the "hid-over-i2c" compatible. But *no* / without a
MODULE_DEVICE_TABLE() for this so that it will not
autoload on regular DT platforms with regular
"hid-over-i2c" compatible touchscreens.

3. We will rely on i2c-hid-of.c error-ing out
due to the missing "hid-descr-addr" and we are ok
with the dev_err() this logs.

4. The new i2c-hid-acpi-loongson.c will in essence
be a copy of i2c-hid-acpi.c without the acpi_match
and with the DMI id + of match as described above.

Yes I think that should work, do you want to just copy
i2c_hid_acpi_get_descriptor() or do you want
i2c-hid-acpi to export this and have i2c-hid-acpi-longsoon
depend on i2c-hid-acpi for this ?

>> Note these "fixed" (as in we cannot fix them) ACPI tables use
>> the generic i2c-over-hid OF/DT compatible _not_ something
>> more specific so we can also not do a more specific driver like
>> i2c-hid-of-elan.c.
> 
> Do they use anything else than the compatible DT entry? regulators and
> others? If not, then the device is pure ACPI, and should not be handled
> by i2c-hid-of.c at all, no?

No they don't use anything other then the compatible. This is
really just a very bad / messy decision by whomever created
the ACPI tables for these 2 laptops. But they are out there, so
we somehow have to deal with them.

<snip>

> so far :)

True, see above.

Regards,

Hans