Re: [PATCH 2/2] gpio: spacemit: Add GPIO support for K3 SoC

[Yixun Lan <dlan@gentoo.org>]

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)