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