| Message ID | 20221017171243.57078-6-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/6] spi: pxa2xx: Simplify with devm_platform_get_and_ioremap_resource() | expand |
On Mon, Oct 17, 2022 at 08:12:43PM +0300, Andy Shevchenko wrote: > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery > that makes it simpler and allows the compiler to remove those functions > if built without CONFIG_PM and CONFIG_PM_SLEEP support. Are you sure this works cleanly and doesn't suffer from similar problems to of_match_ptr() when PM is disabled, leaving some unreferenced statics?
On Mon, Oct 17, 2022 at 06:19:53PM +0100, Mark Brown wrote: > On Mon, Oct 17, 2022 at 08:12:43PM +0300, Andy Shevchenko wrote: > > > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery > > that makes it simpler and allows the compiler to remove those functions > > if built without CONFIG_PM and CONFIG_PM_SLEEP support. > > Are you sure this works cleanly and doesn't suffer from similar problems > to of_match_ptr() when PM is disabled, leaving some unreferenced statics? Yes, this is the trick with PTR_IF() behind it, which is not used by OF code.
On Mon, 17 Oct 2022 20:12:43 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery > that makes 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> FWIW I like these - so drive by review. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> I think you could change the handling of !pm_runtime_suspended() to use pm_runtime_force_suspend() and equivalent for resume path. I haven't checked that closely though - just looks like a typical usecase for those functions that are hardened against some of the corner cases that can occur in interactions between different forms of pm. > --- > drivers/spi/spi-pxa2xx.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c > index 76046612466d..60cab241200b 100644 > --- a/drivers/spi/spi-pxa2xx.c > +++ b/drivers/spi/spi-pxa2xx.c > @@ -1680,7 +1680,6 @@ static int pxa2xx_spi_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM_SLEEP > static int pxa2xx_spi_suspend(struct device *dev) > { > struct driver_data *drv_data = dev_get_drvdata(dev); > @@ -1715,9 +1714,7 @@ static int pxa2xx_spi_resume(struct device *dev) > /* Start the queue running */ > return spi_controller_resume(drv_data->controller); > } > -#endif > > -#ifdef CONFIG_PM > static int pxa2xx_spi_runtime_suspend(struct device *dev) > { > struct driver_data *drv_data = dev_get_drvdata(dev); > @@ -1732,12 +1729,10 @@ static int pxa2xx_spi_runtime_resume(struct device *dev) > > return clk_prepare_enable(drv_data->ssp->clk); > } > -#endif > > static const struct dev_pm_ops pxa2xx_spi_pm_ops = { > - SET_SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume) > - SET_RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, > - pxa2xx_spi_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume) > + RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL) > }; > > #ifdef CONFIG_ACPI > @@ -1762,7 +1757,7 @@ MODULE_DEVICE_TABLE(of, pxa2xx_spi_of_match); > static struct platform_driver driver = { > .driver = { > .name = "pxa2xx-spi", > - .pm = &pxa2xx_spi_pm_ops, > + .pm = pm_ptr(&pxa2xx_spi_pm_ops), > .acpi_match_table = ACPI_PTR(pxa2xx_spi_acpi_match), > .of_match_table = of_match_ptr(pxa2xx_spi_of_match), > },
On Tue, Oct 18, 2022 at 10:14:52AM +0100, Jonathan Cameron wrote: > On Mon, 17 Oct 2022 20:12:43 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > Cleaning up the driver to use pm_ptr() macro instead of ifdeffery > > that makes 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> > FWIW I like these - so drive by review. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > I think you could change the handling of !pm_runtime_suspended() > to use pm_runtime_force_suspend() and equivalent for resume path. > I haven't checked that closely though - just looks like a typical > usecase for those functions that are hardened against some of > the corner cases that can occur in interactions between different > forms of pm. Thanks for an advice. Wouldn't it be matter of a separate change?
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index 76046612466d..60cab241200b 100644 --- a/drivers/spi/spi-pxa2xx.c +++ b/drivers/spi/spi-pxa2xx.c @@ -1680,7 +1680,6 @@ static int pxa2xx_spi_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM_SLEEP static int pxa2xx_spi_suspend(struct device *dev) { struct driver_data *drv_data = dev_get_drvdata(dev); @@ -1715,9 +1714,7 @@ static int pxa2xx_spi_resume(struct device *dev) /* Start the queue running */ return spi_controller_resume(drv_data->controller); } -#endif -#ifdef CONFIG_PM static int pxa2xx_spi_runtime_suspend(struct device *dev) { struct driver_data *drv_data = dev_get_drvdata(dev); @@ -1732,12 +1729,10 @@ static int pxa2xx_spi_runtime_resume(struct device *dev) return clk_prepare_enable(drv_data->ssp->clk); } -#endif static const struct dev_pm_ops pxa2xx_spi_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume) - SET_RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, - pxa2xx_spi_runtime_resume, NULL) + SYSTEM_SLEEP_PM_OPS(pxa2xx_spi_suspend, pxa2xx_spi_resume) + RUNTIME_PM_OPS(pxa2xx_spi_runtime_suspend, pxa2xx_spi_runtime_resume, NULL) }; #ifdef CONFIG_ACPI @@ -1762,7 +1757,7 @@ MODULE_DEVICE_TABLE(of, pxa2xx_spi_of_match); static struct platform_driver driver = { .driver = { .name = "pxa2xx-spi", - .pm = &pxa2xx_spi_pm_ops, + .pm = pm_ptr(&pxa2xx_spi_pm_ops), .acpi_match_table = ACPI_PTR(pxa2xx_spi_acpi_match), .of_match_table = of_match_ptr(pxa2xx_spi_of_match), },
Cleaning up the driver to use pm_ptr() macro instead of ifdeffery that makes 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/spi/spi-pxa2xx.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)