mbox series

[RFC,0/3] GPIO support on the Etron EJ168/EJ188/EJ198 xHCI controllers

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

Message

Martin Blumenstingl Oct. 4, 2020, 4:29 p.m. UTC
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

Comments

Linus Walleij Oct. 7, 2020, 9:17 a.m. UTC | #1
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