diff mbox series

[v2,04/10] pinctrl: intel: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

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

Commit Message

Andy Shevchenko July 17, 2023, 5:28 p.m. UTC
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(-)

Comments

Andy Shevchenko July 17, 2023, 7:33 p.m. UTC | #1
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?
Paul Cercueil July 17, 2023, 7:55 p.m. UTC | #2
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
Jonathan Cameron July 18, 2023, 10:04 a.m. UTC | #3
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 */
Andy Shevchenko July 18, 2023, 12:57 p.m. UTC | #4
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
Andy Shevchenko July 18, 2023, 1:53 p.m. UTC | #5
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.
Paul Cercueil July 18, 2023, 1:55 p.m. UTC | #6
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
Jonathan Cameron July 19, 2023, 10:37 a.m. UTC | #7
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 mbox series

Patch

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 */