OMAP gpio handling

OMAP gpio handling

[Russell King - ARM Linux]

Can someone explain to me this:

#define GPIO_INDEX(bank, gpio) (gpio % bank->width)
#define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))

static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
{
        void __iomem *reg = bank->base + bank->regs->datain;

        return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
}

static int gpio_get(struct gpio_chip *chip, unsigned offset)
{
        struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
        void __iomem *reg = bank->base;
        int gpio = chip->base + offset;
        u32 mask = GPIO_BIT(bank, gpio);

        if (gpio_is_input(bank, mask))
                return _get_gpio_datain(bank, gpio);
        else
                return _get_gpio_dataout(bank, gpio);
}

Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for
any GPIO chip are always aligned to 32 or 16, why does this code bother
adding the chips base gpio number and then modulo the width?

Surely this means if - for argument sake - you registered a GPIO chip
with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0
bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be
chip 1 bit 0..7.

However, if you registered a GPIO chip with 16 lines first, it would
mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be
chip 1 bit 0..15.

Surely this kind of behaviour is not intended?

Is there a reason why the bitmask can't just be (1 << offset) where
offset is passed into these functions as GPIO number - chip->base ?

Re: OMAP gpio handling

[DebBarma, Tarun Kanti]

On Sat, Feb 25, 2012 at 6:34 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> Can someone explain to me this:
>
> #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
> #define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio))
>
> static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
> {
>        void __iomem *reg = bank->base + bank->regs->datain;
>
>        return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
> }
>
> static int gpio_get(struct gpio_chip *chip, unsigned offset)
> {
>        struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
>        void __iomem *reg = bank->base;
>        int gpio = chip->base + offset;
>        u32 mask = GPIO_BIT(bank, gpio);
>
>        if (gpio_is_input(bank, mask))
>                return _get_gpio_datain(bank, gpio);
>        else
>                return _get_gpio_dataout(bank, gpio);
> }
>
> Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for
> any GPIO chip are always aligned to 32 or 16, why does this code bother
> adding the chips base gpio number and then modulo the width?
As far as I understand we could very well use offset directly.
The macro is used to derive the same offset value in this case.
Therefore, _get_gpio_datain(bank, gpio) and _get_gpio_dataout(bank, gpio)
can be passed offset instead of (bank->base + offset) as suggested by you below.
And of course, offset can be used to derive mask both here and in the
called functions.

>
> Surely this means if - for argument sake - you registered a GPIO chip
> with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0
> bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be
> chip 1 bit 0..7.
>
> However, if you registered a GPIO chip with 16 lines first, it would
> mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be
> chip 1 bit 0..15.
>
> Surely this kind of behaviour is not intended?
>
> Is there a reason why the bitmask can't just be (1 << offset) where
> offset is passed into these functions as GPIO number - chip->base ?
Yes, I have made the following changes and tested on OMAP4 and OMAP1(bootup).

[PATCH] gpio/omap: remove redundant decoding of gpio offset
---
 drivers/gpio/gpio-omap.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 5953ece..2196747 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -133,18 +133,18 @@ static void _set_gpio_dataout_mask(struct
gpio_bank *bank, int gpio, int enable)
        bank->context.dataout = l;
 }

-static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
+static int _get_gpio_datain(struct gpio_bank *bank, int offset)
 {
        void __iomem *reg = bank->base + bank->regs->datain;

-       return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
+       return (__raw_readl(reg) & (1 << offset)) != 0;
 }

-static int _get_gpio_dataout(struct gpio_bank *bank, int gpio)
+static int _get_gpio_dataout(struct gpio_bank *bank, int offset)
 {
        void __iomem *reg = bank->base + bank->regs->dataout;

-       return (__raw_readl(reg) & GPIO_BIT(bank, gpio)) != 0;
+       return (__raw_readl(reg) & (1 << offset)) != 0;
 }

 static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
@@ -849,19 +849,15 @@ static int gpio_is_input(struct gpio_bank *bank, int mask)
 static int gpio_get(struct gpio_chip *chip, unsigned offset)
 {
        struct gpio_bank *bank;
-       void __iomem *reg;
-       int gpio;
        u32 mask;

-       gpio = chip->base + offset;
        bank = container_of(chip, struct gpio_bank, chip);
-       reg = bank->base;
-       mask = GPIO_BIT(bank, gpio);
+       mask = (1 << offset);

        if (gpio_is_input(bank, mask))
-               return _get_gpio_datain(bank, gpio);
+               return _get_gpio_datain(bank, offset);
        else
-               return _get_gpio_dataout(bank, gpio);
+               return _get_gpio_dataout(bank, offset);
 }

 static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
--
1.7.0.4

--
Tarun

> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OMAP gpio handling

[Cousson, Benoit]

+ Kevin

Hi Tarun,

On 2/27/2012 8:11 AM, DebBarma, Tarun Kanti wrote:
> On Sat, Feb 25, 2012 at 6:34 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk>  wrote:
>> Can someone explain to me this:
>>
>> #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
>> #define GPIO_BIT(bank, gpio) (1<<  GPIO_INDEX(bank, gpio))
>>
>> static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
>> {
>>         void __iomem *reg = bank->base + bank->regs->datain;
>>
>>         return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
>> }
>>
>> static int gpio_get(struct gpio_chip *chip, unsigned offset)
>> {
>>         struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
>>         void __iomem *reg = bank->base;
>>         int gpio = chip->base + offset;
>>         u32 mask = GPIO_BIT(bank, gpio);
>>
>>         if (gpio_is_input(bank, mask))
>>                 return _get_gpio_datain(bank, gpio);
>>         else
>>                 return _get_gpio_dataout(bank, gpio);
>> }
>>
>> Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for
>> any GPIO chip are always aligned to 32 or 16, why does this code bother
>> adding the chips base gpio number and then modulo the width?
> As far as I understand we could very well use offset directly.
> The macro is used to derive the same offset value in this case.
> Therefore, _get_gpio_datain(bank, gpio) and _get_gpio_dataout(bank, gpio)
> can be passed offset instead of (bank->base + offset) as suggested by you below.
> And of course, offset can be used to derive mask both here and in the
> called functions.
>
>>
>> Surely this means if - for argument sake - you registered a GPIO chip
>> with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0
>> bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be
>> chip 1 bit 0..7.
>>
>> However, if you registered a GPIO chip with 16 lines first, it would
>> mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be
>> chip 1 bit 0..15.
>>
>> Surely this kind of behaviour is not intended?
>>
>> Is there a reason why the bitmask can't just be (1<<  offset) where
>> offset is passed into these functions as GPIO number - chip->base ?
> Yes, I have made the following changes and tested on OMAP4 and OMAP1(bootup).
>
> [PATCH] gpio/omap: remove redundant decoding of gpio offset
> ---
>   drivers/gpio/gpio-omap.c |   18 +++++++-----------
>   1 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 5953ece..2196747 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -133,18 +133,18 @@ static void _set_gpio_dataout_mask(struct
> gpio_bank *bank, int gpio, int enable)
>          bank->context.dataout = l;
>   }
>
> -static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
> +static int _get_gpio_datain(struct gpio_bank *bank, int offset)
>   {
>          void __iomem *reg = bank->base + bank->regs->datain;
>
> -       return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
> +       return (__raw_readl(reg)&  (1<<  offset)) != 0;
>   }
>
> -static int _get_gpio_dataout(struct gpio_bank *bank, int gpio)
> +static int _get_gpio_dataout(struct gpio_bank *bank, int offset)
>   {
>          void __iomem *reg = bank->base + bank->regs->dataout;
>
> -       return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
> +       return (__raw_readl(reg)&  (1<<  offset)) != 0;
>   }
>
>   static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
> @@ -849,19 +849,15 @@ static int gpio_is_input(struct gpio_bank *bank, int mask)
>   static int gpio_get(struct gpio_chip *chip, unsigned offset)
>   {
>          struct gpio_bank *bank;
> -       void __iomem *reg;
> -       int gpio;
>          u32 mask;
>
> -       gpio = chip->base + offset;
>          bank = container_of(chip, struct gpio_bank, chip);
> -       reg = bank->base;

That one seems to be a leftover from before the cleanup and was already 
not needed but it is not related to the offset fix.

You should just add that in the changelog or include it in previous 
cleanup if applicable.

> -       mask = GPIO_BIT(bank, gpio);
> +       mask = (1<<  offset);
>
>          if (gpio_is_input(bank, mask))
> -               return _get_gpio_datain(bank, gpio);
> +               return _get_gpio_datain(bank, offset);
>          else
> -               return _get_gpio_dataout(bank, gpio);
> +               return _get_gpio_dataout(bank, offset);
>   }
>
>   static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
> --
> 1.7.0.4

That patch seems good to me and should definitively be part of your 
cleanup series.
Could you repost to a wider audience including lakml and Grant?

I guess you can do that in the v2 of the previous series including 
Kevin's comments.

Thanks,
Benoit

Re: OMAP gpio handling

[DebBarma, Tarun Kanti]

On Wed, Feb 29, 2012 at 1:59 AM, Cousson, Benoit <b-cousson@ti.com> wrote:
> + Kevin
>
> Hi Tarun,
>
>
> On 2/27/2012 8:11 AM, DebBarma, Tarun Kanti wrote:
>>
>> On Sat, Feb 25, 2012 at 6:34 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk>  wrote:
>>>
>>> Can someone explain to me this:
>>>
>>> #define GPIO_INDEX(bank, gpio) (gpio % bank->width)
>>> #define GPIO_BIT(bank, gpio) (1<<  GPIO_INDEX(bank, gpio))
>>>
>>> static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
>>> {
>>>        void __iomem *reg = bank->base + bank->regs->datain;
>>>
>>>        return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
>>>
>>> }
>>>
>>> static int gpio_get(struct gpio_chip *chip, unsigned offset)
>>> {
>>>        struct gpio_bank *bank = container_of(chip, struct gpio_bank,
>>> chip);
>>>        void __iomem *reg = bank->base;
>>>        int gpio = chip->base + offset;
>>>        u32 mask = GPIO_BIT(bank, gpio);
>>>
>>>        if (gpio_is_input(bank, mask))
>>>                return _get_gpio_datain(bank, gpio);
>>>        else
>>>                return _get_gpio_dataout(bank, gpio);
>>> }
>>>
>>> Given that bank->width on OMAP is either 32 or 16, and GPIO numbers for
>>> any GPIO chip are always aligned to 32 or 16, why does this code bother
>>> adding the chips base gpio number and then modulo the width?
>>
>> As far as I understand we could very well use offset directly.
>> The macro is used to derive the same offset value in this case.
>> Therefore, _get_gpio_datain(bank, gpio) and _get_gpio_dataout(bank, gpio)
>> can be passed offset instead of (bank->base + offset) as suggested by you
>> below.
>> And of course, offset can be used to derive mask both here and in the
>> called functions.
>>
>>>
>>> Surely this means if - for argument sake - you registered a GPIO chip
>>> with 8 lines followed by one with 16 lines, GPIO0..7 would be chip 0
>>> bit 0..7, GPIO8..15 would be chip 1 bit 8..15, GPIO16..23 would be
>>> chip 1 bit 0..7.
>>>
>>> However, if you registered a GPIO chip with 16 lines first, it would
>>> mean GPIO0..15 would be chip 0 bit 0..15, and GPIO16..31 would be
>>> chip 1 bit 0..15.
>>>
>>> Surely this kind of behaviour is not intended?
>>>
>>> Is there a reason why the bitmask can't just be (1<<  offset) where
>>> offset is passed into these functions as GPIO number - chip->base ?
>>
>> Yes, I have made the following changes and tested on OMAP4 and
>> OMAP1(bootup).
>>
>> [PATCH] gpio/omap: remove redundant decoding of gpio offset
>> ---
>>  drivers/gpio/gpio-omap.c |   18 +++++++-----------
>>  1 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 5953ece..2196747 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -133,18 +133,18 @@ static void _set_gpio_dataout_mask(struct
>> gpio_bank *bank, int gpio, int enable)
>>         bank->context.dataout = l;
>>  }
>>
>> -static int _get_gpio_datain(struct gpio_bank *bank, int gpio)
>> +static int _get_gpio_datain(struct gpio_bank *bank, int offset)
>>  {
>>         void __iomem *reg = bank->base + bank->regs->datain;
>>
>> -       return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
>> +       return (__raw_readl(reg)&  (1<<  offset)) != 0;
>>
>>  }
>>
>> -static int _get_gpio_dataout(struct gpio_bank *bank, int gpio)
>> +static int _get_gpio_dataout(struct gpio_bank *bank, int offset)
>>  {
>>         void __iomem *reg = bank->base + bank->regs->dataout;
>>
>> -       return (__raw_readl(reg)&  GPIO_BIT(bank, gpio)) != 0;
>> +       return (__raw_readl(reg)&  (1<<  offset)) != 0;
>>
>>  }
>>
>>  static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool
>> set)
>> @@ -849,19 +849,15 @@ static int gpio_is_input(struct gpio_bank *bank, int
>> mask)
>>  static int gpio_get(struct gpio_chip *chip, unsigned offset)
>>  {
>>         struct gpio_bank *bank;
>> -       void __iomem *reg;
>> -       int gpio;
>>         u32 mask;
>>
>> -       gpio = chip->base + offset;
>>         bank = container_of(chip, struct gpio_bank, chip);
>> -       reg = bank->base;
>
>
> That one seems to be a leftover from before the cleanup and was already not
> needed but it is not related to the offset fix.
>
> You should just add that in the changelog or include it in previous cleanup
> if applicable.
>
>
>> -       mask = GPIO_BIT(bank, gpio);
>> +       mask = (1<<  offset);
>>
>>         if (gpio_is_input(bank, mask))
>> -               return _get_gpio_datain(bank, gpio);
>> +               return _get_gpio_datain(bank, offset);
>>         else
>> -               return _get_gpio_dataout(bank, gpio);
>> +               return _get_gpio_dataout(bank, offset);
>>  }
>>
>>  static int gpio_output(struct gpio_chip *chip, unsigned offset, int
>> value)
>> --
>> 1.7.0.4
>
>
> That patch seems good to me and should definitively be part of your cleanup
> series.
> Could you repost to a wider audience including lakml and Grant?
>
> I guess you can do that in the v2 of the previous series including Kevin's
> comments.
Yes, I have created a patch already as part of v2.
I was waiting for any further comment from the community.
Anyways, thanks.
--
Tarun
>
> Thanks,
> Benoit
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html