| Message ID | 20230227140758.1575-4-kaehndan@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > Bind I2C and GPIO interfaces to subnodes with names > > "i2c" and "gpio" if they exist, respectively. This > > allows the GPIO and I2C controllers to be described > > in firmware as usual. Additionally, support configuring the > > I2C bus speed from the clock-frequency device property. > > A bit shorten indentation... > > Nevertheless what I realized now is that this change, despite being OF > independent by used APIs, still OF-only. I assumed this would practically be the case -- not because of the casing reason you gave (wasn't aware of that, thanks for the FYI), but because it doesn't seem that there's any way to describe USB devices connected to a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black box to me). But it seems reasonable that we should try to use the interface in a way so that it could be described using ACPI at some point (assuming that it isn't currently possible). > > Would it be possible to allow indexed access to child nodes as well, so if > there are no names, we may still be able to use firmware nodes from the correct > children? > Sure, you mean to fallback to using child nodes by index rather than by name in the case that that device_get_named_child_node() fails? Would we need to somehow verify that those nodes are the nodes we expect them to be? (a.e. node 0 is actually the i2c-controller, node 1 is actually the gpio-controller). I don't see a reason why not, though I am curious if there is precedence for this strategy, a.e. in other drivers that use named child nodes. In my initial search through the kernel, I don't think I found anything like this -- does that mean those drivers also inherently won't work with ACPI? The only driver I can find which uses device_get_named_child_node and has an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c > P.S. The problem with ACPI is that "name" of the child node will be in capital > letters as it's in accordance with the specification. > Knowing that this is the limitation, some other potential resolutions to potentially consider might be: - Uppercase the names of the child nodes for the DT binding -- it appears that the child node that chtwc_int33fe.c (the driver mentioned earlier) accesses does indeed have an upper-cased name -- though that driver doesn't have an of_device_id (makes sense, x86...). It seems named child nodes are always lowercase in DT bindings -- not sure if that's a rule, or just how it currently happens to be. - Do a case invariant compare on the names (and/or check for both lowercase and uppercase) - Remove the use of child nodes, and combine the i2c and gpio nodes into the cp2112's fwnode. I didn't do this initially because I wanted to avoid namespace collisions between GPIO hogs and i2c child devices, and thought that logically made sense to keep them separate, but that was before knowing this limitation of ACPI. What are your / others' thoughts? Thanks, Danny Kaehn > -- > With Best Regards, > Andy Shevchenko > >
On Mar 01 2023, Andy Shevchenko wrote: > +Cc: Hans (as some DT/ACPI interesting cases are being discussed). > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > On Mon, Feb 27, 2023 at 5:07 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Feb 27, 2023 at 08:07:58AM -0600, Danny Kaehn wrote: > > > > Bind I2C and GPIO interfaces to subnodes with names > > > > "i2c" and "gpio" if they exist, respectively. This > > > > allows the GPIO and I2C controllers to be described > > > > in firmware as usual. Additionally, support configuring the > > > > I2C bus speed from the clock-frequency device property. > > > > > > A bit shorten indentation... > > > > > > Nevertheless what I realized now is that this change, despite being OF > > > independent by used APIs, still OF-only. > > > > I assumed this would practically be the case -- not because of the casing > > reason you gave (wasn't aware of that, thanks for the FYI), but because it > > doesn't seem that there's any way to describe USB devices connected to > > a USB port in ACPI, at least as far as I can tell (ACPI is still largely a black > > box to me). > > That's not true :-) > > Microsoft created a schema that is not part of the specification, but let's > say a standard de facto. Linux supports that and I even played with it [1] > to get connection of the arbitrary device to USB-2-GPIO/I²C/SPI adapter. > > > But it seems reasonable that we should try to use the interface > > in a way so that it could be described using ACPI at some point (assuming > > that it isn't currently possible). > > > > > Would it be possible to allow indexed access to child nodes as well, so if > > > there are no names, we may still be able to use firmware nodes from the correct > > > children? > > > > Sure, you mean to fallback to using child nodes by index rather than by name > > in the case that that device_get_named_child_node() fails? Would we need to > > somehow verify that those nodes are the nodes we expect them to be? (a.e. > > node 0 is actually the i2c-controller, node 1 is actually the > > gpio-controller). > > Something like that, but I don't know if we can actually validate this without > having a preliminary agreement (hard-coded values) that index 0 is always let's > say I²C controller and so on. > > > I don't see a reason why not, though I am curious if there is > > precedence for this > > strategy, a.e. in other drivers that use named child nodes. In my initial search > > through the kernel, I don't think I found anything like this -- does that mean > > those drivers also inherently won't work with ACPI? > > If they are relying on names, yes, they won't work. It might be that some of them > have specific ACPI approach where different description is in use. > > > The only driver I can find which uses device_get_named_child_node and has > > an acpi_device_id is drivers/platform/x86/intel/chtwc_int33fe.c > > Yes, and you may notice the capitalization of the name. Also note, that relying > on the name in ACPI like now is quite fragile due to no common standard between > OEMs on how to name the same hardware nodes, they are free in that. > > > > P.S. The problem with ACPI is that "name" of the child node will be in capital > > > letters as it's in accordance with the specification. > > > > Knowing that this is the limitation, some other potential resolutions > > to potentially > > consider might be: > > > > - Uppercase the names of the child nodes for the DT binding -- it appears > > that the child node that chtwc_int33fe.c (the driver mentioned earlier) > > accesses does indeed have an upper-cased name -- though that driver doesn't > > have an of_device_id (makes sense, x86...). It seems named child nodes are > > always lowercase in DT bindings -- not sure if that's a rule, or just how > > it currently happens to be. > > Not an option AFAIK, the DT and ACPI specifications are requiring conflicting > naming schema. > > Possible way is to lowering case for ACPI names, but I dunno. We need more > opinions on this. > > > - Do a case invariant compare on the names (and/or check for both lowercase > > and uppercase) > > For ACPI it will be always capital. For DT I have no clue if they allow capital > letters there, but I assume they don't. > > > - Remove the use of child nodes, and combine the i2c and gpio nodes into the > > cp2112's fwnode. I didn't do this initially because I wanted to avoid > > namespace collisions between GPIO hogs and i2c child devices, and thought > > that logically made sense to keep them separate, but that was before > > knowing this limitation of ACPI. > > Seems to me not an option at all, we need to define hardware as is. If it > combines two devices under a hood, should be two devices under a hood in > DT/ACPI. > > [1]: https://stackoverflow.com/a/60855157/2511795 Thanks Andy for your help here, and thanks for that link. I am trying to test Danny's patch as I want to use it for my HID CI, being an owner of a CP2112 device myself. The current setup is using out of the tree patches [2] which are implementing a platform i2c-hid support and some manual addition of a I2C-HID device on top of it. This works fine but gets busted every now and then when the tree sees a change that conflicts with these patches. So with Danny's series, I thought I could have an SSDT override to declare that very same device instead of patching my kernel before testing it. Of course, it gets tricky because I need to run that under qemu. I am currently stuck at the "sharing the firmware_node from usb with HID" step and I'd like to know if you could help me. On my laptop, if I plug the CP2112 (without using a USB hub), I can get: $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 $> ls -l /sys/bus/usb/devices/2-9*/firmware_node lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 So AFAIU the USB device is properly assigned a firmware node. My dsdt also shows the "Device (RHUB)" and I guess everything is fine. However, playing with qemu is not so easy. I am running qemu with the following arguments (well, almost because I have a wrapper script on top of it and I also run the compiled kernel from the current tree): #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ -netdev user,id=hostnet0 \ -device virtio-net-pci,netdev=hostnet0 \ -m 4G \ -enable-kvm \ -cpu host \ -device qemu-xhci -usb \ -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso And this is what I get: #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 #> ls -l /sys/bus/usb/devices/2-1*/firmware_node ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory Looking at the DSDT, I do not see any reference to the USB hub, so I wonder if the firmware_node needs to be populated first in the DSDT. Also note that if I plug the CP2112 over a docking station, I lose the firmware_node sysfs entries on the host too. Do you think it would be achievable to emulate that over qemu and use a mainline kernel without patches? Cheers, Benjamin [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
On Mon, Mar 06, 2023 at 03:48:18PM +0100, Benjamin Tissoires wrote: > > > On Mon, Mar 6, 2023 at 2:07 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Mar 06, 2023 at 01:36:51PM +0100, Benjamin Tissoires wrote: > > > On Mon, Mar 6, 2023 at 11:49 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Mar 02, 2023 at 06:06:06PM +0100, Benjamin Tissoires wrote: > > > > > On Mar 01 2023, Andy Shevchenko wrote: > > > > > > On Tue, Feb 28, 2023 at 01:05:54PM -0600, Daniel Kaehn wrote: > > > > ... > > > > [1]: https://stackoverflow.com/a/60855157/2511795 > > > > > > > Thanks Andy for your help here, and thanks for that link. > > > > > > > > > > I am trying to test Danny's patch as I want to use it for my HID CI, > > > > > being an owner of a CP2112 device myself. > > > > > > > > > > The current setup is using out of the tree patches [2] which are > > > > > implementing a platform i2c-hid support and some manual addition of a > > > > > I2C-HID device on top of it. This works fine but gets busted every now > > > > > and then when the tree sees a change that conflicts with these patches. > > > > > > > > > > So with Danny's series, I thought I could have an SSDT override to > > > > > declare that very same device instead of patching my kernel before > > > > > testing it. > > > > > > > > > > Of course, it gets tricky because I need to run that under qemu. > > > > > > > > > > I am currently stuck at the "sharing the firmware_node from usb with > > > > > HID" step and I'd like to know if you could help me. > > > > > > > > > > On my laptop, if I plug the CP2112 (without using a USB hub), I can get: > > > > > > > > > > $> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/hid/devices/0003:10C4:EA90.0079 -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-9/2-9:1.0/0003:10C4:EA90.0079 > > > > > $> ls -l /sys/bus/usb/devices/2-9*/firmware_node > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:03 /sys/bus/usb/devices/2-9:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > lrwxrwxrwx. 1 root root 0 Mar 2 17:02 /sys/bus/usb/devices/2-9/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:25 > > > > > > > > > > So AFAIU the USB device is properly assigned a firmware node. My dsdt > > > > > also shows the "Device (RHUB)" and I guess everything is fine. > > > > > > > > Yes, so far so good. > > > > > > > > > However, playing with qemu is not so easy. > > > > > > > > > > I am running qemu with the following arguments (well, almost because I > > > > > have a wrapper script on top of it and I also run the compiled kernel > > > > > from the current tree): > > > > > > > > > > #> qemu-system-x86_64 -bios /usr/share/edk2/ovmf/OVMF_CODE.fd \ > > > > > -netdev user,id=hostnet0 \ > > > > > -device virtio-net-pci,netdev=hostnet0 \ > > > > > -m 4G \ > > > > > -enable-kvm \ > > > > > -cpu host \ > > > > > -device qemu-xhci -usb \ > > > > > -device 'usb-host,vendorid=0x10c4,productid=0xea90' \ > > > > > -cdrom ~/Downloads/Fedora-Workstation-Live-x86_64-37-1.7.iso > > > > > > > > Side question, where can I get those blobs from (EDKII and Fedora Live CD)? > > > > I'm using Debian unstable. > > > > > > You can install the ovmf package in debian[3], which should have a > > > similar file. > > > For the Fedora livecd -> https://getfedora.org/fr/workstation/download/ > > > but any other distribution with a recent enough kernel should show the > > > same. > > > > Thank you! > > > > > > > And this is what I get: > > > > > > > > > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.* > > > > > lrwxrwxrwx 1 root root 0 Mar 2 16:10 /sys/bus/hid/devices/0003:10C4:EA90.0001 -> ../../../devices/pci0000:00/0000:00:06.0/usb2/2-1/2-1:1.0/0003:10C4:EA90.0001 > > > > > > > > > > #> ls -l /sys/bus/usb/devices/2-1*/firmware_node > > > > > ls: cannot access '/sys/bus/usb/devices/2-1*/firmware_node': No such file or directory > > > > > > > > > > Looking at the DSDT, I do not see any reference to the USB hub, so I > > > > > wonder if the firmware_node needs to be populated first in the DSDT. > > > > > > > > So, where QEMU takes DSDT (ACPI tables in general) from? Can you patch that? > > > > I believe that's the problem in qemu. > > > > > > That's a good question and it's one I am not sure I have the answer to. > > > I would have assumed that the DSDT was in the OVMF firmware, but given > > > that we can arbitrarily add command line arguments, I believe it > > > probably just provides a baseline and then we are screwed. The OVMF bios > > > is compiled only once, so I doubt there is any mechanism to > > > enable/disable a component in the DSDT, or make it dynamically > > > generated. > > > > We have two ways of filling missing parts: > > 1) update the original source of DSDT (firmware or bootloader, > > whichever provides that); > > 2) adding an overlay. > > > > The 2) works _if and only if_ there is *no* existing object in the tables. > > In such cases, you can simply provide a *full* hierarchy. See an example of > > PCI devices in the kernel documentation on how to do that. I believe something > > similar can be done for USB. > > Please find attached the dsdt from the Qemu VM. Thank you! > And after looking at it, your comments below, I think I am understanding > what is happening (on the qemu side at least): > > #> grep PCI0.S /sys/bus/acpi/devices/*/path > /sys/bus/acpi/devices/device:02/path:\_SB_.PCI0.S00_ > /sys/bus/acpi/devices/device:03/path:\_SB_.PCI0.S10_ > /sys/bus/acpi/devices/device:04/path:\_SB_.PCI0.S18_ > /sys/bus/acpi/devices/device:05/path:\_SB_.PCI0.S20_ > /sys/bus/acpi/devices/device:06/path:\_SB_.PCI0.S28_ > /sys/bus/acpi/devices/device:07/path:\_SB_.PCI0.S30_ > /sys/bus/acpi/devices/device:08/path:\_SB_.PCI0.S38_ > /sys/bus/acpi/devices/device:09/path:\_SB_.PCI0.S40_ > /sys/bus/acpi/devices/device:0a/path:\_SB_.PCI0.S48_ > /sys/bus/acpi/devices/device:0b/path:\_SB_.PCI0.S50_ > /sys/bus/acpi/devices/device:0c/path:\_SB_.PCI0.S58_ > /sys/bus/acpi/devices/device:0d/path:\_SB_.PCI0.S60_ > /sys/bus/acpi/devices/device:0e/path:\_SB_.PCI0.S68_ > /sys/bus/acpi/devices/device:0f/path:\_SB_.PCI0.S70_ > /sys/bus/acpi/devices/device:10/path:\_SB_.PCI0.S78_ > /sys/bus/acpi/devices/device:11/path:\_SB_.PCI0.S80_ > /sys/bus/acpi/devices/device:12/path:\_SB_.PCI0.S88_ > /sys/bus/acpi/devices/device:13/path:\_SB_.PCI0.S90_ > /sys/bus/acpi/devices/device:14/path:\_SB_.PCI0.S98_ > /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.SA0_ > /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.SA8_ > /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.SB0_ > /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.SB8_ > /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.SC0_ > /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.SC8_ > /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.SD0_ > /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.SD8_ > /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.SE0_ > /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.SE8_ > /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.SF0_ > /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.SF8_ Ah, not much to get out of it.
Hello,
Sorry about the radio silence from me --
I've been trying to get this working on my end as well.
I was able to get my passed-through USB device on a qemu system to
have a firmware_node by
using the "Upgrading ACPI tables via initrd" kernel feature [1]. In
case this provides helpful information,
the below describes what I did.
This was using the default yocto core-image-minimal image and
qemu-system-x86_64.
I invoke qemu with the convenience "runqemu" script, as follows:
runqemu nographic qemuparams="
-initrd ../acpi-overlay/instrumented_initrd
-device 'usb-host,vendorid=0x10c4,productid=0xea90'
-pflash ./build/tmp/work/core2-64-poky-linux/ovmf/edk2-stable202211-r0/ovmf/ovmf.fd
"
Which invokes qemu with something like the following (sorry about the
long lines..):
qemu-system-x86_64 \
-device virtio-net-pci,netdev=net0,mac=52:54:00:12:34:02 \
-netdev tap,id=net0,ifname=tap0,script=no,downscript=no \
-object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-pci,rng=rng0 \
-drive file=/home/kaehnd/src/local/x86/build/tmp/deploy/images/qemux86-64/core-image-minimal-qemux86-64-20230306143252.rootfs.ext4,if=virtio,format=raw
\
-usb -device usb-tablet -usb -device usb-kbd -cpu IvyBridge \
-machine q35,i8042=off -smp 4 -m 256 \
-device 'usb-host,vendorid=0x10c4,productid=0xea90' \
-serial mon:stdio -serial null -nographic \
-kernel /home/kaehnd/src/local/x86/build/tmp/deploy/images/qemux86-64/bzImage
\
-append 'root=/dev/vda
rw ip=192.168.7.2::192.168.7.1:255.255.255.0::eth0:off:8.8.8.8
console=ttyS0 console=ttyS1 oprofile.timer=1
tsc=reliable no_timer_check rcupdate.rcu_expedited=1 '
The sysfs path tree for the CP2112 was as follows:
#> ls -l /sys/bus/hid/devices/0003:10C4:EA90.0003
lrwxrwxrwx 1 root root 0 Mar 6 19:24
/sys/bus/hid/devices/0003:10C4:EA90.0003 ->
../../../devices/pci0000:00/0000:00:1d.1/usb3/3-1/3-1:1.0/0003:10C4:EA90.0003
Out of the box, firmware_node files existed only through what I assume
is the PCI bus:
/sys/devices/pci0000:00
It's ACPI path:
#> cat /sys/devices/pci0000:00/firmware_node/path
\_SB_.PCI0
Using the instructions at [1], I grabbed the dsdt table, and modified
it as follows.
Underneath the PCI0 node, I added the following ASL
```
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,
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_
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)"
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....
[1] https://www.kernel.org/doc/html/latest/admin-guide/acpi/initrd_table_override.html
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_ > > 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.
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
On Mar 07 2023, Daniel Kaehn wrote: > On Tue, Mar 7, 2023 at 7:17 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > 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. > > > Ah, interesting.. I tried the SSDT override route initially, but tried > applying it > through EFI variables and through configfs, neither of which worked since > they appeared to be applied after the relevant drivers were already loaded > (at least that was my suspicion). I wonder if loading the overlay through the > initramfs was the key. Yeah, there were a few items missing from a "blank" qemu: - loading the SSDT overlay in the initramfs so it's seen before anything else - actually define the description of the device in the DSDT at the matching PCI bus address - your patch :) > > > 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) > > } > > } > > } > > } > > } > > To get this to work -- I assume you had to change the driver to look > for uppercase > "GPIO" and "I2C", or some similar change? > > > > > > 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\ > > > > Ahh, yep, I've had this issue as well -- I suspect the issue you're > having is that the > CP2112 driver initializes the i2c controller before the gpiochip, and > if any i2c devices > on the bus depend on the CP2112's gpio, the probe will never succeed! > I have made > and been testing with a patch to fix this, but since it was midway > through submitting > this series, thought it might be bad practice to "tack on" additional > patches to a patchset > mid-review (since it only causes an issue in some (admittedly fairly > common) use-cases) > so I was going to send it as an individual patch once (if) these were applied. > I don't think this is the issue. When the device is initially probed, the I2C acpi implementation tries to attach an IRQ, but failed at it, returning -EPROBE_DEFER, which makes the device being retried a few times. After I get my shell available I even get the pr_err I included few seconds later, for a last attempt by the kernel to bind it when everything has settled. 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. Note that I tried applying what was describe at Documentation/firmware_guide/acpi/gpio-properties.rst but the _DSD method doesn't seem to be properly applied to the CP2112 GPIO, which is highly suspicious. > If you think that would be necessary to include for these to merge, > I'd be happy to append > it to this review. I also have another patch which adds i2c bus > recovery to the driver, but > that seems independent enough that it should be sent on its own. As I mentioned above I don't think the issue is in the ordering of the I2C vs gpio resources. > > > > > 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. > > More and more, after actually seeing and working with ACPI, I suspect that > you both are right. Maybe (hopefully) though, there is some unified way that can > be made to do this, so that individual drivers won't have to directly code for / > be aware of the differences in the firmware languages (at least, that seemed > to be the intent of the fw_node/device api in the first place). Maybe a > `device_get_child_by_name_or_index` (terribly long name) sort of function > might fill in that gap? I don't know. Though the _DSD documentation I mentioned above looks very similar to what you are describing in the DT case, so maybe the ACPI folks will just tell us "why don't you use XXXX?" and we will have the solution :) (we can't be the first to have that same issue TBH) > > I plan to send an email asking this question more generically to ACPI & DT folks > as Andy suggested, so hopefully there will be some ideas. > > > > > Thanks a lot to both of you, this will be tremendously helpful to me. > > > > Cheers, > > Benjamin > > > Thanks for testing this out! Glad that ACPI support ended up being worked into > this after all :) Cheers, Benjamin
On Tue, Mar 07, 2023 at 07:53:20AM -0600, Daniel Kaehn wrote: > On Tue, Mar 7, 2023 at 7:17 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: ... > More and more, after actually seeing and working with ACPI, I suspect that > you both are right. Maybe (hopefully) though, there is some unified way that can > be made to do this, so that individual drivers won't have to directly code for / > be aware of the differences in the firmware languages (at least, that seemed > to be the intent of the fw_node/device api in the first place). Maybe a > `device_get_child_by_name_or_index` (terribly long name) sort of function > might fill in that gap? Look also at the solution I proposed in response to Benjamin in my previous email.
On Mar 08 2023, Daniel Kaehn wrote: > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > 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"? yes, sorry I should have mentioned it. This is the only modification I have compared to the upstream kernel plus your patch series. > 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). I don't think it's a fwnode issue, but a problem with the assignment of the parent of the gc: --- dev->gc.parent = &hdev->dev; --- Because the function acpi_gpiochip_find() in drivers/gpio/gpiolib-acpi.c compares the acpi handle returned by fetching the ACPI path ("\\_SB_.PCI0.USB0.RHUB.CP21.GPIO") and the one of gc->parent, which in the hid-cp2112 case is the HID device itself. > > 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) Yeah, sorry I should have been explicit about this. For reference, I am appending the full SSDT override which works. Even if you don't have an i2c-hid device connected, this should at least call the probe function in i2c-hid-core.c, which is a proof that the ACPI binding is properly done (the first SMBus read will fail with a timeout) Also, I played around with the _DSD that Andy was mentioning (and some others), hopefully this will help you getting the mapping from the "cell-names" to the fwnode child index faster. Cheers, Benjamin --- 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) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "cell-names", Package () { "i2c", "gpio", } } } }) Device (I2C) { Name (_ADR, Zero) Name (_STA, 0x0F) } Device (GPIO) { Name (_ADR, One) Name (_STA, 0x0F) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "gpio-hog", 1 }, Package () { "gpios", Package () { 4, 0 } }, Package () { "output-high", 1 }, Package () { "line-name", "gpio4-pullup" }, }, 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 (0x2c, 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 } }) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () { "irq-gpios", Package () { ^TPD0, 1, 0, 0 } }, } }) Method(_CRS, 0x0, NotSerialized) { Return (ConcatenateResTemplate (SBFG, SBFB)) } 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 }) } } } } } } ---
On Wed, Mar 08, 2023 at 04:26:11PM +0100, Benjamin Tissoires 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 wonder if the cp2112 driver is correctly assigning the gc->parent > field. Seems it needs custom fwnode gc->fwnode = child_of_cp_which_is_gpio;
On Wed, Mar 8, 2023 at 10:36 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 08, 2023 at 06:30:46PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 08, 2023 at 04:55:27PM +0100, Benjamin Tissoires wrote: > > > On Mar 08 2023, Daniel Kaehn wrote: > > > > On Wed, Mar 8, 2023 at 9:26 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > ... > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > Package () { > > > Package () { "cell-names", Package () { "i2c", "gpio" } > > > } > > > > Yeah, looking at this, I think it still fragile. First of all, either this is > > missing, or simply wrong. We would need to access indices. ACPI _ADR is in the > > specification. As much as with PCI it may be considered reliable. > > > > So, that said, forget about it, and simply use _ADR as indicator of the node. > > See how MFD (in the Linux kernel) cares about this. Ex. Diolan DLN-2 driver. > > And that said, maybe CP2112 should simply re-use what MFD _already_ provides? Great point -- it definitely seems like this driver belongs in the mfd directory to begin with. It seems like aside from rewriting the CP2112 driver into an mfd driver and two platform drivers, my route forward for now would be to just do something like this (not yet tested): + struct acpi_device *adev = ACPI_COMPANION(&hdev->dev); + if (adev) + ACPI_COMPANION_SET(&dev->adap.dev, acpi_find_child_device(adev, 0x0, false)); + else + device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c")); (and the same for the gpiochip) The follow-up question -- does there exist something analogous to DT bindings for ACPI devices, other than the ACPI spec itself, where this should be documented? Or will consumers truly have to read the driver code to determine that _ADR 0 is I2C and _ADR 1 is GPIO? (I haven't seen anything in my search so far -- but knowing that it truly doesn't exist would make me respect people developing embedded ACPI-based systems all the more!) Thanks for your patience in working through all of this, especially considering how long of an email chain this has become! Thanks, Danny Kaehn
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 27cadadda7c9..491e3c83af12 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) u8 buf[3]; struct cp2112_smbus_config_report config; struct gpio_irq_chip *girq; + struct i2c_timings timings; int ret; dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_power_normal; } + device_set_node(&dev->adap.dev, device_get_named_child_node(&hdev->dev, "i2c")); + i2c_parse_fw_timings(&dev->adap.dev, &timings, true); + + config.clock_speed = cpu_to_be32(timings.bus_freq_hz); config.retry_time = cpu_to_be16(1); ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config), @@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) hid_err(hdev, "error setting SMBus config\n"); if (ret >= 0) ret = -EIO; - goto err_power_normal; + goto err_free_i2c_of; } hid_set_drvdata(hdev, (void *)dev); @@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) if (ret) { hid_err(hdev, "error registering i2c adapter\n"); - goto err_power_normal; + goto err_free_i2c_of; } hid_dbg(hdev, "adapter registered\n"); @@ -1336,6 +1341,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) dev->gc.ngpio = 8; dev->gc.can_sleep = 1; dev->gc.parent = &hdev->dev; + dev->gc.fwnode = device_get_named_child_node(&hdev->dev, "gpio"); dev->irq.name = "cp2112-gpio"; dev->irq.irq_startup = cp2112_gpio_irq_startup; @@ -1376,7 +1382,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) err_gpiochip_remove: gpiochip_remove(&dev->gc); err_free_i2c: + fwnode_handle_put(dev->gc.fwnode); i2c_del_adapter(&dev->adap); +err_free_i2c_of: + fwnode_handle_put(dev_fwnode(&dev->adap.dev)); err_power_normal: hid_hw_power(hdev, PM_HINT_NORMAL); err_hid_close: @@ -1391,6 +1400,8 @@ static void cp2112_remove(struct hid_device *hdev) struct cp2112_device *dev = hid_get_drvdata(hdev); int i; + fwnode_handle_put(dev->gc.fwnode); + fwnode_handle_put(dev_fwnode(&dev->adap.dev)); sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group); i2c_del_adapter(&dev->adap);
Bind I2C and GPIO interfaces to subnodes with names "i2c" and "gpio" if they exist, respectively. This allows the GPIO and I2C controllers to be described in firmware as usual. Additionally, support configuring the I2C bus speed from the clock-frequency device property. Signed-off-by: Danny Kaehn <kaehndan@gmail.com> --- drivers/hid/hid-cp2112.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)