Re: [PATCH 5/8] pinctrl: nuvoton: Add driver for WPCM450

Hi Jonathan!

thanks for your patch!

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
[off-list ref] wrote:

This driver is heavily based on the one for NPCM7xx, because the WPCM450
is a predecessor of those SoCs.

The biggest difference is in how the GPIO controller works. In the
WPCM450, the GPIO registers are not organized in multiple banks, but
rather placed continually into the same register block, and the driver
reflects this.

This is unfortunate because now you can't use GPIO_GENERIC anymore.

Some functionality implemented in the hardware was (for now) left unused
in the driver, specifically blinking and pull-up/down.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

(...)

+config PINCTRL_WPCM450
+       bool "Pinctrl and GPIO driver for Nuvoton WPCM450"
+       depends on (ARCH_WPCM450 || COMPILE_TEST) && OF
+       select PINMUX
+       select PINCONF
+       select GENERIC_PINCONF
+       select GPIOLIB
+       select GPIO_GENERIC

You are not using GPIO_GENERIC

+struct wpcm450_port {
+       /* Range of GPIOs in this port */
+       u8 base;
+       u8 length;
+
+       /* Register offsets (0 = register doesn't exist in this port) */
+       u8 cfg0, cfg1, cfg2;
+       u8 blink;
+       u8 dataout, datain;
+};

If you used to have "GPIO banks" and you now have
"GPIO ports" what is the difference? Why can't these ports
just be individula gpio_chip:s with their own device tree
nodes inside the pin controller node?

If you split it up then you can go back to using
GPIO_GENERIC with bgpio_init() again which is a
big win.

All you seem to be doing is setting consecutive
bits in a register by offset, which is what GPIO_GENERIC
is for, just that it assumes offset is always from zero.
If you split it into individual gpio_chips per register
then you get this nice separation and code reuse.

+static const struct wpcm450_port *to_port(int offset)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(wpcm450_ports); i++)
+               if (offset >= wpcm450_ports[i].base &&
+                   offset - wpcm450_ports[i].base < wpcm450_ports[i].length)
+                       return &wpcm450_ports[i];
+       return NULL;
+}

Then you would also get away from this awkward thing.

+static u32 port_mask(const struct wpcm450_port *port, int offset)
+{
+       return BIT(offset - port->base);
+}

And awkwardness like this.

Generally splitting up gpio_chips is a very good idea.

+static int event_bitmask(int gpio)
+{
+       if (gpio >= 0 && gpio < 16)
+               return BIT(gpio);
+       if (gpio == 24 || gpio == 25)
+               return BIT(gpio - 8);
+       return -EINVAL;
+}
+
+static int event_bitnum_to_gpio(int num)
+{
+       if (num >= 0 && num < 16)
+               return num;
+       if (num == 16 || num == 17)
+               return num + 8;
+       return -EINVAL;
+}

This is also a sign that you have several gpio_chips in practice
and now you need all this awkwardness to get back to which
GPIO is which instead of handling it in a per-chip manner.

This can be done in different ways, the most radical is to have
the pin control driver spawn child devices for each GPIO
block/bank/port with its own driver, but it can also just register
the individual gpio_chips.

Yours,
Linus Walleij