Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support

[Thierry Reding]

On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> <thierry.reding@xxxxxxxxxxxxxxxxx> wrote:
> 
> > This commit adds a driver for the Avionic Design N-bit GPIO expander.
> > The expander provides a variable number of GPIO pins with interrupt
> > support.
> (...)
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > new file mode 100644
> > index 0000000..513a18e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > @@ -0,0 +1,38 @@
> > +Avionic Design N-bit GPIO expander bindings
> > +
> > +Required properties:
> > +- compatible: should be "ad,gpio-adnp"
> > +- reg: The I2C slave address for this device.
> > +- interrupt-parent: phandle of the parent interrupt controller.
> > +- interrupts: Interrupt specifier for the controllers interrupt.
> > +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> > +  second cell is used to specify optional parameters:
> > +  - bit 0: polarity (0: normal, 1: inverted)
> > +- gpio-controller: Marks the device as a GPIO controller
> > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> > +  whereas the second cell is used to specify flags:
> > +    bits[3:0] trigger type and level flags
> > +      1 = low-to-high edge triggered
> > +      2 = high-to-low edge triggered
> > +      4 = active high level-sensitive
> > +      8 = active low level-sensitive
> 
> Why on earth would a bunch of flags be an "interrupt cell"?
> 
> Maybe there is something about DT bindings I don't get so
> please educate me.
> 
> I can see that OMAP is doing this, but is it a good idea?
> I really need Rob/Grant to comment on this.
> 
> > +- interrupt-controller: Marks the device as an interrupt controller.
> > +- nr-gpios: The number of pins supported by the controller.
> 
> These two last things look very generic, like something every GPIO
> driver could want to expose.

As Arnd mentioned, interrupt-controller is a generic property required
by all interrupt controller nodes. Perhaps it shouldn't be listed in the
DT binding for this driver.

As to the nr-gpios property, this is actually not only for informational
purposes, but it also allows the driver to be configured to handle any
number of bits (powers of two). However since this is also a description
of the hardware it may be useful to make this into a generic property
for GPIO controllers.

> I'd really like to have Grant's word on GPIO DT bindings and how these
> should look, I had some discussion with Wolfram (the I2C maintainer)
> about bindings turning out less generic than they ought to be, so we
> need some discussion on this.
> 
> Arnd recently consolidated some MMC props, maybe we need to do
> the same for GPIO drivers.
> 
> (...)
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 502b5ea..d1b0f7d 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> >           Say yes here to enable the adp5588 to be used as an interrupt
> >           controller. It requires the driver to be built in the kernel.
> >
> > +config GPIO_ADNP
> > +       tristate "Avionic Design N-bit GPIO expander"
> > +       depends on I2C && OF
> > +       help
> > +         This option enables support for N GPIOs found on Avionic Design
> > +         I2C GPIO expanders. The register space will be extended by powers
> > +         of two, so the controller will need to accomodate for that. For
> > +         example: if a controller provides 48 pins, 6 registers will be
> > +         enough to represent all pins, but the driver will assume a
> > +         register layout for 64 pins (8 registers).
> > +
> > +config GPIO_ADNP_IRQ
> > +       tristate "Interrupt controller support"
> > +       depends on GPIO_ADNP
> > +       help
> > +         Say yes here to enable the Avionic Design N-bit GPIO expander to
> > +         be used as an interrupt controller.
> 
> First: please describe the usecase where the Avionic driver is used
> without making use of the IRQ, and *why* it should be possible
> to configure this out. E.g. is there a hardware which isn't using the
> IRQ portions? If there is no non-irq usecase just drop this
> config option.

This expander is used in a number of Tegra-based boards and handles
things like enabling or disabling power to a given IC but on other
boards it is also used to handle interrupts from input devices or
card-detect for example.

The controller is synthesized in a CPLD, which is one of the reasons for
the nr-gpios DT property. There is at least one platform that currently
doesn't use the interrupt functionality. Mainly I allowed this to be
configured out in order to reduce the number of interrupts required for
a platform. Another reason was that at the time I first wrote this, IRQ
domains hadn't been available, so the driver couldn't be built as a
module if interrupt support was required. This also no longer applies.

I'm actually fine either way, but I thought it'd be most flexible by
keeping the IRQ support optional for the above reasons.

> If you're keeping it:
> 
> Now this is going to appear as a single line under the ADNP driver
> config entry saying "Interrupt controller support", which is too
> generic.
> 
> Please make it expand to a submenu so that it gets indented below the
> controller driver.
> 
> (...)
> > diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> > new file mode 100644
> > index 0000000..c122ff4
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-adnp.c
> > @@ -0,0 +1,615 @@
> > +/*
> > + * Copyright (C) 2011-2012 Avionic Design GmbH
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqdomain.h>
> 
> Good!
> 
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +
> > +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> > +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> > +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> > +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> > +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> > +
> > +struct adnp {
> > +       struct i2c_client *client;
> > +       struct gpio_chip gpio;
> > +       unsigned int reg_shift;
> > +
> > +       struct mutex i2c_lock;
> > +
> > +       struct irq_domain *domain;
> > +       struct mutex irq_lock;
> > +
> > +       u8 *irq_mask;
> > +       u8 *irq_mask_cur;
> > +       u8 *irq_level;
> > +       u8 *irq_rise;
> > +       u8 *irq_fall;
> > +       u8 *irq_high;
> > +       u8 *irq_low;
> 
> Some or all of this looks like a regmap reimplementation, see below.

Actually none of these represent actual registers, except for irq_mask
and irq_mask_cur. They are used to emulate various IRQ trigger modes.

> 
> > +};
> > +
> > +static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
> 
> I don't know why you name the struct adnp variable "gpio" everywhere,
> that is quite ambigous. Why not name this variable "adnp" consequently
> everywhere?

I can change that, no problem.

> > +{
> > +       int err;
> > +
> > +       err = i2c_smbus_read_byte_data(gpio->client, offset);
> > +       if (err < 0) {
> > +               dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > +                       "i2c_smbus_read_byte_data()", err);
> > +               return err;
> > +       }
> > +
> > +       *value = err;
> > +       return 0;
> > +}
> > +
> > +static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
> > +{
> > +       int err;
> > +
> > +       err = i2c_smbus_write_byte_data(gpio->client, offset, value);
> > +       if (err < 0) {
> > +               dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > +                       "i2c_smbus_write_byte_data()", err);
> > +               return err;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       struct adnp *gpio = container_of(chip, struct adnp, gpio);
> 
> When I do this at several places in a driver I usually define
> a macro like
> 
> #define to_adnp(foo) container_of(foo, struct adnp, gpio)
> 
> Used like so:
> 
> struct adnp *adnp = to_adnp(chip);

Yes, I usually do that as well. I guess this driver has been in the
works in too many variants for too long. =)

> 
> > +       unsigned int reg = offset >> gpio->reg_shift;
> > +       unsigned int pos = offset & 7;
> > +       u8 value;
> > +       int err;
> > +
> > +       mutex_lock(&gpio->i2c_lock);
> 
> The point of taking this mutex here avoids me.
> adnp_read() only performs a single transaction.

I think that's a relic from an earlier version that used to access the
PTR (Pin Tristate Register) as well. At the time I used to return 2 here
to signify a tristated input, which was dependent on the contents of the
PTR. Tristating an output is, I believe, better done using pinmux/
pinctrl nowadays, so I took that code because the only platform where
this was ever used will probably never be mainlined.

On that note, provided there is special additional circuitry, the GPIO
controller is able to detect tristate on an input. I'm not aware that
the pinctrl subsystem provides for that functionality yet, right?

> > +static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > +       struct adnp *gpio = container_of(chip, struct adnp, gpio);
> > +       u8 *base, *ddr, *plr, *ier, *isr, *ptr;
> > +       unsigned int regs, i, j;
> > +       int err;
> > +
> > +       regs = 1 << gpio->reg_shift;
> > +
> > +       base = kzalloc(regs * 5, GFP_KERNEL);
> 
> Why kzalloc()/kfree() when you can just use a
> 
> static u8 base[N];
> 
> where N is the max number of registers on any HW instead?

As I explained above, the number of pins is configurable, so it'd be
quite a waste to always assume a maximum of, say, 256 pins if the
hardware actually only uses 8.

> > +       if (!base)
> > +               return;
> > +
> > +       ddr = base + (regs * 0);
> > +       plr = base + (regs * 1);
> > +       ier = base + (regs * 2);
> > +       isr = base + (regs * 3);
> > +       ptr = base + (regs * 4);
> > +
> > +       for (i = 0; i < regs; i++) {
> > +               err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
> > +               if (err < 0)
> > +                       goto out;
> > +
> > +               err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
> > +               if (err < 0)
> > +                       goto out;
> > +
> > +               err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
> > +               if (err < 0)
> > +                       goto out;
> > +
> > +               err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
> > +               if (err < 0)
> > +                       goto out;
> > +
> > +               err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
> > +               if (err < 0)
> > +                       goto out;
> > +       }
> 
> You must take the mutex around this for-loop, don't you?
> 
> Else browing debugfs will sporadically corrupt transfers,
> or something like that. Or was this intended to cut into any
> ongoing transfer couple?

No, you're right. This should be protected by the mutex.

> > +       for (i = 0; i < regs; i++) {
> > +               for (j = 0; j < 8; j++) {
> > +                       unsigned int bit = (i << gpio->reg_shift) + j;
> > +                       const char *direction = "input ";
> > +                       const char *level = "low ";
> > +                       const char *interrupt = "disabled";
> > +                       const char *pending = "";
> > +
> > +                       if (ddr[i] & BIT(j))
> > +                               direction = "output";
> > +
> > +                       if (plr[i] & BIT(j))
> > +                               level = "high";
> > +
> > +                       if (ier[i] & BIT(j))
> > +                               interrupt = "enabled ";
> > +
> > +                       if (isr[i] & BIT(j))
> > +                               pending = "pending";
> > +
> > +                       seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
> > +                                  direction, level, interrupt, pending);
> > +               }
> > +       }
> > +
> > +out:
> > +       kfree(base);
> > +}
> > +
> > +static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
> > +{
> > +       struct gpio_chip *chip = &gpio->gpio;
> > +
> > +       gpio->reg_shift = get_count_order(num_gpios) - 3;
> > +
> > +       chip->direction_input = adnp_gpio_direction_input;
> > +       chip->direction_output = adnp_gpio_direction_output;
> > +       chip->get = adnp_gpio_get;
> > +       chip->set = adnp_gpio_set;
> > +       chip->can_sleep = 1;
> > +       if (IS_ENABLED(CONFIG_DEBUG_FS))
> > +               chip->dbg_show = adnp_gpio_dbg_show;
> > +
> > +       chip->base = -1;
> > +       chip->ngpio = num_gpios;
> > +       chip->label = gpio->client->name;
> > +       chip->dev = &gpio->client->dev;
> > +       chip->of_node = chip->dev->of_node;
> > +       chip->owner = THIS_MODULE;
> 
> Usually we define the struct gpio_chip as a static const and have
> the state containter hold a static const struct gpio_chip *chip;
> entry assigned to point to the static const at probe time.

The problem with a static const is that you can no longer configure the
number of GPIOs at runtime, which is sort of essential for this driver.

> All other GPIO drivers #ifdef their debugfs code, this probably
> works fine too, but never saw it before.
> 
> I'm ambivalent about this, I sort of like it because it avoids
> #ifdefs and also makes sure the code is always compiled,
> so if you like it like this, keep it.

I've started to use this wherever possible because otherwise you have to
build numerous configurations to ensure complete compile coverage. Then
again this also has the drawback to potentially hide issues if you don't
properly separate conditionalized code.

> (...)
> > +static void adnp_irq_update_mask(struct adnp *gpio)
> > +{
> > +       unsigned int regs = 1 << gpio->reg_shift;
> > +       bool equal = true;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < regs; i++) {
> > +               if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
> > +                       equal = false;
> > +                       break;
> > +               }
> > +       }
> 
> This is not looking good. It looks like you're reimplementing
> parts of regmap.
> 
> In that case, please use <linux/regmap.h> to cache registers
> instead.
> 
> But I'm not sure ...
> 
> (...)
> > +static void adnp_irq_bus_lock(struct irq_data *data)
> > +{
> > +       struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > +       unsigned int regs = 1 << gpio->reg_shift;
> > +
> > +       mutex_lock(&gpio->irq_lock);
> > +       memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
> > +}
> > +
> > +static void adnp_irq_bus_unlock(struct irq_data *data)
> > +{
> > +       struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > +
> > +       adnp_irq_update_mask(gpio);
> > +       mutex_unlock(&gpio->irq_lock);
> > +}
> 
> Actually I'm not following why the IRQ mask registers are shadowed
> at bus_lock and restored at bus_unlock().
> 
> A comment describing why that's needed and how it works is
> definately needed.

I'm not sure that this is required anymore. IIRC I did copy this from
some other driver at the time. This is probably supposed to be an
optimization, but I think I can live without it.

> 
> (...)
> > +static const struct irq_domain_ops adnp_irq_domain_ops = {
> > +       .map = adnp_irq_map,
> > +       .xlate = irq_domain_xlate_twocell,
> > +};
> > +
> > +static int adnp_irq_setup(struct adnp *gpio)
> > +{
> > +       unsigned int regs = 1 << gpio->reg_shift, i;
> > +       struct gpio_chip *chip = &gpio->gpio;
> > +       int err;
> > +
> > +       mutex_init(&gpio->irq_lock);
> > +
> > +       gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> > +       if (!gpio->irq_mask)
> > +               return -ENOMEM;
> > +
> > +       gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> > +       gpio->irq_level = gpio->irq_mask + (regs * 2);
> > +       gpio->irq_rise = gpio->irq_mask + (regs * 3);
> > +       gpio->irq_fall = gpio->irq_mask + (regs * 4);
> > +       gpio->irq_high = gpio->irq_mask + (regs * 5);
> > +       gpio->irq_low = gpio->irq_mask + (regs * 6);
> 
> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
> here? Explain with some comment atleast.

Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
irq_low to keep track of the current level and trigger modes for each
interrupt. Instead of allocating small chunks for each of these I
allocate one chunk and just make the others point into that.

> > +
> > +       for (i = 0; i < regs; i++) {
> > +               err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
> > +               if (err < 0)
> > +                       return err;
> > +       }
> 
> Looks like regmap reimplementation.

This is used to obtain the initial pin levels, which in turn is required
to check for rising and falling edges.

> > +       gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
> > +                                            &adnp_irq_domain_ops, gpio);
> > +
> > +       err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
> > +                                  IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +                                  dev_name(chip->dev), gpio);
> 
> Since you're using devm_* above use it here too:
> devm_request_threaded_irq().
> 
> > +       if (err != 0) {
> > +               dev_err(chip->dev, "can't request IRQ#%d: %d\n",
> > +                       gpio->client->irq, err);
> > +               goto error;
> > +       }
> > +
> > +       gpio->gpio.to_irq = adnp_gpio_to_irq;
> > +       return 0;
> > +
> > +error:
> > +       irq_domain_remove(gpio->domain);
> > +       return err;
> > +}
> > +
> > +static void adnp_irq_teardown(struct adnp *gpio)
> > +{
> > +       unsigned int irq, i;
> > +
> > +       free_irq(gpio->client->irq, gpio);
> 
> If you're using devm to grab the IRQ this is not needed.

I don't think that'll work. In this case the interrupt needs to be freed
before cleaning up, because otherwise the interrupt handler may still be
run after the IRQ domain has already been removed.

> 
> > +
> > +       for (i = 0; i < gpio->gpio.ngpio; i++) {
> > +               irq = irq_find_mapping(gpio->domain, i);
> > +               if (irq > 0)
> > +                       irq_dispose_mapping(irq);
> > +       }
> > +
> > +       irq_domain_remove(gpio->domain);
> > +}
> > +
> > +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> > +                                   const struct i2c_device_id *id)
> > +{
> > +       struct adnp *gpio;
> > +       u32 num_gpios;
> > +       int err;
> > +
> > +       err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> > +                                  &num_gpios);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> > +       if (client->irq == NO_IRQ)
> 
> Just if (!client->irq) since NO_IRQ is 0 nowadays.

Okay, will do.

> 
> > +               return -EPROBE_DEFER;
> 
> Why would you defer in this case? If the IRQ controller appear later
> than the GPIO driver?

Yes. In particular when using DT it can happen that the parent interrupt
controller is probed later than this.

> > +       gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> > +       if (!gpio)
> > +               return -ENOMEM;
> > +
> > +       mutex_init(&gpio->i2c_lock);
> > +       gpio->client = client;
> > +
> > +       err = adnp_gpio_setup(gpio, num_gpios);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
> > +               err = adnp_irq_setup(gpio);
> > +               if (err < 0)
> > +                       goto teardown;
> > +       }
> 
> And that activates the question why this should be conditional,
> please elaborate.

I think I've answered this before.

> > +       err = gpiochip_add(&gpio->gpio);
> > +       if (err < 0)
> > +               goto teardown;
> > +
> > +       i2c_set_clientdata(client, gpio);
> > +       return 0;
> > +
> > +teardown:
> > +       if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
> > +               adnp_irq_teardown(gpio);
> 
> Here too.
> 
> > +
> > +       return err;
> > +}
> 
> Most of the driver looks very good though! The code is
> clean and easy to read and maintain.
> 
> But we need to iron out the details.

Thanks for reviewing. I'll fixup the problems you've pointed out and
will have to retest.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature