[PATCH 0/5] i2c: clock calculation cleanups for Renesas devices

[PATCH 0/5] i2c: clock calculation cleanups for Renesas devices

[Wolfram Sang]

While implementing FastMode+ support for the R-Car IP core, I noticed
potential for other cleanups. It turned out that it makes sense to apply
them first, so here is the series. Tested on a Renesas Falcon board with
an R-Car V3U. The calculated values are identical for 100 and 400kHz.
The RIIC patch is build tested only.

Looking forward to comments!


Wolfram Sang (5):
  i2c: rcar: avoid non-standard use of goto
  i2c: rcar: properly format a debug output
  i2c: rcar: calculate divider instead of brute-forcing it
  i2c: rcar: remove open coded DIV_ROUND_CLOSEST
  i2c: riic: avoid potential division by zero

 drivers/i2c/busses/i2c-rcar.c | 41 +++++++++++++++--------------------
 drivers/i2c/busses/i2c-riic.c |  2 +-
 2 files changed, 19 insertions(+), 24 deletions(-)

-- 
2.35.1

[PATCH 1/5] i2c: rcar: avoid non-standard use of goto

[Wolfram Sang]

Kernel functions goto somewhere on error conditions. Using goto for the
default path is irritating. Let's bail out on error instead and use a
proper retval.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

I dropped the Rev-bys because I think the changes are not trivial. YMMV.

 drivers/i2c/busses/i2c-rcar.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index a32a93f9a60d..49dfbeebf6b8 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -317,12 +317,12 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	for (scgd = 0; scgd < 0x40; scgd++) {
 		scl = ick / (20 + (scgd * 8) + round);
 		if (scl <= t.bus_freq_hz)
-			goto scgd_find;
+			break;
 	}
-	dev_err(dev, "it is impossible to calculate best SCL\n");
-	return -EIO;
 
-scgd_find:
+	if (scgd == 0x40)
+		goto err_no_val;
+
 	dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
 		scl, t.bus_freq_hz, rate, round, cdf, scgd);
 
@@ -330,6 +330,10 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	priv->icccr = scgd << cdf_width | cdf;
 
 	return 0;
+
+err_no_val:
+	dev_err(dev, "it is impossible to calculate best SCL\n");
+	return -EINVAL;
 }
 
 /*
-- 
2.35.1

[PATCH 2/5] i2c: rcar: properly format a debug output

[Wolfram Sang]

Use proper types and spacing.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 49dfbeebf6b8..4bf47e35094f 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -323,7 +323,7 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	if (scgd == 0x40)
 		goto err_no_val;
 
-	dev_dbg(dev, "clk %d/%d(%lu), round %u, CDF:0x%x, SCGD: 0x%x\n",
+	dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u, SCGD: %u\n",
 		scl, t.bus_freq_hz, rate, round, cdf, scgd);
 
 	/* keep icccr value */
-- 
2.35.1

[PATCH 3/5] i2c: rcar: calculate divider instead of brute-forcing it

[Wolfram Sang]

Instead of trying all values, we can actually compute it as the comment
suggests. It is unclear what the comment means with "involved", it works
nicely.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

@Geert: I hope the formulas are more clear now?
@Andi: I don't think that replacing 0x3f with a define helps
understanding the code, but I am open for discussion.

 drivers/i2c/busses/i2c-rcar.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 4bf47e35094f..2585092bed52 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -303,24 +303,16 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	round = (round + 500) / 1000;
 
 	/*
-	 * SCL	= ick / (20 + SCGD * 8 + F[(ticf + tr + intd) * ick])
-	 *
-	 * Calculation result (= SCL) should be less than
-	 * bus_speed for hardware safety
-	 *
-	 * We could use something along the lines of
-	 *	div = ick / (bus_speed + 1) + 1;
-	 *	scgd = (div - 20 - round + 7) / 8;
-	 *	scl = ick / (20 + (scgd * 8) + round);
-	 * (not fully verified) but that would get pretty involved
+	 * SCL	= ick / (20 + 8 * SCGD + F[(ticf + tr + intd) * ick])
+	 * 20 + 8 * SCGD + F[...] = ick / SCL
+	 * SCGD = ((ick / SCL) - 20 - F[...]) / 8
+	 * Result (= SCL) should be less than bus_speed for hardware safety
 	 */
-	for (scgd = 0; scgd < 0x40; scgd++) {
-		scl = ick / (20 + (scgd * 8) + round);
-		if (scl <= t.bus_freq_hz)
-			break;
-	}
+	scgd = DIV_ROUND_UP(ick, t.bus_freq_hz ?: 1);
+	scgd = DIV_ROUND_UP(scgd - 20 - round, 8);
+	scl = ick / (20 + 8 * scgd + round);
 
-	if (scgd == 0x40)
+	if (scgd > 0x3f)
 		goto err_no_val;
 
 	dev_dbg(dev, "clk %u/%u(%lu), round %u, CDF: %u, SCGD: %u\n",
-- 
2.35.1

[PATCH 4/5] i2c: rcar: remove open coded DIV_ROUND_CLOSEST

[Wolfram Sang]

It improves readability if we use the available helper.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 2585092bed52..5e97635faf78 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -291,16 +291,15 @@ static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv)
 	ick = rate / (cdf + 1);
 
 	/*
-	 * it is impossible to calculate large scale
-	 * number on u32. separate it
+	 * It is impossible to calculate a large scale number on u32. Separate it.
 	 *
 	 * F[(ticf + tr + intd) * ick] with sum = (ticf + tr + intd)
 	 *  = F[sum * ick / 1000000000]
 	 *  = F[(ick / 1000000) * sum / 1000]
 	 */
 	sum = t.scl_fall_ns + t.scl_rise_ns + t.scl_int_delay_ns;
-	round = (ick + 500000) / 1000000 * sum;
-	round = (round + 500) / 1000;
+	round = DIV_ROUND_CLOSEST(ick, 1000000);
+	round = DIV_ROUND_CLOSEST(round * sum, 1000);
 
 	/*
 	 * SCL	= ick / (20 + 8 * SCGD + F[(ticf + tr + intd) * ick])
-- 
2.35.1

[PATCH 5/5] i2c: riic: avoid potential division by zero

[Wolfram Sang]

Value comes from DT, so it could be 0. Unlikely, but could be.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 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 f0ee8871d5ae..e43ff483c56e 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -313,7 +313,7 @@ static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 	 * frequency with only 62 clock ticks max (31 high, 31 low).
 	 * Aim for a duty of 60% LOW, 40% HIGH.
 	 */
-	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz);
+	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz ?: 1);
 
 	for (cks = 0; cks < 7; cks++) {
 		/*
-- 
2.35.1

RE: [PATCH 5/5] i2c: riic: avoid potential division by zero

[Biju Das]

Hi Wolfram Sang,

> Subject: [PATCH 5/5] i2c: riic: avoid potential division by zero
> 
> Value comes from DT, so it could be 0. Unlikely, but could be.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  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 f0ee8871d5ae..e43ff483c56e 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -313,7 +313,7 @@ static int riic_init_hw(struct riic_dev *riic, struct
> i2c_timings *t)
>  	 * frequency with only 62 clock ticks max (31 high, 31 low).
>  	 * Aim for a duty of 60% LOW, 40% HIGH.
>  	 */
> -	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz);
> +	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz ?: 1);

Not sure clamping function min_t/min(t->bus_freq_hz, 1)
Or ternary condition is good in this case for avoiding potential division by 0?

Cheers,
Biju

> 
>  	for (cks = 0; cks < 7; cks++) {
>  		/*
> --
> 2.35.1

RE: [PATCH 5/5] i2c: riic: avoid potential division by zero

[Biju Das]

> Subject: RE: [PATCH 5/5] i2c: riic: avoid potential division by zero
> 
> Hi Wolfram Sang,
> 
> > Subject: [PATCH 5/5] i2c: riic: avoid potential division by zero
> >
> > Value comes from DT, so it could be 0. Unlikely, but could be.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >  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 f0ee8871d5ae..e43ff483c56e
> > 100644
> > --- a/drivers/i2c/busses/i2c-riic.c
> > +++ b/drivers/i2c/busses/i2c-riic.c
> > @@ -313,7 +313,7 @@ static int riic_init_hw(struct riic_dev *riic,
> > struct i2c_timings *t)
> >  	 * frequency with only 62 clock ticks max (31 high, 31 low).
> >  	 * Aim for a duty of 60% LOW, 40% HIGH.
> >  	 */
> > -	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz);
> > +	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz ?: 1);
> 
> Not sure clamping function min_t/min(t->bus_freq_hz, 1) 

Typo min->max.

Re: [PATCH 5/5] i2c: riic: avoid potential division by zero

[Wolfram Sang]

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


> > -	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz);
> > +	total_ticks = DIV_ROUND_UP(rate, t->bus_freq_hz ?: 1);
> 
> Not sure clamping function min_t/min(t->bus_freq_hz, 1)
> Or ternary condition is good in this case for avoiding potential division by 0?

Both work. I chose the ternary because I believe it involves less
computation and a tad tighter syntax.


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

Re: [PATCH 1/5] i2c: rcar: avoid non-standard use of goto

[Geert Uytterhoeven]

On Wed, 6 Sep 2023, Wolfram Sang wrote:
> Kernel functions goto somewhere on error conditions. Using goto for the
> default path is irritating. Let's bail out on error instead and use a
> proper retval.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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/5] i2c: rcar: properly format a debug output

[Geert Uytterhoeven]

On Wed, 6 Sep 2023, Wolfram Sang wrote:
> Use proper types and spacing.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 4/5] i2c: rcar: remove open coded DIV_ROUND_CLOSEST

[Geert Uytterhoeven]

On Wed, 6 Sep 2023, Wolfram Sang wrote:
> It improves readability if we use the available helper.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 5/5] i2c: riic: avoid potential division by zero

[Geert Uytterhoeven]

On Wed, 6 Sep 2023, Wolfram Sang wrote:
> Value comes from DT, so it could be 0. Unlikely, but could be.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 3/5] i2c: rcar: calculate divider instead of brute-forcing it

[Geert Uytterhoeven]

On Wed, 6 Sep 2023, Wolfram Sang wrote:
> Instead of trying all values, we can actually compute it as the comment
> suggests. It is unclear what the comment means with "involved", it works
> nicely.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.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 0/5] i2c: clock calculation cleanups for Renesas devices

[Wolfram Sang]

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

On Wed, Sep 06, 2023 at 10:00:18PM +0200, Wolfram Sang wrote:
> While implementing FastMode+ support for the R-Car IP core, I noticed
> potential for other cleanups. It turned out that it makes sense to apply
> them first, so here is the series. Tested on a Renesas Falcon board with
> an R-Car V3U. The calculated values are identical for 100 and 400kHz.
> The RIIC patch is build tested only.
> 
> Looking forward to comments!
> 
> 
> Wolfram Sang (5):
>   i2c: rcar: avoid non-standard use of goto
>   i2c: rcar: properly format a debug output
>   i2c: rcar: calculate divider instead of brute-forcing it
>   i2c: rcar: remove open coded DIV_ROUND_CLOSEST
>   i2c: riic: avoid potential division by zero
> 

Applied to for-next, thanks!


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