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

[PATCH RESEND v5 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]
---
I'm sorry for forgetting to CC Yixun Lan and SpacemiT, and for not syncing the Kconfig.
This re-send also corrects the warning mentioned in the CI test.
That's why I'm resending this.
---
Change in v5:
- Path #1:
        - Add `clock-names` property
        - Modify the clock property into two
- Path #2:
        - Enable the APB clock
        - Fix comment and code styles
        - Fix typo and drop unnecessary description in Kconfig
        - Prefix all macro definitions with SPACEMIT_
        - Rename `spacemit_i2c_bus_reset` to `spacemit_i2c_conditionally_reset_bus`
        - Remove all `unlikely` and `likely`
        - Remove unused register and bit macros
        - Remove the "err" field, as it only contains a subset of the status field
        - Retrieve `clock-frequency` from the device tree instead of using a macro
        - Use a local variable to track the current message
        - Use `i2c->read` to represent read and write statuses instead of `i2c->dir`
Link to v4:
https://lore.kernel.org/all/20241125-k1-i2c-master-v4-0-0f3d5886336b@gmail.com/

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

 .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   |  59 ++
 drivers/i2c/busses/Kconfig                         |  17 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-k1.c                        | 617 +++++++++++++++++++++
 4 files changed, 694 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>

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

[Troy Mitchell]

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

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
---
 .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
 1 file changed, 59 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -0,0 +1,59 @@
+# 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:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    minItems: 2
+    maxItems: 2
+
+  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 176>, <&ccu 90>;
+        clock-names = "apb", "twsi";
+        clock-frequency = <100000>;
+    };
+
+...

-- 
2.34.1

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

[Troy Mitchell]

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  |  17 ++
 drivers/i2c/busses/Makefile |   1 +
 drivers/i2c/busses/i2c-k1.c | 617 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 635 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -783,6 +783,23 @@ 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 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 built as a module.  If so, the
+	  module will be called `i2c-k1`.
+
 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 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -74,6 +74,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 0000000000000000000000000000000000000000..6abe05640782dfa93a15d130c58ac879a423e061
--- /dev/null
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -0,0 +1,617 @@
+// 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 SPACEMIT_ICR		 0x0		/* Control Register */
+#define SPACEMIT_ISR		 0x4		/* Status Register */
+#define SPACEMIT_IDBR		 0xc		/* Data Buffer Register */
+#define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
+
+/* register SPACEMIT_ICR fields */
+#define SPACEMIT_CR_START        BIT(0)		/* start bit */
+#define SPACEMIT_CR_STOP         BIT(1)		/* stop bit */
+#define SPACEMIT_CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
+#define SPACEMIT_CR_TB           BIT(3)		/* transfer byte bit */
+/* Bit 4-7 are reserved */
+#define SPACEMIT_CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
+/* Bit 9 is reserved */
+#define SPACEMIT_CR_UR           BIT(10)	/* unit reset */
+/* Bit 11-12 are reserved */
+#define SPACEMIT_CR_SCLE         BIT(13)	/* master clock enable */
+#define SPACEMIT_CR_IUE          BIT(14)	/* unit enable */
+/* Bit 15-17 are reserved */
+#define SPACEMIT_CR_ALDIE        BIT(18)	/* enable arbitration interrupt */
+#define SPACEMIT_CR_DTEIE        BIT(19)	/* enable tx interrupts */
+#define SPACEMIT_CR_DRFIE        BIT(20)	/* enable rx interrupts */
+#define SPACEMIT_CR_GCD          BIT(21)	/* general call disable */
+#define SPACEMIT_CR_BEIE         BIT(22)	/* enable bus error ints */
+/* Bit 23-24 are reserved */
+#define SPACEMIT_CR_MSDIE        BIT(25)	/* master STOP detected int enable */
+#define SPACEMIT_CR_MSDE         BIT(26)	/* master STOP detected enable */
+#define SPACEMIT_CR_TXDONEIE     BIT(27)	/* transaction done int enable */
+#define SPACEMIT_CR_TXEIE        BIT(28)	/* transmit FIFO empty int enable */
+#define SPACEMIT_CR_RXHFIE       BIT(29)	/* receive FIFO half-full int enable */
+#define SPACEMIT_CR_RXFIE        BIT(30)	/* receive FIFO full int enable */
+#define SPACEMIT_CR_RXOVIE       BIT(31)	/* receive FIFO overrun int enable */
+
+#define SPACEMIT_I2C_INT_CTRL_MASK	(SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \
+					SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \
+					SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \
+					SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \
+					SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE)
+
+/* register SPACEMIT_ISR fields */
+#define SPACEMIT_SR_ACKNAK       BIT(14)	/* ACK/NACK status */
+#define SPACEMIT_SR_UB           BIT(15)	/* unit busy */
+#define SPACEMIT_SR_IBB          BIT(16)	/* i2c bus busy */
+#define SPACEMIT_SR_EBB          BIT(17)	/* early bus busy */
+#define SPACEMIT_SR_ALD          BIT(18)	/* arbitration loss detected */
+#define SPACEMIT_SR_ITE          BIT(19)	/* tx buffer empty */
+#define SPACEMIT_SR_IRF          BIT(20)	/* rx buffer full */
+#define SPACEMIT_SR_GCAD         BIT(21)	/* general call address detected */
+#define SPACEMIT_SR_BED          BIT(22)	/* bus error no ACK/NAK */
+#define SPACEMIT_SR_SAD          BIT(23)	/* slave address detected */
+#define SPACEMIT_SR_SSD          BIT(24)	/* slave stop detected */
+/* Bit 25 is reserved */
+#define SPACEMIT_SR_MSD          BIT(26)	/* master stop detected */
+#define SPACEMIT_SR_TXDONE       BIT(27)	/* transaction done */
+#define SPACEMIT_SR_TXE          BIT(28)	/* tx FIFO empty */
+#define SPACEMIT_SR_RXHF         BIT(29)	/* rx FIFO half-full */
+#define SPACEMIT_SR_RXF          BIT(30)	/* rx FIFO full */
+#define SPACEMIT_SR_RXOV         BIT(31)	/* RX FIFO overrun */
+
+#define SPACEMIT_I2C_INT_STATUS_MASK	(SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \
+					SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \
+					SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \
+					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
+					SPACEMIT_SR_ALD)
+
+/* register SPACEMIT_IBMR fields */
+#define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
+#define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
+
+/* i2c bus recover timeout: us */
+#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT	100000
+
+#define SPACEMIT_I2C_GET_ERR(status)	((status) & \
+					(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD))
+
+enum spacemit_i2c_state {
+	STATE_IDLE,
+	STATE_START,
+	STATE_READ,
+	STATE_WRITE,
+};
+
+/* i2c-spacemit driver's main struct */
+struct spacemit_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapt;
+
+	/* hardware resources */
+	void __iomem *base;
+	int irq;
+	u32 clock_freq;
+
+	struct i2c_msg *msgs;
+	int msg_num;
+
+	/* 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;
+	bool read;
+	struct completion complete;
+	u32 status;
+};
+
+static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + SPACEMIT_ICR);
+	val |= SPACEMIT_CR_IUE;
+	writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + SPACEMIT_ICR);
+	val &= ~SPACEMIT_CR_IUE;
+	writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
+{
+	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
+	udelay(5);
+	writel(0, i2c->base + SPACEMIT_ICR);
+}
+
+static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
+{
+	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
+
+	dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status);
+
+	if (err & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) {
+		spacemit_i2c_reset(i2c);
+		return -EAGAIN;
+	}
+
+	return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
+}
+
+static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
+{
+	u32 status;
+
+	/* if bus is locked, reset unit. 0: locked */
+	status = readl(i2c->base + SPACEMIT_IBMR);
+	if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL))
+		return;
+
+	spacemit_i2c_reset(i2c);
+	usleep_range(10, 20);
+
+	/* check scl status again */
+	status = readl(i2c->base + SPACEMIT_IBMR);
+	if (!(status & SPACEMIT_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 + SPACEMIT_ISR);
+	if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)))
+		return 0;
+
+	ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR, val,
+				 !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
+				 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
+	if (ret)
+		spacemit_i2c_reset(i2c);
+
+	return ret;
+}
+
+static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
+{
+	/* in case bus is not released after transfer completes */
+	if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
+		spacemit_i2c_conditionally_reset_bus(i2c);
+		usleep_range(90, 150);
+	}
+}
+
+static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	/*
+	 * 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 SPACEMIT_CR_TXDONEIE.
+	 */
+	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
+
+	/*
+	 * Unmask interrupt bits for interrupt xfer mode:
+	 * DBR rx full.
+	 * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
+	 * need to enable when trigger byte transfer to start
+	 * data sending.
+	 */
+	val |= SPACEMIT_CR_DRFIE;
+
+	/* set speed bits: default fast mode */
+	val |= SPACEMIT_CR_MODE_FAST;
+
+	/* disable response to general call */
+	val |= SPACEMIT_CR_GCD;
+
+	/* enable SCL clock output */
+	val |= SPACEMIT_CR_SCLE;
+
+	/* enable master stop detected */
+	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
+
+	writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static inline void
+spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
+{
+	writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR);
+}
+
+static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
+{
+	u32 slave_addr_rw, val;
+	struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx;
+
+	i2c->read = cur_msg->flags & I2C_M_RD;
+
+	i2c->state = STATE_START;
+
+	if (cur_msg->flags & I2C_M_RD)
+		slave_addr_rw = ((cur_msg->addr & 0x7f) << 1) | 1;
+	else
+		slave_addr_rw = (cur_msg->addr & 0x7f) << 1;
+
+	writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR);
+
+	/* send start pulse */
+	val = readl(i2c->base + SPACEMIT_ICR);
+	val &= ~SPACEMIT_CR_STOP;
+	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
+	writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+
+	val = readl(i2c->base + SPACEMIT_ICR);
+	val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
+
+	if (i2c->read)
+		val |= SPACEMIT_CR_ACKNAK;
+
+	writel(val, i2c->base + SPACEMIT_ICR);
+}
+
+static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
+{
+	unsigned long time_left;
+	u32 err;
+
+	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
+		i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf;
+		i2c->status = 0;
+		i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len;
+
+		reinit_completion(&i2c->complete);
+
+		spacemit_i2c_start(i2c);
+
+		time_left = wait_for_completion_timeout(&i2c->complete,
+							i2c->adapt.timeout);
+		if (time_left == 0) {
+			dev_err(i2c->dev, "msg completion timeout\n");
+			spacemit_i2c_conditionally_reset_bus(i2c);
+			spacemit_i2c_reset(i2c);
+			return -ETIMEDOUT;
+		}
+
+		err = SPACEMIT_I2C_GET_ERR(i2c->status);
+		if (err)
+			return spacemit_i2c_handle_err(i2c);
+	}
+
+	return 0;
+}
+
+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;
+}
+
+static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
+{
+	/* if transfer completes, SPACEMIT_ISR will handle it */
+	if (i2c->status & SPACEMIT_SR_MSD)
+		return;
+
+	if (i2c->unprocessed) {
+		writel(*i2c->msg_buf++, i2c->base + SPACEMIT_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 + SPACEMIT_IDBR);
+		i2c->unprocessed--;
+	}
+
+	/* if transfer completes, SPACEMIT_ISR will handle it */
+	if (i2c->status & (SPACEMIT_SR_MSD | SPACEMIT_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);
+}
+
+static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
+{
+	u32 val;
+	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
+
+	/*
+	 * send transaction complete signal:
+	 * error happens, detect master stop
+	 */
+	if (!err && !(i2c->status & SPACEMIT_SR_MSD))
+		return;
+
+	/*
+	 * 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 + SPACEMIT_ICR);
+	val &= ~SPACEMIT_I2C_INT_CTRL_MASK;
+	writel(val, i2c->base + SPACEMIT_ICR);
+
+	spacemit_i2c_clear_int_status(i2c, SPACEMIT_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 + SPACEMIT_ISR);
+	if (!status)
+		return IRQ_HANDLED;
+
+	i2c->status = status;
+
+	spacemit_i2c_clear_int_status(i2c, status);
+
+	if (SPACEMIT_I2C_GET_ERR(i2c->status))
+		goto err_out;
+
+	val = readl(i2c->base + SPACEMIT_ICR);
+	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
+	writel(val, i2c->base + SPACEMIT_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 |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
+			writel(val, i2c->base + SPACEMIT_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;
+
+	while (idx < i2c->msg_num) {
+		cnt += (i2c->msgs + idx)->len + 1;
+		idx++;
+	}
+
+	/*
+	 * 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 / i2c->clock_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 (ret)
+		return ret;
+
+	ret = spacemit_i2c_xfer_msg(i2c);
+	if (ret < 0)
+		dev_dbg(i2c->dev, "i2c transfer error\n");
+
+	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;
+	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
+
+	i2c->msgs = msgs;
+	i2c->msg_num = num;
+
+	ret = spacemit_i2c_xfer_core(i2c);
+	if (!ret)
+		spacemit_i2c_check_bus_release(i2c);
+
+	disable_irq(i2c->irq);
+
+	spacemit_i2c_disable(i2c);
+
+	if (ret == -ETIMEDOUT || ret == -EAGAIN)
+		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, 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 clk *clk;
+	struct device *dev = &pdev->dev;
+	struct device_node *of_node = pdev->dev.of_node;
+	struct spacemit_i2c_dev *i2c;
+	int ret = 0;
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to read clock-frequency property");
+
+	/* For now, this driver doesn't support high-speed. */
+	if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) {
+		dev_warn(dev, "unsupport clock frequency: %d, default: 400000", i2c->clock_freq);
+		i2c->clock_freq = 400000;
+	}
+
+	i2c->dev = &pdev->dev;
+
+	i2c->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(i2c->base))
+		return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
+
+	i2c->irq = platform_get_irq(pdev, 0);
+	if (i2c->irq < 0)
+		return dev_err_probe(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(dev, ret, "failed to request irq");
+
+	disable_irq(i2c->irq);
+
+	clk = devm_clk_get_enabled(dev, "apb");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock");
+
+	clk = devm_clk_get_enabled(dev, "twsi");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi 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);
+
+	platform_set_drvdata(pdev, i2c);
+
+	ret = i2c_add_numbered_adapter(&i2c->adapt);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
+
+	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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]


> +/* spacemit i2c registers */
> +#define SPACEMIT_ICR		 0x0		/* Control Register */
> +#define SPACEMIT_ISR		 0x4		/* Status Register */
> +#define SPACEMIT_IDBR		 0xc		/* Data Buffer Register */
> +#define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
> +
> +/* register SPACEMIT_ICR fields */
> +#define SPACEMIT_CR_START        BIT(0)		/* start bit */
> +#define SPACEMIT_CR_STOP         BIT(1)		/* stop bit */
> +#define SPACEMIT_CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
> +#define SPACEMIT_CR_TB           BIT(3)		/* transfer byte bit */

This looks like a lot like a variant of the i2c-pxa register set. Has it
been considered to reuse that driver?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Yao Zi]

On Mon, Mar 03, 2025 at 07:08:41AM +0100, Wolfram Sang wrote:
> 
> > +/* spacemit i2c registers */
> > +#define SPACEMIT_ICR		 0x0		/* Control Register */
> > +#define SPACEMIT_ISR		 0x4		/* Status Register */
> > +#define SPACEMIT_IDBR		 0xc		/* Data Buffer Register */
> > +#define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
> > +
> > +/* register SPACEMIT_ICR fields */
> > +#define SPACEMIT_CR_START        BIT(0)		/* start bit */
> > +#define SPACEMIT_CR_STOP         BIT(1)		/* stop bit */
> > +#define SPACEMIT_CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
> > +#define SPACEMIT_CR_TB           BIT(3)		/* transfer byte bit */
> 
> This looks like a lot like a variant of the i2c-pxa register set. Has it
> been considered to reuse that driver?
> 

Reusing existing driver has been discussed earlier[1] and the answer was
no. It really has been a long time.

Thanks,
Yao Zi

[1]: https://lore.kernel.org/all/6015d35d-6d91-4ac1-8ebf-4f79b304370f@gmail.com/

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]


> Reusing existing driver has been discussed earlier[1] and the answer was
> no. It really has been a long time.

Thanks for the link. I still agree to my opinion from back then :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Yixun Lan]

On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> and supports FIFO transmission.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> ---
>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
>  1 file changed, 59 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -0,0 +1,59 @@
> +# 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:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 2
> +    maxItems: 2
I'd suggest to give a brief description and explicit clock name here,
you can consult marvell,mv64xxx-i2c.yaml for example

> +
> +  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 176>, <&ccu 90>;
> +        clock-names = "apb", "twsi";
9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
from above docs, there are two clocks 
bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable

I'd suggest to name it according to the functionality, thus 'func', 'bus'
clock, not its source.. which would make it more system wide consistent

> +        clock-frequency = <100000>;
> +    };
> +
> +...
> 
> -- 
> 2.34.1
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

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

[Alex Elder]

On 3/2/25 11:30 PM, Troy Mitchell wrote:
> 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>

I have some more comments, and some questions.  I appreciate
seeing some of the changes you've made based on my feedback.

> ---
>   drivers/i2c/busses/Kconfig  |  17 ++
>   drivers/i2c/busses/Makefile |   1 +
>   drivers/i2c/busses/i2c-k1.c | 617 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 635 insertions(+)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index fc438f4457713d5559d163840a7b11e8cdbb8f58..12d0e99566d8625aa374279956b71a4034ded4ac 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -783,6 +783,23 @@ 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 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 built as a module.  If so, the
> +	  module will be called `i2c-k1`.
> +
>   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 1c2a4510abe44a689dfe67d2d64cf0cf3434f510..c1252e2b779e2e47492d66179b442f2202ec0416 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -74,6 +74,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 0000000000000000000000000000000000000000..6abe05640782dfa93a15d130c58ac879a423e061
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-k1.c
> @@ -0,0 +1,617 @@
> +// 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 SPACEMIT_ICR		 0x0		/* Control Register */
> +#define SPACEMIT_ISR		 0x4		/* Status Register */
> +#define SPACEMIT_IDBR		 0xc		/* Data Buffer Register */
> +#define SPACEMIT_IBMR		 0x1c		/* Bus monitor register */
> +
> +/* register SPACEMIT_ICR fields */

This is minor, but to me "SPACEMIT_ICR register fields"
sounds better.  If you decide to rearrange this, do so
for all of them.

> +#define SPACEMIT_CR_START        BIT(0)		/* start bit */
> +#define SPACEMIT_CR_STOP         BIT(1)		/* stop bit */
> +#define SPACEMIT_CR_ACKNAK       BIT(2)		/* send ACK(0) or NAK(1) */
> +#define SPACEMIT_CR_TB           BIT(3)		/* transfer byte bit */
> +/* Bit 4-7 are reserved */

Bits?  (plural)

> +#define SPACEMIT_CR_MODE_FAST    BIT(8)		/* bus mode (master operation) */
> +/* Bit 9 is reserved */
> +#define SPACEMIT_CR_UR           BIT(10)	/* unit reset */
> +/* Bit 11-12 are reserved */
> +#define SPACEMIT_CR_SCLE         BIT(13)	/* master clock enable */
> +#define SPACEMIT_CR_IUE          BIT(14)	/* unit enable */
> +/* Bit 15-17 are reserved */
> +#define SPACEMIT_CR_ALDIE        BIT(18)	/* enable arbitration interrupt */
> +#define SPACEMIT_CR_DTEIE        BIT(19)	/* enable tx interrupts */
> +#define SPACEMIT_CR_DRFIE        BIT(20)	/* enable rx interrupts */
> +#define SPACEMIT_CR_GCD          BIT(21)	/* general call disable */
> +#define SPACEMIT_CR_BEIE         BIT(22)	/* enable bus error ints */
> +/* Bit 23-24 are reserved */
> +#define SPACEMIT_CR_MSDIE        BIT(25)	/* master STOP detected int enable */
> +#define SPACEMIT_CR_MSDE         BIT(26)	/* master STOP detected enable */
> +#define SPACEMIT_CR_TXDONEIE     BIT(27)	/* transaction done int enable */
> +#define SPACEMIT_CR_TXEIE        BIT(28)	/* transmit FIFO empty int enable */
> +#define SPACEMIT_CR_RXHFIE       BIT(29)	/* receive FIFO half-full int enable */
> +#define SPACEMIT_CR_RXFIE        BIT(30)	/* receive FIFO full int enable */
> +#define SPACEMIT_CR_RXOVIE       BIT(31)	/* receive FIFO overrun int enable */
> +
> +#define SPACEMIT_I2C_INT_CTRL_MASK	(SPACEMIT_CR_ALDIE | SPACEMIT_CR_DTEIE | \
> +					SPACEMIT_CR_DRFIE | SPACEMIT_CR_BEIE | \
> +					SPACEMIT_CR_TXDONEIE | SPACEMIT_CR_TXEIE | \
> +					SPACEMIT_CR_RXHFIE | SPACEMIT_CR_RXFIE | \
> +					SPACEMIT_CR_RXOVIE | SPACEMIT_CR_MSDIE)
> +
> +/* register SPACEMIT_ISR fields */

/* Bits 0-13 are reserved */

> +#define SPACEMIT_SR_ACKNAK       BIT(14)	/* ACK/NACK status */
> +#define SPACEMIT_SR_UB           BIT(15)	/* unit busy */
> +#define SPACEMIT_SR_IBB          BIT(16)	/* i2c bus busy */
> +#define SPACEMIT_SR_EBB          BIT(17)	/* early bus busy */
> +#define SPACEMIT_SR_ALD          BIT(18)	/* arbitration loss detected */
> +#define SPACEMIT_SR_ITE          BIT(19)	/* tx buffer empty */
> +#define SPACEMIT_SR_IRF          BIT(20)	/* rx buffer full */
> +#define SPACEMIT_SR_GCAD         BIT(21)	/* general call address detected */
> +#define SPACEMIT_SR_BED          BIT(22)	/* bus error no ACK/NAK */
> +#define SPACEMIT_SR_SAD          BIT(23)	/* slave address detected */
> +#define SPACEMIT_SR_SSD          BIT(24)	/* slave stop detected */
> +/* Bit 25 is reserved */
> +#define SPACEMIT_SR_MSD          BIT(26)	/* master stop detected */
> +#define SPACEMIT_SR_TXDONE       BIT(27)	/* transaction done */
> +#define SPACEMIT_SR_TXE          BIT(28)	/* tx FIFO empty */
> +#define SPACEMIT_SR_RXHF         BIT(29)	/* rx FIFO half-full */
> +#define SPACEMIT_SR_RXF          BIT(30)	/* rx FIFO full */
> +#define SPACEMIT_SR_RXOV         BIT(31)	/* RX FIFO overrun */
> +
> +#define SPACEMIT_I2C_INT_STATUS_MASK	(SPACEMIT_SR_RXOV | SPACEMIT_SR_RXF | SPACEMIT_SR_RXHF | \
> +					SPACEMIT_SR_TXE | SPACEMIT_SR_TXDONE | SPACEMIT_SR_MSD | \
> +					SPACEMIT_SR_SSD | SPACEMIT_SR_SAD | SPACEMIT_SR_BED | \
> +					SPACEMIT_SR_GCAD | SPACEMIT_SR_IRF | SPACEMIT_SR_ITE | \
> +					SPACEMIT_SR_ALD)
> +
> +/* register SPACEMIT_IBMR fields */
> +#define SPACEMIT_BMR_SDA         BIT(0)		/* SDA line level */
> +#define SPACEMIT_BMR_SCL         BIT(1)		/* SCL line level */
> +
> +/* i2c bus recover timeout: us */
> +#define SPACEMIT_I2C_BUS_BUSY_TIMEOUT	100000
> +

I think it's clearer to just define a mask, something like:

#define SPACEMIT_SR_ERROR \
	(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD)

And then in the code where needed, do:

	if (status->i2c & SPACEMIT_ERROR)
		handle_error();

I'll talk about this more where you use it, below.

> +#define SPACEMIT_I2C_GET_ERR(status)	((status) & \
> +					(SPACEMIT_SR_BED | SPACEMIT_SR_RXOV | SPACEMIT_SR_ALD))
> +
> +enum spacemit_i2c_state {
> +	STATE_IDLE,
> +	STATE_START,
> +	STATE_READ,
> +	STATE_WRITE,
> +};
> +
> +/* i2c-spacemit driver's main struct */
> +struct spacemit_i2c_dev {
> +	struct device *dev;
> +	struct i2c_adapter adapt;
> +
> +	/* hardware resources */
> +	void __iomem *base;
> +	int irq;
> +	u32 clock_freq;
> +
> +	struct i2c_msg *msgs;
> +	int msg_num;
> +
> +	/* 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;
> +	bool read;
> +	struct completion complete;
> +	u32 status;
> +};
> +
> +static void spacemit_i2c_enable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val |= SPACEMIT_CR_IUE;
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static void spacemit_i2c_disable(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val &= ~SPACEMIT_CR_IUE;
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static void spacemit_i2c_reset(struct spacemit_i2c_dev *i2c)
> +{
> +	writel(SPACEMIT_CR_UR, i2c->base + SPACEMIT_ICR);
> +	udelay(5);
> +	writel(0, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static int spacemit_i2c_handle_err(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);

The macro call above adds no value.

> +
> +	dev_dbg(i2c->dev, "i2c error status: 0x%08x\n", i2c->status);
> +

Instead, you can just do this check on i2c->status directly.

	if (i2c->status & ( ... )) {

If you wanted to cache the status value in a local variable,
you could do that too, but that doesn't require the macro.

> +	if (err & (SPACEMIT_SR_BED | SPACEMIT_SR_ALD)) {
> +		spacemit_i2c_reset(i2c);
> +		return -EAGAIN;
> +	}
> +
> +	return i2c->status & SPACEMIT_SR_ACKNAK ? -ENXIO : -EIO;
> +}
> +
> +static void spacemit_i2c_conditionally_reset_bus(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 status;
> +
> +	/* if bus is locked, reset unit. 0: locked */
> +	status = readl(i2c->base + SPACEMIT_IBMR);
> +	if ((status & SPACEMIT_BMR_SDA) && (status & SPACEMIT_BMR_SCL))
> +		return;
> +
> +	spacemit_i2c_reset(i2c);
> +	usleep_range(10, 20);
> +
> +	/* check scl status again */
> +	status = readl(i2c->base + SPACEMIT_IBMR);
> +	if (!(status & SPACEMIT_BMR_SCL))
> +		dev_warn_ratelimited(i2c->dev, "unit reset failed\n");
> +}
> +

I think spacemit_i2c_wait_bus_busy() would be a better name
for this function.  Actually, are you waiting for the bus
to *not* be busy?  If so, name it that.

> +static int spacemit_i2c_recover_bus_busy(struct spacemit_i2c_dev *i2c)
> +{
> +	int ret = 0;

There is no need to initialize ret.

> +	u32 val;
> +
> +	val = readl(i2c->base + SPACEMIT_ISR);
> +	if (!(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)))
> +		return 0;
> +
> +	ret = readl_poll_timeout(i2c->base + SPACEMIT_ISR, val,
> +				 !(val & (SPACEMIT_SR_UB | SPACEMIT_SR_IBB)),
> +				 1500, SPACEMIT_I2C_BUS_BUSY_TIMEOUT);
> +	if (ret)
> +		spacemit_i2c_reset(i2c);
> +
> +	return ret;
> +}
> +
> +static void spacemit_i2c_check_bus_release(struct spacemit_i2c_dev *i2c)
> +{
> +	/* in case bus is not released after transfer completes */
> +	if (readl(i2c->base + SPACEMIT_ISR) & SPACEMIT_SR_EBB) {
> +		spacemit_i2c_conditionally_reset_bus(i2c);
> +		usleep_range(90, 150);
> +	}
> +}
> +
> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	/*
> +	 * 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 SPACEMIT_CR_TXDONEIE.
> +	 */
> +	val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
> +
> +	/*
> +	 * Unmask interrupt bits for interrupt xfer mode:
> +	 * DBR rx full.
> +	 * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
> +	 * need to enable when trigger byte transfer to start
> +	 * data sending.
> +	 */
> +	val |= SPACEMIT_CR_DRFIE;
> +
> +	/* set speed bits: default fast mode */

It is not *default* fast mode, it *is* fast mode.  (There
is no other mode used in this driver, right?)

> +	val |= SPACEMIT_CR_MODE_FAST;
> +
> +	/* disable response to general call */
> +	val |= SPACEMIT_CR_GCD;
> +
> +	/* enable SCL clock output */
> +	val |= SPACEMIT_CR_SCLE;
> +
> +	/* enable master stop detected */
> +	val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
> +
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static inline void
> +spacemit_i2c_clear_int_status(struct spacemit_i2c_dev *i2c, u32 mask)
> +{
> +	writel(mask & SPACEMIT_I2C_INT_STATUS_MASK, i2c->base + SPACEMIT_ISR);
> +}
> +
> +static void spacemit_i2c_start(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 slave_addr_rw, val;
> +	struct i2c_msg *cur_msg = i2c->msgs + i2c->msg_idx;
> +
> +	i2c->read = cur_msg->flags & I2C_M_RD;

i2c->read is Boolean.  The assignment above will almost
certainly do the right thing, but if you did it this
way, it would make it clear you were making a Boolean
assingment:

	i2c->read = !!(cur_msg->flags & I2C_M_RD);

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

I think you said you were going to implement my suggestion here:

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

> +
> +	writel(slave_addr_rw, i2c->base + SPACEMIT_IDBR);
> +
> +	/* send start pulse */
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val &= ~SPACEMIT_CR_STOP;
> +	val |= SPACEMIT_CR_START | SPACEMIT_CR_TB | SPACEMIT_CR_DTEIE;
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static void spacemit_i2c_stop(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val |= SPACEMIT_CR_STOP | SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> +
> +	if (i2c->read)
> +		val |= SPACEMIT_CR_ACKNAK;
> +
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +}
> +
> +static int spacemit_i2c_xfer_msg(struct spacemit_i2c_dev *i2c)
> +{
> +	unsigned long time_left;
> +	u32 err;
> +
> +	for (i2c->msg_idx = 0; i2c->msg_idx < i2c->msg_num; i2c->msg_idx++) {
> +		i2c->msg_buf = (i2c->msgs + i2c->msg_idx)->buf;
> +		i2c->status = 0;
> +		i2c->unprocessed = (i2c->msgs + i2c->msg_idx)->len;
> +
> +		reinit_completion(&i2c->complete);
> +
> +		spacemit_i2c_start(i2c);
> +
> +		time_left = wait_for_completion_timeout(&i2c->complete,
> +							i2c->adapt.timeout);
> +		if (time_left == 0) {
> +			dev_err(i2c->dev, "msg completion timeout\n");
> +			spacemit_i2c_conditionally_reset_bus(i2c);
> +			spacemit_i2c_reset(i2c);
> +			return -ETIMEDOUT;
> +		}
> +
> +		err = SPACEMIT_I2C_GET_ERR(i2c->status);

This would be better as:

	if (i2c->status & SPACEMIT_SR_ERROR)
		return spacemit_i2c_handle_err(i2c);

> +		if (err)
> +			return spacemit_i2c_handle_err(i2c);
> +	}
> +
> +	return 0;
> +}
> +
> +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;
> +}
> +
> +static void spacemit_i2c_handle_write(struct spacemit_i2c_dev *i2c)
> +{
> +	/* if transfer completes, SPACEMIT_ISR will handle it */
> +	if (i2c->status & SPACEMIT_SR_MSD)
> +		return;
> +
> +	if (i2c->unprocessed) {
> +		writel(*i2c->msg_buf++, i2c->base + SPACEMIT_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 + SPACEMIT_IDBR);
> +		i2c->unprocessed--;
> +	}
> +
> +	/* if transfer completes, SPACEMIT_ISR will handle it */
> +	if (i2c->status & (SPACEMIT_SR_MSD | SPACEMIT_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);
> +}
> +
> +static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
> +{
> +	u32 val;
> +	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
> +
> +	/*
> +	 * send transaction complete signal:
> +	 * error happens, detect master stop
> +	 */

The next line is the only place you use err in this function.
I think you should instead do:

	if (!(i2c->status & (SPACEMIT_SR_ERROR | SPACEMIT_SR_MSD)))
		return;

> +	if (!err && !(i2c->status & SPACEMIT_SR_MSD))
> +		return;
> +
> +	/*
> +	 * 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 + SPACEMIT_ICR);
> +	val &= ~SPACEMIT_I2C_INT_CTRL_MASK;
> +	writel(val, i2c->base + SPACEMIT_ICR);
> +
> +	spacemit_i2c_clear_int_status(i2c, SPACEMIT_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 + SPACEMIT_ISR);
> +	if (!status)
> +		return IRQ_HANDLED;
> +
> +	i2c->status = status;
> +
> +	spacemit_i2c_clear_int_status(i2c, status);
> +
> +	if (SPACEMIT_I2C_GET_ERR(i2c->status))
> +		goto err_out;
> +
> +	val = readl(i2c->base + SPACEMIT_ICR);
> +	val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> +	writel(val, i2c->base + SPACEMIT_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 |= SPACEMIT_CR_ALDIE | SPACEMIT_CR_TB;
> +			writel(val, i2c->base + SPACEMIT_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;
> +
> +	while (idx < i2c->msg_num) {
> +		cnt += (i2c->msgs + idx)->len + 1;
> +		idx++;
> +	}
> +
> +	/*
> +	 * 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 / i2c->clock_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);

I don't have a lot of experience with I2C drivers, but is it normal
to reset before every transfer?

If it is, just tell me that.  But if it's not, can you explain why
it's necessary here?

> +
> +	spacemit_i2c_calc_timeout(i2c);
> +
> +	spacemit_i2c_init(i2c);
> +

Here too, maybe I just don't know what most I2C drivers do, but
is it necessary to only enable the I2C adapter and its interrupt
handler when performing a transfer?

> +	spacemit_i2c_enable(i2c);
> +	enable_irq(i2c->irq);
> +
> +	/* i2c wait for bus busy */
> +	ret = spacemit_i2c_recover_bus_busy(i2c);
> +	if (ret)
> +		return ret;
> +
> +	ret = spacemit_i2c_xfer_msg(i2c);
> +	if (ret < 0)
> +		dev_dbg(i2c->dev, "i2c transfer error\n");

If you're reporting the error you might as well say what
it is.

	dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);

> +
> +	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;
> +	u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
> +
> +	i2c->msgs = msgs;
> +	i2c->msg_num = num;
> +
> +	ret = spacemit_i2c_xfer_core(i2c);
> +	if (!ret)
> +		spacemit_i2c_check_bus_release(i2c);
> +

The enable_irq() call that matches the disable call below is
found in spacemit_i2c_xfer_core().  That's where this call
belongs.

> +	disable_irq(i2c->irq);
> +

Same with the next call--it should be in the same function
that its corresponding spacemit_i2c_enable() is called.

With these suggestions in mind, I think you can safely
just get rid of spacemit_i2c_xfer_core().  It is only
called in this one spot (above), and you can just do
everything within spacemit_i2c_xfer() instead.

> +	spacemit_i2c_disable(i2c);
> +
> +	if (ret == -ETIMEDOUT || ret == -EAGAIN)
> +		dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret, 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 clk *clk;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *of_node = pdev->dev.of_node;
> +	struct spacemit_i2c_dev *i2c;
> +	int ret = 0;

There is no need to initialize ret.

> +
> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read clock-frequency property");
> +
> +	/* For now, this driver doesn't support high-speed. */
> +	if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) {

In your device tree binding, you indicate that three different
modes are supported, and that the maximum frequency is 3300000 Hz.
This says that only ranges from 1-400000 Hz are allowed.

In fact, although you look up this clock frequency in DT, I see
nothing that actually is affected by this value.  I.e., no I2C
bus frequency changes, regardless of what frequency you specify.
The only place the clock_freq field is used is in calculating
the timeout for a transfer.

So two things:
- My guess is that you are relying on whatever frequency the
   hardware already is using, and maybe that's 400000 Hz.
   That's fine, though at some point it should be more
   directly controlled (set somehow).
- Since you don't actually support any other frequency,
   drop this "clock-frequency" feature for now, and add it
   when you're ready to actually support it.

And I might be wrong about this, but I don't think your
(new) DTS binding should specify behavior that is not
supported by the driver.

					-Alex

> +		dev_warn(dev, "unsupport clock frequency: %d, default: 400000", i2c->clock_freq);
> +		i2c->clock_freq = 400000;
> +	}
> +
> +	i2c->dev = &pdev->dev;
> +
> +	i2c->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(i2c->base))
> +		return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
> +
> +	i2c->irq = platform_get_irq(pdev, 0);
> +	if (i2c->irq < 0)
> +		return dev_err_probe(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(dev, ret, "failed to request irq");
> +
> +	disable_irq(i2c->irq);
> +
> +	clk = devm_clk_get_enabled(dev, "apb");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock");
> +
> +	clk = devm_clk_get_enabled(dev, "twsi");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi 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);
> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	ret = i2c_add_numbered_adapter(&i2c->adapt);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
> +
> +	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");
> 

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

[Samuel Holland]

Hi Troy,

On 2025-03-03 3:35 AM, Yixun Lan wrote:
> On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
>> The I2C of K1 supports fast-speed-mode and high-speed-mode,
>> and supports FIFO transmission.
>>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
>> ---
>>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
>>  1 file changed, 59 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
>> @@ -0,0 +1,59 @@
>> +# 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:
>> +    minItems: 2
>> +    maxItems: 2
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    maxItems: 2
> I'd suggest to give a brief description and explicit clock name here,
> you can consult marvell,mv64xxx-i2c.yaml for example
> 
>> +
>> +  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 176>, <&ccu 90>;
>> +        clock-names = "apb", "twsi";
> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> from above docs, there are two clocks 
> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> 
> I'd suggest to name it according to the functionality, thus 'func', 'bus'
> clock, not its source.. which would make it more system wide consistent

Also in that same register is:

2	RST	RW	0x1	TWSI0 Reset Generation
This field resets both the APB and functional domain.
- 0: No Reset
- 1: Reset

Which means you need a 'resets' property in the binding as well.

Regards,
Samuel

>> +        clock-frequency = <100000>;
>> +    };
>> +
>> +...
>>
>> -- 
>> 2.34.1
>>
> 

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

[Yixun Lan]

Hi Samuel Holland:

On 20:11 Tue 04 Mar     , Samuel Holland wrote:
> Hi Troy,
> 
> On 2025-03-03 3:35 AM, Yixun Lan wrote:
> > On 13:30 Mon 03 Mar     , Troy Mitchell wrote:
> >> The I2C of K1 supports fast-speed-mode and high-speed-mode,
> >> and supports FIFO transmission.
> >>
> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >> Signed-off-by: Troy Mitchell <troymitchell988@gmail.com>
> >> ---
> >>  .../devicetree/bindings/i2c/spacemit,k1-i2c.yaml   | 59 ++++++++++++++++++++++
> >>  1 file changed, 59 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 0000000000000000000000000000000000000000..db49f1f473e6f166f534b276c86b3951d86341c3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> >> @@ -0,0 +1,59 @@
> >> +# 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:
> >> +    minItems: 2
> >> +    maxItems: 2
> >> +
> >> +  clock-names:
> >> +    minItems: 2
> >> +    maxItems: 2
> > I'd suggest to give a brief description and explicit clock name here,
> > you can consult marvell,mv64xxx-i2c.yaml for example
> > 
> >> +
> >> +  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 176>, <&ccu 90>;
> >> +        clock-names = "apb", "twsi";
> > 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> > https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> > from above docs, there are two clocks 
> > bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> > bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> > 
> > I'd suggest to name it according to the functionality, thus 'func', 'bus'
> > clock, not its source.. which would make it more system wide consistent
> 
> Also in that same register is:
> 
> 2	RST	RW	0x1	TWSI0 Reset Generation
> This field resets both the APB and functional domain.
> - 0: No Reset
> - 1: Reset
> 
> Which means you need a 'resets' property in the binding as well.
> 
right, there is reset needed

I'd suggest to add it as an incremental patch later, when we
implement real reset driver, and also complete the calling reset
consumer API in i2c driver

but, let me know if this is not the right way to go

> Regards,
> Samuel
> 
> >> +        clock-frequency = <100000>;
> >> +    };
> >> +
> >> +...
> >>
> >> -- 
> >> 2.34.1
> >>
> > 
> 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

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

[Samuel Holland]

On 2025-03-04 9:05 PM, Yixun Lan wrote:
>>>> +        clocks = <&ccu 176>, <&ccu 90>;
>>>> +        clock-names = "apb", "twsi";
>>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
>>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
>>> from above docs, there are two clocks 
>>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
>>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
>>>
>>> I'd suggest to name it according to the functionality, thus 'func', 'bus'
>>> clock, not its source.. which would make it more system wide consistent
>>
>> Also in that same register is:
>>
>> 2	RST	RW	0x1	TWSI0 Reset Generation
>> This field resets both the APB and functional domain.
>> - 0: No Reset
>> - 1: Reset
>>
>> Which means you need a 'resets' property in the binding as well.
>>
> right, there is reset needed
> 
> I'd suggest to add it as an incremental patch later, when we
> implement real reset driver, and also complete the calling reset
> consumer API in i2c driver
> 
> but, let me know if this is not the right way to go

If you add the resets property later, that's a breaking change to the DT,
because existing devicetrees will not have that property. So you would have to
make the reset consumer in the driver optional, even if it's not really
optional, to work with older DTs. So it is _possible_ to add incrementally, but
not recommended because it adds "legacy" code that never really goes away.

It's okay to define the binding as requiring the resets property now, even
before the reset controller driver is merged. You just won't be able to add the
I2C controller to the DTS until the reset controller binding is merged. But
since the reset controller is the same IP block as the clock controller, its
binding should be available soon anyway.

Regards,
Samuel

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

[Yixun Lan]

Hi Samuel:

On 22:01 Tue 04 Mar     , Samuel Holland wrote:
> On 2025-03-04 9:05 PM, Yixun Lan wrote:
> >>>> +        clocks = <&ccu 176>, <&ccu 90>;
> >>>> +        clock-names = "apb", "twsi";
> >>> 9.1.4.61 TWSI0 CLOCK RESET CONTROL REGISTER(APBC_TWSI0_CLK_RST)
> >>> https://developer.spacemit.com/documentation?token=LCrKwWDasiJuROkVNusc2pWTnEb#part594
> >>> from above docs, there are two clocks 
> >>> bit[1] - FNCLK, TWSI0 Functional Clock Enable/Disable
> >>> bit[0] - APBCLK, TWSI0 APB Bus Clock Enable/Disable
> >>>
> >>> I'd suggest to name it according to the functionality, thus 'func', 'bus'
> >>> clock, not its source.. which would make it more system wide consistent
> >>
> >> Also in that same register is:
> >>
> >> 2	RST	RW	0x1	TWSI0 Reset Generation
> >> This field resets both the APB and functional domain.
> >> - 0: No Reset
> >> - 1: Reset
> >>
> >> Which means you need a 'resets' property in the binding as well.
> >>
> > right, there is reset needed
> > 
> > I'd suggest to add it as an incremental patch later, when we
> > implement real reset driver, and also complete the calling reset
> > consumer API in i2c driver
> > 
> > but, let me know if this is not the right way to go
> 
> If you add the resets property later, that's a breaking change to the DT,
> because existing devicetrees will not have that property. So you would have to
> make the reset consumer in the driver optional, even if it's not really
> optional, to work with older DTs. So it is _possible_ to add incrementally, but
> not recommended because it adds "legacy" code that never really goes away.
> 
Ok, that's fair if we want to keep DT backward compatible..

> It's okay to define the binding as requiring the resets property now, even
> before the reset controller driver is merged. You just won't be able to add the
> I2C controller to the DTS until the reset controller binding is merged. But
> since the reset controller is the same IP block as the clock controller, its
> binding should be available soon anyway.
> 
yes, this should work, thanks
I think we will wait for reset driver

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

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

[Troy Mitchell]

On 2025/3/4 08:01, Alex Elder wrote:

> On 3/2/25 11:30 PM, Troy Mitchell wrote:
>> 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>
>
> I have some more comments, and some questions.  I appreciate
> seeing some of the changes you've made based on my feedback.
Hi, Alex. Thanks for your review.
>> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>> +{
>> +    u32 val;
>> +
>> +    /*
>> +     * 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 SPACEMIT_CR_TXDONEIE.
>> +     */
>> +    val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
>> +
>> +    /*
>> +     * Unmask interrupt bits for interrupt xfer mode:
>> +     * DBR rx full.
>> +     * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
>> +     * need to enable when trigger byte transfer to start
>> +     * data sending.
>> +     */
>> +    val |= SPACEMIT_CR_DRFIE;
>> +
>> +    /* set speed bits: default fast mode */
>
> It is not *default* fast mode, it *is* fast mode.  (There
> is no other mode used in this driver, right?)
yes. I will talk it below.
>
>> +    val |= SPACEMIT_CR_MODE_FAST;
>> +
>> +    /* disable response to general call */
>> +    val |= SPACEMIT_CR_GCD;
>> +
>> +    /* enable SCL clock output */
>> +    val |= SPACEMIT_CR_SCLE;
>> +
>> +    /* enable master stop detected */
>> +    val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
>> +
>> +    writel(val, i2c->base + SPACEMIT_ICR);
>> +}
>> +
>> +
>> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
>> +{
>> +    int ret;
>> +
>> +    spacemit_i2c_reset(i2c);
>
> I don't have a lot of experience with I2C drivers, but is it normal
> to reset before every transfer?
>
> If it is, just tell me that.  But if it's not, can you explain why
> it's necessary here?

My initial idea was to keep the I2C state in its initial state before each
transmission. 

But after testing, this is not necessary. I will move it to `probe` function.

>
>> +
>> +    spacemit_i2c_calc_timeout(i2c);
>> +
>> +    spacemit_i2c_init(i2c);
>> +
>
> Here too, maybe I just don't know what most I2C drivers do, but
> is it necessary to only enable the I2C adapter and its interrupt
> handler when performing a transfer?

It is necessary to enable before each transmission. 

I have tested moving the `spacemit_i2c_enable` to the probe function. 

It will cause transmission errors.

As for the `enable_irq`, I think it can be moved to the `probe` function.

>
>> +    spacemit_i2c_enable(i2c);
>> +    enable_irq(i2c->irq);
>> +
>> +    /* i2c wait for bus busy */
>> +    ret = spacemit_i2c_recover_bus_busy(i2c);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = spacemit_i2c_xfer_msg(i2c);
>> +    if (ret < 0)
>> +        dev_dbg(i2c->dev, "i2c transfer error\n");
>
> If you're reporting the error you might as well say what
> it is.
>
>     dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
>
>> +
>> +    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;
>> +    u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
>> +
>> +    i2c->msgs = msgs;
>> +    i2c->msg_num = num;
>> +
>> +    ret = spacemit_i2c_xfer_core(i2c);
>> +    if (!ret)
>> +        spacemit_i2c_check_bus_release(i2c);
>> +
>
> The enable_irq() call that matches the disable call below is
> found in spacemit_i2c_xfer_core().  That's where this call
> belongs.
>
>> +    disable_irq(i2c->irq);
>> +
>
> Same with the next call--it should be in the same function
> that its corresponding spacemit_i2c_enable() is called.
>
> With these suggestions in mind, I think you can safely
> just get rid of spacemit_i2c_xfer_core().  It is only
> called in this one spot (above), and you can just do
> everything within spacemit_i2c_xfer() instead.
>
>> +    spacemit_i2c_disable(i2c);
>> +
>> +    if (ret == -ETIMEDOUT || ret == -EAGAIN)
>> +        dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret,
>> 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 clk *clk;
>> +    struct device *dev = &pdev->dev;
>> +    struct device_node *of_node = pdev->dev.of_node;
>> +    struct spacemit_i2c_dev *i2c;
>> +    int ret = 0;
>
> There is no need to initialize ret.
>
>> +
>> +    i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>> +    if (!i2c)
>> +        return -ENOMEM;
>> +
>> +    ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "failed to read clock-frequency
>> property");
>> +
>> +    /* For now, this driver doesn't support high-speed. */
>> +    if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) {
>
> In your device tree binding, you indicate that three different
> modes are supported, and that the maximum frequency is 3300000 Hz.
> This says that only ranges from 1-400000 Hz are allowed.
>
> In fact, although you look up this clock frequency in DT, I see
> nothing that actually is affected by this value.  I.e., no I2C
> bus frequency changes, regardless of what frequency you specify.
> The only place the clock_freq field is used is in calculating
> the timeout for a transfer.
>
> So two things:
> - My guess is that you are relying on whatever frequency the
>   hardware already is using, and maybe that's 400000 Hz.
>   That's fine, though at some point it should be more
>   directly controlled (set somehow).
> - Since you don't actually support any other frequency,
>   drop this "clock-frequency" feature for now, and add it
>   when you're ready to actually support it.
>
> And I might be wrong about this, but I don't think your
> (new) DTS binding should specify behavior that is not
> supported by the driver.
>
>                     -Alex

I will support standard mode in next version.

We just need to modify the function `spacemit_i2c_init`.

>
>> +        dev_warn(dev, "unsupport clock frequency: %d, default: 400000",
>> i2c->clock_freq);
>> +        i2c->clock_freq = 400000;
>> +    }
>> +
>> +    i2c->dev = &pdev->dev;
>> +
>> +    i2c->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(i2c->base))
>> +        return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
>> +
>> +    i2c->irq = platform_get_irq(pdev, 0);
>> +    if (i2c->irq < 0)
>> +        return dev_err_probe(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(dev, ret, "failed to request irq");
>> +
>> +    disable_irq(i2c->irq);
>> +
>> +    clk = devm_clk_get_enabled(dev, "apb");
>> +    if (IS_ERR(clk))
>> +        return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock");
>> +
>> +    clk = devm_clk_get_enabled(dev, "twsi");
>> +    if (IS_ERR(clk))
>> +        return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi 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);
>> +
>> +    platform_set_drvdata(pdev, i2c);
>> +
>> +    ret = i2c_add_numbered_adapter(&i2c->adapt);
>> +    if (ret)
>> +        return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
>> +
>> +    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

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

[Alex Elder]

On 3/6/25 7:16 AM, Troy Mitchell wrote:
> On 2025/3/4 08:01, Alex Elder wrote:
> 
>> On 3/2/25 11:30 PM, Troy Mitchell wrote:
>>> 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>
>>
>> I have some more comments, and some questions.  I appreciate
>> seeing some of the changes you've made based on my feedback.
> Hi, Alex. Thanks for your review.
>>> +static void spacemit_i2c_init(struct spacemit_i2c_dev *i2c)
>>> +{
>>> +    u32 val;
>>> +
>>> +    /*
>>> +     * 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 SPACEMIT_CR_TXDONEIE.
>>> +     */
>>> +    val = SPACEMIT_CR_BEIE | SPACEMIT_CR_ALDIE;
>>> +
>>> +    /*
>>> +     * Unmask interrupt bits for interrupt xfer mode:
>>> +     * DBR rx full.
>>> +     * For tx empty interrupt SPACEMIT_CR_DTEIE, we only
>>> +     * need to enable when trigger byte transfer to start
>>> +     * data sending.
>>> +     */
>>> +    val |= SPACEMIT_CR_DRFIE;
>>> +
>>> +    /* set speed bits: default fast mode */
>>
>> It is not *default* fast mode, it *is* fast mode.  (There
>> is no other mode used in this driver, right?)
> yes. I will talk it below.
>>
>>> +    val |= SPACEMIT_CR_MODE_FAST;
>>> +
>>> +    /* disable response to general call */
>>> +    val |= SPACEMIT_CR_GCD;
>>> +
>>> +    /* enable SCL clock output */
>>> +    val |= SPACEMIT_CR_SCLE;
>>> +
>>> +    /* enable master stop detected */
>>> +    val |= SPACEMIT_CR_MSDE | SPACEMIT_CR_MSDIE;
>>> +
>>> +    writel(val, i2c->base + SPACEMIT_ICR);
>>> +}
>>> +
>>> +
>>> +static int spacemit_i2c_xfer_core(struct spacemit_i2c_dev *i2c)
>>> +{
>>> +    int ret;
>>> +
>>> +    spacemit_i2c_reset(i2c);
>>
>> I don't have a lot of experience with I2C drivers, but is it normal
>> to reset before every transfer?
>>
>> If it is, just tell me that.  But if it's not, can you explain why
>> it's necessary here?
> 
> My initial idea was to keep the I2C state in its initial state before each
> transmission.
> 
> But after testing, this is not necessary. I will move it to `probe` function.

OK, that seems better.  But honestly you should do this only
if you're certain the reset isn't required before every transfer.
I don't know, but I assumed it was there for a reason.

>>> +
>>> +    spacemit_i2c_calc_timeout(i2c);
>>> +
>>> +    spacemit_i2c_init(i2c);
>>> +
>>
>> Here too, maybe I just don't know what most I2C drivers do, but
>> is it necessary to only enable the I2C adapter and its interrupt
>> handler when performing a transfer?
> 
> It is necessary to enable before each transmission.
> 
> I have tested moving the `spacemit_i2c_enable` to the probe function.
> 
> It will cause transmission errors.
> 
> As for the `enable_irq`, I think it can be moved to the `probe` function.

It really depends on whether you intend to rule out
any interrupts other than when you are performing
a transfer.

This might be reasonable, but sometimes drivers will
keep an interrupt enabled most of the time, sometimes
they restrict when it's enabled.  Hence my question.

> 
>>
>>> +    spacemit_i2c_enable(i2c);
>>> +    enable_irq(i2c->irq);
>>> +
>>> +    /* i2c wait for bus busy */
>>> +    ret = spacemit_i2c_recover_bus_busy(i2c);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = spacemit_i2c_xfer_msg(i2c);
>>> +    if (ret < 0)
>>> +        dev_dbg(i2c->dev, "i2c transfer error\n");
>>
>> If you're reporting the error you might as well say what
>> it is.
>>
>>      dev_dbg(i2c->dev, "i2c transfer error: %d\n", ret);
>>
>>> +
>>> +    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;
>>> +    u32 err = SPACEMIT_I2C_GET_ERR(i2c->status);
>>> +
>>> +    i2c->msgs = msgs;
>>> +    i2c->msg_num = num;
>>> +
>>> +    ret = spacemit_i2c_xfer_core(i2c);
>>> +    if (!ret)
>>> +        spacemit_i2c_check_bus_release(i2c);
>>> +
>>
>> The enable_irq() call that matches the disable call below is
>> found in spacemit_i2c_xfer_core().  That's where this call
>> belongs.

I think the above comment is important.  I'll look at
your next version of the series to see what you do.

>>
>>> +    disable_irq(i2c->irq);
>>> +
>>
>> Same with the next call--it should be in the same function
>> that its corresponding spacemit_i2c_enable() is called.
>>
>> With these suggestions in mind, I think you can safely
>> just get rid of spacemit_i2c_xfer_core().  It is only
>> called in this one spot (above), and you can just do
>> everything within spacemit_i2c_xfer() instead.
>>
>>> +    spacemit_i2c_disable(i2c);
>>> +
>>> +    if (ret == -ETIMEDOUT || ret == -EAGAIN)
>>> +        dev_alert(i2c->dev, "i2c transfer failed, ret %d err 0x%x\n", ret,
>>> 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 clk *clk;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct device_node *of_node = pdev->dev.of_node;
>>> +    struct spacemit_i2c_dev *i2c;
>>> +    int ret = 0;
>>
>> There is no need to initialize ret.
>>
>>> +
>>> +    i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>>> +    if (!i2c)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = of_property_read_u32(of_node, "clock-frequency", &i2c->clock_freq);
>>> +    if (ret)
>>> +        return dev_err_probe(dev, ret, "failed to read clock-frequency
>>> property");
>>> +
>>> +    /* For now, this driver doesn't support high-speed. */
>>> +    if (i2c->clock_freq < 1 || i2c->clock_freq > 400000) {
>>
>> In your device tree binding, you indicate that three different
>> modes are supported, and that the maximum frequency is 3300000 Hz.
>> This says that only ranges from 1-400000 Hz are allowed.
>>
>> In fact, although you look up this clock frequency in DT, I see
>> nothing that actually is affected by this value.  I.e., no I2C
>> bus frequency changes, regardless of what frequency you specify.
>> The only place the clock_freq field is used is in calculating
>> the timeout for a transfer.
>>
>> So two things:
>> - My guess is that you are relying on whatever frequency the
>>    hardware already is using, and maybe that's 400000 Hz.
>>    That's fine, though at some point it should be more
>>    directly controlled (set somehow).
>> - Since you don't actually support any other frequency,
>>    drop this "clock-frequency" feature for now, and add it
>>    when you're ready to actually support it.
>>
>> And I might be wrong about this, but I don't think your
>> (new) DTS binding should specify behavior that is not
>> supported by the driver.
>>
>>                      -Alex
> 
> I will support standard mode in next version.

I'll wait to see what you do, but please try not to change
anything substantive between versions of a patch series.

> We just need to modify the function `spacemit_i2c_init`.

Thanks for your responses.

					-Alex

>>
>>> +        dev_warn(dev, "unsupport clock frequency: %d, default: 400000",
>>> i2c->clock_freq);
>>> +        i2c->clock_freq = 400000;
>>> +    }
>>> +
>>> +    i2c->dev = &pdev->dev;
>>> +
>>> +    i2c->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(i2c->base))
>>> +        return dev_err_probe(dev, PTR_ERR(i2c->base), "failed to do ioremap");
>>> +
>>> +    i2c->irq = platform_get_irq(pdev, 0);
>>> +    if (i2c->irq < 0)
>>> +        return dev_err_probe(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(dev, ret, "failed to request irq");
>>> +
>>> +    disable_irq(i2c->irq);
>>> +
>>> +    clk = devm_clk_get_enabled(dev, "apb");
>>> +    if (IS_ERR(clk))
>>> +        return dev_err_probe(dev, PTR_ERR(clk), "failed to enable apb clock");
>>> +
>>> +    clk = devm_clk_get_enabled(dev, "twsi");
>>> +    if (IS_ERR(clk))
>>> +        return dev_err_probe(dev, PTR_ERR(clk), "failed to enable twsi 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);
>>> +
>>> +    platform_set_drvdata(pdev, i2c);
>>> +
>>> +    ret = i2c_add_numbered_adapter(&i2c->adapt);
>>> +    if (ret)
>>> +        return dev_err_probe(&pdev->dev, ret, "failed to add i2c adapter");
>>> +
>>> +    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");
>>>
>>