SpacemiT K3 SoC has changed gpio register layout while comparing
with previous generation, the register offset and bank offset
need to be adjusted, introduce a compatible data to extend the
driver to support this.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
 1 file changed, 106 insertions(+), 44 deletions(-)

diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
index eb66a15c002f..02cc5c11b617 100644
--- a/drivers/gpio/gpio-spacemit-k1.c
+++ b/drivers/gpio/gpio-spacemit-k1.c
@@ -15,28 +15,19 @@
 #include <linux/platform_device.h>
 #include <linux/seq_file.h>
 
-/* register offset */
-#define SPACEMIT_GPLR		0x00 /* port level - R */
-#define SPACEMIT_GPDR		0x0c /* port direction - R/W */
-#define SPACEMIT_GPSR		0x18 /* port set - W */
-#define SPACEMIT_GPCR		0x24 /* port clear - W */
-#define SPACEMIT_GRER		0x30 /* port rising edge R/W */
-#define SPACEMIT_GFER		0x3c /* port falling edge R/W */
-#define SPACEMIT_GEDR		0x48 /* edge detect status - R/W1C */
-#define SPACEMIT_GSDR		0x54 /* (set) direction - W */
-#define SPACEMIT_GCDR		0x60 /* (clear) direction - W */
-#define SPACEMIT_GSRER		0x6c /* (set) rising edge detect enable - W */
-#define SPACEMIT_GCRER		0x78 /* (clear) rising edge detect enable - W */
-#define SPACEMIT_GSFER		0x84 /* (set) falling edge detect enable - W */
-#define SPACEMIT_GCFER		0x90 /* (clear) falling edge detect enable - W */
-#define SPACEMIT_GAPMASK	0x9c /* interrupt mask , 0 disable, 1 enable - R/W */
-
 #define SPACEMIT_NR_BANKS		4
 #define SPACEMIT_NR_GPIOS_PER_BANK	32
 
 #define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
+#define to_spacemit_gpio_regs(sg) ((sg)->data->reg_offsets)
 
 struct spacemit_gpio;
+struct spacemit_gpio_reg_offsets;
+
+struct spacemit_gpio_data {
+	struct spacemit_gpio_reg_offsets *reg_offsets;
+	u32 bank_offsets[4];
+};
 
 struct spacemit_gpio_bank {
 	struct gpio_generic_chip chip;
@@ -49,9 +40,28 @@ struct spacemit_gpio_bank {
 
 struct spacemit_gpio {
 	struct device *dev;
+	const struct spacemit_gpio_data *data;
 	struct spacemit_gpio_bank sgb[SPACEMIT_NR_BANKS];
 };
 
+struct spacemit_gpio_reg_offsets {
+	u32 gplr;      /* port level - R */
+	u32 gpdr;      /* port direction - R/W */
+	u32 gpsr;      /* port set - W */
+	u32 gpcr;      /* port clear - W */
+	u32 grer;      /* port rising edge R/W */
+	u32 gfer;      /* port falling edge R/W */
+	u32 gedr;      /* edge detect status - R/W1C */
+	u32 gsdr;      /* (set) direction - W */
+	u32 gcdr;      /* (clear) direction - W */
+	u32 gsrer;     /* (set) rising edge detect enable - W */
+	u32 gcrer;     /* (clear) rising edge detect enable - W */
+	u32 gsfer;     /* (set) falling edge detect enable - W */
+	u32 gcfer;     /* (clear) falling edge detect enable - W */
+	u32 gapmask;   /* interrupt mask , 0 disable, 1 enable - R/W */
+	u32 gcpmask;   /* interrupt mask for K3 */
+};
+
 static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
 {
 	return (u32)(gb - gb->sg->sgb);
@@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
 static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
 {
 	struct spacemit_gpio_bank *gb = dev_id;
+	struct spacemit_gpio *sg = gb->sg;
 	unsigned long pending;
 	u32 n, gedr;
 
-	gedr = readl(gb->base + SPACEMIT_GEDR);
+	gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
 	if (!gedr)
 		return IRQ_NONE;
-	writel(gedr, gb->base + SPACEMIT_GEDR);
+	writel(gedr, gb->base + to_spacemit_gpio_regs(sg)->gedr);
 
 	pending = gedr & gb->irq_mask;
 	if (!pending)
@@ -81,60 +92,64 @@ static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
 static void spacemit_gpio_irq_ack(struct irq_data *d)
 {
 	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	struct spacemit_gpio *sg = gb->sg;
 
-	writel(BIT(irqd_to_hwirq(d)), gb->base + SPACEMIT_GEDR);
+	writel(BIT(irqd_to_hwirq(d)), gb->base + to_spacemit_gpio_regs(sg)->gedr);
 }
 
 static void spacemit_gpio_irq_mask(struct irq_data *d)
 {
 	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	struct spacemit_gpio *sg = gb->sg;
 	u32 bit = BIT(irqd_to_hwirq(d));
 
 	gb->irq_mask &= ~bit;
-	writel(gb->irq_mask, gb->base + SPACEMIT_GAPMASK);
+	writel(gb->irq_mask, gb->base + to_spacemit_gpio_regs(sg)->gapmask);
 
 	if (bit & gb->irq_rising_edge)
-		writel(bit, gb->base + SPACEMIT_GCRER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gcrer);
 
 	if (bit & gb->irq_falling_edge)
-		writel(bit, gb->base + SPACEMIT_GCFER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gcfer);
 }
 
 static void spacemit_gpio_irq_unmask(struct irq_data *d)
 {
 	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	struct spacemit_gpio *sg = gb->sg;
 	u32 bit = BIT(irqd_to_hwirq(d));
 
 	gb->irq_mask |= bit;
 
 	if (bit & gb->irq_rising_edge)
-		writel(bit, gb->base + SPACEMIT_GSRER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gsrer);
 
 	if (bit & gb->irq_falling_edge)
-		writel(bit, gb->base + SPACEMIT_GSFER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gsfer);
 
-	writel(gb->irq_mask, gb->base + SPACEMIT_GAPMASK);
+	writel(gb->irq_mask, gb->base + to_spacemit_gpio_regs(sg)->gapmask);
 }
 
 static int spacemit_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct spacemit_gpio_bank *gb = irq_data_get_irq_chip_data(d);
+	struct spacemit_gpio *sg = gb->sg;
 	u32 bit = BIT(irqd_to_hwirq(d));
 
 	if (type & IRQ_TYPE_EDGE_RISING) {
 		gb->irq_rising_edge |= bit;
-		writel(bit, gb->base + SPACEMIT_GSRER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gsrer);
 	} else {
 		gb->irq_rising_edge &= ~bit;
-		writel(bit, gb->base + SPACEMIT_GCRER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gcrer);
 	}
 
 	if (type & IRQ_TYPE_EDGE_FALLING) {
 		gb->irq_falling_edge |= bit;
-		writel(bit, gb->base + SPACEMIT_GSFER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gsfer);
 	} else {
 		gb->irq_falling_edge &= ~bit;
-		writel(bit, gb->base + SPACEMIT_GCFER);
+		writel(bit, gb->base + to_spacemit_gpio_regs(sg)->gcfer);
 	}
 
 	return 0;
@@ -179,15 +194,15 @@ static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
 	struct device *dev = sg->dev;
 	struct gpio_irq_chip *girq;
 	void __iomem *dat, *set, *clr, *dirin, *dirout;
-	int ret, bank_base[] = { 0x0, 0x4, 0x8, 0x100 };
+	int ret;
 
-	gb->base = regs + bank_base[index];
+	gb->base = regs + sg->data->bank_offsets[index];
 
-	dat	= gb->base + SPACEMIT_GPLR;
-	set	= gb->base + SPACEMIT_GPSR;
-	clr	= gb->base + SPACEMIT_GPCR;
-	dirin	= gb->base + SPACEMIT_GCDR;
-	dirout	= gb->base + SPACEMIT_GSDR;
+	dat	= gb->base + to_spacemit_gpio_regs(sg)->gplr;
+	set	= gb->base + to_spacemit_gpio_regs(sg)->gpsr;
+	clr	= gb->base + to_spacemit_gpio_regs(sg)->gpcr;
+	dirin	= gb->base + to_spacemit_gpio_regs(sg)->gcdr;
+	dirout	= gb->base + to_spacemit_gpio_regs(sg)->gsdr;
 
 	config = (struct gpio_generic_chip_config) {
 		.dev = dev,
@@ -223,13 +238,13 @@ static int spacemit_gpio_add_bank(struct spacemit_gpio *sg,
 	gpio_irq_chip_set_chip(girq, &spacemit_gpio_chip);
 
 	/* Disable Interrupt */
-	writel(0, gb->base + SPACEMIT_GAPMASK);
+	writel(0, gb->base + to_spacemit_gpio_regs(sg)->gapmask);
 	/* Disable Edge Detection Settings */
-	writel(0x0, gb->base + SPACEMIT_GRER);
-	writel(0x0, gb->base + SPACEMIT_GFER);
+	writel(0x0, gb->base + to_spacemit_gpio_regs(sg)->grer);
+	writel(0x0, gb->base + to_spacemit_gpio_regs(sg)->gfer);
 	/* Clear Interrupt */
-	writel(0xffffffff, gb->base + SPACEMIT_GCRER);
-	writel(0xffffffff, gb->base + SPACEMIT_GCFER);
+	writel(0xffffffff, gb->base + to_spacemit_gpio_regs(sg)->gcrer);
+	writel(0xffffffff, gb->base + to_spacemit_gpio_regs(sg)->gcfer);
 
 	ret = devm_request_threaded_irq(dev, irq, NULL,
 					spacemit_gpio_irq_handler,
@@ -260,6 +275,10 @@ static int spacemit_gpio_probe(struct platform_device *pdev)
 	if (!sg)
 		return -ENOMEM;
 
+	sg->data = of_device_get_match_data(dev);
+	if (!sg->data)
+		return dev_err_probe(dev, -EINVAL, "No available compatible data.");
+
 	regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(regs))
 		return PTR_ERR(regs);
@@ -287,8 +306,51 @@ static int spacemit_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct spacemit_gpio_data k1_gpio_data = {
+	.reg_offsets = &(struct spacemit_gpio_reg_offsets) {
+		.gplr    = 0x00,
+		.gpdr    = 0x0c,
+		.gpsr    = 0x18,
+		.gpcr    = 0x24,
+		.grer    = 0x30,
+		.gfer    = 0x3c,
+		.gedr    = 0x48,
+		.gsdr    = 0x54,
+		.gcdr    = 0x60,
+		.gsrer   = 0x6c,
+		.gcrer   = 0x78,
+		.gsfer   = 0x84,
+		.gcfer   = 0x90,
+		.gapmask = 0x9c,
+		.gcpmask = 0xA8,
+	},
+	.bank_offsets = { 0x0, 0x4, 0x8, 0x100 },
+};
+
+static const struct spacemit_gpio_data k3_gpio_data = {
+	.reg_offsets = &(struct spacemit_gpio_reg_offsets) {
+		.gplr    = 0x0,
+		.gpdr    = 0x4,
+		.gpsr    = 0x8,
+		.gpcr    = 0xc,
+		.grer    = 0x10,
+		.gfer    = 0x14,
+		.gedr    = 0x18,
+		.gsdr    = 0x1c,
+		.gcdr    = 0x20,
+		.gsrer   = 0x24,
+		.gcrer   = 0x28,
+		.gsfer   = 0x2c,
+		.gcfer   = 0x30,
+		.gapmask = 0x34,
+		.gcpmask = 0x38,
+	},
+	.bank_offsets = { 0x0, 0x40, 0x80, 0x100 },
+};
+
 static const struct of_device_id spacemit_gpio_dt_ids[] = {
-	{ .compatible = "spacemit,k1-gpio" },
+	{ .compatible = "spacemit,k1-gpio", .data = &k1_gpio_data },
+	{ .compatible = "spacemit,k3-gpio", .data = &k3_gpio_data },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, spacemit_gpio_dt_ids);
@@ -296,12 +358,12 @@ MODULE_DEVICE_TABLE(of, spacemit_gpio_dt_ids);
 static struct platform_driver spacemit_gpio_driver = {
 	.probe		= spacemit_gpio_probe,
 	.driver		= {
-		.name	= "k1-gpio",
+		.name	= "spacemit-gpio",
 		.of_match_table = spacemit_gpio_dt_ids,
 	},
 };
 module_platform_driver(spacemit_gpio_driver);
 
 MODULE_AUTHOR("Yixun Lan <dlan@gentoo.org>");
-MODULE_DESCRIPTION("GPIO driver for SpacemiT K1 SoC");
+MODULE_DESCRIPTION("GPIO driver for SpacemiT K1/K3 SoC");
 MODULE_LICENSE("GPL");

-- 
2.52.0

On Mon, Dec 29, 2025 at 1:47 PM Yixun Lan <dlan@gentoo.org> wrote:
>
> SpacemiT K3 SoC has changed gpio register layout while comparing
> with previous generation, the register offset and bank offset
> need to be adjusted, introduce a compatible data to extend the
> driver to support this.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
>  1 file changed, 106 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> index eb66a15c002f..02cc5c11b617 100644
> --- a/drivers/gpio/gpio-spacemit-k1.c
> +++ b/drivers/gpio/gpio-spacemit-k1.c
> @@ -15,28 +15,19 @@
>  #include <linux/platform_device.h>
>  #include <linux/seq_file.h>
>
> -/* register offset */
> -#define SPACEMIT_GPLR          0x00 /* port level - R */
> -#define SPACEMIT_GPDR          0x0c /* port direction - R/W */
> -#define SPACEMIT_GPSR          0x18 /* port set - W */
> -#define SPACEMIT_GPCR          0x24 /* port clear - W */
> -#define SPACEMIT_GRER          0x30 /* port rising edge R/W */
> -#define SPACEMIT_GFER          0x3c /* port falling edge R/W */
> -#define SPACEMIT_GEDR          0x48 /* edge detect status - R/W1C */
> -#define SPACEMIT_GSDR          0x54 /* (set) direction - W */
> -#define SPACEMIT_GCDR          0x60 /* (clear) direction - W */
> -#define SPACEMIT_GSRER         0x6c /* (set) rising edge detect enable - W */
> -#define SPACEMIT_GCRER         0x78 /* (clear) rising edge detect enable - W */
> -#define SPACEMIT_GSFER         0x84 /* (set) falling edge detect enable - W */
> -#define SPACEMIT_GCFER         0x90 /* (clear) falling edge detect enable - W */
> -#define SPACEMIT_GAPMASK       0x9c /* interrupt mask , 0 disable, 1 enable - R/W */
> -
>  #define SPACEMIT_NR_BANKS              4
>  #define SPACEMIT_NR_GPIOS_PER_BANK     32
>
>  #define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> +#define to_spacemit_gpio_regs(sg) ((sg)->data->reg_offsets)
>
>  struct spacemit_gpio;
> +struct spacemit_gpio_reg_offsets;

Why not move this structure here instead and avoid the forward declaration?

> +
> +struct spacemit_gpio_data {
> +       struct spacemit_gpio_reg_offsets *reg_offsets;
> +       u32 bank_offsets[4];
> +};
>
>  struct spacemit_gpio_bank {
>         struct gpio_generic_chip chip;
> @@ -49,9 +40,28 @@ struct spacemit_gpio_bank {
>
>  struct spacemit_gpio {
>         struct device *dev;
> +       const struct spacemit_gpio_data *data;
>         struct spacemit_gpio_bank sgb[SPACEMIT_NR_BANKS];
>  };
>
> +struct spacemit_gpio_reg_offsets {
> +       u32 gplr;      /* port level - R */
> +       u32 gpdr;      /* port direction - R/W */
> +       u32 gpsr;      /* port set - W */
> +       u32 gpcr;      /* port clear - W */
> +       u32 grer;      /* port rising edge R/W */
> +       u32 gfer;      /* port falling edge R/W */
> +       u32 gedr;      /* edge detect status - R/W1C */
> +       u32 gsdr;      /* (set) direction - W */
> +       u32 gcdr;      /* (clear) direction - W */
> +       u32 gsrer;     /* (set) rising edge detect enable - W */
> +       u32 gcrer;     /* (clear) rising edge detect enable - W */
> +       u32 gsfer;     /* (set) falling edge detect enable - W */
> +       u32 gcfer;     /* (clear) falling edge detect enable - W */
> +       u32 gapmask;   /* interrupt mask , 0 disable, 1 enable - R/W */
> +       u32 gcpmask;   /* interrupt mask for K3 */
> +};
> +
>  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>  {
>         return (u32)(gb - gb->sg->sgb);
> @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
>  {
>         struct spacemit_gpio_bank *gb = dev_id;
> +       struct spacemit_gpio *sg = gb->sg;
>         unsigned long pending;
>         u32 n, gedr;
>
> -       gedr = readl(gb->base + SPACEMIT_GEDR);
> +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);

Since you're already touching all these register accesses - can you
maybe provide dedicated wrapper functions around readl()/writel() and
avoid any file-wide changes in the future if anything requires further
modification?

Bart

Hi Bart,

On 12:10 Fri 02 Jan     , Bartosz Golaszewski wrote:
> On Mon, Dec 29, 2025 at 1:47 PM Yixun Lan <dlan@gentoo.org> wrote:
> >
> > SpacemiT K3 SoC has changed gpio register layout while comparing
> > with previous generation, the register offset and bank offset
> > need to be adjusted, introduce a compatible data to extend the
> > driver to support this.
> >
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
> >  1 file changed, 106 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> > index eb66a15c002f..02cc5c11b617 100644
> > --- a/drivers/gpio/gpio-spacemit-k1.c
> > +++ b/drivers/gpio/gpio-spacemit-k1.c
> > @@ -15,28 +15,19 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/seq_file.h>
> >
> > -/* register offset */
> > -#define SPACEMIT_GPLR          0x00 /* port level - R */
> > -#define SPACEMIT_GPDR          0x0c /* port direction - R/W */
> > -#define SPACEMIT_GPSR          0x18 /* port set - W */
> > -#define SPACEMIT_GPCR          0x24 /* port clear - W */
> > -#define SPACEMIT_GRER          0x30 /* port rising edge R/W */
> > -#define SPACEMIT_GFER          0x3c /* port falling edge R/W */
> > -#define SPACEMIT_GEDR          0x48 /* edge detect status - R/W1C */
> > -#define SPACEMIT_GSDR          0x54 /* (set) direction - W */
> > -#define SPACEMIT_GCDR          0x60 /* (clear) direction - W */
> > -#define SPACEMIT_GSRER         0x6c /* (set) rising edge detect enable - W */
> > -#define SPACEMIT_GCRER         0x78 /* (clear) rising edge detect enable - W */
> > -#define SPACEMIT_GSFER         0x84 /* (set) falling edge detect enable - W */
> > -#define SPACEMIT_GCFER         0x90 /* (clear) falling edge detect enable - W */
> > -#define SPACEMIT_GAPMASK       0x9c /* interrupt mask , 0 disable, 1 enable - R/W */
> > -
> >  #define SPACEMIT_NR_BANKS              4
> >  #define SPACEMIT_NR_GPIOS_PER_BANK     32
> >
> >  #define to_spacemit_gpio_bank(x) container_of((x), struct spacemit_gpio_bank, gc)
> > +#define to_spacemit_gpio_regs(sg) ((sg)->data->reg_offsets)
> >
> >  struct spacemit_gpio;
> > +struct spacemit_gpio_reg_offsets;
> 
> Why not move this structure here instead and avoid the forward declaration?
> 
sure, I will do

> > +
> > +struct spacemit_gpio_data {
> > +       struct spacemit_gpio_reg_offsets *reg_offsets;
> > +       u32 bank_offsets[4];
> > +};
> >
> >  struct spacemit_gpio_bank {
> >         struct gpio_generic_chip chip;
> > @@ -49,9 +40,28 @@ struct spacemit_gpio_bank {
> >
> >  struct spacemit_gpio {
> >         struct device *dev;
> > +       const struct spacemit_gpio_data *data;
> >         struct spacemit_gpio_bank sgb[SPACEMIT_NR_BANKS];
> >  };
> >
> > +struct spacemit_gpio_reg_offsets {
> > +       u32 gplr;      /* port level - R */
> > +       u32 gpdr;      /* port direction - R/W */
> > +       u32 gpsr;      /* port set - W */
> > +       u32 gpcr;      /* port clear - W */
> > +       u32 grer;      /* port rising edge R/W */
> > +       u32 gfer;      /* port falling edge R/W */
> > +       u32 gedr;      /* edge detect status - R/W1C */
> > +       u32 gsdr;      /* (set) direction - W */
> > +       u32 gcdr;      /* (clear) direction - W */
> > +       u32 gsrer;     /* (set) rising edge detect enable - W */
> > +       u32 gcrer;     /* (clear) rising edge detect enable - W */
> > +       u32 gsfer;     /* (set) falling edge detect enable - W */
> > +       u32 gcfer;     /* (clear) falling edge detect enable - W */
> > +       u32 gapmask;   /* interrupt mask , 0 disable, 1 enable - R/W */
> > +       u32 gcpmask;   /* interrupt mask for K3 */
> > +};
> > +
> >  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> >  {
> >         return (u32)(gb - gb->sg->sgb);
> > @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> >  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> >  {
> >         struct spacemit_gpio_bank *gb = dev_id;
> > +       struct spacemit_gpio *sg = gb->sg;
> >         unsigned long pending;
> >         u32 n, gedr;
> >
> > -       gedr = readl(gb->base + SPACEMIT_GEDR);
> > +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
> 
> Since you're already touching all these register accesses - can you
> maybe provide dedicated wrapper functions around readl()/writel() and
> avoid any file-wide changes in the future if anything requires further
> modification?
> 
can you elaborate a bit further on this?
I don't get how a wrapper helper could help to avoid file-wide changes..

-- 
Yixun Lan (dlan)

Hi bart,

On 19:36 Fri 02 Jan     , Yixun Lan wrote:
> Hi Bart,
> 
> On 12:10 Fri 02 Jan     , Bartosz Golaszewski wrote:
> > On Mon, Dec 29, 2025 at 1:47 PM Yixun Lan <dlan@gentoo.org> wrote:
> > >
> > > SpacemiT K3 SoC has changed gpio register layout while comparing
> > > with previous generation, the register offset and bank offset
> > > need to be adjusted, introduce a compatible data to extend the
> > > driver to support this.
> > >
> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > > ---
> > >  drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
> > >  1 file changed, 106 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> > > index eb66a15c002f..02cc5c11b617 100644
> > > --- a/drivers/gpio/gpio-spacemit-k1.c
> > > +++ b/drivers/gpio/gpio-spacemit-k1.c
> > > @@ -15,28 +15,19 @@
> > >  #include <linux/platform_device.h>
> > >  #include <linux/seq_file.h>
> > >
[snip]...
> > >  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> > >  {
> > >         return (u32)(gb - gb->sg->sgb);
> > > @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> > >  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> > >  {
> > >         struct spacemit_gpio_bank *gb = dev_id;
> > > +       struct spacemit_gpio *sg = gb->sg;
> > >         unsigned long pending;
> > >         u32 n, gedr;
> > >
> > > -       gedr = readl(gb->base + SPACEMIT_GEDR);
> > > +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
> > 
> > Since you're already touching all these register accesses - can you
> > maybe provide dedicated wrapper functions around readl()/writel() and
> > avoid any file-wide changes in the future if anything requires further
> > modification?
> > 
> can you elaborate a bit further on this?
> I don't get how a wrapper helper could help to avoid file-wide changes..
> 
here is my attempt to solve this, define a macro to register address:

#define to_spacemit_gpio_regs(gb) ((gb)->sg->data->reg_offsets)

#define SPACEMIT_GEDR(gb)      ((gb)->base + to_spacemit_gpio_regs(gb)->gedr)

	gedr = readl(SPACEMIT_GEDR(gb));

please let me know if this follow your suggestion or not

-- 
Yixun Lan (dlan)

On Fri, 2 Jan 2026 13:20:45 +0100, Yixun Lan <dlan@gentoo.org> said:
> Hi bart,
>
> On 19:36 Fri 02 Jan     , Yixun Lan wrote:
>> Hi Bart,
>>
>> On 12:10 Fri 02 Jan     , Bartosz Golaszewski wrote:
>> > On Mon, Dec 29, 2025 at 1:47 PM Yixun Lan <dlan@gentoo.org> wrote:
>> > >
>> > > SpacemiT K3 SoC has changed gpio register layout while comparing
>> > > with previous generation, the register offset and bank offset
>> > > need to be adjusted, introduce a compatible data to extend the
>> > > driver to support this.
>> > >
>> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
>> > > ---
>> > >  drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
>> > >  1 file changed, 106 insertions(+), 44 deletions(-)
>> > >
>> > > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
>> > > index eb66a15c002f..02cc5c11b617 100644
>> > > --- a/drivers/gpio/gpio-spacemit-k1.c
>> > > +++ b/drivers/gpio/gpio-spacemit-k1.c
>> > > @@ -15,28 +15,19 @@
>> > >  #include <linux/platform_device.h>
>> > >  #include <linux/seq_file.h>
>> > >
> [snip]...
>> > >  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>> > >  {
>> > >         return (u32)(gb - gb->sg->sgb);
>> > > @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>> > >  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
>> > >  {
>> > >         struct spacemit_gpio_bank *gb = dev_id;
>> > > +       struct spacemit_gpio *sg = gb->sg;
>> > >         unsigned long pending;
>> > >         u32 n, gedr;
>> > >
>> > > -       gedr = readl(gb->base + SPACEMIT_GEDR);
>> > > +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
>> >
>> > Since you're already touching all these register accesses - can you
>> > maybe provide dedicated wrapper functions around readl()/writel() and
>> > avoid any file-wide changes in the future if anything requires further
>> > modification?
>> >
>> can you elaborate a bit further on this?
>> I don't get how a wrapper helper could help to avoid file-wide changes..
>>
> here is my attempt to solve this, define a macro to register address:
>
> #define to_spacemit_gpio_regs(gb) ((gb)->sg->data->reg_offsets)
>
> #define SPACEMIT_GEDR(gb)      ((gb)->base + to_spacemit_gpio_regs(gb)->gedr)
>
> 	gedr = readl(SPACEMIT_GEDR(gb));
>
> please let me know if this follow your suggestion or not
>
> --
> Yixun Lan (dlan)
>

I was thinking more of something like this:

enum spacemit_gpio_registers {
	SPACEMIT_GPLR,
	SPACEMIT_GPDR,
	...
};

static const unsigned int spacemit_gpio_k1_offsets = {
	[SPACEMIT_GPLR] = 0x00,
	[SPACEMIT_GPDR] = 0x0c,
	...
};

static const unsigned int spacemit_gpio_k3_offsets = ...

struct spacemit_gpio_data {
	const unsigned int *offsets;
	u32 bank_offsets[4];
};

static void spacemit_gpio_write(struct spacemit_gpio_bank *gb,
				enum spacemit_gpio_registers reg, u32 val)
{
	writel(val, gb->base + gb->data->offsets[reg]);
}

Bart

Hi Bartosz,

On 08:38 Fri 02 Jan     , Bartosz Golaszewski wrote:
> On Fri, 2 Jan 2026 13:20:45 +0100, Yixun Lan <dlan@gentoo.org> said:
> > Hi bart,
> >
> > On 19:36 Fri 02 Jan     , Yixun Lan wrote:
> >> Hi Bart,
> >>
> >> On 12:10 Fri 02 Jan     , Bartosz Golaszewski wrote:
> >> > On Mon, Dec 29, 2025 at 1:47 PM Yixun Lan <dlan@gentoo.org> wrote:
> >> > >
> >> > > SpacemiT K3 SoC has changed gpio register layout while comparing
> >> > > with previous generation, the register offset and bank offset
> >> > > need to be adjusted, introduce a compatible data to extend the
> >> > > driver to support this.
> >> > >
> >> > > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> >> > > ---
> >> > >  drivers/gpio/gpio-spacemit-k1.c | 150 ++++++++++++++++++++++++++++------------
> >> > >  1 file changed, 106 insertions(+), 44 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpio/gpio-spacemit-k1.c b/drivers/gpio/gpio-spacemit-k1.c
> >> > > index eb66a15c002f..02cc5c11b617 100644
> >> > > --- a/drivers/gpio/gpio-spacemit-k1.c
> >> > > +++ b/drivers/gpio/gpio-spacemit-k1.c
> >> > > @@ -15,28 +15,19 @@
> >> > >  #include <linux/platform_device.h>
> >> > >  #include <linux/seq_file.h>
> >> > >
> > [snip]...
> >> > >  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> >> > >  {
> >> > >         return (u32)(gb - gb->sg->sgb);
> >> > > @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
> >> > >  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
> >> > >  {
> >> > >         struct spacemit_gpio_bank *gb = dev_id;
> >> > > +       struct spacemit_gpio *sg = gb->sg;
> >> > >         unsigned long pending;
> >> > >         u32 n, gedr;
> >> > >
> >> > > -       gedr = readl(gb->base + SPACEMIT_GEDR);
> >> > > +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
> >> >
> >> > Since you're already touching all these register accesses - can you
> >> > maybe provide dedicated wrapper functions around readl()/writel() and
> >> > avoid any file-wide changes in the future if anything requires further
> >> > modification?
> >> >
> >> can you elaborate a bit further on this?
> >> I don't get how a wrapper helper could help to avoid file-wide changes..
> >>
> > here is my attempt to solve this, define a macro to register address:
> >
> > #define to_spacemit_gpio_regs(gb) ((gb)->sg->data->reg_offsets)
> >
> > #define SPACEMIT_GEDR(gb)      ((gb)->base + to_spacemit_gpio_regs(gb)->gedr)
> >
> > 	gedr = readl(SPACEMIT_GEDR(gb));
> >
> > please let me know if this follow your suggestion or not
> >
> > --
> > Yixun Lan (dlan)
> >
> 
> I was thinking more of something like this:
> 
> enum spacemit_gpio_registers {
> 	SPACEMIT_GPLR,
> 	SPACEMIT_GPDR,
> 	...
> };
> 
> static const unsigned int spacemit_gpio_k1_offsets = {
> 	[SPACEMIT_GPLR] = 0x00,
> 	[SPACEMIT_GPDR] = 0x0c,
> 	...
> };
> 
> static const unsigned int spacemit_gpio_k3_offsets = ...
> 
> struct spacemit_gpio_data {
> 	const unsigned int *offsets;
> 	u32 bank_offsets[4];
> };
> 
> static void spacemit_gpio_write(struct spacemit_gpio_bank *gb,
> 				enum spacemit_gpio_registers reg, u32 val)
> {
> 	writel(val, gb->base + gb->data->offsets[reg]);
> }
> 
Ok, I will implement this, thanks for the detail prototype

-- 
Yixun Lan (dlan)

On Fri, 2 Jan 2026 12:36:43 +0100, Yixun Lan <dlan@gentoo.org> said:
>
>> >  static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>> >  {
>> >         return (u32)(gb - gb->sg->sgb);
>> > @@ -60,13 +70,14 @@ static u32 spacemit_gpio_bank_index(struct spacemit_gpio_bank *gb)
>> >  static irqreturn_t spacemit_gpio_irq_handler(int irq, void *dev_id)
>> >  {
>> >         struct spacemit_gpio_bank *gb = dev_id;
>> > +       struct spacemit_gpio *sg = gb->sg;
>> >         unsigned long pending;
>> >         u32 n, gedr;
>> >
>> > -       gedr = readl(gb->base + SPACEMIT_GEDR);
>> > +       gedr = readl(gb->base + to_spacemit_gpio_regs(sg)->gedr);
>>
>> Since you're already touching all these register accesses - can you
>> maybe provide dedicated wrapper functions around readl()/writel() and
>> avoid any file-wide changes in the future if anything requires further
>> modification?
>>
> can you elaborate a bit further on this?
> I don't get how a wrapper helper could help to avoid file-wide changes..
>

Just create functions called "spacemit_gpio_read/write()" that wrap the
readl()/writel() and its arguments.

Like:

static u32 spacemit_gpio_read(struct spacemit_gpio_bank *gb, unsigned long reg)
{
	return readl(gb->base + func_to_convert_reg_enum_to_offset(reg));
}

Looks cleaner at call sites even if it's a bit more complex. Just create an
array mapping the register enum to offset and assign it to platform data.

Bart