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

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

Hi Tony,

Sorry for delayed reply.

On 04/23/2015 05:39 PM, Tony Lindgren wrote:
> * Grygorii.Strashko@xxxxxxxxxx <grygorii.strashko@xxxxxxxxxx> [150423 04:13]:
>> On 04/21/2015 07:08 PM, Tony Lindgren wrote:
>>> @@ -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 :(
> 
> Hmm I'm thinking omap_enable/disable_gpio_module() eventually becomes
> our runtime_resume/suspend(). Currently the enabling and disabling is
> buggy for GPIO for some corner cases.. AFAIK the only difference between

It might be very helpful if you'd able to share additional info about
any "corner cases" you know.

> enabling GPIO vs GPIO-IRQ is the calling of omap_set_gpio_direction
> vs omap_set_gpio_triggering. Or at least that's the way it should be
> unless I'm missing something?

I think yes. what i'd like to say, first of all, is that it might be not good idea to mix 
too much functionality in  omap_enable/disable_gpio_module() - especially GPIO-IRQ vs
GPIO-chip ( Very easy to get lost ;)

For example (1) - your change of omap_gpio_request() looks like invalid:

  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);

	^^ above two line should be executed only if GPIO line was not requested as IRQ before

-	}


-	bank->mod_usage |= BIT(offset);
-	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_enable_gpio_module(bank, offset, false);

^^ after your change, it looks like omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE)
   will be called always and GPIO triggering configuration might be lost
  
  	return 0;
  }

Example (2)
 I've found commit 55b6019ae294 "OMAP: GPIO: clear/restore level/edge detect settings on mask/unmask"
 which does the following
  gpio_mask_irq()
   |-  _set_gpio_triggering(bank, get_gpio_index(gpio), IRQ_TYPE_NONE);
  
  gpio_unmask_irq()
   |- u32 trigger = irqd_get_trigger_type(d);
      if (trigger)
             omap_set_gpio_triggering(bank, offset, trigger);

  As result, it seems unreasonable to physically configure IRQ triggering type in GPIO bank registers 
  inside omap_gpio_irq_type(). Of course, such assumption should be double checked taking into account that 
  __irq_set_trigger() can be called any time even before request_irq().
  Also, seems the same could be applied to omap_enable_gpio_module and omap_set_gpio_direction and they
  could be removed from omap_gpio_irq_type().
  
>   
>> Could we keep omap_xxx_gpio_module() functions responsible only for GPIO bank PM and
>> enabling/disabling?
> 
> If you're thinking about just thinking about having separate wrappers around
> it like this::
> 
> static void omap_enable_gpio_module(struct gpio_bank *bank, unsigned offset,
> 					bool is_irq)
> {
> 	...
> }
> 
> static void omap_enable_gpio((struct gpio_bank *bank, unsigned offset)
> {
> 	omap_enable_gpio_module(bpio_bank, offset, 0);
> }
> 
> static void omap_enable_gpio_irq((struct gpio_bank *bank, unsigned offset)
> {
> 	omap_enable_gpio_module(bpio_bank, offset, 1);
> }
> 
> Then yes makes sense to me. Or do you have something else in mind?

Yep. Commented above.

Also, it probably could work if we will set GPIO_CTRL.DISABLEMODULE=1
in omap_gpio_runtime_resume and GPIO_CTRL.DISABLEMODULE=0 in _runtime_suspend,
but it may require from us to split CPUIdle and suspend code path (


-- 
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