diff mbox series

[2/2] spi: rpc-if: Remove CONFIG_PM_SLEEP ifdefery

Message ID 20201230145708.28544-3-prabhakar.mahadev-lad.rj@bp.renesas.com
State Accepted
Commit 9584fc95cadc0b86e5e01cefcff0ab2b31ee3a5b
Headers show
Series spi: rpc-if: Trivial fixes | expand

Commit Message

Prabhakar Mahadev Lad Dec. 30, 2020, 2:57 p.m. UTC
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(-)

Comments

Geert Uytterhoeven Jan. 4, 2021, 12:34 p.m. UTC | #1
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

-- 
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
Lad, Prabhakar Jan. 4, 2021, 9:25 p.m. UTC | #2
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
Mark Brown Jan. 4, 2021, 9:30 p.m. UTC | #3
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.
Pavel Machek Jan. 4, 2021, 11:40 p.m. UTC | #4
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


-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Geert Uytterhoeven Jan. 5, 2021, 7:37 a.m. UTC | #5
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

-- 
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
Pavel Machek Jan. 5, 2021, 10:42 a.m. UTC | #6
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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Geert Uytterhoeven Jan. 5, 2021, 11:11 a.m. UTC | #7
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

-- 
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
diff mbox series

Patch

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