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

[Ryan Chen]

> On Thu, Oct 05, 2023 at 06:21:35AM +0000, Ryan Chen wrote:
> > > From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > > Sent: Tuesday, September 5, 2023 7:55 PM On Tue, Sep 05, 2023 at
> > > 06:52:37AM +0000, Ryan Chen wrote:
> > > > > From: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > > > > Sent: Friday, July 14, 2023 4:55 PM On Fri, Jul 14, 2023 at
> > > > > 03:45:22PM +0800, Ryan Chen wrote:
> 
> ...
> 
> > > > 			divisor = DIV_ROUND_UP(base_clk[3],
> > > i2c_bus->timing_info.bus_freq_hz);
> > > > 			for_each_set_bit(divisor, &divisor, 32) {
> > >
> > > This is wrong.
> > >
> > > > 				if ((divisor + 1) <= 32)
> > > > 					break;
> > >
> > > > 				divisor >>= 1;
> > >
> > > And this.
> > >
> > > > 				baseclk_idx++;
> > >
> > > > 			}
> > >
> > > 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.
> 
> > > > 		} else {
> > > > 			baseclk_idx = i + 1;
> > > > 			divisor = DIV_ROUND_UP(base_clk[i],
> > > i2c_bus->timing_info.bus_freq_hz);
> > > > 		}
> > > > 	}
> 
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;
}
> ...
> 
> > > > 	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
>