[PATCH 00/03] gpio: R-Car and EM GPIO update

[PATCH 00/03] gpio: R-Car and EM GPIO update

[Magnus Damm]

gpio: R-Car and EM GPIO update

[PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend
[PATCH 02/03] gpio: em: Setup gpiochip->dev
[PATCH 03/03] gpio: em: Use lazy disable and mask on suspend

Update the R-Car GPIO driver and the GPIO driver used on EMEV2 to use lazy
disable and mask on suspend. This fixes the Suspend-to-RAM behavior to make
sure wakeup sources are left enabled while suspending. Also include a small
fix for gpio-em that fixes the DT use case.
 
Signed-off-by: Magnus Damm <damm@opensource.se>
---

 Written against renesas.git tag renesas-devel-v3.12-20131112

 drivers/gpio/gpio-em.c   |    5 ++---
 drivers/gpio/gpio-rcar.c |    3 +--
 2 files changed, 3 insertions(+), 5 deletions(-)

[PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Set the ->irq_enable() and ->irq_disable() methods to NULL
to enable lazy disable of interrupts. Also extend the code
to set IRQCHIP_MASK_ON_SUSPEND which tells the core that
only IRQs marked as wakeups need to stay enabled during
Suspend-to-RAM.

Tested on the Lager board with GPIO-keys and Suspend-to-RAM.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/gpio/gpio-rcar.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- 0001/drivers/gpio/gpio-rcar.c
+++ work/drivers/gpio/gpio-rcar.c	2013-11-13 19:26:26.000000000 +0900
@@ -368,10 +368,9 @@ static int gpio_rcar_probe(struct platfo
 	irq_chip->name = name;
 	irq_chip->irq_mask = gpio_rcar_irq_disable;
 	irq_chip->irq_unmask = gpio_rcar_irq_enable;
-	irq_chip->irq_enable = gpio_rcar_irq_enable;
-	irq_chip->irq_disable = gpio_rcar_irq_disable;
 	irq_chip->irq_set_type = gpio_rcar_irq_set_type;
 	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED;
+	irq_chip->flags	|= IRQCHIP_MASK_ON_SUSPEND;
 
 	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
 					      p->config.number_of_pins,

Re: [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 14 November 2013 07:59:36 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Set the ->irq_enable() and ->irq_disable() methods to NULL
> to enable lazy disable of interrupts.

Is that just for optimization purpose, or is there another reason ?

> Also extend the code to set IRQCHIP_MASK_ON_SUSPEND which tells the core
> that only IRQs marked as wakeups need to stay enabled during Suspend-to-RAM.

Shouldn't this be split to a separate patch ?

> Tested on the Lager board with GPIO-keys and Suspend-to-RAM.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>
> ---
> 
>  drivers/gpio/gpio-rcar.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- 0001/drivers/gpio/gpio-rcar.c
> +++ work/drivers/gpio/gpio-rcar.c	2013-11-13 19:26:26.000000000 +0900
> @@ -368,10 +368,9 @@ static int gpio_rcar_probe(struct platfo
>  	irq_chip->name = name;
>  	irq_chip->irq_mask = gpio_rcar_irq_disable;
>  	irq_chip->irq_unmask = gpio_rcar_irq_enable;
> -	irq_chip->irq_enable = gpio_rcar_irq_enable;
> -	irq_chip->irq_disable = gpio_rcar_irq_disable;
>  	irq_chip->irq_set_type = gpio_rcar_irq_set_type;
>  	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED;
> +	irq_chip->flags	|= IRQCHIP_MASK_ON_SUSPEND;

You can write it as

        irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED
                        | IRQCHIP_MASK_ON_SUSPEND;

>  	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
>  					      p->config.number_of_pins,
-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Magnus Damm]

Hi Laurent,

Thanks for checking the patches.

On Thu, Nov 14, 2013 at 9:07 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> Thank you for the patch.
>
> On Thursday 14 November 2013 07:59:36 Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> Set the ->irq_enable() and ->irq_disable() methods to NULL
>> to enable lazy disable of interrupts.
>
> Is that just for optimization purpose, or is there another reason ?

It is needed for IRQCHIP_MASK_ON_SUSPEND operation, which in turn is
needed for Suspend-to-RAM.

>> Also extend the code to set IRQCHIP_MASK_ON_SUSPEND which tells the core
>> that only IRQs marked as wakeups need to stay enabled during Suspend-to-RAM.
>
> Shouldn't this be split to a separate patch ?

I could split them up, but since there is a dependency and since I
mainly care about making sure Suspend-to-RAM works I preferred to keep
them together.

>> Tested on the Lager board with GPIO-keys and Suspend-to-RAM.
>>
>> Signed-off-by: Magnus Damm <damm@opensource.se>
>> ---
>>
>>  drivers/gpio/gpio-rcar.c |    3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> --- 0001/drivers/gpio/gpio-rcar.c
>> +++ work/drivers/gpio/gpio-rcar.c     2013-11-13 19:26:26.000000000 +0900
>> @@ -368,10 +368,9 @@ static int gpio_rcar_probe(struct platfo
>>       irq_chip->name = name;
>>       irq_chip->irq_mask = gpio_rcar_irq_disable;
>>       irq_chip->irq_unmask = gpio_rcar_irq_enable;
>> -     irq_chip->irq_enable = gpio_rcar_irq_enable;
>> -     irq_chip->irq_disable = gpio_rcar_irq_disable;
>>       irq_chip->irq_set_type = gpio_rcar_irq_set_type;
>>       irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED;
>> +     irq_chip->flags |= IRQCHIP_MASK_ON_SUSPEND;
>
> You can write it as
>
>         irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED
>                         | IRQCHIP_MASK_ON_SUSPEND;

I know. =) If that's the preferred way then..

Cheers,

/ magnus

Re: [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Laurent Pinchart]

Hi Magnus,

On Thursday 14 November 2013 11:03:06 Magnus Damm wrote:
> On Thu, Nov 14, 2013 at 9:07 AM, Laurent Pinchart wrote:
> > On Thursday 14 November 2013 07:59:36 Magnus Damm wrote:
> >> From: Magnus Damm <damm@opensource.se>
> >> 
> >> Set the ->irq_enable() and ->irq_disable() methods to NULL
> >> to enable lazy disable of interrupts.
> > 
> > Is that just for optimization purpose, or is there another reason ?
> 
> It is needed for IRQCHIP_MASK_ON_SUSPEND operation, which in turn is
> needed for Suspend-to-RAM.
> 
> >> Also extend the code to set IRQCHIP_MASK_ON_SUSPEND which tells the core
> >> that only IRQs marked as wakeups need to stay enabled during
> >> Suspend-to-RAM.
> >
> > Shouldn't this be split to a separate patch ?
> 
> I could split them up, but since there is a dependency and since I
> mainly care about making sure Suspend-to-RAM works I preferred to keep
> them together.

Fine with me. Could you then please update the commit message to mention that 
lazy disabling is required for IRQCHIP_MASK_ON_SUSPEND ? Same comment for 
patch 03/03.

> >> Tested on the Lager board with GPIO-keys and Suspend-to-RAM.
> >> 
> >> Signed-off-by: Magnus Damm <damm@opensource.se>
> >> ---
> >> 
> >>  drivers/gpio/gpio-rcar.c |    3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >> 
> >> --- 0001/drivers/gpio/gpio-rcar.c
> >> +++ work/drivers/gpio/gpio-rcar.c     2013-11-13 19:26:26.000000000 +0900
> >> @@ -368,10 +368,9 @@ static int gpio_rcar_probe(struct platfo
> >>       irq_chip->name = name;
> >>       irq_chip->irq_mask = gpio_rcar_irq_disable;
> >>       irq_chip->irq_unmask = gpio_rcar_irq_enable;
> >> -     irq_chip->irq_enable = gpio_rcar_irq_enable;
> >> -     irq_chip->irq_disable = gpio_rcar_irq_disable;
> >>       irq_chip->irq_set_type = gpio_rcar_irq_set_type;
> >>       irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED;
> >> +     irq_chip->flags |= IRQCHIP_MASK_ON_SUSPEND;
> > 
> > You can write it as
> > 
> >         irq_chip->flags = IRQCHIP_SKIP_SET_WAKE | IRQCHIP_SET_TYPE_MASKED
> >                         | IRQCHIP_MASK_ON_SUSPEND;
> 
> I know. =) If that's the preferred way then..

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Magnus Damm]

Hi Laurent,

On Thu, Nov 14, 2013 at 11:17 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Thursday 14 November 2013 11:03:06 Magnus Damm wrote:
>> On Thu, Nov 14, 2013 at 9:07 AM, Laurent Pinchart wrote:
>> > On Thursday 14 November 2013 07:59:36 Magnus Damm wrote:
>> >> From: Magnus Damm <damm@opensource.se>
>> >>
>> >> Set the ->irq_enable() and ->irq_disable() methods to NULL
>> >> to enable lazy disable of interrupts.
>> >
>> > Is that just for optimization purpose, or is there another reason ?
>>
>> It is needed for IRQCHIP_MASK_ON_SUSPEND operation, which in turn is
>> needed for Suspend-to-RAM.
>>
>> >> Also extend the code to set IRQCHIP_MASK_ON_SUSPEND which tells the core
>> >> that only IRQs marked as wakeups need to stay enabled during
>> >> Suspend-to-RAM.
>> >
>> > Shouldn't this be split to a separate patch ?
>>
>> I could split them up, but since there is a dependency and since I
>> mainly care about making sure Suspend-to-RAM works I preferred to keep
>> them together.
>
> Fine with me. Could you then please update the commit message to mention that
> lazy disabling is required for IRQCHIP_MASK_ON_SUSPEND ? Same comment for
> patch 03/03.

Since when is it common practise to describe the dependencies for the
various components included in a single commit? My opinion is that the
person reading the code if requiring more information will have to
grep for IRQCHIP_MASK_ON_SUSPEND in the friendly source code and learn
how the subsystem is held together.

In the case you have some special detailed commit message that you
would like me to use then please feel free to propose that. If not
then I will then split up the patches.

/ magnus

Re: [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend

[Laurent Pinchart]

Hi Magnus,

On Thursday 14 November 2013 17:50:09 Magnus Damm wrote:
> On Thu, Nov 14, 2013 at 11:17 AM, Laurent Pinchart wrote:
> > On Thursday 14 November 2013 11:03:06 Magnus Damm wrote:
> >> On Thu, Nov 14, 2013 at 9:07 AM, Laurent Pinchart wrote:
> >> > On Thursday 14 November 2013 07:59:36 Magnus Damm wrote:
> >> >> From: Magnus Damm <damm@opensource.se>
> >> >> 
> >> >> Set the ->irq_enable() and ->irq_disable() methods to NULL
> >> >> to enable lazy disable of interrupts.
> >> > 
> >> > Is that just for optimization purpose, or is there another reason ?
> >> 
> >> It is needed for IRQCHIP_MASK_ON_SUSPEND operation, which in turn is
> >> needed for Suspend-to-RAM.
> >> 
> >> >> Also extend the code to set IRQCHIP_MASK_ON_SUSPEND which tells the
> >> >> core that only IRQs marked as wakeups need to stay enabled during
> >> >> Suspend-to-RAM.
> >> > 
> >> > Shouldn't this be split to a separate patch ?
> >> 
> >> I could split them up, but since there is a dependency and since I
> >> mainly care about making sure Suspend-to-RAM works I preferred to keep
> >> them together.
> > 
> > Fine with me. Could you then please update the commit message to mention
> > that lazy disabling is required for IRQCHIP_MASK_ON_SUSPEND ? Same
> > comment for patch 03/03.
> 
> Since when is it common practise to describe the dependencies for the
> various components included in a single commit?

It's common practice to describe the reason for a commit in the commit 
message. In this case the two changes seemed unrelated at first sight, I 
believe explained why they're both needed would be good. 

> My opinion is that the person reading the code if requiring more information
> will have to grep for IRQCHIP_MASK_ON_SUSPEND in the friendly source code
> and learn how the subsystem is held together.
> 
> In the case you have some special detailed commit message that you would
> like me to use then please feel free to propose that. If not then I will
> then split up the patches.

I would have use something like

"gpio: rcar: Use mask on suspend

Set IRQCHIP_MASK_ON_SUSPEND to tell the core that only IRQs marked as wakeups 
need to stay enabled during Suspend-to-RAM. This requires lazy interrupt 
disabling, so set the ->irq_enable() and ->irq_disable() methods to NULL."

Alternatively you can split the patches, I have no preference.

-- 
Regards,

Laurent Pinchart

[PATCH 02/03] gpio: em: Setup gpiochip->dev

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Make sure gpio_chip->dev is setup so of_gpiochip_add()
will work as expected.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/gpio/gpio-em.c |    1 +
 1 file changed, 1 insertion(+)

--- 0001/drivers/gpio/gpio-em.c
+++ work/drivers/gpio/gpio-em.c	2013-11-13 20:47:40.000000000 +0900
@@ -327,6 +327,7 @@ static int em_gio_probe(struct platform_
 	gpio_chip->request = em_gio_request;
 	gpio_chip->free = em_gio_free;
 	gpio_chip->label = name;
+	gpio_chip->dev = &pdev->dev;
 	gpio_chip->owner = THIS_MODULE;
 	gpio_chip->base = pdata->gpio_base;
 	gpio_chip->ngpio = pdata->number_of_pins;

Re: [PATCH 02/03] gpio: em: Setup gpiochip->dev

[Laurent Pinchart]

Hi Magnus,

Thank you for the patch.

On Thursday 14 November 2013 07:59:46 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> Make sure gpio_chip->dev is setup so of_gpiochip_add()
> will work as expected.
> 
> Signed-off-by: Magnus Damm <damm@opensource.se>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  drivers/gpio/gpio-em.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> --- 0001/drivers/gpio/gpio-em.c
> +++ work/drivers/gpio/gpio-em.c	2013-11-13 20:47:40.000000000 +0900
> @@ -327,6 +327,7 @@ static int em_gio_probe(struct platform_
>  	gpio_chip->request = em_gio_request;
>  	gpio_chip->free = em_gio_free;
>  	gpio_chip->label = name;
> +	gpio_chip->dev = &pdev->dev;
>  	gpio_chip->owner = THIS_MODULE;
>  	gpio_chip->base = pdata->gpio_base;
>  	gpio_chip->ngpio = pdata->number_of_pins;
-- 
Regards,

Laurent Pinchart

[PATCH 03/03] gpio: em: Use lazy disable and mask on suspend

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

Set the ->irq_enable() and ->irq_disable() methods to NULL
to enable lazy disable of interrupts. Also extend the code
to set IRQCHIP_MASK_ON_SUSPEND which tells the core that
only IRQs marked as wakeups need to stay enabled during
Suspend-to-RAM.

Tested on the KZM9D board with GPIO-keys.

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/gpio/gpio-em.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--- 0006/drivers/gpio/gpio-em.c
+++ work/drivers/gpio/gpio-em.c	2013-11-13 22:18:21.000000000 +0900
@@ -336,10 +336,8 @@ static int em_gio_probe(struct platform_
 	irq_chip->name = name;
 	irq_chip->irq_mask = em_gio_irq_disable;
 	irq_chip->irq_unmask = em_gio_irq_enable;
-	irq_chip->irq_enable = em_gio_irq_enable;
-	irq_chip->irq_disable = em_gio_irq_disable;
 	irq_chip->irq_set_type = em_gio_irq_set_type;
-	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE;
+	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
 
 	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
 					      pdata->number_of_pins,

Re: [PATCH 00/03] gpio: R-Car and EM GPIO update

[Linus Walleij]

On Wed, Nov 13, 2013 at 11:59 PM, Magnus Damm <magnus.damm@gmail.com> wrote:

> gpio: R-Car and EM GPIO update
>
> [PATCH 01/03] gpio: rcar: Use lazy disable and mask on suspend
> [PATCH 02/03] gpio: em: Setup gpiochip->dev
> [PATCH 03/03] gpio: em: Use lazy disable and mask on suspend
>
> Update the R-Car GPIO driver and the GPIO driver used on EMEV2 to use lazy
> disable and mask on suspend. This fixes the Suspend-to-RAM behavior to make
> sure wakeup sources are left enabled while suspending. Also include a small
> fix for gpio-em that fixes the DT use case.
>
> Signed-off-by: Magnus Damm <damm@opensource.se>

These look OK but I see you and Laurent are discussing
patch 1, so whenever you feel ready with the series, please
resend so I can apply it.

Yours,
Linus Walleij