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@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://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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/