On Wed Oct 9, 2024 at 3:31 PM CEST, Théo Lebrun wrote: > 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) s/DIV_ROUND_DOWN/DIV_ROUND_UP/ (sorry for the confusion) > > I see no reason to use DIV_ROUND_DOWN_ULL(). It would be useful if s/DIV_ROUND_DOWN_ULL/DIV_ROUND_UP_ULL/ > 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 Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
