[v10,0/2] Add ASPEED AST2600 I2Cv2 controller driver

This series add AST2600 i2cv2 new register set driver. The i2cv2 driver
is new register set that have new clock divider option for more
flexiable generation. And also have separate i2c master and slave register
set for control.

The legacy register layout is mix master/slave register control together.
The following is add more detail description about new register layout.
And new feature set add for register.

-Add new clock divider option for more flexible and accurate clock rate
generation -Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of each
device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating master and slave mode control.
-Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.

And following is new register set for package transfer sequence.
-New Master operation mode:
 S -> Aw -> P
 S -> Aw -> TxD -> P
 S -> Ar -> RxD -> P
 S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-Bus SDA lock auto-release capability for new master DMA command mode.
-Bus auto timeout for new master/slave DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register     
{I2CD04}: Clock and AC Timing Control Register
{I2CD08}: Clock and AC Timing Control Register
{I2CD0C}: Interrupt Control Register
{I2CD10}: Interrupt Status Register 
{I2CD14}: Command/Status Register   
{I2CD18}: Slave Device Address Register
{I2CD1C}: Pool Buffer Control Register
{I2CD20}: Transmit/Receive Byte Buffer Register
{I2CD24}: DMA Mode Buffer Address Register
{I2CD28}: DMA Transfer Length Register
{I2CD2C}: Original DMA Mode Buffer Address Setting
{I2CD30}: Original DMA Transfer Length Setting and Final Status

New Register mode
{I2CC00}: Master/Slave Function Control Register
{I2CC04}: Master/Slave Clock and AC Timing Control Register
{I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
{I2CC0C}: Master/Slave Pool Buffer Control Register
{I2CM10}: Master Interrupt Control Register
{I2CM14}: Master Interrupt Status Register
{I2CM18}: Master Command/Status Register
{I2CM1C}: Master DMA Buffer Length Register
{I2CS20}: Slave~ Interrupt Control Register
{I2CS24}: Slave~ Interrupt Status Register
{I2CS28}: Slave~ Command/Status Register
{I2CS2C}: Slave~ DMA Buffer Length Register
{I2CM30}: Master DMA Mode Tx Buffer Base Address
{I2CM34}: Master DMA Mode Rx Buffer Base Address
{I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
{I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
{I2CS40}: Slave Device Address Register
{I2CM48}: Master DMA Length Status Register
{I2CS4C}: Slave  DMA Length Status Register
{I2CC50}: Current DMA Operating Address Status
{I2CC54}: Current DMA Operating Length  Status

aspeed,global-regs:
This global register is needed, global register is setting for
new clock divide control, and new register set control.

ASPEED SOC chip is server product, i2c bus may have fingerprint
connect to another board. And also support hotplug.
The following is board-specific design example.
Board A                                         Board B
-------------------------                       ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

i2c-scl-clk-low-timeout-us:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx
transmit. So it need timeout setting to enable timeout unlock controller
state. And in another side. In Master side also need avoid suddenly
slave miss(un-plug), Master will timeout and release the SDA/SCL.

aspeed,enable-dma:
For example The bus#1 have trunk data needed for transfer,
it can enable bus dma mode transfer, it can reduce cpu utilized.
Others bus bus#2/3 use defautl buffer mode.

v10:
-aspeed,i2c.yaml
 -move unevaluatedProperties after allOf.
 -remove extra one blank line.
-i2c-ast2600.c
 -no change, the same with v8.

v9:
-aspeed,i2c.yaml
 -backoff to v7.
  -no fix typo in maintainer's name and email. this would be another patch.
  -no remove address-cells, size-cells, this would be another patch.
 -use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
 -fix allOf and else false properties for aspeed,ast2600-i2cv2.
-i2c-ast2600.c
 -no change, the same with v8

v8:
-aspeed,i2c.yaml
 -modify commit message.
  -Fix typo in maintainer's name and email.
 -remove address-cells, size-cells.
-i2c-ast2600.c
 -move "i2c timeout counter" comment description before property_read.
 -remove redundant code "return ret" in probe end.

v7:
-aspeed,i2c.yaml
 -Update ASPEED I2C maintainers email.
 -use aspeed,enable-dma property instead of aspeed,xfer-mode selection.
 -fix allOf and else false properties for aspeed,ast2600-i2cv2.
-i2c-ast2600.c
 -remove aspeed,xfer-mode instead of aspeed,enable-dma mode. buffer mode
is default.
 -remove aspeed,timeout instead of i2c-scl-clk-low-timeout-us for
timeout setting.

v6:
-remove aspeed,i2cv2.yaml, merge to aspeed,i2c.yaml -add support for
 i2cv2 properites.
-i2c-ast2600.c
 -fix ast2600_i2c_remove ordering.
 -remove ast2600_i2c_probe goto labels, and add dev_err_probe -remove
  redundant deb_dbg debug message.
 -rename gr_regmap -> global_regs

v5:
-remove ast2600-i2c-global.yaml, i2c-ast2600-global.c.
-i2c-ast2600.c
 -remove legacy clock divide, all go for new clock divide.
 -remove duplicated read isr.
 -remove no used driver match
 -fix probe return for each labels return.
 -global use mfd driver, driver use phandle to regmap read/write.
-rename aspeed,i2c-ast2600.yaml to aspeed,i2cv2.yaml -remove bus-frequency.
-add required aspeed,gr
-add timeout, byte-mode, buff-mode properites.

v4:
-fix i2c-ast2600.c driver buffer mode use single buffer conflit in
 master slave mode both enable.
-fix kmemleak issue when use dma mode.
-fix typo aspeed,i2c-ast2600.yaml compatible is "aspeed,ast2600-i2c"
-fix typo aspeed,i2c-ast2600.ymal to aspeed,i2c-ast2600.yaml

v3:
-fix i2c global clock divide default value.
-remove i2c slave no used dev_dbg info.

v2:
-add i2c global ymal file commit.
-rename file name from new to ast2600.
 aspeed-i2c-new-global.c -> i2c-ast2600-global.c
 aspeed-i2c-new-global.h -> i2c-ast2600-global.h
 i2c-new-aspeed.c -> i2c-ast2600.c
-rename all driver function name to ast2600.

Ryan Chen (2):
  dt-bindings: i2c: aspeed: support for AST2600-i2cv2
  i2c: aspeed: support ast2600 i2c new register mode driver

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   |   51 +-
 MAINTAINERS                                   |    9 +
 drivers/i2c/busses/Kconfig                    |   11 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-ast2600.c              | 1602 +++++++++++++++++
 5 files changed, 1671 insertions(+), 3 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

Le 15/04/2023 à 03:28, Ryan Chen a écrit :
> 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 <ryan_chen@aspeedtech.com>
> ---

[...]

> +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));
> +
> +	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,
> +	       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)) {
> +		writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> +		r = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +		if (r == 0) {
> +			dev_dbg(i2c_bus->dev, "recovery timed out\n");
> +			ret = -ETIMEDOUT;
> +		} else {
> +			if (i2c_bus->cmd_err) {
> +				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));
> +	if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS) {
> +		dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",

recover?

> +			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> +		ret = -EPROTO;
> +	}
> +
> +	writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_I2C_SLAVE
> +static void ast2600_i2c_slave_packet_dma_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	int slave_rx_len;
> +	u32 cmd = 0;
> +	u8 value;
> +	int i = 0;

No need to init

> +
> +	sts &= ~(AST2600_I2CS_SLAVE_PENDING);
> +	/* Handle i2c slave timeout condition */
> +	if (AST2600_I2CS_INACTIVE_TO & sts) {
> +		cmd = SLAVE_TRIGGER_CMD;
> +		cmd |= AST2600_I2CS_RX_DMA_EN;
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		return;
> +	}
> +
> +	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
> +
> +	switch (sts) {
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
> +						      AST2600_I2CS_DMA_LEN_STS));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
> +					&i2c_bus->slave_dma_buf[i]);
> +		}
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
> +		break;
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
> +		break;
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE_NAK |
> +			AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA |
> +			AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_RX_DONE_NAK | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +		if (sts & AST2600_I2CS_SLAVE_MATCH)
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +
> +		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
> +						      AST2600_I2CS_DMA_LEN_STS));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
> +					&i2c_bus->slave_dma_buf[i]);
> +		}
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		if (sts & AST2600_I2CS_STOP)
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
> +		break;
> +
> +	/* it is Mw data Mr coming -> it need send tx */
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
> +		/* it should be repeat start read */
> +		if (sts & AST2600_I2CS_SLAVE_MATCH)
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +
> +		slave_rx_len = AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
> +						      AST2600_I2CS_DMA_LEN_STS));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
> +					&i2c_bus->slave_dma_buf[i]);
> +		}
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
> +				&i2c_bus->slave_dma_buf[0]);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
> +		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
> +				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
> +		break;
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA:
> +		/* First Start read */
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
> +				&i2c_bus->slave_dma_buf[0]);
> +		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
> +				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
> +		break;
> +	case AST2600_I2CS_WAIT_TX_DMA:
> +		/* it should be next start read */
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED,
> +				&i2c_bus->slave_dma_buf[0]);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
> +		writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
> +				i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
> +		break;

Having a new line or not after breaks is not consistent.

> +
> +	case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
> +		/* it just tx complete */
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
> +		break;
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +		cmd = 0;
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		break;
> +	case AST2600_I2CS_STOP:
> +		cmd = 0;
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		break;
> +	default:
> +		dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts %x\n", sts,
> +			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> +		break;
> +	}
> +
> +	if (cmd)
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +}
> +
> +static void ast2600_i2c_slave_packet_buff_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	int slave_rx_len = 0;
> +	u32 cmd = 0;
> +	u8 value;
> +	int i = 0;

No need to init;

> +
> +	/* due to master slave is common buffer, so need force the master stop not issue */
> +	if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) {
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
> +		i2c_bus->cmd_err = -EBUSY;
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		complete(&i2c_bus->cmd_complete);
> +	}
> +
> +	/* Handle i2c slave timeout condition */
> +	if (AST2600_I2CS_INACTIVE_TO & sts) {
> +		writel(SLAVE_TRIGGER_CMD, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		i2c_bus->slave_operate = 0;
> +		return;
> +	}
> +
> +	sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
> +
> +	if (sts & AST2600_I2CS_SLAVE_MATCH)
> +		i2c_bus->slave_operate = 1;
> +
> +	switch (sts) {
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		fallthrough;
> +	case AST2600_I2CS_SLAVE_PENDING |
> +		 AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		cmd = SLAVE_TRIGGER_CMD;
> +		if (sts & AST2600_I2CS_RX_DONE) {
> +			slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +							       AST2600_I2CC_BUFF_CTRL));
> +			for (i = 0; i < slave_rx_len; i++) {
> +				value = readb(i2c_bus->buf_base + 0x10 + i);
> +				i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +			}
> +		}
> +		if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & AST2600_I2CS_RX_BUFF_EN)
> +			cmd = 0;
> +		else
> +			cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
> +
> +		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
> +					i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		break;
> +	case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_RX_DONE:
> +		cmd = SLAVE_TRIGGER_CMD;
> +		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +						       AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			value = readb(i2c_bus->buf_base + 0x10 + i);
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}
> +		cmd |= AST2600_I2CS_RX_BUFF_EN;
> +		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
> +						i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		break;
> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
> +				AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +		cmd = SLAVE_TRIGGER_CMD;
> +		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +						       AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			value = readb(i2c_bus->buf_base + 0x10 + i);
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		cmd |= AST2600_I2CS_RX_BUFF_EN;
> +		writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
> +						i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		break;
> +

Having a new line or not after breaks is not consistent.

> +	case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +		cmd = SLAVE_TRIGGER_CMD;
> +		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +							   AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			value = readb(i2c_bus->buf_base + 0x10 + i);
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}
> +		/* workaround for avoid next start with len != 0 */
> +		writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		break;
> +
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
> +		cmd = SLAVE_TRIGGER_CMD;
> +		slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +							   AST2600_I2CC_BUFF_CTRL));
> +		for (i = 0; i < slave_rx_len; i++) {
> +			value = readb(i2c_bus->buf_base + 0x10 + i);
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +		}
> +		/* workaround for avoid next start with len != 0 */
> +		writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		break;
> +	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_SLAVE_MATCH:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +		writeb(value, i2c_bus->buf_base);
> +		writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
> +				i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
> +		break;
> +	case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
> +	case AST2600_I2CS_WAIT_TX_DMA:
> +		if (sts & AST2600_I2CS_RX_DONE) {
> +			slave_rx_len = AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
> +							AST2600_I2CC_BUFF_CTRL));
> +			for (i = 0; i < slave_rx_len; i++) {
> +				value = readb(i2c_bus->buf_base + 0x10 + i);
> +				i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &value);
> +			}
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +		} else {
> +			i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> +		}
> +		writeb(value, i2c_bus->buf_base);
> +		writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
> +				i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
> +		break;
> +	/* workaround : trigger the cmd twice to fix next state keep 1000000 */
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +		break;
> +
> +	case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
> +	case AST2600_I2CS_STOP:
> +		cmd = SLAVE_TRIGGER_CMD;
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		break;
> +	default:
> +		dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts %x\n", sts,
> +			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> +		break;
> +	}
> +
> +	if (cmd)
> +		writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +
> +	if ((sts & AST2600_I2CS_STOP) && !(sts & AST2600_I2CS_SLAVE_PENDING))
> +		i2c_bus->slave_operate = 0;
> +

Nit: unneeded empty line.

> +}
> +
> +static void ast2600_i2c_slave_byte_irq(struct ast2600_i2c_bus *i2c_bus, u32 sts)
> +{
> +	u32 i2c_buff = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> +	u32 cmd = AST2600_I2CS_ACTIVE_ALL;
> +	u8 byte_data;
> +	u8 value;
> +
> +	switch (sts) {
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
> +		/* first address match is address */
> +		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
> +		break;
> +	case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
> +		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, &byte_data);
> +		break;
> +	case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
> +		cmd |= AST2600_I2CS_TX_CMD;
> +		byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, &byte_data);
> +		writel(byte_data, i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> +		break;
> +	case AST2600_I2CS_TX_ACK | AST2600_I2CS_WAIT_TX_DMA:
> +		cmd |= AST2600_I2CS_TX_CMD;
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED, &byte_data);
> +		writel(byte_data, i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> +		break;
> +	case AST2600_I2CS_STOP:
> +	case AST2600_I2CS_STOP | AST2600_I2CS_TX_NAK:
> +		i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
> +		break;
> +	default:
> +		dev_dbg(i2c_bus->dev, "unhandled pkt isr %x\n", sts);
> +		break;
> +	}
> +	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	writel(sts, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +	readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +}
> +
> +static int ast2600_i2c_slave_irq(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	u32 ier = readl(i2c_bus->reg_base + AST2600_I2CS_IER);
> +	u32 isr = readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
> +
> +	if (!(isr & ier))
> +		return 0;
> +
> +	/* Slave interrupt coming after Master package done
> +	 * So need handle master first.
> +	 */
> +	if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR) & AST2600_I2CM_PKT_DONE)
> +		return 0;
> +
> +	isr &= ~(AST2600_I2CS_ADDR_INDICATE_MASK);
> +
> +	if (AST2600_I2CS_ADDR1_NAK & isr)
> +		isr &= ~AST2600_I2CS_ADDR1_NAK;
> +
> +	if (AST2600_I2CS_ADDR2_NAK & isr)
> +		isr &= ~AST2600_I2CS_ADDR2_NAK;
> +
> +	if (AST2600_I2CS_ADDR3_NAK & isr)
> +		isr &= ~AST2600_I2CS_ADDR3_NAK;
> +
> +	if (AST2600_I2CS_ADDR_MASK & isr)
> +		isr &= ~AST2600_I2CS_ADDR_MASK;
> +
> +	if (AST2600_I2CS_PKT_DONE & isr) {
> +		if (i2c_bus->mode == DMA_MODE)
> +			ast2600_i2c_slave_packet_dma_irq(i2c_bus, isr);
> +		else
> +			ast2600_i2c_slave_packet_buff_irq(i2c_bus, isr);
> +	} else
> +		ast2600_i2c_slave_byte_irq(i2c_bus, isr);
> +
> +	return 1;
> +}
> +#endif
> +
> +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
> +	int xfer_len = 0;
> +	int i = 0;

No need to init.

> +	u32 cmd;
> +
> +	cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | AST2600_I2CM_START_CMD;
> +
> +	/* send start */
> +	dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
> +		i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
> +		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;

[...]

> +static int ast2600_i2c_master_xfer(struct i2c_adapter *adap,
> +				      struct i2c_msg *msgs, int num)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap);
> +	unsigned long timeout;
> +	int ret = 0;

Looks like this initialization is unneeded.

> +
> +	/* If bus is busy in a single master environment, attempt recovery. */
> +	if (!i2c_bus->multi_master &&
> +	    (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & AST2600_I2CC_BUS_BUSY_STS)) {
> +		int ret;

No need for an extra 'ret'.

> +
> +		ret = ast2600_i2c_recover_bus(i2c_bus);
> +		if (ret)
> +			return ret;
> +	}
> +
> +#ifdef CONFIG_I2C_SLAVE
> +	if (i2c_bus->mode == BUFF_MODE) {
> +		if (i2c_bus->slave_operate)
> +			return -EBUSY;
> +		/* disable slave isr */
> +		writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
> +		if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) || (i2c_bus->slave_operate)) {
> +			writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +			return -EBUSY;
> +		}
> +	}
> +#endif
> +
> +	i2c_bus->cmd_err = 0;
> +	i2c_bus->msgs = msgs;
> +	i2c_bus->msgs_index = 0;
> +	i2c_bus->msgs_count = num;
> +	reinit_completion(&i2c_bus->cmd_complete);
> +	ret = ast2600_i2c_do_start(i2c_bus);
> +#ifdef CONFIG_I2C_SLAVE
> +	/* avoid race condication slave is wait and master wait 1st slave operate */
> +	if (i2c_bus->mode == BUFF_MODE)
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +#endif
> +	if (ret)
> +		goto master_out;
> +	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +	if (timeout == 0) {
> +		u32 isr = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		u32 i2c_status = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
> +
> +		dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n", isr, i2c_status);
> +		if (isr || (i2c_status & AST2600_I2CC_TX_DIR_MASK)) {
> +			u32 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);
> +#ifdef CONFIG_I2C_SLAVE
> +			if (ctrl & AST2600_I2CC_SLAVE_EN) {
> +				u32 cmd = SLAVE_TRIGGER_CMD;
> +
> +				if (i2c_bus->mode == DMA_MODE) {
> +					cmd |= AST2600_I2CS_RX_DMA_EN;
> +					writel(i2c_bus->slave_dma_addr,
> +						   i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
> +					writel(i2c_bus->slave_dma_addr,
> +						   i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
> +					writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +						   i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +				} else if (i2c_bus->mode == BUFF_MODE) {
> +					cmd = SLAVE_TRIGGER_CMD;
> +				} else {
> +					cmd &= ~AST2600_I2CS_PKT_MODE_EN;
> +				}
> +				writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +			}
> +#endif
> +		}
> +		ret = -ETIMEDOUT;
> +	} else {
> +		ret = i2c_bus->cmd_err;
> +	}
> +
> +	dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr, i2c_bus->cmd_err);
> +
> +master_out:
> +	if (i2c_bus->mode == DMA_MODE) {
> +		kfree(i2c_bus->master_safe_buf);
> +	    i2c_bus->master_safe_buf = NULL;

Alignement.

> +	}
> +
> +	return ret;
> +}
> +
> +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	struct platform_device *pdev = to_platform_device(i2c_bus->dev);
> +	u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE | AST2600_I2CC_MASTER_EN;
> +
> +	/* I2C Reset */
> +	writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> +	if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
> +		i2c_bus->multi_master = true;
> +	else
> +		fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
> +
> +	/* Enable Master Mode */
> +	writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +	/* disable slave address */
> +	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +
> +	/* Set AC Timing */
> +	writel(ast2600_select_i2c_clock(i2c_bus), i2c_bus->reg_base + AST2600_I2CC_AC_TIMING);
> +
> +	/* Clear Interrupt */
> +	writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR);
> +
> +#ifdef CONFIG_I2C_SLAVE
> +	/* for memory buffer initial */
> +	if (i2c_bus->mode == DMA_MODE) {
> +		i2c_bus->slave_dma_buf = dma_alloc_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
> +							    &i2c_bus->slave_dma_addr, GFP_KERNEL);

dmam_alloc_coherent() could maybe be used here.
This would simplify the remove function and avoid some conditional 
compilation.
This also fixes a leak if i2c_add_adapter() fails in the probe.

It is also likely that doing so, the devm_free_irq() in the remove 
function also becomes redundant and can be removed as well.

> +		if (!i2c_bus->slave_dma_buf)
> +			return;
> +	}
> +
> +	writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR);
> +
> +	if (i2c_bus->mode == BYTE_MODE) {
> +		writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER);
> +	} else {
> +		/* Set interrupt generation of I2C slave controller */
> +		writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_IER);
> +	}
> +#endif
> +}
> +
> +#ifdef CONFIG_I2C_SLAVE
> +static int ast2600_i2c_reg_slave(struct i2c_client *client)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
> +	u32 cmd = SLAVE_TRIGGER_CMD;
> +
> +	if (i2c_bus->slave)
> +		return -EINVAL;
> +
> +	dev_dbg(i2c_bus->dev, "slave addr %x\n", client->addr);
> +
> +	writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +	writel(AST2600_I2CC_SLAVE_EN | readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL),
> +	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> +	/* trigger rx buffer */
> +	if (i2c_bus->mode == DMA_MODE) {
> +		cmd |= AST2600_I2CS_RX_DMA_EN;
> +		writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
> +		writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
> +		writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
> +		       i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
> +	} else if (i2c_bus->mode == BUFF_MODE) {
> +		cmd = SLAVE_TRIGGER_CMD;
> +	} else {
> +		cmd &= ~AST2600_I2CS_PKT_MODE_EN;
> +	}
> +
> +	writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
> +	i2c_bus->slave = client;
> +	/* Set slave addr. */
> +	writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
> +			i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);

Nit: alignement

> +
> +	return 0;
> +}
> +
> +static int ast2600_i2c_unreg_slave(struct i2c_client *slave)
> +{
> +	struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(slave->adapter);
> +
> +	WARN_ON(!i2c_bus->slave);
> +
> +	/* Turn off slave mode. */
> +	writel(~AST2600_I2CC_SLAVE_EN & readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL),
> +	       i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +	writel(readl(i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL) & ~AST2600_I2CS_ADDR1_MASK,
> +	       i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> +
> +	i2c_bus->slave = NULL;
> +
> +	return 0;
> +}
> +#endif
> +
> +static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SMBUS_BLOCK_DATA;
> +}
> +
> +static const struct i2c_algorithm i2c_ast2600_algorithm = {
> +	.master_xfer = ast2600_i2c_master_xfer,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	.reg_slave = ast2600_i2c_reg_slave,
> +	.unreg_slave = ast2600_i2c_unreg_slave,
> +#endif
> +	.functionality = ast2600_i2c_functionality,
> +};
> +
> +static const struct of_device_id ast2600_i2c_bus_of_table[] = {
> +	{
> +		.compatible = "aspeed,ast2600-i2cv2",
> +	},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table);
> +
> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct ast2600_i2c_bus *i2c_bus;
> +	struct resource *res;
> +	u32 global_ctrl;
> +	int ret = 0;
> +
> +	i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
> +	if (!i2c_bus)
> +		return -ENOMEM;
> +
> +	i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(i2c_bus->reg_base))
> +		return PTR_ERR(i2c_bus->reg_base);
> +
> +	i2c_bus->rst = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(i2c_bus->rst))
> +		return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing reset ctrl\n");
> +
> +	reset_control_deassert(i2c_bus->rst);
> +
> +	i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(np, "aspeed,global-regs");
> +	if (IS_ERR(i2c_bus->global_regs))
> +		return PTR_ERR(i2c_bus->global_regs);
> +
> +	regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
> +	if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
> +		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, AST2600_GLOBAL_INIT);
> +		regmap_write(i2c_bus->global_regs, AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
> +	}
> +
> +	i2c_bus->slave_operate = 0;
> +	i2c_bus->dev = dev;
> +	i2c_bus->mode = BUFF_MODE;
> +
> +	if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma"))
> +		i2c_bus->mode = DMA_MODE;
> +
> +	if (i2c_bus->mode == BUFF_MODE) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (res && resource_size(res) >= 2) {
> +			i2c_bus->buf_base = devm_ioremap_resource(dev, res);
> +
> +			if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
> +				i2c_bus->buf_size = resource_size(res)/2;
> +		} else
> +			i2c_bus->mode = BYTE_MODE;
> +	}
> +
> +	/* i2c timeout counter: use base clk4 1Mhz,
> +	 * per unit: 1/(1000/4096) = 4096us
> +	 */
> +	ret = of_property_read_u32(dev->of_node,
> +							"i2c-scl-clk-low-timeout-us",
> +							&i2c_bus->timeout);

Strange layout.

> +	if (ret < 0)
> +		i2c_bus->timeout = 0;

Nit: This is not really needed since i2c_bus is kzalloc()'ed.

> +	else
> +		i2c_bus->timeout /= 4096;
> +
> +	init_completion(&i2c_bus->cmd_complete);
> +
> +	i2c_bus->irq = platform_get_irq(pdev, 0);
> +	if (i2c_bus->irq < 0)
> +		return i2c_bus->irq;
> +
> +	platform_set_drvdata(pdev, i2c_bus);
> +
> +	i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
> +	if (IS_ERR(i2c_bus->clk))
> +		return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk), "Can't get clock\n");
> +
> +	i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
> +
> +	ret = of_property_read_u32(dev->of_node, "clock-frequency", &i2c_bus->bus_frequency);
> +	if (ret < 0) {
> +		dev_warn(dev, "Could not read clock-frequency property\n");
> +		i2c_bus->bus_frequency = 100000;
> +	}
> +
> +	/* Initialize the I2C adapter */
> +	i2c_bus->adap.owner = THIS_MODULE;
> +	i2c_bus->adap.algo = &i2c_ast2600_algorithm;
> +	i2c_bus->adap.retries = 0;
> +	i2c_bus->adap.dev.parent = i2c_bus->dev;
> +	i2c_bus->adap.dev.of_node = dev->of_node;
> +	i2c_bus->adap.algo_data = i2c_bus;
> +	strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name));
> +	i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
> +
> +	ast2600_i2c_init(i2c_bus);
> +
> +	ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> +			       dev_name(dev), i2c_bus);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Unable to request irq %d\n", i2c_bus->irq);
> +
> +	if (of_property_read_bool(dev->of_node, "smbus-alert")) {
> +		i2c_bus->alert_enable = 1;
> +		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 = 0;
> +		/* Set interrupt generation of I2C master controller */
> +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +				i2c_bus->reg_base + AST2600_I2CM_IER);
> +	}
> +
> +	ret = i2c_add_adapter(&i2c_bus->adap);

Maybe devm_i2c_add_adapter() + remove function simplification.
This change + dmam_alloc_coherent() above + removing devm_free_irq() 
should, all together, not chnage the order of the remove function IIUIC.

CJ

> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n",
> +		 dev->of_node->name, i2c_bus->adap.nr, i2c_bus->bus_frequency / 1000,
> +		 i2c_bus->mode);
> +
> +	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);
> +	devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
> +
> +#ifdef CONFIG_I2C_SLAVE
> +	/* for memory buffer initial */
> +	if (i2c_bus->mode == DMA_MODE)
> +		dma_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
> +				i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);
> +#endif
> +
> +	return 0;
> +}
> +
> +static struct platform_driver ast2600_i2c_bus_driver = {
> +	.probe = ast2600_i2c_probe,
> +	.remove = ast2600_i2c_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = ast2600_i2c_bus_of_table,
> +	},
> +};
> +module_platform_driver(ast2600_i2c_bus_driver);
> +
> +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
> +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
> +MODULE_LICENSE("GPL");

(resending, my mail client removed some addresses. Sorry for the 
duplicated message for the others)


Le 15/04/2023 à 03:28, Ryan Chen a écrit :
 > 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 <ryan_chen@aspeedtech.com>
 > ---

[...]

 > +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));
 > +
 > +    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,
 > +           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)) {
 > +        writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base + 
AST2600_I2CM_CMD_STS);
 > +        r = wait_for_completion_timeout(&i2c_bus->cmd_complete, 
i2c_bus->adap.timeout);
 > +        if (r == 0) {
 > +            dev_dbg(i2c_bus->dev, "recovery timed out\n");
 > +            ret = -ETIMEDOUT;
 > +        } else {
 > +            if (i2c_bus->cmd_err) {
 > +                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));
 > +    if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & 
AST2600_I2CC_BUS_BUSY_STS) {
 > +        dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",

recover?

 > +            readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
 > +        ret = -EPROTO;
 > +    }
 > +
 > +    writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > +    return ret;
 > +}
 > +
 > +#ifdef CONFIG_I2C_SLAVE
 > +static void ast2600_i2c_slave_packet_dma_irq(struct ast2600_i2c_bus 
*i2c_bus, u32 sts)
 > +{
 > +    int slave_rx_len;
 > +    u32 cmd = 0;
 > +    u8 value;
 > +    int i = 0;

No need to init

 > +
 > +    sts &= ~(AST2600_I2CS_SLAVE_PENDING);
 > +    /* Handle i2c slave timeout condition */
 > +    if (AST2600_I2CS_INACTIVE_TO & sts) {
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        cmd |= AST2600_I2CS_RX_DMA_EN;
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + 
AST2600_I2CS_ISR);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        return;
 > +    }
 > +
 > +    sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
 > +
 > +    switch (sts) {
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_WAIT_RX_DMA:
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, 
&value);
 > +        slave_rx_len = 
AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
 > +                              AST2600_I2CS_DMA_LEN_STS));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
 > +                    &i2c_bus->slave_dma_buf[i]);
 > +        }
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
 > +        break;
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
 > +        break;
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE_NAK |
 > +            AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA |
 > +            AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_RX_DONE_NAK | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA | 
AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_STOP:
 > +        if (sts & AST2600_I2CS_SLAVE_MATCH)
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_REQUESTED, &value);
 > +
 > +        slave_rx_len = 
AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
 > +                              AST2600_I2CS_DMA_LEN_STS));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
 > +                    &i2c_bus->slave_dma_buf[i]);
 > +        }
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        if (sts & AST2600_I2CS_STOP)
 > +            i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
 > +        break;
 > +
 > +    /* it is Mw data Mr coming -> it need send tx */
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_WAIT_TX_DMA:
 > +        /* it should be repeat start read */
 > +        if (sts & AST2600_I2CS_SLAVE_MATCH)
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_REQUESTED, &value);
 > +
 > +        slave_rx_len = 
AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
 > +                              AST2600_I2CS_DMA_LEN_STS));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
 > +                    &i2c_bus->slave_dma_buf[i]);
 > +        }
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
 > +                &i2c_bus->slave_dma_buf[0]);
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
 > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
 > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
 > +        break;
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA:
 > +        /* First Start read */
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
 > +                &i2c_bus->slave_dma_buf[0]);
 > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
 > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
 > +        break;
 > +    case AST2600_I2CS_WAIT_TX_DMA:
 > +        /* it should be next start read */
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED,
 > +                &i2c_bus->slave_dma_buf[0]);
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
 > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
 > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
 > +        break;

Having a new line or not after breaks is not consistent.

 > +
 > +    case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
 > +        /* it just tx complete */
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
 > +        break;
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
 > +        cmd = 0;
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, 
&value);
 > +        break;
 > +    case AST2600_I2CS_STOP:
 > +        cmd = 0;
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        break;
 > +    default:
 > +        dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts 
%x\n", sts,
 > +            readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
 > +        break;
 > +    }
 > +
 > +    if (cmd)
 > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +    writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +}
 > +
 > +static void ast2600_i2c_slave_packet_buff_irq(struct ast2600_i2c_bus 
*i2c_bus, u32 sts)
 > +{
 > +    int slave_rx_len = 0;
 > +    u32 cmd = 0;
 > +    u8 value;
 > +    int i = 0;

No need to init;

 > +
 > +    /* due to master slave is common buffer, so need force the 
master stop not issue */
 > +    if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) {
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
 > +        i2c_bus->cmd_err = -EBUSY;
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        complete(&i2c_bus->cmd_complete);
 > +    }
 > +
 > +    /* Handle i2c slave timeout condition */
 > +    if (AST2600_I2CS_INACTIVE_TO & sts) {
 > +        writel(SLAVE_TRIGGER_CMD, i2c_bus->reg_base + 
AST2600_I2CS_CMD_STS);
 > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + 
AST2600_I2CS_ISR);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        i2c_bus->slave_operate = 0;
 > +        return;
 > +    }
 > +
 > +    sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
 > +
 > +    if (sts & AST2600_I2CS_SLAVE_MATCH)
 > +        i2c_bus->slave_operate = 1;
 > +
 > +    switch (sts) {
 > +    case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
 > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_SLAVE_PENDING |
 > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_SLAVE_PENDING |
 > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        fallthrough;
 > +    case AST2600_I2CS_SLAVE_PENDING |
 > +         AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | 
AST2600_I2CS_RX_DONE:
 > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH | 
AST2600_I2CS_RX_DONE:
 > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, 
&value);
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        if (sts & AST2600_I2CS_RX_DONE) {
 > +            slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                                   AST2600_I2CC_BUFF_CTRL));
 > +            for (i = 0; i < slave_rx_len; i++) {
 > +                value = readb(i2c_bus->buf_base + 0x10 + i);
 > +                i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +            }
 > +        }
 > +        if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) & 
AST2600_I2CS_RX_BUFF_EN)
 > +            cmd = 0;
 > +        else
 > +            cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
 > +
 > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
 > +                    i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        break;
 > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_RX_DONE:
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                               AST2600_I2CC_BUFF_CTRL));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            value = readb(i2c_bus->buf_base + 0x10 + i);
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +        }
 > +        cmd |= AST2600_I2CS_RX_BUFF_EN;
 > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
 > +                        i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        break;
 > +    case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_WAIT_RX_DMA |
 > +                AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                               AST2600_I2CC_BUFF_CTRL));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            value = readb(i2c_bus->buf_base + 0x10 + i);
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +        }
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        cmd |= AST2600_I2CS_RX_BUFF_EN;
 > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
 > +                        i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        break;
 > +

Having a new line or not after breaks is not consistent.

 > +    case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_STOP:
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                               AST2600_I2CC_BUFF_CTRL));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            value = readb(i2c_bus->buf_base + 0x10 + i);
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +        }
 > +        /* workaround for avoid next start with len != 0 */
 > +        writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        break;
 > +
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                               AST2600_I2CC_BUFF_CTRL));
 > +        for (i = 0; i < slave_rx_len; i++) {
 > +            value = readb(i2c_bus->buf_base + 0x10 + i);
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +        }
 > +        /* workaround for avoid next start with len != 0 */
 > +        writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        break;
 > +    case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_SLAVE_MATCH:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, 
&value);
 > +        writeb(value, i2c_bus->buf_base);
 > +        writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
 > +                i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
 > +        break;
 > +    case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
 > +    case AST2600_I2CS_WAIT_TX_DMA:
 > +        if (sts & AST2600_I2CS_RX_DONE) {
 > +            slave_rx_len = 
AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
 > +                            AST2600_I2CC_BUFF_CTRL));
 > +            for (i = 0; i < slave_rx_len; i++) {
 > +                value = readb(i2c_bus->buf_base + 0x10 + i);
 > +                i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_WRITE_RECEIVED, &value);
 > +            }
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_READ_REQUESTED, &value);
 > +        } else {
 > +            i2c_slave_event(i2c_bus->slave, 
I2C_SLAVE_READ_PROCESSED, &value);
 > +        }
 > +        writeb(value, i2c_bus->buf_base);
 > +        writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
 > +                i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
 > +        break;
 > +    /* workaround : trigger the cmd twice to fix next state keep 
1000000 */
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, 
&value);
 > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
 > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +        break;
 > +
 > +    case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_STOP:
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        break;
 > +    default:
 > +        dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts 
%x\n", sts,
 > +            readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
 > +        break;
 > +    }
 > +
 > +    if (cmd)
 > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +    writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +
 > +    if ((sts & AST2600_I2CS_STOP) && !(sts & 
AST2600_I2CS_SLAVE_PENDING))
 > +        i2c_bus->slave_operate = 0;
 > +

Nit: unneeded empty line.

 > +}
 > +
 > +static void ast2600_i2c_slave_byte_irq(struct ast2600_i2c_bus 
*i2c_bus, u32 sts)
 > +{
 > +    u32 i2c_buff = readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF);
 > +    u32 cmd = AST2600_I2CS_ACTIVE_ALL;
 > +    u8 byte_data;
 > +    u8 value;
 > +
 > +    switch (sts) {
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_WAIT_RX_DMA:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED, 
&value);
 > +        /* first address match is address */
 > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
 > +        break;
 > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
 > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED, 
&byte_data);
 > +        break;
 > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE | 
AST2600_I2CS_WAIT_TX_DMA:
 > +        cmd |= AST2600_I2CS_TX_CMD;
 > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED, 
&byte_data);
 > +        writel(byte_data, i2c_bus->reg_base + 
AST2600_I2CC_STS_AND_BUFF);
 > +        break;
 > +    case AST2600_I2CS_TX_ACK | AST2600_I2CS_WAIT_TX_DMA:
 > +        cmd |= AST2600_I2CS_TX_CMD;
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED, 
&byte_data);
 > +        writel(byte_data, i2c_bus->reg_base + 
AST2600_I2CC_STS_AND_BUFF);
 > +        break;
 > +    case AST2600_I2CS_STOP:
 > +    case AST2600_I2CS_STOP | AST2600_I2CS_TX_NAK:
 > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
 > +        break;
 > +    default:
 > +        dev_dbg(i2c_bus->dev, "unhandled pkt isr %x\n", sts);
 > +        break;
 > +    }
 > +    writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +    writel(sts, i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +}
 > +
 > +static int ast2600_i2c_slave_irq(struct ast2600_i2c_bus *i2c_bus)
 > +{
 > +    u32 ier = readl(i2c_bus->reg_base + AST2600_I2CS_IER);
 > +    u32 isr = readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +
 > +    if (!(isr & ier))
 > +        return 0;
 > +
 > +    /* Slave interrupt coming after Master package done
 > +     * So need handle master first.
 > +     */
 > +    if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR) & 
AST2600_I2CM_PKT_DONE)
 > +        return 0;
 > +
 > +    isr &= ~(AST2600_I2CS_ADDR_INDICATE_MASK);
 > +
 > +    if (AST2600_I2CS_ADDR1_NAK & isr)
 > +        isr &= ~AST2600_I2CS_ADDR1_NAK;
 > +
 > +    if (AST2600_I2CS_ADDR2_NAK & isr)
 > +        isr &= ~AST2600_I2CS_ADDR2_NAK;
 > +
 > +    if (AST2600_I2CS_ADDR3_NAK & isr)
 > +        isr &= ~AST2600_I2CS_ADDR3_NAK;
 > +
 > +    if (AST2600_I2CS_ADDR_MASK & isr)
 > +        isr &= ~AST2600_I2CS_ADDR_MASK;
 > +
 > +    if (AST2600_I2CS_PKT_DONE & isr) {
 > +        if (i2c_bus->mode == DMA_MODE)
 > +            ast2600_i2c_slave_packet_dma_irq(i2c_bus, isr);
 > +        else
 > +            ast2600_i2c_slave_packet_buff_irq(i2c_bus, isr);
 > +    } else
 > +        ast2600_i2c_slave_byte_irq(i2c_bus, isr);
 > +
 > +    return 1;
 > +}
 > +#endif
 > +
 > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
 > +{
 > +    struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
 > +    int xfer_len = 0;
 > +    int i = 0;

No need to init.

 > +    u32 cmd;
 > +
 > +    cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | 
AST2600_I2CM_START_CMD;
 > +
 > +    /* send start */
 > +    dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
 > +        i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" : "write",
 > +        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;

[...]

 > +static int ast2600_i2c_master_xfer(struct i2c_adapter *adap,
 > +                      struct i2c_msg *msgs, int num)
 > +{
 > +    struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap);
 > +    unsigned long timeout;
 > +    int ret = 0;

Looks like this initialization is unneeded.

 > +
 > +    /* If bus is busy in a single master environment, attempt 
recovery. */
 > +    if (!i2c_bus->multi_master &&
 > +        (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) & 
AST2600_I2CC_BUS_BUSY_STS)) {
 > +        int ret;

No need for an extra 'ret'.

 > +
 > +        ret = ast2600_i2c_recover_bus(i2c_bus);
 > +        if (ret)
 > +            return ret;
 > +    }
 > +
 > +#ifdef CONFIG_I2C_SLAVE
 > +    if (i2c_bus->mode == BUFF_MODE) {
 > +        if (i2c_bus->slave_operate)
 > +            return -EBUSY;
 > +        /* disable slave isr */
 > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
 > +        if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) || 
(i2c_bus->slave_operate)) {
 > +            writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + 
AST2600_I2CS_IER);
 > +            return -EBUSY;
 > +        }
 > +    }
 > +#endif
 > +
 > +    i2c_bus->cmd_err = 0;
 > +    i2c_bus->msgs = msgs;
 > +    i2c_bus->msgs_index = 0;
 > +    i2c_bus->msgs_count = num;
 > +    reinit_completion(&i2c_bus->cmd_complete);
 > +    ret = ast2600_i2c_do_start(i2c_bus);
 > +#ifdef CONFIG_I2C_SLAVE
 > +    /* avoid race condication slave is wait and master wait 1st 
slave operate */
 > +    if (i2c_bus->mode == BUFF_MODE)
 > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + 
AST2600_I2CS_IER);
 > +#endif
 > +    if (ret)
 > +        goto master_out;
 > +    timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, 
i2c_bus->adap.timeout);
 > +    if (timeout == 0) {
 > +        u32 isr = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
 > +        u32 i2c_status = readl(i2c_bus->reg_base + 
AST2600_I2CC_STS_AND_BUFF);
 > +
 > +        dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n", isr, 
i2c_status);
 > +        if (isr || (i2c_status & AST2600_I2CC_TX_DIR_MASK)) {
 > +            u32 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);
 > +#ifdef CONFIG_I2C_SLAVE
 > +            if (ctrl & AST2600_I2CC_SLAVE_EN) {
 > +                u32 cmd = SLAVE_TRIGGER_CMD;
 > +
 > +                if (i2c_bus->mode == DMA_MODE) {
 > +                    cmd |= AST2600_I2CS_RX_DMA_EN;
 > +                    writel(i2c_bus->slave_dma_addr,
 > +                           i2c_bus->reg_base + AST2600_I2CS_RX_DMA);
 > +                    writel(i2c_bus->slave_dma_addr,
 > +                           i2c_bus->reg_base + AST2600_I2CS_TX_DMA);
 > + 
writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +                           i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +                } else if (i2c_bus->mode == BUFF_MODE) {
 > +                    cmd = SLAVE_TRIGGER_CMD;
 > +                } else {
 > +                    cmd &= ~AST2600_I2CS_PKT_MODE_EN;
 > +                }
 > +                writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +            }
 > +#endif
 > +        }
 > +        ret = -ETIMEDOUT;
 > +    } else {
 > +        ret = i2c_bus->cmd_err;
 > +    }
 > +
 > +    dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr, 
i2c_bus->cmd_err);
 > +
 > +master_out:
 > +    if (i2c_bus->mode == DMA_MODE) {
 > +        kfree(i2c_bus->master_safe_buf);
 > +        i2c_bus->master_safe_buf = NULL;

Alignement.

 > +    }
 > +
 > +    return ret;
 > +}
 > +
 > +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
 > +{
 > +    struct platform_device *pdev = to_platform_device(i2c_bus->dev);
 > +    u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE | 
AST2600_I2CC_MASTER_EN;
 > +
 > +    /* I2C Reset */
 > +    writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > +
 > +    if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
 > +        i2c_bus->multi_master = true;
 > +    else
 > +        fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
 > +
 > +    /* Enable Master Mode */
 > +    writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > +    /* disable slave address */
 > +    writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
 > +
 > +    /* Set AC Timing */
 > +    writel(ast2600_select_i2c_clock(i2c_bus), i2c_bus->reg_base + 
AST2600_I2CC_AC_TIMING);
 > +
 > +    /* Clear Interrupt */
 > +    writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR);
 > +
 > +#ifdef CONFIG_I2C_SLAVE
 > +    /* for memory buffer initial */
 > +    if (i2c_bus->mode == DMA_MODE) {
 > +        i2c_bus->slave_dma_buf = dma_alloc_coherent(i2c_bus->dev, 
I2C_SLAVE_MSG_BUF_SIZE,
 > +                                &i2c_bus->slave_dma_addr, GFP_KERNEL);

dmam_alloc_coherent() could maybe be used here.
This would simplify the remove function and avoid some conditional 
compilation.
This also fixes a leak if i2c_add_adapter() fails in the probe.

It is also likely that doing so, the devm_free_irq() in the remove 
function also becomes redundant and can be removed as well.

 > +        if (!i2c_bus->slave_dma_buf)
 > +            return;
 > +    }
 > +
 > +    writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR);
 > +
 > +    if (i2c_bus->mode == BYTE_MODE) {
 > +        writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER);
 > +    } else {
 > +        /* Set interrupt generation of I2C slave controller */
 > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base + 
AST2600_I2CS_IER);
 > +    }
 > +#endif
 > +}
 > +
 > +#ifdef CONFIG_I2C_SLAVE
 > +static int ast2600_i2c_reg_slave(struct i2c_client *client)
 > +{
 > +    struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(client->adapter);
 > +    u32 cmd = SLAVE_TRIGGER_CMD;
 > +
 > +    if (i2c_bus->slave)
 > +        return -EINVAL;
 > +
 > +    dev_dbg(i2c_bus->dev, "slave addr %x\n", client->addr);
 > +
 > +    writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
 > +    writel(AST2600_I2CC_SLAVE_EN | readl(i2c_bus->reg_base + 
AST2600_I2CC_FUN_CTRL),
 > +           i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > +
 > +    /* trigger rx buffer */
 > +    if (i2c_bus->mode == DMA_MODE) {
 > +        cmd |= AST2600_I2CS_RX_DMA_EN;
 > +        writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + 
AST2600_I2CS_RX_DMA);
 > +        writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base + 
AST2600_I2CS_TX_DMA);
 > +        writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
 > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
 > +    } else if (i2c_bus->mode == BUFF_MODE) {
 > +        cmd = SLAVE_TRIGGER_CMD;
 > +    } else {
 > +        cmd &= ~AST2600_I2CS_PKT_MODE_EN;
 > +    }
 > +
 > +    writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
 > +    i2c_bus->slave = client;
 > +    /* Set slave addr. */
 > +    writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
 > +            i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);

Nit: alignement

 > +
 > +    return 0;
 > +}
 > +
 > +static int ast2600_i2c_unreg_slave(struct i2c_client *slave)
 > +{
 > +    struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(slave->adapter);
 > +
 > +    WARN_ON(!i2c_bus->slave);
 > +
 > +    /* Turn off slave mode. */
 > +    writel(~AST2600_I2CC_SLAVE_EN & readl(i2c_bus->reg_base + 
AST2600_I2CC_FUN_CTRL),
 > +           i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
 > +    writel(readl(i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL) & 
~AST2600_I2CS_ADDR1_MASK,
 > +           i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
 > +
 > +    i2c_bus->slave = NULL;
 > +
 > +    return 0;
 > +}
 > +#endif
 > +
 > +static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
 > +{
 > +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | 
I2C_FUNC_SMBUS_BLOCK_DATA;
 > +}
 > +
 > +static const struct i2c_algorithm i2c_ast2600_algorithm = {
 > +    .master_xfer = ast2600_i2c_master_xfer,
 > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
 > +    .reg_slave = ast2600_i2c_reg_slave,
 > +    .unreg_slave = ast2600_i2c_unreg_slave,
 > +#endif
 > +    .functionality = ast2600_i2c_functionality,
 > +};
 > +
 > +static const struct of_device_id ast2600_i2c_bus_of_table[] = {
 > +    {
 > +        .compatible = "aspeed,ast2600-i2cv2",
 > +    },
 > +    {}
 > +};
 > +
 > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table);
 > +
 > +static int ast2600_i2c_probe(struct platform_device *pdev)
 > +{
 > +    struct device_node *np = pdev->dev.of_node;
 > +    struct device *dev = &pdev->dev;
 > +    struct ast2600_i2c_bus *i2c_bus;
 > +    struct resource *res;
 > +    u32 global_ctrl;
 > +    int ret = 0;
 > +
 > +    i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
 > +    if (!i2c_bus)
 > +        return -ENOMEM;
 > +
 > +    i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
 > +    if (IS_ERR(i2c_bus->reg_base))
 > +        return PTR_ERR(i2c_bus->reg_base);
 > +
 > +    i2c_bus->rst = devm_reset_control_get_shared(dev, NULL);
 > +    if (IS_ERR(i2c_bus->rst))
 > +        return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing 
reset ctrl\n");
 > +
 > +    reset_control_deassert(i2c_bus->rst);
 > +
 > +    i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(np, 
"aspeed,global-regs");
 > +    if (IS_ERR(i2c_bus->global_regs))
 > +        return PTR_ERR(i2c_bus->global_regs);
 > +
 > +    regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL, &global_ctrl);
 > +    if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
 > +        regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL, 
AST2600_GLOBAL_INIT);
 > +        regmap_write(i2c_bus->global_regs, 
AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
 > +    }
 > +
 > +    i2c_bus->slave_operate = 0;
 > +    i2c_bus->dev = dev;
 > +    i2c_bus->mode = BUFF_MODE;
 > +
 > +    if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma"))
 > +        i2c_bus->mode = DMA_MODE;
 > +
 > +    if (i2c_bus->mode == BUFF_MODE) {
 > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 > +        if (res && resource_size(res) >= 2) {
 > +            i2c_bus->buf_base = devm_ioremap_resource(dev, res);
 > +
 > +            if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
 > +                i2c_bus->buf_size = resource_size(res)/2;
 > +        } else
 > +            i2c_bus->mode = BYTE_MODE;
 > +    }
 > +
 > +    /* i2c timeout counter: use base clk4 1Mhz,
 > +     * per unit: 1/(1000/4096) = 4096us
 > +     */
 > +    ret = of_property_read_u32(dev->of_node,
 > +                            "i2c-scl-clk-low-timeout-us",
 > +                            &i2c_bus->timeout);

Strange layout.

 > +    if (ret < 0)
 > +        i2c_bus->timeout = 0;

Nit: This is not really needed since i2c_bus is kzalloc()'ed.

 > +    else
 > +        i2c_bus->timeout /= 4096;
 > +
 > +    init_completion(&i2c_bus->cmd_complete);
 > +
 > +    i2c_bus->irq = platform_get_irq(pdev, 0);
 > +    if (i2c_bus->irq < 0)
 > +        return i2c_bus->irq;
 > +
 > +    platform_set_drvdata(pdev, i2c_bus);
 > +
 > +    i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
 > +    if (IS_ERR(i2c_bus->clk))
 > +        return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk), 
"Can't get clock\n");
 > +
 > +    i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
 > +
 > +    ret = of_property_read_u32(dev->of_node, "clock-frequency", 
&i2c_bus->bus_frequency);
 > +    if (ret < 0) {
 > +        dev_warn(dev, "Could not read clock-frequency property\n");
 > +        i2c_bus->bus_frequency = 100000;
 > +    }
 > +
 > +    /* Initialize the I2C adapter */
 > +    i2c_bus->adap.owner = THIS_MODULE;
 > +    i2c_bus->adap.algo = &i2c_ast2600_algorithm;
 > +    i2c_bus->adap.retries = 0;
 > +    i2c_bus->adap.dev.parent = i2c_bus->dev;
 > +    i2c_bus->adap.dev.of_node = dev->of_node;
 > +    i2c_bus->adap.algo_data = i2c_bus;
 > +    strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name));
 > +    i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
 > +
 > +    ast2600_i2c_init(i2c_bus);
 > +
 > +    ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
 > +                   dev_name(dev), i2c_bus);
 > +    if (ret < 0)
 > +        return dev_err_probe(dev, ret, "Unable to request irq %d\n", 
i2c_bus->irq);
 > +
 > +    if (of_property_read_bool(dev->of_node, "smbus-alert")) {
 > +        i2c_bus->alert_enable = 1;
 > +        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 = 0;
 > +        /* Set interrupt generation of I2C master controller */
 > +        writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
 > +                i2c_bus->reg_base + AST2600_I2CM_IER);
 > +    }
 > +
 > +    ret = i2c_add_adapter(&i2c_bus->adap);

Maybe devm_i2c_add_adapter() + remove function simplification.
This change + dmam_alloc_coherent() above + removing devm_free_irq() 
should, all together, not chnage the order of the remove function IIUIC.

CJ

 > +    if (ret)
 > +        return ret;
 > +
 > +    dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n",
 > +         dev->of_node->name, i2c_bus->adap.nr, 
i2c_bus->bus_frequency / 1000,
 > +         i2c_bus->mode);
 > +
 > +    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);
 > +    devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
 > +
 > +#ifdef CONFIG_I2C_SLAVE
 > +    /* for memory buffer initial */
 > +    if (i2c_bus->mode == DMA_MODE)
 > +        dma_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
 > +                i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);
 > +#endif
 > +
 > +    return 0;
 > +}
 > +
 > +static struct platform_driver ast2600_i2c_bus_driver = {
 > +    .probe = ast2600_i2c_probe,
 > +    .remove = ast2600_i2c_remove,
 > +    .driver = {
 > +        .name = KBUILD_MODNAME,
 > +        .of_match_table = ast2600_i2c_bus_of_table,
 > +    },
 > +};
 > +module_platform_driver(ast2600_i2c_bus_driver);
 > +
 > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
 > +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
 > +MODULE_LICENSE("GPL");

Hello Christophe,

> Subject: Re: [PATCH v10 2/2] i2c: aspeed: support ast2600 i2c new register
> mode driver
> 
> (resending, my mail client removed some addresses. Sorry for the duplicated
> message for the others)
> 
> 
> Le 15/04/2023 à 03:28, Ryan Chen a écrit :
>  > 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 <ryan_chen@aspeedtech.com>
>  > ---
> 
> [...]
> 
>  > +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));
>  > +
>  > +    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,
>  > +           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)) {
>  > +        writel(AST2600_I2CM_RECOVER_CMD_EN, i2c_bus->reg_base +
> AST2600_I2CM_CMD_STS);
>  > +        r = wait_for_completion_timeout(&i2c_bus->cmd_complete,
> i2c_bus->adap.timeout);
>  > +        if (r == 0) {
>  > +            dev_dbg(i2c_bus->dev, "recovery timed out\n");
>  > +            ret = -ETIMEDOUT;
>  > +        } else {
>  > +            if (i2c_bus->cmd_err) {
>  > +                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));
>  > +    if (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
> AST2600_I2CC_BUS_BUSY_STS) {
>  > +        dev_dbg(i2c_bus->dev, "Can't recovery bus [%x]\n",
> 
> recover?

Sorry, Do you mean modify the wording "Can’t recover bus"?
> 
>  > +            readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF));
>  > +        ret = -EPROTO;
>  > +    }
>  > +
>  > +    writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>  > +    return ret;
>  > +}
>  > +
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +static void ast2600_i2c_slave_packet_dma_irq(struct ast2600_i2c_bus
> *i2c_bus, u32 sts)
>  > +{
>  > +    int slave_rx_len;
>  > +    u32 cmd = 0;
>  > +    u8 value;
>  > +    int i = 0;
> 
> No need to init

Will remove init.
> 
>  > +
>  > +    sts &= ~(AST2600_I2CS_SLAVE_PENDING);
>  > +    /* Handle i2c slave timeout condition */
>  > +    if (AST2600_I2CS_INACTIVE_TO & sts) {
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        cmd |= AST2600_I2CS_RX_DMA_EN;
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        return;
>  > +    }
>  > +
>  > +    sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
>  > +
>  > +    switch (sts) {
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_WAIT_RX_DMA:
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED,
> &value);
>  > +        slave_rx_len =
> AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
>  > +                              AST2600_I2CS_DMA_LEN_STS));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED,
>  > +                    &i2c_bus->slave_dma_buf[i]);
>  > +        }
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE_NAK
> |
>  > +            AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_RX_DMA
> |
>  > +            AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_RX_DONE_NAK | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA |
> AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
>  > +        if (sts & AST2600_I2CS_SLAVE_MATCH)
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_REQUESTED, &value);
>  > +
>  > +        slave_rx_len =
> AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
>  > +                              AST2600_I2CS_DMA_LEN_STS));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED,
>  > +                    &i2c_bus->slave_dma_buf[i]);
>  > +        }
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        if (sts & AST2600_I2CS_STOP)
>  > +            i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
>  > +        break;
>  > +
>  > +    /* it is Mw data Mr coming -> it need send tx */
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_TX_DMA:
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_WAIT_TX_DMA:
>  > +        /* it should be repeat start read */
>  > +        if (sts & AST2600_I2CS_SLAVE_MATCH)
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_REQUESTED, &value);
>  > +
>  > +        slave_rx_len =
> AST2600_I2C_GET_RX_DMA_LEN(readl(i2c_bus->reg_base +
>  > +                              AST2600_I2CS_DMA_LEN_STS));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED,
>  > +                    &i2c_bus->slave_dma_buf[i]);
>  > +        }
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
>  > +                &i2c_bus->slave_dma_buf[0]);
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
>  > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
>  > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_WAIT_TX_DMA:
>  > +        /* First Start read */
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
>  > +                &i2c_bus->slave_dma_buf[0]);
>  > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
>  > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
>  > +        break;
>  > +    case AST2600_I2CS_WAIT_TX_DMA:
>  > +        /* it should be next start read */
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED,
>  > +                &i2c_bus->slave_dma_buf[0]);
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
>  > +        writel(AST2600_I2CS_SET_TX_DMA_LEN(1),
>  > +                i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_DMA_EN;
>  > +        break;
> 
> Having a new line or not after breaks is not consistent.


Will update for consistent.
> 
>  > +
>  > +    case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
>  > +        /* it just tx complete */
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_DMA_LEN_STS);
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_DMA_EN;
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
>  > +        cmd = 0;
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED,
> &value);
>  > +        break;
>  > +    case AST2600_I2CS_STOP:
>  > +        cmd = 0;
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        break;
>  > +    default:
>  > +        dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts
> %x\n", sts,
>  > +            readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF));
>  > +        break;
>  > +    }
>  > +
>  > +    if (cmd)
>  > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +    writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
>  > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +}
>  > +
>  > +static void ast2600_i2c_slave_packet_buff_irq(struct ast2600_i2c_bus
> *i2c_bus, u32 sts)
>  > +{
>  > +    int slave_rx_len = 0;
>  > +    u32 cmd = 0;
>  > +    u8 value;
>  > +    int i = 0;
> 
> No need to init;


Will remove init 

> 
>  > +
>  > +    /* due to master slave is common buffer, so need force the
> master stop not issue */
>  > +    if (readl(i2c_bus->reg_base + AST2600_I2CM_CMD_STS) & 0xffff) {
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CM_CMD_STS);
>  > +        i2c_bus->cmd_err = -EBUSY;
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        complete(&i2c_bus->cmd_complete);
>  > +    }
>  > +
>  > +    /* Handle i2c slave timeout condition */
>  > +    if (AST2600_I2CS_INACTIVE_TO & sts) {
>  > +        writel(SLAVE_TRIGGER_CMD, i2c_bus->reg_base +
> AST2600_I2CS_CMD_STS);
>  > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        i2c_bus->slave_operate = 0;
>  > +        return;
>  > +    }
>  > +
>  > +    sts &= ~(AST2600_I2CS_PKT_DONE | AST2600_I2CS_PKT_ERROR);
>  > +
>  > +    if (sts & AST2600_I2CS_SLAVE_MATCH)
>  > +        i2c_bus->slave_operate = 1;
>  > +
>  > +    switch (sts) {
>  > +    case AST2600_I2CS_SLAVE_PENDING |
> AST2600_I2CS_WAIT_RX_DMA |
>  > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_SLAVE_PENDING |
>  > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_SLAVE_PENDING |
>  > +         AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_STOP:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        fallthrough;
>  > +    case AST2600_I2CS_SLAVE_PENDING |
>  > +         AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH
> |
> AST2600_I2CS_RX_DONE:
>  > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH
> |
> AST2600_I2CS_RX_DONE:
>  > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_SLAVE_MATCH:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED,
> &value);
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        if (sts & AST2600_I2CS_RX_DONE) {
>  > +            slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                                   AST2600_I2CC_BUFF_CTRL));
>  > +            for (i = 0; i < slave_rx_len; i++) {
>  > +                value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +                i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +            }
>  > +        }
>  > +        if (readl(i2c_bus->reg_base + AST2600_I2CS_CMD_STS) &
> AST2600_I2CS_RX_BUFF_EN)
>  > +            cmd = 0;
>  > +        else
>  > +            cmd = SLAVE_TRIGGER_CMD |
> AST2600_I2CS_RX_BUFF_EN;
>  > +
>  > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
>  > +                    i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        break;
>  > +    case AST2600_I2CS_WAIT_RX_DMA | AST2600_I2CS_RX_DONE:
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                               AST2600_I2CC_BUFF_CTRL));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +        }
>  > +        cmd |= AST2600_I2CS_RX_BUFF_EN;
>  > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
>  > +                        i2c_bus->reg_base +
> AST2600_I2CC_BUFF_CTRL);
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_PENDING |
> AST2600_I2CS_WAIT_RX_DMA |
>  > +                AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                               AST2600_I2CC_BUFF_CTRL));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +        }
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        cmd |= AST2600_I2CS_RX_BUFF_EN;
>  > +        writel(AST2600_I2CC_SET_RX_BUF_LEN(i2c_bus->buf_size),
>  > +                        i2c_bus->reg_base +
> AST2600_I2CC_BUFF_CTRL);
>  > +        break;
>  > +
> 
> Having a new line or not after breaks is not consistent.

Will update for consistent.
> 
>  > +    case AST2600_I2CS_SLAVE_PENDING | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_STOP:
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                               AST2600_I2CC_BUFF_CTRL));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +        }
>  > +        /* workaround for avoid next start with len != 0 */
>  > +        writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        break;
>  > +
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_STOP:
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                               AST2600_I2CC_BUFF_CTRL));
>  > +        for (i = 0; i < slave_rx_len; i++) {
>  > +            value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +        }
>  > +        /* workaround for avoid next start with len != 0 */
>  > +        writel(BIT(0), i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        break;
>  > +    case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_SLAVE_MATCH:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
> &value);
>  > +        writeb(value, i2c_bus->buf_base);
>  > +        writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
>  > +                i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
>  > +        break;
>  > +    case AST2600_I2CS_WAIT_TX_DMA | AST2600_I2CS_RX_DONE:
>  > +    case AST2600_I2CS_WAIT_TX_DMA:
>  > +        if (sts & AST2600_I2CS_RX_DONE) {
>  > +            slave_rx_len =
> AST2600_I2CC_GET_RX_BUF_LEN(readl(i2c_bus->reg_base +
>  > +                            AST2600_I2CC_BUFF_CTRL));
>  > +            for (i = 0; i < slave_rx_len; i++) {
>  > +                value = readb(i2c_bus->buf_base + 0x10 + i);
>  > +                i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_WRITE_RECEIVED, &value);
>  > +            }
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_READ_REQUESTED, &value);
>  > +        } else {
>  > +            i2c_slave_event(i2c_bus->slave,
> I2C_SLAVE_READ_PROCESSED, &value);
>  > +        }
>  > +        writeb(value, i2c_bus->buf_base);
>  > +        writel(AST2600_I2CC_SET_TX_BUF_LEN(1),
>  > +                i2c_bus->reg_base + AST2600_I2CC_BUFF_CTRL);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_TX_BUFF_EN;
>  > +        break;
>  > +    /* workaround : trigger the cmd twice to fix next state keep
> 1000000 */
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED,
> &value);
>  > +        cmd = SLAVE_TRIGGER_CMD | AST2600_I2CS_RX_BUFF_EN;
>  > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +        break;
>  > +
>  > +    case AST2600_I2CS_TX_NAK | AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_STOP:
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        break;
>  > +    default:
>  > +        dev_dbg(i2c_bus->dev, "unhandled slave isr case %x, sts
> %x\n", sts,
>  > +            readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF));
>  > +        break;
>  > +    }
>  > +
>  > +    if (cmd)
>  > +        writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +    writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_ISR);
>  > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +
>  > +    if ((sts & AST2600_I2CS_STOP) && !(sts &
> AST2600_I2CS_SLAVE_PENDING))
>  > +        i2c_bus->slave_operate = 0;
>  > +
> 
> Nit: unneeded empty line.

Will remove it.
> 
>  > +}
>  > +
>  > +static void ast2600_i2c_slave_byte_irq(struct ast2600_i2c_bus
> *i2c_bus, u32 sts)
>  > +{
>  > +    u32 i2c_buff = readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
>  > +    u32 cmd = AST2600_I2CS_ACTIVE_ALL;
>  > +    u8 byte_data;
>  > +    u8 value;
>  > +
>  > +    switch (sts) {
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_WAIT_RX_DMA:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_REQUESTED,
> &value);
>  > +        /* first address match is address */
>  > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
>  > +        break;
>  > +    case AST2600_I2CS_RX_DONE | AST2600_I2CS_WAIT_RX_DMA:
>  > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_WRITE_RECEIVED,
> &byte_data);
>  > +        break;
>  > +    case AST2600_I2CS_SLAVE_MATCH | AST2600_I2CS_RX_DONE |
> AST2600_I2CS_WAIT_TX_DMA:
>  > +        cmd |= AST2600_I2CS_TX_CMD;
>  > +        byte_data = AST2600_I2CC_GET_RX_BUFF(i2c_buff);
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_REQUESTED,
> &byte_data);
>  > +        writel(byte_data, i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
>  > +        break;
>  > +    case AST2600_I2CS_TX_ACK | AST2600_I2CS_WAIT_TX_DMA:
>  > +        cmd |= AST2600_I2CS_TX_CMD;
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_READ_PROCESSED,
> &byte_data);
>  > +        writel(byte_data, i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
>  > +        break;
>  > +    case AST2600_I2CS_STOP:
>  > +    case AST2600_I2CS_STOP | AST2600_I2CS_TX_NAK:
>  > +        i2c_slave_event(i2c_bus->slave, I2C_SLAVE_STOP, &value);
>  > +        break;
>  > +    default:
>  > +        dev_dbg(i2c_bus->dev, "unhandled pkt isr %x\n", sts);
>  > +        break;
>  > +    }
>  > +    writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +    writel(sts, i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +    readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +}
>  > +
>  > +static int ast2600_i2c_slave_irq(struct ast2600_i2c_bus *i2c_bus)
>  > +{
>  > +    u32 ier = readl(i2c_bus->reg_base + AST2600_I2CS_IER);
>  > +    u32 isr = readl(i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +
>  > +    if (!(isr & ier))
>  > +        return 0;
>  > +
>  > +    /* Slave interrupt coming after Master package done
>  > +     * So need handle master first.
>  > +     */
>  > +    if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR) &
> AST2600_I2CM_PKT_DONE)
>  > +        return 0;
>  > +
>  > +    isr &= ~(AST2600_I2CS_ADDR_INDICATE_MASK);
>  > +
>  > +    if (AST2600_I2CS_ADDR1_NAK & isr)
>  > +        isr &= ~AST2600_I2CS_ADDR1_NAK;
>  > +
>  > +    if (AST2600_I2CS_ADDR2_NAK & isr)
>  > +        isr &= ~AST2600_I2CS_ADDR2_NAK;
>  > +
>  > +    if (AST2600_I2CS_ADDR3_NAK & isr)
>  > +        isr &= ~AST2600_I2CS_ADDR3_NAK;
>  > +
>  > +    if (AST2600_I2CS_ADDR_MASK & isr)
>  > +        isr &= ~AST2600_I2CS_ADDR_MASK;
>  > +
>  > +    if (AST2600_I2CS_PKT_DONE & isr) {
>  > +        if (i2c_bus->mode == DMA_MODE)
>  > +            ast2600_i2c_slave_packet_dma_irq(i2c_bus, isr);
>  > +        else
>  > +            ast2600_i2c_slave_packet_buff_irq(i2c_bus, isr);
>  > +    } else
>  > +        ast2600_i2c_slave_byte_irq(i2c_bus, isr);
>  > +
>  > +    return 1;
>  > +}
>  > +#endif
>  > +
>  > +static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
>  > +{
>  > +    struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
>  > +    int xfer_len = 0;
>  > +    int i = 0;
> 
> No need to init.

Will remove init 0.
> 
>  > +    u32 cmd;
>  > +
>  > +    cmd = AST2600_I2CM_PKT_EN |
> AST2600_I2CM_PKT_ADDR(msg->addr) |
> AST2600_I2CM_START_CMD;
>  > +
>  > +    /* send start */
>  > +    dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
>  > +        i2c_bus->msgs_index, msg->flags & I2C_M_RD ? "read" :
> "write",
>  > +        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;
> 
> [...]
> 
>  > +static int ast2600_i2c_master_xfer(struct i2c_adapter *adap,
>  > +                      struct i2c_msg *msgs, int num)
>  > +{
>  > +    struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(adap);
>  > +    unsigned long timeout;
>  > +    int ret = 0;
> 
> Looks like this initialization is unneeded.

Will remove 0.

> 
>  > +
>  > +    /* If bus is busy in a single master environment, attempt
> recovery. */
>  > +    if (!i2c_bus->multi_master &&
>  > +        (readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF) &
> AST2600_I2CC_BUS_BUSY_STS)) {
>  > +        int ret;
> 
> No need for an extra 'ret'.


Will remove it.
> 
>  > +
>  > +        ret = ast2600_i2c_recover_bus(i2c_bus);
>  > +        if (ret)
>  > +            return ret;
>  > +    }
>  > +
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +    if (i2c_bus->mode == BUFF_MODE) {
>  > +        if (i2c_bus->slave_operate)
>  > +            return -EBUSY;
>  > +        /* disable slave isr */
>  > +        writel(0, i2c_bus->reg_base + AST2600_I2CS_IER);
>  > +        if (readl(i2c_bus->reg_base + AST2600_I2CS_ISR) ||
> (i2c_bus->slave_operate)) {
>  > +            writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_IER);
>  > +            return -EBUSY;
>  > +        }
>  > +    }
>  > +#endif
>  > +
>  > +    i2c_bus->cmd_err = 0;
>  > +    i2c_bus->msgs = msgs;
>  > +    i2c_bus->msgs_index = 0;
>  > +    i2c_bus->msgs_count = num;
>  > +    reinit_completion(&i2c_bus->cmd_complete);
>  > +    ret = ast2600_i2c_do_start(i2c_bus);
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +    /* avoid race condication slave is wait and master wait 1st
> slave operate */
>  > +    if (i2c_bus->mode == BUFF_MODE)
>  > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_IER);
>  > +#endif
>  > +    if (ret)
>  > +        goto master_out;
>  > +    timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete,
> i2c_bus->adap.timeout);
>  > +    if (timeout == 0) {
>  > +        u32 isr = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
>  > +        u32 i2c_status = readl(i2c_bus->reg_base +
> AST2600_I2CC_STS_AND_BUFF);
>  > +
>  > +        dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n", isr,
> i2c_status);
>  > +        if (isr || (i2c_status & AST2600_I2CC_TX_DIR_MASK)) {
>  > +            u32 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);
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +            if (ctrl & AST2600_I2CC_SLAVE_EN) {
>  > +                u32 cmd = SLAVE_TRIGGER_CMD;
>  > +
>  > +                if (i2c_bus->mode == DMA_MODE) {
>  > +                    cmd |= AST2600_I2CS_RX_DMA_EN;
>  > +                    writel(i2c_bus->slave_dma_addr,
>  > +                           i2c_bus->reg_base +
> AST2600_I2CS_RX_DMA);
>  > +                    writel(i2c_bus->slave_dma_addr,
>  > +                           i2c_bus->reg_base +
> AST2600_I2CS_TX_DMA);
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +                           i2c_bus->reg_base +
> AST2600_I2CS_DMA_LEN);
>  > +                } else if (i2c_bus->mode == BUFF_MODE) {
>  > +                    cmd = SLAVE_TRIGGER_CMD;
>  > +                } else {
>  > +                    cmd &= ~AST2600_I2CS_PKT_MODE_EN;
>  > +                }
>  > +                writel(cmd, i2c_bus->reg_base +
> AST2600_I2CS_CMD_STS);
>  > +            }
>  > +#endif
>  > +        }
>  > +        ret = -ETIMEDOUT;
>  > +    } else {
>  > +        ret = i2c_bus->cmd_err;
>  > +    }
>  > +
>  > +    dev_dbg(i2c_bus->dev, "bus%d-m: %d end\n", i2c_bus->adap.nr,
> i2c_bus->cmd_err);
>  > +
>  > +master_out:
>  > +    if (i2c_bus->mode == DMA_MODE) {
>  > +        kfree(i2c_bus->master_safe_buf);
>  > +        i2c_bus->master_safe_buf = NULL;
> 
> Alignement.

Sorry, I can't catch the alignment could you help point out or give me example? 

> 
>  > +    }
>  > +
>  > +    return ret;
>  > +}
>  > +
>  > +static void ast2600_i2c_init(struct ast2600_i2c_bus *i2c_bus)
>  > +{
>  > +    struct platform_device *pdev = to_platform_device(i2c_bus->dev);
>  > +    u32 fun_ctrl = AST2600_I2CC_BUS_AUTO_RELEASE |
> AST2600_I2CC_MASTER_EN;
>  > +
>  > +    /* I2C Reset */
>  > +    writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>  > +
>  > +    if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
>  > +        i2c_bus->multi_master = true;
>  > +    else
>  > +        fun_ctrl |= AST2600_I2CC_MULTI_MASTER_DIS;
>  > +
>  > +    /* Enable Master Mode */
>  > +    writel(fun_ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>  > +    /* disable slave address */
>  > +    writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
>  > +
>  > +    /* Set AC Timing */
>  > +    writel(ast2600_select_i2c_clock(i2c_bus), i2c_bus->reg_base +
> AST2600_I2CC_AC_TIMING);
>  > +
>  > +    /* Clear Interrupt */
>  > +    writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CM_ISR);
>  > +
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +    /* for memory buffer initial */
>  > +    if (i2c_bus->mode == DMA_MODE) {
>  > +        i2c_bus->slave_dma_buf = dma_alloc_coherent(i2c_bus->dev,
> I2C_SLAVE_MSG_BUF_SIZE,
>  > +                                &i2c_bus->slave_dma_addr,
> GFP_KERNEL);
> 
> dmam_alloc_coherent() could maybe be used here.
> This would simplify the remove function and avoid some conditional
> compilation.
> This also fixes a leak if i2c_add_adapter() fails in the probe.
> 
> It is also likely that doing so, the devm_free_irq() in the remove
> function also becomes redundant and can be removed as well.
> 
>  > +        if (!i2c_bus->slave_dma_buf)
>  > +            return;
>  > +    }
>  > +
>  > +    writel(0xfffffff, i2c_bus->reg_base + AST2600_I2CS_ISR);
>  > +
>  > +    if (i2c_bus->mode == BYTE_MODE) {
>  > +        writel(0xffff, i2c_bus->reg_base + AST2600_I2CS_IER);
>  > +    } else {
>  > +        /* Set interrupt generation of I2C slave controller */
>  > +        writel(AST2600_I2CS_PKT_DONE, i2c_bus->reg_base +
> AST2600_I2CS_IER);
>  > +    }
>  > +#endif
>  > +}
>  > +
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +static int ast2600_i2c_reg_slave(struct i2c_client *client)
>  > +{
>  > +    struct ast2600_i2c_bus *i2c_bus =
> i2c_get_adapdata(client->adapter);
>  > +    u32 cmd = SLAVE_TRIGGER_CMD;
>  > +
>  > +    if (i2c_bus->slave)
>  > +        return -EINVAL;
>  > +
>  > +    dev_dbg(i2c_bus->dev, "slave addr %x\n", client->addr);
>  > +
>  > +    writel(0, i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
>  > +    writel(AST2600_I2CC_SLAVE_EN | readl(i2c_bus->reg_base +
> AST2600_I2CC_FUN_CTRL),
>  > +           i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>  > +
>  > +    /* trigger rx buffer */
>  > +    if (i2c_bus->mode == DMA_MODE) {
>  > +        cmd |= AST2600_I2CS_RX_DMA_EN;
>  > +        writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base +
> AST2600_I2CS_RX_DMA);
>  > +        writel(i2c_bus->slave_dma_addr, i2c_bus->reg_base +
> AST2600_I2CS_TX_DMA);
>  > +
> writel(AST2600_I2CS_SET_RX_DMA_LEN(I2C_SLAVE_MSG_BUF_SIZE),
>  > +               i2c_bus->reg_base + AST2600_I2CS_DMA_LEN);
>  > +    } else if (i2c_bus->mode == BUFF_MODE) {
>  > +        cmd = SLAVE_TRIGGER_CMD;
>  > +    } else {
>  > +        cmd &= ~AST2600_I2CS_PKT_MODE_EN;
>  > +    }
>  > +
>  > +    writel(cmd, i2c_bus->reg_base + AST2600_I2CS_CMD_STS);
>  > +    i2c_bus->slave = client;
>  > +    /* Set slave addr. */
>  > +    writel(client->addr | AST2600_I2CS_ADDR1_ENABLE,
>  > +            i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
> 
> Nit: alignement
Sorry, what should I do for alignment. 

> 
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static int ast2600_i2c_unreg_slave(struct i2c_client *slave)
>  > +{
>  > +    struct ast2600_i2c_bus *i2c_bus = i2c_get_adapdata(slave->adapter);
>  > +
>  > +    WARN_ON(!i2c_bus->slave);
>  > +
>  > +    /* Turn off slave mode. */
>  > +    writel(~AST2600_I2CC_SLAVE_EN & readl(i2c_bus->reg_base +
> AST2600_I2CC_FUN_CTRL),
>  > +           i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
>  > +    writel(readl(i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL) &
> ~AST2600_I2CS_ADDR1_MASK,
>  > +           i2c_bus->reg_base + AST2600_I2CS_ADDR_CTRL);
>  > +
>  > +    i2c_bus->slave = NULL;
>  > +
>  > +    return 0;
>  > +}
>  > +#endif
>  > +
>  > +static u32 ast2600_i2c_functionality(struct i2c_adapter *adap)
>  > +{
>  > +    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> I2C_FUNC_SMBUS_BLOCK_DATA;
>  > +}
>  > +
>  > +static const struct i2c_algorithm i2c_ast2600_algorithm = {
>  > +    .master_xfer = ast2600_i2c_master_xfer,
>  > +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>  > +    .reg_slave = ast2600_i2c_reg_slave,
>  > +    .unreg_slave = ast2600_i2c_unreg_slave,
>  > +#endif
>  > +    .functionality = ast2600_i2c_functionality,
>  > +};
>  > +
>  > +static const struct of_device_id ast2600_i2c_bus_of_table[] = {
>  > +    {
>  > +        .compatible = "aspeed,ast2600-i2cv2",
>  > +    },
>  > +    {}
>  > +};
>  > +
>  > +MODULE_DEVICE_TABLE(of, ast2600_i2c_bus_of_table);
>  > +
>  > +static int ast2600_i2c_probe(struct platform_device *pdev)
>  > +{
>  > +    struct device_node *np = pdev->dev.of_node;
>  > +    struct device *dev = &pdev->dev;
>  > +    struct ast2600_i2c_bus *i2c_bus;
>  > +    struct resource *res;
>  > +    u32 global_ctrl;
>  > +    int ret = 0;
>  > +
>  > +    i2c_bus = devm_kzalloc(dev, sizeof(*i2c_bus), GFP_KERNEL);
>  > +    if (!i2c_bus)
>  > +        return -ENOMEM;
>  > +
>  > +    i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
>  > +    if (IS_ERR(i2c_bus->reg_base))
>  > +        return PTR_ERR(i2c_bus->reg_base);
>  > +
>  > +    i2c_bus->rst = devm_reset_control_get_shared(dev, NULL);
>  > +    if (IS_ERR(i2c_bus->rst))
>  > +        return dev_err_probe(dev, PTR_ERR(i2c_bus->rst), "Missing
> reset ctrl\n");
>  > +
>  > +    reset_control_deassert(i2c_bus->rst);
>  > +
>  > +    i2c_bus->global_regs = syscon_regmap_lookup_by_phandle(np,
> "aspeed,global-regs");
>  > +    if (IS_ERR(i2c_bus->global_regs))
>  > +        return PTR_ERR(i2c_bus->global_regs);
>  > +
>  > +    regmap_read(i2c_bus->global_regs, AST2600_I2CG_CTRL,
> &global_ctrl);
>  > +    if ((global_ctrl & AST2600_GLOBAL_INIT) != AST2600_GLOBAL_INIT) {
>  > +        regmap_write(i2c_bus->global_regs, AST2600_I2CG_CTRL,
> AST2600_GLOBAL_INIT);
>  > +        regmap_write(i2c_bus->global_regs,
> AST2600_I2CG_CLK_DIV_CTRL, I2CCG_DIV_CTRL);
>  > +    }
>  > +
>  > +    i2c_bus->slave_operate = 0;
>  > +    i2c_bus->dev = dev;
>  > +    i2c_bus->mode = BUFF_MODE;
>  > +
>  > +    if (of_property_read_bool(pdev->dev.of_node, "aspeed,enable-dma"))
>  > +        i2c_bus->mode = DMA_MODE;
>  > +
>  > +    if (i2c_bus->mode == BUFF_MODE) {
>  > +        res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>  > +        if (res && resource_size(res) >= 2) {
>  > +            i2c_bus->buf_base = devm_ioremap_resource(dev, res);
>  > +
>  > +            if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
>  > +                i2c_bus->buf_size = resource_size(res)/2;
>  > +        } else
>  > +            i2c_bus->mode = BYTE_MODE;
>  > +    }
>  > +
>  > +    /* i2c timeout counter: use base clk4 1Mhz,
>  > +     * per unit: 1/(1000/4096) = 4096us
>  > +     */
>  > +    ret = of_property_read_u32(dev->of_node,
>  > +                            "i2c-scl-clk-low-timeout-us",
>  > +                            &i2c_bus->timeout);
> 
> Strange layout.

Sorry, could you point out the strange? It just property read for timeout.
> 
>  > +    if (ret < 0)
>  > +        i2c_bus->timeout = 0;
> 
> Nit: This is not really needed since i2c_bus is kzalloc()'ed.

Got it, will remove it.

> 
>  > +    else
>  > +        i2c_bus->timeout /= 4096;
>  > +
>  > +    init_completion(&i2c_bus->cmd_complete);
>  > +
>  > +    i2c_bus->irq = platform_get_irq(pdev, 0);
>  > +    if (i2c_bus->irq < 0)
>  > +        return i2c_bus->irq;
>  > +
>  > +    platform_set_drvdata(pdev, i2c_bus);
>  > +
>  > +    i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
>  > +    if (IS_ERR(i2c_bus->clk))
>  > +        return dev_err_probe(i2c_bus->dev, PTR_ERR(i2c_bus->clk),
> "Can't get clock\n");
>  > +
>  > +    i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
>  > +
>  > +    ret = of_property_read_u32(dev->of_node, "clock-frequency",
> &i2c_bus->bus_frequency);
>  > +    if (ret < 0) {
>  > +        dev_warn(dev, "Could not read clock-frequency property\n");
>  > +        i2c_bus->bus_frequency = 100000;
>  > +    }
>  > +
>  > +    /* Initialize the I2C adapter */
>  > +    i2c_bus->adap.owner = THIS_MODULE;
>  > +    i2c_bus->adap.algo = &i2c_ast2600_algorithm;
>  > +    i2c_bus->adap.retries = 0;
>  > +    i2c_bus->adap.dev.parent = i2c_bus->dev;
>  > +    i2c_bus->adap.dev.of_node = dev->of_node;
>  > +    i2c_bus->adap.algo_data = i2c_bus;
>  > +    strscpy(i2c_bus->adap.name, pdev->name,
> sizeof(i2c_bus->adap.name));
>  > +    i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
>  > +
>  > +    ast2600_i2c_init(i2c_bus);
>  > +
>  > +    ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
>  > +                   dev_name(dev), i2c_bus);
>  > +    if (ret < 0)
>  > +        return dev_err_probe(dev, ret, "Unable to request irq %d\n",
> i2c_bus->irq);
>  > +
>  > +    if (of_property_read_bool(dev->of_node, "smbus-alert")) {
>  > +        i2c_bus->alert_enable = 1;
>  > +        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 = 0;
>  > +        /* Set interrupt generation of I2C master controller */
>  > +        writel(AST2600_I2CM_PKT_DONE |
> AST2600_I2CM_BUS_RECOVER,
>  > +                i2c_bus->reg_base + AST2600_I2CM_IER);
>  > +    }
>  > +
>  > +    ret = i2c_add_adapter(&i2c_bus->adap);
> 
> Maybe devm_i2c_add_adapter() + remove function simplification.
> This change + dmam_alloc_coherent() above + removing devm_free_irq()
> should, all together, not chnage the order of the remove function IIUIC.
> 
Got it, will check devm_i2c_add_adapter.

> CJ
> 
>  > +    if (ret)
>  > +        return ret;
>  > +
>  > +    dev_info(dev, "%s [%d]: adapter [%d khz] mode [%d]\n",
>  > +         dev->of_node->name, i2c_bus->adap.nr,
> i2c_bus->bus_frequency / 1000,
>  > +         i2c_bus->mode);
>  > +
>  > +    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);
>  > +    devm_free_irq(&pdev->dev, i2c_bus->irq, i2c_bus);
>  > +
>  > +#ifdef CONFIG_I2C_SLAVE
>  > +    /* for memory buffer initial */
>  > +    if (i2c_bus->mode == DMA_MODE)
>  > +        dma_free_coherent(i2c_bus->dev, I2C_SLAVE_MSG_BUF_SIZE,
>  > +                i2c_bus->slave_dma_buf, i2c_bus->slave_dma_addr);
>  > +#endif
>  > +
>  > +    return 0;
>  > +}
>  > +
>  > +static struct platform_driver ast2600_i2c_bus_driver = {
>  > +    .probe = ast2600_i2c_probe,
>  > +    .remove = ast2600_i2c_remove,
>  > +    .driver = {
>  > +        .name = KBUILD_MODNAME,
>  > +        .of_match_table = ast2600_i2c_bus_of_table,
>  > +    },
>  > +};
>  > +module_platform_driver(ast2600_i2c_bus_driver);
>  > +
>  > +MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
>  > +MODULE_DESCRIPTION("ASPEED AST2600 I2C Controller Driver");
>  > +MODULE_LICENSE("GPL");