| Message ID | 1514554304-18989-4-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | renesas: irqchip: Use WAKEUP_PATH driver PM flag | expand |
On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable > when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's > module clock (if exists) is manually kept running during system suspend, to > make sure the device stays active. > > However, this explicit clock handling is merely a workaround for a failure > to properly communicate wakeup information to the PM core. Instead, set the > WAKEUP_PATH driver PM flag to indicate that the device is part of the > wakeup path, which further also enables middle-layers and PM domains (like > genpd) to act on this. > > In case the device is attached to genpd and depending on if it has an > active wakeup configuration, genpd will keep the device active (the clock > running) during system suspend when needed. This enables us to remove all > explicit clock handling code from the driver, so let's do that as well. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [Ulf: Converted to use the WAKEUP_PATH driver PM flag] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Acked-by: Linus Walleij <linus.walleij@linaro.org> I guess it is dependent on the other patches? If you want me to just apply it to the GPIO tree, tell me. 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
Hi Linus, On Tue, Jan 2, 2018 at 10:27 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> From: Geert Uytterhoeven <geert+renesas@glider.be> >> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable >> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's >> module clock (if exists) is manually kept running during system suspend, to >> make sure the device stays active. >> >> However, this explicit clock handling is merely a workaround for a failure >> to properly communicate wakeup information to the PM core. Instead, set the >> WAKEUP_PATH driver PM flag to indicate that the device is part of the >> wakeup path, which further also enables middle-layers and PM domains (like >> genpd) to act on this. >> >> In case the device is attached to genpd and depending on if it has an >> active wakeup configuration, genpd will keep the device active (the clock >> running) during system suspend when needed. This enables us to remove all >> explicit clock handling code from the driver, so let's do that as well. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > I guess it is dependent on the other patches? Yes, it depends on (a) a clock patch queued in clk-next for v4.16, and (b) a PM patch introducing WAKEUP_PATH. Applying it prematurely will cause build or runtime issues. 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 Tue, Jan 2, 2018 at 10:37 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Linus, > > On Tue, Jan 2, 2018 at 10:27 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> From: Geert Uytterhoeven <geert+renesas@glider.be> >>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable >>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's >>> module clock (if exists) is manually kept running during system suspend, to >>> make sure the device stays active. >>> >>> However, this explicit clock handling is merely a workaround for a failure >>> to properly communicate wakeup information to the PM core. Instead, set the >>> WAKEUP_PATH driver PM flag to indicate that the device is part of the >>> wakeup path, which further also enables middle-layers and PM domains (like >>> genpd) to act on this. >>> >>> In case the device is attached to genpd and depending on if it has an >>> active wakeup configuration, genpd will keep the device active (the clock >>> running) during system suspend when needed. This enables us to remove all >>> explicit clock handling code from the driver, so let's do that as well. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> >> I guess it is dependent on the other patches? > > Yes, it depends on (a) a clock patch queued in clk-next for v4.16, and (b) a PM > patch introducing WAKEUP_PATH. Applying it prematurely will cause build > or runtime issues. Plus, I'm not going to apply the WAKEUP_PATH thing just yet. At least not in its current version. Thanks, Rafael -- 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 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > From: Geert Uytterhoeven <geert+renesas@glider.be> > > Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable > when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's > module clock (if exists) is manually kept running during system suspend, to > make sure the device stays active. > > However, this explicit clock handling is merely a workaround for a failure > to properly communicate wakeup information to the PM core. Instead, set the > WAKEUP_PATH driver PM flag to indicate that the device is part of the > wakeup path, which further also enables middle-layers and PM domains (like > genpd) to act on this. > > In case the device is attached to genpd and depending on if it has an > active wakeup configuration, genpd will keep the device active (the clock > running) during system suspend when needed. This enables us to remove all > explicit clock handling code from the driver, so let's do that as well. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > [Ulf: Converted to use the WAKEUP_PATH driver PM flag] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/gpio/gpio-rcar.c | 40 +++++++++++++++------------------------- > 1 file changed, 15 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > index e76de57..d414511 100644 > --- a/drivers/gpio/gpio-rcar.c > +++ b/drivers/gpio/gpio-rcar.c > @@ -14,7 +14,6 @@ > * GNU General Public License for more details. > */ > > -#include <linux/clk.h> > #include <linux/err.h> > #include <linux/gpio.h> > #include <linux/init.h> > @@ -37,10 +36,9 @@ struct gpio_rcar_priv { > struct platform_device *pdev; > struct gpio_chip gpio_chip; > struct irq_chip irq_chip; > - struct clk *clk; > unsigned int irq_parent; > bool has_both_edge_trigger; > - bool needs_clk; > + bool wakeup_path; > }; > > #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */ > @@ -186,14 +184,7 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, unsigned int on) > } > } > > - if (!p->clk) > - return 0; > - > - if (on) > - clk_enable(p->clk); > - else > - clk_disable(p->clk); > - > + p->wakeup_path = on; > return 0; > } > > @@ -330,17 +321,14 @@ static int gpio_rcar_direction_output(struct gpio_chip *chip, unsigned offset, > > struct gpio_rcar_info { > bool has_both_edge_trigger; > - bool needs_clk; > }; > > static const struct gpio_rcar_info gpio_rcar_info_gen1 = { > .has_both_edge_trigger = false, > - .needs_clk = false, > }; > > static const struct gpio_rcar_info gpio_rcar_info_gen2 = { > .has_both_edge_trigger = true, > - .needs_clk = true, > }; > > static const struct of_device_id gpio_rcar_of_table[] = { > @@ -403,7 +391,6 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) > ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args); > *npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK; > p->has_both_edge_trigger = info->has_both_edge_trigger; > - p->needs_clk = info->needs_clk; > > if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) { > dev_warn(&p->pdev->dev, > @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int gpio_rcar_suspend(struct device *dev) > +{ > + struct gpio_rcar_priv *p = dev_get_drvdata(dev); > + > + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); Why don't you simply set dev->power.wakeup_path here? > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, NULL); > + > static int gpio_rcar_probe(struct platform_device *pdev) > { > struct gpio_rcar_priv *p; Thanks, Rafael -- 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
Hi Rafael, On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> From: Geert Uytterhoeven <geert+renesas@glider.be> >> >> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable >> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's >> module clock (if exists) is manually kept running during system suspend, to >> make sure the device stays active. >> >> However, this explicit clock handling is merely a workaround for a failure >> to properly communicate wakeup information to the PM core. Instead, set the >> WAKEUP_PATH driver PM flag to indicate that the device is part of the >> wakeup path, which further also enables middle-layers and PM domains (like >> genpd) to act on this. >> >> In case the device is attached to genpd and depending on if it has an >> active wakeup configuration, genpd will keep the device active (the clock >> running) during system suspend when needed. This enables us to remove all >> explicit clock handling code from the driver, so let's do that as well. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n? >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- a/drivers/gpio/gpio-rcar.c >> +++ b/drivers/gpio/gpio-rcar.c >> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) >> return 0; >> } >> >> +#ifdef CONFIG_PM_SLEEP >> +static int gpio_rcar_suspend(struct device *dev) >> +{ >> + struct gpio_rcar_priv *p = dev_get_drvdata(dev); >> + >> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); > > Why don't you simply set dev->power.wakeup_path here? That's what my v1 did (https://patchwork.kernel.org/patch/10050995/). 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 Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rafael, > > On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> From: Geert Uytterhoeven <geert+renesas@glider.be> >>> >>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable >>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's >>> module clock (if exists) is manually kept running during system suspend, to >>> make sure the device stays active. >>> >>> However, this explicit clock handling is merely a workaround for a failure >>> to properly communicate wakeup information to the PM core. Instead, set the >>> WAKEUP_PATH driver PM flag to indicate that the device is part of the >>> wakeup path, which further also enables middle-layers and PM domains (like >>> genpd) to act on this. >>> >>> In case the device is attached to genpd and depending on if it has an >>> active wakeup configuration, genpd will keep the device active (the clock >>> running) during system suspend when needed. This enables us to remove all >>> explicit clock handling code from the driver, so let's do that as well. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] > > Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n? > >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > >>> --- a/drivers/gpio/gpio-rcar.c >>> +++ b/drivers/gpio/gpio-rcar.c > >>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) >>> return 0; >>> } >>> >>> +#ifdef CONFIG_PM_SLEEP >>> +static int gpio_rcar_suspend(struct device *dev) >>> +{ >>> + struct gpio_rcar_priv *p = dev_get_drvdata(dev); >>> + >>> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); >> >> Why don't you simply set dev->power.wakeup_path here? > > That's what my v1 did (https://patchwork.kernel.org/patch/10050995/). I very much prefer this one. :-) What's wrong with it? Thanks, Rafael -- 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 2 January 2018 at 11:48, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> Hi Rafael, >> >> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> From: Geert Uytterhoeven <geert+renesas@glider.be> >>>> >>>> Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable >>>> when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO block's >>>> module clock (if exists) is manually kept running during system suspend, to >>>> make sure the device stays active. >>>> >>>> However, this explicit clock handling is merely a workaround for a failure >>>> to properly communicate wakeup information to the PM core. Instead, set the >>>> WAKEUP_PATH driver PM flag to indicate that the device is part of the >>>> wakeup path, which further also enables middle-layers and PM domains (like >>>> genpd) to act on this. >>>> >>>> In case the device is attached to genpd and depending on if it has an >>>> active wakeup configuration, genpd will keep the device active (the clock >>>> running) during system suspend when needed. This enables us to remove all >>>> explicit clock handling code from the driver, so let's do that as well. >>>> >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] >> >> Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n? >> >>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> >>>> --- a/drivers/gpio/gpio-rcar.c >>>> +++ b/drivers/gpio/gpio-rcar.c >> >>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_PM_SLEEP >>>> +static int gpio_rcar_suspend(struct device *dev) >>>> +{ >>>> + struct gpio_rcar_priv *p = dev_get_drvdata(dev); >>>> + >>>> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); >>> >>> Why don't you simply set dev->power.wakeup_path here? >> >> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/). > > I very much prefer this one. :-) Okay! The reason why I suggested inventing a new driver PM flag, was because I consider the ->dev.power.wakeup_path, being a status flag/bit, owned by the PM core. In other words, consumers of the flag are allowed to look at it, but not change it. Anyway, I am perfectly fine to drop the DPM_FLAG_WAKEUP_PATH thingy. However, perhaps we should still add a helper function (device_set_wakeup_path() or similar), which users can call to set the flag? > > What's wrong with it? It works, although I would rather change the assignment of the flag to respect if the current value is true, something like this: dev->power.wakeup_path = dev->power.wakeup_path || p->wakeup_path; Kind regards Uffe -- 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
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] > > Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n? Oh, yes - correct! The code looks nicer, with the penalty of one static struct declared and not used, in case CONFIG_PM_SLEEP is unset. Should I revert back to your proposal, I am fine with whatever? [...] Kind regards Uffe -- 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 Tue, Jan 2, 2018 at 1:53 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 2 January 2018 at 11:48, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: >>>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>> --- a/drivers/gpio/gpio-rcar.c >>>>> +++ b/drivers/gpio/gpio-rcar.c >>> >>>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) >>>>> return 0; >>>>> } >>>>> >>>>> +#ifdef CONFIG_PM_SLEEP >>>>> +static int gpio_rcar_suspend(struct device *dev) >>>>> +{ >>>>> + struct gpio_rcar_priv *p = dev_get_drvdata(dev); >>>>> + >>>>> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); >>>> >>>> Why don't you simply set dev->power.wakeup_path here? >>> >>> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/). >> >> I very much prefer this one. :-) > > Okay! >> What's wrong with it? > > It works, although I would rather change the assignment of the flag to > respect if the current value is true, something like this: > > dev->power.wakeup_path = dev->power.wakeup_path || p->wakeup_path; dev->power.wakeup_path |= p->wakeup_path? 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
Hi Ulf, On Tue, Jan 2, 2018 at 2:02 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> >>>> [Ulf: Converted to use the WAKEUP_PATH driver PM flag] >> >> Ulf: + killing the DEV_PM_OPS define, increasing kernel size if PM_SUSPEND=n? > > Oh, yes - correct! > > The code looks nicer, with the penalty of one static struct declared > and not used, in case CONFIG_PM_SLEEP is unset. At 23 pointers of 4 or 8 bytes each in 3 drivers, I don't consider this insignificant. Fortunately this driver is not used on RZ/A1, which you can run without external RAM if you manage to fit everything in 10 MiB of SRAM... > Should I revert back to your proposal, I am fine with whatever? Yes please. 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 2 January 2018 at 14:53, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Jan 2, 2018 at 1:53 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 2 January 2018 at 11:48, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> On Tue, Jan 2, 2018 at 11:44 AM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Tue, Jan 2, 2018 at 11:32 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: >>>>> On Fri, Dec 29, 2017 at 2:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>>>>> --- a/drivers/gpio/gpio-rcar.c >>>>>> +++ b/drivers/gpio/gpio-rcar.c >>>> >>>>>> @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +#ifdef CONFIG_PM_SLEEP >>>>>> +static int gpio_rcar_suspend(struct device *dev) >>>>>> +{ >>>>>> + struct gpio_rcar_priv *p = dev_get_drvdata(dev); >>>>>> + >>>>>> + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); >>>>> >>>>> Why don't you simply set dev->power.wakeup_path here? >>>> >>>> That's what my v1 did (https://patchwork.kernel.org/patch/10050995/). >>> >>> I very much prefer this one. :-) >> >> Okay! > >>> What's wrong with it? >> >> It works, although I would rather change the assignment of the flag to >> respect if the current value is true, something like this: >> >> dev->power.wakeup_path = dev->power.wakeup_path || p->wakeup_path; > > dev->power.wakeup_path |= p->wakeup_path? Yeah, correct. Br Uffe -- 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 e76de57..d414511 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -14,7 +14,6 @@ * GNU General Public License for more details. */ -#include <linux/clk.h> #include <linux/err.h> #include <linux/gpio.h> #include <linux/init.h> @@ -37,10 +36,9 @@ struct gpio_rcar_priv { struct platform_device *pdev; struct gpio_chip gpio_chip; struct irq_chip irq_chip; - struct clk *clk; unsigned int irq_parent; bool has_both_edge_trigger; - bool needs_clk; + bool wakeup_path; }; #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */ @@ -186,14 +184,7 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, unsigned int on) } } - if (!p->clk) - return 0; - - if (on) - clk_enable(p->clk); - else - clk_disable(p->clk); - + p->wakeup_path = on; return 0; } @@ -330,17 +321,14 @@ static int gpio_rcar_direction_output(struct gpio_chip *chip, unsigned offset, struct gpio_rcar_info { bool has_both_edge_trigger; - bool needs_clk; }; static const struct gpio_rcar_info gpio_rcar_info_gen1 = { .has_both_edge_trigger = false, - .needs_clk = false, }; static const struct gpio_rcar_info gpio_rcar_info_gen2 = { .has_both_edge_trigger = true, - .needs_clk = true, }; static const struct of_device_id gpio_rcar_of_table[] = { @@ -403,7 +391,6 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args); *npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK; p->has_both_edge_trigger = info->has_both_edge_trigger; - p->needs_clk = info->needs_clk; if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) { dev_warn(&p->pdev->dev, @@ -415,6 +402,18 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) return 0; } +#ifdef CONFIG_PM_SLEEP +static int gpio_rcar_suspend(struct device *dev) +{ + struct gpio_rcar_priv *p = dev_get_drvdata(dev); + + dev_pm_set_driver_flags(dev, p->wakeup_path ? DPM_FLAG_WAKEUP_PATH : 0); + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, NULL); + static int gpio_rcar_probe(struct platform_device *pdev) { struct gpio_rcar_priv *p; @@ -440,16 +439,6 @@ static int gpio_rcar_probe(struct platform_device *pdev) platform_set_drvdata(pdev, p); - p->clk = devm_clk_get(dev, NULL); - if (IS_ERR(p->clk)) { - if (p->needs_clk) { - dev_err(dev, "unable to get clock\n"); - ret = PTR_ERR(p->clk); - goto err0; - } - p->clk = NULL; - } - pm_runtime_enable(dev); irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); @@ -536,6 +525,7 @@ static struct platform_driver gpio_rcar_device_driver = { .remove = gpio_rcar_remove, .driver = { .name = "gpio_rcar", + .pm = &gpio_rcar_pm_ops, .of_match_table = of_match_ptr(gpio_rcar_of_table), } };