linux-kernel - Re: [PATCH v9 2/5] i2c: Add STM32F4 I2C driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 Jan 2017 21:55:39 +0100
From:   "M'boumba Cedric Madianga" <cedric.madianga@...il.com>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Wolfram Sang <wsa@...-dreams.de>, Rob Herring <robh+dt@...nel.org>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Patrice Chotard <patrice.chotard@...com>,
        Russell King <linux@...linux.org.uk>,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 2/5] i2c: Add STM32F4 I2C driver

2017-01-18 19:42 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@...gutronix.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...

>
>> > CCR is 11 bits wide. A comment confirming that this cannot overflow
>> > would be nice.
>>
>> Again there is no chance of overflow thanks to parent frequency check
>
> Right, this time I saw this myself, so I requested a comment stating
> this fact.

ditto

Best regards,
Cedric