Re: [PATCH v8 3/3] HID: cp2112: Fwnode Support

[Daniel Kaehn <kaehndan@gmail.com>]

On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mar 07 2023, Andy Shevchenko wrote:
> > On Tue, Mar 07, 2023 at 03:48:52PM +0100, Benjamin Tissoires wrote:
> > > On Mar 07 2023, Daniel Kaehn wrote:
> >
> > ...
> >
> > > So I can see that the device gets probed, and that all ACPI resources
> > > are tried to get the IRQ.
> > > Right now, I see that it's attempting to bind to the acpi resource in
> > > acpi_dev_resource_interrupt() (in file drivers/acpi/resources.c), but
> > > instead of having a ACPI_RESOURCE_TYPE_EXTENDED_IRQ I only get a
> > > ACPI_RESOURCE_TYPE_GPIO for the GpioInt() definition in the _CRS method.
> > >
> > > So I am missing the proper transition from GpioInt to IRQ in the acpi.
> >
> > I'm not sure I understand what this means.
> >
> > The Linux kernel takes either Interrupt() resource (which is
> > IOxAPIC / GIC / etc) or GpioInt() (which is GPIO based).
> >
> > In both cases I²C framework submits this into client's IRQ field.
> >
>
> I finally managed to get past the retrieval of the GpioInt.
>
> Turns out that the function acpi_get_gpiod() matches on the parent of
> the gpio_chip (gc->parent), which means that, with the current code and
> my SSDT, it matches on the HID CP2112 ACPI node, not the GPIO one.
>
> For reference (with lots of boiler plate removed):
>
>         Device (CP21) { // the USB-hid & CP2112 shared node
>          Device (GPIO) {
>             Name (_ADR, One)
>             Name (_STA, 0x0F)
>
>             Name (_DSD, Package () {
>               ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>               Package () {
>                 Package () { "gpio-line-names", Package () {
>                             "",
>                             "",
>                             "irq-rmi4",
>                             "",
>                             "power", // set to 1 with gpio-hog above
>                             "",
>                             "",
>                             "",
>                             ""}},
>               }
>             })
>          }
>
>   Scope (\_SB_.PCI0.USB0.RHUB.CP21.I2C)
>   {
>     Device (TPD0)
>     {
>       Name (_HID, "RMI40001")
>       Name (_CID, "PNP0C50")
>       Name (_STA, 0x0F)
>
>       Name (SBFB, ResourceTemplate ()
>       {
>           I2cSerialBusV2 (0x00c, ControllerInitiated, 100000,
>               AddressingMode7Bit, "\\_SB_.PCI0.USB0.RHUB.CP21.I2C",
>               0x00, ResourceConsumer,, Exclusive,
>               )
>       })
>       Name (SBFG, ResourceTemplate ()
>       {
>           GpioInt (Level, ActiveLow, Exclusive, PullDefault, 0x0000,
>               "\\_SB_.PCI0.USB0.RHUB.CP21", 0x00, ResourceConsumer, ,
>               )
>               {   // Pin list
>                   0x0002
>               }
>       })
> ---
>
> But if I refer "\\_SB_.PCI0.USB0.RHUB.CP21.GPIO", the IRQ is never assigned.
> With the parent (CP21), it works.
>
> So I wonder if the cp2112 driver is correctly assigning the gc->parent
> field.


Did you make a change to the CP2112 driver patch to look for uppercase
"I2C" and "GPIO"?
Otherwise, it won't assign those child nodes appropriately, and the
gpiochip code will use
the parent node by default if the gpiochip's fwnode isn't assigned (I believe).

If that was indeed your problem all along (and I'm not missing
something else), sorry about that --
I made a comment above, but didn't add much spacing around it to make
it stand out (since I noticed you didn't reply to that part in your response)

> To get this to work -- I assume you had to change the driver to look
> for uppercase
> "GPIO" and "I2C", or some similar change?

Thanks,

Danny Kaehn