[RFC/RFT 0/2] gpio: move tnetv107x gpio support to drivers/gpio

[RFC/RFT 0/2] gpio: move tnetv107x gpio support to drivers/gpio

[Sekhar Nori]

This patch series moves the tnetv107x GPIO support from
arch/arm/mach-davinci/ to drivers/gpio.

I do not have the tnetv107x EVM hardware so I have not been
able to test this. Any Tested-bys or Acked-bys are welcome
from those who have the hardware.

The patches apply to latest of gpio/next on Grant's tree.

Sekhar Nori (2):
  gpio/basic_mmio: add support for enable register
  davinci: use generic memory mapped gpio for tnetv107x

 arch/arm/mach-davinci/Kconfig             |    1 +
 arch/arm/mach-davinci/Makefile            |    1 -
 arch/arm/mach-davinci/devices-tnetv107x.c |   68 ++++++++++
 arch/arm/mach-davinci/gpio-tnetv107x.c    |  205 -----------------------------
 arch/arm/mach-davinci/tnetv107x.c         |    3 -
 drivers/gpio/gpio-ep93xx.c                |    2 +-
 drivers/gpio/gpio-generic.c               |   45 ++++++-
 drivers/gpio/gpio-mxc.c                   |    2 +-
 drivers/gpio/gpio-mxs.c                   |    2 +-
 include/linux/basic_mmio_gpio.h           |    5 +
 10 files changed, 121 insertions(+), 213 deletions(-)
 delete mode 100644 arch/arm/mach-davinci/gpio-tnetv107x.c

-- 
1.7.3.2

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Ryan Mallon]

On 05/07/11 15:10, Sekhar Nori wrote:
> Some GPIO controllers have an enable register
> which needs to be written to before a GPIO
> can be used.
>
> Add support for enabling the GPIO. At this
> time inverted logic for enabling the GPIO
> is not supported. This can be done by adding
> a disable register as and when a controller
> with this comes along.
>
> Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> ---
>
<snip>

> static int bgpio_setup_io(struct bgpio_chip *bgc,
>   			  void __iomem *dat,
> @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
>   			 void __iomem *clr,
>   			 void __iomem *dirout,
>   			 void __iomem *dirin,
> +			 void __iomem *en,
>   			 bool big_endian)

The arguments to this function are getting a bit unwieldy :-). Maybe we 
need to introduce something like:

struct bgpio_chip_info {
     struct device *dev;
     unsigned long sz;
     void __iomem *dat;
     void __iomem *set;
     void __iomem *clr;
     void __iomem *dirout;
     void __iomem *dirin;
     void __iomem *en;
     bool big_endian;
};

and pass that to bgpio_init instead. It would have the added benefits of 
making the drivers more readable and that bgpio_chip_info structs in the 
drivers can probably be marked __initdata also.

Since you are already having to touch all of the call sites for 
bgpio_init this could be done as a separate patch along with this series.

~Ryan

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Nori, Sekhar]

Hi Ryan,

On Tue, Jul 05, 2011 at 11:45:44, Ryan Mallon wrote:
> On 05/07/11 15:10, Sekhar Nori wrote:
> > Some GPIO controllers have an enable register
> > which needs to be written to before a GPIO
> > can be used.
> >
> > Add support for enabling the GPIO. At this
> > time inverted logic for enabling the GPIO
> > is not supported. This can be done by adding
> > a disable register as and when a controller
> > with this comes along.
> >
> > Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> > ---
> >
> <snip>
> 
> > static int bgpio_setup_io(struct bgpio_chip *bgc,
> >   			  void __iomem *dat,
> > @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> >   			 void __iomem *clr,
> >   			 void __iomem *dirout,
> >   			 void __iomem *dirin,
> > +			 void __iomem *en,
> >   			 bool big_endian)
> 
> The arguments to this function are getting a bit unwieldy :-). Maybe we 
> need to introduce something like:
> 
> struct bgpio_chip_info {
>      struct device *dev;
>      unsigned long sz;
>      void __iomem *dat;
>      void __iomem *set;
>      void __iomem *clr;
>      void __iomem *dirout;
>      void __iomem *dirin;
>      void __iomem *en;
>      bool big_endian;
> };
> 
> and pass that to bgpio_init instead. It would have the added benefits of 
> making the drivers more readable and that bgpio_chip_info structs in the 
> drivers can probably be marked __initdata also.
> 
> Since you are already having to touch all of the call sites for 
> bgpio_init this could be done as a separate patch along with this series.

Agreed. I can do this if Grant too thinks this is a
pre-requisite to adding new functionality.

Thanks,
Sekhar

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Grant Likely]

On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote:
> On 05/07/11 15:10, Sekhar Nori wrote:
> >Some GPIO controllers have an enable register
> >which needs to be written to before a GPIO
> >can be used.
> >
> >Add support for enabling the GPIO. At this
> >time inverted logic for enabling the GPIO
> >is not supported. This can be done by adding
> >a disable register as and when a controller
> >with this comes along.
> >
> >Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> >---
> >
> <snip>
> 
> >static int bgpio_setup_io(struct bgpio_chip *bgc,
> >  			  void __iomem *dat,
> >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> >  			 void __iomem *clr,
> >  			 void __iomem *dirout,
> >  			 void __iomem *dirin,
> >+			 void __iomem *en,
> >  			 bool big_endian)
> 
> The arguments to this function are getting a bit unwieldy :-). Maybe
> we need to introduce something like:
> 
> struct bgpio_chip_info {
>     struct device *dev;
>     unsigned long sz;
>     void __iomem *dat;
>     void __iomem *set;
>     void __iomem *clr;
>     void __iomem *dirout;
>     void __iomem *dirin;
>     void __iomem *en;
>     bool big_endian;
> };
> 
> and pass that to bgpio_init instead. It would have the added
> benefits of making the drivers more readable and that
> bgpio_chip_info structs in the drivers can probably be marked
> __initdata also.

Or, what may be better is to make callers directly update the
bgpio_chip structure.

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Nori, Sekhar]

Hi Grant,

On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote:
> On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote:
> > On 05/07/11 15:10, Sekhar Nori wrote:
> > >Some GPIO controllers have an enable register
> > >which needs to be written to before a GPIO
> > >can be used.
> > >
> > >Add support for enabling the GPIO. At this
> > >time inverted logic for enabling the GPIO
> > >is not supported. This can be done by adding
> > >a disable register as and when a controller
> > >with this comes along.
> > >
> > >Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> > >---
> > >
> > <snip>
> > 
> > >static int bgpio_setup_io(struct bgpio_chip *bgc,
> > >  			  void __iomem *dat,
> > >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> > >  			 void __iomem *clr,
> > >  			 void __iomem *dirout,
> > >  			 void __iomem *dirin,
> > >+			 void __iomem *en,
> > >  			 bool big_endian)
> > 
> > The arguments to this function are getting a bit unwieldy :-). Maybe
> > we need to introduce something like:
> > 
> > struct bgpio_chip_info {
> >     struct device *dev;
> >     unsigned long sz;
> >     void __iomem *dat;
> >     void __iomem *set;
> >     void __iomem *clr;
> >     void __iomem *dirout;
> >     void __iomem *dirin;
> >     void __iomem *en;
> >     bool big_endian;
> > };
> > 
> > and pass that to bgpio_init instead. It would have the added
> > benefits of making the drivers more readable and that
> > bgpio_chip_info structs in the drivers can probably be marked
> > __initdata also.
> 
> Or, what may be better is to make callers directly update the
> bgpio_chip structure.

I started implementing it this way, but felt that the bgpio_chip
structure also has many internal members (like the spinlock) and
this method will require users to spend quite a bit of time figuring
out which structure members they should initialize and which to leave
for bgpio_init() to do for them.

There is also the case of direction register which is set from
either dirout or dirin depending upon whether the logic is inverted
or not. The bgpio_chip however needs to deal with a single direction
register offset.

Considering these, I am getting inclined towards Ryan's suggestion.

Thoughts?

Thanks,
Sekhar

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Grant Likely]

On Thu, Jul 07, 2011 at 10:15:31PM +0530, Nori, Sekhar wrote:
> Hi Grant,
> 
> On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote:
> > On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote:
> > > On 05/07/11 15:10, Sekhar Nori wrote:
> > > >Some GPIO controllers have an enable register
> > > >which needs to be written to before a GPIO
> > > >can be used.
> > > >
> > > >Add support for enabling the GPIO. At this
> > > >time inverted logic for enabling the GPIO
> > > >is not supported. This can be done by adding
> > > >a disable register as and when a controller
> > > >with this comes along.
> > > >
> > > >Signed-off-by: Sekhar Nori<nsekhar@ti.com>
> > > >---
> > > >
> > > <snip>
> > > 
> > > >static int bgpio_setup_io(struct bgpio_chip *bgc,
> > > >  			  void __iomem *dat,
> > > >@@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
> > > >  			 void __iomem *clr,
> > > >  			 void __iomem *dirout,
> > > >  			 void __iomem *dirin,
> > > >+			 void __iomem *en,
> > > >  			 bool big_endian)
> > > 
> > > The arguments to this function are getting a bit unwieldy :-). Maybe
> > > we need to introduce something like:
> > > 
> > > struct bgpio_chip_info {
> > >     struct device *dev;
> > >     unsigned long sz;
> > >     void __iomem *dat;
> > >     void __iomem *set;
> > >     void __iomem *clr;
> > >     void __iomem *dirout;
> > >     void __iomem *dirin;
> > >     void __iomem *en;
> > >     bool big_endian;
> > > };
> > > 
> > > and pass that to bgpio_init instead. It would have the added
> > > benefits of making the drivers more readable and that
> > > bgpio_chip_info structs in the drivers can probably be marked
> > > __initdata also.
> > 
> > Or, what may be better is to make callers directly update the
> > bgpio_chip structure.
> 
> I started implementing it this way, but felt that the bgpio_chip
> structure also has many internal members (like the spinlock) and
> this method will require users to spend quite a bit of time figuring
> out which structure members they should initialize and which to leave
> for bgpio_init() to do for them.
> 
> There is also the case of direction register which is set from
> either dirout or dirin depending upon whether the logic is inverted
> or not. The bgpio_chip however needs to deal with a single direction
> register offset.

We *absolutely* still need the helper function for the complex
settings, but for the non-complex ones, I'd rather just directly
access the structure.  The kerneldoc documentation of the structure
can and should be explicit about what the caller is allowed to do.

g.

[RFC/RFT 1/2] gpio/basic_mmio: add support for enable register

[Ryan Mallon]

On 08/07/11 04:37, Grant Likely wrote:
> On Thu, Jul 07, 2011 at 10:15:31PM +0530, Nori, Sekhar wrote:
>> Hi Grant,
>>
>> On Thu, Jul 07, 2011 at 02:40:54, Grant Likely wrote:
>>> On Tue, Jul 05, 2011 at 04:15:44PM +1000, Ryan Mallon wrote:
>>>> On 05/07/11 15:10, Sekhar Nori wrote:
>>>>> Some GPIO controllers have an enable register
>>>>> which needs to be written to before a GPIO
>>>>> can be used.
>>>>>
>>>>> Add support for enabling the GPIO. At this
>>>>> time inverted logic for enabling the GPIO
>>>>> is not supported. This can be done by adding
>>>>> a disable register as and when a controller
>>>>> with this comes along.
>>>>>
>>>>> Signed-off-by: Sekhar Nori<nsekhar@ti.com>
>>>>> ---
>>>>>
>>>> <snip>
>>>>
>>>>> static int bgpio_setup_io(struct bgpio_chip *bgc,
>>>>>  			  void __iomem *dat,
>>>>> @@ -369,6 +401,7 @@ int __devinit bgpio_init(struct bgpio_chip *bgc,
>>>>>  			 void __iomem *clr,
>>>>>  			 void __iomem *dirout,
>>>>>  			 void __iomem *dirin,
>>>>> +			 void __iomem *en,
>>>>>  			 bool big_endian)
>>>> The arguments to this function are getting a bit unwieldy :-). Maybe
>>>> we need to introduce something like:
>>>>
>>>> struct bgpio_chip_info {
>>>>     struct device *dev;
>>>>     unsigned long sz;
>>>>     void __iomem *dat;
>>>>     void __iomem *set;
>>>>     void __iomem *clr;
>>>>     void __iomem *dirout;
>>>>     void __iomem *dirin;
>>>>     void __iomem *en;
>>>>     bool big_endian;
>>>> };
>>>>
>>>> and pass that to bgpio_init instead. It would have the added
>>>> benefits of making the drivers more readable and that
>>>> bgpio_chip_info structs in the drivers can probably be marked
>>>> __initdata also.
>>> Or, what may be better is to make callers directly update the
>>> bgpio_chip structure.
>> I started implementing it this way, but felt that the bgpio_chip
>> structure also has many internal members (like the spinlock) and
>> this method will require users to spend quite a bit of time figuring
>> out which structure members they should initialize and which to leave
>> for bgpio_init() to do for them.
>>
>> There is also the case of direction register which is set from
>> either dirout or dirin depending upon whether the logic is inverted
>> or not. The bgpio_chip however needs to deal with a single direction
>> register offset.
> We *absolutely* still need the helper function for the complex
> settings, but for the non-complex ones, I'd rather just directly
> access the structure.  The kerneldoc documentation of the structure
> can and should be explicit about what the caller is allowed to do.
>
You could pull out all of the user accessible parts the bgpio_chip
structure in to a structure called bgpio_chip_info (or whatever) that
the drivers fill in and pass to bgpio_init, which then gets assigned as
a member of bgpio_chip.

Not sure what you mean about the helper function. If bgpio_init takes a
structure with all of the information about the particular chip then
initialisation of either a simple or complex gpio chip is done using the
same function. Only the number of fields in the bgpio_chip_info
structure which need to be filled in should change.

~Ryan

[RFC/RFT 2/2] davinci: use generic memory mapped gpio for tnetv107x

[Grant Likely]

On Tue, Jul 05, 2011 at 10:41:00AM +0530, Sekhar Nori wrote:
> The GPIO controller on TNETV107x SoC can use
> the generic memory mapped GPIO driver.
> 
> Shift to the generic driver instead of the
> private implementation.
> 
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>  arch/arm/mach-davinci/Kconfig             |    1 +
>  arch/arm/mach-davinci/Makefile            |    1 -
>  arch/arm/mach-davinci/devices-tnetv107x.c |   68 ++++++++++
>  arch/arm/mach-davinci/gpio-tnetv107x.c    |  205 -----------------------------
>  arch/arm/mach-davinci/tnetv107x.c         |    3 -
>  5 files changed, 69 insertions(+), 209 deletions(-)
>  delete mode 100644 arch/arm/mach-davinci/gpio-tnetv107x.c
> 
> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig
> index c0deaca..ec0c6d3 100644
> --- a/arch/arm/mach-davinci/Kconfig
> +++ b/arch/arm/mach-davinci/Kconfig
> @@ -53,6 +53,7 @@ config ARCH_DAVINCI_DM365
>  config ARCH_DAVINCI_TNETV107X
>  	select CPU_V6
>  	select CP_INTC
> +	select GPIO_GENERIC_PLATFORM
>  	bool "TNETV107X based system"
>  
>  comment "DaVinci Board Type"
> diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach-davinci/Makefile
> index 0b87a1c..40557c8 100644
> --- a/arch/arm/mach-davinci/Makefile
> +++ b/arch/arm/mach-davinci/Makefile
> @@ -17,7 +17,6 @@ obj-$(CONFIG_ARCH_DAVINCI_DM365)	+= dm365.o devices.o
>  obj-$(CONFIG_ARCH_DAVINCI_DA830)        += da830.o devices-da8xx.o
>  obj-$(CONFIG_ARCH_DAVINCI_DA850)        += da850.o devices-da8xx.o
>  obj-$(CONFIG_ARCH_DAVINCI_TNETV107X)    += tnetv107x.o devices-tnetv107x.o
> -obj-$(CONFIG_ARCH_DAVINCI_TNETV107X)    += gpio-tnetv107x.o
>  
>  obj-$(CONFIG_AINTC)			+= irq.o
>  obj-$(CONFIG_CP_INTC)			+= cp_intc.o
> diff --git a/arch/arm/mach-davinci/devices-tnetv107x.c b/arch/arm/mach-davinci/devices-tnetv107x.c
> index 6162cae..eab43b1 100644
> --- a/arch/arm/mach-davinci/devices-tnetv107x.c
> +++ b/arch/arm/mach-davinci/devices-tnetv107x.c
> @@ -18,6 +18,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/clk.h>
>  #include <linux/slab.h>
> +#include <linux/basic_mmio_gpio.h>
>  
>  #include <mach/common.h>
>  #include <mach/irqs.h>
> @@ -31,6 +32,7 @@
>  #define TNETV107X_TPTC0_BASE			0x01c10000
>  #define TNETV107X_TPTC1_BASE			0x01c10400
>  #define TNETV107X_WDOG_BASE			0x08086700
> +#define TNETV107X_GPIO_BASE			0x08088000
>  #define TNETV107X_TSC_BASE			0x08088500
>  #define TNETV107X_SDIO0_BASE			0x08088700
>  #define TNETV107X_SDIO1_BASE			0x08088800
> @@ -362,11 +364,77 @@ static struct platform_device ssp_device = {
>  	.resource	= ssp_resources,
>  };
>  
> +#define TNETV107X_N_GPIO_DEV	DIV_ROUND_UP(TNETV107X_N_GPIO, 32)
> +
> +static struct resource gpio_resources[TNETV107X_N_GPIO_DEV][4] = {
> +	[0 ... TNETV107X_N_GPIO_DEV - 1] = {

If all the data is identical, why does this need to be an array?

> +		{
> +			.name	= "dat",
> +			.start	= TNETV107X_GPIO_BASE + 0x4,
> +			.end	= TNETV107X_GPIO_BASE + 0x4 + 0x4 - 1,
> +		},
> +		{
> +			.name	= "set",
> +			.start	= TNETV107X_GPIO_BASE + 0x10,
> +			.end	= TNETV107X_GPIO_BASE + 0x10 + 0x4 - 1,
> +		},
> +		{
> +			.name	= "dirin",
> +			.start	= TNETV107X_GPIO_BASE + 0x1c,
> +			.end	= TNETV107X_GPIO_BASE + 0x1c + 0x4 - 1,
> +		},
> +		{
> +			.name	= "en",
> +			.start	= TNETV107X_GPIO_BASE + 0x28,
> +			.end	= TNETV107X_GPIO_BASE + 0x28 + 0x4 - 1,
> +		},
> +	},
> +};

Wow, this ends up looking horrible. (yes, I know it is not your
fault).  I backed off earlier on using resources for the offsets, but
I want to change my mind again and make interface a register range +
offsets to the control registers.

> +
> +static struct platform_device gpio_device[] = {
> +	[0 ... TNETV107X_N_GPIO_DEV - 1] = {
> +		.name		= "basic-mmio-gpio",
> +		.num_resources	= 4,
> +	},
> +};
> +
> +static struct bgpio_pdata gpio_pdata[TNETV107X_N_GPIO_DEV];
> +
> +static void __init tnetv107x_gpio_init(void)
> +{
> +	int i, j;
> +
> +	for (i = 1; i < TNETV107X_N_GPIO_DEV; i++) {
> +		for (j = 0; j < 4; j++) {
> +			gpio_resources[i][j].start += 0x4 * i;
> +			gpio_resources[i][j].end += 0x4 * i;
> +		}
> +	}
> +
> +	for (i = 0; i < TNETV107X_N_GPIO_DEV; i++) {
> +		int base = i * 32;
> +
> +		gpio_device[i].id = i;
> +		gpio_device[i].resource = gpio_resources[i];
> +
> +		gpio_pdata[i].base = base;
> +		gpio_pdata[i].ngpio = TNETV107X_N_GPIO - base;

?  This doesn't look right.  Shouldn't ngpio be the same for each gpio
controller instance?

> +		if (gpio_pdata[i].ngpio > 32)
> +			gpio_pdata[i].ngpio = 32;
> +
> +		gpio_device[i].dev.platform_data = &gpio_pdata[i];
> +
> +		platform_device_register(&gpio_device[i]);
> +	}
> +}
> +
>  void __init tnetv107x_devices_init(struct tnetv107x_device_info *info)
>  {
>  	int i, error;
>  	struct clk *tsc_clk;
>  
> +	tnetv107x_gpio_init();
> +
>  	/*
>  	 * The reset defaults for tnetv107x tsc clock divider is set too high.
>  	 * This forces the clock down to a range that allows the ADC to
> diff --git a/arch/arm/mach-davinci/gpio-tnetv107x.c b/arch/arm/mach-davinci/gpio-tnetv107x.c
> deleted file mode 100644
> index 3fa3e28..0000000
> --- a/arch/arm/mach-davinci/gpio-tnetv107x.c
> +++ /dev/null
> @@ -1,205 +0,0 @@
> -/*
> - * Texas Instruments TNETV107X GPIO Controller
> - *
> - * Copyright (C) 2010 Texas Instruments
> - *
> - * 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/init.h>
> -#include <linux/gpio.h>
> -
> -#include <mach/common.h>
> -#include <mach/tnetv107x.h>
> -
> -struct tnetv107x_gpio_regs {
> -	u32	idver;
> -	u32	data_in[3];
> -	u32	data_out[3];
> -	u32	direction[3];
> -	u32	enable[3];
> -};
> -
> -#define gpio_reg_index(gpio)	((gpio) >> 5)
> -#define gpio_reg_bit(gpio)	BIT((gpio) & 0x1f)
> -
> -#define gpio_reg_rmw(reg, mask, val)	\
> -	__raw_writel((__raw_readl(reg) & ~(mask)) | (val), (reg))
> -
> -#define gpio_reg_set_bit(reg, gpio)	\
> -	gpio_reg_rmw((reg) + gpio_reg_index(gpio), 0, gpio_reg_bit(gpio))
> -
> -#define gpio_reg_clear_bit(reg, gpio)	\
> -	gpio_reg_rmw((reg) + gpio_reg_index(gpio), gpio_reg_bit(gpio), 0)
> -
> -#define gpio_reg_get_bit(reg, gpio)	\
> -	(__raw_readl((reg) + gpio_reg_index(gpio)) & gpio_reg_bit(gpio))
> -
> -#define chip2controller(chip)		\
> -	container_of(chip, struct davinci_gpio_controller, chip)
> -
> -#define TNETV107X_GPIO_CTLRS	DIV_ROUND_UP(TNETV107X_N_GPIO, 32)
> -
> -static struct davinci_gpio_controller chips[TNETV107X_GPIO_CTLRS];
> -
> -static int tnetv107x_gpio_request(struct gpio_chip *chip, unsigned offset)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -
> -	gpio_reg_set_bit(regs->enable, gpio);
> -
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -
> -	return 0;
> -}
> -
> -static void tnetv107x_gpio_free(struct gpio_chip *chip, unsigned offset)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -
> -	gpio_reg_clear_bit(regs->enable, gpio);
> -
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -}
> -
> -static int tnetv107x_gpio_dir_in(struct gpio_chip *chip, unsigned offset)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -
> -	gpio_reg_set_bit(regs->direction, gpio);
> -
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -
> -	return 0;
> -}
> -
> -static int tnetv107x_gpio_dir_out(struct gpio_chip *chip,
> -		unsigned offset, int value)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -
> -	if (value)
> -		gpio_reg_set_bit(regs->data_out, gpio);
> -	else
> -		gpio_reg_clear_bit(regs->data_out, gpio);
> -
> -	gpio_reg_clear_bit(regs->direction, gpio);
> -
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -
> -	return 0;
> -}
> -
> -static int tnetv107x_gpio_get(struct gpio_chip *chip, unsigned offset)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	int ret;
> -
> -	ret = gpio_reg_get_bit(regs->data_in, gpio);
> -
> -	return ret ? 1 : 0;
> -}
> -
> -static void tnetv107x_gpio_set(struct gpio_chip *chip,
> -		unsigned offset, int value)
> -{
> -	struct davinci_gpio_controller *ctlr = chip2controller(chip);
> -	struct tnetv107x_gpio_regs __iomem *regs = ctlr->regs;
> -	unsigned gpio = chip->base + offset;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&ctlr->lock, flags);
> -
> -	if (value)
> -		gpio_reg_set_bit(regs->data_out, gpio);
> -	else
> -		gpio_reg_clear_bit(regs->data_out, gpio);
> -
> -	spin_unlock_irqrestore(&ctlr->lock, flags);
> -}
> -
> -static int __init tnetv107x_gpio_setup(void)
> -{
> -	int i, base;
> -	unsigned ngpio;
> -	struct davinci_soc_info *soc_info = &davinci_soc_info;
> -	struct tnetv107x_gpio_regs *regs;
> -	struct davinci_gpio_controller *ctlr;
> -
> -	if (soc_info->gpio_type != GPIO_TYPE_TNETV107X)
> -		return 0;
> -
> -	ngpio = soc_info->gpio_num;
> -	if (ngpio == 0) {
> -		pr_err("GPIO setup:  how many GPIOs?\n");
> -		return -EINVAL;
> -	}
> -
> -	if (WARN_ON(TNETV107X_N_GPIO < ngpio))
> -		ngpio = TNETV107X_N_GPIO;
> -
> -	regs = ioremap(soc_info->gpio_base, SZ_4K);
> -	if (WARN_ON(!regs))
> -		return -EINVAL;
> -
> -	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> -		ctlr = &chips[i];
> -
> -		ctlr->chip.label	= "tnetv107x";
> -		ctlr->chip.can_sleep	= 0;
> -		ctlr->chip.base		= base;
> -		ctlr->chip.ngpio	= ngpio - base;
> -		if (ctlr->chip.ngpio > 32)
> -			ctlr->chip.ngpio = 32;
> -
> -		ctlr->chip.request		= tnetv107x_gpio_request;
> -		ctlr->chip.free			= tnetv107x_gpio_free;
> -		ctlr->chip.direction_input	= tnetv107x_gpio_dir_in;
> -		ctlr->chip.get			= tnetv107x_gpio_get;
> -		ctlr->chip.direction_output	= tnetv107x_gpio_dir_out;
> -		ctlr->chip.set			= tnetv107x_gpio_set;
> -
> -		spin_lock_init(&ctlr->lock);
> -
> -		ctlr->regs	= regs;
> -		ctlr->set_data	= &regs->data_out[i];
> -		ctlr->clr_data	= &regs->data_out[i];
> -		ctlr->in_data	= &regs->data_in[i];
> -
> -		gpiochip_add(&ctlr->chip);
> -	}
> -
> -	soc_info->gpio_ctlrs = chips;
> -	soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> -	return 0;
> -}
> -pure_initcall(tnetv107x_gpio_setup);
> diff --git a/arch/arm/mach-davinci/tnetv107x.c b/arch/arm/mach-davinci/tnetv107x.c
> index 1b28fdd..11399d4 100644
> --- a/arch/arm/mach-davinci/tnetv107x.c
> +++ b/arch/arm/mach-davinci/tnetv107x.c
> @@ -39,7 +39,6 @@
>  #define TNETV107X_TIMER0_BASE			0x08086500
>  #define TNETV107X_TIMER1_BASE			0x08086600
>  #define TNETV107X_CHIP_CFG_BASE			0x08087000
> -#define TNETV107X_GPIO_BASE			0x08088000
>  #define TNETV107X_CLOCK_CONTROL_BASE		0x0808a000
>  #define TNETV107X_PSC_BASE			0x0808b000
>  
> @@ -746,9 +745,7 @@ static struct davinci_soc_info tnetv107x_soc_info = {
>  	.intc_irq_prios		= irq_prios,
>  	.intc_irq_num		= TNETV107X_N_CP_INTC_IRQ,
>  	.intc_host_map		= intc_host_map,
> -	.gpio_base		= TNETV107X_GPIO_BASE,
>  	.gpio_type		= GPIO_TYPE_TNETV107X,
> -	.gpio_num		= TNETV107X_N_GPIO,
>  	.timer_info		= &timer_info,
>  	.serial_dev		= &tnetv107x_serial_device,
>  	.reset			= tnetv107x_watchdog_reset,
> -- 
> 1.7.3.2
> 

[RFC/RFT 2/2] davinci: use generic memory mapped gpio for tnetv107x

[Nori, Sekhar]

Hi Grant,

On Thu, Jul 07, 2011 at 03:32:40, Grant Likely wrote:
> On Tue, Jul 05, 2011 at 10:41:00AM +0530, Sekhar Nori wrote:

> > +#define TNETV107X_N_GPIO_DEV	DIV_ROUND_UP(TNETV107X_N_GPIO, 32)
> > +
> > +static struct resource gpio_resources[TNETV107X_N_GPIO_DEV][4] = {
> > +	[0 ... TNETV107X_N_GPIO_DEV - 1] = {
> 
> If all the data is identical, why does this need to be an array?

The data is initialized identically, but there is
a loop down below which updates the start and end
address for each GPIO device. This way I get to
initialize the name and flags statically.

BTW, I just noticed that I forgot to initialize the
flags in the resource structure below. Will fix
that.

> 
> > +		{
> > +			.name	= "dat",
> > +			.start	= TNETV107X_GPIO_BASE + 0x4,
> > +			.end	= TNETV107X_GPIO_BASE + 0x4 + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "set",
> > +			.start	= TNETV107X_GPIO_BASE + 0x10,
> > +			.end	= TNETV107X_GPIO_BASE + 0x10 + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "dirin",
> > +			.start	= TNETV107X_GPIO_BASE + 0x1c,
> > +			.end	= TNETV107X_GPIO_BASE + 0x1c + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "en",
> > +			.start	= TNETV107X_GPIO_BASE + 0x28,
> > +			.end	= TNETV107X_GPIO_BASE + 0x28 + 0x4 - 1,
> > +		},
> > +	},
> > +};
> 
> Wow, this ends up looking horrible. (yes, I know it is not your
> fault).  I backed off earlier on using resources for the offsets, but
> I want to change my mind again and make interface a register range +
> offsets to the control registers.

Okay. More work on cleaning the generic driver :)

> 
> > +
> > +static struct platform_device gpio_device[] = {
> > +	[0 ... TNETV107X_N_GPIO_DEV - 1] = {
> > +		.name		= "basic-mmio-gpio",
> > +		.num_resources	= 4,
> > +	},
> > +};
> > +
> > +static struct bgpio_pdata gpio_pdata[TNETV107X_N_GPIO_DEV];
> > +
> > +static void __init tnetv107x_gpio_init(void)
> > +{
> > +	int i, j;
> > +
> > +	for (i = 1; i < TNETV107X_N_GPIO_DEV; i++) {
> > +		for (j = 0; j < 4; j++) {
> > +			gpio_resources[i][j].start += 0x4 * i;
> > +			gpio_resources[i][j].end += 0x4 * i;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < TNETV107X_N_GPIO_DEV; i++) {
> > +		int base = i * 32;
> > +
> > +		gpio_device[i].id = i;
> > +		gpio_device[i].resource = gpio_resources[i];
> > +
> > +		gpio_pdata[i].base = base;
> > +		gpio_pdata[i].ngpio = TNETV107X_N_GPIO - base;
> 
> ?  This doesn't look right.  Shouldn't ngpio be the same for each gpio
> controller instance?

Yes, ngpio is same (32) for each GPIO device except for
the last GPIO device which may have lesser number of
valid GPIO pins. The if statement below takes care of that.

> 
> > +		if (gpio_pdata[i].ngpio > 32)
> > +			gpio_pdata[i].ngpio = 32;
> > +
> > +		gpio_device[i].dev.platform_data = &gpio_pdata[i];
> > +
> > +		platform_device_register(&gpio_device[i]);
> > +	}
> > +}
> > +

Thanks,
Sekhar

[RFC/RFT 2/2] davinci: use generic memory mapped gpio for tnetv107x

[Anton Vorontsov]

Hi Grant,

On Wed, Jul 06, 2011 at 04:02:40PM -0600, Grant Likely wrote:
[...]
> > +		{
> > +			.name	= "dat",
> > +			.start	= TNETV107X_GPIO_BASE + 0x4,
> > +			.end	= TNETV107X_GPIO_BASE + 0x4 + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "set",
> > +			.start	= TNETV107X_GPIO_BASE + 0x10,
> > +			.end	= TNETV107X_GPIO_BASE + 0x10 + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "dirin",
> > +			.start	= TNETV107X_GPIO_BASE + 0x1c,
> > +			.end	= TNETV107X_GPIO_BASE + 0x1c + 0x4 - 1,
> > +		},
> > +		{
> > +			.name	= "en",
> > +			.start	= TNETV107X_GPIO_BASE + 0x28,
> > +			.end	= TNETV107X_GPIO_BASE + 0x28 + 0x4 - 1,
> > +		},
> > +	},
> > +};
> 
> Wow, this ends up looking horrible. (yes, I know it is not your
> fault).  I backed off earlier on using resources for the offsets, but
> I want to change my mind again and make interface a register range +
> offsets to the control registers.

Why is this horrible? Are you proposing a single resource + platform
data for the offsets? If so, this won't look any better, but in return

- this would complicate device registration logic and driver logic
  itself (i.e. we need to allocate platform data in the arch code,
  then parse and store the structure in the driver). The platform
  data is simply unnecessary -- we have resources that describe
  memory just fine, much better then raw 'unsigned long offset';

- we lose the ability to operate on spread registers (think of
  "enable" register is down below 2MB gap, near the pinmux
  registers block);

- In the device tree, we really want to describe registers in the
  regs = <> property, because that's where memory resources should
  be. (We also want to map address position into resource name, but
  that's different story).

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru at gmail.com