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

[Benjamin Tissoires <bentiss@kernel.org>]

On Jun 03 2026, Hans de Goede wrote:
> 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@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.
> >>
> >> 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

Yes

> 
> 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.

Yes

> 
> 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.

Yes, if we want the big fat warning that Dmitry was mentioning.
Otherwise we could also have a deny list table, but that's more work :)

> 
> 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.

Sort of, yeah

> 
> 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 ?

Export i2c_hid_acpi_get_descriptor() from i2c-hid-acpi and have
i2c-hid-acpi-longsoon depend on it.

Also, this would have the good side effect of being able to set the post
power and post reset delays based on the DMI. Kind of like a compatible
driver in the DT world.

> 
> >> 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.

Yeah, so I think we better have this special driver for the Loongson
platform, and have device crap handled in this place without polluting
the rest.

Cheers,
Benjamin

> 
> <snip>
> 
> > so far :)
> 
> True, see above.
> 
> Regards,
> 
> Hans
> 
> 
> 
>