[PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

Updated from v2 with change to use the set/dat registers for input/output
register pair devices and removed some rebasing breakage.

Jamie Iles (7):
  basic_mmio_gpio: remove runtime width/endianness evaluation
  basic_mmio_gpio: convert to platform_{get,set}_drvdata()
  basic_mmio_gpio: allow overriding number of gpio
  basic_mmio_gpio: request register regions
  basic_mmio_gpio: detect output method at probe time
  basic_mmio_gpio: support different input/output registers
  basic_mmio_gpio: support direction registers

 drivers/gpio/basic_mmio_gpio.c  |  390 +++++++++++++++++++++++++++++++--------
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 311 insertions(+), 80 deletions(-)

-- 
1.7.4.2

[PATCHv3 1/7] basic_mmio_gpio: remove runtime width/endianness evaluation

[Jamie Iles]

Remove endianness/width calculations at runtime by installing function
pointers for bit-to-mask conversion and register accessors.

Reported-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |  136 +++++++++++++++++++++++++++-------------
 1 files changed, 92 insertions(+), 44 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 3addea6..e935a24 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -63,6 +63,10 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 
 struct bgpio_chip {
 	struct gpio_chip gc;
+
+	unsigned long (*read_reg)(void __iomem *reg);
+	void (*write_reg)(void __iomem *reg, unsigned long data);
+
 	void __iomem *reg_dat;
 	void __iomem *reg_set;
 	void __iomem *reg_clr;
@@ -74,7 +78,7 @@ struct bgpio_chip {
 	 * Some GPIO controllers work with the big-endian bits notation,
 	 * e.g. in a 8-bits register, GPIO7 is the least significant bit.
 	 */
-	int big_endian_bits;
+	unsigned long (*pin2mask)(struct bgpio_chip *bgc, unsigned int pin);
 
 	/*
 	 * Used to lock bgpio_chip->data. Also, this is needed to keep
@@ -91,70 +95,77 @@ static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
 	return container_of(gc, struct bgpio_chip, gc);
 }
 
-static unsigned long bgpio_in(struct bgpio_chip *bgc)
+static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
-	switch (bgc->bits) {
-	case 8:
-		return __raw_readb(bgc->reg_dat);
-	case 16:
-		return __raw_readw(bgc->reg_dat);
-	case 32:
-		return __raw_readl(bgc->reg_dat);
-#if BITS_PER_LONG >= 64
-	case 64:
-		return __raw_readq(bgc->reg_dat);
-#endif
-	}
-	return -EINVAL;
+	__raw_writeb(data, reg);
 }
 
-static void bgpio_out(struct bgpio_chip *bgc, void __iomem *reg,
-		      unsigned long data)
+static unsigned long bgpio_read8(void __iomem *reg)
 {
-	switch (bgc->bits) {
-	case 8:
-		__raw_writeb(data, reg);
-		return;
-	case 16:
-		__raw_writew(data, reg);
-		return;
-	case 32:
-		__raw_writel(data, reg);
-		return;
+	return __raw_readb(reg);
+}
+
+static void bgpio_write16(void __iomem *reg, unsigned long data)
+{
+	__raw_writew(data, reg);
+}
+
+static unsigned long bgpio_read16(void __iomem *reg)
+{
+	return __raw_readw(reg);
+}
+
+static void bgpio_write32(void __iomem *reg, unsigned long data)
+{
+	__raw_writel(data, reg);
+}
+
+static unsigned long bgpio_read32(void __iomem *reg)
+{
+	return __raw_readl(reg);
+}
+
 #if BITS_PER_LONG >= 64
-	case 64:
-		__raw_writeq(data, reg);
-		return;
-#endif
-	}
+static void bgpio_write64(void __iomem *reg, unsigned long data)
+{
+	__raw_writeq(data, reg);
 }
 
+static unsigned long bgpio_read64(void __iomem *reg)
+{
+	return __raw_readq(reg);
+}
+#endif /* BITS_PER_LONG >= 64 */
+
 static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
 {
-	if (bgc->big_endian_bits)
-		return 1 << (bgc->bits - 1 - pin);
-	else
-		return 1 << pin;
+	return 1 << pin;
+}
+
+static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
+				       unsigned int pin)
+{
+	return 1 << (bgc->bits - 1 - pin);
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 
-	return bgpio_in(bgc) & bgpio_pin2mask(bgc, gpio);
+	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
 }
 
 static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
-	unsigned long mask = bgpio_pin2mask(bgc, gpio);
+	unsigned long mask = bgc->pin2mask(bgc, gpio);
 	unsigned long flags;
 
 	if (bgc->reg_set) {
 		if (val)
-			bgpio_out(bgc, bgc->reg_set, mask);
+			bgc->write_reg(bgc->reg_set, mask);
 		else
-			bgpio_out(bgc, bgc->reg_clr, mask);
+			bgc->write_reg(bgc->reg_clr, mask);
 		return;
 	}
 
@@ -165,7 +176,7 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	else
 		bgc->data &= ~mask;
 
-	bgpio_out(bgc, bgc->reg_dat, bgc->data);
+	bgc->write_reg(bgc->reg_dat, bgc->data);
 
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
@@ -181,9 +192,44 @@ static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
-static int __devinit bgpio_probe(struct platform_device *pdev)
+static int bgpio_setup_accessors(struct platform_device *pdev,
+				 struct bgpio_chip *bgc)
 {
 	const struct platform_device_id *platid = platform_get_device_id(pdev);
+
+	switch (bgc->bits) {
+	case 8:
+		bgc->read_reg	= bgpio_read8;
+		bgc->write_reg	= bgpio_write8;
+		break;
+	case 16:
+		bgc->read_reg	= bgpio_read16;
+		bgc->write_reg	= bgpio_write16;
+		break;
+	case 32:
+		bgc->read_reg	= bgpio_read32;
+		bgc->write_reg	= bgpio_write32;
+		break;
+#if BITS_PER_LONG >= 64
+	case 64:
+		bgc->read_reg	= bgpio_read64;
+		bgc->write_reg	= bgpio_write64;
+		break;
+#endif /* BITS_PER_LONG >= 64 */
+	default:
+		dev_err(&pdev->dev, "unsupported data width %u bits\n",
+			bgc->bits);
+		return -EINVAL;
+	}
+
+	bgc->pin2mask = strcmp(platid->name, "basic-mmio-gpio-be") ?
+		bgpio_pin2mask : bgpio_pin2mask_be;
+
+	return 0;
+}
+
+static int __devinit bgpio_probe(struct platform_device *pdev)
+{
 	struct device *dev = &pdev->dev;
 	struct bgpio_pdata *pdata = dev_get_platdata(dev);
 	struct bgpio_chip *bgc;
@@ -232,9 +278,11 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	spin_lock_init(&bgc->lock);
 
 	bgc->bits = bits;
-	bgc->big_endian_bits = !strcmp(platid->name, "basic-mmio-gpio-be");
-	bgc->data = bgpio_in(bgc);
+	ret = bgpio_setup_accessors(pdev, bgc);
+	if (ret)
+		return ret;
 
+	bgc->data = bgc->read_reg(bgc->reg_dat);
 	bgc->gc.ngpio = bits;
 	bgc->gc.direction_input = bgpio_dir_in;
 	bgc->gc.direction_output = bgpio_dir_out;
-- 
1.7.4.2

Re: [PATCHv3 1/7] basic_mmio_gpio: remove runtime width/endianness evaluation

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:48PM +0100, Jamie Iles wrote:
> Remove endianness/width calculations at runtime by installing function
> pointers for bit-to-mask conversion and register accessors.
> 
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |  136 +++++++++++++++++++++++++++-------------
>  1 files changed, 92 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 3addea6..e935a24 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -63,6 +63,10 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
>  
>  struct bgpio_chip {
>  	struct gpio_chip gc;
> +
> +	unsigned long (*read_reg)(void __iomem *reg);
> +	void (*write_reg)(void __iomem *reg, unsigned long data);
> +
>  	void __iomem *reg_dat;
>  	void __iomem *reg_set;
>  	void __iomem *reg_clr;
> @@ -74,7 +78,7 @@ struct bgpio_chip {
>  	 * Some GPIO controllers work with the big-endian bits notation,
>  	 * e.g. in a 8-bits register, GPIO7 is the least significant bit.
>  	 */
> -	int big_endian_bits;
> +	unsigned long (*pin2mask)(struct bgpio_chip *bgc, unsigned int pin);
>  
>  	/*
>  	 * Used to lock bgpio_chip->data. Also, this is needed to keep
> @@ -91,70 +95,77 @@ static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
>  	return container_of(gc, struct bgpio_chip, gc);
>  }
>  
> -static unsigned long bgpio_in(struct bgpio_chip *bgc)
> +static void bgpio_write8(void __iomem *reg, unsigned long data)
>  {
> -	switch (bgc->bits) {
> -	case 8:
> -		return __raw_readb(bgc->reg_dat);
> -	case 16:
> -		return __raw_readw(bgc->reg_dat);
> -	case 32:
> -		return __raw_readl(bgc->reg_dat);
> -#if BITS_PER_LONG >= 64
> -	case 64:
> -		return __raw_readq(bgc->reg_dat);
> -#endif
> -	}
> -	return -EINVAL;
> +	__raw_writeb(data, reg);
>  }
>  
> -static void bgpio_out(struct bgpio_chip *bgc, void __iomem *reg,
> -		      unsigned long data)
> +static unsigned long bgpio_read8(void __iomem *reg)
>  {
> -	switch (bgc->bits) {
> -	case 8:
> -		__raw_writeb(data, reg);
> -		return;
> -	case 16:
> -		__raw_writew(data, reg);
> -		return;
> -	case 32:
> -		__raw_writel(data, reg);
> -		return;
> +	return __raw_readb(reg);
> +}
> +
> +static void bgpio_write16(void __iomem *reg, unsigned long data)
> +{
> +	__raw_writew(data, reg);
> +}
> +
> +static unsigned long bgpio_read16(void __iomem *reg)
> +{
> +	return __raw_readw(reg);
> +}
> +
> +static void bgpio_write32(void __iomem *reg, unsigned long data)
> +{
> +	__raw_writel(data, reg);
> +}
> +
> +static unsigned long bgpio_read32(void __iomem *reg)
> +{
> +	return __raw_readl(reg);
> +}
> +
>  #if BITS_PER_LONG >= 64
> -	case 64:
> -		__raw_writeq(data, reg);
> -		return;
> -#endif
> -	}
> +static void bgpio_write64(void __iomem *reg, unsigned long data)
> +{
> +	__raw_writeq(data, reg);
>  }
>  
> +static unsigned long bgpio_read64(void __iomem *reg)
> +{
> +	return __raw_readq(reg);
> +}
> +#endif /* BITS_PER_LONG >= 64 */
> +
>  static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
>  {
> -	if (bgc->big_endian_bits)
> -		return 1 << (bgc->bits - 1 - pin);
> -	else
> -		return 1 << pin;
> +	return 1 << pin;
> +}
> +
> +static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
> +				       unsigned int pin)
> +{
> +	return 1 << (bgc->bits - 1 - pin);
>  }
>  
>  static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
>  {
>  	struct bgpio_chip *bgc = to_bgpio_chip(gc);
>  
> -	return bgpio_in(bgc) & bgpio_pin2mask(bgc, gpio);
> +	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
>  }
>  
>  static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  {
>  	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> -	unsigned long mask = bgpio_pin2mask(bgc, gpio);
> +	unsigned long mask = bgc->pin2mask(bgc, gpio);
>  	unsigned long flags;
>  
>  	if (bgc->reg_set) {
>  		if (val)
> -			bgpio_out(bgc, bgc->reg_set, mask);
> +			bgc->write_reg(bgc->reg_set, mask);
>  		else
> -			bgpio_out(bgc, bgc->reg_clr, mask);
> +			bgc->write_reg(bgc->reg_clr, mask);
>  		return;
>  	}
>  
> @@ -165,7 +176,7 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  	else
>  		bgc->data &= ~mask;
>  
> -	bgpio_out(bgc, bgc->reg_dat, bgc->data);
> +	bgc->write_reg(bgc->reg_dat, bgc->data);
>  
>  	spin_unlock_irqrestore(&bgc->lock, flags);
>  }
> @@ -181,9 +192,44 @@ static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  	return 0;
>  }
>  
> -static int __devinit bgpio_probe(struct platform_device *pdev)
> +static int bgpio_setup_accessors(struct platform_device *pdev,
> +				 struct bgpio_chip *bgc)
>  {
>  	const struct platform_device_id *platid = platform_get_device_id(pdev);
> +
> +	switch (bgc->bits) {
> +	case 8:
> +		bgc->read_reg	= bgpio_read8;
> +		bgc->write_reg	= bgpio_write8;
> +		break;
> +	case 16:
> +		bgc->read_reg	= bgpio_read16;
> +		bgc->write_reg	= bgpio_write16;
> +		break;
> +	case 32:
> +		bgc->read_reg	= bgpio_read32;
> +		bgc->write_reg	= bgpio_write32;
> +		break;
> +#if BITS_PER_LONG >= 64
> +	case 64:
> +		bgc->read_reg	= bgpio_read64;
> +		bgc->write_reg	= bgpio_write64;
> +		break;
> +#endif /* BITS_PER_LONG >= 64 */
> +	default:
> +		dev_err(&pdev->dev, "unsupported data width %u bits\n",
> +			bgc->bits);
> +		return -EINVAL;
> +	}
> +
> +	bgc->pin2mask = strcmp(platid->name, "basic-mmio-gpio-be") ?
> +		bgpio_pin2mask : bgpio_pin2mask_be;
> +
> +	return 0;
> +}
> +
> +static int __devinit bgpio_probe(struct platform_device *pdev)
> +{
>  	struct device *dev = &pdev->dev;
>  	struct bgpio_pdata *pdata = dev_get_platdata(dev);
>  	struct bgpio_chip *bgc;
> @@ -232,9 +278,11 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	spin_lock_init(&bgc->lock);
>  
>  	bgc->bits = bits;
> -	bgc->big_endian_bits = !strcmp(platid->name, "basic-mmio-gpio-be");
> -	bgc->data = bgpio_in(bgc);
> +	ret = bgpio_setup_accessors(pdev, bgc);
> +	if (ret)
> +		return ret;
>  
> +	bgc->data = bgc->read_reg(bgc->reg_dat);
>  	bgc->gc.ngpio = bits;
>  	bgc->gc.direction_input = bgpio_dir_in;
>  	bgc->gc.direction_output = bgpio_dir_out;
> -- 
> 1.7.4.2
> 

[PATCHv3 2/7] basic_mmio_gpio: convert to platform_{get,set}_drvdata()

[Jamie Iles]

Use the platform drvdata helpers rather than working on the struct
device itself.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index e935a24..5db5de4 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -296,7 +296,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	else
 		bgc->gc.base = -1;
 
-	dev_set_drvdata(dev, bgc);
+	platform_set_drvdata(pdev, bgc);
 
 	ret = gpiochip_add(&bgc->gc);
 	if (ret)
@@ -307,7 +307,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 
 static int __devexit bgpio_remove(struct platform_device *pdev)
 {
-	struct bgpio_chip *bgc = dev_get_drvdata(&pdev->dev);
+	struct bgpio_chip *bgc = platform_get_drvdata(pdev);
 
 	return gpiochip_remove(&bgc->gc);
 }
-- 
1.7.4.2

Re: [PATCHv3 2/7] basic_mmio_gpio: convert to platform_{get,set}_drvdata()

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:49PM +0100, Jamie Iles wrote:
> Use the platform drvdata helpers rather than working on the struct
> device itself.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index e935a24..5db5de4 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -296,7 +296,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	else
>  		bgc->gc.base = -1;
>  
> -	dev_set_drvdata(dev, bgc);
> +	platform_set_drvdata(pdev, bgc);
>  
>  	ret = gpiochip_add(&bgc->gc);
>  	if (ret)
> @@ -307,7 +307,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  
>  static int __devexit bgpio_remove(struct platform_device *pdev)
>  {
> -	struct bgpio_chip *bgc = dev_get_drvdata(&pdev->dev);
> +	struct bgpio_chip *bgc = platform_get_drvdata(pdev);
>  
>  	return gpiochip_remove(&bgc->gc);
>  }
> -- 
> 1.7.4.2
> 

[PATCHv3 3/7] basic_mmio_gpio: allow overriding number of gpio

[Jamie Iles]

Some platforms may have a number of GPIO that is less than the register
width of the peripheral.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c  |   18 ++++++++++++------
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 5db5de4..2b2d384 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -239,6 +239,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	resource_size_t dat_sz;
 	int bits;
 	int ret;
+	int ngpio;
 
 	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!res_dat)
@@ -249,6 +250,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	bits = dat_sz * 8;
+	ngpio = bits;
 	if (bits > BITS_PER_LONG)
 		return -EINVAL;
 
@@ -277,13 +279,22 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 
 	spin_lock_init(&bgc->lock);
 
+	if (pdata) {
+		bgc->gc.base = pdata->base;
+		if (pdata->ngpio > 0)
+			ngpio = pdata->ngpio;
+	} else {
+		bgc->gc.base = -1;
+	}
+
 	bgc->bits = bits;
 	ret = bgpio_setup_accessors(pdev, bgc);
 	if (ret)
 		return ret;
 
 	bgc->data = bgc->read_reg(bgc->reg_dat);
-	bgc->gc.ngpio = bits;
+
+	bgc->gc.ngpio = ngpio;
 	bgc->gc.direction_input = bgpio_dir_in;
 	bgc->gc.direction_output = bgpio_dir_out;
 	bgc->gc.get = bgpio_get;
@@ -291,11 +302,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	bgc->gc.dev = dev;
 	bgc->gc.label = dev_name(dev);
 
-	if (pdata)
-		bgc->gc.base = pdata->base;
-	else
-		bgc->gc.base = -1;
-
 	platform_set_drvdata(pdev, bgc);
 
 	ret = gpiochip_add(&bgc->gc);
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index 198087a..f23ec73 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -15,6 +15,7 @@
 
 struct bgpio_pdata {
 	int base;
+	int ngpio;
 };
 
 #endif /* __BASIC_MMIO_GPIO_H */
-- 
1.7.4.2

Re: [PATCHv3 3/7] basic_mmio_gpio: allow overriding number of gpio

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:50PM +0100, Jamie Iles wrote:
> Some platforms may have a number of GPIO that is less than the register
> width of the peripheral.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c  |   18 ++++++++++++------
>  include/linux/basic_mmio_gpio.h |    1 +
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 5db5de4..2b2d384 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -239,6 +239,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	resource_size_t dat_sz;
>  	int bits;
>  	int ret;
> +	int ngpio;
>  
>  	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>  	if (!res_dat)
> @@ -249,6 +250,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  
>  	bits = dat_sz * 8;
> +	ngpio = bits;
>  	if (bits > BITS_PER_LONG)
>  		return -EINVAL;
>  
> @@ -277,13 +279,22 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&bgc->lock);
>  
> +	if (pdata) {
> +		bgc->gc.base = pdata->base;
> +		if (pdata->ngpio > 0)
> +			ngpio = pdata->ngpio;
> +	} else {
> +		bgc->gc.base = -1;
> +	}
> +
>  	bgc->bits = bits;
>  	ret = bgpio_setup_accessors(pdev, bgc);
>  	if (ret)
>  		return ret;
>  
>  	bgc->data = bgc->read_reg(bgc->reg_dat);
> -	bgc->gc.ngpio = bits;
> +
> +	bgc->gc.ngpio = ngpio;
>  	bgc->gc.direction_input = bgpio_dir_in;
>  	bgc->gc.direction_output = bgpio_dir_out;
>  	bgc->gc.get = bgpio_get;
> @@ -291,11 +302,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	bgc->gc.dev = dev;
>  	bgc->gc.label = dev_name(dev);
>  
> -	if (pdata)
> -		bgc->gc.base = pdata->base;
> -	else
> -		bgc->gc.base = -1;
> -
>  	platform_set_drvdata(pdev, bgc);
>  
>  	ret = gpiochip_add(&bgc->gc);
> diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
> index 198087a..f23ec73 100644
> --- a/include/linux/basic_mmio_gpio.h
> +++ b/include/linux/basic_mmio_gpio.h
> @@ -15,6 +15,7 @@
>  
>  struct bgpio_pdata {
>  	int base;
> +	int ngpio;
>  };
>  
>  #endif /* __BASIC_MMIO_GPIO_H */
> -- 
> 1.7.4.2
> 

[PATCHv3 4/7] basic_mmio_gpio: request register regions

[Jamie Iles]

Make sure that we get the register regions with request_mem_region()
before ioremap() to make sure we have exclusive access.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 2b2d384..90f7f89 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -192,6 +192,16 @@ static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
+static void __iomem *bgpio_request_and_map(struct device *dev,
+					   struct resource *res)
+{
+	if (!devm_request_mem_region(dev, res->start, resource_size(res),
+				     res->name ?: "mmio_gpio"))
+		return NULL;
+
+	return devm_ioremap(dev, res->start, resource_size(res));
+}
+
 static int bgpio_setup_accessors(struct platform_device *pdev,
 				 struct bgpio_chip *bgc)
 {
@@ -258,7 +268,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (!bgc)
 		return -ENOMEM;
 
-	bgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
+	bgc->reg_dat = bgpio_request_and_map(dev, res_dat);
 	if (!bgc->reg_dat)
 		return -ENOMEM;
 
@@ -269,8 +279,8 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 				resource_size(res_set) != dat_sz)
 			return -EINVAL;
 
-		bgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
-		bgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
+		bgc->reg_set = bgpio_request_and_map(dev, res_set);
+		bgc->reg_clr = bgpio_request_and_map(dev, res_clr);
 		if (!bgc->reg_set || !bgc->reg_clr)
 			return -ENOMEM;
 	} else if (res_set || res_clr) {
-- 
1.7.4.2

Re: [PATCHv3 4/7] basic_mmio_gpio: request register regions

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:51PM +0100, Jamie Iles wrote:
> Make sure that we get the register regions with request_mem_region()
> before ioremap() to make sure we have exclusive access.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 2b2d384..90f7f89 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -192,6 +192,16 @@ static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  	return 0;
>  }
>  
> +static void __iomem *bgpio_request_and_map(struct device *dev,
> +					   struct resource *res)
> +{
> +	if (!devm_request_mem_region(dev, res->start, resource_size(res),
> +				     res->name ?: "mmio_gpio"))
> +		return NULL;
> +
> +	return devm_ioremap(dev, res->start, resource_size(res));
> +}
> +
>  static int bgpio_setup_accessors(struct platform_device *pdev,
>  				 struct bgpio_chip *bgc)
>  {
> @@ -258,7 +268,7 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	if (!bgc)
>  		return -ENOMEM;
>  
> -	bgc->reg_dat = devm_ioremap(dev, res_dat->start, dat_sz);
> +	bgc->reg_dat = bgpio_request_and_map(dev, res_dat);
>  	if (!bgc->reg_dat)
>  		return -ENOMEM;
>  
> @@ -269,8 +279,8 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  				resource_size(res_set) != dat_sz)
>  			return -EINVAL;
>  
> -		bgc->reg_set = devm_ioremap(dev, res_set->start, dat_sz);
> -		bgc->reg_clr = devm_ioremap(dev, res_clr->start, dat_sz);
> +		bgc->reg_set = bgpio_request_and_map(dev, res_set);
> +		bgc->reg_clr = bgpio_request_and_map(dev, res_clr);
>  		if (!bgc->reg_set || !bgc->reg_clr)
>  			return -ENOMEM;
>  	} else if (res_set || res_clr) {
> -- 
> 1.7.4.2
> 

[PATCHv3 5/7] basic_mmio_gpio: detect output method at probe time

[Jamie Iles]

Rather than detecting the output method each time in the .set()
callback, do it at probe time and set the appropriate callback.

Changes since v2: moved the reg_dat initialization into
bgpio_setup_io().

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Cc: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |   90 ++++++++++++++++++++++++++--------------
 1 files changed, 59 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 90f7f89..728bc67 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -161,14 +161,6 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	unsigned long mask = bgc->pin2mask(bgc, gpio);
 	unsigned long flags;
 
-	if (bgc->reg_set) {
-		if (val)
-			bgc->write_reg(bgc->reg_set, mask);
-		else
-			bgc->write_reg(bgc->reg_clr, mask);
-		return;
-	}
-
 	spin_lock_irqsave(&bgc->lock, flags);
 
 	if (val)
@@ -181,6 +173,18 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
+static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio,
+				 int val)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long mask = bgc->pin2mask(bgc, gpio);
+
+	if (val)
+		bgc->write_reg(bgc->reg_set, mask);
+	else
+		bgc->write_reg(bgc->reg_clr, mask);
+}
+
 static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
 	return 0;
@@ -188,7 +192,8 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 
 static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 {
-	bgpio_set(gc, gpio, val);
+	gc->set(gc, gpio, val);
+
 	return 0;
 }
 
@@ -238,18 +243,25 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
 	return 0;
 }
 
-static int __devinit bgpio_probe(struct platform_device *pdev)
+/*
+ * Create the device and allocate the resources.  For setting GPIO's there are
+ * two supported configurations:
+ *
+ *	- single output register resource (named "dat").
+ *	- set/clear pair (named "set" and "clr").
+ *
+ * For the single output register, this drives a 1 by setting a bit and a zero
+ * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
+ * in the set register and clears it by setting a bit in the clear register.
+ * The configuration is detected by which resources are present.
+ */
+static int bgpio_setup_io(struct platform_device *pdev,
+			  struct bgpio_chip *bgc)
 {
-	struct device *dev = &pdev->dev;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
-	struct bgpio_chip *bgc;
-	struct resource *res_dat;
 	struct resource *res_set;
 	struct resource *res_clr;
+	struct resource *res_dat;
 	resource_size_t dat_sz;
-	int bits;
-	int ret;
-	int ngpio;
 
 	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
 	if (!res_dat)
@@ -259,16 +271,11 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (!is_power_of_2(dat_sz))
 		return -EINVAL;
 
-	bits = dat_sz * 8;
-	ngpio = bits;
-	if (bits > BITS_PER_LONG)
+	bgc->bits = dat_sz * 8;
+	if (bgc->bits > BITS_PER_LONG)
 		return -EINVAL;
 
-	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
-	if (!bgc)
-		return -ENOMEM;
-
-	bgc->reg_dat = bgpio_request_and_map(dev, res_dat);
+	bgc->reg_dat = bgpio_request_and_map(&pdev->dev, res_dat);
 	if (!bgc->reg_dat)
 		return -ENOMEM;
 
@@ -276,19 +283,41 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
 	if (res_set && res_clr) {
 		if (resource_size(res_set) != resource_size(res_clr) ||
-				resource_size(res_set) != dat_sz)
+		    resource_size(res_set) != resource_size(res_dat))
 			return -EINVAL;
 
-		bgc->reg_set = bgpio_request_and_map(dev, res_set);
-		bgc->reg_clr = bgpio_request_and_map(dev, res_clr);
+		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
+		bgc->reg_clr = bgpio_request_and_map(&pdev->dev, res_clr);
 		if (!bgc->reg_set || !bgc->reg_clr)
 			return -ENOMEM;
+
+		bgc->gc.set = bgpio_set_with_clear;
 	} else if (res_set || res_clr) {
 		return -EINVAL;
+	} else {
+		bgc->gc.set = bgpio_set;
 	}
 
-	spin_lock_init(&bgc->lock);
+	return 0;
+}
+
+static int __devinit bgpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+	struct bgpio_chip *bgc;
+	int ret;
+	int ngpio;
+
+	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
+	if (!bgc)
+		return -ENOMEM;
+
+	ret = bgpio_setup_io(pdev, bgc);
+	if (ret)
+		return ret;
 
+	ngpio = bgc->bits;
 	if (pdata) {
 		bgc->gc.base = pdata->base;
 		if (pdata->ngpio > 0)
@@ -297,18 +326,17 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 		bgc->gc.base = -1;
 	}
 
-	bgc->bits = bits;
 	ret = bgpio_setup_accessors(pdev, bgc);
 	if (ret)
 		return ret;
 
+	spin_lock_init(&bgc->lock);
 	bgc->data = bgc->read_reg(bgc->reg_dat);
 
 	bgc->gc.ngpio = ngpio;
 	bgc->gc.direction_input = bgpio_dir_in;
 	bgc->gc.direction_output = bgpio_dir_out;
 	bgc->gc.get = bgpio_get;
-	bgc->gc.set = bgpio_set;
 	bgc->gc.dev = dev;
 	bgc->gc.label = dev_name(dev);
 
-- 
1.7.4.2

Re: [PATCHv3 5/7] basic_mmio_gpio: detect output method at probe time

[Anton Vorontsov]

On Mon, Apr 11, 2011 at 12:21:52PM +0100, Jamie Iles wrote:
> Rather than detecting the output method each time in the .set()
> callback, do it at probe time and set the appropriate callback.
> 
> Changes since v2: moved the reg_dat initialization into
> bgpio_setup_io().
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---

Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks Jamie!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 5/7] basic_mmio_gpio: detect output method at probe time

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:52PM +0100, Jamie Iles wrote:
> Rather than detecting the output method each time in the .set()
> callback, do it at probe time and set the appropriate callback.
> 
> Changes since v2: moved the reg_dat initialization into
> bgpio_setup_io().
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |   90 ++++++++++++++++++++++++++--------------
>  1 files changed, 59 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 90f7f89..728bc67 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -161,14 +161,6 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  	unsigned long mask = bgc->pin2mask(bgc, gpio);
>  	unsigned long flags;
>  
> -	if (bgc->reg_set) {
> -		if (val)
> -			bgc->write_reg(bgc->reg_set, mask);
> -		else
> -			bgc->write_reg(bgc->reg_clr, mask);
> -		return;
> -	}
> -
>  	spin_lock_irqsave(&bgc->lock, flags);
>  
>  	if (val)
> @@ -181,6 +173,18 @@ static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  	spin_unlock_irqrestore(&bgc->lock, flags);
>  }
>  
> +static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio,
> +				 int val)
> +{
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long mask = bgc->pin2mask(bgc, gpio);
> +
> +	if (val)
> +		bgc->write_reg(bgc->reg_set, mask);
> +	else
> +		bgc->write_reg(bgc->reg_clr, mask);
> +}
> +
>  static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
>  {
>  	return 0;
> @@ -188,7 +192,8 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
>  
>  static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  {
> -	bgpio_set(gc, gpio, val);
> +	gc->set(gc, gpio, val);
> +
>  	return 0;
>  }
>  
> @@ -238,18 +243,25 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static int __devinit bgpio_probe(struct platform_device *pdev)
> +/*
> + * Create the device and allocate the resources.  For setting GPIO's there are
> + * two supported configurations:
> + *
> + *	- single output register resource (named "dat").
> + *	- set/clear pair (named "set" and "clr").
> + *
> + * For the single output register, this drives a 1 by setting a bit and a zero
> + * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
> + * in the set register and clears it by setting a bit in the clear register.
> + * The configuration is detected by which resources are present.
> + */
> +static int bgpio_setup_io(struct platform_device *pdev,
> +			  struct bgpio_chip *bgc)
>  {
> -	struct device *dev = &pdev->dev;
> -	struct bgpio_pdata *pdata = dev_get_platdata(dev);
> -	struct bgpio_chip *bgc;
> -	struct resource *res_dat;
>  	struct resource *res_set;
>  	struct resource *res_clr;
> +	struct resource *res_dat;
>  	resource_size_t dat_sz;
> -	int bits;
> -	int ret;
> -	int ngpio;
>  
>  	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
>  	if (!res_dat)
> @@ -259,16 +271,11 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	if (!is_power_of_2(dat_sz))
>  		return -EINVAL;
>  
> -	bits = dat_sz * 8;
> -	ngpio = bits;
> -	if (bits > BITS_PER_LONG)
> +	bgc->bits = dat_sz * 8;
> +	if (bgc->bits > BITS_PER_LONG)
>  		return -EINVAL;
>  
> -	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
> -	if (!bgc)
> -		return -ENOMEM;
> -
> -	bgc->reg_dat = bgpio_request_and_map(dev, res_dat);
> +	bgc->reg_dat = bgpio_request_and_map(&pdev->dev, res_dat);
>  	if (!bgc->reg_dat)
>  		return -ENOMEM;
>  
> @@ -276,19 +283,41 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
>  	if (res_set && res_clr) {
>  		if (resource_size(res_set) != resource_size(res_clr) ||
> -				resource_size(res_set) != dat_sz)
> +		    resource_size(res_set) != resource_size(res_dat))
>  			return -EINVAL;
>  
> -		bgc->reg_set = bgpio_request_and_map(dev, res_set);
> -		bgc->reg_clr = bgpio_request_and_map(dev, res_clr);
> +		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
> +		bgc->reg_clr = bgpio_request_and_map(&pdev->dev, res_clr);
>  		if (!bgc->reg_set || !bgc->reg_clr)
>  			return -ENOMEM;
> +
> +		bgc->gc.set = bgpio_set_with_clear;
>  	} else if (res_set || res_clr) {
>  		return -EINVAL;
> +	} else {
> +		bgc->gc.set = bgpio_set;
>  	}
>  
> -	spin_lock_init(&bgc->lock);
> +	return 0;
> +}
> +
> +static int __devinit bgpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bgpio_pdata *pdata = dev_get_platdata(dev);
> +	struct bgpio_chip *bgc;
> +	int ret;
> +	int ngpio;
> +
> +	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
> +	if (!bgc)
> +		return -ENOMEM;
> +
> +	ret = bgpio_setup_io(pdev, bgc);
> +	if (ret)
> +		return ret;
>  
> +	ngpio = bgc->bits;
>  	if (pdata) {
>  		bgc->gc.base = pdata->base;
>  		if (pdata->ngpio > 0)
> @@ -297,18 +326,17 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  		bgc->gc.base = -1;
>  	}
>  
> -	bgc->bits = bits;
>  	ret = bgpio_setup_accessors(pdev, bgc);
>  	if (ret)
>  		return ret;
>  
> +	spin_lock_init(&bgc->lock);
>  	bgc->data = bgc->read_reg(bgc->reg_dat);
>  
>  	bgc->gc.ngpio = ngpio;
>  	bgc->gc.direction_input = bgpio_dir_in;
>  	bgc->gc.direction_output = bgpio_dir_out;
>  	bgc->gc.get = bgpio_get;
> -	bgc->gc.set = bgpio_set;
>  	bgc->gc.dev = dev;
>  	bgc->gc.label = dev_name(dev);
>  
> -- 
> 1.7.4.2
> 

[PATCHv3 6/7] basic_mmio_gpio: support different input/output registers

[Jamie Iles]

Some controllers have separate input and output registers.  For these
controllers, use "set" for the output and "dat" for the input.

Changes since v2: reuse "set" for output and "dat" for input rather than
adding a new "in" register.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Cc: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |   38 +++++++++++++++++++++++++++++++++-----
 1 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 728bc67..6b99489 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -185,6 +185,24 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio,
 		bgc->write_reg(bgc->reg_clr, mask);
 }
 
+static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long mask = bgc->pin2mask(bgc, gpio);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	if (val)
+		bgc->data |= mask;
+	else
+		bgc->data &= ~mask;
+
+	bgc->write_reg(bgc->reg_set, bgc->data);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
 static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
 	return 0;
@@ -245,10 +263,12 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
 
 /*
  * Create the device and allocate the resources.  For setting GPIO's there are
- * two supported configurations:
+ * three supported configurations:
  *
- *	- single output register resource (named "dat").
+ *	- single input/output register resource (named "dat").
  *	- set/clear pair (named "set" and "clr").
+ *	- single output register resource and single input resource ("set" and
+ *	dat").
  *
  * For the single output register, this drives a 1 by setting a bit and a zero
  * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
@@ -292,12 +312,21 @@ static int bgpio_setup_io(struct platform_device *pdev,
 			return -ENOMEM;
 
 		bgc->gc.set = bgpio_set_with_clear;
-	} else if (res_set || res_clr) {
-		return -EINVAL;
+	} else if (res_set && !res_clr) {
+		if (resource_size(res_set) != resource_size(res_dat))
+			return -EINVAL;
+
+		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
+		if (!bgc->reg_set)
+			return -ENOMEM;
+
+		bgc->gc.set = bgpio_set_set;
 	} else {
 		bgc->gc.set = bgpio_set;
 	}
 
+	bgc->gc.get = bgpio_get;
+
 	return 0;
 }
 
@@ -336,7 +365,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	bgc->gc.ngpio = ngpio;
 	bgc->gc.direction_input = bgpio_dir_in;
 	bgc->gc.direction_output = bgpio_dir_out;
-	bgc->gc.get = bgpio_get;
 	bgc->gc.dev = dev;
 	bgc->gc.label = dev_name(dev);
 
-- 
1.7.4.2

Re: [PATCHv3 6/7] basic_mmio_gpio: support different input/output registers

[Anton Vorontsov]

On Mon, Apr 11, 2011 at 12:21:53PM +0100, Jamie Iles wrote:
> Some controllers have separate input and output registers.  For these
> controllers, use "set" for the output and "dat" for the input.
> 
> Changes since v2: reuse "set" for output and "dat" for input rather than
> adding a new "in" register.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---

Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 6/7] basic_mmio_gpio: support different input/output registers

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:53PM +0100, Jamie Iles wrote:
> Some controllers have separate input and output registers.  For these
> controllers, use "set" for the output and "dat" for the input.
> 
> Changes since v2: reuse "set" for output and "dat" for input rather than
> adding a new "in" register.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |   38 +++++++++++++++++++++++++++++++++-----
>  1 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 728bc67..6b99489 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -185,6 +185,24 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio,
>  		bgc->write_reg(bgc->reg_clr, mask);
>  }
>  
> +static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long mask = bgc->pin2mask(bgc, gpio);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	if (val)
> +		bgc->data |= mask;
> +	else
> +		bgc->data &= ~mask;
> +
> +	bgc->write_reg(bgc->reg_set, bgc->data);
> +
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +}
> +
>  static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
>  {
>  	return 0;
> @@ -245,10 +263,12 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
>  
>  /*
>   * Create the device and allocate the resources.  For setting GPIO's there are
> - * two supported configurations:
> + * three supported configurations:
>   *
> - *	- single output register resource (named "dat").
> + *	- single input/output register resource (named "dat").
>   *	- set/clear pair (named "set" and "clr").
> + *	- single output register resource and single input resource ("set" and
> + *	dat").
>   *
>   * For the single output register, this drives a 1 by setting a bit and a zero
>   * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
> @@ -292,12 +312,21 @@ static int bgpio_setup_io(struct platform_device *pdev,
>  			return -ENOMEM;
>  
>  		bgc->gc.set = bgpio_set_with_clear;
> -	} else if (res_set || res_clr) {
> -		return -EINVAL;
> +	} else if (res_set && !res_clr) {
> +		if (resource_size(res_set) != resource_size(res_dat))
> +			return -EINVAL;
> +
> +		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
> +		if (!bgc->reg_set)
> +			return -ENOMEM;
> +
> +		bgc->gc.set = bgpio_set_set;
>  	} else {
>  		bgc->gc.set = bgpio_set;
>  	}
>  
> +	bgc->gc.get = bgpio_get;
> +
>  	return 0;
>  }
>  
> @@ -336,7 +365,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	bgc->gc.ngpio = ngpio;
>  	bgc->gc.direction_input = bgpio_dir_in;
>  	bgc->gc.direction_output = bgpio_dir_out;
> -	bgc->gc.get = bgpio_get;
>  	bgc->gc.dev = dev;
>  	bgc->gc.label = dev_name(dev);
>  
> -- 
> 1.7.4.2
> 

[PATCHv3 7/7] basic_mmio_gpio: support direction registers

[Jamie Iles]

Most controllers require the direction of a GPIO to be set by writing to
a direction register.  Add support for either an input direction
register or an output direction register.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |  114 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 6b99489..f4afd96 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -70,6 +70,7 @@ struct bgpio_chip {
 	void __iomem *reg_dat;
 	void __iomem *reg_set;
 	void __iomem *reg_clr;
+	void __iomem *reg_dir;
 
 	/* Number of bits (GPIOs): <register width> * 8. */
 	int bits;
@@ -88,6 +89,9 @@ struct bgpio_chip {
 
 	/* Shadowed data register to clear/set bits safely. */
 	unsigned long data;
+
+	/* Shadowed direction registers to clear/set direction safely. */
+	unsigned long dir;
 };
 
 static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
@@ -203,15 +207,80 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val)
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
+static int bgpio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	return 0;
+}
+
+static int bgpio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio,
+				int val)
+{
+	gc->set(gc, gpio, val);
+
+	return 0;
+}
+
 static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->dir &= ~bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dir, bgc->dir);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
 	return 0;
 }
 
 static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
 {
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	gc->set(gc, gpio, val);
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->dir |= bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dir, bgc->dir);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+
+static int bgpio_dir_in_inv(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->dir |= bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dir, bgc->dir);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
+	return 0;
+}
+
+static int bgpio_dir_out_inv(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
 	gc->set(gc, gpio, val);
 
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->dir &= ~bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dir, bgc->dir);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
 	return 0;
 }
 
@@ -274,6 +343,14 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
  * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
  * in the set register and clears it by setting a bit in the clear register.
  * The configuration is detected by which resources are present.
+ *
+ * For setting the GPIO direction, there are three supported configurations:
+ *
+ *	- simple bidirection GPIO that requires no configuration.
+ *	- an output direction register (named "dirout") where a 1 bit
+ *	indicates the GPIO is an output.
+ *	- an input direction register (named "dirin") where a 1 bit indicates
+ *	the GPIO is an input.
  */
 static int bgpio_setup_io(struct platform_device *pdev,
 			  struct bgpio_chip *bgc)
@@ -330,6 +407,37 @@ static int bgpio_setup_io(struct platform_device *pdev,
 	return 0;
 }
 
+static int bgpio_setup_direction(struct platform_device *pdev,
+				 struct bgpio_chip *bgc)
+{
+	struct resource *res_dirout;
+	struct resource *res_dirin;
+
+	res_dirout = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						  "dirout");
+	res_dirin = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirin");
+	if (res_dirout && res_dirin) {
+		return -EINVAL;
+	} else if (res_dirout) {
+		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirout);
+		if (!bgc->reg_dir)
+			return -ENOMEM;
+		bgc->gc.direction_output = bgpio_dir_out;
+		bgc->gc.direction_input = bgpio_dir_in;
+	} else if (res_dirin) {
+		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirin);
+		if (!bgc->reg_dir)
+			return -ENOMEM;
+		bgc->gc.direction_output = bgpio_dir_out_inv;
+		bgc->gc.direction_input = bgpio_dir_in_inv;
+	} else {
+		bgc->gc.direction_output = bgpio_simple_dir_out;
+		bgc->gc.direction_input = bgpio_simple_dir_in;
+	}
+
+	return 0;
+}
+
 static int __devinit bgpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -360,11 +468,13 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 		return ret;
 
 	spin_lock_init(&bgc->lock);
+	ret = bgpio_setup_direction(pdev, bgc);
+	if (ret)
+		return ret;
+
 	bgc->data = bgc->read_reg(bgc->reg_dat);
 
 	bgc->gc.ngpio = ngpio;
-	bgc->gc.direction_input = bgpio_dir_in;
-	bgc->gc.direction_output = bgpio_dir_out;
 	bgc->gc.dev = dev;
 	bgc->gc.label = dev_name(dev);
 
-- 
1.7.4.2

Re: [PATCHv3 7/7] basic_mmio_gpio: support direction registers

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:54PM +0100, Jamie Iles wrote:
> Most controllers require the direction of a GPIO to be set by writing to
> a direction register.  Add support for either an input direction
> register or an output direction register.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Merged, thanks.

g.

> ---
>  drivers/gpio/basic_mmio_gpio.c |  114 +++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 112 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 6b99489..f4afd96 100644
> --- a/drivers/gpio/basic_mmio_gpio.c
> +++ b/drivers/gpio/basic_mmio_gpio.c
> @@ -70,6 +70,7 @@ struct bgpio_chip {
>  	void __iomem *reg_dat;
>  	void __iomem *reg_set;
>  	void __iomem *reg_clr;
> +	void __iomem *reg_dir;
>  
>  	/* Number of bits (GPIOs): <register width> * 8. */
>  	int bits;
> @@ -88,6 +89,9 @@ struct bgpio_chip {
>  
>  	/* Shadowed data register to clear/set bits safely. */
>  	unsigned long data;
> +
> +	/* Shadowed direction registers to clear/set direction safely. */
> +	unsigned long dir;
>  };
>  
>  static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
> @@ -203,15 +207,80 @@ static void bgpio_set_set(struct gpio_chip *gc, unsigned int gpio, int val)
>  	spin_unlock_irqrestore(&bgc->lock, flags);
>  }
>  
> +static int bgpio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	return 0;
> +}
> +
> +static int bgpio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio,
> +				int val)
> +{
> +	gc->set(gc, gpio, val);
> +
> +	return 0;
> +}
> +
>  static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
>  {
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	bgc->dir &= ~bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);
> +
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +
>  	return 0;
>  }
>  
>  static int bgpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
>  {
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long flags;
> +
> +	gc->set(gc, gpio, val);
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	bgc->dir |= bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);
> +
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bgpio_dir_in_inv(struct gpio_chip *gc, unsigned int gpio)
> +{
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	bgc->dir |= bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);
> +
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int bgpio_dir_out_inv(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> +	struct bgpio_chip *bgc = to_bgpio_chip(gc);
> +	unsigned long flags;
> +
>  	gc->set(gc, gpio, val);
>  
> +	spin_lock_irqsave(&bgc->lock, flags);
> +
> +	bgc->dir &= ~bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);
> +
> +	spin_unlock_irqrestore(&bgc->lock, flags);
> +
>  	return 0;
>  }
>  
> @@ -274,6 +343,14 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
>   * by clearing a bit.  For the set clr pair, this drives a 1 by setting a bit
>   * in the set register and clears it by setting a bit in the clear register.
>   * The configuration is detected by which resources are present.
> + *
> + * For setting the GPIO direction, there are three supported configurations:
> + *
> + *	- simple bidirection GPIO that requires no configuration.
> + *	- an output direction register (named "dirout") where a 1 bit
> + *	indicates the GPIO is an output.
> + *	- an input direction register (named "dirin") where a 1 bit indicates
> + *	the GPIO is an input.
>   */
>  static int bgpio_setup_io(struct platform_device *pdev,
>  			  struct bgpio_chip *bgc)
> @@ -330,6 +407,37 @@ static int bgpio_setup_io(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static int bgpio_setup_direction(struct platform_device *pdev,
> +				 struct bgpio_chip *bgc)
> +{
> +	struct resource *res_dirout;
> +	struct resource *res_dirin;
> +
> +	res_dirout = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						  "dirout");
> +	res_dirin = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirin");
> +	if (res_dirout && res_dirin) {
> +		return -EINVAL;
> +	} else if (res_dirout) {
> +		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirout);
> +		if (!bgc->reg_dir)
> +			return -ENOMEM;
> +		bgc->gc.direction_output = bgpio_dir_out;
> +		bgc->gc.direction_input = bgpio_dir_in;
> +	} else if (res_dirin) {
> +		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirin);
> +		if (!bgc->reg_dir)
> +			return -ENOMEM;
> +		bgc->gc.direction_output = bgpio_dir_out_inv;
> +		bgc->gc.direction_input = bgpio_dir_in_inv;
> +	} else {
> +		bgc->gc.direction_output = bgpio_simple_dir_out;
> +		bgc->gc.direction_input = bgpio_simple_dir_in;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __devinit bgpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -360,11 +468,13 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	spin_lock_init(&bgc->lock);
> +	ret = bgpio_setup_direction(pdev, bgc);
> +	if (ret)
> +		return ret;
> +
>  	bgc->data = bgc->read_reg(bgc->reg_dat);
>  
>  	bgc->gc.ngpio = ngpio;
> -	bgc->gc.direction_input = bgpio_dir_in;
> -	bgc->gc.direction_output = bgpio_dir_out;
>  	bgc->gc.dev = dev;
>  	bgc->gc.label = dev_name(dev);
>  
> -- 
> 1.7.4.2
> 

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

On Mon, Apr 11, 2011 at 12:21:47PM +0100, Jamie Iles wrote:
> Updated from v2 with change to use the set/dat registers for input/output
> register pair devices and removed some rebasing breakage.
> 
> Jamie Iles (7):
>   basic_mmio_gpio: remove runtime width/endianness evaluation
>   basic_mmio_gpio: convert to platform_{get,set}_drvdata()
>   basic_mmio_gpio: allow overriding number of gpio
>   basic_mmio_gpio: request register regions
>   basic_mmio_gpio: detect output method at probe time
>   basic_mmio_gpio: support different input/output registers
>   basic_mmio_gpio: support direction registers
> 
>  drivers/gpio/basic_mmio_gpio.c  |  390 +++++++++++++++++++++++++++++++--------
>  include/linux/basic_mmio_gpio.h |    1 +
>  2 files changed, 311 insertions(+), 80 deletions(-)

Hey Jamie,

Thanks a lot for putting this series together.

While on the topic of a generic mmio gpio driver, I've been thinking a
lot about things that Alan, Anton, and others have been doing, and I
took a good look at the irq_chip_generic work[1] that tglx (cc'd) put
together.

There are two things that stood out.  Alan pointed out (IIRC) that a
generic gpio driver should not require each bank to be encapsulated in
a separate struct platform_device, and after mulling over it a while I
agree.  It was also pointed out by Anton that often GPIO controllers
are embedded into other devices register addresses intertwined with
other gpio banks, or even other functions.

In parallel, tglx posted the irq_chip_generic patch[1] which has to
deal with pretty much the same set of issues.  I took a close look at
how he handled it for interrupt controllers, and I think it is
entirely appropriate to use the same pattern for creating a
gpio_mmio_generic library.

[1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/113697

So, the direction I would like to go is to split the basic_mmio_gpio
drivers into two parts;
  - a platform_driver, and
  - a gpio_mmio_generic library.

The platform driver would be responsible for parsing pdata and/or
device tree node data, but would call into the gpio_mmio_generic
library for actually registering the gpio banks.

I envision the gpio_mmio_generic library would look something like
this:

First, a structure for storing the register offsets froma  base
address:

struct gpio_mmio_regs {
};

Next, a structure for each generic mmio gpio instance:

struct gpio_mmio_generic {
	spinlock_t	lock;

	/* initialized by user */
	unsigned long	reg_data;
	unsigned long	reg_set;
	unsigned long	reg_clr;
	unsigned long	reg_dir;

	void __iomem	*reg_base;

	/* Runtime register value caches; may be initialized by user */
	u32		data_cache;
	u32		dir_cache;

	/* Embedded gpio_chip.  Helpers functions set up accessors, but user
	 * can override before calling gpio_mmio_generic_add() */
	struct gpio_chip chip;
};

And then some helpers for initializing/adding/removing the embedded gpio_chip:

void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);

gpio_mmio_generic_setup() sets up the common callback ops in the
embedded gpio_chip, but the decisions it makes could be overridden by
the user before calling gpio_mmio_generic_add().

I've not had time to prototype this yet, but I wanted to get it
written down and out onto the list for feedback since I probably won't
have any chance to get to it until after UDS.  Bonus points to anyone
who wants to take the initiative to hack it together before I get to
it.  Extra bonus points to anyone who also converts some of the other
gpio controllers to use it.  :-D

Thoughts?
g.

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

Oops, forgot to cc tglx...

On Tue, May 3, 2011 at 3:09 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Apr 11, 2011 at 12:21:47PM +0100, Jamie Iles wrote:
>> Updated from v2 with change to use the set/dat registers for input/output
>> register pair devices and removed some rebasing breakage.
>>
>> Jamie Iles (7):
>>   basic_mmio_gpio: remove runtime width/endianness evaluation
>>   basic_mmio_gpio: convert to platform_{get,set}_drvdata()
>>   basic_mmio_gpio: allow overriding number of gpio
>>   basic_mmio_gpio: request register regions
>>   basic_mmio_gpio: detect output method at probe time
>>   basic_mmio_gpio: support different input/output registers
>>   basic_mmio_gpio: support direction registers
>>
>>  drivers/gpio/basic_mmio_gpio.c  |  390 +++++++++++++++++++++++++++++++--------
>>  include/linux/basic_mmio_gpio.h |    1 +
>>  2 files changed, 311 insertions(+), 80 deletions(-)
>
> Hey Jamie,
>
> Thanks a lot for putting this series together.
>
> While on the topic of a generic mmio gpio driver, I've been thinking a
> lot about things that Alan, Anton, and others have been doing, and I
> took a good look at the irq_chip_generic work[1] that tglx (cc'd) put
> together.
>
> There are two things that stood out.  Alan pointed out (IIRC) that a
> generic gpio driver should not require each bank to be encapsulated in
> a separate struct platform_device, and after mulling over it a while I
> agree.  It was also pointed out by Anton that often GPIO controllers
> are embedded into other devices register addresses intertwined with
> other gpio banks, or even other functions.
>
> In parallel, tglx posted the irq_chip_generic patch[1] which has to
> deal with pretty much the same set of issues.  I took a close look at
> how he handled it for interrupt controllers, and I think it is
> entirely appropriate to use the same pattern for creating a
> gpio_mmio_generic library.
>
> [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/113697
>
> So, the direction I would like to go is to split the basic_mmio_gpio
> drivers into two parts;
>  - a platform_driver, and
>  - a gpio_mmio_generic library.
>
> The platform driver would be responsible for parsing pdata and/or
> device tree node data, but would call into the gpio_mmio_generic
> library for actually registering the gpio banks.
>
> I envision the gpio_mmio_generic library would look something like
> this:
>
> First, a structure for storing the register offsets froma  base
> address:
>
> struct gpio_mmio_regs {
> };
>
> Next, a structure for each generic mmio gpio instance:
>
> struct gpio_mmio_generic {
>        spinlock_t      lock;
>
>        /* initialized by user */
>        unsigned long   reg_data;
>        unsigned long   reg_set;
>        unsigned long   reg_clr;
>        unsigned long   reg_dir;
>
>        void __iomem    *reg_base;
>
>        /* Runtime register value caches; may be initialized by user */
>        u32             data_cache;
>        u32             dir_cache;
>
>        /* Embedded gpio_chip.  Helpers functions set up accessors, but user
>         * can override before calling gpio_mmio_generic_add() */
>        struct gpio_chip chip;
> };
>
> And then some helpers for initializing/adding/removing the embedded gpio_chip:
>
> void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
> int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
> void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);
>
> gpio_mmio_generic_setup() sets up the common callback ops in the
> embedded gpio_chip, but the decisions it makes could be overridden by
> the user before calling gpio_mmio_generic_add().
>
> I've not had time to prototype this yet, but I wanted to get it
> written down and out onto the list for feedback since I probably won't
> have any chance to get to it until after UDS.  Bonus points to anyone
> who wants to take the initiative to hack it together before I get to
> it.  Extra bonus points to anyone who also converts some of the other
> gpio controllers to use it.  :-D

And triple bonus points to anyone who thinks this is stupid and comes
up with a better approach.  :-)

g.

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

On Tue, May 3, 2011 at 3:13 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> Oops, forgot to cc tglx...
>
> On Tue, May 3, 2011 at 3:09 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> While on the topic of a generic mmio gpio driver, I've been thinking a
>> lot about things that Alan, Anton, and others have been doing, and I
>> took a good look at the irq_chip_generic work[1] that tglx (cc'd) put
>> together.
>>
>> There are two things that stood out.  Alan pointed out (IIRC) that a
>> generic gpio driver should not require each bank to be encapsulated in
>> a separate struct platform_device, and after mulling over it a while I
>> agree.  It was also pointed out by Anton that often GPIO controllers
>> are embedded into other devices register addresses intertwined with
>> other gpio banks, or even other functions.
>>
>> In parallel, tglx posted the irq_chip_generic patch[1] which has to
>> deal with pretty much the same set of issues.  I took a close look at
>> how he handled it for interrupt controllers, and I think it is
>> entirely appropriate to use the same pattern for creating a
>> gpio_mmio_generic library.
>>
>> [1] http://comments.gmane.org/gmane.linux.ports.arm.kernel/113697
>>
>> So, the direction I would like to go is to split the basic_mmio_gpio
>> drivers into two parts;
>>  - a platform_driver, and
>>  - a gpio_mmio_generic library.
>>
>> The platform driver would be responsible for parsing pdata and/or
>> device tree node data, but would call into the gpio_mmio_generic
>> library for actually registering the gpio banks.
>>
>> I envision the gpio_mmio_generic library would look something like
>> this:
>>
>> First, a structure for storing the register offsets froma  base
>> address:
>>
>> struct gpio_mmio_regs {
>> };

Gah, I'm doing real well this afternoon.  Ignore the above structure.
I intended to delete it from the email.

>>
>> Next, a structure for each generic mmio gpio instance:
>>
>> struct gpio_mmio_generic {
>>        spinlock_t      lock;
>>
>>        /* initialized by user */
>>        unsigned long   reg_data;
>>        unsigned long   reg_set;
>>        unsigned long   reg_clr;
>>        unsigned long   reg_dir;

And these are intended to be register offsets from the base address;
however, since the caller is responsible for ioremapping anyway, this
could just as easily be direct pointers to the registers.

>>
>>        void __iomem    *reg_base;
>>
>>        /* Runtime register value caches; may be initialized by user */
>>        u32             data_cache;
>>        u32             dir_cache;
>>
>>        /* Embedded gpio_chip.  Helpers functions set up accessors, but user
>>         * can override before calling gpio_mmio_generic_add() */
>>        struct gpio_chip chip;
>> };
>>
>> And then some helpers for initializing/adding/removing the embedded gpio_chip:
>>
>> void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
>> int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
>> void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);
>>
>> gpio_mmio_generic_setup() sets up the common callback ops in the
>> embedded gpio_chip, but the decisions it makes could be overridden by
>> the user before calling gpio_mmio_generic_add().
>>
>> I've not had time to prototype this yet, but I wanted to get it
>> written down and out onto the list for feedback since I probably won't
>> have any chance to get to it until after UDS.  Bonus points to anyone
>> who wants to take the initiative to hack it together before I get to
>> it.  Extra bonus points to anyone who also converts some of the other
>> gpio controllers to use it.  :-D
>
> And triple bonus points to anyone who thinks this is stupid and comes
> up with a better approach.  :-)
>
> g.
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Tue, May 03, 2011 at 03:13:20PM -0600, Grant Likely wrote:
> > struct gpio_mmio_generic {
> >        spinlock_t      lock;
> >
> >        /* initialized by user */
> >        unsigned long   reg_data;
> >        unsigned long   reg_set;
> >        unsigned long   reg_clr;
> >        unsigned long   reg_dir;
> >
> >        void __iomem    *reg_base;

This assumes that all reg_* will be mapped with a single ioremap().
My solution (see below) doesn't have this issue.

> > void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
> > int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
> > void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);

I'm not sure you need that complex API.

> > gpio_mmio_generic_setup() sets up the common callback ops in the
> > embedded gpio_chip, but the decisions it makes could be overridden by
> > the user before calling gpio_mmio_generic_add().
> >
> > I've not had time to prototype this yet, but I wanted to get it
> > written down and out onto the list for feedback since I probably won't
> > have any chance to get to it until after UDS.  Bonus points to anyone
> > who wants to take the initiative to hack it together before I get to
> > it.  Extra bonus points to anyone who also converts some of the other
> > gpio controllers to use it.  :-D
> 
> And triple bonus points to anyone who thinks this is stupid and comes
> up with a better approach.  :-)

This isn't stupid. And I started working on this, so what about
http://lkml.org/lkml/2011/4/19/401 ? This is pretty much the same
that you propose.

Thanks.

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

On Wed, May 04, 2011 at 01:52:11AM +0400, Anton Vorontsov wrote:
> On Tue, May 03, 2011 at 03:13:20PM -0600, Grant Likely wrote:
> > > struct gpio_mmio_generic {
> > >        spinlock_t      lock;
> > >
> > >        /* initialized by user */
> > >        unsigned long   reg_data;
> > >        unsigned long   reg_set;
> > >        unsigned long   reg_clr;
> > >        unsigned long   reg_dir;
> > >
> > >        void __iomem    *reg_base;
> 
> This assumes that all reg_* will be mapped with a single ioremap().
> My solution (see below) doesn't have this issue.
> 
> > > void gpio_mmio_generic_setup(struct gpio_mmio_generic *gmg, int register_width);
> > > int gpio_mmio_generic_add(struct gpio_mmio_generic *gmg);
> > > void gpio_mmio_generic_remove(struct gpio_mmio_generic *gmg);
> 
> I'm not sure you need that complex API.
> 
> > > gpio_mmio_generic_setup() sets up the common callback ops in the
> > > embedded gpio_chip, but the decisions it makes could be overridden by
> > > the user before calling gpio_mmio_generic_add().
> > >
> > > I've not had time to prototype this yet, but I wanted to get it
> > > written down and out onto the list for feedback since I probably won't
> > > have any chance to get to it until after UDS.  Bonus points to anyone
> > > who wants to take the initiative to hack it together before I get to
> > > it.  Extra bonus points to anyone who also converts some of the other
> > > gpio controllers to use it.  :-D
> > 
> > And triple bonus points to anyone who thinks this is stupid and comes
> > up with a better approach.  :-)
> 
> This isn't stupid. And I started working on this, so what about
> http://lkml.org/lkml/2011/4/19/401 ? This is pretty much the same
> that you propose.

The advantage that Grant's proposal has though is that the user can 
override the gpio_chip callbacks.  When I tried porting over some 
existing ARM platforms, one of the blocking issues was that lots of 
platforms had some annoying small detail that was slightly different 
(such as doing muxing in the _get() callback or needing a to_irq 
callback).

If we make bgpio_chip public and return that from bgpio_probe 
unregistered then the calling code can override some of the methods then 
register the gpio_chip.

As a slight aside, if we don't want a platform_device per bank for 
devices with multiple banks then I don't think the named resource 
approach will work (at least I can't see a particularly nice mechanism).  
Any ideas?

Jamie

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
[...]
> The advantage that Grant's proposal has though is that the user can 
> override the gpio_chip callbacks.  When I tried porting over some 
> existing ARM platforms, one of the blocking issues was that lots of 
> platforms had some annoying small detail that was slightly different 
> (such as doing muxing in the _get() callback or needing a to_irq 
> callback).
> 
> If we make bgpio_chip public and return that from bgpio_probe 
> unregistered then the calling code can override some of the methods then 
> register the gpio_chip.

Oh, that makes sense, right.

> As a slight aside, if we don't want a platform_device per bank for 
> devices with multiple banks then I don't think the named resource 
> approach will work (at least I can't see a particularly nice mechanism).  
> Any ideas?

I think Grant misunderstood Alan's words. If a PCI device registers
platform devices to represent each of PCI device's banks -- that is not
good. It's waste of devices, complicates sysfs/device heirarchy and so
on. And that's why bgpio_probe() thing started, to not create platform
devices when you already have one.

But personally I think it's OK for platforms (arch/ code) to register
each bank as a separate device. In some cases, that describes hardware
even better. And that makes life easier for device-tree stuff as well.

And if you really don't want this behaviour for your platform, you can
create your own driver that would use "bgpio library", and would
register several banks for a single device (as in PCI case).

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

On Wed, May 04, 2011 at 02:34:15AM +0400, Anton Vorontsov wrote:
> On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
> [...]
> > The advantage that Grant's proposal has though is that the user can 
> > override the gpio_chip callbacks.  When I tried porting over some 
> > existing ARM platforms, one of the blocking issues was that lots of 
> > platforms had some annoying small detail that was slightly different 
> > (such as doing muxing in the _get() callback or needing a to_irq 
> > callback).
> > 
> > If we make bgpio_chip public and return that from bgpio_probe 
> > unregistered then the calling code can override some of the methods then 
> > register the gpio_chip.
> 
> Oh, that makes sense, right.
> 
> > As a slight aside, if we don't want a platform_device per bank for 
> > devices with multiple banks then I don't think the named resource 
> > approach will work (at least I can't see a particularly nice mechanism).  
> > Any ideas?
> 
> I think Grant misunderstood Alan's words. If a PCI device registers
> platform devices to represent each of PCI device's banks -- that is not
> good. It's waste of devices, complicates sysfs/device heirarchy and so
> on. And that's why bgpio_probe() thing started, to not create platform
> devices when you already have one.

Actually, I did understand what Alan was suggesting.  If I gave the
impression that existing platform devices should be consolidated into
single devices, regardless of whether or not they were related, then
that was not my intent.

*however*, for devices that do implement a multi-function register
block, I do think it is better to have a single driver perform a
single ioremap and then register the N interfaces that use it against
a single device.  I certainly don't see this as a hard and fast rule,
but it is definitely my preference.

> 
> But personally I think it's OK for platforms (arch/ code) to register
> each bank as a separate device. In some cases, that describes hardware
> even better. And that makes life easier for device-tree stuff as well.

>From the device tree use-case, I personally still prefer a binding
that provides a single 'reg' entry for the register block and explicit
offsets in the binding to specify where/how the gpio registers are
layed out.  It just fits better with existing binding practices.

Also, if you're talking about a gpio device with, say, 128 gpios on an
soc, then the natural binding probably will be to have a single device
tree node covering all 4 banks because that is the way the
documentation lays out the device.  Perhaps something like this
(completely off the top of my head):

gpio@fedc0000 {
	compatible = "acme,super-soc-gpio", "mmio-gpio";
	reg = <0xfedc0000 0x100>;
	gpio-controller;
	#gpio-cells = <1>;

	mmgpio-regoffset-data = <0x00 0x04 0x08 0x0c>;
	mmgpio-regoffset-dir  = <0x20 0x24 0x28 0x2c>;
	mmgpio-regoffset-set  = <0x10 0x14 0x18 0x1c>;
	mmgpio-regoffset-clr  = <0x30 0x34 0x38 0x3c>;
};

... where an array of regoffset values allows for multiple banks.
Although this might be completely insane and it would be better to
make the kernel key directly off the 'acme,super-soc-gpio' value.

> And if you really don't want this behaviour for your platform, you can
> create your own driver that would use "bgpio library", and would
> register several banks for a single device (as in PCI case).

Exactly.

g.

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Tue, May 03, 2011 at 06:00:55PM -0600, Grant Likely wrote:
[...]
> From the device tree use-case, I personally still prefer a binding
> that provides a single 'reg' entry for the register block and explicit
> offsets in the binding to specify where/how the gpio registers are
> layed out.  It just fits better with existing binding practices.

Hm. AFAIK, it's quite the reverse. Existing device-tree bindings
practices prefer per-bank device bindings. Ie. MPC8xxx, CPM, QE,
Simple GPIOs, etc.

> Also, if you're talking about a gpio device with, say, 128 gpios on an
> soc, then the natural binding probably will be to have a single device
> tree node covering all 4 banks because that is the way the
> documentation lays out the device.  Perhaps something like this
> (completely off the top of my head):
> 
> gpio@fedc0000 {
> 	compatible = "acme,super-soc-gpio", "mmio-gpio";
> 	reg = <0xfedc0000 0x100>;
> 	gpio-controller;
> 	#gpio-cells = <1>;
> 
> 	mmgpio-regoffset-data = <0x00 0x04 0x08 0x0c>;
> 	mmgpio-regoffset-dir  = <0x20 0x24 0x28 0x2c>;
> 	mmgpio-regoffset-set  = <0x10 0x14 0x18 0x1c>;
> 	mmgpio-regoffset-clr  = <0x30 0x34 0x38 0x3c>;
> };
> 
> ... where an array of regoffset values allows for multiple banks.
> Although this might be completely insane and it would be better to
> make the kernel key directly off the 'acme,super-soc-gpio' value.

Oh, I really don't understand why you advocate this approach. It
gives us nothing, except maybe saving a few lines in the device
tree, but in return you abuse hierarchy (you flatten it). From the
hardware point of view, it's even worse -- the bindings would not
let us describe bank's power properties, sleep/wake behaviour etc.
Or this will lead to something like the ugly
mmgpio-sleep = <0 1 1 1>; maps.

I understand that with bgpio library both approaches may easily
coexist, so I'm mostly talking about device tree bindings here.

IMO, the thing we should approach with device tree is these bindings
(example from arch/powerpc/boot/dts/mpc832x_rdb.dts):

                par_io@1400 {
                        #address-cells = <1>;
                        #size-cells = <1>;
                        reg = <0x1400 0x100>;
                        ranges = <3 0x1448 0x18>;
                        compatible = "fsl,mpc8323-qe-pario";

                        qe_pio_d: gpio-controller@1448 {
                                #gpio-cells = <2>;
                                compatible = "fsl,mpc8323-qe-pario-bank";
                                reg = <3 0x18>;
                                gpio-controller;
                        };

			... more banks here ...
                }

Note that in this case bank's reg specifies the whole registers range,
which should be split into several reg entries (inside the reg = <>,
not reg-stuff1, reg-stuff2 thing).

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

On Wed, May 04, 2011 at 02:34:15AM +0400, Anton Vorontsov wrote:
> On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
> [...]
> > The advantage that Grant's proposal has though is that the user can 
> > override the gpio_chip callbacks.  When I tried porting over some 
> > existing ARM platforms, one of the blocking issues was that lots of 
> > platforms had some annoying small detail that was slightly different 
> > (such as doing muxing in the _get() callback or needing a to_irq 
> > callback).
> > 
> > If we make bgpio_chip public and return that from bgpio_probe 
> > unregistered then the calling code can override some of the methods then 
> > register the gpio_chip.
> 
> Oh, that makes sense, right.

I've just given this a try and it largely works, but it's probably 
better if we allow bgpio_chip to be embedded in other structures.  For 
example, the langwell driver has a gpio_to_irq callback that we would 
need to get the IRQ base for the bank.  We could add a void *priv member 
to bgpio_chip but that doesn't feel quite right.

So,
	int bgpio_init(struct bgpio_chip *bgc, struct device *dev, 
		       unsigned long sz, void __iomem *dat, ...)

rather than a probe() that returns the bgpio_chip?

Jamie

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Wed, May 04, 2011 at 12:09:39PM +0100, Jamie Iles wrote:
> On Wed, May 04, 2011 at 02:34:15AM +0400, Anton Vorontsov wrote:
> > On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
> > [...]
> > > The advantage that Grant's proposal has though is that the user can 
> > > override the gpio_chip callbacks.  When I tried porting over some 
> > > existing ARM platforms, one of the blocking issues was that lots of 
> > > platforms had some annoying small detail that was slightly different 
> > > (such as doing muxing in the _get() callback or needing a to_irq 
> > > callback).
> > > 
> > > If we make bgpio_chip public and return that from bgpio_probe 
> > > unregistered then the calling code can override some of the methods then 
> > > register the gpio_chip.
> > 
> > Oh, that makes sense, right.
> 
> I've just given this a try and it largely works, but it's probably 
> better if we allow bgpio_chip to be embedded in other structures.  For 
> example, the langwell driver has a gpio_to_irq callback that we would 
> need to get the IRQ base for the bank.  We could add a void *priv member 
> to bgpio_chip but that doesn't feel quite right.
> 
> So,
> 	int bgpio_init(struct bgpio_chip *bgc, struct device *dev, 
> 		       unsigned long sz, void __iomem *dat, ...)
> 
> rather than a probe() that returns the bgpio_chip?

Sounds good to me.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Wed, May 04, 2011 at 03:31:31PM +0400, Anton Vorontsov wrote:
> On Wed, May 04, 2011 at 12:09:39PM +0100, Jamie Iles wrote:
> > On Wed, May 04, 2011 at 02:34:15AM +0400, Anton Vorontsov wrote:
> > > On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
> > > [...]
> > > > The advantage that Grant's proposal has though is that the user can 
> > > > override the gpio_chip callbacks.  When I tried porting over some 
> > > > existing ARM platforms, one of the blocking issues was that lots of 
> > > > platforms had some annoying small detail that was slightly different 
> > > > (such as doing muxing in the _get() callback or needing a to_irq 
> > > > callback).
> > > > 
> > > > If we make bgpio_chip public and return that from bgpio_probe 
> > > > unregistered then the calling code can override some of the methods then 
> > > > register the gpio_chip.
> > > 
> > > Oh, that makes sense, right.
> > 
> > I've just given this a try and it largely works, but it's probably 
> > better if we allow bgpio_chip to be embedded in other structures.  For 
> > example, the langwell driver has a gpio_to_irq callback that we would 
> > need to get the IRQ base for the bank.  We could add a void *priv member 
> > to bgpio_chip but that doesn't feel quite right.
> > 
> > So,
> > 	int bgpio_init(struct bgpio_chip *bgc, struct device *dev, 
> > 		       unsigned long sz, void __iomem *dat, ...)
> > 
> > rather than a probe() that returns the bgpio_chip?
> 
> Sounds good to me.

OK, so here's what I've got so far (patches attached).  I've updated the 
basic_mmio_gpio library with your initial lkml patch and updated it to 
allow bgpio_chip to be embedded in another structure.  I've also 
attempted to convert over the bt8xx and langwell drivers but they're a 
little rough around the edges in places (and untested as I don't have 
the hardware).

Jamie

[-- Attachment #2: 0001-basic_mmio_gpio-split-into-a-gpio-library-and-platfo.patch --]
[-- Type: text/plain, Size: 14587 bytes --]

>From 21f25152816acfb145daebbaeb75f33d70c7ec53 Mon Sep 17 00:00:00 2001
From: Jamie Iles <jamie@jamieiles.com>
Date: Wed, 4 May 2011 14:27:58 +0100
Subject: [PATCH 1/3] basic_mmio_gpio: split into a gpio library and platform
 device

Allow GPIO_BASIC_MMIO_CORE to be used to provide an accessor library
for implementing GPIO drivers whilst abstracting the register access
detail.  Based on a patch from Anton Vorontsov[1] and adapted to allow
bgpio_chip to be embedded in another structure.

1. https://lkml.org/lkml/2011/4/19/401
---
 drivers/gpio/Kconfig            |    6 +
 drivers/gpio/Makefile           |    1 +
 drivers/gpio/basic_mmio_gpio.c  |  317 +++++++++++++++++++++------------------
 include/linux/basic_mmio_gpio.h |   55 +++++++
 4 files changed, 231 insertions(+), 148 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b7fac15..ca16a30 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -70,8 +70,14 @@ config GPIO_MAX730X
 
 comment "Memory mapped GPIO drivers:"
 
+config GPIO_BASIC_MMIO_CORE
+	tristate
+	help
+	  Provides core functionality for basic memory-mapped GPIO controllers.
+
 config GPIO_BASIC_MMIO
 	tristate "Basic memory-mapped GPIO controllers support"
+	select GPIO_BASIC_MMIO_CORE
 	help
 	  Say yes here to support basic memory-mapped GPIO controllers.
 
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index cfbdef1..91a295d 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_GPIOLIB)		+= gpiolib.o
 
 obj-$(CONFIG_GPIO_ADP5520)	+= adp5520-gpio.o
 obj-$(CONFIG_GPIO_ADP5588)	+= adp5588-gpio.o
+obj-$(CONFIG_GPIO_BASIC_MMIO_CORE)	+= basic_mmio_gpio.o
 obj-$(CONFIG_GPIO_BASIC_MMIO)	+= basic_mmio_gpio.o
 obj-$(CONFIG_GPIO_LANGWELL)	+= langwell_gpio.o
 obj-$(CONFIG_GPIO_MAX730X)	+= max730x.o
diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index b2ec45f..6b1c6e5 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -45,6 +45,7 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
  */
 
 #include <linux/init.h>
+#include <linux/err.h>
 #include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -61,44 +62,6 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/mod_devicetable.h>
 #include <linux/basic_mmio_gpio.h>
 
-struct bgpio_chip {
-	struct gpio_chip gc;
-
-	unsigned long (*read_reg)(void __iomem *reg);
-	void (*write_reg)(void __iomem *reg, unsigned long data);
-
-	void __iomem *reg_dat;
-	void __iomem *reg_set;
-	void __iomem *reg_clr;
-	void __iomem *reg_dir;
-
-	/* Number of bits (GPIOs): <register width> * 8. */
-	int bits;
-
-	/*
-	 * Some GPIO controllers work with the big-endian bits notation,
-	 * e.g. in a 8-bits register, GPIO7 is the least significant bit.
-	 */
-	unsigned long (*pin2mask)(struct bgpio_chip *bgc, unsigned int pin);
-
-	/*
-	 * Used to lock bgpio_chip->data. Also, this is needed to keep
-	 * shadowed and real data registers writes together.
-	 */
-	spinlock_t lock;
-
-	/* Shadowed data register to clear/set bits safely. */
-	unsigned long data;
-
-	/* Shadowed direction registers to clear/set direction safely. */
-	unsigned long dir;
-};
-
-static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
-{
-	return container_of(gc, struct bgpio_chip, gc);
-}
-
 static void bgpio_write8(void __iomem *reg, unsigned long data)
 {
 	writeb(data, reg);
@@ -284,20 +247,10 @@ static int bgpio_dir_out_inv(struct gpio_chip *gc, unsigned int gpio, int val)
 	return 0;
 }
 
-static void __iomem *bgpio_request_and_map(struct device *dev,
-					   struct resource *res)
-{
-	if (!devm_request_mem_region(dev, res->start, resource_size(res),
-				     res->name ?: "mmio_gpio"))
-		return NULL;
-
-	return devm_ioremap(dev, res->start, resource_size(res));
-}
-
-static int bgpio_setup_accessors(struct platform_device *pdev,
-				 struct bgpio_chip *bgc)
+static int bgpio_setup_accessors(struct device *dev,
+				 struct bgpio_chip *bgc,
+				 bool be)
 {
-	const struct platform_device_id *platid = platform_get_device_id(pdev);
 
 	switch (bgc->bits) {
 	case 8:
@@ -319,13 +272,11 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
 		break;
 #endif /* BITS_PER_LONG >= 64 */
 	default:
-		dev_err(&pdev->dev, "unsupported data width %u bits\n",
-			bgc->bits);
+		dev_err(dev, "unsupported data width %u bits\n", bgc->bits);
 		return -EINVAL;
 	}
 
-	bgc->pin2mask = strcmp(platid->name, "basic-mmio-gpio-be") ?
-		bgpio_pin2mask : bgpio_pin2mask_be;
+	bgc->pin2mask = be ? bgpio_pin2mask_be : bgpio_pin2mask;
 
 	return 0;
 }
@@ -352,51 +303,22 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
  *	- an input direction register (named "dirin") where a 1 bit indicates
  *	the GPIO is an input.
  */
-static int bgpio_setup_io(struct platform_device *pdev,
-			  struct bgpio_chip *bgc)
+static int bgpio_setup_io(struct bgpio_chip *bgc,
+			  void __iomem *dat,
+			  void __iomem *set,
+			  void __iomem *clr)
 {
-	struct resource *res_set;
-	struct resource *res_clr;
-	struct resource *res_dat;
-	resource_size_t dat_sz;
 
-	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
-	if (!res_dat)
-		return -EINVAL;
-
-	dat_sz = resource_size(res_dat);
-	if (!is_power_of_2(dat_sz))
-		return -EINVAL;
-
-	bgc->bits = dat_sz * 8;
-	if (bgc->bits > BITS_PER_LONG)
-		return -EINVAL;
-
-	bgc->reg_dat = bgpio_request_and_map(&pdev->dev, res_dat);
+	bgc->reg_dat = dat;
 	if (!bgc->reg_dat)
-		return -ENOMEM;
-
-	res_set = platform_get_resource_byname(pdev, IORESOURCE_MEM, "set");
-	res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
-	if (res_set && res_clr) {
-		if (resource_size(res_set) != resource_size(res_clr) ||
-		    resource_size(res_set) != resource_size(res_dat))
-			return -EINVAL;
-
-		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
-		bgc->reg_clr = bgpio_request_and_map(&pdev->dev, res_clr);
-		if (!bgc->reg_set || !bgc->reg_clr)
-			return -ENOMEM;
+		return -EINVAL;
 
+	if (set && clr) {
+		bgc->reg_set = set;
+		bgc->reg_clr = clr;
 		bgc->gc.set = bgpio_set_with_clear;
-	} else if (res_set && !res_clr) {
-		if (resource_size(res_set) != resource_size(res_dat))
-			return -EINVAL;
-
-		bgc->reg_set = bgpio_request_and_map(&pdev->dev, res_set);
-		if (!bgc->reg_set)
-			return -ENOMEM;
-
+	} else if (set && !clr) {
+		bgc->reg_set = set;
 		bgc->gc.set = bgpio_set_set;
 	} else {
 		bgc->gc.set = bgpio_set;
@@ -407,27 +329,18 @@ static int bgpio_setup_io(struct platform_device *pdev,
 	return 0;
 }
 
-static int bgpio_setup_direction(struct platform_device *pdev,
-				 struct bgpio_chip *bgc)
+static int bgpio_setup_direction(struct bgpio_chip *bgc,
+				 void __iomem *dirout,
+				 void __iomem *dirin)
 {
-	struct resource *res_dirout;
-	struct resource *res_dirin;
-
-	res_dirout = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						  "dirout");
-	res_dirin = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirin");
-	if (res_dirout && res_dirin) {
+	if (dirout && dirin) {
 		return -EINVAL;
-	} else if (res_dirout) {
-		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirout);
-		if (!bgc->reg_dir)
-			return -ENOMEM;
+	} else if (dirout) {
+		bgc->reg_dir = dirout;
 		bgc->gc.direction_output = bgpio_dir_out;
 		bgc->gc.direction_input = bgpio_dir_in;
-	} else if (res_dirin) {
-		bgc->reg_dir = bgpio_request_and_map(&pdev->dev, res_dirin);
-		if (!bgc->reg_dir)
-			return -ENOMEM;
+	} else if (dirin) {
+		bgc->reg_dir = dirin;
 		bgc->gc.direction_output = bgpio_dir_out_inv;
 		bgc->gc.direction_input = bgpio_dir_in_inv;
 	} else {
@@ -438,60 +351,166 @@ static int bgpio_setup_direction(struct platform_device *pdev,
 	return 0;
 }
 
-static int __devinit bgpio_probe(struct platform_device *pdev)
+int __devexit bgpio_remove(struct bgpio_chip *bgc)
+{
+	int err = gpiochip_remove(&bgc->gc);
+
+	kfree(bgc);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(bgpio_remove);
+
+int __devinit bgpio_init(struct bgpio_chip *bgc,
+			 struct device *dev,
+			 unsigned long sz,
+			 void __iomem *dat,
+			 void __iomem *set,
+			 void __iomem *clr,
+			 void __iomem *dirout,
+			 void __iomem *dirin,
+			 bool big_endian)
 {
-	struct device *dev = &pdev->dev;
-	struct bgpio_pdata *pdata = dev_get_platdata(dev);
-	struct bgpio_chip *bgc;
 	int ret;
-	int ngpio;
 
-	bgc = devm_kzalloc(dev, sizeof(*bgc), GFP_KERNEL);
-	if (!bgc)
-		return -ENOMEM;
+	if (!is_power_of_2(sz))
+		return -EINVAL;
 
-	ret = bgpio_setup_io(pdev, bgc);
+	bgc->bits = sz * 8;
+	if (bgc->bits > BITS_PER_LONG)
+		return -EINVAL;
+
+	spin_lock_init(&bgc->lock);
+	bgc->gc.dev = dev;
+	bgc->gc.label = dev_name(dev);
+	bgc->gc.base = -1;
+	bgc->gc.ngpio = bgc->bits;
+
+	ret = bgpio_setup_io(bgc, dat, set, clr);
 	if (ret)
 		return ret;
 
-	ngpio = bgc->bits;
-	if (pdata) {
-		bgc->gc.base = pdata->base;
-		if (pdata->ngpio > 0)
-			ngpio = pdata->ngpio;
-	} else {
-		bgc->gc.base = -1;
-	}
-
-	ret = bgpio_setup_accessors(pdev, bgc);
+	ret = bgpio_setup_accessors(dev, bgc, big_endian);
 	if (ret)
 		return ret;
 
-	spin_lock_init(&bgc->lock);
-	ret = bgpio_setup_direction(pdev, bgc);
+	ret = bgpio_setup_direction(bgc, dirout, dirin);
 	if (ret)
 		return ret;
 
 	bgc->data = bgc->read_reg(bgc->reg_dat);
 
-	bgc->gc.ngpio = ngpio;
-	bgc->gc.dev = dev;
-	bgc->gc.label = dev_name(dev);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bgpio_init);
 
-	platform_set_drvdata(pdev, bgc);
+#ifdef CONFIG_GPIO_BASIC_MMIO
 
-	ret = gpiochip_add(&bgc->gc);
-	if (ret)
-		dev_err(dev, "gpiochip_add() failed: %d\n", ret);
+static void __iomem *bgpio_map(struct platform_device *pdev,
+			       const char *name,
+			       resource_size_t sane_sz,
+			       int *err)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *r;
+	resource_size_t start;
+	resource_size_t sz;
+	void __iomem *ret;
+
+	*err = 0;
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	if (!r)
+		return NULL;
+
+	sz = resource_size(r);
+	if (sz != sane_sz) {
+		*err = -EINVAL;
+		return NULL;
+	}
+
+	start = r->start;
+	if (!devm_request_mem_region(dev, start, sz, r->name)) {
+		*err = -EBUSY;
+		return NULL;
+	}
+
+	ret = devm_ioremap(dev, start, sz);
+	if (!ret) {
+		*err = -ENOMEM;
+		return NULL;
+	}
 
 	return ret;
 }
 
-static int __devexit bgpio_remove(struct platform_device *pdev)
+static int __devinit bgpio_pdev_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *r;
+	void __iomem *dat;
+	void __iomem *set;
+	void __iomem *clr;
+	void __iomem *dirout;
+	void __iomem *dirin;
+	unsigned long sz;
+	bool be;
+	int err;
+	struct bgpio_chip *bgc;
+	struct bgpio_pdata *pdata = dev_get_platdata(dev);
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
+	if (!r)
+		return -EINVAL;
+
+	sz = resource_size(r);
+
+	dat = bgpio_map(pdev, "dat", sz, &err);
+	if (!dat)
+		return err ? err : -EINVAL;
+
+	set = bgpio_map(pdev, "set", sz, &err);
+	if (err)
+		return err;
+
+	clr = bgpio_map(pdev, "clr", sz, &err);
+	if (err)
+		return err;
+
+	dirout = bgpio_map(pdev, "dirout", sz, &err);
+	if (err)
+		return err;
+
+	dirin = bgpio_map(pdev, "dirin", sz, &err);
+	if (err)
+		return err;
+
+	be = !strcmp(platform_get_device_id(pdev)->name, "basic-mmio-gpio-be");
+
+	bgc = devm_kzalloc(&pdev->dev, sizeof(*bgc), GFP_KERNEL);
+	if (!bgc)
+		return -ENOMEM;
+
+	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, be);
+	if (err)
+		return err;
+
+	if (pdata) {
+		bgc->gc.base = pdata->base;
+		if (pdata->ngpio > 0)
+			bgc->gc.ngpio = pdata->ngpio;
+	}
+
+	platform_set_drvdata(pdev, bgc);
+
+	return 0;
+}
+
+static int __devexit bgpio_pdev_remove(struct platform_device *pdev)
 {
 	struct bgpio_chip *bgc = platform_get_drvdata(pdev);
 
-	return gpiochip_remove(&bgc->gc);
+	return bgpio_remove(bgc);
 }
 
 static const struct platform_device_id bgpio_id_table[] = {
@@ -506,21 +525,23 @@ static struct platform_driver bgpio_driver = {
 		.name = "basic-mmio-gpio",
 	},
 	.id_table = bgpio_id_table,
-	.probe = bgpio_probe,
-	.remove = __devexit_p(bgpio_remove),
+	.probe = bgpio_pdev_probe,
+	.remove = __devexit_p(bgpio_pdev_remove),
 };
 
-static int __init bgpio_init(void)
+static int __init bgpio_platform_init(void)
 {
 	return platform_driver_register(&bgpio_driver);
 }
-module_init(bgpio_init);
+module_init(bgpio_platform_init);
 
-static void __exit bgpio_exit(void)
+static void __exit bgpio_platform_exit(void)
 {
 	platform_driver_unregister(&bgpio_driver);
 }
-module_exit(bgpio_exit);
+module_exit(bgpio_platform_exit);
+
+#endif /* CONFIG_GPIO_BASIC_MMIO */
 
 MODULE_DESCRIPTION("Driver for basic memory-mapped GPIO controllers");
 MODULE_AUTHOR("Anton Vorontsov <cbouatmailru@gmail.com>");
diff --git a/include/linux/basic_mmio_gpio.h b/include/linux/basic_mmio_gpio.h
index f23ec73..1ae1271 100644
--- a/include/linux/basic_mmio_gpio.h
+++ b/include/linux/basic_mmio_gpio.h
@@ -13,9 +13,64 @@
 #ifndef __BASIC_MMIO_GPIO_H
 #define __BASIC_MMIO_GPIO_H
 
+#include <linux/gpio.h>
+#include <linux/types.h>
+#include <linux/compiler.h>
+
 struct bgpio_pdata {
 	int base;
 	int ngpio;
 };
 
+struct device;
+
+struct bgpio_chip {
+	struct gpio_chip gc;
+
+	unsigned long (*read_reg)(void __iomem *reg);
+	void (*write_reg)(void __iomem *reg, unsigned long data);
+
+	void __iomem *reg_dat;
+	void __iomem *reg_set;
+	void __iomem *reg_clr;
+	void __iomem *reg_dir;
+
+	/* Number of bits (GPIOs): <register width> * 8. */
+	int bits;
+
+	/*
+	 * Some GPIO controllers work with the big-endian bits notation,
+	 * e.g. in a 8-bits register, GPIO7 is the least significant bit.
+	 */
+	unsigned long (*pin2mask)(struct bgpio_chip *bgc, unsigned int pin);
+
+	/*
+	 * Used to lock bgpio_chip->data. Also, this is needed to keep
+	 * shadowed and real data registers writes together.
+	 */
+	spinlock_t lock;
+
+	/* Shadowed data register to clear/set bits safely. */
+	unsigned long data;
+
+	/* Shadowed direction registers to clear/set direction safely. */
+	unsigned long dir;
+};
+
+static inline struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
+{
+	return container_of(gc, struct bgpio_chip, gc);
+}
+
+int __devexit bgpio_remove(struct bgpio_chip *bgc);
+int __devinit bgpio_init(struct bgpio_chip *bgc,
+			 struct device *dev,
+			 unsigned long sz,
+			 void __iomem *dat,
+			 void __iomem *set,
+			 void __iomem *clr,
+			 void __iomem *dirout,
+			 void __iomem *dirin,
+			 bool big_endian);
+
 #endif /* __BASIC_MMIO_GPIO_H */
-- 
1.7.4.4


[-- Attachment #3: 0002-gpio-bt8xxgpio-convert-to-use-basic_mmio_gpio-librar.patch --]
[-- Type: text/plain, Size: 5728 bytes --]

>From 34a2b274369fb136d2111a0d5747f42daeb3a9b9 Mon Sep 17 00:00:00 2001
From: Jamie Iles <jamie@jamieiles.com>
Date: Wed, 4 May 2011 14:29:40 +0100
Subject: [PATCH 2/3] gpio/bt8xxgpio: convert to use basic_mmio_gpio library

Use the basic_mmio_gpio library for the register accesses.  The driver
itself is now only concerned with PCI and hardware initialisation.
---
 drivers/gpio/Kconfig     |    1 +
 drivers/gpio/bt8xxgpio.c |  119 ++++++++++------------------------------------
 2 files changed, 26 insertions(+), 94 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ca16a30..898cdb2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -332,6 +332,7 @@ config GPIO_CS5535
 config GPIO_BT8XX
 	tristate "BT8XX GPIO abuser"
 	depends on PCI && VIDEO_BT848=n
+	select GPIO_BASIC_MMIO_CORE
 	help
 	  The BT8xx frame grabber chip has 24 GPIO pins than can be abused
 	  as a cheap PCI GPIO card.
diff --git a/drivers/gpio/bt8xxgpio.c b/drivers/gpio/bt8xxgpio.c
index aa4f09a..63ed0cb 100644
--- a/drivers/gpio/bt8xxgpio.c
+++ b/drivers/gpio/bt8xxgpio.c
@@ -43,11 +43,13 @@
     Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
 */
 
+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/spinlock.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/basic_mmio_gpio.h>
 
 /* Steal the hardware definitions from the bttv driver. */
 #include "../media/video/bt8xx/bt848.h"
@@ -61,7 +63,7 @@ struct bt8xxgpio {
 
 	void __iomem *mmio;
 	struct pci_dev *pdev;
-	struct gpio_chip gpio;
+	struct bgpio_chip bgc;
 
 #ifdef CONFIG_PM
 	u32 saved_outen;
@@ -77,101 +79,23 @@ static int modparam_gpiobase = -1/* dynamic */;
 module_param_named(gpiobase, modparam_gpiobase, int, 0444);
 MODULE_PARM_DESC(gpiobase, "The GPIO number base. -1 means dynamic, which is the default.");
 
-
-static int bt8xxgpio_gpio_direction_input(struct gpio_chip *gpio, unsigned nr)
+static int bt8xxgpio_gpio_setup(struct bt8xxgpio *bg)
 {
-	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
-	unsigned long flags;
-	u32 outen, data;
-
-	spin_lock_irqsave(&bg->lock, flags);
-
-	data = bgread(BT848_GPIO_DATA);
-	data &= ~(1 << nr);
-	bgwrite(data, BT848_GPIO_DATA);
-
-	outen = bgread(BT848_GPIO_OUT_EN);
-	outen &= ~(1 << nr);
-	bgwrite(outen, BT848_GPIO_OUT_EN);
-
-	spin_unlock_irqrestore(&bg->lock, flags);
-
-	return 0;
-}
-
-static int bt8xxgpio_gpio_get(struct gpio_chip *gpio, unsigned nr)
-{
-	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
-	unsigned long flags;
-	u32 val;
-
-	spin_lock_irqsave(&bg->lock, flags);
-	val = bgread(BT848_GPIO_DATA);
-	spin_unlock_irqrestore(&bg->lock, flags);
-
-	return !!(val & (1 << nr));
-}
-
-static int bt8xxgpio_gpio_direction_output(struct gpio_chip *gpio,
-					unsigned nr, int val)
-{
-	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
-	unsigned long flags;
-	u32 outen, data;
-
-	spin_lock_irqsave(&bg->lock, flags);
-
-	outen = bgread(BT848_GPIO_OUT_EN);
-	outen |= (1 << nr);
-	bgwrite(outen, BT848_GPIO_OUT_EN);
+	void __iomem *dir_out = bg->mmio + BT848_GPIO_OUT_EN;
+	void __iomem *dat = bg->mmio + BT848_GPIO_DATA;
+	int err;
 
-	data = bgread(BT848_GPIO_DATA);
-	if (val)
-		data |= (1 << nr);
-	else
-		data &= ~(1 << nr);
-	bgwrite(data, BT848_GPIO_DATA);
+	err = bgpio_init(&bg->bgc, &bg->pdev->dev, 4, dat, NULL, NULL,
+			 dir_out, NULL, 0);
+	if (err)
+		return err;
 
-	spin_unlock_irqrestore(&bg->lock, flags);
+	bg->bgc.gc.base = modparam_gpiobase;
+	bg->bgc.gc.ngpio = BT8XXGPIO_NR_GPIOS;
 
 	return 0;
 }
 
-static void bt8xxgpio_gpio_set(struct gpio_chip *gpio,
-			    unsigned nr, int val)
-{
-	struct bt8xxgpio *bg = container_of(gpio, struct bt8xxgpio, gpio);
-	unsigned long flags;
-	u32 data;
-
-	spin_lock_irqsave(&bg->lock, flags);
-
-	data = bgread(BT848_GPIO_DATA);
-	if (val)
-		data |= (1 << nr);
-	else
-		data &= ~(1 << nr);
-	bgwrite(data, BT848_GPIO_DATA);
-
-	spin_unlock_irqrestore(&bg->lock, flags);
-}
-
-static void bt8xxgpio_gpio_setup(struct bt8xxgpio *bg)
-{
-	struct gpio_chip *c = &bg->gpio;
-
-	c->label = dev_name(&bg->pdev->dev);
-	c->owner = THIS_MODULE;
-	c->direction_input = bt8xxgpio_gpio_direction_input;
-	c->get = bt8xxgpio_gpio_get;
-	c->direction_output = bt8xxgpio_gpio_direction_output;
-	c->set = bt8xxgpio_gpio_set;
-	c->dbg_show = NULL;
-	c->base = modparam_gpiobase;
-	c->ngpio = BT8XXGPIO_NR_GPIOS;
-	c->can_sleep = 0;
-}
-
 static int bt8xxgpio_probe(struct pci_dev *dev,
 			const struct pci_device_id *pci_id)
 {
@@ -216,18 +140,25 @@ static int bt8xxgpio_probe(struct pci_dev *dev,
 	bgwrite(0, BT848_GPIO_REG_INP);
 	bgwrite(0, BT848_GPIO_OUT_EN);
 
-	bt8xxgpio_gpio_setup(bg);
-	err = gpiochip_add(&bg->gpio);
+	err = bt8xxgpio_gpio_setup(bg);
 	if (err) {
-		printk(KERN_ERR "bt8xxgpio: Failed to register GPIOs\n");
+		printk(KERN_ERR "bt8xxgpio: Failed to init GPIOs\n");
 		goto err_release_mem;
 	}
 
+	err = gpiochip_add(&bg->bgc.gc);
+	if (err) {
+		printk(KERN_ERR "bt8xxgpio: Failed to register GPIOs\n");
+		goto err_release_bgc;
+	}
+
 	printk(KERN_INFO "bt8xxgpio: Abusing BT8xx card for GPIOs %d to %d\n",
-	       bg->gpio.base, bg->gpio.base + BT8XXGPIO_NR_GPIOS - 1);
+	       bg->bgc.gc.base, bg->bgc.gc.base + BT8XXGPIO_NR_GPIOS - 1);
 
 	return 0;
 
+err_release_bgc:
+	bgpio_remove(&bg->bgc);
 err_release_mem:
 	release_mem_region(pci_resource_start(dev, 0),
 			   pci_resource_len(dev, 0));
@@ -244,7 +175,7 @@ static void bt8xxgpio_remove(struct pci_dev *pdev)
 {
 	struct bt8xxgpio *bg = pci_get_drvdata(pdev);
 
-	gpiochip_remove(&bg->gpio);
+	bgpio_remove(&bg->bgc);
 
 	bgwrite(0, BT848_INT_MASK);
 	bgwrite(~0x0, BT848_INT_STAT);
-- 
1.7.4.4


[-- Attachment #4: 0003-gpio-langwell-convert-to-use-basic_mmio_gpio-library.patch --]
[-- Type: text/plain, Size: 10878 bytes --]

>From 2426c6da6d14c3ec95b03dbc570f4fc0baa14948 Mon Sep 17 00:00:00 2001
From: Jamie Iles <jamie@jamieiles.com>
Date: Wed, 4 May 2011 14:30:59 +0100
Subject: [PATCH 3/3] gpio/langwell: convert to use basic_mmio_gpio library

Use the basic_mmio_gpio library for register accessors.  The driver is
now only concerned with hardware initialisation and interrupts.
---
 drivers/gpio/Kconfig         |    1 +
 drivers/gpio/langwell_gpio.c |  253 ++++++++++++++++++++---------------------
 2 files changed, 124 insertions(+), 130 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 898cdb2..a0e3370 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -349,6 +349,7 @@ config GPIO_BT8XX
 config GPIO_LANGWELL
 	bool "Intel Langwell/Penwell GPIO support"
 	depends on PCI && X86
+	select GPIO_BASIC_MMIO_CORE
 	help
 	  Say Y here to support Intel Langwell/Penwell GPIO.
 
diff --git a/drivers/gpio/langwell_gpio.c b/drivers/gpio/langwell_gpio.c
index 1b06f67..a2898cd 100644
--- a/drivers/gpio/langwell_gpio.c
+++ b/drivers/gpio/langwell_gpio.c
@@ -33,6 +33,7 @@
 #include <linux/io.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/basic_mmio_gpio.h>
 
 /*
  * Langwell chip has 64 pins and thus there are 2 32bit registers to control
@@ -58,106 +59,53 @@ enum GPIO_REG {
 	GEDR,		/* edge detect result */
 };
 
-struct lnw_gpio {
-	struct gpio_chip		chip;
-	void				*reg_base;
-	spinlock_t			lock;
+struct lnw_bank {
+	struct bgpio_chip		bgc;
+	void __iomem			*grer, *gfer, *gedr;
 	unsigned			irq_base;
 };
 
-static void __iomem *gpio_reg(struct gpio_chip *chip, unsigned offset,
-			enum GPIO_REG reg_type)
-{
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
-	unsigned nreg = chip->ngpio / 32;
-	u8 reg = offset / 32;
-	void __iomem *ptr;
-
-	ptr = (void __iomem *)(lnw->reg_base + reg_type * nreg * 4 + reg * 4);
-	return ptr;
-}
-
-static int lnw_gpio_get(struct gpio_chip *chip, unsigned offset)
-{
-	void __iomem *gplr = gpio_reg(chip, offset, GPLR);
-
-	return readl(gplr) & BIT(offset % 32);
-}
-
-static void lnw_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
-{
-	void __iomem *gpsr, *gpcr;
-
-	if (value) {
-		gpsr = gpio_reg(chip, offset, GPSR);
-		writel(BIT(offset % 32), gpsr);
-	} else {
-		gpcr = gpio_reg(chip, offset, GPCR);
-		writel(BIT(offset % 32), gpcr);
-	}
-}
-
-static int lnw_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+static inline struct lnw_bank *to_lnw_bank(struct bgpio_chip *bgc)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
-	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
-	u32 value;
-	unsigned long flags;
-
-	spin_lock_irqsave(&lnw->lock, flags);
-	value = readl(gpdr);
-	value &= ~BIT(offset % 32);
-	writel(value, gpdr);
-	spin_unlock_irqrestore(&lnw->lock, flags);
-	return 0;
+	return container_of(bgc, struct lnw_bank, bgc);
 }
 
-static int lnw_gpio_direction_output(struct gpio_chip *chip,
-			unsigned offset, int value)
-{
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
-	void __iomem *gpdr = gpio_reg(chip, offset, GPDR);
-	unsigned long flags;
-
-	lnw_gpio_set(chip, offset, value);
-	spin_lock_irqsave(&lnw->lock, flags);
-	value = readl(gpdr);
-	value |= BIT(offset % 32);
-	writel(value, gpdr);
-	spin_unlock_irqrestore(&lnw->lock, flags);
-	return 0;
-}
+struct lnw_gpio {
+	struct lnw_bank			banks[3];
+	int				nr_banks;
+	void __iomem			*reg_base;
+};
 
 static int lnw_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	struct lnw_gpio *lnw = container_of(chip, struct lnw_gpio, chip);
-	return lnw->irq_base + offset;
+	struct bgpio_chip *bgc = to_bgpio_chip(chip);
+	struct lnw_bank *bank = to_lnw_bank(bgc);
+
+	return bank->irq_base + offset;
 }
 
 static int lnw_irq_type(struct irq_data *d, unsigned type)
 {
-	struct lnw_gpio *lnw = irq_data_get_irq_chip_data(d);
-	u32 gpio = d->irq - lnw->irq_base;
+	struct lnw_bank *bank = irq_data_get_irq_chip_data(d);
+	u32 gpio = d->irq - bank->irq_base;
 	unsigned long flags;
 	u32 value;
-	void __iomem *grer = gpio_reg(&lnw->chip, gpio, GRER);
-	void __iomem *gfer = gpio_reg(&lnw->chip, gpio, GFER);
 
-	if (gpio >= lnw->chip.ngpio)
+	if (gpio >= bank->bgc.gc.ngpio)
 		return -EINVAL;
-	spin_lock_irqsave(&lnw->lock, flags);
+	spin_lock_irqsave(&bank->bgc.lock, flags);
 	if (type & IRQ_TYPE_EDGE_RISING)
-		value = readl(grer) | BIT(gpio % 32);
+		value = readl(bank->grer) | BIT(gpio % 32);
 	else
-		value = readl(grer) & (~BIT(gpio % 32));
-	writel(value, grer);
+		value = readl(bank->grer) & (~BIT(gpio % 32));
+	writel(value, bank->grer);
 
 	if (type & IRQ_TYPE_EDGE_FALLING)
-		value = readl(gfer) | BIT(gpio % 32);
+		value = readl(bank->gfer) | BIT(gpio % 32);
 	else
-		value = readl(gfer) & (~BIT(gpio % 32));
-	writel(value, gfer);
-	spin_unlock_irqrestore(&lnw->lock, flags);
+		value = readl(bank->gfer) & (~BIT(gpio % 32));
+	writel(value, bank->gfer);
+	spin_unlock_irqrestore(&bank->bgc.lock, flags);
 
 	return 0;
 }
@@ -192,30 +140,101 @@ static void lnw_irq_handler(unsigned irq, struct irq_desc *desc)
 	struct irq_chip *chip = irq_data_get_irq_chip(data);
 	u32 base, gpio, mask;
 	unsigned long pending;
-	void __iomem *gedr;
+	int i;
 
 	/* check GPIO controller to check which pin triggered the interrupt */
-	for (base = 0; base < lnw->chip.ngpio; base += 32) {
-		gedr = gpio_reg(&lnw->chip, base, GEDR);
-		pending = readl(gedr);
+	for (base = 0, i = 0; i < lnw->nr_banks; base += 32, i++) {
+		struct lnw_bank *bank = &lnw->banks[i];
+
+		pending = readl(bank->gedr);
 		while (pending) {
 			gpio = __ffs(pending) - 1;
 			mask = BIT(gpio);
 			pending &= ~mask;
 			/* Clear before handling so we can't lose an edge */
-			writel(mask, gedr);
-			generic_handle_irq(lnw->irq_base + base + gpio);
+			writel(mask, bank->gedr);
+			generic_handle_irq(bank->irq_base + gpio);
 		}
 	}
 
 	chip->irq_eoi(data);
 }
 
+static int bank_init(struct lnw_bank *bank, struct device *dev,
+		     void __iomem *reg_base, int bank_nr, int ngpio)
+{
+	unsigned nreg = ngpio / 32;
+	void __iomem *dat = reg_base + GPLR * nreg * 4 + bank_nr * 4;
+	void __iomem *dirout = reg_base + GPDR * nreg * 4 + bank_nr * 4;
+	void __iomem *set = reg_base + GPSR * nreg * 4 + bank_nr * 4;
+	void __iomem *clr = reg_base + GPCR * nreg * 4 + bank_nr * 4;
+
+	bank->grer = reg_base + GRER * nreg * 4 + bank_nr * 4;
+	bank->gfer = reg_base + GFER * nreg * 4 + bank_nr * 4;
+	bank->gedr = reg_base + GEDR * nreg * 4 + bank_nr * 4;
+
+	return bgpio_init(&bank->bgc, dev, 4, dat, set, clr, dirout, NULL, 0);
+}
+
+static int lnw_init_banks(struct lnw_gpio *lnw, int nr_banks,
+			  struct device *dev, int irq_base, int gpio_base)
+{
+	int retval, i;
+	lnw->nr_banks = nr_banks;
+
+	for (i = 0; i < lnw->nr_banks; i++) {
+		struct lnw_bank *bank = &lnw->banks[i];
+		int j;
+
+		retval = bank_init(bank, dev, lnw->reg_base, i, nr_banks * 32);
+		if (retval) {
+			dev_err(dev, "langwell failed to init bank %d (%d)\n",
+				i, retval);
+			goto out_remove;
+		}
+		bank->bgc.gc.ngpio = 32;
+		bank->bgc.gc.base = gpio_base + i * 32;
+
+		if (irq_base > 0) {
+			bank->irq_base = irq_base + i * 32;
+			bank->bgc.gc.to_irq = lnw_gpio_to_irq;
+			for (j = 0; j < 32; j++) {
+				irq_set_chip_and_handler_name(
+					j + bank->irq_base, &lnw_irqchip,
+					handle_simple_irq, "demux");
+				irq_set_chip_data(j + bank->irq_base, bank);
+			}
+		}
+
+		retval = gpiochip_add(&bank->bgc.gc);
+		if (retval) {
+			dev_err(dev, "langwell gpiochip_add error %d\n",
+				retval);
+			goto out_remove;
+		}
+	}
+
+	return 0;
+
+out_remove:
+	while (--i >= 0) {
+		int j;
+		struct lnw_bank *bank = &lnw->banks[i];
+		bgpio_remove(&bank->bgc);
+		if (irq_base > 0) {
+			for (j = 0; j < 32; ++j)
+				irq_set_chip_and_handler(j + bank->irq_base,
+							 NULL, NULL);
+		}
+	}
+
+	return retval;
+}
+
 static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
-			const struct pci_device_id *id)
+				    const struct pci_device_id *id)
 {
 	void *base;
-	int i;
 	resource_size_t start, len;
 	struct lnw_gpio *lnw;
 	u32 irq_base;
@@ -259,32 +278,17 @@ static int __devinit lnw_gpio_probe(struct pci_dev *pdev,
 		retval = -ENOMEM;
 		goto err4;
 	}
+
 	lnw->reg_base = base;
-	lnw->irq_base = irq_base;
-	lnw->chip.label = dev_name(&pdev->dev);
-	lnw->chip.direction_input = lnw_gpio_direction_input;
-	lnw->chip.direction_output = lnw_gpio_direction_output;
-	lnw->chip.get = lnw_gpio_get;
-	lnw->chip.set = lnw_gpio_set;
-	lnw->chip.to_irq = lnw_gpio_to_irq;
-	lnw->chip.base = gpio_base;
-	lnw->chip.ngpio = id->driver_data;
-	lnw->chip.can_sleep = 0;
-	pci_set_drvdata(pdev, lnw);
-	retval = gpiochip_add(&lnw->chip);
-	if (retval) {
-		dev_err(&pdev->dev, "langwell gpiochip_add error %d\n", retval);
+	retval = lnw_init_banks(lnw, id->driver_data / 32, &pdev->dev,
+				irq_base, gpio_base);
+	if (retval)
 		goto err5;
-	}
+
+	pci_set_drvdata(pdev, lnw);
 	irq_set_handler_data(pdev->irq, lnw);
 	irq_set_chained_handler(pdev->irq, lnw_irq_handler);
-	for (i = 0; i < lnw->chip.ngpio; i++) {
-		irq_set_chip_and_handler_name(i + lnw->irq_base, &lnw_irqchip,
-					      handle_simple_irq, "demux");
-		irq_set_chip_data(i + lnw->irq_base, lnw);
-	}
 
-	spin_lock_init(&lnw->lock);
 	goto done;
 err5:
 	kfree(lnw);
@@ -304,11 +308,9 @@ static struct pci_driver lnw_gpio_driver = {
 	.probe		= lnw_gpio_probe,
 };
 
-
 static int __devinit wp_gpio_probe(struct platform_device *pdev)
 {
 	struct lnw_gpio *lnw;
-	struct gpio_chip *gc;
 	struct resource *rc;
 	int retval = 0;
 
@@ -327,24 +329,10 @@ static int __devinit wp_gpio_probe(struct platform_device *pdev)
 		retval = -EINVAL;
 		goto err_kmalloc;
 	}
-	spin_lock_init(&lnw->lock);
-	gc = &lnw->chip;
-	gc->label = dev_name(&pdev->dev);
-	gc->owner = THIS_MODULE;
-	gc->direction_input = lnw_gpio_direction_input;
-	gc->direction_output = lnw_gpio_direction_output;
-	gc->get = lnw_gpio_get;
-	gc->set = lnw_gpio_set;
-	gc->to_irq = NULL;
-	gc->base = 0;
-	gc->ngpio = 64;
-	gc->can_sleep = 0;
-	retval = gpiochip_add(gc);
-	if (retval) {
-		dev_err(&pdev->dev, "whitneypoint gpiochip_add error %d\n",
-								retval);
+
+	retval = lnw_init_banks(lnw, 2, &pdev->dev, -1, 0);
+	if (retval)
 		goto err_ioremap;
-	}
 	platform_set_drvdata(pdev, lnw);
 	return 0;
 err_ioremap:
@@ -357,10 +345,15 @@ err_kmalloc:
 static int __devexit wp_gpio_remove(struct platform_device *pdev)
 {
 	struct lnw_gpio *lnw = platform_get_drvdata(pdev);
-	int err;
-	err = gpiochip_remove(&lnw->chip);
-	if (err)
-		dev_err(&pdev->dev, "failed to remove gpio_chip.\n");
+	int i, err;
+
+	for (i = 0; i < lnw->nr_banks; ++i) {
+		struct lnw_bank *bank = &lnw->banks[i];
+
+		err = bgpio_remove(&bank->bgc);
+		if (err)
+			dev_err(&pdev->dev, "failed to remove gpio_chip.\n");
+	}
 	iounmap(lnw->reg_base);
 	kfree(lnw);
 	platform_set_drvdata(pdev, NULL);
-- 
1.7.4.4

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Grant Likely]

Can you repost as a proper series please?  Don't force me to muck
about with attachments.

g.

On Wed, May 4, 2011 at 8:37 AM, Jamie Iles <jamie@jamieiles.com> wrote:
> On Wed, May 04, 2011 at 03:31:31PM +0400, Anton Vorontsov wrote:
>> On Wed, May 04, 2011 at 12:09:39PM +0100, Jamie Iles wrote:
>> > On Wed, May 04, 2011 at 02:34:15AM +0400, Anton Vorontsov wrote:
>> > > On Tue, May 03, 2011 at 11:04:08PM +0100, Jamie Iles wrote:
>> > > [...]
>> > > > The advantage that Grant's proposal has though is that the user can
>> > > > override the gpio_chip callbacks.  When I tried porting over some
>> > > > existing ARM platforms, one of the blocking issues was that lots of
>> > > > platforms had some annoying small detail that was slightly different
>> > > > (such as doing muxing in the _get() callback or needing a to_irq
>> > > > callback).
>> > > >
>> > > > If we make bgpio_chip public and return that from bgpio_probe
>> > > > unregistered then the calling code can override some of the methods then
>> > > > register the gpio_chip.
>> > >
>> > > Oh, that makes sense, right.
>> >
>> > I've just given this a try and it largely works, but it's probably
>> > better if we allow bgpio_chip to be embedded in other structures.  For
>> > example, the langwell driver has a gpio_to_irq callback that we would
>> > need to get the IRQ base for the bank.  We could add a void *priv member
>> > to bgpio_chip but that doesn't feel quite right.
>> >
>> > So,
>> >     int bgpio_init(struct bgpio_chip *bgc, struct device *dev,
>> >                    unsigned long sz, void __iomem *dat, ...)
>> >
>> > rather than a probe() that returns the bgpio_chip?
>>
>> Sounds good to me.
>
> OK, so here's what I've got so far (patches attached).  I've updated the
> basic_mmio_gpio library with your initial lkml patch and updated it to
> allow bgpio_chip to be embedded in another structure.  I've also
> attempted to convert over the bt8xx and langwell drivers but they're a
> little rough around the edges in places (and untested as I don't have
> the hardware).
>
> Jamie
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Alan Cox]

> OK, so here's what I've got so far (patches attached).  I've updated the 
> basic_mmio_gpio library with your initial lkml patch and updated it to 
> allow bgpio_chip to be embedded in another structure.  I've also 
> attempted to convert over the bt8xx and langwell drivers but they're a 
> little rough around the edges in places (and untested as I don't have 
> the hardware).

Looking at the Langwell driver you replace 130 lines of code that do the
job, with 126 lines of code that do setup for a whole extra module which
makes it bigger and slower as well as much harder to maintain.

That sounds to me like for Langwell at least it is not worth doing
because all you've done is added complexity, indirection and overhead. So
NAK the Langwell one.

The bt8xx looks a nice example of one of the cases where it will help
however.

Alan

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

Hi Alan,

On Wed, May 04, 2011 at 03:44:40PM +0100, Alan Cox wrote:
> > OK, so here's what I've got so far (patches attached).  I've updated the 
> > basic_mmio_gpio library with your initial lkml patch and updated it to 
> > allow bgpio_chip to be embedded in another structure.  I've also 
> > attempted to convert over the bt8xx and langwell drivers but they're a 
> > little rough around the edges in places (and untested as I don't have 
> > the hardware).
> 
> Looking at the Langwell driver you replace 130 lines of code that do the
> job, with 126 lines of code that do setup for a whole extra module which
> makes it bigger and slower as well as much harder to maintain.
> 
> That sounds to me like for Langwell at least it is not worth doing
> because all you've done is added complexity, indirection and overhead. So
> NAK the Langwell one.

I picked that one because I thought it might convert nicely, but it 
didn't!  I posted it anyway in case I'm missing a better way to do it 
and to give a fair representation.

With regards to the ARM drivers, lots of these aren't currently devices 
(in the driver model) so converting many of these would be net code 
increase but I suspect that it's probably worth it in these cases 
(providing that's the only complexity).

> The bt8xx looks a nice example of one of the cases where it will help
> however.

Yes, and I suspect there are a few others too.

Jamie

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Wed, May 04, 2011 at 03:37:57PM +0100, Jamie Iles wrote:
[...]
> +	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, be);
> +	if (err)
> +		return err;
> +
> +	if (pdata) {
> +		bgc->gc.base = pdata->base;
> +		if (pdata->ngpio > 0)
> +			bgc->gc.ngpio = pdata->ngpio;
> +	}
> +
> +	platform_set_drvdata(pdev, bgc);
> +
> +	return 0;

return gpio_chip_add(&bgc->gc)? Or bgpio_chip_add(bgc);

Otherwise this looks great. Feel free to add my

Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Jamie Iles]

On Wed, May 04, 2011 at 07:02:34PM +0400, Anton Vorontsov wrote:
> On Wed, May 04, 2011 at 03:37:57PM +0100, Jamie Iles wrote:
> [...]
> > +	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, be);
> > +	if (err)
> > +		return err;
> > +
> > +	if (pdata) {
> > +		bgc->gc.base = pdata->base;
> > +		if (pdata->ngpio > 0)
> > +			bgc->gc.ngpio = pdata->ngpio;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, bgc);
> > +
> > +	return 0;
> 
> return gpio_chip_add(&bgc->gc)? Or bgpio_chip_add(bgc);

Yes, this got lost in the move from bgpio_probe() to bgpio_init().  
Thanks!

Jamie

Re: [PATCHv3 0/7] gpio: extend basic_mmio_gpio for different controllers

[Anton Vorontsov]

On Wed, May 04, 2011 at 07:02:34PM +0400, Anton Vorontsov wrote:
> On Wed, May 04, 2011 at 03:37:57PM +0100, Jamie Iles wrote:
> [...]
> > +	err = bgpio_init(bgc, dev, sz, dat, set, clr, dirout, dirin, be);
> > +	if (err)
> > +		return err;
> > +
> > +	if (pdata) {
> > +		bgc->gc.base = pdata->base;
> > +		if (pdata->ngpio > 0)
> > +			bgc->gc.ngpio = pdata->ngpio;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, bgc);
> > +
> > +	return 0;
> 
> return gpio_chip_add(&bgc->gc)? Or bgpio_chip_add(bgc);
> 
> Otherwise this looks great. Feel free to add my
> 
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>

What happened w/ this patch? Jamie, would you resend it so that
Grant could apply it?

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com