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

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 <danny.kaehn@plexus.com>
---

Note -- Reviewed-By tags have been removed as suggested by Benjamin, since
1. It has been 6+ months since this binding was reviewed, and a lot can
change upstream in that time
2. There has been some contention between using named child nodes to
identify i2c and gpio nodes, and also making the driver implementing this
binding compatible with ACPI, since names aren't significant for ACPI
nodes, and ACPI names are always automatically uppercased. It has been
suggested that perhaps the DT binding should use child nodes with
addressable `reg` properties to identify the child nodes, instead of by
name [1].

Of course, I acknowledge that other firmware languages and kernel details
shouldn't impact DT bindings, but it also seems that there should
be some consistent way to specify sub-functions like this accross DT
and ACPI. Some additional commentary / requests for comment about the
seemingly missing glue here can be found in [2].

Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated

[1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
[2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.gmail.com/

 .../bindings/i2c/silabs,cp2112.yaml           | 113 ++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml

On Mon, Feb 05, 2024 at 11:09:20AM -0600, Danny Kaehn wrote:
> 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 <danny.kaehn@plexus.com>
> ---
> 
> Note -- Reviewed-By tags have been removed as suggested by Benjamin, since
> 1. It has been 6+ months since this binding was reviewed, and a lot can
> change upstream in that time
> 2. There has been some contention between using named child nodes to
> identify i2c and gpio nodes, and also making the driver implementing this
> binding compatible with ACPI, since names aren't significant for ACPI
> nodes, and ACPI names are always automatically uppercased. It has been
> suggested that perhaps the DT binding should use child nodes with
> addressable `reg` properties to identify the child nodes, instead of by
> name [1].

'reg' only makes sense if there are values which relate to the h/w. If 
your addresses are indices, that will be suspect.

There's documented nodenames for specific device classes in DT. You have 
to use those whether there's 'reg' and a unit-address or not. I'm not 
really clear what the problem is.

> 
> Of course, I acknowledge that other firmware languages and kernel details
> shouldn't impact DT bindings, but it also seems that there should
> be some consistent way to specify sub-functions like this accross DT
> and ACPI. Some additional commentary / requests for comment about the
> seemingly missing glue here can be found in [2].

I have little interest in worrying about ACPI as I have limited 
knowledge in ACPI requirements, what I do know is the model for bindings 
are fundamentally differ, and no one has stepped up to maintain bindings 
from an ACPI perspective.

> Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated
> 
> [1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
> [2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.gmail.com/
> 
>  .../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

Why do you have GPIOs if this is a proper controller?

> +
> +      clock-frequency:
> +        minimum: 10000
> +        default: 100000
> +        maximum: 400000
> +
> +  gpio:
> +    description: The GPIO controller node for the CP2112

There's no need for a child node here. All these properties can be part 
of the parent.


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

Thanks for taking a look Rob.

On Tue, 2024-02-13 at 09:28 -0600, Rob Herring wrote:
> On Mon, Feb 05, 2024 at 11:09:20AM -0600, Danny Kaehn wrote:
> > This is a USB HID device which includes an I2C controller and 8 GPIO pins.
...
> > 2. There has been some contention between using named child nodes to
> > identify i2c and gpio nodes, and also making the driver implementing this
> > binding compatible with ACPI, since names aren't significant for ACPI
> > nodes, and ACPI names are always automatically uppercased. It has been
> > suggested that perhaps the DT binding should use child nodes with
> > addressable `reg` properties to identify the child nodes, instead of by
> > name [1].
> 
> 'reg' only makes sense if there are values which relate to the h/w. If 
> your addresses are indices, that will be suspect.
> 
> There's documented nodenames for specific device classes in DT. You have 
> to use those whether there's 'reg' and a unit-address or not. I'm not 
> really clear what the problem is.
> 
Ack. Mostly just forwarding on Andy Shevchenko's suggestion for making a more
consistent interface across ACPI and DT since ACPI doesn't support identifying
children by named nodes.

> > 
> > Of course, I acknowledge that other firmware languages and kernel details
> > shouldn't impact DT bindings, but it also seems that there should
> > be some consistent way to specify sub-functions like this accross DT
> > and ACPI. Some additional commentary / requests for comment about the
> > seemingly missing glue here can be found in [2].
> 
> I have little interest in worrying about ACPI as I have limited 
> knowledge in ACPI requirements, what I do know is the model for bindings 
> are fundamentally differ, and no one has stepped up to maintain bindings 
> from an ACPI perspective.
> 

Fair enough.

> > Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated
> > 
> > [1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
> > [2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.gmail.com/
> > 
> >  .../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
> 
> Why do you have GPIOs if this is a proper controller?

This is exclusively for bus recovery (not implemented in the driver patch
sent here). I believe there's precedent for this in bindings like i2c-imx.yaml?

(skip if the above was all you needed):

Hopefully without going into more details than you're interested in, the
CP2112 hardware doesn't implement any runtime bus recovery algorithms.
Even just by bridging two of the CP2112's GPIOs with SCL and SDA,
I was able to use the generic GPIO bus recovery routine to recover a stuck bus.
This was especially important since v2 of the CP2112 hardware has an errata
which can cause uncompleted I2C transactions on a semi-regular basis.

> 
> > +
> > +      clock-frequency:
> > +        minimum: 10000
> > +        default: 100000
> > +        maximum: 400000
> > +
> > +  gpio:
> > +    description: The GPIO controller node for the CP2112
> 
> There's no need for a child node here. All these properties can be part 
> of the parent.

I had gone back and forth on this for quite some time. Would you suggest this
just because it _can_ be combined, due to the naming constraint on the "hog"
nodes? (as opposed to the i2c not being able to be combined, due to
unconstrained names of child nodes?).

I had initially thought this approach would scale better -- say there was a
similar chip with I2C, GPIO, SPI, and UART interfaces -- would GPIO still
share a node with the parent? And is there a reason that gpio is
special, or just it _can_ be combined due to the naming restrictions? Looking
at some of the bindings under mfd/ I see the GPIO controller broken into a
named child node, although I see they also have their own compatible strings...

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

Thanks,

Danny Kaehn