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
Remove endianness/width calculations at runtime by installing function
pointers for bit-to-mask conversion and register accessors.
Reported-by: Thomas Gleixner <[email protected]>
Signed-off-by: Jamie Iles <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
Some platforms may have a number of GPIO that is less than the register
width of the peripheral.
Signed-off-by: Jamie Iles <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
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 <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
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 <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
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 <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
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 <[email protected]>
Cc: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
Use the platform drvdata helpers rather than working on the struct
device itself.
Signed-off-by: Jamie Iles <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
Cc: Grant Likely <[email protected]>
---
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
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 <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
> ---
Acked-by: Anton Vorontsov <[email protected]>
Thanks Jamie!
--
Anton Vorontsov
Email: [email protected]
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 <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
> ---
Acked-by: Anton Vorontsov <[email protected]>
Thanks,
--
Anton Vorontsov
Email: [email protected]
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 <[email protected]>
> Signed-off-by: Jamie Iles <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Cc: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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 <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> Cc: Grant Likely <[email protected]>
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
>
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.
Oops, forgot to cc tglx...
On Tue, May 3, 2011 at 3:09 PM, Grant Likely <[email protected]> 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.
On Tue, May 3, 2011 at 3:13 PM, Grant Likely <[email protected]> wrote:
> Oops, forgot to cc tglx...
>
> On Tue, May 3, 2011 at 3:09 PM, Grant Likely <[email protected]> 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.
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: [email protected]
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
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: [email protected]
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.
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: [email protected]
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
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: [email protected]
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
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 <[email protected]> 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.
> 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
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
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 <[email protected]>
Thanks!
--
Anton Vorontsov
Email: [email protected]
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
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 <[email protected]>
What happened w/ this patch? Jamie, would you resend it so that
Grant could apply it?
Thanks,
--
Anton Vorontsov
Email: [email protected]