[PATCH v1 0/2] Add Aspeed G7 gpio support

[PATCH v1 0/2] Add Aspeed G7 gpio support

[Billy Tsai]

The Aspeed 7th generation SoC features two GPIO controllers: one with 12
GPIO pins and another with 216 GPIO pins. The main difference from the
previous generation is that the control logic has been updated to support
per-pin control, allowing each pin to have its own 32-bit register for
configuring value, direction, interrupt type, and more.

Billy Tsai (2):
  dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700
  gpio: Add G7 Aspeed gpio controller driver

 .../bindings/gpio/aspeed,ast2400-gpio.yaml    |  46 +-
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-aspeed-g7.c                 | 831 ++++++++++++++++++
 4 files changed, 884 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpio/gpio-aspeed-g7.c

-- 
2.25.1

[PATCH v1 1/2] dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700

[Billy Tsai]

The AST2700 is the 7th generation SoC from Aspeed, featuring two GPIO
controllers: one with 12 GPIO pins and another with 216 GPIO pins.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 46 ++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
index cf11aa7ec8c7..4d439972c14b 100644
--- a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
@@ -15,6 +15,7 @@ properties:
       - aspeed,ast2400-gpio
       - aspeed,ast2500-gpio
       - aspeed,ast2600-gpio
+      - aspeed,ast2700-gpio
 
   reg:
     maxItems: 1
@@ -42,7 +43,7 @@ properties:
     const: 2
 
   ngpios:
-    minimum: 36
+    minimum: 12
     maximum: 232
 
 required:
@@ -93,6 +94,20 @@ allOf:
           enum: [ 36, 208 ]
       required:
         - ngpios
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2700-gpio
+    then:
+      properties:
+        gpio-line-names:
+          minItems: 12
+          maxItems: 216
+        ngpios:
+          enum: [ 12, 216 ]
+      required:
+        - ngpios
 
 additionalProperties: false
 
@@ -146,3 +161,32 @@ examples:
         gpio-ranges = <&pinctrl 0 208 36>;
         ngpios = <36>;
     };
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+        #include <dt-bindings/interrupt-controller/arm-gic.h>
+        gpio2: gpio@14c0b000 {
+            compatible = "aspeed,ast2700-gpio";
+            reg = <0x0 0x14c0b000 0x0 0x1000>;
+            interrupts-extended = <&soc1_intc2 18>;
+            interrupt-controller;
+            #interrupt-cells = <2>;
+            #gpio-cells = <2>;
+            gpio-controller;
+            gpio-ranges = <&pinctrl1 0 0 216>;
+            ngpios = <216>;
+        };
+
+        gpio3: gpio@12c11000 {
+            compatible = "aspeed,ast2700-gpio";
+            reg = <0x0 0x12c11000 0x0 0x1000>;
+            interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-controller;
+            #interrupt-cells = <2>;
+            #gpio-cells = <2>;
+            gpio-controller;
+            gpio-ranges = <&pinctrl0 0 0 12>;
+            ngpios = <12>;
+        };
+    };
-- 
2.25.1

Re: [PATCH v1 1/2] dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700

[Krzysztof Kozlowski]

On Wed, Aug 21, 2024 at 03:07:39PM +0800, Billy Tsai wrote:
> The AST2700 is the 7th generation SoC from Aspeed, featuring two GPIO
> controllers: one with 12 GPIO pins and another with 216 GPIO pins.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 46 ++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> index cf11aa7ec8c7..4d439972c14b 100644
> --- a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> +++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> @@ -15,6 +15,7 @@ properties:
>        - aspeed,ast2400-gpio
>        - aspeed,ast2500-gpio
>        - aspeed,ast2600-gpio
> +      - aspeed,ast2700-gpio
>  
>    reg:
>      maxItems: 1
> @@ -42,7 +43,7 @@ properties:
>      const: 2
>  
>    ngpios:
> -    minimum: 36
> +    minimum: 12
>      maximum: 232
>  
>  required:
> @@ -93,6 +94,20 @@ allOf:
>            enum: [ 36, 208 ]
>        required:
>          - ngpios
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2700-gpio
> +    then:
> +      properties:
> +        gpio-line-names:
> +          minItems: 12
> +          maxItems: 216
> +        ngpios:
> +          enum: [ 12, 216 ]
> +      required:
> +        - ngpios
>  
>  additionalProperties: false
>  
> @@ -146,3 +161,32 @@ examples:
>          gpio-ranges = <&pinctrl 0 208 36>;
>          ngpios = <36>;
>      };
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> +        gpio2: gpio@14c0b000 {
> +            compatible = "aspeed,ast2700-gpio";

No need for new example, no relavant/important differences here.


> +            reg = <0x0 0x14c0b000 0x0 0x1000>;
> +            interrupts-extended = <&soc1_intc2 18>;
> +            interrupt-controller;
> +            #interrupt-cells = <2>;
> +            #gpio-cells = <2>;
> +            gpio-controller;
> +            gpio-ranges = <&pinctrl1 0 0 216>;
> +            ngpios = <216>;
> +        };
> +
> +        gpio3: gpio@12c11000 {
> +            compatible = "aspeed,ast2700-gpio";

Especially for two the same examples...

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: gpio: aspeed,ast2400-gpio: Support ast2700

[Billy Tsai]

Hi Krzysztof,

I'm sorry for missing your comment.

I will remove the example in the next version of the patch.

Thanks

Best Regards,
Billy Tsai

> > The AST2700 is the 7th generation SoC from Aspeed, featuring two GPIO
> > controllers: one with 12 GPIO pins and another with 216 GPIO pins.
> > 
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > ---
> >  .../bindings/gpio/aspeed,ast2400-gpio.yaml    | 46 ++++++++++++++++++-
> >  1 file changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml b/Documentation/devicetree/bindings/gpio/> aspeed,ast2400-gpio.yaml
> > index cf11aa7ec8c7..4d439972c14b 100644
> > --- a/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> > +++ b/Documentation/devicetree/bindings/gpio/aspeed,ast2400-gpio.yaml
> > @@ -15,6 +15,7 @@ properties:
> >        - aspeed,ast2400-gpio
> >        - aspeed,ast2500-gpio
> >        - aspeed,ast2600-gpio
> > +      - aspeed,ast2700-gpio
> >  
> >    reg:
> >      maxItems: 1
> > @@ -42,7 +43,7 @@ properties:
> >      const: 2
> >  
> >    ngpios:
> > -    minimum: 36
> > +    minimum: 12
> >      maximum: 232
> >  
> >  required:
> > @@ -93,6 +94,20 @@ allOf:
> >            enum: [ 36, 208 ]
> >        required:
> >          - ngpios
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2700-gpio
> > +    then:
> > +      properties:
> > +        gpio-line-names:
> > +          minItems: 12
> > +          maxItems: 216
> > +        ngpios:
> > +          enum: [ 12, 216 ]
> > +      required:
> > +        - ngpios
> >  
> >  additionalProperties: false
> >  
> > @@ -146,3 +161,32 @@ examples:
> >          gpio-ranges = <&pinctrl 0 208 36>;
> >          ngpios = <36>;
> >      };
> > +  - |
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +        #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +        gpio2: gpio@14c0b000 {
> > +            compatible = "aspeed,ast2700-gpio";

> No need for new example, no relavant/important differences here.


> +            reg = <0x0 0x14c0b000 0x0 0x1000>;
> +            interrupts-extended = <&soc1_intc2 18>;
> +            interrupt-controller;
> +            #interrupt-cells = <2>;
> +            #gpio-cells = <2>;
> +            gpio-controller;
> +            gpio-ranges = <&pinctrl1 0 0 216>;
> +            ngpios = <216>;
> +        };
> +
> +        gpio3: gpio@12c11000 {
> +            compatible = "aspeed,ast2700-gpio";

> Especially for two the same examples...

[PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Billy Tsai]

In the 7th generation of the SoC from Aspeed, the control logic of the
GPIO controller has been updated to support per-pin control. Each pin now
has its own 32-bit register, allowing for individual control of the pin’s
value, direction, interrupt type, and other settings.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/gpio/Kconfig          |   7 +
 drivers/gpio/Makefile         |   1 +
 drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
 3 files changed, 839 insertions(+)
 create mode 100644 drivers/gpio/gpio-aspeed-g7.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 58f43bcced7c..93f237429b92 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -172,6 +172,13 @@ config GPIO_ASPEED
 	help
 	  Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
 
+config GPIO_ASPEED_G7
+	tristate "Aspeed G7 GPIO support"
+	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say Y here to support Aspeed AST2700 GPIO controllers.
+
 config GPIO_ASPEED_SGPIO
 	bool "Aspeed SGPIO support"
 	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 64dd6d9d730d..e830291761ee 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH)		+= gpio-amd-fch.o
 obj-$(CONFIG_GPIO_AMDPT)		+= gpio-amdpt.o
 obj-$(CONFIG_GPIO_ARIZONA)		+= gpio-arizona.o
 obj-$(CONFIG_GPIO_ASPEED)		+= gpio-aspeed.o
+obj-$(CONFIG_GPIO_ASPEED_G7)		+= gpio-aspeed-g7.o
 obj-$(CONFIG_GPIO_ASPEED_SGPIO)		+= gpio-aspeed-sgpio.o
 obj-$(CONFIG_GPIO_ATH79)		+= gpio-ath79.o
 obj-$(CONFIG_GPIO_BCM_KONA)		+= gpio-bcm-kona.o
diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
new file mode 100644
index 000000000000..dbca097de6ea
--- /dev/null
+++ b/drivers/gpio/gpio-aspeed-g7.c
@@ -0,0 +1,831 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2024 Aspeed Technology Inc.
+ *
+ * Billy Tsai <billy_tsai@aspeedtech.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio/aspeed.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#include <asm/div64.h>
+
+#define GPIO_G7_IRQ_STS_BASE 0x100
+#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
+#define GPIO_G7_CTRL_REG_BASE 0x180
+#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
+#define GPIO_G7_OUT_DATA BIT(0)
+#define GPIO_G7_DIR BIT(1)
+#define GPIO_G7_IRQ_EN BIT(2)
+#define GPIO_G7_IRQ_TYPE0 BIT(3)
+#define GPIO_G7_IRQ_TYPE1 BIT(4)
+#define GPIO_G7_IRQ_TYPE2 BIT(5)
+#define GPIO_G7_RST_TOLERANCE BIT(6)
+#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
+#define GPIO_G7_INPUT_MASK BIT(9)
+#define GPIO_G7_IRQ_STS BIT(12)
+#define GPIO_G7_IN_DATA BIT(13)
+/*
+ * The configuration of the following registers should be determined
+ * outside of the GPIO driver.
+ */
+#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
+#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
+#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
+#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
+#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
+#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
+#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
+#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
+#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
+#define GPIO_G7_IRQ_TO_SIO BIT(3)
+#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
+#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
+
+static inline u32 field_get(u32 _mask, u32 _val)
+{
+	return (((_val) & (_mask)) >> (ffs(_mask) - 1));
+}
+
+static inline u32 field_prep(u32 _mask, u32 _val)
+{
+	return (((_val) << (ffs(_mask) - 1)) & (_mask));
+}
+
+static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
+{
+	iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
+}
+
+static inline void ast_clr_bits(void __iomem *addr, u32 mask)
+{
+	iowrite32((ioread32(addr) & ~(mask)), addr);
+}
+
+struct aspeed_bank_props {
+	unsigned int bank;
+	u32 input;
+	u32 output;
+};
+
+struct aspeed_gpio_g7_config {
+	unsigned int nr_gpios;
+	const struct aspeed_bank_props *props;
+};
+
+/*
+ * @offset_timer: Maps an offset to an @timer_users index, or zero if disabled
+ * @timer_users: Tracks the number of users for each timer
+ *
+ * The @timer_users has four elements but the first element is unused. This is
+ * to simplify accounting and indexing, as a zero value in @offset_timer
+ * represents disabled debouncing for the GPIO. Any other value for an element
+ * of @offset_timer is used as an index into @timer_users. This behaviour of
+ * the zero value aligns with the behaviour of zero built from the timer
+ * configuration registers (i.e. debouncing is disabled).
+ */
+struct aspeed_gpio_g7 {
+	struct gpio_chip chip;
+	struct device *dev;
+	raw_spinlock_t lock;
+	void __iomem *base;
+	int irq;
+	const struct aspeed_gpio_g7_config *config;
+
+	u8 *offset_timer;
+	unsigned int timer_users[4];
+	struct clk *clk;
+
+	u32 *dcache;
+};
+
+/*
+ * Note: The "value" register returns the input value sampled on the
+ *       line even when the GPIO is configured as an output. Since
+ *       that input goes through synchronizers, writing, then reading
+ *       back may not return the written value right away.
+ *
+ *       The "rdata" register returns the content of the write latch
+ *       and thus can be used to read back what was last written
+ *       reliably.
+ */
+
+static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 0x1f)
+#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
+
+static inline bool is_bank_props_sentinel(const struct aspeed_bank_props *props)
+{
+	return !(props->input || props->output);
+}
+
+static inline const struct aspeed_bank_props *find_bank_props(struct aspeed_gpio_g7 *gpio,
+							      unsigned int offset)
+{
+	const struct aspeed_bank_props *props = gpio->config->props;
+
+	while (!is_bank_props_sentinel(props)) {
+		if (props->bank == GPIO_BANK(offset))
+			return props;
+		props++;
+	}
+
+	return NULL;
+}
+
+static inline bool have_gpio(struct aspeed_gpio_g7 *gpio, unsigned int offset)
+{
+	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+	if (offset > gpio->chip.ngpio)
+		return false;
+
+	return (!props || ((props->input | props->output) & GPIO_BIT(offset)));
+}
+
+static inline bool have_input(struct aspeed_gpio_g7 *gpio, unsigned int offset)
+{
+	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+	return !props || (props->input & GPIO_BIT(offset));
+}
+
+#define have_irq(g, o) have_input((g), (o))
+#define have_debounce(g, o) have_input((g), (o))
+
+static inline bool have_output(struct aspeed_gpio_g7 *gpio, unsigned int offset)
+{
+	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
+
+	return !props || (props->output & GPIO_BIT(offset));
+}
+
+static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
+}
+
+static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	u32 reg;
+
+	reg = gpio->dcache[GPIO_BANK(offset)];
+
+	if (val)
+		reg |= GPIO_BIT(offset);
+	else
+		reg &= ~GPIO_BIT(offset);
+	gpio->dcache[GPIO_BANK(offset)] = reg;
+
+	ast_write_bits(addr, GPIO_G7_OUT_DATA, val);
+}
+
+static void aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	__aspeed_gpio_g7_set(gc, offset, val);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_gpio_g7_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	unsigned long flags;
+
+	if (!have_input(gpio, offset))
+		return -EOPNOTSUPP;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	ast_clr_bits(addr, GPIO_G7_DIR);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_gpio_g7_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	unsigned long flags;
+
+	if (!have_output(gpio, offset))
+		return -EOPNOTSUPP;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	__aspeed_gpio_g7_set(gc, offset, val);
+	ast_write_bits(addr, GPIO_G7_DIR, 1);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_gpio_g7_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+	unsigned long flags;
+	u32 val;
+
+	if (!have_input(gpio, offset))
+		return GPIO_LINE_DIRECTION_OUT;
+
+	if (!have_output(gpio, offset))
+		return GPIO_LINE_DIRECTION_IN;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	val = ioread32(addr) & GPIO_G7_DIR;
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static inline int irqd_to_aspeed_gpio_g7_data(struct irq_data *d, struct aspeed_gpio_g7 **gpio,
+					      int *offset)
+{
+	struct aspeed_gpio_g7 *internal;
+
+	*offset = irqd_to_hwirq(d);
+
+	internal = irq_data_get_irq_chip_data(d);
+
+	/* This might be a bit of a questionable place to check */
+	if (!have_irq(internal, *offset))
+		return -EOPNOTSUPP;
+
+	*gpio = internal;
+
+	return 0;
+}
+
+static void aspeed_gpio_g7_irq_ack(struct irq_data *d)
+{
+	struct aspeed_gpio_g7 *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	int rc, offset;
+
+	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
+	if (rc)
+		return;
+
+	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	ast_write_bits(addr, GPIO_G7_IRQ_STS, 1);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_gpio_g7_irq_set_mask(struct irq_data *d, bool set)
+{
+	struct aspeed_gpio_g7 *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	int rc, offset;
+
+	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
+	if (rc)
+		return;
+
+	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	/* Unmasking the IRQ */
+	if (set)
+		gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	if (set)
+		ast_write_bits(addr, GPIO_G7_IRQ_EN, 1);
+	else
+		ast_clr_bits(addr, GPIO_G7_IRQ_EN);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	/* Masking the IRQ */
+	if (!set)
+		gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(d));
+}
+
+static void aspeed_gpio_g7_irq_mask(struct irq_data *d)
+{
+	aspeed_gpio_g7_irq_set_mask(d, false);
+}
+
+static void aspeed_gpio_g7_irq_unmask(struct irq_data *d)
+{
+	aspeed_gpio_g7_irq_set_mask(d, true);
+}
+
+static int aspeed_gpio_g7_set_type(struct irq_data *d, unsigned int type)
+{
+	u32 type0 = 0;
+	u32 type1 = 0;
+	u32 type2 = 0;
+	irq_flow_handler_t handler;
+	struct aspeed_gpio_g7 *gpio;
+	unsigned long flags;
+	void __iomem *addr;
+	int rc, offset;
+
+	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
+	if (rc)
+		return -EINVAL;
+	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_BOTH:
+		type2 = 1;
+		fallthrough;
+	case IRQ_TYPE_EDGE_RISING:
+		type0 = 1;
+		fallthrough;
+	case IRQ_TYPE_EDGE_FALLING:
+		handler = handle_edge_irq;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		type0 |= 1;
+		fallthrough;
+	case IRQ_TYPE_LEVEL_LOW:
+		type1 |= 1;
+		handler = handle_level_irq;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	ast_write_bits(addr, GPIO_G7_IRQ_TYPE2, type2);
+	ast_write_bits(addr, GPIO_G7_IRQ_TYPE1, type1);
+	ast_write_bits(addr, GPIO_G7_IRQ_TYPE0, type0);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	irq_set_handler_locked(d, handler);
+
+	return 0;
+}
+
+static void aspeed_gpio_g7_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct irq_chip *ic = irq_desc_get_chip(desc);
+	unsigned int i, p, banks;
+	unsigned long reg;
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	void __iomem *addr;
+
+	chained_irq_enter(ic, desc);
+
+	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
+	for (i = 0; i < banks; i++) {
+		addr = gpio->base + GPIO_G7_IRQ_STS_OFFSET(i);
+
+		reg = ioread32(addr);
+
+		for_each_set_bit(p, &reg, 32)
+			generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
+	}
+
+	chained_irq_exit(ic, desc);
+}
+
+static void aspeed_init_irq_valid_mask(struct gpio_chip *gc, unsigned long *valid_mask,
+				       unsigned int ngpios)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
+	const struct aspeed_bank_props *props = gpio->config->props;
+
+	while (!is_bank_props_sentinel(props)) {
+		unsigned int offset;
+		const unsigned long input = props->input;
+
+		/* Pretty crummy approach, but similar to GPIO core */
+		for_each_clear_bit(offset, &input, 32) {
+			unsigned int i = props->bank * 32 + offset;
+
+			if (i >= gpio->chip.ngpio)
+				break;
+
+			clear_bit(i, valid_mask);
+		}
+
+		props++;
+	}
+}
+
+static int aspeed_gpio_g7_reset_tolerance(struct gpio_chip *chip, unsigned int offset, bool enable)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
+	unsigned long flags;
+	void __iomem *addr;
+
+	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	if (enable)
+		ast_write_bits(addr, GPIO_G7_RST_TOLERANCE, 1);
+	else
+		ast_clr_bits(addr, GPIO_G7_RST_TOLERANCE);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return 0;
+}
+
+static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
+{
+	if (!have_gpio(gpiochip_get_data(chip), offset))
+		return -ENODEV;
+
+	return pinctrl_gpio_request(chip->base + offset);
+}
+
+static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
+{
+	pinctrl_gpio_free(chip->base + offset);
+}
+
+static int usecs_to_cycles(struct aspeed_gpio_g7 *gpio, unsigned long usecs, u32 *cycles)
+{
+	u64 rate;
+	u64 n;
+	u32 r;
+
+	rate = clk_get_rate(gpio->clk);
+	if (!rate)
+		return -EOPNOTSUPP;
+
+	n = rate * usecs;
+	r = do_div(n, 1000000);
+
+	if (n >= U32_MAX)
+		return -ERANGE;
+
+	/* At least as long as the requested time */
+	*cycles = n + (!!r);
+
+	return 0;
+}
+
+/* Call under gpio->lock */
+static int register_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset,
+				    unsigned int timer)
+{
+	if (WARN(gpio->offset_timer[offset] != 0, "Offset %d already allocated timer %d\n", offset,
+		 gpio->offset_timer[offset]))
+		return -EINVAL;
+
+	if (WARN(gpio->timer_users[timer] == UINT_MAX, "Timer user count would overflow\n"))
+		return -EPERM;
+
+	gpio->offset_timer[offset] = timer;
+	gpio->timer_users[timer]++;
+
+	return 0;
+}
+
+/* Call under gpio->lock */
+static int unregister_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset)
+{
+	if (WARN(gpio->offset_timer[offset] == 0, "No timer allocated to offset %d\n", offset))
+		return -EINVAL;
+
+	if (WARN(gpio->timer_users[gpio->offset_timer[offset]] == 0,
+		 "No users recorded for timer %d\n", gpio->offset_timer[offset]))
+		return -EINVAL;
+
+	gpio->timer_users[gpio->offset_timer[offset]]--;
+	gpio->offset_timer[offset] = 0;
+
+	return 0;
+}
+
+/* Call under gpio->lock */
+static inline bool timer_allocation_registered(struct aspeed_gpio_g7 *gpio, unsigned int offset)
+{
+	return gpio->offset_timer[offset] > 0;
+}
+
+static void configure_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset, unsigned int timer)
+{
+	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
+
+	/* Note: Debounce timer isn't under control of the command
+	 * source registers, so no need to sync with the coprocessor
+	 */
+	ast_write_bits(addr, GPIO_G7_DEBOUNCE_SEL, timer);
+}
+
+static int enable_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
+	u32 requested_cycles;
+	unsigned long flags;
+	int rc;
+	int i;
+
+	if (!gpio->clk)
+		return -EINVAL;
+
+	rc = usecs_to_cycles(gpio, usecs, &requested_cycles);
+	if (rc < 0) {
+		dev_warn(chip->parent, "Failed to convert %luus to cycles at %luHz: %d\n", usecs,
+			 clk_get_rate(gpio->clk), rc);
+		return rc;
+	}
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	if (timer_allocation_registered(gpio, offset)) {
+		rc = unregister_allocated_timer(gpio, offset);
+		if (rc < 0)
+			goto out;
+	}
+
+	/* Try to find a timer already configured for the debounce period */
+	for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
+		u32 cycles;
+
+		cycles = ioread32(gpio->base + debounce_timers[i]);
+		if (requested_cycles == cycles)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(debounce_timers)) {
+		int j;
+
+		/*
+		 * As there are no timers configured for the requested debounce
+		 * period, find an unused timer instead
+		 */
+		for (j = 1; j < ARRAY_SIZE(gpio->timer_users); j++) {
+			if (gpio->timer_users[j] == 0)
+				break;
+		}
+
+		if (j == ARRAY_SIZE(gpio->timer_users)) {
+			dev_warn(chip->parent,
+				 "Debounce timers exhausted, cannot debounce for period %luus\n",
+				 usecs);
+
+			rc = -EPERM;
+
+			/*
+			 * We already adjusted the accounting to remove @offset
+			 * as a user of its previous timer, so also configure
+			 * the hardware so @offset has timers disabled for
+			 * consistency.
+			 */
+			configure_timer(gpio, offset, 0);
+			goto out;
+		}
+
+		i = j;
+
+		iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
+	}
+
+	if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	register_allocated_timer(gpio, offset, i);
+	configure_timer(gpio, offset, i);
+
+out:
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return rc;
+}
+
+static int disable_debounce(struct gpio_chip *chip, unsigned int offset)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
+	unsigned long flags;
+	int rc;
+
+	raw_spin_lock_irqsave(&gpio->lock, flags);
+
+	rc = unregister_allocated_timer(gpio, offset);
+	if (!rc)
+		configure_timer(gpio, offset, 0);
+
+	raw_spin_unlock_irqrestore(&gpio->lock, flags);
+
+	return rc;
+}
+
+static int set_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
+{
+	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
+
+	if (!have_debounce(gpio, offset))
+		return -EOPNOTSUPP;
+
+	if (usecs)
+		return enable_debounce(chip, offset, usecs);
+
+	return disable_debounce(chip, offset);
+}
+
+static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
+				     unsigned long config)
+{
+	unsigned long param = pinconf_to_config_param(config);
+	u32 arg = pinconf_to_config_argument(config);
+
+	if (param == PIN_CONFIG_INPUT_DEBOUNCE)
+		return set_debounce(chip, offset, arg);
+	else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
+		 param == PIN_CONFIG_DRIVE_STRENGTH)
+		return pinctrl_gpio_set_config(offset, config);
+	else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
+		/* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
+		return -EOPNOTSUPP;
+	else if (param == PIN_CONFIG_PERSIST_STATE)
+		return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
+
+	return -EOPNOTSUPP;
+}
+
+static void aspeed_gpio_g7_irq_print_chip(struct irq_data *d, struct seq_file *p)
+{
+	struct aspeed_gpio_g7 *gpio;
+	int rc, offset;
+
+	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
+	if (rc)
+		return;
+
+	seq_printf(p, dev_name(gpio->dev));
+}
+
+static const struct irq_chip aspeed_gpio_g7_irq_chip = {
+	.irq_ack = aspeed_gpio_g7_irq_ack,
+	.irq_mask = aspeed_gpio_g7_irq_mask,
+	.irq_unmask = aspeed_gpio_g7_irq_unmask,
+	.irq_set_type = aspeed_gpio_g7_set_type,
+	.irq_print_chip = aspeed_gpio_g7_irq_print_chip,
+	.flags = IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static const struct aspeed_bank_props ast2700_bank_props[] = {
+	/*     input	  output   */
+	{ 1, 0x0fffffff, 0x0fffffff }, /* E/F/G/H, 4-GPIO hole */
+	{ 6, 0x00ffffff, 0x00ffffff }, /* Y/Z/AA */
+	{},
+};
+
+static const struct aspeed_gpio_g7_config ast2700_config =
+	/*
+	 * ast2700 has two controllers one with 212 GPIOs and one with 16 GPIOs.
+	 * 216 for simplicity, actual number is 212 (4-GPIO hole in GPIOH)
+	 * We expect ngpio being set in the device tree and this is a fallback
+	 * option.
+	 */
+	{
+		.nr_gpios = 216,
+		.props = ast2700_bank_props,
+	};
+
+static const struct of_device_id aspeed_gpio_g7_of_table[] = {
+	{
+		.compatible = "aspeed,ast2700-gpio",
+		.data = &ast2700_config,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, aspeed_gpio_g7_of_table);
+
+static int __init aspeed_gpio_g7_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *gpio_id;
+	struct aspeed_gpio_g7 *gpio;
+	int rc, banks, err;
+	u32 ngpio;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(gpio->base))
+		return PTR_ERR(gpio->base);
+
+	gpio->dev = &pdev->dev;
+
+	raw_spin_lock_init(&gpio->lock);
+
+	gpio_id = of_match_node(aspeed_gpio_g7_of_table, pdev->dev.of_node);
+	if (!gpio_id)
+		return -EINVAL;
+
+	gpio->clk = of_clk_get(pdev->dev.of_node, 0);
+	if (IS_ERR(gpio->clk)) {
+		dev_warn(&pdev->dev, "Failed to get clock from devicetree, debouncing disabled\n");
+		gpio->clk = NULL;
+	}
+
+	gpio->config = gpio_id->data;
+
+	gpio->chip.parent = &pdev->dev;
+	err = of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio);
+	gpio->chip.ngpio = (u16)ngpio;
+	if (err)
+		gpio->chip.ngpio = gpio->config->nr_gpios;
+	gpio->chip.direction_input = aspeed_gpio_g7_dir_in;
+	gpio->chip.direction_output = aspeed_gpio_g7_dir_out;
+	gpio->chip.get_direction = aspeed_gpio_g7_get_direction;
+	gpio->chip.request = aspeed_gpio_g7_request;
+	gpio->chip.free = aspeed_gpio_g7_free;
+	gpio->chip.get = aspeed_gpio_g7_get;
+	gpio->chip.set = aspeed_gpio_g7_set;
+	gpio->chip.set_config = aspeed_gpio_g7_set_config;
+	gpio->chip.label = dev_name(&pdev->dev);
+	gpio->chip.base = -1;
+
+	/* Allocate a cache of the output registers */
+	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
+	gpio->dcache = devm_kcalloc(&pdev->dev, banks, sizeof(u32), GFP_KERNEL);
+	if (!gpio->dcache)
+		return -ENOMEM;
+
+	/* Optionally set up an irqchip if there is an IRQ */
+	rc = platform_get_irq(pdev, 0);
+	if (rc > 0) {
+		struct gpio_irq_chip *girq;
+
+		gpio->irq = rc;
+		girq = &gpio->chip.irq;
+		gpio_irq_chip_set_chip(girq, &aspeed_gpio_g7_irq_chip);
+		girq->chip->name = dev_name(&pdev->dev);
+
+		girq->parent_handler = aspeed_gpio_g7_irq_handler;
+		girq->num_parents = 1;
+		girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), GFP_KERNEL);
+		if (!girq->parents)
+			return -ENOMEM;
+		girq->parents[0] = gpio->irq;
+		girq->default_type = IRQ_TYPE_NONE;
+		girq->handler = handle_bad_irq;
+		girq->init_valid_mask = aspeed_init_irq_valid_mask;
+	}
+
+	gpio->offset_timer = devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);
+	if (!gpio->offset_timer)
+		return -ENOMEM;
+
+	rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static struct platform_driver aspeed_gpio_g7_driver = {
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = aspeed_gpio_g7_of_table,
+	},
+};
+
+module_platform_driver_probe(aspeed_gpio_g7_driver, aspeed_gpio_g7_probe);
+
+MODULE_DESCRIPTION("Aspeed G7 GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Bartosz Golaszewski]

On Wed, Aug 21, 2024 at 9:07 AM Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>
> In the 7th generation of the SoC from Aspeed, the control logic of the
> GPIO controller has been updated to support per-pin control. Each pin now
> has its own 32-bit register, allowing for individual control of the pin’s
> value, direction, interrupt type, and other settings.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/gpio/Kconfig          |   7 +
>  drivers/gpio/Makefile         |   1 +
>  drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
>  3 files changed, 839 insertions(+)
>  create mode 100644 drivers/gpio/gpio-aspeed-g7.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 58f43bcced7c..93f237429b92 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -172,6 +172,13 @@ config GPIO_ASPEED
>         help
>           Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
>
> +config GPIO_ASPEED_G7
> +       tristate "Aspeed G7 GPIO support"
> +       depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> +       select GPIOLIB_IRQCHIP
> +       help
> +         Say Y here to support Aspeed AST2700 GPIO controllers.
> +
>  config GPIO_ASPEED_SGPIO
>         bool "Aspeed SGPIO support"
>         depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 64dd6d9d730d..e830291761ee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH)            += gpio-amd-fch.o
>  obj-$(CONFIG_GPIO_AMDPT)               += gpio-amdpt.o
>  obj-$(CONFIG_GPIO_ARIZONA)             += gpio-arizona.o
>  obj-$(CONFIG_GPIO_ASPEED)              += gpio-aspeed.o
> +obj-$(CONFIG_GPIO_ASPEED_G7)           += gpio-aspeed-g7.o
>  obj-$(CONFIG_GPIO_ASPEED_SGPIO)                += gpio-aspeed-sgpio.o
>  obj-$(CONFIG_GPIO_ATH79)               += gpio-ath79.o
>  obj-$(CONFIG_GPIO_BCM_KONA)            += gpio-bcm-kona.o
> diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
> new file mode 100644
> index 000000000000..dbca097de6ea
> --- /dev/null
> +++ b/drivers/gpio/gpio-aspeed-g7.c
> @@ -0,0 +1,831 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 Aspeed Technology Inc.
> + *
> + * Billy Tsai <billy_tsai@aspeedtech.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio/aspeed.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hashtable.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +
> +#include <asm/div64.h>
> +
> +#define GPIO_G7_IRQ_STS_BASE 0x100
> +#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
> +#define GPIO_G7_CTRL_REG_BASE 0x180
> +#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
> +#define GPIO_G7_OUT_DATA BIT(0)
> +#define GPIO_G7_DIR BIT(1)
> +#define GPIO_G7_IRQ_EN BIT(2)
> +#define GPIO_G7_IRQ_TYPE0 BIT(3)
> +#define GPIO_G7_IRQ_TYPE1 BIT(4)
> +#define GPIO_G7_IRQ_TYPE2 BIT(5)
> +#define GPIO_G7_RST_TOLERANCE BIT(6)
> +#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
> +#define GPIO_G7_INPUT_MASK BIT(9)
> +#define GPIO_G7_IRQ_STS BIT(12)
> +#define GPIO_G7_IN_DATA BIT(13)
> +/*
> + * The configuration of the following registers should be determined
> + * outside of the GPIO driver.
> + */
> +#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
> +#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
> +#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
> +#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
> +#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
> +#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
> +#define GPIO_G7_IRQ_TO_SIO BIT(3)
> +#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
> +#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
> +
> +static inline u32 field_get(u32 _mask, u32 _val)
> +{
> +       return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> +}
> +
> +static inline u32 field_prep(u32 _mask, u32 _val)
> +{
> +       return (((_val) << (ffs(_mask) - 1)) & (_mask));
> +}
> +
> +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> +{
> +       iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> +}
> +
> +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> +{
> +       iowrite32((ioread32(addr) & ~(mask)), addr);
> +}

For all of the above and similar instances below - can you add the
aspeed prefix to symbols?

[snip]

> +
> +/*
> + * Note: The "value" register returns the input value sampled on the
> + *       line even when the GPIO is configured as an output. Since
> + *       that input goes through synchronizers, writing, then reading

Drop the leading tabs indentations from the comment.

[snip]

> +
> +       register_allocated_timer(gpio, offset, i);
> +       configure_timer(gpio, offset, i);
> +
> +out:
> +       raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +

How about using scoped guards across the driver? You'll avoid such labels.

[snip]

> +
> +static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
> +                                    unsigned long config)
> +{
> +       unsigned long param = pinconf_to_config_param(config);
> +       u32 arg = pinconf_to_config_argument(config);
> +
> +       if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> +               return set_debounce(chip, offset, arg);
> +       else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
> +                param == PIN_CONFIG_DRIVE_STRENGTH)
> +               return pinctrl_gpio_set_config(offset, config);
> +       else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
> +               /* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
> +               return -EOPNOTSUPP;
> +       else if (param == PIN_CONFIG_PERSIST_STATE)
> +               return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
> +

Please use a switch here like everyone else.

> +       return -EOPNOTSUPP;
> +}
> +
> +static void aspeed_gpio_g7_irq_print_chip(struct irq_data *d, struct seq_file *p)
> +{
> +       struct aspeed_gpio_g7 *gpio;
> +       int rc, offset;
> +
> +       rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> +       if (rc)
> +               return;
> +
> +       seq_printf(p, dev_name(gpio->dev));
> +}
> +
> +static const struct irq_chip aspeed_gpio_g7_irq_chip = {
> +       .irq_ack = aspeed_gpio_g7_irq_ack,
> +       .irq_mask = aspeed_gpio_g7_irq_mask,
> +       .irq_unmask = aspeed_gpio_g7_irq_unmask,
> +       .irq_set_type = aspeed_gpio_g7_set_type,
> +       .irq_print_chip = aspeed_gpio_g7_irq_print_chip,
> +       .flags = IRQCHIP_IMMUTABLE,
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};
> +
> +static const struct aspeed_bank_props ast2700_bank_props[] = {
> +       /*     input      output   */
> +       { 1, 0x0fffffff, 0x0fffffff }, /* E/F/G/H, 4-GPIO hole */
> +       { 6, 0x00ffffff, 0x00ffffff }, /* Y/Z/AA */
> +       {},
> +};
> +
> +static const struct aspeed_gpio_g7_config ast2700_config =
> +       /*
> +        * ast2700 has two controllers one with 212 GPIOs and one with 16 GPIOs.
> +        * 216 for simplicity, actual number is 212 (4-GPIO hole in GPIOH)
> +        * We expect ngpio being set in the device tree and this is a fallback
> +        * option.
> +        */
> +       {
> +               .nr_gpios = 216,
> +               .props = ast2700_bank_props,
> +       };
> +
> +static const struct of_device_id aspeed_gpio_g7_of_table[] = {
> +       {
> +               .compatible = "aspeed,ast2700-gpio",
> +               .data = &ast2700_config,
> +       },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_gpio_g7_of_table);
> +
> +static int __init aspeed_gpio_g7_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *gpio_id;
> +       struct aspeed_gpio_g7 *gpio;
> +       int rc, banks, err;
> +       u32 ngpio;
> +
> +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> +       if (!gpio)
> +               return -ENOMEM;
> +
> +       gpio->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(gpio->base))
> +               return PTR_ERR(gpio->base);
> +
> +       gpio->dev = &pdev->dev;
> +
> +       raw_spin_lock_init(&gpio->lock);
> +
> +       gpio_id = of_match_node(aspeed_gpio_g7_of_table, pdev->dev.of_node);

Please use device_get_match_data() and elsewhere use generic device
property getters instead of the specialized OF variants.

> +       if (!gpio_id)
> +               return -EINVAL;
> +
> +       gpio->clk = of_clk_get(pdev->dev.of_node, 0);
> +       if (IS_ERR(gpio->clk)) {
> +               dev_warn(&pdev->dev, "Failed to get clock from devicetree, debouncing disabled\n");
> +               gpio->clk = NULL;
> +       }
> +
> +       gpio->config = gpio_id->data;
> +
> +       gpio->chip.parent = &pdev->dev;
> +       err = of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio);
> +       gpio->chip.ngpio = (u16)ngpio;
> +       if (err)
> +               gpio->chip.ngpio = gpio->config->nr_gpios;
> +       gpio->chip.direction_input = aspeed_gpio_g7_dir_in;
> +       gpio->chip.direction_output = aspeed_gpio_g7_dir_out;
> +       gpio->chip.get_direction = aspeed_gpio_g7_get_direction;
> +       gpio->chip.request = aspeed_gpio_g7_request;
> +       gpio->chip.free = aspeed_gpio_g7_free;
> +       gpio->chip.get = aspeed_gpio_g7_get;
> +       gpio->chip.set = aspeed_gpio_g7_set;
> +       gpio->chip.set_config = aspeed_gpio_g7_set_config;
> +       gpio->chip.label = dev_name(&pdev->dev);
> +       gpio->chip.base = -1;
> +
> +       /* Allocate a cache of the output registers */
> +       banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
> +       gpio->dcache = devm_kcalloc(&pdev->dev, banks, sizeof(u32), GFP_KERNEL);
> +       if (!gpio->dcache)
> +               return -ENOMEM;
> +
> +       /* Optionally set up an irqchip if there is an IRQ */
> +       rc = platform_get_irq(pdev, 0);
> +       if (rc > 0) {
> +               struct gpio_irq_chip *girq;
> +
> +               gpio->irq = rc;
> +               girq = &gpio->chip.irq;
> +               gpio_irq_chip_set_chip(girq, &aspeed_gpio_g7_irq_chip);
> +               girq->chip->name = dev_name(&pdev->dev);
> +
> +               girq->parent_handler = aspeed_gpio_g7_irq_handler;
> +               girq->num_parents = 1;
> +               girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), GFP_KERNEL);
> +               if (!girq->parents)
> +                       return -ENOMEM;
> +               girq->parents[0] = gpio->irq;
> +               girq->default_type = IRQ_TYPE_NONE;
> +               girq->handler = handle_bad_irq;
> +               girq->init_valid_mask = aspeed_init_irq_valid_mask;
> +       }
> +
> +       gpio->offset_timer = devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);
> +       if (!gpio->offset_timer)
> +               return -ENOMEM;
> +
> +       rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);

Just return devm_gpiochip_add_data().

> +       if (rc < 0)
> +               return rc;
> +
> +       return 0;
> +}
> +
> +static struct platform_driver aspeed_gpio_g7_driver = {
> +       .driver = {
> +               .name = KBUILD_MODNAME,
> +               .of_match_table = aspeed_gpio_g7_of_table,
> +       },
> +};
> +
> +module_platform_driver_probe(aspeed_gpio_g7_driver, aspeed_gpio_g7_probe);

I see that it was done like this in other aspeed drivers but I would
need some explanation as to why you think it's needed here. You do get
some resources in probe() that may defer the probing of this driver
and this macro doesn't allow it. Unless you have a very good reason, I
suspect you want to use module_platform_driver() here instead.

> +
> +MODULE_DESCRIPTION("Aspeed G7 GPIO Driver");
> +MODULE_LICENSE("GPL");

MODULE_AUTHOR()?

Bart

> --
> 2.25.1
>

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Billy Tsai]

> >
> > In the 7th generation of the SoC from Aspeed, the control logic of the
> > GPIO controller has been updated to support per-pin control. Each pin now
> > has its own 32-bit register, allowing for individual control of the pin’s
> > value, direction, interrupt type, and other settings.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > ---
> >  drivers/gpio/Kconfig          |   7 +
> >  drivers/gpio/Makefile         |   1 +
> >  drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 839 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-aspeed-g7.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 58f43bcced7c..93f237429b92 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -172,6 +172,13 @@ config GPIO_ASPEED
> >         help
> >           Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
> >
> > +config GPIO_ASPEED_G7
> > +       tristate "Aspeed G7 GPIO support"
> > +       depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> > +       help
> > +         Say Y here to support Aspeed AST2700 GPIO controllers.
> > +
> >  config GPIO_ASPEED_SGPIO
> >         bool "Aspeed SGPIO support"
> >         depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 64dd6d9d730d..e830291761ee 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH)            += gpio-amd-fch.o
> >  obj-$(CONFIG_GPIO_AMDPT)               += gpio-amdpt.o
> >  obj-$(CONFIG_GPIO_ARIZONA)             += gpio-arizona.o
> >  obj-$(CONFIG_GPIO_ASPEED)              += gpio-aspeed.o
> > +obj-$(CONFIG_GPIO_ASPEED_G7)           += gpio-aspeed-g7.o
> >  obj-$(CONFIG_GPIO_ASPEED_SGPIO)                += gpio-aspeed-sgpio.o
> >  obj-$(CONFIG_GPIO_ATH79)               += gpio-ath79.o
> >  obj-$(CONFIG_GPIO_BCM_KONA)            += gpio-bcm-kona.o
> > diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
> > new file mode 100644
> > index 000000000000..dbca097de6ea
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-aspeed-g7.c
> > @@ -0,0 +1,831 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 Aspeed Technology Inc.
> > + *
> > + * Billy Tsai <billy_tsai@aspeedtech.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/gpio/aspeed.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +
> > +#include <asm/div64.h>
> > +
> > +#define GPIO_G7_IRQ_STS_BASE 0x100
> > +#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
> > +#define GPIO_G7_CTRL_REG_BASE 0x180
> > +#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
> > +#define GPIO_G7_OUT_DATA BIT(0)
> > +#define GPIO_G7_DIR BIT(1)
> > +#define GPIO_G7_IRQ_EN BIT(2)
> > +#define GPIO_G7_IRQ_TYPE0 BIT(3)
> > +#define GPIO_G7_IRQ_TYPE1 BIT(4)
> > +#define GPIO_G7_IRQ_TYPE2 BIT(5)
> > +#define GPIO_G7_RST_TOLERANCE BIT(6)
> > +#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
> > +#define GPIO_G7_INPUT_MASK BIT(9)
> > +#define GPIO_G7_IRQ_STS BIT(12)
> > +#define GPIO_G7_IN_DATA BIT(13)
> > +/*
> > + * The configuration of the following registers should be determined
> > + * outside of the GPIO driver.
> > + */
> > +#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
> > +#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
> > +#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
> > +#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
> > +#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
> > +#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
> > +#define GPIO_G7_IRQ_TO_SIO BIT(3)
> > +#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
> > +#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
> > +
> > +static inline u32 field_get(u32 _mask, u32 _val)
> > +{
> > +       return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> > +}
> > +
> > +static inline u32 field_prep(u32 _mask, u32 _val)
> > +{
> > +       return (((_val) << (ffs(_mask) - 1)) & (_mask));
> > +}
> > +
> > +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> > +{
> > +       iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> > +}
> > +
> > +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> > +{
> > +       iowrite32((ioread32(addr) & ~(mask)), addr);
> > +}

> For all of the above and similar instances below - can you add the
> aspeed prefix to symbols?

Okay, I will add the aspeed prefix to these functions or use regmap to replace them.

> [snip]


> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + *       line even when the GPIO is configured as an output. Since
> > + *       that input goes through synchronizers, writing, then reading

> Drop the leading tabs indentations from the comment.

Okay.

> [snip]
 
> > +
> > +       register_allocated_timer(gpio, offset, i);
> > +       configure_timer(gpio, offset, i);
> > +
> > +out:
> > +       raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> 
> How about using scoped guards across the driver? You'll avoid such labels.

Okay, I will use the guard(raw_spinlock_irqsave)(&gpio->lock) to replace it.

> [snip]

> > +
> > +static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
> > +                                    unsigned long config)
> > +{
> > +       unsigned long param = pinconf_to_config_param(config);
> > +       u32 arg = pinconf_to_config_argument(config);
> > +
> > +       if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> > +               return set_debounce(chip, offset, arg);
> > +       else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
> > +                param == PIN_CONFIG_DRIVE_STRENGTH)
> > +               return pinctrl_gpio_set_config(offset, config);
> > +       else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
> > +               /* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
> > +               return -EOPNOTSUPP;
> > +       else if (param == PIN_CONFIG_PERSIST_STATE)
> > +               return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
> > +
 
> Please use a switch here like everyone else.

Okay.

> > +       return -EOPNOTSUPP;
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_print_chip(struct irq_data *d, struct seq_file *p)
> > +{
> > +       struct aspeed_gpio_g7 *gpio;
> > +       int rc, offset;
> > +
> > +       rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > +       if (rc)
> > +               return;
> > +
> > +       seq_printf(p, dev_name(gpio->dev));
> > +}
> > +
> > +static const struct irq_chip aspeed_gpio_g7_irq_chip = {
> > +       .irq_ack = aspeed_gpio_g7_irq_ack,
> > +       .irq_mask = aspeed_gpio_g7_irq_mask,
> > +       .irq_unmask = aspeed_gpio_g7_irq_unmask,
> > +       .irq_set_type = aspeed_gpio_g7_set_type,
> > +       .irq_print_chip = aspeed_gpio_g7_irq_print_chip,
> > +       .flags = IRQCHIP_IMMUTABLE,
> > +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> > +};
> > +
> > +static const struct aspeed_bank_props ast2700_bank_props[] = {
> > +       /*     input      output   */
> > +       { 1, 0x0fffffff, 0x0fffffff }, /* E/F/G/H, 4-GPIO hole */
> > +       { 6, 0x00ffffff, 0x00ffffff }, /* Y/Z/AA */
> > +       {},
> > +};
> > +
> > +static const struct aspeed_gpio_g7_config ast2700_config =
> > +       /*
> > +        * ast2700 has two controllers one with 212 GPIOs and one with 16 GPIOs.
> > +        * 216 for simplicity, actual number is 212 (4-GPIO hole in GPIOH)
> > +        * We expect ngpio being set in the device tree and this is a fallback
> > +        * option.
> > +        */
> > +       {
> > +               .nr_gpios = 216,
> > +               .props = ast2700_bank_props,
> > +       };
> > +
> > +static const struct of_device_id aspeed_gpio_g7_of_table[] = {
> > +       {
> > +               .compatible = "aspeed,ast2700-gpio",
> > +               .data = &ast2700_config,
> > +       },
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_gpio_g7_of_table);
> > +
> > +static int __init aspeed_gpio_g7_probe(struct platform_device *pdev)
> > +{
> > +       const struct of_device_id *gpio_id;
> > +       struct aspeed_gpio_g7 *gpio;
> > +       int rc, banks, err;
> > +       u32 ngpio;
> > +
> > +       gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > +       if (!gpio)
> > +               return -ENOMEM;
> > +
> > +       gpio->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(gpio->base))
> > +               return PTR_ERR(gpio->base);
> > +
> > +       gpio->dev = &pdev->dev;
> > +
> > +       raw_spin_lock_init(&gpio->lock);
> > +
> > +       gpio_id = of_match_node(aspeed_gpio_g7_of_table, pdev->dev.of_node);

> Please use device_get_match_data() and elsewhere use generic device
> property getters instead of the specialized OF variants.

Okay.

> > +       if (!gpio_id)
> > +               return -EINVAL;
> > +
> > +       gpio->clk = of_clk_get(pdev->dev.of_node, 0);
> > +       if (IS_ERR(gpio->clk)) {
> > +               dev_warn(&pdev->dev, "Failed to get clock from devicetree, debouncing disabled\n");
> > +               gpio->clk = NULL;
> > +       }
> > +
> > +       gpio->config = gpio_id->data;
> > +
> > +       gpio->chip.parent = &pdev->dev;
> > +       err = of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio);
> > +       gpio->chip.ngpio = (u16)ngpio;
> > +       if (err)
> > +               gpio->chip.ngpio = gpio->config->nr_gpios;
> > +       gpio->chip.direction_input = aspeed_gpio_g7_dir_in;
> > +       gpio->chip.direction_output = aspeed_gpio_g7_dir_out;
> > +       gpio->chip.get_direction = aspeed_gpio_g7_get_direction;
> > +       gpio->chip.request = aspeed_gpio_g7_request;
> > +       gpio->chip.free = aspeed_gpio_g7_free;
> > +       gpio->chip.get = aspeed_gpio_g7_get;
> > +       gpio->chip.set = aspeed_gpio_g7_set;
> > +       gpio->chip.set_config = aspeed_gpio_g7_set_config;
> > +       gpio->chip.label = dev_name(&pdev->dev);
> > +       gpio->chip.base = -1;
> > +
> > +       /* Allocate a cache of the output registers */
> > +       banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
> > +       gpio->dcache = devm_kcalloc(&pdev->dev, banks, sizeof(u32), GFP_KERNEL);
> > +       if (!gpio->dcache)
> > +               return -ENOMEM;
> > +
> > +       /* Optionally set up an irqchip if there is an IRQ */
> > +       rc = platform_get_irq(pdev, 0);
> > +       if (rc > 0) {
> > +               struct gpio_irq_chip *girq;
> > +
> > +               gpio->irq = rc;
> > +               girq = &gpio->chip.irq;
> > +               gpio_irq_chip_set_chip(girq, &aspeed_gpio_g7_irq_chip);
> > +               girq->chip->name = dev_name(&pdev->dev);
> > +
> > +               girq->parent_handler = aspeed_gpio_g7_irq_handler;
> > +               girq->num_parents = 1;
> > +               girq->parents = devm_kcalloc(&pdev->dev, 1, sizeof(*girq->parents), GFP_KERNEL);
> > +               if (!girq->parents)
> > +                       return -ENOMEM;
> > +               girq->parents[0] = gpio->irq;
> > +               girq->default_type = IRQ_TYPE_NONE;
> > +               girq->handler = handle_bad_irq;
> > +               girq->init_valid_mask = aspeed_init_irq_valid_mask;
> > +       }
> > +
> > +       gpio->offset_timer = devm_kzalloc(&pdev->dev, gpio->chip.ngpio, GFP_KERNEL);
> > +       if (!gpio->offset_timer)
> > +               return -ENOMEM;
> > +
> > +       rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
 
> Just return devm_gpiochip_add_data().

Okay.

> > +       if (rc < 0)
> > +               return rc;
> > +
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver aspeed_gpio_g7_driver = {
> > +       .driver = {
> > +               .name = KBUILD_MODNAME,
> > +               .of_match_table = aspeed_gpio_g7_of_table,
> > +       },
> > +};
> > +
> > +module_platform_driver_probe(aspeed_gpio_g7_driver, aspeed_gpio_g7_probe);

> I see that it was done like this in other aspeed drivers but I would
> need some explanation as to why you think it's needed here. You do get
> some resources in probe() that may defer the probing of this driver
> and this macro doesn't allow it. Unless you have a very good reason, I
> suspect you want to use module_platform_driver() here instead.

Okay, I will replace it to module_platform_driver(aspeed_gpio_g7_driver); and hook the probe
function into the aspeed_gpio_g7_driver structure.

> > +
> > +MODULE_DESCRIPTION("Aspeed G7 GPIO Driver");
> > +MODULE_LICENSE("GPL");

> MODULE_AUTHOR()?

Okay.

Best regards,
Billy Tsai

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[kernel test robot]

Hi Billy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on linus/master v6.11-rc4 next-20240821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/dt-bindings-gpio-aspeed-ast2400-gpio-Support-ast2700/20240821-150951
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240821070740.2378602-3-billy_tsai%40aspeedtech.com
patch subject: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240822/202408220439.BUcaNSTv-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408220439.BUcaNSTv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408220439.BUcaNSTv-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_request':
>> drivers/gpio/gpio-aspeed-g7.c:474:48: warning: passing argument 1 of 'pinctrl_gpio_request' makes pointer from integer without a cast [-Wint-conversion]
     474 |         return pinctrl_gpio_request(chip->base + offset);
         |                                     ~~~~~~~~~~~^~~~~~~~
         |                                                |
         |                                                unsigned int
   In file included from drivers/gpio/gpio-aspeed-g7.c:16:
   include/linux/pinctrl/consumer.h:30:44: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      30 | int pinctrl_gpio_request(struct gpio_chip *gc, unsigned int offset);
         |                          ~~~~~~~~~~~~~~~~~~^~
   drivers/gpio/gpio-aspeed-g7.c:474:16: error: too few arguments to function 'pinctrl_gpio_request'
     474 |         return pinctrl_gpio_request(chip->base + offset);
         |                ^~~~~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:30:5: note: declared here
      30 | int pinctrl_gpio_request(struct gpio_chip *gc, unsigned int offset);
         |     ^~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_free':
>> drivers/gpio/gpio-aspeed-g7.c:479:38: warning: passing argument 1 of 'pinctrl_gpio_free' makes pointer from integer without a cast [-Wint-conversion]
     479 |         pinctrl_gpio_free(chip->base + offset);
         |                           ~~~~~~~~~~~^~~~~~~~
         |                                      |
         |                                      unsigned int
   include/linux/pinctrl/consumer.h:31:42: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      31 | void pinctrl_gpio_free(struct gpio_chip *gc, unsigned int offset);
         |                        ~~~~~~~~~~~~~~~~~~^~
   drivers/gpio/gpio-aspeed-g7.c:479:9: error: too few arguments to function 'pinctrl_gpio_free'
     479 |         pinctrl_gpio_free(chip->base + offset);
         |         ^~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:31:6: note: declared here
      31 | void pinctrl_gpio_free(struct gpio_chip *gc, unsigned int offset);
         |      ^~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_set_config':
>> drivers/gpio/gpio-aspeed-g7.c:676:48: warning: passing argument 1 of 'pinctrl_gpio_set_config' makes pointer from integer without a cast [-Wint-conversion]
     676 |                 return pinctrl_gpio_set_config(offset, config);
         |                                                ^~~~~~
         |                                                |
         |                                                unsigned int
   include/linux/pinctrl/consumer.h:36:47: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      36 | int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
         |                             ~~~~~~~~~~~~~~~~~~^~
   drivers/gpio/gpio-aspeed-g7.c:676:24: error: too few arguments to function 'pinctrl_gpio_set_config'
     676 |                 return pinctrl_gpio_set_config(offset, config);
         |                        ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:36:5: note: declared here
      36 | int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_request':
   drivers/gpio/gpio-aspeed-g7.c:475:1: warning: control reaches end of non-void function [-Wreturn-type]
     475 | }
         | ^


vim +/pinctrl_gpio_request +474 drivers/gpio/gpio-aspeed-g7.c

   468	
   469	static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
   470	{
   471		if (!have_gpio(gpiochip_get_data(chip), offset))
   472			return -ENODEV;
   473	
 > 474		return pinctrl_gpio_request(chip->base + offset);
   475	}
   476	
   477	static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
   478	{
 > 479		pinctrl_gpio_free(chip->base + offset);
   480	}
   481	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[kernel test robot]

Hi Billy,

kernel test robot noticed the following build errors:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.11-rc4 next-20240821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/dt-bindings-gpio-aspeed-ast2400-gpio-Support-ast2700/20240821-150951
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240821070740.2378602-3-billy_tsai%40aspeedtech.com
patch subject: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20240822/202408220503.hxIARcuf-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408220503.hxIARcuf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408220503.hxIARcuf-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_request':
>> drivers/gpio/gpio-aspeed-g7.c:474:48: error: passing argument 1 of 'pinctrl_gpio_request' makes pointer from integer without a cast [-Wint-conversion]
     474 |         return pinctrl_gpio_request(chip->base + offset);
         |                                     ~~~~~~~~~~~^~~~~~~~
         |                                                |
         |                                                unsigned int
   In file included from drivers/gpio/gpio-aspeed-g7.c:16:
   include/linux/pinctrl/consumer.h:30:44: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      30 | int pinctrl_gpio_request(struct gpio_chip *gc, unsigned int offset);
         |                          ~~~~~~~~~~~~~~~~~~^~
>> drivers/gpio/gpio-aspeed-g7.c:474:16: error: too few arguments to function 'pinctrl_gpio_request'
     474 |         return pinctrl_gpio_request(chip->base + offset);
         |                ^~~~~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:30:5: note: declared here
      30 | int pinctrl_gpio_request(struct gpio_chip *gc, unsigned int offset);
         |     ^~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_free':
>> drivers/gpio/gpio-aspeed-g7.c:479:38: error: passing argument 1 of 'pinctrl_gpio_free' makes pointer from integer without a cast [-Wint-conversion]
     479 |         pinctrl_gpio_free(chip->base + offset);
         |                           ~~~~~~~~~~~^~~~~~~~
         |                                      |
         |                                      unsigned int
   include/linux/pinctrl/consumer.h:31:42: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      31 | void pinctrl_gpio_free(struct gpio_chip *gc, unsigned int offset);
         |                        ~~~~~~~~~~~~~~~~~~^~
>> drivers/gpio/gpio-aspeed-g7.c:479:9: error: too few arguments to function 'pinctrl_gpio_free'
     479 |         pinctrl_gpio_free(chip->base + offset);
         |         ^~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:31:6: note: declared here
      31 | void pinctrl_gpio_free(struct gpio_chip *gc, unsigned int offset);
         |      ^~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_set_config':
>> drivers/gpio/gpio-aspeed-g7.c:676:48: error: passing argument 1 of 'pinctrl_gpio_set_config' makes pointer from integer without a cast [-Wint-conversion]
     676 |                 return pinctrl_gpio_set_config(offset, config);
         |                                                ^~~~~~
         |                                                |
         |                                                unsigned int
   include/linux/pinctrl/consumer.h:36:47: note: expected 'struct gpio_chip *' but argument is of type 'unsigned int'
      36 | int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
         |                             ~~~~~~~~~~~~~~~~~~^~
>> drivers/gpio/gpio-aspeed-g7.c:676:24: error: too few arguments to function 'pinctrl_gpio_set_config'
     676 |                 return pinctrl_gpio_set_config(offset, config);
         |                        ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/pinctrl/consumer.h:36:5: note: declared here
      36 | int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c: In function 'aspeed_gpio_g7_request':
>> drivers/gpio/gpio-aspeed-g7.c:475:1: warning: control reaches end of non-void function [-Wreturn-type]
     475 | }
         | ^


vim +/pinctrl_gpio_request +474 drivers/gpio/gpio-aspeed-g7.c

   468	
   469	static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
   470	{
   471		if (!have_gpio(gpiochip_get_data(chip), offset))
   472			return -ENODEV;
   473	
 > 474		return pinctrl_gpio_request(chip->base + offset);
 > 475	}
   476	
   477	static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
   478	{
 > 479		pinctrl_gpio_free(chip->base + offset);
   480	}
   481	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Andrew Jeffery]

Hi Billy,

On Wed, 2024-08-21 at 15:07 +0800, Billy Tsai wrote:
> In the 7th generation of the SoC from Aspeed, the control logic of the
> GPIO controller has been updated to support per-pin control. Each pin now
> has its own 32-bit register, allowing for individual control of the pin’s
> value, direction, interrupt type, and other settings.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/gpio/Kconfig          |   7 +
>  drivers/gpio/Makefile         |   1 +
>  drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
>  3 files changed, 839 insertions(+)
>  create mode 100644 drivers/gpio/gpio-aspeed-g7.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 58f43bcced7c..93f237429b92 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -172,6 +172,13 @@ config GPIO_ASPEED
>  	help
>  	  Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
>  
> +config GPIO_ASPEED_G7
> +	tristate "Aspeed G7 GPIO support"
> +	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> +	select GPIOLIB_IRQCHIP
> +	help
> +	  Say Y here to support Aspeed AST2700 GPIO controllers.
> +
>  config GPIO_ASPEED_SGPIO
>  	bool "Aspeed SGPIO support"
>  	depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 64dd6d9d730d..e830291761ee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH)		+= gpio-amd-fch.o
>  obj-$(CONFIG_GPIO_AMDPT)		+= gpio-amdpt.o
>  obj-$(CONFIG_GPIO_ARIZONA)		+= gpio-arizona.o
>  obj-$(CONFIG_GPIO_ASPEED)		+= gpio-aspeed.o
> +obj-$(CONFIG_GPIO_ASPEED_G7)		+= gpio-aspeed-g7.o
>  obj-$(CONFIG_GPIO_ASPEED_SGPIO)		+= gpio-aspeed-sgpio.o
>  obj-$(CONFIG_GPIO_ATH79)		+= gpio-ath79.o
>  obj-$(CONFIG_GPIO_BCM_KONA)		+= gpio-bcm-kona.o
> diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
> new file mode 100644
> index 000000000000..dbca097de6ea
> --- /dev/null
> +++ b/drivers/gpio/gpio-aspeed-g7.c
> @@ -0,0 +1,831 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2024 Aspeed Technology Inc.
> + *
> + * Billy Tsai <billy_tsai@aspeedtech.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio/aspeed.h>

I think you should either drop this include or rework the existing
implementations so the coprocessor handshake works correctly.

> +#include <linux/gpio/driver.h>
> +#include <linux/hashtable.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/seq_file.h>
> +#include <linux/spinlock.h>
> +#include <linux/string.h>
> +
> +#include <asm/div64.h>
> +
> +#define GPIO_G7_IRQ_STS_BASE 0x100
> +#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
> +#define GPIO_G7_CTRL_REG_BASE 0x180
> +#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
> +#define GPIO_G7_OUT_DATA BIT(0)
> +#define GPIO_G7_DIR BIT(1)
> +#define GPIO_G7_IRQ_EN BIT(2)
> +#define GPIO_G7_IRQ_TYPE0 BIT(3)
> +#define GPIO_G7_IRQ_TYPE1 BIT(4)
> +#define GPIO_G7_IRQ_TYPE2 BIT(5)
> +#define GPIO_G7_RST_TOLERANCE BIT(6)
> +#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
> +#define GPIO_G7_INPUT_MASK BIT(9)
> +#define GPIO_G7_IRQ_STS BIT(12)
> +#define GPIO_G7_IN_DATA BIT(13)

Can you please add `CTRL` into these field macro names so it's clear
they relate to the control register?

> +/*
> + * The configuration of the following registers should be determined
> + * outside of the GPIO driver.

Where though?

> + */
> +#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
> +#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
> +#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
> +#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
> +#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
> +#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
> +#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
> +#define GPIO_G7_IRQ_TO_SIO BIT(3)
> +#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
> +#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
> +
> +static inline u32 field_get(u32 _mask, u32 _val)
> +{
> +	return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> +}
> +
> +static inline u32 field_prep(u32 _mask, u32 _val)
> +{
> +	return (((_val) << (ffs(_mask) - 1)) & (_mask));
> +}

So linux/bitfield.h provides a lot of APIs along these lines, perhaps
use them instead?

> +
> +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> +{
> +	iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> +}
> +
> +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> +{
> +	iowrite32((ioread32(addr) & ~(mask)), addr);
> +}
> +
> +struct aspeed_bank_props {
> +	unsigned int bank;
> +	u32 input;
> +	u32 output;
> +};
> +
> +struct aspeed_gpio_g7_config {
> +	unsigned int nr_gpios;
> +	const struct aspeed_bank_props *props;
> +};
> +
> +/*
> + * @offset_timer: Maps an offset to an @timer_users index, or zero if disabled
> + * @timer_users: Tracks the number of users for each timer
> + *
> + * The @timer_users has four elements but the first element is unused. This is
> + * to simplify accounting and indexing, as a zero value in @offset_timer
> + * represents disabled debouncing for the GPIO. Any other value for an element
> + * of @offset_timer is used as an index into @timer_users. This behaviour of
> + * the zero value aligns with the behaviour of zero built from the timer
> + * configuration registers (i.e. debouncing is disabled).
> + */
> +struct aspeed_gpio_g7 {
> +	struct gpio_chip chip;
> +	struct device *dev;
> +	raw_spinlock_t lock;
> +	void __iomem *base;
> +	int irq;
> +	const struct aspeed_gpio_g7_config *config;
> +
> +	u8 *offset_timer;
> +	unsigned int timer_users[4];
> +	struct clk *clk;
> +
> +	u32 *dcache;
> +};
> +
> +/*
> + * Note: The "value" register returns the input value sampled on the
> + *       line even when the GPIO is configured as an output. Since
> + *       that input goes through synchronizers, writing, then reading
> + *       back may not return the written value right away.
> + *
> + *       The "rdata" register returns the content of the write latch
> + *       and thus can be used to read back what was last written
> + *       reliably.
> + */
> +
> +static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };

This is all largely copy/pasted from gpio-aspeed.c. Can we split it out
and share the definitions?

> +
> +#define GPIO_BANK(x) ((x) >> 5)
> +#define GPIO_OFFSET(x) ((x) & 0x1f)
> +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> +
> +static inline bool is_bank_props_sentinel(const struct aspeed_bank_props *props)
> +{
> +	return !(props->input || props->output);
> +}
> +
> +static inline const struct aspeed_bank_props *find_bank_props(struct aspeed_gpio_g7 *gpio,
> +							      unsigned int offset)
> +{
> +	const struct aspeed_bank_props *props = gpio->config->props;
> +
> +	while (!is_bank_props_sentinel(props)) {
> +		if (props->bank == GPIO_BANK(offset))
> +			return props;
> +		props++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static inline bool have_gpio(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> +{
> +	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> +
> +	if (offset > gpio->chip.ngpio)
> +		return false;
> +
> +	return (!props || ((props->input | props->output) & GPIO_BIT(offset)));
> +}
> +
> +static inline bool have_input(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> +{
> +	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> +
> +	return !props || (props->input & GPIO_BIT(offset));
> +}
> +
> +#define have_irq(g, o) have_input((g), (o))
> +#define have_debounce(g, o) have_input((g), (o))
> +
> +static inline bool have_output(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> +{
> +	const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> +
> +	return !props || (props->output & GPIO_BIT(offset));
> +}
> +

This is all common as well.

> +static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
> +}
> +
> +static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);

The rest of the implementation of this function is broadly the same as
in gpio-aspeed.c. The main difference is accounting for the address to
access and the bit to whack. If we define some callbacks that replace
GPIO_BANK()/to_bank() and GPIO_BIT() that can account for the
differences in register layout, perhaps this could be common?

The trade-off is some complexity vs copy-paste, but there does seem to
be an awful lot of the latter with only minor changes so far.

> +	u32 reg;
> +
> +	reg = gpio->dcache[GPIO_BANK(offset)];
> +
> +	if (val)
> +		reg |= GPIO_BIT(offset);
> +	else
> +		reg &= ~GPIO_BIT(offset);
> +	gpio->dcache[GPIO_BANK(offset)] = reg;
> +
> +	ast_write_bits(addr, GPIO_G7_OUT_DATA, val);
> +}
> +
> +static void aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	__aspeed_gpio_g7_set(gc, offset, val);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static int aspeed_gpio_g7_dir_in(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +	unsigned long flags;
> +
> +	if (!have_input(gpio, offset))
> +		return -EOPNOTSUPP;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	ast_clr_bits(addr, GPIO_G7_DIR);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int aspeed_gpio_g7_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +	unsigned long flags;
> +
> +	if (!have_output(gpio, offset))
> +		return -EOPNOTSUPP;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	__aspeed_gpio_g7_set(gc, offset, val);
> +	ast_write_bits(addr, GPIO_G7_DIR, 1);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int aspeed_gpio_g7_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (!have_input(gpio, offset))
> +		return GPIO_LINE_DIRECTION_OUT;
> +
> +	if (!have_output(gpio, offset))
> +		return GPIO_LINE_DIRECTION_IN;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	val = ioread32(addr) & GPIO_G7_DIR;
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> +}

On top of handling the differences in the register layout as I
mentioned above, the main difference in these get/set implementations
is dropping the calls through the coprocessor handshake APIs. If you
stub out the implementation of the coprocessor APIs I think it's likely
these can be common. To do that you would need to make them use
callbacks into the SoC-specific driver. To stub out the implementation
you could leave the callback pointer as NULL for now.

> +
> +static inline int irqd_to_aspeed_gpio_g7_data(struct irq_data *d, struct aspeed_gpio_g7 **gpio,
> +					      int *offset)
> +{
> +	struct aspeed_gpio_g7 *internal;
> +
> +	*offset = irqd_to_hwirq(d);
> +
> +	internal = irq_data_get_irq_chip_data(d);
> +
> +	/* This might be a bit of a questionable place to check */
> +	if (!have_irq(internal, *offset))
> +		return -EOPNOTSUPP;
> +
> +	*gpio = internal;
> +
> +	return 0;
> +}

You do have different data-types here (struct aspeed_gpio_g7), but
possibly with appropriate struct definitions and use of container_of()
by the caller, this could be common too?

> +
> +static void aspeed_gpio_g7_irq_ack(struct irq_data *d)
> +{
> +	struct aspeed_gpio_g7 *gpio;
> +	unsigned long flags;
> +	void __iomem *addr;
> +	int rc, offset;
> +
> +	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> +	if (rc)
> +		return;
> +
> +	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	ast_write_bits(addr, GPIO_G7_IRQ_STS, 1);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +}
> +
> +static void aspeed_gpio_g7_irq_set_mask(struct irq_data *d, bool set)
> +{
> +	struct aspeed_gpio_g7 *gpio;
> +	unsigned long flags;
> +	void __iomem *addr;
> +	int rc, offset;
> +
> +	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> +	if (rc)
> +		return;
> +
> +	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	/* Unmasking the IRQ */
> +	if (set)
> +		gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	if (set)
> +		ast_write_bits(addr, GPIO_G7_IRQ_EN, 1);
> +	else
> +		ast_clr_bits(addr, GPIO_G7_IRQ_EN);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	/* Masking the IRQ */
> +	if (!set)
> +		gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(d));
> +}
> +
> +static void aspeed_gpio_g7_irq_mask(struct irq_data *d)
> +{
> +	aspeed_gpio_g7_irq_set_mask(d, false);
> +}
> +
> +static void aspeed_gpio_g7_irq_unmask(struct irq_data *d)
> +{
> +	aspeed_gpio_g7_irq_set_mask(d, true);
> +}
> +
> +static int aspeed_gpio_g7_set_type(struct irq_data *d, unsigned int type)
> +{
> +	u32 type0 = 0;
> +	u32 type1 = 0;
> +	u32 type2 = 0;
> +	irq_flow_handler_t handler;
> +	struct aspeed_gpio_g7 *gpio;
> +	unsigned long flags;
> +	void __iomem *addr;
> +	int rc, offset;
> +
> +	rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> +	if (rc)
> +		return -EINVAL;
> +	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +		type2 = 1;
> +		fallthrough;
> +	case IRQ_TYPE_EDGE_RISING:
> +		type0 = 1;
> +		fallthrough;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		handler = handle_edge_irq;
> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		type0 |= 1;
> +		fallthrough;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		type1 |= 1;
> +		handler = handle_level_irq;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	ast_write_bits(addr, GPIO_G7_IRQ_TYPE2, type2);
> +	ast_write_bits(addr, GPIO_G7_IRQ_TYPE1, type1);
> +	ast_write_bits(addr, GPIO_G7_IRQ_TYPE0, type0);

Can we write them as a field in the one call? They're all in the same
register, which was not true in the previous controller register
layout.

> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	irq_set_handler_locked(d, handler);
> +
> +	return 0;
> +}
> +
> +static void aspeed_gpio_g7_irq_handler(struct irq_desc *desc)
> +{
> +	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> +	struct irq_chip *ic = irq_desc_get_chip(desc);
> +	unsigned int i, p, banks;
> +	unsigned long reg;
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	void __iomem *addr;
> +
> +	chained_irq_enter(ic, desc);
> +
> +	banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
> +	for (i = 0; i < banks; i++) {
> +		addr = gpio->base + GPIO_G7_IRQ_STS_OFFSET(i);
> +
> +		reg = ioread32(addr);
> +
> +		for_each_set_bit(p, &reg, 32)
> +			generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
> +	}
> +
> +	chained_irq_exit(ic, desc);
> +}

The only thing that's different for the IRQ status handling is the
spread of the registers in the layout. In terms of the bits in each
bank's IRQ status register, the layout is the same. By implementing the
means to locate the status register as a callback this code could be
common between the drivers.

> +
> +static void aspeed_init_irq_valid_mask(struct gpio_chip *gc, unsigned long *valid_mask,
> +				       unsigned int ngpios)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> +	const struct aspeed_bank_props *props = gpio->config->props;
> +
> +	while (!is_bank_props_sentinel(props)) {
> +		unsigned int offset;
> +		const unsigned long input = props->input;
> +
> +		/* Pretty crummy approach, but similar to GPIO core */
> +		for_each_clear_bit(offset, &input, 32) {
> +			unsigned int i = props->bank * 32 + offset;
> +
> +			if (i >= gpio->chip.ngpio)
> +				break;
> +
> +			clear_bit(i, valid_mask);
> +		}
> +
> +		props++;
> +	}
> +}

This is the same except for the change to use `struct aspeed_gpio_g7`?
Can we make this common?

> +
> +static int aspeed_gpio_g7_reset_tolerance(struct gpio_chip *chip, unsigned int offset, bool enable)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> +	unsigned long flags;
> +	void __iomem *addr;
> +
> +	addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	if (enable)
> +		ast_write_bits(addr, GPIO_G7_RST_TOLERANCE, 1);
> +	else
> +		ast_clr_bits(addr, GPIO_G7_RST_TOLERANCE);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	if (!have_gpio(gpiochip_get_data(chip), offset))
> +		return -ENODEV;
> +
> +	return pinctrl_gpio_request(chip->base + offset);

pinctrl_gpio_request() takes the chip and the offset value separately.
It looks like you've developed this patch against an older kernel tree?

> +}
> +
> +static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +	pinctrl_gpio_free(chip->base + offset);

Same as above for pinctrl_gpio_free().

> +}
> +
> +static int usecs_to_cycles(struct aspeed_gpio_g7 *gpio, unsigned long usecs, u32 *cycles)
> +{
> +	u64 rate;
> +	u64 n;
> +	u32 r;
> +
> +	rate = clk_get_rate(gpio->clk);
> +	if (!rate)
> +		return -EOPNOTSUPP;
> +
> +	n = rate * usecs;
> +	r = do_div(n, 1000000);
> +
> +	if (n >= U32_MAX)
> +		return -ERANGE;
> +
> +	/* At least as long as the requested time */
> +	*cycles = n + (!!r);
> +
> +	return 0;
> +}
> +
> +/* Call under gpio->lock */
> +static int register_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset,
> +				    unsigned int timer)
> +{
> +	if (WARN(gpio->offset_timer[offset] != 0, "Offset %d already allocated timer %d\n", offset,
> +		 gpio->offset_timer[offset]))
> +		return -EINVAL;
> +
> +	if (WARN(gpio->timer_users[timer] == UINT_MAX, "Timer user count would overflow\n"))
> +		return -EPERM;
> +
> +	gpio->offset_timer[offset] = timer;
> +	gpio->timer_users[timer]++;
> +
> +	return 0;
> +}
> +
> +/* Call under gpio->lock */
> +static int unregister_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> +{
> +	if (WARN(gpio->offset_timer[offset] == 0, "No timer allocated to offset %d\n", offset))
> +		return -EINVAL;
> +
> +	if (WARN(gpio->timer_users[gpio->offset_timer[offset]] == 0,
> +		 "No users recorded for timer %d\n", gpio->offset_timer[offset]))
> +		return -EINVAL;
> +
> +	gpio->timer_users[gpio->offset_timer[offset]]--;
> +	gpio->offset_timer[offset] = 0;
> +
> +	return 0;
> +}
> +
> +/* Call under gpio->lock */
> +static inline bool timer_allocation_registered(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> +{
> +	return gpio->offset_timer[offset] > 0;
> +}

The above functions have all been copy/pasted, can we make them common?
> +
> +static void configure_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset, unsigned int timer)
> +{
> +	void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> +
> +	/* Note: Debounce timer isn't under control of the command
> +	 * source registers, so no need to sync with the coprocessor
> +	 */
> +	ast_write_bits(addr, GPIO_G7_DEBOUNCE_SEL, timer);
> +}
> +
> +static int enable_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> +	u32 requested_cycles;
> +	unsigned long flags;
> +	int rc;
> +	int i;
> +
> +	if (!gpio->clk)
> +		return -EINVAL;
> +
> +	rc = usecs_to_cycles(gpio, usecs, &requested_cycles);
> +	if (rc < 0) {
> +		dev_warn(chip->parent, "Failed to convert %luus to cycles at %luHz: %d\n", usecs,
> +			 clk_get_rate(gpio->clk), rc);
> +		return rc;
> +	}
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	if (timer_allocation_registered(gpio, offset)) {
> +		rc = unregister_allocated_timer(gpio, offset);
> +		if (rc < 0)
> +			goto out;
> +	}
> +
> +	/* Try to find a timer already configured for the debounce period */
> +	for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
> +		u32 cycles;
> +
> +		cycles = ioread32(gpio->base + debounce_timers[i]);
> +		if (requested_cycles == cycles)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(debounce_timers)) {
> +		int j;
> +
> +		/*
> +		 * As there are no timers configured for the requested debounce
> +		 * period, find an unused timer instead
> +		 */
> +		for (j = 1; j < ARRAY_SIZE(gpio->timer_users); j++) {
> +			if (gpio->timer_users[j] == 0)
> +				break;
> +		}
> +
> +		if (j == ARRAY_SIZE(gpio->timer_users)) {
> +			dev_warn(chip->parent,
> +				 "Debounce timers exhausted, cannot debounce for period %luus\n",
> +				 usecs);
> +
> +			rc = -EPERM;
> +
> +			/*
> +			 * We already adjusted the accounting to remove @offset
> +			 * as a user of its previous timer, so also configure
> +			 * the hardware so @offset has timers disabled for
> +			 * consistency.
> +			 */
> +			configure_timer(gpio, offset, 0);
> +			goto out;
> +		}
> +
> +		i = j;
> +
> +		iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
> +	}
> +
> +	if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
> +	register_allocated_timer(gpio, offset, i);
> +	configure_timer(gpio, offset, i);
> +
> +out:
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return rc;
> +}
> +
> +static int disable_debounce(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> +	unsigned long flags;
> +	int rc;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	rc = unregister_allocated_timer(gpio, offset);
> +	if (!rc)
> +		configure_timer(gpio, offset, 0);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return rc;
> +}
> +
> +static int set_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> +{
> +	struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> +
> +	if (!have_debounce(gpio, offset))
> +		return -EOPNOTSUPP;
> +
> +	if (usecs)
> +		return enable_debounce(chip, offset, usecs);
> +
> +	return disable_debounce(chip, offset);
> +}

These are all copy/pasted, save for changing to `struct
aspeed_gpio_g7`. Can we make them common?

> +
> +static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
> +				     unsigned long config)
> +{
> +	unsigned long param = pinconf_to_config_param(config);
> +	u32 arg = pinconf_to_config_argument(config);
> +
> +	if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> +		return set_debounce(chip, offset, arg);
> +	else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
> +		 param == PIN_CONFIG_DRIVE_STRENGTH)
> +		return pinctrl_gpio_set_config(offset, config);
> +	else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
> +		/* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
> +		return -EOPNOTSUPP;
> +	else if (param == PIN_CONFIG_PERSIST_STATE)
> +		return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
> +
> +	return -EOPNOTSUPP;
> +}

This is copy/paste, save for the call to
aspeed_gpio_g7_reset_tolerance(). Can we make it common?

Andrew

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Billy Tsai]

Hi Andrew,

Thanks for your suggestion. As I understand it, you’re suggesting that this driver should share the
common parts with aspeed-gpio.c, correct?
However, I don’t think that’s necessary. You can treat it as a new GPIO controller because the
register layout is quite different from aspeed-gpio.c.
If I try to make it common, the driver could become too complex, potentially requiring a separate
gpio-aspeed-common.c and necessitating changes to the existing aspeed-gpio.c
Maybe the discussion of merging aspeed-gpio.c and this driver can be postponed until after this one
is accepted?

For Others, please see the reply inline.

Best regards,
Billy Tsai

> > In the 7th generation of the SoC from Aspeed, the control logic of the
> > GPIO controller has been updated to support per-pin control. Each pin now
> > has its own 32-bit register, allowing for individual control of the pin’s
> > value, direction, interrupt type, and other settings.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > ---
> >  drivers/gpio/Kconfig          |   7 +
> >  drivers/gpio/Makefile         |   1 +
> >  drivers/gpio/gpio-aspeed-g7.c | 831 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 839 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-aspeed-g7.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 58f43bcced7c..93f237429b92 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -172,6 +172,13 @@ config GPIO_ASPEED
> >       help
> >         Say Y here to support Aspeed AST2400 and AST2500 GPIO controllers.
> >
> > +config GPIO_ASPEED_G7
> > +     tristate "Aspeed G7 GPIO support"
> > +     depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > +     select GPIOLIB_IRQCHIP
> > +     help
> > +       Say Y here to support Aspeed AST2700 GPIO controllers.
> > +
> >  config GPIO_ASPEED_SGPIO
> >       bool "Aspeed SGPIO support"
> >       depends on (ARCH_ASPEED || COMPILE_TEST) && OF_GPIO
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 64dd6d9d730d..e830291761ee 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -34,6 +34,7 @@ obj-$(CONFIG_GPIO_AMD_FCH)          += gpio-amd-fch.o
> >  obj-$(CONFIG_GPIO_AMDPT)             += gpio-amdpt.o
> >  obj-$(CONFIG_GPIO_ARIZONA)           += gpio-arizona.o
> >  obj-$(CONFIG_GPIO_ASPEED)            += gpio-aspeed.o
> > +obj-$(CONFIG_GPIO_ASPEED_G7)         += gpio-aspeed-g7.o
> >  obj-$(CONFIG_GPIO_ASPEED_SGPIO)              += gpio-aspeed-sgpio.o
> >  obj-$(CONFIG_GPIO_ATH79)             += gpio-ath79.o
> >  obj-$(CONFIG_GPIO_BCM_KONA)          += gpio-bcm-kona.o
> > diff --git a/drivers/gpio/gpio-aspeed-g7.c b/drivers/gpio/gpio-aspeed-g7.c
> > new file mode 100644
> > index 000000000000..dbca097de6ea
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-aspeed-g7.c
> > @@ -0,0 +1,831 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 Aspeed Technology Inc.
> > + *
> > + * Billy Tsai <billy_tsai@aspeedtech.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/gpio/aspeed.h>

> I think you should either drop this include or rework the existing
> implementations so the coprocessor handshake works correctly.

The coprocessor handshake will be implemented later, so I will remove the related include for now.

> > +#include <linux/gpio/driver.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +
> > +#include <asm/div64.h>
> > +
> > +#define GPIO_G7_IRQ_STS_BASE 0x100
> > +#define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
> > +#define GPIO_G7_CTRL_REG_BASE 0x180
> > +#define GPIO_G7_CTRL_REG_OFFSET(x) (GPIO_G7_CTRL_REG_BASE + (x) * 0x4)
> > +#define GPIO_G7_OUT_DATA BIT(0)
> > +#define GPIO_G7_DIR BIT(1)
> > +#define GPIO_G7_IRQ_EN BIT(2)
> > +#define GPIO_G7_IRQ_TYPE0 BIT(3)
> > +#define GPIO_G7_IRQ_TYPE1 BIT(4)
> > +#define GPIO_G7_IRQ_TYPE2 BIT(5)
> > +#define GPIO_G7_RST_TOLERANCE BIT(6)
> > +#define GPIO_G7_DEBOUNCE_SEL GENMASK(8, 7)
> > +#define GPIO_G7_INPUT_MASK BIT(9)
> > +#define GPIO_G7_IRQ_STS BIT(12)
> > +#define GPIO_G7_IN_DATA BIT(13)

> Can you please add `CTRL` into these field macro names so it's clear
> they relate to the control register?

Okay.

> > +/*
> > + * The configuration of the following registers should be determined
> > + * outside of the GPIO driver.

> Where though?

The usage of the following registers hasn’t been implemented yet, so I will remove them for now.

> > + */
> > +#define GPIO_G7_PRIVILEGE_W_REG_BASE 0x810
> > +#define GPIO_G7_PRIVILEGE_W_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_W_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_PRIVILEGE_R_REG_BASE 0x910
> > +#define GPIO_G7_PRIVILEGE_R_REG_OFFSET(x) (GPIO_G7_PRIVILEGE_R_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TARGET_REG_BASE 0xA10
> > +#define GPIO_G7_IRQ_TARGET_REG_OFFSET(x) (GPIO_G7_IRQ_TARGET_REG_BASE + ((x) >> 2) * 0x4)
> > +#define GPIO_G7_IRQ_TO_INTC2_18 BIT(0)
> > +#define GPIO_G7_IRQ_TO_INTC2_19 BIT(1)
> > +#define GPIO_G7_IRQ_TO_INTC2_20 BIT(2)
> > +#define GPIO_G7_IRQ_TO_SIO BIT(3)
> > +#define GPIO_G7_IRQ_TARGET_RESET_TOLERANCE BIT(6)
> > +#define GPIO_G7_IRQ_TARGET_W_PROTECT BIT(7)
> > +
> > +static inline u32 field_get(u32 _mask, u32 _val)
> > +{
> > +     return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> > +}
> > +
> > +static inline u32 field_prep(u32 _mask, u32 _val)
> > +{
> > +     return (((_val) << (ffs(_mask) - 1)) & (_mask));
> > +}

> So linux/bitfield.h provides a lot of APIs along these lines, perhaps
> use them instead?

I will use FIELD_GET and FIELD_PREP to replace them.

> > +
> > +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> > +{
> > +     iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> > +}
> > +
> > +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> > +{
> > +     iowrite32((ioread32(addr) & ~(mask)), addr);
> > +}
> > +
> > +struct aspeed_bank_props {
> > +     unsigned int bank;
> > +     u32 input;
> > +     u32 output;
> > +};
> > +
> > +struct aspeed_gpio_g7_config {
> > +     unsigned int nr_gpios;
> > +     const struct aspeed_bank_props *props;
> > +};
> > +
> > +/*
> > + * @offset_timer: Maps an offset to an @timer_users index, or zero if disabled
> > + * @timer_users: Tracks the number of users for each timer
> > + *
> > + * The @timer_users has four elements but the first element is unused. This is
> > + * to simplify accounting and indexing, as a zero value in @offset_timer
> > + * represents disabled debouncing for the GPIO. Any other value for an element
> > + * of @offset_timer is used as an index into @timer_users. This behaviour of
> > + * the zero value aligns with the behaviour of zero built from the timer
> > + * configuration registers (i.e. debouncing is disabled).
> > + */
> > +struct aspeed_gpio_g7 {
> > +     struct gpio_chip chip;
> > +     struct device *dev;
> > +     raw_spinlock_t lock;
> > +     void __iomem *base;
> > +     int irq;
> > +     const struct aspeed_gpio_g7_config *config;
> > +
> > +     u8 *offset_timer;
> > +     unsigned int timer_users[4];
> > +     struct clk *clk;
> > +
> > +     u32 *dcache;
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value sampled on the
> > + *       line even when the GPIO is configured as an output. Since
> > + *       that input goes through synchronizers, writing, then reading
> > + *       back may not return the written value right away.
> > + *
> > + *       The "rdata" register returns the content of the write latch
> > + *       and thus can be used to read back what was last written
> > + *       reliably.
> > + */
> > +
> > +static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };

> This is all largely copy/pasted from gpio-aspeed.c. Can we split it out
> and share the definitions?

Do you mean moving them into the common header file? 
The structure is fine, but I’m unsure about the debounce_timers. 
It’s a static array, so I don’t think it needs to be shared with gpio-aspeed.c.

> > +
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> > +
> > +static inline bool is_bank_props_sentinel(const struct aspeed_bank_props *props)
> > +{
> > +     return !(props->input || props->output);
> > +}
> > +
> > +static inline const struct aspeed_bank_props *find_bank_props(struct aspeed_gpio_g7 *gpio,
> > +                                                           unsigned int offset)
> > +{
> > +     const struct aspeed_bank_props *props = gpio->config->props;
> > +
> > +     while (!is_bank_props_sentinel(props)) {
> > +             if (props->bank == GPIO_BANK(offset))
> > +                     return props;
> > +             props++;
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +static inline bool have_gpio(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > +     const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > +     if (offset > gpio->chip.ngpio)
> > +             return false;
> > +
> > +     return (!props || ((props->input | props->output) & GPIO_BIT(offset)));
> > +}
> > +
> > +static inline bool have_input(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > +     const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > +     return !props || (props->input & GPIO_BIT(offset));
> > +}
> > +
> > +#define have_irq(g, o) have_input((g), (o))
> > +#define have_debounce(g, o) have_input((g), (o))
> > +
> > +static inline bool have_output(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > +     const struct aspeed_bank_props *props = find_bank_props(gpio, offset);
> > +
> > +     return !props || (props->output & GPIO_BIT(offset));
> > +}
> > +

> This is all common as well.

Moving them into the common header file?

> > +static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
> > +}
> > +
> > +static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);

> The rest of the implementation of this function is broadly the same as
> in gpio-aspeed.c. The main difference is accounting for the address to
> access and the bit to whack. If we define some callbacks that replace
> GPIO_BANK()/to_bank() and GPIO_BIT() that can account for the
> differences in register layout, perhaps this could be common?

> The trade-off is some complexity vs copy-paste, but there does seem to
> be an awful lot of the latter with only minor changes so far.

Do you mean I need to create a gpio-aspeed-common.c, define the necessary common APIs,
and have gpio-aspeed.c and this driver hook into those APIs? 

> > +     u32 reg;
> > +
> > +     reg = gpio->dcache[GPIO_BANK(offset)];
> > +
> > +     if (val)
> > +             reg |= GPIO_BIT(offset);
> > +     else
> > +             reg &= ~GPIO_BIT(offset);
> > +     gpio->dcache[GPIO_BANK(offset)] = reg;
> > +
> > +     ast_write_bits(addr, GPIO_G7_OUT_DATA, val);
> > +}
> > +
> > +static void aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     unsigned long flags;
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     __aspeed_gpio_g7_set(gc, offset, val);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static int aspeed_gpio_g7_dir_in(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +     unsigned long flags;
> > +
> > +     if (!have_input(gpio, offset))
> > +             return -EOPNOTSUPP;
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     ast_clr_bits(addr, GPIO_G7_DIR);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +     unsigned long flags;
> > +
> > +     if (!have_output(gpio, offset))
> > +             return -EOPNOTSUPP;
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     __aspeed_gpio_g7_set(gc, offset, val);
> > +     ast_write_bits(addr, GPIO_G7_DIR, 1);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_get_direction(struct gpio_chip *gc, unsigned int offset)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +     unsigned long flags;
> > +     u32 val;
> > +
> > +     if (!have_input(gpio, offset))
> > +             return GPIO_LINE_DIRECTION_OUT;
> > +
> > +     if (!have_output(gpio, offset))
> > +             return GPIO_LINE_DIRECTION_IN;
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     val = ioread32(addr) & GPIO_G7_DIR;
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return val ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
> > +}

> On top of handling the differences in the register layout as I
> mentioned above, the main difference in these get/set implementations
> is dropping the calls through the coprocessor handshake APIs. If you
> stub out the implementation of the coprocessor APIs I think it's likely
> these can be common. To do that you would need to make them use
> callbacks into the SoC-specific driver. To stub out the implementation
> you could leave the callback pointer as NULL for now.

Same as above?

> > +
> > +static inline int irqd_to_aspeed_gpio_g7_data(struct irq_data *d, struct aspeed_gpio_g7 **gpio,
> > +                                           int *offset)
> > +{
> > +     struct aspeed_gpio_g7 *internal;
> > +
> > +     *offset = irqd_to_hwirq(d);
> > +
> > +     internal = irq_data_get_irq_chip_data(d);
> > +
> > +     /* This might be a bit of a questionable place to check */
> > +     if (!have_irq(internal, *offset))
> > +             return -EOPNOTSUPP;
> > +
> > +     *gpio = internal;
> > +
> > +     return 0;
> > +}

> You do have different data-types here (struct aspeed_gpio_g7), but
> possibly with appropriate struct definitions and use of container_of()
> by the caller, this could be common too?

> > +
> > +static void aspeed_gpio_g7_irq_ack(struct irq_data *d)
> > +{
> > +     struct aspeed_gpio_g7 *gpio;
> > +     unsigned long flags;
> > +     void __iomem *addr;
> > +     int rc, offset;
> > +
> > +     rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > +     if (rc)
> > +             return;
> > +
> > +     addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     ast_write_bits(addr, GPIO_G7_IRQ_STS, 1);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_set_mask(struct irq_data *d, bool set)
> > +{
> > +     struct aspeed_gpio_g7 *gpio;
> > +     unsigned long flags;
> > +     void __iomem *addr;
> > +     int rc, offset;
> > +
> > +     rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > +     if (rc)
> > +             return;
> > +
> > +     addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     /* Unmasking the IRQ */
> > +     if (set)
> > +             gpiochip_enable_irq(&gpio->chip, irqd_to_hwirq(d));
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     if (set)
> > +             ast_write_bits(addr, GPIO_G7_IRQ_EN, 1);
> > +     else
> > +             ast_clr_bits(addr, GPIO_G7_IRQ_EN);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     /* Masking the IRQ */
> > +     if (!set)
> > +             gpiochip_disable_irq(&gpio->chip, irqd_to_hwirq(d));
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_mask(struct irq_data *d)
> > +{
> > +     aspeed_gpio_g7_irq_set_mask(d, false);
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_unmask(struct irq_data *d)
> > +{
> > +     aspeed_gpio_g7_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_gpio_g7_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     u32 type0 = 0;
> > +     u32 type1 = 0;
> > +     u32 type2 = 0;
> > +     irq_flow_handler_t handler;
> > +     struct aspeed_gpio_g7 *gpio;
> > +     unsigned long flags;
> > +     void __iomem *addr;
> > +     int rc, offset;
> > +
> > +     rc = irqd_to_aspeed_gpio_g7_data(d, &gpio, &offset);
> > +     if (rc)
> > +             return -EINVAL;
> > +     addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_EDGE_BOTH:
> > +             type2 = 1;
> > +             fallthrough;
> > +     case IRQ_TYPE_EDGE_RISING:
> > +             type0 = 1;
> > +             fallthrough;
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +             handler = handle_edge_irq;
> > +             break;
> > +     case IRQ_TYPE_LEVEL_HIGH:
> > +             type0 |= 1;
> > +             fallthrough;
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             type1 |= 1;
> > +             handler = handle_level_irq;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     ast_write_bits(addr, GPIO_G7_IRQ_TYPE2, type2);
> > +     ast_write_bits(addr, GPIO_G7_IRQ_TYPE1, type1);
> > +     ast_write_bits(addr, GPIO_G7_IRQ_TYPE0, type0);

> Can we write them as a field in the one call? They're all in the same
> register, which was not true in the previous controller register
> layout.

Okay.

> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     irq_set_handler_locked(d, handler);
> > +
> > +     return 0;
> > +}
> > +
> > +static void aspeed_gpio_g7_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > +     struct irq_chip *ic = irq_desc_get_chip(desc);
> > +     unsigned int i, p, banks;
> > +     unsigned long reg;
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     void __iomem *addr;
> > +
> > +     chained_irq_enter(ic, desc);
> > +
> > +     banks = DIV_ROUND_UP(gpio->chip.ngpio, 32);
> > +     for (i = 0; i < banks; i++) {
> > +             addr = gpio->base + GPIO_G7_IRQ_STS_OFFSET(i);
> > +
> > +             reg = ioread32(addr);
> > +
> > +             for_each_set_bit(p, &reg, 32)
> > +                     generic_handle_domain_irq(gc->irq.domain, i * 32 + p);
> > +     }
> > +
> > +     chained_irq_exit(ic, desc);
> > +}

> The only thing that's different for the IRQ status handling is the
> spread of the registers in the layout. In terms of the bits in each
> bank's IRQ status register, the layout is the same. By implementing the
> means to locate the status register as a callback this code could be
> common between the drivers.

> > +
> > +static void aspeed_init_irq_valid_mask(struct gpio_chip *gc, unsigned long *valid_mask,
> > +                                    unsigned int ngpios)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > +     const struct aspeed_bank_props *props = gpio->config->props;
> > +
> > +     while (!is_bank_props_sentinel(props)) {
> > +             unsigned int offset;
> > +             const unsigned long input = props->input;
> > +
> > +             /* Pretty crummy approach, but similar to GPIO core */
> > +             for_each_clear_bit(offset, &input, 32) {
> > +                     unsigned int i = props->bank * 32 + offset;
> > +
> > +                     if (i >= gpio->chip.ngpio)
> > +                             break;
> > +
> > +                     clear_bit(i, valid_mask);
> > +             }
> > +
> > +             props++;
> > +     }
> > +}

> This is the same except for the change to use `struct aspeed_gpio_g7`?
> Can we make this common?

> > +
> > +static int aspeed_gpio_g7_reset_tolerance(struct gpio_chip *chip, unsigned int offset, bool enable)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > +     unsigned long flags;
> > +     void __iomem *addr;
> > +
> > +     addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     if (enable)
> > +             ast_write_bits(addr, GPIO_G7_RST_TOLERANCE, 1);
> > +     else
> > +             ast_clr_bits(addr, GPIO_G7_RST_TOLERANCE);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +     if (!have_gpio(gpiochip_get_data(chip), offset))
> > +             return -ENODEV;
> > +
> > +     return pinctrl_gpio_request(chip->base + offset);

> pinctrl_gpio_request() takes the chip and the offset value separately.
> It looks like you've developed this patch against an older kernel tree?

I will fix it.

> > +}
> > +
> > +static void aspeed_gpio_g7_free(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +     pinctrl_gpio_free(chip->base + offset);

> Same as above for pinctrl_gpio_free().

I will fix it.

> > +}
> > +
> > +static int usecs_to_cycles(struct aspeed_gpio_g7 *gpio, unsigned long usecs, u32 *cycles)
> > +{
> > +     u64 rate;
> > +     u64 n;
> > +     u32 r;
> > +
> > +     rate = clk_get_rate(gpio->clk);
> > +     if (!rate)
> > +             return -EOPNOTSUPP;
> > +
> > +     n = rate * usecs;
> > +     r = do_div(n, 1000000);
> > +
> > +     if (n >= U32_MAX)
> > +             return -ERANGE;
> > +
> > +     /* At least as long as the requested time */
> > +     *cycles = n + (!!r);
> > +
> > +     return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static int register_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset,
> > +                                 unsigned int timer)
> > +{
> > +     if (WARN(gpio->offset_timer[offset] != 0, "Offset %d already allocated timer %d\n", offset,
> > +              gpio->offset_timer[offset]))
> > +             return -EINVAL;
> > +
> > +     if (WARN(gpio->timer_users[timer] == UINT_MAX, "Timer user count would overflow\n"))
> > +             return -EPERM;
> > +
> > +     gpio->offset_timer[offset] = timer;
> > +     gpio->timer_users[timer]++;
> > +
> > +     return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static int unregister_allocated_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > +     if (WARN(gpio->offset_timer[offset] == 0, "No timer allocated to offset %d\n", offset))
> > +             return -EINVAL;
> > +
> > +     if (WARN(gpio->timer_users[gpio->offset_timer[offset]] == 0,
> > +              "No users recorded for timer %d\n", gpio->offset_timer[offset]))
> > +             return -EINVAL;
> > +
> > +     gpio->timer_users[gpio->offset_timer[offset]]--;
> > +     gpio->offset_timer[offset] = 0;
> > +
> > +     return 0;
> > +}
> > +
> > +/* Call under gpio->lock */
> > +static inline bool timer_allocation_registered(struct aspeed_gpio_g7 *gpio, unsigned int offset)
> > +{
> > +     return gpio->offset_timer[offset] > 0;
> > +}

> The above functions have all been copy/pasted, can we make them common?

> > +
> > +static void configure_timer(struct aspeed_gpio_g7 *gpio, unsigned int offset, unsigned int timer)
> > +{
> > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > +
> > +     /* Note: Debounce timer isn't under control of the command
> > +      * source registers, so no need to sync with the coprocessor
> > +      */
> > +     ast_write_bits(addr, GPIO_G7_DEBOUNCE_SEL, timer);
> > +}
> > +
> > +static int enable_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > +     u32 requested_cycles;
> > +     unsigned long flags;
> > +     int rc;
> > +     int i;
> > +
> > +     if (!gpio->clk)
> > +             return -EINVAL;
> > +
> > +     rc = usecs_to_cycles(gpio, usecs, &requested_cycles);
> > +     if (rc < 0) {
> > +             dev_warn(chip->parent, "Failed to convert %luus to cycles at %luHz: %d\n", usecs,
> > +                      clk_get_rate(gpio->clk), rc);
> > +             return rc;
> > +     }
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     if (timer_allocation_registered(gpio, offset)) {
> > +             rc = unregister_allocated_timer(gpio, offset);
> > +             if (rc < 0)
> > +                     goto out;
> > +     }
> > +
> > +     /* Try to find a timer already configured for the debounce period */
> > +     for (i = 1; i < ARRAY_SIZE(debounce_timers); i++) {
> > +             u32 cycles;
> > +
> > +             cycles = ioread32(gpio->base + debounce_timers[i]);
> > +             if (requested_cycles == cycles)
> > +                     break;
> > +     }
> > +
> > +     if (i == ARRAY_SIZE(debounce_timers)) {
> > +             int j;
> > +
> > +             /*
> > +              * As there are no timers configured for the requested debounce
> > +              * period, find an unused timer instead
> > +              */
> > +             for (j = 1; j < ARRAY_SIZE(gpio->timer_users); j++) {
> > +                     if (gpio->timer_users[j] == 0)
> > +                             break;
> > +             }
> > +
> > +             if (j == ARRAY_SIZE(gpio->timer_users)) {
> > +                     dev_warn(chip->parent,
> > +                              "Debounce timers exhausted, cannot debounce for period %luus\n",
> > +                              usecs);
> > +
> > +                     rc = -EPERM;
> > +
> > +                     /*
> > +                      * We already adjusted the accounting to remove @offset
> > +                      * as a user of its previous timer, so also configure
> > +                      * the hardware so @offset has timers disabled for
> > +                      * consistency.
> > +                      */
> > +                     configure_timer(gpio, offset, 0);
> > +                     goto out;
> > +             }
> > +
> > +             i = j;
> > +
> > +             iowrite32(requested_cycles, gpio->base + debounce_timers[i]);
> > +     }
> > +
> > +     if (WARN(i == 0, "Cannot register index of disabled timer\n")) {
> > +             rc = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     register_allocated_timer(gpio, offset, i);
> > +     configure_timer(gpio, offset, i);
> > +
> > +out:
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return rc;
> > +}
> > +
> > +static int disable_debounce(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > +     unsigned long flags;
> > +     int rc;
> > +
> > +     raw_spin_lock_irqsave(&gpio->lock, flags);
> > +
> > +     rc = unregister_allocated_timer(gpio, offset);
> > +     if (!rc)
> > +             configure_timer(gpio, offset, 0);
> > +
> > +     raw_spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > +     return rc;
> > +}
> > +
> > +static int set_debounce(struct gpio_chip *chip, unsigned int offset, unsigned long usecs)
> > +{
> > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(chip);
> > +
> > +     if (!have_debounce(gpio, offset))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (usecs)
> > +             return enable_debounce(chip, offset, usecs);
> > +
> > +     return disable_debounce(chip, offset);
> > +}

> These are all copy/pasted, save for changing to `struct
> aspeed_gpio_g7`. Can we make them common?

> > +
> > +static int aspeed_gpio_g7_set_config(struct gpio_chip *chip, unsigned int offset,
> > +                                  unsigned long config)
> > +{
> > +     unsigned long param = pinconf_to_config_param(config);
> > +     u32 arg = pinconf_to_config_argument(config);
> > +
> > +     if (param == PIN_CONFIG_INPUT_DEBOUNCE)
> > +             return set_debounce(chip, offset, arg);
> > +     else if (param == PIN_CONFIG_BIAS_DISABLE || param == PIN_CONFIG_BIAS_PULL_DOWN ||
> > +              param == PIN_CONFIG_DRIVE_STRENGTH)
> > +             return pinctrl_gpio_set_config(offset, config);
> > +     else if (param == PIN_CONFIG_DRIVE_OPEN_DRAIN || param == PIN_CONFIG_DRIVE_OPEN_SOURCE)
> > +             /* Return -EOPNOTSUPP to trigger emulation, as per datasheet */
> > +             return -EOPNOTSUPP;
> > +     else if (param == PIN_CONFIG_PERSIST_STATE)
> > +             return aspeed_gpio_g7_reset_tolerance(chip, offset, arg);
> > +
> > +     return -EOPNOTSUPP;
> > +}

> This is copy/paste, save for the call to
> aspeed_gpio_g7_reset_tolerance(). Can we make it common?

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Andrew Jeffery]

On Tue, 2024-08-27 at 02:45 +0000, Billy Tsai wrote:
> Hi Andrew,
> 
> Thanks for your suggestion. As I understand it, you’re suggesting that this driver should share the
> common parts with aspeed-gpio.c, correct?
> However, I don’t think that’s necessary. You can treat it as a new GPIO controller because the
> register layout is quite different from aspeed-gpio.c.

Well, we could, but both share a lot of the same capabilities. aspeed-
gpio.c already has to abstract over the register layout because it's so
haphazard. What I was suggesting was to formalise this a bit more by
converting some of the inline functions and macros to callbacks that
can be implemented for each controller.

I haven't tried it myself, but it feels feasible?

> If I try to make it common, the driver could become too complex, potentially requiring a separate
> gpio-aspeed-common.c and necessitating changes to the existing aspeed-gpio.c

I felt the trade-off between the volume of copy/paste and the
complexity of adding a few callbacks weighed in favour of the latter.

Also, given the volume of copy/paste, I think it would be best to
retain the copyright information from aspeed-gpio.c if the outcome is
these must be separate drivers.

Many of the changes seemed to be dealing with the difference between
`struct aspeed_gpio` and `struct aspeed_gpio_g7`, which might be
addressed by some careful struct design and use of container_of().

> Maybe the discussion of merging aspeed-gpio.c and this driver can be postponed until after this one
> is accepted?

Yeah, but I suspect the discussion just won't happen if this is merged.
Now's the time to get consensus on a way forward, while the driver is
yet to be merged.

> > > +
> > > +static const int debounce_timers[4] = { 0x00, 0x04, 0x00, 0x08 };
> 
> > This is all largely copy/pasted from gpio-aspeed.c. Can we split it out
> > and share the definitions?
> 
> Do you mean moving them into the common header file? 
> The structure is fine, but I’m unsure about the debounce_timers. 
> It’s a static array, so I don’t think it needs to be shared with gpio-aspeed.c.

That can be changed though? An appropriate pointer can be point into
the driver struct.

> > > +static int aspeed_gpio_g7_get(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> > > +
> > > +     return !!(field_get(GPIO_G7_IN_DATA, ioread32(addr)));
> > > +}
> > > +
> > > +static void __aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> > > +{
> > > +     struct aspeed_gpio_g7 *gpio = gpiochip_get_data(gc);
> > > +     void __iomem *addr = gpio->base + GPIO_G7_CTRL_REG_OFFSET(offset);
> 
> > The rest of the implementation of this function is broadly the same as
> > in gpio-aspeed.c. The main difference is accounting for the address to
> > access and the bit to whack. If we define some callbacks that replace
> > GPIO_BANK()/to_bank() and GPIO_BIT() that can account for the
> > differences in register layout, perhaps this could be common?
> 
> > The trade-off is some complexity vs copy-paste, but there does seem to
> > be an awful lot of the latter with only minor changes so far.
> 
> Do you mean I need to create a gpio-aspeed-common.c, define the necessary common APIs,
> and have gpio-aspeed.c and this driver hook into those APIs?

Well, you may not have to do that if we can put it all in gpio-
aspeed.c?

My suspicion is the g7 support could largely become some well-chosen
callbacks.

Andrew

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Billy Tsai]

Hi Andrew,

I’ve complicated the merging of the two drivers. I’ll try to follow your suggestion and put everything into gpio-aspeed.c.

Thanks
Best regards,
Billy Tsai

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[kernel test robot]

Hi Billy,

kernel test robot noticed the following build errors:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on linus/master v6.11-rc4 next-20240821]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/dt-bindings-gpio-aspeed-ast2400-gpio-Support-ast2700/20240821-150951
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240821070740.2378602-3-billy_tsai%40aspeedtech.com
patch subject: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240822/202408221624.UtsHD8HQ-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 26670e7fa4f032a019d23d56c6a02926e854e8af)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240822/202408221624.UtsHD8HQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408221624.UtsHD8HQ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from drivers/gpio/gpio-aspeed-g7.c:10:
   In file included from include/linux/gpio/driver.h:8:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from drivers/gpio/gpio-aspeed-g7.c:10:
   In file included from include/linux/gpio/driver.h:8:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   In file included from drivers/gpio/gpio-aspeed-g7.c:10:
   In file included from include/linux/gpio/driver.h:8:
   In file included from include/linux/irqchip/chained_irq.h:10:
   In file included from include/linux/irq.h:591:
   In file included from arch/s390/include/asm/hw_irq.h:6:
   In file included from include/linux/pci.h:37:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2228:
   include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     500 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     501 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     507 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     508 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     519 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     520 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     528 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     529 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-aspeed-g7.c:474:49: error: too few arguments to function call, expected 2, have 1
     474 |         return pinctrl_gpio_request(chip->base + offset);
         |                ~~~~~~~~~~~~~~~~~~~~                    ^
   include/linux/pinctrl/consumer.h:30:5: note: 'pinctrl_gpio_request' declared here
      30 | int pinctrl_gpio_request(struct gpio_chip *gc, unsigned int offset);
         |     ^                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c:479:39: error: too few arguments to function call, expected 2, have 1
     479 |         pinctrl_gpio_free(chip->base + offset);
         |         ~~~~~~~~~~~~~~~~~                    ^
   include/linux/pinctrl/consumer.h:31:6: note: 'pinctrl_gpio_free' declared here
      31 | void pinctrl_gpio_free(struct gpio_chip *gc, unsigned int offset);
         |      ^                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpio/gpio-aspeed-g7.c:676:48: error: too few arguments to function call, expected 3, have 2
     676 |                 return pinctrl_gpio_set_config(offset, config);
         |                        ~~~~~~~~~~~~~~~~~~~~~~~               ^
   include/linux/pinctrl/consumer.h:36:5: note: 'pinctrl_gpio_set_config' declared here
      36 | int pinctrl_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
         |     ^                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      37 |                                 unsigned long config);
         |                                 ~~~~~~~~~~~~~~~~~~~~
   17 warnings and 3 errors generated.


vim +474 drivers/gpio/gpio-aspeed-g7.c

   468	
   469	static int aspeed_gpio_g7_request(struct gpio_chip *chip, unsigned int offset)
   470	{
   471		if (!have_gpio(gpiochip_get_data(chip), offset))
   472			return -ENODEV;
   473	
 > 474		return pinctrl_gpio_request(chip->base + offset);
   475	}
   476	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/2] gpio: Add G7 Aspeed gpio controller driver

[Linus Walleij]

Hi Billy,

thanks for your patch!

On Wed, Aug 21, 2024 at 9:07 AM Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> In the 7th generation of the SoC from Aspeed, the control logic of the
> GPIO controller has been updated to support per-pin control. Each pin now
> has its own 32-bit register, allowing for individual control of the pin’s
> value, direction, interrupt type, and other settings.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
(...)

> +static inline u32 field_get(u32 _mask, u32 _val)
> +{
> +       return (((_val) & (_mask)) >> (ffs(_mask) - 1));
> +}
> +
> +static inline u32 field_prep(u32 _mask, u32 _val)
> +{
> +       return (((_val) << (ffs(_mask) - 1)) & (_mask));
> +}

Can't you use FIELD_GET and FIELD_PREP from
<linux/bitfield.h> instead?

> +static inline void ast_write_bits(void __iomem *addr, u32 mask, u32 val)
> +{
> +       iowrite32((ioread32(addr) & ~(mask)) | field_prep(mask, val), addr);
> +}
> +
> +static inline void ast_clr_bits(void __iomem *addr, u32 mask)
> +{
> +       iowrite32((ioread32(addr) & ~(mask)), addr);
> +}

This as a whole looks a bit like a reimplementation of regmap-mmio, can you
look into using that instead?

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: Add G7 Aspeed gpio controller driver

[Markus Elfring]

…
> +++ b/drivers/gpio/gpio-aspeed-g7.c
> @@ -0,0 +1,831 @@
…
> +static void aspeed_gpio_g7_set(struct gpio_chip *gc, unsigned int offset, int val)
> +{
…
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	__aspeed_gpio_g7_set(gc, offset, val);
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +}
…

Under which circumstances would you become interested to apply a statement
like “guard(raw_spinlock_irqsave)(&gpio->lock);”?
https://elixir.bootlin.com/linux/v6.11-rc5/source/include/linux/spinlock.h#L551

Regards,
Markus