| Message ID | 20201230145708.28544-3-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | spi: rpc-if: Trivial fixes | expand |
On Wed 2020-12-30 14:57:08, Lad Prabhakar wrote: > Use __maybe_unused for the suspend()/resume() hooks and get rid of > the CONFIG_PM_SLEEP ifdefery to improve the code. > > Suggested-by: Pavel Machek <pavel@denx.de> Acked-by: Pavel Machek <pavel@denx.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Hi Prabhakar, On Wed, Dec 30, 2020 at 4:00 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > Use __maybe_unused for the suspend()/resume() hooks and get rid of > the CONFIG_PM_SLEEP ifdefery to improve the code. > > Suggested-by: Pavel Machek <pavel@denx.de> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/spi/spi-rpc-if.c > +++ b/drivers/spi/spi-rpc-if.c > @@ -176,15 +176,14 @@ static int rpcif_spi_remove(struct platform_device *pdev) > return 0; > } > > -#ifdef CONFIG_PM_SLEEP > -static int rpcif_spi_suspend(struct device *dev) > +static int __maybe_unused rpcif_spi_suspend(struct device *dev) > { > struct spi_controller *ctlr = dev_get_drvdata(dev); > > return spi_controller_suspend(ctlr); > } > > -static int rpcif_spi_resume(struct device *dev) > +static int __maybe_unused rpcif_spi_resume(struct device *dev) > { > struct spi_controller *ctlr = dev_get_drvdata(dev); > > @@ -192,17 +191,13 @@ static int rpcif_spi_resume(struct device *dev) > } > > static SIMPLE_DEV_PM_OPS(rpcif_spi_pm_ops, rpcif_spi_suspend, rpcif_spi_resume); > -#define DEV_PM_OPS (&rpcif_spi_pm_ops) > -#else > -#define DEV_PM_OPS NULL > -#endif > > static struct platform_driver rpcif_spi_driver = { > .probe = rpcif_spi_probe, > .remove = rpcif_spi_remove, > .driver = { > .name = "rpc-if-spi", > - .pm = DEV_PM_OPS, > + .pm = &rpcif_spi_pm_ops, You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? This may matter for RZ/A SoCs running from internal SRAM. > }, > }; > module_platform_driver(rpcif_spi_driver); Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Mon, Jan 4, 2021 at 12:34 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, Dec 30, 2020 at 4:00 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Use __maybe_unused for the suspend()/resume() hooks and get rid of > > the CONFIG_PM_SLEEP ifdefery to improve the code. > > > > Suggested-by: Pavel Machek <pavel@denx.de> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/spi/spi-rpc-if.c > > +++ b/drivers/spi/spi-rpc-if.c > > @@ -176,15 +176,14 @@ static int rpcif_spi_remove(struct platform_device *pdev) > > return 0; > > } > > > > -#ifdef CONFIG_PM_SLEEP > > -static int rpcif_spi_suspend(struct device *dev) > > +static int __maybe_unused rpcif_spi_suspend(struct device *dev) > > { > > struct spi_controller *ctlr = dev_get_drvdata(dev); > > > > return spi_controller_suspend(ctlr); > > } > > > > -static int rpcif_spi_resume(struct device *dev) > > +static int __maybe_unused rpcif_spi_resume(struct device *dev) > > { > > struct spi_controller *ctlr = dev_get_drvdata(dev); > > > > @@ -192,17 +191,13 @@ static int rpcif_spi_resume(struct device *dev) > > } > > > > static SIMPLE_DEV_PM_OPS(rpcif_spi_pm_ops, rpcif_spi_suspend, rpcif_spi_resume); > > -#define DEV_PM_OPS (&rpcif_spi_pm_ops) > > -#else > > -#define DEV_PM_OPS NULL > > -#endif > > > > static struct platform_driver rpcif_spi_driver = { > > .probe = rpcif_spi_probe, > > .remove = rpcif_spi_remove, > > .driver = { > > .name = "rpc-if-spi", > > - .pm = DEV_PM_OPS, > > + .pm = &rpcif_spi_pm_ops, > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > This may matter for RZ/A SoCs running from internal SRAM. > Hmm didn't realise this would be an issue on RZ/A. Mark, could you please drop this patch from your branch. Cheers, Prabhakar > > }, > > }; > > module_platform_driver(rpcif_spi_driver); > > 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
On Mon, Jan 04, 2021 at 09:25:17PM +0000, Lad, Prabhakar wrote: > > > .name = "rpc-if-spi", > > > - .pm = DEV_PM_OPS, > > > + .pm = &rpcif_spi_pm_ops, > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > > This may matter for RZ/A SoCs running from internal SRAM. > Hmm didn't realise this would be an issue on RZ/A. > Mark, could you please drop this patch from your branch. Please send an incremental patch with an appropriate changelog.
Hi! > > > > .name = "rpc-if-spi", > > > > - .pm = DEV_PM_OPS, > > > > + .pm = &rpcif_spi_pm_ops, > > > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > > > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > > > This may matter for RZ/A SoCs running from internal SRAM. > > > Hmm didn't realise this would be an issue on RZ/A. > > > Mark, could you please drop this patch from your branch. > > Please send an incremental patch with an appropriate changelog. Let's fix this properly. I'm pretty sure we have some macros that can solve this without re-introducing the ifdefs... (Besides... 92 bytes. How big is kernel these days? 4MB? More? How much SRAM do you have?) Best regards, Pavel
Hi Pavel, On Tue, Jan 5, 2021 at 12:40 AM Pavel Machek <pavel@denx.de> wrote: > > > > > .name = "rpc-if-spi", > > > > > - .pm = DEV_PM_OPS, > > > > > + .pm = &rpcif_spi_pm_ops, > > > > > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > > > > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > > > > This may matter for RZ/A SoCs running from internal SRAM. > > > > > Hmm didn't realise this would be an issue on RZ/A. > > > > > Mark, could you please drop this patch from your branch. > > > > Please send an incremental patch with an appropriate changelog. > > Let's fix this properly. I'm pretty sure we have some macros that can > solve this without re-introducing the ifdefs... There's pm_ptr(), but it uses CONFIG_PM as a selector, not CONFIG_PM_SLEEP. > (Besides... 92 bytes. How big is kernel these days? 4MB? More? How > much SRAM do you have?) 92 bytes is indeed not much (on 64-bit it would be doubled). Still, it's good to make people think about innocent looking changes, once in a while. RZ/A1H and RZ/A1M have 10 resp. 5 MiB of SRAM. RZ/A2 has 4 MiB SRAM, which is sufficient to run Linux when used with XIP (requires a one-line Kconfig change rmk has been vetoing for years). Gr{oetje,eeting}s, Geert
Hi! > On Tue, Jan 5, 2021 at 12:40 AM Pavel Machek <pavel@denx.de> wrote: > > > > > > .name = "rpc-if-spi", > > > > > > - .pm = DEV_PM_OPS, > > > > > > + .pm = &rpcif_spi_pm_ops, > > > > > > > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > > > > > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > > > > > This may matter for RZ/A SoCs running from internal SRAM. > > > > > > > Hmm didn't realise this would be an issue on RZ/A. > > > > > > > Mark, could you please drop this patch from your branch. > > > > > > Please send an incremental patch with an appropriate changelog. > > > > Let's fix this properly. I'm pretty sure we have some macros that can > > solve this without re-introducing the ifdefs... > > There's pm_ptr(), but it uses CONFIG_PM as a selector, not CONFIG_PM_SLEEP. Okay; so we could introduce pm_sleep_ptr(). Or we could just put single #ifdef CONFIG_PM_SLEEP around the .pm assignment? That would be improvement on the original, and still result in the same binary, right? > > (Besides... 92 bytes. How big is kernel these days? 4MB? More? How > > much SRAM do you have?) > > 92 bytes is indeed not much (on 64-bit it would be doubled). > Still, it's good to make people think about innocent looking changes, > once in a while. > > RZ/A1H and RZ/A1M have 10 resp. 5 MiB of SRAM. > RZ/A2 has 4 MiB SRAM, which is sufficient to run Linux when used with > XIP (requires a one-line Kconfig change rmk has been vetoing for > years). Aha, that is a bit smaller than I expected. Best regards, Pavel
Hi Pavel, On Tue, Jan 5, 2021 at 11:42 AM Pavel Machek <pavel@denx.de> wrote: > > On Tue, Jan 5, 2021 at 12:40 AM Pavel Machek <pavel@denx.de> wrote: > > > > > > > .name = "rpc-if-spi", > > > > > > > - .pm = DEV_PM_OPS, > > > > > > > + .pm = &rpcif_spi_pm_ops, > > > > > > > > > > You're aware rpcif_spi_pm_ops is now always referenced and thus emitted, > > > > > > increasing kernel size by 92 bytes if CONFIG_PM_SLEEP=n? > > > > > > This may matter for RZ/A SoCs running from internal SRAM. > > > > > > > > > Hmm didn't realise this would be an issue on RZ/A. > > > > > > > > > Mark, could you please drop this patch from your branch. > > > > > > > > Please send an incremental patch with an appropriate changelog. > > > > > > Let's fix this properly. I'm pretty sure we have some macros that can > > > solve this without re-introducing the ifdefs... > > > > There's pm_ptr(), but it uses CONFIG_PM as a selector, not CONFIG_PM_SLEEP. > > Okay; so we could introduce pm_sleep_ptr(). > > Or we could just put single #ifdef CONFIG_PM_SLEEP around the .pm > assignment? That would be improvement on the original, and still > result in the same binary, right? Indeed. Gr{oetje,eeting}s, Geert
diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c index bf64da322e67..b600becd4691 100644 --- a/drivers/spi/spi-rpc-if.c +++ b/drivers/spi/spi-rpc-if.c @@ -176,15 +176,14 @@ static int rpcif_spi_remove(struct platform_device *pdev) return 0; } -#ifdef CONFIG_PM_SLEEP -static int rpcif_spi_suspend(struct device *dev) +static int __maybe_unused rpcif_spi_suspend(struct device *dev) { struct spi_controller *ctlr = dev_get_drvdata(dev); return spi_controller_suspend(ctlr); } -static int rpcif_spi_resume(struct device *dev) +static int __maybe_unused rpcif_spi_resume(struct device *dev) { struct spi_controller *ctlr = dev_get_drvdata(dev); @@ -192,17 +191,13 @@ static int rpcif_spi_resume(struct device *dev) } static SIMPLE_DEV_PM_OPS(rpcif_spi_pm_ops, rpcif_spi_suspend, rpcif_spi_resume); -#define DEV_PM_OPS (&rpcif_spi_pm_ops) -#else -#define DEV_PM_OPS NULL -#endif static struct platform_driver rpcif_spi_driver = { .probe = rpcif_spi_probe, .remove = rpcif_spi_remove, .driver = { .name = "rpc-if-spi", - .pm = DEV_PM_OPS, + .pm = &rpcif_spi_pm_ops, }, }; module_platform_driver(rpcif_spi_driver);
Use __maybe_unused for the suspend()/resume() hooks and get rid of the CONFIG_PM_SLEEP ifdefery to improve the code. Suggested-by: Pavel Machek <pavel@denx.de> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- drivers/spi/spi-rpc-if.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)