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@xxxxxxxxxxxxxxx> 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
