Hello,
I like it now. There are only a few small nitpicks, not sure its worth
to respin if noone else has concerns. See below.
On Thu, Dec 11, 2014 at 02:00:49PM +0800, Addy Ke wrote:
> The number of clock cycles to be written into the CLKDIV register
> that determines the I2C clk high phase includes the rise time.
> So to meet the timing requirements defined in the I2C specification
> which defines the minimal time SCL has to be high, the rise time
> has to taken into account. The same applies to the low phase with
> falling time.
>
> In my test on RK3288-Pink2 board, which is not an upstream board yet,
> if external pull-up resistor is 4.7K, rise_ns is about 700ns.
> So the measured high_ns is about 3900ns, which is less than 4000ns
> (the minimum high_ns in I2C specification for Standard-mode).
>
> To fix this bug, min_low_ns should include fall time and min_high_ns
s/,//
> should include rise time too.
I'd skip the "too". If you want to keep it, s/time/time,/.
> This patch merged the patch that Doug submitted to chromium project,
AFAIK s/,//
For correctness, does this patch needs Doug's Sob?
> which can get the rise and fall times for signals from the device tree.
> This allows us to more accurately calculate timings. see:
> https://chromium-review.googlesource.com/#/c/232774/
>
> Signed-off-by: Addy Ke <addy.ke at rock-chips.com>
> ---
> [...]
> @@ -469,29 +476,33 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> [...]
> if (scl_rate <= 100000) {
> - min_low_ns = 4700;
> - min_high_ns = 4000;
> - max_data_hold_ns = 3450;
> - data_hold_buffer_ns = 50;
> + /* Standard-mode */
> + spec_min_low_ns = 4700;
> + spec_min_high_ns = 4000;
> + spec_max_data_hold_ns = 3450;
> + spec_data_hold_buffer_ns = 50;
> } else {
> - min_low_ns = 1300;
> - min_high_ns = 600;
> - max_data_hold_ns = 900;
> - data_hold_buffer_ns = 50;
> + /* Fast-mode */
> + spec_min_low_ns = 1300;
> + spec_min_high_ns = 600;
> + spec_max_data_hold_ns = 900;
> + spec_data_hold_buffer_ns = 50;
The background of my question regarding data_hold_buffer_ns in the last
round was: If data_hold_buffer_ns doesn't appear in the I2C
specification, should it be renamed to spec_... ? *shrug*
> }
> - max_low_ns = max_data_hold_ns * 2 - data_hold_buffer_ns;
> + min_low_ns = spec_min_low_ns + fall_ns;
> + min_high_ns = spec_min_high_ns + rise_ns;
> + max_low_ns = spec_max_data_hold_ns * 2 - spec_data_hold_buffer_ns;
> min_total_ns = min_low_ns + min_high_ns;
>
> /* Adjust to avoid overflow */
> @@ -510,8 +521,8 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> min_div_for_hold = (min_low_div + min_high_div);
>
> /*
> - * This is the maximum divider so we don't go over the max.
> - * We don't round up here (we round down) since this is a max.
> + * This is the maximum divider so we don't go over the maximum.
> + * We don't round up here (we round down) since this is a maximum.
> */
> max_low_div = clk_rate_khz * max_low_ns / (8 * 1000000);
>
> @@ -544,7 +555,7 @@ static int rk3x_i2c_calc_divs(unsigned long clk_rate, unsigned long scl_rate,
> ideal_low_div = DIV_ROUND_UP(clk_rate_khz * min_low_ns,
> scl_rate_khz * 8 * min_total_ns);
>
> - /* Don't allow it to go over the max */
> + /* Don't allow it to go over the maximum */
> if (ideal_low_div > max_low_div)
> ideal_low_div = max_low_div;
>
> @@ -588,8 +599,8 @@ static void rk3x_i2c_adapt_div(struct rk3x_i2c *i2c, unsigned long clk_rate)
> u64 t_low_ns, t_high_ns;
> int ret;
>
> - ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, &div_low,
> - &div_high);
> + ret = rk3x_i2c_calc_divs(clk_rate, i2c->scl_frequency, i2c->rise_ns,
> + i2c->fall_ns, &div_low, &div_high);
>
> WARN_ONCE(ret != 0, "Could not reach SCL freq %u", i2c->scl_frequency);
>
> @@ -633,9 +644,9 @@ static int rk3x_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
> switch (event) {
> case PRE_RATE_CHANGE:
> if (rk3x_i2c_calc_divs(ndata->new_rate, i2c->scl_frequency,
> - &div_low, &div_high) != 0) {
> + i2c->rise_ns, i2c->fall_ns, &div_low,
> + &div_high) != 0)
> return NOTIFY_STOP;
> - }
>
> /* scale up */
> if (ndata->new_rate > ndata->old_rate)
> @@ -859,6 +870,21 @@ static int rk3x_i2c_probe(struct platform_device *pdev)
> i2c->scl_frequency = DEFAULT_SCL_RATE;
> }
>
> + /*
> + * Read rise and fall ns.
> + * If not, there use the default maximum from specification.
I'd write:
Read rise and fall time from device tree. If not available use
the default maximum timing from the specification.
(Otherwise I think the comma needs to go after "there" in your
sentence.)
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |