[PATCH v5 0/5] riscv: spacemit: add gpio support for K1 SoC

[PATCH v5 0/5] riscv: spacemit: add gpio support for K1 SoC

[Yixun Lan]

The gpio controller of K1 support basic GPIO functions,
which capable of enabling as input, output. It can also be used
as GPIO interrupt which able to detect rising edge, falling edge,
or both. There are four GPIO ports, each consisting of 32 pins and
has indepedent register sets, while still sharing IRQ line and clocks.

The GPIO controller request the clock source from APBC block,
In this series, I haven't added the clock support, but plan
to fix it after clock driver is merged.

Due to first three GPIO ports has interleave register settings, some
resources (IRQ, clock) are shared by all pins.

The GPIO docs of K1 SoC can be found here, chapter 16.4 GPIO [1]

Note, this patch is rebased to v6.14-rc1.

This patch series has been tested on Bananapi-F3 board,
with following GPIO cases passed:
 1) gpio input
 2) gpio output - set to high, low
 3) gpio interrupt - rising trigger, falling trigger, both edge trigger

This version should resolve DT related concern in V4, and register each bank as
indepedent gpio chip in driver, no more sub children gpio DT node needed.

Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf [1]
Link: https://lore.kernel.org/all/20240730-k1-01-basic-dt-v5-0-98263aae83be@gentoo.org [2]
Link: https://lore.kernel.org/all/20241016-02-k1-pinctrl-v5-0-03d395222e4f@gentoo.org/ [3]
Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Changes in v5:
- export add_pin_range() from gpio core, support to add custom version
- change to 3 gpio cells, model to <bank number>, <bank offset>, <gpio flag>
- fold children DT nodes into parent
- Link to v4: https://lore.kernel.org/r/20250121-03-k1-gpio-v4-0-4641c95c0194@gentoo.org

Changes in v4:
- gpio: re-construct gpio as four independent ports, also leverage gpio mmio API
- gpio interrupt: convert to generic gpio irqchip
- Link to v3: https://lore.kernel.org/r/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org

Changes in v3:
- dt: drop ranges, interrupt-names property
- Link to v2: https://lore.kernel.org/r/20241219-03-k1-gpio-v2-0-28444fd221cd@gentoo.org

Changes in v2:
- address dt-bindings comments, simplify example
- rebase to 6.13-rc3 
- Link to v1: https://lore.kernel.org/r/20240904-03-k1-gpio-v1-0-6072ebeecae0@gentoo.org

---
Yixun Lan (5):
      gpio: of: support to add custom add pin range function
      dt-bindings: gpio: spacemit: add support for K1 SoC
      gpio: spacemit: add support for K1 SoC
      riscv: dts: spacemit: add gpio support for K1 SoC
      riscv: dts: spacemit: add gpio LED for system heartbeat

 .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml |  81 +++++
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts    |  11 +
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi       |   3 +
 arch/riscv/boot/dts/spacemit/k1.dtsi               |  15 +
 drivers/gpio/Kconfig                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-spacemit-k1.c                    | 376 +++++++++++++++++++++
 drivers/gpio/gpiolib-of.c                          |   5 +-
 include/linux/gpio/driver.h                        |   7 +
 9 files changed, 506 insertions(+), 1 deletion(-)
---
base-commit: 3d72d603afa72082501e9076eed61e0531339ef8
change-id: 20240828-03-k1-gpio-61bf92f9032c

Best regards,
-- 
Yixun Lan

[PATCH v5 1/5] gpio: of: support to add custom add pin range function

[Yixun Lan]

Export custom function to add gpio pin range from pinctrl
subsystem. This would make it possible to add pins to multi
gpio chips.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/gpio/gpiolib-of.c   | 5 ++++-
 include/linux/gpio/driver.h | 7 +++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 2e537ee979f3e2b6e8d5f86f3e121a66f2a8e083..64c8a153b823d65faebed9c4cd87952359b42765 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -1170,7 +1170,10 @@ int of_gpiochip_add(struct gpio_chip *chip)
 	if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
 		return -EINVAL;
 
-	ret = of_gpiochip_add_pin_range(chip);
+	if (!chip->of_add_pin_range)
+		chip->of_add_pin_range = of_gpiochip_add_pin_range;
+
+	ret = chip->of_add_pin_range(chip);
 	if (ret)
 		return ret;
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270a68ddedbcdd5d44e0d0f88dfa785..a7b966c78a2f62075fb7804f6e96028564dda161 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -528,6 +528,13 @@ struct gpio_chip {
 	 */
 	int (*of_xlate)(struct gpio_chip *gc,
 			const struct of_phandle_args *gpiospec, u32 *flags);
+
+	/**
+	 * @of_add_pin_range:
+	 *
+	 * Callback to add pin ranges from pinctrl
+	 */
+	int (*of_add_pin_range)(struct gpio_chip *chip);
 #endif /* CONFIG_OF_GPIO */
 };
 

-- 
2.48.1

Re: [PATCH v5 1/5] gpio: of: support to add custom add pin range function

[Bartosz Golaszewski]

On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
>
> Export custom function to add gpio pin range from pinctrl
> subsystem. This would make it possible to add pins to multi
> gpio chips.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  drivers/gpio/gpiolib-of.c   | 5 ++++-
>  include/linux/gpio/driver.h | 7 +++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 2e537ee979f3e2b6e8d5f86f3e121a66f2a8e083..64c8a153b823d65faebed9c4cd87952359b42765 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -1170,7 +1170,10 @@ int of_gpiochip_add(struct gpio_chip *chip)
>         if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
>                 return -EINVAL;
>
> -       ret = of_gpiochip_add_pin_range(chip);
> +       if (!chip->of_add_pin_range)
> +               chip->of_add_pin_range = of_gpiochip_add_pin_range;
> +
> +       ret = chip->of_add_pin_range(chip);
>         if (ret)
>                 return ret;
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 2dd7cb9cc270a68ddedbcdd5d44e0d0f88dfa785..a7b966c78a2f62075fb7804f6e96028564dda161 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -528,6 +528,13 @@ struct gpio_chip {
>          */
>         int (*of_xlate)(struct gpio_chip *gc,
>                         const struct of_phandle_args *gpiospec, u32 *flags);
> +
> +       /**
> +        * @of_add_pin_range:
> +        *
> +        * Callback to add pin ranges from pinctrl
> +        */

Please, make the API contract more specific: describe the return value
and check it in the call place if it can return errors.

Also: is this even OF-specific if it doesn't take any OF argument? Why
not just add_pin_range()?

Bart


> +       int (*of_add_pin_range)(struct gpio_chip *chip);
>  #endif /* CONFIG_OF_GPIO */
>  };
>
>
> --
> 2.48.1
>

Re: [PATCH v5 1/5] gpio: of: support to add custom add pin range function

[Yixun Lan]

Hi Bartosz Golaszewski:

On 11:22 Thu 20 Feb     , Bartosz Golaszewski wrote:
> On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
> >
> > Export custom function to add gpio pin range from pinctrl
> > subsystem. This would make it possible to add pins to multi
> > gpio chips.
> >
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  drivers/gpio/gpiolib-of.c   | 5 ++++-
> >  include/linux/gpio/driver.h | 7 +++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 2e537ee979f3e2b6e8d5f86f3e121a66f2a8e083..64c8a153b823d65faebed9c4cd87952359b42765 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -1170,7 +1170,10 @@ int of_gpiochip_add(struct gpio_chip *chip)
> >         if (chip->of_gpio_n_cells > MAX_PHANDLE_ARGS)
> >                 return -EINVAL;
> >
> > -       ret = of_gpiochip_add_pin_range(chip);
> > +       if (!chip->of_add_pin_range)
> > +               chip->of_add_pin_range = of_gpiochip_add_pin_range;
> > +
> > +       ret = chip->of_add_pin_range(chip);
> >         if (ret)
> >                 return ret;
> >
> > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> > index 2dd7cb9cc270a68ddedbcdd5d44e0d0f88dfa785..a7b966c78a2f62075fb7804f6e96028564dda161 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -528,6 +528,13 @@ struct gpio_chip {
> >          */
> >         int (*of_xlate)(struct gpio_chip *gc,
> >                         const struct of_phandle_args *gpiospec, u32 *flags);
> > +
> > +       /**
> > +        * @of_add_pin_range:
> > +        *
> > +        * Callback to add pin ranges from pinctrl
> > +        */
> 
> Please, make the API contract more specific: describe the return value
> and check it in the call place if it can return errors.
> 
> Also: is this even OF-specific if it doesn't take any OF argument? Why
> not just add_pin_range()?
> 
now, this patch is obsolete, please ignore
it will be replaced by the one sent by LinusW
https://lore.kernel.org/all/20250218-gpio-ranges-fourcell-v1-0-b1f3db6c8036@linaro.org/

> Bart
> 
> 
> > +       int (*of_add_pin_range)(struct gpio_chip *chip);
> >  #endif /* CONFIG_OF_GPIO */
> >  };
> >
> >
> > --
> > 2.48.1
> >
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

[PATCH v5 2/5] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

The GPIO controller of K1 support basic functions as input/output,
all pins can be used as interrupt which route to one IRQ line,
trigger type can be select between rising edge, failing edge, or both.
There are four GPIO banks, each consisting of 32 pins.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 81 ++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..72a3ed2882782e5d22cf7d7a499c9084aefd961a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/spacemit,k1-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 GPIO controller
+
+maintainers:
+  - Yixun Lan <dlan@gentoo.org>
+
+description:
+  The controller's registers are organized as sets of eight 32-bit
+  registers with each set of port controlling 32 pins.  A single
+  interrupt line is shared for all of the pins by the controller.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    const: spacemit,k1-gpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 3
+    description:
+      The first two cells are the GPIO bank index and offset inside the bank,
+      the third cell should specify GPIO flag.
+
+  gpio-ranges: true
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 3
+    description:
+      The first two cells are the GPIO bank index and offset inside the bank,
+      the third cell should specify interrupt flag. The controller does not
+      support level interrupts, so flags of IRQ_TYPE_LEVEL_HIGH, IRQ_TYPE_LEVEL_LOW
+      should not be used. Refer <dt-bindings/interrupt-controller/irq.h> for valid flags.
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+  - gpio-ranges
+
+additionalProperties: false
+
+dependencies:
+  interrupt-controller: [ interrupts ]
+
+examples:
+  - |
+    gpio: gpio@d4019000 {
+      compatible = "spacemit,k1-gpio";
+      reg = <0xd4019000 0x800>;
+      gpio-controller;
+      #gpio-cells = <3>;
+      interrupts = <58>;
+      interrupt-controller;
+      interrupt-parent = <&plic>;
+      #interrupt-cells = <3>;
+      gpio-ranges = <&pinctrl 0 0 32>,
+                    <&pinctrl 0 32 32>,
+                    <&pinctrl 0 64 32>,
+                    <&pinctrl 0 96 32>;
+    };
+...

-- 
2.48.1

Re: [PATCH v5 2/5] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

Hi Yixun,

On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:

> The GPIO controller of K1 support basic functions as input/output,
> all pins can be used as interrupt which route to one IRQ line,
> trigger type can be select between rising edge, failing edge, or both.
> There are four GPIO banks, each consisting of 32 pins.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
(...)

> +      gpio-ranges = <&pinctrl 0 0 32>,
> +                    <&pinctrl 0 32 32>,
> +                    <&pinctrl 0 64 32>,
> +                    <&pinctrl 0 96 32>;

In my core patch I assume that we encode the gpiochip instance number
also into the ranges:
<&pinctrl 0 0 0 32>, <&pinctrl 1 0 64 32> etc.

Yours,
Linus Walleij

[PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Implement GPIO functionality which capable of setting pin as
input, output. Also, each pin can be used as interrupt which
support rising, failing, or both edge type trigger.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/gpio/Kconfig            |   8 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-spacemit-k1.c | 376 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 385 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index add5ad29a673c09082a913cb2404073b2034af48..eaae729eec00a3d6d2b83769aed3e2b0ca9927e5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -655,6 +655,14 @@ config GPIO_SNPS_CREG
 	  where only several fields in register belong to GPIO lines and
 	  each GPIO line owns a field with different length and on/off value.
 
+config GPIO_SPACEMIT_K1
+	bool "SPACEMIT K1 GPIO support"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the SpacemiT's K1 GPIO device.
+
 config GPIO_SPEAR_SPICS
 	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
 	depends on PLAT_SPEAR
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index af3ba4d81b583842893ea69e677fbe2abf31bc7b..6709ce511a0cf10310a94521c85a2d382dcfa696 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -156,6 +156,7 @@ obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
+obj-$(CONFIG_GPIO_SPACEMIT_K1)		+= gpio-spacemit-k1.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
 obj-$(CONFIG_GPIO_STMPE)		+= gpio-stmpe.o
diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
new file mode 100644
index 0000000000000000000000000000000000000000..f72511b5ab8f8f0b1d1c9e89d2f9ca07b623a866
--- /dev/null
+++ b/drivers/gpio/gpio-spacemit-k1.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
+ * Copyright (C) 2025 Yixun Lan <dlan@gentoo.org>
+ */
+
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/gpio/driver.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/property.h>
+#include <linux/seq_file.h>
+#include <linux/module.h>
+
+#include "gpiolib.h"
+
+/* register offset */
+/* GPIO port level register */
+#define GPLR		0x00
+/* GPIO port direction register - R/W */
+#define GPDR		0x0c
+/* GPIO port set register - W */
+#define GPSR		0x18
+/* GPIO port clear register - W */
+#define GPCR		0x24
+/* GPIO port rising edge register R/W */
+#define GRER		0x30
+/* GPIO port falling edge register R/W */
+#define GFER		0x3c
+/* GPIO edge detect status register - R/W1C */
+#define GEDR		0x48
+/*  GPIO (set) direction register - W */
+#define GSDR		0x54
+/* GPIO (clear) direction register - W */
+#define GCDR		0x60
+/* GPIO (set) rising edge detect enable register - W */
+#define GSRER		0x6c
+/* GPIO (clear) rising edge detect enable register - W */
+#define GCRER		0x78
+/* GPIO (set) falling edge detect enable register - W */
+#define GSFER		0x84
+/* GPIO (clear) falling edge detect enable register - W */
+#define GCFER		0x90
+/* GPIO interrupt mask register, 0 disable, 1 enable - R/W */
+#define GAPMASK		0x9c
+
+#define NR_BANKS		4
+#define NR_GPIOS_PER_BANK	32
+
+#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
+
+struct spacemit_gpio;
+
+struct spacemit_gpio_bank {
+	struct gpio_chip		gc;
+	struct spacemit_gpio		*sg;
+	void __iomem			*base;
+	u32				index;
+	u32				irq_mask;
+	u32				irq_rising_edge;
+	u32				irq_falling_edge;
+};
+
+struct spacemit_gpio {
+	struct	device			*dev;
+	struct	spacemit_gpio_bank	sgb[NR_BANKS];
+};
+
+static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
+{
+	struct spacemit_gpio_bank *gb = dev_id;
+	unsigned long pending;
+	u32 n, gedr;
+
+	gedr = readl(gb->base + GEDR);
+	if (!gedr)
+		return IRQ_NONE;
+	writel(gedr, gb->base + GEDR);
+
+	gedr = gedr & gb->irq_mask;
+	if (!gedr)
+		return IRQ_NONE;
+
+	pending = gedr;
+	for_each_set_bit(n, &pending, BITS_PER_LONG)
+		handle_nested_irq(irq_find_mapping(gb->gc.irq.domain, n));
+
+	return IRQ_HANDLED;
+}
+
+static void spacemit_gpio_irq_ack(struct irq_data *d)
+{
+	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+
+	writel(BIT(irqd_to_hwirq(d)), gb->base + GEDR);
+}
+
+static void spacemit_gpio_irq_mask(struct irq_data *d)
+{
+	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	gb->irq_mask &= ~bit;
+
+	if (bit & gb->irq_rising_edge)
+		writel(bit, gb->base + GCRER);
+
+	if (bit & gb->irq_falling_edge)
+		writel(bit, gb->base + GCFER);
+}
+
+static void spacemit_gpio_irq_unmask(struct irq_data *d)
+{
+	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	gb->irq_mask |= bit;
+
+	if (bit & gb->irq_rising_edge)
+		writel(bit,  gb->base + GSRER);
+
+	if (bit & gb->irq_falling_edge)
+		writel(bit, gb->base + GSFER);
+}
+
+static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	if (type & IRQ_TYPE_EDGE_RISING) {
+		gb->irq_rising_edge |= bit;
+		writel(bit, gb->base + GSRER);
+	} else {
+		gb->irq_rising_edge &= ~bit;
+		writel(bit, gb->base + GCRER);
+	}
+
+	if (type & IRQ_TYPE_EDGE_FALLING) {
+		gb->irq_falling_edge |= bit;
+		writel(bit, gb->base + GSFER);
+	} else {
+		gb->irq_falling_edge &= ~bit;
+		writel(bit, gb->base + GCFER);
+	}
+
+	return 0;
+}
+
+static void spacemit_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
+{
+	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(data);
+
+	seq_printf(p, "%s-%d", dev_name(gb->gc.parent), gb->index);
+}
+
+static struct irq_chip spacemit_gpio_chip = {
+	.name		= "k1-gpio-irqchip",
+	.irq_ack	= spacemit_gpio_irq_ack,
+	.irq_mask	= spacemit_gpio_irq_mask,
+	.irq_unmask	= spacemit_gpio_irq_unmask,
+	.irq_set_type	= spacemit_gpio_irq_set_type,
+	.irq_print_chip	= spacemit_gpio_irq_print_chip,
+	.flags		= IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int spacemit_gpio_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec, u32 *flags)
+{
+	struct spacemit_gpio_bank *gb = gpiochip_get_data(gc);
+	struct spacemit_gpio *sg = gb->sg;
+
+	int i;
+
+	if (gc->of_gpio_n_cells != 3)
+		return -EINVAL;
+
+	if (gpiospec->args_count < gc->of_gpio_n_cells)
+		return -EINVAL;
+
+	i = gpiospec->args[0];
+	if (i >= NR_BANKS)
+		return -EINVAL;
+
+	if (gc != &sg->sgb[i].gc)
+		return -EINVAL;
+
+	if (gpiospec->args[1] >= gc->ngpio)
+		return -EINVAL;
+
+	if (flags)
+		*flags = gpiospec->args[2];
+
+	return gpiospec->args[1];
+}
+
+static int spacemit_add_pin_range(struct gpio_chip *gc)
+{
+	struct spacemit_gpio_bank *gb;
+	struct of_phandle_args pinspec;
+	struct pinctrl_dev *pctldev;
+	struct device_node *np;
+	int ret, trim;
+
+	np = dev_of_node(&gc->gpiodev->dev);
+	if (!np)
+		return 0;
+
+	gb = to_spacemit_gpio_bank(gc);
+
+	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+					       gb->index, &pinspec);
+	if (ret)
+		return ret;
+
+	pctldev = of_pinctrl_get(pinspec.np);
+	of_node_put(pinspec.np);
+	if (!pctldev)
+		return -EPROBE_DEFER;
+
+	/* Ignore ranges outside of this GPIO chip */
+	if (pinspec.args[0] >= (gc->offset + gc->ngpio))
+		return -EINVAL;
+
+	if (pinspec.args[0] + pinspec.args[2] <= gc->offset)
+		return -EINVAL;
+
+	if (!pinspec.args[2])
+		return -EINVAL;
+
+	/* Trim the range to fit this GPIO chip */
+	if (gc->offset > pinspec.args[0]) {
+		trim = gc->offset - pinspec.args[0];
+		pinspec.args[2] -= trim;
+		pinspec.args[1] += trim;
+		pinspec.args[0] = 0;
+	} else {
+		pinspec.args[0] -= gc->offset;
+	}
+	if ((pinspec.args[0] + pinspec.args[2]) > gc->ngpio)
+		pinspec.args[2] = gc->ngpio - pinspec.args[0];
+
+	ret = gpiochip_add_pin_range(gc,
+				     pinctrl_dev_get_devname(pctldev),
+				     pinspec.args[0],
+				     pinspec.args[1],
+				     pinspec.args[2]);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
+				  void __iomem *regs,
+				  int index, int irq)
+{
+	struct spacemit_gpio_bank *gb = &sg->sgb[index];
+	struct gpio_chip *gc = &gb->gc;
+	struct device *dev = sg->dev;
+	struct gpio_irq_chip *girq;
+	void __iomem *dat, *set, *clr, *dirin, *dirout;
+	int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
+
+	gb->index = index;
+	gb->base = regs + bank_base[index];
+
+	dat	= gb->base + GPLR;
+	set	= gb->base + GPSR;
+	clr	= gb->base + GPCR;
+	dirin	= gb->base + GCDR;
+	dirout	= gb->base + GSDR;
+
+	/* This registers 32 GPIO lines per bank */
+	ret = bgpio_init(gc, dev, 4, dat, set, clr, dirout, dirin,
+			 BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init gpio chip\n");
+
+	gb->sg = sg;
+
+	gc->label		= dev_name(dev);
+	gc->request		= gpiochip_generic_request;
+	gc->free		= gpiochip_generic_free;
+	gc->ngpio		= NR_GPIOS_PER_BANK;
+	gc->base		= -1;
+
+#ifdef CONFIG_OF_GPIO
+	gc->of_xlate		= spacemit_gpio_xlate;
+	gc->of_add_pin_range	= spacemit_add_pin_range;
+	gc->of_gpio_n_cells	= 3;
+#endif
+
+	girq			= &gc->irq;
+	girq->threaded		= true;
+	girq->handler		= handle_simple_irq;
+
+	gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
+
+	/* Clear Edge Detection Settings */
+	writel(0x0, gb->base + GRER);
+	writel(0x0, gb->base + GFER);
+	/* Clear and Disable Interrupt */
+	writel(0xffffffff, gb->base + GCFER);
+	writel(0xffffffff, gb->base + GCRER);
+	writel(0, gb->base + GAPMASK);
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					spacemit_gpio_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED,
+					gb->gc.label, gb);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to register IRQ\n");
+
+	ret = devm_gpiochip_add_data(dev, gc, gb);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to add gpio chip\n");
+
+	/* Eable Interrupt */
+	writel(0xffffffff, gb->base + GAPMASK);
+
+	return 0;
+}
+
+static int spacemit_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct spacemit_gpio *sg;
+	struct resource *res;
+	void __iomem *regs;
+	int i, irq, ret;
+
+	sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
+	if (!sg)
+		return -ENOMEM;
+
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	sg->dev	= dev;
+
+	for (i = 0; i < NR_BANKS; i++) {
+		ret = spacemit_gpio_add_bank(sg, regs, i, irq);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id spacemit_gpio_dt_ids[] = {
+	{ .compatible = "spacemit,k1-gpio" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver spacemit_gpio_driver = {
+	.probe		= spacemit_gpio_probe,
+	.driver		= {
+		.name	= "k1-gpio",
+		.of_match_table = spacemit_gpio_dt_ids,
+	},
+};
+module_platform_driver(spacemit_gpio_driver);
+
+MODULE_AUTHOR("Yixun Lan <dlan@gentoo.org>");
+MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
+MODULE_LICENSE("GPL");

-- 
2.48.1

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Bartosz Golaszewski]

On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
>
> Implement GPIO functionality which capable of setting pin as
> input, output. Also, each pin can be used as interrupt which
> support rising, failing, or both edge type trigger.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  drivers/gpio/Kconfig            |   8 +
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-spacemit-k1.c | 376 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 385 insertions(+)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index add5ad29a673c09082a913cb2404073b2034af48..eaae729eec00a3d6d2b83769aed3e2b0ca9927e5 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -655,6 +655,14 @@ config GPIO_SNPS_CREG
>           where only several fields in register belong to GPIO lines and
>           each GPIO line owns a field with different length and on/off value.
>
> +config GPIO_SPACEMIT_K1
> +       bool "SPACEMIT K1 GPIO support"
> +       depends on ARCH_SPACEMIT || COMPILE_TEST
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say yes here to support the SpacemiT's K1 GPIO device.
> +
>  config GPIO_SPEAR_SPICS
>         bool "ST SPEAr13xx SPI Chip Select as GPIO support"
>         depends on PLAT_SPEAR
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index af3ba4d81b583842893ea69e677fbe2abf31bc7b..6709ce511a0cf10310a94521c85a2d382dcfa696 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -156,6 +156,7 @@ obj-$(CONFIG_GPIO_SIOX)                     += gpio-siox.o
>  obj-$(CONFIG_GPIO_SL28CPLD)            += gpio-sl28cpld.o
>  obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
>  obj-$(CONFIG_GPIO_SODAVILLE)           += gpio-sodaville.o
> +obj-$(CONFIG_GPIO_SPACEMIT_K1)         += gpio-spacemit-k1.o
>  obj-$(CONFIG_GPIO_SPEAR_SPICS)         += gpio-spear-spics.o
>  obj-$(CONFIG_GPIO_SPRD)                        += gpio-sprd.o
>  obj-$(CONFIG_GPIO_STMPE)               += gpio-stmpe.o
> diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f72511b5ab8f8f0b1d1c9e89d2f9ca07b623a866
> --- /dev/null
> +++ b/drivers/gpio/gpio-spacemit-k1.c
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
> + * Copyright (C) 2025 Yixun Lan <dlan@gentoo.org>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/property.h>
> +#include <linux/seq_file.h>
> +#include <linux/module.h>
> +

Please order includes alphabetically.

> +#include "gpiolib.h"
> +
> +/* register offset */
> +/* GPIO port level register */
> +#define GPLR           0x00
> +/* GPIO port direction register - R/W */
> +#define GPDR           0x0c
> +/* GPIO port set register - W */
> +#define GPSR           0x18
> +/* GPIO port clear register - W */
> +#define GPCR           0x24
> +/* GPIO port rising edge register R/W */
> +#define GRER           0x30
> +/* GPIO port falling edge register R/W */
> +#define GFER           0x3c
> +/* GPIO edge detect status register - R/W1C */
> +#define GEDR           0x48
> +/*  GPIO (set) direction register - W */
> +#define GSDR           0x54
> +/* GPIO (clear) direction register - W */
> +#define GCDR           0x60
> +/* GPIO (set) rising edge detect enable register - W */
> +#define GSRER          0x6c
> +/* GPIO (clear) rising edge detect enable register - W */
> +#define GCRER          0x78
> +/* GPIO (set) falling edge detect enable register - W */
> +#define GSFER          0x84
> +/* GPIO (clear) falling edge detect enable register - W */
> +#define GCFER          0x90
> +/* GPIO interrupt mask register, 0 disable, 1 enable - R/W */
> +#define GAPMASK                0x9c
> +
> +#define NR_BANKS               4
> +#define NR_GPIOS_PER_BANK      32
> +

Please use a common prefix for all symbols, even for defines. Use
SPACEMIT_GCFER, etc.

> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> +
> +struct spacemit_gpio;
> +
> +struct spacemit_gpio_bank {
> +       struct gpio_chip                gc;
> +       struct spacemit_gpio            *sg;
> +       void __iomem                    *base;
> +       u32                             index;
> +       u32                             irq_mask;
> +       u32                             irq_rising_edge;
> +       u32                             irq_falling_edge;
> +};
> +
> +struct spacemit_gpio {
> +       struct  device                  *dev;
> +       struct  spacemit_gpio_bank      sgb[NR_BANKS];
> +};

Please don't use tabs in struct definitions.

> +
> +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> +{
> +       struct spacemit_gpio_bank *gb = dev_id;
> +       unsigned long pending;
> +       u32 n, gedr;
> +
> +       gedr = readl(gb->base + GEDR);
> +       if (!gedr)
> +               return IRQ_NONE;
> +       writel(gedr, gb->base + GEDR);
> +
> +       gedr = gedr & gb->irq_mask;
> +       if (!gedr)
> +               return IRQ_NONE;
> +
> +       pending = gedr;
> +       for_each_set_bit(n, &pending, BITS_PER_LONG)
> +               handle_nested_irq(irq_find_mapping(gb->gc.irq.domain, n));
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void spacemit_gpio_irq_ack(struct irq_data *d)
> +{
> +       struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +
> +       writel(BIT(irqd_to_hwirq(d)), gb->base + GEDR);
> +}
> +
> +static void spacemit_gpio_irq_mask(struct irq_data *d)
> +{
> +       struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +       u32 bit = BIT(irqd_to_hwirq(d));
> +
> +       gb->irq_mask &= ~bit;
> +
> +       if (bit & gb->irq_rising_edge)
> +               writel(bit, gb->base + GCRER);
> +
> +       if (bit & gb->irq_falling_edge)
> +               writel(bit, gb->base + GCFER);
> +}
> +
> +static void spacemit_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +       u32 bit = BIT(irqd_to_hwirq(d));
> +
> +       gb->irq_mask |= bit;
> +
> +       if (bit & gb->irq_rising_edge)
> +               writel(bit,  gb->base + GSRER);
> +
> +       if (bit & gb->irq_falling_edge)
> +               writel(bit, gb->base + GSFER);
> +}
> +
> +static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +       u32 bit = BIT(irqd_to_hwirq(d));
> +
> +       if (type & IRQ_TYPE_EDGE_RISING) {
> +               gb->irq_rising_edge |= bit;
> +               writel(bit, gb->base + GSRER);
> +       } else {
> +               gb->irq_rising_edge &= ~bit;
> +               writel(bit, gb->base + GCRER);
> +       }
> +
> +       if (type & IRQ_TYPE_EDGE_FALLING) {
> +               gb->irq_falling_edge |= bit;
> +               writel(bit, gb->base + GSFER);
> +       } else {
> +               gb->irq_falling_edge &= ~bit;
> +               writel(bit, gb->base + GCFER);
> +       }
> +
> +       return 0;
> +}
> +
> +static void spacemit_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> +{
> +       struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(data);
> +
> +       seq_printf(p, "%s-%d", dev_name(gb->gc.parent), gb->index);
> +}
> +
> +static struct irq_chip spacemit_gpio_chip = {
> +       .name           = "k1-gpio-irqchip",
> +       .irq_ack        = spacemit_gpio_irq_ack,
> +       .irq_mask       = spacemit_gpio_irq_mask,
> +       .irq_unmask     = spacemit_gpio_irq_unmask,
> +       .irq_set_type   = spacemit_gpio_irq_set_type,
> +       .irq_print_chip = spacemit_gpio_irq_print_chip,
> +       .flags          = IRQCHIP_IMMUTABLE,
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +
> +static int spacemit_gpio_xlate(struct gpio_chip *gc,
> +                              const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> +       struct spacemit_gpio_bank *gb = gpiochip_get_data(gc);
> +       struct spacemit_gpio *sg = gb->sg;
> +

Stray newline.

> +       int i;
> +
> +       if (gc->of_gpio_n_cells != 3)
> +               return -EINVAL;
> +
> +       if (gpiospec->args_count < gc->of_gpio_n_cells)
> +               return -EINVAL;
> +
> +       i = gpiospec->args[0];
> +       if (i >= NR_BANKS)
> +               return -EINVAL;
> +
> +       if (gc != &sg->sgb[i].gc)
> +               return -EINVAL;
> +
> +       if (gpiospec->args[1] >= gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[2];
> +
> +       return gpiospec->args[1];
> +}
> +
> +static int spacemit_add_pin_range(struct gpio_chip *gc)
> +{
> +       struct spacemit_gpio_bank *gb;
> +       struct of_phandle_args pinspec;
> +       struct pinctrl_dev *pctldev;
> +       struct device_node *np;
> +       int ret, trim;
> +
> +       np = dev_of_node(&gc->gpiodev->dev);
> +       if (!np)
> +               return 0;
> +
> +       gb = to_spacemit_gpio_bank(gc);
> +
> +       ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +                                              gb->index, &pinspec);
> +       if (ret)
> +               return ret;
> +
> +       pctldev = of_pinctrl_get(pinspec.np);
> +       of_node_put(pinspec.np);
> +       if (!pctldev)
> +               return -EPROBE_DEFER;
> +
> +       /* Ignore ranges outside of this GPIO chip */
> +       if (pinspec.args[0] >= (gc->offset + gc->ngpio))
> +               return -EINVAL;
> +
> +       if (pinspec.args[0] + pinspec.args[2] <= gc->offset)
> +               return -EINVAL;
> +
> +       if (!pinspec.args[2])
> +               return -EINVAL;
> +
> +       /* Trim the range to fit this GPIO chip */
> +       if (gc->offset > pinspec.args[0]) {
> +               trim = gc->offset - pinspec.args[0];
> +               pinspec.args[2] -= trim;
> +               pinspec.args[1] += trim;
> +               pinspec.args[0] = 0;
> +       } else {
> +               pinspec.args[0] -= gc->offset;
> +       }
> +       if ((pinspec.args[0] + pinspec.args[2]) > gc->ngpio)
> +               pinspec.args[2] = gc->ngpio - pinspec.args[0];
> +
> +       ret = gpiochip_add_pin_range(gc,
> +                                    pinctrl_dev_get_devname(pctldev),
> +                                    pinspec.args[0],
> +                                    pinspec.args[1],
> +                                    pinspec.args[2]);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
> +                                 void __iomem *regs,
> +                                 int index, int irq)
> +{
> +       struct spacemit_gpio_bank *gb = &sg->sgb[index];
> +       struct gpio_chip *gc = &gb->gc;
> +       struct device *dev = sg->dev;
> +       struct gpio_irq_chip *girq;
> +       void __iomem *dat, *set, *clr, *dirin, *dirout;
> +       int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
> +
> +       gb->index = index;
> +       gb->base = regs + bank_base[index];
> +
> +       dat     = gb->base + GPLR;
> +       set     = gb->base + GPSR;
> +       clr     = gb->base + GPCR;
> +       dirin   = gb->base + GCDR;
> +       dirout  = gb->base + GSDR;
> +
> +       /* This registers 32 GPIO lines per bank */
> +       ret = bgpio_init(gc, dev, 4, dat, set, clr, dirout, dirin,
> +                        BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "failed to init gpio chip\n");
> +
> +       gb->sg = sg;
> +
> +       gc->label               = dev_name(dev);
> +       gc->request             = gpiochip_generic_request;
> +       gc->free                = gpiochip_generic_free;
> +       gc->ngpio               = NR_GPIOS_PER_BANK;
> +       gc->base                = -1;
> +
> +#ifdef CONFIG_OF_GPIO
> +       gc->of_xlate            = spacemit_gpio_xlate;
> +       gc->of_add_pin_range    = spacemit_add_pin_range;
> +       gc->of_gpio_n_cells     = 3;
> +#endif
> +
> +       girq                    = &gc->irq;
> +       girq->threaded          = true;
> +       girq->handler           = handle_simple_irq;
> +
> +       gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
> +
> +       /* Clear Edge Detection Settings */
> +       writel(0x0, gb->base + GRER);
> +       writel(0x0, gb->base + GFER);
> +       /* Clear and Disable Interrupt */
> +       writel(0xffffffff, gb->base + GCFER);
> +       writel(0xffffffff, gb->base + GCRER);
> +       writel(0, gb->base + GAPMASK);
> +
> +       ret = devm_request_threaded_irq(dev, irq, NULL,
> +                                       spacemit_gpio_irq_handler,
> +                                       IRQF_ONESHOT | IRQF_SHARED,
> +                                       gb->gc.label, gb);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> +       ret = devm_gpiochip_add_data(dev, gc, gb);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to add gpio chip\n");
> +
> +       /* Eable Interrupt */

Enable.

> +       writel(0xffffffff, gb->base + GAPMASK);
> +
> +       return 0;
> +}
> +
> +static int spacemit_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct spacemit_gpio *sg;
> +       struct resource *res;
> +       void __iomem *regs;
> +       int i, irq, ret;
> +
> +       sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
> +       if (!sg)
> +               return -ENOMEM;
> +
> +       regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +       if (IS_ERR(regs))
> +               return PTR_ERR(regs);
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0)
> +               return irq;
> +
> +       sg->dev = dev;
> +
> +       for (i = 0; i < NR_BANKS; i++) {
> +               ret = spacemit_gpio_add_bank(sg, regs, i, irq);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id spacemit_gpio_dt_ids[] = {
> +       { .compatible = "spacemit,k1-gpio" },
> +       { /* sentinel */ }
> +};
> +
> +static struct platform_driver spacemit_gpio_driver = {
> +       .probe          = spacemit_gpio_probe,
> +       .driver         = {
> +               .name   = "k1-gpio",
> +               .of_match_table = spacemit_gpio_dt_ids,
> +       },
> +};
> +module_platform_driver(spacemit_gpio_driver);
> +
> +MODULE_AUTHOR("Yixun Lan <dlan@gentoo.org>");
> +MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
> +MODULE_LICENSE("GPL");
>
> --
> 2.48.1
>

Bart

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Chen Wang]

On 2025/2/20 21:34, Bartosz Golaszewski wrote:
> On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
[......]
>> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
>> +
>> +struct spacemit_gpio;
>> +
>> +struct spacemit_gpio_bank {
>> +       struct gpio_chip                gc;
>> +       struct spacemit_gpio            *sg;
>> +       void __iomem                    *base;
>> +       u32                             index;
>> +       u32                             irq_mask;
>> +       u32                             irq_rising_edge;
>> +       u32                             irq_falling_edge;
>> +};
>> +
>> +struct spacemit_gpio {
>> +       struct  device                  *dev;
>> +       struct  spacemit_gpio_bank      sgb[NR_BANKS];
>> +};
> Please don't use tabs in struct definitions.

Why not？I see 
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

[......]


> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Bartosz Golaszewski]

On Fri, Feb 21, 2025 at 12:36 AM Chen Wang <unicorn_wang@outlook.com> wrote:
>
>
> On 2025/2/20 21:34, Bartosz Golaszewski wrote:
> > On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
> [......]
> >> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> >> +
> >> +struct spacemit_gpio;
> >> +
> >> +struct spacemit_gpio_bank {
> >> +       struct gpio_chip                gc;
> >> +       struct spacemit_gpio            *sg;
> >> +       void __iomem                    *base;
> >> +       u32                             index;
> >> +       u32                             irq_mask;
> >> +       u32                             irq_rising_edge;
> >> +       u32                             irq_falling_edge;
> >> +};
> >> +
> >> +struct spacemit_gpio {
> >> +       struct  device                  *dev;
> >> +       struct  spacemit_gpio_bank      sgb[NR_BANKS];
> >> +};
> > Please don't use tabs in struct definitions.
>
> Why not？I see
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
>

This is for the tip tree, not treewide.

It's my personal maintainer preference. We do use both under
drivers/gpio/ but I prefer no-tabs in new code.

Bart

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Bartosz Golaszewski:

On 09:37 Fri 21 Feb     , Bartosz Golaszewski wrote:
> On Fri, Feb 21, 2025 at 12:36 AM Chen Wang <unicorn_wang@outlook.com> wrote:
> >
> >
> > On 2025/2/20 21:34, Bartosz Golaszewski wrote:
> > > On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
> > [......]
> > >> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> > >> +
> > >> +struct spacemit_gpio;
> > >> +
> > >> +struct spacemit_gpio_bank {
> > >> +       struct gpio_chip                gc;
> > >> +       struct spacemit_gpio            *sg;
> > >> +       void __iomem                    *base;
> > >> +       u32                             index;
> > >> +       u32                             irq_mask;
> > >> +       u32                             irq_rising_edge;
> > >> +       u32                             irq_falling_edge;
> > >> +};
> > >> +
> > >> +struct spacemit_gpio {
> > >> +       struct  device                  *dev;
> > >> +       struct  spacemit_gpio_bank      sgb[NR_BANKS];
> > >> +};
> > > Please don't use tabs in struct definitions.
> >
> > Why not？I see
> > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
> >
> 
> This is for the tip tree, not treewide.
> 
> It's my personal maintainer preference. We do use both under
> drivers/gpio/ but I prefer no-tabs in new code.
> 

thanks for this explanation..

my intention was trying to keep struct members aligned
if tabs is a no-go, would using multi blank spaces to align be acceptable?

something like:

struct spacemit_gpio_bank {
	struct gpio_chip       gc;
	struct spacemit_gpio   *sg;
	void   __iomem         *base;
       ...
}

> Bart

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Bartosz Golaszewski]

On Fri, Feb 21, 2025 at 1:21 PM Yixun Lan <dlan@gentoo.org> wrote:
>
> Hi Bartosz Golaszewski:
>
> On 09:37 Fri 21 Feb     , Bartosz Golaszewski wrote:
> > On Fri, Feb 21, 2025 at 12:36 AM Chen Wang <unicorn_wang@outlook.com> wrote:
> > >
> > >
> > > On 2025/2/20 21:34, Bartosz Golaszewski wrote:
> > > > On Mon, Feb 17, 2025 at 1:58 PM Yixun Lan <dlan@gentoo.org> wrote:
> > > [......]
> > > >> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> > > >> +
> > > >> +struct spacemit_gpio;
> > > >> +
> > > >> +struct spacemit_gpio_bank {
> > > >> +       struct gpio_chip                gc;
> > > >> +       struct spacemit_gpio            *sg;
> > > >> +       void __iomem                    *base;
> > > >> +       u32                             index;
> > > >> +       u32                             irq_mask;
> > > >> +       u32                             irq_rising_edge;
> > > >> +       u32                             irq_falling_edge;
> > > >> +};
> > > >> +
> > > >> +struct spacemit_gpio {
> > > >> +       struct  device                  *dev;
> > > >> +       struct  spacemit_gpio_bank      sgb[NR_BANKS];
> > > >> +};
> > > > Please don't use tabs in struct definitions.
> > >
> > > Why not？I see
> > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
> > >
> >
> > This is for the tip tree, not treewide.
> >
> > It's my personal maintainer preference. We do use both under
> > drivers/gpio/ but I prefer no-tabs in new code.
> >
>
> thanks for this explanation..
>
> my intention was trying to keep struct members aligned
> if tabs is a no-go, would using multi blank spaces to align be acceptable?
>
> something like:
>
> struct spacemit_gpio_bank {
>         struct gpio_chip       gc;
>         struct spacemit_gpio   *sg;
>         void   __iomem         *base;
>        ...
> }
>

No, that's even worse. :/

Why are we even wasting time arguing, can you just use a single space?
You'll get a driver merged and may just disappear from kernel
development, Linus and I will have to maintain the code so we do get
some degree of discretion when it comes to coding style.

Bart

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Alex Elder]

On 2/17/25 6:57 AM, Yixun Lan wrote:
> Implement GPIO functionality which capable of setting pin as
> input, output. Also, each pin can be used as interrupt which
> support rising, failing, or both edge type trigger.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>

This looks nicer!

I have some more comments, but they're pretty minor.

> ---
>   drivers/gpio/Kconfig            |   8 +
>   drivers/gpio/Makefile           |   1 +
>   drivers/gpio/gpio-spacemit-k1.c | 376 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 385 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index add5ad29a673c09082a913cb2404073b2034af48..eaae729eec00a3d6d2b83769aed3e2b0ca9927e5 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -655,6 +655,14 @@ config GPIO_SNPS_CREG
>   	  where only several fields in register belong to GPIO lines and
>   	  each GPIO line owns a field with different length and on/off value.
>   
> +config GPIO_SPACEMIT_K1
> +	bool "SPACEMIT K1 GPIO support"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on OF_GPIO
> +	select GPIOLIB_IRQCHIP
> +	help
> +	  Say yes here to support the SpacemiT's K1 GPIO device.
> +
>   config GPIO_SPEAR_SPICS
>   	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
>   	depends on PLAT_SPEAR
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index af3ba4d81b583842893ea69e677fbe2abf31bc7b..6709ce511a0cf10310a94521c85a2d382dcfa696 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -156,6 +156,7 @@ obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
>   obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
>   obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
>   obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
> +obj-$(CONFIG_GPIO_SPACEMIT_K1)		+= gpio-spacemit-k1.o
>   obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
>   obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
>   obj-$(CONFIG_GPIO_STMPE)		+= gpio-stmpe.o
> diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f72511b5ab8f8f0b1d1c9e89d2f9ca07b623a866
> --- /dev/null
> +++ b/drivers/gpio/gpio-spacemit-k1.c
> @@ -0,0 +1,376 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
> + * Copyright (C) 2025 Yixun Lan <dlan@gentoo.org>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/property.h>
> +#include <linux/seq_file.h>
> +#include <linux/module.h>
> +
> +#include "gpiolib.h"
> +
> +/* register offset */

The comments are great, but I think I'd like to see them be abbreviated
further and added to the right of the definitions, if you can do that.

I think you can drop "GPIO" and "register" in each one of them, and
that might get you close to an 80 column limit.  See what you can do.

> +/* GPIO port level register */

I think the port level register is read-only, and you didn't include
that annotation.

> +#define GPLR		0x00
> +/* GPIO port direction register - R/W */
> +#define GPDR		0x0c
> +/* GPIO port set register - W */
> +#define GPSR		0x18
> +/* GPIO port clear register - W */
> +#define GPCR		0x24
> +/* GPIO port rising edge register R/W */
> +#define GRER		0x30
> +/* GPIO port falling edge register R/W */
> +#define GFER		0x3c
> +/* GPIO edge detect status register - R/W1C */
> +#define GEDR		0x48
> +/*  GPIO (set) direction register - W */

Delete the extra space above.

> +#define GSDR		0x54
> +/* GPIO (clear) direction register - W */
> +#define GCDR		0x60
> +/* GPIO (set) rising edge detect enable register - W */
> +#define GSRER		0x6c
> +/* GPIO (clear) rising edge detect enable register - W */
> +#define GCRER		0x78
> +/* GPIO (set) falling edge detect enable register - W */
> +#define GSFER		0x84
> +/* GPIO (clear) falling edge detect enable register - W */
> +#define GCFER		0x90
> +/* GPIO interrupt mask register, 0 disable, 1 enable - R/W */
> +#define GAPMASK		0x9c
> +
> +#define NR_BANKS		4
> +#define NR_GPIOS_PER_BANK	32
> +
> +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> +
> +struct spacemit_gpio;
> +
> +struct spacemit_gpio_bank {
> +	struct gpio_chip		gc;
> +	struct spacemit_gpio		*sg;
> +	void __iomem			*base;
> +	u32				index;

You almost never use the index field.  It could easily be
computed rather than stored:

static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
{
	return (u32)(gb - gb->sg->sgb);
}

> +	u32				irq_mask;
> +	u32				irq_rising_edge;
> +	u32				irq_falling_edge;
> +};
> +
> +struct spacemit_gpio {
> +	struct	device			*dev;
> +	struct	spacemit_gpio_bank	sgb[NR_BANKS];
> +};
> +
> +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> +{
> +	struct spacemit_gpio_bank *gb = dev_id;
> +	unsigned long pending;
> +	u32 n, gedr;
> +
> +	gedr = readl(gb->base + GEDR);
> +	if (!gedr)
> +		return IRQ_NONE;
> +	writel(gedr, gb->base + GEDR);
> +
> +	gedr = gedr & gb->irq_mask;
> +	if (!gedr)
> +		return IRQ_NONE;
> +
> +	pending = gedr;

Instead, do:

	pending = gedr & gb->irq_mask;
	if (!pending)
		return IRQ_NONE;

> +	for_each_set_bit(n, &pending, BITS_PER_LONG)
> +		handle_nested_irq(irq_find_mapping(gb->gc.irq.domain, n));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void spacemit_gpio_irq_ack(struct irq_data *d)
> +{
> +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +
> +	writel(BIT(irqd_to_hwirq(d)), gb->base + GEDR);
> +}
> +
> +static void spacemit_gpio_irq_mask(struct irq_data *d)
> +{
> +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +
> +	gb->irq_mask &= ~bit;

This is a minor suggestion, and I'm not sure how much difference
it makes.  But here (and one or two more times below) you could
avoid the writel() calls if you know the particular IRQ was
already disabled.  (Maybe that won't ever happen?)

	if (!(gb->irq_mask & bit))
		return;

	gb->irq_mask &= !bit;
	...

This should work because in spacemit_gpio_add_bank() you reset
all the IRQ state and disable all IRQs, so the cached copy of
the irq_mask and the rising and falling edge masks should match
reality.

> +
> +	if (bit & gb->irq_rising_edge)
> +		writel(bit, gb->base + GCRER);
> +
> +	if (bit & gb->irq_falling_edge)
> +		writel(bit, gb->base + GCFER);
> +}
> +
> +static void spacemit_gpio_irq_unmask(struct irq_data *d)
> +{
> +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +


Same thought here.

	if (gb->irq_mask & bit)
		return;

> +	gb->irq_mask |= bit;
> +
> +	if (bit & gb->irq_rising_edge)
> +		writel(bit,  gb->base + GSRER);
> +
> +	if (bit & gb->irq_falling_edge)
> +		writel(bit, gb->base + GSFER);
> +}
> +
> +static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +

Same thought in this function, although it gets a little
messier looking.

> +	if (type & IRQ_TYPE_EDGE_RISING) {
> +		gb->irq_rising_edge |= bit;
> +		writel(bit, gb->base + GSRER);
> +	} else {
> +		gb->irq_rising_edge &= ~bit;
> +		writel(bit, gb->base + GCRER);
> +	}

Otherwise:

     if (type & IRQ_TYPE_EDGE_RISING)
	gb->irq_rising_edge |= bit;
     else
	gb->irq_rising_edge &= ~bit;
     writel(bit, gb->base + GSRER);

and again below.

> +
> +	if (type & IRQ_TYPE_EDGE_FALLING) {
> +		gb->irq_falling_edge |= bit;
> +		writel(bit, gb->base + GSFER);
> +	} else {
> +		gb->irq_falling_edge &= ~bit;
> +		writel(bit, gb->base + GCFER);
> +	}
> +
> +	return 0;
> +}
> +

You added this function in version 5 of the series.  Please call
attention to additions (or removals) like this in your cover page,
and/or in notes at the top of this patch.

> +static void spacemit_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> +{
> +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(data);
> +
> +	seq_printf(p, "%s-%d", dev_name(gb->gc.parent), gb->index);

Does this look like "gpiochip2-15" or something?  I wasn't able
to find it in the debugfs file system.

> +}
> +
> +static struct irq_chip spacemit_gpio_chip = {
> +	.name		= "k1-gpio-irqchip",
> +	.irq_ack	= spacemit_gpio_irq_ack,
> +	.irq_mask	= spacemit_gpio_irq_mask,
> +	.irq_unmask	= spacemit_gpio_irq_unmask,
> +	.irq_set_type	= spacemit_gpio_irq_set_type,
> +	.irq_print_chip	= spacemit_gpio_irq_print_chip,
> +	.flags		= IRQCHIP_IMMUTABLE,

Last time your flags value was IRQCHIP_SET_WAKE.  Why the change?

> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +

Maybe you could add a comment indicating that gpiospec->args[]
will contain:
0:  bank index
1:  GPIO offset within the bank
2:  flags

(And the GPIO chip instance number as Linus suggested.)

> +static int spacemit_gpio_xlate(struct gpio_chip *gc,
> +			       const struct of_phandle_args *gpiospec, u32 *flags)
> +{
> +	struct spacemit_gpio_bank *gb = gpiochip_get_data(gc);
> +	struct spacemit_gpio *sg = gb->sg;
> +

Get rid of the above blank line.

> +	int i;
> +

I'm not sure the context in which this runs.  Can it be given
arbitrary content from a DTB?  Mainly I'm interested to know
whether any of these checks can be eliminated.  If it's called
while parsing a DTB I can see why you'd need to verify all
input values for validity.

> +	if (gc->of_gpio_n_cells != 3)
> +		return -EINVAL;
> +
> +	if (gpiospec->args_count < gc->of_gpio_n_cells)
> +		return -EINVAL;
> +
> +	i = gpiospec->args[0];
> +	if (i >= NR_BANKS)
> +		return -EINVAL;
> +
> +	if (gc != &sg->sgb[i].gc)
> +		return -EINVAL;
> +
> +	if (gpiospec->args[1] >= gc->ngpio)
> +		return -EINVAL;
> +
> +	if (flags)
> +		*flags = gpiospec->args[2];
> +
> +	return gpiospec->args[1];
> +}
> +
> +static int spacemit_add_pin_range(struct gpio_chip *gc)
> +{
> +	struct spacemit_gpio_bank *gb;
> +	struct of_phandle_args pinspec;
> +	struct pinctrl_dev *pctldev;
> +	struct device_node *np;
> +	int ret, trim;
> +
> +	np = dev_of_node(&gc->gpiodev->dev);
> +	if (!np)
> +		return 0;
> +
> +	gb = to_spacemit_gpio_bank(gc);
> +
> +	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +					       gb->index, &pinspec);
> +	if (ret)
> +		return ret;
> +
> +	pctldev = of_pinctrl_get(pinspec.np);
> +	of_node_put(pinspec.np);
> +	if (!pctldev)
> +		return -EPROBE_DEFER;
> +
> +	/* Ignore ranges outside of this GPIO chip */
> +	if (pinspec.args[0] >= (gc->offset + gc->ngpio))
> +		return -EINVAL;
> +
> +	if (pinspec.args[0] + pinspec.args[2] <= gc->offset)
> +		return -EINVAL;
> +

I would do the following test earlier.

> +	if (!pinspec.args[2])
> +		return -EINVAL;
> +
> +	/* Trim the range to fit this GPIO chip */
> +	if (gc->offset > pinspec.args[0]) {
> +		trim = gc->offset - pinspec.args[0];
> +		pinspec.args[2] -= trim;
> +		pinspec.args[1] += trim;
> +		pinspec.args[0] = 0;
> +	} else {
> +		pinspec.args[0] -= gc->offset;
> +	}
> +	if ((pinspec.args[0] + pinspec.args[2]) > gc->ngpio)
> +		pinspec.args[2] = gc->ngpio - pinspec.args[0];
> +
> +	ret = gpiochip_add_pin_range(gc,
> +				     pinctrl_dev_get_devname(pctldev),
> +				     pinspec.args[0],
> +				     pinspec.args[1],
> +				     pinspec.args[2]);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Just do this:

     return  gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev),
				   pinspec.args[0], pinspec.args[2]);

> +}
> +
> +static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
> +				  void __iomem *regs,
> +				  int index, int irq)
> +{
> +	struct spacemit_gpio_bank *gb = &sg->sgb[index];
> +	struct gpio_chip *gc = &gb->gc;
> +	struct device *dev = sg->dev;
> +	struct gpio_irq_chip *girq;
> +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> +	int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
> +
> +	gb->index = index;
> +	gb->base = regs + bank_base[index];
> +
> +	dat	= gb->base + GPLR;
> +	set	= gb->base + GPSR;
> +	clr	= gb->base + GPCR;
> +	dirin	= gb->base + GCDR;
> +	dirout	= gb->base + GSDR;
> +
> +	/* This registers 32 GPIO lines per bank */
> +	ret = bgpio_init(gc, dev, 4, dat, set, clr, dirout, dirin,
> +			 BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init gpio chip\n");
> +
> +	gb->sg = sg;
> +
> +	gc->label		= dev_name(dev);
> +	gc->request		= gpiochip_generic_request;
> +	gc->free		= gpiochip_generic_free;
> +	gc->ngpio		= NR_GPIOS_PER_BANK;
> +	gc->base		= -1;
> +
> +#ifdef CONFIG_OF_GPIO

Why are these lines conditionally defined?  Is it intended
to allow CONFIG_COMPILE_TEST to work?  Your Kconfig states
that this *depends on* OF_GPIO, so this is probably not
needed.

You don't define spacemit_gpio_xlate() earlier conditionally.
Nor spacemit_add_pin_range().

> +	gc->of_xlate		= spacemit_gpio_xlate;
> +	gc->of_add_pin_range	= spacemit_add_pin_range;
> +	gc->of_gpio_n_cells	= 3;
> +#endif
> +
> +	girq			= &gc->irq;
> +	girq->threaded		= true;
> +	girq->handler		= handle_simple_irq;
> +
> +	gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
> +
> +	/* Clear Edge Detection Settings */
> +	writel(0x0, gb->base + GRER);
> +	writel(0x0, gb->base + GFER);
> +	/* Clear and Disable Interrupt */
> +	writel(0xffffffff, gb->base + GCFER);
> +	writel(0xffffffff, gb->base + GCRER);

It seems that GAPMASK is an overall interrupt mask register.
I assume that means that by writing 0 here, no interrupts
of any kind will be generated for any of the 32 GPIO ports.

If that's true, I would write this first, *then* disable
the rising and falling edge detection interrupts, *then*
clear any pending interrupts.

Are there any interrupt types other than rising and falling
edge?  Does this just provide an atomic way to disable both
types at once?  If there are no other interrupt types maybe
this could be used rather than disabling both types
separately using GCFER etc. in spacemit_gpio_irq_*mask().

					-Alex

> +	writel(0, gb->base + GAPMASK);
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL,
> +					spacemit_gpio_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					gb->gc.label, gb);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to register IRQ\n");
> +
> +	ret = devm_gpiochip_add_data(dev, gc, gb);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to add gpio chip\n");
> +
> +	/* Eable Interrupt */
> +	writel(0xffffffff, gb->base + GAPMASK);
> +
> +	return 0;
> +}
> +
> +static int spacemit_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spacemit_gpio *sg;
> +	struct resource *res;
> +	void __iomem *regs;
> +	int i, irq, ret;
> +
> +	sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
> +	if (!sg)
> +		return -ENOMEM;
> +
> +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	sg->dev	= dev;
> +
> +	for (i = 0; i < NR_BANKS; i++) {
> +		ret = spacemit_gpio_add_bank(sg, regs, i, irq);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id spacemit_gpio_dt_ids[] = {
> +	{ .compatible = "spacemit,k1-gpio" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver spacemit_gpio_driver = {
> +	.probe		= spacemit_gpio_probe,
> +	.driver		= {
> +		.name	= "k1-gpio",
> +		.of_match_table = spacemit_gpio_dt_ids,
> +	},
> +};
> +module_platform_driver(spacemit_gpio_driver);
> +
> +MODULE_AUTHOR("Yixun Lan <dlan@gentoo.org>");
> +MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v5 3/5] gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Alex:

thanks for the review

On 11:27 Fri 21 Feb     , Alex Elder wrote:
> On 2/17/25 6:57 AM, Yixun Lan wrote:
> > Implement GPIO functionality which capable of setting pin as
> > input, output. Also, each pin can be used as interrupt which
> > support rising, failing, or both edge type trigger.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> 
> This looks nicer!
> 
> I have some more comments, but they're pretty minor.
> 
> > ---
> >   drivers/gpio/Kconfig            |   8 +
> >   drivers/gpio/Makefile           |   1 +
> >   drivers/gpio/gpio-spacemit-k1.c | 376 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 385 insertions(+)
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index add5ad29a673c09082a913cb2404073b2034af48..eaae729eec00a3d6d2b83769aed3e2b0ca9927e5 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -655,6 +655,14 @@ config GPIO_SNPS_CREG
> >   	  where only several fields in register belong to GPIO lines and
> >   	  each GPIO line owns a field with different length and on/off value.
> >   
> > +config GPIO_SPACEMIT_K1
> > +	bool "SPACEMIT K1 GPIO support"
> > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > +	depends on OF_GPIO
> > +	select GPIOLIB_IRQCHIP
> > +	help
> > +	  Say yes here to support the SpacemiT's K1 GPIO device.
> > +
> >   config GPIO_SPEAR_SPICS
> >   	bool "ST SPEAr13xx SPI Chip Select as GPIO support"
> >   	depends on PLAT_SPEAR
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index af3ba4d81b583842893ea69e677fbe2abf31bc7b..6709ce511a0cf10310a94521c85a2d382dcfa696 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
> >   obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
> >   obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
> >   obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
> > +obj-$(CONFIG_GPIO_SPACEMIT_K1)		+= gpio-spacemit-k1.o
> >   obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
> >   obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
> >   obj-$(CONFIG_GPIO_STMPE)		+= gpio-stmpe.o
> > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f72511b5ab8f8f0b1d1c9e89d2f9ca07b623a866
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-spacemit-k1.c
> > @@ -0,0 +1,376 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR MIT
> > +/*
> > + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
> > + * Copyright (C) 2025 Yixun Lan <dlan@gentoo.org>
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/init.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/property.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/module.h>
> > +
> > +#include "gpiolib.h"
> > +
> > +/* register offset */
> 
> The comments are great, but I think I'd like to see them be abbreviated
> further and added to the right of the definitions, if you can do that.
> 
> I think you can drop "GPIO" and "register" in each one of them, and
> that might get you close to an 80 column limit.  See what you can do.
> 
sure, I can do this
> > +/* GPIO port level register */
> 
> I think the port level register is read-only, and you didn't include
> that annotation.
> 
right, I will add that annotation
> > +#define GPLR		0x00
> > +/* GPIO port direction register - R/W */
> > +#define GPDR		0x0c
> > +/* GPIO port set register - W */
> > +#define GPSR		0x18
> > +/* GPIO port clear register - W */
> > +#define GPCR		0x24
> > +/* GPIO port rising edge register R/W */
> > +#define GRER		0x30
> > +/* GPIO port falling edge register R/W */
> > +#define GFER		0x3c
> > +/* GPIO edge detect status register - R/W1C */
> > +#define GEDR		0x48
> > +/*  GPIO (set) direction register - W */
> 
> Delete the extra space above.
> 
will do
> > +#define GSDR		0x54
> > +/* GPIO (clear) direction register - W */
> > +#define GCDR		0x60
> > +/* GPIO (set) rising edge detect enable register - W */
> > +#define GSRER		0x6c
> > +/* GPIO (clear) rising edge detect enable register - W */
> > +#define GCRER		0x78
> > +/* GPIO (set) falling edge detect enable register - W */
> > +#define GSFER		0x84
> > +/* GPIO (clear) falling edge detect enable register - W */
> > +#define GCFER		0x90
> > +/* GPIO interrupt mask register, 0 disable, 1 enable - R/W */
> > +#define GAPMASK		0x9c
> > +
> > +#define NR_BANKS		4
> > +#define NR_GPIOS_PER_BANK	32
> > +
> > +#define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> > +
> > +struct spacemit_gpio;
> > +
> > +struct spacemit_gpio_bank {
> > +	struct gpio_chip		gc;
> > +	struct spacemit_gpio		*sg;
> > +	void __iomem			*base;
> > +	u32				index;
> 
> You almost never use the index field.  It could easily be
> computed rather than stored:
> 
> static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> {
> 	return (u32)(gb - gb->sg->sgb);
> }
> 
ok
> > +	u32				irq_mask;
> > +	u32				irq_rising_edge;
> > +	u32				irq_falling_edge;
> > +};
> > +
> > +struct spacemit_gpio {
> > +	struct	device			*dev;
> > +	struct	spacemit_gpio_bank	sgb[NR_BANKS];
> > +};
> > +
> > +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> > +{
> > +	struct spacemit_gpio_bank *gb = dev_id;
> > +	unsigned long pending;
> > +	u32 n, gedr;
> > +
> > +	gedr = readl(gb->base + GEDR);
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +	writel(gedr, gb->base + GEDR);
> > +
> > +	gedr = gedr & gb->irq_mask;
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +
> > +	pending = gedr;
> 
> Instead, do:
> 
> 	pending = gedr & gb->irq_mask;
> 	if (!pending)
> 		return IRQ_NONE;
> 
good suggestion
> > +	for_each_set_bit(n, &pending, BITS_PER_LONG)
> > +		handle_nested_irq(irq_find_mapping(gb->gc.irq.domain, n));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void spacemit_gpio_irq_ack(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +
> > +	writel(BIT(irqd_to_hwirq(d)), gb->base + GEDR);
> > +}
> > +
> > +static void spacemit_gpio_irq_mask(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> > +	gb->irq_mask &= ~bit;
> 
> This is a minor suggestion, and I'm not sure how much difference
> it makes.  But here (and one or two more times below) you could
> avoid the writel() calls if you know the particular IRQ was
> already disabled.  (Maybe that won't ever happen?)
> 
pratically, this should be called once irq unmasked (in pair),
besides, it won't worth to do the optimization as not called frequently

> 	if (!(gb->irq_mask & bit))
> 		return;
> 
> 	gb->irq_mask &= !bit;
> 	...
> 
> This should work because in spacemit_gpio_add_bank() you reset
> all the IRQ state and disable all IRQs, so the cached copy of
> the irq_mask and the rising and falling edge masks should match
> reality.
> 
> > +
> > +	if (bit & gb->irq_rising_edge)
> > +		writel(bit, gb->base + GCRER);
> > +
> > +	if (bit & gb->irq_falling_edge)
> > +		writel(bit, gb->base + GCFER);
> > +}
> > +
> > +static void spacemit_gpio_irq_unmask(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> 
> 
> Same thought here.
> 
> 	if (gb->irq_mask & bit)
> 		return;
> 
> > +	gb->irq_mask |= bit;
> > +
> > +	if (bit & gb->irq_rising_edge)
> > +		writel(bit,  gb->base + GSRER);
> > +
> > +	if (bit & gb->irq_falling_edge)
> > +		writel(bit, gb->base + GSFER);
> > +}
> > +
> > +static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> 
> Same thought in this function, although it gets a little
> messier looking.
> 
> > +	if (type & IRQ_TYPE_EDGE_RISING) {
> > +		gb->irq_rising_edge |= bit;
> > +		writel(bit, gb->base + GSRER);
> > +	} else {
> > +		gb->irq_rising_edge &= ~bit;
> > +		writel(bit, gb->base + GCRER);
> > +	}
> 
> Otherwise:
> 
>      if (type & IRQ_TYPE_EDGE_RISING)
> 	gb->irq_rising_edge |= bit;
>      else
> 	gb->irq_rising_edge &= ~bit;
>      writel(bit, gb->base + GSRER);
> 
no, it's two different registers: GSRER vs GCRER
> and again below.
> 
> > +
> > +	if (type & IRQ_TYPE_EDGE_FALLING) {
> > +		gb->irq_falling_edge |= bit;
> > +		writel(bit, gb->base + GSFER);
> > +	} else {
> > +		gb->irq_falling_edge &= ~bit;
> > +		writel(bit, gb->base + GCFER);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> You added this function in version 5 of the series.  Please call
> attention to additions (or removals) like this in your cover page,
> and/or in notes at the top of this patch.
> 
ok
> > +static void spacemit_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> > +{
> > +	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(data);
> > +
> > +	seq_printf(p, "%s-%d", dev_name(gb->gc.parent), gb->index);
> 
> Does this look like "gpiochip2-15" or something?  I wasn't able
> to find it in the debugfs file system.
> 
it shows in /proc/interrupts once irq registered..
something will look like this - d4019000.gpio-$index

> > +}
> > +
> > +static struct irq_chip spacemit_gpio_chip = {
> > +	.name		= "k1-gpio-irqchip",
> > +	.irq_ack	= spacemit_gpio_irq_ack,
> > +	.irq_mask	= spacemit_gpio_irq_mask,
> > +	.irq_unmask	= spacemit_gpio_irq_unmask,
> > +	.irq_set_type	= spacemit_gpio_irq_set_type,
> > +	.irq_print_chip	= spacemit_gpio_irq_print_chip,
> > +	.flags		= IRQCHIP_IMMUTABLE,
> 
> Last time your flags value was IRQCHIP_SET_WAKE.  Why the change?
> 
I was about to check this..

the gpio controller doesn't support irq wake up,
I will add this flag in next version

> > +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > +};
> > +
> 
> Maybe you could add a comment indicating that gpiospec->args[]
> will contain:
> 0:  bank index
> 1:  GPIO offset within the bank
> 2:  flags
> 
> (And the GPIO chip instance number as Linus suggested.)
> 
please ignore, I will drop this function as LinusW promote this
to gpio core
> > +static int spacemit_gpio_xlate(struct gpio_chip *gc,
> > +			       const struct of_phandle_args *gpiospec, u32 *flags)
> > +{
> > +	struct spacemit_gpio_bank *gb = gpiochip_get_data(gc);
> > +	struct spacemit_gpio *sg = gb->sg;
> > +
> 
> Get rid of the above blank line.
> 
> > +	int i;
> > +
> 
> I'm not sure the context in which this runs.  Can it be given
> arbitrary content from a DTB?  Mainly I'm interested to know
> whether any of these checks can be eliminated.  If it's called
> while parsing a DTB I can see why you'd need to verify all
> input values for validity.
> 
> > +	if (gc->of_gpio_n_cells != 3)
> > +		return -EINVAL;
> > +
> > +	if (gpiospec->args_count < gc->of_gpio_n_cells)
> > +		return -EINVAL;
> > +
> > +	i = gpiospec->args[0];
> > +	if (i >= NR_BANKS)
> > +		return -EINVAL;
> > +
> > +	if (gc != &sg->sgb[i].gc)
> > +		return -EINVAL;
> > +
> > +	if (gpiospec->args[1] >= gc->ngpio)
> > +		return -EINVAL;
> > +
> > +	if (flags)
> > +		*flags = gpiospec->args[2];
> > +
> > +	return gpiospec->args[1];
> > +}
> > +
> > +static int spacemit_add_pin_range(struct gpio_chip *gc)
> > +{
> > +	struct spacemit_gpio_bank *gb;
> > +	struct of_phandle_args pinspec;
> > +	struct pinctrl_dev *pctldev;
> > +	struct device_node *np;
> > +	int ret, trim;
> > +
> > +	np = dev_of_node(&gc->gpiodev->dev);
> > +	if (!np)
> > +		return 0;
> > +
> > +	gb = to_spacemit_gpio_bank(gc);
> > +
> > +	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> > +					       gb->index, &pinspec);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pctldev = of_pinctrl_get(pinspec.np);
> > +	of_node_put(pinspec.np);
> > +	if (!pctldev)
> > +		return -EPROBE_DEFER;
> > +
> > +	/* Ignore ranges outside of this GPIO chip */
> > +	if (pinspec.args[0] >= (gc->offset + gc->ngpio))
> > +		return -EINVAL;
> > +
> > +	if (pinspec.args[0] + pinspec.args[2] <= gc->offset)
> > +		return -EINVAL;
> > +
> 
> I would do the following test earlier.
> 
ditto, ignore this, as move to gpio core
> > +	if (!pinspec.args[2])
> > +		return -EINVAL;
> > +
> > +	/* Trim the range to fit this GPIO chip */
> > +	if (gc->offset > pinspec.args[0]) {
> > +		trim = gc->offset - pinspec.args[0];
> > +		pinspec.args[2] -= trim;
> > +		pinspec.args[1] += trim;
> > +		pinspec.args[0] = 0;
> > +	} else {
> > +		pinspec.args[0] -= gc->offset;
> > +	}
> > +	if ((pinspec.args[0] + pinspec.args[2]) > gc->ngpio)
> > +		pinspec.args[2] = gc->ngpio - pinspec.args[0];
> > +
> > +	ret = gpiochip_add_pin_range(gc,
> > +				     pinctrl_dev_get_devname(pctldev),
> > +				     pinspec.args[0],
> > +				     pinspec.args[1],
> > +				     pinspec.args[2]);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> Just do this:
> 
>      return  gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev),
> 				   pinspec.args[0], pinspec.args[2]);
> 
> > +}
> > +
> > +static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
> > +				  void __iomem *regs,
> > +				  int index, int irq)
> > +{
> > +	struct spacemit_gpio_bank *gb = &sg->sgb[index];
> > +	struct gpio_chip *gc = &gb->gc;
> > +	struct device *dev = sg->dev;
> > +	struct gpio_irq_chip *girq;
> > +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> > +	int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
> > +
> > +	gb->index = index;
> > +	gb->base = regs + bank_base[index];
> > +
> > +	dat	= gb->base + GPLR;
> > +	set	= gb->base + GPSR;
> > +	clr	= gb->base + GPCR;
> > +	dirin	= gb->base + GCDR;
> > +	dirout	= gb->base + GSDR;
> > +
> > +	/* This registers 32 GPIO lines per bank */
> > +	ret = bgpio_init(gc, dev, 4, dat, set, clr, dirout, dirin,
> > +			 BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to init gpio chip\n");
> > +
> > +	gb->sg = sg;
> > +
> > +	gc->label		= dev_name(dev);
> > +	gc->request		= gpiochip_generic_request;
> > +	gc->free		= gpiochip_generic_free;
> > +	gc->ngpio		= NR_GPIOS_PER_BANK;
> > +	gc->base		= -1;
> > +
> > +#ifdef CONFIG_OF_GPIO
> 
> Why are these lines conditionally defined?  Is it intended
> to allow CONFIG_COMPILE_TEST to work?  Your Kconfig states
> that this *depends on* OF_GPIO, so this is probably not
> needed.
> 
> You don't define spacemit_gpio_xlate() earlier conditionally.
> Nor spacemit_add_pin_range().
> 
make sense, I will drop this #ifdef
> > +	gc->of_xlate		= spacemit_gpio_xlate;
> > +	gc->of_add_pin_range	= spacemit_add_pin_range;
> > +	gc->of_gpio_n_cells	= 3;
> > +#endif
> > +
> > +	girq			= &gc->irq;
> > +	girq->threaded		= true;
> > +	girq->handler		= handle_simple_irq;
> > +
> > +	gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
> > +
> > +	/* Clear Edge Detection Settings */
> > +	writel(0x0, gb->base + GRER);
> > +	writel(0x0, gb->base + GFER);
> > +	/* Clear and Disable Interrupt */
> > +	writel(0xffffffff, gb->base + GCFER);
> > +	writel(0xffffffff, gb->base + GCRER);
> 
> It seems that GAPMASK is an overall interrupt mask register.
> I assume that means that by writing 0 here, no interrupts
> of any kind will be generated for any of the 32 GPIO ports.
> 
yes

> If that's true, I would write this first, *then* disable
> the rising and falling edge detection interrupts, *then*
> clear any pending interrupts.
> 
ok, I will take your suggestion, this is more strict

> Are there any interrupt types other than rising and falling
> edge?  Does this just provide an atomic way to disable both
only two types, rising, falling, and both can be enabled simultaneously
(there is no level trigger interrupt)

> types at once?  If there are no other interrupt types maybe
> this could be used rather than disabling both types
> separately using GCFER etc. in spacemit_gpio_irq_*mask().
> 
you are right, I think we can do this

> 					-Alex
> 
> > +	writel(0, gb->base + GAPMASK);
> > +
> > +	ret = devm_request_threaded_irq(dev, irq, NULL,
> > +					spacemit_gpio_irq_handler,
> > +					IRQF_ONESHOT | IRQF_SHARED,
> > +					gb->gc.label, gb);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to register IRQ\n");
> > +
> > +	ret = devm_gpiochip_add_data(dev, gc, gb);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to add gpio chip\n");
> > +
> > +	/* Eable Interrupt */
> > +	writel(0xffffffff, gb->base + GAPMASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static int spacemit_gpio_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct spacemit_gpio *sg;
> > +	struct resource *res;
> > +	void __iomem *regs;
> > +	int i, irq, ret;
> > +
> > +	sg = devm_kzalloc(dev, sizeof(*sg), GFP_KERNEL);
> > +	if (!sg)
> > +		return -ENOMEM;
> > +
> > +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return irq;
> > +
> > +	sg->dev	= dev;
> > +
> > +	for (i = 0; i < NR_BANKS; i++) {
> > +		ret = spacemit_gpio_add_bank(sg, regs, i, irq);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id spacemit_gpio_dt_ids[] = {
> > +	{ .compatible = "spacemit,k1-gpio" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver spacemit_gpio_driver = {
> > +	.probe		= spacemit_gpio_probe,
> > +	.driver		= {
> > +		.name	= "k1-gpio",
> > +		.of_match_table = spacemit_gpio_dt_ids,
> > +	},
> > +};
> > +module_platform_driver(spacemit_gpio_driver);
> > +
> > +MODULE_AUTHOR("Yixun Lan <dlan@gentoo.org>");
> > +MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
> > +MODULE_LICENSE("GPL");
> > 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

[PATCH v5 4/5] riscv: dts: spacemit: add gpio support for K1 SoC

[Yixun Lan]

Populate the GPIO node in the device tree for SpacemiT K1 SoC.
Each of 32 pins will act as one bank and map pins to pinctrl controller.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi |  3 +++
 arch/riscv/boot/dts/spacemit/k1.dtsi         | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
index a8eac5517f8578d60cb45214589ccb45ac376b9a..283663647a86ff137917ced8bfe79a129c86342a 100644
--- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
@@ -7,6 +7,9 @@
 
 #define K1_PADCONF(pin, func) (((pin) << 16) | (func))
 
+/* Map GPIO pin to each bank's <index, offset> */
+#define K1_GPIO(x)	(x / 32) (x % 32)
+
 &pinctrl {
 	uart0_2_cfg: uart0-2-cfg {
 		uart0-2-pins {
diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index c670ebf8fa12917aa6493fcd89fdd1409529538b..d65ff76ead8cbe303412954c8abafbefecf8081e 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -404,6 +404,21 @@ uart9: serial@d4017800 {
 			status = "disabled";
 		};
 
+		gpio: gpio@d4019000 {
+			compatible = "spacemit,k1-gpio";
+			reg = <0x0 0xd4019000 0x0 0x100>;
+			gpio-controller;
+			#gpio-cells = <3>;
+			interrupts = <58>;
+			interrupt-parent = <&plic>;
+			interrupt-controller;
+			#interrupt-cells = <3>;
+			gpio-ranges = <&pinctrl 0 0 32>,
+				      <&pinctrl 0 32 32>,
+				      <&pinctrl 0 64 32>,
+				      <&pinctrl 0 96 32>;
+		};
+
 		pinctrl: pinctrl@d401e000 {
 			compatible = "spacemit,k1-pinctrl";
 			reg = <0x0 0xd401e000 0x0 0x400>;

-- 
2.48.1

[PATCH v5 5/5] riscv: dts: spacemit: add gpio LED for system heartbeat

[Yixun Lan]

Leverage GPIO to support system LED to indicate activity of CPUs.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 1d617b40a2d51ee464b57234d248798aeb218643..816ef1bc358ec490aff184d5915d680dbd9f00cb 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -17,6 +17,17 @@ aliases {
 	chosen {
 		stdout-path = "serial0";
 	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		led1 {
+			label = "sys-led";
+			gpios = <&gpio K1_GPIO(96) GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+			default-state = "on";
+		};
+	};
 };
 
 &uart0 {

-- 
2.48.1