[PATCH 0/8] Nuvoton WPCM450 pinctrl and GPIO driver

[PATCH 0/8] Nuvoton WPCM450 pinctrl and GPIO driver

[Jonathan Neuschäfer]

This series adds support for pinctrl and GPIO in the Nuvoton WPCM450 SoC.
Both my DT bindings and my driver are based on the work done by others for
the newer Nuvoton NPCM7xx SoC, and I've tried to keep both similar.

Instead of extending the pinctrl-npcm7xx driver to add WPCM450 support,
I copied/forked it. The pinmux mechanism is very similar (using MFSEL1 and
MFSEL2 registers), but the GPIO register interface has been redesigned for
NPCM7xx; adding support for the older GPIO controller would make the driver
harder to understand, while only enabling a small amount of code sharing.

The DT binding in YAML format might make a good template for also converting
the nuvoton,npcm7xx-pinctrl binding to YAML.

Both in the DT binding and in the driver I kept the name "pinctrl". For the
driver, I find it accurate enough because it handles pinctrl and GPIO. For
the DT node, it's a bit less accurate because the register block at 0xb8003000
is about GPIOs, and pin control happens in the global control registers (GCR)
block, except for input debouncing. So, "GPIO" might be the more appropriate
name component there.


Jonathan Neuschäfer (8):
  dt-bindings: arm/npcm: Add binding for global control registers (GCR)
  MAINTAINERS: Match all of bindings/arm/npcm/ as part of NPCM
    architecture
  ARM: dts: wpcm450: Add global control registers (GCR) node
  dt-bindings: pinctrl: Add Nuvoton WPCM450
  pinctrl: nuvoton: Add driver for WPCM450
  ARM: dts: wpcm450: Add pinctrl node
  ARM: dts: wpcm450: Add pin functions
  ARM: dts: wpcm450-supermicro-x9sci-ln4f: Add GPIO LEDs and buttons

 .../bindings/arm/npcm/nuvoton,gcr.yaml        |   38 +
 .../pinctrl/nuvoton,wpcm450-pinctrl.yaml      |  142 ++
 MAINTAINERS                                   |    2 +
 .../nuvoton-wpcm450-supermicro-x9sci-ln4f.dts |   27 +
 arch/arm/boot/dts/nuvoton-wpcm450.dtsi        |  321 +++++
 drivers/pinctrl/Makefile                      |    2 +-
 drivers/pinctrl/nuvoton/Kconfig               |   14 +
 drivers/pinctrl/nuvoton/Makefile              |    1 +
 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c     | 1230 +++++++++++++++++
 9 files changed, 1776 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
 create mode 100644 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c

--
2.30.2

[PATCH 1/8] dt-bindings: arm/npcm: Add binding for global control registers (GCR)

[Jonathan Neuschäfer]

A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will
be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and
WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and
version information.

This patch adds a binding to describe this node.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../bindings/arm/npcm/nuvoton,gcr.yaml        | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml

diff --git a/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
new file mode 100644
index 0000000000000..3174279f7713a
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/npcm/nuvoton,gcr.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Global Control Registers block in Nuvoton SoCs
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+description: |
+  The Global Control Registers (GCR) are a block of registers in Nuvoton SoCs
+  that expose misc functionality such as chip model and version information or
+  pinmux settings.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - nuvoton,wpcm450-gcr
+          - nuvoton,npcm750-gcr
+      - const: syscon
+      - const: simple-mfd
+  reg: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    gcr: gcr@800000 {
+      compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd";
+      reg = <0x800000 0x1000>;
+    };
--
2.30.2

[PATCH 2/8] MAINTAINERS: Match all of bindings/arm/npcm/ as part of NPCM architecture

[Jonathan Neuschäfer]

All files in Documentation/devicetree/bindings/arm/npcm/ belong to the
Nuvoton NPCM architecture, even when their names might not spell it out
explicitly.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 79fb23f576218..288d6a1f2bb1f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2220,6 +2220,7 @@ L:	openbmc@lists.ozlabs.org (moderated for non-subscribers)
 S:	Supported
 F:	Documentation/devicetree/bindings/*/*/*npcm*
 F:	Documentation/devicetree/bindings/*/*npcm*
+F:	Documentation/devicetree/bindings/arm/npcm/*
 F:	arch/arm/boot/dts/nuvoton-npcm*
 F:	arch/arm/mach-npcm/
 F:	drivers/*/*npcm*
--
2.30.2

[PATCH 3/8] ARM: dts: wpcm450: Add global control registers (GCR) node

[Jonathan Neuschäfer]

The Global Control Registers (GCR) are a block of registers in Nuvoton
SoCs that expose misc functionality such as chip model and version
information or pinmux settings.

This patch adds a GCR node to nuvoton-wpcm450.dtsi in preparation for
enabling pinctrl on this SoC.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/arm/boot/dts/nuvoton-wpcm450.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
index d7cbeb1874840..8eba4897b41bc 100644
--- a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
+++ b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
@@ -33,6 +33,11 @@ soc {
 		interrupt-parent = <&aic>;
 		ranges;

+		gcr: gcr@b0000000 {
+			compatible = "nuvoton,wpcm450-gcr", "syscon", "simple-mfd";
+			reg = <0xb0000000 0x200>;
+		};
+
 		serial0: serial@b8000000 {
 			compatible = "nuvoton,wpcm450-uart";
 			reg = <0xb8000000 0x20>;
--
2.30.2

[PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

[Jonathan Neuschäfer]

This binding is heavily based on the one for NPCM7xx, because the
hardware is similar. One notable difference is that there are no
sub-nodes for GPIO banks, because the GPIO registers are arranged
differently.

Certain pins support blink patterns in hardware. This is currently not
modelled in the DT binding.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../pinctrl/nuvoton,wpcm450-pinctrl.yaml      | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
new file mode 100644
index 0000000000000..0664fe2b90db6
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
@@ -0,0 +1,142 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/nuvoton,wpcm450-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton WPCM450 pin control and GPIO
+
+maintainers:
+  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
+
+properties:
+  compatible:
+    const: "nuvoton,wpcm450-pinctrl"
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts: true
+
+patternProperties:
+  # There are two kinds of subnodes:
+  # 1. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
+  # 2. a pinctrl node configures properties of a single pin
+  "^.*$":
+    if:
+      type: object
+    then:
+      allOf:
+        - $ref: pincfg-node.yaml#
+        - $ref: pinmux-node.yaml#
+      properties:
+        groups:
+          description:
+            One or more groups of pins to mux to a certain function
+          minItems: 1
+          items:
+            enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
+                    hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo,
+                    clko, smi, uinc, gspi, mben, xcs2, xcs1, sdio, sspi, fi0,
+                    fi1, fi2, fi3, fi4, fi5, fi6, fi7, fi8, fi9, fi10, fi11,
+                    fi12, fi13, fi14, fi15, pwm0, pwm1, pwm2, pwm3, pwm4, pwm5,
+                    pwm6, pwm7, hg0, hg1, hg2, hg3, hg4, hg5, hg6, hg7 ]
+        function:
+          description:
+            The function that a group of pins is muxed to
+          enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
+                  hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo0,
+                  dvo1, dvo2, dvo3, dvo4, dvo5, dvo6, dvo7, clko, smi, uinc,
+                  gspi, mben, xcs2, xcs1, sdio, sspi, fi0, fi1, fi2, fi3, fi4,
+                  fi5, fi6, fi7, fi8, fi9, fi10, fi11, fi12, fi13, fi14, fi15,
+                  pwm0, pwm1, pwm2, pwm3, pwm4, pwm5, pwm6, pwm7, hg0, hg1,
+                  hg2, hg3, hg4, hg5, hg6, hg7 ]
+
+        pins:
+          description:
+            A list of pins to configure in certain ways, such as enabling
+            debouncing
+          minItems: 1
+          items:
+            enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7,
+                    gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio14,
+                    gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gpio21,
+                    gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, gpio28,
+                    gpio29, gpio30, gpio31, gpio32, gpio33, gpio34, gpio35,
+                    gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, gpio42,
+                    gpio43, gpio44, gpio45, gpio46, gpio47, gpio48, gpio49,
+                    gpio50, gpio51, gpio52, gpio53, gpio54, gpio55, gpio56,
+                    gpio57, gpio58, gpio59, gpio60, gpio61, gpio62, gpio63,
+                    gpio64, gpio65, gpio66, gpio67, gpio68, gpio69, gpio70,
+                    gpio71, gpio72, gpio73, gpio74, gpio75, gpio76, gpio77,
+                    gpio78, gpio79, gpio80, gpio81, gpio82, gpio83, gpio84,
+                    gpio85, gpio86, gpio87, gpio88, gpio89, gpio90, gpio91,
+                    gpio92, gpio93, gpio94, gpio95, gpio96, gpio97, gpio98,
+                    gpio99, gpio100, gpio101, gpio102, gpio103, gpio104,
+                    gpio105, gpio106, gpio107, gpio108, gpio109, gpio110,
+                    gpio111, gpio112, gpio113, gpio114, gpio115, gpio116,
+                    gpio117, gpio118, gpio119, gpio120, gpio121, gpio122,
+                    gpio123, gpio124, gpio125, gpio126, gpio127 ]
+
+        input-debounce: true
+        phandle: true
+
+      dependencies:
+        groups: [ function ]
+        function: [ groups ]
+
+      additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - '#gpio-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+    pinctrl: pinctrl@b8003000 {
+      compatible = "nuvoton,wpcm450-pinctrl";
+      reg = <0xb8003000 0x1000>;
+      gpio-controller;
+      #gpio-cells = <2>;
+      interrupts = <2 IRQ_TYPE_LEVEL_HIGH
+                    3 IRQ_TYPE_LEVEL_HIGH
+                    4 IRQ_TYPE_LEVEL_HIGH
+                    5 IRQ_TYPE_LEVEL_HIGH>;
+      rmii2 {
+        groups = "rmii2";
+        function = "rmii2";
+      };
+
+      pinctrl_uid: uid {
+        pins = "gpio14";
+        input-debounce = <1>;
+      };
+    };
+
+    gpio-keys {
+      compatible = "gpio-keys";
+      pinctrl-names = "default";
+      pinctrl-0 = <&pinctrl_uid>;
+
+      uid {
+        label = "UID";
+        linux,code = <102>;
+        gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;
+      };
+    };
--
2.30.2

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

[Jonathan Neuschäfer]

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.

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

This patch has a few checkpatch warnings, which are inherited from the
pinctrl-npcm7xx driver. I decided to ignore those. Specifically:
(1) the Kconfig help text is considered too short; (2) WPCM450_GRPS is
an unwrapped list; (3) use of -ENOTSUPP is discouraged.
---
 MAINTAINERS                               |    1 +
 drivers/pinctrl/Makefile                  |    2 +-
 drivers/pinctrl/nuvoton/Kconfig           |   14 +
 drivers/pinctrl/nuvoton/Makefile          |    1 +
 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c | 1230 +++++++++++++++++++++
 5 files changed, 1247 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 288d6a1f2bb1f..ca5cb7f94b115 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2234,6 +2234,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/*/*wpcm*
 F:	arch/arm/boot/dts/nuvoton-wpcm450*
 F:	arch/arm/mach-npcm/wpcm450.c
+F:	drivers/*/*/*wpcm*
 F:	drivers/*/*wpcm*

 ARM/OPENMOKO NEO FREERUNNER (GTA02) MACHINE SUPPORT
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 5ef5334a797fc..e355c78eecd58 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -56,7 +56,7 @@ obj-y				+= freescale/
 obj-$(CONFIG_X86)		+= intel/
 obj-y				+= mvebu/
 obj-y				+= nomadik/
-obj-$(CONFIG_ARCH_NPCM7XX)	+= nuvoton/
+obj-y				+= nuvoton/
 obj-$(CONFIG_PINCTRL_PXA)	+= pxa/
 obj-$(CONFIG_ARCH_QCOM)		+= qcom/
 obj-$(CONFIG_PINCTRL_RALINK)	+= ralink/
diff --git a/drivers/pinctrl/nuvoton/Kconfig b/drivers/pinctrl/nuvoton/Kconfig
index 48ba0469edda6..f15d345b90083 100644
--- a/drivers/pinctrl/nuvoton/Kconfig
+++ b/drivers/pinctrl/nuvoton/Kconfig
@@ -1,4 +1,18 @@
 # SPDX-License-Identifier: GPL-2.0-only
+
+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
+	select GPIOLIB_IRQCHIP
+	help
+	  Say Y here to enable pin controller and GPIO support
+	  for the Nuvoton WPCM450 SoC.
+
 config PINCTRL_NPCM7XX
 	bool "Pinctrl and GPIO driver for Nuvoton NPCM7XX"
 	depends on (ARCH_NPCM7XX || COMPILE_TEST) && OF
diff --git a/drivers/pinctrl/nuvoton/Makefile b/drivers/pinctrl/nuvoton/Makefile
index 886d00784cef5..9e66f5dc74bfc 100644
--- a/drivers/pinctrl/nuvoton/Makefile
+++ b/drivers/pinctrl/nuvoton/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 # Nuvoton pinctrl support

+obj-$(CONFIG_PINCTRL_WPCM450)	+= pinctrl-wpcm450.o
 obj-$(CONFIG_PINCTRL_NPCM7XX)	+= pinctrl-npcm7xx.o
diff --git a/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
new file mode 100644
index 0000000000000..b978b315ffc68
--- /dev/null
+++ b/drivers/pinctrl/nuvoton/pinctrl-wpcm450.c
@@ -0,0 +1,1230 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2016-2018 Nuvoton Technology corporation.
+// Copyright (c) 2016, Dell Inc
+// Copyright (c) 2021 Jonathan Neuschäfer
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pinctrl/machine.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* GCR registers */
+#define WPCM450_GCR_MFSEL1	0x0C
+#define WPCM450_GCR_MFSEL2	0x10
+#define WPCM450_GCR_NONE	0
+
+/* GPIO event (interrupt) registers */
+#define WPCM450_GPEVTYPE	0x00
+#define WPCM450_GPEVPOL		0x04
+#define WPCM450_GPEVDBNC	0x08
+#define WPCM450_GPEVEN		0x0c
+#define WPCM450_GPEVST		0x10
+
+#define WPCM450_NUM_PORTS	8
+#define WPCM450_NUM_GPIOS	128
+#define WPCM450_NUM_GPIO_IRQS	4
+
+struct wpcm450_pinctrl {
+	struct pinctrl_dev	*pctldev;
+	struct device		*dev;
+	struct irq_domain	*domain;
+	struct regmap		*gcr_regmap;
+
+	void __iomem		*gpio_base;
+	struct gpio_chip	gc;
+	struct irq_chip		irqc;
+	u32			both_edges;
+
+	/*
+	 * This spinlock protects registers and struct wpcm450_pinctrl fields
+	 * against concurrent access.
+	 */
+	spinlock_t		lock;
+};
+
+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;
+};
+
+static const struct wpcm450_port wpcm450_ports[WPCM450_NUM_PORTS] = {
+	/*  range   cfg0  cfg1  cfg2 blink  out   in  */
+	{   0, 16,  0x14, 0x18,    0,    0, 0x1c, 0x20 },
+	{  16, 16,  0x24, 0x28, 0x2c, 0x30, 0x34, 0x38 },
+	{  32, 16,  0x3c, 0x40, 0x44,    0, 0x48, 0x4c },
+	{  48, 16,  0x50, 0x54, 0x58,    0, 0x5c, 0x60 },
+	{  64, 16,  0x64, 0x68, 0x6c,    0, 0x70, 0x74 },
+	{  80, 16,  0x78, 0x7c, 0x80,    0, 0x84, 0x88 },
+	{  96, 18,     0,    0,    0,    0,    0, 0x8c },
+	{ 114, 14,  0x90, 0x94, 0x98,    0, 0x9c, 0xa0 },
+};
+
+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;
+}
+
+static u32 port_mask(const struct wpcm450_port *port, int offset)
+{
+	return BIT(offset - port->base);
+}
+
+/* GPIO handling in the pinctrl driver */
+
+static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct wpcm450_port *port = to_port(offset);
+	unsigned long flags;
+	u32 cfg0;
+	int dir;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	if (port->cfg0) {
+		cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
+		dir = !(cfg0 & port_mask(port, offset));
+	} else {
+		/* If cfg0 is unavailable, the GPIO is always an input */
+		dir = 1;
+	}
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+	return dir;
+}
+
+static int wpcm450_gpio_direction_input(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct wpcm450_port *port = to_port(offset);
+	unsigned long flags;
+	u32 cfg0;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	if (port->cfg0) {
+		cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
+		cfg0 &= ~port_mask(port, offset);
+		iowrite32(cfg0, pctrl->gpio_base + port->cfg0);
+	}
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+	return 0;
+}
+
+static int wpcm450_gpio_direction_output(struct gpio_chip *chip,
+					 unsigned int offset, int value)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct wpcm450_port *port = to_port(offset);
+	unsigned long flags;
+	u32 dataout, cfg0;
+	int ret = 0;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	if (port->cfg0) {
+		/* set output value first, direction second, to avoid glitches */
+		dataout = ioread32(pctrl->gpio_base + port->dataout);
+		if (value)
+			dataout |= port_mask(port, offset);
+		else
+			dataout &= ~port_mask(port, offset);
+		iowrite32(dataout, pctrl->gpio_base + port->dataout);
+
+		cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
+		cfg0 |= port_mask(port, offset);
+		iowrite32(cfg0, pctrl->gpio_base + port->cfg0);
+	} else {
+		ret = -EINVAL;
+	}
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+	return ret;
+}
+
+static int wpcm450_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct wpcm450_port *port = to_port(offset);
+	unsigned long flags;
+	u32 datain;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	datain = ioread32(pctrl->gpio_base + port->datain);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return !!(datain & port_mask(port, offset));
+}
+
+static void wpcm450_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
+	const struct wpcm450_port *port = to_port(offset);
+	unsigned long flags;
+	u32 dataout;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	if (port->dataout) {
+		dataout = ioread32(pctrl->gpio_base + port->dataout);
+		if (value)
+			dataout |= port_mask(port, offset);
+		else
+			dataout &= ~port_mask(port, offset);
+		iowrite32(dataout, pctrl->gpio_base + port->dataout);
+	}
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+/* Interrupt support */
+
+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;
+}
+
+static void wpcm450_gpio_irq_ack(struct irq_data *d)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	int mask = event_bitmask(d->hwirq);
+	unsigned long flags;
+
+	if (mask < 0)
+		return;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	iowrite32(mask, pctrl->gpio_base + WPCM450_GPEVST);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+static void wpcm450_gpio_irq_mask(struct irq_data *d)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	int mask = event_bitmask(d->hwirq);
+	unsigned long flags;
+	u32 even;
+
+	if (mask < 0)
+		return;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	even = ioread32(pctrl->gpio_base + WPCM450_GPEVEN);
+	even &= ~mask;
+	iowrite32(even, pctrl->gpio_base + WPCM450_GPEVEN);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+static void wpcm450_gpio_irq_unmask(struct irq_data *d)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	int mask = event_bitmask(d->hwirq);
+	unsigned long flags;
+	u32 even;
+
+	if (mask < 0)
+		return;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	even = ioread32(pctrl->gpio_base + WPCM450_GPEVEN);
+	even |= mask;
+	iowrite32(even, pctrl->gpio_base + WPCM450_GPEVEN);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
+static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all)
+{
+	int bitnum;
+
+	for_each_set_bit(bitnum, &all, 32) {
+		int gpio = event_bitnum_to_gpio(bitnum);
+		u32 mask = BIT(bitnum), evpol;
+		int level;
+
+		do {
+			evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
+			level = wpcm450_gpio_get(&pctrl->gc, gpio);
+
+			/* Switch event polarity to the opposite of the current level */
+			if (level)
+				evpol &= ~mask;
+			else
+				evpol |= mask;
+
+			iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL);
+		} while (wpcm450_gpio_get(&pctrl->gc, gpio) != level);
+	}
+}
+
+static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+	int mask = event_bitmask(d->hwirq);
+	unsigned long flags;
+	u32 evtype, evpol;
+	int ret = 0;
+
+	if (mask < 0)
+		return mask;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	evtype = ioread32(pctrl->gpio_base + WPCM450_GPEVTYPE);
+	evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
+	pctrl->both_edges &= ~mask;
+	switch (flow_type) {
+	case IRQ_TYPE_LEVEL_LOW:
+		evtype |= mask;
+		evpol &= ~mask;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		evtype |= mask;
+		evpol |= mask;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		evtype &= ~mask;
+		evpol &= ~mask;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		evtype &= ~mask;
+		evpol |= mask;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		evtype &= ~mask;
+		pctrl->both_edges |= mask;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	iowrite32(evtype, pctrl->gpio_base + WPCM450_GPEVTYPE);
+	iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL);
+
+	/* clear the event status for good measure */
+	iowrite32(mask, pctrl->gpio_base + WPCM450_GPEVST);
+
+	/* fix event polarity after clearing event status */
+	wpcm450_gpio_fix_evpol(pctrl, mask);
+
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	return ret;
+}
+
+static const struct irq_chip wpcm450_gpio_irqchip = {
+	.name = "WPCM450-GPIO-IRQ",
+	.irq_ack = wpcm450_gpio_irq_ack,
+	.irq_unmask = wpcm450_gpio_irq_unmask,
+	.irq_mask = wpcm450_gpio_irq_mask,
+	.irq_set_type = wpcm450_gpio_set_irq_type,
+};
+
+static void wpcm450_gpio_irqhandler(struct irq_desc *desc)
+{
+	struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_desc_get_handler_data(desc));
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long pending;
+	unsigned long flags;
+	int bitnum;
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+
+	pending = ioread32(pctrl->gpio_base + WPCM450_GPEVST);
+	pending &= ioread32(pctrl->gpio_base + WPCM450_GPEVEN);
+
+	if (pending & pctrl->both_edges)
+		wpcm450_gpio_fix_evpol(pctrl, pending & pctrl->both_edges);
+
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	chained_irq_enter(chip, desc);
+	for_each_set_bit(bitnum, &pending, 32) {
+		int gpio = event_bitnum_to_gpio(bitnum);
+		int irq = irq_find_mapping(pctrl->gc.irq.domain, gpio);
+
+		generic_handle_irq(irq);
+	}
+	chained_irq_exit(chip, desc);
+}
+
+/* pinmux handing */
+
+static const int smb0_pins[]  = { 115, 114 };
+static const int smb1_pins[]  = { 117, 116 };
+static const int smb2_pins[]  = { 119, 118 };
+static const int smb3_pins[]  = { 30, 31 };
+static const int smb4_pins[]  = { 28, 29 };
+static const int smb5_pins[]  = { 26, 27 };
+
+static const int scs1_pins[] = { 32 };
+static const int scs2_pins[] = { 33 };
+static const int scs3_pins[] = { 34 };
+
+static const int bsp_pins[] = { 41, 42 };
+static const int hsp1_pins[] = { 43, 44, 45, 46, 47, 61, 62, 63 };
+static const int hsp2_pins[] = { 48, 49, 50, 51, 52, 53, 54, 55 };
+
+static const int r1err_pins[] = { 56 };
+static const int r1md_pins[] = { 57, 58 };
+static const int rmii2_pins[] = { 84, 85, 86, 87, 88, 89 };
+static const int r2err_pins[] = { 90 };
+static const int r2md_pins[] = { 91, 92 };
+
+static const int kbcc_pins[] = { 94, 93 };
+static const int clko_pins[] = { 96 };
+static const int smi_pins[] = { 97 };
+static const int uinc_pins[] = { 19 };
+static const int mben_pins[] = {};
+
+static const int gspi_pins[] = { 12, 13, 14, 15 };
+static const int sspi_pins[] = { 12, 13, 14, 15 };
+
+static const int xcs1_pins[] = { 35 };
+static const int xcs2_pins[] = { 36 };
+
+static const int sdio_pins[] = { 7, 22, 43, 44, 45, 46, 47, 60 };
+
+static const int fi0_pins[] = { 64 };
+static const int fi1_pins[] = { 65 };
+static const int fi2_pins[] = { 66 };
+static const int fi3_pins[] = { 67 };
+static const int fi4_pins[] = { 68 };
+static const int fi5_pins[] = { 69 };
+static const int fi6_pins[] = { 70 };
+static const int fi7_pins[] = { 71 };
+static const int fi8_pins[] = { 72 };
+static const int fi9_pins[] = { 73 };
+static const int fi10_pins[] = { 74 };
+static const int fi11_pins[] = { 75 };
+static const int fi12_pins[] = { 76 };
+static const int fi13_pins[] = { 77 };
+static const int fi14_pins[] = { 78 };
+static const int fi15_pins[] = { 79 };
+
+static const int pwm0_pins[] = { 80 };
+static const int pwm1_pins[] = { 81 };
+static const int pwm2_pins[] = { 82 };
+static const int pwm3_pins[] = { 83 };
+static const int pwm4_pins[] = { 20 };
+static const int pwm5_pins[] = { 21 };
+static const int pwm6_pins[] = { 16 };
+static const int pwm7_pins[] = { 17 };
+
+static const int hg0_pins[] = { 20 };
+static const int hg1_pins[] = { 21 };
+static const int hg2_pins[] = { 22 };
+static const int hg3_pins[] = { 23 };
+static const int hg4_pins[] = { 24 };
+static const int hg5_pins[] = { 25 };
+static const int hg6_pins[] = { 59 };
+static const int hg7_pins[] = { 60 };
+
+/*
+ * pin:	     name, number
+ * group:    name, npins,   pins
+ * function: name, ngroups, groups
+ */
+struct wpcm450_group {
+	const char *name;
+	const unsigned int *pins;
+	int npins;
+};
+
+#define WPCM450_GRPS \
+	WPCM450_GRP(smb3), \
+	WPCM450_GRP(smb4), \
+	WPCM450_GRP(smb5), \
+	WPCM450_GRP(scs1), \
+	WPCM450_GRP(scs2), \
+	WPCM450_GRP(scs3), \
+	WPCM450_GRP(smb0), \
+	WPCM450_GRP(smb1), \
+	WPCM450_GRP(smb2), \
+	WPCM450_GRP(bsp), \
+	WPCM450_GRP(hsp1), \
+	WPCM450_GRP(hsp2), \
+	WPCM450_GRP(r1err), \
+	WPCM450_GRP(r1md), \
+	WPCM450_GRP(rmii2), \
+	WPCM450_GRP(r2err), \
+	WPCM450_GRP(r2md), \
+	WPCM450_GRP(kbcc), \
+	WPCM450_GRP(clko), \
+	WPCM450_GRP(smi), \
+	WPCM450_GRP(uinc), \
+	WPCM450_GRP(gspi), \
+	WPCM450_GRP(mben), \
+	WPCM450_GRP(xcs2), \
+	WPCM450_GRP(xcs1), \
+	WPCM450_GRP(sdio), \
+	WPCM450_GRP(sspi), \
+	WPCM450_GRP(fi0), \
+	WPCM450_GRP(fi1), \
+	WPCM450_GRP(fi2), \
+	WPCM450_GRP(fi3), \
+	WPCM450_GRP(fi4), \
+	WPCM450_GRP(fi5), \
+	WPCM450_GRP(fi6), \
+	WPCM450_GRP(fi7), \
+	WPCM450_GRP(fi8), \
+	WPCM450_GRP(fi9), \
+	WPCM450_GRP(fi10), \
+	WPCM450_GRP(fi11), \
+	WPCM450_GRP(fi12), \
+	WPCM450_GRP(fi13), \
+	WPCM450_GRP(fi14), \
+	WPCM450_GRP(fi15), \
+	WPCM450_GRP(pwm0), \
+	WPCM450_GRP(pwm1), \
+	WPCM450_GRP(pwm2), \
+	WPCM450_GRP(pwm3), \
+	WPCM450_GRP(pwm4), \
+	WPCM450_GRP(pwm5), \
+	WPCM450_GRP(pwm6), \
+	WPCM450_GRP(pwm7), \
+	WPCM450_GRP(hg0), \
+	WPCM450_GRP(hg1), \
+	WPCM450_GRP(hg2), \
+	WPCM450_GRP(hg3), \
+	WPCM450_GRP(hg4), \
+	WPCM450_GRP(hg5), \
+	WPCM450_GRP(hg6), \
+	WPCM450_GRP(hg7), \
+
+enum {
+#define WPCM450_GRP(x) fn_ ## x
+	WPCM450_GRPS
+	/* add placeholder for none/gpio */
+	WPCM450_GRP(none),
+	WPCM450_GRP(gpio),
+#undef WPCM450_GRP
+};
+
+static struct wpcm450_group wpcm450_groups[] = {
+#define WPCM450_GRP(x) { .name = #x, .pins = x ## _pins, \
+			.npins = ARRAY_SIZE(x ## _pins) }
+	WPCM450_GRPS
+#undef WPCM450_GRP
+};
+
+#define WPCM450_SFUNC(a) WPCM450_FUNC(a, #a)
+#define WPCM450_FUNC(a, b...) static const char *a ## _grp[] = { b }
+#define WPCM450_MKFUNC(nm) { .name = #nm, .ngroups = ARRAY_SIZE(nm ## _grp), \
+			.groups = nm ## _grp }
+struct wpcm450_func {
+	const char *name;
+	const unsigned int ngroups;
+	const char *const *groups;
+};
+
+WPCM450_SFUNC(smb3);
+WPCM450_SFUNC(smb4);
+WPCM450_SFUNC(smb5);
+WPCM450_SFUNC(scs1);
+WPCM450_SFUNC(scs2);
+WPCM450_SFUNC(scs3);
+WPCM450_SFUNC(smb0);
+WPCM450_SFUNC(smb1);
+WPCM450_SFUNC(smb2);
+WPCM450_SFUNC(bsp);
+WPCM450_SFUNC(hsp1);
+WPCM450_SFUNC(hsp2);
+WPCM450_SFUNC(r1err);
+WPCM450_SFUNC(r1md);
+WPCM450_SFUNC(rmii2);
+WPCM450_SFUNC(r2err);
+WPCM450_SFUNC(r2md);
+WPCM450_SFUNC(kbcc);
+WPCM450_SFUNC(clko);
+WPCM450_SFUNC(smi);
+WPCM450_SFUNC(uinc);
+WPCM450_SFUNC(gspi);
+WPCM450_SFUNC(mben);
+WPCM450_SFUNC(xcs2);
+WPCM450_SFUNC(xcs1);
+WPCM450_SFUNC(sdio);
+WPCM450_SFUNC(sspi);
+WPCM450_SFUNC(fi0);
+WPCM450_SFUNC(fi1);
+WPCM450_SFUNC(fi2);
+WPCM450_SFUNC(fi3);
+WPCM450_SFUNC(fi4);
+WPCM450_SFUNC(fi5);
+WPCM450_SFUNC(fi6);
+WPCM450_SFUNC(fi7);
+WPCM450_SFUNC(fi8);
+WPCM450_SFUNC(fi9);
+WPCM450_SFUNC(fi10);
+WPCM450_SFUNC(fi11);
+WPCM450_SFUNC(fi12);
+WPCM450_SFUNC(fi13);
+WPCM450_SFUNC(fi14);
+WPCM450_SFUNC(fi15);
+WPCM450_SFUNC(pwm0);
+WPCM450_SFUNC(pwm1);
+WPCM450_SFUNC(pwm2);
+WPCM450_SFUNC(pwm3);
+WPCM450_SFUNC(pwm4);
+WPCM450_SFUNC(pwm5);
+WPCM450_SFUNC(pwm6);
+WPCM450_SFUNC(pwm7);
+WPCM450_SFUNC(hg0);
+WPCM450_SFUNC(hg1);
+WPCM450_SFUNC(hg2);
+WPCM450_SFUNC(hg3);
+WPCM450_SFUNC(hg4);
+WPCM450_SFUNC(hg5);
+WPCM450_SFUNC(hg6);
+WPCM450_SFUNC(hg7);
+
+/* Function names */
+static struct wpcm450_func wpcm450_funcs[] = {
+	WPCM450_MKFUNC(smb3),
+	WPCM450_MKFUNC(smb4),
+	WPCM450_MKFUNC(smb5),
+	WPCM450_MKFUNC(scs1),
+	WPCM450_MKFUNC(scs2),
+	WPCM450_MKFUNC(scs3),
+	WPCM450_MKFUNC(smb0),
+	WPCM450_MKFUNC(smb1),
+	WPCM450_MKFUNC(smb2),
+	WPCM450_MKFUNC(bsp),
+	WPCM450_MKFUNC(hsp1),
+	WPCM450_MKFUNC(hsp2),
+	WPCM450_MKFUNC(r1err),
+	WPCM450_MKFUNC(r1md),
+	WPCM450_MKFUNC(rmii2),
+	WPCM450_MKFUNC(r2err),
+	WPCM450_MKFUNC(r2md),
+	WPCM450_MKFUNC(kbcc),
+	WPCM450_MKFUNC(clko),
+	WPCM450_MKFUNC(smi),
+	WPCM450_MKFUNC(uinc),
+	WPCM450_MKFUNC(gspi),
+	WPCM450_MKFUNC(mben),
+	WPCM450_MKFUNC(xcs2),
+	WPCM450_MKFUNC(xcs1),
+	WPCM450_MKFUNC(sdio),
+	WPCM450_MKFUNC(sspi),
+	WPCM450_MKFUNC(fi0),
+	WPCM450_MKFUNC(fi1),
+	WPCM450_MKFUNC(fi2),
+	WPCM450_MKFUNC(fi3),
+	WPCM450_MKFUNC(fi4),
+	WPCM450_MKFUNC(fi5),
+	WPCM450_MKFUNC(fi6),
+	WPCM450_MKFUNC(fi7),
+	WPCM450_MKFUNC(fi8),
+	WPCM450_MKFUNC(fi9),
+	WPCM450_MKFUNC(fi10),
+	WPCM450_MKFUNC(fi11),
+	WPCM450_MKFUNC(fi12),
+	WPCM450_MKFUNC(fi13),
+	WPCM450_MKFUNC(fi14),
+	WPCM450_MKFUNC(fi15),
+	WPCM450_MKFUNC(pwm0),
+	WPCM450_MKFUNC(pwm1),
+	WPCM450_MKFUNC(pwm2),
+	WPCM450_MKFUNC(pwm3),
+	WPCM450_MKFUNC(pwm4),
+	WPCM450_MKFUNC(pwm5),
+	WPCM450_MKFUNC(pwm6),
+	WPCM450_MKFUNC(pwm7),
+	WPCM450_MKFUNC(hg0),
+	WPCM450_MKFUNC(hg1),
+	WPCM450_MKFUNC(hg2),
+	WPCM450_MKFUNC(hg3),
+	WPCM450_MKFUNC(hg4),
+	WPCM450_MKFUNC(hg5),
+	WPCM450_MKFUNC(hg6),
+	WPCM450_MKFUNC(hg7),
+};
+
+#define WPCM450_PINCFG(a, b, c, d, e, f, g) \
+	[a] { .fn0 = fn_ ## b, .reg0 = WPCM450_GCR_ ## c, .bit0 = d, \
+	      .fn1 = fn_ ## e, .reg1 = WPCM450_GCR_ ## f, .bit1 = g }
+
+struct wpcm450_pincfg {
+	int fn0, reg0, bit0;
+	int fn1, reg1, bit1;
+};
+
+static const struct wpcm450_pincfg pincfg[] = {
+	/*		PIN	  FUNCTION 1		   FUNCTION 2 */
+	WPCM450_PINCFG(0,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(1,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(2,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(3,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(4,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(5,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(6,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(7,	 none, NONE, 0,		  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(8,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(9,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(10,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(11,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(12,	 gspi, MFSEL1, 24,	  sspi, MFSEL1, 31),
+	WPCM450_PINCFG(13,	 gspi, MFSEL1, 24,	  sspi, MFSEL1, 31),
+	WPCM450_PINCFG(14,	 gspi, MFSEL1, 24,	  sspi, MFSEL1, 31),
+	WPCM450_PINCFG(15,	 gspi, MFSEL1, 24,	  sspi, MFSEL1, 31),
+	WPCM450_PINCFG(16,	 none, NONE, 0,		  pwm6, MFSEL2, 22),
+	WPCM450_PINCFG(17,	 none, NONE, 0,		  pwm7, MFSEL2, 23),
+	WPCM450_PINCFG(18,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(19,	 uinc, MFSEL1, 23,	  none, NONE, 0),
+	WPCM450_PINCFG(20,	  hg0, MFSEL2, 24,	  pwm4, MFSEL2, 20),
+	WPCM450_PINCFG(21,	  hg1, MFSEL2, 25,	  pwm5, MFSEL2, 21),
+	WPCM450_PINCFG(22,	  hg2, MFSEL2, 26,	  none, NONE, 0),
+	WPCM450_PINCFG(23,	  hg3, MFSEL2, 27,	  none, NONE, 0),
+	WPCM450_PINCFG(24,	  hg4, MFSEL2, 28,	  none, NONE, 0),
+	WPCM450_PINCFG(25,	  hg5, MFSEL2, 29,	  none, NONE, 0),
+	WPCM450_PINCFG(26,	 smb5, MFSEL1, 2,	  none, NONE, 0),
+	WPCM450_PINCFG(27,	 smb5, MFSEL1, 2,	  none, NONE, 0),
+	WPCM450_PINCFG(28,	 smb4, MFSEL1, 1,	  none, NONE, 0),
+	WPCM450_PINCFG(29,	 smb4, MFSEL1, 1,	  none, NONE, 0),
+	WPCM450_PINCFG(30,	 smb3, MFSEL1, 0,	  none, NONE, 0),
+	WPCM450_PINCFG(31,	 smb3, MFSEL1, 0,	  none, NONE, 0),
+
+	WPCM450_PINCFG(32,	 scs1, MFSEL1, 3,	  none, NONE, 0),
+	WPCM450_PINCFG(33,	 scs2, MFSEL1, 4,	  none, NONE, 0),
+	WPCM450_PINCFG(34,	 scs3, MFSEL1, 5,	  none, NONE, 0),
+	WPCM450_PINCFG(35,	 xcs1, MFSEL1, 29,	  none, NONE, 0),
+	WPCM450_PINCFG(36,	 xcs2, MFSEL1, 28,	  none, NONE, 0),
+	WPCM450_PINCFG(37,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(38,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(39,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(40,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(41,	  bsp, MFSEL1, 9,	  none, NONE, 0),
+	WPCM450_PINCFG(42,	  bsp, MFSEL1, 9,	  none, NONE, 0),
+	WPCM450_PINCFG(43,	 hsp1, MFSEL1, 10,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(44,	 hsp1, MFSEL1, 10,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(45,	 hsp1, MFSEL1, 10,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(46,	 hsp1, MFSEL1, 10,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(47,	 hsp1, MFSEL1, 10,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(48,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(49,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(50,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(51,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(52,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(53,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(54,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(55,	 hsp2, MFSEL1, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(56,	r1err, MFSEL1, 12,	  none, NONE, 0),
+	WPCM450_PINCFG(57,	 r1md, MFSEL1, 13,	  none, NONE, 0),
+	WPCM450_PINCFG(58,	 r1md, MFSEL1, 13,	  none, NONE, 0),
+	WPCM450_PINCFG(59,	  hg6, MFSEL2, 30,	  none, NONE, 0),
+	WPCM450_PINCFG(60,	  hg7, MFSEL2, 31,	  sdio, MFSEL1, 30),
+	WPCM450_PINCFG(61,	 hsp1, MFSEL1, 10,	  none, NONE, 0),
+	WPCM450_PINCFG(62,	 hsp1, MFSEL1, 10,	  none, NONE, 0),
+	WPCM450_PINCFG(63,	 hsp1, MFSEL1, 10,	  none, NONE, 0),
+
+	WPCM450_PINCFG(64,	  fi0, MFSEL2, 0,	  none, NONE, 0),
+	WPCM450_PINCFG(65,	  fi1, MFSEL2, 1,	  none, NONE, 0),
+	WPCM450_PINCFG(66,	  fi2, MFSEL2, 2,	  none, NONE, 0),
+	WPCM450_PINCFG(67,	  fi3, MFSEL2, 3,	  none, NONE, 0),
+	WPCM450_PINCFG(68,	  fi4, MFSEL2, 4,	  none, NONE, 0),
+	WPCM450_PINCFG(69,	  fi5, MFSEL2, 5,	  none, NONE, 0),
+	WPCM450_PINCFG(70,	  fi6, MFSEL2, 6,	  none, NONE, 0),
+	WPCM450_PINCFG(71,	  fi7, MFSEL2, 7,	  none, NONE, 0),
+	WPCM450_PINCFG(72,	  fi8, MFSEL2, 8,	  none, NONE, 0),
+	WPCM450_PINCFG(73,	  fi9, MFSEL2, 9,	  none, NONE, 0),
+	WPCM450_PINCFG(74,	 fi10, MFSEL2, 10,	  none, NONE, 0),
+	WPCM450_PINCFG(75,	 fi11, MFSEL2, 11,	  none, NONE, 0),
+	WPCM450_PINCFG(76,	 fi12, MFSEL2, 12,	  none, NONE, 0),
+	WPCM450_PINCFG(77,	 fi13, MFSEL2, 13,	  none, NONE, 0),
+	WPCM450_PINCFG(78,	 fi14, MFSEL2, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(79,	 fi15, MFSEL2, 15,	  none, NONE, 0),
+	WPCM450_PINCFG(80,	 pwm0, MFSEL2, 16,	  none, NONE, 0),
+	WPCM450_PINCFG(81,	 pwm1, MFSEL2, 17,	  none, NONE, 0),
+	WPCM450_PINCFG(82,	 pwm2, MFSEL2, 18,	  none, NONE, 0),
+	WPCM450_PINCFG(83,	 pwm3, MFSEL2, 19,	  none, NONE, 0),
+	WPCM450_PINCFG(84,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(85,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(86,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(87,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(88,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(89,	rmii2, MFSEL1, 14,	  none, NONE, 0),
+	WPCM450_PINCFG(90,	r2err, MFSEL1, 15,	  none, NONE, 0),
+	WPCM450_PINCFG(91,	 r2md, MFSEL1, 16,	  none, NONE, 0),
+	WPCM450_PINCFG(92,	 r2md, MFSEL1, 16,	  none, NONE, 0),
+	WPCM450_PINCFG(93,	 kbcc, MFSEL1, 17,	  none, NONE, 0),
+	WPCM450_PINCFG(94,	 kbcc, MFSEL1, 17,	  none, NONE, 0),
+	WPCM450_PINCFG(95,	 none, NONE, 0,		  none, NONE, 0),
+
+	WPCM450_PINCFG(96,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(97,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(98,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(99,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(100,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(101,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(102,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(103,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(104,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(105,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(106,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(107,	 none, NONE, 0,		  none, NONE, 0),
+	WPCM450_PINCFG(108,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(109,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(110,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(111,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(112,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(113,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(114,	 smb0, MFSEL1, 6,	  none, NONE, 0),
+	WPCM450_PINCFG(115,	 smb0, MFSEL1, 6,	  none, NONE, 0),
+	WPCM450_PINCFG(116,	 smb1, MFSEL1, 7,	  none, NONE, 0),
+	WPCM450_PINCFG(117,	 smb1, MFSEL1, 7,	  none, NONE, 0),
+	WPCM450_PINCFG(118,	 smb2, MFSEL1, 8,	  none, NONE, 0),
+	WPCM450_PINCFG(119,	 smb2, MFSEL1, 8,	  none, NONE, 0),
+	WPCM450_PINCFG(120,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(121,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(122,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(123,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(124,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(125,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(126,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+	WPCM450_PINCFG(127,	 none, NONE, 0,		  none, NONE, 0), /* DVO */
+};
+
+#define WPCM450_PIN(n)		PINCTRL_PIN(n, "gpio" #n)
+
+static const struct pinctrl_pin_desc wpcm450_pins[] = {
+	WPCM450_PIN(0),   WPCM450_PIN(1),   WPCM450_PIN(2),   WPCM450_PIN(3),
+	WPCM450_PIN(4),   WPCM450_PIN(5),   WPCM450_PIN(6),   WPCM450_PIN(7),
+	WPCM450_PIN(8),   WPCM450_PIN(9),   WPCM450_PIN(10),  WPCM450_PIN(11),
+	WPCM450_PIN(12),  WPCM450_PIN(13),  WPCM450_PIN(14),  WPCM450_PIN(15),
+	WPCM450_PIN(16),  WPCM450_PIN(17),  WPCM450_PIN(18),  WPCM450_PIN(19),
+	WPCM450_PIN(20),  WPCM450_PIN(21),  WPCM450_PIN(22),  WPCM450_PIN(23),
+	WPCM450_PIN(24),  WPCM450_PIN(25),  WPCM450_PIN(26),  WPCM450_PIN(27),
+	WPCM450_PIN(28),  WPCM450_PIN(29),  WPCM450_PIN(30),  WPCM450_PIN(31),
+	WPCM450_PIN(32),  WPCM450_PIN(33),  WPCM450_PIN(34),  WPCM450_PIN(35),
+	WPCM450_PIN(36),  WPCM450_PIN(37),  WPCM450_PIN(38),  WPCM450_PIN(39),
+	WPCM450_PIN(40),  WPCM450_PIN(41),  WPCM450_PIN(42),  WPCM450_PIN(43),
+	WPCM450_PIN(44),  WPCM450_PIN(45),  WPCM450_PIN(46),  WPCM450_PIN(47),
+	WPCM450_PIN(48),  WPCM450_PIN(49),  WPCM450_PIN(50),  WPCM450_PIN(51),
+	WPCM450_PIN(52),  WPCM450_PIN(53),  WPCM450_PIN(54),  WPCM450_PIN(55),
+	WPCM450_PIN(56),  WPCM450_PIN(57),  WPCM450_PIN(58),  WPCM450_PIN(59),
+	WPCM450_PIN(60),  WPCM450_PIN(61),  WPCM450_PIN(62),  WPCM450_PIN(63),
+	WPCM450_PIN(64),  WPCM450_PIN(65),  WPCM450_PIN(66),  WPCM450_PIN(67),
+	WPCM450_PIN(68),  WPCM450_PIN(69),  WPCM450_PIN(70),  WPCM450_PIN(71),
+	WPCM450_PIN(72),  WPCM450_PIN(73),  WPCM450_PIN(74),  WPCM450_PIN(75),
+	WPCM450_PIN(76),  WPCM450_PIN(77),  WPCM450_PIN(78),  WPCM450_PIN(79),
+	WPCM450_PIN(80),  WPCM450_PIN(81),  WPCM450_PIN(82),  WPCM450_PIN(83),
+	WPCM450_PIN(84),  WPCM450_PIN(85),  WPCM450_PIN(86),  WPCM450_PIN(87),
+	WPCM450_PIN(88),  WPCM450_PIN(89),  WPCM450_PIN(90),  WPCM450_PIN(91),
+	WPCM450_PIN(92),  WPCM450_PIN(93),  WPCM450_PIN(94),  WPCM450_PIN(95),
+	WPCM450_PIN(96),  WPCM450_PIN(97),  WPCM450_PIN(98),  WPCM450_PIN(99),
+	WPCM450_PIN(100), WPCM450_PIN(101), WPCM450_PIN(102), WPCM450_PIN(103),
+	WPCM450_PIN(104), WPCM450_PIN(105), WPCM450_PIN(106), WPCM450_PIN(107),
+	WPCM450_PIN(108), WPCM450_PIN(109), WPCM450_PIN(110), WPCM450_PIN(111),
+	WPCM450_PIN(112), WPCM450_PIN(113), WPCM450_PIN(114), WPCM450_PIN(115),
+	WPCM450_PIN(116), WPCM450_PIN(117), WPCM450_PIN(118), WPCM450_PIN(119),
+	WPCM450_PIN(120), WPCM450_PIN(121), WPCM450_PIN(122), WPCM450_PIN(123),
+	WPCM450_PIN(124), WPCM450_PIN(125), WPCM450_PIN(126), WPCM450_PIN(127),
+};
+
+/* Enable mode in pin group */
+static void wpcm450_setfunc(struct regmap *gcr_regmap, const unsigned int *pin,
+			    int npins, int mode)
+{
+	const struct wpcm450_pincfg *cfg;
+	int i;
+
+	for (i = 0; i < npins; i++) {
+		cfg = &pincfg[pin[i]];
+		if (mode == fn_gpio || cfg->fn0 == mode || cfg->fn1 == mode) {
+			if (cfg->reg0)
+				regmap_update_bits(gcr_regmap, cfg->reg0,
+						   BIT(cfg->bit0),
+						   (cfg->fn0 == mode) ?  BIT(cfg->bit0) : 0);
+			if (cfg->reg1)
+				regmap_update_bits(gcr_regmap, cfg->reg1,
+						   BIT(cfg->bit1),
+						   (cfg->fn1 == mode) ?  BIT(cfg->bit1) : 0);
+		}
+	}
+}
+
+/* pinctrl_ops */
+static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
+
+	dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
+	return ARRAY_SIZE(wpcm450_groups);
+}
+
+static const char *wpcm450_get_group_name(struct pinctrl_dev *pctldev,
+					  unsigned int selector)
+{
+	return wpcm450_groups[selector].name;
+}
+
+static int wpcm450_get_group_pins(struct pinctrl_dev *pctldev,
+				  unsigned int selector,
+				  const unsigned int **pins,
+				  unsigned int *npins)
+{
+	*npins = wpcm450_groups[selector].npins;
+	*pins  = wpcm450_groups[selector].pins;
+
+	return 0;
+}
+
+static int wpcm450_dt_node_to_map(struct pinctrl_dev *pctldev,
+				  struct device_node *np_config,
+				  struct pinctrl_map **map,
+				  u32 *num_maps)
+{
+	struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
+
+	dev_dbg(npcm->dev, "dt_node_to_map: %s\n", np_config->name);
+	return pinconf_generic_dt_node_to_map(pctldev, np_config,
+					      map, num_maps,
+					      PIN_MAP_TYPE_INVALID);
+}
+
+static void wpcm450_dt_free_map(struct pinctrl_dev *pctldev,
+				struct pinctrl_map *map, u32 num_maps)
+{
+	kfree(map);
+}
+
+static const struct pinctrl_ops wpcm450_pinctrl_ops = {
+	.get_groups_count = wpcm450_get_groups_count,
+	.get_group_name = wpcm450_get_group_name,
+	.get_group_pins = wpcm450_get_group_pins,
+	.dt_node_to_map = wpcm450_dt_node_to_map,
+	.dt_free_map = wpcm450_dt_free_map,
+};
+
+/* pinmux_ops  */
+static int wpcm450_get_functions_count(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(wpcm450_funcs);
+}
+
+static const char *wpcm450_get_function_name(struct pinctrl_dev *pctldev,
+					     unsigned int function)
+{
+	return wpcm450_funcs[function].name;
+}
+
+static int wpcm450_get_function_groups(struct pinctrl_dev *pctldev,
+				       unsigned int function,
+				       const char * const **groups,
+				       unsigned int * const ngroups)
+{
+	*ngroups = wpcm450_funcs[function].ngroups;
+	*groups	 = wpcm450_funcs[function].groups;
+
+	return 0;
+}
+
+static int wpcm450_pinmux_set_mux(struct pinctrl_dev *pctldev,
+				  unsigned int function,
+				  unsigned int group)
+{
+	struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
+
+	dev_dbg(npcm->dev, "set_mux: %d, %d[%s]\n", function, group,
+		wpcm450_groups[group].name);
+
+	wpcm450_setfunc(npcm->gcr_regmap, wpcm450_groups[group].pins,
+			wpcm450_groups[group].npins, group);
+
+	return 0;
+}
+
+static int wpcm450_gpio_request_enable(struct pinctrl_dev *pctldev,
+				       struct pinctrl_gpio_range *range,
+				       unsigned int offset)
+{
+	struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!range) {
+		dev_err(npcm->dev, "invalid range\n");
+		return -EINVAL;
+	}
+	if (!range->gc) {
+		dev_err(npcm->dev, "invalid gpiochip\n");
+		return -EINVAL;
+	}
+
+	wpcm450_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio);
+
+	return 0;
+}
+
+/* Release GPIO back to pinctrl mode */
+static void wpcm450_gpio_request_free(struct pinctrl_dev *pctldev,
+				      struct pinctrl_gpio_range *range,
+				      unsigned int offset)
+{
+	struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	int virq;
+
+	virq = irq_find_mapping(pctrl->domain, offset);
+	if (virq)
+		irq_dispose_mapping(virq);
+}
+
+static const struct pinmux_ops wpcm450_pinmux_ops = {
+	.get_functions_count = wpcm450_get_functions_count,
+	.get_function_name = wpcm450_get_function_name,
+	.get_function_groups = wpcm450_get_function_groups,
+	.set_mux = wpcm450_pinmux_set_mux,
+	.gpio_request_enable = wpcm450_gpio_request_enable,
+	.gpio_disable_free = wpcm450_gpio_request_free,
+};
+
+/* pinconf_ops */
+static int debounce_bitmask(int gpio)
+{
+	if (gpio >= 0 && gpio < 16)
+		return BIT(gpio);
+	return -EINVAL;
+}
+
+static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			      unsigned long *config)
+{
+	struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	unsigned long flags;
+	int mask;
+	u32 reg;
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		mask = debounce_bitmask(pin);
+		if (mask < 0)
+			return mask;
+
+		spin_lock_irqsave(&pctrl->lock, flags);
+		reg = ioread32(pctrl->gpio_base + WPCM450_GPEVDBNC);
+		spin_unlock_irqrestore(&pctrl->lock, flags);
+
+		*config = pinconf_to_config_packed(param, !!(reg & mask));
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int wpcm450_config_set_one(struct wpcm450_pinctrl *pctrl,
+				  unsigned int pin, unsigned long config)
+{
+	enum pin_config_param param = pinconf_to_config_param(config);
+	unsigned long flags;
+	int mask;
+	u32 reg;
+	int arg;
+
+	switch (param) {
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		mask = event_bitmask(pin);
+		if (mask < 0)
+			return mask;
+
+		arg = pinconf_to_config_argument(config);
+
+		spin_lock_irqsave(&pctrl->lock, flags);
+		reg = ioread32(pctrl->gpio_base + WPCM450_GPEVDBNC);
+		if (arg)
+			reg |= mask;
+		else
+			reg &= ~mask;
+		iowrite32(reg, pctrl->gpio_base + WPCM450_GPEVDBNC);
+		spin_unlock_irqrestore(&pctrl->lock, flags);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+/* Set multiple configuration settings for a pin */
+static int wpcm450_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			      unsigned long *configs, unsigned int num_configs)
+{
+	struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	int rc;
+
+	while (num_configs--) {
+		rc = wpcm450_config_set_one(pctrl, pin, *configs++);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops wpcm450_pinconf_ops = {
+	.is_generic = true,
+	.pin_config_get = wpcm450_config_get,
+	.pin_config_set = wpcm450_config_set,
+};
+
+static struct pinctrl_desc wpcm450_pinctrl_desc = {
+	.name = "wpcm450-pinctrl",
+	.pins = wpcm450_pins,
+	.npins = ARRAY_SIZE(wpcm450_pins),
+	.pctlops = &wpcm450_pinctrl_ops,
+	.pmxops = &wpcm450_pinmux_ops,
+	.confops = &wpcm450_pinconf_ops,
+	.owner = THIS_MODULE,
+};
+
+static int wpcm450_gpio_register(struct wpcm450_pinctrl *pctrl)
+{
+	struct device_node *np = pctrl->dev->of_node;
+	struct gpio_irq_chip *girq;
+	int ret;
+	int i;
+
+	if (!of_find_property(np, "gpio-controller", NULL))
+		return -ENODEV;
+
+	pctrl->gpio_base = of_iomap(np, 0);
+	if (!pctrl->gpio_base) {
+		dev_err(pctrl->dev, "Resource fail for GPIO controller\n");
+		return -ENOMEM;
+	}
+
+	pctrl->gc.label = dev_name(pctrl->dev);
+	pctrl->gc.owner = THIS_MODULE;
+	pctrl->gc.parent = pctrl->dev;
+
+	pctrl->gc.get_direction = wpcm450_gpio_get_direction;
+	pctrl->gc.direction_input = wpcm450_gpio_direction_input;
+	pctrl->gc.direction_output = wpcm450_gpio_direction_output;
+	pctrl->gc.get = wpcm450_gpio_get;
+	pctrl->gc.set = wpcm450_gpio_set;
+
+	pctrl->gc.base = -1;
+	pctrl->gc.ngpio = WPCM450_NUM_GPIOS;
+	pctrl->gc.can_sleep = false;
+
+	pctrl->irqc = wpcm450_gpio_irqchip;
+	girq = &pctrl->gc.irq;
+	girq->chip = &pctrl->irqc;
+	girq->parent_handler = wpcm450_gpio_irqhandler;
+	girq->num_parents = WPCM450_NUM_GPIO_IRQS;
+	girq->parents = devm_kcalloc(pctrl->dev, WPCM450_NUM_GPIO_IRQS,
+				     sizeof(*girq->parents), GFP_KERNEL);
+	if (!girq->parents)
+		return -ENOMEM;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
+	for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
+		int irq = irq_of_parse_and_map(np, i);
+
+		if (irq < 0) {
+			dev_err(pctrl->dev, "No IRQ for GPIO controller\n");
+			return irq;
+		}
+		girq->parents[i] = irq;
+	}
+
+	ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->gc, pctrl);
+	if (ret) {
+		dev_err(pctrl->dev, "Failed to add GPIO chip: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int wpcm450_pinctrl_probe(struct platform_device *pdev)
+{
+	struct wpcm450_pinctrl *pctrl;
+	int ret;
+
+	pctrl = devm_kzalloc(&pdev->dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->dev = &pdev->dev;
+	spin_lock_init(&pctrl->lock);
+	dev_set_drvdata(&pdev->dev, pctrl);
+
+	pctrl->gcr_regmap =
+		syscon_regmap_lookup_by_compatible("nuvoton,wpcm450-gcr");
+	if (IS_ERR(pctrl->gcr_regmap)) {
+		dev_err(pctrl->dev, "didn't find nuvoton,wpcm450-gcr\n");
+		return PTR_ERR(pctrl->gcr_regmap);
+	}
+
+	pctrl->pctldev = devm_pinctrl_register(&pdev->dev,
+					       &wpcm450_pinctrl_desc, pctrl);
+	if (IS_ERR(pctrl->pctldev)) {
+		dev_err(&pdev->dev, "Failed to register pinctrl device\n");
+		return PTR_ERR(pctrl->pctldev);
+	}
+
+	ret = wpcm450_gpio_register(pctrl);
+	if (ret < 0)
+		return ret;
+
+	pr_info("WPCM450 pinctrl and GPIO driver probed\n");
+	return 0;
+}
+
+static const struct of_device_id wpcm450_pinctrl_match[] = {
+	{ .compatible = "nuvoton,wpcm450-pinctrl" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, wpcm450_pinctrl_match);
+
+static struct platform_driver wpcm450_pinctrl_driver = {
+	.probe = wpcm450_pinctrl_probe,
+	.driver = {
+		.name = "wpcm450-pinctrl",
+		.of_match_table = wpcm450_pinctrl_match,
+		.suppress_bind_attrs = true,
+	},
+};
+
+static int __init wpcm450_pinctrl_register(void)
+{
+	return platform_driver_register(&wpcm450_pinctrl_driver);
+}
+arch_initcall(wpcm450_pinctrl_register);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("Nuvoton WPCM450 Pinctrl and GPIO driver");
--
2.30.2

[PATCH 6/8] ARM: dts: wpcm450: Add pinctrl node

[Jonathan Neuschäfer]

This patch adds the GPIO and pin controller to the devicetree for the
WPCM450 SoC.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/arm/boot/dts/nuvoton-wpcm450.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
index 8eba4897b41bc..1b63943b2a42b 100644
--- a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
+++ b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
@@ -77,5 +77,16 @@ aic: interrupt-controller@b8002000 {
 			interrupt-controller;
 			#interrupt-cells = <2>;
 		};
+
+		pinctrl: pinctrl@b8003000 {
+			compatible = "nuvoton,wpcm450-pinctrl";
+			reg = <0xb8003000 0x1000>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			interrupts = <2 IRQ_TYPE_LEVEL_HIGH
+				      3 IRQ_TYPE_LEVEL_HIGH
+				      4 IRQ_TYPE_LEVEL_HIGH
+				      5 IRQ_TYPE_LEVEL_HIGH>;
+		};
 	};
 };
--
2.30.2

[PATCH 7/8] ARM: dts: wpcm450: Add pin functions

[Jonathan Neuschäfer]

As done in nuvoton-common-npcm7xx.dtsi, this patch adds pinmux nodes for
all pin functions to nuvoton-wpcm450.dtsi.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 arch/arm/boot/dts/nuvoton-wpcm450.dtsi | 305 +++++++++++++++++++++++++
 1 file changed, 305 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
index 1b63943b2a42b..f1a1b1fdcb3e4 100644
--- a/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
+++ b/arch/arm/boot/dts/nuvoton-wpcm450.dtsi
@@ -87,6 +87,311 @@ pinctrl: pinctrl@b8003000 {
 				      3 IRQ_TYPE_LEVEL_HIGH
 				      4 IRQ_TYPE_LEVEL_HIGH
 				      5 IRQ_TYPE_LEVEL_HIGH>;
+
+			smb3_pins: smb3-pins {
+				groups = "smb3";
+				function = "smb3";
+			};
+
+			smb4_pins: smb4-pins {
+				groups = "smb4";
+				function = "smb4";
+			};
+
+			smb5_pins: smb5-pins {
+				groups = "smb5";
+				function = "smb5";
+			};
+
+			scs1_pins: scs1-pins {
+				groups = "scs1";
+				function = "scs1";
+			};
+
+			scs2_pins: scs2-pins {
+				groups = "scs2";
+				function = "scs2";
+			};
+
+			scs3_pins: scs3-pins {
+				groups = "scs3";
+				function = "scs3";
+			};
+
+			smb0_pins: smb0-pins {
+				groups = "smb0";
+				function = "smb0";
+			};
+
+			smb1_pins: smb1-pins {
+				groups = "smb1";
+				function = "smb1";
+			};
+
+			smb2_pins: smb2-pins {
+				groups = "smb2";
+				function = "smb2";
+			};
+
+			bsp_pins: bsp-pins {
+				groups = "bsp";
+				function = "bsp";
+			};
+
+			hsp1_pins: hsp1-pins {
+				groups = "hsp1";
+				function = "hsp1";
+			};
+
+			hsp2_pins: hsp2-pins {
+				groups = "hsp2";
+				function = "hsp2";
+			};
+
+			r1err_pins: r1err-pins {
+				groups = "r1err";
+				function = "r1err";
+			};
+
+			r1md_pins: r1md-pins {
+				groups = "r1md";
+				function = "r1md";
+			};
+
+			rmii2_pins: rmii2-pins {
+				groups = "rmii2";
+				function = "rmii2";
+			};
+
+			r2err_pins: r2err-pins {
+				groups = "r2err";
+				function = "r2err";
+			};
+
+			r2md_pins: r2md-pins {
+				groups = "r2md";
+				function = "r2md";
+			};
+
+			kbcc_pins: kbcc-pins {
+				groups = "kbcc";
+				function = "kbcc";
+			};
+
+			dvo0_pins: dvo0-pins {
+				groups = "dvo";
+				function = "dvo0";
+			};
+
+			dvo3_pins: dvo3-pins {
+				groups = "dvo";
+				function = "dvo3";
+			};
+
+			clko_pins: clko-pins {
+				groups = "clko";
+				function = "clko";
+			};
+
+			smi_pins: smi-pins {
+				groups = "smi";
+				function = "smi";
+			};
+
+			uinc_pins: uinc-pins {
+				groups = "uinc";
+				function = "uinc";
+			};
+
+			gspi_pins: gspi-pins {
+				groups = "gspi";
+				function = "gspi";
+			};
+
+			mben_pins: mben-pins {
+				groups = "mben";
+				function = "mben";
+			};
+
+			xcs2_pins: xcs2-pins {
+				groups = "xcs2";
+				function = "xcs2";
+			};
+
+			xcs1_pins: xcs1-pins {
+				groups = "xcs1";
+				function = "xcs1";
+			};
+
+			sdio_pins: sdio-pins {
+				groups = "sdio";
+				function = "sdio";
+			};
+
+			sspi_pins: sspi-pins {
+				groups = "sspi";
+				function = "sspi";
+			};
+
+			fi0_pins: fi0-pins {
+				groups = "fi0";
+				function = "fi0";
+			};
+
+			fi1_pins: fi1-pins {
+				groups = "fi1";
+				function = "fi1";
+			};
+
+			fi2_pins: fi2-pins {
+				groups = "fi2";
+				function = "fi2";
+			};
+
+			fi3_pins: fi3-pins {
+				groups = "fi3";
+				function = "fi3";
+			};
+
+			fi4_pins: fi4-pins {
+				groups = "fi4";
+				function = "fi4";
+			};
+
+			fi5_pins: fi5-pins {
+				groups = "fi5";
+				function = "fi5";
+			};
+
+			fi6_pins: fi6-pins {
+				groups = "fi6";
+				function = "fi6";
+			};
+
+			fi7_pins: fi7-pins {
+				groups = "fi7";
+				function = "fi7";
+			};
+
+			fi8_pins: fi8-pins {
+				groups = "fi8";
+				function = "fi8";
+			};
+
+			fi9_pins: fi9-pins {
+				groups = "fi9";
+				function = "fi9";
+			};
+
+			fi10_pins: fi10-pins {
+				groups = "fi10";
+				function = "fi10";
+			};
+
+			fi11_pins: fi11-pins {
+				groups = "fi11";
+				function = "fi11";
+			};
+
+			fi12_pins: fi12-pins {
+				groups = "fi12";
+				function = "fi12";
+			};
+
+			fi13_pins: fi13-pins {
+				groups = "fi13";
+				function = "fi13";
+			};
+
+			fi14_pins: fi14-pins {
+				groups = "fi14";
+				function = "fi14";
+			};
+
+			fi15_pins: fi15-pins {
+				groups = "fi15";
+				function = "fi15";
+			};
+
+			pwm0_pins: pwm0-pins {
+				groups = "pwm0";
+				function = "pwm0";
+			};
+
+			pwm1_pins: pwm1-pins {
+				groups = "pwm1";
+				function = "pwm1";
+			};
+
+			pwm2_pins: pwm2-pins {
+				groups = "pwm2";
+				function = "pwm2";
+			};
+
+			pwm3_pins: pwm3-pins {
+				groups = "pwm3";
+				function = "pwm3";
+			};
+
+			pwm4_pins: pwm4-pins {
+				groups = "pwm4";
+				function = "pwm4";
+			};
+
+			pwm5_pins: pwm5-pins {
+				groups = "pwm5";
+				function = "pwm5";
+			};
+
+			pwm6_pins: pwm6-pins {
+				groups = "pwm6";
+				function = "pwm6";
+			};
+
+			pwm7_pins: pwm7-pins {
+				groups = "pwm7";
+				function = "pwm7";
+			};
+
+			hg0_pins: hg0-pins {
+				groups = "hg0";
+				function = "hg0";
+			};
+
+			hg1_pins: hg1-pins {
+				groups = "hg1";
+				function = "hg1";
+			};
+
+			hg2_pins: hg2-pins {
+				groups = "hg2";
+				function = "hg2";
+			};
+
+			hg3_pins: hg3-pins {
+				groups = "hg3";
+				function = "hg3";
+			};
+
+			hg4_pins: hg4-pins {
+				groups = "hg4";
+				function = "hg4";
+			};
+
+			hg5_pins: hg5-pins {
+				groups = "hg5";
+				function = "hg5";
+			};
+
+			hg6_pins: hg6-pins {
+				groups = "hg6";
+				function = "hg6";
+			};
+
+			hg7_pins: hg7-pins {
+				groups = "hg7";
+				function = "hg7";
+			};
 		};
 	};
 };
--
2.30.2

[PATCH 8/8] ARM: dts: wpcm450-supermicro-x9sci-ln4f: Add GPIO LEDs and buttons

[Jonathan Neuschäfer]

The Supermicro X9SCi-LN4F server mainboard has a two LEDs and a button
under the control of the BMC. This patch makes them accessible under
Linux running on the BMC.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../nuvoton-wpcm450-supermicro-x9sci-ln4f.dts | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton-wpcm450-supermicro-x9sci-ln4f.dts b/arch/arm/boot/dts/nuvoton-wpcm450-supermicro-x9sci-ln4f.dts
index 83f27fbf4e939..176e22216a75e 100644
--- a/arch/arm/boot/dts/nuvoton-wpcm450-supermicro-x9sci-ln4f.dts
+++ b/arch/arm/boot/dts/nuvoton-wpcm450-supermicro-x9sci-ln4f.dts
@@ -8,6 +8,9 @@

 #include "nuvoton-wpcm450.dtsi"

+#include <dt-bindings/input/linux-event-codes.h>
+#include <dt-bindings/gpio/gpio.h>
+
 / {
 	model = "Supermicro X9SCi-LN4F BMC";
 	compatible = "supermicro,x9sci-ln4f-bmc", "nuvoton,wpcm450";
@@ -20,6 +23,30 @@ memory@0 {
 		device_type = "memory";
 		reg = <0 0x08000000>; /* 128 MiB */
 	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		uid {
+			label = "UID button";
+			linux,code = <KEY_HOME>;
+			gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;
+		};
+	};
+
+	gpio-leds {
+		compatible = "gpio-leds";
+
+		uid {
+			label = "UID";
+			gpios = <&pinctrl 23 GPIO_ACTIVE_HIGH>;
+		};
+
+		heartbeat {
+			label = "heartbeat";
+			gpios = <&pinctrl 20 GPIO_ACTIVE_LOW>;
+		};
+	};
 };

 &serial0 {
--
2.30.2

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

[Andy Shevchenko]

On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> 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.
>
> Some functionality implemented in the hardware was (for now) left unused
> in the driver, specifically blinking and pull-up/down.

...

> +config PINCTRL_WPCM450
> +       bool "Pinctrl and GPIO driver for Nuvoton WPCM450"

Why it can't be a module?

> +       depends on (ARCH_WPCM450 || COMPILE_TEST) && OF

Is it really OF dependent (see below)?

> +       select PINMUX
> +       select PINCONF
> +       select GENERIC_PINCONF
> +       select GPIOLIB
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say Y here to enable pin controller and GPIO support
> +         for the Nuvoton WPCM450 SoC.

From this help it's not clear why user should or shouldn't enable it.
Please, elaborate (and hence fix checkpatch warning).

...

> +#include <linux/module.h>

mod_devicetable.h

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>

> +#include <linux/pinctrl/machine.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>

Can you move this group...

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

...here?

It will show the relation with pin control subsystem and separate from
global / generic headers.

...

> +       /*
> +        * This spinlock protects registers and struct wpcm450_pinctrl fields

spin lock

> +        * against concurrent access.
> +        */

...

> +/* GPIO handling in the pinctrl driver */
> +

Useless.

...

> +static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
> +                                     unsigned int offset)
> +{
> +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> +       const struct wpcm450_port *port = to_port(offset);
> +       unsigned long flags;
> +       u32 cfg0;
> +       int dir;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       if (port->cfg0) {
> +               cfg0 = ioread32(pctrl->gpio_base + port->cfg0);

> +               dir = !(cfg0 & port_mask(port, offset));
> +       } else {
> +               /* If cfg0 is unavailable, the GPIO is always an input */
> +               dir = 1;
> +       }

Why above is under spin lock?
Same question for all other similar places in different functions, if any.

> +       spin_unlock_irqrestore(&pctrl->lock, flags);
> +       return dir;
> +}

...

> +static int wpcm450_gpio_direction_output(struct gpio_chip *chip,
> +                                        unsigned int offset, int value)
> +{
> +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> +       const struct wpcm450_port *port = to_port(offset);
> +       unsigned long flags;
> +       u32 dataout, cfg0;

> +       int ret = 0;

Redundant. Can do it without it.

> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       if (port->cfg0) {

> +       } else {
> +               ret = -EINVAL;
> +       }
> +       spin_unlock_irqrestore(&pctrl->lock, flags);
> +       return ret;
> +}

...

> +/* Interrupt support */
> +

Useless besides being in a bad style.

...

> +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;
> +}

Can you consider to use bitmap_bitremap()

> +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;
> +}

Ditto.

See gpio-xilinx.c for example.

...

> +static void wpcm450_gpio_irq_ack(struct irq_data *d)
> +{
> +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));

> +       int mask = event_bitmask(d->hwirq);

Move the assignment closer to the check.
Ditto for other same and similar cases in the code.

> +       unsigned long flags;
> +
> +       if (mask < 0)
> +               return;

> +}

...

> +       int mask = event_bitmask(d->hwirq);

Use irqd_to_hwirq() (please check that I spelled it correctly).
Same for all hwirq getters.

...

> +static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all)
> +{
> +       int bitnum;

Can it be negative?

> +       for_each_set_bit(bitnum, &all, 32) {

> +               int gpio = event_bitnum_to_gpio(bitnum);
> +               u32 mask = BIT(bitnum), evpol;

unsigned long evpol;

> +               int level;
> +
> +               do {
> +                       evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
> +                       level = wpcm450_gpio_get(&pctrl->gc, gpio);

> +                       /* Switch event polarity to the opposite of the current level */
> +                       if (level)
> +                               evpol &= ~mask;
> +                       else
> +                               evpol |= mask;

__assign_bit(bitnum, &evpol, level);

> +
> +                       iowrite32(evpol, pctrl->gpio_base + WPCM450_GPEVPOL);
> +               } while (wpcm450_gpio_get(&pctrl->gc, gpio) != level);
> +       }
> +}

...

> +static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type)
> +{

Consider to assign handler type here.

> +}

...

> +/* pinmux handing */
> +

Useless.

...

> +/*
> + * pin:             name, number
> + * group:    name, npins,   pins
> + * function: name, ngroups, groups
> + */
> +struct wpcm450_group {
> +       const char *name;
> +       const unsigned int *pins;
> +       int npins;
> +};

Use struct group_desc from core.h.

...

> +/* pinctrl_ops */

Useless.

> +static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);

> +       dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));

Ditto.

> +       return ARRAY_SIZE(wpcm450_groups);
> +}

...

> +/* pinmux_ops  */

Useless.

...

> +static int wpcm450_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                      struct pinctrl_gpio_range *range,
> +                                      unsigned int offset)
> +{
> +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);

> +       if (!range) {
> +               dev_err(npcm->dev, "invalid range\n");
> +               return -EINVAL;
> +       }

Dead code?

> +       if (!range->gc) {
> +               dev_err(npcm->dev, "invalid gpiochip\n");
> +               return -EINVAL;
> +       }

Dead code?

> +       wpcm450_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio);
> +
> +       return 0;
> +}

...

> +/* Release GPIO back to pinctrl mode */
> +static void wpcm450_gpio_request_free(struct pinctrl_dev *pctldev,
> +                                     struct pinctrl_gpio_range *range,
> +                                     unsigned int offset)
> +{
> +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       int virq;
> +
> +       virq = irq_find_mapping(pctrl->domain, offset);
> +       if (virq)
> +               irq_dispose_mapping(virq);

Why it needs to be done here? What about the GPIO library API that
does some additional stuff?

> +}

...

> +/* pinconf_ops */

Useless.

...

> +static int debounce_bitmask(int gpio)
> +{
> +       if (gpio >= 0 && gpio < 16)
> +               return BIT(gpio);
> +       return -EINVAL;
> +}

I don't see users of it except one below, care to inline?

> +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> +                             unsigned long *config)
> +{

> +       switch (param) {
> +       case PIN_CONFIG_INPUT_DEBOUNCE:
> +               mask = debounce_bitmask(pin);
> +               if (mask < 0)
> +                       return mask;

> +               break;
> +       default:
> +               return -ENOTSUPP;
> +       }
> +
> +       return 0;
> +}

...

> +/* Set multiple configuration settings for a pin */

Useless.

...

> +static int wpcm450_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> +                             unsigned long *configs, unsigned int num_configs)
> +{
> +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);

> +       int rc;

Why out of a sudden different (inconsistent) name?

> +       return 0;
> +}

...

> +       if (!of_find_property(np, "gpio-controller", NULL))
> +               return -ENODEV;

Dead code?

...

> +       pctrl->gpio_base = of_iomap(np, 0);

devm_platform_ioremap_resource();

> +       if (!pctrl->gpio_base) {
> +               dev_err(pctrl->dev, "Resource fail for GPIO controller\n");
> +               return -ENOMEM;
> +       }

Here leak of resources. See above.

...

> +       pctrl->gc.get_direction = wpcm450_gpio_get_direction;
> +       pctrl->gc.direction_input = wpcm450_gpio_direction_input;
> +       pctrl->gc.direction_output = wpcm450_gpio_direction_output;
> +       pctrl->gc.get = wpcm450_gpio_get;
> +       pctrl->gc.set = wpcm450_gpio_set;

No ->set_config()?

...

> +       girq->default_type = IRQ_TYPE_NONE;

> +       girq->handler = handle_level_irq;

Use ->irq_set_type() for this. Here should be handle_bad_irq().

> +       for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {

> +               int irq = irq_of_parse_and_map(np, i);

fwnode_get_irq()

> +               if (irq < 0) {
> +                       dev_err(pctrl->dev, "No IRQ for GPIO controller\n");
> +                       return irq;
> +               }
> +               girq->parents[i] = irq;
> +       }

...

> +       pctrl->pctldev = devm_pinctrl_register(&pdev->dev,
> +                                              &wpcm450_pinctrl_desc, pctrl);
> +       if (IS_ERR(pctrl->pctldev)) {

> +               dev_err(&pdev->dev, "Failed to register pinctrl device\n");
> +               return PTR_ERR(pctrl->pctldev);

Shouldn't it be return dev_err_probe(...); here?

> +       }

...

> +       pr_info("WPCM450 pinctrl and GPIO driver probed\n");

Besides you have to use dev_*() this is completely useless and noisy message.

...

> +static const struct of_device_id wpcm450_pinctrl_match[] = {
> +       { .compatible = "nuvoton,wpcm450-pinctrl" },

> +       { },

Comma is not needed for terminator line.

> +};

...

> +               .suppress_bind_attrs = true,

Why?

-- 
With Best Regards,
Andy Shevchenko

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4342 bytes --]

Hi "Jonathan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pinctrl/devel]
[also build test WARNING on robh/for-next linus/master v5.13-rc4 next-20210602]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jonathan-Neusch-fer/Nuvoton-WPCM450-pinctrl-and-GPIO-driver/20210602-200629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/42f026e0692ea0083822284f98d2b82dcb6141ef
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonathan-Neusch-fer/Nuvoton-WPCM450-pinctrl-and-GPIO-driver/20210602-200629
        git checkout 42f026e0692ea0083822284f98d2b82dcb6141ef
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:409,
                    from include/linux/kernel.h:17,
                    from include/asm-generic/bug.h:20,
                    from arch/alpha/include/asm/bug.h:23,
                    from include/linux/bug.h:5,
                    from include/linux/thread_info.h:12,
                    from include/asm-generic/current.h:5,
                    from ./arch/alpha/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/ratelimit.h:6,
                    from include/linux/dev_printk.h:16,
                    from include/linux/device.h:15,
                    from drivers/pinctrl/nuvoton/pinctrl-wpcm450.c:6:
   drivers/pinctrl/nuvoton/pinctrl-wpcm450.c: In function 'wpcm450_get_groups_count':
>> drivers/pinctrl/nuvoton/pinctrl-wpcm450.c:882:21: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat=]
     882 |  dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
         |                     ^~~~~~~~~~~~~~~~~~
   include/linux/dynamic_debug.h:129:15: note: in definition of macro '__dynamic_func_call'
     129 |   func(&id, ##__VA_ARGS__);  \
         |               ^~~~~~~~~~~
   include/linux/dynamic_debug.h:161:2: note: in expansion of macro '_dynamic_func_call'
     161 |  _dynamic_func_call(fmt,__dynamic_dev_dbg,   \
         |  ^~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:2: note: in expansion of macro 'dynamic_dev_dbg'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |  ^~~~~~~~~~~~~~~
   include/linux/dev_printk.h:123:23: note: in expansion of macro 'dev_fmt'
     123 |  dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                       ^~~~~~~
   drivers/pinctrl/nuvoton/pinctrl-wpcm450.c:882:2: note: in expansion of macro 'dev_dbg'
     882 |  dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
         |  ^~~~~~~
   drivers/pinctrl/nuvoton/pinctrl-wpcm450.c:882:35: note: format string is defined here
     882 |  dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
         |                                  ~^
         |                                   |
         |                                   int
         |                                  %ld


vim +882 drivers/pinctrl/nuvoton/pinctrl-wpcm450.c

   876	
   877	/* pinctrl_ops */
   878	static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev)
   879	{
   880		struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
   881	
 > 882		dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
   883		return ARRAY_SIZE(wpcm450_groups);
   884	}
   885	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68321 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3318 bytes --]

Hi "Jonathan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pinctrl/devel]
[also build test WARNING on robh/for-next linus/master v5.13-rc4 next-20210603]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jonathan-Neusch-fer/Nuvoton-WPCM450-pinctrl-and-GPIO-driver/20210602-200629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-randconfig-a004-20210603 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d8e0ae9a76a62bdc6117630d59bf9967ac9bb4ea)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/42f026e0692ea0083822284f98d2b82dcb6141ef
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jonathan-Neusch-fer/Nuvoton-WPCM450-pinctrl-and-GPIO-driver/20210602-200629
        git checkout 42f026e0692ea0083822284f98d2b82dcb6141ef
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c:1564:41: warning: format specifies type 'int' but the argument has type 'unsigned long' [-Wformat]
           dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(npcm7xx_groups));
                                           ~~     ^~~~~~~~~~~~~~~~~~~~~~~~~~
                                           %lu
   include/linux/dev_printk.h:126:46: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
                                               ~~~     ^~~~~~~~~~~
   include/linux/kernel.h:42:25: note: expanded from macro 'ARRAY_SIZE'
   #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +1564 drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c

3b588e43ee5c7a Tomer Maimon 2018-08-08  1559  
3b588e43ee5c7a Tomer Maimon 2018-08-08  1560  static int npcm7xx_get_groups_count(struct pinctrl_dev *pctldev)
3b588e43ee5c7a Tomer Maimon 2018-08-08  1561  {
3b588e43ee5c7a Tomer Maimon 2018-08-08  1562  	struct npcm7xx_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
3b588e43ee5c7a Tomer Maimon 2018-08-08  1563  
3b588e43ee5c7a Tomer Maimon 2018-08-08 @1564  	dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(npcm7xx_groups));
3b588e43ee5c7a Tomer Maimon 2018-08-08  1565  	return ARRAY_SIZE(npcm7xx_groups);
3b588e43ee5c7a Tomer Maimon 2018-08-08  1566  }
3b588e43ee5c7a Tomer Maimon 2018-08-08  1567  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43675 bytes --]

Re: [PATCH 1/8] dt-bindings: arm/npcm: Add binding for global control registers (GCR)

[Linus Walleij]

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will
> be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and
> WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and
> version information.
>
> This patch adds a binding to describe this node.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

(...)

> +    gcr: gcr@800000 {
> +      compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd";
> +      reg = <0x800000 0x1000>;
> +    };

gcr looks a bit idiomatic, isn't

syscon:  syscon@... better?

Nitpicky though and looks good to me either way:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 3/8] ARM: dts: wpcm450: Add global control registers (GCR) node

[Linus Walleij]

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> The Global Control Registers (GCR) are a block of registers in Nuvoton
> SoCs that expose misc functionality such as chip model and version
> information or pinmux settings.
>
> This patch adds a GCR node to nuvoton-wpcm450.dtsi in preparation for
> enabling pinctrl on this SoC.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

As noted I would name this architecture-neutral with
syscon@...

Yours,
Linus Walleij

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

[Linus Walleij]

Hi Jonathan!

thanks for your patch!

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> 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

Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

[Linus Walleij]

Hi Jonathan!

thanks for your patch!

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> +  interrupts: true

maxitems 4 right?

Make an enum:

interrupts:
  - description: what IRQ0 is for
  - description: what IRQ1 is for
  - description: what IRQ2 is for
  - description: what IRQ3 is for

And describe how these interrupts are used. Because I am suspicious that they
actually correspond to 4 different GPIO blocks, which should then be their own
nodes.

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    pinctrl: pinctrl@b8003000 {
> +      compatible = "nuvoton,wpcm450-pinctrl";
> +      reg = <0xb8003000 0x1000>;
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      interrupts = <2 IRQ_TYPE_LEVEL_HIGH
> +                    3 IRQ_TYPE_LEVEL_HIGH
> +                    4 IRQ_TYPE_LEVEL_HIGH
> +                    5 IRQ_TYPE_LEVEL_HIGH>;

So these.

> +      rmii2 {
> +        groups = "rmii2";
> +        function = "rmii2";
> +      };
> +
> +      pinctrl_uid: uid {
> +        pins = "gpio14";
> +        input-debounce = <1>;
> +      };

I challenge you here and encourage you to put a node for each
GPIO "port":

  port0: gpio@0 {
 ....
  };
  port1: gpio@1 {
 ....
  };


> +    gpio-keys {
> +      compatible = "gpio-keys";
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_uid>;
> +
> +      uid {
> +        label = "UID";
> +        linux,code = <102>;
> +        gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;

Would be gpios <&port0 14...>

Yours,
Linus Walleij

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

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 15146 bytes --]

Hello,

On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> 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.
> >
> > Some functionality implemented in the hardware was (for now) left unused
> > in the driver, specifically blinking and pull-up/down.
> 
> ...
> 
> > +config PINCTRL_WPCM450
> > +       bool "Pinctrl and GPIO driver for Nuvoton WPCM450"
> 
> Why it can't be a module?

My mistake, it does indeed work as a module. I'll change it to tristate.

> > +       depends on (ARCH_WPCM450 || COMPILE_TEST) && OF
> 
> Is it really OF dependent (see below)?

After switching to the fwnode API, no. I'll drop the dependency.

> > +       select PINMUX
> > +       select PINCONF
> > +       select GENERIC_PINCONF
> > +       select GPIOLIB
> > +       select GPIO_GENERIC
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         Say Y here to enable pin controller and GPIO support
> > +         for the Nuvoton WPCM450 SoC.
> 
> >From this help it's not clear why user should or shouldn't enable it.
> Please, elaborate (and hence fix checkpatch warning).

I'll try something like this, but I'm open for better ideas:

	help
	  Say Y or M here to enable pin controller and GPIO support for
	  the Nuvoton WPCM450 SoC. This is strongly recommended when
	  building a kernel that will run on this chip.

	  If this driver is compiled as a module, it will be named
	  pinctrl-wpcm450.


I could mention some examples of functionality enabled by this driver:
LEDs, host power control, Ethernet.

(LEDs and host power control use GPIOs, at least on the Supermicro X9
 board I've been using. Ethernet MDIO must be enabled through the
 pinctrl driver, unless the bootloader has done so already; on my board
 the bootloader doesn't do this.)


> > +#include <linux/module.h>
> 
> mod_devicetable.h

I'll add it.

> 
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> 
> > +#include <linux/pinctrl/machine.h>
> > +#include <linux/pinctrl/pinconf.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinmux.h>
> 
> Can you move this group...
> 
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> 
> ...here?
> 
> It will show the relation with pin control subsystem and separate from
> global / generic headers.

Will do.


> > +        * This spinlock protects registers and struct wpcm450_pinctrl fields
> 
> spin lock

Ok


> > +/* GPIO handling in the pinctrl driver */
> > +
> 
> Useless.

Ok

> 
> ...
> 
> > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
> > +                                     unsigned int offset)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> > +       const struct wpcm450_port *port = to_port(offset);
> > +       unsigned long flags;
> > +       u32 cfg0;
> > +       int dir;
> > +
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       if (port->cfg0) {
> > +               cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
> 
> > +               dir = !(cfg0 & port_mask(port, offset));
> > +       } else {
> > +               /* If cfg0 is unavailable, the GPIO is always an input */
> > +               dir = 1;
> > +       }
> 
> Why above is under spin lock?
> Same question for all other similar places in different functions, if any.

My intention was to protect the ioread32. But given that it's just a
single MMIO read, this might be unnecessary.

wpcm450_gpio_direction_input and a few other functions implement a
read/modify/write cycle, where the lock makes more sense:

	spin_lock_irqsave(&pctrl->lock, flags);
	if (port->cfg0) {
		cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
		cfg0 &= ~port_mask(port, offset);
		iowrite32(cfg0, pctrl->gpio_base + port->cfg0);
	}
	spin_unlock_irqrestore(&pctrl->lock, flags);

(Here, as above, the locking calls moved into the if block, but I'm not
 sure how much of an improvement that would be.)

> 
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +       return dir;
> > +}
> 
> ...
> 
> > +static int wpcm450_gpio_direction_output(struct gpio_chip *chip,
> > +                                        unsigned int offset, int value)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> > +       const struct wpcm450_port *port = to_port(offset);
> > +       unsigned long flags;
> > +       u32 dataout, cfg0;
> 
> > +       int ret = 0;
> 
> Redundant. Can do it without it.
> 
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +       if (port->cfg0) {
> 
> > +       } else {
> > +               ret = -EINVAL;
> > +       }
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +       return ret;
> > +}

I'll refactor it to return -EINVAL early.


> > +/* Interrupt support */
> > +
> 
> Useless besides being in a bad style.

Ok, will remove.
> 
> ...
> 
> > +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;
> > +}
> 
> Can you consider to use bitmap_bitremap()

> > +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;
> > +}
> 
> Ditto.
> 
> See gpio-xilinx.c for example.

I looked at it now. I'm not convinced it'll improve readability here.

> 
> ...
> 
> > +static void wpcm450_gpio_irq_ack(struct irq_data *d)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> 
> > +       int mask = event_bitmask(d->hwirq);
> 
> Move the assignment closer to the check.
> Ditto for other same and similar cases in the code.

Will do.

> 
> > +       unsigned long flags;
> > +
> > +       if (mask < 0)
> > +               return;
> 
> > +}
> 
> ...
> 
> > +       int mask = event_bitmask(d->hwirq);
> 
> Use irqd_to_hwirq() (please check that I spelled it correctly).
> Same for all hwirq getters.

Will do.

> 
> ...
> 
> > +static void wpcm450_gpio_fix_evpol(struct wpcm450_pinctrl *pctrl, unsigned long all)
> > +{
> > +       int bitnum;
> 
> Can it be negative?

No. I'll change it to unsigned int.
> 
> > +       for_each_set_bit(bitnum, &all, 32) {
> 
> > +               int gpio = event_bitnum_to_gpio(bitnum);
> > +               u32 mask = BIT(bitnum), evpol;
> 
> unsigned long evpol;
> 
> > +               int level;
> > +
> > +               do {
> > +                       evpol = ioread32(pctrl->gpio_base + WPCM450_GPEVPOL);
> > +                       level = wpcm450_gpio_get(&pctrl->gc, gpio);
> 
> > +                       /* Switch event polarity to the opposite of the current level */
> > +                       if (level)
> > +                               evpol &= ~mask;
> > +                       else
> > +                               evpol |= mask;
> 
> __assign_bit(bitnum, &evpol, level);

Ah, very useful. I'll use __assign_bit in a few places.


> > +static int wpcm450_gpio_set_irq_type(struct irq_data *d, unsigned int flow_type)
> > +{
> 
> Consider to assign handler type here.

Will do.


> > +/* pinmux handing */
> > +
> 
> Useless.

Ok.


> > +/*
> > + * pin:             name, number
> > + * group:    name, npins,   pins
> > + * function: name, ngroups, groups
> > + */
> > +struct wpcm450_group {
> > +       const char *name;
> > +       const unsigned int *pins;
> > +       int npins;
> > +};
> 
> Use struct group_desc from core.h.

Ok.

> > +/* pinctrl_ops */
> 
> Useless.

Ok

> 
> > +static int wpcm450_get_groups_count(struct pinctrl_dev *pctldev)
> > +{
> > +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);
> 
> > +       dev_dbg(npcm->dev, "group size: %d\n", ARRAY_SIZE(wpcm450_groups));
> 
> Ditto.

Removing this and similar messages.


> > +/* pinmux_ops  */
> 
> Useless.

Ok

> 
> ...
> 
> > +static int wpcm450_gpio_request_enable(struct pinctrl_dev *pctldev,
> > +                                      struct pinctrl_gpio_range *range,
> > +                                      unsigned int offset)
> > +{
> > +       struct wpcm450_pinctrl *npcm = pinctrl_dev_get_drvdata(pctldev);

Oops, I forgot to change the name to pctrl here.

> 
> > +       if (!range) {
> > +               dev_err(npcm->dev, "invalid range\n");
> > +               return -EINVAL;
> > +       }
> 
> Dead code?

Indeed, the range pointer is checked in pin_request().

> 
> > +       if (!range->gc) {
> > +               dev_err(npcm->dev, "invalid gpiochip\n");
> > +               return -EINVAL;
> > +       }
> 
> Dead code?

... and range->gc isn't used here... I'll remove the check.
> 
> > +       wpcm450_setfunc(npcm->gcr_regmap, &offset, 1, fn_gpio);
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +/* Release GPIO back to pinctrl mode */
> > +static void wpcm450_gpio_request_free(struct pinctrl_dev *pctldev,
> > +                                     struct pinctrl_gpio_range *range,
> > +                                     unsigned int offset)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> > +       int virq;
> > +
> > +       virq = irq_find_mapping(pctrl->domain, offset);
> > +       if (virq)
> > +               irq_dispose_mapping(virq);
> 
> Why it needs to be done here?

I copied this function from the pinctrl-npcm7xx driver; I don't really
see a reason to call irq_dispose_mapping here.

> What about the GPIO library API that does some additional stuff?

I don't know which gpiolib function would be appropriate here, sorry.


> > +/* pinconf_ops */
> 
> Useless.

Ok


> > +static int debounce_bitmask(int gpio)
> > +{
> > +       if (gpio >= 0 && gpio < 16)
> > +               return BIT(gpio);
> > +       return -EINVAL;
> > +}
> 
> I don't see users of it except one below, care to inline?

It should have been used in wpcm450_config_set_one, but I missed that.

> > +static int wpcm450_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                             unsigned long *config)
> > +{
> 
> > +       switch (param) {
> > +       case PIN_CONFIG_INPUT_DEBOUNCE:
> > +               mask = debounce_bitmask(pin);
> > +               if (mask < 0)
> > +                       return mask;
> 
> > +               break;
> > +       default:
> > +               return -ENOTSUPP;
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +/* Set multiple configuration settings for a pin */
> 
> Useless.

Ok.

> 
> ...
> 
> > +static int wpcm450_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> > +                             unsigned long *configs, unsigned int num_configs)
> > +{
> > +       struct wpcm450_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> 
> > +       int rc;
> 
> Why out of a sudden different (inconsistent) name?

No particular reason. I'll change it to ret.

> 
> > +       return 0;
> > +}
> 
> ...
> 
> > +       if (!of_find_property(np, "gpio-controller", NULL))
> > +               return -ENODEV;
> 
> Dead code?

The point here was to check if the node is marked as a GPIO controller,
with the boolean property "gpio-controller" (so device_property_read_bool
would probably be more appropriate).

However, since the gpio-controller property is already defined as
required in the DT binding, I'm not sure it's worth checking here.

> 
> ...
> 
> > +       pctrl->gpio_base = of_iomap(np, 0);
> 
> devm_platform_ioremap_resource();

Will change.
> 
> > +       if (!pctrl->gpio_base) {
> > +               dev_err(pctrl->dev, "Resource fail for GPIO controller\n");
> > +               return -ENOMEM;
> > +       }
> 
> Here leak of resources. See above.
> 
> ...
> 
> > +       pctrl->gc.get_direction = wpcm450_gpio_get_direction;
> > +       pctrl->gc.direction_input = wpcm450_gpio_direction_input;
> > +       pctrl->gc.direction_output = wpcm450_gpio_direction_output;
> > +       pctrl->gc.get = wpcm450_gpio_get;
> > +       pctrl->gc.set = wpcm450_gpio_set;
> 
> No ->set_config()?

Good point, I'll hook it up to wpcm450_config_set_one().

> 
> ...
> 
> > +       girq->default_type = IRQ_TYPE_NONE;
> 
> > +       girq->handler = handle_level_irq;
> 
> Use ->irq_set_type() for this. Here should be handle_bad_irq().

Ok.

> 
> > +       for (i = 0; i < WPCM450_NUM_GPIO_IRQS; i++) {
> 
> > +               int irq = irq_of_parse_and_map(np, i);
> 
> fwnode_get_irq()

Indeed, will switch to fwnode_irq_get.

> 
> > +               if (irq < 0) {
> > +                       dev_err(pctrl->dev, "No IRQ for GPIO controller\n");
> > +                       return irq;
> > +               }
> > +               girq->parents[i] = irq;
> > +       }
> 
> ...
> 
> > +       pctrl->pctldev = devm_pinctrl_register(&pdev->dev,
> > +                                              &wpcm450_pinctrl_desc, pctrl);
> > +       if (IS_ERR(pctrl->pctldev)) {
> 
> > +               dev_err(&pdev->dev, "Failed to register pinctrl device\n");
> > +               return PTR_ERR(pctrl->pctldev);
> 
> Shouldn't it be return dev_err_probe(...); here?

It's so far a rare pattern after devm_pinctrl_register, but it makes
sense. Will change (in both cases above).

(Perhaps I should also use dev_err_probe in wpcm450_gpio_register, since
it's called from the probe function.)

> 
> > +       }
> 
> ...
> 
> > +       pr_info("WPCM450 pinctrl and GPIO driver probed\n");
> 
> Besides you have to use dev_*() this is completely useless and noisy message.

I'll remove this message and review the other messages.

> 
> ...
> 
> > +static const struct of_device_id wpcm450_pinctrl_match[] = {
> > +       { .compatible = "nuvoton,wpcm450-pinctrl" },
> 
> > +       { },
> 
> Comma is not needed for terminator line.

True, will remove.
> 
> > +};
> 
> ...
> 
> > +               .suppress_bind_attrs = true,
> 
> Why?

Copied from pinctrl-npcm7xx, but I see no reason to keep it.

> 
> -- 
> With Best Regards,
> Andy Shevchenko


Thank you for your detailed review,
Jonthan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/8] dt-bindings: arm/npcm: Add binding for global control registers (GCR)

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

On Fri, Jun 04, 2021 at 10:00:18AM +0200, Linus Walleij wrote:
> On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> 
> > A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will
> > be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and
> > WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and
> > version information.
> >
> > This patch adds a binding to describe this node.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> (...)
> 
> > +    gcr: gcr@800000 {
> > +      compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd";
> > +      reg = <0x800000 0x1000>;
> > +    };
> 
> gcr looks a bit idiomatic, isn't
> 
> syscon:  syscon@... better?

I think I'll go with syscon@..., because it's the right generic name,
but gcr for the label, matching current usage.

> Nitpicky though and looks good to me either way:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/8] ARM: dts: wpcm450: Add global control registers (GCR) node

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Fri, Jun 04, 2021 at 10:01:07AM +0200, Linus Walleij wrote:
> On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> 
> > The Global Control Registers (GCR) are a block of registers in Nuvoton
> > SoCs that expose misc functionality such as chip model and version
> > information or pinmux settings.
> >
> > This patch adds a GCR node to nuvoton-wpcm450.dtsi in preparation for
> > enabling pinctrl on this SoC.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> As noted I would name this architecture-neutral with
> syscon@...

Will do.


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]

On Fri, Jun 04, 2021 at 11:35:48AM +0200, Linus Walleij wrote:
> Hi Jonathan!
> 
> thanks for your patch!

Hi again!

> On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> 
> > +  interrupts: true
> 
> maxitems 4 right?

Yes.

> Make an enum:
> 
> interrupts:
>   - description: what IRQ0 is for
>   - description: what IRQ1 is for
>   - description: what IRQ2 is for
>   - description: what IRQ3 is for
> 
> And describe how these interrupts are used.

Good point.

- IRQ0 is for events (interrupts) from GPIOs  0 to  3.
- IRQ1 is for events (interrupts) from GPIOs  4 to 11.
- IRQ2 is for events (interrupts) from GPIOs 12 to 15.
- IRQ3 is for events (interrupts) from GPIOs 24 to 25.

> Because I am suspicious that they actually correspond to 4 different
> GPIO blocks, which should then be their own nodes.

Unfortunately, It's not that simple. The GPIO ports (as defined by the
groups of registers that do GPIO direction/input/output) are organised
like this:

- GPIO port 0 starts at GPIO   0 and is 16 GPIOs long.
- GPIO port 1 starts at GPIO  16 and is 16 GPIOs long.
- GPIO port 2 starts at GPIO  32 and is 16 GPIOs long.
- GPIO port 3 starts at GPIO  48 and is 16 GPIOs long.
- GPIO port 4 starts at GPIO  64 and is 16 GPIOs long.
- GPIO port 5 starts at GPIO  80 and is 16 GPIOs long.
- GPIO port 6 starts at GPIO  96 and is 18 GPIOs long.
- GPIO port 7 starts at GPIO 114 and is 14 GPIOs long.

(They didn't even make it so that each one has 16 GPIOs...)

As you can see, only a few GPIOs are connected to interrupt logic; most
of them are in port 0, and the remaining two are in port 1.

Forthermore, the GPIO ports don't all have the same set of registers, so
that the register layout of each can't be predicted by the offset of the
first register.

> 
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    pinctrl: pinctrl@b8003000 {
> > +      compatible = "nuvoton,wpcm450-pinctrl";
> > +      reg = <0xb8003000 0x1000>;
> > +      gpio-controller;
> > +      #gpio-cells = <2>;
> > +      interrupts = <2 IRQ_TYPE_LEVEL_HIGH
> > +                    3 IRQ_TYPE_LEVEL_HIGH
> > +                    4 IRQ_TYPE_LEVEL_HIGH
> > +                    5 IRQ_TYPE_LEVEL_HIGH>;
> 
> So these.
> 
> > +      rmii2 {
> > +        groups = "rmii2";
> > +        function = "rmii2";
> > +      };
> > +
> > +      pinctrl_uid: uid {
> > +        pins = "gpio14";
> > +        input-debounce = <1>;
> > +      };
> 
> I challenge you here and encourage you to put a node for each
> GPIO "port":
> 
>   port0: gpio@0 {
>  ....
>   };
>   port1: gpio@1 {
>  ....
>   };

Hmm, well, if the unit addresses simply go from 0 to 7, rather than
encoding offsets, this could work. But it won't help much with the IRQ
problem.


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Andy Shevchenko]

On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:
> On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer
> > <j.neuschaefer@gmx.net> wrote:

...

> > > +       help
> > > +         Say Y here to enable pin controller and GPIO support
> > > +         for the Nuvoton WPCM450 SoC.
> >
> > >From this help it's not clear why user should or shouldn't enable it.
> > Please, elaborate (and hence fix checkpatch warning).
>
> I'll try something like this, but I'm open for better ideas:
>
>         help
>           Say Y or M here to enable pin controller and GPIO support for
>           the Nuvoton WPCM450 SoC. This is strongly recommended when
>           building a kernel that will run on this chip.
>
>           If this driver is compiled as a module, it will be named
>           pinctrl-wpcm450.

This looks good enough.

> I could mention some examples of functionality enabled by this driver:
> LEDs, host power control, Ethernet.
>
> (LEDs and host power control use GPIOs, at least on the Supermicro X9
>  board I've been using. Ethernet MDIO must be enabled through the
>  pinctrl driver, unless the bootloader has done so already; on my board
>  the bootloader doesn't do this.)

...

> > > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
> > > +                                     unsigned int offset)
> > > +{
> > > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> > > +       const struct wpcm450_port *port = to_port(offset);
> > > +       unsigned long flags;
> > > +       u32 cfg0;
> > > +       int dir;
> > > +
> > > +       spin_lock_irqsave(&pctrl->lock, flags);
> > > +       if (port->cfg0) {
> > > +               cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
> >
> > > +               dir = !(cfg0 & port_mask(port, offset));
> > > +       } else {
> > > +               /* If cfg0 is unavailable, the GPIO is always an input */
> > > +               dir = 1;
> > > +       }
> >
> > Why above is under spin lock?
> > Same question for all other similar places in different functions, if any.
>
> My intention was to protect the ioread32. But given that it's just a
> single MMIO read, this might be unnecessary.

Sometimes it's necessary and I'm not talking about it. (I put blank
lines around the code I was commenting on)

So, What I meant above is to get something like this

if (port->cfg0) {
  spin lock
  ...
  spin unlock
} else {
  ...
}

or equivalent ideas.

> > > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > > +       return dir;
> > > +}

...

> > > +static int wpcm450_gpio_direction_output(struct gpio_chip *chip,
> > > +                                        unsigned int offset, int value)
> > > +{
> > > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> > > +       const struct wpcm450_port *port = to_port(offset);
> > > +       unsigned long flags;
> > > +       u32 dataout, cfg0;
> >
> > > +       int ret = 0;
> >
> > Redundant. Can do it without it.
> >
> > > +       spin_lock_irqsave(&pctrl->lock, flags);
> > > +       if (port->cfg0) {
> >
> > > +       } else {
> > > +               ret = -EINVAL;
> > > +       }
> > > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > > +       return ret;
> > > +}
>
> I'll refactor it to return -EINVAL early.

Here a similar approach can be used.

...

> > What about the GPIO library API that does some additional stuff?
>
> I don't know which gpiolib function would be appropriate here, sorry.

When you leave those request and release callbacks untouched the GPIO
library will assign default ones. You may see what they do.

...

> > > +       if (!of_find_property(np, "gpio-controller", NULL))
> > > +               return -ENODEV;
> >
> > Dead code?
>
> The point here was to check if the node is marked as a GPIO controller,
> with the boolean property "gpio-controller" (so device_property_read_bool
> would probably be more appropriate).
>
> However, since the gpio-controller property is already defined as
> required in the DT binding, I'm not sure it's worth checking here.

Exactly my point.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

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
> <j.neuschaefer@gmx.net> 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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Sun, Jun 13, 2021 at 01:06:15PM +0300, Andy Shevchenko wrote:
> On Sun, Jun 13, 2021 at 2:20 AM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> > On Wed, Jun 02, 2021 at 03:50:39PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 2, 2021 at 3:05 PM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
[...]
> > > > +static int wpcm450_gpio_get_direction(struct gpio_chip *chip,
> > > > +                                     unsigned int offset)
> > > > +{
> > > > +       struct wpcm450_pinctrl *pctrl = gpiochip_get_data(chip);
> > > > +       const struct wpcm450_port *port = to_port(offset);
> > > > +       unsigned long flags;
> > > > +       u32 cfg0;
> > > > +       int dir;
> > > > +
> > > > +       spin_lock_irqsave(&pctrl->lock, flags);
> > > > +       if (port->cfg0) {
> > > > +               cfg0 = ioread32(pctrl->gpio_base + port->cfg0);
> > >
> > > > +               dir = !(cfg0 & port_mask(port, offset));
> > > > +       } else {
> > > > +               /* If cfg0 is unavailable, the GPIO is always an input */
> > > > +               dir = 1;
> > > > +       }
> > >
> > > Why above is under spin lock?
> > > Same question for all other similar places in different functions, if any.
> >
> > My intention was to protect the ioread32. But given that it's just a
> > single MMIO read, this might be unnecessary.
> 
> Sometimes it's necessary and I'm not talking about it. (I put blank
> lines around the code I was commenting on)
> 
> So, What I meant above is to get something like this
> 
> if (port->cfg0) {
>   spin lock
>   ...
>   spin unlock
> } else {
>   ...
> }
> 
> or equivalent ideas.

Ah, in other words: Narrowing the scope of the lock as far as possible.
I'll keep it in mind for v2.


> > > What about the GPIO library API that does some additional stuff?
> >
> > I don't know which gpiolib function would be appropriate here, sorry.
> 
> When you leave those request and release callbacks untouched the GPIO
> library will assign default ones. You may see what they do.

Ah, I see. I'll look into it.


> ...
> 
> > > > +       if (!of_find_property(np, "gpio-controller", NULL))
> > > > +               return -ENODEV;
> > >
> > > Dead code?
> >
> > The point here was to check if the node is marked as a GPIO controller,
> > with the boolean property "gpio-controller" (so device_property_read_bool
> > would probably be more appropriate).
> >
> > However, since the gpio-controller property is already defined as
> > required in the DT binding, I'm not sure it's worth checking here.
> 
> Exactly my point.

Alright.


Thanks,
Jonthan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/8] dt-bindings: arm/npcm: Add binding for global control registers (GCR)

[Rob Herring]

On Wed, Jun 02, 2021 at 02:03:22PM +0200, Jonathan Neuschäfer wrote:
> A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will
> be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and
> WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and
> version information.
> 
> This patch adds a binding to describe this node.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../bindings/arm/npcm/nuvoton,gcr.yaml        | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
> new file mode 100644
> index 0000000000000..3174279f7713a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/npcm/nuvoton,gcr.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/npcm/nuvoton,gcr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Global Control Registers block in Nuvoton SoCs
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +description: |
> +  The Global Control Registers (GCR) are a block of registers in Nuvoton SoCs
> +  that expose misc functionality such as chip model and version information or
> +  pinmux settings.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nuvoton,wpcm450-gcr
> +          - nuvoton,npcm750-gcr
> +      - const: syscon
> +      - const: simple-mfd

How is this a simple-mfd? There are no child nodes.

> +  reg: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gcr: gcr@800000 {
> +      compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd";
> +      reg = <0x800000 0x1000>;
> +    };
> --
> 2.30.2

Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

[Rob Herring]

On Wed, Jun 02, 2021 at 02:03:25PM +0200, Jonathan Neuschäfer wrote:
> This binding is heavily based on the one for NPCM7xx, because the
> hardware is similar. One notable difference is that there are no
> sub-nodes for GPIO banks, because the GPIO registers are arranged
> differently.
> 
> Certain pins support blink patterns in hardware. This is currently not
> modelled in the DT binding.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../pinctrl/nuvoton,wpcm450-pinctrl.yaml      | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> new file mode 100644
> index 0000000000000..0664fe2b90db6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,wpcm450-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton WPCM450 pin control and GPIO
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +properties:
> +  compatible:
> +    const: "nuvoton,wpcm450-pinctrl"

Don't need quotes.

> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts: true
> +
> +patternProperties:
> +  # There are two kinds of subnodes:
> +  # 1. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
> +  # 2. a pinctrl node configures properties of a single pin
> +  "^.*$":
> +    if:
> +      type: object
> +    then:

Don't do this hack for new bindings. Pick a node name pattern you can 
match on.

> +      allOf:
> +        - $ref: pincfg-node.yaml#
> +        - $ref: pinmux-node.yaml#
> +      properties:
> +        groups:
> +          description:
> +            One or more groups of pins to mux to a certain function
> +          minItems: 1
> +          items:
> +            enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
> +                    hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo,
> +                    clko, smi, uinc, gspi, mben, xcs2, xcs1, sdio, sspi, fi0,
> +                    fi1, fi2, fi3, fi4, fi5, fi6, fi7, fi8, fi9, fi10, fi11,
> +                    fi12, fi13, fi14, fi15, pwm0, pwm1, pwm2, pwm3, pwm4, pwm5,
> +                    pwm6, pwm7, hg0, hg1, hg2, hg3, hg4, hg5, hg6, hg7 ]
> +        function:
> +          description:
> +            The function that a group of pins is muxed to
> +          enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
> +                  hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo0,
> +                  dvo1, dvo2, dvo3, dvo4, dvo5, dvo6, dvo7, clko, smi, uinc,
> +                  gspi, mben, xcs2, xcs1, sdio, sspi, fi0, fi1, fi2, fi3, fi4,
> +                  fi5, fi6, fi7, fi8, fi9, fi10, fi11, fi12, fi13, fi14, fi15,
> +                  pwm0, pwm1, pwm2, pwm3, pwm4, pwm5, pwm6, pwm7, hg0, hg1,
> +                  hg2, hg3, hg4, hg5, hg6, hg7 ]
> +
> +        pins:
> +          description:
> +            A list of pins to configure in certain ways, such as enabling
> +            debouncing
> +          minItems: 1
> +          items:
> +            enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7,
> +                    gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio14,
> +                    gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gpio21,
> +                    gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, gpio28,
> +                    gpio29, gpio30, gpio31, gpio32, gpio33, gpio34, gpio35,
> +                    gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, gpio42,
> +                    gpio43, gpio44, gpio45, gpio46, gpio47, gpio48, gpio49,
> +                    gpio50, gpio51, gpio52, gpio53, gpio54, gpio55, gpio56,
> +                    gpio57, gpio58, gpio59, gpio60, gpio61, gpio62, gpio63,
> +                    gpio64, gpio65, gpio66, gpio67, gpio68, gpio69, gpio70,
> +                    gpio71, gpio72, gpio73, gpio74, gpio75, gpio76, gpio77,
> +                    gpio78, gpio79, gpio80, gpio81, gpio82, gpio83, gpio84,
> +                    gpio85, gpio86, gpio87, gpio88, gpio89, gpio90, gpio91,
> +                    gpio92, gpio93, gpio94, gpio95, gpio96, gpio97, gpio98,
> +                    gpio99, gpio100, gpio101, gpio102, gpio103, gpio104,
> +                    gpio105, gpio106, gpio107, gpio108, gpio109, gpio110,
> +                    gpio111, gpio112, gpio113, gpio114, gpio115, gpio116,
> +                    gpio117, gpio118, gpio119, gpio120, gpio121, gpio122,
> +                    gpio123, gpio124, gpio125, gpio126, gpio127 ]
> +
> +        input-debounce: true
> +        phandle: true

Needing this should be fixed now.

> +
> +      dependencies:
> +        groups: [ function ]
> +        function: [ groups ]
> +
> +      additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    pinctrl: pinctrl@b8003000 {
> +      compatible = "nuvoton,wpcm450-pinctrl";
> +      reg = <0xb8003000 0x1000>;
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      interrupts = <2 IRQ_TYPE_LEVEL_HIGH
> +                    3 IRQ_TYPE_LEVEL_HIGH
> +                    4 IRQ_TYPE_LEVEL_HIGH
> +                    5 IRQ_TYPE_LEVEL_HIGH>;
> +      rmii2 {
> +        groups = "rmii2";
> +        function = "rmii2";
> +      };
> +
> +      pinctrl_uid: uid {
> +        pins = "gpio14";
> +        input-debounce = <1>;
> +      };
> +    };
> +
> +    gpio-keys {
> +      compatible = "gpio-keys";
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_uid>;
> +
> +      uid {
> +        label = "UID";
> +        linux,code = <102>;
> +        gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> --
> 2.30.2

Re: [PATCH 1/8] dt-bindings: arm/npcm: Add binding for global control registers (GCR)

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On Tue, Jun 15, 2021 at 05:43:21PM -0600, Rob Herring wrote:
> On Wed, Jun 02, 2021 at 02:03:22PM +0200, Jonathan Neuschäfer wrote:
> > A nuvoton,*-gcr node is present in nuvoton-common-npcm7xx.dtsi and will
> > be added to nuvoton-wpcm450.dtsi. It is necessary for the NPCM7xx and
> > WPCM450 pinctrl drivers, and may later be used to retrieve SoC model and
> > version information.
> > 
> > This patch adds a binding to describe this node.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - nuvoton,wpcm450-gcr
> > +          - nuvoton,npcm750-gcr
> > +      - const: syscon
> > +      - const: simple-mfd
> 
> How is this a simple-mfd? There are no child nodes.

Some device trees, such as arch/arm/boot/dts/nuvoton-npcm730-gbs.dts,
have subnodes of the GCR node (specifically, they (ab)use mmio-mux to
set specific registers to specific values).

> > +  reg: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    gcr: gcr@800000 {
> > +      compatible = "nuvoton,npcm750-gcr", "syscon", "simple-mfd";
> > +      reg = <0x800000 0x1000>;
> > +    };

... I guess I should add a child node to the example, here.



Best regards,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1687 bytes --]

On Tue, Jun 15, 2021 at 05:45:58PM -0600, Rob Herring wrote:
> On Wed, Jun 02, 2021 at 02:03:25PM +0200, Jonathan Neuschäfer wrote:
> > This binding is heavily based on the one for NPCM7xx, because the
> > hardware is similar. One notable difference is that there are no
> > sub-nodes for GPIO banks, because the GPIO registers are arranged
> > differently.
> > 
> > Certain pins support blink patterns in hardware. This is currently not
> > modelled in the DT binding.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +properties:
> > +  compatible:
> > +    const: "nuvoton,wpcm450-pinctrl"
> 
> Don't need quotes.

Ok, I'll remove them.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 2

and I just noticed the inconsistency in quotes here. I'll fix it.

> > +
> > +  interrupts: true
> > +
> > +patternProperties:
> > +  # There are two kinds of subnodes:
> > +  # 1. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
> > +  # 2. a pinctrl node configures properties of a single pin
> > +  "^.*$":
> > +    if:
> > +      type: object
> > +    then:
> 
> Don't do this hack for new bindings. Pick a node name pattern you can 
> match on.

Ok.

> 
> > +      allOf:
> > +        - $ref: pincfg-node.yaml#
> > +        - $ref: pinmux-node.yaml#
> > +      properties:
[...]
> > +        phandle: true
> 
> Needing this should be fixed now.

Ok, I'll drop it.



Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]