[PATCH v2 00/10] pinctrl: Provide NOIRQ PM helper and use it

[PATCH v2 00/10] pinctrl: Provide NOIRQ PM helper and use it

[Andy Shevchenko]

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.

Changelog v2:
- rewritten commit message in patch 1 (Rafael)
- converted non-Intel pin control drivers as well
- added couple of kinda related patches to use pm_ptr()

Andy Shevchenko (10):
  pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: baytrail: Make use of pm_ptr()
  pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: intel: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: lynxpoint: Make use of pm_ptr()
  pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: mvebu: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper
  pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

 drivers/pinctrl/intel/pinctrl-baytrail.c      | 11 +++--------
 drivers/pinctrl/intel/pinctrl-cherryview.c    |  9 ++-------
 drivers/pinctrl/intel/pinctrl-intel.c         |  5 +----
 drivers/pinctrl/intel/pinctrl-intel.h         |  9 ++-------
 drivers/pinctrl/intel/pinctrl-lynxpoint.c     |  7 +++----
 drivers/pinctrl/mediatek/pinctrl-mtk-common.c |  5 +----
 drivers/pinctrl/mediatek/pinctrl-paris.c      |  9 +++------
 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 +----
 include/linux/pm.h                            |  9 +++++++++
 12 files changed, 39 insertions(+), 70 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

_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.
+ */
+#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))
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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))
>  

Re: [PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

On Mon, 17 Jul 2023 20:28:12 +0300
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>

Seems reasonable to me given it is fairly common

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.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.
> + */
> +#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))
>  

Re: [PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Geert Uytterhoeven]

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

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

Re: [PATCH v2 01/10] pm: Introduce DEFINE_NOIRQ_DEV_PM_OPS() helper

[Rafael J. Wysocki]

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>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

and please feel free to route this how you see fit.

> ---
>  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.
> + */
> +#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))
>
> --
> 2.40.0.1.gaa8946217a0b
>

[PATCH v2 02/10] pinctrl: baytrail: Make use of pm_ptr()

[Andy Shevchenko]

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>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 27aef62fc7c0..66aabac6be9c 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1733,7 +1733,6 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int byt_gpio_suspend(struct device *dev)
 {
 	struct intel_pinctrl *vg = dev_get_drvdata(dev);
@@ -1817,9 +1816,7 @@ static int byt_gpio_resume(struct device *dev)
 	raw_spin_unlock_irqrestore(&byt_lock, flags);
 	return 0;
 }
-#endif
 
-#ifdef CONFIG_PM
 static int byt_gpio_runtime_suspend(struct device *dev)
 {
 	return 0;
@@ -1829,19 +1826,17 @@ static int byt_gpio_runtime_resume(struct device *dev)
 {
 	return 0;
 }
-#endif
 
 static const struct dev_pm_ops byt_gpio_pm_ops = {
-	SET_LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend, byt_gpio_resume)
-	SET_RUNTIME_PM_OPS(byt_gpio_runtime_suspend, byt_gpio_runtime_resume,
-			   NULL)
+	LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend, byt_gpio_resume)
+	RUNTIME_PM_OPS(byt_gpio_runtime_suspend, byt_gpio_runtime_resume, NULL)
 };
 
 static struct platform_driver byt_gpio_driver = {
 	.probe          = byt_pinctrl_probe,
 	.driver         = {
 		.name			= "byt_gpio",
-		.pm			= &byt_gpio_pm_ops,
+		.pm			= pm_ptr(&byt_gpio_pm_ops),
 		.acpi_match_table	= byt_gpio_acpi_match,
 		.suppress_bind_attrs	= true,
 	},
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 02/10] pinctrl: baytrail: Make use of pm_ptr()

[Paul Cercueil]

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-baytrail.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c
> b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 27aef62fc7c0..66aabac6be9c 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1733,7 +1733,6 @@ static int byt_pinctrl_probe(struct
> platform_device *pdev)
>         return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int byt_gpio_suspend(struct device *dev)
>  {
>         struct intel_pinctrl *vg = dev_get_drvdata(dev);
> @@ -1817,9 +1816,7 @@ static int byt_gpio_resume(struct device *dev)
>         raw_spin_unlock_irqrestore(&byt_lock, flags);
>         return 0;
>  }
> -#endif
>  
> -#ifdef CONFIG_PM
>  static int byt_gpio_runtime_suspend(struct device *dev)
>  {
>         return 0;
> @@ -1829,19 +1826,17 @@ static int byt_gpio_runtime_resume(struct
> device *dev)
>  {
>         return 0;
>  }
> -#endif
>  
>  static const struct dev_pm_ops byt_gpio_pm_ops = {
> -       SET_LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend,
> byt_gpio_resume)
> -       SET_RUNTIME_PM_OPS(byt_gpio_runtime_suspend,
> byt_gpio_runtime_resume,
> -                          NULL)
> +       LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend, byt_gpio_resume)
> +       RUNTIME_PM_OPS(byt_gpio_runtime_suspend,
> byt_gpio_runtime_resume, NULL)
>  };
>  
>  static struct platform_driver byt_gpio_driver = {
>         .probe          = byt_pinctrl_probe,
>         .driver         = {
>                 .name                   = "byt_gpio",
> -               .pm                     = &byt_gpio_pm_ops,
> +               .pm                     = pm_ptr(&byt_gpio_pm_ops),
>                 .acpi_match_table       = byt_gpio_acpi_match,
>                 .suppress_bind_attrs    = true,
>         },

Re: [PATCH v2 02/10] pinctrl: baytrail: Make use of pm_ptr()

[Jonathan Cameron]

On Mon, 17 Jul 2023 20:28:13 +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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

LGTM but why is it in a set that claims to be about NOIRQ PM helper?

FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pinctrl/intel/pinctrl-baytrail.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 27aef62fc7c0..66aabac6be9c 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1733,7 +1733,6 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int byt_gpio_suspend(struct device *dev)
>  {
>  	struct intel_pinctrl *vg = dev_get_drvdata(dev);
> @@ -1817,9 +1816,7 @@ static int byt_gpio_resume(struct device *dev)
>  	raw_spin_unlock_irqrestore(&byt_lock, flags);
>  	return 0;
>  }
> -#endif
>  
> -#ifdef CONFIG_PM
>  static int byt_gpio_runtime_suspend(struct device *dev)
>  {
>  	return 0;
> @@ -1829,19 +1826,17 @@ static int byt_gpio_runtime_resume(struct device *dev)
>  {
>  	return 0;
>  }
> -#endif
>  
>  static const struct dev_pm_ops byt_gpio_pm_ops = {
> -	SET_LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend, byt_gpio_resume)
> -	SET_RUNTIME_PM_OPS(byt_gpio_runtime_suspend, byt_gpio_runtime_resume,
> -			   NULL)
> +	LATE_SYSTEM_SLEEP_PM_OPS(byt_gpio_suspend, byt_gpio_resume)
> +	RUNTIME_PM_OPS(byt_gpio_runtime_suspend, byt_gpio_runtime_resume, NULL)
>  };
>  
>  static struct platform_driver byt_gpio_driver = {
>  	.probe          = byt_pinctrl_probe,
>  	.driver         = {
>  		.name			= "byt_gpio",
> -		.pm			= &byt_gpio_pm_ops,
> +		.pm			= pm_ptr(&byt_gpio_pm_ops),
>  		.acpi_match_table	= byt_gpio_acpi_match,
>  		.suppress_bind_attrs	= true,
>  	},

Re: [PATCH v2 02/10] pinctrl: baytrail: Make use of pm_ptr()

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 10:53:22AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Jul 2023 20:28:13 +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.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> LGTM but why is it in a set that claims to be about NOIRQ PM helper?

Have you chance to read a cover letter? I explained that I added them as
they are "kinda" to the topic (PM related).

I can apply them and drop for next version.

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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-cherryview.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index eee0f9bc3d32..7a2fc9fe175d 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1793,7 +1793,6 @@ static int chv_pinctrl_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int chv_pinctrl_suspend_noirq(struct device *dev)
 {
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
@@ -1877,12 +1876,8 @@ static int chv_pinctrl_resume_noirq(struct device *dev)
 
 	return 0;
 }
-#endif
 
-static const struct dev_pm_ops chv_pinctrl_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(chv_pinctrl_suspend_noirq,
-				      chv_pinctrl_resume_noirq)
-};
+static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops, chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);
 
 static const struct acpi_device_id chv_pinctrl_acpi_match[] = {
 	{ "INT33FF", (kernel_ulong_t)chv_soc_data },
@@ -1895,7 +1890,7 @@ static struct platform_driver chv_pinctrl_driver = {
 	.remove = chv_pinctrl_remove,
 	.driver = {
 		.name = "cherryview-pinctrl",
-		.pm = &chv_pinctrl_pm_ops,
+		.pm = pm_sleep_ptr(&chv_pinctrl_pm_ops),
 		.acpi_match_table = chv_pinctrl_acpi_match,
 	},
 };
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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/intel/pinctrl-cherryview.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c
> b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index eee0f9bc3d32..7a2fc9fe175d 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1793,7 +1793,6 @@ static int chv_pinctrl_remove(struct
> platform_device *pdev)
>         return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int chv_pinctrl_suspend_noirq(struct device *dev)
>  {
>         struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
> @@ -1877,12 +1876,8 @@ static int chv_pinctrl_resume_noirq(struct
> device *dev)
>  
>         return 0;
>  }
> -#endif
>  
> -static const struct dev_pm_ops chv_pinctrl_pm_ops = {
> -       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(chv_pinctrl_suspend_noirq,
> -                                     chv_pinctrl_resume_noirq)
> -};
> +static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops,
> chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);

That's over 100 characters, please break this line.

Cheers,
-Paul

>  
>  static const struct acpi_device_id chv_pinctrl_acpi_match[] = {
>         { "INT33FF", (kernel_ulong_t)chv_soc_data },
> @@ -1895,7 +1890,7 @@ static struct platform_driver
> chv_pinctrl_driver = {
>         .remove = chv_pinctrl_remove,
>         .driver = {
>                 .name = "cherryview-pinctrl",
> -               .pm = &chv_pinctrl_pm_ops,
> +               .pm = pm_sleep_ptr(&chv_pinctrl_pm_ops),
>                 .acpi_match_table = chv_pinctrl_acpi_match,
>         },
>  };

Re: [PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 10:22 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :

...

> > +static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops,
> > chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);
>
> That's over 100 characters, please break this line.

If it's a problem, I prefer to shorten the names of the callbacks.
Would it work for you?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

Le lundi 17 juillet 2023 à 22:27 +0300, Andy Shevchenko a écrit :
> On Mon, Jul 17, 2023 at 10:22 PM Paul Cercueil <paul@crapouillou.net>
> wrote:
> > Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :
> 
> ...
> 
> > > +static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops,
> > > chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);
> > 
> > That's over 100 characters, please break this line.
> 
> If it's a problem, I prefer to shorten the names of the callbacks.
> Would it work for you?
> 

That works.

-Paul

Re: [PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

On Mon, 17 Jul 2023 20:28:14 +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>
Formatting comment inline. Otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index eee0f9bc3d32..7a2fc9fe175d 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1793,7 +1793,6 @@ static int chv_pinctrl_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int chv_pinctrl_suspend_noirq(struct device *dev)
>  {
>  	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
> @@ -1877,12 +1876,8 @@ static int chv_pinctrl_resume_noirq(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
> -static const struct dev_pm_ops chv_pinctrl_pm_ops = {
> -	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(chv_pinctrl_suspend_noirq,
> -				      chv_pinctrl_resume_noirq)
> -};
> + static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops, chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);

Very long line and readability not hurt by breaking it.

>  
>  static const struct acpi_device_id chv_pinctrl_acpi_match[] = {
>  	{ "INT33FF", (kernel_ulong_t)chv_soc_data },
> @@ -1895,7 +1890,7 @@ static struct platform_driver chv_pinctrl_driver = {
>  	.remove = chv_pinctrl_remove,
>  	.driver = {
>  		.name = "cherryview-pinctrl",
> -		.pm = &chv_pinctrl_pm_ops,
> +		.pm = pm_sleep_ptr(&chv_pinctrl_pm_ops),
>  		.acpi_match_table = chv_pinctrl_acpi_match,
>  	},
>  };

Re: [PATCH v2 03/10] pinctrl: cherryview: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

On Tue, Jul 18, 2023 at 10:54:53AM +0100, Jonathan Cameron wrote:
> On Mon, 17 Jul 2023 20:28:14 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks!

...

> > + static DEFINE_NOIRQ_DEV_PM_OPS(chv_pinctrl_pm_ops, chv_pinctrl_suspend_noirq, chv_pinctrl_resume_noirq);
> 
> Very long line and readability not hurt by breaking it.

Sure.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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
 
 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 */
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

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

[Paul Cercueil]

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

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

[Andy Shevchenko]

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

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

[Paul Cercueil]

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

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

[Jonathan Cameron]

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

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

[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.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

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

[PATCH v2 05/10] pinctrl: lynxpoint: Make use of pm_ptr()

[Andy Shevchenko]

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>
---
 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,
 	},
 };
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 05/10] pinctrl: lynxpoint: Make use of pm_ptr()

[Paul Cercueil]

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,
>         },
>  };

Re: [PATCH v2 05/10] pinctrl: lynxpoint: Make use of pm_ptr()

[Jonathan Cameron]

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,
>  	},
>  };

Re: [PATCH v2 05/10] pinctrl: lynxpoint: Make use of pm_ptr()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 06/10] pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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/pinctrl-at91.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 39956d821ad7..608f55c5ba5f 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1657,7 +1657,7 @@ static int gpio_irq_set_wake(struct irq_data *d, unsigned state)
 	return 0;
 }
 
-static int __maybe_unused at91_gpio_suspend(struct device *dev)
+static int at91_gpio_suspend(struct device *dev)
 {
 	struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
 	void __iomem *pio = at91_chip->regbase;
@@ -1675,7 +1675,7 @@ static int __maybe_unused at91_gpio_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused at91_gpio_resume(struct device *dev)
+static int at91_gpio_resume(struct device *dev)
 {
 	struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
 	void __iomem *pio = at91_chip->regbase;
@@ -1903,15 +1903,13 @@ static int at91_gpio_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct dev_pm_ops at91_gpio_pm_ops = {
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(at91_gpio_suspend, at91_gpio_resume)
-};
+static DEFINE_NOIRQ_DEV_PM_OPS(at91_gpio_pm_ops, at91_gpio_suspend, at91_gpio_resume);
 
 static struct platform_driver at91_gpio_driver = {
 	.driver = {
 		.name = "gpio-at91",
 		.of_match_table = at91_gpio_of_match,
-		.pm = pm_ptr(&at91_gpio_pm_ops),
+		.pm = pm_sleep_ptr(&at91_gpio_pm_ops),
 	},
 	.probe = at91_gpio_probe,
 };
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 06/10] pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Although you could add a bit more info in the message of this patch, to
explain why it's OK to remove the __maybe_unused tags (the code is
always visible) and why switch from pm_ptr() to pm_sleep_ptr() (it's
only used for system-PM callbacks).

Cheers,
-Paul

> ---
>  drivers/pinctrl/pinctrl-at91.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c
> b/drivers/pinctrl/pinctrl-at91.c
> index 39956d821ad7..608f55c5ba5f 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -1657,7 +1657,7 @@ static int gpio_irq_set_wake(struct irq_data
> *d, unsigned state)
>         return 0;
>  }
>  
> -static int __maybe_unused at91_gpio_suspend(struct device *dev)
> +static int at91_gpio_suspend(struct device *dev)
>  {
>         struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
>         void __iomem *pio = at91_chip->regbase;
> @@ -1675,7 +1675,7 @@ static int __maybe_unused
> at91_gpio_suspend(struct device *dev)
>         return 0;
>  }
>  
> -static int __maybe_unused at91_gpio_resume(struct device *dev)
> +static int at91_gpio_resume(struct device *dev)
>  {
>         struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
>         void __iomem *pio = at91_chip->regbase;
> @@ -1903,15 +1903,13 @@ static int at91_gpio_probe(struct
> platform_device *pdev)
>         return 0;
>  }
>  
> -static const struct dev_pm_ops at91_gpio_pm_ops = {
> -       NOIRQ_SYSTEM_SLEEP_PM_OPS(at91_gpio_suspend,
> at91_gpio_resume)
> -};
> +static DEFINE_NOIRQ_DEV_PM_OPS(at91_gpio_pm_ops, at91_gpio_suspend,
> at91_gpio_resume);
>  
>  static struct platform_driver at91_gpio_driver = {
>         .driver = {
>                 .name = "gpio-at91",
>                 .of_match_table = at91_gpio_of_match,
> -               .pm = pm_ptr(&at91_gpio_pm_ops),
> +               .pm = pm_sleep_ptr(&at91_gpio_pm_ops),
>         },
>         .probe = at91_gpio_probe,
>  };

Re: [PATCH v2 06/10] pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 10:26 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>
>
> Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Thank you!

> Although you could add a bit more info in the message of this patch, to
> explain why it's OK to remove the __maybe_unused tags (the code is
> always visible) and why switch from pm_ptr() to pm_sleep_ptr() (it's
> only used for system-PM callbacks).

Sure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 06/10] pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

On Mon, 17 Jul 2023 20:28:17 +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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v2 06/10] pinctrl: at91: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[claudiu beznea]

On 17.07.2023 20:28, 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>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

> ---
>   drivers/pinctrl/pinctrl-at91.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 39956d821ad7..608f55c5ba5f 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -1657,7 +1657,7 @@ static int gpio_irq_set_wake(struct irq_data *d, unsigned state)
>   	return 0;
>   }
>   
> -static int __maybe_unused at91_gpio_suspend(struct device *dev)
> +static int at91_gpio_suspend(struct device *dev)
>   {
>   	struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
>   	void __iomem *pio = at91_chip->regbase;
> @@ -1675,7 +1675,7 @@ static int __maybe_unused at91_gpio_suspend(struct device *dev)
>   	return 0;
>   }
>   
> -static int __maybe_unused at91_gpio_resume(struct device *dev)
> +static int at91_gpio_resume(struct device *dev)
>   {
>   	struct at91_gpio_chip *at91_chip = dev_get_drvdata(dev);
>   	void __iomem *pio = at91_chip->regbase;
> @@ -1903,15 +1903,13 @@ static int at91_gpio_probe(struct platform_device *pdev)
>   	return 0;
>   }
>   
> -static const struct dev_pm_ops at91_gpio_pm_ops = {
> -	NOIRQ_SYSTEM_SLEEP_PM_OPS(at91_gpio_suspend, at91_gpio_resume)
> -};
> +static DEFINE_NOIRQ_DEV_PM_OPS(at91_gpio_pm_ops, at91_gpio_suspend, at91_gpio_resume);
>   
>   static struct platform_driver at91_gpio_driver = {
>   	.driver = {
>   		.name = "gpio-at91",
>   		.of_match_table = at91_gpio_of_match,
> -		.pm = pm_ptr(&at91_gpio_pm_ops),
> +		.pm = pm_sleep_ptr(&at91_gpio_pm_ops),
>   	},
>   	.probe = at91_gpio_probe,
>   };

[PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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)
 {
 	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");
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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/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);

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().

Cheers,
-Paul

>  
>  MODULE_LICENSE("GPL v2");
>  MODULE_DESCRIPTION("MediaTek Pinctrl Common Driver V2 Paris");

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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.

Cheers,
-Paul

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[AngeloGioacchino Del Regno]

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");

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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 ^^^

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 07/10] pinctrl: mediatek: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

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");

[PATCH v2 08/10] pinctrl: mvebu: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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/mvebu/pinctrl-armada-37xx.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index 67c6751a6f06..46351c00ee73 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -1013,7 +1013,6 @@ static int armada_37xx_pinctrl_register(struct platform_device *pdev,
 	return 0;
 }
 
-#if defined(CONFIG_PM)
 static int armada_3700_pinctrl_suspend(struct device *dev)
 {
 	struct armada_37xx_pinctrl *info = dev_get_drvdata(dev);
@@ -1107,15 +1106,8 @@ static int armada_3700_pinctrl_resume(struct device *dev)
  * Since pinctrl is an infrastructure module, its resume should be issued prior
  * to other IO drivers.
  */
-static const struct dev_pm_ops armada_3700_pinctrl_pm_ops = {
-	.suspend_noirq = armada_3700_pinctrl_suspend,
-	.resume_noirq = armada_3700_pinctrl_resume,
-};
-
-#define PINCTRL_ARMADA_37XX_DEV_PM_OPS (&armada_3700_pinctrl_pm_ops)
-#else
-#define PINCTRL_ARMADA_37XX_DEV_PM_OPS NULL
-#endif /* CONFIG_PM */
+static DEFINE_NOIRQ_DEV_PM_OPS(armada_3700_pinctrl_pm_ops,
+			       armada_3700_pinctrl_suspend, armada_3700_pinctrl_resume);
 
 static const struct of_device_id armada_37xx_pinctrl_of_match[] = {
 	{
@@ -1182,7 +1174,7 @@ static struct platform_driver armada_37xx_pinctrl_driver = {
 	.driver = {
 		.name = "armada-37xx-pinctrl",
 		.of_match_table = armada_37xx_pinctrl_of_match,
-		.pm = PINCTRL_ARMADA_37XX_DEV_PM_OPS,
+		.pm = pm_sleep_ptr(&armada_3700_pinctrl_pm_ops),
 	},
 };
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 08/10] pinctrl: mvebu: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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>

Reviewed-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> index 67c6751a6f06..46351c00ee73 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> @@ -1013,7 +1013,6 @@ static int armada_37xx_pinctrl_register(struct
> platform_device *pdev,
>         return 0;
>  }
>  
> -#if defined(CONFIG_PM)
>  static int armada_3700_pinctrl_suspend(struct device *dev)
>  {
>         struct armada_37xx_pinctrl *info = dev_get_drvdata(dev);
> @@ -1107,15 +1106,8 @@ static int armada_3700_pinctrl_resume(struct
> device *dev)
>   * Since pinctrl is an infrastructure module, its resume should be
> issued prior
>   * to other IO drivers.
>   */
> -static const struct dev_pm_ops armada_3700_pinctrl_pm_ops = {
> -       .suspend_noirq = armada_3700_pinctrl_suspend,
> -       .resume_noirq = armada_3700_pinctrl_resume,
> -};
> -
> -#define PINCTRL_ARMADA_37XX_DEV_PM_OPS (&armada_3700_pinctrl_pm_ops)
> -#else
> -#define PINCTRL_ARMADA_37XX_DEV_PM_OPS NULL
> -#endif /* CONFIG_PM */
> +static DEFINE_NOIRQ_DEV_PM_OPS(armada_3700_pinctrl_pm_ops,
> +                              armada_3700_pinctrl_suspend,
> armada_3700_pinctrl_resume);
>  
>  static const struct of_device_id armada_37xx_pinctrl_of_match[] = {
>         {
> @@ -1182,7 +1174,7 @@ static struct platform_driver
> armada_37xx_pinctrl_driver = {
>         .driver = {
>                 .name = "armada-37xx-pinctrl",
>                 .of_match_table = armada_37xx_pinctrl_of_match,
> -               .pm = PINCTRL_ARMADA_37XX_DEV_PM_OPS,
> +               .pm = pm_sleep_ptr(&armada_3700_pinctrl_pm_ops),
>         },
>  };
>  

Re: [PATCH v2 08/10] pinctrl: mvebu: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

On Mon, 17 Jul 2023 20:28:19 +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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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),
 	},
 };
 
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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

>         },
>  };
>  

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Geert Uytterhoeven]

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

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

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Geert Uytterhoeven]

On Mon, Jul 17, 2023 at 7:28 PM 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v2 09/10] pinctrl: renesas: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

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>

[PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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)
 {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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.

Cheers,
-Paul

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

On Mon, Jul 17, 2023 at 10:14 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lundi 17 juillet 2023 à 20:28 +0300, Andy Shevchenko a écrit :

...

> Another driver where using EXPORT_GPL_DEV_PM_OPS() would make more
> sense.

OK.

...

Thank you for the review of the series!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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.

Cheers,
-Paul

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3990 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Jonathan Cameron]

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()?

> ---
>  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)
>  {

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Paul Cercueil]

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

Re: [PATCH v2 10/10] pinctrl: tegra: Switch to use DEFINE_NOIRQ_DEV_PM_OPS() helper

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 00/10] pinctrl: Provide NOIRQ PM helper and use it

[Andy Shevchenko]

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(-)


-- 
With Best Regards,
Andy Shevchenko