Hello,

On Wed, May 20, 2015 at 09:03:40PM +0800, Yingjoe Chen wrote:

On Wed, 2015-05-20 at 10:57 +0200, Uwe Kleine-K?nig wrote:

quoted

On Tue, May 19, 2015 at 12:40:08AM +0800, Eddie Huang wrote:

quoted

+	if (i2c->speed_hz > MAX_HS_MODE_SPEED)
+		return -EINVAL;

According to the plan to tune for the highest possible rate <=
i2c->speed_hz, you should handle the case (i2c->speed_hz >
MAX_HS_MODE_SPEED) like i2c->speed_hz == MAX_HS_MODE_SPEED.
Well, you might want to prevent an overflow in the calculation below
however.

The check here means we don't support speed > MAX_HS_MODE_SPEED. This is
different then slightly slower bus speed due to rounding error.

Well from outside there no difference between asking for 100 with
getting 98 or asking for 405 with getting 400. IMHO the expectation is
that you set the maximal possible rate when something too big for you is
requested. Consider a communication with a i2c device that supports 600
kHz. You have the choice to communicate with 400 kHz with that or not at
all.

quoted

quoted

+	best_mul = MAX_SAMPLE_CNT_DIV * max_step_cnt;
+
+	for (sample_cnt = 1; sample_cnt <= MAX_SAMPLE_CNT_DIV; sample_cnt++) {
+		step_cnt = (min_div + sample_cnt - 1) / sample_cnt;

DIV_ROUND_UP

quoted

+		cnt_mul = step_cnt * sample_cnt;
+		if (step_cnt > max_step_cnt)
+			continue;

I think it can happen that you have step_cnt > max_step_cnt here, but
that (sample_cnt, max_step_cnt) still is a good pair to consider. So:

If step_cnt > max_step_cnt, then sample_cnt * max_step_cnt < min_div.
This means (sample_cnt, max_step_cnt) is not a valid.

You're right.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |