Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation

[Théo Lebrun]

Hello Linus,

On Wed Oct 9, 2024 at 1:34 PM CEST, Linus Walleij wrote:
> On Wed, Oct 9, 2024 at 12:23 PM Théo Lebrun <theo.lebrun@xxxxxxxxxxx> wrote:
> > --- a/drivers/i2c/busses/i2c-nomadik.c
> > +++ b/drivers/i2c/busses/i2c-nomadik.c
> > @@ -454,9 +454,12 @@ static void setup_i2c_controller(struct nmk_i2c_dev *priv)
> >          * operation, and the other is for std, fast mode, fast mode
> >          * plus operation. Currently we do not supprt high speed mode
> >          * so set brcr1 to 0.
> > +        *
> > +        * BRCR is a clock divider amount. Pick highest value that
> > +        * leads to rate strictly below target.
> >          */
>
> You could push in some more details from the commit message here so it's not
> so terse.

Most of the details from the commit message come from behavior changes:
what was done previously versus what is the new behavior we implement.

Having a clock divider picking the bus rate that is below the target
speed rather than above sounds rather intuitive. Eg when you ask for
400kHz you want <=400kHz, not >=400kHz.

I'll add that last sentence "Eg when you ask for 400kHz you want a bus
rate <=400kHz (and not >=400kHz)". It is straight forward and easy to
understand.

> >         brcr1 = FIELD_PREP(I2C_BRCR_BRCNT1, 0);
> > -       brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div));
> > +       brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_freq * div) + 1);
>
> Doesn't the last part correspond to something like
> #include <linux/math.h>
> u64 scaler = DIV_ROUND_DOWN_ULL(i2c_clk, (priv->clk_freq * div));
> brcr2 = FIELD_PREP(I2C_BRCR_BRCNT2, (u32)scaler);
>
> Certianly one of the in-kernel division helpers like DIV_ROUND_DOWN
> round_up() etc are better to use IMO, but I might not be understanding the
> fine details of the math here.

Indeed what we want is:
	DIV_ROUND_DOWN(i2c_clk, priv->clk_freq * div)

I see no reason to use DIV_ROUND_DOWN_ULL(). It would be useful if
	i2c_clk + (priv->clk_freq * div)
had a chance to overflow.

Worst case is:
	3_400_000 + (48_000_000 * 3) = 147_400_000

Will send v3 straight away as this is a significant change,
thanks Linus!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com