[PATCH 1/3] i2c: rzv2m: Drop extra space

[PATCH 1/3] i2c: rzv2m: Drop extra space

[Biju Das]

Drop extra space from the I2C_RZV2M config help description.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/busses/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 87600b4aacb3..31c0f54b0b8c 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1025,7 +1025,7 @@ config I2C_RZV2M
 	depends on ARCH_RENESAS || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for the
-	  Renesas RZ/V2M  I2C interface.
+	  Renesas RZ/V2M I2C interface.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-rzv2m.
-- 
2.25.1

[PATCH 2/3] i2c: rzv2m: Rename macro names in all uppercase

[Biju Das]

Normally we'd put macro names in all uppercase.
Rename bit_setl->BIT_SETL and bit_clrl->BIT_CLRL.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/busses/i2c-rzv2m.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rzv2m.c b/drivers/i2c/busses/i2c-rzv2m.c
index 56d0faee5c46..ccd337f2e8c3 100644
--- a/drivers/i2c/busses/i2c-rzv2m.c
+++ b/drivers/i2c/busses/i2c-rzv2m.c
@@ -50,8 +50,8 @@
 #define IICB0MDSC	BIT(7)		/* Bus Mode */
 #define IICB0SLSE	BIT(1)		/* Start condition output */
 
-#define bit_setl(addr, val)		writel(readl(addr) | (val), (addr))
-#define bit_clrl(addr, val)		writel(readl(addr) & ~(val), (addr))
+#define BIT_SETL(addr, val)		writel(readl(addr) | (val), (addr))
+#define BIT_CLRL(addr, val)		writel(readl(addr) & ~(val), (addr))
 
 struct rzv2m_i2c_priv {
 	void __iomem *base;
@@ -198,7 +198,7 @@ static int rzv2m_i2c_read_with_ack(struct rzv2m_i2c_priv *priv, u8 *data,
 	reinit_completion(&priv->msg_tia_done);
 
 	/* Interrupt request timing : 8th clock */
-	bit_clrl(priv->base + IICB0CTL0, IICB0SLWT);
+	BIT_CLRL(priv->base + IICB0CTL0, IICB0SLWT);
 
 	/* Exit the wait state */
 	writel(IICB0WRET, priv->base + IICB0TRG);
@@ -211,13 +211,13 @@ static int rzv2m_i2c_read_with_ack(struct rzv2m_i2c_priv *priv, u8 *data,
 
 	if (last) {
 		/* Disable ACK */
-		bit_clrl(priv->base + IICB0CTL0, IICB0SLAC);
+		BIT_CLRL(priv->base + IICB0CTL0, IICB0SLAC);
 
 		/* Read data*/
 		data_tmp = readl(priv->base + IICB0DAT);
 
 		/* Interrupt request timing : 9th clock */
-		bit_setl(priv->base + IICB0CTL0, IICB0SLWT);
+		BIT_SETL(priv->base + IICB0CTL0, IICB0SLWT);
 
 		/* Exit the wait state */
 		writel(IICB0WRET, priv->base + IICB0TRG);
@@ -229,7 +229,7 @@ static int rzv2m_i2c_read_with_ack(struct rzv2m_i2c_priv *priv, u8 *data,
 			return -ETIMEDOUT;
 
 		/* Enable ACK */
-		bit_setl(priv->base + IICB0CTL0, IICB0SLAC);
+		BIT_SETL(priv->base + IICB0CTL0, IICB0SLAC);
 	} else {
 		/* Read data */
 		data_tmp = readl(priv->base + IICB0DAT);
@@ -466,7 +466,7 @@ static int rzv2m_i2c_remove(struct platform_device *pdev)
 	struct device *dev = priv->adap.dev.parent;
 
 	i2c_del_adapter(&priv->adap);
-	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);
 	pm_runtime_disable(dev);
 
 	return 0;
@@ -481,7 +481,7 @@ static int rzv2m_i2c_suspend(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	bit_clrl(priv->base + IICB0CTL0, IICB0IICE);
+	BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);
 	pm_runtime_put(dev);
 
 	return 0;
-- 
2.25.1

[PATCH 3/3] i2c: rzv2m: Disable the operation of unit in case of error

[Biju Das]

The remove and suspend callbacks disable the operation of the unit.
Do the same in probe() in case of error.

Reported-by: Pavel Machek <pavel@denx.de>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/i2c/busses/i2c-rzv2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rzv2m.c b/drivers/i2c/busses/i2c-rzv2m.c
index ccd337f2e8c3..3e9fc269e4fd 100644
--- a/drivers/i2c/busses/i2c-rzv2m.c
+++ b/drivers/i2c/busses/i2c-rzv2m.c
@@ -454,8 +454,10 @@ static int rzv2m_i2c_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 
 	ret = i2c_add_numbered_adapter(adap);
-	if (ret < 0)
+	if (ret < 0) {
+		BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);
 		pm_runtime_disable(dev);
+	}
 
 	return ret;
 }
-- 
2.25.1

Re: [PATCH 1/3] i2c: rzv2m: Drop extra space

[Geert Uytterhoeven]

On Thu, May 25, 2023 at 3:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Drop extra space from the I2C_RZV2M config help description.
>
> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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/3] i2c: rzv2m: Rename macro names in all uppercase

[Geert Uytterhoeven]

Hi Biju,

On Thu, May 25, 2023 at 3:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Normally we'd put macro names in all uppercase.
> Rename bit_setl->BIT_SETL and bit_clrl->BIT_CLRL.
>
> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/i2c/busses/i2c-rzv2m.c
> +++ b/drivers/i2c/busses/i2c-rzv2m.c
> @@ -50,8 +50,8 @@
>  #define IICB0MDSC      BIT(7)          /* Bus Mode */
>  #define IICB0SLSE      BIT(1)          /* Start condition output */
>
> -#define bit_setl(addr, val)            writel(readl(addr) | (val), (addr))
> -#define bit_clrl(addr, val)            writel(readl(addr) & ~(val), (addr))
> +#define BIT_SETL(addr, val)            writel(readl(addr) | (val), (addr))
> +#define BIT_CLRL(addr, val)            writel(readl(addr) & ~(val), (addr))

I'd rather change them to static inline functions instead.

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/3] i2c: rzv2m: Rename macro names in all uppercase

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Thursday, May 25, 2023 5:07 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Wolfram Sang <wsa@kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org; Pavel Machek
> <pavel@denx.de>
> Subject: Re: [PATCH 2/3] i2c: rzv2m: Rename macro names in all uppercase
> 
> Hi Biju,
> 
> On Thu, May 25, 2023 at 3:51 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > Normally we'd put macro names in all uppercase.
> > Rename bit_setl->BIT_SETL and bit_clrl->BIT_CLRL.
> >
> > Reported-by: Pavel Machek <pavel@denx.de>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/i2c/busses/i2c-rzv2m.c
> > +++ b/drivers/i2c/busses/i2c-rzv2m.c
> > @@ -50,8 +50,8 @@
> >  #define IICB0MDSC      BIT(7)          /* Bus Mode */
> >  #define IICB0SLSE      BIT(1)          /* Start condition output */
> >
> > -#define bit_setl(addr, val)            writel(readl(addr) | (val),
> (addr))
> > -#define bit_clrl(addr, val)            writel(readl(addr) & ~(val),
> (addr))
> > +#define BIT_SETL(addr, val)            writel(readl(addr) | (val),
> (addr))
> > +#define BIT_CLRL(addr, val)            writel(readl(addr) & ~(val),
> (addr))
> 
> I'd rather change them to static inline functions instead.

OK, will change it to static inline fns.

Cheers,
Biju

Re: [PATCH 3/3] i2c: rzv2m: Disable the operation of unit in case of error

[Geert Uytterhoeven]

Hi Biju,

Thanks for your patch!

On Thu, May 25, 2023 at 3:51 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The remove and suspend callbacks disable the operation of the unit.
> Do the same in probe() in case of error.

Makes perfect sense.

> Reported-by: Pavel Machek <pavel@denx.de>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> --- a/drivers/i2c/busses/i2c-rzv2m.c
> +++ b/drivers/i2c/busses/i2c-rzv2m.c
> @@ -454,8 +454,10 @@ static int rzv2m_i2c_probe(struct platform_device *pdev)
>         platform_set_drvdata(pdev, priv);
>
>         ret = i2c_add_numbered_adapter(adap);
> -       if (ret < 0)
> +       if (ret < 0) {
> +               BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);

This needs to be wrapped inside pm_runtime_resume_and_get()/
pm_runtime_put(), like is done in the .suspend() callback.

Note that this is also lacking from the .remove() callback.

>                 pm_runtime_disable(dev);
> +       }
>
>         return ret;
>  }

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 3/3] i2c: rzv2m: Disable the operation of unit in case of error

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Thursday, May 25, 2023 5:15 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Wolfram Sang <wsa@kernel.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Philipp Zabel
> <p.zabel@pengutronix.de>; linux-i2c@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>; linux-renesas-soc@vger.kernel.org; Pavel Machek
> <pavel@denx.de>
> Subject: Re: [PATCH 3/3] i2c: rzv2m: Disable the operation of unit in
> case of error
> 
> Hi Biju,
> 
> Thanks for your patch!
> 
> On Thu, May 25, 2023 at 3:51 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The remove and suspend callbacks disable the operation of the unit.
> > Do the same in probe() in case of error.
> 
> Makes perfect sense.
> 
> > Reported-by: Pavel Machek <pavel@denx.de>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > --- a/drivers/i2c/busses/i2c-rzv2m.c
> > +++ b/drivers/i2c/busses/i2c-rzv2m.c
> > @@ -454,8 +454,10 @@ static int rzv2m_i2c_probe(struct platform_device
> *pdev)
> >         platform_set_drvdata(pdev, priv);
> >
> >         ret = i2c_add_numbered_adapter(adap);
> > -       if (ret < 0)
> > +       if (ret < 0) {
> > +               BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);
> 
> This needs to be wrapped inside pm_runtime_resume_and_get()/
> pm_runtime_put(), like is done in the .suspend() callback.

OK will use a helper function to disable I2C and share the code
between probe error path, suspend and remove.

+static int rzv2m_i2c_disable(struct device *dev, struct rzv2m_i2c_priv *priv)
+{
+       int ret;
+
+       ret = pm_runtime_resume_and_get(dev);
+       if (ret < 0)
+               return ret;
+
+       BIT_CLRL(priv->base + IICB0CTL0, IICB0IICE);
+       pm_runtime_put(dev);
+
+       return 0;
+}

Cheers,
Biju

> 
> Note that this is also lacking from the .remove() callback.
> 
> >                 pm_runtime_disable(dev);
> > +       }
> >
> >         return ret;
> >  }
> 
> 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