[PATCH 1/2] gpio: mpc8xxx: order headers alphabetically

[PATCH 1/2] gpio: mpc8xxx: order headers alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Cleanup the includes by putting them in alphabetical order.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-mpc8xxx.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index ab30c911c9d50..e084e08f54387 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -7,19 +7,19 @@
  */
 
 #include <linux/acpi.h>
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
-#include <linux/spinlock.h>
-#include <linux/io.h>
-#include <linux/of.h>
-#include <linux/property.h>
-#include <linux/mod_devicetable.h>
-#include <linux/slab.h>
-#include <linux/irq.h>
-#include <linux/gpio/driver.h>
 #include <linux/bitops.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #define MPC8XXX_GPIO_PINS	32
 
-- 
2.43.0

[PATCH 2/2] gpio: mpc8xxx: switch to using DEFINE_RUNTIME_DEV_PM_OPS()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the preferred API for assigning system sleep pm callbacks in drivers.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-mpc8xxx.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c
index e084e08f54387..edb228d9af277 100644
--- a/drivers/gpio/gpio-mpc8xxx.c
+++ b/drivers/gpio/gpio-mpc8xxx.c
@@ -17,6 +17,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -431,30 +432,28 @@ static void mpc8xxx_remove(struct platform_device *pdev)
 	}
 }
 
-#ifdef CONFIG_PM
-static int mpc8xxx_suspend(struct platform_device *pdev, pm_message_t state)
+static int mpc8xxx_suspend(struct device *dev)
 {
-	struct mpc8xxx_gpio_chip *mpc8xxx_gc = platform_get_drvdata(pdev);
+	struct mpc8xxx_gpio_chip *mpc8xxx_gc = dev_get_drvdata(dev);
 
-	if (mpc8xxx_gc->irqn && device_may_wakeup(&pdev->dev))
+	if (mpc8xxx_gc->irqn && device_may_wakeup(dev))
 		enable_irq_wake(mpc8xxx_gc->irqn);
 
 	return 0;
 }
 
-static int mpc8xxx_resume(struct platform_device *pdev)
+static int mpc8xxx_resume(struct device *dev)
 {
-	struct mpc8xxx_gpio_chip *mpc8xxx_gc = platform_get_drvdata(pdev);
+	struct mpc8xxx_gpio_chip *mpc8xxx_gc = dev_get_drvdata(dev);
 
-	if (mpc8xxx_gc->irqn && device_may_wakeup(&pdev->dev))
+	if (mpc8xxx_gc->irqn && device_may_wakeup(dev))
 		disable_irq_wake(mpc8xxx_gc->irqn);
 
 	return 0;
 }
-#else
-#define mpc8xxx_suspend NULL
-#define mpc8xxx_resume NULL
-#endif
+
+static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops, mpc8xxx_suspend,
+				 mpc8xxx_resume, NULL);
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id gpio_acpi_ids[] = {
@@ -467,12 +466,11 @@ MODULE_DEVICE_TABLE(acpi, gpio_acpi_ids);
 static struct platform_driver mpc8xxx_plat_driver = {
 	.probe		= mpc8xxx_probe,
 	.remove_new	= mpc8xxx_remove,
-	.suspend	= mpc8xxx_suspend,
-	.resume		= mpc8xxx_resume,
 	.driver		= {
 		.name = "gpio-mpc8xxx",
 		.of_match_table	= mpc8xxx_gpio_ids,
 		.acpi_match_table = ACPI_PTR(gpio_acpi_ids),
+		.pm = pm_sleep_ptr(&mpc8xx_pm_ops),
 	},
 };
 
-- 
2.43.0

Re: [PATCH 1/2] gpio: mpc8xxx: order headers alphabetically

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 05:45:32PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Cleanup the includes by putting them in alphabetical order.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: mpc8xxx: switch to using DEFINE_RUNTIME_DEV_PM_OPS()

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 05:45:33PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Use the preferred API for assigning system sleep pm callbacks in drivers.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>

Hmm... Maybe I should pay more attention when answering emails.
Please, use my @linux.intel.com address for Linux kernel contributions.

...

>  #include <linux/mod_devicetable.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

You need pm.h as macros defined there.

>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops, mpc8xxx_suspend,
> +				 mpc8xxx_resume, NULL);

I would split logically, i.e.

static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops,
				 mpc8xxx_suspend, mpc8xxx_resume, NULL);

OR

static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops,
				 mpc8xxx_suspend, mpc8xxx_resume,
				 NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] gpio: mpc8xxx: switch to using DEFINE_RUNTIME_DEV_PM_OPS()

[Andy Shevchenko]

On Tue, Sep 03, 2024 at 07:23:31PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 03, 2024 at 05:45:33PM +0200, Bartosz Golaszewski wrote:

...

> >  #include <linux/mod_devicetable.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> 
> You need pm.h as macros defined there.

...or both...

> >  #include <linux/property.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>

...

> > +static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops, mpc8xxx_suspend,
> > +				 mpc8xxx_resume, NULL);

This one comes from pm_runtime.h, but pm*_ptr() ones from pm.h.

And it seems you wanted pm_ptr().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpio: mpc8xxx: order headers alphabetically

[Martyn Welch]

On Tue, 2024-09-03 at 17:45 +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Cleanup the includes by putting them in alphabetical order.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpio-mpc8xxx.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-
> mpc8xxx.c
> index ab30c911c9d50..e084e08f54387 100644
> --- a/drivers/gpio/gpio-mpc8xxx.c
> +++ b/drivers/gpio/gpio-mpc8xxx.c
> @@ -7,19 +7,19 @@
>   */
>  
>  #include <linux/acpi.h>
> -#include <linux/kernel.h>
> -#include <linux/init.h>
> -#include <linux/platform_device.h>
> -#include <linux/spinlock.h>
> -#include <linux/io.h>
> -#include <linux/of.h>
> -#include <linux/property.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/slab.h>
> -#include <linux/irq.h>
> -#include <linux/gpio/driver.h>
>  #include <linux/bitops.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
>  #include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #define MPC8XXX_GPIO_PINS	32
>  

Reviewed-by: Martyn Welch <martyn.welch@collabora.com>

Re: [PATCH 2/2] gpio: mpc8xxx: switch to using DEFINE_RUNTIME_DEV_PM_OPS()

[Bartosz Golaszewski]

On Tue, 3 Sept 2024 at 18:25, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Sep 03, 2024 at 07:23:31PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 03, 2024 at 05:45:33PM +0200, Bartosz Golaszewski wrote:
>
> ...
>
> > >  #include <linux/mod_devicetable.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> >
> > You need pm.h as macros defined there.
>
> ...or both...
>

pm_runtime.h implies pm.h.

> > >  #include <linux/property.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/spinlock.h>
>
> ...
>
> > > +static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops, mpc8xxx_suspend,
> > > +                            mpc8xxx_resume, NULL);
>
> This one comes from pm_runtime.h, but pm*_ptr() ones from pm.h.
>
> And it seems you wanted pm_ptr().
>

Yeah, I'm not sure really. The suspend and resume callbacks for
platform devices are not documented but it looks like they're only
used for system sleep. Martyn: which one do we actually need?

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/2] gpio: mpc8xxx: order headers alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Tue, 03 Sep 2024 17:45:32 +0200, Bartosz Golaszewski wrote:
> Cleanup the includes by putting them in alphabetical order.
> 
> 

Applied, thanks!

[1/2] gpio: mpc8xxx: order headers alphabetically
      commit: ccaf84694ce7e7438706185c726310be51954fd3

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH 2/2] gpio: mpc8xxx: switch to using DEFINE_RUNTIME_DEV_PM_OPS()

[Martyn Welch]

On Tue, 2024-09-03 at 20:36 +0200, Bartosz Golaszewski wrote:
> On Tue, 3 Sept 2024 at 18:25, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > 
> > On Tue, Sep 03, 2024 at 07:23:31PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 03, 2024 at 05:45:33PM +0200, Bartosz Golaszewski
> > > wrote:
> > 
> > ...
> > 
> > > >  #include <linux/mod_devicetable.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > > +#include <linux/pm_runtime.h>
> > > 
> > > You need pm.h as macros defined there.
> > 
> > ...or both...
> > 
> 
> pm_runtime.h implies pm.h.
> 
> > > >  #include <linux/property.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/spinlock.h>
> > 
> > ...
> > 
> > > > +static DEFINE_RUNTIME_DEV_PM_OPS(mpc8xx_pm_ops,
> > > > mpc8xxx_suspend,
> > > > +                            mpc8xxx_resume, NULL);
> > 
> > This one comes from pm_runtime.h, but pm*_ptr() ones from pm.h.
> > 
> > And it seems you wanted pm_ptr().
> > 
> 
> Yeah, I'm not sure really. The suspend and resume callbacks for
> platform devices are not documented but it looks like they're only
> used for system sleep. Martyn: which one do we actually need?
> 

Looking at commit 9d8619190031 (PM: runtime: Add
DEFINE_RUNTIME_DEV_PM_OPS() macro), the use of
DEFINE_RUNTIME_DEV_PM_OPS() sets up PM operations for all situations,
not just sleeping, so I think we need pm_ptr() here.

I'd explicitly added the functionality for suspension due to sleep,
though if a GPIO line is being used as some form of external interrupt,
there may well be cases where it would be beneficial to be able to
receive it in other low power states?

Martyn  

> Bart
> 
> > --
> > With Best Regards,
> > Andy Shevchenko
> > 
> >