[PATCH v8 0/3] Firmware Support for USB-HID Devices and CP2112

[PATCH v8 0/3] Firmware Support for USB-HID Devices and CP2112

[Danny Kaehn]

This patchset allows USB-HID devices to have DeviceTree bindings through sharing
the USB fwnode with the HID driver, and adds such a binding and driver
implementation for the CP2112 USB to SMBus Bridge (which necessitated the
USB-HID change). This change allows a CP2112 permanently attached in hardware to
be described in DT and interoperate with other drivers.

Thanks to all for your patience in working with me on these! I'll have
many lessons learned for future submissions.

Changes in v8:
- Apply Review tags retroactively to patches previously reviewed

Changes in v7:
- Use dev_fwnode when calling fwnod_handle_put in i2c_adapter in hid-cp2112.c
- Capitalize I2C and GPIO in commit message for patch 0003

Changes in v6:
- Fix fwnode_handle reference leaks in hid-cp21112.c
- Simplify hog node pattern in silabs,cp2112.yaml

Changes in v5:
 - Use fwnode API instead of of_node api in hid-core.c and hid-cp2112.c
 - Include sda-gpios and scl-gpios in silabs,cp2112.yaml
 - Additional fixups to silabs,cp2112.yaml to address comments
 - Submit threaded interrupt bugfix separately from this patchset, as requested

Changes in v4:
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/i2c

Changes in v3:
 - Additional fixups to silabs,cp2112.yaml to address comments

Changes in v2:
 - Added more detail to silabs,cp2112.yaml dt-binding
 - Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/input
 - Added support for setting smbus clock-frequency from DT in hid-cp2112.c
 - Added freeing of of_nodes on error paths of _probe in hid-cp2112.c

Danny Kaehn (3):
  dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
  HID: usbhid: Share USB device firmware node with child HID device
  HID: cp2112: Fwnode Support

 .../bindings/i2c/silabs,cp2112.yaml           | 113 ++++++++++++++++++
 drivers/hid/hid-cp2112.c                      |  15 ++-
 drivers/hid/usbhid/hid-core.c                 |   2 +
 3 files changed, 128 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

-- 
2.25.1

[PATCH v8 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge

[Danny Kaehn]

This is a USB HID device which includes an I2C controller and 8 GPIO pins.

The binding allows describing the chip's gpio and i2c controller in DT
using the subnodes named "gpio" and "i2c", respectively. This is
intended to be used in configurations where the CP2112 is permanently
connected in hardware.

Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/i2c/silabs,cp2112.yaml           | 113 ++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..a27509627804
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/silabs,cp2112.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CP2112 HID USB to SMBus/I2C Bridge
+
+maintainers:
+  - Danny Kaehn <kaehndan@gmail.com>
+
+description:
+  The CP2112 is a USB HID device which includes an integrated I2C controller
+  and 8 GPIO pins. Its GPIO pins can each be configured as inputs, open-drain
+  outputs, or push-pull outputs.
+
+properties:
+  compatible:
+    const: usb10c4,ea90
+
+  reg:
+    maxItems: 1
+    description: The USB port number on the host controller
+
+  i2c:
+    description: The SMBus/I2C controller node for the CP2112
+    $ref: /schemas/i2c/i2c-controller.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      sda-gpios:
+        maxItems: 1
+
+      scl-gpios:
+        maxItems: 1
+
+      clock-frequency:
+        minimum: 10000
+        default: 100000
+        maximum: 400000
+
+  gpio:
+    description: The GPIO controller node for the CP2112
+    type: object
+    unevaluatedProperties: false
+
+    properties:
+      interrupt-controller: true
+      "#interrupt-cells":
+        const: 2
+
+      gpio-controller: true
+      "#gpio-cells":
+        const: 2
+
+      gpio-line-names:
+        minItems: 1
+        maxItems: 8
+
+    patternProperties:
+      "-hog(-[0-9]+)?$":
+        type: object
+
+        required:
+          - gpio-hog
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    usb {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      device@1 {
+        compatible = "usb10c4,ea90";
+        reg = <1>;
+
+        i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          sda-gpios = <&cp2112_gpio 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+          scl-gpios = <&cp2112_gpio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+          temp@48 {
+            compatible = "national,lm75";
+            reg = <0x48>;
+          };
+        };
+
+        cp2112_gpio: gpio {
+          gpio-controller;
+          interrupt-controller;
+          #gpio-cells = <2>;
+          gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
+            "TEST3","TEST4", "TEST5", "TEST6";
+
+          fan-rst-hog {
+              gpio-hog;
+              gpios = <7 GPIO_ACTIVE_HIGH>;
+              output-high;
+              line-name = "FAN_RST";
+          };
+        };
+      };
+    };
-- 
2.25.1

[PATCH v8 2/3] HID: usbhid: Share USB device firmware node with child HID device

[Danny Kaehn]

USB HID core now shares its fwnode with its child HID device.
Since there can only be one HID device on a USB interface, it is redundant
to specify a hid node under the USB device. This allows usb HID device
drivers to be described in firmware and make use of device properties.

Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/usbhid/hid-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 257dd73e37bf..090260d99c84 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/spinlock.h>
 #include <asm/unaligned.h>
 #include <asm/byteorder.h>
@@ -1374,6 +1375,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 	hid->hiddev_report_event = hiddev_report_event;
 #endif
 	hid->dev.parent = &intf->dev;
+	device_set_node(&hid->dev, dev_fwnode(&intf->dev));
 	hid->bus = BUS_USB;
 	hid->vendor = le16_to_cpu(dev->descriptor.idVendor);
 	hid->product = le16_to_cpu(dev->descriptor.idProduct);
-- 
2.25.1

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

[Danny Kaehn]

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(-)

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);
 
-- 
2.25.1

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

[Andy Shevchenko]

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.

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?

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

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

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

[Andy Shevchenko]

+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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Mar 01, 2023 at 04:59:07PM +0200, Andy Shevchenko wrote:
> 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.

Forgot to mention, that you need to take into consideration the drivers that are:
1) can be compiled with CONFIG_OF=n (i.e. using agnostic APIs);
2) having OF ID table;
3) being platform independent (no Kconfig dependency to something that's known
   to be an ARCH/CPU/etc one).

All of those can be instantiated in ACPI via special PRP0001 ACPI ID [2].

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

[2]: https://docs.kernel.org/firmware-guide/acpi/enumeration.html#device-tree-namespace-link-device-id

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

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

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

[Andy Shevchenko]

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.

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

> Also note that if I plug the CP2112 over a docking station, I lose the
> firmware_node sysfs entries on the host too.

This seems like a lack of firmware node propagating in the USB hub code in
the Linux kernel.

> Do you think it would be achievable to emulate that over qemu and use a
> mainline kernel without patches?

As long as qemu provides correct DSDT it should work I assume.

> [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

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

>
> > Also note that if I plug the CP2112 over a docking station, I lose the
> > firmware_node sysfs entries on the host too.
>
> This seems like a lack of firmware node propagating in the USB hub code in
> the Linux kernel.

That would make a lot of sense.

FWIW, in the VM I see a firmware node on the pci controller itself:
#> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node
   lrwxrwxrwx 1 root root 0 Mar  6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07

And one the host, through a USB hub:

#> ls -l /sys/bus/hid/devices/0003:10C4:EA90.*
   lrwxrwxrwx. 1 root root 0 Mar  6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C
#> ls -l /sys/bus/usb/devices/2-8*/firmware_node
   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e

Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not
having a firmware node.


>
> > Do you think it would be achievable to emulate that over qemu and use a
> > mainline kernel without patches?
>
> As long as qemu provides correct DSDT it should work I assume.

Just to be sure I understand, for this to work, we need the DSDT to
export a "Device(RHUB)"?
Or if we fix the USB fw_node propagation, could we just overwrite
"\_SB_.PCI0.S30_"?  "\_SB_.PCI0.S30_" is the name the ACPI is giving to
the USB port in my VM case AFAIU.

Cheers,
Benjamin

>
> > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM

[3] https://packages.debian.org/buster/all/ovmf/filelist

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

[Andy Shevchenko]

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.

> > > Also note that if I plug the CP2112 over a docking station, I lose the
> > > firmware_node sysfs entries on the host too.
> > 
> > This seems like a lack of firmware node propagating in the USB hub code in
> > the Linux kernel.
> 
> That would make a lot of sense.
> 
> FWIW, in the VM I see a firmware node on the pci controller itself:
> #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node
>   lrwxrwxrwx 1 root root 0 Mar  6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07
> 
> And one the host, through a USB hub:
> 
> #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.*
>   lrwxrwxrwx. 1 root root 0 Mar  6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C
> #> ls -l /sys/bus/usb/devices/2-8*/firmware_node
>   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
>   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> 
> Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not
> having a firmware node.

It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`,
filter out unneeded ones, and for the rest also print their paths:
`cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done`

With this we will see what devices are actually present and up and running
in the system and what their paths in the ACPI namespace.

> > > Do you think it would be achievable to emulate that over qemu and use a
> > > mainline kernel without patches?
> > 
> > As long as qemu provides correct DSDT it should work I assume.
> 
> Just to be sure I understand, for this to work, we need the DSDT to
> export a "Device(RHUB)"?

Not sure I understand the term "export" here. We need a description
of the (to describe) missing parts.

> Or if we fix the USB fw_node propagation, could we just overwrite
> "\_SB_.PCI0.S30_"?  "\_SB_.PCI0.S30_" is the name the ACPI is giving to
> the USB port in my VM case AFAIU.

I have no idea what is the S30 node.

[2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
[3] https://packages.debian.org/buster/all/ovmf/filelist

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

[-- Attachment #1: Type: text/plain, Size: 13762 bytes --]



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.


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_

And those translate on the DSDT as (for the S30/S38 chunk I am
interested in):

             Device (S30)
             {
                 Name (_ADR, 0x00060000)  // _ADR: Address
                 Name (ASUN, 0x06)
                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                 {
                     Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                 }

                 Name (_SUN, 0x06)  // _SUN: Slot User Number
                 Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                 {
                     PCEJ (BSEL, _SUN)
                 }
             }

             Device (S38)
             {
                 Name (_ADR, 0x00070000)  // _ADR: Address
                 Name (ASUN, 0x07)
                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                 {
                     Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                 }

                 Name (_SUN, 0x07)  // _SUN: Slot User Number
                 Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                 {
                     PCEJ (BSEL, _SUN)
                 }
             }

The forwarded USB node is actually on device:07 -> S30_, and as much as
I'd like it to be a regular USB hub, this looks like a virtio node entry
that allows to forward a physical device to the VM.

So IMO, the missing piece might rely on the virtio-usb code which
doesn't export the firmware node, which means I can not extend the
device with an SSDT overlay ATM because the USB node doesn't have the
fw_node.

>
> > > > Also note that if I plug the CP2112 over a docking station, I lose the
> > > > firmware_node sysfs entries on the host too.
> > >
> > > This seems like a lack of firmware node propagating in the USB hub code in
> > > the Linux kernel.
> >
> > That would make a lot of sense.
> >
> > FWIW, in the VM I see a firmware node on the pci controller itself:
> > #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node
> >   lrwxrwxrwx 1 root root 0 Mar  6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07
> >
> > And one the host, through a USB hub:
> >
> > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.*
> >   lrwxrwxrwx. 1 root root 0 Mar  6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C
> > #> ls -l /sys/bus/usb/devices/2-8*/firmware_node
> >   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> >   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> >
> > Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not
> > having a firmware node.
>
> It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`,

This command (both on the host and on the VM) does not show any USB
device or even the PCI USB controller itself (PNP0A08 or PNP0A03).

> filter out unneeded ones, and for the rest also print their paths:
> `cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done`

see above for the VM case, and in the host:

#> grep XHC /sys/bus/acpi/devices/*/path
/sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.XHC_
/sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.XHC_.RHUB
/sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.XHC_.RHUB.HS01
/sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.XHC_.RHUB.HS02
/sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.XHC_.RHUB.HS03
/sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.XHC_.RHUB.HS04
/sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.XHC_.RHUB.HS05
/sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.XHC_.RHUB.HS06
/sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.XHC_.RHUB.HS07
/sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.XHC_.RHUB.HS08
/sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.XHC_.RHUB.SS01
/sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.XHC_.RHUB.SS02
/sys/bus/acpi/devices/device:21/path:\_SB_.PCI0.XHC_.RHUB.SS03
/sys/bus/acpi/devices/device:22/path:\_SB_.PCI0.XHC_.RHUB.SS04
/sys/bus/acpi/devices/device:23/path:\_SB_.PCI0.XHC_.RHUB.SS05
/sys/bus/acpi/devices/device:24/path:\_SB_.PCI0.XHC_.RHUB.SS06
/sys/bus/acpi/devices/device:25/path:\_SB_.PCI0.XHC_.RHUB.HS09
/sys/bus/acpi/devices/device:26/path:\_SB_.PCI0.XHC_.RHUB.HS10
/sys/bus/acpi/devices/device:27/path:\_SB_.PCI0.XHC_.RHUB.USR1
/sys/bus/acpi/devices/device:28/path:\_SB_.PCI0.XHC_.RHUB.USR2
/sys/bus/acpi/devices/device:85/path:\_SB_.PCI0.TXHC
/sys/bus/acpi/devices/device:86/path:\_SB_.PCI0.TXHC.RHUB
/sys/bus/acpi/devices/device:87/path:\_SB_.PCI0.TXHC.RHUB.HS01
/sys/bus/acpi/devices/device:88/path:\_SB_.PCI0.TXHC.RHUB.SS01
/sys/bus/acpi/devices/device:89/path:\_SB_.PCI0.TXHC.RHUB.SS02
/sys/bus/acpi/devices/device:8a/path:\_SB_.PCI0.TXHC.RHUB.SS03
/sys/bus/acpi/devices/device:8b/path:\_SB_.PCI0.TXHC.RHUB.SS04

Which is coherent with the ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
I get when looking at the USB port.


>
> With this we will see what devices are actually present and up and running
> in the system and what their paths in the ACPI namespace.

So it seems that the USB hub functionality is not creating fw_nodes for
its children. But I am not sure this is a battle we want to fight right
now, because it doesn't make a lot of sense IMO to add an SSDT overlay
on a hub.

>
> > > > Do you think it would be achievable to emulate that over qemu and use a
> > > > mainline kernel without patches?
> > >
> > > As long as qemu provides correct DSDT it should work I assume.
> >
> > Just to be sure I understand, for this to work, we need the DSDT to
> > export a "Device(RHUB)"?
>
> Not sure I understand the term "export" here. We need a description
> of the (to describe) missing parts.

Yes, I meant "to describe" it.

>
> > Or if we fix the USB fw_node propagation, could we just overwrite
> > "\_SB_.PCI0.S30_"?  "\_SB_.PCI0.S30_" is the name the ACPI is giving to
> > the USB port in my VM case AFAIU.
>
> I have no idea what is the S30 node.

That gave me the hint I needed, I think. The problem must be in the
virtio drivers, where it doesn't attach the fw_node to the components it
creates. We probably need kind of the same patch Danny is sending in 2/3
in this series, but for virtio.

Cheers,
Benjamin

>
> [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
> [3] https://packages.debian.org/buster/all/ovmf/filelist
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

[-- Attachment #2: dsdt.dsl --]
[-- Type: text/x-dsl, Size: 49650 bytes --]

/*
 * Intel ACPI Component Architecture
 * AML/ASL+ Disassembler version 20220331 (64-bit version)
 * Copyright (c) 2000 - 2022 Intel Corporation
 * 
 * Disassembling to symbolic ASL+ operators
 *
 * Disassembly of dsdt.dat, Thu Mar  2 15:16:39 2023
 *
 * Original Table Header:
 *     Signature        "DSDT"
 *     Length           0x00001795 (6037)
 *     Revision         0x01 **** 32-bit table (V1), no 64-bit math support
 *     Checksum         0xDE
 *     OEM ID           "BOCHS "
 *     OEM Table ID     "BXPC    "
 *     OEM Revision     0x00000001 (1)
 *     Compiler ID      "BXPC"
 *     Compiler Version 0x00000001 (1)
 */
DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPC    ", 0x00000001)
{
    Scope (\)
    {
        OperationRegion (DBG, SystemIO, 0x0402, One)
        Field (DBG, ByteAcc, NoLock, Preserve)
        {
            DBGB,   8
        }

        Method (DBUG, 1, NotSerialized)
        {
            ToHexString (Arg0, Local0)
            ToBuffer (Local0, Local0)
            Local1 = (SizeOf (Local0) - One)
            Local2 = Zero
            While ((Local2 < Local1))
            {
                DBGB = DerefOf (Local0 [Local2])
                Local2++
            }

            DBGB = 0x0A
        }
    }

    Scope (_SB)
    {
        Device (PCI0)
        {
            Name (_HID, EisaId ("PNP0A03") /* PCI Bus */)  // _HID: Hardware ID
            Name (_ADR, Zero)  // _ADR: Address
            Name (_UID, Zero)  // _UID: Unique ID
        }
    }

    Scope (_SB)
    {
        Device (HPET)
        {
            Name (_HID, EisaId ("PNP0103") /* HPET System Timer */)  // _HID: Hardware ID
            Name (_UID, Zero)  // _UID: Unique ID
            OperationRegion (HPTM, SystemMemory, 0xFED00000, 0x0400)
            Field (HPTM, DWordAcc, Lock, Preserve)
            {
                VEND,   32, 
                PRD,    32
            }

            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Local0 = VEND /* \_SB_.HPET.VEND */
                Local1 = PRD /* \_SB_.HPET.PRD_ */
                Local0 >>= 0x10
                If (((Local0 == Zero) || (Local0 == 0xFFFF)))
                {
                    Return (Zero)
                }

                If (((Local1 == Zero) || (Local1 > 0x05F5E100)))
                {
                    Return (Zero)
                }

                Return (0x0F)
            }

            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                Memory32Fixed (ReadOnly,
                    0xFED00000,         // Address Base
                    0x00000400,         // Address Length
                    )
            })
        }
    }

    Scope (_SB.PCI0)
    {
        Device (ISA)
        {
            Name (_ADR, 0x00010000)  // _ADR: Address
            OperationRegion (P40C, PCI_Config, 0x60, 0x04)
        }
    }

    Scope (_SB.PCI0.ISA)
    {
        Device (KBD)
        {
            Name (_HID, EisaId ("PNP0303") /* IBM Enhanced Keyboard (101/102-key, PS/2 Mouse) */)  // _HID: Hardware ID
            Name (_STA, 0x0F)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0060,             // Range Minimum
                    0x0060,             // Range Maximum
                    0x01,               // Alignment
                    0x01,               // Length
                    )
                IO (Decode16,
                    0x0064,             // Range Minimum
                    0x0064,             // Range Maximum
                    0x01,               // Alignment
                    0x01,               // Length
                    )
                IRQNoFlags ()
                    {1}
            })
        }

        Device (MOU)
        {
            Name (_HID, EisaId ("PNP0F13") /* PS/2 Mouse */)  // _HID: Hardware ID
            Name (_STA, 0x0F)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IRQNoFlags ()
                    {12}
            })
        }

        Device (FDC0)
        {
            Name (_HID, EisaId ("PNP0700"))  // _HID: Hardware ID
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x03F2,             // Range Minimum
                    0x03F2,             // Range Maximum
                    0x00,               // Alignment
                    0x04,               // Length
                    )
                IO (Decode16,
                    0x03F7,             // Range Minimum
                    0x03F7,             // Range Maximum
                    0x00,               // Alignment
                    0x01,               // Length
                    )
                IRQNoFlags ()
                    {6}
                DMA (Compatibility, NotBusMaster, Transfer8, )
                    {2}
            })
            Device (FLPA)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (_FDI, Package (0x10)  // _FDI: Floppy Drive Information
                {
                    Zero, 
                    0x05, 
                    0x4F, 
                    0x30, 
                    One, 
                    0xAF, 
                    0x02, 
                    0x25, 
                    0x02, 
                    0x12, 
                    0x1B, 
                    0xFF, 
                    0x6C, 
                    0xF6, 
                    0x0F, 
                    0x08
                })
            }

            Name (_FDE, Buffer (0x14)  // _FDE: Floppy Disk Enumerate
            {
                /* 0000 */  0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  // ........
                /* 0008 */  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  // ........
                /* 0010 */  0x02, 0x00, 0x00, 0x00                           // ....
            })
        }

        Device (LPT1)
        {
            Name (_HID, EisaId ("PNP0400") /* Standard LPT Parallel Port */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_STA, 0x0F)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0378,             // Range Minimum
                    0x0378,             // Range Maximum
                    0x08,               // Alignment
                    0x08,               // Length
                    )
                IRQNoFlags ()
                    {7}
            })
        }

        Device (COM1)
        {
            Name (_HID, EisaId ("PNP0501") /* 16550A-compatible COM Serial Port */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_STA, 0x0F)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x03F8,             // Range Minimum
                    0x03F8,             // Range Maximum
                    0x00,               // Alignment
                    0x08,               // Length
                    )
                IRQNoFlags ()
                    {4}
            })
        }

        Device (RTC)
        {
            Name (_HID, EisaId ("PNP0B00") /* AT Real-Time Clock */)  // _HID: Hardware ID
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0070,             // Range Minimum
                    0x0070,             // Range Maximum
                    0x01,               // Alignment
                    0x08,               // Length
                    )
                IRQNoFlags ()
                    {8}
            })
        }
    }

    Scope (_SB.PCI0)
    {
        OperationRegion (PCST, SystemIO, 0xAE00, 0x08)
        Field (PCST, DWordAcc, NoLock, WriteAsZeros)
        {
            PCIU,   32, 
            PCID,   32
        }

        OperationRegion (SEJ, SystemIO, 0xAE08, 0x04)
        Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
        {
            B0EJ,   32
        }

        OperationRegion (BNMR, SystemIO, 0xAE10, 0x08)
        Field (BNMR, DWordAcc, NoLock, WriteAsZeros)
        {
            BNUM,   32, 
            PIDX,   32
        }

        Mutex (BLCK, 0x00)
        Method (PCEJ, 2, NotSerialized)
        {
            Acquire (BLCK, 0xFFFF)
            BNUM = Arg0
            B0EJ = (One << Arg1)
            Release (BLCK)
            Return (Zero)
        }

        Method (AIDX, 2, NotSerialized)
        {
            Acquire (BLCK, 0xFFFF)
            BNUM = Arg0
            PIDX = (One << Arg1)
            Local0 = PIDX /* \_SB_.PCI0.PIDX */
            Release (BLCK)
            Return (Local0)
        }

        Method (PDSM, 6, Serialized)
        {
            If ((Arg0 == ToUUID ("e5c937d0-3553-4d7a-9117-ea4d19c3434d") /* Device Labeling Interface */))
            {
                Local0 = AIDX (Arg4, Arg5)
                If ((Arg2 == Zero))
                {
                    If ((Arg1 == 0x02))
                    {
                        If (!((Local0 == Zero) | (Local0 == 0xFFFFFFFF)))
                        {
                            Return (Buffer (One)
                            {
                                 0x81                                             // .
                            })
                        }
                    }

                    Return (Buffer (One)
                    {
                         0x00                                             // .
                    })
                }
                ElseIf ((Arg2 == 0x07))
                {
                    Local1 = Package (0x02)
                        {
                            Zero, 
                            ""
                        }
                    Local1 [Zero] = Local0
                    Return (Local1)
                }
            }
        }
    }

    Scope (_SB)
    {
        Scope (PCI0)
        {
            Method (_PRT, 0, NotSerialized)  // _PRT: PCI Routing Table
            {
                Local0 = Package (0x80) {}
                Local1 = Zero
                While ((Local1 < 0x80))
                {
                    Local2 = (Local1 >> 0x02)
                    Local3 = ((Local1 + Local2) & 0x03)
                    If ((Local3 == Zero))
                    {
                        Local4 = Package (0x04)
                            {
                                Zero, 
                                Zero, 
                                LNKD, 
                                Zero
                            }
                    }

                    If ((Local3 == One))
                    {
                        If ((Local1 == 0x04))
                        {
                            Local4 = Package (0x04)
                                {
                                    Zero, 
                                    Zero, 
                                    LNKS, 
                                    Zero
                                }
                        }
                        Else
                        {
                            Local4 = Package (0x04)
                                {
                                    Zero, 
                                    Zero, 
                                    LNKA, 
                                    Zero
                                }
                        }
                    }

                    If ((Local3 == 0x02))
                    {
                        Local4 = Package (0x04)
                            {
                                Zero, 
                                Zero, 
                                LNKB, 
                                Zero
                            }
                    }

                    If ((Local3 == 0x03))
                    {
                        Local4 = Package (0x04)
                            {
                                Zero, 
                                Zero, 
                                LNKC, 
                                Zero
                            }
                    }

                    Local4 [Zero] = ((Local2 << 0x10) | 0xFFFF)
                    Local4 [One] = (Local1 & 0x03)
                    Local0 [Local1] = Local4
                    Local1++
                }

                Return (Local0)
            }
        }

        Field (PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
        {
            PRQ0,   8, 
            PRQ1,   8, 
            PRQ2,   8, 
            PRQ3,   8
        }

        Method (IQST, 1, NotSerialized)
        {
            If ((0x80 & Arg0))
            {
                Return (0x09)
            }

            Return (0x0B)
        }

        Method (IQCR, 1, Serialized)
        {
            Name (PRR0, ResourceTemplate ()
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, _Y00)
                {
                    0x00000000,
                }
            })
            CreateDWordField (PRR0, \_SB.IQCR._Y00._INT, PRRI)  // _INT: Interrupts
            If ((Arg0 < 0x80))
            {
                PRRI = Arg0
            }

            Return (PRR0) /* \_SB_.IQCR.PRR0 */
        }

        Device (LNKA)
        {
            Name (_HID, EisaId ("PNP0C0F") /* PCI Interrupt Link Device */)  // _HID: Hardware ID
            Name (_UID, Zero)  // _UID: Unique ID
            Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
                {
                    0x00000005,
                    0x0000000A,
                    0x0000000B,
                }
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (IQST (PRQ0))
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
                PRQ0 |= 0x80
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Return (IQCR (PRQ0))
            }

            Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
            {
                CreateDWordField (Arg0, 0x05, PRRI)
                PRQ0 = PRRI /* \_SB_.LNKA._SRS.PRRI */
            }
        }

        Device (LNKB)
        {
            Name (_HID, EisaId ("PNP0C0F") /* PCI Interrupt Link Device */)  // _HID: Hardware ID
            Name (_UID, One)  // _UID: Unique ID
            Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
                {
                    0x00000005,
                    0x0000000A,
                    0x0000000B,
                }
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (IQST (PRQ1))
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
                PRQ1 |= 0x80
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Return (IQCR (PRQ1))
            }

            Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
            {
                CreateDWordField (Arg0, 0x05, PRRI)
                PRQ1 = PRRI /* \_SB_.LNKB._SRS.PRRI */
            }
        }

        Device (LNKC)
        {
            Name (_HID, EisaId ("PNP0C0F") /* PCI Interrupt Link Device */)  // _HID: Hardware ID
            Name (_UID, 0x02)  // _UID: Unique ID
            Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
                {
                    0x00000005,
                    0x0000000A,
                    0x0000000B,
                }
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (IQST (PRQ2))
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
                PRQ2 |= 0x80
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Return (IQCR (PRQ2))
            }

            Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
            {
                CreateDWordField (Arg0, 0x05, PRRI)
                PRQ2 = PRRI /* \_SB_.LNKC._SRS.PRRI */
            }
        }

        Device (LNKD)
        {
            Name (_HID, EisaId ("PNP0C0F") /* PCI Interrupt Link Device */)  // _HID: Hardware ID
            Name (_UID, 0x03)  // _UID: Unique ID
            Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
                {
                    0x00000005,
                    0x0000000A,
                    0x0000000B,
                }
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (IQST (PRQ3))
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
                PRQ3 |= 0x80
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Return (IQCR (PRQ3))
            }

            Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
            {
                CreateDWordField (Arg0, 0x05, PRRI)
                PRQ3 = PRRI /* \_SB_.LNKD._SRS.PRRI */
            }
        }

        Device (LNKS)
        {
            Name (_HID, EisaId ("PNP0C0F") /* PCI Interrupt Link Device */)  // _HID: Hardware ID
            Name (_UID, 0x04)  // _UID: Unique ID
            Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
            {
                Interrupt (ResourceConsumer, Level, ActiveHigh, Shared, ,, )
                {
                    0x00000009,
                }
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0B)
            }

            Method (_DIS, 0, NotSerialized)  // _DIS: Disable Device
            {
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Return (_PRS) /* \_SB_.LNKS._PRS */
            }

            Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
            {
            }
        }
    }

    Scope (_SB)
    {
        Device (\_SB.PCI0.PRES)
        {
            Name (_HID, EisaId ("PNP0A06") /* Generic Container Device */)  // _HID: Hardware ID
            Name (_UID, "CPU Hotplug resources")  // _UID: Unique ID
            Mutex (CPLK, 0x00)
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0xAF00,             // Range Minimum
                    0xAF00,             // Range Maximum
                    0x01,               // Alignment
                    0x0C,               // Length
                    )
            })
            OperationRegion (PRST, SystemIO, 0xAF00, 0x0C)
            Field (PRST, ByteAcc, NoLock, WriteAsZeros)
            {
                Offset (0x04), 
                CPEN,   1, 
                CINS,   1, 
                CRMV,   1, 
                CEJ0,   1, 
                CEJF,   1, 
                Offset (0x05), 
                CCMD,   8
            }

            Field (PRST, DWordAcc, NoLock, Preserve)
            {
                CSEL,   32, 
                Offset (0x08), 
                CDAT,   32
            }

            Method (_INI, 0, Serialized)  // _INI: Initialize
            {
                CSEL = Zero
            }
        }

        Device (\_SB.CPUS)
        {
            Name (_HID, "ACPI0010" /* Processor Container Device */)  // _HID: Hardware ID
            Name (_CID, EisaId ("PNP0A05") /* Generic Container Device */)  // _CID: Compatible ID
            Method (CTFY, 2, NotSerialized)
            {
                If ((Arg0 == Zero))
                {
                    Notify (C000, Arg1)
                }
            }

            Method (CSTA, 1, Serialized)
            {
                Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
                \_SB.PCI0.PRES.CSEL = Arg0
                Local0 = Zero
                If ((\_SB.PCI0.PRES.CPEN == One))
                {
                    Local0 = 0x0F
                }

                Release (\_SB.PCI0.PRES.CPLK)
                Return (Local0)
            }

            Method (CEJ0, 1, Serialized)
            {
                Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
                \_SB.PCI0.PRES.CSEL = Arg0
                \_SB.PCI0.PRES.CEJ0 = One
                Release (\_SB.PCI0.PRES.CPLK)
            }

            Method (CSCN, 0, Serialized)
            {
                Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
                Name (CNEW, Package (0xFF) {})
                Local3 = Zero
                Local4 = One
                While ((Local4 == One))
                {
                    Local4 = Zero
                    Local0 = One
                    Local1 = Zero
                    While (((Local0 == One) && (Local3 < One)))
                    {
                        Local0 = Zero
                        \_SB.PCI0.PRES.CSEL = Local3
                        \_SB.PCI0.PRES.CCMD = Zero
                        If ((\_SB.PCI0.PRES.CDAT < Local3))
                        {
                            Break
                        }

                        If ((Local1 == 0xFF))
                        {
                            Local4 = One
                            Break
                        }

                        Local3 = \_SB.PCI0.PRES.CDAT
                        If ((\_SB.PCI0.PRES.CINS == One))
                        {
                            CNEW [Local1] = Local3
                            Local1++
                            Local0 = One
                        }
                        ElseIf ((\_SB.PCI0.PRES.CRMV == One))
                        {
                            CTFY (Local3, 0x03)
                            \_SB.PCI0.PRES.CRMV = One
                            Local0 = One
                        }

                        Local3++
                    }

                    Local2 = Zero
                    While ((Local2 < Local1))
                    {
                        Local3 = DerefOf (CNEW [Local2])
                        CTFY (Local3, One)
                        Debug = Local3
                        \_SB.PCI0.PRES.CSEL = Local3
                        \_SB.PCI0.PRES.CINS = One
                        Local2++
                    }
                }

                Release (\_SB.PCI0.PRES.CPLK)
            }

            Method (COST, 4, Serialized)
            {
                Acquire (\_SB.PCI0.PRES.CPLK, 0xFFFF)
                \_SB.PCI0.PRES.CSEL = Arg0
                \_SB.PCI0.PRES.CCMD = One
                \_SB.PCI0.PRES.CDAT = Arg1
                \_SB.PCI0.PRES.CCMD = 0x02
                \_SB.PCI0.PRES.CDAT = Arg2
                Release (\_SB.PCI0.PRES.CPLK)
            }

            Processor (C000, 0x00, 0x00000000, 0x00)
            {
                Method (_STA, 0, Serialized)  // _STA: Status
                {
                    Return (CSTA (Zero))
                }

                Name (_MAT, Buffer (0x08)  // _MAT: Multiple APIC Table Entry
                {
                     0x00, 0x08, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00   // ........
                })
                Method (_OST, 3, Serialized)  // _OST: OSPM Status Indication
                {
                    COST (Zero, Arg0, Arg1, Arg2)
                }
            }
        }
    }

    Method (\_GPE._E02, 0, NotSerialized)  // _Exx: Edge-Triggered GPE, xx=0x00-0xFF
    {
        \_SB.CPUS.CSCN ()
    }

    Scope (_GPE)
    {
        Name (_HID, "ACPI0006" /* GPE Block Device */)  // _HID: Hardware ID
        Method (_E01, 0, NotSerialized)  // _Exx: Edge-Triggered GPE, xx=0x00-0xFF
        {
            Acquire (\_SB.PCI0.BLCK, 0xFFFF)
            \_SB.PCI0.PCNT ()
            Release (\_SB.PCI0.BLCK)
        }
    }

    Scope (\_SB.PCI0)
    {
        Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
        {
            WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
                0x0000,             // Granularity
                0x0000,             // Range Minimum
                0x00FF,             // Range Maximum
                0x0000,             // Translation Offset
                0x0100,             // Length
                ,, )
            IO (Decode16,
                0x0CF8,             // Range Minimum
                0x0CF8,             // Range Maximum
                0x01,               // Alignment
                0x08,               // Length
                )
            WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
                0x0000,             // Granularity
                0x0000,             // Range Minimum
                0x0CF7,             // Range Maximum
                0x0000,             // Translation Offset
                0x0CF8,             // Length
                ,, , TypeStatic, DenseTranslation)
            WordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
                0x0000,             // Granularity
                0x0D00,             // Range Minimum
                0xFFFF,             // Range Maximum
                0x0000,             // Translation Offset
                0xF300,             // Length
                ,, , TypeStatic, DenseTranslation)
            DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite,
                0x00000000,         // Granularity
                0x000A0000,         // Range Minimum
                0x000BFFFF,         // Range Maximum
                0x00000000,         // Translation Offset
                0x00020000,         // Length
                ,, , AddressRangeMemory, TypeStatic)
            DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                0x00000000,         // Granularity
                0xC0000000,         // Range Minimum
                0xFEBFFFFF,         // Range Maximum
                0x00000000,         // Translation Offset
                0x3EC00000,         // Length
                ,, , AddressRangeMemory, TypeStatic)
            QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite,
                0x0000000000000000, // Granularity
                0x0000000140000000, // Range Minimum
                0x00000001BFFFFFFF, // Range Maximum
                0x0000000000000000, // Translation Offset
                0x0000000080000000, // Length
                ,, , AddressRangeMemory, TypeStatic)
        })
        Device (GPE0)
        {
            Name (_HID, "PNP0A06" /* Generic Container Device */)  // _HID: Hardware ID
            Name (_UID, "GPE0 resources")  // _UID: Unique ID
            Name (_STA, 0x0B)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0xAFE0,             // Range Minimum
                    0xAFE0,             // Range Maximum
                    0x01,               // Alignment
                    0x04,               // Length
                    )
            })
        }

        Device (PHPR)
        {
            Name (_HID, "PNP0A06" /* Generic Container Device */)  // _HID: Hardware ID
            Name (_UID, "PCI Hotplug resources")  // _UID: Unique ID
            Name (_STA, 0x0B)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0xAE00,             // Range Minimum
                    0xAE00,             // Range Maximum
                    0x01,               // Alignment
                    0x18,               // Length
                    )
            })
        }
    }

    Scope (\)
    {
        Name (_S3, Package (0x04)  // _S3_: S3 System State
        {
            One, 
            One, 
            Zero, 
            Zero
        })
        Name (_S4, Package (0x04)  // _S4_: S4 System State
        {
            0x02, 
            0x02, 
            Zero, 
            Zero
        })
        Name (_S5, Package (0x04)  // _S5_: S5 System State
        {
            Zero, 
            Zero, 
            Zero, 
            Zero
        })
    }

    Scope (\_SB.PCI0)
    {
        Device (FWCF)
        {
            Name (_HID, "QEMU0002")  // _HID: Hardware ID
            Name (_STA, 0x0B)  // _STA: Status
            Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
            {
                IO (Decode16,
                    0x0510,             // Range Minimum
                    0x0510,             // Range Maximum
                    0x01,               // Alignment
                    0x0C,               // Length
                    )
            })
        }
    }

    Scope (\_SB)
    {
        Scope (PCI0)
        {
            Name (BSEL, Zero)
            Device (S00)
            {
                Name (_ADR, Zero)  // _ADR: Address
                Name (ASUN, Zero)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }
            }

            Device (S10)
            {
                Name (_ADR, 0x00020000)  // _ADR: Address
                Name (ASUN, 0x02)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Method (_S1D, 0, NotSerialized)  // _S1D: S1 Device State
                {
                    Return (Zero)
                }

                Method (_S2D, 0, NotSerialized)  // _S2D: S2 Device State
                {
                    Return (Zero)
                }

                Method (_S3D, 0, NotSerialized)  // _S3D: S3 Device State
                {
                    Return (Zero)
                }
            }

            Device (S18)
            {
                Name (_ADR, 0x00030000)  // _ADR: Address
                Name (ASUN, 0x03)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Name (_SUN, 0x03)  // _SUN: Slot User Number
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }
            }

            Device (S20)
            {
                Name (_ADR, 0x00040000)  // _ADR: Address
                Name (ASUN, 0x04)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Name (_SUN, 0x04)  // _SUN: Slot User Number
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }
            }

            Device (S28)
            {
                Name (_ADR, 0x00050000)  // _ADR: Address
                Name (ASUN, 0x05)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Name (_SUN, 0x05)  // _SUN: Slot User Number
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }
            }

            Device (S30)
            {
                Name (_ADR, 0x00060000)  // _ADR: Address
                Name (ASUN, 0x06)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Name (_SUN, 0x06)  // _SUN: Slot User Number
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }
            }

            Device (S38)
            {
                Name (_ADR, 0x00070000)  // _ADR: Address
                Name (ASUN, 0x07)
                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                }

                Name (_SUN, 0x07)  // _SUN: Slot User Number
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }
            }

            Device (S40)
            {
                Name (_SUN, 0x08)  // _SUN: Slot User Number
                Name (_ADR, 0x00080000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S48)
            {
                Name (_SUN, 0x09)  // _SUN: Slot User Number
                Name (_ADR, 0x00090000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S50)
            {
                Name (_SUN, 0x0A)  // _SUN: Slot User Number
                Name (_ADR, 0x000A0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S58)
            {
                Name (_SUN, 0x0B)  // _SUN: Slot User Number
                Name (_ADR, 0x000B0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S60)
            {
                Name (_SUN, 0x0C)  // _SUN: Slot User Number
                Name (_ADR, 0x000C0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S68)
            {
                Name (_SUN, 0x0D)  // _SUN: Slot User Number
                Name (_ADR, 0x000D0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S70)
            {
                Name (_SUN, 0x0E)  // _SUN: Slot User Number
                Name (_ADR, 0x000E0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S78)
            {
                Name (_SUN, 0x0F)  // _SUN: Slot User Number
                Name (_ADR, 0x000F0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S80)
            {
                Name (_SUN, 0x10)  // _SUN: Slot User Number
                Name (_ADR, 0x00100000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S88)
            {
                Name (_SUN, 0x11)  // _SUN: Slot User Number
                Name (_ADR, 0x00110000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S90)
            {
                Name (_SUN, 0x12)  // _SUN: Slot User Number
                Name (_ADR, 0x00120000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (S98)
            {
                Name (_SUN, 0x13)  // _SUN: Slot User Number
                Name (_ADR, 0x00130000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SA0)
            {
                Name (_SUN, 0x14)  // _SUN: Slot User Number
                Name (_ADR, 0x00140000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SA8)
            {
                Name (_SUN, 0x15)  // _SUN: Slot User Number
                Name (_ADR, 0x00150000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SB0)
            {
                Name (_SUN, 0x16)  // _SUN: Slot User Number
                Name (_ADR, 0x00160000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SB8)
            {
                Name (_SUN, 0x17)  // _SUN: Slot User Number
                Name (_ADR, 0x00170000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SC0)
            {
                Name (_SUN, 0x18)  // _SUN: Slot User Number
                Name (_ADR, 0x00180000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SC8)
            {
                Name (_SUN, 0x19)  // _SUN: Slot User Number
                Name (_ADR, 0x00190000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SD0)
            {
                Name (_SUN, 0x1A)  // _SUN: Slot User Number
                Name (_ADR, 0x001A0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SD8)
            {
                Name (_SUN, 0x1B)  // _SUN: Slot User Number
                Name (_ADR, 0x001B0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SE0)
            {
                Name (_SUN, 0x1C)  // _SUN: Slot User Number
                Name (_ADR, 0x001C0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SE8)
            {
                Name (_SUN, 0x1D)  // _SUN: Slot User Number
                Name (_ADR, 0x001D0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SF0)
            {
                Name (_SUN, 0x1E)  // _SUN: Slot User Number
                Name (_ADR, 0x001E0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Device (SF8)
            {
                Name (_SUN, 0x1F)  // _SUN: Slot User Number
                Name (_ADR, 0x001F0000)  // _ADR: Address
                Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
                {
                    PCEJ (BSEL, _SUN)
                }

                Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                {
                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
                }
            }

            Method (DVNT, 2, NotSerialized)
            {
                If ((Arg0 & 0x08))
                {
                    Notify (S18, Arg1)
                }

                If ((Arg0 & 0x10))
                {
                    Notify (S20, Arg1)
                }

                If ((Arg0 & 0x20))
                {
                    Notify (S28, Arg1)
                }

                If ((Arg0 & 0x40))
                {
                    Notify (S30, Arg1)
                }

                If ((Arg0 & 0x80))
                {
                    Notify (S38, Arg1)
                }

                If ((Arg0 & 0x0100))
                {
                    Notify (S40, Arg1)
                }

                If ((Arg0 & 0x0200))
                {
                    Notify (S48, Arg1)
                }

                If ((Arg0 & 0x0400))
                {
                    Notify (S50, Arg1)
                }

                If ((Arg0 & 0x0800))
                {
                    Notify (S58, Arg1)
                }

                If ((Arg0 & 0x1000))
                {
                    Notify (S60, Arg1)
                }

                If ((Arg0 & 0x2000))
                {
                    Notify (S68, Arg1)
                }

                If ((Arg0 & 0x4000))
                {
                    Notify (S70, Arg1)
                }

                If ((Arg0 & 0x8000))
                {
                    Notify (S78, Arg1)
                }

                If ((Arg0 & 0x00010000))
                {
                    Notify (S80, Arg1)
                }

                If ((Arg0 & 0x00020000))
                {
                    Notify (S88, Arg1)
                }

                If ((Arg0 & 0x00040000))
                {
                    Notify (S90, Arg1)
                }

                If ((Arg0 & 0x00080000))
                {
                    Notify (S98, Arg1)
                }

                If ((Arg0 & 0x00100000))
                {
                    Notify (SA0, Arg1)
                }

                If ((Arg0 & 0x00200000))
                {
                    Notify (SA8, Arg1)
                }

                If ((Arg0 & 0x00400000))
                {
                    Notify (SB0, Arg1)
                }

                If ((Arg0 & 0x00800000))
                {
                    Notify (SB8, Arg1)
                }

                If ((Arg0 & 0x01000000))
                {
                    Notify (SC0, Arg1)
                }

                If ((Arg0 & 0x02000000))
                {
                    Notify (SC8, Arg1)
                }

                If ((Arg0 & 0x04000000))
                {
                    Notify (SD0, Arg1)
                }

                If ((Arg0 & 0x08000000))
                {
                    Notify (SD8, Arg1)
                }

                If ((Arg0 & 0x10000000))
                {
                    Notify (SE0, Arg1)
                }

                If ((Arg0 & 0x20000000))
                {
                    Notify (SE8, Arg1)
                }

                If ((Arg0 & 0x40000000))
                {
                    Notify (SF0, Arg1)
                }

                If ((Arg0 & 0x80000000))
                {
                    Notify (SF8, Arg1)
                }
            }

            Method (PCNT, 0, NotSerialized)
            {
                BNUM = Zero
                DVNT (PCIU, One)
                DVNT (PCID, 0x03)
            }
        }
    }
}

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

[Andy Shevchenko]

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.

From DSDT _ADR() you may deduct the PCI BDF of each device in the topology.

> And those translate on the DSDT as (for the S30/S38 chunk I am
> interested in):
> 
>             Device (S30)
>             {
>                 Name (_ADR, 0x00060000)  // _ADR: Address

In PCI this is 00:06.0

>                 Name (ASUN, 0x06)
>                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
>                 {
>                     Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
>                 }
> 
>                 Name (_SUN, 0x06)  // _SUN: Slot User Number
>                 Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
>                 {
>                     PCEJ (BSEL, _SUN)
>                 }
>             }
> 
>             Device (S38)
>             {
>                 Name (_ADR, 0x00070000)  // _ADR: Address

00:07.0 respectively.

>                 Name (ASUN, 0x07)
>                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
>                 {
>                     Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
>                 }
> 
>                 Name (_SUN, 0x07)  // _SUN: Slot User Number
>                 Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device, x=0-9
>                 {
>                     PCEJ (BSEL, _SUN)
>                 }
>             }
> 
> The forwarded USB node is actually on device:07 -> S30_, and as much as
> I'd like it to be a regular USB hub, this looks like a virtio node entry
> that allows to forward a physical device to the VM.
> 
> So IMO, the missing piece might rely on the virtio-usb code which
> doesn't export the firmware node, which means I can not extend the
> device with an SSDT overlay ATM because the USB node doesn't have the
> fw_node.

Ah, that very much may explain this!

> > > > > Also note that if I plug the CP2112 over a docking station, I lose the
> > > > > firmware_node sysfs entries on the host too.
> > > >
> > > > This seems like a lack of firmware node propagating in the USB hub code in
> > > > the Linux kernel.
> > >
> > > That would make a lot of sense.
> > >
> > > FWIW, in the VM I see a firmware node on the pci controller itself:
> > > #> ls -l /sys/devices/pci0000\:00/0000\:00\:06.0/firmware_node
> > >   lrwxrwxrwx 1 root root 0 Mar  6 12:24 /sys/devices/pci0000:00/0000:00:06.0/firmware_node -> ../../LNXSYSTM:00/LNXSYBUS:00/PNP0A03:00/device:07
> > >
> > > And one the host, through a USB hub:
> > >
> > > #> ls -l /sys/bus/hid/devices/0003:10C4:EA90.*
> > >   lrwxrwxrwx. 1 root root 0 Mar  6 13:26 /sys/bus/hid/devices/0003:10C4:EA90.007C -> ../../../devices/pci0000:00/0000:00:14.0/usb2/2-8/2-8.2/2-8.2.4/2-8.2.4:1.0/0003:10C4:EA90.007C
> > > #> ls -l /sys/bus/usb/devices/2-8*/firmware_node
> > >   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8:1.0/firmware_node -> ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> > >   lrwxrwxrwx. 1 root root 0 Mar  2 16:53 /sys/bus/usb/devices/2-8/firmware_node -> ../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> > >
> > > Note that the firmware node propagation stopped at 2-8, and 2.8.2 is not
> > > having a firmware node.
> > 
> > It would be nice if you can run `grep -H 15 /sys/bus/acpi/devices/*/status`,
> 
> This command (both on the host and on the VM) does not show any USB
> device or even the PCI USB controller itself (PNP0A08 or PNP0A03).
> 
> > filter out unneeded ones, and for the rest also print their paths:
> > `cat filtered_list_of_acpi_devs | while read p; do grep -H . $p/path; done`
> 
> see above for the VM case, and in the host:
> 
> #> grep XHC /sys/bus/acpi/devices/*/path
> /sys/bus/acpi/devices/device:15/path:\_SB_.PCI0.XHC_
> /sys/bus/acpi/devices/device:16/path:\_SB_.PCI0.XHC_.RHUB
> /sys/bus/acpi/devices/device:17/path:\_SB_.PCI0.XHC_.RHUB.HS01
> /sys/bus/acpi/devices/device:18/path:\_SB_.PCI0.XHC_.RHUB.HS02
> /sys/bus/acpi/devices/device:19/path:\_SB_.PCI0.XHC_.RHUB.HS03
> /sys/bus/acpi/devices/device:1a/path:\_SB_.PCI0.XHC_.RHUB.HS04
> /sys/bus/acpi/devices/device:1b/path:\_SB_.PCI0.XHC_.RHUB.HS05
> /sys/bus/acpi/devices/device:1c/path:\_SB_.PCI0.XHC_.RHUB.HS06
> /sys/bus/acpi/devices/device:1d/path:\_SB_.PCI0.XHC_.RHUB.HS07
> /sys/bus/acpi/devices/device:1e/path:\_SB_.PCI0.XHC_.RHUB.HS08
> /sys/bus/acpi/devices/device:1f/path:\_SB_.PCI0.XHC_.RHUB.SS01
> /sys/bus/acpi/devices/device:20/path:\_SB_.PCI0.XHC_.RHUB.SS02
> /sys/bus/acpi/devices/device:21/path:\_SB_.PCI0.XHC_.RHUB.SS03
> /sys/bus/acpi/devices/device:22/path:\_SB_.PCI0.XHC_.RHUB.SS04
> /sys/bus/acpi/devices/device:23/path:\_SB_.PCI0.XHC_.RHUB.SS05
> /sys/bus/acpi/devices/device:24/path:\_SB_.PCI0.XHC_.RHUB.SS06
> /sys/bus/acpi/devices/device:25/path:\_SB_.PCI0.XHC_.RHUB.HS09
> /sys/bus/acpi/devices/device:26/path:\_SB_.PCI0.XHC_.RHUB.HS10
> /sys/bus/acpi/devices/device:27/path:\_SB_.PCI0.XHC_.RHUB.USR1
> /sys/bus/acpi/devices/device:28/path:\_SB_.PCI0.XHC_.RHUB.USR2
> /sys/bus/acpi/devices/device:85/path:\_SB_.PCI0.TXHC
> /sys/bus/acpi/devices/device:86/path:\_SB_.PCI0.TXHC.RHUB
> /sys/bus/acpi/devices/device:87/path:\_SB_.PCI0.TXHC.RHUB.HS01
> /sys/bus/acpi/devices/device:88/path:\_SB_.PCI0.TXHC.RHUB.SS01
> /sys/bus/acpi/devices/device:89/path:\_SB_.PCI0.TXHC.RHUB.SS02
> /sys/bus/acpi/devices/device:8a/path:\_SB_.PCI0.TXHC.RHUB.SS03
> /sys/bus/acpi/devices/device:8b/path:\_SB_.PCI0.TXHC.RHUB.SS04
> 
> Which is coherent with the ../../../../../LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:15/device:16/device:1e
> I get when looking at the USB port.
> 
> > With this we will see what devices are actually present and up and running
> > in the system and what their paths in the ACPI namespace.
> 
> So it seems that the USB hub functionality is not creating fw_nodes for
> its children. But I am not sure this is a battle we want to fight right
> now, because it doesn't make a lot of sense IMO to add an SSDT overlay
> on a hub.

The description of the attachable devices should really be in the overlays
if user wants them, but it's another story.

> > > > > Do you think it would be achievable to emulate that over qemu and use a
> > > > > mainline kernel without patches?
> > > >
> > > > As long as qemu provides correct DSDT it should work I assume.
> > >
> > > Just to be sure I understand, for this to work, we need the DSDT to
> > > export a "Device(RHUB)"?
> > 
> > Not sure I understand the term "export" here. We need a description
> > of the (to describe) missing parts.
> 
> Yes, I meant "to describe" it.
> 
> > > Or if we fix the USB fw_node propagation, could we just overwrite
> > > "\_SB_.PCI0.S30_"?  "\_SB_.PCI0.S30_" is the name the ACPI is giving to
> > > the USB port in my VM case AFAIU.
> > 
> > I have no idea what is the S30 node.
> 
> That gave me the hint I needed, I think. The problem must be in the
> virtio drivers, where it doesn't attach the fw_node to the components it
> creates. We probably need kind of the same patch Danny is sending in 2/3
> in this series, but for virtio.

Sounds like that, indeed.

> > [2] https://gitlab.freedesktop.org/bentiss/gitlab-kernel-ci/-/tree/master/VM
> > [3] https://packages.debian.org/buster/all/ovmf/filelist

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

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

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

[Daniel Kaehn]

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.

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

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.

> > > 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 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 :)

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

[Benjamin Tissoires]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

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.

Cheers,
Benjamin

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

[Daniel Kaehn]

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

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

[Benjamin Tissoires]

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 })
          }
        }
      }
    }
  }
}
---

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

[Andy Shevchenko]

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:
> > > 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;
> ---

I don't think so. The parent should point to the _physical_ device, which is
CP2112, which is correct in my opinion.

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

We have specifically gc->fwnode for cases like this.

...

>         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" }
> 			}

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.

> 		})
> 
>           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
>                             "",
>                             "",
>                             "",
>                             ""}},
>               }
>             })
>           }
>         }

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

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

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

[Andy Shevchenko]

On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:
> 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 can be iteratively converted later on.

> 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));

ACPI_COMPANION_SET() is something different to simple device_set_node().
I would expect that in this driver we simply use the child fwnode as is.
But since you are not using so called secondary fwnode, I believe it's
fine for now.

> + 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!)

See how the acpi_get_local_address() is used in the 3 users of it.

Ideally we need a new callback in the fwnode ops to return either
(least) 32-bit of _ADR or "reg" property.

Dunno, if "reg" is actually what suits here.

That said, I would do something like (pseudo-code)

device_for_each_child_node() {
	if (name == $NAME)
		$NAME->fwnode = child;
	else if (_ADR = $INDEX)
		$NAME->fwnode = child;
}


> Thanks for your patience in working through all of this, especially
> considering how long of an email
> chain this has become!

You're welcome!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Mar 09, 2023 at 01:43:02PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 08, 2023 at 12:32:07PM -0600, Daniel Kaehn wrote:
> > 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 can be iteratively converted later on.
> 
> > 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));
> 
> ACPI_COMPANION_SET() is something different to simple device_set_node().
> I would expect that in this driver we simply use the child fwnode as is.
> But since you are not using so called secondary fwnode, I believe it's
> fine for now.
> 
> > + 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!)

The below misplaced, so here is the answer to the followup.
The _DSD heavily relies on the DT schemas and other standards such as MIPI.
For many cases there are no standards or any developed approaches.

Feel free to add a piece of documentation for the devices that are utilising
_ADR in ACPI (we have at least I²C/GPIO controller on Intel Quark —
drivers/mfd/intel_quark_i2c_gpio.c, and mentioned earlier the Diolan DLN-2 —
drivers/mfd/dln2.c).

> See how the acpi_get_local_address() is used in the 3 users of it.
> 
> Ideally we need a new callback in the fwnode ops to return either
> (least) 32-bit of _ADR or "reg" property.
> 
> Dunno, if "reg" is actually what suits here.
> 
> That said, I would do something like (pseudo-code)
> 
> device_for_each_child_node() {
> 	if (name == $NAME)
> 		$NAME->fwnode = child;
> 	else if (_ADR = $INDEX)
> 		$NAME->fwnode = child;
> }
> 
> > Thanks for your patience in working through all of this, especially
> > considering how long of an email
> > chain this has become!
> 
> You're welcome!

-- 
With Best Regards,
Andy Shevchenko

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

[Benjamin Tissoires]

On Mar 08 2023, 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:
> > > > 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;
> > ---
> 
> I don't think so. The parent should point to the _physical_ device, which is
> CP2112, which is correct in my opinion.
> 

Right. I tend to agree, and then the problem seems to be relying in
gpiolib-acpi.c

> > 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.
> 
> We have specifically gc->fwnode for cases like this.

Looks like gpiolib-acpi.c doesn't care about fwnode at all.

if I do the following:

---
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d8a421ce26a8..5aebc266426b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
-       return gc->parent && device_match_acpi_handle(gc->parent, data);
+       return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
 }
 
 /**
---

I can now directly reference the GPIO ACPI node in my GpioInt()
declaration. And AFAICT this should be safe to do because gpiolib ensure
that gc->fwnode is set, using the one from the parent if it is not set
previously.

I need to check if this works with my icelake laptop, and if so I'll
send it to the list.

The reason the intel gpios are working (the only one I checked) is
because the \\SB.GPI0 node refers to the pinctrl controller (driver
pinctrl-icelake.c in my case, which then creates a subdevice for
handling the gpio).

> 
> ...
> 

Cheers,
Benjamin

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

[Andy Shevchenko]

On Thu, Mar 09, 2023 at 10:38:11AM +0100, Benjamin Tissoires wrote:
> On Mar 08 2023, Andy Shevchenko wrote:

...

> Looks like gpiolib-acpi.c doesn't care about fwnode at all.
> 
> if I do the following:
> 
> ---
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index d8a421ce26a8..5aebc266426b 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -126,7 +126,7 @@ static bool acpi_gpio_deferred_req_irqs_done;
>  
>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  {
> -       return gc->parent && device_match_acpi_handle(gc->parent, data);
> +       return ACPI_HANDLE_FWNODE(gc->fwnode) == data;
>  }
>  
>  /**
> ---

This seems a legit fix.

> I can now directly reference the GPIO ACPI node in my GpioInt()
> declaration. And AFAICT this should be safe to do because gpiolib ensure
> that gc->fwnode is set, using the one from the parent if it is not set
> previously.
> 
> I need to check if this works with my icelake laptop, and if so I'll
> send it to the list.
> 
> The reason the intel gpios are working (the only one I checked) is
> because the \\SB.GPI0 node refers to the pinctrl controller (driver
> pinctrl-icelake.c in my case, which then creates a subdevice for
> handling the gpio).

Good catch!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote:
> On Mar 06 2023, Andy Shevchenko wrote:
> > On Mon, Mar 06, 2023 at 01:40:16PM -0600, Daniel Kaehn wrote:

...

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

You can try to utilize _DEP method, but I dunno if it implemented that way in
the Linux that helps your case.

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

The solution is to provide in the main node the list of cell names, that way
you will always know the indices:

  Device (DEV) {
	  _DSD
		  "cell-names" { "i2c", "gpio" } // index of the name is the
						 // index of the cell

	Device (I2C0) {
	}

	Device (GPI0) {
	}
  }

Problem solved.

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

You're welcome!

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

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Kaehn]

On Tue, Mar 7, 2023 at 12:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote:
> > On Mar 06 2023, Andy Shevchenko wrote:
> > 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.
>
> The solution is to provide in the main node the list of cell names, that way
> you will always know the indices:
>
>   Device (DEV) {
>           _DSD
>                   "cell-names" { "i2c", "gpio" } // index of the name is the
>                                                  // index of the cell
>
>         Device (I2C0) {
>         }
>
>         Device (GPI0) {
>         }
>   }
>
> Problem solved.
>

Just to make sure I'm understanding you correctly:

Are you proposing that specifically this driver directly reads "cell-names"
from the ACPI DSD to implement this indexing? Or are you proposing a
kernel-wide mechanism of "overriding" a fwnode name with ACPI?
(assuming this doesn't exist already, and I'm not just missing it in
the kernel source)

Or are you proposing something else entirely?
(apologies if this should be obvious -- throwing up the ACPI newbie
card again here :) )

In any case, would this be something I should post to the email chain
with DT and ACPI
folks for opinions before I start to implement it?

Thanks,

Danny Kaehn

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

[Andy Shevchenko]

On Tue, Mar 07, 2023 at 01:57:27PM -0600, Daniel Kaehn wrote:
> On Tue, Mar 7, 2023 at 12:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Mar 07, 2023 at 02:17:06PM +0100, Benjamin Tissoires wrote:
> > > On Mar 06 2023, Andy Shevchenko wrote:

...

> > > 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.
> >
> > The solution is to provide in the main node the list of cell names, that way
> > you will always know the indices:
> >
> >   Device (DEV) {
> >           _DSD
> >                   "cell-names" { "i2c", "gpio" } // index of the name is the
> >                                                  // index of the cell
> >
> >         Device (I2C0) {
> >         }
> >
> >         Device (GPI0) {
> >         }
> >   }
> >
> > Problem solved.
> 
> Just to make sure I'm understanding you correctly:
> 
> Are you proposing that specifically this driver directly reads "cell-names"
> from the ACPI DSD to implement this indexing? Or are you proposing a
> kernel-wide mechanism of "overriding" a fwnode name with ACPI?
> (assuming this doesn't exist already, and I'm not just missing it in
> the kernel source)
> 
> Or are you proposing something else entirely?
> (apologies if this should be obvious -- throwing up the ACPI newbie
> card again here :) )

Out of the three listed above I'm proposing the first one.
(Maybe it can be called 'reg-names', but it depends on the hardware)

Also look into hierarchical _DSD (it is similar to the children
of the device node without being separate devices).

> In any case, would this be something I should post to the email chain
> with DT and ACPI
> folks for opinions before I start to implement it?

May be, the -names approach is already used widely for clock, interrupt, reset,
phy and nvmem. So it's something we know about.

-- 
With Best Regards,
Andy Shevchenko