Add ASPEED AST2600 I2Cv2 controller driver

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

drivers/i2c/busses/i2c-ast2600.c new file +15 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

I have doubts that you tested this. I think you have here double free/del of the adapter.

On 14/07/2023 09:45, Ryan Chen wrote:

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	i2c_del_adapter(&i2c_bus->adap);

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-altera.c#L473

Do you mean it is not necessary?

Hello,

Quoted 8 lines · Ryan Chen

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

...

Quoted 15 lines

drivers/i2c/busses/i2c-ast2600.c new file +15 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

Quoted 1 line

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-altera.c#L473

Do you mean it is not necessary?

Quoted 8 lines

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

...

Quoted 16 lines

drivers/i2c/busses/i2c-ast2600.c new file +15 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

Quoted 1 line

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-n
pcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-a
ltera.c#L473

Do you mean it is not necessary?

Instead of giving you the fish, I think much more learning experience is to teach you how to fish. Please unbind your driver (echo the device name to proper unbind file in sysfs). The best if you build your kernel with KASAN.

Quoted 8 lines

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

...

Quoted 16 lines

drivers/i2c/busses/i2c-ast2600.c new file +15 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

Quoted 1 line

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-n
pcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-a
ltera.c#L473

Do you mean it is not necessary?

Instead of giving you the fish, I think much more learning experience is to teach you how to fish. Please unbind your driver (echo the device name to proper unbind file in sysfs). The best if you build your kernel with KASAN.

Thanks, will do this test with unbind to understand your point.

Quoted 19 lines

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-n
pcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-a
ltera.c#L473

Quoted 5 lines

Do you mean it is not necessary?

Instead of giving you the fish, I think much more learning experience is to teach you how to fish. Please unbind your driver (echo the device name to proper unbind file in sysfs). The best if you build your kernel with KASAN.

Thanks, will do this test with unbind to understand your point.

I do my the test with enable kernel config KASAN.
I don't see any error dump. You can see the following. It use i2c-0 bind/unbind. Did I miss some test ??

root at ast2600-default:/sys/bus/platform/drivers/i2c_ast2600# i2cdetect -l

i2c-0   i2c             1e78a080.i2c-bus                        I2C adapter
i2c-1   i2c             1e78a100.i2c-bus                        I2C adapter
i2c-2   i2c             1e78a180.i2c-bus                        I2C adapter
i2c-3   i2c             1e78a200.i2c-bus                        I2C adapter
i2c-4   i2c             1e78a280.i2c-bus                        I2C adapter
i2c-5   i2c             1e78a300.i2c-bus                        I2C adapter
i2c-6   i2c             1e78a380.i2c-bus                        I2C adapter
i2c-7   i2c             1e78a400.i2c-bus                        I2C adapter
i2c-8   i2c             1e78a480.i2c-bus                        I2C adapter
i2c-9   i2c             1e78a500.i2c-bus                        I2C adapter
i2c-12  i2c             1e78a680.i2c-bus                        I2C adapter
i2c-13  i2c             1e78a700.i2c-bus                        I2C adapter
i2c-14  i2c             1e78a780.i2c-bus                        I2C adapter
i2c-15  i2c             1e78a800.i2c-bus                        I2C adapter

root at ast2600-default:/sys/bus/platform/drivers/i2c_ast2600# echo 1e78a080.i2c-bus > unbind

Quoted 8 lines

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---

...

Quoted 16 lines

drivers/i2c/busses/i2c-ast2600.c new file +15 -0

+	ret = devm_i2c_add_adapter(dev, &i2c_bus->adap);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ast2600_i2c_remove(struct platform_device *pdev) {
+	struct ast2600_i2c_bus *i2c_bus = platform_get_drvdata(pdev);
+
+	/* Disable everything. */
+	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
+
+	i2c_del_adapter(&i2c_bus->adap);

Quoted 1 line

I have doubts that you tested this. I think you have here double free/del of the adapter.

Sorry, i can't catch your point for double free the adapter. It should use i2c_del_adapter in driver remove function. All the driver doing this
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-n
pcm7xx.c#L2373
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-a
ltera.c#L473

Do you mean it is not necessary?

Instead of giving you the fish, I think much more learning experience is to teach you how to fish. Please unbind your driver (echo the device name to proper unbind file in sysfs). The best if you build your kernel with KASAN.

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+	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;
+}

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	reinit_completion(&i2c_bus->cmd_complete);
+	i2c_bus->cmd_err = 0;
+
+	/* Check 0x14's SDA and SCL status */

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+		ret = -EPROTO;
+	}
+
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}

+#ifdef CONFIG_I2C_SLAVE

+#endif

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+		} else if (i2c_bus->mode == BUFF_MODE) {
+			/* buff mode */
+			cmd |= AST2600_I2CM_RX_BUFF_EN;
 
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+			writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+			       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		} else {
 

(quoted diff) +1 -0

+		}

drivers/i2c/busses/i2c-ast2600.c new file +12 -0

+			u8 wbuf[4];
+			/* buff mode */
+			if (msg->len > i2c_bus->buf_size) {
+				xfer_len = i2c_bus->buf_size;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_BUFF_EN | AST2600_I2CM_TX_CMD;
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))

(quoted diff) +1 -0

+			}

+static int ast2600_i2c_is_irq_error(u32 irq_status)

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+{
+	if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+		return -EAGAIN;
+	if (irq_status & (AST2600_I2CM_SDA_DL_TO | AST2600_I2CM_SCL_LOW_TO))
+		return -EBUSY;
+	if (irq_status & (AST2600_I2CM_ABNORMAL))
+		return -EPROTO;
+
+	return 0;
+}

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+				u8 wbuf[4];
+
+				cmd |= AST2600_I2CM_TX_BUFF_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= AST2600_I2CM_STOP_CMD;

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_PKT_MODE_EN) {
+			u32 slave_cmd = readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(slave_cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+		}
+		fallthrough;
+#endif

drivers/i2c/busses/i2c-ast2600.c new file +47 -0

+	if (!i2c_bus->alert_enable)
+		sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+	if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		i2c_bus->cmd_err = -EPROTO;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_BUS_RECOVER & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		i2c_bus->cmd_err = 0;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_SMBUS_ALT & sts) {
+		if (ier & AST2600_I2CM_SMBUS_ALT) {
+			/* Disable ALT INT */
+			writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_IER);
+			i2c_handle_smbus_alert(i2c_bus->ara);
+			writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_ISR);
+			dev_err(i2c_bus->dev,
+				"ast2600_master_alert_recv bus id %d, Disable Alt, Please Imple\n",
+				i2c_bus->adap.nr);
+			return 1;
+		}
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+	if (i2c_bus->cmd_err) {
+		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_PKT_DONE & sts) {
+		ast2600_i2c_master_package_irq(i2c_bus, sts);
+		return 1;
+	}
+
+	return 0;
+}

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+			i2c_bus->buf_size = resource_size(res) / 2;
+		else
+			i2c_bus->mode = BYTE_MODE;
+	}

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+	if (!ret)
+		i2c_bus->timeout /= 4096;

drivers/i2c/busses/i2c-ast2600.c new file +13 -0

+		i2c_bus->alert_enable = true;
+		i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
+		if (!i2c_bus->ara)
+			dev_warn(dev, "Failed to register ARA client\n");
+
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT,
+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	} else {
+		i2c_bus->alert_enable = false;
+		/* Set interrupt generation of I2C master controller */
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	}

Quoted 5 lines

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

...

+ bits.h

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>

drivers/i2c/busses/i2c-ast2600.c line 20

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
 
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>

You missed property.h
and these of*.h probably not needed at all, see below.

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>

...

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) &

GENMASK(7, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) &

GENMASK(5, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) &

GENMASK(4, 0)) + 1)

With right shifts it's better to have GENMASK to be applied first, it will show exact MSB of the bitfield.

(Ditto for other cases similar to these)

...

drivers/i2c/busses/i2c-ast2600.c

Quoted 12 lines

drivers/i2c/busses/i2c-ast2600.c new file +12 -0

+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+	unsigned long base_clk[4];
+	int baseclk_idx;
+	u32 clk_div_reg;
+	u32 scl_low;
+	u32 scl_high;
+	int divisor;
+	int inc = 0;
+	u32 data;
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL,

&clk_div_reg);

drivers/i2c/busses/i2c-ast2600.c line 304

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	for (int i = 0; i < 4; i++) {

See below.

drivers/i2c/busses/i2c-ast2600.c line 306

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		base_clk[i] = (i2c_bus->apb_clk * 10) /
+		(((((clk_div_reg >> (i * 8)) & GENMASK(7, 0)) + 2) * 10) / 2);

Second line is wrongly indented.

drivers/i2c/busses/i2c-ast2600.c

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	}
 
+	if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 0;
+		divisor = DIV_ROUND_UP(i2c_bus->apb_clk,

i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 314

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+	} else {
 
+		int i;
+

Just add to the definition block:

	unsigned int i;

drivers/i2c/busses/i2c-ast2600.c

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		for (i = 0; i < 4; i++) {
+			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32) {
 
+				baseclk_idx = i + 1;
+				divisor = DIV_ROUND_UP(base_clk[i],

i2c_bus->bus_frequency);

These two can be moved outside of the loop

Review

Quoted 2 lines

(quoted diff) +2 -0

+				break;
+			}

			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32)
				break;

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		}
+		if (i == 4) {
+			baseclk_idx = 4;
+			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 327

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+			while ((divisor + inc) > 32) {
+				inc |= divisor & 0x1;
+				divisor >>= 1;

	unsigned long divisor;

			for_each_set_bit(divisor, ...)

I.o.w. think about this, maybe you can refactor with the above.

drivers/i2c/busses/i2c-ast2600.c line 330

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+				baseclk_idx++;
+			}
+			divisor += inc;

		} else {
			...those two lines...

drivers/i2c/busses/i2c-ast2600.c line 337

Quoted 9 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+		}
 
+	}
+
+	divisor = min_t(int, divisor, 32);
+	baseclk_idx &= GENMASK(3, 0);
 
+	scl_low = ((divisor * 9) / 16) - 1;
+	scl_low = min_t(u32, scl_low, GENMASK(3, 0));

(with the divisor being unsigned long) this can be rewritten as

	scl_low = min(divisor * 9 / 16 - 1, GENMASK(3, 0));

which improves type checking and readability.

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+	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;
+}

...

drivers/i2c/busses/i2c-ast2600.c line 357

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) {
+	int ret = 0;
+	u32 ctrl;
+	u32 state;
+	int r;
 
+	dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr,
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Why you can't reuse "state" (assigned below)?
If not, then something like

	/* ...comment that state can be changed... */
	state = ...
	dev_dbg(state);

drivers/i2c/busses/i2c-ast2600.c

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+	ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) |
+AST2600_I2CC_MASTER_EN,

will it be different from ctrl value?

drivers/i2c/busses/i2c-ast2600.c

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	reinit_completion(&i2c_bus->cmd_complete);
+	i2c_bus->cmd_err = 0;
+
+	/* Check 0x14's SDA and SCL status */
+	state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state &

AST2600_I2CC_SCL_LINE_STS)) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base +

AST2600_I2CM_CMD_STS);

Review

Quoted 1 line

(quoted diff) +1 -0

+		r = wait_for_completion_timeout(&i2c_bus->cmd_complete,

i2c_bus->adap.timeout);

drivers/i2c/busses/i2c-ast2600.c line 379

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+		if (r == 0) {
+			dev_dbg(i2c_bus->dev, "recovery timed out\n");
+			ret = -ETIMEDOUT;
 
+		} else {
+			if (i2c_bus->cmd_err) {

		} else if (...) {

drivers/i2c/busses/i2c-ast2600.c line 387

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+				dev_dbg(i2c_bus->dev, "recovery error\n");
+				ret = -EPROTO;
+			}
+		}
+	}
+
+	dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

As above.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
+AST2600_I2CC_BUS_BUSY_STS) {

Two sequential reads may give you different values?

drivers/i2c/busses/i2c-ast2600.c line 390

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n",
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Again? With this inconsistency it will be "nice" to debug.

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+		ret = -EPROTO;
+	}
+
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}

...

Quoted 1 line

+#ifdef CONFIG_I2C_SLAVE

For (at least) review purposes I recommend to split slave out to the separate change. This driver is 16 hundreds LoCs long...

Quoted 1 line

+#endif

...

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+		} else if (i2c_bus->mode == BUFF_MODE) {
+			/* buff mode */
+			cmd |= AST2600_I2CM_RX_BUFF_EN;
 
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");

drivers/i2c/busses/i2c-ast2600.c line 838

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+				xfer_len = 1;
+			} else {
+				if (msg->len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					xfer_len = msg->len;
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}

This...

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+			writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+			       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		} else {
 

drivers/i2c/busses/i2c-ast2600.c line 851

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+			/* byte mode */
+			xfer_len = 1;
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) {
+					if (msg->len == 1)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}

...and this have a lot in common, can it be deduplicated?

Quoted 1 line

(quoted diff) +1 -0

+		}

...

drivers/i2c/busses/i2c-ast2600.c line 862

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+			if (msg->len > AST2600_I2C_DMA_SIZE) {
+				xfer_len = AST2600_I2C_DMA_SIZE;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}

See above.

...

drivers/i2c/busses/i2c-ast2600.c

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+			u8 wbuf[4];
+			/* buff mode */
+			if (msg->len > i2c_bus->buf_size) {
+				xfer_len = i2c_bus->buf_size;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_BUFF_EN |

AST2600_I2CM_TX_CMD;

drivers/i2c/busses/i2c-ast2600.c line 904

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i];
+					if (i % 4 == 3)
 
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);

This is incorrect memory accessor.

drivers/i2c/busses/i2c-ast2600.c line 907

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));

Ditto.

Quoted 1 line

(quoted diff) +1 -0

+			}

...

Quoted 1 line

+static int ast2600_i2c_is_irq_error(u32 irq_status)

This function is not boolean, so "_is_" seems misleading.

This is basically error code conversion, something like

ast2600_i2c_irq_err_to_errno(u32 irq_status)

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+{
+	if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+		return -EAGAIN;
+	if (irq_status & (AST2600_I2CM_SDA_DL_TO |

AST2600_I2CM_SCL_LOW_TO))

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+		return -EBUSY;
+	if (irq_status & (AST2600_I2CM_ABNORMAL))
+		return -EPROTO;
+
+	return 0;
+}

...

Quoted 14 lines

drivers/i2c/busses/i2c-ast2600.c new file +14 -0

+				u8 wbuf[4];
+
+				cmd |= AST2600_I2CM_TX_BUFF_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= AST2600_I2CM_STOP_CMD;
+				}
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
+					if (i % 4 == 3)
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);

drivers/i2c/busses/i2c-ast2600.c line 1039

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Wrong memory accessors. You should use something from asm/byteorder.h which includes linux/byteorder/generic.h.

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+#ifdef CONFIG_I2C_SLAVE
+		/* Workaround for master/slave package mode enable rx done stuck

issue

Review

Quoted 1 line

(quoted diff) +1 -0

+		 * When master go for first read (RX_DONE), slave mode will also

effect

drivers/i2c/busses/i2c-ast2600.c line 1057

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		 * Then controller will send nack, not operate anymore.
+		 */

/*
* Wrong style of multi-line
* comments. You need to fix
* them in the entire driver.
*/

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) &

AST2600_I2CS_PKT_MODE_EN) {

Review

Quoted 1 line

(quoted diff) +1 -0

+			u32 slave_cmd = readl(i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

drivers/i2c/busses/i2c-ast2600.c

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(slave_cmd, i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+		}
+		fallthrough;
+#endif

...

drivers/i2c/busses/i2c-ast2600.c line 1155

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus) {
+	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER);
+	u32 ctrl = 0;

Redundant assignment.

drivers/i2c/busses/i2c-ast2600.c

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+	if (!i2c_bus->alert_enable)
+		sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+	if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base +

AST2600_I2CM_ISR);

drivers/i2c/busses/i2c-ast2600.c

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		i2c_bus->cmd_err = -EPROTO;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_BUS_RECOVER & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base +

AST2600_I2CM_ISR);

drivers/i2c/busses/i2c-ast2600.c

Quoted 9 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+		i2c_bus->cmd_err = 0;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_SMBUS_ALT & sts) {
+		if (ier & AST2600_I2CM_SMBUS_ALT) {
+			/* Disable ALT INT */
+			writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base +

AST2600_I2CM_IER);

Review

Quoted 2 lines

(quoted diff) +2 -0

+			i2c_handle_smbus_alert(i2c_bus->ara);
+			writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base +

AST2600_I2CM_ISR);

Review

Quoted 2 lines

(quoted diff) +2 -0

+			dev_err(i2c_bus->dev,
+				"ast2600_master_alert_recv bus id %d, Disable Alt, Please

Imple\n",

drivers/i2c/busses/i2c-ast2600.c

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+				i2c_bus->adap.nr);
+			return 1;
+		}
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+	if (i2c_bus->cmd_err) {
+		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base +

AST2600_I2CM_ISR);

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_PKT_DONE & sts) {
+		ast2600_i2c_master_package_irq(i2c_bus, sts);
+		return 1;
+	}
+
+	return 0;
+}

...

drivers/i2c/busses/i2c-ast2600.c line 1317

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+		i2c_bus->multi_master = true;
+	else
+		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

	i2c_bus->multi_master = device_property_read_bool(&pdev->dev,
"multi-master");
	if (!i2c_bus->multi_master)
		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

...

drivers/i2c/busses/i2c-ast2600.c line 1428

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	struct device_node *np = pdev->dev.of_node;

It should use dev, but see below.

drivers/i2c/busses/i2c-ast2600.c line 1433

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	u32 global_ctrl;
 
+	int ret = 0;

Do you need this assignment?

...

drivers/i2c/busses/i2c-ast2600.c line 1468

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		i2c_bus->buf_base = devm_platform_ioremap_resource(pdev, 1);
 
+		if (!IS_ERR_OR_NULL(i2c_bus->buf_base))

Why not positive check?

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+			i2c_bus->buf_size = resource_size(res) / 2;
+		else
+			i2c_bus->mode = BYTE_MODE;
+	}

...

drivers/i2c/busses/i2c-ast2600.c line 1480

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+	ret = of_property_read_u32(dev->of_node,
+				   "i2c-scl-clk-low-timeout-us",
+				   &i2c_bus->timeout);

	device_property_read_u32()

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+	if (!ret)
+		i2c_bus->timeout /= 4096;

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	ret = device_property_read_u32(&pdev->dev, "clock-frequency",

&i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 1502

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	if (ret < 0) {
+		dev_warn(dev, "Could not read clock-frequency property\n");
+		i2c_bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
+	}

Use standard API from I2C core for this.

...

drivers/i2c/busses/i2c-ast2600.c line 1521

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	if (of_property_read_bool(dev->of_node, "smbus-alert")) {

device_property_read_bool()

Doesn't I2C core handle this property?

Review

Quoted 2 lines

(quoted diff) +2 -0

+		i2c_bus->alert_enable = true;
+		i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap,

&i2c_bus->alert_data);

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		if (!i2c_bus->ara)
+			dev_warn(dev, "Failed to register ARA client\n");
+
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER |

AST2600_I2CM_SMBUS_ALT,

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	} else {
+		i2c_bus->alert_enable = false;
+		/* Set interrupt generation of I2C master controller */
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	}

...

drivers/i2c/busses/i2c-ast2600.c line 1552

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);

Why explicit call?

...

drivers/i2c/busses/i2c-ast2600.c line 1558

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		dmam_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
+				   i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);

Ditto.

...

It looks to me like you ignored part of my comments. If so, I would like to know why.

--
With Best Regards,
Andy Shevchenko

Quoted 1 line · Ryan Chen

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c line 1039

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Wrong memory accessors. You should use something from asm/byteorder.h which includes linux/byteorder/generic.h.

Are you preferring add comment to explain more by following?
/*
* The controller's buffer register supports dword writes only.
* Therefore, write dwords to the buffer register in a 4-byte aligned,
* and write the remaining unaligned data at the end. */

				for (i = 0; i < xfer_len; i++) {
					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
					/* accumulating 4 bytes of data, write as a Dword to the buffer register */
					if (i % 4 == 3)
						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
				}
				/* less than 4 bytes of remaining data, write the remaining part as a Dword */
				if (--i % 4 != 3)
					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Or more columns (use get_unaligned_le32(wbuf); ) by following.

	for (i = 0; i < xfer_len; i++) {
		wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
		if (i % 4 == 3) {
			wbuf_dword = get_unaligned_le32(wbuf);
			writel(wbuf_dword, i2c_bus->buf_base + i - 3);
		}
	}

	if (--i % 4 != 3) {
		wbuf_dword = get_unaligned_le32(wbuf);
		writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4));
	}

-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM
To: Ryan Chen <[email protected]>
Cc: jk at codeconstruct.com.au; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; Wolfram Sang <[email protected]>; linux-i2c at vger.kernel.org; Florian Fainelli <[email protected]>; Jean Delvare <[email protected]>; William Zhang <[email protected]>; Tyrone Ting <[email protected]>; Tharun Kumar P <[email protected]>; Conor Dooley <[email protected]>; Phil Edworthy <[email protected]>; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; =linux-kernel at vger.kernel.org; Andi Shyti <[email protected]>
Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

Quoted 5 lines · Ryan Chen

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

...

+ bits.h

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>

drivers/i2c/busses/i2c-ast2600.c line 20

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+#include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
 
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>

You missed property.h
and these of*.h probably not needed at all, see below.

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>

...

Quoted 2 lines

+#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) &

GENMASK(7, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) &

GENMASK(5, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) &

GENMASK(4, 0)) + 1)

With right shifts it's better to have GENMASK to be applied first, it will show exact MSB of the bitfield.

(Ditto for other cases similar to these)

...

drivers/i2c/busses/i2c-ast2600.c

Quoted 13 lines

drivers/i2c/busses/i2c-ast2600.c new file +12 -0

+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+	unsigned long base_clk[4];
+	int baseclk_idx;
+	u32 clk_div_reg;
+	u32 scl_low;
+	u32 scl_high;
+	int divisor;
+	int inc = 0;
+	u32 data;
+
+	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL,

&clk_div_reg);

drivers/i2c/busses/i2c-ast2600.c line 304

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	for (int i = 0; i < 4; i++) {

See below.

drivers/i2c/busses/i2c-ast2600.c line 306

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		base_clk[i] = (i2c_bus->apb_clk * 10) /
+		(((((clk_div_reg >> (i * 8)) & GENMASK(7, 0)) + 2) * 10) / 2);

Second line is wrongly indented.

drivers/i2c/busses/i2c-ast2600.c

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	}
 
+	if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
+		baseclk_idx = 0;
+		divisor = DIV_ROUND_UP(i2c_bus->apb_clk,

i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 314

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+	} else {
 
+		int i;
+

Just add to the definition block:

	unsigned int i;

drivers/i2c/busses/i2c-ast2600.c

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		for (i = 0; i < 4; i++) {
+			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32) {
 
+				baseclk_idx = i + 1;
+				divisor = DIV_ROUND_UP(base_clk[i],

i2c_bus->bus_frequency);

These two can be moved outside of the loop

Review

Quoted 2 lines

(quoted diff) +2 -0

+				break;
+			}

			if ((base_clk[i] / i2c_bus->bus_frequency) <= 32)
				break;

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		}
+		if (i == 4) {
+			baseclk_idx = 4;
+			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 327

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+			while ((divisor + inc) > 32) {
+				inc |= divisor & 0x1;
+				divisor >>= 1;

	unsigned long divisor;

			for_each_set_bit(divisor, ...)

I.o.w. think about this, maybe you can refactor with the above.

drivers/i2c/busses/i2c-ast2600.c line 330

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+				baseclk_idx++;
+			}
+			divisor += inc;

		} else {
			...those two lines...

drivers/i2c/busses/i2c-ast2600.c line 337

Quoted 9 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+		}
 
+	}
+
+	divisor = min_t(int, divisor, 32);
+	baseclk_idx &= GENMASK(3, 0);
 
+	scl_low = ((divisor * 9) / 16) - 1;
+	scl_low = min_t(u32, scl_low, GENMASK(3, 0));

(with the divisor being unsigned long) this can be rewritten as

	scl_low = min(divisor * 9 / 16 - 1, GENMASK(3, 0));

which improves type checking and readability.

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+	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;
+}

...

drivers/i2c/busses/i2c-ast2600.c line 357

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+static u8 ast2600_i2c_recover_bus(struct ast2600_i2c_bus *i2c_bus) {
+	int ret = 0;
+	u32 ctrl;
+	u32 state;
+	int r;
 
+	dev_dbg(i2c_bus->dev, "%d-bus recovery bus [%x]\n", i2c_bus->adap.nr,
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Why you can't reuse "state" (assigned below)?
If not, then something like

	/* ...comment that state can be changed... */
	state = ...
	dev_dbg(state);

drivers/i2c/busses/i2c-ast2600.c

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+	ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(ctrl & ~(AST2600_I2CC_MASTER_EN | AST2600_I2CC_SLAVE_EN),
+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	writel(readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL) |
+AST2600_I2CC_MASTER_EN,

will it be different from ctrl value?

drivers/i2c/busses/i2c-ast2600.c

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+
+	reinit_completion(&i2c_bus->cmd_complete);
+	i2c_bus->cmd_err = 0;
+
+	/* Check 0x14's SDA and SCL status */
+	state = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
+	if (!(state & AST2600_I2CC_SDA_LINE_STS) && (state &

AST2600_I2CC_SCL_LINE_STS)) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base +

AST2600_I2CM_CMD_STS);

Review

Quoted 1 line

(quoted diff) +1 -0

+		r = wait_for_completion_timeout(&i2c_bus->cmd_complete,

i2c_bus->adap.timeout);

drivers/i2c/busses/i2c-ast2600.c line 379

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+		if (r == 0) {
+			dev_dbg(i2c_bus->dev, "recovery timed out\n");
+			ret = -ETIMEDOUT;
 
+		} else {
+			if (i2c_bus->cmd_err) {

		} else if (...) {

drivers/i2c/busses/i2c-ast2600.c line 387

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+				dev_dbg(i2c_bus->dev, "recovery error\n");
+				ret = -EPROTO;
+			}
+		}
+	}
+
+	dev_dbg(i2c_bus->dev, "Recovery done [%x]\n",
+		readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

As above.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
+AST2600_I2CC_BUS_BUSY_STS) {

Two sequential reads may give you different values?

drivers/i2c/busses/i2c-ast2600.c line 390

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		dev_dbg(i2c_bus->dev, "Can't recover bus [%x]\n",
+			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));

Again? With this inconsistency it will be "nice" to debug.

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+		ret = -EPROTO;
+	}
+
+	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+	return ret;
+}

...

Quoted 1 line

+#ifdef CONFIG_I2C_SLAVE

For (at least) review purposes I recommend to split slave out to the separate change. This driver is 16 hundreds LoCs long...

Quoted 1 line

+#endif

...

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+		} else if (i2c_bus->mode == BUFF_MODE) {
+			/* buff mode */
+			cmd |= AST2600_I2CM_RX_BUFF_EN;
 
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");

drivers/i2c/busses/i2c-ast2600.c line 838

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+				xfer_len = 1;
+			} else {
+				if (msg->len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					xfer_len = msg->len;
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}

This...

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+			writel(AST2600_I2CC_SET_RX_BUF_LEN(xfer_len),
+			       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+		} else {
 

drivers/i2c/busses/i2c-ast2600.c line 851

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+			/* byte mode */
+			xfer_len = 1;
+			if (msg->flags & I2C_M_RECV_LEN) {
+				dev_dbg(i2c_bus->dev, "smbus read\n");
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count) {
+					if (msg->len == 1)
+						cmd |= MASTER_TRIGGER_LAST_STOP;
+				}
+			}

...and this have a lot in common, can it be deduplicated?

Quoted 1 line

(quoted diff) +1 -0

+		}

...

drivers/i2c/busses/i2c-ast2600.c line 862

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+			if (msg->len > AST2600_I2C_DMA_SIZE) {
+				xfer_len = AST2600_I2C_DMA_SIZE;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}

See above.

...

drivers/i2c/busses/i2c-ast2600.c

Quoted 12 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+			u8 wbuf[4];
+			/* buff mode */
+			if (msg->len > i2c_bus->buf_size) {
+				xfer_len = i2c_bus->buf_size;
+			} else {
+				if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+					cmd |= AST2600_I2CM_STOP_CMD;
+				xfer_len = msg->len;
+			}
+			if (xfer_len) {
+				cmd |= AST2600_I2CM_TX_BUFF_EN |

AST2600_I2CM_TX_CMD;

drivers/i2c/busses/i2c-ast2600.c line 904

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
+				if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR))
+					return -ENOMEM;
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i];
+					if (i % 4 == 3)
 
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);

This is incorrect memory accessor.

drivers/i2c/busses/i2c-ast2600.c line 907

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));

Ditto.

Quoted 1 line

(quoted diff) +1 -0

+			}

...

Quoted 1 line

+static int ast2600_i2c_is_irq_error(u32 irq_status)

This function is not boolean, so "_is_" seems misleading.

This is basically error code conversion, something like

ast2600_i2c_irq_err_to_errno(u32 irq_status)

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+{
+	if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+		return -EAGAIN;
+	if (irq_status & (AST2600_I2CM_SDA_DL_TO |

AST2600_I2CM_SCL_LOW_TO))

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +6 -0

+		return -EBUSY;
+	if (irq_status & (AST2600_I2CM_ABNORMAL))
+		return -EPROTO;
+
+	return 0;
+}

...

Quoted 9 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+				u8 wbuf[4];
+
+				cmd |= AST2600_I2CM_TX_BUFF_EN;
+				xfer_len = msg->len - i2c_bus->master_xfer_cnt;
+				if (xfer_len > i2c_bus->buf_size) {
+					xfer_len = i2c_bus->buf_size;
+				} else {
+					if (i2c_bus->msgs_index + 1 == i2c_bus->msgs_count)
+						cmd |= AST2600_I2CM_STOP_CMD;

drivers/i2c/busses/i2c-ast2600.c line 1039

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+				}
+				for (i = 0; i < xfer_len; i++) {
+					wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];
+					if (i % 4 == 3)
+						writel(*(u32 *)wbuf, i2c_bus->buf_base + i - 3);
+				}
+				if (--i % 4 != 3)
+					writel(*(u32 *)wbuf, i2c_bus->buf_base + i - (i % 4));
+				writel(AST2600_I2CC_SET_TX_BUF_LEN(xfer_len),
+				       i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);

Wrong memory accessors. You should use something from asm/byteorder.h which includes linux/byteorder/generic.h.

...

Review

Quoted 3 lines

(quoted diff) +2 -0

+#ifdef CONFIG_I2C_SLAVE
+		/* Workaround for master/slave package mode enable rx done stuck

issue

Review

Quoted 1 line

(quoted diff) +1 -0

+		 * When master go for first read (RX_DONE), slave mode will also

effect

drivers/i2c/busses/i2c-ast2600.c line 1057

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		 * Then controller will send nack, not operate anymore.
+		 */

/*
* Wrong style of multi-line
* comments. You need to fix
* them in the entire driver.
*/

Review

Quoted 1 line

(quoted diff) +1 -0

+		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) &

AST2600_I2CS_PKT_MODE_EN) {

Review

Quoted 1 line

(quoted diff) +1 -0

+			u32 slave_cmd = readl(i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

drivers/i2c/busses/i2c-ast2600.c

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+
+			writel(0, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
+			writel(slave_cmd, i2c_bus->reg_base +

AST2600_I2CS_CMD_STS);

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+		}
+		fallthrough;
+#endif

...

drivers/i2c/busses/i2c-ast2600.c line 1155

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus) {
+	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER);
+	u32 ctrl = 0;

Redundant assignment.

drivers/i2c/busses/i2c-ast2600.c

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+	if (!i2c_bus->alert_enable)
+		sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+	if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base +

AST2600_I2CM_ISR);

drivers/i2c/busses/i2c-ast2600.c

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600.c new file +10 -0

+		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+		i2c_bus->cmd_err = -EPROTO;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_BUS_RECOVER & sts) {
+		writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base +

AST2600_I2CM_ISR);

drivers/i2c/busses/i2c-ast2600.c

Quoted 9 lines

drivers/i2c/busses/i2c-ast2600.c new file +9 -0

+		i2c_bus->cmd_err = 0;
+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_SMBUS_ALT & sts) {
+		if (ier & AST2600_I2CM_SMBUS_ALT) {
+			/* Disable ALT INT */
+			writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base +

AST2600_I2CM_IER);

Review

Quoted 2 lines

(quoted diff) +2 -0

+			i2c_handle_smbus_alert(i2c_bus->ara);
+			writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base +

AST2600_I2CM_ISR);

Review

Quoted 2 lines

(quoted diff) +2 -0

+			dev_err(i2c_bus->dev,
+				"ast2600_master_alert_recv bus id %d, Disable Alt, Please

Imple\n",

drivers/i2c/busses/i2c-ast2600.c

Quoted 8 lines

drivers/i2c/busses/i2c-ast2600.c new file +8 -0

+				i2c_bus->adap.nr);
+			return 1;
+		}
+	}
+
+	i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+	if (i2c_bus->cmd_err) {
+		writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base +

AST2600_I2CM_ISR);

Quoted 11 lines

drivers/i2c/busses/i2c-ast2600.c new file +11 -0

+		complete(&i2c_bus->cmd_complete);
+		return 1;
+	}
+
+	if (AST2600_I2CM_PKT_DONE & sts) {
+		ast2600_i2c_master_package_irq(i2c_bus, sts);
+		return 1;
+	}
+
+	return 0;
+}

...

drivers/i2c/busses/i2c-ast2600.c line 1317

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+		i2c_bus->multi_master = true;
+	else
+		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

	i2c_bus->multi_master = device_property_read_bool(&pdev->dev,
"multi-master");
	if (!i2c_bus->multi_master)
		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;

...

drivers/i2c/busses/i2c-ast2600.c line 1428

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	struct device_node *np = pdev->dev.of_node;

It should use dev, but see below.

drivers/i2c/busses/i2c-ast2600.c line 1433

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600.c new file +5 -0

+	struct device *dev = &pdev->dev;
+	struct ast2600_i2c_bus *i2c_bus;
+	struct resource *res;
+	u32 global_ctrl;
 
+	int ret = 0;

Do you need this assignment?

...

drivers/i2c/busses/i2c-ast2600.c line 1468

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		i2c_bus->buf_base = devm_platform_ioremap_resource(pdev, 1);
 
+		if (!IS_ERR_OR_NULL(i2c_bus->buf_base))

Why not positive check?

Quoted 5 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+			i2c_bus->buf_size = resource_size(res) / 2;
+		else
+			i2c_bus->mode = BYTE_MODE;
+	}

...

drivers/i2c/busses/i2c-ast2600.c line 1480

Quoted 3 lines

drivers/i2c/busses/i2c-ast2600.c new file +3 -0

+	ret = of_property_read_u32(dev->of_node,
+				   "i2c-scl-clk-low-timeout-us",
+				   &i2c_bus->timeout);

	device_property_read_u32()

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+	if (!ret)
+		i2c_bus->timeout /= 4096;

...

Review

Quoted 1 line

(quoted diff) +1 -0

+	ret = device_property_read_u32(&pdev->dev, "clock-frequency",

&i2c_bus->bus_frequency);

drivers/i2c/busses/i2c-ast2600.c line 1502

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+	if (ret < 0) {
+		dev_warn(dev, "Could not read clock-frequency property\n");
+		i2c_bus->bus_frequency = I2C_MAX_STANDARD_MODE_FREQ;
+	}

Use standard API from I2C core for this.

...

drivers/i2c/busses/i2c-ast2600.c line 1521

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	if (of_property_read_bool(dev->of_node, "smbus-alert")) {

device_property_read_bool()

Doesn't I2C core handle this property?

Review

Quoted 3 lines

(quoted diff) +2 -0

+		i2c_bus->alert_enable = true;
+		i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap,

&i2c_bus->alert_data);

drivers/i2c/busses/i2c-ast2600.c

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+		if (!i2c_bus->ara)
+			dev_warn(dev, "Failed to register ARA client\n");
+
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER |

AST2600_I2CM_SMBUS_ALT,

Quoted 7 lines

drivers/i2c/busses/i2c-ast2600.c new file +7 -0

+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	} else {
+		i2c_bus->alert_enable = false;
+		/* Set interrupt generation of I2C master controller */
+		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+		       i2c_bus->reg_base + AST2600_I2CM_IER);
+	}

...

drivers/i2c/busses/i2c-ast2600.c line 1552

Quoted 1 line

drivers/i2c/busses/i2c-ast2600.c new file +1 -0

+	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);

Why explicit call?

...

drivers/i2c/busses/i2c-ast2600.c line 1558

Quoted 2 lines

drivers/i2c/busses/i2c-ast2600.c new file +2 -0

+		dmam_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
+				   i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);

Ditto.

...

It looks to me like you ignored part of my comments. If so, I would like to know why.

--
With Best Regards,
Andy Shevchenko

Quoted 3 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM
On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

Quoted 9 lines

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) &

GENMASK(7, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) &

GENMASK(5, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) &

GENMASK(4, 0)) + 1)

Quoted 4 lines

With right shifts it's better to have GENMASK to be applied first, it will show exact MSB of the bitfield.

(Ditto for other cases similar to these)

It will update next version.
#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24)) >> 24)
#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8)) >> 8) + 1)

			divisor = DIV_ROUND_UP(base_clk[3], i2c_bus->timing_info.bus_freq_hz);
			for_each_set_bit(divisor, &divisor, 32) {

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

				divisor >>= 1;

				baseclk_idx++;

			}

		} else {
			baseclk_idx = i + 1;
			divisor = DIV_ROUND_UP(base_clk[i], i2c_bus->timing_info.bus_freq_hz);
		}
	}

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

Sorry I don't catch this split slave out to separate change? Do you mean go for another file name example ast2600_i2c_slave.c ?

static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)

{
	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];

	int ret;

	/* send start */
	dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
		i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD),
		msg->len, msg->len > 1 ? "s" : "",

		msg->flags & I2C_M_RD ? "from" : "to", msg->addr);

	i2c_bus->master_xfer_cnt = 0;
	i2c_bus->buf_index = 0;

	if (msg->flags & I2C_M_RD) {
		if (i2c_bus->mode == DMA_MODE)
			ret = ast2600_i2c_setup_dma_rx(i2c_bus);

		else if (i2c_bus->mode == BUFF_MODE)
			ret = ast2600_i2c_setup_buff_rx(i2c_bus);
		else
			ret = ast2600_i2c_setup_byte_rx(i2c_bus);

	} else {
		if (i2c_bus->mode == DMA_MODE)
			ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD, i2c_bus);
		else if (i2c_bus->mode == BUFF_MODE)
			ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD, i2c_bus);
		else
			ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD, i2c_bus);

	}

	return ret;
}

Quoted 2 lines

Wrong memory accessors. You should use something from asm/byteorder.h which includes linux/byteorder/generic.h.

Sorry, about these parts. I quit no idea.
This is chip register limited, it only support dword write, not support byte write. So the only way I have is use get_unaligned_le32 function get the byte buffer to align dword write. Or I may need your help point me a good way.

(quoted diff) +0 -0

 	for (i = 0; i < xfer_len; i++) {
 		wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];

-----Original Message-----
From: Andy Shevchenko <[email protected]>
Sent: Tuesday, September 5, 2023 7:55 PM
To: Ryan Chen <[email protected]>
Cc: jk at codeconstruct.com.au; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; Wolfram Sang <[email protected]>; linux-i2c at vger.kernel.org; Florian Fainelli <[email protected]>; Jean Delvare <[email protected]>; William Zhang <[email protected]>; Tyrone Ting <[email protected]>; Tharun Kumar P <[email protected]>; Conor Dooley <[email protected]>; Phil Edworthy <[email protected]>; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; =linux-kernel at vger.kernel.org; Andi Shyti <[email protected]>
Subject: Re: [PATCH v12 2/2] i2c: aspeed: support ast2600 i2c new register mode driver

Quoted 3 lines · Ryan Chen

Quoted 3 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM
On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

...

Quoted 16 lines

Quoted 6 lines

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUFF(x) (((x) >> 8) &

GENMASK(7, 0))

Quoted 1 line

+#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) >> 24) &

GENMASK(5, 0))

Quoted 7 lines

Quoted 1 line

+#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) >> 8) &

GENMASK(4, 0)) + 1)

With right shifts it's better to have GENMASK to be applied first, it will show exact MSB of the bitfield.

(Ditto for other cases similar to these)

It will update next version.
#define AST2600_I2CC_GET_RX_BUF_LEN(x) (((x) & GENMASK(29, 24))

Quoted 3 lines

Quoted 1 line

24)

#define AST2600_I2CC_GET_TX_BUF_LEN(x) ((((x) & GENMASK(12, 8))

Quoted 1 line

8) + 1)

Note, these were just an example, check _all_ of the similar cases.

In general any reviewer's comment should be considered for the entire code where it makes sense.

...

Quoted 1 line

			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->timing_info.bus_freq_hz);

Quoted 1 line

			for_each_set_bit(divisor, &divisor, 32) {

This is wrong.

Quoted 4 lines

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

				divisor >>= 1;

And this.

Quoted 3 lines

				baseclk_idx++;

			}

for_each_set_bit() should use two different variables.

Quoted 4 lines

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

i2c_bus->timing_info.bus_freq_hz);

Quoted 2 lines

		}
	}

...

Quoted 1 line

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

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

...

Quoted 2 lines

Sorry I don't catch this split slave out to separate change? Do you mean go for another file name example ast2600_i2c_slave.c ?

No, I mean

patch 1: Introduce the driver with only master support patch 2: Add slave
capability (all what is under ifdeffery for I2C_SLAVE)

...

Quoted 4 lines

static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) {
	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];

	int ret;

This is not needed, you may return directly.

Quoted 4 lines

	/* send start */
	dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
		i2c_bus->msgs_index, str_read_write(msg->flags & I2C_M_RD),
		msg->len, msg->len > 1 ? "s" : "",

Quoted 8 lines

		msg->flags & I2C_M_RD ? "from" : "to", msg->addr);

	i2c_bus->master_xfer_cnt = 0;
	i2c_bus->buf_index = 0;

	if (msg->flags & I2C_M_RD) {
		if (i2c_bus->mode == DMA_MODE)
			ret = ast2600_i2c_setup_dma_rx(i2c_bus);

			return ...;
		if ...

Quoted 8 lines

		else if (i2c_bus->mode == BUFF_MODE)
			ret = ast2600_i2c_setup_buff_rx(i2c_bus);
		else
			ret = ast2600_i2c_setup_byte_rx(i2c_bus);

	} else {
		if (i2c_bus->mode == DMA_MODE)
			ret = ast2600_i2c_setup_dma_tx(AST2600_I2CM_START_CMD,

i2c_bus);

Quoted 2 lines

		else if (i2c_bus->mode == BUFF_MODE)
			ret = ast2600_i2c_setup_buff_tx(AST2600_I2CM_START_CMD,

i2c_bus);

Quoted 2 lines

		else
			ret = ast2600_i2c_setup_byte_tx(AST2600_I2CM_START_CMD,

i2c_bus);

Same way.

Quoted 4 lines

	}

	return ret;
}

...

Quoted 5 lines

Quoted 2 lines

Wrong memory accessors. You should use something from asm/byteorder.h which includes linux/byteorder/generic.h.

Sorry, about these parts. I quit no idea.
This is chip register limited, it only support dword write, not support byte

write.

Quoted 1 line

So the only way I have is use get_unaligned_le32 function get the byte buffer

to align dword write.

Quoted 4 lines

Or I may need your help point me a good way.

(quoted diff) +0 -0

 	for (i = 0; i < xfer_len; i++) {
 		wbuf[i % 4] = msg->buf[i2c_bus->master_xfer_cnt + i];

Review

Quoted 10 lines

(quoted diff) +0 -0

 		if (i % 4 == 3) {
 			wbuf_dword = get_unaligned_le32(wbuf);
 			writel(wbuf_dword, i2c_bus->buf_base + i - 3);
 		}
 	}
 
 	if (--i % 4 != 3) {
 		wbuf_dword = get_unaligned_le32(wbuf);
 		writel(wbuf_dword, i2c_bus->buf_base + i - (i % 4));
 	}

Something like that. The most problematic part in your original code is dereferencing byte memory as 32-bit memory with all possible problems behind.
With this code it's gone. The code itself might be improved even more, you can think about it, you still have time (we are now in v6.7 cycle).

--
With Best Regards,
Andy Shevchenko

Quoted 6 lines

From: Andy Shevchenko <[email protected]>
Sent: Tuesday, September 5, 2023 7:55 PM

Quoted 3 lines · Ryan Chen

Quoted 3 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM
On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

Quoted 17 lines

Quoted 1 line

			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->timing_info.bus_freq_hz);

Quoted 1 line

			for_each_set_bit(divisor, &divisor, 32) {

This is wrong.

Quoted 4 lines

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

				divisor >>= 1;

And this.

Quoted 3 lines

				baseclk_idx++;

			}

Quoted 1 line

for_each_set_bit() should use two different variables.

Will update by following.

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

Quoted 6 lines

Quoted 3 lines

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

i2c_bus->timing_info.bus_freq_hz);

Quoted 2 lines

		}
	}

Quoted 4 lines

Quoted 1 line

	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);

Quoted 6 lines

Quoted 6 lines

From: Andy Shevchenko <[email protected]>
Sent: Tuesday, September 5, 2023 7:55 PM On Tue, Sep 05, 2023 at

Quoted 3 lines · Ryan Chen

Quoted 3 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

...

Quoted 21 lines

Quoted 11 lines

Quoted 1 line

			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->timing_info.bus_freq_hz);

Quoted 1 line

			for_each_set_bit(divisor, &divisor, 32) {

This is wrong.

Quoted 4 lines

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

				divisor >>= 1;

Quoted 7 lines

And this.

Quoted 3 lines

				baseclk_idx++;

			}

for_each_set_bit() should use two different variables.

Will update by following.

Quoted 4 lines

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 6 lines

Quoted 6 lines

Quoted 3 lines

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

i2c_bus->timing_info.bus_freq_hz);

Quoted 2 lines

		}
	}

...

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

	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

Quoted 32 lines · Ryan Chen

Quoted 6 lines

Quoted 6 lines

From: Andy Shevchenko <[email protected]>
Sent: Tuesday, September 5, 2023 7:55 PM On Tue, Sep 05, 2023 at

Quoted 3 lines · Ryan Chen

Quoted 3 lines

From: Andy Shevchenko <[email protected]>
Sent: Friday, July 14, 2023 4:55 PM On Fri, Jul 14, 2023 at 03:45:22PM +0800, Ryan Chen wrote:

...

Quoted 21 lines

Quoted 11 lines

Quoted 1 line

			divisor = DIV_ROUND_UP(base_clk[3],

i2c_bus->timing_info.bus_freq_hz);

Quoted 1 line

			for_each_set_bit(divisor, &divisor, 32) {

This is wrong.

Quoted 4 lines

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

				divisor >>= 1;

Quoted 7 lines

And this.

Quoted 3 lines

				baseclk_idx++;

			}

for_each_set_bit() should use two different variables.

Will update by following.

Quoted 15 lines

Quoted 4 lines

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 6 lines

Quoted 6 lines

Quoted 3 lines

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

i2c_bus->timing_info.bus_freq_hz);

Quoted 2 lines

		}
	}

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 16 lines

...

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

	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