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

[Andreas Larsson <andreas@gaisler.com>]

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