| Message ID | 20230717172821.62827-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | pinctrl: Provide NOIRQ PM helper and use it | expand |
Hi Andy, Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/renesas/core.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/drivers/pinctrl/renesas/core.c > b/drivers/pinctrl/renesas/core.c > index 0c8d081da6a8..34232b016960 100644 > --- a/drivers/pinctrl/renesas/core.c > +++ b/drivers/pinctrl/renesas/core.c > @@ -649,7 +649,7 @@ static const struct of_device_id > sh_pfc_of_table[] = { > }; > #endif > > -#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW) > +#if defined(CONFIG_ARM_PSCI_FW) > static void sh_pfc_nop_reg(struct sh_pfc *pfc, u32 reg, unsigned int > idx) > { > } > @@ -732,15 +732,13 @@ static int sh_pfc_resume_noirq(struct device > *dev) > sh_pfc_walk_regs(pfc, sh_pfc_restore_reg); > return 0; > } > - > -static const struct dev_pm_ops sh_pfc_pm = { > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(sh_pfc_suspend_noirq, > sh_pfc_resume_noirq) > -}; > -#define DEV_PM_OPS &sh_pfc_pm > #else > static int sh_pfc_suspend_init(struct sh_pfc *pfc) { return 0; } > -#define DEV_PM_OPS NULL > -#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */ > +static int sh_pfc_suspend_noirq(struct device *dev) { return 0; } > +static int sh_pfc_resume_noirq(struct device *dev) { return 0; } > +#endif /* CONFIG_ARM_PSCI_FW */ > + > +static DEFINE_NOIRQ_DEV_PM_OPS(sh_pfc_pm, sh_pfc_suspend_noirq, > sh_pfc_resume_noirq); > > #ifdef DEBUG > #define SH_PFC_MAX_REGS 300 > @@ -1418,7 +1416,7 @@ static struct platform_driver sh_pfc_driver = { > .driver = { > .name = DRV_NAME, > .of_match_table = of_match_ptr(sh_pfc_of_table), > - .pm = DEV_PM_OPS, > + .pm = pm_sleep_ptr(&sh_pfc_pm), I think you could do: .pm = IF_PTR(IS_ENABLED(CONFIG_ARM_PSCI_FW), pm_sleep_ptr(&sh_pfc_pm)), Then you wouldn't need the #if defined(CONFIG_ARM_PSCI_FW) guard either (as long as the code still compiles fine when that config option is disabled), and you wouldn't need those dummy callbacks. Cheers, -Paul > }, > }; >
Hi Andy, Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > _DEFINE_DEV_PM_OPS() helps to define PM operations for the system > sleep > and/or runtime PM cases. Some of the existing users want to have > _noirq() > variants to be set. For that purpose introduce a new helper which > sets > up _noirq() callbacks to be set and struct dev_pm_ops be provided. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/pm.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/pm.h b/include/linux/pm.h > index badad7d11f4f..0f19af8d5493 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -448,6 +448,15 @@ const struct dev_pm_ops __maybe_unused name = { > \ > SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn) \ > } > > +/* > + * Use this if you want to have the suspend and resume callbacks be > called > + * with disabled IRQs. with disabled IRQs -> with IRQs disabled? I'm not really sure that we need this macro, but I don't really object either. As long as it has callers I guess it's fine, I just don't want <linux/pm.h> to become too bloated and confusing. Anyway: Reviewed-by: Paul Cercueil <paul@crapouillou.net> Cheers, -Paul > + */ > +#define DEFINE_NOIRQ_DEV_PM_OPS(name, suspend_fn, resume_fn) \ > +const struct dev_pm_ops name = { \ > + NOIRQ_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > +} > + > #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr)) > #define pm_sleep_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM_SLEEP), > (_ptr)) >
Hi Andy, Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > Cleaning up the driver to use pm_ptr() and *_PM_OPS() macros that > make it simpler and allows the compiler to remove those functions > if built without CONFIG_PM and CONFIG_PM_SLEEP support. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Paul Cercueil <paul@crapouillou.net> Cheers, -Paul > --- > drivers/pinctrl/intel/pinctrl-lynxpoint.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c > b/drivers/pinctrl/intel/pinctrl-lynxpoint.c > index cdace55aaeac..50d92bf80e20 100644 > --- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c > +++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c > @@ -948,9 +948,8 @@ static int lp_gpio_resume(struct device *dev) > } > > static const struct dev_pm_ops lp_gpio_pm_ops = { > - .runtime_suspend = lp_gpio_runtime_suspend, > - .runtime_resume = lp_gpio_runtime_resume, > - .resume = lp_gpio_resume, > + SYSTEM_SLEEP_PM_OPS(NULL, lp_gpio_resume) > + RUNTIME_PM_OPS(lp_gpio_runtime_suspend, > lp_gpio_runtime_resume, NULL) > }; > > static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = { > @@ -965,7 +964,7 @@ static struct platform_driver lp_gpio_driver = { > .remove = lp_gpio_remove, > .driver = { > .name = "lp_gpio", > - .pm = &lp_gpio_pm_ops, > + .pm = pm_ptr(&lp_gpio_pm_ops), > .acpi_match_table = lynxpoint_gpio_acpi_match, > }, > };
On Mon, Jul 17, 2023 at 10:19 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > I'm not really sure that we need this macro, but I don't really object > either. As long as it has callers I guess it's fine, I just don't want > <linux/pm.h> to become too bloated and confusing. Isn't theidea behind all helpers to simplify life of the users by the cost of bloaring up (a bit) the common file (header and/or C file)? As cover letter shows, despite having several LoCs added to the pm.h we saved already a few dozens of LoCs. And this it not the end, there are more users can come. Moreover, there are some deprecated macros and those that starts with SET_*(). Removing them can make balance go too negative for the pm.h (in terms of +- LoCs). So I can't really consider above as argument. > Anyway: > Reviewed-by: Paul Cercueil <paul@crapouillou.net> Thank you!
On Mon, Jul 17, 2023 at 10:12 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > I think you could do: > > .pm = IF_PTR(IS_ENABLED(CONFIG_ARM_PSCI_FW), pm_sleep_ptr(&sh_pfc_pm)), > > Then you wouldn't need the #if defined(CONFIG_ARM_PSCI_FW) guard either > (as long as the code still compiles fine when that config option is > disabled), and you wouldn't need those dummy callbacks. Thanks for the hint, but it's ugly looking code. If we go this direction, we would need local arm_psci_fw_ptr() macro or so.
Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > > ... > > > Unrelated change. > > OK. > > ... > > > So the correct way to update this driver would be to have a > > conditionally-exported dev_pm_ops structure: > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > intel_pinctrl_resume_noirq), > > }; > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that way, > but it seems pm.h in such case needs EXPORT for NOIRQ case as well. It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is garbage-collected along with all its callbacks. I know it looks ugly, but we already have 4 variants (regular, namespace, GPL, namespace + GPL), if we start to add macros for specific use-cases then it will become bloated really quick. And the "bloat" I'm trying to avoid here is the extreme expansion of the API which makes it hard for people not familiar to the code to understand what should be used and how. > > Then your two callbacks can be "static" and without #ifdef guards. > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in > > the > > pinctrl-intel.h without any guards, as long as it is only > > referenced > > with the pm_ptr() macro. > > I'm not sure I got this. Currently drivers do not have any guards. > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" conditionally depending on CONFIG_PM. We could add variants that export it conditionally depending on CONFIG_PM_SLEEP, but we're back at the problem of adding bloat. You could use pm_sleep_ptr() indeed, with the existing macros, with the drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the dev_pm_ops + callbacks are compiled in but never referenced. Cheers, -Paul
Le lundi 17 juillet 2023 à 22:38 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:12 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > > ... > > > I think you could do: > > > > .pm = IF_PTR(IS_ENABLED(CONFIG_ARM_PSCI_FW), > > pm_sleep_ptr(&sh_pfc_pm)), > > > > Then you wouldn't need the #if defined(CONFIG_ARM_PSCI_FW) guard > > either > > (as long as the code still compiles fine when that config option is > > disabled), and you wouldn't need those dummy callbacks. > > Thanks for the hint, but it's ugly looking code. > If we go this direction, we would need local arm_psci_fw_ptr() macro > or so. > Sure, a small macro works too. -Paul
On Mon, Jul 17, 2023 at 08:28:21PM +0300, Andy Shevchenko wrote: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) Acked-by: Thierry Reding <treding@nvidia.com>
Il 17/07/23 19:28, Andy Shevchenko ha scritto: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 5 +---- > drivers/pinctrl/mediatek/pinctrl-paris.c | 9 +++------ > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > index 665dec419e7c..2bf5082d3aa9 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > @@ -922,10 +922,7 @@ static int mtk_eint_resume(struct device *device) > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_eint_pm_ops = { > - .suspend_noirq = mtk_eint_suspend, > - .resume_noirq = mtk_eint_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_eint_pm_ops, mtk_eint_suspend, mtk_eint_resume); > > static int mtk_pctrl_build_state(struct platform_device *pdev) > { > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 33d6c3fb7908..b1cbd5bafa2e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -1119,24 +1119,21 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev) > } > EXPORT_SYMBOL_GPL(mtk_paris_pinctrl_probe); > > -static int mtk_paris_pinctrl_suspend(struct device *device) > +static int mtk_paris_suspend(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_suspend(pctl->eint); > } > > -static int mtk_paris_pinctrl_resume(struct device *device) > +static int mtk_paris_resume(struct device *device) What's the reason why you changed the suspend/resume function names? I don't really mind, but please at least mention that in the commit description. Thanks, Angelo > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_paris_pinctrl_pm_ops = { > - .suspend_noirq = mtk_paris_pinctrl_suspend, > - .resume_noirq = mtk_paris_pinctrl_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); > > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");
On Mon, Jul 17, 2023 at 7:28 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > _DEFINE_DEV_PM_OPS() helps to define PM operations for the system sleep > and/or runtime PM cases. Some of the existing users want to have _noirq() > variants to be set. For that purpose introduce a new helper which sets > up _noirq() callbacks to be set and struct dev_pm_ops be provided. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Mon, 17 Jul 2023 20:28:15 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 5 +---- > drivers/pinctrl/intel/pinctrl-intel.h | 9 ++------- > 2 files changed, 3 insertions(+), 11 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index 64c3e62b4348..40e45c4889ee 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -929,7 +929,7 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset, > * > * Return: a GPIO offset, or negative error code if translation can't be done. > */ > -static __maybe_unused int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > +static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin) > { > const struct intel_community *community; > const struct intel_padgroup *padgrp; > @@ -1488,7 +1488,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > if (!communities) > return -ENOMEM; > > - > for (i = 0; i < pctrl->ncommunities; i++) { > struct intel_community *community = &pctrl->communities[i]; > u32 *intmask, *hostown; > @@ -1712,7 +1711,6 @@ const struct intel_pinctrl_soc_data *intel_pinctrl_get_soc_data(struct platform_ > } > EXPORT_SYMBOL_GPL(intel_pinctrl_get_soc_data); > > -#ifdef CONFIG_PM_SLEEP > static bool __intel_gpio_is_direct_irq(u32 value) > { > return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) && > @@ -1913,7 +1911,6 @@ int intel_pinctrl_resume_noirq(struct device *dev) > return 0; > } > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > -#endif Can you check if this is successfully removed? I think it won't be. Not immediately obvious how to tidy that up given these are used in a macro called from lots of drivers. Maybe just leaving the ifdef is best we can do here. > > MODULE_AUTHOR("Mathias Nyman <mathias.nyman@linux.intel.com>"); > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h > index 1faf2ada480a..65b1699a2ed5 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.h > +++ b/drivers/pinctrl/intel/pinctrl-intel.h > @@ -255,15 +255,10 @@ struct intel_pinctrl { > int intel_pinctrl_probe_by_hid(struct platform_device *pdev); > int intel_pinctrl_probe_by_uid(struct platform_device *pdev); > > -#ifdef CONFIG_PM_SLEEP > int intel_pinctrl_suspend_noirq(struct device *dev); > int intel_pinctrl_resume_noirq(struct device *dev); > -#endif > > -#define INTEL_PINCTRL_PM_OPS(_name) \ > -const struct dev_pm_ops _name = { \ > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, \ > - intel_pinctrl_resume_noirq) \ > -} > +#define INTEL_PINCTRL_PM_OPS(_name) \ > + DEFINE_NOIRQ_DEV_PM_OPS(_name, intel_pinctrl_suspend_noirq, intel_pinctrl_resume_noirq) > > #endif /* PINCTRL_INTEL_H */
Hi Paul, On Mon, Jul 17, 2023 at 9:12 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > switch the driver to use it instead of open coded variant. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/pinctrl/renesas/core.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pinctrl/renesas/core.c > > b/drivers/pinctrl/renesas/core.c > > index 0c8d081da6a8..34232b016960 100644 > > --- a/drivers/pinctrl/renesas/core.c > > +++ b/drivers/pinctrl/renesas/core.c > > @@ -649,7 +649,7 @@ static const struct of_device_id > > sh_pfc_of_table[] = { > > }; > > #endif > > > > -#if defined(CONFIG_PM_SLEEP) && defined(CONFIG_ARM_PSCI_FW) > > +#if defined(CONFIG_ARM_PSCI_FW) > > static void sh_pfc_nop_reg(struct sh_pfc *pfc, u32 reg, unsigned int > > idx) > > { > > } > > @@ -732,15 +732,13 @@ static int sh_pfc_resume_noirq(struct device > > *dev) > > sh_pfc_walk_regs(pfc, sh_pfc_restore_reg); > > return 0; > > } > > - > > -static const struct dev_pm_ops sh_pfc_pm = { > > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(sh_pfc_suspend_noirq, > > sh_pfc_resume_noirq) > > -}; > > -#define DEV_PM_OPS &sh_pfc_pm > > #else > > static int sh_pfc_suspend_init(struct sh_pfc *pfc) { return 0; } > > -#define DEV_PM_OPS NULL > > -#endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */ > > +static int sh_pfc_suspend_noirq(struct device *dev) { return 0; } > > +static int sh_pfc_resume_noirq(struct device *dev) { return 0; } > > +#endif /* CONFIG_ARM_PSCI_FW */ > > + > > +static DEFINE_NOIRQ_DEV_PM_OPS(sh_pfc_pm, sh_pfc_suspend_noirq, > > sh_pfc_resume_noirq); > > > > #ifdef DEBUG > > #define SH_PFC_MAX_REGS 300 > > @@ -1418,7 +1416,7 @@ static struct platform_driver sh_pfc_driver = { > > .driver = { > > .name = DRV_NAME, > > .of_match_table = of_match_ptr(sh_pfc_of_table), > > - .pm = DEV_PM_OPS, > > + .pm = pm_sleep_ptr(&sh_pfc_pm), > > I think you could do: > > .pm = IF_PTR(IS_ENABLED(CONFIG_ARM_PSCI_FW), pm_sleep_ptr(&sh_pfc_pm)), > > Then you wouldn't need the #if defined(CONFIG_ARM_PSCI_FW) guard either > (as long as the code still compiles fine when that config option is > disabled), and you wouldn't need those dummy callbacks. Unfortunately not, as the code refers to psci_ops.cpu_suspend. You could create a small wrapper for that, though. Gr{oetje,eeting}s, Geert
On Mon, 17 Jul 2023 20:28:16 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Cleaning up the driver to use pm_ptr() and *_PM_OPS() macros that > make it simpler and allows the compiler to remove those functions > if built without CONFIG_PM and CONFIG_PM_SLEEP support. > Those macros add a load more callbacks... Whilst that may well be fine, you should definitely mention that in this patch description. > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-lynxpoint.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c b/drivers/pinctrl/intel/pinctrl-lynxpoint.c > index cdace55aaeac..50d92bf80e20 100644 > --- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c > +++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c > @@ -948,9 +948,8 @@ static int lp_gpio_resume(struct device *dev) > } > > static const struct dev_pm_ops lp_gpio_pm_ops = { > - .runtime_suspend = lp_gpio_runtime_suspend, > - .runtime_resume = lp_gpio_runtime_resume, > - .resume = lp_gpio_resume, > + SYSTEM_SLEEP_PM_OPS(NULL, lp_gpio_resume) > + RUNTIME_PM_OPS(lp_gpio_runtime_suspend, lp_gpio_runtime_resume, NULL) > }; > > static const struct acpi_device_id lynxpoint_gpio_acpi_match[] = { > @@ -965,7 +964,7 @@ static struct platform_driver lp_gpio_driver = { > .remove = lp_gpio_remove, > .driver = { > .name = "lp_gpio", > - .pm = &lp_gpio_pm_ops, > + .pm = pm_ptr(&lp_gpio_pm_ops), > .acpi_match_table = lynxpoint_gpio_acpi_match, > }, > };
On Mon, 17 Jul 2023 20:28:18 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. Good to mention the renames as well. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 5 +---- > drivers/pinctrl/mediatek/pinctrl-paris.c | 9 +++------ > 2 files changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > index 665dec419e7c..2bf5082d3aa9 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c > @@ -922,10 +922,7 @@ static int mtk_eint_resume(struct device *device) > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_eint_pm_ops = { > - .suspend_noirq = mtk_eint_suspend, > - .resume_noirq = mtk_eint_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_eint_pm_ops, mtk_eint_suspend, mtk_eint_resume); > > static int mtk_pctrl_build_state(struct platform_device *pdev) > { > diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.c b/drivers/pinctrl/mediatek/pinctrl-paris.c > index 33d6c3fb7908..b1cbd5bafa2e 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-paris.c > +++ b/drivers/pinctrl/mediatek/pinctrl-paris.c > @@ -1119,24 +1119,21 @@ int mtk_paris_pinctrl_probe(struct platform_device *pdev) > } > EXPORT_SYMBOL_GPL(mtk_paris_pinctrl_probe); > > -static int mtk_paris_pinctrl_suspend(struct device *device) > +static int mtk_paris_suspend(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_suspend(pctl->eint); > } > > -static int mtk_paris_pinctrl_resume(struct device *device) > +static int mtk_paris_resume(struct device *device) > { > struct mtk_pinctrl *pctl = dev_get_drvdata(device); > > return mtk_eint_do_resume(pctl->eint); > } > > -const struct dev_pm_ops mtk_paris_pinctrl_pm_ops = { > - .suspend_noirq = mtk_paris_pinctrl_suspend, > - .resume_noirq = mtk_paris_pinctrl_resume, > -}; > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); > > MODULE_LICENSE("GPL v2"); > MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");
On Mon, 17 Jul 2023 20:28:20 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Since pm.h provides a helper for system no-IRQ PM callbacks, > switch the driver to use it instead of open coded variant. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Slightly more complex case, but looks fine to me. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
On Tue, Jul 18, 2023 at 11:11:43AM +0100, Jonathan Cameron wrote: > On Mon, 17 Jul 2023 20:28:21 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > switch the driver to use it instead of open coded variant. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > No pm_sleep_ptr()? pm_sleep_ptr() is pointless on this driver. This driver is selected by ARCH_TEGRA and ARCH_TEGRA also always selects PM. Thierry
Le mardi 18 juillet 2023 à 13:41 +0200, Thierry Reding a écrit : > On Tue, Jul 18, 2023 at 10:42:47AM +0200, Paul Cercueil wrote: > > Hi Thierry, > > > > Le mardi 18 juillet 2023 à 09:45 +0200, Thierry Reding a écrit : > > > On Mon, Jul 17, 2023 at 09:14:12PM +0200, Paul Cercueil wrote: > > > > Hi Andy, > > > > > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a > > > > écrit : > > > > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > > > > switch the driver to use it instead of open coded variant. > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> > > > > > --- > > > > > drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +---- > > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > index 4547cf66d03b..734c71ef005b 100644 > > > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > @@ -747,10 +747,7 @@ static int tegra_pinctrl_resume(struct > > > > > device > > > > > *dev) > > > > > return 0; > > > > > } > > > > > > > > > > -const struct dev_pm_ops tegra_pinctrl_pm = { > > > > > - .suspend_noirq = &tegra_pinctrl_suspend, > > > > > - .resume_noirq = &tegra_pinctrl_resume > > > > > -}; > > > > > +DEFINE_NOIRQ_DEV_PM_OPS(tegra_pinctrl_pm, > > > > > tegra_pinctrl_suspend, > > > > > tegra_pinctrl_resume); > > > > > > > > > > static bool tegra_pinctrl_gpio_node_has_range(struct > > > > > tegra_pmx > > > > > *pmx) > > > > > { > > > > > > > > Another driver where using EXPORT_GPL_DEV_PM_OPS() would make > > > > more > > > > sense. > > > > > > We don't currently export these PM ops because none of the Tegra > > > pinctrl > > > drivers can be built as a module. > > > > This doesn't change anything. You'd want to use > > EXPORT_GPL_DEV_PM_OPS > > (or better, the namespaced version) so that the PM ops can be > > defined > > in one file and referenced in another, while still having them > > garbage- > > collected when CONFIG_PM is disabled. > > Looking at the definition of EXPORT_GPL_DEV_PM_OPS(), it will cause > an > EXPORT_SYMBOL_GPL() to be added. So there very well is a change. And > it's a completely bogus change because no module is ever going to use > that symbol. If we were to ever support building the pinctrl driver > as > a module, then this would perhaps make sense, but we don't. In this particular case the EXPORT_SYMBOL_GPL() isn't really important, the rest of EXPORT_GPL_DEV_PM_OPS() is. I don't think having a symbol exported it is a big deal, TBH, if you use the namespaced version. If you really don't want that, we need a version of EXPORT_GPL_DEV_PM_OPS() that doesn't export the symbol. -Paul
Hi Thierry, Le mardi 18 juillet 2023 à 13:38 +0200, Thierry Reding a écrit : > On Tue, Jul 18, 2023 at 11:11:43AM +0100, Jonathan Cameron wrote: > > On Mon, 17 Jul 2023 20:28:21 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > > switch the driver to use it instead of open coded variant. > > > > > > Signed-off-by: Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> > > > > No pm_sleep_ptr()? > > pm_sleep_ptr() is pointless on this driver. This driver is selected > by > ARCH_TEGRA and ARCH_TEGRA also always selects PM. If I'm not mistaken, ARCH_TEGRA selects CONFIG_PM, not CONFIG_PM_SLEEP. Cheers, -Paul
On Tue, Jul 18, 2023 at 1:12 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jul 17, 2023 at 9:12 PM Paul Cercueil <paul@crapouillou.net> wrote: > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > > I think you could do: > > > > .pm = IF_PTR(IS_ENABLED(CONFIG_ARM_PSCI_FW), pm_sleep_ptr(&sh_pfc_pm)), > > > > Then you wouldn't need the #if defined(CONFIG_ARM_PSCI_FW) guard either > > (as long as the code still compiles fine when that config option is > > disabled), and you wouldn't need those dummy callbacks. > > Unfortunately not, as the code refers to psci_ops.cpu_suspend. > > You could create a small wrapper for that, though. I think it's already too many wrappers mentioned and since you reviewed and acknowledged the change (thanks!) I will stick with my initial version.
On Tue, Jul 18, 2023 at 12:47 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > Il 17/07/23 19:28, Andy Shevchenko ha scritto: ... > > -static int mtk_paris_pinctrl_suspend(struct device *device) > > +static int mtk_paris_suspend(struct device *device) > > -static int mtk_paris_pinctrl_resume(struct device *device) > > +static int mtk_paris_resume(struct device *device) > > What's the reason why you changed the suspend/resume function names? > I don't really mind, but please at least mention that in the commit description. To put it on a single line. I will amend the commit message, thank you for review! ... > > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, mtk_paris_suspend, mtk_paris_resume); ...here ^^^
On Mon, Jul 17, 2023 at 10:57 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 22:36 +0300, Andy Shevchenko a écrit : > > On Mon, Jul 17, 2023 at 10:07 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > > > > +DEFINE_NOIRQ_DEV_PM_OPS(mtk_paris_pinctrl_pm_ops, > > > > mtk_paris_suspend, > > > > mtk_paris_resume); > > > > > > It's a bit more work, but I think you should use > > > EXPORT_GPL_DEV_PM_OPS > > > (or even better, EXPORT_NS_GPL_DEV_PM_OPS) so that the dev_pm_ops > > > is > > > conditionally exported. All callers would have to be updated to use > > > pm_ptr(). > > > > Why pm_ptr()? What did I miss? > > The rest is OK. > > Or pm_sleep_ptr(). As I said in my answer to the other patch, > EXPORT_*_DEV_PM_OPS() currently only exports on CONFIG_PM, so it > doesn't really matter which one you use. Yes, I need to think about it. I don't like the inconsistency the EXPORT*PM_OPS() brings in this case.
On Tue, Jul 18, 2023 at 2:55 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le mardi 18 juillet 2023 à 13:41 +0200, Thierry Reding a écrit : > > On Tue, Jul 18, 2023 at 10:42:47AM +0200, Paul Cercueil wrote: ... > > Looking at the definition of EXPORT_GPL_DEV_PM_OPS(), it will cause > > an > > EXPORT_SYMBOL_GPL() to be added. So there very well is a change. And > > it's a completely bogus change because no module is ever going to use > > that symbol. If we were to ever support building the pinctrl driver > > as > > a module, then this would perhaps make sense, but we don't. > > In this particular case the EXPORT_SYMBOL_GPL() isn't really important, > the rest of EXPORT_GPL_DEV_PM_OPS() is. > > I don't think having a symbol exported it is a big deal, TBH, if you > use the namespaced version. If you really don't want that, we need a > version of EXPORT_GPL_DEV_PM_OPS() that doesn't export the symbol. Ah, I agree with Thierry and it is another point why I do not like those EXPORT*PM_OPS() macros. Polluting an exported space (even namespaced) is a big deal, so, definitely no from my side.
On Tue, Jul 18, 2023 at 11:43 AM Paul Cercueil <paul@crapouillou.net> wrote: > Le mardi 18 juillet 2023 à 09:45 +0200, Thierry Reding a écrit : ... > (or better, the namespaced version) I am all for consistency, I agree on this whenever the driver is _already_ using namespaces. Having one macro with namespace and disrupting tons of the drivers (MediaTek case?) is not an option in my opinion.
On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <paul@crapouillou.net> wrote: > Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil <paul@crapouillou.net> > > wrote: > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit : ... > > > So the correct way to update this driver would be to have a > > > conditionally-exported dev_pm_ops structure: > > > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > > intel_pinctrl_resume_noirq), > > > }; > > > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that way, > > but it seems pm.h in such case needs EXPORT for NOIRQ case as well. > > It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is > garbage-collected along with all its callbacks. > > I know it looks ugly, but we already have 4 variants (regular, > namespace, GPL, namespace + GPL), if we start to add macros for > specific use-cases then it will become bloated really quick. Maybe macros can be replaced / changed to make it scale? > And the "bloat" I'm trying to avoid here is the extreme expansion of > the API which makes it hard for people not familiar to the code to > understand what should be used and how. So far, based on the rest of the messages in the thread the EXPORT*PM_OPS() have the following issues: 1) do not scale (for variants with different scope we need new set of macros); 2) do not cover cases with pm_sleep_ptr(); 3) export symbols in case when it's not needed. Am I right? > > > Then your two callbacks can be "static" and without #ifdef guards. > > > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" in > > > the > > > pinctrl-intel.h without any guards, as long as it is only > > > referenced > > > with the pm_ptr() macro. > > > > I'm not sure I got this. Currently drivers do not have any guards. > > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" > conditionally depending on CONFIG_PM. We could add variants that export > it conditionally depending on CONFIG_PM_SLEEP, but we're back at the > problem of adding bloat. Exactly. > You could use pm_sleep_ptr() indeed, with the existing macros, with the > drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the > dev_pm_ops + callbacks are compiled in but never referenced. And exactly. I don't think they are ready to use (in the current form). But let's see what we may do better here... -- With Best Regards, Andy Shevchenko
On Tue, Jul 18, 2023 at 02:01:27PM +0200, Paul Cercueil wrote: > Hi Thierry, > > Le mardi 18 juillet 2023 à 13:38 +0200, Thierry Reding a écrit : > > On Tue, Jul 18, 2023 at 11:11:43AM +0100, Jonathan Cameron wrote: > > > On Mon, 17 Jul 2023 20:28:21 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > > > switch the driver to use it instead of open coded variant. > > > > > > > > Signed-off-by: Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> > > > > > > No pm_sleep_ptr()? > > > > pm_sleep_ptr() is pointless on this driver. This driver is selected > > by > > ARCH_TEGRA and ARCH_TEGRA also always selects PM. > > If I'm not mistaken, ARCH_TEGRA selects CONFIG_PM, not CONFIG_PM_SLEEP. Indeed. I suppose pm_sleep_ptr() would make sense, then. Thierry
On Tue, Jul 18, 2023 at 01:55:05PM +0200, Paul Cercueil wrote: > Le mardi 18 juillet 2023 à 13:41 +0200, Thierry Reding a écrit : > > On Tue, Jul 18, 2023 at 10:42:47AM +0200, Paul Cercueil wrote: > > > Hi Thierry, > > > > > > Le mardi 18 juillet 2023 à 09:45 +0200, Thierry Reding a écrit : > > > > On Mon, Jul 17, 2023 at 09:14:12PM +0200, Paul Cercueil wrote: > > > > > Hi Andy, > > > > > > > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a > > > > > écrit : > > > > > > Since pm.h provides a helper for system no-IRQ PM callbacks, > > > > > > switch the driver to use it instead of open coded variant. > > > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > > <andriy.shevchenko@linux.intel.com> > > > > > > --- > > > > > > drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +---- > > > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > index 4547cf66d03b..734c71ef005b 100644 > > > > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > @@ -747,10 +747,7 @@ static int tegra_pinctrl_resume(struct > > > > > > device > > > > > > *dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -const struct dev_pm_ops tegra_pinctrl_pm = { > > > > > > - .suspend_noirq = &tegra_pinctrl_suspend, > > > > > > - .resume_noirq = &tegra_pinctrl_resume > > > > > > -}; > > > > > > +DEFINE_NOIRQ_DEV_PM_OPS(tegra_pinctrl_pm, > > > > > > tegra_pinctrl_suspend, > > > > > > tegra_pinctrl_resume); > > > > > > > > > > > > static bool tegra_pinctrl_gpio_node_has_range(struct > > > > > > tegra_pmx > > > > > > *pmx) > > > > > > { > > > > > > > > > > Another driver where using EXPORT_GPL_DEV_PM_OPS() would make > > > > > more > > > > > sense. > > > > > > > > We don't currently export these PM ops because none of the Tegra > > > > pinctrl > > > > drivers can be built as a module. > > > > > > This doesn't change anything. You'd want to use > > > EXPORT_GPL_DEV_PM_OPS > > > (or better, the namespaced version) so that the PM ops can be > > > defined > > > in one file and referenced in another, while still having them > > > garbage- > > > collected when CONFIG_PM is disabled. > > > > Looking at the definition of EXPORT_GPL_DEV_PM_OPS(), it will cause > > an > > EXPORT_SYMBOL_GPL() to be added. So there very well is a change. And > > it's a completely bogus change because no module is ever going to use > > that symbol. If we were to ever support building the pinctrl driver > > as > > a module, then this would perhaps make sense, but we don't. > > In this particular case the EXPORT_SYMBOL_GPL() isn't really important, > the rest of EXPORT_GPL_DEV_PM_OPS() is. > > I don't think having a symbol exported it is a big deal, TBH, if you > use the namespaced version. If you really don't want that, we need a > version of EXPORT_GPL_DEV_PM_OPS() that doesn't export the symbol. I do think it's a big deal to export a symbol if there's no reason to do so. And please, can we stop adding these macros for every possible scenario? Maybe I'm just getting old, but I find it increasingly difficult to understand what all of these are supposed to be. I get that people want to get rid of boilerplate, but I think we need to more carefully balance boilerplate vs. simplicity. I'm seeing the same thing with stuff like those mass conversions to atrocities like devm_platform_ioremap_resource() and devm_platform_get_and_ioremap_resource(). There's so much churn involved in getting those merged for usually saving a single line of code. And it's not even mass conversions, but people tend to send these as one patch per driver, which doesn't exactly help (except perhaps for patch statistics). Thierry
Hi Thierry, Le mardi 18 juillet 2023 à 15:20 +0200, Thierry Reding a écrit : > On Tue, Jul 18, 2023 at 01:55:05PM +0200, Paul Cercueil wrote: > > Le mardi 18 juillet 2023 à 13:41 +0200, Thierry Reding a écrit : > > > On Tue, Jul 18, 2023 at 10:42:47AM +0200, Paul Cercueil wrote: > > > > Hi Thierry, > > > > > > > > Le mardi 18 juillet 2023 à 09:45 +0200, Thierry Reding a > > > > écrit : > > > > > On Mon, Jul 17, 2023 at 09:14:12PM +0200, Paul Cercueil > > > > > wrote: > > > > > > Hi Andy, > > > > > > > > > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a > > > > > > écrit : > > > > > > > Since pm.h provides a helper for system no-IRQ PM > > > > > > > callbacks, > > > > > > > switch the driver to use it instead of open coded > > > > > > > variant. > > > > > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > > > <andriy.shevchenko@linux.intel.com> > > > > > > > --- > > > > > > > drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +---- > > > > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > > b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > > index 4547cf66d03b..734c71ef005b 100644 > > > > > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c > > > > > > > @@ -747,10 +747,7 @@ static int > > > > > > > tegra_pinctrl_resume(struct > > > > > > > device > > > > > > > *dev) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > -const struct dev_pm_ops tegra_pinctrl_pm = { > > > > > > > - .suspend_noirq = &tegra_pinctrl_suspend, > > > > > > > - .resume_noirq = &tegra_pinctrl_resume > > > > > > > -}; > > > > > > > +DEFINE_NOIRQ_DEV_PM_OPS(tegra_pinctrl_pm, > > > > > > > tegra_pinctrl_suspend, > > > > > > > tegra_pinctrl_resume); > > > > > > > > > > > > > > static bool tegra_pinctrl_gpio_node_has_range(struct > > > > > > > tegra_pmx > > > > > > > *pmx) > > > > > > > { > > > > > > > > > > > > Another driver where using EXPORT_GPL_DEV_PM_OPS() would > > > > > > make > > > > > > more > > > > > > sense. > > > > > > > > > > We don't currently export these PM ops because none of the > > > > > Tegra > > > > > pinctrl > > > > > drivers can be built as a module. > > > > > > > > This doesn't change anything. You'd want to use > > > > EXPORT_GPL_DEV_PM_OPS > > > > (or better, the namespaced version) so that the PM ops can be > > > > defined > > > > in one file and referenced in another, while still having them > > > > garbage- > > > > collected when CONFIG_PM is disabled. > > > > > > Looking at the definition of EXPORT_GPL_DEV_PM_OPS(), it will > > > cause > > > an > > > EXPORT_SYMBOL_GPL() to be added. So there very well is a change. > > > And > > > it's a completely bogus change because no module is ever going to > > > use > > > that symbol. If we were to ever support building the pinctrl > > > driver > > > as > > > a module, then this would perhaps make sense, but we don't. > > > > In this particular case the EXPORT_SYMBOL_GPL() isn't really > > important, > > the rest of EXPORT_GPL_DEV_PM_OPS() is. > > > > I don't think having a symbol exported it is a big deal, TBH, if > > you > > use the namespaced version. If you really don't want that, we need > > a > > version of EXPORT_GPL_DEV_PM_OPS() that doesn't export the symbol. > > I do think it's a big deal to export a symbol if there's no reason to > do > so. > > And please, can we stop adding these macros for every possible > scenario? Yes, as you can read from my other responses, I am not really keen on having a multiplication of these macros. > Maybe I'm just getting old, but I find it increasingly difficult to > understand what all of these are supposed to be. I get that people > want > to get rid of boilerplate, but I think we need to more carefully > balance > boilerplate vs. simplicity. The EXPORT_GPL_DEV_PM_OPS() macro does more than get rid of boilerplate, it gets rid of dead code. If we take this driver as an example, before the patch the "tegra_pinctrl_pm" struct, as well as the "tegra_pinctrl_suspend" and "tegra_pinctrl_resume" functions were always compiled in, even if CONFIG_PM_SLEEP is disabled in the config. The status-quo before the introduction of the new PM macros was to just wrap the dev_pm_ops struct + callbacks with a #ifdef CONFIG_PM_SLEEP. This was pretty bad as the code was then conditionally compiled. With the new PM macros this code is always compiled, independently of any Kconfig option; and thanks to that, bugs and regressions are subsequently easier to catch. Cheers, -Paul > I'm seeing the same thing with stuff like those mass conversions to > atrocities like devm_platform_ioremap_resource() and > devm_platform_get_and_ioremap_resource(). There's so much churn > involved > in getting those merged for usually saving a single line of code. And > it's not even mass conversions, but people tend to send these as one > patch per driver, which doesn't exactly help (except perhaps for > patch > statistics). > > Thierry
On Tue, Jul 18, 2023 at 11:04:51AM +0100, Jonathan Cameron wrote: > On Mon, 17 Jul 2023 20:28:15 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > > Can you check if this is successfully removed? I think it won't be. > Not immediately obvious how to tidy that up given these are used > in a macro called from lots of drivers. That's what Paul noticed I think with his proposal to export only the ops variable and make these to be static. > Maybe just leaving the ifdef is best we can do here. See above.
On Tue, Jul 18, 2023 at 11:06:20AM +0100, Jonathan Cameron wrote: > On Mon, 17 Jul 2023 20:28:16 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Cleaning up the driver to use pm_ptr() and *_PM_OPS() macros that > > make it simpler and allows the compiler to remove those functions > > if built without CONFIG_PM and CONFIG_PM_SLEEP support. > Those macros I believe you meant here "The SYSTEM_SLEEP... macro..." Or is runtime PM also altered? Hmm... > add a load more callbacks... Whilst that may well be fine, > you should definitely mention that in this patch description. Sure.
Hi Andy, Le mardi 18 juillet 2023 à 15:57 +0300, Andy Shevchenko a écrit : > On Mon, Jul 17, 2023 at 10:56 PM Paul Cercueil <paul@crapouillou.net> > wrote: > > Le lundi 17 juillet 2023 à 22:33 +0300, Andy Shevchenko a écrit : > > > On Mon, Jul 17, 2023 at 10:02 PM Paul Cercueil > > > <paul@crapouillou.net> > > > wrote: > > > > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit > > > > : > > ... > > > > > So the correct way to update this driver would be to have a > > > > conditionally-exported dev_pm_ops structure: > > > > > > > > EXPORT_GPL_DEV_PM_OPS(intel_pinctrl_pm_ops) = { > > > > NOIRQ_SYSTEM_SLEEP_PM_OPS(intel_pinctrl_suspend_noirq, > > > > intel_pinctrl_resume_noirq), > > > > }; > > > > > > This looks ugly. I didn't know that EXPORT*PM_OPS designed that > > > way, > > > but it seems pm.h in such case needs EXPORT for NOIRQ case as > > > well. > > > > It's designed so that when CONFIG_PM is disabled, the dev_pm_ops is > > garbage-collected along with all its callbacks. > > > > I know it looks ugly, but we already have 4 variants (regular, > > namespace, GPL, namespace + GPL), if we start to add macros for > > specific use-cases then it will become bloated really quick. > > Maybe macros can be replaced / changed to make it scale? If you have any ideas, then yes absolutely. > > > And the "bloat" I'm trying to avoid here is the extreme expansion > > of > > the API which makes it hard for people not familiar to the code to > > understand what should be used and how. > > So far, based on the rest of the messages in the thread the > EXPORT*PM_OPS() have the following issues: > 1) do not scale (for variants with different scope we need new set of > macros); > 2) do not cover cases with pm_sleep_ptr(); > 3) export symbols in case when it's not needed. > > Am I right? I think that's right yes. > > > > > Then your two callbacks can be "static" and without #ifdef > > > > guards. > > > > > > > > The resulting "intel_pinctrl_pm_ops" can be marked as "extern" > > > > in > > > > the > > > > pinctrl-intel.h without any guards, as long as it is only > > > > referenced > > > > with the pm_ptr() macro. > > > > > > I'm not sure I got this. Currently drivers do not have any > > > guards. > > > Moreover, the correct one for noirq is pm_sleep_ptr(), isn't it? > > > > The EXPORT_*_DEV_PM_OPS() macros do export the "dev_pm_ops" > > conditionally depending on CONFIG_PM. We could add variants that > > export > > it conditionally depending on CONFIG_PM_SLEEP, but we're back at > > the > > problem of adding bloat. > > Exactly. > > > You could use pm_sleep_ptr() indeed, with the existing macros, with > > the > > drawback that in the case where CONFIG_PM && !CONFIG_PM_SLEEP, the > > dev_pm_ops + callbacks are compiled in but never referenced. > > And exactly. > > I don't think they are ready to use (in the current form). But let's > see what we may do better here... They were OK when Jonathan and myself were updating the IIO drivers - but now they definitely show their limitations. Cheers, -Paul
On Tue, 18 Jul 2023 16:53:29 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Jul 18, 2023 at 11:04:51AM +0100, Jonathan Cameron wrote: > > On Mon, 17 Jul 2023 20:28:15 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > EXPORT_SYMBOL_GPL(intel_pinctrl_resume_noirq); > > > > Can you check if this is successfully removed? I think it won't be. > > Not immediately obvious how to tidy that up given these are used > > in a macro called from lots of drivers. > > That's what Paul noticed I think with his proposal to export only the ops > variable and make these to be static. > > > Maybe just leaving the ifdef is best we can do here. > > See above. > Ah. I noticed it was a macro, but not that all it did was set the name of the resulting structure (so thought you couldn't use the export approach). Indeed that's the best option here Jonathan
On Mon, Jul 17, 2023 at 08:28:11PM +0300, Andy Shevchenko wrote: > Intel pin control drivers use NOIRQ variant of the PM callbacks. > Besides that several other drivers do similar. Provide a helper > to make them smaller and less error prone against different > kernel configurations (with possible defined but not used variables). > > The idea is to have an immutable branch that PM tree can pull as well as > main pin control one. We also can do other way around, if Rafael prefers > that. I have partially applied the series to my review and testing queue with the following changes (besides the tags added): - split pm_ptr() patches to be first with lynxpoint commit message updated - fixed wording in the pm.h comment - amended cherryview to wrap long line - explained __maybe_unused and pm_ptr() changes in at91 commit message - added pm_sleep_ptr() and explained that in the tegra commit message - renesas and mvebu went as is - intel and mediatek left aside for better rework drivers/pinctrl/intel/pinctrl-baytrail.c | 11 +++-------- drivers/pinctrl/intel/pinctrl-cherryview.c | 10 +++------- drivers/pinctrl/intel/pinctrl-lynxpoint.c | 7 +++---- drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 14 +++----------- drivers/pinctrl/pinctrl-at91.c | 10 ++++------ drivers/pinctrl/renesas/core.c | 16 +++++++--------- drivers/pinctrl/tegra/pinctrl-tegra.c | 5 +---- drivers/pinctrl/tegra/pinctrl-tegra210.c | 2 +- include/linux/pm.h | 9 +++++++++ 9 files changed, 34 insertions(+), 50 deletions(-)