Re: [PATCH 1/1] gpio: omap: Fix PM runtime issue and remove most BANK_USED macros

["Grygorii.Strashko@xxxxxxxxxx" <grygorii.strashko@xxxxxxxxxx>]

Hi Tony,

On 04/21/2015 07:08 PM, Tony Lindgren wrote:
> Looks like omap_gpio_irq_type can return early at several places
> leaving a GPIO bank enabled without doing pm_runtime_put if wrong
> GPIO arguments are passed.
> 
> Instead of adding more complicated BANK_USED macros, let's fix the
> issue properly. We can pass is_irq flag to omap_enable_gpio_module
> and omap_disble_gpio_module. And with that we can remove all the
> similar code elsewhere to get rid of most BANK_USED macros.
> 
> Note that the reason for the BANK_USED macro is that we need to manage
> PM runtime on per GPIO bank basis. In the long run we want to move to
> using PM runtime counts for each GPIO line to determine if a GPIO
> bank is used. Once we have a solution for omap_enable_gpio_module
> and omap_disable_gpio_module, we can remove the remaining BANK_USED
> macros.

In general, this approach is ok for me - I've had not able to test this patch, but
I'm going to take a deeper look on it on Friday or at the beginning of next week.

But, honestly, there is one thing I really don't like :( see below pls.

> 
> Cc: Felipe Balbi <balbi@xxxxxx>
> Cc: Grygorii Strashko <grygorii.strashko@xxxxxxxxxx>
> Cc: Javier Martinez Canillas <jmartinez@xxxxxxxxxxxxxx>
> Cc: Nishanth Menon <nm@xxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>   drivers/gpio/gpio-omap.c | 111 +++++++++++++++++------------------------------
>   1 file changed, 40 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index d44e617..39a6312 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -86,6 +86,7 @@ struct gpio_bank {
>   #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
>   #define LINE_USED(line, offset) (line & (BIT(offset)))
>   
> +static void omap_reset_gpio(struct gpio_bank *bank, unsigned offset);
>   static void omap_gpio_unmask_irq(struct irq_data *d);
>   
>   static inline struct gpio_bank *omap_irq_data_get_bank(struct irq_data *d)
> @@ -419,8 +420,16 @@ static int omap_set_gpio_triggering(struct gpio_bank *bank, int gpio,
>   	return 0;
>   }
>   
> -static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
> +static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
> +				    bool is_irq)
>   {
> +	unsigned long flags;
> +
> +	/* PM runtime is per bank, not per GPIO line */
> +	if (!BANK_USED(bank))
> +		pm_runtime_get_sync(bank->dev);
> +
> +	spin_lock_irqsave(&bank->lock, flags);
>   	if (bank->regs->pinctrl) {
>   		void __iomem *reg = bank->base + bank->regs->pinctrl;
>   
> @@ -438,11 +447,30 @@ static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset)
>   		writel_relaxed(ctrl, reg);
>   		bank->context.ctrl = ctrl;
>   	}
> +
> +	if (is_irq) {
> +		omap_set_gpio_direction(bank, offset, 1);
> +		bank->irq_usage |= BIT(offset);
> +	} else {
> +		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> +		bank->mod_usage |= BIT(offset);
> +	}

The OMAP GPIO driver implements two Core interfaces IRQ-chip and GPIO-chip which, in general,
more or less independent. 

So, I don't think, that it's good to mix GPIO-IRQ-chip specific code with GPIO-chip code.
And this even don't really correspond the purpose of omap_enable_gpio_module() :( and might
introduce misunderstanding of code. The worst thing is that future fixes in IRQ-chip may 
affect on on GPIO-chip and vise versa :(

Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and
enabling/disabling?

> +	spin_unlock_irqrestore(&bank->lock, flags);
>   }
>   
> -static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
> +static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset,
> +				     bool is_irq)
>   {
>   	void __iomem *base = bank->base;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->lock, flags);
> +	if (is_irq)
> +		bank->irq_usage &= ~(BIT(offset));
> +	else
> +		bank->mod_usage &= ~(BIT(offset));
> +
> +	omap_reset_gpio(bank, offset);
>   
>   	if (bank->regs->wkup_en &&
>   	    !LINE_USED(bank->mod_usage, offset) &&
> @@ -463,6 +491,11 @@ static void omap_disable_gpio_module(struct gpio_bank *bank, unsigned offset)
>   		writel_relaxed(ctrl, reg);
>   		bank->context.ctrl = ctrl;
>   	}
> +	spin_unlock_irqrestore(&bank->lock, flags);
> +
> +	/* PM runtime is per bank, not per GPIO line */
> +	if (!BANK_USED(bank))
> +		pm_runtime_put(bank->dev);
>   }
>   
>   static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
> @@ -472,15 +505,6 @@ static int omap_gpio_is_input(struct gpio_bank *bank, unsigned offset)
>   	return readl_relaxed(reg) & BIT(offset);
>   }
>   
> -static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned offset)
> -{
> -	if (!LINE_USED(bank->mod_usage, offset)) {
> -		omap_enable_gpio_module(bank, offset);
> -		omap_set_gpio_direction(bank, offset, 1);
> -	}
> -	bank->irq_usage |= BIT(offset);
> -}
> -
>   static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> @@ -488,9 +512,6 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>   	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
>   	if (type & ~IRQ_TYPE_SENSE_MASK)
>   		return -EINVAL;
>   
> @@ -498,13 +519,9 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>   		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
>   		return -EINVAL;
>   
> +	omap_enable_gpio_module(bank, offset, true);
>   	spin_lock_irqsave(&bank->lock, flags);
>   	retval = omap_set_gpio_triggering(bank, offset, type);
> -	omap_gpio_init_irq(bank, offset);
> -	if (!omap_gpio_is_input(bank, offset)) {
> -		spin_unlock_irqrestore(&bank->lock, flags);
> -		return -EINVAL;
> -	}
>   	spin_unlock_irqrestore(&bank->lock, flags);
>   
>   	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
> @@ -659,26 +676,8 @@ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable)
>   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 this is the first gpio_request for the bank,
> -	 * enable the bank module.
> -	 */
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	/* Set trigger to none. You need to enable the desired trigger with
> -	 * request_irq() or set_irq_type(). Only do this if the IRQ line has
> -	 * not already been requested.
> -	 */
> -	if (!LINE_USED(bank->irq_usage, offset)) {
> -		omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> -		omap_enable_gpio_module(bank, offset);
> -	}
> -	bank->mod_usage |= BIT(offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> +	omap_enable_gpio_module(bank, offset, false);
>   
>   	return 0;
>   }
> @@ -686,20 +685,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>   static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>   {
>   	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	bank->mod_usage &= ~(BIT(offset));
> -	omap_disable_gpio_module(bank, offset);
> -	omap_reset_gpio(bank, offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
>   
> -	/*
> -	 * 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);
> +	omap_disable_gpio_module(bank, offset, false);
>   }
>   
>   /*
> @@ -788,15 +775,9 @@ exit:
>   static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> -	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	if (!BANK_USED(bank))
> -		pm_runtime_get_sync(bank->dev);
> -
> -	spin_lock_irqsave(&bank->lock, flags);
> -	omap_gpio_init_irq(bank, offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> +	omap_enable_gpio_module(bank, offset, true);
>   	omap_gpio_unmask_irq(d);
>   
>   	return 0;
> @@ -805,21 +786,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>   static void omap_gpio_irq_shutdown(struct irq_data *d)
>   {
>   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> -	unsigned long flags;
>   	unsigned offset = d->hwirq;
>   
> -	spin_lock_irqsave(&bank->lock, flags);
> -	bank->irq_usage &= ~(BIT(offset));
> -	omap_disable_gpio_module(bank, offset);
> -	omap_reset_gpio(bank, offset);
> -	spin_unlock_irqrestore(&bank->lock, flags);
> -
> -	/*
> -	 * 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);
> +	omap_disable_gpio_module(bank, offset, true);
>   }
>   
>   static void omap_gpio_ack_irq(struct irq_data *d)
> 


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