[PATCH 0/2] gpio: Fix gpio-generic bug and add driver for GRGPIO cores

[PATCH 0/2] gpio: Fix gpio-generic bug and add driver for GRGPIO cores

[Andreas Larsson]

The second patch is v2 of the earlier single GRGPIO patch

Andreas Larsson (2):
  gpio: gpio-generic: Fix bug in big endian bit conversion
  gpio: Add device driver for GRGPIO cores

 drivers/gpio/Kconfig        |    8 ++
 drivers/gpio/Makefile       |    1 +
 drivers/gpio/gpio-generic.c |    5 +-
 drivers/gpio/gpio-grgpio.c  |  244 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 257 insertions(+), 1 deletions(-)
 create mode 100644 drivers/gpio/gpio-grgpio.c

[PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Andreas Larsson]

The swap to convert LE to BE in bgpio_pin2mask_be should be on byte level, not
on bit level.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
 drivers/gpio/gpio-generic.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
index 05fcc0f..7f11537 100644
--- a/drivers/gpio/gpio-generic.c
+++ b/drivers/gpio/gpio-generic.c
@@ -112,7 +112,10 @@ static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
 static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
 				       unsigned int pin)
 {
-	return 1 << (bgc->bits - 1 - pin);
+	unsigned int bit = pin & 0x7; /* Bit number within byte */
+	unsigned int base = pin - bit; /* Pin that is bit 0 within byte */
+
+	return 1 << ((bgc->bits - base - 8) + bit); /* shifted base + bit */
 }
 
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
-- 
1.7.0.4

[PATCH 2/2] gpio: Add device driver for GRGPIO cores

[Andreas Larsson]

This driver supports GRGPIO gpio cores available in the GRLIB VHDL IP core
library.

Signed-off-by: Andreas Larsson <andreas@gaisler.com>
---
v2 of the eariler single GRGPIO patch

 drivers/gpio/Kconfig       |    8 ++
 drivers/gpio/Makefile      |    1 +
 drivers/gpio/gpio-grgpio.c |  244 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-grgpio.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 682de75..8437ab7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -298,6 +298,14 @@ config GPIO_GE_FPGA
 	  and write pin state) for GPIO implemented in a number of GE single
 	  board computers.
 
+config GPIO_GRGPIO
+	tristate "Aeroflex Gaisler GRGPIO support"
+	depends on OF
+	select GPIO_GENERIC
+	help
+	  Select this to support Aeroflex Gaisler GRGPIO cores from the GRLIB
+	  VHDL IP core library.
+
 comment "I2C GPIO expanders:"
 
 config GPIO_ARIZONA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c5aebd0..ca5d7a3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_ARCH_DAVINCI)	+= gpio-davinci.o
 obj-$(CONFIG_GPIO_EM)		+= gpio-em.o
 obj-$(CONFIG_GPIO_EP93XX)	+= gpio-ep93xx.o
 obj-$(CONFIG_GPIO_GE_FPGA)	+= gpio-ge.o
+obj-$(CONFIG_GPIO_GRGPIO)	+= gpio-grgpio.o
 obj-$(CONFIG_GPIO_ICH)		+= gpio-ich.o
 obj-$(CONFIG_GPIO_IT8761E)	+= gpio-it8761e.o
 obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
diff --git a/drivers/gpio/gpio-grgpio.c b/drivers/gpio/gpio-grgpio.c
new file mode 100644
index 0000000..87eca73
--- /dev/null
+++ b/drivers/gpio/gpio-grgpio.c
@@ -0,0 +1,244 @@
+/*
+ * Driver for Aeroflex Gaisler GRGPIO General Purpose I/O cores.
+ *
+ * 2013 (c) Aeroflex Gaisler AB
+ *
+ * This driver supports the GRGPIO GPIO core available in the GRLIB VHDL IP core
+ * library.
+ *
+ * Full documentation of the GRGPIO core can be found here:
+ * http://www.gaisler.com/products/grlib/grip.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ * Contributors: Andreas Larsson <andreas@gaisler.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_irq.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/irq.h>
+#include <linux/basic_mmio_gpio.h>
+
+#define DRV_NAME	"grgpio"
+
+#define GRGPIO_MAX_NGPIO 32
+
+struct grgpio_regs {
+	u32 data;	/* 0x00 */
+	u32 output;	/* 0x04 */
+	u32 dir;	/* 0x08 */
+	u32 imask;	/* 0x0c */
+	u32 ipol;	/* 0x10  */
+	u32 iedge;	/* 0x14 */
+	u32 bypass;	/* 0x18 */
+	u32 __reserved;	/* 0x1c */
+	u32 imap[8];	/* 0x20-0x3c */
+};
+
+struct grgpio_priv {
+	struct bgpio_chip bgc;
+	struct grgpio_regs __iomem *regs;
+
+	u32 imask;	/* irq mask shadow register */
+	s32 *irqmap;	/* maps offset to irq or -1 if no irq */
+};
+
+static inline struct grgpio_priv *grgpio_gc_to_priv(struct gpio_chip *gc)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	return container_of(bgc, struct grgpio_priv, bgc);
+}
+
+static void grgpio_set_imask(struct grgpio_priv *priv, unsigned int offset,
+			     int val)
+{
+	struct bgpio_chip *bgc = &priv->bgc;
+	unsigned long mask = bgc->pin2mask(bgc, offset);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	if (val)
+		priv->imask |= mask;
+	else
+		priv->imask &= ~mask;
+	bgc->write_reg(&priv->regs->imask, priv->imask);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static int grgpio_to_irq(struct gpio_chip *gc, unsigned offset)
+{
+	struct grgpio_priv *priv = grgpio_gc_to_priv(gc);
+	int index, irq;
+
+	if (!priv->irqmap || offset > gc->ngpio)
+		return -ENXIO;
+
+	index = priv->irqmap[offset];
+	if (index < 0)
+		return -ENXIO;
+
+	irq = irq_of_parse_and_map(priv->bgc.gc.dev->of_node, index);
+	if (irq) {
+		/* Enable interrupt and return irq */
+		grgpio_set_imask(priv, offset, 1);
+		return irq;
+	} else {
+		return -ENXIO;
+	}
+}
+
+static void grgpio_free(struct gpio_chip *gc, unsigned offset)
+{
+	struct grgpio_priv *priv = grgpio_gc_to_priv(gc);
+	struct bgpio_chip *bgc = &priv->bgc;
+	unsigned long mask = bgc->pin2mask(bgc, offset);
+
+	if (unlikely(priv->imask & mask))
+		grgpio_set_imask(priv, offset, 0);
+}
+
+static int grgpio_probe(struct platform_device *ofdev)
+{
+	struct device_node *np = ofdev->dev.of_node;
+	struct grgpio_regs __iomem *regs;
+	struct gpio_chip *gc;
+	struct bgpio_chip *bgc;
+	struct grgpio_priv *priv;
+	struct resource *res;
+	int err, i, size;
+	u32 prop;
+	s32 *irqmap;
+	char *label;
+
+	priv = devm_kzalloc(&ofdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&ofdev->dev, "Could not allocate priv\n");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(ofdev, IORESOURCE_MEM, 0);
+	regs = devm_request_and_ioremap(&ofdev->dev, res);
+	if (!regs) {
+		dev_err(&ofdev->dev, "Couldn't map registers\n");
+		return -EADDRNOTAVAIL;
+	}
+
+	bgc = &priv->bgc;
+	err = bgpio_init(bgc, &ofdev->dev, 4, &regs->data, &regs->output, NULL,
+			 &regs->dir, NULL, BGPIOF_BIG_ENDIAN);
+	if (err) {
+		dev_err(&ofdev->dev, "bgpio_init() failed\n");
+		return err;
+	}
+
+	priv->regs = regs;
+	priv->imask = bgc->read_reg(&regs->imask);
+
+	gc = &bgc->gc;
+	gc->of_node = np;
+	gc->owner = THIS_MODULE;
+	gc->to_irq = grgpio_to_irq;
+	gc->free = grgpio_free;
+
+	err = of_property_read_u32(np, "base", &prop);
+	if (err) {
+		dev_dbg(&ofdev->dev, "No base property: use dynamic base\n");
+		gc->base = -1;
+	} else {
+		gc->base = prop;
+	}
+
+	err = of_property_read_u32(np, "nbits", &prop);
+	if (err || prop <= 0 || prop > GRGPIO_MAX_NGPIO) {
+		gc->ngpio = GRGPIO_MAX_NGPIO;
+		dev_dbg(&ofdev->dev,
+			"No or invalid nbits property: assume %d\n", gc->ngpio);
+	} else {
+		gc->ngpio = prop;
+	}
+
+	irqmap = (s32 *)of_get_property(np, "irqmap", &size);
+	if (irqmap) {
+		if (size < gc->ngpio) {
+			dev_err(&ofdev->dev,
+				"irqmap shorter than ngpio (%d < %d)\n",
+				size, gc->ngpio);
+			return -EINVAL;
+		}
+
+		priv->irqmap = devm_kzalloc(&ofdev->dev,
+					    gc->ngpio * sizeof(s32),
+					    GFP_KERNEL);
+		if (!priv->irqmap) {
+			dev_err(&ofdev->dev,
+				"Could not allocate memory for irqmap\n");
+			return -ENOMEM;
+		}
+		for (i = 0; i < gc->ngpio; i++)
+			priv->irqmap[i] = irqmap[i];
+	} else {
+		dev_dbg(&ofdev->dev, "No irqmap\n");
+	}
+
+	label = kstrdup(np->full_name, GFP_KERNEL);
+	if (label)
+		gc->label = label;
+
+	platform_set_drvdata(ofdev, priv);
+
+	err = gpiochip_add(gc);
+	if (err) {
+		dev_err(&ofdev->dev, "Couldn't add gpiochip\n");
+		return err;
+	}
+
+	dev_info(&ofdev->dev, "regs=0x%p, base=%d, npgio=%d\n",
+		 priv->regs, gc->base, gc->ngpio);
+
+	return 0;
+}
+
+static int grgpio_remove(struct platform_device *ofdev)
+{
+	dev_set_drvdata(&ofdev->dev, NULL);
+
+	return 0;
+}
+
+static struct of_device_id grgpio_match[] = {
+	{.name = "GAISLER_GPIO"},
+	{.name = "01_01a"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, grgpio_match);
+
+static struct platform_driver grgpio_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = grgpio_match,
+	},
+	.probe = grgpio_probe,
+	.remove = grgpio_remove,
+};
+
+module_platform_driver(grgpio_driver);
+
+MODULE_AUTHOR("Aeroflex Gaisler AB.");
+MODULE_DESCRIPTION("Driver for Aeroflex Gaisler GRGPIO");
+MODULE_LICENSE("GPL");
-- 
1.7.0.4

Re: [PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Grant Likely]

On Tue,  5 Feb 2013 11:33:02 +0100, Andreas Larsson <andreas@gaisler.com> wrote:
> The swap to convert LE to BE in bgpio_pin2mask_be should be on byte level, not
> on bit level.
> 
> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> ---
>  drivers/gpio/gpio-generic.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> index 05fcc0f..7f11537 100644
> --- a/drivers/gpio/gpio-generic.c
> +++ b/drivers/gpio/gpio-generic.c
> @@ -112,7 +112,10 @@ static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
>  static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
>  				       unsigned int pin)
>  {
> -	return 1 << (bgc->bits - 1 - pin);
> +	unsigned int bit = pin & 0x7; /* Bit number within byte */
> +	unsigned int base = pin - bit; /* Pin that is bit 0 within byte */
> +
> +	return 1 << ((bgc->bits - base - 8) + bit); /* shifted base + bit */

Ah, sorry for my previous reply. I see you have seen gpio-generic.  :-)

No, the original calculation is correct. BE and LE bit numbering are
opposite, bit Linux always uses LE numbers as far as bit masks are
concerned. Therefore pin 0 is the most significant bit, and
pin (nr_bits-1) is the least significant bit.

g.

Re: [PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Grant Likely]

On Sat, 09 Feb 2013 14:58:55 +0000, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue,  5 Feb 2013 11:33:02 +0100, Andreas Larsson <andreas@gaisler.com> wrote:
> > The swap to convert LE to BE in bgpio_pin2mask_be should be on byte level, not
> > on bit level.
> > 
> > Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> > ---
> >  drivers/gpio/gpio-generic.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> > index 05fcc0f..7f11537 100644
> > --- a/drivers/gpio/gpio-generic.c
> > +++ b/drivers/gpio/gpio-generic.c
> > @@ -112,7 +112,10 @@ static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
> >  static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
> >  				       unsigned int pin)
> >  {
> > -	return 1 << (bgc->bits - 1 - pin);
> > +	unsigned int bit = pin & 0x7; /* Bit number within byte */
> > +	unsigned int base = pin - bit; /* Pin that is bit 0 within byte */
> > +
> > +	return 1 << ((bgc->bits - base - 8) + bit); /* shifted base + bit */
> 
> Ah, sorry for my previous reply. I see you have seen gpio-generic.  :-)
> 
> No, the original calculation is correct. BE and LE bit numbering are
> opposite, bit Linux always uses LE numbers as far as bit masks are
> concerned. Therefore pin 0 is the most significant bit, and
> pin (nr_bits-1) is the least significant bit.

Hi Andreas,

Actually I'm wrong here (at least partially) after looking closer at the
generic gpio driver. The problem is that things get messed up on 16 or
32 bit access depending on how the hardware expects to be accessed.
bgpio always uses iowrite/ioread for register access which is inherently
little endian, but if the hardware is wired up as a big-endian device
then you have to do the fiddly bit you did above to get the bit
positions to line up where you what then. So, there could potentially be
4 different ways to count bits on a value ioread() from a gpio register:

little-endian, LSB = 0 (sane)
little-endian, MSB = 0 (odd)
big-endian (bytes swapped), MSB = 0 (common on BE platforms)
big-endian (bytes swapped), LSB = 0 (why are you making my life hard?)

We /could/ have a ioread32be/write32be mode in the driver, but I don't
think that is the right approach. It means we need yet another set of
accessors for register except using the 'be' variants. Blech. What I'd
actually like to do is add a bitmask field to the gpio_desc which can be
calculated ahead of time to whatever madness is required from the way
the device is wired. Then the access routines don't need to even care.
they just apply the bitmask to ioread/iowrite and it doesn't even need
to know what the bit number actually is. The new support for gpio_desc
in the core code makes this feasable.

g.

Re: [PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Andreas Larsson]

On 2013-02-26 08:52, Grant Likely wrote:
> On Sat, 09 Feb 2013 14:58:55 +0000, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Tue,  5 Feb 2013 11:33:02 +0100, Andreas Larsson <andreas@gaisler.com> wrote:
>>> The swap to convert LE to BE in bgpio_pin2mask_be should be on byte level, not
>>> on bit level.
>>>
>>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
>>> ---
>>>   drivers/gpio/gpio-generic.c |    5 ++++-
>>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
>>> index 05fcc0f..7f11537 100644
>>> --- a/drivers/gpio/gpio-generic.c
>>> +++ b/drivers/gpio/gpio-generic.c
>>> @@ -112,7 +112,10 @@ static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
>>>   static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
>>>   				       unsigned int pin)
>>>   {
>>> -	return 1 << (bgc->bits - 1 - pin);
>>> +	unsigned int bit = pin & 0x7; /* Bit number within byte */
>>> +	unsigned int base = pin - bit; /* Pin that is bit 0 within byte */
>>> +
>>> +	return 1 << ((bgc->bits - base - 8) + bit); /* shifted base + bit */
>>
>> Ah, sorry for my previous reply. I see you have seen gpio-generic.  :-)
>>
>> No, the original calculation is correct. BE and LE bit numbering are
>> opposite, bit Linux always uses LE numbers as far as bit masks are
>> concerned. Therefore pin 0 is the most significant bit, and
>> pin (nr_bits-1) is the least significant bit.
>
> Hi Andreas,
>
> Actually I'm wrong here (at least partially) after looking closer at the
> generic gpio driver. The problem is that things get messed up on 16 or
> 32 bit access depending on how the hardware expects to be accessed.
> bgpio always uses iowrite/ioread for register access which is inherently
> little endian, but if the hardware is wired up as a big-endian device
> then you have to do the fiddly bit you did above to get the bit
> positions to line up where you what then. So, there could potentially be
> 4 different ways to count bits on a value ioread() from a gpio register:
>
> little-endian, LSB = 0 (sane)
> little-endian, MSB = 0 (odd)
> big-endian (bytes swapped), MSB = 0 (common on BE platforms)
> big-endian (bytes swapped), LSB = 0 (why are you making my life hard?)

Yes, in v4 of my patch I solved it using custom accessors set outside of 
gpio-generic internally using ioread32be/iowrite32be instead of adding a 
whole set of be variants in gpio-generic.


> We /could/ have a ioread32be/write32be mode in the driver, but I don't
> think that is the right approach. It means we need yet another set of
> accessors for register except using the 'be' variants. Blech. What I'd
> actually like to do is add a bitmask field to the gpio_desc which can be
> calculated ahead of time to whatever madness is required from the way
> the device is wired. Then the access routines don't need to even care.
> they just apply the bitmask to ioread/iowrite and it doesn't even need
> to know what the bit number actually is. The new support for gpio_desc
> in the core code makes this feasable.

I am not sure I understand what you mean here or what new support for 
gpio_desc you are referring to (looking in gpio/next at 
git://git.secretlab.ca/git/linux-2.6).

Do you mean to add something like an 'unsigned long bitmask[64]' bitmap 
array with one bitmap for each gpio line to struct gpio_desc and use 
this primarily by gpio-generic.c, populated in bgpio_init? Is gpio_desc 
now available outside of gpiolib.c in some repository/branch that I 
might be unaware of?

Cheers,
Andreas

Re: [PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Andreas Larsson]

On 2013-02-26 11:46, Andreas Larsson wrote:
> On 2013-02-26 08:52, Grant Likely wrote:
>> On Sat, 09 Feb 2013 14:58:55 +0000, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>> We /could/ have a ioread32be/write32be mode in the driver, but I don't
>> think that is the right approach. It means we need yet another set of
>> accessors for register except using the 'be' variants. Blech. What I'd
>> actually like to do is add a bitmask field to the gpio_desc which can be
>> calculated ahead of time to whatever madness is required from the way
>> the device is wired. Then the access routines don't need to even care.
>> they just apply the bitmask to ioread/iowrite and it doesn't even need
>> to know what the bit number actually is. The new support for gpio_desc
>> in the core code makes this feasable.
>
> I am not sure I understand what you mean here or what new support for
> gpio_desc you are referring to (looking in gpio/next at
> git://git.secretlab.ca/git/linux-2.6).
>
> Do you mean to add something like an 'unsigned long bitmask[64]' bitmap
> array with one bitmap for each gpio line to struct gpio_desc and use
> this primarily by gpio-generic.c, populated in bgpio_init? Is gpio_desc
> now available outside of gpiolib.c in some repository/branch that I
> might be unaware of?

Ah, I realize that it is the "gpiolib: remove gpio_desc[] static array" 
patch series you refer to, not yet pushed to gpio/next at 
git://git.secretlab.ca/git/linux-2.6.

The question on how you intend the bitmap field to be 
defined/used/populated remains.

Cheers,
Andreas

Re: [PATCH 1/2] gpio: gpio-generic: Fix bug in big endian bit conversion

[Grant Likely]

On Tue, 26 Feb 2013 11:46:31 +0100, Andreas Larsson <andreas@gaisler.com> wrote:
> On 2013-02-26 08:52, Grant Likely wrote:
> > On Sat, 09 Feb 2013 14:58:55 +0000, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> On Tue,  5 Feb 2013 11:33:02 +0100, Andreas Larsson <andreas@gaisler.com> wrote:
> >>> The swap to convert LE to BE in bgpio_pin2mask_be should be on byte level, not
> >>> on bit level.
> >>>
> >>> Signed-off-by: Andreas Larsson <andreas@gaisler.com>
> >>> ---
> >>>   drivers/gpio/gpio-generic.c |    5 ++++-
> >>>   1 files changed, 4 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/gpio/gpio-generic.c b/drivers/gpio/gpio-generic.c
> >>> index 05fcc0f..7f11537 100644
> >>> --- a/drivers/gpio/gpio-generic.c
> >>> +++ b/drivers/gpio/gpio-generic.c
> >>> @@ -112,7 +112,10 @@ static unsigned long bgpio_pin2mask(struct bgpio_chip *bgc, unsigned int pin)
> >>>   static unsigned long bgpio_pin2mask_be(struct bgpio_chip *bgc,
> >>>   				       unsigned int pin)
> >>>   {
> >>> -	return 1 << (bgc->bits - 1 - pin);
> >>> +	unsigned int bit = pin & 0x7; /* Bit number within byte */
> >>> +	unsigned int base = pin - bit; /* Pin that is bit 0 within byte */
> >>> +
> >>> +	return 1 << ((bgc->bits - base - 8) + bit); /* shifted base + bit */
> >>
> >> Ah, sorry for my previous reply. I see you have seen gpio-generic.  :-)
> >>
> >> No, the original calculation is correct. BE and LE bit numbering are
> >> opposite, bit Linux always uses LE numbers as far as bit masks are
> >> concerned. Therefore pin 0 is the most significant bit, and
> >> pin (nr_bits-1) is the least significant bit.
> >
> > Hi Andreas,
> >
> > Actually I'm wrong here (at least partially) after looking closer at the
> > generic gpio driver. The problem is that things get messed up on 16 or
> > 32 bit access depending on how the hardware expects to be accessed.
> > bgpio always uses iowrite/ioread for register access which is inherently
> > little endian, but if the hardware is wired up as a big-endian device
> > then you have to do the fiddly bit you did above to get the bit
> > positions to line up where you what then. So, there could potentially be
> > 4 different ways to count bits on a value ioread() from a gpio register:
> >
> > little-endian, LSB = 0 (sane)
> > little-endian, MSB = 0 (odd)
> > big-endian (bytes swapped), MSB = 0 (common on BE platforms)
> > big-endian (bytes swapped), LSB = 0 (why are you making my life hard?)
> 
> Yes, in v4 of my patch I solved it using custom accessors set outside of 
> gpio-generic internally using ioread32be/iowrite32be instead of adding a 
> whole set of be variants in gpio-generic.

Until we've got something better, please do add the be accessor versions
to gpio-generic. I'm not thrilled with the need for them, but it is
still better than coding your own version.

> > We /could/ have a ioread32be/write32be mode in the driver, but I don't
> > think that is the right approach. It means we need yet another set of
> > accessors for register except using the 'be' variants. Blech. What I'd
> > actually like to do is add a bitmask field to the gpio_desc which can be
> > calculated ahead of time to whatever madness is required from the way
> > the device is wired. Then the access routines don't need to even care.
> > they just apply the bitmask to ioread/iowrite and it doesn't even need
> > to know what the bit number actually is. The new support for gpio_desc
> > in the core code makes this feasable.
> 
> I am not sure I understand what you mean here or what new support for 
> gpio_desc you are referring to (looking in gpio/next at 
> git://git.secretlab.ca/git/linux-2.6).
> 
> Do you mean to add something like an 'unsigned long bitmask[64]' bitmap 
> array with one bitmap for each gpio line to struct gpio_desc and use 
> this primarily by gpio-generic.c, populated in bgpio_init? Is gpio_desc 
> now available outside of gpiolib.c in some repository/branch that I 
> might be unaware of?

No this isn't something that is anywhere, neither on the list nor in a
branch. It's an idea that I've been toying around with along side
pulling basic MMIO access directly into gpiolib to create a fast path.
It's not even a fully thought-through idea.  :-)

The idea is that each gpio_desc would get a hwbit field that would be
pre-populated with the bit used when ioread/iowrite (non-be) accessors
are used. If it is a BE register, then the bit positions can be munged
at setup time so that the accessor itself doesn't have to muck around
with bit position calcuation. This isn't significant for i2c or spi GPIO
controllers because they are slow anyway, but for MMIO, it is a really
big deal. It would mean dropping an entire callback call which is 10's
of instructions compaired to the 2 or 3 required to write the bit to a
set/clr register.

g.