diff mbox series

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

Message ID 20240605-cp2112-dt-v11-1-d55f0f945a62@plexus.com (mailing list archive)
State New
Headers show
Series Firmware Support for USB-HID Devices and CP2112 | expand

Commit Message

Danny Kaehn June 5, 2024, 11:12 p.m. UTC
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>
---
 .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105 +++++++++++++++++++++
 1 file changed, 105 insertions(+)

Comments

Rob Herring (Arm) June 6, 2024, 12:18 a.m. UTC | #1
On Wed, 05 Jun 2024 18:12:44 -0500, 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>
> ---
>  .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/i2c/silabs,cp2112.example.dts:41.13-29 Properties must precede subnodes
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:427: Documentation/devicetree/bindings/i2c/silabs,cp2112.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240605-cp2112-dt-v11-1-d55f0f945a62@plexus.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski June 6, 2024, 6:28 a.m. UTC | #2
On 06/06/2024 01:12, 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>
> ---
>  .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)

So this is v11 but was never tested?

Changelog does not help me understanding what happened with this binding...

Best regards,
Krzysztof
Danny Kaehn June 6, 2024, 3:12 p.m. UTC | #3
On Thu, 2024-06-06 at 08:28 +0200, Krzysztof Kozlowski wrote:
> PLEXUS SECURITY WARNING
> You have not previously corresponded with this sender.       
> On 06/06/2024 01:12, 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>
> > ---
> >  .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105
> +++++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> 
> So this is v11 but was never tested?

My apologies -- initially `DT_SCHEMA_FILES=silabs,cp2112.yaml make
dt_binding_check` was completing without any output (and I assumed
success), but after a clean, I get make errors, either relating to "no
rule to make ... *.example.dtb", or yamllint usage errors. Have tried
updating dtschema and/or reinstalling, and also started from scratch on
a different system with the same issues. I will get this working and
run this before submitting additional revisions, apologies again!
(and I will of course fix the issue reported by the bot)
> 
> Changelog does not help me understanding what happened with this
> binding...

I'll keep in mind better specifying changes to the binding in the
changelog!

Since v6, where Rob added his review tag [1], the only changes were
eliminating the gpio subnode and combining it with the parent, and
updating my email address.

Since v4, where you had added your review tag [2], I addressed Rob's
comments in [3], including:
- Removing the ngpios property
- Constraining the hog pattern more to a single naming scheme
- Removing unneeded properties from the hog which are provided by the
parent schema
- Adding an example of the hog to test the schema

Additionally, I added sda-gpios and scl-gpios to the i2c node as well
as usage of it in the example dts. This is intended to be used for bus
recovery GPIOs (not yet included in the kernel drivers)

[1]: 
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230217184904.1290-2-kaehndan@gmail.com/

[2]: 
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230206135016.6737-2-kaehndan@gmail.com/

[3]: 
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230205145450.3396-2-kaehndan@gmail.com/#3051932

Thanks,

Danny Kaehn

> Best regards,
> Krzysztof
>
Rob Herring (Arm) June 6, 2024, 3:18 p.m. UTC | #4
On Wed, Jun 05, 2024 at 06:12:44PM -0500, 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

There's no more gpio subnode.

> intended to be used in configurations where the CP2112 is permanently
> connected in hardware.
> 
> Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
> ---
>  .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105 +++++++++++++++++++++
>  1 file changed, 105 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> new file mode 100644
> index 000000000000..0108f2e43c8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> @@ -0,0 +1,105 @@
> +# 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 <danny.kaehn@plexus.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

Or hub ports. Just drop '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

These are because I2C can be on any of the pins? It's a bit odd if they 
aren't used as gpios. Probably should be pinmux, but that's overkill for 
2 pins.

> +
> +      clock-frequency:
> +        minimum: 10000
> +        default: 100000
> +        maximum: 400000
> +
> +  interrupt-controller: true
> +  "#interrupt-cells":
> +    const: 2

Where does the 
> +
> +  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>;
> +
> +      cp2112: device@1 {
> +        compatible = "usb10c4,ea90";
> +        reg = <1>;
> +
> +        i2c {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          sda-gpios = <&cp2112 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +          scl-gpios = <&cp2112 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +
> +          temp@48 {
> +            compatible = "national,lm75";
> +            reg = <0x48>;
> +          };
> +        };
> +
> +        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
>
Danny Kaehn June 6, 2024, 4:24 p.m. UTC | #5
I appreciate you taking a look (even in spite of the dt_binding_check
failure).

On Thu, Jun 06, 2024 at 09:18:59AM -0600, Rob Herring wrote:
> On Wed, Jun 05, 2024 at 06:12:44PM -0500, 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
> 
> There's no more gpio subnode.
>
Will fix.

> > intended to be used in configurations where the CP2112 is permanently
> > connected in hardware.
> > 
> > Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
> > ---
> >  .../devicetree/bindings/i2c/silabs,cp2112.yaml     | 105 +++++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> > new file mode 100644
> > index 000000000000..0108f2e43c8c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> > @@ -0,0 +1,105 @@
> > +# 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 <danny.kaehn@plexus.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
> 
> Or hub ports. Just drop 'on the host controller'.
>

Will fix.

> > +
> > +  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
> 
> These are because I2C can be on any of the pins? It's a bit odd if they 
> aren't used as gpios. Probably should be pinmux, but that's overkill for 
> 2 pins.
>

I'm beginning to realize now that this may be a bit non-standard, but it
did solve a stuck bus issue under some conditions.

The CP2112's I2C controller is self-contained and can only be on the
specific pins it is attached to (no pinmux, etc..).

In this case, these properties are ment to specify additional gpio pins
which are connected to the SCL and SDA lines (this then also assumes those
are configured to be open drain / inputs to not interfere with the bus
during normal operation). This was inspired by what is done ini2c-imx.yaml,
but I realize this is a bit different due to using external pins rather
than pinmuxing to the GPIOs.

How I used this was to actually connect some of the CP2112's own GPIO pins
to the SDA and SCL lines to be able to use those pins to recover the
bus. This was able to solve a stuck bus under some real-world cases with
the v2 of the CP2112 containing an errata which caused this
semi-frequently.

> > +
> > +      clock-frequency:
> > +        minimum: 10000
> > +        default: 100000
> > +        maximum: 400000
> > +
> > +  interrupt-controller: true
> > +  "#interrupt-cells":
> > +    const: 2
> 
> Where does the 

Unsure what you intended to ask here -- but the interrupt comes from the
GPIO chip. It is unfortunately not a hardware interrupt, but is
accomplished through polling.

> > +
> > +  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>;
> > +
> > +      cp2112: device@1 {
> > +        compatible = "usb10c4,ea90";
> > +        reg = <1>;
> > +
> > +        i2c {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +          sda-gpios = <&cp2112 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +          scl-gpios = <&cp2112 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +
> > +          temp@48 {
> > +            compatible = "national,lm75";
> > +            reg = <0x48>;
> > +          };
> > +        };
> > +
> > +        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
Andy Shevchenko June 6, 2024, 7:49 p.m. UTC | #6
On Thu, Jun 06, 2024 at 11:24:38AM -0500, Danny Kaehn wrote:
> On Thu, Jun 06, 2024 at 09:18:59AM -0600, Rob Herring wrote:
> > On Wed, Jun 05, 2024 at 06:12:44PM -0500, Danny Kaehn wrote:

...

> > > +  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
> > 
> > These are because I2C can be on any of the pins? It's a bit odd if they 
> > aren't used as gpios. Probably should be pinmux, but that's overkill for 
> > 2 pins.
> >
> 
> I'm beginning to realize now that this may be a bit non-standard, but it
> did solve a stuck bus issue under some conditions.
> 
> The CP2112's I2C controller is self-contained and can only be on the
> specific pins it is attached to (no pinmux, etc..).
> 
> In this case, these properties are ment to specify additional gpio pins
> which are connected to the SCL and SDA lines (this then also assumes those
> are configured to be open drain / inputs to not interfere with the bus
> during normal operation). This was inspired by what is done ini2c-imx.yaml,
> but I realize this is a bit different due to using external pins rather
> than pinmuxing to the GPIOs.
> 
> How I used this was to actually connect some of the CP2112's own GPIO pins
> to the SDA and SCL lines to be able to use those pins to recover the
> bus. This was able to solve a stuck bus under some real-world cases with
> the v2 of the CP2112 containing an errata which caused this
> semi-frequently.

Aren't they just for I²C recovery?
Danny Kaehn June 7, 2024, 6:38 p.m. UTC | #7
On Thu, Jun 06, 2024 at 10:49:00PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 06, 2024 at 11:24:38AM -0500, Danny Kaehn wrote:
> > On Thu, Jun 06, 2024 at 09:18:59AM -0600, Rob Herring wrote:
> > > On Wed, Jun 05, 2024 at 06:12:44PM -0500, Danny Kaehn wrote:
> 
> ...
> 
> > > > +  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
> > > 
> > > These are because I2C can be on any of the pins? It's a bit odd if they 
> > > aren't used as gpios. Probably should be pinmux, but that's overkill for 
> > > 2 pins.
> > >
> > 
> > I'm beginning to realize now that this may be a bit non-standard, but it
> > did solve a stuck bus issue under some conditions.
> > 
> > The CP2112's I2C controller is self-contained and can only be on the
> > specific pins it is attached to (no pinmux, etc..).
> > 
> > In this case, these properties are ment to specify additional gpio pins
> > which are connected to the SCL and SDA lines (this then also assumes those
> > are configured to be open drain / inputs to not interfere with the bus
> > during normal operation). This was inspired by what is done ini2c-imx.yaml,
> > but I realize this is a bit different due to using external pins rather
> > than pinmuxing to the GPIOs.
> > 
> > How I used this was to actually connect some of the CP2112's own GPIO pins
> > to the SDA and SCL lines to be able to use those pins to recover the
> > bus. This was able to solve a stuck bus under some real-world cases with
> > the v2 of the CP2112 containing an errata which caused this
> > semi-frequently.
> 
> Aren't they just for I²C recovery?

Not sure if you were looking for my reply here, but yes. I guess the
only "non-standard" thing really here is the idea of using
external/separate GPIO pins to do the recovery, rather than pinmuxing the
I2C pins to GPIO which most current implementations seem to do.

Thanks,

Danny Kaehn
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..0108f2e43c8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,105 @@ 
+# 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 <danny.kaehn@plexus.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
+
+  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>;
+
+      cp2112: device@1 {
+        compatible = "usb10c4,ea90";
+        reg = <1>;
+
+        i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+          sda-gpios = <&cp2112 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+          scl-gpios = <&cp2112 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+          temp@48 {
+            compatible = "national,lm75";
+            reg = <0x48>;
+          };
+        };
+
+        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";
+        };
+      };
+    };