Re: [PATCH v3 13/13] gpio/omap: fix incorrect update to context.irqenable1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 13, 2012 at 3:39 AM, Kevin Hilman <khilman@xxxxxx> wrote:
> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> writes:
>
>> In _enable_gpio_irqbank() when bank->regs->set_irqenable is valid,
>> gpio_mask can be directly set by writing to set_irqenable register
>> without overwriting current value.
>
> Ouch.  Nice catch.
>
>> In order to ensure the same is
>> stored in context.irqenable1, we must read from regs->irqenable
>> instead of overwriting it with gpio_mask.
>> The overwriting makes sense only in the second case where irqenable
>> is explicitly read and updated with new gpio_mask before writing it
>> back. However, for consistency reading regs->irqenable into the
>> bank->context.irqenable1 takes care of both the scenarios.
>
> ...takes care of both scenarios, but adds and extra duplicate read for
> the second.
Right. I wanted to keep code change minimum.
>
> Instead, how about just move the context update into each side of the
> if/else?  untested patch below to show what I mean.
Yes, initially I thought of doing in this way as well. I will make the change.
--
Tarun
>
> Kevin
>
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 752ae9b..f8b7099 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -442,6 +442,7 @@ static void _enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>        if (bank->regs->set_irqenable) {
>                reg += bank->regs->set_irqenable;
>                l = gpio_mask;
> +               bank->context.irqenable1 |= gpio_mask;
>        } else {
>                reg += bank->regs->irqenable;
>                l = __raw_readl(reg);
> @@ -449,10 +450,10 @@ static void _enable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>                        l &= ~gpio_mask;
>                else
>                        l |= gpio_mask;
> +               bank->context.irqenable1 = l;
>        }
>
>        __raw_writel(l, reg);
> -       bank->context.irqenable1 = l;
>  }
>
>  static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
> @@ -463,6 +464,7 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>        if (bank->regs->clr_irqenable) {
>                reg += bank->regs->clr_irqenable;
>                l = gpio_mask;
> +               bank->context.irqenable1 &= ~gpio_mask;
>        } else {
>                reg += bank->regs->irqenable;
>                l = __raw_readl(reg);
> @@ -470,10 +472,10 @@ static void _disable_gpio_irqbank(struct gpio_bank *bank, int gpio_mask)
>                        l |= gpio_mask;
>                else
>                        l &= ~gpio_mask;
> +               bank->context.irqenable1 = l;
>        }
>
>        __raw_writel(l, reg);
> -       bank->context.irqenable1 = l;
>  }
>
>  static inline void _set_gpio_irqenable(struct gpio_bank *bank, int gpio, int enable)
>
--
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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux