| Message ID | 20161208173228.16835-3-niklas.soderlund+renesas@ragnatech.se |
|---|---|
| State | New |
| Headers | show |
Hi Niklas, Thank you for the patch. On Thursday 08 Dec 2016 18:32:28 Niklas Söderlund wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Currently gpio modules are runtime-resumed at probe time. This means the > gpio module will be active all the time (except during system suspend, > if not configured as a wake-up source). > > While an R-Car Gen2 gpio module retains pins configured for output at > the requested level while put in standby mode, gpio register cannot be > accessed while suspended. Unfortunately pm_runtime_get_sync() cannot be > called from all contexts where gpio register access is needed. Hence > move the Runtime PM handling from probe/remove time to gpio request/free > time, which is probably the best we can do. > > On r8a7791/koelsch, gpio modules 0, 1, 3, and 4 are now suspended during > normal use (gpio2 is used for LEDs and regulators, gpio5 for keys, gpio6 > for SD-Card CD & WP, gpio7 for keys and regulators). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [Niklas: s/gpio_to_priv(chip)/gpiochip_get_data(chip)/] Just curious, what's the rationale for this ? > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpio/gpio-rcar.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > index 3b77c10..31ad288 100644 > --- a/drivers/gpio/gpio-rcar.c > +++ b/drivers/gpio/gpio-rcar.c > @@ -242,11 +242,24 @@ static void > gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip, > > static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset) > { > - return pinctrl_request_gpio(chip->base + offset); > + struct gpio_rcar_priv *p = gpiochip_get_data(chip); > + int error; > + > + error = pm_runtime_get_sync(&p->pdev->dev); > + if (error < 0) > + return error; > + > + error = pinctrl_request_gpio(chip->base + offset); > + if (error) > + pm_runtime_put(&p->pdev->dev); > + > + return error; > } > > static void gpio_rcar_free(struct gpio_chip *chip, unsigned offset) > { > + struct gpio_rcar_priv *p = gpiochip_get_data(chip); > + > pinctrl_free_gpio(chip->base + offset); > > /* > @@ -254,6 +267,8 @@ static void gpio_rcar_free(struct gpio_chip *chip, > unsigned offset) * drive the GPIO pin as an output. > */ > gpio_rcar_config_general_input_output_mode(chip, offset, false); > + > + pm_runtime_put(&p->pdev->dev); > } > > static int gpio_rcar_direction_input(struct gpio_chip *chip, unsigned > offset) @@ -426,7 +441,6 @@ static int gpio_rcar_probe(struct > platform_device *pdev) } > > pm_runtime_enable(dev); > - pm_runtime_get_sync(dev); > > io = platform_get_resource(pdev, IORESOURCE_MEM, 0); > irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > @@ -495,7 +509,6 @@ static int gpio_rcar_probe(struct platform_device *pdev) > err1: > gpiochip_remove(gpio_chip); > err0: > - pm_runtime_put(dev); > pm_runtime_disable(dev); > return ret; > } > @@ -506,7 +519,6 @@ static int gpio_rcar_remove(struct platform_device > *pdev) > > gpiochip_remove(&p->gpio_chip); > > - pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > return 0; > }
Hi Laurent, On 2016-12-08 23:40:42 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thursday 08 Dec 2016 18:32:28 Niklas Söderlund wrote: > > From: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Currently gpio modules are runtime-resumed at probe time. This means the > > gpio module will be active all the time (except during system suspend, > > if not configured as a wake-up source). > > > > While an R-Car Gen2 gpio module retains pins configured for output at > > the requested level while put in standby mode, gpio register cannot be > > accessed while suspended. Unfortunately pm_runtime_get_sync() cannot be > > called from all contexts where gpio register access is needed. Hence > > move the Runtime PM handling from probe/remove time to gpio request/free > > time, which is probably the best we can do. > > > > On r8a7791/koelsch, gpio modules 0, 1, 3, and 4 are now suspended during > > normal use (gpio2 is used for LEDs and regulators, gpio5 for keys, gpio6 > > for SD-Card CD & WP, gpio7 for keys and regulators). > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > [Niklas: s/gpio_to_priv(chip)/gpiochip_get_data(chip)/] > > Just curious, what's the rationale for this ? This was changed for the whole driver after the original patch was applied (at the time of change the patch was not yet reverted), see [1]. I needed to update this when I resurrected the patch, maybe I could have been more clever and reverted the revert patch but this felt cleaner, if it's better to do it the other way around and revert a revert please let me know so I can do so in the future. [1] c7b6f457cb53bcee ("gpio: rcar: use gpiochip data pointer"). > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > drivers/gpio/gpio-rcar.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > > index 3b77c10..31ad288 100644 > > --- a/drivers/gpio/gpio-rcar.c > > +++ b/drivers/gpio/gpio-rcar.c > > @@ -242,11 +242,24 @@ static void > > gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip, > > > > static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset) > > { > > - return pinctrl_request_gpio(chip->base + offset); > > + struct gpio_rcar_priv *p = gpiochip_get_data(chip); > > + int error; > > + > > + error = pm_runtime_get_sync(&p->pdev->dev); > > + if (error < 0) > > + return error; > > + > > + error = pinctrl_request_gpio(chip->base + offset); > > + if (error) > > + pm_runtime_put(&p->pdev->dev); > > + > > + return error; > > } > > > > static void gpio_rcar_free(struct gpio_chip *chip, unsigned offset) > > { > > + struct gpio_rcar_priv *p = gpiochip_get_data(chip); > > + > > pinctrl_free_gpio(chip->base + offset); > > > > /* > > @@ -254,6 +267,8 @@ static void gpio_rcar_free(struct gpio_chip *chip, > > unsigned offset) * drive the GPIO pin as an output. > > */ > > gpio_rcar_config_general_input_output_mode(chip, offset, false); > > + > > + pm_runtime_put(&p->pdev->dev); > > } > > > > static int gpio_rcar_direction_input(struct gpio_chip *chip, unsigned > > offset) @@ -426,7 +441,6 @@ static int gpio_rcar_probe(struct > > platform_device *pdev) } > > > > pm_runtime_enable(dev); > > - pm_runtime_get_sync(dev); > > > > io = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > @@ -495,7 +509,6 @@ static int gpio_rcar_probe(struct platform_device *pdev) > > err1: > > gpiochip_remove(gpio_chip); > > err0: > > - pm_runtime_put(dev); > > pm_runtime_disable(dev); > > return ret; > > } > > @@ -506,7 +519,6 @@ static int gpio_rcar_remove(struct platform_device > > *pdev) > > > > gpiochip_remove(&p->gpio_chip); > > > > - pm_runtime_put(&pdev->dev); > > pm_runtime_disable(&pdev->dev); > > return 0; > > } > > -- > Regards, > > Laurent Pinchart >
Hi Niklas, On Friday 09 Dec 2016 00:15:51 Niklas Söderlund wrote: > On 2016-12-08 23:40:42 +0200, Laurent Pinchart wrote: > > On Thursday 08 Dec 2016 18:32:28 Niklas Söderlund wrote: > >> From: Geert Uytterhoeven <geert+renesas@glider.be> > >> > >> Currently gpio modules are runtime-resumed at probe time. This means the > >> gpio module will be active all the time (except during system suspend, > >> if not configured as a wake-up source). > >> > >> While an R-Car Gen2 gpio module retains pins configured for output at > >> the requested level while put in standby mode, gpio register cannot be > >> accessed while suspended. Unfortunately pm_runtime_get_sync() cannot be > >> called from all contexts where gpio register access is needed. Hence > >> move the Runtime PM handling from probe/remove time to gpio request/free > >> time, which is probably the best we can do. > >> > >> On r8a7791/koelsch, gpio modules 0, 1, 3, and 4 are now suspended during > >> normal use (gpio2 is used for LEDs and regulators, gpio5 for keys, gpio6 > >> for SD-Card CD & WP, gpio7 for keys and regulators). > >> > >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > >> [Niklas: s/gpio_to_priv(chip)/gpiochip_get_data(chip)/] > > > > Just curious, what's the rationale for this ? > > This was changed for the whole driver after the original patch was > applied (at the time of change the patch was not yet reverted), see [1]. > I needed to update this when I resurrected the patch, maybe I could have > been more clever and reverted the revert patch but this felt cleaner, if > it's better to do it the other way around and revert a revert please let > me know so I can do so in the future. No, this is fine. I'm a bit dubious about [1] given that it consumes more CPU cycles without any benefit as far as I can see. Maybe I can't see far enough though, Linus Walleij could prove me wrong :-) > [1] c7b6f457cb53bcee ("gpio: rcar: use gpiochip data pointer").
Hi Niklas, On Thu, Dec 8, 2016 at 6:32 PM, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Currently gpio modules are runtime-resumed at probe time. This means the > gpio module will be active all the time (except during system suspend, > if not configured as a wake-up source). > > While an R-Car Gen2 gpio module retains pins configured for output at > the requested level while put in standby mode, gpio register cannot be > accessed while suspended. Unfortunately pm_runtime_get_sync() cannot be > called from all contexts where gpio register access is needed. Hence > move the Runtime PM handling from probe/remove time to gpio request/free > time, which is probably the best we can do. > > On r8a7791/koelsch, gpio modules 0, 1, 3, and 4 are now suspended during > normal use (gpio2 is used for LEDs and regulators, gpio5 for keys, gpio6 > for SD-Card CD & WP, gpio7 for keys and regulators). FWIW, gpio module 3 is no longer suspended, as gpio3 is used by HDMI since commit 83a0731b39f3bc24 ("ARM: shmobile: koelsch: Add DU HDMI output support"). > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [Niklas: s/gpio_to_priv(chip)/gpiochip_get_data(chip)/] > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thanks for fixing and resurrecting! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
On Fri, Dec 9, 2016 at 12:21 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Friday 09 Dec 2016 00:15:51 Niklas Söderlund wrote: >> This was changed for the whole driver after the original patch was >> applied (at the time of change the patch was not yet reverted), see [1]. >> I needed to update this when I resurrected the patch, maybe I could have >> been more clever and reverted the revert patch but this felt cleaner, if >> it's better to do it the other way around and revert a revert please let >> me know so I can do so in the future. > > No, this is fine. I'm a bit dubious about [1] given that it consumes more CPU > cycles without any benefit as far as I can see. Maybe I can't see far enough > though, Linus Walleij could prove me wrong :-) This was changed mainly for maintainability and code readability. Lots of subsystems have the option to carry around a data poiner and/or allocate an extra memory chunk for state containers. I am relaxed on it, if someone very much want to use container_of() because of $REASON I am not really bothered. Yours, Linus Walleij -- 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
On Thu, Dec 8, 2016 at 6:32 PM, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Currently gpio modules are runtime-resumed at probe time. This means the > gpio module will be active all the time (except during system suspend, > if not configured as a wake-up source). > > While an R-Car Gen2 gpio module retains pins configured for output at > the requested level while put in standby mode, gpio register cannot be > accessed while suspended. Unfortunately pm_runtime_get_sync() cannot be > called from all contexts where gpio register access is needed. Hence > move the Runtime PM handling from probe/remove time to gpio request/free > time, which is probably the best we can do. > > On r8a7791/koelsch, gpio modules 0, 1, 3, and 4 are now suspended during > normal use (gpio2 is used for LEDs and regulators, gpio5 for keys, gpio6 > for SD-Card CD & WP, gpio7 for keys and regulators). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [Niklas: s/gpio_to_priv(chip)/gpiochip_get_data(chip)/] > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Patch applied. Yours, Linus Walleij -- 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 --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index 3b77c10..31ad288 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -242,11 +242,24 @@ static void gpio_rcar_config_general_input_output_mode(struct gpio_chip *chip, static int gpio_rcar_request(struct gpio_chip *chip, unsigned offset) { - return pinctrl_request_gpio(chip->base + offset); + struct gpio_rcar_priv *p = gpiochip_get_data(chip); + int error; + + error = pm_runtime_get_sync(&p->pdev->dev); + if (error < 0) + return error; + + error = pinctrl_request_gpio(chip->base + offset); + if (error) + pm_runtime_put(&p->pdev->dev); + + return error; } static void gpio_rcar_free(struct gpio_chip *chip, unsigned offset) { + struct gpio_rcar_priv *p = gpiochip_get_data(chip); + pinctrl_free_gpio(chip->base + offset); /* @@ -254,6 +267,8 @@ static void gpio_rcar_free(struct gpio_chip *chip, unsigned offset) * drive the GPIO pin as an output. */ gpio_rcar_config_general_input_output_mode(chip, offset, false); + + pm_runtime_put(&p->pdev->dev); } static int gpio_rcar_direction_input(struct gpio_chip *chip, unsigned offset) @@ -426,7 +441,6 @@ static int gpio_rcar_probe(struct platform_device *pdev) } pm_runtime_enable(dev); - pm_runtime_get_sync(dev); io = platform_get_resource(pdev, IORESOURCE_MEM, 0); irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); @@ -495,7 +509,6 @@ static int gpio_rcar_probe(struct platform_device *pdev) err1: gpiochip_remove(gpio_chip); err0: - pm_runtime_put(dev); pm_runtime_disable(dev); return ret; } @@ -506,7 +519,6 @@ static int gpio_rcar_remove(struct platform_device *pdev) gpiochip_remove(&p->gpio_chip); - pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); return 0; }