Message-ID: <CACRpkdYGksEseGaO49=1rUi5WBdOxwAd-F1gpgZn+BFatRCdDg@mail.gmail.com>
Date:	Tue, 13 Jan 2015 09:53:12 +0100
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Ray Jui <rjui@...adcom.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Christian Daudt <bcm@...thebug.org>,
	Matt Porter <mporter@...aro.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Russell King <linux@....linux.org.uk>,
	Joe Perches <joe@...ches.com>, Arnd Bergmann <arnd@...db.de>,
	Scott Branden <sbranden@...adcom.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 2/3] gpio: Cygnus: add GPIO driver

On Tue, Dec 16, 2014 at 3:18 AM, Ray Jui <rjui@...adcom.com> wrote:

> This GPIO driver supports all 3 GPIO controllers in the Broadcom Cygnus
> SoC. The 3 GPIO controllers are 1) the ASIU GPIO controller, 2) the
> chipCommonG GPIO controller, and 3) the ALWAYS-ON GPIO controller
>
> Signed-off-by: Ray Jui <rjui@...adcom.com>
> Reviewed-by: Scott Branden <sbranden@...adcom.com>

(Big thanks to Alexandre for doing the major part of the review,
good work with following up so far!)

(...)
> +config GPIO_BCM_CYGNUS
> +       bool "Broadcom Cygnus GPIO support"
> +       depends on ARCH_BCM_CYGNUS && OF_GPIO

select GPIOLIB_IRQCHIP

See more about this below.

> +++ b/drivers/gpio/gpio-bcm-cygnus.c
> @@ -0,0 +1,607 @@
> +/*
> + * Copyright (C) 2014 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/gpio.h>
> +#include <linux/ioport.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqchip/chained_irq.h>

Skip <linux/irq.h> and <linux/irqchip/chained_irq.h>
as these move to the core with GPIOLIB_IRQCHIP

> +#define CYGNUS_GPIO_DATA_IN_OFFSET   0x00
> +#define CYGNUS_GPIO_DATA_OUT_OFFSET  0x04
> +#define CYGNUS_GPIO_OUT_EN_OFFSET    0x08
> +#define CYGNUS_GPIO_IN_TYPE_OFFSET   0x0c
> +#define CYGNUS_GPIO_INT_DE_OFFSET    0x10
> +#define CYGNUS_GPIO_INT_EDGE_OFFSET  0x14
> +#define CYGNUS_GPIO_INT_MSK_OFFSET   0x18
> +#define CYGNUS_GPIO_INT_STAT_OFFSET  0x1c
> +#define CYGNUS_GPIO_INT_MSTAT_OFFSET 0x20
> +#define CYGNUS_GPIO_INT_CLR_OFFSET   0x24
> +#define CYGNUS_GPIO_PAD_RES_OFFSET   0x34
> +#define CYGNUS_GPIO_RES_EN_OFFSET    0x38
> +
> +/* drive strength control for ASIU GPIO */
> +#define CYGNUS_GPIO_ASIU_DRV0_CTRL_OFFSET 0x58
> +
> +/* drive strength control for CCM GPIO */
> +#define CYGNUS_GPIO_CCM_DRV0_CTRL_OFFSET  0x00

This stuff (drive strength) is pin control, pin config.
It does not belong in a pure GPIO driver. If you're
making a combined pin control + GPIO driver, it
shall be put in drivers/pinctrl/*

> +#define GPIO_BANK_SIZE 0x200
> +#define NGPIOS_PER_BANK 32
> +#define GPIO_BANK(pin) ((pin) / NGPIOS_PER_BANK)
> +
> +#define CYGNUS_GPIO_REG(pin, reg) (GPIO_BANK(pin) * GPIO_BANK_SIZE + (reg))
> +#define CYGNUS_GPIO_SHIFT(pin) ((pin) % NGPIOS_PER_BANK)
> +
> +#define GPIO_FLAG_BIT_MASK           0xffff
> +#define GPIO_PULL_BIT_SHIFT          16
> +#define GPIO_PULL_BIT_MASK           0x3
> +
> +#define GPIO_DRV_STRENGTH_BIT_SHIFT  20
> +#define GPIO_DRV_STRENGTH_BITS       3
> +#define GPIO_DRV_STRENGTH_BIT_MASK   ((1 << GPIO_DRV_STRENGTH_BITS) - 1)
> +
> +/*
> + * For GPIO internal pull up/down registers
> + */
> +enum gpio_pull {
> +       GPIO_PULL_NONE = 0,
> +       GPIO_PULL_UP,
> +       GPIO_PULL_DOWN,
> +       GPIO_PULL_INVALID,
> +};
> +
> +/*
> + * GPIO drive strength
> + */
> +enum gpio_drv_strength {
> +       GPIO_DRV_STRENGTH_2MA = 0,
> +       GPIO_DRV_STRENGTH_4MA,
> +       GPIO_DRV_STRENGTH_6MA,
> +       GPIO_DRV_STRENGTH_8MA,
> +       GPIO_DRV_STRENGTH_10MA,
> +       GPIO_DRV_STRENGTH_12MA,
> +       GPIO_DRV_STRENGTH_14MA,
> +       GPIO_DRV_STRENGTH_16MA,
> +       GPIO_DRV_STRENGTH_INVALID,
> +};


All this pull up/down and drive strength is pin config for
the pin control subsystem.

> +struct cygnus_gpio {
> +       struct device *dev;
> +       void __iomem *base;
> +       void __iomem *io_ctrl;
> +       spinlock_t lock;
> +       struct gpio_chip gc;
> +       unsigned num_banks;
> +       int irq;
> +       struct irq_domain *irq_domain;

Skip irq and irqdomain and use GPIOLIB_IRQCHIP

> +static u32 cygnus_readl(struct cygnus_gpio *cygnus_gpio, unsigned int offset)
> +{
> +       return readl(cygnus_gpio->base + offset);
> +}
> +
> +static void cygnus_writel(struct cygnus_gpio *cygnus_gpio,
> +                         unsigned int offset, u32 val)
> +{
> +       writel(val, cygnus_gpio->base + offset);
> +}

I don't see the value of using these accessors over just inlining
your readl/writel stuff.

(...)
> +static int cygnus_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> +{
> +       struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
> +
> +       return irq_find_mapping(cygnus_gpio->irq_domain, offset);
> +}

This goes away to the core with GPIOLIB_IRQCHIP

> +static void cygnus_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
> +{
> +       struct cygnus_gpio *cygnus_gpio;
> +       struct irq_chip *chip = irq_desc_get_chip(desc);
> +       int i, bit;
> +
> +       chained_irq_enter(chip, desc);
> +
> +       cygnus_gpio = irq_get_handler_data(irq);
> +
> +       /* go through the entire GPIO banks and handle all interrupts */
> +       for (i = 0; i < cygnus_gpio->num_banks; i++) {
> +               unsigned long val = cygnus_readl(cygnus_gpio,
> +                               (i * GPIO_BANK_SIZE) +
> +                               CYGNUS_GPIO_INT_MSTAT_OFFSET);
> +
> +               for_each_set_bit(bit, &val, NGPIOS_PER_BANK) {
> +                       unsigned pin = NGPIOS_PER_BANK * i + bit;
> +                       int child_irq =
> +                               cygnus_gpio_to_irq(&cygnus_gpio->gc, pin);
> +
> +                       /*
> +                        * Clear the interrupt before invoking the
> +                        * handler, so we do not leave any window
> +                        */
> +                       cygnus_writel(cygnus_gpio, (i * GPIO_BANK_SIZE) +
> +                               CYGNUS_GPIO_INT_CLR_OFFSET, BIT(bit));
> +
> +                       generic_handle_irq(child_irq);
> +               }
> +       }
> +
> +       chained_irq_exit(chip, desc);
> +}

Looks good, but you will need to have the struct gpio_chip * as
handler data to use GPIOLIB_IRQCHIP, so get from there to
the struct cygnus_gpio something like:

struct gpio_chip *gc = irq_desc_get_handler_data(desc);
struct cygnus_gpio *cyg = to_cygnus_gpio(gc);

> +static int cygnus_gpio_get(struct gpio_chip *gc, unsigned gpio)
> +{
> +       struct cygnus_gpio *cygnus_gpio = to_cygnus_gpio(gc);
> +       unsigned int offset = CYGNUS_GPIO_REG(gpio,
> +                       CYGNUS_GPIO_DATA_IN_OFFSET);
> +       unsigned int shift = CYGNUS_GPIO_SHIFT(gpio);
> +       u32 val;
> +
> +       val = cygnus_readl(cygnus_gpio, offset);
> +       val = (val >> shift) & 1;

No, do this:

return !!(cygnus_readl(cygnus_gpio, offset) & BIT(shift));

Maybe rename the "shift" variable to "bit" or just use the macro
directly in the readl().

> +static int cygnus_gpio_irq_map(struct irq_domain *d, unsigned int irq,
> +                              irq_hw_number_t hwirq)
> +{
> +       int ret;
> +
> +       ret = irq_set_chip_data(irq, d->host_data);
> +       if (ret < 0)
> +               return ret;
> +       irq_set_lockdep_class(irq, &gpio_lock_class);
> +       irq_set_chip_and_handler(irq, &cygnus_gpio_irq_chip,
> +                       handle_simple_irq);
> +       set_irq_flags(irq, IRQF_VALID);
> +
> +       return 0;
> +}
> +
> +static void cygnus_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
> +{
> +       irq_set_chip_and_handler(irq, NULL, NULL);
> +       irq_set_chip_data(irq, NULL);
> +}
> +
> +static struct irq_domain_ops cygnus_irq_ops = {
> +       .map = cygnus_gpio_irq_map,
> +       .unmap = cygnus_gpio_irq_unmap,
> +       .xlate = irq_domain_xlate_twocell,
> +};

All this goes away with GPIOLIB_IRQCHIP (that is what is good about it).

> +#ifdef CONFIG_OF_GPIO

What, that should be defined all the time, you depend on it in
Kconfig!

> +static void cygnus_gpio_set_pull(struct cygnus_gpio *cygnus_gpio,
> +                                unsigned gpio, enum gpio_pull pull)
(...)
> +static void cygnus_gpio_set_strength(struct cygnus_gpio *cygnus_gpio,
> +               unsigned gpio, enum gpio_drv_strength strength)
(...)
> +static int cygnus_gpio_of_xlate(struct gpio_chip *gc,
> +               const struct of_phandle_args *gpiospec, u32 *flags)

NAK. This is pin control, put this in the pin control driver.

I guess the same that is part of this patch series.

(...)
> +static int cygnus_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct resource *res;
> +       struct cygnus_gpio *cygnus_gpio;
> +       struct gpio_chip *gc;
> +       u32 i, ngpios;
> +       int ret;
> +
> +       cygnus_gpio = devm_kzalloc(dev, sizeof(*cygnus_gpio), GFP_KERNEL);
> +       if (!cygnus_gpio)
> +               return -ENOMEM;
> +
> +       cygnus_gpio->dev = dev;
> +       platform_set_drvdata(pdev, cygnus_gpio);
> +
> +       if (of_property_read_u32(dev->of_node, "ngpios", &ngpios)) {
> +               dev_err(&pdev->dev, "missing ngpios DT property\n");
> +               return -ENODEV;
> +       }
> +       cygnus_gpio->num_banks = (ngpios + NGPIOS_PER_BANK - 1) /
> +               NGPIOS_PER_BANK;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       cygnus_gpio->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(cygnus_gpio->base)) {
> +               dev_err(&pdev->dev, "unable to map I/O memory\n");
> +               return PTR_ERR(cygnus_gpio->base);
> +       }
> +
> +       /*
> +        * Only certain types of Cygnus GPIO interfaces have I/O control
> +        * registers
> +        */
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       if (res) {
> +               cygnus_gpio->io_ctrl = devm_ioremap_resource(dev, res);
> +               if (IS_ERR(cygnus_gpio->io_ctrl)) {
> +                       dev_err(&pdev->dev, "unable to map I/O memory\n");
> +                       return PTR_ERR(cygnus_gpio->io_ctrl);
> +               }
> +       }

This is a good indication that it's a separate piece of HW and should
be a separate pin control driver.

> +
> +       spin_lock_init(&cygnus_gpio->lock);
> +
> +       gc = &cygnus_gpio->gc;
> +       gc->base = -1;
> +       gc->ngpio = ngpios;
> +       gc->label = dev_name(dev);
> +       gc->dev = dev;
> +#ifdef CONFIG_OF_GPIO

You depend on this symbol.

> +       gc->of_node = dev->of_node;
> +       gc->of_gpio_n_cells = 2;
> +       gc->of_xlate = cygnus_gpio_of_xlate;
> +#endif
> +       gc->direction_input = cygnus_gpio_direction_input;
> +       gc->direction_output = cygnus_gpio_direction_output;
> +       gc->set = cygnus_gpio_set;
> +       gc->get = cygnus_gpio_get;
> +       gc->to_irq = cygnus_gpio_to_irq;
> +
> +       ret = gpiochip_add(gc);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "unable to add GPIO chip\n");
> +               return ret;
> +       }
> +
> +       /*
> +        * Some of the GPIO interfaces do not have interrupt wired to the main
> +        * processor
> +        */
> +       cygnus_gpio->irq = platform_get_irq(pdev, 0);
> +       if (cygnus_gpio->irq < 0) {
> +               ret = cygnus_gpio->irq;
> +               if (ret == -EPROBE_DEFER)
> +                       goto err_rm_gpiochip;
> +
> +               dev_info(&pdev->dev, "no interrupt hook\n");
> +       }

>From here:

> +       cygnus_gpio->irq_domain = irq_domain_add_linear(dev->of_node,
> +                       gc->ngpio, &cygnus_irq_ops, cygnus_gpio);
> +       if (!cygnus_gpio->irq_domain) {
> +               dev_err(&pdev->dev, "unable to allocate IRQ domain\n");
> +               ret = -ENXIO;
> +               goto err_rm_gpiochip;
> +       }
> +
> +       for (i = 0; i < gc->ngpio; i++) {
> +               int irq = irq_create_mapping(cygnus_gpio->irq_domain, i);
> +
> +               irq_set_lockdep_class(irq, &gpio_lock_class);
> +               irq_set_chip_data(irq, cygnus_gpio);
> +               irq_set_chip_and_handler(irq, &cygnus_gpio_irq_chip,
> +                               handle_simple_irq);
> +               set_irq_flags(irq, IRQF_VALID);
> +       }
> +
> +       irq_set_chained_handler(cygnus_gpio->irq, cygnus_gpio_irq_handler);
> +       irq_set_handler_data(cygnus_gpio->irq, cygnus_gpio);

To here, replace with a single call to
gpiochip_set_chained_irqchip(chip *, irq_chip *, irq, handler)...

Look at other drivers using GPIOLIB_IRQCHIP for inspiration.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists