[PATCH v9 0/3] Add Nuvoton NPCM SGPIO feature

[PATCH v9 0/3] Add Nuvoton NPCM SGPIO feature

[Jim Liu]

This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC.
Nuvoton NPCM SGPIO module is combine serial to parallel IC (HC595)
and parallel to serial IC (HC165), and use APB3 clock to control it.
This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
NPCM7xx/NPCM8xx have two sgpio module each module can support up
to 64 output pins,and up to 64 input pin, the pin is only for GPI or GPO.
GPIO pins have sequential, First half is GPO and second half is GPI.

Jim Liu (3):
  dt-bindings: gpio: add NPCM sgpio driver bindings
  arm: dts: nuvoton: npcm: Add sgpio feature
  gpio: nuvoton: Add Nuvoton NPCM sgpio driver

 .../bindings/gpio/nuvoton,sgpio.yaml          |  86 +++
 .../dts/nuvoton/nuvoton-common-npcm7xx.dtsi   |  24 +
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-npcm-sgpio.c                | 635 ++++++++++++++++++
 5 files changed, 753 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
 create mode 100644 drivers/gpio/gpio-npcm-sgpio.c

-- 
2.25.1

[PATCH v9 1/3] dt-bindings: gpio: add NPCM sgpio driver bindings

[Jim Liu]

Add dt-bindings document for the Nuvoton NPCM7xx sgpio driver

Signed-off-by: Jim Liu <jim.t90615@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes for v9:
   - no changed
Changes for v8:
   - no changed
Changes for v7:
   - no changed
---
 .../bindings/gpio/nuvoton,sgpio.yaml          | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
new file mode 100644
index 000000000000..84e0dbcb066c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/nuvoton,sgpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton SGPIO controller
+
+maintainers:
+  - Jim LIU <JJLIU0@nuvoton.com>
+
+description: |
+  This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC.
+  Nuvoton NPCM7xx SGPIO module is combine serial to parallel IC (HC595)
+  and parallel to serial IC (HC165), and use APB3 clock to control it.
+  This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
+  NPCM7xx/NPCM8xx have two sgpio module each module can support up
+  to 64 output pins,and up to 64 input pin, the pin is only for gpi or gpo.
+  GPIO pins have sequential, First half is gpo and second half is gpi.
+  GPIO pins can be programmed to support the following options
+  - Support interrupt option for each input port and various interrupt
+    sensitivity option (level-high, level-low, edge-high, edge-low)
+  - ngpios is number of nuvoton,input-ngpios GPIO lines and nuvoton,output-ngpios GPIO lines.
+    nuvoton,input-ngpios GPIO lines is only for gpi.
+    nuvoton,output-ngpios GPIO lines is only for gpo.
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,npcm750-sgpio
+      - nuvoton,npcm845-sgpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  nuvoton,input-ngpios:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The numbers of GPIO's exposed. GPIO lines is only for gpi.
+    minimum: 0
+    maximum: 64
+
+  nuvoton,output-ngpios:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The numbers of GPIO's exposed. GPIO lines is only for gpo.
+    minimum: 0
+    maximum: 64
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - '#gpio-cells'
+  - interrupts
+  - nuvoton,input-ngpios
+  - nuvoton,output-ngpios
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    gpio8: gpio@101000 {
+        compatible = "nuvoton,npcm750-sgpio";
+        reg = <0x101000 0x200>;
+        clocks = <&clk NPCM7XX_CLK_APB3>;
+        interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        nuvoton,input-ngpios = <64>;
+        nuvoton,output-ngpios = <64>;
+    };
-- 
2.25.1

[PATCH v9 2/3] arm: dts: nuvoton: npcm: Add sgpio feature

[Jim Liu]

Add the SGPIO controller to the NPCM7xx devicetree

Signed-off-by: Jim Liu <jim.t90615@gmail.com>
---
Changes for v9:
   - no changed
Changes for v8:
   - no changed
Changes for v7:
   - no changed
---
 .../dts/nuvoton/nuvoton-common-npcm7xx.dtsi   | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
index 868454ae6bde..df91517a4842 100644
--- a/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
+++ b/arch/arm/boot/dts/nuvoton/nuvoton-common-npcm7xx.dtsi
@@ -372,6 +372,30 @@ &fanin12_pins &fanin13_pins
 				status = "disabled";
 			};
 
+			gpio8: gpio@101000 {
+				compatible = "nuvoton,npcm750-sgpio";
+				reg = <0x101000 0x200>;
+				clocks = <&clk NPCM7XX_CLK_APB3>;
+				interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				nuvoton,input-ngpios = <64>;
+				nuvoton,output-ngpios = <64>;
+				status = "disabled";
+			};
+
+			gpio9: gpio@102000 {
+				compatible = "nuvoton,npcm750-sgpio";
+				reg = <0x102000 0x200>;
+				clocks = <&clk NPCM7XX_CLK_APB3>;
+				interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				nuvoton,input-ngpios = <64>;
+				nuvoton,output-ngpios = <64>;
+				status = "disabled";
+			};
+
 			i2c0: i2c@80000 {
 				reg = <0x80000 0x1000>;
 				compatible = "nuvoton,npcm750-i2c";
-- 
2.25.1

[PATCH v9 3/3] gpio: nuvoton: Add Nuvoton NPCM sgpio driver

[Jim Liu]

Add Nuvoton BMC NPCM7xx/NPCM8xx sgpio driver support.
Nuvoton NPCM SGPIO module is combine serial to parallel IC (HC595)
and parallel to serial IC (HC165), and use APB3 clock to control it.
This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
BMC can use this driver to increase 64 GPI pins and 64 GPO pins to use.

Signed-off-by: Jim Liu <jim.t90615@gmail.com>
---
Changes for v9:
   - fix kernel rebot test warning
Changes for v8:
   - Remove OF_GPIO/GPIO_GENERIC and redundant assignments
   - Use GENMASK() and BIT()
   - Use dev_WARN and dev_err_probe
   - Check indentation issue
   - Use raw_spinlock_t
Changes for v7:
   - Remove unused variable
   - Remove return in bank_reg function
   - Fix warning for const issue
---
 drivers/gpio/Kconfig           |   7 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-npcm-sgpio.c | 635 +++++++++++++++++++++++++++++++++
 3 files changed, 643 insertions(+)
 create mode 100644 drivers/gpio/gpio-npcm-sgpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b3a133ed31ee..efbdc93819d4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -478,6 +478,13 @@ config GPIO_MXS
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
 
+config GPIO_NPCM_SGPIO
+	bool "Nuvoton SGPIO support"
+	depends on ARCH_NPCM || COMPILE_TEST
+	select GPIOLIB_IRQCHIP
+	help
+	  Say Y here to support Nuvoton NPCM7XX/NPCM8XX SGPIO functionality.
+
 config GPIO_OCTEON
 	tristate "Cavium OCTEON GPIO"
 	depends on CAVIUM_OCTEON_SOC
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index eb73b5d633eb..373aa2943de5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -116,6 +116,7 @@ obj-$(CONFIG_GPIO_MT7621)		+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)		+= gpio-mvebu.o
 obj-$(CONFIG_GPIO_MXC)			+= gpio-mxc.o
 obj-$(CONFIG_GPIO_MXS)			+= gpio-mxs.o
+obj-$(CONFIG_GPIO_NPCM_SGPIO)		+= gpio-npcm-sgpio.o
 obj-$(CONFIG_GPIO_OCTEON)		+= gpio-octeon.o
 obj-$(CONFIG_GPIO_OMAP)			+= gpio-omap.o
 obj-$(CONFIG_GPIO_PALMAS)		+= gpio-palmas.o
diff --git a/drivers/gpio/gpio-npcm-sgpio.c b/drivers/gpio/gpio-npcm-sgpio.c
new file mode 100644
index 000000000000..1fb9750cb70e
--- /dev/null
+++ b/drivers/gpio/gpio-npcm-sgpio.c
@@ -0,0 +1,635 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NPCM Serial GPIO Driver
+ *
+ * Copyright (C) 2021 Nuvoton Technologies
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#define MAX_NR_HW_SGPIO		64
+
+#define  NPCM_IOXCFG1		0x2A
+#define  NPCM_IOXCFG1_SFT_CLK	GENMASK(3, 0)
+#define  NPCM_IOXCFG1_SCLK_POL	BIT(4)
+#define  NPCM_IOXCFG1_LDSH_POL	BIT(5)
+
+#define  NPCM_IOXCTS			0x28
+#define  NPCM_IOXCTS_IOXIF_EN		BIT(7)
+#define  NPCM_IOXCTS_RD_MODE		GENMASK(2, 1)
+#define  NPCM_IOXCTS_RD_MODE_PERIODIC	BIT(2)
+
+#define  NPCM_IOXCFG2		0x2B
+#define  NPCM_IOXCFG2_PORT	GENMASK(3, 0)
+
+#define  NPCM_IXOEVCFG_MASK	GENMASK(1, 0)
+#define  NPCM_IXOEVCFG_FALLING	BIT(1)
+#define  NPCM_IXOEVCFG_RISING	BIT(0)
+#define  NPCM_IXOEVCFG_BOTH	(NPCM_IXOEVCFG_FALLING | NPCM_IXOEVCFG_RISING)
+
+#define NPCM_CLK_MHZ	8000000
+
+#define GPIO_BANK(x)    ((x) / 8)
+#define GPIO_BIT(x)     ((x) % 8)
+
+/*
+ * Select the frequency of shift clock.
+ * The shift clock is a division of the APB clock.
+ */
+struct npcm_clk_cfg {
+	unsigned int	*sft_clk;
+	unsigned int	*clk_sel;
+	unsigned int	cfg_opt;
+};
+
+struct npcm_sgpio {
+	struct gpio_chip chip;
+	struct clk *pclk;
+	struct irq_chip intc;
+	raw_spinlock_t lock;	/*protect event config*/
+	void __iomem *base;
+	int irq;
+	u8 nin_sgpio;
+	u8 nout_sgpio;
+	u8 in_port;
+	u8 out_port;
+	u8 int_type[MAX_NR_HW_SGPIO];
+};
+
+struct npcm_sgpio_bank {
+	u8 rdata_reg;
+	u8 wdata_reg;
+	u8 event_config;
+	u8 event_status;
+};
+
+enum npcm_sgpio_reg {
+	READ_DATA,
+	WRITE_DATA,
+	EVENT_CFG,
+	EVENT_STS,
+};
+
+static const struct npcm_sgpio_bank npcm_sgpio_banks[] = {
+	{
+		.wdata_reg = 0x00,
+		.rdata_reg = 0x08,
+		.event_config = 0x10,
+		.event_status = 0x20,
+	},
+	{
+		.wdata_reg = 0x01,
+		.rdata_reg = 0x09,
+		.event_config = 0x12,
+		.event_status = 0x21,
+	},
+	{
+		.wdata_reg = 0x02,
+		.rdata_reg = 0x0a,
+		.event_config = 0x14,
+		.event_status = 0x22,
+	},
+	{
+		.wdata_reg = 0x03,
+		.rdata_reg = 0x0b,
+		.event_config = 0x16,
+		.event_status = 0x23,
+	},
+	{
+		.wdata_reg = 0x04,
+		.rdata_reg = 0x0c,
+		.event_config = 0x18,
+		.event_status = 0x24,
+	},
+	{
+		.wdata_reg = 0x05,
+		.rdata_reg = 0x0d,
+		.event_config = 0x1a,
+		.event_status = 0x25,
+	},
+	{
+		.wdata_reg = 0x06,
+		.rdata_reg = 0x0e,
+		.event_config = 0x1c,
+		.event_status = 0x26,
+	},
+	{
+		.wdata_reg = 0x07,
+		.rdata_reg = 0x0f,
+		.event_config = 0x1e,
+		.event_status = 0x27,
+	},
+
+};
+
+static void __iomem *bank_reg(struct npcm_sgpio *gpio,
+			      const struct npcm_sgpio_bank *bank,
+			      const enum npcm_sgpio_reg reg)
+{
+	switch (reg) {
+	case READ_DATA:
+		return gpio->base + bank->rdata_reg;
+	case WRITE_DATA:
+		return gpio->base + bank->wdata_reg;
+	case EVENT_CFG:
+		return gpio->base + bank->event_config;
+	case EVENT_STS:
+		return gpio->base + bank->event_status;
+	default:
+		/* actually if code runs to here, it's an error case */
+		dev_WARN(gpio->chip.parent, "Getting here is an error condition");
+	}
+	return 0;
+}
+
+static const struct npcm_sgpio_bank *offset_to_bank(unsigned int offset)
+{
+	unsigned int bank = GPIO_BANK(offset);
+
+	return &npcm_sgpio_banks[bank];
+}
+
+static void irqd_to_npcm_sgpio_data(struct irq_data *d,
+				    struct npcm_sgpio **gpio,
+				    const struct npcm_sgpio_bank **bank,
+				    u8 *bit, unsigned int *offset)
+{
+	struct npcm_sgpio *internal;
+
+	*offset = irqd_to_hwirq(d);
+	internal = irq_data_get_irq_chip_data(d);
+
+	*gpio = internal;
+	*offset -= internal->nout_sgpio;
+	*bank = offset_to_bank(*offset);
+	*bit = GPIO_BIT(*offset);
+}
+
+static int npcm_sgpio_init_port(struct npcm_sgpio *gpio)
+{
+	u8 in_port, out_port, set_port, reg;
+
+	in_port = GPIO_BANK(gpio->nin_sgpio);
+	if (GPIO_BIT(gpio->nin_sgpio) > 0)
+		in_port += 1;
+
+	out_port = GPIO_BANK(gpio->nout_sgpio);
+	if (GPIO_BIT(gpio->nout_sgpio) > 0)
+		out_port += 1;
+
+	gpio->in_port = in_port;
+	gpio->out_port = out_port;
+	set_port = ((out_port & NPCM_IOXCFG2_PORT) << 4) | (in_port & NPCM_IOXCFG2_PORT);
+	iowrite8(set_port, gpio->base + NPCM_IOXCFG2);
+
+	reg = ioread8(gpio->base + NPCM_IOXCFG2);
+
+	return reg == set_port ? 0 : -EINVAL;
+
+}
+
+static int npcm_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+
+	return offset <	gpio->nout_sgpio ? -EINVAL : 0;
+
+}
+
+static int npcm_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	gc->set(gc, offset, val);
+
+	return 0;
+
+}
+
+static int npcm_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+
+	if (offset > gpio->chip.ngpio)
+		return -EINVAL;
+
+	if (offset < gpio->nout_sgpio)
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
+static void npcm_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+	const struct  npcm_sgpio_bank *bank = offset_to_bank(offset);
+	void __iomem *addr;
+	u8 reg = 0;
+
+	addr = bank_reg(gpio, bank, WRITE_DATA);
+	reg = ioread8(addr);
+
+	if (val)
+		reg |= (val << GPIO_BIT(offset));
+	else
+		reg &= ~(1 << GPIO_BIT(offset));
+
+	iowrite8(reg, addr);
+}
+
+static int npcm_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+	const struct  npcm_sgpio_bank *bank;
+	void __iomem *addr;
+	u8 reg;
+	int dir;
+
+	dir = npcm_sgpio_get_direction(gc, offset);
+	if (dir == 0) {
+		bank = offset_to_bank(offset);
+
+		addr = bank_reg(gpio, bank, WRITE_DATA);
+		reg = ioread8(addr);
+		reg = !!(reg & GPIO_BIT(offset));
+	} else {
+		offset -= gpio->nout_sgpio;
+		bank = offset_to_bank(offset);
+
+		addr = bank_reg(gpio, bank, READ_DATA);
+		reg = ioread8(addr);
+		reg = !!(reg & GPIO_BIT(offset));
+	}
+
+	return reg;
+}
+
+static void npcm_sgpio_setup_enable(struct npcm_sgpio *gpio, bool enable)
+{
+	u8 reg = 0;
+
+	reg = ioread8(gpio->base + NPCM_IOXCTS);
+	reg = reg & ~NPCM_IOXCTS_RD_MODE;
+	reg = reg | NPCM_IOXCTS_RD_MODE_PERIODIC;
+
+	if (enable) {
+		reg |= NPCM_IOXCTS_IOXIF_EN;
+		iowrite8(reg, gpio->base + NPCM_IOXCTS);
+	} else {
+		reg &= ~NPCM_IOXCTS_IOXIF_EN;
+		iowrite8(reg, gpio->base + NPCM_IOXCTS);
+	}
+}
+
+static int npcm_sgpio_setup_clk(struct npcm_sgpio *gpio,
+				const struct npcm_clk_cfg *clk_cfg)
+{
+	unsigned long apb_freq;
+	u32 val;
+	u8 tmp;
+	int i;
+
+	apb_freq = clk_get_rate(gpio->pclk);
+	tmp = ioread8(gpio->base + NPCM_IOXCFG1) & ~NPCM_IOXCFG1_SFT_CLK;
+
+	for (i = 0; i < clk_cfg->cfg_opt; i++) {
+		val = apb_freq / clk_cfg->sft_clk[i];
+		if ((NPCM_CLK_MHZ < val) && (i != 0) ) {
+			iowrite8(clk_cfg->clk_sel[i-1] | tmp, gpio->base + NPCM_IOXCFG1);
+			return 0;
+		} else if (i == (clk_cfg->cfg_opt-1) && (NPCM_CLK_MHZ > val)) {
+			iowrite8(clk_cfg->clk_sel[i] | tmp, gpio->base + NPCM_IOXCFG1);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static void npcm_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
+					   unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+	int n = gpio->nin_sgpio;
+
+	/* input GPIOs in the high range */
+	bitmap_set(valid_mask, gpio->nout_sgpio, n);
+	bitmap_clear(valid_mask, 0, gpio->nout_sgpio);
+}
+
+static void npcm_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+	const struct npcm_sgpio_bank *bank;
+	struct npcm_sgpio *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	unsigned int offset;
+	u16 reg, type;
+	u8 bit;
+
+	irqd_to_npcm_sgpio_data(d, &gpio, &bank, &bit, &offset);
+	addr = bank_reg(gpio, bank, EVENT_CFG);
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	npcm_sgpio_setup_enable(gpio, false);
+
+	reg = ioread16(addr);
+	if (set) {
+		reg &= ~(NPCM_IXOEVCFG_MASK << (bit * 2));
+	} else {
+		type = gpio->int_type[offset];
+		reg |= (type << (bit * 2));
+	}
+
+	iowrite16(reg, addr);
+
+	npcm_sgpio_setup_enable(gpio, true);
+
+	addr = bank_reg(gpio, bank, EVENT_STS);
+	reg = ioread8(addr);
+	reg |= BIT(bit);
+	iowrite8(reg, addr);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void npcm_sgpio_irq_ack(struct irq_data *d)
+{
+	const struct npcm_sgpio_bank *bank;
+	struct npcm_sgpio *gpio;
+	unsigned long flags;
+	void __iomem *status_addr;
+	unsigned int offset;
+	u8 bit;
+
+	irqd_to_npcm_sgpio_data(d, &gpio, &bank, &bit, &offset);
+	status_addr = bank_reg(gpio, bank, EVENT_STS);
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+	iowrite8(BIT(bit), status_addr);
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void npcm_sgpio_irq_mask(struct irq_data *d)
+{
+	npcm_sgpio_irq_set_mask(d, true);
+}
+
+static void npcm_sgpio_irq_unmask(struct irq_data *d)
+{
+	npcm_sgpio_irq_set_mask(d, false);
+}
+
+static int npcm_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+	const struct npcm_sgpio_bank *bank;
+	irq_flow_handler_t handler;
+	struct npcm_sgpio *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	unsigned int offset;
+	u16 reg, val;
+	u8 bit;
+
+	irqd_to_npcm_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		val = NPCM_IXOEVCFG_BOTH;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		val = NPCM_IXOEVCFG_RISING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		val = NPCM_IXOEVCFG_FALLING;
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		val = NPCM_IXOEVCFG_RISING;
+		handler = handle_level_irq;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		val = NPCM_IXOEVCFG_FALLING;
+		handler = handle_level_irq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	gpio->int_type[offset] = val;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+	npcm_sgpio_setup_enable(gpio, false);
+	addr = bank_reg(gpio, bank, EVENT_CFG);
+	reg = ioread16(addr);
+
+	reg |= (val << (bit * 2));
+
+	iowrite16(reg, addr);
+	npcm_sgpio_setup_enable(gpio, true);
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	irq_set_handler_locked(d, handler);
+
+	return 0;
+}
+
+static void npcm_sgpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct irq_chip *ic = irq_desc_get_chip(desc);
+	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
+	unsigned int i, j, girq;
+	unsigned long reg;
+
+	chained_irq_enter(ic, desc);
+
+	for (i = 0; i < ARRAY_SIZE(npcm_sgpio_banks); i++) {
+		const struct npcm_sgpio_bank *bank = &npcm_sgpio_banks[i];
+
+		reg = ioread8(bank_reg(gpio, bank, EVENT_STS));
+		for_each_set_bit(j, &reg, 8) {
+			girq = irq_find_mapping(gc->irq.domain, i * 8 + gpio->nout_sgpio + j);
+			generic_handle_irq(girq);
+		}
+	}
+
+	chained_irq_exit(ic, desc);
+}
+
+static const struct irq_chip sgpio_irq_chip = {
+	.name = "sgpio-irq",
+	.irq_ack = npcm_sgpio_irq_ack,
+	.irq_mask = npcm_sgpio_irq_mask,
+	.irq_unmask = npcm_sgpio_irq_unmask,
+	.irq_set_type = npcm_sgpio_set_type,
+	.flags	= IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int npcm_sgpio_setup_irqs(struct npcm_sgpio *gpio,
+				 struct platform_device *pdev)
+{
+	int rc, i;
+	struct gpio_irq_chip *irq;
+
+	rc = platform_get_irq(pdev, 0);
+	if (rc < 0)
+		return rc;
+
+	gpio->irq = rc;
+
+	npcm_sgpio_setup_enable(gpio, false);
+
+	/* Disable IRQ and clear Interrupt status registers for all SGPIO Pins. */
+	for (i = 0; i < ARRAY_SIZE(npcm_sgpio_banks); i++) {
+		const struct npcm_sgpio_bank *bank = &npcm_sgpio_banks[i];
+
+		iowrite16(0x0000, bank_reg(gpio, bank, EVENT_CFG));
+		iowrite8(0xff, bank_reg(gpio, bank, EVENT_STS));
+	}
+
+	irq = &gpio->chip.irq;
+	gpio_irq_chip_set_chip(irq, &sgpio_irq_chip);
+	irq->init_valid_mask = npcm_sgpio_irq_init_valid_mask;
+	irq->handler = handle_bad_irq;
+	irq->default_type = IRQ_TYPE_NONE;
+	irq->parent_handler = npcm_sgpio_irq_handler;
+	irq->parent_handler_data = gpio;
+	irq->parents = &gpio->irq;
+	irq->num_parents = 1;
+
+	return 0;
+}
+
+static unsigned int npcm750_SFT_CLK[] = {
+	1024, 32, 8, 4, 3, 2,
+};
+
+static unsigned int npcm750_CLK_SEL[] = {
+	0x00, 0x05, 0x07, 0x0C, 0x0D, 0x0E,
+};
+
+static unsigned int npcm845_SFT_CLK[] = {
+	1024, 32, 16, 8, 4,
+};
+
+static unsigned int npcm845_CLK_SEL[] = {
+	0x00, 0x05, 0x06, 0x07, 0x0C,
+};
+
+static struct npcm_clk_cfg npcm750_sgpio_pdata = {
+	.sft_clk = npcm750_SFT_CLK,
+	.clk_sel = npcm750_CLK_SEL,
+	.cfg_opt = 6,
+};
+
+static const struct npcm_clk_cfg npcm845_sgpio_pdata = {
+	.sft_clk = npcm845_SFT_CLK,
+	.clk_sel = npcm845_CLK_SEL,
+	.cfg_opt = 5,
+};
+
+static const struct of_device_id npcm_sgpio_of_table[] = {
+	{ .compatible = "nuvoton,npcm750-sgpio", .data = &npcm750_sgpio_pdata, },
+	{ .compatible = "nuvoton,npcm845-sgpio", .data = &npcm845_sgpio_pdata, },
+	{}
+};
+MODULE_DEVICE_TABLE(of, npcm_sgpio_of_table);
+
+static int npcm_sgpio_probe(struct platform_device *pdev)
+{
+	struct npcm_sgpio *gpio;
+	const struct npcm_clk_cfg *clk_cfg;
+	int rc;
+	u32 nin_gpios, nout_gpios;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gpio->base))
+		return PTR_ERR(gpio->base);
+
+	clk_cfg = device_get_match_data(&pdev->dev);
+	if (!clk_cfg)
+		return -EINVAL;
+
+	rc = device_property_read_u32(&pdev->dev, "nuvoton,input-ngpios", &nin_gpios);
+	if (rc < 0)
+		return dev_err_probe(&pdev->dev, rc, "Could not read ngpios property\n");
+
+	rc = device_property_read_u32(&pdev->dev, "nuvoton,output-ngpios", &nout_gpios);
+	if (rc < 0)
+		return dev_err_probe(&pdev->dev, rc, "Could not read ngpios property\n");
+
+	gpio->nin_sgpio = nin_gpios;
+	gpio->nout_sgpio = nout_gpios;
+	if (gpio->nin_sgpio > MAX_NR_HW_SGPIO || gpio->nout_sgpio > MAX_NR_HW_SGPIO) {
+		dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: input: %d output: %d\n",
+			MAX_NR_HW_SGPIO, nin_gpios, nout_gpios);
+		return -EINVAL;
+	}
+
+	gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(gpio->pclk)) {
+		dev_err(&pdev->dev, "Could not get pclk\n");
+		return PTR_ERR(gpio->pclk);
+	}
+
+	rc = npcm_sgpio_setup_clk(gpio, clk_cfg);
+	if (rc < 0)
+		return dev_err_probe(&pdev->dev, rc, "Failed to setup clock\n");
+
+	raw_spin_lock_init(&gpio->lock);
+	gpio->chip.parent = &pdev->dev;
+	gpio->chip.ngpio = gpio->nin_sgpio + gpio->nout_sgpio;
+	gpio->chip.direction_input = npcm_sgpio_dir_in;
+	gpio->chip.direction_output = npcm_sgpio_dir_out;
+	gpio->chip.get_direction = npcm_sgpio_get_direction;
+	gpio->chip.get = npcm_sgpio_get;
+	gpio->chip.set = npcm_sgpio_set;
+	gpio->chip.label = dev_name(&pdev->dev);
+	gpio->chip.base = -1;
+
+	rc = npcm_sgpio_init_port(gpio);
+	if (rc < 0)
+		return rc;
+
+	rc = npcm_sgpio_setup_irqs(gpio, pdev);
+	if (rc < 0)
+		return rc;
+
+	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	if (rc < 0)
+		return rc;
+
+	npcm_sgpio_setup_enable(gpio, true);
+
+	return 0;
+}
+
+static struct platform_driver npcm_sgpio_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = npcm_sgpio_of_table,
+	},
+	.probe	= npcm_sgpio_probe,
+};
+module_platform_driver(npcm_sgpio_driver);
+
+MODULE_AUTHOR("Jim Liu <jjliu0@nuvoton.com>");
+MODULE_AUTHOR("Joseph Liu <kwliu@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton NPCM Serial GPIO Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH v9 1/3] dt-bindings: gpio: add NPCM sgpio driver bindings

[Paul Menzel]

Dear Jim,


Thank you for your patch.

Am 12.12.23 um 07:51 schrieb Jim Liu:
> Add dt-bindings document for the Nuvoton NPCM7xx sgpio driver
> 
> Signed-off-by: Jim Liu <jim.t90615@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>

As you seem to be employed by Nuvoton, should your company/work email be 
listed somehow, and even be used for the author address?

> ---
> Changes for v9:
>     - no changed
> Changes for v8:
>     - no changed
> Changes for v7:
>     - no changed
> ---
>   .../bindings/gpio/nuvoton,sgpio.yaml          | 86 +++++++++++++++++++
>   1 file changed, 86 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> new file mode 100644
> index 000000000000..84e0dbcb066c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> @@ -0,0 +1,86 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/nuvoton,sgpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton SGPIO controller
> +
> +maintainers:
> +  - Jim LIU <JJLIU0@nuvoton.com>
> +
> +description: |
> +  This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC.
> +  Nuvoton NPCM7xx SGPIO module is combine serial to parallel IC (HC595)

s/is combine/combines a/

> +  and parallel to serial IC (HC165), and use APB3 clock to control it.

use*s*

> +  This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).

Only one space before the (.

> +  NPCM7xx/NPCM8xx have two sgpio module each module can support up

… modules. Each module …

> +  to 64 output pins,and up to 64 input pin, the pin is only for gpi or gpo.

1.  Space after the comma.
2.  64 input pin*s

> +  GPIO pins have sequential, First half is gpo and second half is gpi.

have sequential ?.

> +  GPIO pins can be programmed to support the following options
> +  - Support interrupt option for each input port and various interrupt
> +    sensitivity option (level-high, level-low, edge-high, edge-low)

option*s*

> +  - ngpios is number of nuvoton,input-ngpios GPIO lines and nuvoton,output-ngpios GPIO lines.
> +    nuvoton,input-ngpios GPIO lines is only for gpi.

s/is/are/

> +    nuvoton,output-ngpios GPIO lines is only for gpo.

s/is/are/

It’d be great if you mentioned the datasheet name and revision in the 
description.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,npcm750-sgpio
> +      - nuvoton,npcm845-sgpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  nuvoton,input-ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The numbers of GPIO's exposed. GPIO lines is only for gpi.

s/is/are/

> +    minimum: 0
> +    maximum: 64
> +
> +  nuvoton,output-ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The numbers of GPIO's exposed. GPIO lines is only for gpo.

s/is/are/

> +    minimum: 0
> +    maximum: 64
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +  - interrupts
> +  - nuvoton,input-ngpios
> +  - nuvoton,output-ngpios
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    gpio8: gpio@101000 {
> +        compatible = "nuvoton,npcm750-sgpio";
> +        reg = <0x101000 0x200>;
> +        clocks = <&clk NPCM7XX_CLK_APB3>;
> +        interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        nuvoton,input-ngpios = <64>;
> +        nuvoton,output-ngpios = <64>;
> +    };

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul Menzel

Re: [PATCH v9 1/3] dt-bindings: gpio: add NPCM sgpio driver bindings

[Jim Liu]

Hi Paul

Thanks for your review.
I will modify it in the next version.

Best regards,
Jim

On Tue, Dec 12, 2023 at 3:00 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Jim,
>
>
> Thank you for your patch.
>
> Am 12.12.23 um 07:51 schrieb Jim Liu:
> > Add dt-bindings document for the Nuvoton NPCM7xx sgpio driver
> >
> > Signed-off-by: Jim Liu <jim.t90615@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> As you seem to be employed by Nuvoton, should your company/work email be
> listed somehow, and even be used for the author address?
>
> > ---
> > Changes for v9:
> >     - no changed
> > Changes for v8:
> >     - no changed
> > Changes for v7:
> >     - no changed
> > ---
> >   .../bindings/gpio/nuvoton,sgpio.yaml          | 86 +++++++++++++++++++
> >   1 file changed, 86 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> > new file mode 100644
> > index 000000000000..84e0dbcb066c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/nuvoton,sgpio.yaml
> > @@ -0,0 +1,86 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/nuvoton,sgpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Nuvoton SGPIO controller
> > +
> > +maintainers:
> > +  - Jim LIU <JJLIU0@nuvoton.com>
> > +
> > +description: |
> > +  This SGPIO controller is for NUVOTON NPCM7xx and NPCM8xx SoC.
> > +  Nuvoton NPCM7xx SGPIO module is combine serial to parallel IC (HC595)
>
> s/is combine/combines a/
>
> > +  and parallel to serial IC (HC165), and use APB3 clock to control it.
>
> use*s*
>
> > +  This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
>
> Only one space before the (.
>
> > +  NPCM7xx/NPCM8xx have two sgpio module each module can support up
>
> … modules. Each module …
>
> > +  to 64 output pins,and up to 64 input pin, the pin is only for gpi or gpo.
>
> 1.  Space after the comma.
> 2.  64 input pin*s
>
> > +  GPIO pins have sequential, First half is gpo and second half is gpi.
>
> have sequential ?.
>
> > +  GPIO pins can be programmed to support the following options
> > +  - Support interrupt option for each input port and various interrupt
> > +    sensitivity option (level-high, level-low, edge-high, edge-low)
>
> option*s*
>
> > +  - ngpios is number of nuvoton,input-ngpios GPIO lines and nuvoton,output-ngpios GPIO lines.
> > +    nuvoton,input-ngpios GPIO lines is only for gpi.
>
> s/is/are/
>
> > +    nuvoton,output-ngpios GPIO lines is only for gpo.
>
> s/is/are/
>
> It’d be great if you mentioned the datasheet name and revision in the
> description.
>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nuvoton,npcm750-sgpio
> > +      - nuvoton,npcm845-sgpio
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  nuvoton,input-ngpios:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      The numbers of GPIO's exposed. GPIO lines is only for gpi.
>
> s/is/are/
>
> > +    minimum: 0
> > +    maximum: 64
> > +
> > +  nuvoton,output-ngpios:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      The numbers of GPIO's exposed. GPIO lines is only for gpo.
>
> s/is/are/
>
> > +    minimum: 0
> > +    maximum: 64
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - gpio-controller
> > +  - '#gpio-cells'
> > +  - interrupts
> > +  - nuvoton,input-ngpios
> > +  - nuvoton,output-ngpios
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/nuvoton,npcm7xx-clock.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    gpio8: gpio@101000 {
> > +        compatible = "nuvoton,npcm750-sgpio";
> > +        reg = <0x101000 0x200>;
> > +        clocks = <&clk NPCM7XX_CLK_APB3>;
> > +        interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        nuvoton,input-ngpios = <64>;
> > +        nuvoton,output-ngpios = <64>;
> > +    };
>
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
>
> Kind regards,
>
> Paul Menzel

Re: [PATCH v9 3/3] gpio: nuvoton: Add Nuvoton NPCM sgpio driver

[Andy Shevchenko]

On Tue, Dec 12, 2023 at 02:51:47PM +0800, Jim Liu wrote:
> Add Nuvoton BMC NPCM7xx/NPCM8xx sgpio driver support.
> Nuvoton NPCM SGPIO module is combine serial to parallel IC (HC595)
> and parallel to serial IC (HC165), and use APB3 clock to control it.
> This interface has 4 pins  (D_out , D_in, S_CLK, LDSH).
> BMC can use this driver to increase 64 GPI pins and 64 GPO pins to use.

...

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hashtable.h>
> +#include <linux/init.h>
> +#include <linux/io.h>

> +#include <linux/kernel.h>

Is this a proxy to some other headers like array_size.h? Please use respective
headers directly.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>

...

> +#define NPCM_CLK_MHZ	8000000

HZ_PER_MHZ from units.h?

...

> +struct npcm_sgpio {
> +	struct gpio_chip chip;
> +	struct clk *pclk;
> +	struct irq_chip intc;
> +	raw_spinlock_t lock;	/*protect event config*/

Missing spaces.

> +	void __iomem *base;
> +	int irq;
> +	u8 nin_sgpio;
> +	u8 nout_sgpio;
> +	u8 in_port;
> +	u8 out_port;
> +	u8 int_type[MAX_NR_HW_SGPIO];
> +};

...

> +	{
> +		.wdata_reg = 0x07,
> +		.rdata_reg = 0x0f,
> +		.event_config = 0x1e,
> +		.event_status = 0x27,
> +	},
> +

Redundant blank line.

...

> +static void __iomem *bank_reg(struct npcm_sgpio *gpio,
> +			      const struct npcm_sgpio_bank *bank,
> +			      const enum npcm_sgpio_reg reg)
> +{
> +	switch (reg) {
> +	case READ_DATA:
> +		return gpio->base + bank->rdata_reg;
> +	case WRITE_DATA:
> +		return gpio->base + bank->wdata_reg;
> +	case EVENT_CFG:
> +		return gpio->base + bank->event_config;
> +	case EVENT_STS:
> +		return gpio->base + bank->event_status;
> +	default:
> +		/* actually if code runs to here, it's an error case */
> +		dev_WARN(gpio->chip.parent, "Getting here is an error condition");

...then return an error here.

> +	}
> +	return 0;

See above.

> +}

> +static void irqd_to_npcm_sgpio_data(struct irq_data *d,

Respect namespace, here it's better to have npcm_sgpio_irqd_to_data().

> +				    struct npcm_sgpio **gpio,
> +				    const struct npcm_sgpio_bank **bank,
> +				    u8 *bit, unsigned int *offset)

...

> +static int npcm_sgpio_init_port(struct npcm_sgpio *gpio)
> +{
> +	u8 in_port, out_port, set_port, reg;
> +
> +	in_port = GPIO_BANK(gpio->nin_sgpio);
> +	if (GPIO_BIT(gpio->nin_sgpio) > 0)
> +		in_port += 1;

This is strange... So, you are telling that offsets start from 1 and not 0?

> +	out_port = GPIO_BANK(gpio->nout_sgpio);
> +	if (GPIO_BIT(gpio->nout_sgpio) > 0)
> +		out_port += 1;

Ditto.

...

> +	set_port = ((out_port & NPCM_IOXCFG2_PORT) << 4) | (in_port & NPCM_IOXCFG2_PORT);

Outer parentheses are redundant.

...

> +static int npcm_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	gc->set(gc, offset, val);
> +
> +	return 0;

> +

Redundant blank line.

> +}
> +
> +static int npcm_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct npcm_sgpio *gpio = gpiochip_get_data(gc);

> +	if (offset > gpio->chip.ngpio)
> +		return -EINVAL;

Why do you need this check?

> +	if (offset < gpio->nout_sgpio)
> +		return GPIO_LINE_DIRECTION_OUT;
> +
> +	return GPIO_LINE_DIRECTION_IN;
> +}

...

> +static void npcm_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct npcm_sgpio *gpio = gpiochip_get_data(gc);
> +	const struct  npcm_sgpio_bank *bank = offset_to_bank(offset);
> +	void __iomem *addr;
> +	u8 reg = 0;
> +
> +	addr = bank_reg(gpio, bank, WRITE_DATA);
> +	reg = ioread8(addr);
> +
> +	if (val)
> +		reg |= (val << GPIO_BIT(offset));

...and if val is not 1?..

> +	else
> +		reg &= ~(1 << GPIO_BIT(offset));

In both cases use BIT().

> +	iowrite8(reg, addr);
> +}

...

> +	dir = npcm_sgpio_get_direction(gc, offset);
> +	if (dir == 0) {
> +		bank = offset_to_bank(offset);
> +
> +		addr = bank_reg(gpio, bank, WRITE_DATA);
> +		reg = ioread8(addr);
> +		reg = !!(reg & GPIO_BIT(offset));
> +	} else {
> +		offset -= gpio->nout_sgpio;
> +		bank = offset_to_bank(offset);
> +
> +		addr = bank_reg(gpio, bank, READ_DATA);
> +		reg = ioread8(addr);
> +		reg = !!(reg & GPIO_BIT(offset));
> +	}

Instead of conditional(s) use arithmetics. You can get all these directly
from the properly formed calculations.

...

> +static void npcm_sgpio_setup_enable(struct npcm_sgpio *gpio, bool enable)
> +{

> +	u8 reg = 0;
Redundant assignment.

> +	reg = ioread8(gpio->base + NPCM_IOXCTS);

> +	reg = reg & ~NPCM_IOXCTS_RD_MODE;
> +	reg = reg | NPCM_IOXCTS_RD_MODE_PERIODIC;

Combine them.

> +	if (enable) {
> +		reg |= NPCM_IOXCTS_IOXIF_EN;
> +		iowrite8(reg, gpio->base + NPCM_IOXCTS);
> +	} else {
> +		reg &= ~NPCM_IOXCTS_IOXIF_EN;
> +		iowrite8(reg, gpio->base + NPCM_IOXCTS);
> +	}
> +}

...

> +static int npcm_sgpio_setup_clk(struct npcm_sgpio *gpio,
> +				const struct npcm_clk_cfg *clk_cfg)
> +{
> +	unsigned long apb_freq;
> +	u32 val;
> +	u8 tmp;
> +	int i;
> +
> +	apb_freq = clk_get_rate(gpio->pclk);
> +	tmp = ioread8(gpio->base + NPCM_IOXCFG1) & ~NPCM_IOXCFG1_SFT_CLK;
> +
> +	for (i = 0; i < clk_cfg->cfg_opt; i++) {
> +		val = apb_freq / clk_cfg->sft_clk[i];
> +		if ((NPCM_CLK_MHZ < val) && (i != 0) ) {
> +			iowrite8(clk_cfg->clk_sel[i-1] | tmp, gpio->base + NPCM_IOXCFG1);
> +			return 0;
> +		} else if (i == (clk_cfg->cfg_opt-1) && (NPCM_CLK_MHZ > val)) {
> +			iowrite8(clk_cfg->clk_sel[i] | tmp, gpio->base + NPCM_IOXCFG1);
> +			return 0;
> +		}

These i == / i != checks probably due to wrong operator be chosen. Consider using
while-loop, or do-while. I believe it will make code better.

> +	}
> +
> +	return -EINVAL;
> +}

...

> +static void npcm_sgpio_irq_init_valid_mask(struct gpio_chip *gc,
> +					   unsigned long *valid_mask, unsigned int ngpios)
> +{
> +	struct npcm_sgpio *gpio = gpiochip_get_data(gc);

> +	int n = gpio->nin_sgpio;

Why do you need this variable, what for?

> +	/* input GPIOs in the high range */
> +	bitmap_set(valid_mask, gpio->nout_sgpio, n);
> +	bitmap_clear(valid_mask, 0, gpio->nout_sgpio);
> +}

...

> +	raw_spin_lock_irqsave(&gpio->lock, flags);

Don't you need to use spin lock in the other APIs? It seem whole driver works by luck.

> +	npcm_sgpio_setup_enable(gpio, false);
> +
> +	reg = ioread16(addr);
> +	if (set) {
> +		reg &= ~(NPCM_IXOEVCFG_MASK << (bit * 2));
> +	} else {
> +		type = gpio->int_type[offset];
> +		reg |= (type << (bit * 2));

At least the calculations can be done outside of the lock.

> +	}
> +
> +	iowrite16(reg, addr);
> +
> +	npcm_sgpio_setup_enable(gpio, true);
> +
> +	addr = bank_reg(gpio, bank, EVENT_STS);
> +	reg = ioread8(addr);
> +	reg |= BIT(bit);
> +	iowrite8(reg, addr);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);

...

> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +		val = NPCM_IXOEVCFG_BOTH;
> +		handler = handle_edge_irq;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		val = NPCM_IXOEVCFG_RISING;
> +		handler = handle_edge_irq;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		val = NPCM_IXOEVCFG_FALLING;
> +		handler = handle_edge_irq;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		val = NPCM_IXOEVCFG_RISING;
> +		handler = handle_level_irq;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		val = NPCM_IXOEVCFG_FALLING;
> +		handler = handle_level_irq;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

You can split the handler setup and make this function less by 5 LoCs or so.

See, for example, gpio-tangier.c.

...

> +	for (i = 0; i < ARRAY_SIZE(npcm_sgpio_banks); i++) {
> +		const struct npcm_sgpio_bank *bank = &npcm_sgpio_banks[i];
> +
> +		reg = ioread8(bank_reg(gpio, bank, EVENT_STS));
> +		for_each_set_bit(j, &reg, 8) {
> +			girq = irq_find_mapping(gc->irq.domain, i * 8 + gpio->nout_sgpio + j);
> +			generic_handle_irq(girq);

generic_handle_domain_irq()

> +		}
> +	}

...

> +static int npcm_sgpio_setup_irqs(struct npcm_sgpio *gpio,
> +				 struct platform_device *pdev)
> +{
> +	int rc, i;
> +	struct gpio_irq_chip *irq;
> +
> +	rc = platform_get_irq(pdev, 0);
> +	if (rc < 0)
> +		return rc;
> +
> +	gpio->irq = rc;
> +
> +	npcm_sgpio_setup_enable(gpio, false);
> +
> +	/* Disable IRQ and clear Interrupt status registers for all SGPIO Pins. */
> +	for (i = 0; i < ARRAY_SIZE(npcm_sgpio_banks); i++) {
> +		const struct npcm_sgpio_bank *bank = &npcm_sgpio_banks[i];

> +		iowrite16(0x0000, bank_reg(gpio, bank, EVENT_CFG));

0 is enough.

> +		iowrite8(0xff, bank_reg(gpio, bank, EVENT_STS));

GENMASK() ?

> +	}
> +
> +	irq = &gpio->chip.irq;
> +	gpio_irq_chip_set_chip(irq, &sgpio_irq_chip);
> +	irq->init_valid_mask = npcm_sgpio_irq_init_valid_mask;
> +	irq->handler = handle_bad_irq;
> +	irq->default_type = IRQ_TYPE_NONE;
> +	irq->parent_handler = npcm_sgpio_irq_handler;
> +	irq->parent_handler_data = gpio;
> +	irq->parents = &gpio->irq;
> +	irq->num_parents = 1;
> +
> +	return 0;
> +}

...

> +static struct npcm_clk_cfg npcm750_sgpio_pdata = {
> +	.sft_clk = npcm750_SFT_CLK,
> +	.clk_sel = npcm750_CLK_SEL,
> +	.cfg_opt = 6,

Define this magic and use it in the above arrays as the capacity.

> +};
> +
> +static const struct npcm_clk_cfg npcm845_sgpio_pdata = {
> +	.sft_clk = npcm845_SFT_CLK,
> +	.clk_sel = npcm845_CLK_SEL,

> +	.cfg_opt = 5,

Ditto.

> +};

...

> +static const struct of_device_id npcm_sgpio_of_table[] = {
> +	{ .compatible = "nuvoton,npcm750-sgpio", .data = &npcm750_sgpio_pdata, },
> +	{ .compatible = "nuvoton,npcm845-sgpio", .data = &npcm845_sgpio_pdata, },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, npcm_sgpio_of_table);

Move this closer to its user below.

...

> +	if (gpio->nin_sgpio > MAX_NR_HW_SGPIO || gpio->nout_sgpio > MAX_NR_HW_SGPIO) {
> +		dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: input: %d output: %d\n",
> +			MAX_NR_HW_SGPIO, nin_gpios, nout_gpios);
> +		return -EINVAL;

		return dev_err_probe(...);

> +	}
> +
> +	gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(gpio->pclk)) {
> +		dev_err(&pdev->dev, "Could not get pclk\n");
> +		return PTR_ERR(gpio->pclk);

Ditto.

> +	}

...

> +	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> +	if (rc < 0)

Here and in the other cases, why ' < 0'? Does it have positive value
to be returned in some cases?

> +		return rc;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 3/3] gpio: nuvoton: Add Nuvoton NPCM sgpio driver

[Jim Liu]

Hi Andy

Thanks for your comments.
I will modify it in the next version.

But some description as below



> > +static int npcm_sgpio_init_port(struct npcm_sgpio *gpio)
> > +{
> > +     u8 in_port, out_port, set_port, reg;
> > +
> > +     in_port = GPIO_BANK(gpio->nin_sgpio);
> > +     if (GPIO_BIT(gpio->nin_sgpio) > 0)
> > +             in_port += 1;
>
> This is strange... So, you are telling that offsets start from 1 and not 0?
>
> > +     out_port = GPIO_BANK(gpio->nout_sgpio);
> > +     if (GPIO_BIT(gpio->nout_sgpio) > 0)
> > +             out_port += 1;
>
> Ditto.
>
Yes,  if the customer has defined the in/out pins the offsets start from 1.
The NPCM_IOXCFG2_PORT register is the set number of in/out ports.
NPCM_IOXCFG2_PORT register define as below:
0~3 bit is the number of input ports
4~7 bit is the number of output ports
Each module can support 8 input ports and 8 output ports.
> ...
>
> > +     set_port = ((out_port & NPCM_IOXCFG2_PORT) << 4) | (in_port & NPCM_IOXCFG2_PORT);
>
> Outer parentheses are redundant.
>
> ...
>

Best regards,
Jim

Re: [PATCH v9 3/3] gpio: nuvoton: Add Nuvoton NPCM sgpio driver

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 02:27:13PM +0800, Jim Liu wrote:

...

> > > +     in_port = GPIO_BANK(gpio->nin_sgpio);
> > > +     if (GPIO_BIT(gpio->nin_sgpio) > 0)
> > > +             in_port += 1;
> >
> > This is strange... So, you are telling that offsets start from 1 and not 0?
> >
> > > +     out_port = GPIO_BANK(gpio->nout_sgpio);
> > > +     if (GPIO_BIT(gpio->nout_sgpio) > 0)
> > > +             out_port += 1;
> >
> > Ditto.
> >
> Yes,  if the customer has defined the in/out pins the offsets start from 1.

Why?

> The NPCM_IOXCFG2_PORT register is the set number of in/out ports.
> NPCM_IOXCFG2_PORT register define as below:
> 0~3 bit is the number of input ports
> 4~7 bit is the number of output ports
> Each module can support 8 input ports and 8 output ports.

Right, this doesn't answer why.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 3/3] gpio: nuvoton: Add Nuvoton NPCM sgpio driver

[Jim Liu]

Hi  Andy

Thanks for your reply.
The description as below


> > > +     in_port = GPIO_BANK(gpio->nin_sgpio);
> > > +     if (GPIO_BIT(gpio->nin_sgpio) > 0)
> > > +             in_port += 1;
> >
> > This is strange... So, you are telling that offsets start from 1 and not 0?
> >
> > > +     out_port = GPIO_BANK(gpio->nout_sgpio);
> > > +     if (GPIO_BIT(gpio->nout_sgpio) > 0)
> > > +             out_port += 1;
> >
> > Ditto.
> >
> Yes,  if the customer has defined the in/out pins the offsets start from 1.

>Why?

The NPCM_IOXCFG2_PORT  default setting is to enable 0 input port and 0
output port.
The register default value is 0x0. Each port can support 8 pins.
If the register value is 0x31 means enable 3 output ports and 1 input port.
If customer has define nuvoton,input-ngpios  or  nuvoton,output-ngpios
dts property

For example , nuvoton,output-ngpios = <9>
> > > +     out_port = GPIO_BANK(gpio->nout_sgpio);
The out_port value is 1 but one port only supports 8 pins.
> > > +     if (GPIO_BIT(gpio->nout_sgpio) > 0)
> > > +             out_port += 1;
This out_port value is 2, the driver will enable two port to support 9 pins.

Maybe it is my expression error , the out_port and in_port default value is 0.


> The NPCM_IOXCFG2_PORT register is the set number of in/out ports.
> NPCM_IOXCFG2_PORT register define as below:
> 0~3 bit is the number of input ports
> 4~7 bit is the number of output ports
> Each module can support 8 input ports and 8 output ports.

Best regards,
Jim