* [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers @ 2020-10-04 16:29 Martin Blumenstingl 2020-10-04 16:29 ` [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 Martin Blumenstingl ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-04 16:29 UTC (permalink / raw) To: linux-pci, devicetree, linux-gpio, robh+dt, bgolaszewski, linus.walleij Cc: linux-kernel, bhelgaas, Martin Blumenstingl, systemchip Hello, I have a "Belkin F9K115v2" (wifi router) [0]. It comes with an Etron EJ168 xHCI controllers soldered to the board. One of the LEDs on this device is connected to one of the four GPIO lines provided by the Etron xHCI controller. The goal of this series to add support for the GPIO controller on the Etron EJ168/EJ188/EJ198 controllers. Unfortunately there's no (public) datasheet available. I have Cc'ed Etron and I'm hoping that they can either provide a datasheet or at least some code-review feedback. Instead I used the GPL tarball [0] for this device. Inside this tarball the relevant "reference" code is in: linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c Unfortunately it uses magic numbers for the registers instead of human-readable names. The register names are what I came up with. For reference, I have tested this on a patched OpenWrt build with the following .dts changes (I am showing these here so it will be easier to review the whole series): &pcie1 { status = "okay"; xhci: usb-controller@0,0,0 { compatible = "pci1b6f,7023"; reg = <0x0 0x0 0x0 0x0 0x1000>; #address-cells = <1>; #size-cells = <0>; gpio-controller; #gpio-cells = <2>; xhci_port0: port@1 { reg = <1>; #trigger-source-cells = <0>; }; }; }; leds { compatible = "gpio-leds"; usb3 { label = "green:usb3"; gpios = <&xhci 2 GPIO_ACTIVE_LOW>; trigger-sources = <&xhci_port0>; linux,default-trigger = "usbport"; }; }; In general I followed [2] because it says: PCI drivers should have a really good reason for not using the pci_register_driver() [...] The main reason [...] is because one PCI device implements several different HW services. My understanding that my driver fits into this category. I am sending this as RFC because this is my first self-written GPIO driver as well as my first PCI device driver. Any feedback is welcome! Best regards, Martin [0] https://openwrt.org/toh/belkin/f9k1115v2 [1] https://www.belkin.com/support/dl/F9K1115v2.03.97-GPL-10.2.85.tar.gz [2] https://www.kernel.org/doc/html/latest/PCI/pci.html#how-to-find-pci-devices-manually Martin Blumenstingl (3): PCI: Add the IDs for Etron EJ168 and EJ188 dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 .../devicetree/bindings/gpio/etron,ej1x8.yaml | 48 +++ drivers/gpio/Kconfig | 9 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-ej1x8.c | 311 ++++++++++++++++++ include/linux/pci_ids.h | 4 + 5 files changed, 373 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/etron,ej1x8.yaml create mode 100644 drivers/gpio/gpio-ej1x8.c -- 2.28.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 2020-10-04 16:29 [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Martin Blumenstingl @ 2020-10-04 16:29 ` Martin Blumenstingl 2020-10-07 9:14 ` Linus Walleij 2020-10-04 16:29 ` [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 Martin Blumenstingl ` (2 subsequent siblings) 3 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-04 16:29 UTC (permalink / raw) To: linux-pci, devicetree, linux-gpio, robh+dt, bgolaszewski, linus.walleij Cc: linux-kernel, bhelgaas, Martin Blumenstingl Add the vendor ID for Etron Technology, Inc. as well as the device IDs for the two USB xHCI controllers EJ168 and EJ188. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- include/linux/pci_ids.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index 1ab1e24bcbce..1c8370aa4b95 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2587,6 +2587,10 @@ #define PCI_VENDOR_ID_REDHAT 0x1b36 +#define PCI_VENDOR_ID_ETRON 0x1b6f +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 + #define PCI_VENDOR_ID_AMAZON_ANNAPURNA_LABS 0x1c36 #define PCI_VENDOR_ID_CIRCUITCO 0x1cc8 -- 2.28.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 2020-10-04 16:29 ` [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 Martin Blumenstingl @ 2020-10-07 9:14 ` Linus Walleij 2020-10-07 19:45 ` Martin Blumenstingl 0 siblings, 1 reply; 23+ messages in thread From: Linus Walleij @ 2020-10-07 9:14 UTC (permalink / raw) To: Martin Blumenstingl, linux-usb Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Add the vendor ID for Etron Technology, Inc. as well as the device IDs > for the two USB xHCI controllers EJ168 and EJ188. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> (...) > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > +#define PCI_VENDOR_ID_ETRON 0x1b6f > +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 > +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 If you're defining that here, I think it should also be removed in drivers/usb/host/xhci-pci.c by including this file instead? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 2020-10-07 9:14 ` Linus Walleij @ 2020-10-07 19:45 ` Martin Blumenstingl 2020-11-03 22:32 ` Bjorn Helgaas 0 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-07 19:45 UTC (permalink / raw) To: Linus Walleij Cc: linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas Hi Linus, On Wed, Oct 7, 2020 at 11:14 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > > Add the vendor ID for Etron Technology, Inc. as well as the device IDs > > for the two USB xHCI controllers EJ168 and EJ188. > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > (...) > > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > > > +#define PCI_VENDOR_ID_ETRON 0x1b6f > > +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 > > +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 > > If you're defining that here, I think it should also be > removed in > drivers/usb/host/xhci-pci.c > by including this file instead? you are absolutely right - I missed that part I will change this in v2 - thanks for pointing it out! Best regards, Martin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 2020-10-07 19:45 ` Martin Blumenstingl @ 2020-11-03 22:32 ` Bjorn Helgaas 0 siblings, 0 replies; 23+ messages in thread From: Bjorn Helgaas @ 2020-11-03 22:32 UTC (permalink / raw) To: Martin Blumenstingl Cc: Linus Walleij, linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas On Wed, Oct 07, 2020 at 09:45:23PM +0200, Martin Blumenstingl wrote: > Hi Linus, > > On Wed, Oct 7, 2020 at 11:14 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > > > > Add the vendor ID for Etron Technology, Inc. as well as the device IDs > > > for the two USB xHCI controllers EJ168 and EJ188. > > > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > > (...) > > > > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > > > > > +#define PCI_VENDOR_ID_ETRON 0x1b6f > > > +#define PCI_DEVICE_ID_ETRON_EJ168 0x7023 > > > +#define PCI_DEVICE_ID_ETRON_EJ188 0x7052 > > > > If you're defining that here, I think it should also be > > removed in > > drivers/usb/host/xhci-pci.c > > by including this file instead? > you are absolutely right - I missed that part > I will change this in v2 - thanks for pointing it out! And please mention the 2+ places these are used in the commit log (see the comment at the top of include/linux/pci_ids.h). ^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-04 16:29 [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Martin Blumenstingl 2020-10-04 16:29 ` [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 Martin Blumenstingl @ 2020-10-04 16:29 ` Martin Blumenstingl 2020-10-06 21:25 ` Rob Herring 2020-10-07 9:19 ` Linus Walleij 2020-10-04 16:29 ` [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 Martin Blumenstingl 2020-10-07 9:17 ` [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Linus Walleij 3 siblings, 2 replies; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-04 16:29 UTC (permalink / raw) To: linux-pci, devicetree, linux-gpio, robh+dt, bgolaszewski, linus.walleij Cc: linux-kernel, bhelgaas, Martin Blumenstingl 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>; + }; + }; + +... -- 2.28.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-04 16:29 ` [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 Martin Blumenstingl @ 2020-10-06 21:25 ` Rob Herring 2020-10-07 19:57 ` Martin Blumenstingl 2020-10-07 9:19 ` Linus Walleij 1 sibling, 1 reply; 23+ messages in thread From: Rob Herring @ 2020-10-06 21:25 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-pci, devicetree, linux-gpio, bgolaszewski, linus.walleij, linux-kernel, bhelgaas 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 > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-06 21:25 ` Rob Herring @ 2020-10-07 19:57 ` Martin Blumenstingl 0 siblings, 0 replies; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-07 19:57 UTC (permalink / raw) To: Rob Herring Cc: linux-pci, devicetree, linux-gpio, bgolaszewski, linus.walleij, linux-kernel, bhelgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-04 16:29 ` [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 Martin Blumenstingl 2020-10-06 21:25 ` Rob Herring @ 2020-10-07 9:19 ` Linus Walleij 2020-10-07 19:57 ` Martin Blumenstingl 1 sibling, 1 reply; 23+ messages in thread From: Linus Walleij @ 2020-10-07 9:19 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-07 9:19 ` Linus Walleij @ 2020-10-07 19:57 ` Martin Blumenstingl 2020-10-13 13:27 ` Linus Walleij 0 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-07 19:57 UTC (permalink / raw) To: Linus Walleij Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-07 19:57 ` Martin Blumenstingl @ 2020-10-13 13:27 ` Linus Walleij 2020-10-14 12:43 ` Rob Herring 0 siblings, 1 reply; 23+ messages in thread From: Linus Walleij @ 2020-10-13 13:27 UTC (permalink / raw) To: Martin Blumenstingl Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-13 13:27 ` Linus Walleij @ 2020-10-14 12:43 ` Rob Herring 2020-10-16 20:52 ` Martin Blumenstingl 0 siblings, 1 reply; 23+ messages in thread From: Rob Herring @ 2020-10-14 12:43 UTC (permalink / raw) To: Linus Walleij Cc: Martin Blumenstingl, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-14 12:43 ` Rob Herring @ 2020-10-16 20:52 ` Martin Blumenstingl 2020-10-29 17:11 ` Linus Walleij 0 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-16 20:52 UTC (permalink / raw) To: Linus Walleij Cc: Rob Herring, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 2020-10-16 20:52 ` Martin Blumenstingl @ 2020-10-29 17:11 ` Linus Walleij 0 siblings, 0 replies; 23+ messages in thread From: Linus Walleij @ 2020-10-29 17:11 UTC (permalink / raw) To: Martin Blumenstingl Cc: Rob Herring, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-10-04 16:29 [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Martin Blumenstingl 2020-10-04 16:29 ` [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 Martin Blumenstingl 2020-10-04 16:29 ` [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 Martin Blumenstingl @ 2020-10-04 16:29 ` Martin Blumenstingl 2020-10-07 9:29 ` Linus Walleij 2020-10-07 9:17 ` [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Linus Walleij 3 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-04 16:29 UTC (permalink / raw) To: linux-pci, devicetree, linux-gpio, robh+dt, bgolaszewski, linus.walleij Cc: linux-kernel, bhelgaas, Martin Blumenstingl EJ168/EJ188/EJ198 are USB xHCI controllers. They also contain four GPIO lines which are used on some systems to toggle an LED based on whether a USB device is connected. There is no public datasheet available for this hardware. All information in this driver is taken from the "F9K1115v2.03.97-GPL-10.2.85-20140313" GPL code dump of the Belkin F9K1115v2. This board comes with an EJ168 USB xHCI controller and the USB 3.0 LED is connected to one of the GPIOs. Inside the GPL source archive the related code can be found in: linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/gpio/Kconfig | 9 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-ej1x8.c | 311 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 321 insertions(+) create mode 100644 drivers/gpio/gpio-ej1x8.c diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 8030fd91a3cc..88820b04ffa5 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -215,6 +215,15 @@ config GPIO_EIC_SPRD help Say yes here to support Spreadtrum EIC device. +config GPIO_EJ1X8 + tristate "Etron Tech Inc. EJ168/EJ188/EJ198 GPIO driver" + depends on OF_GPIO && PCI + help + Selecting this option will enable the GPIO pins present on + the Etron Tech Inc. EJ168/EJ188/EJ198 USB xHCI controllers. + + If unsure, say N. + config GPIO_EM tristate "Emma Mobile GPIO" depends on (ARCH_EMEV2 || COMPILE_TEST) && OF_GPIO diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 4f9abff4f2dc..6d5e345b1f2d 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -55,6 +55,7 @@ obj-$(CONFIG_GPIO_DAVINCI) += gpio-davinci.o obj-$(CONFIG_GPIO_DLN2) += gpio-dln2.o obj-$(CONFIG_GPIO_DWAPB) += gpio-dwapb.o obj-$(CONFIG_GPIO_EIC_SPRD) += gpio-eic-sprd.o +obj-$(CONFIG_GPIO_EJ1X8) += gpio-ej1x8.o obj-$(CONFIG_GPIO_EM) += gpio-em.o obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o obj-$(CONFIG_GPIO_EXAR) += gpio-exar.o diff --git a/drivers/gpio/gpio-ej1x8.c b/drivers/gpio/gpio-ej1x8.c new file mode 100644 index 000000000000..c673e62c34f8 --- /dev/null +++ b/drivers/gpio/gpio-ej1x8.c @@ -0,0 +1,311 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (C) 2020 Martin Blumenstingl <martin.blumenstingl@googlemail.com> */ + +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/gpio/driver.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/pci.h> +#include <linux/pci_ids.h> +#include <linux/property.h> +#include <linux/spinlock.h> +#include <linux/slab.h> +#include <linux/types.h> + +#define EJ1X8_GPIO_INIT 0x44 +#define EJ1X8_GPIO_WRITE 0x68 +#define EJ1X8_GPIO_READ 0x6c + +#define EJ1X8_GPIO_CTRL 0x18005020 +#define EJ1X8_GPIO_CTRL_READ_ALL_MASK GENMASK(7, 0) +#define EJ1X8_GPIO_CTRL_WRITE_ALL_MASK GENMASK(23, 16) +#define EJ1X8_GPIO_CTRL_OUT_LOW 0x0 +#define EJ1X8_GPIO_CTRL_OUT_HIGH 0x1 +#define EJ1X8_GPIO_CTRL_IN 0x2 +#define EJ1X8_GPIO_CTRL_MASK 0x3 + +#define EJ1X8_GPIO_MODE 0x18005022 +#define EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK GENMASK(23, 16) +#define EJ1X8_GPIO_MODE_DISABLE 0x0 +#define EJ1X8_GPIO_MODE_ENABLE 0x1 +#define EJ1X8_GPIO_MODE_MASK 0x3 + +static LIST_HEAD(ej1x8_gpios); + +struct ej1x8_gpio { + spinlock_t lock; + struct pci_dev *pci_dev; + struct gpio_chip chip; + struct list_head list_head; +}; + +static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask) +{ + return (gpio * fls(mask)); +} + +static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask) +{ + return mask << ej1x8_gpio_shift(gpio, mask); +} + +static int ej1x8_gpio_read(struct gpio_chip *gc, u32 reg, u32 *value) +{ + struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc); + int err; + + err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_WRITE, reg); + if (err) { + dev_err(gc->parent, "Failed to select 0x%08x register\n", reg); + return err; + } + + usleep_range(1000, 10000); + + err = pci_read_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_READ, value); + if (err) { + dev_err(gc->parent, "Failed to read 0x%08x register\n", reg); + return err; + } + + return 0; +} + +static int ej1x8_gpio_write(struct gpio_chip *gc, u32 reg, u32 value) +{ + struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc); + int err; + + err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_WRITE, + reg | value | BIT(24)); + if (err) { + dev_err(gc->parent, "Failed to write 0x%08x register\n", reg); + return err; + } + + usleep_range(1000, 10000); + + return 0; +} + +static int ej1x8_gpio_config(struct gpio_chip *gc, unsigned int gpio, u8 mode, + u8 ctrl) +{ + struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc); + u8 all_gpio_ctrl, all_gpio_mode; + u32 temp; + int err; + + spin_lock(&ej1x8->lock); + + err = pci_read_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_INIT, &temp); + if (err) { + dev_err(gc->parent, "Failed to read INIT register\n"); + return err; + } + + err = pci_write_config_dword(ej1x8->pci_dev, EJ1X8_GPIO_INIT, + temp | 0x1); + if (err) { + dev_err(gc->parent, "Failed to write INIT register\n"); + return err; + } + + err = ej1x8_gpio_read(gc, EJ1X8_GPIO_CTRL, &temp); + if (err) + goto err_unlock; + + all_gpio_ctrl = FIELD_GET(EJ1X8_GPIO_CTRL_READ_ALL_MASK, temp); + all_gpio_ctrl &= ~ej1x8_gpio_mask(gpio, EJ1X8_GPIO_CTRL_MASK); + all_gpio_ctrl |= ctrl << ej1x8_gpio_shift(gpio, EJ1X8_GPIO_CTRL_MASK); + + err = ej1x8_gpio_read(gc, EJ1X8_GPIO_MODE, &temp); + if (err) + goto err_unlock; + + all_gpio_mode = FIELD_GET(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK, temp); + all_gpio_mode &= ~ej1x8_gpio_mask(gpio, EJ1X8_GPIO_MODE_MASK); + all_gpio_mode |= mode << ej1x8_gpio_shift(gpio, EJ1X8_GPIO_MODE_MASK); + + err = ej1x8_gpio_write(gc, EJ1X8_GPIO_CTRL, + FIELD_PREP(EJ1X8_GPIO_CTRL_WRITE_ALL_MASK, + all_gpio_ctrl)); + if (err) + goto err_unlock; + + err = ej1x8_gpio_write(gc, EJ1X8_GPIO_MODE, + FIELD_PREP(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK, + all_gpio_mode)); + if (err) + goto err_unlock; + + spin_unlock(&ej1x8->lock); + + return 0; + +err_unlock: + spin_unlock(&ej1x8->lock); + return err; +} + +static int ej1x8_gpio_get_mode(struct gpio_chip *gc, unsigned int gpio, u8 *mode) +{ + struct ej1x8_gpio *ej1x8 = gpiochip_get_data(gc); + u32 temp, all_gpio_mode; + int err; + + spin_lock(&ej1x8->lock); + err = ej1x8_gpio_read(gc, EJ1X8_GPIO_MODE, &temp); + spin_unlock(&ej1x8->lock); + + if (err) + return err; + + all_gpio_mode = FIELD_GET(EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK, temp); + *mode = all_gpio_mode >> ej1x8_gpio_shift(gpio, EJ1X8_GPIO_MODE_MASK); + *mode &= EJ1X8_GPIO_MODE_MASK; + + return 0; +} + +static void ej1x8_gpio_free(struct gpio_chip *gc, unsigned int gpio) +{ + ej1x8_gpio_config(gc, gpio, EJ1X8_GPIO_MODE_DISABLE, EJ1X8_GPIO_CTRL_IN); +} + +static int ej1x8_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) +{ + u8 mode; + int err; + + err = ej1x8_gpio_get_mode(gc, gpio, &mode); + if (err) + return err; + + switch (mode) { + case EJ1X8_GPIO_CTRL_IN: + return GPIO_LINE_DIRECTION_IN; + + case EJ1X8_GPIO_CTRL_OUT_HIGH: + case EJ1X8_GPIO_CTRL_OUT_LOW: + return GPIO_LINE_DIRECTION_OUT; + + default: + return -EINVAL; + } +} + +static int ej1x8_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio, + int value) +{ + u8 gpio_ctrl; + + if (value) + gpio_ctrl = EJ1X8_GPIO_CTRL_OUT_HIGH; + else + gpio_ctrl = EJ1X8_GPIO_CTRL_OUT_LOW; + + return ej1x8_gpio_config(gc, gpio, EJ1X8_GPIO_MODE_ENABLE, gpio_ctrl); +} + +static int ej1x8_gpio_get_value(struct gpio_chip *gc, unsigned int gpio) +{ + u8 mode; + int err; + + err = ej1x8_gpio_get_mode(gc, gpio, &mode); + if (err) + return err; + + switch (mode) { + case EJ1X8_GPIO_CTRL_OUT_HIGH: + return 1; + + case EJ1X8_GPIO_CTRL_OUT_LOW: + return 0; + + default: + return -EINVAL; + } +} + +static void ej1x8_gpio_set_value(struct gpio_chip *gc, unsigned int gpio, int value) +{ + ej1x8_gpio_direction_output(gc, gpio, value); +} + +static const struct pci_device_id ej1x8_gpio_pci_ids[] __initconst = { + { PCI_DEVICE(PCI_VENDOR_ID_ETRON, PCI_DEVICE_ID_ETRON_EJ168) }, + { PCI_DEVICE(PCI_VENDOR_ID_ETRON, PCI_DEVICE_ID_ETRON_EJ188) }, + { /* sentinel */ } +}; + +static int __init ej1x8_gpio_init(void) +{ + struct pci_dev *pci_dev = NULL; + struct ej1x8_gpio *ej1x8; + int err; + + for_each_pci_dev(pci_dev) { + if (!pci_match_id(ej1x8_gpio_pci_ids, pci_dev)) + continue; + + if (!device_property_read_bool(&pci_dev->dev, "gpio-controller")) + continue; + + ej1x8 = kzalloc(sizeof(*ej1x8), GFP_KERNEL); + if (!ej1x8) + continue; + + spin_lock_init(&ej1x8->lock); + ej1x8->pci_dev = pci_dev_get(pci_dev); + + /* TODO: input mode is supported by the hardware but not the driver */ + ej1x8->chip.label = dev_name(&pci_dev->dev); + ej1x8->chip.owner = THIS_MODULE; + ej1x8->chip.parent = &pci_dev->dev; + ej1x8->chip.of_node = pci_dev->dev.of_node; + ej1x8->chip.free = ej1x8_gpio_free; + ej1x8->chip.get_direction = ej1x8_gpio_get_direction; + ej1x8->chip.direction_output = ej1x8_gpio_direction_output; + ej1x8->chip.get = ej1x8_gpio_get_value; + ej1x8->chip.set = ej1x8_gpio_set_value; + ej1x8->chip.base = -1; + ej1x8->chip.ngpio = 4; + + err = gpiochip_add_data(&ej1x8->chip, ej1x8); + if (err) { + dev_warn(&pci_dev->dev, + "Failed to register GPIO device: %d\n", err); + pci_dev_put(ej1x8->pci_dev); + kfree(ej1x8); + continue; + } + + list_add(&ej1x8->list_head, &ej1x8_gpios); + } + + return 0; +} + +static void __exit ej1x8_gpio_exit(void) +{ + struct ej1x8_gpio *ej1x8, *tmp; + + list_for_each_entry_safe(ej1x8, tmp, &ej1x8_gpios, list_head) { + gpiochip_remove(&ej1x8->chip); + pci_dev_put(ej1x8->pci_dev); + list_del(&ej1x8->list_head); + kfree(ej1x8); + } +} + +module_init(ej1x8_gpio_init); +module_exit(ej1x8_gpio_exit); + +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>"); +MODULE_DESCRIPTION("Etron Technology Inc. EJ168/EJ188/EJ198 GPIO driver"); +MODULE_LICENSE("GPL v2"); -- 2.28.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-10-04 16:29 ` [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 Martin Blumenstingl @ 2020-10-07 9:29 ` Linus Walleij 2020-10-07 19:44 ` Martin Blumenstingl 0 siblings, 1 reply; 23+ messages in thread From: Linus Walleij @ 2020-10-07 9:29 UTC (permalink / raw) To: Martin Blumenstingl, linux-usb Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas Hi Martin, thanks for your patch! As noted on the earlier patches I think this should be folded into the existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that gets messy, as a separate bolt-on, something like xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. You can use a Kconfig symbol for the GPIO portions or not. On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > EJ168/EJ188/EJ198 are USB xHCI controllers. They also contain four GPIO > lines which are used on some systems to toggle an LED based on whether a > USB device is connected. > > There is no public datasheet available for this hardware. All > information in this driver is taken from the > "F9K1115v2.03.97-GPL-10.2.85-20140313" GPL code dump of the Belkin > F9K1115v2. This board comes with an EJ168 USB xHCI controller and the > USB 3.0 LED is connected to one of the GPIOs. Inside the GPL source > archive the related code can be found in: > linux/kernels/mips-linux-2.6.31/drivers/usb/host/etxhci-pci.c > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> (...) > +config GPIO_EJ1X8 > + tristate "Etron Tech Inc. EJ168/EJ188/EJ198 GPIO driver" > + depends on OF_GPIO && PCI It is fine to just select GPIOLIB if you want this to always be compiled-in (if the USB maintainers agree). > + help > + Selecting this option will enable the GPIO pins present on > + the Etron Tech Inc. EJ168/EJ188/EJ198 USB xHCI controllers. > + > + If unsure, say N. (...) > +#define EJ1X8_GPIO_INIT 0x44 > +#define EJ1X8_GPIO_WRITE 0x68 > +#define EJ1X8_GPIO_READ 0x6c > + > +#define EJ1X8_GPIO_CTRL 0x18005020 > +#define EJ1X8_GPIO_CTRL_READ_ALL_MASK GENMASK(7, 0) > +#define EJ1X8_GPIO_CTRL_WRITE_ALL_MASK GENMASK(23, 16) > +#define EJ1X8_GPIO_CTRL_OUT_LOW 0x0 > +#define EJ1X8_GPIO_CTRL_OUT_HIGH 0x1 > +#define EJ1X8_GPIO_CTRL_IN 0x2 > +#define EJ1X8_GPIO_CTRL_MASK 0x3 > + > +#define EJ1X8_GPIO_MODE 0x18005022 > +#define EJ1X8_GPIO_MODE_READ_WRITE_ALL_MASK GENMASK(23, 16) > +#define EJ1X8_GPIO_MODE_DISABLE 0x0 > +#define EJ1X8_GPIO_MODE_ENABLE 0x1 > +#define EJ1X8_GPIO_MODE_MASK 0x3 Nice that you got all of this out of reverse-engineering! > +static LIST_HEAD(ej1x8_gpios); This should not be necessary. Tie the GPIO state into the PCI device driver state, possibly using some #ifdefs. > +static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask) > +{ > + return (gpio * fls(mask)); > +} > + > +static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask) > +{ > + return mask << ej1x8_gpio_shift(gpio, mask); > +} This looks a bit like regmap but trying to use regmap for this would probably be overengineering. Looking at the code I get annoyed that it uses the config space to manipulate the GPIOs, else you could have used GPIO_GENERIC but now you can't, how typical. Other than that the code looks nice, but fold it into the USB host driver somehow unless there is a compelling argument as to why not. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-10-07 9:29 ` Linus Walleij @ 2020-10-07 19:44 ` Martin Blumenstingl 2020-12-21 15:28 ` Martin Blumenstingl 0 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2020-10-07 19:44 UTC (permalink / raw) To: Linus Walleij Cc: linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas Hi Linus, On Wed, Oct 7, 2020 at 11:29 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Martin, > > thanks for your patch! thank you for reviewing the whole series! > As noted on the earlier patches I think this should be folded into the > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that > gets messy, as a separate bolt-on, something like > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. > You can use a Kconfig symbol for the GPIO portions or not. OK, I will do that if there are no objections from other developers I am intending to place the relevant code in xhci-pci-etron.c, similar to what we already have with xhci-pci-renesas.c [...] > This should not be necessary. Tie the GPIO state into the PCI device > driver state, possibly using some #ifdefs. > > > +static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask) > > +{ > > + return (gpio * fls(mask)); > > +} > > + > > +static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask) > > +{ > > + return mask << ej1x8_gpio_shift(gpio, mask); > > +} > > This looks a bit like regmap but trying to use regmap for this > would probably be overengineering. the problem is also the "INIT" register which needs to be set before writing the registers > Looking at the code I get annoyed that it uses the config space to > manipulate the GPIOs, else you could have used GPIO_GENERIC > but now you can't, how typical. I think this won't work in practice because of the EJ1X8_GPIO_CTRL for which we have to read from bits [7:0] but write to bits [23:16] due to this (and the INIT register as mentioned above) I did not consider GPIO_GENERIC any further > Other than that the code looks nice, but fold it into the USB > host driver somehow unless there is a compelling argument > as to why not. will do so, thanks! Best regards, Martin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-10-07 19:44 ` Martin Blumenstingl @ 2020-12-21 15:28 ` Martin Blumenstingl 2020-12-31 0:15 ` Martin Blumenstingl 2021-01-05 22:23 ` Linus Walleij 0 siblings, 2 replies; 23+ messages in thread From: Martin Blumenstingl @ 2020-12-21 15:28 UTC (permalink / raw) To: Linus Walleij Cc: linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas Hi Linus, On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: [...] > > As noted on the earlier patches I think this should be folded into the > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that > > gets messy, as a separate bolt-on, something like > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. > > You can use a Kconfig symbol for the GPIO portions or not. > OK, I will do that if there are no objections from other developers > I am intending to place the relevant code in xhci-pci-etron.c, similar > to what we already have with xhci-pci-renesas.c I tried this and unfortunately there's a catch. the nice thing about having a separate GPIO driver means that the xhci-pci driver doesn't need to know about it. I implemented xhci-pci-etron.c and gave it a Kconfig option. xhci-pci is then calling into xhci-pci-etron (through some etron_xhci_pci_probe function). unfortunately this means that xhci-pci now depends on xhci-pci-etron. for xhci-pci-renesas this is fine (I think) because that part of the code is needed to get the xHCI controller going but for xhci-pci-etron this is a different story: the GPIO controller is entirely optional and only used on few devices my goal is (at some point in the future) to have the GPIO driver in OpenWrt. I am not sure if they would accept a patch where xhci-pci would then pull in the dependencies for that Etron controller, even though most boards don't need it. Please let me know if you have any idea on how to solve this. Best regards, Martin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-12-21 15:28 ` Martin Blumenstingl @ 2020-12-31 0:15 ` Martin Blumenstingl 2021-01-05 22:23 ` Linus Walleij 1 sibling, 0 replies; 23+ messages in thread From: Martin Blumenstingl @ 2020-12-31 0:15 UTC (permalink / raw) To: Linus Walleij Cc: linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas Hi Linus, On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Linus, > > On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] > > > As noted on the earlier patches I think this should be folded into the > > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that > > > gets messy, as a separate bolt-on, something like > > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. > > > You can use a Kconfig symbol for the GPIO portions or not. > > OK, I will do that if there are no objections from other developers > > I am intending to place the relevant code in xhci-pci-etron.c, similar > > to what we already have with xhci-pci-renesas.c > I tried this and unfortunately there's a catch. > the nice thing about having a separate GPIO driver means that the > xhci-pci driver doesn't need to know about it. > > I implemented xhci-pci-etron.c and gave it a Kconfig option. > xhci-pci is then calling into xhci-pci-etron (through some > etron_xhci_pci_probe function). > unfortunately this means that xhci-pci now depends on xhci-pci-etron. > for xhci-pci-renesas this is fine (I think) because that part of the > code is needed to get the xHCI controller going > but for xhci-pci-etron this is a different story: the GPIO controller > is entirely optional and only used on few devices > > my goal is (at some point in the future) to have the GPIO driver in OpenWrt. > I am not sure if they would accept a patch where xhci-pci would then > pull in the dependencies for that Etron controller, even though most > boards don't need it. > > Please let me know if you have any idea on how to solve this. next week I have some free time to work on this I am still interested in any ideas that you have about this Best regards, Martin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2020-12-21 15:28 ` Martin Blumenstingl 2020-12-31 0:15 ` Martin Blumenstingl @ 2021-01-05 22:23 ` Linus Walleij 2021-01-06 15:17 ` Martin Blumenstingl 1 sibling, 1 reply; 23+ messages in thread From: Linus Walleij @ 2021-01-05 22:23 UTC (permalink / raw) To: Martin Blumenstingl, Bjorn Helgaas Cc: linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > [...] > > > As noted on the earlier patches I think this should be folded into the > > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that > > > gets messy, as a separate bolt-on, something like > > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. > > > You can use a Kconfig symbol for the GPIO portions or not. > > OK, I will do that if there are no objections from other developers > > I am intending to place the relevant code in xhci-pci-etron.c, similar > > to what we already have with xhci-pci-renesas.c > > I tried this and unfortunately there's a catch. > the nice thing about having a separate GPIO driver means that the > xhci-pci driver doesn't need to know about it. Since PCI devices have device-wide power management and things like that I think that is a really dangerous idea. What if the GPIO driver starts poking around in this PCI device when the main driver is also probed and has put the device into sleep state? This type of set-up needs to be discussed with the PCI maintainer to make sure it is safe. > I implemented xhci-pci-etron.c and gave it a Kconfig option. > xhci-pci is then calling into xhci-pci-etron (through some > etron_xhci_pci_probe function). This sounds about right. > unfortunately this means that xhci-pci now depends on xhci-pci-etron. > for xhci-pci-renesas this is fine (I think) because that part of the > code is needed to get the xHCI controller going > but for xhci-pci-etron this is a different story: the GPIO controller > is entirely optional and only used on few devices I might be naive but should it not be the other way around? That xhci-pci-etron is dependent on xhci-pci? I imagine it would be an optional add-on. > my goal is (at some point in the future) to have the GPIO driver in OpenWrt. > I am not sure if they would accept a patch where xhci-pci would then > pull in the dependencies for that Etron controller, even though most > boards don't need it. Make sure the etron part is an additional module that can be loaded after xhci-pci. OpenWrt support optional modules to be compiled per-system. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2021-01-05 22:23 ` Linus Walleij @ 2021-01-06 15:17 ` Martin Blumenstingl 2021-01-07 10:36 ` Linus Walleij 0 siblings, 1 reply; 23+ messages in thread From: Martin Blumenstingl @ 2021-01-06 15:17 UTC (permalink / raw) To: Linus Walleij Cc: Bjorn Helgaas, linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel Hi Linus, On Tue, Jan 5, 2021 at 11:23 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Dec 21, 2020 at 4:28 PM Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > On Wed, Oct 7, 2020 at 9:44 PM Martin Blumenstingl > > <martin.blumenstingl@googlemail.com> wrote: > > [...] > > > > As noted on the earlier patches I think this should be folded into the > > > > existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that > > > > gets messy, as a separate bolt-on, something like > > > > xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory. > > > > You can use a Kconfig symbol for the GPIO portions or not. > > > OK, I will do that if there are no objections from other developers > > > I am intending to place the relevant code in xhci-pci-etron.c, similar > > > to what we already have with xhci-pci-renesas.c > > > > I tried this and unfortunately there's a catch. > > the nice thing about having a separate GPIO driver means that the > > xhci-pci driver doesn't need to know about it. > > Since PCI devices have device-wide power management and things > like that I think that is a really dangerous idea. > > What if the GPIO driver starts poking around in this PCI device > when the main driver is also probed and has put the device > into sleep state? that is asking for trouble, indeed. [...] > > I implemented xhci-pci-etron.c and gave it a Kconfig option. > > xhci-pci is then calling into xhci-pci-etron (through some > > etron_xhci_pci_probe function). > > This sounds about right. > > > unfortunately this means that xhci-pci now depends on xhci-pci-etron. > > for xhci-pci-renesas this is fine (I think) because that part of the > > code is needed to get the xHCI controller going > > but for xhci-pci-etron this is a different story: the GPIO controller > > is entirely optional and only used on few devices > > I might be naive but should it not be the other way around? > That xhci-pci-etron is dependent on xhci-pci? I imagine > it would be an optional add-on. the only way to achieve this that I can think of is to basically have xhci-pci-etron implement it's own pci_driver and then call xhci_pci_probe, xhci_pci_remove, etc. but then it depends on the driver load order if the GPIO controller is exposed what structure did you have in mind to achieve this? > > my goal is (at some point in the future) to have the GPIO driver in OpenWrt. > > I am not sure if they would accept a patch where xhci-pci would then > > pull in the dependencies for that Etron controller, even though most > > boards don't need it. > > Make sure the etron part is an additional module that can be > loaded after xhci-pci. my approach from above unfortunately would not achieve this so if you have an idea how to achieve this (or have any other driver in mind that I can use as reference, even if not related to GPIO/USB/PCI then please let me know) > OpenWrt support optional modules to be compiled per-system. that I already found out. That's why I think that I need to get the driver part "right" and then get the OpenWrt part done in just a few lines of their build-system Best regards, Martin ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 2021-01-06 15:17 ` Martin Blumenstingl @ 2021-01-07 10:36 ` Linus Walleij 0 siblings, 0 replies; 23+ messages in thread From: Linus Walleij @ 2021-01-07 10:36 UTC (permalink / raw) To: Martin Blumenstingl Cc: Bjorn Helgaas, linux-usb, linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel On Wed, Jan 6, 2021 at 4:17 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > > unfortunately this means that xhci-pci now depends on xhci-pci-etron. > > > for xhci-pci-renesas this is fine (I think) because that part of the > > > code is needed to get the xHCI controller going > > > but for xhci-pci-etron this is a different story: the GPIO controller > > > is entirely optional and only used on few devices > > > > I might be naive but should it not be the other way around? > > That xhci-pci-etron is dependent on xhci-pci? I imagine > > it would be an optional add-on. > > the only way to achieve this that I can think of is to basically have > xhci-pci-etron implement it's own pci_driver and then call > xhci_pci_probe, xhci_pci_remove, etc. > but then it depends on the driver load order if the GPIO controller is exposed > > what structure did you have in mind to achieve this? Something that is compiled and called conditionally with stubs in the local .h file. Kconfig: config FOO tristate "Main matter" config FOO_ADD_ON tristate "Optional on" depends on FOO Makefile: obj-$(CONFIG_FOO) += foo.o obj-$(CONFIG_FOO_ADD_ON) += foo-add-on.o foo.h: struct foo { ... }; #if IS_ENABLED(CONFIG_FOO_ADD_ON) int foo_add_on_init(struct foo *); #else /* No CONFIG_FOO_ADD_ON */ static int foo_add_on_init(struct foo *) { return 0; } #endif foo.c: #include "foo.h" ret = foo_add_on_init(foo); (...) foo-add-on.c: int foo_add_on_init(struct foo *) { (...) } EXPORT_SYMBOL_GPL(foo_add_on_init); > > Make sure the etron part is an additional module that can be > > loaded after xhci-pci. > > my approach from above unfortunately would not achieve this > so if you have an idea how to achieve this (or have any other driver > in mind that I can use as reference, even if not related to > GPIO/USB/PCI then please let me know) See per above. I don't see any problem with this, it will be an additional module that does not feature a probe() call and device driver bind. I think it is also possible to link both files into the same object if the optional add on is enabled, so it is part of the main module when modprobing. The foo.h stubs are still needed, then the binary will just be smaller if the add-on is not enabled. There are solutions like this in the kernel, I just don't remember one right now so grep around. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers 2020-10-04 16:29 [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Martin Blumenstingl ` (2 preceding siblings ...) 2020-10-04 16:29 ` [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 Martin Blumenstingl @ 2020-10-07 9:17 ` Linus Walleij 3 siblings, 0 replies; 23+ messages in thread From: Linus Walleij @ 2020-10-07 9:17 UTC (permalink / raw) To: Martin Blumenstingl, linux-usb Cc: linux-pci, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, open list:GPIO SUBSYSTEM, Rob Herring, Bartosz Golaszewski, linux-kernel, Bjorn Helgaas, systemchip On Sun, Oct 4, 2020 at 8:00 PM Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > The goal of this series to add support for the GPIO controller on the > Etron EJ168/EJ188/EJ198 controllers. This overall is a fine driver, but have you considered the option of just implementing the GPIO chip in drivers/usb/host/xhci-pci.c? There are several USB serial adapters that have a GPIO chip embedded and we just add the GPIO chip into the serial driver. I have done the same with some networking switches. It is perfectly fine for drivers outside of drivers/gpio to occasionally define a minor GPIO chip if GPIO is not their primary function. Please consider simply activating the XHCI driver and make it instantiate a GPIO chip if it happens to be an EJ168/EJ188/EJ198 controller. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2021-01-07 10:37 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-04 16:29 [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Martin Blumenstingl 2020-10-04 16:29 ` [RFC PATCH 1/3] PCI: Add the IDs for Etron EJ168 and EJ188 Martin Blumenstingl 2020-10-07 9:14 ` Linus Walleij 2020-10-07 19:45 ` Martin Blumenstingl 2020-11-03 22:32 ` Bjorn Helgaas 2020-10-04 16:29 ` [RFC PATCH 2/3] dt-bindings: gpio: Add binding documentation for Etron EJ168/EJ188/EJ198 Martin Blumenstingl 2020-10-06 21:25 ` Rob Herring 2020-10-07 19:57 ` Martin Blumenstingl 2020-10-07 9:19 ` Linus Walleij 2020-10-07 19:57 ` Martin Blumenstingl 2020-10-13 13:27 ` Linus Walleij 2020-10-14 12:43 ` Rob Herring 2020-10-16 20:52 ` Martin Blumenstingl 2020-10-29 17:11 ` Linus Walleij 2020-10-04 16:29 ` [RFC PATCH 3/3] gpio: ej1x8: Add GPIO driver for Etron Tech Inc. EJ168/EJ188/EJ198 Martin Blumenstingl 2020-10-07 9:29 ` Linus Walleij 2020-10-07 19:44 ` Martin Blumenstingl 2020-12-21 15:28 ` Martin Blumenstingl 2020-12-31 0:15 ` Martin Blumenstingl 2021-01-05 22:23 ` Linus Walleij 2021-01-06 15:17 ` Martin Blumenstingl 2021-01-07 10:36 ` Linus Walleij 2020-10-07 9:17 ` [RFC PATCH 0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).