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

From: Andreas Larsson
Date: Tue Feb 26 2013 - 05:46:39 EST

On 2013-02-26 08:52, Grant Likely wrote:
On Sat, 09 Feb 2013 14:58:55 +0000, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
On Tue, 5 Feb 2013 11:33:02 +0100, Andreas Larsson <andreas@xxxxxxxxxxx> 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@xxxxxxxxxxx>
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://

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?


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at