RE: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

On Thu, Oct 05, 2023 at 06:21:35AM +0000, Ryan Chen wrote:

quoted

quoted

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Tuesday, September 5, 2023 7:55 PM On Tue, Sep 05, 2023 at
06:52:37AM +0000, Ryan Chen wrote:

quoted

quoted

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Friday, July 14, 2023 4:55 PM On Fri, Jul 14, 2023 at
03:45:22PM +0800, Ryan Chen wrote:

...

quoted

quoted

quoted

			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->timing_info.bus_freq_hz);

quoted

			for_each_set_bit(divisor, &divisor, 32) {

This is wrong.

quoted

				if ((divisor + 1) <= 32)
					break;

quoted

				divisor >>= 1;

And this.

quoted

				baseclk_idx++;

quoted

			}

for_each_set_bit() should use two different variables.

Will update by following.

for_each_set_bit(bit, &divisor, 32) {
    divisor >>= 1;
    baseclk_idx++;
}

It's unclear what you are trying to achieve here as the code is not equivalent to
the above.

quoted

quoted

quoted

		} else {
			baseclk_idx = i + 1;
			divisor = DIV_ROUND_UP(base_clk[i],

i2c_bus->timing_info.bus_freq_hz);

quoted

		}
	}

Hello Andy, 
	I modify it to be simple way by following.
	Because baseclk_idx will be set to register [I2CD04[3:0]] that is indicate the 0~15 different base clk selection.
	So I initial the base_clk[15] array and assign initial clk value, and then find which clk idx will be right SCL clk selection.

	000: pclk
	001: base_clk1
	002: base_clk2
	003: base_clk3
	004: base_clk4
	005: base_clk4/2
	006: base_clk4/4 
	006: base_clk4/8
	.....

static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
{
	unsigned long base_clk[15];
	int baseclk_idx;
	u32 clk_div_reg;
	u32 scl_low;
	u32 scl_high;
	int divisor = 0;
	u32 data;
	int i;

	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
	base_clk[0] = i2c_bus->apb_clk;
	for (i = 1; i < 5; i++) {
		base_clk[i] = (i2c_bus->apb_clk * 2) /
			(((clk_div_reg >> (i * 8)) & GENMASK(7, 0)) + 2);
	}
	for (i = 5; i < 16; i++) {
		base_clk[i] = base_clk[4] / (1 << (i - 5));
	}

	for (i = 0; i < 16; i++) {
		if ((base_clk[i] / i2c_bus->timing_info.bus_freq_hz) <= 32)
			break;
	}
	baseclk_idx = i;
	baseclk_idx = min(baseclk_idx, 15);
	divisor = min(divisor, 32);
	scl_low = min(divisor * 9 / 16 - 1, 15);
	scl_high = (divisor - scl_low - 2) & GENMASK(3, 0);
	data = (scl_high - 1) << 20 | scl_high << 16 | scl_low << 12 | baseclk_idx;
	if (i2c_bus->timeout) {
		data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);
		data |= AST2600_I2CC_TTIMEOUT(i2c_bus->timeout);
	}

	return data;
}

...

quoted

quoted

quoted

	divisor = min_t(unsigned long, divisor, 32);

Can't you use min()? min_t is a beast with some subtle corner cases.

Will update to
divisor = min(divisor, (unsigned long)32);

No, the idea behind min() is that _both_ arguments are of the same type, the
proposed above is wrong.

--
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel