| Message ID | 20230717172821.62827-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pinctrl: Provide NOIRQ PM helper and use it | expand |
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. > 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?
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
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 */
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 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.
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
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 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 */
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(-)