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

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

> If we could fix the ACPI tables we would just
> change the ACPI HID to the standard PNPxxxx ACPI HID for i2c-hid
> devices.
> 
> The first version of this patch-set added an of match table
> to i2c-hid-acpi making it also load and probe i2c-hid devices
> described in devicetree on devicetree only systems which IMHO
> is very messy.

Yeah, this one is slightly less messy than the previous versions. It is
just sad to mess up with a clean separation and make the i2c-hid-acpi
driver just a placeholder for a probe function.

> 
> So this new series is the least ugly way to deal with this.

so far :)

Cheers,
Benjamin