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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Mar 06 2023, Andy Shevchenko wrote:
> On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote:
> 
> ...
> 
> > Device (SE9)
> > {
> >     Name (_ADR, 0x001D0001) // _ADR: Address
> >     Device (RHUB)
> >     {
> >         Name (_ADR, Zero)
> >         Device (CP2) // the USB-hid & CP2112 shared node
> >         {
> >             Name (_ADR, One)
> >         }
> >     }
> > }
> > 
> > If I'm understanding correctly, this adds the SE9 device as function 1
> > of PCI device 0x1d,
> 
> To be precise this does not add the device. It adds a description of
> the companion device in case the real one will appear on the PCI bus
> with BDF 00:1d.1.
> 
> > then RHUB as the USB controller it provides, and finally, CP2 as the
> > USB device attached to port 1 of the controller.
> > 
> > With this as the loaded dsdt table, the USB device now has a firmware_node :)
> > #> cat /sys/bus/usb/devices/3-1:1.0/firmware_node/path
> > \_SB_.PCI0.SE9_.RHUB.CP2_
> > 
> > After applying my patches, the HID device also references this node:
> > #> cat /sys/bus/hid/devices/0003:10C4:EA90.0003/firmware_node/path
> > \_SB_.PCI0.SE9_.RHUB.CP2_
> > 

Great! Thanks a lot for that. Turns out that with both of your inputs I
can also do the same, but without the need for OVMF and DSDT patching,
with just an SSDT override.

Turns out that the override documentation [1] mentions "This option
allows loading of user defined SSDTs from initrd and it is useful when
the system does not support EFI or ..."

FWIW, I am attaching my full DSDT override in case it is valuable:
(on my system, the default USB controller (non-xhc) is at PCI address
1.2, which explains the slight difference). It can be loaded in the same
way you are overriding the full DSDT, but with just that compilation
output:

---
DefinitionBlock ("cp2112.aml", "SSDT", 5, "", "CP2112", 1)
{
  External (_SB_.PCI0, DeviceObj)

  Scope (\_SB_.PCI0)
  {
    Device (USB0)
    {
      Name (_ADR, 0x00010002) // _ADR: Address
      Device (RHUB)
      {
        Name (_ADR, Zero)
        Device (CP21) // the USB-hid & CP2112 shared node
        {
          Name (_ADR, One)
          Device (I2C)
          {
            Name (_ADR, Zero)
            Name (_STA, 0x0F)
          }

          Device (GPIO)
          {
            Name (_ADR, One)
            Name (_STA, 0x0F)
          }
        }
      }
    }
  }

  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.GPIO", 0x00, ResourceConsumer, ,
              )
              {   // Pin list
                  0x0002
              }
      })
      Method(_CRS, 0x0, NotSerialized)
      {
        Return (ConcatenateResTemplate (SBFB, SBFG))
      }

      Method(_DSM, 0x4, Serialized)
      {
        // DSM UUID
        switch (ToBuffer (Arg0))
        {
          // ACPI DSM UUID for HIDI2C
          case (ToUUID ("3CDFF6F7-4267-4555-AD05-B30A3D8938DE"))
          {
              // DSM Function
              switch (ToInteger (Arg2))
              {
                  // Function 0: Query function, return based on revision
                  case(0)
                  {
                      // DSM Revision
                      switch (ToInteger (Arg1))
                      {
                          // Revision 1: Function 1 supported
                          case (1)
                          {
                              Return (Buffer (One) { 0x03 })
                          }

                          default
                          {
                              // Revision 2+: no functions supported
                              Return (Buffer (One) { 0x00 })
                          }
                      }
                  }

                  // Function 1 : HID Function
                  case(1)
                  {
                      // HID Descriptor Address
                      Return (0x0020)
                  }

                  default
                  {
                      // Functions 2+: not supported
                      Return (Buffer (One) { 0x00 })
                  }
              }
          }

          default
          {
              // No other GUIDs supported
              Return (Buffer (One) { 0x00 })
          }
        }
      }
    }
  }
}
---

This almost works. Almost because the I2C device is correctly created,
but I have an issue with the GpioInt call which is not properly set by
the kernel and which returns -EDEFER. /o\ 

> > With this all said -- I noticed iasl prints this statement when trying
> > to create a node with a lowercase name:
> > "At least one lower case letter found in NameSeg, ASL is case
> > insensitive - converting to upper case (GPIO)"
> 
> Yes, because it should be in the upper case.
> 
> > I wonder if this suggests that adding a call to toupper() to
> > acpi_fwnode_get_named_child_node would be
> > an appropriate solution for the node name casing issue....
> 
> I dunno. You need to ask in the linux-acpi@ mailing list.
> To me this is corner case that can't be easily solved
> (because two different specifications treat it differently.
> 
> You also need to ask DT people about capital letters there.
> And my guts tell me that it's probably also carved in the spec
> as "must be lower case" or alike.

FWIW while trying to enable this, at some point I named the I2C and the
GPIO entries "I2C0" and "GPI0" (with the number '0', not the letter
'o'), and it was not working as you would expect.

It is commonly accepted in the ACPI world that the names do not carry
meaning AFAICT, and so I think I agree with Andy's initial comment
regarding using indexes, not names to also fetch the I2C and GPIO nodes.
You can probably have a fallback mechanism for when "i2c" is not
present, or simply check if you are in DT or not and use the names only
if we are in DT.

Thanks a lot to both of you, this will be tremendously helpful to me.

Cheers,
Benjamin

[1] https://www.kernel.org/doc/html/latest/admin-guide/acpi/ssdt-overlays.html#loading-acpi-ssdts-from-initrd