Re: [PATCH] gpio: pcf857x: Simpify wake-up handling

[Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>]

Hi Geert,

On 2019-02-13 14:14:09 +0100, Geert Uytterhoeven wrote:
> Unlike gpio_rcar_irq_set_wake(), where this solution was based on,

s/where/which/ ?

> pcf857x_irq_set_wake() does not need to do anything else than calling
> irq_set_irq_wake() for the upstream interrupt controller.  Hence there
> is no reason for making this call conditional, and no longer a need for
> keeping a copy of the interrupt number that also serves as a flag.
> 
> Just propagate irq_set_irq_wake() to the upstream interrupt controller,
> using the original interrupt number.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

This change makes sens to me,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>

> ---
> I can no longer reproduce the issue fixed by commit ffb8e44bd7617ede
> ("gpio: pcf857x: Check for irq_set_irq_wake() failures") on current
> kernels: if .irq_set_wake() fails, system suspend is aborted, and the
> imbalance on resume never happens.
> 
> Looking at old logs, the system used to continue suspending, leading to
> the imbalance.  Presumably I could run git bisect to find out when that
> was changed, but that would require downgrading my cross-compiler (Linux
> v4.1-rc3 didn't support building using gcc-7)...
> ---
>  drivers/gpio/gpio-pcf857x.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
> index 68a35b65925aca76..c9b650f617fa687c 100644
> --- a/drivers/gpio/gpio-pcf857x.c
> +++ b/drivers/gpio/gpio-pcf857x.c
> @@ -89,7 +89,6 @@ struct pcf857x {
>  	struct mutex		lock;		/* protect 'out' */
>  	unsigned		out;		/* software latch */
>  	unsigned		status;		/* current status */
> -	unsigned int		irq_parent;
>  	unsigned		irq_enabled;	/* enabled irqs */
>  
>  	int (*write)(struct i2c_client *client, unsigned data);
> @@ -211,18 +210,7 @@ static int pcf857x_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>  	struct pcf857x *gpio = irq_data_get_irq_chip_data(data);
>  
> -	int error = 0;
> -
> -	if (gpio->irq_parent) {
> -		error = irq_set_irq_wake(gpio->irq_parent, on);
> -		if (error) {
> -			dev_dbg(&gpio->client->dev,
> -				"irq %u doesn't support irq_set_wake\n",
> -				gpio->irq_parent);
> -			gpio->irq_parent = 0;
> -		}
> -	}
> -	return error;
> +	return irq_set_irq_wake(gpio->client->irq, on);
>  }
>  
>  static void pcf857x_irq_enable(struct irq_data *data)
> @@ -392,7 +380,6 @@ static int pcf857x_probe(struct i2c_client *client,
>  
>  		gpiochip_set_nested_irqchip(&gpio->chip, &gpio->irqchip,
>  					    client->irq);
> -		gpio->irq_parent = client->irq;
>  	}
>  
>  	/* Let platform code set up the GPIOs and their users.
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund