On Wed, Feb 29, 2012 at 1:59 AM, Cousson, Benoit <b-cousson@xxxxxx> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
