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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On Fri, 2019-10-11 at 09:43 +0200, Linus Walleij wrote:
> Hi Chris!
> 
> Thanks for your patch!
> 
> On Fri, Oct 4, 2019 at 3:25 AM Chris Packham
> <chris.packham@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> 
> (...)
> 
> > +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.
> 

Will do.

> > +/* 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?
> 

I'll look into it. We might already have the irq_data we need so the
callers could use "pin = d->hwirq;".

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

I don't think a chained interrupt handler can work. The problem is that
the parent irq on the SoC is shared between the gpio and uart0 (why
it's this way with two IP blocks in the same SoC I'll never know). When
a chained interrupt handler is registered I lose the serial interrupts.
Please correct me if there is some way to make the chained handlers
deal with sharing interrupts.

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

Will look into it.

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

Will that work if I've squirreled the irq_domain away in struct
iproc_gpio_chip? See above as to why I'm not using the embedded
gpio_irq_chip.

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

I don't know that there is one. On the particular SoC I have the number
is 12 but the datasheets I have access to are really hard to follow
(the SoC part is tacked on to the end of the datasheet for the switch
ASIC). 32 would be vaguely sane since they're all 32-bit wide
registers.

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

As I've said above I'm not sure I can due to sharing interrupts with
uart0. I'd be happy to be proven wrong as the generic irq code removes
a great deal of boiler plate for me.