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

From: Manivannan Sadhasivam
Date: Sat Oct 19 2019 - 12:05:35 EST


Hi Linus,

Thanks for the review! Please see comments inline.

On Wed, Oct 16, 2019 at 02:41:32PM +0200, Linus Walleij wrote:
> Hi Manivannan!
>
> Thanks for your patch!
>
> On Tue, Oct 15, 2019 at 7:30 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> > Add support for GPIO controller from RDA Micro.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
>
> Please use a little bit more verbose commit message, who
> made this hardware and what is it for. If you know!
>

okay.

> > +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
>

hmm.. I don't think this driver can use it. Please see the justification
below.

> > +#include <linux/bitops.h>
>
> Do you need this or just <linux/bits.h>?
>

I need this for for_each_set_bit() macro.

> > +#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.
>

So, I'd be happy to use gpio-mmio driver if applicable. In fact, I looked into
that while starting to write this driver since most of the `set*` APIs are
like dups. But one thing which blocked me was, `gpio_get` API.

As you can see in this driver, there are 2 separate registers needs to be
read in order to get the value. RDA_GPIO_VAL needs to be read when the pin
is in input state and RDA_GPIO_SET needs to be read when the pin is in output
state.

The MMIO driver relies on a single `dat` register to read the GPIO state and
this won't fit for this driver and hence my justification for not using it.

> > +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.
>

It is being called from multiple places, so I'd like to keep it as a normal
function.

> > +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.
>

Well, this routine sets the irq_type. But it has multiple usecase.
Like, it is being used to unmask as irq and also to set irq type.
So to be in a equillibrium state, I went for rda_gpio_set_irq().

> 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".
>

Ah, a name change again... will do ;-)

> > + 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...)
>

okay.

> > + /*
> > + * 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.)
>

no issues for me, will do.

Thanks,
Mani

> Yours,
> Linus Walleij