[PATCH v4 0/4] riscv: spacemit: add gpio support for K1 SoC

[PATCH v4 0/4] 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, so all GPIO ports have
been organized as sub nodes in the device tree.

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

Note, this patch is based on 'for-next' branch of SpacemiT's SoC tree [4],
due to there is DT dependency.

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

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]
Link: https://github.com/spacemit-com/linux [4]
Signed-off-by: Yixun Lan <dlan@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 (4):
      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 | 116 ++++++++
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts    |  11 +
 arch/riscv/boot/dts/spacemit/k1.dtsi               |  55 ++++
 drivers/gpio/Kconfig                               |   7 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-spacemit-k1.c                    | 295 +++++++++++++++++++++
 6 files changed, 485 insertions(+)
---
base-commit: 3d72d603afa72082501e9076eed61e0531339ef8
change-id: 20240828-03-k1-gpio-61bf92f9032c

Best regards,
-- 
Yixun Lan

[PATCH v4 1/4] 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 ports, each consisting of 32 pins.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
 1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
@@ -0,0 +1,116 @@
+# 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.
+  Each port will be represented as child nodes with the generic
+  GPIO-controller properties in this bindings file.
+
+properties:
+  $nodename:
+    pattern: "^gpio@[0-9a-f]+$"
+
+  compatible:
+    const: spacemit,k1-gpio
+
+  reg:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^gpio-port@[0-9a-f]+$":
+    type: object
+    properties:
+      compatible:
+        const: spacemit,k1-gpio-port
+
+      reg:
+        maxItems: 1
+
+      gpio-controller: true
+
+      "#gpio-cells":
+        const: 2
+
+      gpio-ranges: true
+
+      interrupts:
+        maxItems: 1
+
+      interrupt-controller: true
+
+      "#interrupt-cells":
+        const: 2
+        description:
+          The first cell is the GPIO number, the second 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"
+
+    dependencies:
+      interrupt-controller: [ interrupts ]
+
+    additionalProperties: false
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    gpio: gpio@d4019000 {
+      compatible = "spacemit,k1-gpio";
+      reg = <0xd4019000 0x800>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      port0: gpio-port@0 {
+        compatible = "spacemit,k1-gpio-port";
+        reg = <0>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-parent = <&plic>;
+        interrupts = <58>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        gpio-ranges = <&pinctrl 0 0 32>;
+      };
+
+      port1: gpio-port@4 {
+        compatible = "spacemit,k1-gpio-port";
+        reg = <4>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-parent = <&plic>;
+        interrupts = <58>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        gpio-ranges = <&pinctrl 0 32 32>;
+      };
+    };
+...

-- 
2.48.0

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Olof Johansson]

Hi,

On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan 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 ports, each consisting of 32 pins.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
>  1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> @@ -0,0 +1,116 @@
> +# 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.
> +  Each port will be represented as child nodes with the generic
> +  GPIO-controller properties in this bindings file.

There's only one interrupt line for all ports, but you have a binding that
duplicates them for every set of ports. That seems overly complicated,
doesn't it? They'd all bind the same handler, so there's no benefit in
providing the flexibility,.

> +properties:
> +  $nodename:
> +    pattern: "^gpio@[0-9a-f]+$"
> +
> +  compatible:
> +    const: spacemit,k1-gpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +patternProperties:
> +  "^gpio-port@[0-9a-f]+$":
> +    type: object
> +    properties:
> +      compatible:
> +        const: spacemit,k1-gpio-port
> +
> +      reg:
> +        maxItems: 1
> +
> +      gpio-controller: true
> +
> +      "#gpio-cells":
> +        const: 2
> +
> +      gpio-ranges: true
> +
> +      interrupts:
> +        maxItems: 1
> +
> +      interrupt-controller: true
> +
> +      "#interrupt-cells":
> +        const: 2
> +        description:
> +          The first cell is the GPIO number, the second 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.

Same here, since there's no real flexibility between the banks, it might
make sense to consider a 3-cell GPIO specifier instead, and having
the first cell indicate bank. I could see this argument go in either
direction, but I'm not sure I understand why to provide a gpio-controller
per bank.

Comparing to say Rockchip, where each bank has a separate interrupt line
-- so there the granularity makes sense.


-Olof

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Olof:
 thanks for your reivew

On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> Hi,
> 
> On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan 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 ports, each consisting of 32 pins.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> >  1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > @@ -0,0 +1,116 @@
> > +# 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.
> > +  Each port will be represented as child nodes with the generic
> > +  GPIO-controller properties in this bindings file.
> 
> There's only one interrupt line for all ports, but you have a binding that
> duplicates them for every set of ports. That seems overly complicated,
> doesn't it? They'd all bind the same handler, so there's no benefit in
> providing the flexibility,.
> 
yes, all ports share same interrupt line, but each port has its own
irq related handling register, so it make sense to describe as per gpio irqchip

also see comments below

> > +properties:
> > +  $nodename:
> > +    pattern: "^gpio@[0-9a-f]+$"
> > +
> > +  compatible:
> > +    const: spacemit,k1-gpio
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^gpio-port@[0-9a-f]+$":
> > +    type: object
> > +    properties:
> > +      compatible:
> > +        const: spacemit,k1-gpio-port
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      gpio-controller: true
> > +
> > +      "#gpio-cells":
> > +        const: 2
> > +
> > +      gpio-ranges: true
> > +
> > +      interrupts:
> > +        maxItems: 1
> > +
> > +      interrupt-controller: true
> > +
> > +      "#interrupt-cells":
> > +        const: 2
> > +        description:
> > +          The first cell is the GPIO number, the second 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.
> 
> Same here, since there's no real flexibility between the banks, it might
> make sense to consider a 3-cell GPIO specifier instead, and having
how to handle the fourth gpio port? I would like to have uniform driver for all ports

> the first cell indicate bank. I could see this argument go in either
> direction, but I'm not sure I understand why to provide a gpio-controller
> per bank.
> 

IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
 while combining all four ports into one irqchip, which NACKed by maintainer[2].
 I tend to agree having a gpio-controller per bank provide more flexibility,
 easy to leverage generic gpio framework, even each port can be disabled or enabled,
 and IMO having shared irq handler isn't really a problem..

[1] https://lore.kernel.org/r/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org
[2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com
https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com


> Comparing to say Rockchip, where each bank has a separate interrupt line
> -- so there the granularity makes sense.
> 
> 
> -Olof

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Olof Johansson]

On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> Hi Olof:
>  thanks for your reivew
> 
> On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > Hi,
> > 
> > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan 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 ports, each consisting of 32 pins.
> > > 
> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > ---
> > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > >  1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > @@ -0,0 +1,116 @@
> > > +# 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.
> > > +  Each port will be represented as child nodes with the generic
> > > +  GPIO-controller properties in this bindings file.
> > 
> > There's only one interrupt line for all ports, but you have a binding that
> > duplicates them for every set of ports. That seems overly complicated,
> > doesn't it? They'd all bind the same handler, so there's no benefit in
> > providing the flexibility,.
> > 
> yes, all ports share same interrupt line, but each port has its own
> irq related handling register, so it make sense to describe as per gpio irqchip
> 
> also see comments below
> 
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "^gpio@[0-9a-f]+$"
> > > +
> > > +  compatible:
> > > +    const: spacemit,k1-gpio
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^gpio-port@[0-9a-f]+$":
> > > +    type: object
> > > +    properties:
> > > +      compatible:
> > > +        const: spacemit,k1-gpio-port
> > > +
> > > +      reg:
> > > +        maxItems: 1
> > > +
> > > +      gpio-controller: true
> > > +
> > > +      "#gpio-cells":
> > > +        const: 2
> > > +
> > > +      gpio-ranges: true
> > > +
> > > +      interrupts:
> > > +        maxItems: 1
> > > +
> > > +      interrupt-controller: true
> > > +
> > > +      "#interrupt-cells":
> > > +        const: 2
> > > +        description:
> > > +          The first cell is the GPIO number, the second 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.
> > 
> > Same here, since there's no real flexibility between the banks, it might
> > make sense to consider a 3-cell GPIO specifier instead, and having
> how to handle the fourth gpio port? I would like to have uniform driver for all ports
> 
> > the first cell indicate bank. I could see this argument go in either
> > direction, but I'm not sure I understand why to provide a gpio-controller
> > per bank.
> > 
> 
> IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
>  while combining all four ports into one irqchip, which NACKed by maintainer[2].
>  I tend to agree having a gpio-controller per bank provide more flexibility,
>  easy to leverage generic gpio framework, even each port can be disabled or enabled,
>  and IMO having shared irq handler isn't really a problem..
> 
> [1] https://lore.kernel.org/r/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org
> [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com
> https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com

Hmm, I don't understand the reasoning there, but it's not my subsystem.

It seems worse to me to misdescribe the hardware as separate blocks
with a device-tree binding that no longer describes the actual hardware,
but it's not up to me.

Let's get the platform support merged, ignore my feedback here -- we need more
SoCs supported upstream and this code is good enough to go in as-is.


-Olof

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Rob Herring]

On Thu, Jan 23, 2025 at 03:19:18PM -0800, Olof Johansson wrote:
> On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> > Hi Olof:
> >  thanks for your reivew
> > 
> > On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > > Hi,
> > > 
> > > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan 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 ports, each consisting of 32 pins.
> > > > 
> > > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > > ---
> > > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > > >  1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > @@ -0,0 +1,116 @@
> > > > +# 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.
> > > > +  Each port will be represented as child nodes with the generic
> > > > +  GPIO-controller properties in this bindings file.
> > > 
> > > There's only one interrupt line for all ports, but you have a binding that
> > > duplicates them for every set of ports. That seems overly complicated,
> > > doesn't it? They'd all bind the same handler, so there's no benefit in
> > > providing the flexibility,.
> > > 
> > yes, all ports share same interrupt line, but each port has its own
> > irq related handling register, so it make sense to describe as per gpio irqchip
> > 
> > also see comments below
> > 
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^gpio@[0-9a-f]+$"
> > > > +
> > > > +  compatible:
> > > > +    const: spacemit,k1-gpio
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +patternProperties:
> > > > +  "^gpio-port@[0-9a-f]+$":
> > > > +    type: object
> > > > +    properties:
> > > > +      compatible:
> > > > +        const: spacemit,k1-gpio-port
> > > > +
> > > > +      reg:
> > > > +        maxItems: 1
> > > > +
> > > > +      gpio-controller: true
> > > > +
> > > > +      "#gpio-cells":
> > > > +        const: 2
> > > > +
> > > > +      gpio-ranges: true
> > > > +
> > > > +      interrupts:
> > > > +        maxItems: 1
> > > > +
> > > > +      interrupt-controller: true
> > > > +
> > > > +      "#interrupt-cells":
> > > > +        const: 2
> > > > +        description:
> > > > +          The first cell is the GPIO number, the second 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.
> > > 
> > > Same here, since there's no real flexibility between the banks, it might
> > > make sense to consider a 3-cell GPIO specifier instead, and having
> > how to handle the fourth gpio port? I would like to have uniform driver for all ports
> > 
> > > the first cell indicate bank. I could see this argument go in either
> > > direction, but I'm not sure I understand why to provide a gpio-controller
> > > per bank.
> > > 
> > 
> > IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
> >  while combining all four ports into one irqchip, which NACKed by maintainer[2].
> >  I tend to agree having a gpio-controller per bank provide more flexibility,
> >  easy to leverage generic gpio framework, even each port can be disabled or enabled,
> >  and IMO having shared irq handler isn't really a problem..
> > 
> > [1] https://lore.kernel.org/r/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org
> > [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com
> > https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com
> 
> Hmm, I don't understand the reasoning there, but it's not my subsystem.
> 
> It seems worse to me to misdescribe the hardware as separate blocks
> with a device-tree binding that no longer describes the actual hardware,
> but it's not up to me.

I agree. It's clearly 1 block given the first 3 banks are interleaved.

If Linux can't handle 1 node for N gpio_chip's, then that's a Linux 
problem. Maybe it can, IDK. The lookup from a DT node to gpio_chip just 
needs to match on more than just DT node pointer, but look at the node 
ptr and arg cells.

Rob

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Rob:

On 12:17 Mon 27 Jan     , Rob Herring wrote:
> On Thu, Jan 23, 2025 at 03:19:18PM -0800, Olof Johansson wrote:
> > On Thu, Jan 23, 2025 at 11:30:42AM +0000, Yixun Lan wrote:
> > > Hi Olof:
> > >  thanks for your reivew
> > > 
> > > On 12:03 Wed 22 Jan     , Olof Johansson wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Jan 21, 2025 at 11:38:11AM +0800, Yixun Lan 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 ports, each consisting of 32 pins.
> > > > > 
> > > > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > > > ---
> > > > >  .../devicetree/bindings/gpio/spacemit,k1-gpio.yaml | 116 +++++++++++++++++++++
> > > > >  1 file changed, 116 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..dd9459061aecfcba84e6a3c5052fbcddf6c61150
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/gpio/spacemit,k1-gpio.yaml
> > > > > @@ -0,0 +1,116 @@
> > > > > +# 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.
> > > > > +  Each port will be represented as child nodes with the generic
> > > > > +  GPIO-controller properties in this bindings file.
> > > > 
> > > > There's only one interrupt line for all ports, but you have a binding that
> > > > duplicates them for every set of ports. That seems overly complicated,
> > > > doesn't it? They'd all bind the same handler, so there's no benefit in
> > > > providing the flexibility,.
> > > > 
> > > yes, all ports share same interrupt line, but each port has its own
> > > irq related handling register, so it make sense to describe as per gpio irqchip
> > > 
> > > also see comments below
> > > 
> > > > > +properties:
> > > > > +  $nodename:
> > > > > +    pattern: "^gpio@[0-9a-f]+$"
> > > > > +
> > > > > +  compatible:
> > > > > +    const: spacemit,k1-gpio
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#address-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +  "#size-cells":
> > > > > +    const: 0
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^gpio-port@[0-9a-f]+$":
> > > > > +    type: object
> > > > > +    properties:
> > > > > +      compatible:
> > > > > +        const: spacemit,k1-gpio-port
> > > > > +
> > > > > +      reg:
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      gpio-controller: true
> > > > > +
> > > > > +      "#gpio-cells":
> > > > > +        const: 2
> > > > > +
> > > > > +      gpio-ranges: true
> > > > > +
> > > > > +      interrupts:
> > > > > +        maxItems: 1
> > > > > +
> > > > > +      interrupt-controller: true
> > > > > +
> > > > > +      "#interrupt-cells":
> > > > > +        const: 2
> > > > > +        description:
> > > > > +          The first cell is the GPIO number, the second 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.
> > > > 
> > > > Same here, since there's no real flexibility between the banks, it might
> > > > make sense to consider a 3-cell GPIO specifier instead, and having
> > > how to handle the fourth gpio port? I would like to have uniform driver for all ports
> > > 
> > > > the first cell indicate bank. I could see this argument go in either
> > > > direction, but I'm not sure I understand why to provide a gpio-controller
> > > > per bank.
> > > > 
> > > 
> > > IIUC, your suggestion here was same as the implementation of patch v3 of this driver[1],
> > >  while combining all four ports into one irqchip, which NACKed by maintainer[2].
> > >  I tend to agree having a gpio-controller per bank provide more flexibility,
> > >  easy to leverage generic gpio framework, even each port can be disabled or enabled,
> > >  and IMO having shared irq handler isn't really a problem..
> > > 
> > > [1] https://lore.kernel.org/r/20241225-03-k1-gpio-v3-0-27bb7b441d62@gentoo.org
> > > [2] https://lore.kernel.org/r/CACRpkdZPD2C2iPwOX_kW1Ug8jVkdHhhc7iFycHtzj5LQ0XWNgQ@mail.gmail.com
> > > https://lore.kernel.org/r/CACRpkdYgGho=VQabonq4HccEiXBH2qM76K45oDaV1Jyi0xZ-YA@mail.gmail.com
> > 
> > Hmm, I don't understand the reasoning there, but it's not my subsystem.
> > 
> > It seems worse to me to misdescribe the hardware as separate blocks
> > with a device-tree binding that no longer describes the actual hardware,
> > but it's not up to me.
> 
> I agree. It's clearly 1 block given the first 3 banks are interleaved.
> 
yes, it's kind of weird hardware design, the first 3 gpio are register interleaved,
while the 4th has independent space

> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux 
> problem. Maybe it can, IDK. 
I haven't seen somthing like this to register 1 node for multi gpio_chips..
To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?

>The lookup from a DT node to gpio_chip just 
> needs to match on more than just DT node pointer, but look at the node 
> ptr and arg cells.
> 
IIUC, are you suggesting to add a index cells to match additional gpio bank?
so the underlying driver can still register 4 gpio chips?

               gpio: gpio@d4019000 {
                        compatible = "spacemit,k1-gpio";
                        reg = <0x0 0xd4019000 0x0 0x800>;
                        interrupt-controller;
			#interrupt-cells = <3>; // additional cell
                        gpio-controller;
                        #gpio-cells = <3>; // additional cell
			...
		};

on comsumer side, it will be something like this:
		gpios = <&gpio INDEX0 0 GPIO_ACTIVE_HIGH>;
		interrupts = <&gpio INDEX0 0 IRQ_TYPE_EDGE_RISING>;
(INDEX0 possiblely can be numeric vale (0, 1, 2, 3) or register base: 0x0 0x4 0x8 0x100)

one thing I'm not sure about how to map the pinctrl pins via "gpio-ranges" to each gpio_chip,
currently, in v4 verion, this info is populated via sub node of gpios (port1, port2 ...)

I will investigate more on this.. but need suggestion to know if I proceed at right direction

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:

> [Rob]
> > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > problem. Maybe it can, IDK.
>
> I haven't seen somthing like this to register 1 node for multi gpio_chips..
> To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?

For Linux we can call bgpio_init() three times and
devm_gpiochip_add_data() three times on the result and if we use the
approach with three cells (where the second is instance 0,1,2 and the
last one the offset 0..31) then it will work all just the same I guess?

foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;

for offset 7 on block 2 for example.

We need a custom xlate function I suppose.

It just has not been done that way before, everybody just did
2-cell GPIOs.

Yours,
Linus Walleij

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Rob Herring]

On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:
>
> > [Rob]
> > > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > > problem. Maybe it can, IDK.
> >
> > I haven't seen somthing like this to register 1 node for multi gpio_chips..
> > To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
>
> For Linux we can call bgpio_init() three times and
> devm_gpiochip_add_data() three times on the result and if we use the
> approach with three cells (where the second is instance 0,1,2 and the
> last one the offset 0..31) then it will work all just the same I guess?
>
> foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>
> for offset 7 on block 2 for example.
>
> We need a custom xlate function I suppose.
>
> It just has not been done that way before, everybody just did
> 2-cell GPIOs.

You can do either 3 cells or 2 cells splitting the 1st cell into
<bank><index>. I'm pretty sure we have some cases of the latter.

Rob

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Samuel Holland]

On 2025-01-28 10:58 AM, Rob Herring wrote:
> On Tue, Jan 28, 2025 at 10:03 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:
>>
>>> [Rob]
>>>> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
>>>> problem. Maybe it can, IDK.
>>>
>>> I haven't seen somthing like this to register 1 node for multi gpio_chips..
>>> To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
>>
>> For Linux we can call bgpio_init() three times and
>> devm_gpiochip_add_data() three times on the result and if we use the
>> approach with three cells (where the second is instance 0,1,2 and the
>> last one the offset 0..31) then it will work all just the same I guess?
>>
>> foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>>
>> for offset 7 on block 2 for example.
>>
>> We need a custom xlate function I suppose.
>>
>> It just has not been done that way before, everybody just did
>> 2-cell GPIOs.
> 
> You can do either 3 cells or 2 cells splitting the 1st cell into
> <bank><index>. I'm pretty sure we have some cases of the latter.

There is also at least one example of 3-cell GPIOs:

Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml

It supports controllers with varying numbers of pins per bank and banks in each
instance. Compared to the design described above, it shares a single gpio_chip
across all banks in a controller instance.

Regards,
Samuel

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

Hi Yixun,

On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:
>
> > [Rob]
> > > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > > problem. Maybe it can, IDK.
> >
> > I haven't seen somthing like this to register 1 node for multi gpio_chips..
> > To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
>
> For Linux we can call bgpio_init() three times and
> devm_gpiochip_add_data() three times on the result and if we use the
> approach with three cells (where the second is instance 0,1,2 and the
> last one the offset 0..31) then it will work all just the same I guess?
>
> foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>
> for offset 7 on block 2 for example.
>
> We need a custom xlate function I suppose.
>
> It just has not been done that way before, everybody just did
> 2-cell GPIOs.

does this approach work for you? I think it's the most diplomatic.

I'm sorry about the hopeless back-and-forth with the bindings, also
for contributing to the messy debate. I do want developers to feel
encouraged to contribute and not get stuck in too long debates.

Yours,
Linus Walleij

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

hi Linus

Thanks for the ping..

On 10:18 Thu 06 Feb     , Linus Walleij wrote:
> Hi Yixun,
> 
> On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:
> >
> > > [Rob]
> > > > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > > > problem. Maybe it can, IDK.
> > >
> > > I haven't seen somthing like this to register 1 node for multi gpio_chips..
> > > To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
> >
> > For Linux we can call bgpio_init() three times and
> > devm_gpiochip_add_data() three times on the result and if we use the
yes, even I've already done this in v4

> > approach with three cells (where the second is instance 0,1,2 and the
> > last one the offset 0..31) then it will work all just the same I guess?
> >
agree, I just need to connect dots.. parse dts & adjust the driver code

> > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
> >
> > for offset 7 on block 2 for example.
> >
> > We need a custom xlate function I suppose.
> >
> > It just has not been done that way before, everybody just did
> > 2-cell GPIOs.
> 
> does this approach work for you? I think it's the most diplomatic.
> 
I like the approach which make sense

> I'm sorry about the hopeless back-and-forth with the bindings, also
> for contributing to the messy debate. I do want developers to feel
> encouraged to contribute and not get stuck in too long debates.
> 
no problem, thanks for the encouragement..

I planed to go for the implementation, and raise any actual problem I
may find, but it turns out taking more time than I expected (some reason
to due long chinese new year holiday..)

> Yours,
> Linus Walleij

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Linus and DT maintainers:

On 10:18 Thu 06 Feb     , Linus Walleij wrote:
> Hi Yixun,
> 
> On Tue, Jan 28, 2025 at 5:03 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Tue, Jan 28, 2025 at 4:17 AM Yixun Lan <dlan@gentoo.org> wrote:
> >
> > > [Rob]
> > > > If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> > > > problem. Maybe it can, IDK.
> > >
> > > I haven't seen somthing like this to register 1 node for multi gpio_chips..
> > > To gpio/pinctrl maintainer (Linus Walleij), do you have suggestion on this?
> >
> > For Linux we can call bgpio_init() three times and
> > devm_gpiochip_add_data() three times on the result and if we use the
> > approach with three cells (where the second is instance 0,1,2 and the
> > last one the offset 0..31) then it will work all just the same I guess?
> >
both bgpio_init() and devm_gpiochip_add_data() operate on per "struct gpio_chip" bias,
which mean they need to request three independent gpio chips..

> > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
 which mean one gpio chip combine three banks.. I've looked at the sunxi driver which
Samuel pointed, imply same example as this.

if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
 to the <bank, offset> array in the underlying gpio driver

the v4 patch is very similar to drivers/gpio/gpio-dwapb.c

If had to choose the direction between v1 and v4, I personally would favor the latter,
 as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
 merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
 lots underlying generic gpio APIs, result in much simplified/clean code base..

> >
> > for offset 7 on block 2 for example.
> >
> > We need a custom xlate function I suppose.
> >
> > It just has not been done that way before, everybody just did
> > 2-cell GPIOs.
> 
> does this approach work for you? I think it's the most diplomatic.
> 
> I'm sorry about the hopeless back-and-forth with the bindings, also
> for contributing to the messy debate. I do want developers to feel
> encouraged to contribute and not get stuck in too long debates.
> 
> Yours,
> Linus Walleij

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote:

> > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
>
> if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
>  which mean one gpio chip combine three banks..

Not really: the fact that there is just one gpio node in the device
tree does not
mean that it needs to correspond to one single gpio_chip instance inside the
Linux kernel.

It's just what the current existing bindings and the code in the GPIO subsystem
assumes. It does not have to assume that: we can change it.

I'm sorry if this is not entirely intuitive :(

One node can very well spawn three gpio_chip instances, but it requires
some core changes. But I think it's the most elegant.

> if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
> then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
>  to the <bank, offset> array in the underlying gpio driver
>
> the v4 patch is very similar to drivers/gpio/gpio-dwapb.c
>
> If had to choose the direction between v1 and v4, I personally would favor the latter,
>  as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
>  merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
>  lots underlying generic gpio APIs, result in much simplified/clean code base..

So what I would suggest is a combination of the two.

One gpio node in the device tree, like the DT maintainers want it.

Three struct gpio_chip instances inside the driver, all three spawn from
that single gpio device, and from that single platform_device.

What we are suggesting is a three-cell phandle in the device tree:

foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>;
bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>;

Notice the new first cell which is 0 or 2.

The first one is what was previously called gpio 7.
The second one is what was 2*32+31 = gpio 95.

So internally in the driver it is easy to use the first cell (0 or 2) to map to
the right struct gpio_chip if you have it in your driver something like this:

struct spacemit_gpio {
    struct gpio_chip gcs[3];
...
};

struct spacemit_gpio *sg;
struct gpio_chip *gc;
int ret;

for (i = 0; i++; i < 3) {
     ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg);
     if (ret)
        return ret;
     gc = sg->gcs[i];
     .... do stuff with this instance ....
}

Callbacks etc should work as before.

Then these phandles needs to be properly translated, which is done with the
struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c
you will see that chip->of_xlate() is called to map the phandle cells
to a certain GPIO line).

In most cases, drivers do not assign the chip->of_xlate callback
(one exception is gpio-pxa.c) and then it is default-assigned to
of_gpio_simple_xlate() which you can find in gpiolib-of.c as well.

You need to copy this callback to your driver and augment it
properly.

The xlate callback is used to locate the struct gpio_chip and
struct gpio_device as well, by just calling the xlate callback, so if
you code up the right xlate callback, everything should just
work by itself.

this is a guess on what it would look like (just dry coding,
but hopefully the idea works!):

static int spacemit_gpio_xlate(struct gpio_chip *gc,
                                const struct of_phandle_args *gpiospec,
                                u32 *flags)
{
        struct spacemit_gpio *sg = gpiochip_get_data(gc);
        int i;

        if (gc->of_gpio_n_cells != 3)
                return -EINVAL;

        if (gpiospec->args_count < gc->of_gpio_n_cells)
                return -EINVAL;

        /* We support maximum 3 gpio_chip instances */
        i = gpiospec->args[0];
        if (i >= 3)
                return -EINVAL;

        /* OK is this the right gpio_chip out of the three ? */
        if (gc != sg->gcs[i])
                return -EINVAL;

        /* Are we in range for this GPIO chip */
        if (gpiospec->args[1] >= gc->ngpio)
                return -EINVAL;

        if (flags)
                *flags = gpiospec->args[2];

        /* Return the hw index */
        return gpiospec->args[1];
}

...
gc->of_gpio_n_cells = 3;
gc->of_xlate = spacemit_gpio_xlate;

If it works as I hope, this will make the code in gpiolib-of.c in
of_find_gpio_device_by_xlate() calling gpio_device_find()
(which will iterate over all registered gpio_chips and then
of_gpiochip_match_node_and_xlate() will call this custom function
to see if it's the right one and return > 0 when we have the right
chip.

This should work for gpios *only*. When we then come to irqs,
these assume (see gpiolib.c) that we are using
irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so
you either need to roll your own irqchip code or we should fix
the core (I can help with this if the above works).

Several gpio chips use their own domain translation outside
of the gpiolib so you can use this as an intermediate step:
git grep irq_domain_ops drivers/gpio/
... but if you get here, let's patch the core to deal with custom
irqdomain xlate functions in the same manner as above.

I hope this isn't terribly unclear or complicated?
Otherwise tell me and I will try to ... explain more or give
up and say you can use a single 96-pin gpio_chip.

Yours,
Linus Walleij

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Linus:

On 14:07 Thu 13 Feb     , Linus Walleij wrote:
> On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote:
> 
> > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
> >
> > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
> >  which mean one gpio chip combine three banks..
> 
> Not really: the fact that there is just one gpio node in the device
> tree does not
> mean that it needs to correspond to one single gpio_chip instance inside the
> Linux kernel.
> 
> It's just what the current existing bindings and the code in the GPIO subsystem
> assumes. It does not have to assume that: we can change it.
> 
> I'm sorry if this is not entirely intuitive :(
> 
> One node can very well spawn three gpio_chip instances, but it requires
> some core changes. But I think it's the most elegant.
> 
> > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
> > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
> >  to the <bank, offset> array in the underlying gpio driver
> >
> > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c
> >
> > If had to choose the direction between v1 and v4, I personally would favor the latter,
> >  as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
> >  merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
> >  lots underlying generic gpio APIs, result in much simplified/clean code base..
> 
> So what I would suggest is a combination of the two.
> 
> One gpio node in the device tree, like the DT maintainers want it.
> 
> Three struct gpio_chip instances inside the driver, all three spawn from
> that single gpio device, and from that single platform_device.
> 
> What we are suggesting is a three-cell phandle in the device tree:
> 
> foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>;
> bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>;
> 
> Notice the new first cell which is 0 or 2.
> 
> The first one is what was previously called gpio 7.
> The second one is what was 2*32+31 = gpio 95.
> 
> So internally in the driver it is easy to use the first cell (0 or 2) to map to
> the right struct gpio_chip if you have it in your driver something like this:
> 
> struct spacemit_gpio {
>     struct gpio_chip gcs[3];
> ...
> };
> 
> struct spacemit_gpio *sg;
> struct gpio_chip *gc;
> int ret;
> 
> for (i = 0; i++; i < 3) {
>      ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg);
>      if (ret)
>         return ret;
>      gc = sg->gcs[i];
>      .... do stuff with this instance ....
> }
> 
> Callbacks etc should work as before.
> 
> Then these phandles needs to be properly translated, which is done with the
> struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c
> you will see that chip->of_xlate() is called to map the phandle cells
> to a certain GPIO line).
> 
> In most cases, drivers do not assign the chip->of_xlate callback
> (one exception is gpio-pxa.c) and then it is default-assigned to
> of_gpio_simple_xlate() which you can find in gpiolib-of.c as well.
> 
> You need to copy this callback to your driver and augment it
> properly.
> 
> The xlate callback is used to locate the struct gpio_chip and
> struct gpio_device as well, by just calling the xlate callback, so if
> you code up the right xlate callback, everything should just
> work by itself.
> 
> this is a guess on what it would look like (just dry coding,
> but hopefully the idea works!):
> 
> static int spacemit_gpio_xlate(struct gpio_chip *gc,
>                                 const struct of_phandle_args *gpiospec,
>                                 u32 *flags)
> {
>         struct spacemit_gpio *sg = gpiochip_get_data(gc);
>         int i;
> 
>         if (gc->of_gpio_n_cells != 3)
>                 return -EINVAL;
> 
>         if (gpiospec->args_count < gc->of_gpio_n_cells)
>                 return -EINVAL;
> 
>         /* We support maximum 3 gpio_chip instances */
>         i = gpiospec->args[0];
>         if (i >= 3)
>                 return -EINVAL;
> 
>         /* OK is this the right gpio_chip out of the three ? */
>         if (gc != sg->gcs[i])
>                 return -EINVAL;
> 
>         /* Are we in range for this GPIO chip */
>         if (gpiospec->args[1] >= gc->ngpio)
>                 return -EINVAL;
> 
>         if (flags)
>                 *flags = gpiospec->args[2];
> 
>         /* Return the hw index */
>         return gpiospec->args[1];
> }
> 
thanks for this very detail prototype! it works mostly, with one problem:

how to map gpio correctly to the pin from pinctrl subsystem?

for example, I specify gpio-ranges in dts, then 
                gpio0: gpio@d4019000 {
                        compatible = "spacemit,k1-gpio";
                        reg = <0x0 0xd4019000 0x0 0x100>;
			...
                        gpio-ranges = <&pinctrl 0 0 96>;
                };

		foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;

It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28

Probably there is something I missed...
> ...
> gc->of_gpio_n_cells = 3;
> gc->of_xlate = spacemit_gpio_xlate;
> 
> If it works as I hope, this will make the code in gpiolib-of.c in
> of_find_gpio_device_by_xlate() calling gpio_device_find()
> (which will iterate over all registered gpio_chips and then
> of_gpiochip_match_node_and_xlate() will call this custom function
> to see if it's the right one and return > 0 when we have the right
> chip.
> 
> This should work for gpios *only*. When we then come to irqs,
> these assume (see gpiolib.c) that we are using
> irq_domain_xlate_twocell() when using GPIOLIB_IRQCHIP, so
> you either need to roll your own irqchip code or we should fix
Sounds I should implement something like irq_domain_xlate_threecell()?

> the core (I can help with this if the above works).
> 
> Several gpio chips use their own domain translation outside
> of the gpiolib so you can use this as an intermediate step:
> git grep irq_domain_ops drivers/gpio/
..
> ... but if you get here, let's patch the core to deal with custom
> irqdomain xlate functions in the same manner as above.
> 
I like this direction, but how we should proceed?

> I hope this isn't terribly unclear or complicated?
> Otherwise tell me and I will try to ... explain more or give
> up and say you can use a single 96-pin gpio_chip.
> 
Let's try first, sounds it's a feasible way.

Many thanks!

> Yours,
> Linus Walleij

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Linus:

On 11:54 Fri 14 Feb     , Yixun Lan wrote:
> Hi Linus:
> 
> On 14:07 Thu 13 Feb     , Linus Walleij wrote:
> > On Thu, Feb 6, 2025 at 2:32 PM Yixun Lan <dlan@gentoo.org> wrote:
> > 
> > > > > foo-gpios <&gpio 2 7 GPIO_ACTIVE_LOW>;
> > >
> > > if we model the dts as above, then "&gpio" will register itself as one sole "struct gpio_chip",
> > >  which mean one gpio chip combine three banks..
> > 
> > Not really: the fact that there is just one gpio node in the device
> > tree does not
> > mean that it needs to correspond to one single gpio_chip instance inside the
> > Linux kernel.
> > 
> > It's just what the current existing bindings and the code in the GPIO subsystem
> > assumes. It does not have to assume that: we can change it.
> > 
> > I'm sorry if this is not entirely intuitive :(
> > 
> > One node can very well spawn three gpio_chip instances, but it requires
> > some core changes. But I think it's the most elegant.
> > 
> > > if taking "one gpio chip support multi banks" direction, then it will be reverted back as patch V1,
> > > then, even the three gpio-cells model is unnecessary needed, as we can map gpio number
> > >  to the <bank, offset> array in the underlying gpio driver
> > >
> > > the v4 patch is very similar to drivers/gpio/gpio-dwapb.c
> > >
> > > If had to choose the direction between v1 and v4, I personally would favor the latter,
> > >  as from hw perspective, each gpio bank is quite indepedent - has its own io/irq registers,
> > >  merely has interleaved io memory space, one shared IRQ line.. also the patch v4 leverage
> > >  lots underlying generic gpio APIs, result in much simplified/clean code base..
> > 
> > So what I would suggest is a combination of the two.
> > 
> > One gpio node in the device tree, like the DT maintainers want it.
> > 
> > Three struct gpio_chip instances inside the driver, all three spawn from
> > that single gpio device, and from that single platform_device.
> > 
> > What we are suggesting is a three-cell phandle in the device tree:
> > 
> > foo-gpios = <&gpio 0 7 GPIO_ACTIVE_HIGH>;
> > bar-gpios = <&gpio 2 31 GPIO_ACTIVE_HIGH>;
> > 
> > Notice the new first cell which is 0 or 2.
> > 
> > The first one is what was previously called gpio 7.
> > The second one is what was 2*32+31 = gpio 95.
> > 
> > So internally in the driver it is easy to use the first cell (0 or 2) to map to
> > the right struct gpio_chip if you have it in your driver something like this:
> > 
> > struct spacemit_gpio {
> >     struct gpio_chip gcs[3];
> > ...
> > };
> > 
> > struct spacemit_gpio *sg;
> > struct gpio_chip *gc;
> > int ret;
> > 
> > for (i = 0; i++; i < 3) {
> >      ret = devm_gpiochip_add_data(dev, &sg->gcs[i], sg);
> >      if (ret)
> >         return ret;
> >      gc = sg->gcs[i];
> >      .... do stuff with this instance ....
> > }
> > 
> > Callbacks etc should work as before.
> > 
> > Then these phandles needs to be properly translated, which is done with the
> > struct gpio_chip .of_xlate() callback. (If you look inside gpiolib-of.c
> > you will see that chip->of_xlate() is called to map the phandle cells
> > to a certain GPIO line).
> > 
> > In most cases, drivers do not assign the chip->of_xlate callback
> > (one exception is gpio-pxa.c) and then it is default-assigned to
> > of_gpio_simple_xlate() which you can find in gpiolib-of.c as well.
> > 
> > You need to copy this callback to your driver and augment it
> > properly.
> > 
> > The xlate callback is used to locate the struct gpio_chip and
> > struct gpio_device as well, by just calling the xlate callback, so if
> > you code up the right xlate callback, everything should just
> > work by itself.
> > 
> > this is a guess on what it would look like (just dry coding,
> > but hopefully the idea works!):
> > 
> > static int spacemit_gpio_xlate(struct gpio_chip *gc,
> >                                 const struct of_phandle_args *gpiospec,
> >                                 u32 *flags)
> > {
> >         struct spacemit_gpio *sg = gpiochip_get_data(gc);
> >         int i;
> > 
> >         if (gc->of_gpio_n_cells != 3)
> >                 return -EINVAL;
> > 
> >         if (gpiospec->args_count < gc->of_gpio_n_cells)
> >                 return -EINVAL;
> > 
> >         /* We support maximum 3 gpio_chip instances */
> >         i = gpiospec->args[0];
> >         if (i >= 3)
> >                 return -EINVAL;
> > 
> >         /* OK is this the right gpio_chip out of the three ? */
> >         if (gc != sg->gcs[i])
> >                 return -EINVAL;
> > 
> >         /* Are we in range for this GPIO chip */
> >         if (gpiospec->args[1] >= gc->ngpio)
> >                 return -EINVAL;
> > 
> >         if (flags)
> >                 *flags = gpiospec->args[2];
> > 
> >         /* Return the hw index */
> >         return gpiospec->args[1];
> > }
> > 
> thanks for this very detail prototype! it works mostly, with one problem:
> 
> how to map gpio correctly to the pin from pinctrl subsystem?
> 
> for example, I specify gpio-ranges in dts, then 
>                 gpio0: gpio@d4019000 {
>                         compatible = "spacemit,k1-gpio";
>                         reg = <0x0 0xd4019000 0x0 0x100>;
> 			...
>                         gpio-ranges = <&pinctrl 0 0 96>;
>                 };
> 
> 		foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;
> 
> It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28
> 
> Probably there is something I missed...
to make the gpio part work, we need additional custom gpio-ranges parser,
which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c
(at least gpio core need to adjust to call custom this function)

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

On Fri, Feb 14, 2025 at 12:54 PM Yixun Lan <dlan@gentoo.org> wrote:

> thanks for this very detail prototype! it works mostly, with one problem:
>
> how to map gpio correctly to the pin from pinctrl subsystem?
>
> for example, I specify gpio-ranges in dts, then
>                 gpio0: gpio@d4019000 {
>                         compatible = "spacemit,k1-gpio";
>                         reg = <0x0 0xd4019000 0x0 0x100>;
>                         ...
>                         gpio-ranges = <&pinctrl 0 0 96>;
>                 };
>
>                 foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;
>
> It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28
>
> Probably there is something I missed...

No it's just me missing the complexity!

> to make the gpio part work, we need additional custom gpio-ranges parser,
> which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c
> (at least gpio core need to adjust to call custom this function)

Let me send a patch set to bring threecell into the core instead,
and see if it works for you!

I will post it real soon.

Yours,
Linus Walleij

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Linus:

On 10:44 Tue 18 Feb     , Linus Walleij wrote:
> On Fri, Feb 14, 2025 at 12:54 PM Yixun Lan <dlan@gentoo.org> wrote:
> 
> > thanks for this very detail prototype! it works mostly, with one problem:
> >
> > how to map gpio correctly to the pin from pinctrl subsystem?
> >
> > for example, I specify gpio-ranges in dts, then
> >                 gpio0: gpio@d4019000 {
> >                         compatible = "spacemit,k1-gpio";
> >                         reg = <0x0 0xd4019000 0x0 0x100>;
> >                         ...
> >                         gpio-ranges = <&pinctrl 0 0 96>;
> >                 };
> >
> >                 foo-gpios = <&gpio0 2 28 GPIO_ACTIVE_LOW>;
> >
> > It should get GPIO_92 ( 92 = 2 * 32 + 28), but turns out GPIO_28
> >
> > Probably there is something I missed...
> 
> No it's just me missing the complexity!
> 
> > to make the gpio part work, we need additional custom gpio-ranges parser,
> > which should similar to of_gpiochip_add_pin_range() in gpiolib-of.c
> > (at least gpio core need to adjust to call custom this function)
> 
> Let me send a patch set to bring threecell into the core instead,
> and see if it works for you!
> 
> I will post it real soon.
> 
can you check the v5 of the patch here [1]? which I just sent out yesterday
it does 1) implement xlate() 2) instroduce custom add_pin_page()
the gpio part works as I tested, the gpio irq probably need more testing

> Yours,
> Linus Walleij

[1] https://lore.kernel.org/spacemit/20250217-03-k1-gpio-v5-0-2863ec3e7b67@gentoo.org/

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

On Tue, Feb 18, 2025 at 10:55 AM Yixun Lan <dlan@gentoo.org> wrote:

> > I will post it real soon.
> >
> can you check the v5 of the patch here [1]? which I just sent out yesterday
> it does 1) implement xlate() 2) instroduce custom add_pin_page()
> the gpio part works as I tested, the gpio irq probably need more testing

Ah nice! I have the same idea, but I just bring all the stuff you
need to reimplement in your driver into the core instead.

Your driver and bindings will look the same, you will just do
not need to reimplement the translation functions (if my code
works as I intended...)

Yours,
Linus Walleij

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Linus:

On 11:17 Tue 18 Feb     , Linus Walleij wrote:
> On Tue, Feb 18, 2025 at 10:55 AM Yixun Lan <dlan@gentoo.org> wrote:
> 
> > > I will post it real soon.
> > >
> > can you check the v5 of the patch here [1]? which I just sent out yesterday
> > it does 1) implement xlate() 2) instroduce custom add_pin_page()
> > the gpio part works as I tested, the gpio irq probably need more testing
> 
> Ah nice! I have the same idea, but I just bring all the stuff you
> need to reimplement in your driver into the core instead.
> 
> Your driver and bindings will look the same, you will just do
> not need to reimplement the translation functions (if my code
> works as I intended...)
> 
great! I will test and let you know if it works, many thanks..

> Yours,
> Linus Walleij

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

Re: [PATCH v4 1/4] dt-bindings: gpio: spacemit: add support for K1 SoC

[Linus Walleij]

On Mon, Jan 27, 2025 at 7:17 PM Rob Herring <robh@kernel.org> wrote:
> [Olof]
> > It seems worse to me to misdescribe the hardware as separate blocks
> > with a device-tree binding that no longer describes the actual hardware,
> > but it's not up to me.
>
> I agree. It's clearly 1 block given the first 3 banks are interleaved.
>
> If Linux can't handle 1 node for N gpio_chip's, then that's a Linux
> problem. Maybe it can, IDK. The lookup from a DT node to gpio_chip just
> needs to match on more than just DT node pointer, but look at the node
> ptr and arg cells.

Any operating system benefits from modeling the GPIOs such that
one set of 32bit registers [r0, r1 .. rn] becomes a discrete entity for
the OS.

Reasoning: any OS will want to be able to control several lines in
a single hardware operation, such as a register write, for example
to shake a clock and data line with a single write_to_register()
operation. If the hardware is described in chunks of 32 bit registers,
this is easy - Data Out Register, Data In Register, Direction
Register n bits, if an multiple-write/read operation hits this entity, we
know it can be handled with a single register write or read.

Yes, the same can be achieved by hardcoding this split into the
driver. But having the binding like such encourages it.

foo-gpios = <&gpio2 0>, <&gpio2 7>;

both need to be set high at outset, well they are in the same
entity and controlled by a single register, so (+/- overhead):

fooreg = fooreg | (1 << 0) | (1 << 7);

I agree this hardware is harder to classify as such since the blocks
share a single IRQ line - if they had individual IRQ lines it would be
a done deal, they are subblocks - yet shared IRQ lines are not *that*
uncommon.

Does this modeling reflect how the hardware actually looks? Likely.

The way GPIOs are built up from silicon io-cells are not that complex.
All the 32 bits from the set of registers will be routed to consecutive
pins, looking at the pin layout of the SoC would confirm if subsequent
bits map to subsequent pins.

Yours,
Linus Walleij

[PATCH v4 2/4] 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            |   7 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-spacemit-k1.c | 295 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 303 insertions(+)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 56fee58e281e7cac7f287eb04e4c17a17f75ed04..c153f5439649da020ee42c38e88cb8df31a8e307 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -654,6 +654,13 @@ 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
+	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..de0491af494c10f528095efee9b3a140bdcc0b0d
--- /dev/null
+++ b/drivers/gpio/gpio-spacemit-k1.c
@@ -0,0 +1,295 @@
+// 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/property.h>
+#include <linux/seq_file.h>
+#include <linux/module.h>
+
+/* register offset */
+#define GPLR		0x00
+#define GPDR		0x0c
+#define GPSR		0x18
+#define GPCR		0x24
+#define GRER		0x30
+#define GFER		0x3c
+#define GEDR		0x48
+#define GSDR		0x54
+#define GCDR		0x60
+#define GSRER		0x6c
+#define GCRER		0x78
+#define GSFER		0x84
+#define GCFER		0x90
+#define GAPMASK		0x9c
+#define GCPMASK		0xa8
+
+#define K1_BANK_GPIO_NUMBER	(32)
+
+#define to_spacemit_gpio_port(x) container_of((x), struct spacemit_gpio_port, gc)
+
+struct spacemit_gpio;
+
+struct spacemit_gpio_port {
+	struct gpio_chip		gc;
+	struct spacemit_gpio		*gpio;
+	struct fwnode_handle		*fwnode;
+	void __iomem			*base;
+	int				irq;
+	u32				irq_mask;
+	u32				irq_rising_edge;
+	u32				irq_falling_edge;
+	u32				index;
+};
+
+struct spacemit_gpio {
+	struct	device			*dev;
+	struct spacemit_gpio_port	*ports;
+	u32				nr_ports;
+};
+
+static inline void spacemit_clear_edge_detection(struct spacemit_gpio_port *port, u32 bit)
+{
+	writel(bit, port->base + GCRER);
+	writel(bit, port->base + GCFER);
+}
+
+static inline void spacemit_set_edge_detection(struct spacemit_gpio_port *port, u32 bit)
+{
+	writel(bit & port->irq_rising_edge,  port->base + GSRER);
+	writel(bit & port->irq_falling_edge, port->base + GSFER);
+}
+
+static inline void spacemit_reset_edge_detection(struct spacemit_gpio_port *port)
+{
+	writel(0xffffffff, port->base + GCFER);
+	writel(0xffffffff, port->base + GCRER);
+	writel(0xffffffff, port->base + GAPMASK);
+}
+
+static int spacemit_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	if (type & IRQ_TYPE_EDGE_RISING) {
+		port->irq_rising_edge |= bit;
+		writel(bit, port->base + GSRER);
+	} else {
+		port->irq_rising_edge &= ~bit;
+		writel(bit, port->base + GCRER);
+	}
+
+	if (type & IRQ_TYPE_EDGE_FALLING) {
+		port->irq_falling_edge |= bit;
+		writel(bit, port->base + GSFER);
+	} else {
+		port->irq_falling_edge &= ~bit;
+		writel(bit, port->base + GCFER);
+	}
+
+	return 0;
+}
+
+static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
+{
+	struct spacemit_gpio_port *port = dev_id;
+	unsigned long pending;
+	u32 n, gedr;
+
+	gedr = readl(port->base + GEDR);
+	if (!gedr)
+		return IRQ_NONE;
+
+	writel(gedr, port->base + GEDR);
+	gedr = gedr & port->irq_mask;
+
+	if (!gedr)
+		return IRQ_NONE;
+
+	pending = gedr;
+
+	for_each_set_bit(n, &pending, BITS_PER_LONG)
+		handle_nested_irq(irq_find_mapping(port->gc.irq.domain, n));
+
+	return IRQ_HANDLED;
+}
+
+static void spacemit_ack_muxed_gpio(struct irq_data *d)
+{
+	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
+
+	writel(BIT(irqd_to_hwirq(d)), port->base + GEDR);
+}
+
+static void spacemit_mask_muxed_gpio(struct irq_data *d)
+{
+	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	port->irq_mask &= ~bit;
+
+	spacemit_clear_edge_detection(port, bit);
+}
+
+static void spacemit_unmask_muxed_gpio(struct irq_data *d)
+{
+	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
+	u32 bit = BIT(irqd_to_hwirq(d));
+
+	port->irq_mask |= bit;
+
+	spacemit_set_edge_detection(port, bit);
+}
+
+static void spacemit_irq_print_chip(struct irq_data *data, struct seq_file *p)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct spacemit_gpio_port *port = to_spacemit_gpio_port(gc);
+
+	seq_printf(p, "%s-%d", dev_name(gc->parent), port->index);
+}
+
+static struct irq_chip spacemit_muxed_gpio_chip = {
+	.name		= "k1-gpio-irqchip",
+	.irq_ack	= spacemit_ack_muxed_gpio,
+	.irq_mask	= spacemit_mask_muxed_gpio,
+	.irq_unmask	= spacemit_unmask_muxed_gpio,
+	.irq_set_type	= spacemit_gpio_irq_type,
+	.irq_print_chip	= spacemit_irq_print_chip,
+	.flags		= IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int spacemit_gpio_get_ports(struct device *dev, struct spacemit_gpio *gpio,
+				   void __iomem *regs)
+{
+	struct spacemit_gpio_port *port;
+	u32 i = 0, offset;
+
+	gpio->nr_ports = device_get_child_node_count(dev);
+	if (gpio->nr_ports == 0)
+		return -ENODEV;
+
+	gpio->ports = devm_kcalloc(dev, gpio->nr_ports, sizeof(*gpio->ports), GFP_KERNEL);
+	if (!gpio->ports)
+		return -ENOMEM;
+
+	device_for_each_child_node_scoped(dev, fwnode)  {
+		port		= &gpio->ports[i];
+		port->fwnode	= fwnode;
+		port->index	= i++;
+
+		if (fwnode_property_read_u32(fwnode, "reg", &offset))
+			return -EINVAL;
+
+		port->base	= regs + offset;
+		port->irq	= fwnode_irq_get(fwnode, 0);
+	}
+
+	return 0;
+}
+
+static int spacemit_gpio_add_port(struct spacemit_gpio *gpio, int index)
+{
+	struct spacemit_gpio_port *port;
+	struct device *dev = gpio->dev;
+	struct gpio_irq_chip *girq;
+	void __iomem *dat, *set, *clr, *dirin, *dirout;
+	int ret;
+
+	port = &gpio->ports[index];
+	port->gpio = gpio;
+
+	dat	= port->base + GPLR;
+	set	= port->base + GPSR;
+	clr	= port->base + GPCR;
+	dirin	= port->base + GCDR;
+	dirout	= port->base + GSDR;
+
+	/* This registers 32 GPIO lines per port */
+	ret = bgpio_init(&port->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 for port\n");
+
+	port->gc.label		= dev_name(dev);
+	port->gc.fwnode		= port->fwnode;
+	port->gc.request	= gpiochip_generic_request;
+	port->gc.free		= gpiochip_generic_free;
+	port->gc.ngpio		= K1_BANK_GPIO_NUMBER;
+	port->gc.base		= -1;
+
+	girq			= &port->gc.irq;
+	girq->threaded		= true;
+	girq->handler		= handle_simple_irq;
+
+	gpio_irq_chip_set_chip(girq, &spacemit_muxed_gpio_chip);
+
+	spacemit_reset_edge_detection(port);
+
+	ret = devm_request_threaded_irq(dev, port->irq, NULL,
+					spacemit_gpio_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED,
+					port->gc.label, port);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to request IRQ\n");
+
+	return devm_gpiochip_add_data(dev, &port->gc, port);
+}
+
+static int spacemit_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct spacemit_gpio *gpio;
+	struct resource *res;
+	void __iomem *regs;
+	int i, ret;
+
+	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->dev = dev;
+
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	ret = spacemit_gpio_get_ports(dev, gpio, regs);
+	if (ret)
+		return dev_err_probe(dev, ret, "fail to get gpio ports\n");
+
+	for (i = 0; i < gpio->nr_ports; i++) {
+		ret = spacemit_gpio_add_port(gpio, i);
+		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.0

Re: [PATCH v4 2/4] gpio: spacemit: add support for K1 SoC

[Yixun Lan]

On 11:38 Tue 21 Jan     , 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>
> ---
>  drivers/gpio/Kconfig            |   7 +
>  drivers/gpio/Makefile           |   1 +
>  drivers/gpio/gpio-spacemit-k1.c | 295 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 303 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7cac7f287eb04e4c17a17f75ed04..c153f5439649da020ee42c38e88cb8df31a8e307 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -654,6 +654,13 @@ 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
> +	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..de0491af494c10f528095efee9b3a140bdcc0b0d
> --- /dev/null
> +++ b/drivers/gpio/gpio-spacemit-k1.c
> @@ -0,0 +1,295 @@
> +// 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>
> + */
> +
omit ..
> +static int spacemit_gpio_get_ports(struct device *dev, struct spacemit_gpio *gpio,
> +				   void __iomem *regs)
> +{
> +	struct spacemit_gpio_port *port;
> +	u32 i = 0, offset;
> +
> +	gpio->nr_ports = device_get_child_node_count(dev);
> +	if (gpio->nr_ports == 0)
> +		return -ENODEV;
> +
> +	gpio->ports = devm_kcalloc(dev, gpio->nr_ports, sizeof(*gpio->ports), GFP_KERNEL);
> +	if (!gpio->ports)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node_scoped(dev, fwnode)  {
> +		port		= &gpio->ports[i];
> +		port->fwnode	= fwnode;
> +		port->index	= i++;
> +
> +		if (fwnode_property_read_u32(fwnode, "reg", &offset))
> +			return -EINVAL;
> +
> +		port->base	= regs + offset;
> +		port->irq	= fwnode_irq_get(fwnode, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int spacemit_gpio_add_port(struct spacemit_gpio *gpio, int index)
> +{
> +	struct spacemit_gpio_port *port;
> +	struct device *dev = gpio->dev;
> +	struct gpio_irq_chip *girq;
> +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> +	int ret;
> +
> +	port = &gpio->ports[index];
> +	port->gpio = gpio;
> +
> +	dat	= port->base + GPLR;
> +	set	= port->base + GPSR;
> +	clr	= port->base + GPCR;
> +	dirin	= port->base + GCDR;
> +	dirout	= port->base + GSDR;
> +
> +	/* This registers 32 GPIO lines per port */
> +	ret = bgpio_init(&port->gc, dev, 4, dat, set, clr, dirout, dirin,
> +			 BGPIOF_UNREADABLE_REG_SET | BGPIOF_UNREADABLE_REG_DIR);

Esmil point out bgpio_init() require a GPIO_GENERIC dependency, will fix it

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

Re: [PATCH v4 2/4] gpio: spacemit: add support for K1 SoC

[Alex Elder]

On 1/20/25 9:38 PM, 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>

It sounds like the hardware will be modeled in DTS with
explicit banks, which makes sense.  The hardware looks like:

     GPIO controller ---> bank0 --> GPIOs 0..31
                     |
                     |--> bank1 --> GPIOs 0..31
                     |
                     |--> bank2 --> GPIOs 0..31
                     |
                     ---> bank3 --> GPIOs 0..31

Each bank has its own set of 15 registers, but all are managed
by the same controller (driver instance).

Anyway, I'm going to comment on just the code...

> ---
>   drivers/gpio/Kconfig            |   7 +
>   drivers/gpio/Makefile           |   1 +
>   drivers/gpio/gpio-spacemit-k1.c | 295 ++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 303 insertions(+)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7cac7f287eb04e4c17a17f75ed04..c153f5439649da020ee42c38e88cb8df31a8e307 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -654,6 +654,13 @@ 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
> +	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..de0491af494c10f528095efee9b3a140bdcc0b0d
> --- /dev/null
> +++ b/drivers/gpio/gpio-spacemit-k1.c
> @@ -0,0 +1,295 @@
> +// 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/property.h>
> +#include <linux/seq_file.h>
> +#include <linux/module.h>
> +
> +/* register offset */

Please add one-line comments explaining the purpose of these registers.
I explain my understanding below but you can maybe shorten it and add
something to the right of each definition.

> +#define GPLR		0x00

Current port (or pin?) level (port value, regardless of in or out) (r)

> +#define GPDR		0x0c

Current port direction (clear/0 = in, set/1 = out) (r)
This is currently never used.

> +#define GPSR		0x18

Set port value (set output level high for any set bits) (w)

> +#define GPCR		0x24

Clear port value (set output level low for any set bits) (w)

> +#define GRER		0x30

Ports configured for rising edge detect (r)
This is currently never used.

> +#define GFER		0x3c

Ports configured for falling edge detect (r)
This is currently never used.

> +#define GEDR		0x48

Edge detect status register (set bits indicate edge detected) (r/w)

> +#define GSDR		0x54

Set port direction (set bits indicate output pins) (w)

> +#define GCDR		0x60

Clear port direction (set bits indicate input pins) (w)

> +#define GSRER		0x6c

Enable rising edge detect (set bits indicate rising edge detect) (w)

> +#define GCRER		0x78

Disable rising edge detect (w)

> +#define GSFER		0x84

Enable falling edge detect (set bits indicate falling edge detect) (w)

> +#define GCFER		0x90

Disable falling edge detect (w)

> +#define GAPMASK		0x9c

I don't know what this is.  You write it with ~0 after you
clear both rising and falling edge detection on all 32 pins.

> +#define GCPMASK		0xa8

I don't know what this is.  It's currently never used.

> +
> +#define K1_BANK_GPIO_NUMBER	(32)

No need for parentheses around a simple constant.

> +
> +#define to_spacemit_gpio_port(x) container_of((x), struct spacemit_gpio_port, gc)
> +
> +struct spacemit_gpio;
> +

I might just not understand what a "port" means in this context
(I think here it represents a "bank" of 32 GPIOs.)  Based on the
DT discussion it seems like your structures might change, and
I'd like to know what you'll call them.

Here are terms I'll use:  There will be a top-level "controller"
structure which will be able to access four distinct "banks",
each of which has 32 "ports".

> +struct spacemit_gpio_port {
> +	struct gpio_chip		gc;
> +	struct spacemit_gpio		*gpio;
> +	struct fwnode_handle		*fwnode;
> +	void __iomem			*base;
> +	int				irq;
> +	u32				irq_mask;
> +	u32				irq_rising_edge;
> +	u32				irq_falling_edge;
> +	u32				index;
> +};
> +
> +struct spacemit_gpio {
> +	struct	device			*dev;
> +	struct spacemit_gpio_port	*ports;
> +	u32				nr_ports;
> +};
> +

Basically all of the write registers allow you to specify an
entire mask of bits, where the register write changes the
state of every port in a bank whose corresponding bit is set.
That capability is probably worth exposing, even though the
driver currently doesn't use it.

Meanwhile, most (maybe all) of your functions are only used
to update the state of a single port in a bank.

I think the argument names should distinguish these two cases.
I find the argument name "bit" (which now represents a 32-bit
mask with only one bit set) to be ambiguous.

If exactly one port is affected, maybe its number (0-31) can
be passed as the argument.  But where multiple ports in a
bank can be affected by the same operation, pass a "mask".

> +static inline void spacemit_clear_edge_detection(struct spacemit_gpio_port *port, u32 bit)
> +{

I'd do:
	if (bit & port->irq_rising_edge)
		writel(bit, port->base + GSRER);

(And similar below, and in spacemit_set_edge_detection().)

> +	writel(bit, port->base + GCRER);
> +	writel(bit, port->base + GCFER);
> +}
> +

Two comments about the function above and the next one:
- I think their names should be about masking IRQs, not about
   edge detection.
- Each is called only once, and they're trivial enough that I
   don't think encapsulating them in a function adds any value.
   Just open-code them where they're used.

> +static inline void spacemit_set_edge_detection(struct spacemit_gpio_port *port, u32 bit)
> +{
> +	writel(bit & port->irq_rising_edge,  port->base + GSRER);
> +	writel(bit & port->irq_falling_edge, port->base + GSFER);
> +}
> +

This is used to disable IRQ generation on all ports in a bank.
I would offer the same two comments about this function as I
gave above.

> +static inline void spacemit_reset_edge_detection(struct spacemit_gpio_port *port)
> +{
> +	writel(0xffffffff, port->base + GCFER);
> +	writel(0xffffffff, port->base + GCRER);
> +	writel(0xffffffff, port->base + GAPMASK);
> +}
> +

Use names that align with the callback function names.  I.e.,
this should be spacemit_irq_set_type().

> +static int spacemit_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +
> +	if (type & IRQ_TYPE_EDGE_RISING) {
> +		port->irq_rising_edge |= bit;

Here too, maybe avoid doing the write if the bit was already
set in the rising edge mask.

> +		writel(bit, port->base + GSRER);
> +	} else {
> +		port->irq_rising_edge &= ~bit;
> +		writel(bit, port->base + GCRER);
> +	}
> +
> +	if (type & IRQ_TYPE_EDGE_FALLING) {
> +		port->irq_falling_edge |= bit;
> +		writel(bit, port->base + GSFER);
> +	} else {
> +		port->irq_falling_edge &= ~bit;
> +		writel(bit, port->base + GCFER);
> +	}
> +
> +	return 0;
> +}
> +
> +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> +{
> +	struct spacemit_gpio_port *port = dev_id;
> +	unsigned long pending;
> +	u32 n, gedr;
> +
> +	gedr = readl(port->base + GEDR);
> +	if (!gedr)
> +		return IRQ_NONE;
> +
> +	writel(gedr, port->base + GEDR);

I'd have the blank line here, instead of above the writel().

> +	gedr = gedr & port->irq_mask;

	pending = gedr & port->irq_mask;
	if (!pending)

> +
> +	if (!gedr)
> +		return IRQ_NONE;
> +
> +	pending = gedr;
> +
> +	for_each_set_bit(n, &pending, BITS_PER_LONG)
> +		handle_nested_irq(irq_find_mapping(port->gc.irq.domain, n));
> +
> +	return IRQ_HANDLED;
> +}
> +

I don't think "muxed" is necessary in these names.  Just call
this spacemit_irq_ack().

> +static void spacemit_ack_muxed_gpio(struct irq_data *d)
> +{
> +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> +
> +	writel(BIT(irqd_to_hwirq(d)), port->base + GEDR);
> +}
> +

spacemit_irq_mask()

> +static void spacemit_mask_muxed_gpio(struct irq_data *d)
> +{
> +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +
> +	port->irq_mask &= ~bit;
> +

As I said earlier, I think you should just open-code the two
writes done by the next function.

> +	spacemit_clear_edge_detection(port, bit);
> +}
> +

spacemit_irq_unmask()

> +static void spacemit_unmask_muxed_gpio(struct irq_data *d)
> +{
> +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> +	u32 bit = BIT(irqd_to_hwirq(d));
> +
> +	port->irq_mask |= bit;
> +

Open-code this call too.

> +	spacemit_set_edge_detection(port, bit);
> +}
> +
> +static void spacemit_irq_print_chip(struct irq_data *data, struct seq_file *p)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> +	struct spacemit_gpio_port *port = to_spacemit_gpio_port(gc);
> +
> +	seq_printf(p, "%s-%d", dev_name(gc->parent), port->index);
> +}
> +
> +static struct irq_chip spacemit_muxed_gpio_chip = {
> +	.name		= "k1-gpio-irqchip",
> +	.irq_ack	= spacemit_ack_muxed_gpio,
> +	.irq_mask	= spacemit_mask_muxed_gpio,
> +	.irq_unmask	= spacemit_unmask_muxed_gpio,
> +	.irq_set_type	= spacemit_gpio_irq_type,
> +	.irq_print_chip	= spacemit_irq_print_chip,
> +	.flags		= IRQCHIP_IMMUTABLE,
> +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +
> +static int spacemit_gpio_get_ports(struct device *dev, struct spacemit_gpio *gpio,
> +				   void __iomem *regs)
> +{
> +	struct spacemit_gpio_port *port;
> +	u32 i = 0, offset;
> +
> +	gpio->nr_ports = device_get_child_node_count(dev);
> +	if (gpio->nr_ports == 0)
> +		return -ENODEV;
> +
> +	gpio->ports = devm_kcalloc(dev, gpio->nr_ports, sizeof(*gpio->ports), GFP_KERNEL);
> +	if (!gpio->ports)
> +		return -ENOMEM;
> +
> +	device_for_each_child_node_scoped(dev, fwnode)  {

Make sure i never exceeds gpio->nr_ports.

> +		port		= &gpio->ports[i];
> +		port->fwnode	= fwnode;
> +		port->index	= i++;
> +
> +		if (fwnode_property_read_u32(fwnode, "reg", &offset))
> +			return -EINVAL;
> +
> +		port->base	= regs + offset;
> +		port->irq	= fwnode_irq_get(fwnode, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int spacemit_gpio_add_port(struct spacemit_gpio *gpio, int index)
> +{
> +	struct spacemit_gpio_port *port;
> +	struct device *dev = gpio->dev;
> +	struct gpio_irq_chip *girq;
> +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> +	int ret;
> +
> +	port = &gpio->ports[index];
> +	port->gpio = gpio;
> +
> +	dat	= port->base + GPLR;
> +	set	= port->base + GPSR;
> +	clr	= port->base + GPCR;
> +	dirin	= port->base + GCDR;
> +	dirout	= port->base + GSDR;
> +
> +	/* This registers 32 GPIO lines per port */
> +	ret = bgpio_init(&port->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 for port\n");
> +
> +	port->gc.label		= dev_name(dev);
> +	port->gc.fwnode		= port->fwnode;
> +	port->gc.request	= gpiochip_generic_request;
> +	port->gc.free		= gpiochip_generic_free;
> +	port->gc.ngpio		= K1_BANK_GPIO_NUMBER;
> +	port->gc.base		= -1;
> +
> +	girq			= &port->gc.irq;
> +	girq->threaded		= true;
> +	girq->handler		= handle_simple_irq;
> +
> +	gpio_irq_chip_set_chip(girq, &spacemit_muxed_gpio_chip);
> +
> +	spacemit_reset_edge_detection(port);
> +

I *think* you should call devm_gpiochip_add_data() *before*
you register the interrupt handler, because conceivably an
interrupt could fire the instant it's registered.  Maybe it
doesn't matter though.

					-Alex

> +	ret = devm_request_threaded_irq(dev, port->irq, NULL,
> +					spacemit_gpio_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					port->gc.label, port);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to request IRQ\n");
> +
> +	return devm_gpiochip_add_data(dev, &port->gc, port);
> +}
> +
> +static int spacemit_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spacemit_gpio *gpio;
> +	struct resource *res;
> +	void __iomem *regs;
> +	int i, ret;
> +
> +	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;
> +
> +	gpio->dev = dev;
> +
> +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	ret = spacemit_gpio_get_ports(dev, gpio, regs);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "fail to get gpio ports\n");
> +
> +	for (i = 0; i < gpio->nr_ports; i++) {
> +		ret = spacemit_gpio_add_port(gpio, i);
> +		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 v4 2/4] gpio: spacemit: add support for K1 SoC

[Yixun Lan]

Hi Alex:

thanks for your fully review

On 15:11 Sat 15 Feb     , Alex Elder wrote:
> On 1/20/25 9:38 PM, 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>
> 
> It sounds like the hardware will be modeled in DTS with
> explicit banks, which makes sense.  The hardware looks like:
> 
>      GPIO controller ---> bank0 --> GPIOs 0..31
>                      |
>                      |--> bank1 --> GPIOs 0..31
>                      |
>                      |--> bank2 --> GPIOs 0..31
>                      |
>                      ---> bank3 --> GPIOs 0..31
> 
> Each bank has its own set of 15 registers, but all are managed
> by the same controller (driver instance).
> 
Yes, this is the hardware, the driver will populate each bank with one gpio chip

and notice that, we've got some comments from DT maintainer that it's better to
fold children nodes into parent and switch to three cells gpio node.
see following link for more detail discussion
https://lore.kernel.org/all/CACRpkdZYYZ5tUR4gJXuCrix0k56rPPB2TUGP3KpwqMgjs_Vd5w@mail.gmail.com/

so, probably I will massively re-construct this driver in next version...

> Anyway, I'm going to comment on just the code...
> 
> > ---
> >   drivers/gpio/Kconfig            |   7 +
> >   drivers/gpio/Makefile           |   1 +
> >   drivers/gpio/gpio-spacemit-k1.c | 295 ++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 303 insertions(+)
> > 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56fee58e281e7cac7f287eb04e4c17a17f75ed04..c153f5439649da020ee42c38e88cb8df31a8e307 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -654,6 +654,13 @@ 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
> > +	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..de0491af494c10f528095efee9b3a140bdcc0b0d
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-spacemit-k1.c
> > @@ -0,0 +1,295 @@
> > +// 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/property.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/module.h>
> > +
> > +/* register offset */
> 
> Please add one-line comments explaining the purpose of these registers.
> I explain my understanding below but you can maybe shorten it and add
> something to the right of each definition.
> 
I didn't consider to add comments in the first place, it's kind of tedious,
 also it will mostly duplicate what in the docs from spacemit

but I can do this, I will try to come up with a short version,
if it's not enough, then people can always consult the documentation

> > +#define GPLR		0x00
> 
> Current port (or pin?) level (port value, regardless of in or out) (r)
> 
#define GPLR               0x00	/* GPIO port level register */

> > +#define GPDR		0x0c
> 
> Current port direction (clear/0 = in, set/1 = out) (r)
> This is currently never used.
> 
#define GPDR               0x0c /* GPIO port direction register - R/W */

> > +#define GPSR		0x18
> 
> Set port value (set output level high for any set bits) (w)
> 
#define GPSR               0x18	/* GPIO port set register - W */
> > +#define GPCR		0x24
> 
> Clear port value (set output level low for any set bits) (w)
#define GPCR               0x24 /* GPIO port clear register - W */
> 
..
> > +#define GRER		0x30
> 
> Ports configured for rising edge detect (r)
> This is currently never used.
> 
#define GRER               0x30 /* GPIO port rising edge register R/W */
> > +#define GFER		0x3c
> 
> Ports configured for falling edge detect (r)
> This is currently never used.
> 
#define GFER               0x3c /* GPIO port falling edge register R/W */

No, the above two registers are not used, as I think it's has duplicated functionality
 with GSRER, GCRER, GSFER, GCFER

but, I don't know why the hw design like this, may need more clarification from vendor
the code from vendor also doesn't touch these two registers
from my testing, the GPIO interrupt's rising/falling edge trigger works fine without them

> > +#define GEDR		0x48
> 
> Edge detect status register (set bits indicate edge detected) (r/w)
> 
#define GEDR               0x48 /* GPIO Edge Detect Status Registers - R/W1C */

> > +#define GSDR		0x54
> 
> Set port direction (set bits indicate output pins) (w)
> 
#define GSDR               0x54 /*  GPIO (set) Direction Registers - W */

> > +#define GCDR		0x60
> 
> Clear port direction (set bits indicate input pins) (w)
> 
#define GCDR               0x60 /* GPIO (clear) Direction Registers - W */
> > +#define GSRER		0x6c
> 
> Enable rising edge detect (set bits indicate rising edge detect) (w)
> 
#define GSRER              0x6c /* GPIO (set) rising edge detect enable register - W */


> > +#define GCRER		0x78
> 
> Disable rising edge detect (w)
> 
#define GCRER              0x78 /* GPIO (clear) rising edge detect enable register - W */

> > +#define GSFER		0x84
> 
> Enable falling edge detect (set bits indicate falling edge detect) (w)
> 

#define GSFER              0x84 /* GPIO (set) falling edge detect enable register - W */

> > +#define GCFER		0x90
> 
> Disable falling edge detect (w)
> 
#define GCFER              0x90 /* GPIO (clear) falling edge detect enable register - W */

> > +#define GAPMASK		0x9c
> 
> I don't know what this is.  You write it with ~0 after you
> clear both rising and falling edge detection on all 32 pins.
> 
I haven't found documentation from spacemit's web, just copied the logic from vendor code..

from my understanding (best guess), it's the mask bit to AP (application processor)
writing 1 should enable GPIO interrupt functionality to AP?

> > +#define GCPMASK		0xa8
> 
> I don't know what this is.  It's currently never used.
> 
my guess, it's used to mask bits to disable interrupt to AP? 
since we don't need to do this, so not used 

> > +
> > +#define K1_BANK_GPIO_NUMBER	(32)
> 
> No need for parentheses around a simple constant.
> 
Ok

> > +
> > +#define to_spacemit_gpio_port(x) container_of((x), struct spacemit_gpio_port, gc)
> > +
> > +struct spacemit_gpio;
> > +
> 
> I might just not understand what a "port" means in this context
> (I think here it represents a "bank" of 32 GPIOs.)  Based on the
> DT discussion it seems like your structures might change, and
> I'd like to know what you'll call them.
> 
> Here are terms I'll use:  There will be a top-level "controller"
> structure which will be able to access four distinct "banks",
> each of which has 32 "ports".
> 
what I mean is "ports == banks", but after looking at docs and take your suggestion here,
we'd might better rename "ports" to "banks"

> > +struct spacemit_gpio_port {
> > +	struct gpio_chip		gc;
> > +	struct spacemit_gpio		*gpio;
> > +	struct fwnode_handle		*fwnode;
> > +	void __iomem			*base;
> > +	int				irq;
> > +	u32				irq_mask;
> > +	u32				irq_rising_edge;
> > +	u32				irq_falling_edge;
> > +	u32				index;
> > +};
> > +
> > +struct spacemit_gpio {
> > +	struct	device			*dev;
> > +	struct spacemit_gpio_port	*ports;
> > +	u32				nr_ports;
> > +};
> > +
> 
> Basically all of the write registers allow you to specify an
> entire mask of bits, where the register write changes the
> state of every port in a bank whose corresponding bit is set.
yes, these registers are only able to W1 (write), not readable,
also write 0 takes no effect

> That capability is probably worth exposing, even though the
> driver currently doesn't use it.
> 
no idea how to expose, or where it can be used..

> Meanwhile, most (maybe all) of your functions are only used
> to update the state of a single port in a bank.
> 
right

> I think the argument names should distinguish these two cases.
> I find the argument name "bit" (which now represents a 32-bit
> mask with only one bit set) to be ambiguous.
> 
> If exactly one port is affected, maybe its number (0-31) can
> be passed as the argument.  But where multiple ports in a
> bank can be affected by the same operation, pass a "mask".
> 
I got your idea, and I will see what I can do..

> > +static inline void spacemit_clear_edge_detection(struct spacemit_gpio_port *port, u32 bit)
> > +{
> 
> I'd do:
> 	if (bit & port->irq_rising_edge)
> 		writel(bit, port->base + GSRER);
> 
Ok, strictly we can add extra checking

> (And similar below, and in spacemit_set_edge_detection().)
> 
> > +	writel(bit, port->base + GCRER);
> > +	writel(bit, port->base + GCFER);
> > +}
> > +
> 
> Two comments about the function above and the next one:
> - I think their names should be about masking IRQs, not about
>    edge detection.
> - Each is called only once, and they're trivial enough that I
>    don't think encapsulating them in a function adds any value.
>    Just open-code them where they're used.
> 
I thought giving a function name would make it more readable,
it shouldn't bring extra price as compiler will inline it

but, I'm open mind with this, "open-code + one line comment" is also fine

> > +static inline void spacemit_set_edge_detection(struct spacemit_gpio_port *port, u32 bit)
> > +{
> > +	writel(bit & port->irq_rising_edge,  port->base + GSRER);
> > +	writel(bit & port->irq_falling_edge, port->base + GSFER);
> > +}
> > +
> 
> This is used to disable IRQ generation on all ports in a bank.
> I would offer the same two comments about this function as I
> gave above.
> 
ok

> > +static inline void spacemit_reset_edge_detection(struct spacemit_gpio_port *port)
> > +{
> > +	writel(0xffffffff, port->base + GCFER);
> > +	writel(0xffffffff, port->base + GCRER);
> > +	writel(0xffffffff, port->base + GAPMASK);
> > +
> 
> Use names that align with the callback function names.  I.e.,
> this should be spacemit_irq_set_type().
> 
ok
> > +static int spacemit_gpio_irq_type(struct irq_data *d, unsigned int type)
> > +{
> > +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> > +	if (type & IRQ_TYPE_EDGE_RISING) {
> > +		port->irq_rising_edge |= bit;
> 
> Here too, maybe avoid doing the write if the bit was already
> set in the rising edge mask.
> 
I thought it's tedious to do extra checking.. kind of unnecessary
making the mask bit match underlying hw write operation is more intuitive

> > +		writel(bit, port->base + GSRER);
> > +	} else {
> > +		port->irq_rising_edge &= ~bit;
> > +		writel(bit, port->base + GCRER);
> > +	}
> > +
> > +	if (type & IRQ_TYPE_EDGE_FALLING) {
> > +		port->irq_falling_edge |= bit;
> > +		writel(bit, port->base + GSFER);
> > +	} else {
> > +		port->irq_falling_edge &= ~bit;
> > +		writel(bit, port->base + GCFER);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> > +{
> > +	struct spacemit_gpio_port *port = dev_id;
> > +	unsigned long pending;
> > +	u32 n, gedr;
> > +
> > +	gedr = readl(port->base + GEDR);
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +
> > +	writel(gedr, port->base + GEDR);
> 
> I'd have the blank line here, instead of above the writel().
> 
ok

> > +	gedr = gedr & port->irq_mask;
> 
> 	pending = gedr & port->irq_mask;
> 	if (!pending)
> 
> > +
> > +	if (!gedr)
> > +		return IRQ_NONE;
> > +
> > +	pending = gedr;
> > +
> > +	for_each_set_bit(n, &pending, BITS_PER_LONG)
> > +		handle_nested_irq(irq_find_mapping(port->gc.irq.domain, n));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> I don't think "muxed" is necessary in these names.  Just call
> this spacemit_irq_ack().
> 
ok
> > +static void spacemit_ack_muxed_gpio(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> > +
> > +	writel(BIT(irqd_to_hwirq(d)), port->base + GEDR);
> > +}
> > +
> 
> spacemit_irq_mask()
> 
ok
> > +static void spacemit_mask_muxed_gpio(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> > +	port->irq_mask &= ~bit;
> > +
> 
> As I said earlier, I think you should just open-code the two
> writes done by the next function.
> 
ok
> > +	spacemit_clear_edge_detection(port, bit);
> > +}
> > +
> 
> spacemit_irq_unmask()
> 
ok
> > +static void spacemit_unmask_muxed_gpio(struct irq_data *d)
> > +{
> > +	struct spacemit_gpio_port *port = irq_data_get_irq_chip_data(d);
> > +	u32 bit = BIT(irqd_to_hwirq(d));
> > +
> > +	port->irq_mask |= bit;
> > +
> 
> Open-code this call too.
> 
ok
> > +	spacemit_set_edge_detection(port, bit);
> > +}
> > +
> > +static void spacemit_irq_print_chip(struct irq_data *data, struct seq_file *p)
> > +{
> > +	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> > +	struct spacemit_gpio_port *port = to_spacemit_gpio_port(gc);
> > +
> > +	seq_printf(p, "%s-%d", dev_name(gc->parent), port->index);
> > +}
> > +
> > +static struct irq_chip spacemit_muxed_gpio_chip = {
> > +	.name		= "k1-gpio-irqchip",
> > +	.irq_ack	= spacemit_ack_muxed_gpio,
> > +	.irq_mask	= spacemit_mask_muxed_gpio,
> > +	.irq_unmask	= spacemit_unmask_muxed_gpio,
> > +	.irq_set_type	= spacemit_gpio_irq_type,
> > +	.irq_print_chip	= spacemit_irq_print_chip,
> > +	.flags		= IRQCHIP_IMMUTABLE,
> > +	GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > +};
> > +
> > +static int spacemit_gpio_get_ports(struct device *dev, struct spacemit_gpio *gpio,
> > +				   void __iomem *regs)
> > +{
> > +	struct spacemit_gpio_port *port;
> > +	u32 i = 0, offset;
> > +
> > +	gpio->nr_ports = device_get_child_node_count(dev);
> > +	if (gpio->nr_ports == 0)
> > +		return -ENODEV;
> > +
> > +	gpio->ports = devm_kcalloc(dev, gpio->nr_ports, sizeof(*gpio->ports), GFP_KERNEL);
> > +	if (!gpio->ports)
> > +		return -ENOMEM;
> > +
> > +	device_for_each_child_node_scoped(dev, fwnode)  {
> 
> Make sure i never exceeds gpio->nr_ports.
> 
Ok, will see, as this function need to refactor

> > +		port		= &gpio->ports[i];
> > +		port->fwnode	= fwnode;
> > +		port->index	= i++;
> > +
> > +		if (fwnode_property_read_u32(fwnode, "reg", &offset))
> > +			return -EINVAL;
> > +
> > +		port->base	= regs + offset;
> > +		port->irq	= fwnode_irq_get(fwnode, 0);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int spacemit_gpio_add_port(struct spacemit_gpio *gpio, int index)
> > +{
> > +	struct spacemit_gpio_port *port;
> > +	struct device *dev = gpio->dev;
> > +	struct gpio_irq_chip *girq;
> > +	void __iomem *dat, *set, *clr, *dirin, *dirout;
> > +	int ret;
> > +
> > +	port = &gpio->ports[index];
> > +	port->gpio = gpio;
> > +
> > +	dat	= port->base + GPLR;
> > +	set	= port->base + GPSR;
> > +	clr	= port->base + GPCR;
> > +	dirin	= port->base + GCDR;
> > +	dirout	= port->base + GSDR;
> > +
> > +	/* This registers 32 GPIO lines per port */
> > +	ret = bgpio_init(&port->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 for port\n");
> > +
> > +	port->gc.label		= dev_name(dev);
> > +	port->gc.fwnode		= port->fwnode;
> > +	port->gc.request	= gpiochip_generic_request;
> > +	port->gc.free		= gpiochip_generic_free;
> > +	port->gc.ngpio		= K1_BANK_GPIO_NUMBER;
> > +	port->gc.base		= -1;
> > +
> > +	girq			= &port->gc.irq;
> > +	girq->threaded		= true;
> > +	girq->handler		= handle_simple_irq;
> > +
> > +	gpio_irq_chip_set_chip(girq, &spacemit_muxed_gpio_chip);
> > +
> > +	spacemit_reset_edge_detection(port);
> > +
> 
> I *think* you should call devm_gpiochip_add_data() *before*
> you register the interrupt handler, because conceivably an
> interrupt could fire the instant it's registered.  Maybe it
> doesn't matter though.
> 
good point, I will check this, we probably better to enable(unmask) interrupt
 after all registration is done

> 					-Alex
> 
> > +	ret = devm_request_threaded_irq(dev, port->irq, NULL,
> > +					spacemit_gpio_irq_handler,
> > +					IRQF_ONESHOT | IRQF_SHARED,
> > +					port->gc.label, port);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to request IRQ\n");
> > +
> > +	return devm_gpiochip_add_data(dev, &port->gc, port);
> > +}
> > +
> > +static int spacemit_gpio_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct spacemit_gpio *gpio;
> > +	struct resource *res;
> > +	void __iomem *regs;
> > +	int i, ret;
> > +
> > +	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> > +	if (!gpio)
> > +		return -ENOMEM;
> > +
> > +	gpio->dev = dev;
> > +
> > +	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	ret = spacemit_gpio_get_ports(dev, gpio, regs);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "fail to get gpio ports\n");
> > +
> > +	for (i = 0; i < gpio->nr_ports; i++) {
> > +		ret = spacemit_gpio_add_port(gpio, i);
> > +		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 v4 3/4] 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 port and map to the pinctrl controller.

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

diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index c670ebf8fa12917aa6493fcd89fdd1409529538b..005f24b95d9ddae686dda07932d0086379cff219 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -404,6 +404,61 @@ uart9: serial@d4017800 {
 			status = "disabled";
 		};
 
+		gpio: gpio@d4019000 {
+			compatible = "spacemit,k1-gpio";
+			reg = <0x0 0xd4019000 0x0 0x800>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port0: gpio-port@0 {
+				compatible = "spacemit,k1-gpio-port";
+				reg = <0x0>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <58>;
+				interrupt-parent = <&plic>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				gpio-ranges = <&pinctrl 0 0 32>;
+			};
+
+			port1: gpio-port@4 {
+				compatible = "spacemit,k1-gpio-port";
+				reg = <0x4>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <58>;
+				interrupt-parent = <&plic>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				gpio-ranges = <&pinctrl 0 32 32>;
+			};
+
+			port2: gpio-port@8 {
+				compatible = "spacemit,k1-gpio-port";
+				reg = <0x8>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <58>;
+				interrupt-parent = <&plic>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				gpio-ranges = <&pinctrl 0 64 32>;
+			};
+
+			port3: gpio-port@100 {
+				compatible = "spacemit,k1-gpio-port";
+				reg = <0x100>;
+				gpio-controller;
+				#gpio-cells = <2>;
+				interrupts = <58>;
+				interrupt-parent = <&plic>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				gpio-ranges = <&pinctrl 0 96 32>;
+			};
+		};
+
 		pinctrl: pinctrl@d401e000 {
 			compatible = "spacemit,k1-pinctrl";
 			reg = <0x0 0xd401e000 0x0 0x400>;

-- 
2.48.0

[PATCH v4 4/4] 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..6113e7523109076b99c493c8ac9ba69efd734620 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 = <&port3 0 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "heartbeat";
+			default-state = "on";
+		};
+	};
 };
 
 &uart0 {

-- 
2.48.0