[PATCH v3 0/8] i2c: riic: driver cleanup and improvements

[PATCH v3 0/8] i2c: riic: driver cleanup and improvements

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Simplify and modernize the RIIC I2C driver with the following changes:

1. Refactor error handling in `riic_i2c_probe()` and `riic_init_hw()` by
   replacing `dev_err()` with `dev_err_probe()` and using a local `dev`
   pointer.
2. Use `BIT()` and `GENMASK()` macros for consistent and clear bit
   handling.
3. Manage reset lines with `devm_reset_control_get_exclusive()` to
   simplify resource handling.
4. Mark `riic_irqs` as `const` and simplify clock tick calculations with
   predefined macros.
5. Add `riic_bus_barrier()` to check bus availability and improve
   reliability.

Lad Prabhakar (8):
  i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
  i2c: riic: Use local `dev` pointer in `dev_err_probe()`
  i2c: riic: Use BIT macro consistently
  i2c: riic: Use GENMASK() macro for bitmask definitions
  i2c: riic: Make use of devres helper to request deasserted reset line
  i2c: riic: Mark riic_irqs array as const
  i2c: riic: Use predefined macro and simplify clock tick calculation
  i2c: riic: Add `riic_bus_barrier()` to check bus availability

 drivers/i2c/busses/i2c-riic.c | 123 ++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 58 deletions(-)

-- 
2.43.0

[PATCH v3 1/8] i2c: riic: Use dev_err_probe in probe and riic_init_hw functions

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Refactor error handling in the riic_i2c_probe() and riic_init_hw()
functions by replacing multiple dev_err() calls with dev_err_probe().

Additionally, update the riic_init_hw() function to use a local `dev`
pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
adapter is not initialized at this stage.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Squashed dev_err_probe() change from patch #2 into patch #1
- Updated commit message
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 9264adc97ca9..e1babd5077d4 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -356,11 +356,9 @@ static int riic_init_hw(struct riic_dev *riic)
 		rate /= 2;
 	}
 
-	if (brl > (0x1F + 3)) {
-		dev_err(&riic->adapter.dev, "invalid speed (%lu). Too slow.\n",
-			(unsigned long)t->bus_freq_hz);
-		return -EINVAL;
-	}
+	if (brl > (0x1F + 3))
+		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
+				     (unsigned long)t->bus_freq_hz);
 
 	brh = total_ticks - brl;
 
@@ -445,10 +443,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(riic->base);
 
 	riic->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(riic->clk)) {
-		dev_err(dev, "missing controller clock");
-		return PTR_ERR(riic->clk);
-	}
+	if (IS_ERR(riic->clk))
+		return dev_err_probe(dev, PTR_ERR(riic->clk),
+				     "missing controller clock");
 
 	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
 	if (IS_ERR(riic->rstc))
@@ -470,10 +467,9 @@ static int riic_i2c_probe(struct platform_device *pdev)
 
 		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
 				       0, riic_irqs[i].name, riic);
-		if (ret) {
-			dev_err(dev, "failed to request irq %s\n", riic_irqs[i].name);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to request irq %s\n",
+					     riic_irqs[i].name);
 	}
 
 	riic->info = of_device_get_match_data(dev);
-- 
2.43.0

Re: [PATCH v3 1/8] i2c: riic: Use dev_err_probe in probe and riic_init_hw functions

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:51:47AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Refactor error handling in the riic_i2c_probe() and riic_init_hw()
> functions by replacing multiple dev_err() calls with dev_err_probe().
> 
> Additionally, update the riic_init_hw() function to use a local `dev`
> pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
> adapter is not initialized at this stage.

...

> +	if (brl > (0x1F + 3))
> +		return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> +				     (unsigned long)t->bus_freq_hz);

There is nothing special about bus_freq_hz. Why casting?

...

>  		ret = devm_request_irq(dev, ret, riic_irqs[i].isr,

I hate code doing

		ret = foo(ret);

>  				       0, riic_irqs[i].name, riic);

> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to request irq %s\n",
> +					     riic_irqs[i].name);

While this following the original code, with the above change (introducing a
separate variable for IRQ) this might also print it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/8] i2c: riic: Use dev_err_probe in probe and riic_init_hw functions

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 28, 2024 at 11:35 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:47AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Refactor error handling in the riic_i2c_probe() and riic_init_hw()
> > functions by replacing multiple dev_err() calls with dev_err_probe().
> >
> > Additionally, update the riic_init_hw() function to use a local `dev`
> > pointer instead of `riic->adapter.dev` for dev_err_probe(), as the I2C
> > adapter is not initialized at this stage.
>
> ...
>
> > +     if (brl > (0x1F + 3))
> > +             return dev_err_probe(dev, -EINVAL, "invalid speed (%lu). Too slow.\n",
> > +                                  (unsigned long)t->bus_freq_hz);
>
> There is nothing special about bus_freq_hz. Why casting?
>
Ok, I'll update it like below:

    if (brl > (0x1F + 3))
        return dev_err_probe(dev, -EINVAL, "invalid speed (%uHz). Too slow.\n",
                     t->bus_freq_hz);

> ...
>
> >               ret = devm_request_irq(dev, ret, riic_irqs[i].isr,
>
> I hate code doing
>
>                 ret = foo(ret);
>
> >                                      0, riic_irqs[i].name, riic);
>
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "failed to request irq %s\n",
> > +                                          riic_irqs[i].name);
>
> While this following the original code, with the above change (introducing a
> separate variable for IRQ) this might also print it.
>
Ok, I'll create a new patch for this and have something like below:


    for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
        int irq = platform_get_irq(pdev, riic_irqs[i].res_num);

        ret = devm_request_irq(dev, irq, riic_irqs[i].isr,
                       0, riic_irqs[i].name, riic);
        if (ret)
            return ret;
    }

Cheers,
Prabhakar

[PATCH v3 2/8] i2c: riic: Use local `dev` pointer in `dev_err_probe()`

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Update the `riic_init_hw()` function to use the local `dev` pointer in
calls to `dev_err_probe()`. Previously, `riic_init_hw()` used
`riic->adapter.dev` in error reporting. Since this function is invoked
during the probe phase, the I2C adapter is not yet initialized, leading to
`(null) ...` being printed in error messages. This patch fixes the issue
by consistently using the local `dev` pointer, which points to
`riic->adapter.dev.parent`.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Moved replacing dev_err -> dev_err_probe into patch#1
- Dropped fixes tags
- Updated commit message
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index e1babd5077d4..01195ffd4c07 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -320,7 +320,7 @@ static int riic_init_hw(struct riic_dev *riic)
 				      : I2C_MAX_FAST_MODE_FREQ;
 
 	if (t->bus_freq_hz > max_freq)
-		return dev_err_probe(&riic->adapter.dev, -EINVAL,
+		return dev_err_probe(dev, -EINVAL,
 				     "unsupported bus speed %uHz (%u max)\n",
 				     t->bus_freq_hz, max_freq);
 
-- 
2.43.0

[PATCH v3 3/8] i2c: riic: Use BIT macro consistently

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Easier to read and ensures proper types.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 36 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 01195ffd4c07..954e066d61a8 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -46,32 +46,32 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#define ICCR1_ICE	0x80
-#define ICCR1_IICRST	0x40
-#define ICCR1_SOWP	0x10
+#define ICCR1_ICE	BIT(7)
+#define ICCR1_IICRST	BIT(6)
+#define ICCR1_SOWP	BIT(4)
 
-#define ICCR2_BBSY	0x80
-#define ICCR2_SP	0x08
-#define ICCR2_RS	0x04
-#define ICCR2_ST	0x02
+#define ICCR2_BBSY	BIT(7)
+#define ICCR2_SP	BIT(3)
+#define ICCR2_RS	BIT(2)
+#define ICCR2_ST	BIT(1)
 
 #define ICMR1_CKS_MASK	0x70
-#define ICMR1_BCWP	0x08
+#define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
-#define ICMR3_RDRFS	0x20
-#define ICMR3_ACKWP	0x10
-#define ICMR3_ACKBT	0x08
+#define ICMR3_RDRFS	BIT(5)
+#define ICMR3_ACKWP	BIT(4)
+#define ICMR3_ACKBT	BIT(3)
 
-#define ICFER_FMPE	0x80
+#define ICFER_FMPE	BIT(7)
 
-#define ICIER_TIE	0x80
-#define ICIER_TEIE	0x40
-#define ICIER_RIE	0x20
-#define ICIER_NAKIE	0x10
-#define ICIER_SPIE	0x08
+#define ICIER_TIE	BIT(7)
+#define ICIER_TEIE	BIT(6)
+#define ICIER_RIE	BIT(5)
+#define ICIER_NAKIE	BIT(4)
+#define ICIER_SPIE	BIT(3)
 
-#define ICSR2_NACKF	0x10
+#define ICSR2_NACKF	BIT(4)
 
 #define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
 
-- 
2.43.0

Re: [PATCH v3 3/8] i2c: riic: Use BIT macro consistently

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:51:49AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Easier to read and ensures proper types.

...

>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>

Does it include bits.h or equivalent (bitops.h, bitmap.h)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/8] i2c: riic: Use BIT macro consistently

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 28, 2024 at 11:35 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:49AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Easier to read and ensures proper types.
>
> ...
>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/reset.h>
>
> Does it include bits.h or equivalent (bitops.h, bitmap.h)?
>
No, I'll include bits.h so that even patch 4/8 can benefit from it.

Cheers,
Prabhakar

[PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
driver to improve readability and maintain consistency.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 954e066d61a8..ddae4b74a86b 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -55,7 +55,7 @@
 #define ICCR2_RS	BIT(2)
 #define ICCR2_ST	BIT(1)
 
-#define ICMR1_CKS_MASK	0x70
+#define ICMR1_CKS_MASK	GENMASK(6, 4)
 #define ICMR1_BCWP	BIT(3)
 #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
 
@@ -73,7 +73,7 @@
 
 #define ICSR2_NACKF	BIT(4)
 
-#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
+#define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */
 
 #define RIIC_INIT_MSG	-1
 
-- 
2.43.0

Re: [PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:51:50AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> driver to improve readability and maintain consistency.

...

> -#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
> +#define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */

I don't understand the comment. Does it mean the value should be 0x20?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[Lad, Prabhakar]

Hi Andy,

On Sat, Dec 28, 2024 at 11:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:50AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> > driver to improve readability and maintain consistency.
>
> ...
>
> > -#define ICBR_RESERVED        0xe0 /* Should be 1 on writes */
> > +#define ICBR_RESERVED        GENMASK(7, 5) /* Should be 1 on writes */
>
> I don't understand the comment. Does it mean the value should be 0x20?
>
Bit's 5-7 are marked as reserved and these should always be set to `1`
on write operation.

Cheers,
Prabhakar

Re: [PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[David Laight]

On Fri, 27 Dec 2024 11:51:50 +0000
Prabhakar <prabhakar.csengg@gmail.com> wrote:

> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> driver to improve readability and maintain consistency.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
> v2->v3
> - Collected RB and tested tags
> 
> v1->v2
> - Collected RB tag from Geert
> ---
>  drivers/i2c/busses/i2c-riic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> index 954e066d61a8..ddae4b74a86b 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -55,7 +55,7 @@
>  #define ICCR2_RS	BIT(2)
>  #define ICCR2_ST	BIT(1)
>  
> -#define ICMR1_CKS_MASK	0x70
> +#define ICMR1_CKS_MASK	GENMASK(6, 4)
>  #define ICMR1_BCWP	BIT(3)
>  #define ICMR1_CKS(_x)	((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)

I'm really not at all sure how this makes it 'more readable'.
Call me 'old fashioned' but I like hex constants - especially for bytes.
In this case it might be best to be consistent with the hardware datasheet.

Pretty much the only time I've actually used anything like BIT() was
for a spec that numbered the bits from 1 to 32 with bit 1 being the most
significant.

>  
> @@ -73,7 +73,7 @@
>  
>  #define ICSR2_NACKF	BIT(4)
>  
> -#define ICBR_RESERVED	0xe0 /* Should be 1 on writes */
> +#define ICBR_RESERVED	GENMASK(7, 5) /* Should be 1 on writes */

	'Should all be set on writes' ?

	David

>  
>  #define RIIC_INIT_MSG	-1
>  

Re: [PATCH v3 4/8] i2c: riic: Use GENMASK() macro for bitmask definitions

[Lad, Prabhakar]

Hi Andi,

On Wed, Jan 1, 2025 at 12:11 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Fri, 27 Dec 2024 11:51:50 +0000
> Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Replace raw bitmask values with the `GENMASK()` macro in the `i2c-riic`
> > driver to improve readability and maintain consistency.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > ---
> > v2->v3
> > - Collected RB and tested tags
> >
> > v1->v2
> > - Collected RB tag from Geert
> > ---
> >  drivers/i2c/busses/i2c-riic.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
> > index 954e066d61a8..ddae4b74a86b 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -55,7 +55,7 @@
> >  #define ICCR2_RS     BIT(2)
> >  #define ICCR2_ST     BIT(1)
> >
> > -#define ICMR1_CKS_MASK       0x70
> > +#define ICMR1_CKS_MASK       GENMASK(6, 4)
> >  #define ICMR1_BCWP   BIT(3)
> >  #define ICMR1_CKS(_x)        ((((_x) << 4) & ICMR1_CKS_MASK) | ICMR1_BCWP)
>
> I'm really not at all sure how this makes it 'more readable'.
> Call me 'old fashioned' but I like hex constants - especially for bytes.
> In this case it might be best to be consistent with the hardware datasheet.
>
Let me know if you dont accept such patches. I'll drop it from the next version.

> Pretty much the only time I've actually used anything like BIT() was
> for a spec that numbered the bits from 1 to 32 with bit 1 being the most
> significant.
>
> >
> > @@ -73,7 +73,7 @@
> >
> >  #define ICSR2_NACKF  BIT(4)
> >
> > -#define ICBR_RESERVED        0xe0 /* Should be 1 on writes */
> > +#define ICBR_RESERVED        GENMASK(7, 5) /* Should be 1 on writes */
>
>         'Should all be set on writes' ?
>
Yes.

Cheers,
Prabhakar

[PATCH v3 5/8] i2c: riic: Make use of devres helper to request deasserted reset line

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Simplify the `riic_i2c_probe()` function by using the
`devm_reset_control_get_optional_exclusive_deasserted()` API to request a
deasserted reset line. This eliminates the need to manually deassert the
reset control and the additional cleanup.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Updated error message
---
 drivers/i2c/busses/i2c-riic.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index ddae4b74a86b..edf2212e96ea 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -422,11 +422,6 @@ static struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 5, .isr = riic_tend_isr, .name = "riic-nack" },
 };
 
-static void riic_reset_control_assert(void *data)
-{
-	reset_control_assert(data);
-}
-
 static int riic_i2c_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -447,18 +442,10 @@ static int riic_i2c_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(riic->clk),
 				     "missing controller clock");
 
-	riic->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
+	riic->rstc = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
 	if (IS_ERR(riic->rstc))
 		return dev_err_probe(dev, PTR_ERR(riic->rstc),
-				     "Error: missing reset ctrl\n");
-
-	ret = reset_control_deassert(riic->rstc);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(dev, riic_reset_control_assert, riic->rstc);
-	if (ret)
-		return ret;
+				     "failed to acquire deasserted reset\n");
 
 	for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) {
 		ret = platform_get_irq(pdev, riic_irqs[i].res_num);
-- 
2.43.0

[PATCH v3 6/8] i2c: riic: Mark riic_irqs array as const

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The riic_irqs array describes the supported IRQs by the RIIC driver and
does not change at runtime.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index edf2212e96ea..378887b133a5 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -414,7 +414,7 @@ static int riic_init_hw(struct riic_dev *riic)
 	return 0;
 }
 
-static struct riic_irq_desc riic_irqs[] = {
+static const struct riic_irq_desc riic_irqs[] = {
 	{ .res_num = 0, .isr = riic_tend_isr, .name = "riic-tend" },
 	{ .res_num = 1, .isr = riic_rdrf_isr, .name = "riic-rdrf" },
 	{ .res_num = 2, .isr = riic_tdre_isr, .name = "riic-tdre" },
-- 
2.43.0

[PATCH v3 7/8] i2c: riic: Use predefined macro and simplify clock tick calculation

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Replace the hardcoded `1000000000` with the predefined `NANO` macro for
clarity. Simplify the code by introducing a `ns_per_tick` variable to
store `NANO / rate`, reducing redundancy and improving readability.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Collected RB tag from Geert
---
 drivers/i2c/busses/i2c-riic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 378887b133a5..a2d0cde5ac54 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -45,6 +45,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
+#include <linux/units.h>
 
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
@@ -312,6 +313,7 @@ static int riic_init_hw(struct riic_dev *riic)
 {
 	int ret;
 	unsigned long rate;
+	unsigned long ns_per_tick;
 	int total_ticks, cks, brl, brh;
 	struct i2c_timings *t = &riic->i2c_t;
 	struct device *dev = riic->adapter.dev.parent;
@@ -375,8 +377,9 @@ static int riic_init_hw(struct riic_dev *riic)
 	 * Remove clock ticks for rise and fall times. Convert ns to clock
 	 * ticks.
 	 */
-	brl -= t->scl_fall_ns / (1000000000 / rate);
-	brh -= t->scl_rise_ns / (1000000000 / rate);
+	ns_per_tick = NANO / rate;
+	brl -= t->scl_fall_ns / ns_per_tick;
+	brh -= t->scl_rise_ns / ns_per_tick;
 
 	/* Adjust for min register values for when SCLE=1 and NFE=1 */
 	if (brl < 1)
@@ -386,8 +389,7 @@ static int riic_init_hw(struct riic_dev *riic)
 
 	pr_debug("i2c-riic: freq=%lu, duty=%d, fall=%lu, rise=%lu, cks=%d, brl=%d, brh=%d\n",
 		 rate / total_ticks, ((brl + 3) * 100) / (brl + brh + 6),
-		 t->scl_fall_ns / (1000000000 / rate),
-		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
+		 t->scl_fall_ns / ns_per_tick, t->scl_rise_ns / ns_per_tick, cks, brl, brh);
 
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
-- 
2.43.0

Re: [PATCH v3 7/8] i2c: riic: Use predefined macro and simplify clock tick calculation

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:51:53AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Replace the hardcoded `1000000000` with the predefined `NANO` macro for
> clarity. Simplify the code by introducing a `ns_per_tick` variable to
> store `NANO / rate`, reducing redundancy and improving readability.

...

> -	brl -= t->scl_fall_ns / (1000000000 / rate);
> -	brh -= t->scl_rise_ns / (1000000000 / rate);
> +	ns_per_tick = NANO / rate;

So, why NANO and not NSEC_PER_SEC?

> +	brl -= t->scl_fall_ns / ns_per_tick;
> +	brh -= t->scl_rise_ns / ns_per_tick;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 7/8] i2c: riic: Use predefined macro and simplify clock tick calculation

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 28, 2024 at 11:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:53AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Replace the hardcoded `1000000000` with the predefined `NANO` macro for
> > clarity. Simplify the code by introducing a `ns_per_tick` variable to
> > store `NANO / rate`, reducing redundancy and improving readability.
>
> ...
>
> > -     brl -= t->scl_fall_ns / (1000000000 / rate);
> > -     brh -= t->scl_rise_ns / (1000000000 / rate);
> > +     ns_per_tick = NANO / rate;
>
> So, why NANO and not NSEC_PER_SEC?
>
Agreed, I'll switch to NSEC_PER_SEC.

Cheers,
Prabhakar

[PATCH v3 8/8] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Introduce a new `riic_bus_barrier()` function to verify bus availability
before initiating an I2C transfer. This function enhances the bus
arbitration check by ensuring that the SDA and SCL lines are not held low,
in addition to checking the BBSY flag using `readb_poll_timeout()`.

Previously, only the BBSY flag was checked to determine bus availability.
However, it is possible for the SDA line to remain low even when BBSY = 0.
This new implementation performs an additional check on the SDA and SCL
lines to avoid potential bus contention issues.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
v2->v3
- Collected RB and tested tags

v1->v2
- Used single register read to check SDA/SCL lines
---
 drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index a2d0cde5ac54..cf20e75da044 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -40,6 +40,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -50,6 +51,8 @@
 #define ICCR1_ICE	BIT(7)
 #define ICCR1_IICRST	BIT(6)
 #define ICCR1_SOWP	BIT(4)
+#define ICCR1_SCLI	BIT(1)
+#define ICCR1_SDAI	BIT(0)
 
 #define ICCR2_BBSY	BIT(7)
 #define ICCR2_SP	BIT(3)
@@ -135,6 +138,27 @@ static inline void riic_clear_set_bit(struct riic_dev *riic, u8 clear, u8 set, u
 	riic_writeb(riic, (riic_readb(riic, reg) & ~clear) | set, reg);
 }
 
+static int riic_bus_barrier(struct riic_dev *riic)
+{
+	int ret;
+	u8 val;
+
+	/*
+	 * The SDA line can still be low even when BBSY = 0. Therefore, after checking
+	 * the BBSY flag, also verify that the SDA and SCL lines are not being held low.
+	 */
+	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
+				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
+	if (ret)
+		return -EBUSY;
+
+	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
+	     (ICCR1_SDAI | ICCR1_SCLI))
+		return -EBUSY;
+
+	return 0;
+}
+
 static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct riic_dev *riic = i2c_get_adapdata(adap);
@@ -147,13 +171,11 @@ static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (ret)
 		return ret;
 
-	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
-		riic->err = -EBUSY;
+	riic->err = riic_bus_barrier(riic);
+	if (riic->err)
 		goto out;
-	}
 
 	reinit_completion(&riic->msg_done);
-	riic->err = 0;
 
 	riic_writeb(riic, 0, RIIC_ICSR2);
 
-- 
2.43.0

Re: [PATCH v3 8/8] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:51:54AM +0000, Prabhakar kirjoitti:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Introduce a new `riic_bus_barrier()` function to verify bus availability
> before initiating an I2C transfer. This function enhances the bus
> arbitration check by ensuring that the SDA and SCL lines are not held low,
> in addition to checking the BBSY flag using `readb_poll_timeout()`.
> 
> Previously, only the BBSY flag was checked to determine bus availability.
> However, it is possible for the SDA line to remain low even when BBSY = 0.
> This new implementation performs an additional check on the SDA and SCL
> lines to avoid potential bus contention issues.

...

> +	/*
> +	 * The SDA line can still be low even when BBSY = 0. Therefore, after checking
> +	 * the BBSY flag, also verify that the SDA and SCL lines are not being held low.
> +	 */
> +	ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
> +				 !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
> +	if (ret)
> +		return -EBUSY;

Why the return code is shadowed? Is it requirement by ABI?

> +	if ((riic_readb(riic, RIIC_ICCR1) & (ICCR1_SDAI | ICCR1_SCLI)) !=
> +	     (ICCR1_SDAI | ICCR1_SCLI))
> +		return -EBUSY;
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 8/8] i2c: riic: Add `riic_bus_barrier()` to check bus availability

[Lad, Prabhakar]

Hi Andy,

Thank you for the review.

On Sat, Dec 28, 2024 at 11:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Fri, Dec 27, 2024 at 11:51:54AM +0000, Prabhakar kirjoitti:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Introduce a new `riic_bus_barrier()` function to verify bus availability
> > before initiating an I2C transfer. This function enhances the bus
> > arbitration check by ensuring that the SDA and SCL lines are not held low,
> > in addition to checking the BBSY flag using `readb_poll_timeout()`.
> >
> > Previously, only the BBSY flag was checked to determine bus availability.
> > However, it is possible for the SDA line to remain low even when BBSY = 0.
> > This new implementation performs an additional check on the SDA and SCL
> > lines to avoid potential bus contention issues.
>
> ...
>
> > +     /*
> > +      * The SDA line can still be low even when BBSY = 0. Therefore, after checking
> > +      * the BBSY flag, also verify that the SDA and SCL lines are not being held low.
> > +      */
> > +     ret = readb_poll_timeout(riic->base + riic->info->regs[RIIC_ICCR2], val,
> > +                              !(val & ICCR2_BBSY), 10, riic->adapter.timeout);
> > +     if (ret)
> > +             return -EBUSY;
>
> Why the return code is shadowed? Is it requirement by ABI?
>
I will propagate the error instead of shadowing it.

Cheers,
Prabhakar

Re: [PATCH v3 0/8] i2c: riic: driver cleanup and improvements

[Andi Shyti]

Hi,

> Lad Prabhakar (8):
>   i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
>   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
>   i2c: riic: Use BIT macro consistently
>   i2c: riic: Use GENMASK() macro for bitmask definitions
>   i2c: riic: Make use of devres helper to request deasserted reset line
>   i2c: riic: Mark riic_irqs array as const
>   i2c: riic: Use predefined macro and simplify clock tick calculation
>   i2c: riic: Add `riic_bus_barrier()` to check bus availability

merged to i2c/i2c-host.

Thanks,
Andi

Re: [PATCH v3 0/8] i2c: riic: driver cleanup and improvements

[Andy Shevchenko]

Fri, Dec 27, 2024 at 11:10:22PM +0100, Andi Shyti kirjoitti:

...

> >   i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
> >   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
> >   i2c: riic: Use BIT macro consistently
> >   i2c: riic: Use GENMASK() macro for bitmask definitions
> >   i2c: riic: Make use of devres helper to request deasserted reset line
> >   i2c: riic: Mark riic_irqs array as const
> >   i2c: riic: Use predefined macro and simplify clock tick calculation
> >   i2c: riic: Add `riic_bus_barrier()` to check bus availability
> 
> merged to i2c/i2c-host.

There are some comments, up to you how to proceed, they seem not to be any
critical.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/8] i2c: riic: driver cleanup and improvements

[Andi Shyti]

Hi Andy,

On Sun, Dec 29, 2024 at 01:40:54AM +0200, Andy Shevchenko wrote:
> Fri, Dec 27, 2024 at 11:10:22PM +0100, Andi Shyti kirjoitti:
> 
> ...
> 
> > >   i2c: riic: Use dev_err_probe in probe and riic_init_hw functions
> > >   i2c: riic: Use local `dev` pointer in `dev_err_probe()`
> > >   i2c: riic: Use BIT macro consistently
> > >   i2c: riic: Use GENMASK() macro for bitmask definitions
> > >   i2c: riic: Make use of devres helper to request deasserted reset line
> > >   i2c: riic: Mark riic_irqs array as const
> > >   i2c: riic: Use predefined macro and simplify clock tick calculation
> > >   i2c: riic: Add `riic_bus_barrier()` to check bus availability
> > 
> > merged to i2c/i2c-host.
> 
> There are some comments, up to you how to proceed, they seem not to be any
> critical.

first of all, welcome back :-)

I'd like the comments to be addressed, even if they are not
critical.

So that I'm going to remove this series for now until there are
no more questions.

Thanks for looking into this series,
Andi