> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Tuesday, August 10, 2010 5:51 AM > To: Varadarajan, Charulatha > Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; Cousson, > Benoit; Nayak, Rajendra; Basak, Partha > Subject: Re: [PATCH 12/13 v5] OMAP: GPIO: Use dev_pm_ops > instead of sys_dev_class > > Charulatha V <charu@xxxxxx> writes: > > > This patch makes GPIO driver to use dev_pm_ops instead of > > sysdev_class. With this approach, gpio_bank_suspend & > gpio_bank_resume > > are not part of sys_dev_class. <<snip>> > > /* > > * OMAP1510 GPIO registers > > @@ -179,7 +179,6 @@ struct gpio_bank { > > * related to all instances of the device > > */ > > static struct gpio_bank *gpio_bank; > > - > > static int bank_width; > > > > /* TODO: Analyze removing gpio_bank_count usage from driver code */ > > @@ -1045,6 +1044,9 @@ static int omap_gpio_request(struct > gpio_chip *chip, unsigned offset) > > struct gpio_bank *bank = container_of(chip, struct > gpio_bank, chip); > > unsigned long flags; > > > > + if (!bank->mod_usage) > > + pm_runtime_get_sync(bank->dev); > > + > > Would be fine to skip the 'if' here and let runtime PM continue the > usecounting. Since we'll have trace tools that instrument runtime PM, > it will be nice to be able to trace all the users instead of just the > first one to request a GPIO in a given bank. > We are continuing to use mod_usage checks for the following reasons: 1. In the absence of mod_usage, pm_runtime_getsync() would be called in the omap_gpio_request()once per pin for each bank. However, a matching pm_runtime_putsync() would be called in the CPU_Idle path only once for a given bank. This would lead to atomic_dec_and_test(&dev->power.usage_count) to return false and the put_sync will not be effective. 2. Consider a case that a bank is not requested at all but in the CPU_Idle path we go-ahead and call pm_runtime_putsync() for that bank. Since usage_count is already zero, this call makes it negative. Now, a subsequent call to get_sync() will increment it to 0 and enable the clocks. This leads to an error-scenario where clocks are enabled with usage_cnt = 0. 3. Ideally we should not be even attempting to fiddle with the un-requested GPIO banks in the CPU_Idle path. > > spin_lock_irqsave(&bank->lock, flags); > > > > /* Set trigger to none. You need to enable the desired > trigger with > > @@ -1061,22 +1063,19 @@ static int omap_gpio_request(struct > gpio_chip *chip, unsigned offset) > > __raw_writel(__raw_readl(reg) | (1 << offset), reg); > > } > > #endif > > - if (!cpu_class_is_omap1()) { > > - if (!bank->mod_usage) { > > - void __iomem *reg = bank->base; > > - u32 ctrl; > > - > > - if (cpu_is_omap24xx() || cpu_is_omap34xx()) > > - reg += OMAP24XX_GPIO_CTRL; > > - else if (cpu_is_omap44xx()) > > - reg += OMAP4_GPIO_CTRL; > > - ctrl = __raw_readl(reg); > > - /* Module is enabled, clocks are not gated */ > > - ctrl &= 0xFFFFFFFE; > > - __raw_writel(ctrl, reg); > > - } > > - bank->mod_usage |= 1 << offset; > > + if ((!bank->mod_usage) && (!cpu_class_is_omap1())) { > > + void __iomem *reg = bank->base; > > + u32 ctrl; > > + if (bank->method == METHOD_GPIO_24XX) > > + reg += OMAP24XX_GPIO_CTRL; > > + else if (bank->method == METHOD_GPIO_44XX) > > + reg += OMAP4_GPIO_CTRL; > > + ctrl = __raw_readl(reg); > > + /* Module is enabled, clocks are not gated */ > > + ctrl &= 0xFFFFFFFE; > > + __raw_writel(ctrl, reg); > > If you get rid of the 'if (!mod_usage)' check above for the > pm_runtime_get, you could just get rid of the mod_usage flag all > together and do this section in the runtime_resume hook. -- 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
