[PATCH v4 0/2] riscv: spacemit: add i2c support to K1 SoC

[PATCH v4 0/2] riscv: spacemit: add i2c support to K1 SoC

[Troy Mitchell]

Hi all,

This patch implements I2C driver for the SpacemiT K1 SoC,
providing basic support for I2C read/write communication which
compatible with standard I2C bus specifications.

In this version, the driver defaults to use fast-speed-mode and
interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.

The docs of I2C can be found here, in chapter 16.1 I2C [1]

Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
---
Change in v4:
- Patch #1:
	- Change the default value of clock-frequency from 100000 to
	  400000. This is to correspond to the driver's default value.
	- Drop the minimum of clock-frequency
	- Modify the description of clock-frequency
- Patch #2:
	- Drop the `inline` qualifier from the `spacemit_i2c_xfer_core` function
	- Drop the initialization of `ret` to 0 in `spacemit_i2c_xfer_core` function
	- Drop useless wrap
Link to v3:
https://lore.kernel.org/all/20241112-k1-i2c-master-v3-0-5005b70dc208@gmail.com/

Change in v3:
- Patch #1:
	- Change the maxItems of reg from 2 to 1 in properties
	- Modify reg in dts example 
	- Changed the enum selection for clock-frequency to a range,
	  setting a minimum value of 1 and a maximum value of 3,300,000.
- Patch #2:
	- Drop unused judgement in `spacemit_i2c_xfer_msg`
	- Fix the dangling else warning in `spacemit_i2c_is_last_msg`
	- Fix the error check for `i2c->base`
	- Modify Kconfig dependencies
Link to v2:
https://lore.kernel.org/all/20241028053220.346283-1-TroyMitchell988@gmail.com/

Change in v2:
- Patch #1:
	- Change the maxItems of reg from 1 to 2 in properties
	- Change 'i2c' to 'I2C' in the commit message.
	- Drop fifo-disable property
	- Drop alias in dts example
	- Move `unevaluatedProperties` after `required:` block
- Patch #2:
	- Alphabetize Makefile and Kconfig
	- Change `.remove_new` to `.remove` in `struct platform_driver`
	- Change `dev_alert` to `dev_warn_ratelimited` in `spacemit_i2c_bus_reset`
	- Change `spacemit_i2c_read/write_reg` to `read/writel`
	- Change `spacemit_i2c_dt_match` to `spacemit_i2c_of_match`
	- Clean up code flow
	- Fix unnecessary line wraps
	- Move `spacemit_i2c_handle_err` to a suitable location
	- Modify Kconfig dependencies
	- Use `PTR_ERR(i2c->base)` directly as the `dev_err_probe` parameter instead of
	  the intermediate variable
Link to v1:
https://lore.kernel.org/all/20241015075134.1449458-1-TroyMitchell988@gmail.com/
---

Troy Mitchell (2):
  dt-bindings: i2c: spacemit: add support for K1 SoC
  i2c: spacemit: add support for SpacemiT K1 SoC

 .../bindings/i2c/spacemit,k1-i2c.yaml         |  51 ++
 drivers/i2c/busses/Kconfig                    |  18 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-k1.c                   | 658 ++++++++++++++++++
 4 files changed, 728 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-k1.c

--
2.34.1

---
Changes in v4:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v3: https://lore.kernel.org/r/20241112-k1-i2c-master-v3-0-5005b70dc208@gmail.com

---
Troy Mitchell (2):
      dt-bindings: i2c: spacemit: add support for K1 SoC
      i2c: spacemit: add support for SpacemiT K1 SoC

 .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   |  53 ++
 drivers/i2c/busses/Kconfig                         |  19 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-k1.c                        | 655 +++++++++++++++++++++
 4 files changed, 728 insertions(+)
---
base-commit: 8e929cb546ee42c9a61d24fae60605e9e3192354
change-id: 20241031-k1-i2c-master-fe7f7b0dce93
prerequisite-change-id: 20240626-k1-01-basic-dt-1aa31eeebcd2:v5
prerequisite-patch-id: 47dcf6861f7d434d25855b379e6d7ef4ce369c9c
prerequisite-patch-id: 77787fe82911923aff15ccf565e8fa451538c3a6
prerequisite-patch-id: b0bdb1742d96c5738f05262c3b0059102761390b
prerequisite-patch-id: 3927d39d8d77e35d5bfe53d9950da574ff8f2054
prerequisite-patch-id: a98039136a4796252a6029e474f03906f2541643
prerequisite-patch-id: c95f6dc0547a2a63a76e3cba0cf5c623b212b4e6
prerequisite-patch-id: 66e750e438ee959ddc2a6f0650814a2d8c989139
prerequisite-patch-id: 29a0fd8c36c1a4340f0d0b68a4c34d2b8abfb1ab
prerequisite-patch-id: 0bdfff661c33c380d1cf00a6c68688e05f88c0b3
prerequisite-patch-id: 99f15718e0bfbb7ed1a96dfa19f35841b004dae9

Best regards,
-- 
Troy Mitchell <TroyMitchell988@gmail.com>


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

[PATCH v4 1/2] dt-bindings: i2c: spacemit: add support for K1 SoC

[Troy Mitchell]

From: Troy Mitchell <troymitchell988@gmail.com>

The I2C of K1 supports fast-speed-mode and high-speed-mode,
and supports FIFO transmission.

Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
---
 .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
new file mode 100644
index 000000000000..e7cc62f39e28
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/spacemit,k1-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: I2C controller embedded in SpacemiT's K1 SoC
+
+maintainers:
+  - Troy Mitchell <troymitchell988@gmail.com>
+
+properties:
+  compatible:
+    const: spacemit,k1-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency:
+    description: |
+      K1 support three different modes which running different frequencies
+      standard speed mode: up to 100000 (100Hz)
+      fast speed mode    : up to 400000 (400Hz)
+      high speed mode    : up to 3300000 (3.3Mhz)
+    default: 400000
+    maximum: 3300000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@d4010800 {
+        compatible = "spacemit,k1-i2c";
+        reg = <0xd4010800 0x38>;
+        interrupt-parent = <&plic>;
+        interrupts = <36>;
+        clocks = <&ccu 90>;
+        clock-frequency = <100000>;
+    };
+
+...

-- 
2.34.1


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

[PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

From: Troy Mitchell <troymitchell988@gmail.com>

This patch introduces basic I2C support for the SpacemiT K1 SoC,
utilizing interrupts for transfers.

The driver has been tested using i2c-tools on a Bananapi-F3 board,
and basic I2C read/write operations have been confirmed to work.

Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
---
 drivers/i2c/busses/Kconfig  |  19 ++
 drivers/i2c/busses/Makefile |   1 +
 drivers/i2c/busses/i2c-k1.c | 655 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 675 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 6b3ba7e5723a..91630f55667f 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -779,6 +779,25 @@ config I2C_JZ4780
 
 	 If you don't know what to do here, say N.
 
+config I2C_K1
+	tristate "Spacemit K1 I2C adapter"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on OF
+	help
+	  This option enables support for the I2C interface on the Spacemit K1
+	  platform.
+
+	  If you enable this configuration, the kernel will include support for
+	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
+	  be used to manage I2C bus transactions, which are necessary for
+	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
+	  devices.
+
+	  This driver can also be compiled as a module. If you choose to build
+	  it as a module, the resulting kernel module will be named `i2c-k1`.
+	  Loading this module will enable the I2C functionality for the K1
+	  platform dynamically, without requiring a rebuild of the kernel.
+
 config I2C_KEBA
 	tristate "KEBA I2C controller support"
 	depends on HAS_IOMEM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index ecc07c50f2a0..9744b0e58598 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_IMX)		+= i2c-imx.o
 obj-$(CONFIG_I2C_IMX_LPI2C)	+= i2c-imx-lpi2c.o
 obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
+obj-$(CONFIG_I2C_K1)		+= i2c-k1.o
 obj-$(CONFIG_I2C_KEBA)		+= i2c-keba.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
 obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
new file mode 100644
index 000000000000..5812296bfc32
--- /dev/null
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -0,0 +1,655 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
+ */
+
+ #include <linux/clk.h>
+ #include <linux/i2c.h>
+ #include <linux/iopoll.h>
+ #include <linux/module.h>
+ #include <linux/of_address.h>
+ #include <linux/platform_device.h>
+
+/* spacemit i2c registers */
+#define ICR          0x0		/* Control Register */
+#define ISR          0x4		/* Status Register */
+#define ISAR         0x8		/* Slave Address Register */
+#define IDBR         0xc		/* Data Buffer Register */
+#define ILCR         0x10		/* Load Count Register */
+#define IWCR         0x14		/* Wait Count Register */
+#define IRST_CYC     0x18		/* Bus reset cycle counter */
+#define IBMR         0x1c		/* Bus monitor register */
+#define IWFIFO       0x20		/* Write FIFO Register */
+#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
+#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
+#define IRFIFO       0x2c		/* Read FIFO Register */
+#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
+#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */
+
+/* register ICR fields */
+#define CR_START        BIT(0)		/* start bit */
+#define CR_STOP         BIT(1)		/* stop bit */
+#define CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
+#define CR_TB           BIT(3)		/* transfer byte bit */
+#define CR_TXBEGIN      BIT(4)		/* transaction begin */
+#define CR_FIFOEN       BIT(5)		/* enable FIFO mode */
+#define CR_GPIOEN       BIT(6)		/* enable GPIO mode for SCL in HS */
+#define CR_DMAEN        BIT(7)		/* enable DMA for TX and RX FIFOs */
+#define CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
+#define CR_MODE_HIGH    BIT(9)		/* bus mode (master operation) */
+#define CR_UR           BIT(10)		/* unit reset */
+#define CR_RSTREQ       BIT(11)		/* i2c bus reset request */
+#define CR_MA           BIT(12)		/* master abort */
+#define CR_SCLE         BIT(13)		/* master clock enable */
+#define CR_IUE          BIT(14)		/* unit enable */
+#define CR_HS_STRETCH   BIT(16)		/* I2C hs stretch */
+#define CR_ALDIE        BIT(18)		/* enable arbitration interrupt */
+#define CR_DTEIE        BIT(19)		/* enable tx interrupts */
+#define CR_DRFIE        BIT(20)		/* enable rx interrupts */
+#define CR_GCD          BIT(21)		/* general call disable */
+#define CR_BEIE         BIT(22)		/* enable bus error ints */
+#define CR_SADIE        BIT(23)		/* slave address detected int enable */
+#define CR_SSDIE        BIT(24)		/* slave STOP detected int enable */
+#define CR_MSDIE        BIT(25)		/* master STOP detected int enable */
+#define CR_MSDE         BIT(26)		/* master STOP detected enable */
+#define CR_TXDONEIE     BIT(27)		/* transaction done int enable */
+#define CR_TXEIE        BIT(28)		/* transmit FIFO empty int enable */
+#define CR_RXHFIE       BIT(29)		/* receive FIFO half-full int enable */
+#define CR_RXFIE        BIT(30)		/* receive FIFO full int enable */
+#define CR_RXOVIE       BIT(31)		/* receive FIFO overrun int enable */
+
+/* register ISR fields */
+#define SR_RWM          BIT(13)		/* read/write mode */
+#define SR_ACKNAK       BIT(14)		/* ACK/NACK status */
+#define SR_UB           BIT(15)		/* unit busy */
+#define SR_IBB          BIT(16)		/* i2c bus busy */
+#define SR_EBB          BIT(17)		/* early bus busy */
+#define SR_ALD          BIT(18)		/* arbitration loss detected */
+#define SR_ITE          BIT(19)		/* tx buffer empty */
+#define SR_IRF          BIT(20)		/* rx buffer full */
+#define SR_GCAD         BIT(21)		/* general call address detected */
+#define SR_BED          BIT(22)		/* bus error no ACK/NAK */
+#define SR_SAD          BIT(23)		/* slave address detected */
+#define SR_SSD          BIT(24)		/* slave stop detected */
+#define SR_MSD          BIT(26)		/* master stop detected */
+#define SR_TXDONE       BIT(27)		/* transaction done */
+#define SR_TXE          BIT(28)		/* tx FIFO empty */
+#define SR_RXHF         BIT(29)		/* rx FIFO half-full */
+#define SR_RXF          BIT(30)		/* rx FIFO full */
+#define SR_RXOV         BIT(31)		/* RX FIFO overrun */
+
+/* register ILCR fields */
+#define LCR_SLV         0x000001FF	/* SLV: bit[8:0] */
+#define LCR_FLV         0x0003FE00	/* FLV: bit[17:9] */
+#define LCR_HLVH        0x07FC0000	/* HLVH: bit[26:18] */
+#define LCR_HLVL        0xF8000000	/* HLVL: bit[31:27] */
+
+/* register IWCR fields */
+#define WCR_COUNT       0x0000001F	/* COUNT: bit[4:0] */
+#define WCR_COUNT1      0x000003E0	/* HS_COUNT1: bit[9:5] */
+#define WCR_COUNT2      0x00007C00	/* HS_COUNT2: bit[14:10] */
+
+/* register IBMR fields */
+#define BMR_SDA         BIT(0)		/* SDA line level */
+#define BMR_SCL         BIT(1)		/* SCL line level */
+
+/* register IWFIFO fields */
+#define WFIFO_DATA_MSK      0x000000FF  /* data: bit[7:0] */
+#define WFIFO_CTRL_MSK      0x000003E0  /* control: bit[11:8] */
+#define WFIFO_CTRL_START    BIT(8)      /* start bit */
+#define WFIFO_CTRL_STOP     BIT(9)      /* stop bit */
+#define WFIFO_CTRL_ACKNAK   BIT(10)     /* send ACK(0) or NAK(1) */
+#define WFIFO_CTRL_TB       BIT(11)     /* transfer byte bit */
+
+/* status register init value */
+#define I2C_INT_STATUS_MASK    0xfffc0000  /* SR bits[31:18] */
+#define I2C_INT_CTRL_MASK      (CR_ALDIE | CR_DTEIE | CR_DRFIE | \
+				CR_BEIE | CR_TXDONEIE | CR_TXEIE | \
+				CR_RXHFIE | CR_RXFIE | CR_RXOVIE | \
+				CR_MSDIE)
+
+/* i2c bus recover timeout: us */
+#define I2C_BUS_RECOVER_TIMEOUT		(100000)
+
+#define I2C_FAST_MODE_FREQ		(400000)
+
+enum spacemit_i2c_state {
+	STATE_IDLE,
+	STATE_START,
+	STATE_READ,
+	STATE_WRITE,
+};
+
+enum spacemit_i2c_dir {
+	DIR_WRITE,
+	DIR_READ
+};
+
+/* i2c-spacemit driver's main struct */
+struct spacemit_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapt;
+
+	/* hardware resources */
+	void __iomem *base;
+	int irq;
+
+	struct i2c_msg *msgs;
+	int msg_num;
+	struct i2c_msg *cur_msg;
+
+	/* index of the current message being processed */
+	int msg_idx;
+	u8 *msg_buf;
+	/* the number of unprocessed bytes remaining in each message  */
+	size_t unprocessed;
+
+	enum spacemit_i2c_state state;
+	enum spacemit_i2c_dir dir;
+	struct completion complete;
+	u32 status;
+	u32 err;
+};
+
+static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+	writel(val | CR_IUE, i2c->base + ICR);
+}
+
+static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+	val &= ~CR_IUE;
+	writel(val, i2c->base + ICR);
+}
+
+static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
+{
+	writel(CR_UR, i2c->base + ICR);
+	udelay(5);
+	writel(0, i2c->base + ICR);
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c);
+
+static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
+{
+	u32 status;
+
+	/* if bus is locked, reset unit. 0: locked */
+	status = readl(i2c->base + IBMR);
+	if ((status & BMR_SDA) && (status & BMR_SCL))
+		return;
+
+	spacemit_i2c_reset(i2c);
+	usleep_range(10, 20);
+
+	/* check scl status again */
+	status = readl(i2c->base + IBMR);
+	if (!(status & BMR_SCL))
+		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
+}
+
+static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
+{
+	int ret = 0;
+	u32 val;
+
+	val = readl(i2c->base + ISR);
+	if (likely(!(val & (SR_UB | SR_IBB))))
+		return 0;
+
+	ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
+				 1500, I2C_BUS_RECOVER_TIMEOUT);
+	if (unlikely(ret)) {
+		spacemit_i2c_reset(i2c);
+		ret = -EAGAIN;
+	}
+
+	return ret;
+}
+
+static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
+{
+	/* in case bus is not released after transfer completes */
+	if (unlikely(readl(i2c->base + ISR) & SR_EBB)) {
+		spacemit_i2c_bus_reset(i2c);
+		usleep_range(90, 150);
+	}
+}
+
+static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
+{
+	u32 val = 0;
+
+	/*
+	 * Unmask interrupt bits for all xfer mode:
+	 * bus error, arbitration loss detected.
+	 * For transaction complete signal, we use master stop
+	 * interrupt, so we don't need to unmask CR_TXDONEIE.
+	 */
+	val |= CR_BEIE | CR_ALDIE;
+
+	/*
+	 * Unmask interrupt bits for interrupt xfer mode:
+	 * DBR rx full.
+	 * For tx empty interrupt CR_DTEIE, we only
+	 * need to enable when trigger byte transfer to start
+	 * data sending.
+	 */
+	val |= CR_DRFIE;
+
+	/* set speed bits: default fast mode */
+	val |= CR_MODE_FAST;
+
+	/* disable response to general call */
+	val |= CR_GCD;
+
+	/* enable SCL clock output */
+	val |= CR_SCLE;
+
+	/* enable master stop detected */
+	val |= CR_MSDE | CR_MSDIE;
+
+	writel(val, i2c->base + ICR);
+}
+
+static inline void
+spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
+{
+	writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
+}
+
+static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
+{
+	u32 slave_addr_rw, val;
+
+	i2c->dir = i2c->cur_msg->flags & I2C_M_RD;
+	i2c->state = STATE_START;
+
+	if (i2c->cur_msg->flags & I2C_M_RD)
+		slave_addr_rw = ((i2c->cur_msg->addr & 0x7f) << 1) | 1;
+	else
+		slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;
+
+	writel(slave_addr_rw, i2c->base + IDBR);
+
+	val = readl(i2c->base + ICR);
+
+	/* send start pulse */
+	val &= ~CR_STOP;
+	val |= CR_START | CR_TB | CR_DTEIE;
+	writel(val, i2c->base + ICR);
+}
+
+static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + ICR);
+
+	val |= CR_STOP | CR_ALDIE | CR_TB;
+
+	if (i2c->dir == DIR_READ)
+		val |= CR_ACKNAK;
+
+	writel(val, i2c->base + ICR);
+}
+
+static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
+{
+	unsigned long time_left;
+
+	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
+		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
+		i2c->msg_buf = i2c->cur_msg->buf;
+		i2c->err = 0;
+		i2c->status = 0;
+		i2c->unprocessed = i2c->cur_msg->len;
+
+		reinit_completion(&i2c->complete);
+
+		spacemit_i2c_start(i2c);
+
+		time_left = wait_for_completion_timeout(&i2c->complete,
+							i2c->adapt.timeout);
+		if (unlikely(time_left == 0)) {
+			dev_alert(i2c->dev, "msg completion timeout\n");
+			spacemit_i2c_bus_reset(i2c);
+			spacemit_i2c_reset(i2c);
+			return -ETIMEDOUT;
+		}
+
+		if (unlikely(i2c->err))
+			return spacemit_i2c_handle_err(i2c);
+	}
+
+	return 0;
+}
+
+static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->dir == DIR_READ) {
+		if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
+			return 1;
+	} else if (i2c->dir == DIR_WRITE) {
+		if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
+			return 1;
+	}
+
+	return 0;
+}
+
+static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
+{
+	/* if transfer completes, ISR will handle it */
+	if (i2c->status & SR_MSD)
+		return;
+
+	if (i2c->unprocessed) {
+		writel(*i2c->msg_buf++, i2c->base + IDBR);
+		i2c->unprocessed--;
+		return;
+	}
+
+	/* STATE_IDLE avoids trigger next byte */
+	i2c->state = STATE_IDLE;
+	complete(&i2c->complete);
+}
+
+static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->unprocessed) {
+		*i2c->msg_buf++ = readl(i2c->base + IDBR);
+		i2c->unprocessed--;
+	}
+
+	/* if transfer completes, ISR will handle it */
+	if (i2c->status & (SR_MSD | SR_ACKNAK))
+		return;
+
+	/* it has to append stop bit in icr that read last byte */
+	if (i2c->unprocessed)
+		return;
+
+	/* STATE_IDLE avoids trigger next byte */
+	i2c->state = STATE_IDLE;
+	complete(&i2c->complete);
+}
+
+static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
+{
+	if (i2c->dir == DIR_READ) {
+		i2c->state = STATE_READ;
+		return;
+	}
+
+	if (i2c->dir == DIR_WRITE) {
+		i2c->state = STATE_WRITE;
+		spacemit_i2c_handle_write(i2c);
+	}
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
+{
+	if (!i2c->err)
+		return 0;
+
+	dev_dbg(i2c->dev, "i2c error status: 0x%08x\n",	i2c->status);
+
+	if (i2c->err & (SR_BED | SR_ALD))
+		spacemit_i2c_reset(i2c);
+
+	if (i2c->err & (SR_RXOV | SR_ALD))
+		return -EAGAIN;
+
+	return (i2c->status & SR_ACKNAK) ? -ENXIO : -EIO;
+}
+
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+	/*
+	 * send transaction complete signal:
+	 * error happens, detect master stop
+	 */
+	if (likely(i2c->err || (i2c->status & SR_MSD))) {
+		/*
+		 * Here the transaction is already done, we don't need any
+		 * other interrupt signals from now, in case any interrupt
+		 * happens before spacemit_i2c_xfer to disable irq and i2c unit,
+		 * we mask all the interrupt signals and clear the interrupt
+		 * status.
+		 */
+		val = readl(i2c->base + ICR);
+		val &= ~I2C_INT_CTRL_MASK;
+		writel(val, i2c->base + ICR);
+
+		spacemit_i2c_clear_int_status(i2c, I2C_INT_STATUS_MASK);
+
+		i2c->state = STATE_IDLE;
+		complete(&i2c->complete);
+	}
+}
+
+static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
+{
+	struct spacemit_i2c_dev *i2c = devid;
+	u32 status, val;
+
+	status = readl(i2c->base + ISR);
+	if (!status)
+		return IRQ_HANDLED;
+
+	i2c->status = status;
+
+	i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);
+
+	spacemit_i2c_clear_int_status(i2c, status);
+
+	if (unlikely(i2c->err))
+		goto err_out;
+
+	val = readl(i2c->base + ICR);
+
+	val &= ~(CR_TB | CR_ACKNAK | CR_STOP | CR_START);
+	writel(val, i2c->base + ICR);
+
+	switch (i2c->state) {
+	case STATE_START:
+		spacemit_i2c_handle_start(i2c);
+		break;
+	case STATE_READ:
+		spacemit_i2c_handle_read(i2c);
+		break;
+	case STATE_WRITE:
+		spacemit_i2c_handle_write(i2c);
+		break;
+	default:
+		break;
+	}
+
+	if (i2c->state != STATE_IDLE) {
+		if (spacemit_i2c_is_last_msg(i2c)) {
+			/* trigger next byte with stop */
+			spacemit_i2c_stop(i2c);
+		} else {
+			/* trigger next byte */
+			val |= CR_ALDIE | CR_TB;
+			writel(val, i2c->base + ICR);
+		}
+	}
+
+err_out:
+	spacemit_i2c_err_check(i2c);
+	return IRQ_HANDLED;
+}
+
+static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
+{
+	unsigned long timeout;
+	int idx = 0, cnt = 0, freq;
+
+	while (idx < i2c->msg_num) {
+		cnt += (i2c->msgs + idx)->len + 1;
+		idx++;
+	}
+
+	freq = I2C_FAST_MODE_FREQ;
+	/*
+	 * multiply by 9 because each byte in I2C transmission requires
+	 * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit.
+	 */
+	timeout = cnt * 9 * USEC_PER_SEC / freq;
+
+	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num;
+}
+
+static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
+{
+	int ret;
+
+	spacemit_i2c_reset(i2c);
+
+	spacemit_i2c_calc_timeout(i2c);
+
+	spacemit_i2c_init(i2c);
+
+	spacemit_i2c_enable(i2c);
+	enable_irq(i2c->irq);
+
+	/* i2c wait for bus busy */
+	ret = spacemit_i2c_recover_bus_busy(i2c);
+	if (unlikely(ret))
+		return ret;
+
+	ret = spacemit_i2c_xfer_msg(i2c);
+	if (unlikely(ret < 0)) {
+		dev_dbg(i2c->dev, "i2c transfer error\n");
+		/* timeout error should not be overridden, and the transfer
+		 * error will be confirmed by err handle function latter,
+		 * the reset should be invalid argument error.
+		 */
+		if (ret != -ETIMEDOUT)
+			ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg msgs[], int num)
+{
+	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
+	int ret;
+
+	i2c->msgs = msgs;
+	i2c->msg_num = num;
+
+	ret = spacemit_i2c_xfer_core(i2c);
+	if (likely(!ret))
+		spacemit_i2c_check_bus_release(i2c);
+
+	disable_irq(i2c->irq);
+
+	spacemit_i2c_disable(i2c);
+
+	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
+		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, i2c->err);
+
+	return ret < 0 ? ret : num;
+}
+
+static u32 spacemit_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+}
+
+static const struct i2c_algorithm spacemit_i2c_algo = {
+	.xfer = spacemit_i2c_xfer,
+	.functionality = spacemit_i2c_func,
+};
+
+static int spacemit_i2c_probe(struct platform_device *pdev)
+{
+	struct spacemit_i2c_dev *i2c;
+	struct device_node *of_node = pdev->dev.of_node;
+	struct clk *clk;
+	int ret = 0;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->dev = &pdev->dev;
+
+	i2c->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(i2c->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->base), "failed to do ioremap");
+
+	i2c->irq = platform_get_irq(pdev, 0);
+	if (i2c->irq < 0)
+		return dev_err_probe(&pdev->dev, i2c->irq, "failed to get irq resource");
+
+	ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler,
+			       IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to request irq");
+
+	disable_irq(i2c->irq);
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to enable clock");
+
+	i2c_set_adapdata(&i2c->adapt, i2c);
+	i2c->adapt.owner = THIS_MODULE;
+	i2c->adapt.algo = &spacemit_i2c_algo;
+	i2c->adapt.dev.parent = i2c->dev;
+	i2c->adapt.nr = pdev->id;
+
+	i2c->adapt.dev.of_node = of_node;
+	i2c->adapt.algo_data = i2c;
+
+	strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
+
+	init_completion(&i2c->complete);
+
+	ret = i2c_add_numbered_adapter(&i2c->adapt);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
+
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static void spacemit_i2c_remove(struct platform_device *pdev)
+{
+	struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&i2c->adapt);
+}
+
+static const struct of_device_id spacemit_i2c_of_match[] = {
+	{ .compatible = "spacemit,k1-i2c", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match);
+
+static struct platform_driver spacemit_i2c_driver = {
+	.probe = spacemit_i2c_probe,
+	.remove = spacemit_i2c_remove,
+	.driver = {
+		.name = "i2c-k1",
+		.of_match_table = spacemit_i2c_of_match,
+	},
+};
+module_platform_driver(spacemit_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");

-- 
2.34.1


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

Re: [PATCH v4 0/2] riscv: spacemit: add i2c support to K1 SoC

[Krzysztof Kozlowski]

On 25/11/2024 07:49, Troy Mitchell wrote:
> Hi all,
> 
> This patch implements I2C driver for the SpacemiT K1 SoC,
> providing basic support for I2C read/write communication which
> compatible with standard I2C bus specifications.
> 
> In this version, the driver defaults to use fast-speed-mode and
> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
> 
> The docs of I2C can be found here, in chapter 16.1 I2C [1]
> 
> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
> ---
> Change in v4:
> - Patch #1:
> 	- Change the default value of clock-frequency from 100000 to
> 	  400000. This is to correspond to the driver's default value.
> 	- Drop the minimum of clock-frequency
> 	- Modify the description of clock-frequency

Explain why do you request re-review.

<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

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

Re: [PATCH v4 0/2] riscv: spacemit: add i2c support to K1 SoC

[Troy Mitchell]

On 2024/11/25 16:06, Krzysztof Kozlowski wrote:
> On 25/11/2024 07:49, Troy Mitchell wrote:
>> Hi all,
>>
>> This patch implements I2C driver for the SpacemiT K1 SoC,
>> providing basic support for I2C read/write communication which
>> compatible with standard I2C bus specifications.
>>
>> In this version, the driver defaults to use fast-speed-mode and
>> interrupts for transmission, and does not support DMA, high-speed mode, or FIFO.
>>
>> The docs of I2C can be found here, in chapter 16.1 I2C [1]
>>
>> Link: https://developer.spacemit.com/documentation?token=Rn9Kw3iFHirAMgkIpTAcV2Arnkf#part5 [1]
>> ---
>> Change in v4:
>> - Patch #1:
>> 	- Change the default value of clock-frequency from 100000 to
>> 	  400000. This is to correspond to the driver's default value.
>> 	- Drop the minimum of clock-frequency
>> 	- Modify the description of clock-frequency
> 
> Explain why do you request re-review.
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It looks like you received a tag and forgot to add it.
> 
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, under or above your Signed-off-by tag. Tag is "received", when
> provided in a message replied to you on the mailing list. Tools like b4
> can help here. However, there's no need to repost patches *only* to add
> the tags. The upstream maintainer will do that for tags received on the
> version they apply.
> 
> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> 
> If a tag was not added on purpose, please state why and what changed.
> </form letter>
> 
> Best regards,
> Krzysztof
Thank you, I accidentally forgot to add it, and it was not intentional.

-- 
Troy Mitchell

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

Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Andi Shyti]

Hi Troy,

sorry for having kept this patch unanswered for so long. I had a
quick look and I have a few comments through the lines.

...

> +config I2C_K1
> +	tristate "Spacemit K1 I2C adapter"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on OF
> +	help
> +	  This option enables support for the I2C interface on the Spacemit K1
> +	  platform.
> +
> +	  If you enable this configuration, the kernel will include support for
> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca

/ca/can/

> +	  be used to manage I2C bus transactions, which are necessary for
> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
> +	  devices.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
> +	  Loading this module will enable the I2C functionality for the K1
> +	  platform dynamically, without requiring a rebuild of the kernel.

This last paragraph contains more information than necessary,
please check other similar cases and keep the same format.

(E.g.: "This driver can also be built as a module.  If so, the
module will be called i2c-ali1563.". People know already what
compiling as module means :-)).

>  config I2C_KEBA
>  	tristate "KEBA I2C controller support"
>  	depends on HAS_IOMEM

...

> +/* spacemit i2c registers */
> +#define ICR          0x0		/* Control Register */
> +#define ISR          0x4		/* Status Register */
> +#define ISAR         0x8		/* Slave Address Register */
> +#define IDBR         0xc		/* Data Buffer Register */
> +#define ILCR         0x10		/* Load Count Register */
> +#define IWCR         0x14		/* Wait Count Register */
> +#define IRST_CYC     0x18		/* Bus reset cycle counter */
> +#define IBMR         0x1c		/* Bus monitor register */
> +#define IWFIFO       0x20		/* Write FIFO Register */
> +#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
> +#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
> +#define IRFIFO       0x2c		/* Read FIFO Register */
> +#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
> +#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */

Please do use a prefix for all the defines here, e.g. SPACEMINT_

...

> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> +{
> +	unsigned long time_left;
> +
> +	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
> +		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
> +		i2c->msg_buf = i2c->cur_msg->buf;
> +		i2c->err = 0;
> +		i2c->status = 0;
> +		i2c->unprocessed = i2c->cur_msg->len;
> +
> +		reinit_completion(&i2c->complete);
> +
> +		spacemit_i2c_start(i2c);
> +
> +		time_left = wait_for_completion_timeout(&i2c->complete,
> +							i2c->adapt.timeout);
> +		if (unlikely(time_left == 0)) {

no need for unlikely here.

> +			dev_alert(i2c->dev, "msg completion timeout\n");

dev_alert is a bit too much, please use dev_err instead.

> +			spacemit_i2c_bus_reset(i2c);
> +			spacemit_i2c_reset(i2c);
> +			return -ETIMEDOUT;
> +		}

...

> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +	/*
> +	 * send transaction complete signal:
> +	 * error happens, detect master stop
> +	 */
> +	if (likely(i2c->err || (i2c->status & SR_MSD))) {

I don't see a need for likely here.

> +		/*
> +		 * Here the transaction is already done, we don't need any

...

> +	ret = spacemit_i2c_xfer_msg(i2c);
> +	if (unlikely(ret < 0)) {
> +		dev_dbg(i2c->dev, "i2c transfer error\n");
> +		/* timeout error should not be overridden, and the transfer
> +		 * error will be confirmed by err handle function latter,
> +		 * the reset should be invalid argument error.
> +		 */

Please fix the commenting style (refer to
Documentation/process/coding-style.rst).

Besides, please, do not shorten words (err instead of error), we
are not in urge to save comment space. Please reword this comment
to be understood.

> +		if (ret != -ETIMEDOUT)
> +			ret = -EINVAL;

why do we need to change to -EINVAL? Doesn't seem like a good
practice

> +	}
> +
> +	return ret;
> +}

...

> +	spacemit_i2c_disable(i2c);
> +
> +	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))

is unlikely necessary? What if ret has a different value?

Andi

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

Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Alex Elder]

On 11/25/24 12:49 AM, Troy Mitchell wrote:
> From: Troy Mitchell <troymitchell988@gmail.com>
> 
> This patch introduces basic I2C support for the SpacemiT K1 SoC,
> utilizing interrupts for transfers.
> 
> The driver has been tested using i2c-tools on a Bananapi-F3 board,
> and basic I2C read/write operations have been confirmed to work.
> 
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>

This is a very detailed review.  My hope is that this helps
get this driver ready for acceptance more quickly.

> ---
>   drivers/i2c/busses/Kconfig  |  19 ++
>   drivers/i2c/busses/Makefile |   1 +
>   drivers/i2c/busses/i2c-k1.c | 655 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 675 insertions(+)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 6b3ba7e5723a..91630f55667f 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -779,6 +779,25 @@ config I2C_JZ4780
>   
>   	 If you don't know what to do here, say N.
>   
> +config I2C_K1
> +	tristate "Spacemit K1 I2C adapter"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on OF
> +	help
> +	  This option enables support for the I2C interface on the Spacemit K1
> +	  platform.
> +
> +	  If you enable this configuration, the kernel will include support for
> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
> +	  be used to manage I2C bus transactions, which are necessary for
> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
> +	  devices.
> +
> +	  This driver can also be compiled as a module. If you choose to build
> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
> +	  Loading this module will enable the I2C functionality for the K1
> +	  platform dynamically, without requiring a rebuild of the kernel.
> +
>   config I2C_KEBA
>   	tristate "KEBA I2C controller support"
>   	depends on HAS_IOMEM
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index ecc07c50f2a0..9744b0e58598 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_IMX)		+= i2c-imx.o
>   obj-$(CONFIG_I2C_IMX_LPI2C)	+= i2c-imx-lpi2c.o
>   obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
>   obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
> +obj-$(CONFIG_I2C_K1)		+= i2c-k1.o
>   obj-$(CONFIG_I2C_KEBA)		+= i2c-keba.o
>   obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
>   obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> new file mode 100644
> index 000000000000..5812296bfc32
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -0,0 +1,655 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
> + */
> +
> + #include <linux/clk.h>
> + #include <linux/i2c.h>
> + #include <linux/iopoll.h>
> + #include <linux/module.h>
> + #include <linux/of_address.h>
> + #include <linux/platform_device.h>
> +
> +/* spacemit i2c registers */

These are good register descriptions.

> +#define ICR          0x0		/* Control Register */
> +#define ISR          0x4		/* Status Register */
> +#define ISAR         0x8		/* Slave Address Register */

You never set the slave address register.  Is there any reason to?

> +#define IDBR         0xc		/* Data Buffer Register */

You never use the next three registers.  You could simplify things
by only defining them when support is added for them in the driver.

> +#define ILCR         0x10		/* Load Count Register */
> +#define IWCR         0x14		/* Wait Count Register */
> +#define IRST_CYC     0x18		/* Bus reset cycle counter */
> +#define IBMR         0x1c		/* Bus monitor register */

You do not implement any FIFO functionality currently.  I think
you can simply omit the following register definitions until
they are actually needed.

> +#define IWFIFO       0x20		/* Write FIFO Register */
> +#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
> +#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
> +#define IRFIFO       0x2c		/* Read FIFO Register */
> +#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
> +#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */
> +
> +/* register ICR fields */

The descriptions below are also good.  Maybe be consistent
with your capitalization in comments?  (Though I suppose
the capitalization above is to explain the name of the
register.)

> +#define CR_START        BIT(0)		/* start bit */
> +#define CR_STOP         BIT(1)		/* stop bit */
> +#define CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
> +#define CR_TB           BIT(3)		/* transfer byte bit */
> +#define CR_TXBEGIN      BIT(4)		/* transaction begin */
> +#define CR_FIFOEN       BIT(5)		/* enable FIFO mode */
> +#define CR_GPIOEN       BIT(6)		/* enable GPIO mode for SCL in HS */
> +#define CR_DMAEN        BIT(7)		/* enable DMA for TX and RX FIFOs */
> +#define CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
> +#define CR_MODE_HIGH    BIT(9)		/* bus mode (master operation) */
> +#define CR_UR           BIT(10)		/* unit reset */
> +#define CR_RSTREQ       BIT(11)		/* i2c bus reset request */
> +#define CR_MA           BIT(12)		/* master abort */
> +#define CR_SCLE         BIT(13)		/* master clock enable */
> +#define CR_IUE          BIT(14)		/* unit enable */

/* Bit 15 is reserved */

> +#define CR_HS_STRETCH   BIT(16)		/* I2C hs stretch */

/* Bit 17 is reserved */

Actually, why is the CURSRC_FIX_BYPASS bit (17) not defined?

For the *IE symbols that follow, why not use *_IE as the spec
does?

> +#define CR_ALDIE        BIT(18)		/* enable arbitration interrupt */
> +#define CR_DTEIE        BIT(19)		/* enable tx interrupts */
> +#define CR_DRFIE        BIT(20)		/* enable rx interrupts */
> +#define CR_GCD          BIT(21)		/* general call disable */
> +#define CR_BEIE         BIT(22)		/* enable bus error ints */
> +#define CR_SADIE        BIT(23)		/* slave address detected int enable */
> +#define CR_SSDIE        BIT(24)		/* slave STOP detected int enable */
> +#define CR_MSDIE        BIT(25)		/* master STOP detected int enable */
> +#define CR_MSDE         BIT(26)		/* master STOP detected enable */
> +#define CR_TXDONEIE     BIT(27)		/* transaction done int enable */
> +#define CR_TXEIE        BIT(28)		/* transmit FIFO empty int enable */
> +#define CR_RXHFIE       BIT(29)		/* receive FIFO half-full int enable */
> +#define CR_RXFIE        BIT(30)		/* receive FIFO full int enable */
> +#define CR_RXOVIE       BIT(31)		/* receive FIFO overrun int enable */
> +
> +/* register ISR fields */

/* Bits 0-12 are reserved */

> +#define SR_RWM          BIT(13)		/* read/write mode */
> +#define SR_ACKNAK       BIT(14)		/* ACK/NACK status */
> +#define SR_UB           BIT(15)		/* unit busy */
> +#define SR_IBB          BIT(16)		/* i2c bus busy */
> +#define SR_EBB          BIT(17)		/* early bus busy */
> +#define SR_ALD          BIT(18)		/* arbitration loss detected */
> +#define SR_ITE          BIT(19)		/* tx buffer empty */
> +#define SR_IRF          BIT(20)		/* rx buffer full */
> +#define SR_GCAD         BIT(21)		/* general call address detected */
> +#define SR_BED          BIT(22)		/* bus error no ACK/NAK */
> +#define SR_SAD          BIT(23)		/* slave address detected */
> +#define SR_SSD          BIT(24)		/* slave stop detected */

/* Bit 25 is reserved */

> +#define SR_MSD          BIT(26)		/* master stop detected */
> +#define SR_TXDONE       BIT(27)		/* transaction done */
> +#define SR_TXE          BIT(28)		/* tx FIFO empty */
> +#define SR_RXHF         BIT(29)		/* rx FIFO half-full */
> +#define SR_RXF          BIT(30)		/* rx FIFO full */
> +#define SR_RXOV         BIT(31)		/* RX FIFO overrun */
> +
> +/* register ILCR fields */

Please use GENMASK(hi, lo) to define these fields, and all other
similar fields below.

> +#define LCR_SLV         0x000001FF	/* SLV: bit[8:0] */

#define LCR_SLV		GENMASK(8, 0)	/* Standard-mode load value */

You actually never touch the ILCR register.  (Perhaps you should
do that at startup time.)  Until you do, I suggest you just remove
these definitions; they simply add space and apparent complexity
to the driver.

> +#define LCR_FLV         0x0003FE00	/* FLV: bit[17:9] */
> +#define LCR_HLVH        0x07FC0000	/* HLVH: bit[26:18] */
> +#define LCR_HLVL        0xF8000000	/* HLVL: bit[31:27] */
> +

You never touch the IWCR register, so I suggest you omit the
definition of its fields.

> +/* register IWCR fields */
> +#define WCR_COUNT       0x0000001F	/* COUNT: bit[4:0] */
> +#define WCR_COUNT1      0x000003E0	/* HS_COUNT1: bit[9:5] */
> +#define WCR_COUNT2      0x00007C00	/* HS_COUNT2: bit[14:10] */
> +
> +/* register IBMR fields */
> +#define BMR_SDA         BIT(0)		/* SDA line level */
> +#define BMR_SCL         BIT(1)		/* SCL line level */
> +

Here too, I suggest all these FIFO field definitions can be omitted.

> +/* register IWFIFO fields */
> +#define WFIFO_DATA_MSK      0x000000FF  /* data: bit[7:0] */
> +#define WFIFO_CTRL_MSK      0x000003E0  /* control: bit[11:8] */
> +#define WFIFO_CTRL_START    BIT(8)      /* start bit */
> +#define WFIFO_CTRL_STOP     BIT(9)      /* stop bit */
> +#define WFIFO_CTRL_ACKNAK   BIT(10)     /* send ACK(0) or NAK(1) */
> +#define WFIFO_CTRL_TB       BIT(11)     /* transfer byte bit */
> +
> +/* status register init value */
> +#define I2C_INT_STATUS_MASK    0xfffc0000  /* SR bits[31:18] */

Actually, this should be 0xfdff0000, because bit 25 reserved.
I would group this definition with the rest of the ISR field
definitions, above.

Above you use a hex value; below you compute the mask value
to use symbolically.  Do one or the other in both cases.

I'd group the next symbol with the ICR field definitions, above.

This describes all the interrupt-enable bits--except it omits
CR_SADIE and CR_SSDIE.  Maybe add a comment indicating this,
and explaining why.

> +#define I2C_INT_CTRL_MASK      (CR_ALDIE | CR_DTEIE | CR_DRFIE | \
> +				CR_BEIE | CR_TXDONEIE | CR_TXEIE | \
> +				CR_RXHFIE | CR_RXFIE | CR_RXOVIE | \
> +				CR_MSDIE)
> +
> +/* i2c bus recover timeout: us */

What you call "bus recover" is waiting for the bus to be free
before beginning a transfer.  I'm no I2C expert, and maybe this
is a normal term, but "bus busy" timeout would be more meaningful
for me.

> +#define I2C_BUS_RECOVER_TIMEOUT		(100000)

Parentheses are not required around simple constant values.

This is the only supported frequency, correct?  Is it
simply assumed to be already configured that way?  (This
driver doesn't set it at initialization time.)  Someday
we should set things up explicitly.

The only place the next symbol is used is in computing a
custom timeout period before initiating a transfer.

> +#define I2C_FAST_MODE_FREQ		(400000)
> +
> +enum spacemit_i2c_state {
> +	STATE_IDLE,
> +	STATE_START,
> +	STATE_READ,
> +	STATE_WRITE,
> +};
> +

Don't bother with spacemit_i2c_dir enumeration.  Just define
a Boolean flag in the spacemit_i2c_dev structure that indicates
whether the current transfer mode is read or not (read == true
means the same as dir == DIR_READ).

> +enum spacemit_i2c_dir {
> +	DIR_WRITE,
> +	DIR_READ
> +};
> +
> +/* i2c-spacemit driver's main struct */
> +struct spacemit_i2c_dev {
> +	struct device *dev;
> +	struct i2c_adapter adapt;
> +
> +	/* hardware resources */
> +	void __iomem *base;
> +	int irq;
> +
> +	struct i2c_msg *msgs;
> +	int msg_num;

I don't think there's any need to keep a pointer to the
current message.  It's always i2c->msgs[msg_idx].

> +	struct i2c_msg *cur_msg;
> +
> +	/* index of the current message being processed */
> +	int msg_idx;
> +	u8 *msg_buf;
> +	/* the number of unprocessed bytes remaining in each message  */
> +	size_t unprocessed;
> +
> +	enum spacemit_i2c_state state;
> +	enum spacemit_i2c_dir dir;
> +	struct completion complete;
> +	u32 status;
> +	u32 err;

I think you should remove the "err" field.  It only contains a
subset of what's in the status field.  It's set in the IRQ
handler:
         i2c->status = status;
         i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);
And the only other time either is assigned is when they are
both zeroed in spacemit_i2c_xfer_msg().

> +};
> +
> +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + ICR);

In every other place you do read/modify/write, you do it with
three lines.  So I suggest you do this here, for consistency:

         val = readl(i2c->base + ICR);
	val |= CR_IUE;
         writel(val, i2c->base + ICR);

> +	writel(val | CR_IUE, i2c->base + ICR);
> +}
> +
> +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + ICR);
> +	val &= ~CR_IUE;
> +	writel(val, i2c->base + ICR);
> +}
> +
> +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> +{
> +	writel(CR_UR, i2c->base + ICR);
> +	udelay(5);

The spec says you should ensure the unit is not busy before
asserting the reset.  It also says you should clear the ISR
register before clearing the ICR here.  (Apparently your code
still works; I'm just reporting what I read.)

> +	writel(0, i2c->base + ICR);
> +}
> +
> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c);

Just define i2c_handle_err() here, rather than declaring it
here and defining it later.

The next function only resets the bus if it's not idle (where
"idle" means both SCL and SDA are high).  Based on the name
of the function, I'd expect it to always reset.  In other
words, I'd rather this be named something slightly different,
or have the check of SDA and SCL be done in the caller(s).

> +static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 status;
> +
> +	/* if bus is locked, reset unit. 0: locked */
> +	status = readl(i2c->base + IBMR);
> +	if ((status & BMR_SDA) && (status & BMR_SCL))
> +		return;
> +
> +	spacemit_i2c_reset(i2c);
> +	usleep_range(10, 20);
> +
> +	/* check scl status again */
> +	status = readl(i2c->base + IBMR);
> +	if (!(status & BMR_SCL))
> +		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
> +}
> +
> +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
> +{
> +	int ret = 0;
> +	u32 val;
> +

I think the next 4 lines can be deleted.  The readl_poll_timeout()
immediately does what they do, without delay.

> +	val = readl(i2c->base + ISR);
> +	if (likely(!(val & (SR_UB | SR_IBB))))
> +		return 0;
> +
> +	ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
> +				 1500, I2C_BUS_RECOVER_TIMEOUT);
> +	if (unlikely(ret)) {
> +		spacemit_i2c_reset(i2c);

If readl_poll_timeout() returns non-zero, it is -ETIMEDOUT.
Why change it to -EAGAIN?  (It ultimately gets consumed by
spacemit_i2c_xfer(), which handles -ETIMEDOUT and -EAGAIN
identically.

> +		ret = -EAGAIN;
> +	}
> +
> +	return ret;
> +}
> +
> +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
> +{
> +	/* in case bus is not released after transfer completes */

As said by others, you essentially *never* need to specify
likely() or unlikely().  And nowhere in this code are you
running in such a performance-critical way that it matters.

> +	if (unlikely(readl(i2c->base + ISR) & SR_EBB)) {
> +		spacemit_i2c_bus_reset(i2c);
> +		usleep_range(90, 150);
> +	}
> +}
> +
> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val = 0;
> +
> +	/*
> +	 * Unmask interrupt bits for all xfer mode:
> +	 * bus error, arbitration loss detected.
> +	 * For transaction complete signal, we use master stop
> +	 * interrupt, so we don't need to unmask CR_TXDONEIE.
> +	 */
> +	val |= CR_BEIE | CR_ALDIE;

I'd just skip zeroing val above and doing this here:
	val = CR_BEIE | CR_ALDIE;
> +
> +	/*
> +	 * Unmask interrupt bits for interrupt xfer mode:
> +	 * DBR rx full.
> +	 * For tx empty interrupt CR_DTEIE, we only
> +	 * need to enable when trigger byte transfer to start
> +	 * data sending.
> +	 */
> +	val |= CR_DRFIE;
> +
> +	/* set speed bits: default fast mode */
> +	val |= CR_MODE_FAST;
> +
> +	/* disable response to general call */
> +	val |= CR_GCD;
> +
> +	/* enable SCL clock output */
> +	val |= CR_SCLE;
> +
> +	/* enable master stop detected */
> +	val |= CR_MSDE | CR_MSDIE;
> +
> +	writel(val, i2c->base + ICR);
> +}
> +

The following function is used in two places, but I don't think
it really adds any value.  Just open-code the writel() call in
those two spots.

> +static inline void
> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
> +{
> +	writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
> +}
> +
> +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 slave_addr_rw, val;

Use a local variable to track the current message.  There's
no need to stash it in the i2c structure.

	struct i2c_msg *cur_msg = i2c->msgs[i2c->msg_idx];
> +
> +	i2c->dir = i2c->cur_msg->flags & I2C_M_RD;

As suggested before, use a Boolean "read" flag instead.

	i2c->read = i2c->cur_msg->flags & I2C_M_RD;

(This doesn't assume I2C_M_RD == DIR_READ either, though
technically I2C_M_RD is guaranteed to be 1...)

> +	i2c->state = STATE_START;
> +
> +	if (i2c->cur_msg->flags & I2C_M_RD)
> +		slave_addr_rw = ((i2c->cur_msg->addr & 0x7f) << 1) | 1;
> +	else
> +		slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;

	slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;
	if (i2c->read)
		slave_addr_rw |= I2C_M_RD;

> +
> +	writel(slave_addr_rw, i2c->base + IDBR);
> +

Put the "send start pulse" comment here, then group the lines
that read/modify/write the register together.

> +	val = readl(i2c->base + ICR);
> +
> +	/* send start pulse */
> +	val &= ~CR_STOP;
> +	val |= CR_START | CR_TB | CR_DTEIE;
> +	writel(val, i2c->base + ICR);
> +}
> +
> +static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + ICR);
> +
> +	val |= CR_STOP | CR_ALDIE | CR_TB;
> +
> +	if (i2c->dir == DIR_READ)
> +		val |= CR_ACKNAK;
> +
> +	writel(val, i2c->base + ICR);
> +}
> +
> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> +{
> +	unsigned long time_left;
> +
> +	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
> +		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
> +		i2c->msg_buf = i2c->cur_msg->buf;
> +		i2c->err = 0;
> +		i2c->status = 0;
> +		i2c->unprocessed = i2c->cur_msg->len;
> +
> +		reinit_completion(&i2c->complete);
> +
> +		spacemit_i2c_start(i2c);
> +
> +		time_left = wait_for_completion_timeout(&i2c->complete,
> +							i2c->adapt.timeout);
> +		if (unlikely(time_left == 0)) {

		if (!time_left) {

> +			dev_alert(i2c->dev, "msg completion timeout\n");
> +			spacemit_i2c_bus_reset(i2c);
> +			spacemit_i2c_reset(i2c);
> +			return -ETIMEDOUT;
> +		}
> +
> +		if (unlikely(i2c->err))
> +			return spacemit_i2c_handle_err(i2c);
> +	}
> +
> +	return 0;
> +}
> +
> +static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> +{

static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
{
	if (i2c->msg_idx != i2c->msg_num - 1)
		return false;
	
	if (i2c->read)
		return i2c->unprocessed == 1;

	return !i2c->unprocessed;
}

> +	if (i2c->dir == DIR_READ) {
> +		if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
> +			return 1;
> +	} else if (i2c->dir == DIR_WRITE) {
> +		if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> +{
> +	/* if transfer completes, ISR will handle it */
> +	if (i2c->status & SR_MSD)
> +		return;
> +
> +	if (i2c->unprocessed) {
> +		writel(*i2c->msg_buf++, i2c->base + IDBR);
> +		i2c->unprocessed--;
> +		return;
> +	}
> +
> +	/* STATE_IDLE avoids trigger next byte */
> +	i2c->state = STATE_IDLE;
> +	complete(&i2c->complete);
> +}
> +
> +static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
> +{
> +	if (i2c->unprocessed) {
> +		*i2c->msg_buf++ = readl(i2c->base + IDBR);
> +		i2c->unprocessed--;
> +	}
> +
> +	/* if transfer completes, ISR will handle it */
> +	if (i2c->status & (SR_MSD | SR_ACKNAK))
> +		return;
> +
> +	/* it has to append stop bit in icr that read last byte */
> +	if (i2c->unprocessed)
> +		return;
> +
> +	/* STATE_IDLE avoids trigger next byte */
> +	i2c->state = STATE_IDLE;
> +	complete(&i2c->complete);
> +}
> +
> +static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
> +{
{
	i2c->state = i2c->read ? STATE_READ : STATE_WRITE;
	if (i2c->state == STATE_WRITE)
		spacemit_i2c_handle_write(i2c);
}

> +	if (i2c->dir == DIR_READ) {
> +		i2c->state = STATE_READ;
> +		return;
> +	}
> +
> +	if (i2c->dir == DIR_WRITE) {
> +		i2c->state = STATE_WRITE;
> +		spacemit_i2c_handle_write(i2c);
> +	}
> +}
> +
> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
> +{

This function is only called in one place, and in that place
the caller already verified that i2c->err is non-zero.  So
drop the next if statement.

> +	if (!i2c->err)
> +		return 0;
> +
> +	dev_dbg(i2c->dev, "i2c error status: 0x%08x\n",	i2c->status);
> +
> +	if (i2c->err & (SR_BED | SR_ALD))
> +		spacemit_i2c_reset(i2c);
> +
> +	if (i2c->err & (SR_RXOV | SR_ALD))
> +		return -EAGAIN;
> +
> +	return (i2c->status & SR_ACKNAK) ? -ENXIO : -EIO;

No need for parentheses in the above line.

> +}
> +
> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;

Add a blank line here.

Change the logic to:
	if (!i2c->err && !(i2c->status & SR_MSD))
		return;

Then the rest of the function can be shifted out a level.

If you get rid of i2c->err, this can be:
	u32 error = i2c->status & (SR_ALD | SR_BED | SR_MSD | SR_RXOV);

	if (!error)
		return;

> +	/*
> +	 * send transaction complete signal:
> +	 * error happens, detect master stop
> +	 */
> +	if (likely(i2c->err || (i2c->status & SR_MSD))) {
> +		/*
> +		 * Here the transaction is already done, we don't need any
> +		 * other interrupt signals from now, in case any interrupt
> +		 * happens before spacemit_i2c_xfer to disable irq and i2c unit,
> +		 * we mask all the interrupt signals and clear the interrupt
> +		 * status.
> +		 */
> +		val = readl(i2c->base + ICR);
> +		val &= ~I2C_INT_CTRL_MASK;
> +		writel(val, i2c->base + ICR);
> +

Why are *all* interrupt status bits cleared here, rather than
just the ones with bits set in i2c->status?

> +		spacemit_i2c_clear_int_status(i2c, I2C_INT_STATUS_MASK);
> +
> +		i2c->state = STATE_IDLE;
> +		complete(&i2c->complete);
> +	}
> +}
> +
> +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
> +{
> +	struct spacemit_i2c_dev *i2c = devid;
> +	u32 status, val;
> +
> +	status = readl(i2c->base + ISR);

Is there any reason not to assign i2c->status here?

> +	if (!status)
> +		return IRQ_HANDLED;
> +
> +	i2c->status = status;
> +
> +	i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);

As I said earlier, I think you can simply check these bits in
the status field rather than tracking i2c->err separately.
I think the result will actually be a little simpler.

> +
> +	spacemit_i2c_clear_int_status(i2c, status);
> +
> +	if (unlikely(i2c->err))
> +		goto err_out;
> +
> +	val = readl(i2c->base + ICR);
> +
> +	val &= ~(CR_TB | CR_ACKNAK | CR_STOP | CR_START);
> +	writel(val, i2c->base + ICR);
> +
> +	switch (i2c->state) {
> +	case STATE_START:
> +		spacemit_i2c_handle_start(i2c);
> +		break;
> +	case STATE_READ:
> +		spacemit_i2c_handle_read(i2c);
> +		break;
> +	case STATE_WRITE:
> +		spacemit_i2c_handle_write(i2c);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (i2c->state != STATE_IDLE) {
> +		if (spacemit_i2c_is_last_msg(i2c)) {
> +			/* trigger next byte with stop */
> +			spacemit_i2c_stop(i2c);
> +		} else {
> +			/* trigger next byte */
> +			val |= CR_ALDIE | CR_TB;
> +			writel(val, i2c->base + ICR);
> +		}
> +	}
> +
> +err_out:
> +	spacemit_i2c_err_check(i2c);
> +	return IRQ_HANDLED;
> +}
> +
> +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
> +{
> +	unsigned long timeout;
> +	int idx = 0, cnt = 0, freq;
> +

This loop counts the number of i2c_msg structs, and adds up
the number of bytes that must be transferred (including the
target address for each) for them all.

> +	while (idx < i2c->msg_num) {
> +		cnt += (i2c->msgs + idx)->len + 1;
> +		idx++;
> +	}
> +
> +	freq = I2C_FAST_MODE_FREQ;

You can just use I2C_FAST_MODE_FREQ below, and get rid of the
local variable.

> +	/*
> +	 * multiply by 9 because each byte in I2C transmission requires
> +	 * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit.
> +	 */
> +	timeout = cnt * 9 * USEC_PER_SEC / freq;

So now, timeout is the number of microseconds required to transfer
all bits across all messages.

The next line has me confused.

You add half a second, and convert it to jiffies.  (Why add half
a second?)

Then you divide that by the number of messages.  (Why divide by
this?  This timeout applies to the time required to transfer
all message segments in a tranasaction, right.  I.e., the time
required for a call to call spacemit_i2c_xfer()?)

> +
> +	i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) / i2c->msg_num;
> +}
> +
> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
> +{
> +	int ret;
> +
> +	spacemit_i2c_reset(i2c);
> +
> +	spacemit_i2c_calc_timeout(i2c);
> +
> +	spacemit_i2c_init(i2c);
> +
> +	spacemit_i2c_enable(i2c);
> +	enable_irq(i2c->irq);
> +
> +	/* i2c wait for bus busy */
> +	ret = spacemit_i2c_recover_bus_busy(i2c);
> +	if (unlikely(ret))
> +		return ret;
> +
> +	ret = spacemit_i2c_xfer_msg(i2c);
> +	if (unlikely(ret < 0)) {
> +		dev_dbg(i2c->dev, "i2c transfer error\n");

Put the dev_dbg() call under the next comment.

> +		/* timeout error should not be overridden, and the transfer
> +		 * error will be confirmed by err handle function latter,
> +		 * the reset should be invalid argument error.
> +		 */
> +		if (ret != -ETIMEDOUT)
> +			ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg msgs[], int num)

Why do you use msgs[] in the prototype rather than *msgs as the
definition of the i2c_algorithm structure does?

> +{
> +	struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
> +	int ret;
> +
> +	i2c->msgs = msgs;
> +	i2c->msg_num = num;
> +
> +	ret = spacemit_i2c_xfer_core(i2c);
> +	if (likely(!ret))
> +		spacemit_i2c_check_bus_release(i2c);
> +
> +	disable_irq(i2c->irq);
> +
> +	spacemit_i2c_disable(i2c);
> +
> +	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
> +		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, i2c->err);
> +
> +	return ret < 0 ? ret : num;
> +}
> +
> +static u32 spacemit_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +}
> +
> +static const struct i2c_algorithm spacemit_i2c_algo = {
> +	.xfer = spacemit_i2c_xfer,
> +	.functionality = spacemit_i2c_func,
> +};
> +
> +static int spacemit_i2c_probe(struct platform_device *pdev)
> +{
> +	struct spacemit_i2c_dev *i2c;
> +	struct device_node *of_node = pdev->dev.of_node;

	struct device *dev = &pdev->dev;

Then use the local "dev" pointer in all calls below.

> +	struct clk *clk;
> +	int ret = 0;
> +
> +	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	i2c->dev = &pdev->dev;
> +
> +	i2c->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(i2c->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(i2c->base), "failed to do ioremap");
> +
> +	i2c->irq = platform_get_irq(pdev, 0);
> +	if (i2c->irq < 0)
> +		return dev_err_probe(&pdev->dev, i2c->irq, "failed to get irq resource");
> +
> +	ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler,
> +			       IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to request irq");
> +
> +	disable_irq(i2c->irq);
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to enable clock");
> +
> +	i2c_set_adapdata(&i2c->adapt, i2c);
> +	i2c->adapt.owner = THIS_MODULE;
> +	i2c->adapt.algo = &spacemit_i2c_algo;
> +	i2c->adapt.dev.parent = i2c->dev;
> +	i2c->adapt.nr = pdev->id;
> +
> +	i2c->adapt.dev.of_node = of_node;
> +	i2c->adapt.algo_data = i2c;
> +
> +	strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
> +
> +	init_completion(&i2c->complete);
> +

Set the driver data here so it's fully initialized
before you add it.

					-Alex

> +	ret = i2c_add_numbered_adapter(&i2c->adapt);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	return 0;
> +}
> +
> +static void spacemit_i2c_remove(struct platform_device *pdev)
> +{
> +	struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adapt);
> +}
> +
> +static const struct of_device_id spacemit_i2c_of_match[] = {
> +	{ .compatible = "spacemit,k1-i2c", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match);
> +
> +static struct platform_driver spacemit_i2c_driver = {
> +	.probe = spacemit_i2c_probe,
> +	.remove = spacemit_i2c_remove,
> +	.driver = {
> +		.name = "i2c-k1",
> +		.of_match_table = spacemit_i2c_of_match,
> +	},
> +};
> +module_platform_driver(spacemit_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");
> 


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

Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

Hi! Alex.
Thanks for ur detailed review.

On 2025/2/12 05:39, Alex Elder wrote:
> On 11/25/24 12:49 AM, Troy Mitchell wrote:
>> From: Troy Mitchell <troymitchell988@gmail.com>
>>
>> This patch introduces basic I2C support for the SpacemiT K1 SoC,
>> utilizing interrupts for transfers.
>>
>> The driver has been tested using i2c-tools on a Bananapi-F3 board,
>> and basic I2C read/write operations have been confirmed to work.
>>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> 
> This is a very detailed review.  My hope is that this helps
> get this driver ready for acceptance more quickly.
Thanks. I will handle it ASAP.
> 
>> ---
>>   drivers/i2c/busses/Kconfig  |  19 ++
>>   drivers/i2c/busses/Makefile |   1 +
>>   drivers/i2c/busses/i2c-k1.c | 655 ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 675 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 6b3ba7e5723a..91630f55667f 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -779,6 +779,25 @@ config I2C_JZ4780
>>          If you don't know what to do here, say N.
>>   +config I2C_K1
>> +    tristate "Spacemit K1 I2C adapter"
>> +    depends on ARCH_SPACEMIT || COMPILE_TEST
>> +    depends on OF
>> +    help
>> +      This option enables support for the I2C interface on the Spacemit K1
>> +      platform.
>> +
>> +      If you enable this configuration, the kernel will include support for
>> +      the I2C adapter specific to the Spacemit K1 platform. This driver ca
>> +      be used to manage I2C bus transactions, which are necessary for
>> +      interfacing with I2C peripherals such as sensors, EEPROMs, and other
>> +      devices.
>> +
>> +      This driver can also be compiled as a module. If you choose to build
>> +      it as a module, the resulting kernel module will be named `i2c-k1`.
>> +      Loading this module will enable the I2C functionality for the K1
>> +      platform dynamically, without requiring a rebuild of the kernel.
>> +
>>   config I2C_KEBA
>>       tristate "KEBA I2C controller support"
>>       depends on HAS_IOMEM
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index ecc07c50f2a0..9744b0e58598 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_IMX)        += i2c-imx.o
>>   obj-$(CONFIG_I2C_IMX_LPI2C)    += i2c-imx-lpi2c.o
>>   obj-$(CONFIG_I2C_IOP3XX)    += i2c-iop3xx.o
>>   obj-$(CONFIG_I2C_JZ4780)    += i2c-jz4780.o
>> +obj-$(CONFIG_I2C_K1)        += i2c-k1.o
>>   obj-$(CONFIG_I2C_KEBA)        += i2c-keba.o
>>   obj-$(CONFIG_I2C_KEMPLD)    += i2c-kempld.o
>>   obj-$(CONFIG_I2C_LPC2K)        += i2c-lpc2k.o
>> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
>> new file mode 100644
>> index 000000000000..5812296bfc32
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-k1.c
>> @@ -0,0 +1,655 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
>> + */
>> +
>> + #include <linux/clk.h>
>> + #include <linux/i2c.h>
>> + #include <linux/iopoll.h>
>> + #include <linux/module.h>
>> + #include <linux/of_address.h>
>> + #include <linux/platform_device.h>
>> +
>> +/* spacemit i2c registers */
> 
> These are good register descriptions.
> 
>> +#define ICR          0x0        /* Control Register */
>> +#define ISR          0x4        /* Status Register */
>> +#define ISAR         0x8        /* Slave Address Register */
> 
> You never set the slave address register.  Is there any reason to?
This register is used for slave mode and currently no slave functionality is added.
I will drop it.
> 
>> +#define IDBR         0xc        /* Data Buffer Register */
> 
> You never use the next three registers.  You could simplify things
> by only defining them when support is added for them in the driver.
I will drop them.
> 
>> +#define ILCR         0x10        /* Load Count Register */
>> +#define IWCR         0x14        /* Wait Count Register */
>> +#define IRST_CYC     0x18        /* Bus reset cycle counter */
>> +#define IBMR         0x1c        /* Bus monitor register */
> 
> You do not implement any FIFO functionality currently.  I think
> you can simply omit the following register definitions until
> they are actually needed.
I will do.
> 
>> +#define IWFIFO       0x20        /* Write FIFO Register */
>> +#define IWFIFO_WPTR  0x24        /* Write FIFO Write Pointer Register */
>> +#define IWFIFO_RPTR  0x28        /* Write FIFO Read Pointer Register */
>> +#define IRFIFO       0x2c        /* Read FIFO Register */
>> +#define IRFIFO_WPTR  0x30        /* Read FIFO Write Pointer Register */
>> +#define IRFIFO_RPTR  0x34        /* Read FIFO Read Pointer Register */
>> +
>> +/* register ICR fields */
> 
> The descriptions below are also good.  Maybe be consistent
> with your capitalization in comments?  (Though I suppose
> the capitalization above is to explain the name of the
> register.)
> 
Yes. The above is to explain the full name.
So I still need to keep them consistent?
>> +#define CR_START        BIT(0)        /* start bit */
>> +#define CR_STOP         BIT(1)        /* stop bit */
>> +#define CR_ACKNAK       BIT(2)        /* send ACK(0) or NAK(1) */
>> +#define CR_TB           BIT(3)        /* transfer byte bit */
>> +#define CR_TXBEGIN      BIT(4)        /* transaction begin */
>> +#define CR_FIFOEN       BIT(5)        /* enable FIFO mode */
>> +#define CR_GPIOEN       BIT(6)        /* enable GPIO mode for SCL in HS */
>> +#define CR_DMAEN        BIT(7)        /* enable DMA for TX and RX FIFOs */
>> +#define CR_MODE_FAST    BIT(8)        /* bus mode (master operation) */
>> +#define CR_MODE_HIGH    BIT(9)        /* bus mode (master operation) */
>> +#define CR_UR           BIT(10)        /* unit reset */
>> +#define CR_RSTREQ       BIT(11)        /* i2c bus reset request */
>> +#define CR_MA           BIT(12)        /* master abort */
>> +#define CR_SCLE         BIT(13)        /* master clock enable */
>> +#define CR_IUE          BIT(14)        /* unit enable */
> 
> /* Bit 15 is reserved */
> 
>> +#define CR_HS_STRETCH   BIT(16)        /* I2C hs stretch */
> 
> /* Bit 17 is reserved */
> 
> Actually, why is the CURSRC_FIX_BYPASS bit (17) not defined?
I never use it. Like the registers above that are not used.
Are register bits different from registers? Do register bits need to be defined
even if they are not used?
> 
> For the *IE symbols that follow, why not use *_IE as the spec
> does?
The symbols follow the datasheet of SpacemiT.
Even if the data sheet doesn't have IE, should I add it myself?
> 
>> +#define CR_ALDIE        BIT(18)        /* enable arbitration interrupt */
>> +#define CR_DTEIE        BIT(19)        /* enable tx interrupts */
>> +#define CR_DRFIE        BIT(20)        /* enable rx interrupts */
>> +#define CR_GCD          BIT(21)        /* general call disable */
>> +#define CR_BEIE         BIT(22)        /* enable bus error ints */
>> +#define CR_SADIE        BIT(23)        /* slave address detected int enable */
>> +#define CR_SSDIE        BIT(24)        /* slave STOP detected int enable */
>> +#define CR_MSDIE        BIT(25)        /* master STOP detected int enable */
>> +#define CR_MSDE         BIT(26)        /* master STOP detected enable */
>> +#define CR_TXDONEIE     BIT(27)        /* transaction done int enable */
>> +#define CR_TXEIE        BIT(28)        /* transmit FIFO empty int enable */
>> +#define CR_RXHFIE       BIT(29)        /* receive FIFO half-full int enable */
>> +#define CR_RXFIE        BIT(30)        /* receive FIFO full int enable */
>> +#define CR_RXOVIE       BIT(31)        /* receive FIFO overrun int enable */
>> +
>> +/* register ISR fields */
> 
> /* Bits 0-12 are reserved */
I will add it.
> 
>> +#define SR_RWM          BIT(13)        /* read/write mode */
>> +#define SR_ACKNAK       BIT(14)        /* ACK/NACK status */
>> +#define SR_UB           BIT(15)        /* unit busy */
>> +#define SR_IBB          BIT(16)        /* i2c bus busy */
>> +#define SR_EBB          BIT(17)        /* early bus busy */
>> +#define SR_ALD          BIT(18)        /* arbitration loss detected */
>> +#define SR_ITE          BIT(19)        /* tx buffer empty */
>> +#define SR_IRF          BIT(20)        /* rx buffer full */
>> +#define SR_GCAD         BIT(21)        /* general call address detected */
>> +#define SR_BED          BIT(22)        /* bus error no ACK/NAK */
>> +#define SR_SAD          BIT(23)        /* slave address detected */
>> +#define SR_SSD          BIT(24)        /* slave stop detected */
> 
> /* Bit 25 is reserved */
I will add it.
> 
>> +#define SR_MSD          BIT(26)        /* master stop detected */
>> +#define SR_TXDONE       BIT(27)        /* transaction done */
>> +#define SR_TXE          BIT(28)        /* tx FIFO empty */
>> +#define SR_RXHF         BIT(29)        /* rx FIFO half-full */
>> +#define SR_RXF          BIT(30)        /* rx FIFO full */
>> +#define SR_RXOV         BIT(31)        /* RX FIFO overrun */
>> +
>> +/* register ILCR fields */
> 
> Please use GENMASK(hi, lo) to define these fields, and all other
> similar fields below.
> 
>> +#define LCR_SLV         0x000001FF    /* SLV: bit[8:0] */
> 
> #define LCR_SLV        GENMASK(8, 0)    /* Standard-mode load value */
> 
> You actually never touch the ILCR register.  (Perhaps you should
> do that at startup time.)  Until you do, I suggest you just remove
> these definitions; they simply add space and apparent complexity
> to the driver.
I will. Thanks.
> 
>> +#define LCR_FLV         0x0003FE00    /* FLV: bit[17:9] */
>> +#define LCR_HLVH        0x07FC0000    /* HLVH: bit[26:18] */
>> +#define LCR_HLVL        0xF8000000    /* HLVL: bit[31:27] */
>> +
> 
> You never touch the IWCR register, so I suggest you omit the
> definition of its fields.
I will.
> 
>> +/* register IWCR fields */
>> +#define WCR_COUNT       0x0000001F    /* COUNT: bit[4:0] */
>> +#define WCR_COUNT1      0x000003E0    /* HS_COUNT1: bit[9:5] */
>> +#define WCR_COUNT2      0x00007C00    /* HS_COUNT2: bit[14:10] */
>> +
>> +/* register IBMR fields */
>> +#define BMR_SDA         BIT(0)        /* SDA line level */
>> +#define BMR_SCL         BIT(1)        /* SCL line level */
>> +
> 
> Here too, I suggest all these FIFO field definitions can be omitted.
I will do.
> 
>> +/* register IWFIFO fields */
>> +#define WFIFO_DATA_MSK      0x000000FF  /* data: bit[7:0] */
>> +#define WFIFO_CTRL_MSK      0x000003E0  /* control: bit[11:8] */
>> +#define WFIFO_CTRL_START    BIT(8)      /* start bit */
>> +#define WFIFO_CTRL_STOP     BIT(9)      /* stop bit */
>> +#define WFIFO_CTRL_ACKNAK   BIT(10)     /* send ACK(0) or NAK(1) */
>> +#define WFIFO_CTRL_TB       BIT(11)     /* transfer byte bit */
>> +
>> +/* status register init value */
>> +#define I2C_INT_STATUS_MASK    0xfffc0000  /* SR bits[31:18] */
> 
> Actually, this should be 0xfdff0000, because bit 25 reserved.
> I would group this definition with the rest of the ISR field
> definitions, above.
> 
> Above you use a hex value; below you compute the mask value
> to use symbolically.  Do one or the other in both cases.
> 
> I'd group the next symbol with the ICR field definitions, above.

Makes sense. Thanks!
Very detailed.

> 
> This describes all the interrupt-enable bits--except it omits
> CR_SADIE and CR_SSDIE.  Maybe add a comment indicating this,
> and explaining why.

I think this is consistent with the above question. I would remove these two and
other unused bits to avoid complicating the driver definition.

> 
>> +#define I2C_INT_CTRL_MASK      (CR_ALDIE | CR_DTEIE | CR_DRFIE | \
>> +                CR_BEIE | CR_TXDONEIE | CR_TXEIE | \
>> +                CR_RXHFIE | CR_RXFIE | CR_RXOVIE | \
>> +                CR_MSDIE)
>> +
>> +/* i2c bus recover timeout: us */
> 
> What you call "bus recover" is waiting for the bus to be free
> before beginning a transfer.  I'm no I2C expert, and maybe this
> is a normal term, but "bus busy" timeout would be more meaningful
> for me.
makes sense.
> 
>> +#define I2C_BUS_RECOVER_TIMEOUT        (100000)
> 
> Parentheses are not required around simple constant values.
> 
> This is the only supported frequency, correct? 

That's the only frequency that driver supports now.

> Is it simply assumed to be already configured that way?  (This
> driver doesn't set it at initialization time.)  Someday
> we should set things up explicitly.
yes. by dts.
I will handle frequency that is from dts in next version.
> 
> The only place the next symbol is used is in computing a
> custom timeout period before initiating a transfer.
Yes, but I wanna explain the freq number is fast mode.
So I define it here.
But don't worry, I will drop it in next version and accept freq from dts.
> 
>> +#define I2C_FAST_MODE_FREQ        (400000)
>> +
>> +enum spacemit_i2c_state {
>> +    STATE_IDLE,
>> +    STATE_START,
>> +    STATE_READ,
>> +    STATE_WRITE,
>> +};
>> +
> 
> Don't bother with spacemit_i2c_dir enumeration.  Just define
> a Boolean flag in the spacemit_i2c_dev structure that indicates
> whether the current transfer mode is read or not (read == true
> means the same as dir == DIR_READ).
I will do.
> 
>> +enum spacemit_i2c_dir {
>> +    DIR_WRITE,
>> +    DIR_READ
>> +};
>> +
>> +/* i2c-spacemit driver's main struct */
>> +struct spacemit_i2c_dev {
>> +    struct device *dev;
>> +    struct i2c_adapter adapt;
>> +
>> +    /* hardware resources */
>> +    void __iomem *base;
>> +    int irq;
>> +
>> +    struct i2c_msg *msgs;
>> +    int msg_num;
> 
> I don't think there's any need to keep a pointer to the
> current message.  It's always i2c->msgs[msg_idx].
It is just for shorter name to avoid more line breaks...
I will drop it and use local var.
> 
>> +    struct i2c_msg *cur_msg;
>> +
>> +    /* index of the current message being processed */
>> +    int msg_idx;
>> +    u8 *msg_buf;
>> +    /* the number of unprocessed bytes remaining in each message  */
>> +    size_t unprocessed;
>> +
>> +    enum spacemit_i2c_state state;
>> +    enum spacemit_i2c_dir dir;
>> +    struct completion complete;
>> +    u32 status;
>> +    u32 err;
> 
> I think you should remove the "err" field.  It only contains a
> subset of what's in the status field.  It's set in the IRQ
> handler:
>         i2c->status = status;
>         i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);
> And the only other time either is assigned is when they are
> both zeroed in spacemit_i2c_xfer_msg().
I will.
> 
>> +};
>> +
>> +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(i2c->base + ICR);
> 
> In every other place you do read/modify/write, you do it with
> three lines.  So I suggest you do this here, for consistency:
> 
>         val = readl(i2c->base + ICR);
>     val |= CR_IUE;
>         writel(val, i2c->base + ICR);
Thanks. Your eyes are really good.
> 
>> +    writel(val | CR_IUE, i2c->base + ICR);
>> +}
>> +
>> +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(i2c->base + ICR);
>> +    val &= ~CR_IUE;
>> +    writel(val, i2c->base + ICR);
>> +}
>> +
>> +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
>> +{
>> +    writel(CR_UR, i2c->base + ICR);
>> +    udelay(5);
> 
> The spec says you should ensure the unit is not busy before
> asserting the reset.  It also says you should clear the ISR
> register before clearing the ICR here.  (Apparently your code
> still works; I'm just reporting what I read.)
> 
>> +    writel(0, i2c->base + ICR);
>> +}
>> +
>> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c);
> 
> Just define i2c_handle_err() here, rather than declaring it
> here and defining it later.
> 
> The next function only resets the bus if it's not idle (where
> "idle" means both SCL and SDA are high).  Based on the name
> of the function, I'd expect it to always reset.  In other
> words, I'd rather this be named something slightly different,
> or have the check of SDA and SCL be done in the caller(s).
How about `spacemit_i2c_conditional_reset`?
Because I am worried that in the future, when the fifo or other reset is called,
SDA and SCL will always be checked.
> 
>> +static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 status;
>> +
>> +    /* if bus is locked, reset unit. 0: locked */
>> +    status = readl(i2c->base + IBMR);
>> +    if ((status & BMR_SDA) && (status & BMR_SCL))
>> +        return;
>> +
>> +    spacemit_i2c_reset(i2c);
>> +    usleep_range(10, 20);
>> +
>> +    /* check scl status again */
>> +    status = readl(i2c->base + IBMR);
>> +    if (!(status & BMR_SCL))
>> +        dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
>> +}
>> +
>> +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
>> +{
>> +    int ret = 0;
>> +    u32 val;
>> +
> 
> I think the next 4 lines can be deleted.  The readl_poll_timeout()
> immediately does what they do, without delay.
The register value is always returned directly。
but it needs to be judged if the bus is busy by !(val & (SR_UB | SR_IBB).
> 
>> +    val = readl(i2c->base + ISR);
>> +    if (likely(!(val & (SR_UB | SR_IBB))))
>> +        return 0;
>> +
>> +    ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
>> +                 1500, I2C_BUS_RECOVER_TIMEOUT);
>> +    if (unlikely(ret)) {
>> +        spacemit_i2c_reset(i2c);
> 
> If readl_poll_timeout() returns non-zero, it is -ETIMEDOUT.
> Why change it to -EAGAIN?  (It ultimately gets consumed by
> spacemit_i2c_xfer(), which handles -ETIMEDOUT and -EAGAIN
> identically.
I will drop it.
> 
>> +        ret = -EAGAIN;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
>> +{
>> +    /* in case bus is not released after transfer completes */
> 
> As said by others, you essentially *never* need to specify
> likely() or unlikely().  And nowhere in this code are you
> running in such a performance-critical way that it matters.
I will drop them.
> 
>> +    if (unlikely(readl(i2c->base + ISR) & SR_EBB)) {
>> +        spacemit_i2c_bus_reset(i2c);
>> +        usleep_range(90, 150);
>> +    }
>> +}
>> +
>> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val = 0;
>> +
>> +    /*
>> +     * Unmask interrupt bits for all xfer mode:
>> +     * bus error, arbitration loss detected.
>> +     * For transaction complete signal, we use master stop
>> +     * interrupt, so we don't need to unmask CR_TXDONEIE.
>> +     */
>> +    val |= CR_BEIE | CR_ALDIE;
> 
> I'd just skip zeroing val above and doing this here:
>     val = CR_BEIE | CR_ALDIE;
I will. Thanks.
>> +
>> +    /*
>> +     * Unmask interrupt bits for interrupt xfer mode:
>> +     * DBR rx full.
>> +     * For tx empty interrupt CR_DTEIE, we only
>> +     * need to enable when trigger byte transfer to start
>> +     * data sending.
>> +     */
>> +    val |= CR_DRFIE;
>> +
>> +    /* set speed bits: default fast mode */
>> +    val |= CR_MODE_FAST;
>> +
>> +    /* disable response to general call */
>> +    val |= CR_GCD;
>> +
>> +    /* enable SCL clock output */
>> +    val |= CR_SCLE;
>> +
>> +    /* enable master stop detected */
>> +    val |= CR_MSDE | CR_MSDIE;
>> +
>> +    writel(val, i2c->base + ICR);
>> +}
>> +
> 
> The following function is used in two places, but I don't think
> it really adds any value.  Just open-code the writel() call in
> those two spots.
I want to explain more clearly what is done in the init phase, so I put them
into a function and assign values step by step.
Is it not necessary?
or maybe I can add `inline`.
> 
>> +static inline void
>> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
>> +{
>> +    writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
>> +}
>> +
>> +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 slave_addr_rw, val;
> 
> Use a local variable to track the current message.  There's
> no need to stash it in the i2c structure.
I will.
> 
>     struct i2c_msg *cur_msg = i2c->msgs[i2c->msg_idx];
>> +
>> +    i2c->dir = i2c->cur_msg->flags & I2C_M_RD;
> 
> As suggested before, use a Boolean "read" flag instead.
I will.
> 
>     i2c->read = i2c->cur_msg->flags & I2C_M_RD;
> 
> (This doesn't assume I2C_M_RD == DIR_READ either, though
> technically I2C_M_RD is guaranteed to be 1...>
>> +    i2c->state = STATE_START;
>> +
>> +    if (i2c->cur_msg->flags & I2C_M_RD)
>> +        slave_addr_rw = ((i2c->cur_msg->addr & 0x7f) << 1) | 1;
>> +    else
>> +        slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;
> 
>     slave_addr_rw = (i2c->cur_msg->addr & 0x7f) << 1;
>     if (i2c->read)
>         slave_addr_rw |= I2C_M_RD;
cool, I will
> 
>> +
>> +    writel(slave_addr_rw, i2c->base + IDBR);
>> +
> 
> Put the "send start pulse" comment here, then group the lines
> that read/modify/write the register together.
I will.
> 
>> +    val = readl(i2c->base + ICR);
>> +
>> +    /* send start pulse */
>> +    val &= ~CR_STOP;
>> +    val |= CR_START | CR_TB | CR_DTEIE;
>> +    writel(val, i2c->base + ICR);
>> +}
>> +
>> +static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(i2c->base + ICR);
>> +
>> +    val |= CR_STOP | CR_ALDIE | CR_TB;
>> +
>> +    if (i2c->dir == DIR_READ)
>> +        val |= CR_ACKNAK;
>> +
>> +    writel(val, i2c->base + ICR);
>> +}
>> +
>> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>> +{
>> +    unsigned long time_left;
>> +
>> +    for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
>> +        i2c->cur_msg = i2c->msgs + i2c->msg_idx;
>> +        i2c->msg_buf = i2c->cur_msg->buf;
>> +        i2c->err = 0;
>> +        i2c->status = 0;
>> +        i2c->unprocessed = i2c->cur_msg->len;
>> +
>> +        reinit_completion(&i2c->complete);
>> +
>> +        spacemit_i2c_start(i2c);
>> +
>> +        time_left = wait_for_completion_timeout(&i2c->complete,
>> +                            i2c->adapt.timeout);
>> +        if (unlikely(time_left == 0)) {
> 
>         if (!time_left) {
I will drop all unlikely/likely
> 
>> +            dev_alert(i2c->dev, "msg completion timeout\n");
>> +            spacemit_i2c_bus_reset(i2c);
>> +            spacemit_i2c_reset(i2c);
>> +            return -ETIMEDOUT;
>> +        }
>> +
>> +        if (unlikely(i2c->err))
>> +            return spacemit_i2c_handle_err(i2c);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
>> +{
> 
> static bool spacemit_i2c_is_last_msg(struct spacemit_i2c_dev *i2c)
> {
>     if (i2c->msg_idx != i2c->msg_num - 1)
>         return false;
>     
>     if (i2c->read)
>         return i2c->unprocessed == 1;
> 
>     return !i2c->unprocessed;
> }
> 
cool method. I learn a lot from ur review.
Thanks again.
>> +    if (i2c->dir == DIR_READ) {
>> +        if (i2c->unprocessed == 1 && i2c->msg_idx == i2c->msg_num - 1)
>> +            return 1;
>> +    } else if (i2c->dir == DIR_WRITE) {
>> +        if (!i2c->unprocessed && i2c->msg_idx == i2c->msg_num - 1)
>> +            return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
>> +{
>> +    /* if transfer completes, ISR will handle it */
>> +    if (i2c->status & SR_MSD)
>> +        return;
>> +
>> +    if (i2c->unprocessed) {
>> +        writel(*i2c->msg_buf++, i2c->base + IDBR);
>> +        i2c->unprocessed--;
>> +        return;
>> +    }
>> +
>> +    /* STATE_IDLE avoids trigger next byte */
>> +    i2c->state = STATE_IDLE;
>> +    complete(&i2c->complete);
>> +}
>> +
>> +static void spacemit_i2c_handle_read(struct spacemit_i2c_dev *i2c)
>> +{
>> +    if (i2c->unprocessed) {
>> +        *i2c->msg_buf++ = readl(i2c->base + IDBR);
>> +        i2c->unprocessed--;
>> +    }
>> +
>> +    /* if transfer completes, ISR will handle it */
>> +    if (i2c->status & (SR_MSD | SR_ACKNAK))
>> +        return;
>> +
>> +    /* it has to append stop bit in icr that read last byte */
>> +    if (i2c->unprocessed)
>> +        return;
>> +
>> +    /* STATE_IDLE avoids trigger next byte */
>> +    i2c->state = STATE_IDLE;
>> +    complete(&i2c->complete);
>> +}
>> +
>> +static void spacemit_i2c_handle_start(struct spacemit_i2c_dev *i2c)
>> +{
> {
>     i2c->state = i2c->read ? STATE_READ : STATE_WRITE;
>     if (i2c->state == STATE_WRITE)
>         spacemit_i2c_handle_write(i2c);
> }
> 
cool.
>> +    if (i2c->dir == DIR_READ) {
>> +        i2c->state = STATE_READ;
>> +        return;
>> +    }
>> +
>> +    if (i2c->dir == DIR_WRITE) {
>> +        i2c->state = STATE_WRITE;
>> +        spacemit_i2c_handle_write(i2c);
>> +    }
>> +}
>> +
>> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
>> +{
> 
> This function is only called in one place, and in that place
> the caller already verified that i2c->err is non-zero.  So
> drop the next if statement.
I will.
> 
>> +    if (!i2c->err)
>> +        return 0;
>> +
>> +    dev_dbg(i2c->dev, "i2c error status: 0x%08x\n",    i2c->status);
>> +
>> +    if (i2c->err & (SR_BED | SR_ALD))
>> +        spacemit_i2c_reset(i2c);
>> +
>> +    if (i2c->err & (SR_RXOV | SR_ALD))
>> +        return -EAGAIN;
>> +
>> +    return (i2c->status & SR_ACKNAK) ? -ENXIO : -EIO;
> 
> No need for parentheses in the above line.
I will drop.
> 
>> +}
>> +
>> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val;
> 
> Add a blank line here.
Thanks!
> 
> Change the logic to:
>     if (!i2c->err && !(i2c->status & SR_MSD))
>         return;
> 
> Then the rest of the function can be shifted out a level.
> 
> If you get rid of i2c->err, this can be:
>     u32 error = i2c->status & (SR_ALD | SR_BED | SR_MSD | SR_RXOV);
> 
>     if (!error)
>         return;
> 
That's so detailed.
Sry for my newbie behavior
>> +    /*
>> +     * send transaction complete signal:
>> +     * error happens, detect master stop
>> +     */
>> +    if (likely(i2c->err || (i2c->status & SR_MSD))) {
>> +        /*
>> +         * Here the transaction is already done, we don't need any
>> +         * other interrupt signals from now, in case any interrupt
>> +         * happens before spacemit_i2c_xfer to disable irq and i2c unit,
>> +         * we mask all the interrupt signals and clear the interrupt
>> +         * status.
>> +         */
>> +        val = readl(i2c->base + ICR);
>> +        val &= ~I2C_INT_CTRL_MASK;
>> +        writel(val, i2c->base + ICR);
>> +
> 
> Why are *all* interrupt status bits cleared here, rather than
> just the ones with bits set in i2c->status?
Because if there are any errors, we don't need interrupts to avoid further
issues. just exit.

> 
>> +        spacemit_i2c_clear_int_status(i2c, I2C_INT_STATUS_MASK);
>> +
>> +        i2c->state = STATE_IDLE;
>> +        complete(&i2c->complete);
>> +    }
>> +}
>> +
>> +static irqreturn_t spacemit_i2c_irq_handler(int irq, void *devid)
>> +{
>> +    struct spacemit_i2c_dev *i2c = devid;
>> +    u32 status, val;
>> +
>> +    status = readl(i2c->base + ISR);
> 
> Is there any reason not to assign i2c->status here?
actualy to avoid zero.
But I think there is not reason to avoid zero.
I will drop local var.
> 
>> +    if (!status)
>> +        return IRQ_HANDLED;
>> +
>> +    i2c->status = status;
>> +
>> +    i2c->err = status & (SR_BED | SR_RXOV | SR_ALD);
> 
> As I said earlier, I think you can simply check these bits in
> the status field rather than tracking i2c->err separately.
> I think the result will actually be a little simpler.
ok.
> 
>> +
>> +    spacemit_i2c_clear_int_status(i2c, status);
>> +
>> +    if (unlikely(i2c->err))
>> +        goto err_out;
>> +
>> +    val = readl(i2c->base + ICR);
>> +
>> +    val &= ~(CR_TB | CR_ACKNAK | CR_STOP | CR_START);
>> +    writel(val, i2c->base + ICR);
>> +
>> +    switch (i2c->state) {
>> +    case STATE_START:
>> +        spacemit_i2c_handle_start(i2c);
>> +        break;
>> +    case STATE_READ:
>> +        spacemit_i2c_handle_read(i2c);
>> +        break;
>> +    case STATE_WRITE:
>> +        spacemit_i2c_handle_write(i2c);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    if (i2c->state != STATE_IDLE) {
>> +        if (spacemit_i2c_is_last_msg(i2c)) {
>> +            /* trigger next byte with stop */
>> +            spacemit_i2c_stop(i2c);
>> +        } else {
>> +            /* trigger next byte */
>> +            val |= CR_ALDIE | CR_TB;
>> +            writel(val, i2c->base + ICR);
>> +        }
>> +    }
>> +
>> +err_out:
>> +    spacemit_i2c_err_check(i2c);
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static void spacemit_i2c_calc_timeout(struct spacemit_i2c_dev *i2c)
>> +{
>> +    unsigned long timeout;
>> +    int idx = 0, cnt = 0, freq;
>> +
> 
> This loop counts the number of i2c_msg structs, and adds up
> the number of bytes that must be transferred (including the
> target address for each) for them all.>
>> +    while (idx < i2c->msg_num) {
>> +        cnt += (i2c->msgs + idx)->len + 1;
>> +        idx++;
>> +    }
>> +
>> +    freq = I2C_FAST_MODE_FREQ;
> 
> You can just use I2C_FAST_MODE_FREQ below, and get rid of the
> local variable.
In this version, we can use I2C_FAST_MODE_FREQ.
but as I said earlier, I will read freq from dts.
so I will add i2c->freq to storage and modify this function.
> 
>> +    /*
>> +     * multiply by 9 because each byte in I2C transmission requires
>> +     * 9 clock cycles: 8 bits of data plus 1 ACK/NACK bit.
>> +     */
>> +    timeout = cnt * 9 * USEC_PER_SEC / freq;
> 
> So now, timeout is the number of microseconds required to transfer
> all bits across all messages.
> 
> The next line has me confused.
> 
> You add half a second, and convert it to jiffies.  (Why add half
> a second?)
half a second is buffer time to avoid extremely low probability events.
> 
> Then you divide that by the number of messages.  (Why divide by
> this?  This timeout applies to the time required to transfer
> all message segments in a tranasaction, right.  I.e., the time
> required for a call to call spacemit_i2c_xfer()?)
just one msg instead of all msgs.
so that's why we divide that by the number of msgs.
and half a second is equally divided among each message.
> 
>> +
>> +    i2c->adapt.timeout = usecs_to_jiffies(timeout + USEC_PER_SEC / 2) /
>> i2c->msg_num;
>> +}
>> +
>> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
>> +{
>> +    int ret;
>> +
>> +    spacemit_i2c_reset(i2c);
>> +
>> +    spacemit_i2c_calc_timeout(i2c);
>> +
>> +    spacemit_i2c_init(i2c);
>> +
>> +    spacemit_i2c_enable(i2c);
>> +    enable_irq(i2c->irq);
>> +
>> +    /* i2c wait for bus busy */
>> +    ret = spacemit_i2c_recover_bus_busy(i2c);
>> +    if (unlikely(ret))
>> +        return ret;
>> +
>> +    ret = spacemit_i2c_xfer_msg(i2c);
>> +    if (unlikely(ret < 0)) {
>> +        dev_dbg(i2c->dev, "i2c transfer error\n");
> 
> Put the dev_dbg() call under the next comment.
I will.
> 
>> +        /* timeout error should not be overridden, and the transfer
>> +         * error will be confirmed by err handle function latter,
>> +         * the reset should be invalid argument error.
>> +         */
>> +        if (ret != -ETIMEDOUT)
>> +            ret = -EINVAL;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int spacemit_i2c_xfer(struct i2c_adapter *adapt, struct i2c_msg
>> msgs[], int num)
> 
> Why do you use msgs[] in the prototype rather than *msgs as the
> definition of the i2c_algorithm structure does?
I will.
> 
>> +{
>> +    struct spacemit_i2c_dev *i2c = i2c_get_adapdata(adapt);
>> +    int ret;
>> +
>> +    i2c->msgs = msgs;
>> +    i2c->msg_num = num;
>> +
>> +    ret = spacemit_i2c_xfer_core(i2c);
>> +    if (likely(!ret))
>> +        spacemit_i2c_check_bus_release(i2c);
>> +
>> +    disable_irq(i2c->irq);
>> +
>> +    spacemit_i2c_disable(i2c);
>> +
>> +    if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
>> +        dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret,
>> i2c->err);
>> +
>> +    return ret < 0 ? ret : num;
>> +}
>> +
>> +static u32 spacemit_i2c_func(struct i2c_adapter *adap)
>> +{
>> +    return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
>> +}
>> +
>> +static const struct i2c_algorithm spacemit_i2c_algo = {
>> +    .xfer = spacemit_i2c_xfer,
>> +    .functionality = spacemit_i2c_func,
>> +};
>> +
>> +static int spacemit_i2c_probe(struct platform_device *pdev)
>> +{
>> +    struct spacemit_i2c_dev *i2c;
>> +    struct device_node *of_node = pdev->dev.of_node;
> 
>     struct device *dev = &pdev->dev;
> 
> Then use the local "dev" pointer in all calls below.
I will.
> 
>> +    struct clk *clk;
>> +    int ret = 0;
>> +
>> +    i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
>> +    if (!i2c)
>> +        return -ENOMEM;
>> +
>> +    i2c->dev = &pdev->dev;
>> +
>> +    i2c->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(i2c->base))
>> +        return dev_err_probe(&pdev->dev, PTR_ERR(i2c->base), "failed to do
>> ioremap");
>> +
>> +    i2c->irq = platform_get_irq(pdev, 0);
>> +    if (i2c->irq < 0)
>> +        return dev_err_probe(&pdev->dev, i2c->irq, "failed to get irq
>> resource");
>> +
>> +    ret = devm_request_irq(i2c->dev, i2c->irq, spacemit_i2c_irq_handler,
>> +                   IRQF_NO_SUSPEND | IRQF_ONESHOT, dev_name(i2c->dev), i2c);
>> +    if (ret)
>> +        return dev_err_probe(&pdev->dev, ret, "failed to request irq");
>> +
>> +    disable_irq(i2c->irq);
>> +
>> +    clk = devm_clk_get_enabled(&pdev->dev, NULL);
>> +    if (IS_ERR(clk))
>> +        return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to enable
>> clock");
>> +
>> +    i2c_set_adapdata(&i2c->adapt, i2c);
>> +    i2c->adapt.owner = THIS_MODULE;
>> +    i2c->adapt.algo = &spacemit_i2c_algo;
>> +    i2c->adapt.dev.parent = i2c->dev;
>> +    i2c->adapt.nr = pdev->id;
>> +
>> +    i2c->adapt.dev.of_node = of_node;
>> +    i2c->adapt.algo_data = i2c;
>> +
>> +    strscpy(i2c->adapt.name, "spacemit-i2c-adapter", sizeof(i2c->adapt.name));
>> +
>> +    init_completion(&i2c->complete);
>> +
> 
> Set the driver data here so it's fully initialized
> before you add it.
I will.
> 
>                     -Alex
Thank you very much Alex for giving me very detailed instructions.
> 
>> +    ret = i2c_add_numbered_adapter(&i2c->adapt);
>> +    if (ret)
>> +        return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
>> +
>> +    platform_set_drvdata(pdev, i2c);
>> +
>> +    return 0;
>> +}
>> +
>> +static void spacemit_i2c_remove(struct platform_device *pdev)
>> +{
>> +    struct spacemit_i2c_dev *i2c = platform_get_drvdata(pdev);
>> +
>> +    i2c_del_adapter(&i2c->adapt);
>> +}
>> +
>> +static const struct of_device_id spacemit_i2c_of_match[] = {
>> +    { .compatible = "spacemit,k1-i2c", },
>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, spacemit_i2c_of_match);
>> +
>> +static struct platform_driver spacemit_i2c_driver = {
>> +    .probe = spacemit_i2c_probe,
>> +    .remove = spacemit_i2c_remove,
>> +    .driver = {
>> +        .name = "i2c-k1",
>> +        .of_match_table = spacemit_i2c_of_match,
>> +    },
>> +};
>> +module_platform_driver(spacemit_i2c_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("I2C bus driver for SpacemiT K1 SoC");
>>
> 

-- 
Troy Mitchell

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

Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Troy Mitchell]

On 2025/1/4 08:34, Andi Shyti wrote:
> Hi Troy,
> 
> sorry for having kept this patch unanswered for so long. I had a
> quick look and I have a few comments through the lines.>
> ...
> 
>> +config I2C_K1
>> +	tristate "Spacemit K1 I2C adapter"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on OF
>> +	help
>> +	  This option enables support for the I2C interface on the Spacemit K1
>> +	  platform.
>> +
>> +	  If you enable this configuration, the kernel will include support for
>> +	  the I2C adapter specific to the Spacemit K1 platform. This driver ca
> 
> /ca/can/
I will
> 
>> +	  be used to manage I2C bus transactions, which are necessary for
>> +	  interfacing with I2C peripherals such as sensors, EEPROMs, and other
>> +	  devices.
>> +
>> +	  This driver can also be compiled as a module. If you choose to build
>> +	  it as a module, the resulting kernel module will be named `i2c-k1`.
>> +	  Loading this module will enable the I2C functionality for the K1
>> +	  platform dynamically, without requiring a rebuild of the kernel.
> 
> This last paragraph contains more information than necessary,
> please check other similar cases and keep the same format.
> 
> (E.g.: "This driver can also be built as a module.  If so, the
> module will be called i2c-ali1563.". People know already what
> compiling as module means :-)).
thanks :)
> 
>>  config I2C_KEBA
>>  	tristate "KEBA I2C controller support"
>>  	depends on HAS_IOMEM
> 
> ...
> 
>> +/* spacemit i2c registers */
>> +#define ICR          0x0		/* Control Register */
>> +#define ISR          0x4		/* Status Register */
>> +#define ISAR         0x8		/* Slave Address Register */
>> +#define IDBR         0xc		/* Data Buffer Register */
>> +#define ILCR         0x10		/* Load Count Register */
>> +#define IWCR         0x14		/* Wait Count Register */
>> +#define IRST_CYC     0x18		/* Bus reset cycle counter */
>> +#define IBMR         0x1c		/* Bus monitor register */
>> +#define IWFIFO       0x20		/* Write FIFO Register */
>> +#define IWFIFO_WPTR  0x24		/* Write FIFO Write Pointer Register */
>> +#define IWFIFO_RPTR  0x28		/* Write FIFO Read Pointer Register */
>> +#define IRFIFO       0x2c		/* Read FIFO Register */
>> +#define IRFIFO_WPTR  0x30		/* Read FIFO Write Pointer Register */
>> +#define IRFIFO_RPTR  0x34		/* Read FIFO Read Pointer Register */
> 
> Please do use a prefix for all the defines here, e.g. SPACEMINT_
I will.
> 
> ...
> 
>> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
>> +{
>> +	unsigned long time_left;
>> +
>> +	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
>> +		i2c->cur_msg = i2c->msgs + i2c->msg_idx;
>> +		i2c->msg_buf = i2c->cur_msg->buf;
>> +		i2c->err = 0;
>> +		i2c->status = 0;
>> +		i2c->unprocessed = i2c->cur_msg->len;
>> +
>> +		reinit_completion(&i2c->complete);
>> +
>> +		spacemit_i2c_start(i2c);
>> +
>> +		time_left = wait_for_completion_timeout(&i2c->complete,
>> +							i2c->adapt.timeout);
>> +		if (unlikely(time_left == 0)) {
> 
> no need for unlikely here.
I will drop all unlikely.
> 
>> +			dev_alert(i2c->dev, "msg completion timeout\n");
> 
> dev_alert is a bit too much, please use dev_err instead.
ok.
> 
>> +			spacemit_i2c_bus_reset(i2c);
>> +			spacemit_i2c_reset(i2c);
>> +			return -ETIMEDOUT;
>> +		}
> 
> ...
> 
>> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>> +{
>> +	u32 val;
>> +	/*
>> +	 * send transaction complete signal:
>> +	 * error happens, detect master stop
>> +	 */
>> +	if (likely(i2c->err || (i2c->status & SR_MSD))) {
> 
> I don't see a need for likely here.
> 
>> +		/*
>> +		 * Here the transaction is already done, we don't need any
> 
> ...
> 
>> +	ret = spacemit_i2c_xfer_msg(i2c);
>> +	if (unlikely(ret < 0)) {
>> +		dev_dbg(i2c->dev, "i2c transfer error\n");
>> +		/* timeout error should not be overridden, and the transfer
>> +		 * error will be confirmed by err handle function latter,
>> +		 * the reset should be invalid argument error.
>> +		 */
> 
> Please fix the commenting style (refer to
> Documentation/process/coding-style.rst).
> 
> Besides, please, do not shorten words (err instead of error), we
> are not in urge to save comment space. Please reword this comment
> to be understood.
I will.
> 
>> +		if (ret != -ETIMEDOUT)
>> +			ret = -EINVAL;
> 
> why do we need to change to -EINVAL? Doesn't seem like a good
> practice
I will fix it.
> 
>> +	}
>> +
>> +	return ret;
>> +}
> 
> ...
> 
>> +	spacemit_i2c_disable(i2c);
>> +
>> +	if (unlikely((ret == -ETIMEDOUT || ret == -EAGAIN)))
> 
> is unlikely necessary? What if ret has a different value?
I will drop all unlikely/likely. It's not necessary.
> 
> Andi
Thank you Andi.

-- 
Troy Mitchell

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

Re: [PATCH v4 2/2] i2c: spacemit: add support for SpacemiT K1 SoC

[Alex Elder]

On 2/13/25 5:38 AM, Troy Mitchell wrote:
> Hi! Alex.
> Thanks for ur detailed review.

Troy, I see you've sent a new version, and I'm going to
provide a review for it today.  But I somehow missed
this message, and it looks like I should have responded.

I know it's late, but I'm doing that now anyway.

> On 2025/2/12 05:39, Alex Elder wrote:
>> On 11/25/24 12:49 AM, Troy Mitchell wrote:
>>> From: Troy Mitchell <troymitchell988@gmail.com>
>>>
>>> This patch introduces basic I2C support for the SpacemiT K1 SoC,
>>> utilizing interrupts for transfers.
>>>
>>> The driver has been tested using i2c-tools on a Bananapi-F3 board,
>>> and basic I2C read/write operations have been confirmed to work.
>>>
>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>

. . .

>> The descriptions below are also good.  Maybe be consistent
>> with your capitalization in comments?  (Though I suppose
>> the capitalization above is to explain the name of the
>> register.)
>>
> Yes. The above is to explain the full name.
> So I still need to keep them consistent?

You'll never get everything consistent, and even getting
two people to agree on what things should be consistent
might be difficult.

I think what you have is OK, but I might (for example)
capitalize every comment (or *not* capitalize).

Not a big deal.

>>> +#define CR_START        BIT(0)        /* start bit */
>>> +#define CR_STOP         BIT(1)        /* stop bit */

. . .

>> For the *IE symbols that follow, why not use *_IE as the spec
>> does?
> The symbols follow the datasheet of SpacemiT.
> Even if the data sheet doesn't have IE, should I add it myself?

I think it's good to use naming that matches what the hardware
specifications say.  That said, it doesn't always work well.
I worked on a driver where most of the hardware names were
30 or more characters wide.  That leads to unreadable code.

I would *much* rather have code readability than have the
symbol names match the hardware spec.  So I personally
focus on that, and just try to ensure it's easy to map
the name in the code back to what it represents in the
hardware.

>>> +#define CR_ALDIE        BIT(18)        /* enable arbitration interrupt */
>>> +#define CR_DTEIE        BIT(19)        /* enable tx interrupts */
>>> +#define CR_DRFIE        BIT(20)        /* enable rx interrupts */

. . .

>> Is it simply assumed to be already configured that way?  (This
>> driver doesn't set it at initialization time.)  Someday
>> we should set things up explicitly.
> yes. by dts.
> I will handle frequency that is from dts in next version.

In general it is a bad idea to add new features between
versions of a patch series--unless specifically requested
by a reviewer.  Once the series is accepted it should be easy
to add features later.

>> The only place the next symbol is used is in computing a
>> custom timeout period before initiating a transfer.
> Yes, but I wanna explain the freq number is fast mode.
> So I define it here.
> But don't worry, I will drop it in next version and accept freq from dts.

. . .

>> The next function only resets the bus if it's not idle (where
>> "idle" means both SCL and SDA are high).  Based on the name
>> of the function, I'd expect it to always reset.  In other
>> words, I'd rather this be named something slightly different,
>> or have the check of SDA and SCL be done in the caller(s).
> How about `spacemit_i2c_conditional_reset`?

That name is fine, though I think I'll be commenting on it
in my review shortly.

> Because I am worried that in the future, when the fifo or other reset is called,
> SDA and SCL will always be checked.

If you add a feature in the future that makes the current code not
make sense, you can change the code *then* to be more suitable.
Don't get too far ahead of yourself.

Write code that does the job, and is readable and simple.

>>> +static void spacemit_i2c_bus_reset(struct spacemit_i2c_dev *i2c)
>>> +{
>>> +    u32 status;
>>> +
>>> +    /* if bus is locked, reset unit. 0: locked */
>>> +    status = readl(i2c->base + IBMR);
>>> +    if ((status & BMR_SDA) && (status & BMR_SCL))
>>> +        return;
>>> +
>>> +    spacemit_i2c_reset(i2c);
>>> +    usleep_range(10, 20);
>>> +
>>> +    /* check scl status again */
>>> +    status = readl(i2c->base + IBMR);
>>> +    if (!(status & BMR_SCL))
>>> +        dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
>>> +}
>>> +
>>> +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
>>> +{
>>> +    int ret = 0;
>>> +    u32 val;
>>> +
>>
>> I think the next 4 lines can be deleted.  The readl_poll_timeout()
>> immediately does what they do, without delay.
> The register value is always returned directly。
> but it needs to be judged if the bus is busy by !(val & (SR_UB | SR_IBB).

I might be wrong, but my point was that the readl_poll_timeout()
call does exactly what you're doing here.  So you could simply
do the readl_poll_timeout() without doing this readl() call first.

>>
>>> +    val = readl(i2c->base + ISR);
>>> +    if (likely(!(val & (SR_UB | SR_IBB))))
>>> +        return 0;
>>> +
>>> +    ret = readl_poll_timeout(i2c->base + ISR, val, !(val & (SR_UB | SR_IBB)),
>>> +                 1500, I2C_BUS_RECOVER_TIMEOUT);
>>> +    if (unlikely(ret)) {
>>> +        spacemit_i2c_reset(i2c);
>>
>> If readl_poll_timeout() returns non-zero, it is -ETIMEDOUT.
>> Why change it to -EAGAIN?  (It ultimately gets consumed by
>> spacemit_i2c_xfer(), which handles -ETIMEDOUT and -EAGAIN
>> identically.
> I will drop it.

. . .

>> The following function is used in two places, but I don't think
>> it really adds any value.  Just open-code the writel() call in
>> those two spots.
> I want to explain more clearly what is done in the init phase, so I put them
> into a function and assign values step by step.
> Is it not necessary?
> or maybe I can add `inline`.

No.  People accustomed to reading kernel code know exactly
what a call to writel() does.  When you wrap that in a
function like this, it seems like you might be doing
something more complicated than that--and it's distracting.

It is best to simply use writel() (in this case) and get
rid of this trivial wrapper function.  Rely on well-named
variables to make things clearer, and if you think there's
something more, say so in a comment.

Also, much like the likely() and unlikely() calls, it is
almost never warranted to use an inline function in a C
file.  There are some cases where it serves a purpose,
but in general you can depend on the compiler to do the
inlining if it improves the way the code executes.

>>> +static inline void
>>> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
>>> +{
>>> +    writel(mask & I2C_INT_STATUS_MASK, i2c->base + ISR);
>>> +}

. . .

That's enough.  I'm going to review your new version now.

					-Alex

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