[PATCH 2/2] gpio: Add Avionic Design N-bit GPIO expander support

[Thierry Reding]

* Grant Likely wrote:
> On Thu, 12 Apr 2012 13:24:18 +0200, Thierry Reding <thierry.reding at avionic-design.de> 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.
> 
> Hi Thierry,  comments below...
> 
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 7875c3f..d86c3b7 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -373,6 +373,25 @@ 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
> > +	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
> > +	bool "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. It requires the driver to be
> > +	  built in the kernel.
> > +
> 
> Why does it require the driver to be built in?

That's primarily a relic from pre-IRQ domain days when it wasn't possible to
register interrupt controllers after the boot phase. While this should now no
longer be a problem, I still haven't been able to find out how to properly
clean up the IRQ domain when the module is unloaded. I feel that the cleanup
path should be properly implemented before allowing this to be built as a
module.

Can you enlighten me?

> > +struct adnp {
> > +	struct i2c_client *client;
> > +	struct gpio_chip gpio;
> > +	unsigned int reg_shift;
> > +
> > +	struct mutex i2c_lock;
> > +
> > +#ifdef CONFIG_GPIO_ADNP_IRQ
> > +	struct irq_domain *domain;
> > +	struct mutex irq_lock;
> > +
> > +	unsigned int irq_base;
> > +	unsigned int nrirq;
> 
> This driver should use the irq_domain linear mapping which makes
> irq_base unnecessary.

Okay, I'll have a look.

> > +static int adnp_gpio_setup(struct adnp *gpio, struct adnp_platform_data *pdata)
> > +{
> > +	struct gpio_chip *chip = &gpio->gpio;
> > +
> > +	gpio->reg_shift = get_count_order(pdata->nr_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->dbg_show = adnp_gpio_dbg_show;
> > +	chip->can_sleep = 1;
> > +
> > +	chip->base = pdata->gpio_base;
> > +	chip->ngpio = pdata->nr_gpios;
> > +	chip->label = gpio->client->name;
> > +	chip->dev = &gpio->client->dev;
> > +	chip->owner = THIS_MODULE;
> > +	chip->names = pdata->names;
> > +
> > +#ifdef CONFIG_OF_GPIO
> > +	chip->of_node = chip->dev->of_node;
> > +#endif
> 
> Drop the #ifdef protection.  dev->of_node always exists so this
> assignment is safe.

I'll do that.

> > +static const struct i2c_device_id adnp_i2c_id[] = {
> > +	{ "gpio-adnp" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, adnp_i2c_id);
> 
> Should use an of_device_id table since this is a DT-enabled
> driver.

Depending on the discussion below, should it be using both an I2C and an OF
table in case it will be used in a non-DT setup.

> > +
> > +static struct i2c_driver adnp_i2c_driver = {
> > +	.driver = {
> > +		.name = "gpio-adnp",
> > +		.owner = THIS_MODULE,
> > +	},
> > +	.probe = adnp_i2c_probe,
> > +	.remove = __devexit_p(adnp_i2c_remove),
> > +	.id_table = adnp_i2c_id,
> > +};
> > +module_i2c_driver(adnp_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("Avionic Design N-bit GPIO expander");
> > +MODULE_AUTHOR("Thierry Reding <thierry.reding at avionic-design.de>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/i2c/gpio-adnp.h b/include/linux/i2c/gpio-adnp.h
> > new file mode 100644
> > index 0000000..6e077a3
> > --- /dev/null
> > +++ b/include/linux/i2c/gpio-adnp.h
> > @@ -0,0 +1,19 @@
> > +/*
> > + * 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.
> > + */
> > +
> > +#ifndef _LINUX_I2C_ADNP_H
> > +#define _LINUX_I2C_ADNP_H 1
> > +
> > +struct adnp_platform_data {
> > +	unsigned gpio_base;
> > +	unsigned nr_gpios;
> > +	int irq_base;
> > +	const char *const *names;
> > +};
> 
> From the start this is a DT enabled driver.  There should be no reason
> to also include platform_data support.

I thought it might be a good idea to support both for now. There have been
some discussions that the device tree should be just an alternative source
for platform data, the main argument being that platform data could be used
to overwrite data provided via DT.

Also I use this driver on some older kernels that can't boot from DT yet, but
I guess that argument isn't very valid because those kernels don't have IRQ
domain support either and the driver needs more changes anyway.

Is the official position to introduce new drivers as DT only?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/devicetree-discuss/attachments/20120521/0fa619c4/attachment-0001.sig>