| Message ID | 20240903154533.101258-2-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: mpc8xxx: order headers alphabetically | expand |
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);
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().
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 > >
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 > > > >
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), }, };