[PATCH v9 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

ok fine

2017-01-19 9:02 GMT+01:00 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
> Hello Cedric,
>
> On Wed, Jan 18, 2017 at 09:55:39PM +0100, M'boumba Cedric Madianga wrote:
>> 2017-01-18 19:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig at pengutronix.de>:
>> > Hello Cedric,
>> >
>> > On Wed, Jan 18, 2017 at 04:21:17PM +0100, M'boumba Cedric Madianga wrote:
>> >> >> +      * In standard mode, the maximum allowed SCL rise time is 1000 ns.
>> >> >> +      * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> >> >> +      * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> >> >> +      * programmed with 09h.(1000 ns / 125 ns = 8 + 1)
>> >> >
>> >> >         * programmed with 0x9.
>> >> > (1000 ns / 125 ns = 8)
>> >> >
>> >> >> +      * So, for I2C standard mode TRISE = FREQ[5:0] + 1
>> >> >> +      *
>> >> >> +      * In fast mode, the maximum allowed SCL rise time is 300 ns.
>> >> >> +      * If, in the I2C_CR2 register, the value of FREQ[5:0] bits is equal to
>> >> >> +      * 0x08 so period = 125 ns therefore the TRISE[5:0] bits must be
>> >> >> +      * programmed with 03h.(300 ns / 125 ns = 2 + 1)
>> >> >
>> >> > as above s/03h/0x3/;
>> >>
>> >> ok
>> >>
>> >> > s/.(/. (/;
>> >> ok
>> >>
>> >> > s/+ 1//;
>> >> This formula is use to understand how we find the result 0x3
>> >> So, 0x3 => 300 ns / 125ns = 2 + 1
>> >
>> > Yeah, I understood that, but writing 300 ns / 125ns = 2 + 1 is
>> > irritating at best.
>>
>> Ok. I will write 0x3 (300 ns / 125 ns + 1) and 0x9 (1000 ns / 125 ns + 1)
>>
>> >> >         [...]
>> >> >         If DUTY = 1: (to reach 400 kHz)
>> >> >
>> >> > Strange.
>> >> >
>> >> >> +             val = DIV_ROUND_UP(i2c_dev->parent_rate, 400000 * 3);
>> >> >
>> >> > the manual reads:
>> >> >
>> >> >         The minimum allowed value is 0x04, except in FAST DUTY mode
>> >> >         where the minimum allowed value is 0x01
>> >> >
>> >> > You don't check for that, right?
>> >>
>> >> As the minimum freq value is 6 Mhz in fast mode the minimum CCR is 5
>> >> as described in the comment.
>> >> So I don't need to check that again as it is already done by checking
>> >> parent frequency.
>> >
>> > That would then go into a comment.
>>
>> Is it really needed ?
>> Adding some comments to explain implementation choices or  hardware
>> way of working is clearly useful.
>> But for this kind of thing, I am really surprised...
>
> TL;DR: It's not needed, but it probably helps.
>
> Consider someone sees a breakage in your driver in five years. By then
> you either have other interests or at least forgot 95 % of the thoughts
> you had when implementing the driver.
>
> So when I see:
>
>         val = DIV_ROUND_UP(i2c_dev->parent_rate, 400000 * 3);
>         ccr |= STM32F4_I2C_CCR_CCR(val);
>         writel_relaxed(ccr, i2c_dev->base + STM32F4_I2C_CCR);
>
> after seeing that the bus freq is wrong the obvious thoughts are:
>
>  - Does this use the right algorithm?
>  - Does this calculation result in values that are usable by the
>    hardware?
>
> That you thought about this today doesn't mean it's still right in five
> years. During that time a new hardware variant is available with a
> higher parent freq. Or there is a new errata available for the SoC.
>
> So to help answer the questions above it helps if you add today the
> formulas from the manual and a quick reason for why val fits into the
> respective bits in the CCR register. That comment might be wrong until
> then, too, but that only means you should make it easy to verify.
> Something like:
>
>         /*
>          * Function bla_blub made sure that parent_rate is not higher
>          * than 23 * pi MHz. As a result val is at most 13.2 bits wide
>          * and so fits into the CCR bits.
>          */
>
> This gives you in five years time the opportunity to quickly check
> bla_blub if this is still true, add a printk for parent_rate to check
> this, or quickly identify the bug in the code or the mismatch to the
> manual.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |