[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
> 	.....
> 

Sorry updated.

> static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus) {
> 	unsigned long base_clk[15];
	unsigned long base_clk[16];

> 	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)
		{
			baseclk_idx = i;
			divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->timing_info.bus_freq_hz);
		}
> 			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
> >