Re: [PATCH v2 3/4] gpio: Add RDA Micro GPIO controller support

[Linus Walleij <linus.walleij@linaro.org>]

Hi Manivannan!

Thanks for your patch!

On Tue, Oct 15, 2019 at 7:30 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:

> Add support for GPIO controller from RDA Micro.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Please use a little bit more verbose commit message, who
made this hardware and what is it for. If you know!

> +config GPIO_RDA
> +       bool "RDA Micro GPIO controller support"
> +       depends on ARCH_RDA || COMPILE_TEST
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP

select GPIO_GENERIC

> +#include <linux/bitops.h>

Do you need this or just <linux/bits.h>?

> +#define RDA_GPIO_OEN_VAL               0x00
> +#define RDA_GPIO_OEN_SET_OUT           0x04
> +#define RDA_GPIO_OEN_SET_IN            0x08
> +#define RDA_GPIO_VAL                   0x0c
> +#define RDA_GPIO_SET                   0x10
> +#define RDA_GPIO_CLR                   0x14
> +#define RDA_GPIO_INT_CTRL_SET          0x18
> +#define RDA_GPIO_INT_CTRL_CLR          0x1c
> +#define RDA_GPIO_INT_CLR               0x20
> +#define RDA_GPIO_INT_STATUS            0x24

This is a very clear cut MMIO GPIO so use GPIO_GENERIC with this
hardware.

> +static void rda_gpio_update(struct gpio_chip *chip, unsigned int offset,
> +                           u16 reg, int val)

Maybe keep this if it saves code from the IRQ callbacks,
inline it to register writes if it doesn't get called much.

> +static int rda_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> +static int rda_gpio_direction_output(struct gpio_chip *chip,
> +                                    unsigned int offset, int value)
> +static int rda_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +static void rda_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)

This can all be replaces by select GPIO_GENERIC and passing
the right offsets into bgpio_init(). Look at for example
gpio-ftgpio010.c and the documentation for bgpio_init()
in gpio-mmio.c for help.

This will also implement get/set_multiple for you for
free!

> +static void rda_gpio_irq_mask(struct irq_data *data)
> +static void rda_gpio_irq_ack(struct irq_data *data)

Looks good

> +static int rda_gpio_set_irq(struct gpio_chip *chip, u32 offset,
> +                           unsigned int flow_type)

Maybe _setup_irq()? Not sure, just that the name doesn't
obviously imply how it is used as it is called from two
places.

The rest of the IRQ code looks good!

> +static int rda_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct gpio_irq_chip *irq_chip;

Since irq_chip is the name of a struct in the kernel I usually
just call this "girq" as in "GPIO irq chip".

> +       struct rda_gpio *rda_gpio;
> +       u32 ngpios;
> +       int ret;

Create a struct device *dev = &pdev->dev; helper variable
to make the following code easier to read. (The pointer
&pdev->dev is used in many places...)

> +       /*
> +        * Not all ports have interrupt capability. For instance, on
> +        * RDA8810PL, GPIOC doesn't support interrupt. So we must handle
> +        * those also.
> +        */
> +       rda_gpio->irq = platform_get_irq(pdev, 0);
> +
> +       rda_gpio->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(rda_gpio->base))
> +               return PTR_ERR(rda_gpio->base);
> +
> +       spin_lock_init(&rda_gpio->lock);
> +
> +       rda_gpio->chip.label = dev_name(&pdev->dev);
> +       rda_gpio->chip.ngpio = ngpios;
> +       rda_gpio->chip.base = -1;
> +       rda_gpio->chip.parent = &pdev->dev;
> +       rda_gpio->chip.of_node = np;
> +       rda_gpio->chip.get = rda_gpio_get;
> +       rda_gpio->chip.set = rda_gpio_set;
> +       rda_gpio->chip.direction_input = rda_gpio_direction_input;
> +       rda_gpio->chip.direction_output = rda_gpio_direction_output;
> +
> +       if (rda_gpio->irq >= 0) {
> +               rda_gpio->irq_chip.name = "rda-gpio",
> +               rda_gpio->irq_chip.irq_ack = rda_gpio_irq_ack,
> +               rda_gpio->irq_chip.irq_mask = rda_gpio_irq_mask,
> +               rda_gpio->irq_chip.irq_unmask = rda_gpio_irq_unmask,
> +               rda_gpio->irq_chip.irq_set_type = rda_gpio_irq_set_type,
> +               rda_gpio->irq_chip.flags = IRQCHIP_SKIP_SET_WAKE,
> +
> +               irq_chip = &rda_gpio->chip.irq;
> +               irq_chip->chip = &rda_gpio->irq_chip;
> +               irq_chip->handler = handle_bad_irq;
> +               irq_chip->default_type = IRQ_TYPE_NONE;
> +               irq_chip->parent_handler = rda_gpio_irq_handler;
> +               irq_chip->parent_handler_data = rda_gpio;
> +               irq_chip->num_parents = 1;
> +               irq_chip->parents = &rda_gpio->irq;

That works but ... please devm_kzalloc() like the other drivers
do:

girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
                                     GFP_KERNEL);
        if (!girq->parents) {
                ret = -ENOMEM;
(...)

Unless you have a real good reason to optimize it. I just
want it to follow the pattern since I want to minimize
cognitive stress for the maintainers. (Me.)

Yours,
Linus Walleij