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

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

[Jamie Iles]

This patch series extends the basic_mmio_gpio driver to support
controllers with different register sets and number of GPIOs.  There are
also changes to configure more at probe time rather than runtime
evaluation.

I've tested this on an out-of-tree platform with a Synopsys DesignWare
GPIO controller.

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  |  386 ++++++++++++++++++++++++++++++--------
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 305 insertions(+), 82 deletions(-)

-- 
1.7.4

[RFC PATCH 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.

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 |  140 +++++++++++++++++++++++++++-------------
 1 files changed, 94 insertions(+), 46 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 3addea6..26f84e2 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,8 @@ 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 +96,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 unsigned long bgpio_write64(void __iomem *reg, unsigned long data)
+{
+	return __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 +177,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 +193,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;
@@ -229,12 +276,13 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	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;
 
+	spin_lock_init(&bgc->lock);
+	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

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

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:10:57PM +0100, Jamie Iles wrote:
> Remove endianness/width calculations at runtime by installing function
> pointers for bit-to-mask conversion and register accessors.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>

I guess there should be Suggested-by: Thomas Gleixner <tglx@linutronix.de>

:-)

[...]
> @@ -74,7 +78,8 @@ 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);
> +
>
>       /*

No need for this new empty line, I think.

[...]
> +static unsigned long bgpio_write64(void __iomem *reg, unsigned long data)
> +{
> +	return __raw_writeq(data, reg);
> +}

I was getting this on a 64 bit machine:

  CHECK   drivers/gpio/basic_mmio_gpio.c
drivers/gpio/basic_mmio_gpio.c:138:16: warning: incorrect type in return expression (different base types)
drivers/gpio/basic_mmio_gpio.c:138:16:    expected unsigned long
drivers/gpio/basic_mmio_gpio.c:138:16:    got void
drivers/gpio/basic_mmio_gpio.c:302:33: warning: incorrect type in assignment (different base types)
drivers/gpio/basic_mmio_gpio.c:302:33:    expected void ( *write_reg )( ... )
drivers/gpio/basic_mmio_gpio.c:302:33:    got unsigned long ( static [toplevel] *<noident> )( ... )
  CC      drivers/gpio/basic_mmio_gpio.o
drivers/gpio/basic_mmio_gpio.c: In function ‘bgpio_write64’:
drivers/gpio/basic_mmio_gpio.c:138: error: void value not ignored as it ought to be
drivers/gpio/basic_mmio_gpio.c: In function ‘bgpio_setup_accessors’:
drivers/gpio/basic_mmio_gpio.c:302: warning: assignment from incompatible pointer type
make[1]: *** [drivers/gpio/basic_mmio_gpio.o] Error 1

And I fixed it with this:

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 9dad485..3ddc4b2 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -133,9 +133,9 @@ static unsigned long bgpio_read32(void __iomem *reg)
 }
 
 #if BITS_PER_LONG >= 64
-static unsigned long bgpio_write64(void __iomem *reg, unsigned long data)
+static void bgpio_write64(void __iomem *reg, unsigned long data)
 {
-	return __raw_writeq(data, reg);
+	__raw_writeq(data, reg);
 }
 
 static unsigned long bgpio_read64(void __iomem *reg)


Otherwise, the patch looks perfect.

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

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[RFC PATCH 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>
Cc: 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 26f84e2..e6cce48 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

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

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:10:58PM +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>
> 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

[RFC PATCH 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>
Cc: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c  |   17 +++++++++++------
 include/linux/basic_mmio_gpio.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index e6cce48..68f8b8b 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -240,6 +240,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)
@@ -250,6 +251,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;
 
@@ -276,6 +278,13 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	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)
@@ -283,7 +292,8 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 
 	spin_lock_init(&bgc->lock);
 	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 +301,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

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

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:10:59PM +0100, Jamie Iles wrote:
[...]
> +	if (pdata) {
> +		bgc->gc.base = pdata->base;
> +		if (pdata->ngpio > 0)
> +			ngpio = pdata->ngpio;
> +	} else
> +		bgc->gc.base = -1;

Just a minor comstic nit: 'else' case should be with braces as well.
I.e. '} else {'.

But techincally, the patch looks perfect,

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

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[RFC PATCH 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>
Cc: 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 68f8b8b..afa854b 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -193,6 +193,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)
 {
@@ -259,7 +269,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;
 
@@ -270,8 +280,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

Re: [RFC PATCH 4/7] basic_mmio_gpio: request register regions

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:11:00PM +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>
> 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

[RFC PATCH 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.

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 |   85 +++++++++++++++++++++++++++-------------
 1 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index afa854b..500eb6ae 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -162,14 +162,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)
@@ -182,6 +174,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;
@@ -189,7 +193,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;
 }
 
@@ -239,14 +244,52 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
 	return 0;
 }
 
+/*
+ * 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 resource *res_set;
+	struct resource *res_clr;
+
+	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) != bgc->bits)
+			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;
+
+		bgc->gc.set = bgpio_set_with_clear;
+	} else if (res_set || res_clr) {
+		return -EINVAL;
+	} else {
+		bgc->gc.set = bgpio_set;
+	}
+
+	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;
 	struct resource *res_dat;
-	struct resource *res_set;
-	struct resource *res_clr;
 	resource_size_t dat_sz;
 	int bits;
 	int ret;
@@ -273,21 +316,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	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) != dat_sz)
-			return -EINVAL;
-
-		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) {
-		return -EINVAL;
-	}
-
 	if (pdata) {
 		bgc->gc.base = pdata->base;
 		if (pdata->ngpio > 0)
@@ -300,6 +328,10 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = bgpio_setup_io(pdev, bgc);
+	if (ret)
+		return ret;
+
 	spin_lock_init(&bgc->lock);
 	bgc->data = bgc->read_reg(bgc->reg_dat);
 
@@ -307,7 +339,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	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

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

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:11:01PM +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.
> 
> 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

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

[Jamie Iles]

Some controllers have separate input and output registers.  For these
controllers, allow a register named "in" to be used for reading the
value of a GPIO pin.

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 |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 500eb6ae..d18a866 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_in;
 
 	/* Number of bits (GPIOs): <register width> * 8. */
 	int bits;
@@ -149,13 +150,20 @@ static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
 	return 1 << (bgc->bits - 1 - pin);
 }
 
-static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
+static int bgpio_get_dat(struct gpio_chip *gc, unsigned int gpio)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
 
 	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
 }
 
+static int bgpio_get_in(struct gpio_chip *gc, unsigned int gpio)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	return bgc->read_reg(bgc->reg_in) & 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);
@@ -255,12 +263,18 @@ 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 getting GPIO values, there are two supported configurations:
+ *
+ *	- single output/input register resource (named "dat").
+ *	- separate output/input register resources (named "dat" and "in").
  */
 static int bgpio_setup_io(struct platform_device *pdev,
 			  struct bgpio_chip *bgc)
 {
 	struct resource *res_set;
 	struct resource *res_clr;
+	struct resource *res_in;
 
 	res_set = platform_get_resource_byname(pdev, IORESOURCE_MEM, "set");
 	res_clr = platform_get_resource_byname(pdev, IORESOURCE_MEM, "clr");
@@ -281,6 +295,16 @@ static int bgpio_setup_io(struct platform_device *pdev,
 		bgc->gc.set = bgpio_set;
 	}
 
+	res_in = platform_get_resource_byname(pdev, IORESOURCE_MEM, "in");
+	if (res_in) {
+		bgc->reg_in = bgpio_request_and_map(&pdev->dev, res_in);
+		if (!bgc->reg_in)
+			return -ENOMEM;
+		bgc->gc.get = bgpio_get_in;
+	} else {
+		bgc->gc.get = bgpio_get_dat;
+	}
+
 	return 0;
 }
 
@@ -316,6 +340,8 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (!bgc->reg_dat)
 		return -ENOMEM;
 
+	spin_lock_init(&bgc->lock);
+
 	if (pdata) {
 		bgc->gc.base = pdata->base;
 		if (pdata->ngpio > 0)
@@ -338,7 +364,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

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

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:11:02PM +0100, Jamie Iles wrote:
> Some controllers have separate input and output registers.  For these
> controllers, allow a register named "in" to be used for reading the
> value of a GPIO pin.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
[...]
> @@ -316,6 +340,8 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
>  	if (!bgc->reg_dat)
>  		return -ENOMEM;
>  
> +	spin_lock_init(&bgc->lock);
> +

Hm. This looks like a stray change, now bgpio_probe() calls spin_lock_init()
twice.

Other than that, the patch seems to be OK.

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

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[RFC PATCH 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>
Cc: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |  107 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index d18a866..9dad485 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -71,6 +71,8 @@ struct bgpio_chip {
 	void __iomem *reg_set;
 	void __iomem *reg_clr;
 	void __iomem *reg_in;
+	void __iomem *reg_dirout;
+	void __iomem *reg_dirin;
 
 	/* Number of bits (GPIOs): <register width> * 8. */
 	int bits;
@@ -90,6 +92,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 outputs, inputs;
 };
 
 static struct bgpio_chip *to_bgpio_chip(struct gpio_chip *gc)
@@ -194,15 +199,72 @@ static void bgpio_set_with_clear(struct gpio_chip *gc, unsigned int gpio,
 		bgc->write_reg(bgc->reg_clr, mask);
 }
 
+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->outputs &= ~bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dirout, bgc->outputs);
+	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->outputs |= bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dirout, bgc->outputs);
+	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->inputs |= bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dirin, bgc->inputs);
+	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->inputs &= ~bgc->pin2mask(bgc, gpio);
+	bgc->write_reg(bgc->reg_dirin, bgc->inputs);
+	spin_unlock_irqrestore(&bgc->lock, flags);
+
 	return 0;
 }
 
@@ -268,6 +330,14 @@ static int bgpio_setup_accessors(struct platform_device *pdev,
  *
  *	- single output/input register resource (named "dat").
  *	- separate output/input register resources (named "dat" and "in").
+ *
+ * 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)
@@ -308,6 +378,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_dirout = bgpio_request_and_map(&pdev->dev, res_dirout);
+		if (!bgc->reg_dirout)
+			return -ENOMEM;
+		bgc->gc.direction_output = bgpio_dir_out;
+		bgc->gc.direction_input = bgpio_dir_in;
+	} else if (res_dirin) {
+		bgc->reg_dirin = bgpio_request_and_map(&pdev->dev, res_dirin);
+		if (!bgc->reg_dirin)
+			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;
@@ -358,12 +459,14 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = bgpio_setup_direction(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.dev = dev;
 	bgc->gc.label = dev_name(dev);
 
-- 
1.7.4

Re: [RFC PATCH 7/7] basic_mmio_gpio: support direction registers

[Anton Vorontsov]

On Wed, Apr 06, 2011 at 12:11:03PM +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>
> Cc: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
[...]
> @@ -71,6 +71,8 @@ struct bgpio_chip {
>       void __iomem *reg_set;
>       void __iomem *reg_clr;
>       void __iomem *reg_in;
> +     void __iomem *reg_dirout;
> +     void __iomem *reg_dirin;
>

I guess you don't need both reg_dirout and reg_dirin in the runtime.
How about just renaming it to "reg_dir" and just assinging it with
either dirout or dirin in bgpio_setup_direction()?

[...]
> +	/* Shadowed direction registers to clear/set direction safely. */
> +	unsigned long outputs, inputs;

Same as obove, maybe just a single 'dir' variable?

Plus, a minor nit: the coding style suggests:

	unsigned long outputs;
	unsigned long inputs;

[...]
>  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->outputs &= ~bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dirout, bgc->outputs);
> +	spin_unlock_irqrestore(&bgc->lock, flags);
>

Because of the lock, the code in these routines is dense and hard to
read, so I would rather add empty lines near the locking calls, just
like in bgpio_set() (also makes it look consistent).

Otherwise,

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

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [RFC PATCH 7/7] basic_mmio_gpio: support direction registers

[Jamie Iles]

Hi Anton,

On Wed, Apr 06, 2011 at 04:12:25PM +0400, Anton Vorontsov wrote:
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

Thanks for going through these, I'll make all of the fixes you've 
suggested - I don't disagree with any of them and repost.

Jamie