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

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

[Jamie Iles]

I've updated the series with all of Anton's comments (thanks Anton) and
retested on the Synopsys controller with multiple banks.

Thanks,

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

-- 
1.7.4.2

[PATCHv2 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

[PATCHv2 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

[PATCHv2 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

[PATCHv2 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

[PATCHv2 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>
Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/gpio/basic_mmio_gpio.c |   88 +++++++++++++++++++++++++++-------------
 1 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 90f7f89..9be1867 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,14 +243,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;
@@ -272,23 +315,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;
-	}
-
-	spin_lock_init(&bgc->lock);
-
 	if (pdata) {
 		bgc->gc.base = pdata->base;
 		if (pdata->ngpio > 0)
@@ -302,13 +328,17 @@ 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);
 
 	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

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

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 9be1867..ed4b0c6 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;
@@ -148,13 +149,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);
@@ -254,12 +262,19 @@ 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");
@@ -280,6 +295,19 @@ 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;
+}
+
 	return 0;
 }
 
@@ -338,7 +366,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: [PATCHv2 6/7] basic_mmio_gpio: support different input/output registers

[Anton Vorontsov]

On Fri, Apr 08, 2011 at 03:16:50PM +0100, Jamie Iles wrote:
[...]
> +	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;
> +}
> +
>  	return 0;
>  }

Hm.. will this compile?

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

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

[Jamie Iles]

On Fri, Apr 08, 2011 at 06:36:41PM +0400, Anton Vorontsov wrote:
> On Fri, Apr 08, 2011 at 03:16:50PM +0100, Jamie Iles wrote:
> [...]
> > +	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;
> > +}
> > +
> >  	return 0;
> >  }
> 
> Hm.. will this compile?

No, I accidentally squashed patches 6 and 7 together then obviously 
didn't split them properly.  I'll fix this (and the spacing around the 
locking and repost).

Sorry!

Jamie

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

[Anton Vorontsov]

On Fri, Apr 08, 2011 at 03:16:50PM +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>
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/gpio/basic_mmio_gpio.c |   31 +++++++++++++++++++++++++++++--
>  1 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> index 9be1867..ed4b0c6 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;

Btw, I wonder if it makes sense (and more logical) for separate output/
input registers case to use "reg_dat" for 'input' and 'reg_set' for
output?

I.e. no need for 'reg_in', but instead modify some logic in
bgpio_setup_io(), and implement bgpio_set_set:

	if (res_dat && res_set && !res_clr) {
		bgc->gc.get = bgpio_get_dat;
		bgc->gc.set = bgpio_set_set;
	} else {
		bgc->gc.get = bgpio_get_dat;
		bgc->gc.set = bgpio_set_dat;
	}

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

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

[Jamie Iles]

On Fri, Apr 08, 2011 at 06:46:28PM +0400, Anton Vorontsov wrote:
> On Fri, Apr 08, 2011 at 03:16:50PM +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>
> > Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >  drivers/gpio/basic_mmio_gpio.c |   31 +++++++++++++++++++++++++++++--
> >  1 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
> > index 9be1867..ed4b0c6 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;
> 
> Btw, I wonder if it makes sense (and more logical) for separate output/
> input registers case to use "reg_dat" for 'input' and 'reg_set' for
> output?

Sounds like a good idea.  Something like this?

Jamie

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index 054fbee..fc3826a 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
@@ -260,6 +280,15 @@ static int bgpio_setup_io(struct platform_device *pdev,
 {
 	struct resource *res_set;
 	struct resource *res_clr;
+	struct resource *res_dat;
+
+	res_dat = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dat");
+	if (!res_dat)
+		return -EINVAL;
+
+	bgc->reg_dat = bgpio_request_and_map(&pdev->dev, res_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");
@@ -274,12 +303,22 @@ 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) ||
+		    resource_size(res_set) != bgc->bits / 8)
+			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;
 }
 
@@ -311,10 +350,6 @@ static int __devinit bgpio_probe(struct platform_device *pdev)
 	if (!bgc)
 		return -ENOMEM;
 
-	bgc->reg_dat = bgpio_request_and_map(dev, res_dat);
-	if (!bgc->reg_dat)
-		return -ENOMEM;
-
 	if (pdata) {
 		bgc->gc.base = pdata->base;
 		if (pdata->ngpio > 0)
@@ -338,7 +373,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: [PATCHv2 6/7] basic_mmio_gpio: support different input/output registers

[Anton Vorontsov]

On Fri, Apr 08, 2011 at 05:11:13PM +0100, Jamie Iles wrote:
[...]
> > > +++ 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;
> > 
> > Btw, I wonder if it makes sense (and more logical) for separate output/
> > input registers case to use "reg_dat" for 'input' and 'reg_set' for
> > output?
> 
> Sounds like a good idea.  Something like this?

Yep, exactly.

[...]
> +	} else if (res_set && !res_clr) {
> +		if (resource_size(res_set) != resource_size(res_dat) ||
> +		    resource_size(res_set) != bgc->bits / 8)

We derive bgc->bits from resource_size(res_dat), so '!= bgc->bits / 8'
condition is uneeded.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

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

[Jamie Iles]

On Fri, Apr 08, 2011 at 08:26:30PM +0400, Anton Vorontsov wrote:
> On Fri, Apr 08, 2011 at 05:11:13PM +0100, Jamie Iles wrote:
> [...]
> > > > +++ 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;
> > > 
> > > Btw, I wonder if it makes sense (and more logical) for separate output/
> > > input registers case to use "reg_dat" for 'input' and 'reg_set' for
> > > output?
> > 
> > Sounds like a good idea.  Something like this?
> 
> Yep, exactly.
> 
> [...]
> > +	} else if (res_set && !res_clr) {
> > +		if (resource_size(res_set) != resource_size(res_dat) ||
> > +		    resource_size(res_set) != bgc->bits / 8)
> 
> We derive bgc->bits from resource_size(res_dat), so '!= bgc->bits / 8'
> condition is uneeded.

Doh!  Not sure how I missed that!  I'll fix these up and repost them all 
on Monday.

Thanks,

Jamie

[PATCHv2 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 |  102 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 100 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/basic_mmio_gpio.c b/drivers/gpio/basic_mmio_gpio.c
index ed4b0c6..9e0157a 100644
--- a/drivers/gpio/basic_mmio_gpio.c
+++ b/drivers/gpio/basic_mmio_gpio.c
@@ -71,6 +71,7 @@ struct bgpio_chip {
 	void __iomem *reg_set;
 	void __iomem *reg_clr;
 	void __iomem *reg_in;
+	void __iomem *reg_dir;
 
 	/* Number of bits (GPIOs): <register width> * 8. */
 	int bits;
@@ -89,6 +90,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)
@@ -193,15 +197,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->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;
 }
 
@@ -268,6 +329,13 @@ 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 +376,34 @@ 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;
 }
 
@@ -361,11 +457,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: [PATCHv2 7/7] basic_mmio_gpio: support direction registers

[Anton Vorontsov]

On Fri, Apr 08, 2011 at 03:16:51PM +0100, Jamie Iles wrote:
[...]
>  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);

For readability, I would still add an empty line here.

> +	bgc->dir &= ~bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);

And here.

> +	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);

Ditto.

> +	bgc->dir |= bgc->pin2mask(bgc, gpio);
> +	bgc->write_reg(bgc->reg_dir, bgc->dir);

Ditto.

> +	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);

etc...

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com