Re: [PATCH 2/2] gpio: Add xgs-iproc driver

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

Hi Chris!

Thanks for your patch!

On Fri, Oct 4, 2019 at 3:25 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:

> This driver supports the Chip Common A GPIO controller present on a
> number of Broadcom switch ASICs with integrated SoCs. The controller is
> similar to the pinctrl-nsp-gpio and pinctrl-iproc-gpio blocks but
> different enough that a separate driver is required.
>
> This has been ported from Broadcom's XLDK 5.0.3 retaining only the CCA
> support (pinctrl-iproc-gpio covers CCB).
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

(...)

> +config GPIO_XGS_IPROC
> +       tristate "BRCM XGS iProc GPIO support"
> +       depends on OF_GPIO && (ARCH_BCM_IPROC || COMPILE_TEST)
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP

Nice reuse of abstractions.

> +static u32 iproc_gpio_readl(struct iproc_gpio_chip *chip, int reg)
> +{
> +       return readl(chip->base + reg);
> +}
> +
> +static void iproc_gpio_writel(struct iproc_gpio_chip *chip, u32 val, int reg)
> +{
> +       writel(val, chip->base + reg);
> +}

These wrappers don't really add anything do they? Just inline the
direct readl()/writel() to base + reg everywhere instead.

> +/* returns the corresponding gpio register bit */
> +static int iproc_irq_to_gpio(struct iproc_gpio_chip *chip, u32 irq)
> +{
> +       struct irq_data *data = irq_domain_get_irq_data(chip->irq_domain, irq);
> +
> +       return data->hwirq;
> +}

I would name it something clearer since "gpio" is pretty ambigous.

Like iproc_irq_to_gpio_offset()

Maybe this is also a bit of unnecessary wrapper?

> +static irqreturn_t iproc_gpio_irq_handler(int irq, void *data)
> +{
> +       struct iproc_gpio_chip *chip = (struct iproc_gpio_chip *)data;
> +       struct gpio_chip gc = chip->gc;
> +       int bit;
> +       unsigned long int_bits = 0;
> +       u32 int_status;
> +
> +       /* go through the entire GPIOs and handle all interrupts */
> +       int_status = readl(chip->intr + CCA_INT_STS);
> +       if (int_status & CCA_INT_F_GPIOINT) {
> +               u32 event, level;
> +
> +               /* Get level and edge interrupts */
> +               event = readl(chip->base + GPIO_CCA_INT_EVENT_MASK);
> +               event &= readl(chip->base + GPIO_CCA_INT_EVENT);
> +               level = readl(chip->base + GPIO_CCA_DIN);
> +               level ^= readl(chip->base + GPIO_CCA_INT_LEVEL);
> +               level &= readl(chip->base + GPIO_CCA_INT_LEVEL_MASK);
> +               int_bits = level | event;
> +
> +               for_each_set_bit(bit, &int_bits, gc.ngpio)
> +                       generic_handle_irq(
> +                               irq_linear_revmap(chip->irq_domain, bit));
> +       }
> +
> +       return  int_bits ? IRQ_HANDLED : IRQ_NONE;
> +}

I think this should be a chained interrupt handler (see below how to
register it).

See e.g. drivers/gpio/gpio-ftgpio010.c for an example:
change function prototype, no return value, use
chained_irq_enter/exit(irqchip, desc); etc.

> +static int iproc_gpiolib_input(struct gpio_chip *gc, u32 gpio)
> +static int iproc_gpiolib_output(struct gpio_chip *gc, u32 gpio, int value)
> +static void iproc_gpiolib_set(struct gpio_chip *gc, u32 gpio, int value)
> +static int iproc_gpiolib_get(struct gpio_chip *gc, u32 gpio)

These callbacks seems to reimplement parts of GPIO_GENERIC
that you should already be using.

Again look at drivers/gpio/gpio-ftgpio010.c() use bgpio_init()
to set up the library callbacks, look in
drivers/gpio/gpio-mmio.c for kerneldoc on the function.

> +static int iproc_gpiolib_to_irq(struct gpio_chip *gc, u32 offset)
> +{
> +       struct iproc_gpio_chip *chip = gpiochip_get_data(gc);
> +
> +       return irq_linear_revmap(chip->irq_domain, offset);
> +}

GPIOLIB_IRQCHIP provides a .to_irq() implementation
so drop this and let the library handle this.

> +static int iproc_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *dn = pdev->dev.of_node;
> +       struct iproc_gpio_chip *chip;
> +       u32 num_gpios;
> +       int irq, ret;
> +
> +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;
> +
> +       chip->dev = dev;
> +       platform_set_drvdata(pdev, chip);
> +
> +       chip->gc.label = dev_name(dev);
> +
> +       chip->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(chip->base))
> +               return PTR_ERR(chip->base);
> +
> +       /* Get number of GPIO pin */
> +       if (of_property_read_u32(dn, "ngpios", &num_gpios)) {
> +               dev_err(dev, "missing ngpios DT property\n");
> +               return -EINVAL;
> +       }

Maybe provide a sensible default?

> +       chip->gc.ngpio = num_gpios;
> +       chip->gc.parent = dev;
> +       chip->gc.of_node = dn;
> +       chip->gc.direction_input = iproc_gpiolib_input;
> +       chip->gc.direction_output = iproc_gpiolib_output;
> +       chip->gc.set = iproc_gpiolib_set;
> +       chip->gc.get = iproc_gpiolib_get;

Drop this and call bgpio_init() to set up the callbacks
instead. However, set up .ngpio *after* calling
bgpio_init() so users can't access the nonexisting
gpios.

> +       chip->gc.to_irq = iproc_gpiolib_to_irq;

Drop this and use the GPIOLIB_IRQCHIP.

> +       ret = gpiochip_add_data(&chip->gc, chip);
> +       if (ret) {
> +               dev_err(dev, "unable to add GPIO chip\n");
> +               return ret;
> +       }

Why not use devm_gpiochip_add_data()?

> +       irq = platform_get_irq(pdev, 0);
> +       if (irq > 0) {
> +               u32 val, count;
> +               struct irq_chip *irqc;
> +
> +               irqc = &chip->irqchip;
> +               irqc->name = dev_name(dev);
> +               irqc->irq_ack = iproc_gpio_irq_ack;
> +               irqc->irq_mask = iproc_gpio_irq_mask;
> +               irqc->irq_unmask = iproc_gpio_irq_unmask;
> +               irqc->irq_set_type = iproc_gpio_irq_set_type;
> +               irqc->irq_enable = iproc_gpio_irq_unmask;
> +               irqc->irq_disable = iproc_gpio_irq_mask;
> +               chip->intr = devm_platform_ioremap_resource(pdev, 1);
> +               if (IS_ERR(chip->intr))
> +                       return PTR_ERR(chip->intr);

Move all this above the [devm_]gpiochip_add_data()
call and add:

struct gpio_irq_chip *girq;

girq = &chip->gc.irq;
girq->chip = irqc;

etc follow the pattern from other drivers like the
mentioned ftgpio010.c.

> +               /* Create irq domain */
> +               chip->irq_domain = irq_domain_add_linear(dn, num_gpios,
> +                               &irq_domain_simple_ops, chip);
> +
> +               if (!chip->irq_domain) {
> +                       dev_err(dev, "Couldn't allocate IRQ domain\n");
> +                       ret = -ENODEV;
> +                       goto err_irq_domain;
> +               }
> +
> +               /* Map each gpio pin to an IRQ and set the handler */
> +               for (count = 0; count < num_gpios; count++) {
> +                       int irq;
> +
> +                       irq = irq_create_mapping(chip->irq_domain, count);
> +                       irq_set_chip_and_handler(irq, irqc, handle_simple_irq);
> +                       irq_set_chip_data(irq, chip);
> +               }

Drop this and let the generic GPIO irqchip handle
the domain.

> +               /* Enable GPIO interrupts for CCA GPIO */
> +               val = readl(chip->intr + CCA_INT_MASK);
> +               val |= CCA_INT_F_GPIOINT;
> +               writel(val, chip->intr + CCA_INT_MASK);

Move this before registering the chip.

> +               /* Install ISR for this GPIO controller */
> +               ret = devm_request_irq(dev, irq, iproc_gpio_irq_handler,
> +                                      IRQF_SHARED, chip->gc.label, chip);
> +               if (ret) {
> +                       dev_err(dev, "Fail to request IRQ%d: %d\n", irq, ret);
> +                       goto err_irq_request;
> +               }

Drop this and use the gpiolib irqchip library.

Yours,
Linus Walleij