diff mbox

[RFC/RFT,2/2] gpio: omap: ensure that runtime pm will disable unused gpio banks

Message ID 1425670017-19598-2-git-send-email-grygorii.strashko@linaro.org
State New
Headers show

Commit Message

grygorii.strashko@linaro.org March 6, 2015, 7:26 p.m. UTC 
From: Grygorii Strashko <grygorii.strashko@linaro.org>

Now there are two points related to Runtime PM usage:
1) bank state doesn't need to be checked in places where
Rintime PM is used, bacause Runtime PM maintains its
own usage counter:
      if (!BANK_USED(bank))
               pm_runtime_get_sync(bank->dev);
so, it's safe to drop such checks.

2) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
but no corresponding put. As result, GPIO devices could be
powered on forever if at least one GPIO was used as IRQ.
Hence, allow powering off GPIO banks by adding missed
pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().

Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
---
 drivers/gpio/gpio-omap.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

Tony Lindgren March 6, 2015, 11:15 p.m. UTC | #1 
* grygorii.strashko@linaro.org <grygorii.strashko@linaro.org> [150306 11:27]:
> From: Grygorii Strashko <grygorii.strashko@linaro.org>
> 
> Now there are two points related to Runtime PM usage:
> 1) bank state doesn't need to be checked in places where
> Rintime PM is used, bacause Runtime PM maintains its
> own usage counter:
>       if (!BANK_USED(bank))
>                pm_runtime_get_sync(bank->dev);
> so, it's safe to drop such checks.
> 
> 2) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
> but no corresponding put. As result, GPIO devices could be
> powered on forever if at least one GPIO was used as IRQ.
> Hence, allow powering off GPIO banks by adding missed
> pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().

Nice to see this happening, but I think before merging this we need
to test to be sure that the pm_runtime calls actually match.. I'm
not convinced right now.. We may still have uninitialized entry
points similar to 3d009c8c61f9 ("gpio: omap: Fix bad device
access with setup_irq()").

Regards,

Tony
 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> ---
>  drivers/gpio/gpio-omap.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 2b2fc4b..b1176c5 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -497,8 +497,7 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>  	unsigned long flags;
>  	unsigned offset;
>  
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> +	pm_runtime_get_sync(bank->dev);
>  
>  #ifdef CONFIG_ARCH_OMAP1
>  	if (d->irq > IH_MPUIO_BASE)
> @@ -530,6 +529,8 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>  	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>  		__irq_set_handler_locked(d->irq, handle_edge_irq);
>  
> +	pm_runtime_put(bank->dev);
> +
>  	return retval;
>  }
>  
> @@ -680,8 +681,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>  	 * If this is the first gpio_request for the bank,
>  	 * enable the bank module.
>  	 */
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> +	pm_runtime_get_sync(bank->dev);
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	/* Set trigger to none. You need to enable the desired trigger with
> @@ -713,8 +713,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>  	 * If this is the last gpio to be freed in the bank,
>  	 * disable the bank module.
>  	 */
> -	if (!BANK_USED(bank))
> -		pm_runtime_put(bank->dev);
> +	pm_runtime_put(bank->dev);
>  }
>  
>  /*
> @@ -807,8 +806,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>  	unsigned long flags;
>  	unsigned offset = GPIO_INDEX(bank, gpio);
>  
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> +	pm_runtime_get_sync(bank->dev);
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	omap_gpio_init_irq(bank, gpio, offset);
> @@ -835,8 +833,7 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
>  	 * If this is the last IRQ to be freed in the bank,
>  	 * disable the bank module.
>  	 */
> -	if (!BANK_USED(bank))
> -		pm_runtime_put(bank->dev);
> +	pm_runtime_put(bank->dev);
>  }
>  
>  static void omap_gpio_ack_irq(struct irq_data *d)
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren March 19, 2015, 11:03 p.m. UTC | #2 
* Tony Lindgren <tony@atomide.com> [150307 00:08]:
> * grygorii.strashko@linaro.org <grygorii.strashko@linaro.org> [150306 11:27]:
> > From: Grygorii Strashko <grygorii.strashko@linaro.org>
> > 
> > Now there are two points related to Runtime PM usage:
> > 1) bank state doesn't need to be checked in places where
> > Rintime PM is used, bacause Runtime PM maintains its
> > own usage counter:
> >       if (!BANK_USED(bank))
> >                pm_runtime_get_sync(bank->dev);
> > so, it's safe to drop such checks.
> > 
> > 2) There is a call of pm_runtime_get_sync() in omap_gpio_irq_type(),
> > but no corresponding put. As result, GPIO devices could be
> > powered on forever if at least one GPIO was used as IRQ.
> > Hence, allow powering off GPIO banks by adding missed
> > pm_runtime_put(bank->dev) at the end of omap_gpio_irq_type().
> 
> Nice to see this happening, but I think before merging this we need
> to test to be sure that the pm_runtime calls actually match.. I'm
> not convinced right now.. We may still have uninitialized entry
> points similar to 3d009c8c61f9 ("gpio: omap: Fix bad device
> access with setup_irq()").

OK so I finally got around testing this along with your bank
removal set. Looks like this patch causes a regression at least
with n900 keyboard LEDs with off-idle. The LED won't come back
on after restore from off-idle. Anyways, now we have something
reproducable :) So I'll try to debug it further at some point,
might be few days before I get to it.

Regards,

Tony

 
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@linaro.org>
> > ---
> >  drivers/gpio/gpio-omap.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 2b2fc4b..b1176c5 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -497,8 +497,7 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
> >  	unsigned long flags;
> >  	unsigned offset;
> >  
> > -	if (!BANK_USED(bank))
> > -		pm_runtime_get_sync(bank->dev);
> > +	pm_runtime_get_sync(bank->dev);
> >  
> >  #ifdef CONFIG_ARCH_OMAP1
> >  	if (d->irq > IH_MPUIO_BASE)
> > @@ -530,6 +529,8 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
> >  	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> >  		__irq_set_handler_locked(d->irq, handle_edge_irq);
> >  
> > +	pm_runtime_put(bank->dev);
> > +
> >  	return retval;
> >  }
> >  
> > @@ -680,8 +681,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
> >  	 * If this is the first gpio_request for the bank,
> >  	 * enable the bank module.
> >  	 */
> > -	if (!BANK_USED(bank))
> > -		pm_runtime_get_sync(bank->dev);
> > +	pm_runtime_get_sync(bank->dev);
> >  
> >  	spin_lock_irqsave(&bank->lock, flags);
> >  	/* Set trigger to none. You need to enable the desired trigger with
> > @@ -713,8 +713,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> >  	 * If this is the last gpio to be freed in the bank,
> >  	 * disable the bank module.
> >  	 */
> > -	if (!BANK_USED(bank))
> > -		pm_runtime_put(bank->dev);
> > +	pm_runtime_put(bank->dev);
> >  }
> >  
> >  /*
> > @@ -807,8 +806,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
> >  	unsigned long flags;
> >  	unsigned offset = GPIO_INDEX(bank, gpio);
> >  
> > -	if (!BANK_USED(bank))
> > -		pm_runtime_get_sync(bank->dev);
> > +	pm_runtime_get_sync(bank->dev);
> >  
> >  	spin_lock_irqsave(&bank->lock, flags);
> >  	omap_gpio_init_irq(bank, gpio, offset);
> > @@ -835,8 +833,7 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
> >  	 * If this is the last IRQ to be freed in the bank,
> >  	 * disable the bank module.
> >  	 */
> > -	if (!BANK_USED(bank))
> > -		pm_runtime_put(bank->dev);
> > +	pm_runtime_put(bank->dev);
> >  }
> >  
> >  static void omap_gpio_ack_irq(struct irq_data *d)
> > -- 
> > 1.9.1
> > 
> --
> 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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 2b2fc4b..b1176c5 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -497,8 +497,7 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	unsigned long flags;
 	unsigned offset;
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 #ifdef CONFIG_ARCH_OMAP1
 	if (d->irq > IH_MPUIO_BASE)
@@ -530,6 +529,8 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
 		__irq_set_handler_locked(d->irq, handle_edge_irq);
 
+	pm_runtime_put(bank->dev);
+
 	return retval;
 }
 
@@ -680,8 +681,7 @@  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	 * If this is the first gpio_request for the bank,
 	 * enable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 	/* Set trigger to none. You need to enable the desired trigger with
@@ -713,8 +713,7 @@  static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	 * If this is the last gpio to be freed in the bank,
 	 * disable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 }
 
 /*
@@ -807,8 +806,7 @@  static unsigned int omap_gpio_irq_startup(struct irq_data *d)
 	unsigned long flags;
 	unsigned offset = GPIO_INDEX(bank, gpio);
 
-	if (!BANK_USED(bank))
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 	omap_gpio_init_irq(bank, gpio, offset);
@@ -835,8 +833,7 @@  static void omap_gpio_irq_shutdown(struct irq_data *d)
 	 * If this is the last IRQ to be freed in the bank,
 	 * disable the bank module.
 	 */
-	if (!BANK_USED(bank))
-		pm_runtime_put(bank->dev);
+	pm_runtime_put(bank->dev);
 }
 
 static void omap_gpio_ack_irq(struct irq_data *d)