Re: [PATCH v3 2/3] gpio: spacemit: add support for K1 SoC

[Linus Walleij]

Hi Yixun,

thanks for your patch!

Some comments below:

On Wed, Dec 25, 2024 at 1:33 AM Yixun Lan <dlan@xxxxxxxxxx> wrote:

> Implement GPIO functionality which capable of setting pin as
> input, output. Also, each pin can be used as interrupt which
> support rising/failing edge type trigger, or both.
>
> Signed-off-by: Yixun Lan <dlan@xxxxxxxxxx>
(...)
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>

Please drop this legacy include. It should not be needed.

> +#include <linux/gpio/driver.h>

This should be enough for any driver.

> +/* register offset */
> +#define GPLR           0x00
> +#define GPDR           0x0c
> +#define GPSR           0x18
> +#define GPCR           0x24
> +#define GRER           0x30
> +#define GFER           0x3c
> +#define GEDR           0x48
> +#define GSDR           0x54
> +#define GCDR           0x60
> +#define GSRER          0x6c
> +#define GCRER          0x78
> +#define GSFER          0x84
> +#define GCFER          0x90
> +#define GAPMASK                0x9c
> +#define GCPMASK                0xa8

This looks like the archetype of a memory-mapped GPIO chip.

> +#define spacemit_gpio_to_bank_idx(gpio)                ((gpio) / K1_BANK_GPIO_NUMBER)
> +#define spacemit_gpio_to_bank_offset(gpio)     ((gpio) & BANK_GPIO_MASK)
> +#define spacemit_bank_to_gpio(idx, offset)     \
> +       (((idx) * K1_BANK_GPIO_NUMBER) | ((offset) & BANK_GPIO_MASK))
> +
> +static u32 k1_gpio_bank_offset[] = { 0x0, 0x4, 0x8, 0x100 };

Yet this is not registered on a per-bank basis, instead all four banks
are hammered into one single chip. Why?

Can you please split this into 4 instances, also in the device
tree. It makes everything much simpler.

> +struct spacemit_gpio_chip {
> +       struct gpio_chip                chip;
> +       struct irq_domain               *domain;

If you're using GPIOLIB_IRQCHIP you should not
also define youe own irq_domain, the gpiolib handles this.

> +       struct spacemit_gpio_bank       *banks;
> +       void __iomem                    *reg_base;
> +       unsigned int                    ngpio;

Is this ever used after initialization?

> +       unsigned int                    nbank;
> +       int                             irq;

Or this?

> +static int spacemit_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct spacemit_gpio_chip *schip = to_spacemit_gpio_chip(chip);
> +
> +       return irq_create_mapping(schip->domain, offset);
> +}

Should not be needed when using GPIOLIB_IRQCHIP.

> +       schip = to_spacemit_gpio_chip(chip);
> +       bank = spacemit_gpio_get_bank(schip, offset);
> +       bit = BIT(spacemit_gpio_to_bank_offset(offset));

#include <linux/bits.h> to use the BIT() macro.

This driver becomes unnecessarily complex due to collecting 4 GPIO chips
into one.

Please split it into 4 instances of the same driver, then look at other
drivers using select GPIO_GENERIC such as
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-ftgpio010.c
for examples of how to create a very compact and simple driver
reusing the generic GPIO helpers, this will also provide
get/set_multiple implementations and other useful things.

Yours,
Linus Walleij