diff mbox series

[RFC,2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198

Message ID 20201004162908.3216898-3-martin.blumenstingl@googlemail.com
State Not Applicable
Headers show
Series GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers | expand

Commit Message

Martin Blumenstingl Oct. 4, 2020, 4:29 p.m. UTC
Etron EJ168/EJ188/EJ198 are USB xHCI host controllers which embed a GPIO
controller.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../devicetree/bindings/gpio/etron,ej1x8.yaml | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml

Comments

Rob Herring Oct. 6, 2020, 9:25 p.m. UTC | #1
On Sun, Oct 04, 2020 at 06:29:07PM +0200, Martin Blumenstingl wrote:
> Etron EJ168/EJ188/EJ198 are USB xHCI host controllers which embed a GPIO
> controller.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../devicetree/bindings/gpio/etron,ej1x8.yaml | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml
> new file mode 100644
> index 000000000000..fa554045bdb5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml
> @@ -0,0 +1,48 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/etron,ej1x8.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO controller embedded into the EJ168/EJ188/EJ198 xHCI controllers
> +
> +maintainers:
> +  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - pci1b6f,7023
> +      - pci1b6f,7052
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      pcie {
> +        #address-cells = <3>;
> +        #size-cells = <2>;
> +
> +        gpio@0,0,0 {
> +          compatible = "pci1b6f,7023";
> +          reg = <0x0 0x0 0x0 0x0 0x1000>;
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +        };

How would this look if you needed to describe the XHCI controller? 
That's another PCI function?

> +      };
> +
> +...
> -- 
> 2.28.0
>
Linus Walleij Oct. 7, 2020, 9:19 a.m. UTC | #2
On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:

> +properties:
> +  compatible:
> +    enum:
> +      - pci1b6f,7023
> +      - pci1b6f,7052

I think it is better to let the PCI driver for the whole hardware in
drivers/usb/host/xhci-pci.c probe from the PCI configuration space
numbers, and then add a gpio_chip to xhci-pci.c.

Thanks!
Linus Walleij
Martin Blumenstingl Oct. 7, 2020, 7:57 p.m. UTC | #3
Hi Rob,

On Tue, Oct 6, 2020 at 11:25 PM Rob Herring <robh@kernel.org> wrote:
[...]
> > +examples:
> > +  - |
> > +      pcie {
> > +        #address-cells = <3>;
> > +        #size-cells = <2>;
> > +
> > +        gpio@0,0,0 {
> > +          compatible = "pci1b6f,7023";
> > +          reg = <0x0 0x0 0x0 0x0 0x1000>;
> > +          gpio-controller;
> > +          #gpio-cells = <2>;
> > +        };
>
> How would this look if you needed to describe the XHCI controller?
> That's another PCI function?
unfortunately I have no documentation about this
the "reference driver" is poking the PCI config space registers of the
xHCI controller - without any dedicated function for this


Best regards,
Martin
Martin Blumenstingl Oct. 7, 2020, 7:57 p.m. UTC | #4
Hi Linus,

On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - pci1b6f,7023
> > +      - pci1b6f,7052
>
> I think it is better to let the PCI driver for the whole hardware in
> drivers/usb/host/xhci-pci.c probe from the PCI configuration space
> numbers, and then add a gpio_chip to xhci-pci.c.
to have everything consistent I will move the binding to
Documentation/devicetree/bindings/usb


Best regards,
Martin
Linus Walleij Oct. 13, 2020, 1:27 p.m. UTC | #5
On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> wrote:
> >
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - pci1b6f,7023
> > > +      - pci1b6f,7052
> >
> > I think it is better to let the PCI driver for the whole hardware in
> > drivers/usb/host/xhci-pci.c probe from the PCI configuration space
> > numbers, and then add a gpio_chip to xhci-pci.c.
>
> to have everything consistent I will move the binding to
> Documentation/devicetree/bindings/usb

I do not understand why a PCI device would need a DT binding
at all. They just probe from the magic number in the PCI
config space, they spawn struct pci_dev PCI devices, not the
type of platform devices coming out of the DT parser code.
No DT compatible needed.

What am I not understanding about this?

Yours,
Linus Walleij
Rob Herring Oct. 14, 2020, 12:43 p.m. UTC | #6
On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> > On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com> wrote:
> > >
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - pci1b6f,7023
> > > > +      - pci1b6f,7052
> > >
> > > I think it is better to let the PCI driver for the whole hardware in
> > > drivers/usb/host/xhci-pci.c probe from the PCI configuration space
> > > numbers, and then add a gpio_chip to xhci-pci.c.
> >
> > to have everything consistent I will move the binding to
> > Documentation/devicetree/bindings/usb
>
> I do not understand why a PCI device would need a DT binding
> at all. They just probe from the magic number in the PCI
> config space, they spawn struct pci_dev PCI devices, not the
> type of platform devices coming out of the DT parser code.
> No DT compatible needed.

Same reason for all the discoverable buses need bindings. There can be
parts that are not discoverable or connections with non-discoverable
nodes. There's also cases where the discoverable device has to be
powered, reset deasserted, clocks enabled, etc. first to be
discovered.

If the GPIOs here had connections elsewhere in the DT, then we have to
describe the provider in DT.

Rob
Martin Blumenstingl Oct. 16, 2020, 8:52 p.m. UTC | #7
Hi Linus,

On Wed, Oct 14, 2020 at 2:43 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Wed, Oct 7, 2020 at 9:58 PM Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> wrote:
> > > On Wed, Oct 7, 2020 at 11:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl
> > > > <martin.blumenstingl@googlemail.com> wrote:
> > > >
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - pci1b6f,7023
> > > > > +      - pci1b6f,7052
> > > >
> > > > I think it is better to let the PCI driver for the whole hardware in
> > > > drivers/usb/host/xhci-pci.c probe from the PCI configuration space
> > > > numbers, and then add a gpio_chip to xhci-pci.c.
> > >
> > > to have everything consistent I will move the binding to
> > > Documentation/devicetree/bindings/usb
> >
> > I do not understand why a PCI device would need a DT binding
> > at all. They just probe from the magic number in the PCI
> > config space, they spawn struct pci_dev PCI devices, not the
> > type of platform devices coming out of the DT parser code.
> > No DT compatible needed.
>
> Same reason for all the discoverable buses need bindings. There can be
> parts that are not discoverable or connections with non-discoverable
> nodes. There's also cases where the discoverable device has to be
> powered, reset deasserted, clocks enabled, etc. first to be
> discovered.
>
> If the GPIOs here had connections elsewhere in the DT, then we have to
> describe the provider in DT.
this is exactly what I need it for: that platform has hand-written
.dts files and I need to wire up a GPIO LED


Best regards,
Martin
Linus Walleij Oct. 29, 2020, 5:11 p.m. UTC | #8
On Fri, Oct 16, 2020 at 10:52 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> On Wed, Oct 14, 2020 at 2:43 PM Rob Herring <robh+dt@kernel.org> wrote:
> > On Tue, Oct 13, 2020 at 8:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> > > I do not understand why a PCI device would need a DT binding
> > > at all. They just probe from the magic number in the PCI
> > > config space, they spawn struct pci_dev PCI devices, not the
> > > type of platform devices coming out of the DT parser code.
> > > No DT compatible needed.
> >
> > Same reason for all the discoverable buses need bindings. There can be
> > parts that are not discoverable or connections with non-discoverable
> > nodes. There's also cases where the discoverable device has to be
> > powered, reset deasserted, clocks enabled, etc. first to be
> > discovered.
> >
> > If the GPIOs here had connections elsewhere in the DT, then we have to
> > describe the provider in DT.
>
> this is exactly what I need it for: that platform has hand-written
> .dts files and I need to wire up a GPIO LED

Aha I get it! OK then :)

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml
new file mode 100644
index 000000000000..fa554045bdb5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml
@@ -0,0 +1,48 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/etron,ej1x8.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO controller embedded into the EJ168/EJ188/EJ198 xHCI controllers
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+properties:
+  compatible:
+    enum:
+      - pci1b6f,7023
+      - pci1b6f,7052
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+examples:
+  - |
+      pcie {
+        #address-cells = <3>;
+        #size-cells = <2>;
+
+        gpio@0,0,0 {
+          compatible = "pci1b6f,7023";
+          reg = <0x0 0x0 0x0 0x0 0x1000>;
+          gpio-controller;
+          #gpio-cells = <2>;
+        };
+      };
+
+...