ok fine 2017-01-19 9:02 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>: > 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@xxxxxxxxxxxxxx>: >> > 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/ | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
