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
