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

On Fri, Jun 04, 2021 at 11:31:07AM +0200, Linus Walleij wrote:

Hi Jonathan!

thanks for your patch!

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

quoted

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.

quoted

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>

(...)

quoted

+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

I'll remove the this line (depending on the outcome of the rest of the
discussion).

quoted

+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?

The naming difference is a fairly arbitrary choice by me.

The real difference is in how the GPIO registers are laid out.
On NPCM7xx, there are blocks of registers at +0, +0x1000, +0x2000,
etc., and within a block, the registers have the same offsets.
On WPCM450, the registers are all mushed together as tightly as
possible[1], so that (a) the ports/banks don't start at nice addresses,
and (b) the register layout can't be predicted from the offset of the
first register in a port (because not all ports have all registers).

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.

Indeed, if I keep the wpcm450_ports table but use it to call bgpio_init()
with the right register addresses, I think bgpio_init() can work.


Thanks,
Jonathan Neuschäfer


[1]: https://github.com/neuschaefer/wpcm450/wiki/GPIOs-and-pinmux#gpio