[PATCH 0/2] spi: rpc-if: Trivial fixes

[PATCH 0/2] spi: rpc-if: Trivial fixes

[Lad Prabhakar]

Hi All,

These patches are trivial fixes for rpc-if SPI driver.

Cheers,
Prabhakar

Lad Prabhakar (2):
  spi: rpc-if: Avoid use of C++ style comments
  spi: rpc-if: Remove CONFIG_PM_SLEEP ifdefery

 drivers/spi/spi-rpc-if.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/2] spi: rpc-if: Avoid use of C++ style comments

[Lad Prabhakar]

Replace C++ style comment with C style.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/spi/spi-rpc-if.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-rpc-if.c b/drivers/spi/spi-rpc-if.c
index 3579675485a5..bf64da322e67 100644
--- a/drivers/spi/spi-rpc-if.c
+++ b/drivers/spi/spi-rpc-if.c
@@ -1,11 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
-//
-// RPC-IF SPI/QSPI/Octa driver
-//
-// Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
-// Copyright (C) 2019 Macronix International Co., Ltd.
-// Copyright (C) 2019 - 2020 Cogent Embedded, Inc.
-//
+/*
+ * RPC-IF SPI/QSPI/Octa driver
+ *
+ * Copyright (C) 2018 ~ 2019 Renesas Solutions Corp.
+ * Copyright (C) 2019 Macronix International Co., Ltd.
+ * Copyright (C) 2019 - 2020 Cogent Embedded, Inc.
+ */
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
-- 
2.17.1

Re: [PATCH 1/2] spi: rpc-if: Avoid use of C++ style comments

[Sergei Shtylyov]

On 12/30/20 5:57 PM, Lad Prabhakar wrote:

> Replace C++ style comment with C style.

   Note that the switch to // was made following the SPI maintainer's request...

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
[...]

MBR, Sergei

Re: [PATCH 1/2] spi: rpc-if: Avoid use of C++ style comments

[Lad, Prabhakar]

Hi Sergei,

On Wed, Dec 30, 2020 at 4:27 PM Sergei Shtylyov
<sergei.shtylyov@gmail.com> wrote:
>
> On 12/30/20 5:57 PM, Lad Prabhakar wrote:
>
> > Replace C++ style comment with C style.
>
>    Note that the switch to // was made following the SPI maintainer's request...
>
Thanks for letting me know, let's drop this patch.

Cheers,
Prabhakar

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> [...]
>
> MBR, Sergei

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

[Lad Prabhakar]

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

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

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>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Geert Uytterhoeven]

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

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

[Lad, Prabhakar]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Geert Uytterhoeven]

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

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Geert Uytterhoeven]

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

Re: (subset) [PATCH 0/2] spi: rpc-if: Trivial fixes

[Mark Brown]

On Wed, 30 Dec 2020 14:57:06 +0000, Lad Prabhakar wrote:
> These patches are trivial fixes for rpc-if SPI driver.
> 
> Cheers,
> Prabhakar
> 
> Lad Prabhakar (2):
>   spi: rpc-if: Avoid use of C++ style comments
>   spi: rpc-if: Remove CONFIG_PM_SLEEP ifdefery
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

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

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark