[PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support

[PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support

[Binbin Zhou]

Hi all:

This patch set describes the I2C controller integrated the
Loongson-2K0300 chip.

It has a significantly different design from the previous I2C
controller(i2c-ls2x), such as support for master-slave transfer mode,
and  DMA transfers (implementation in progress), etc. Therefore, we try
to name it i2c-ls2x-v2.

Thanks.

======
V2:
Patch (1/2):
 - Add Acked-by tag from Conor, thanks.

Patch (2/2):
 - Reorder the definitions of read() and write();
 - Adjust the calculation method for bus speed.

Link to V1:
https://lore.kernel.org/all/cover.1763018288.git.zhoubinbin@loongson.cn/

Binbin Zhou (2):
  dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible
  i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

 .../bindings/i2c/loongson,ls2x-i2c.yaml       |  15 +
 MAINTAINERS                                   |   1 +
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-ls2x-v2.c              | 545 ++++++++++++++++++
 5 files changed, 572 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x-v2.c


base-commit: 880977fdc7f67923d1904ee23ca75fa1e375ea46
-- 
2.47.3

[PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible

[Binbin Zhou]

Add "loongson,ls2k0300-i2c" dedicated compatible for representing I2C of
Loongson-2K0300 chip, because its HW integration is quiet different from
others.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 .../bindings/i2c/loongson,ls2x-i2c.yaml           | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
index 67882ec6e06a..4c381559f7f0 100644
--- a/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
@@ -16,6 +16,7 @@ properties:
   compatible:
     enum:
       - loongson,ls2k-i2c
+      - loongson,ls2k0300-i2c
       - loongson,ls7a-i2c
 
   reg:
@@ -24,6 +25,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -31,6 +35,17 @@ required:
 
 unevaluatedProperties: false
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - loongson,ls2k0300-i2c
+
+then:
+  required:
+    - clocks
+
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
-- 
2.47.3

[PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Binbin Zhou]

This I2C module is integrated into the Loongson-2K0300 SoCs.

It provides multi-master functionality and controls all I2C bus-specific
timing, protocols, arbitration, and timing. It supports both standard
and fast modes.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 MAINTAINERS                      |   1 +
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-ls2x-v2.c | 545 +++++++++++++++++++++++++++++++
 4 files changed, 557 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x-v2.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5b11839cba9d..01fd37fdd29b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14783,6 +14783,7 @@ M:	Binbin Zhou <zhoubinbin@loongson.cn>
 L:	linux-i2c@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
+F:	drivers/i2c/busses/i2c-ls2x-v2.c
 F:	drivers/i2c/busses/i2c-ls2x.c
 
 LOONGSON PWM DRIVER
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 09ba55bae1fa..d409401c9c94 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -857,6 +857,16 @@ config I2C_LS2X
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-ls2x.
 
+config I2C_LS2X_V2
+	tristate "Loongson-2 Fast Speed I2C adapter"
+	depends on LOONGARCH || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Loongson-2K0300 SoCs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-ls2x-v2.
+
 config I2C_MLXBF
         tristate "Mellanox BlueField I2C controller"
         depends on (MELLANOX_PLATFORM && ARM64) || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fb985769f5ff..8cdfc30b79e9 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_I2C_KEBA)		+= i2c-keba.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
 obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
 obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
+obj-$(CONFIG_I2C_LS2X_V2)	+= i2c-ls2x-v2.o
 obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
 obj-$(CONFIG_I2C_MICROCHIP_CORE)	+= i2c-microchip-corei2c.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
diff --git a/drivers/i2c/busses/i2c-ls2x-v2.c b/drivers/i2c/busses/i2c-ls2x-v2.c
new file mode 100644
index 000000000000..93d24c9482f0
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x-v2.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Loongson-2K fast I2C controller driver
+ *
+ * Copyright (C) 2025-2026 Loongson Technology Corporation Limited
+ *
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+
+/* Loongson-2 fast I2C offset registers */
+#define LOONGSON2_I2C_CR1	0x00	/* I2C control 1 register */
+#define LOONGSON2_I2C_CR2	0x04	/* I2C control 2 register */
+#define LOONGSON2_I2C_OAR	0x08	/* I2C slave address register */
+#define LOONGSON2_I2C_DR	0x10	/* I2C data register */
+#define LOONGSON2_I2C_SR1	0x14	/* I2C status 1 register */
+#define LOONGSON2_I2C_SR2	0x18	/* I2C status 2 register */
+#define LOONGSON2_I2C_CCR	0x1C	/* I2C clock control register */
+#define LOONGSON2_I2C_TRISE	0x20	/* I2C trise register */
+#define LOONGSON2_I2C_FLTR	0x24
+
+/* Bitfields of I2C control 1 register */
+#define LOONGSON2_I2C_CR1_PE		BIT(0)
+#define LOONGSON2_I2C_CR1_START		BIT(8)
+#define LOONGSON2_I2C_CR1_STOP		BIT(9)
+#define LOONGSON2_I2C_CR1_ACK		BIT(10)
+#define LOONGSON2_I2C_CR1_POS		BIT(11)
+
+#define LOONGSON2_I2C_CR1_OP_MASK	(LOONGSON2_I2C_CR1_START | LOONGSON2_I2C_CR1_STOP)
+
+/* Bitfields of I2C control 2 register */
+#define LOONGSON2_I2C_CR2_FREQ		GENMASK(5, 0)
+#define LOONGSON2_I2C_CR2_ITERREN	BIT(8)
+#define LOONGSON2_I2C_CR2_ITEVTEN	BIT(9)
+#define LOONGSON2_I2C_CR2_ITBUFEN	BIT(10)
+
+#define LOONGSON2_I2C_CR2_IRQ_MASK	(LOONGSON2_I2C_CR2_ITBUFEN | \
+					 LOONGSON2_I2C_CR2_ITEVTEN | \
+					 LOONGSON2_I2C_CR2_ITERREN)
+
+/* Bitfields of I2C status 1 register */
+#define LOONGSON2_I2C_SR1_SB		BIT(0)
+#define LOONGSON2_I2C_SR1_ADDR		BIT(1)
+#define LOONGSON2_I2C_SR1_BTF		BIT(2)
+#define LOONGSON2_I2C_SR1_RXNE		BIT(6)
+#define LOONGSON2_I2C_SR1_TXE		BIT(7)
+#define LOONGSON2_I2C_SR1_BERR		BIT(8)
+#define LOONGSON2_I2C_SR1_ARLO		BIT(9)
+#define LOONGSON2_I2C_SR1_AF		BIT(10)
+
+#define LOONGSON2_I2C_SR1_ITEVTEN_MASK	(LOONGSON2_I2C_SR1_BTF | \
+					 LOONGSON2_I2C_SR1_ADDR | \
+					 LOONGSON2_I2C_SR1_SB)
+#define LOONGSON2_I2C_SR1_ITBUFEN_MASK	(LOONGSON2_I2C_SR1_TXE | LOONGSON2_I2C_SR1_RXNE)
+#define LOONGSON2_I2C_SR1_ITERREN_MASK	(LOONGSON2_I2C_SR1_AF | \
+					 LOONGSON2_I2C_SR1_ARLO | \
+					 LOONGSON2_I2C_SR1_BERR)
+
+/* Bitfields of I2C status 2 register */
+#define LOONGSON2_I2C_SR2_BUSY		BIT(1)
+
+/* Bitfields of I2C clock control register */
+#define LOONGSON2_I2C_CCR_CCR		GENMASK(11, 0)
+#define LOONGSON2_I2C_CCR_DUTY		BIT(14)
+#define LOONGSON2_I2C_CCR_FS		BIT(15)
+
+/* Bitfields of I2C trise register */
+#define LOONGSON2_I2C_TRISE_SCL		GENMASK(5, 0)
+
+#define LOONGSON2_I2C_FREE_SLEEP_US	1000
+#define LOONGSON2_I2C_FREE_TIMEOUT_US	5000
+
+#define LOONGSON2_I2C_MIN_STD_FREQ	2U
+#define LOONGSON2_I2C_MIN_FAST_FREQ	6U
+#define LOONGSON2_I2C_MAX_FREQ		46U
+#define HZ_TO_MHZ			1000000
+
+enum loongson2_i2c_speed {
+	LOONGSON2_I2C_SPEED_STANDARD, /* 100 kHz */
+	LOONGSON2_I2C_SPEED_FAST, /* 400 kHz */
+	LOONGSON2_I2C_SPEED_FAST_PLUS, /* 1 MHz */
+	LOONGSON2_I2C_SPEED_END,
+};
+
+/*
+ * struct loongson2_i2c_msg - client specific data
+ * @addr: 8-bit slave addr, including r/w bit
+ * @count: number of bytes to be transferred
+ * @buf: data buffer
+ * @stop: last I2C msg to be sent, i.e. STOP to be generated
+ * @result: result of the transfer
+ */
+struct loongson2_i2c_msg {
+	u8 addr;
+	u32 count;
+	u8 *buf;
+	bool stop;
+	int result;
+};
+
+/*
+ * struct loongson2_i2c_priv - private data of the controller
+ * @adapter: I2C adapter for this controller
+ * @dev: device for this controller
+ * @clk: hw i2c clock
+ * @complete: completion of I2C message
+ * @regmap: regmap of the I2C device
+ * @speed: I2C clock frequency of the controller. Standard or Fast are supported
+ * @parent_rate: I2C clock parent rate in MHz
+ * @msg: I2C transfer information
+ */
+struct loongson2_i2c_priv {
+	struct i2c_adapter adapter;
+	struct device *dev;
+	struct clk *clk;
+	struct completion complete;
+	struct regmap *regmap;
+	int speed;
+	int parent_rate;
+	struct loongson2_i2c_msg msg;
+};
+
+static void loongson2_i2c_disable_irq(struct loongson2_i2c_priv *priv)
+{
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_IRQ_MASK, 0);
+}
+
+static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
+{
+	u32 status;
+	int ret;
+
+	ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
+				       !(status & LOONGSON2_I2C_SR2_BUSY),
+				       LOONGSON2_I2C_FREE_SLEEP_US,
+				       LOONGSON2_I2C_FREE_TIMEOUT_US);
+	if (ret) {
+		dev_dbg(priv->dev, "I2C bus free failed.\n");
+		ret = -EBUSY;
+	}
+
+	return ret;
+}
+
+static void loongson2_i2c_read_msg(struct loongson2_i2c_priv *priv)
+{
+	struct loongson2_i2c_msg *msg = &priv->msg;
+	u32 rbuf;
+
+	regmap_read(priv->regmap, LOONGSON2_I2C_DR, &rbuf);
+	*msg->buf++ = rbuf;
+	msg->count--;
+}
+
+static void loongson2_i2c_write_msg(struct loongson2_i2c_priv *priv, u8 byte)
+{
+	regmap_write(priv->regmap, LOONGSON2_I2C_DR, byte);
+}
+
+static void loongson2_i2c_terminate_xfer(struct loongson2_i2c_priv *priv)
+{
+	struct loongson2_i2c_msg *msg = &priv->msg;
+
+	loongson2_i2c_disable_irq(priv);
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
+			   msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
+	complete(&priv->complete);
+}
+
+static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
+{
+	struct loongson2_i2c_msg *msg = &priv->msg;
+	bool changed;
+	int i;
+
+	switch (msg->count) {
+	case 1:
+		/* only transmit 1 bytes condition */
+		loongson2_i2c_disable_irq(priv);
+		loongson2_i2c_read_msg(priv);
+		complete(&priv->complete);
+		break;
+	case 2:
+		if (flag != 1) {
+			/* ensure only transmit 2 bytes condition */
+			regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
+					   LOONGSON2_I2C_CR2_ITBUFEN, 0);
+			break;
+		}
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
+				   msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
+
+		loongson2_i2c_disable_irq(priv);
+
+		for (i = 2; i > 0; i--)
+			loongson2_i2c_read_msg(priv);
+
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
+		complete(&priv->complete);
+		break;
+	case 3:
+		regmap_update_bits_check(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
+					 0, &changed);
+		if (changed)
+			break;
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
+		fallthrough;
+	default:
+		loongson2_i2c_read_msg(priv);
+	}
+}
+
+static void loongson2_i2c_handle_write(struct loongson2_i2c_priv *priv)
+{
+	struct loongson2_i2c_msg *msg = &priv->msg;
+
+	if (msg->count) {
+		loongson2_i2c_write_msg(priv, *msg->buf++);
+		msg->count--;
+		if (!msg->count)
+			regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
+					   LOONGSON2_I2C_CR2_ITBUFEN, 0);
+	} else {
+		loongson2_i2c_terminate_xfer(priv);
+	}
+}
+
+static void loongson2_i2c_handle_rx_addr(struct loongson2_i2c_priv *priv)
+{
+	struct loongson2_i2c_msg *msg = &priv->msg;
+
+	switch (msg->count) {
+	case 0:
+		loongson2_i2c_terminate_xfer(priv);
+		break;
+	case 1:
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1,
+				   LOONGSON2_I2C_CR1_ACK | LOONGSON2_I2C_CR1_POS, 0);
+		/* start or stop */
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
+				   msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
+		break;
+	case 2:
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS,
+				   LOONGSON2_I2C_CR1_POS);
+		break;
+
+	default:
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK,
+				   LOONGSON2_I2C_CR1_ACK);
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
+	}
+}
+
+static irqreturn_t loongson2_i2c_isr_error(u32 status, void *data)
+{
+	struct loongson2_i2c_priv *priv = data;
+	struct loongson2_i2c_msg *msg = &priv->msg;
+
+	/* Arbitration lost */
+	if (status & LOONGSON2_I2C_SR1_ARLO) {
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_ARLO, 0);
+		msg->result = -EAGAIN;
+	}
+
+	/*
+	 * Acknowledge failure:
+	 * In master transmitter mode a Stop must be generated by software
+	 */
+	if (status & LOONGSON2_I2C_SR1_AF) {
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_STOP,
+				   LOONGSON2_I2C_CR1_STOP);
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_AF, 0);
+		msg->result = -EIO;
+	}
+
+	/* Bus error */
+	if (status & LOONGSON2_I2C_SR1_BERR) {
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_BERR, 0);
+		msg->result = -EIO;
+	}
+
+	loongson2_i2c_disable_irq(priv);
+	complete(&priv->complete);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
+{
+	u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
+	struct loongson2_i2c_priv *priv = data;
+	struct loongson2_i2c_msg *msg = &priv->msg;
+	u32 status, ien, event, cr2;
+
+	regmap_read(priv->regmap, LOONGSON2_I2C_SR1, &status);
+	if (status & LOONGSON2_I2C_SR1_ITERREN_MASK)
+		return loongson2_i2c_isr_error(status, data);
+
+	regmap_read(priv->regmap, LOONGSON2_I2C_CR2, &cr2);
+	ien = cr2 & LOONGSON2_I2C_CR2_IRQ_MASK;
+
+	/* Update possible_status if buffer interrupt is enabled */
+	if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
+		possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
+
+	event = status & possible_status;
+	if (!event) {
+		dev_dbg(priv->dev, "spurious evt irq (status=0x%08x, ien=0x%08x)\n", status, ien);
+		return IRQ_NONE;
+	}
+
+	/* Start condition generated */
+	if (event & LOONGSON2_I2C_SR1_SB)
+		loongson2_i2c_write_msg(priv, msg->addr);
+
+	/* I2C Address sent */
+	if (event & LOONGSON2_I2C_SR1_ADDR) {
+		if (msg->addr & I2C_M_RD)
+			loongson2_i2c_handle_rx_addr(priv);
+		/* Clear ADDR flag */
+		regmap_read(priv->regmap, LOONGSON2_I2C_SR2, &status);
+		/* Enable buffer interrupts for RX/TX not empty events */
+		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
+				   LOONGSON2_I2C_CR2_ITBUFEN);
+	}
+
+	if (msg->addr & I2C_M_RD) {
+		/* RX not empty */
+		if (event & LOONGSON2_I2C_SR1_RXNE)
+			loongson2_i2c_handle_read(priv, 0);
+
+		if (event & LOONGSON2_I2C_SR1_BTF)
+			loongson2_i2c_handle_read(priv, 1);
+	} else {
+		/* TX empty */
+		if (event & LOONGSON2_I2C_SR1_TXE)
+			loongson2_i2c_handle_write(priv);
+
+		if (event & LOONGSON2_I2C_SR1_BTF)
+			loongson2_i2c_handle_write(priv);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int loongson2_i2c_xfer_msg(struct loongson2_i2c_priv *priv, struct i2c_msg *msg,
+				  bool is_stop)
+{
+	struct loongson2_i2c_msg *l_msg = &priv->msg;
+	unsigned long timeout;
+	int ret;
+
+	l_msg->addr   = i2c_8bit_addr_from_msg(msg);
+	l_msg->buf    = msg->buf;
+	l_msg->count  = msg->len;
+	l_msg->stop   = is_stop;
+	l_msg->result = 0;
+
+	reinit_completion(&priv->complete);
+
+	/* Enable events and errors interrupts */
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
+			   LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN,
+			   LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN);
+
+	timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
+	ret = l_msg->result;
+
+	if (!timeout)
+		ret = -ETIMEDOUT;
+
+	return ret;
+}
+
+static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num)
+{
+	struct loongson2_i2c_priv *priv = i2c_get_adapdata(i2c_adap);
+	int ret = 0, i;
+
+	ret = loongson2_i2c_wait_free_bus(priv);
+	if (ret)
+		return ret;
+
+	/* START generation */
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_START,
+			   LOONGSON2_I2C_CR1_START);
+
+	for (i = 0; i < num && !ret; i++)
+		ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
+
+	return (ret < 0) ? ret : num;
+}
+
+static u32 loongson2_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm loongson2_i2c_algo = {
+	.master_xfer = loongson2_i2c_xfer,
+	.functionality = loongson2_i2c_func,
+};
+
+static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
+{
+	u32 val, freq, ccr = 0, cr2 = 0;
+
+	priv->parent_rate = clk_get_rate(priv->clk);
+	freq = DIV_ROUND_UP(priv->parent_rate, HZ_TO_MHZ);
+
+	cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);
+	regmap_write(priv->regmap, LOONGSON2_I2C_CR2, cr2);
+
+	if (priv->speed == LOONGSON2_I2C_SPEED_STANDARD) {
+		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);
+	} else {
+		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
+
+		/* Select Fast mode */
+		ccr |= LOONGSON2_I2C_CCR_FS;
+	}
+
+	ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);
+	regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
+
+	/* reference clock determination the configure val(0x3f) */
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
+			   LOONGSON2_I2C_CR2_FREQ);
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_TRISE, LOONGSON2_I2C_TRISE_SCL,
+			   LOONGSON2_I2C_TRISE_SCL);
+
+	/* Enable I2C */
+	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_PE,
+			   LOONGSON2_I2C_CR1_PE);
+
+	return 0;
+}
+
+static const struct regmap_config loongson2_i2c_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = LOONGSON2_I2C_TRISE,
+};
+
+static int loongson2_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct loongson2_i2c_priv *priv;
+	struct i2c_adapter *adap;
+	void __iomem *base;
+	u32 clk_rate;
+	int irq, ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base),
+				     "Failed to ioremap resource.\n");
+
+	priv->regmap = devm_regmap_init_mmio(dev, base,
+					     &loongson2_i2c_regmap_config);
+	if (IS_ERR(priv->regmap))
+		return dev_err_probe(dev, PTR_ERR(priv->regmap),
+				     "Failed to init regmap.\n");
+
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "Failed  to enable clock.\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -EINVAL;
+
+	priv->dev = dev;
+
+	adap = &priv->adapter;
+	adap->retries = 5;
+	adap->nr = pdev->id;
+	adap->dev.parent = dev;
+	adap->owner = THIS_MODULE;
+	adap->algo = &loongson2_i2c_algo;
+	adap->timeout = 2 * HZ;
+	device_set_node(&adap->dev, dev_fwnode(dev));
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->complete);
+	platform_set_drvdata(pdev, priv);
+
+	priv->speed = LOONGSON2_I2C_SPEED_STANDARD;
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);
+	if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
+		priv->speed = LOONGSON2_I2C_SPEED_FAST;
+
+	ret = loongson2_i2c_adjust_bus_speed(priv);
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(dev, irq, loongson2_i2c_isr_event, IRQF_SHARED, pdev->name, priv);
+	if (ret)
+		return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
+
+	return devm_i2c_add_adapter(dev, adap);
+}
+
+static const struct of_device_id loongson2_i2c_id_table[] = {
+	{ .compatible = "loongson,ls2k0300-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, loongson2_i2c_id_table);
+
+static struct platform_driver loongson2_i2c_driver = {
+	.driver = {
+		.name = "loongson2-i2c-v2",
+		.of_match_table = loongson2_i2c_id_table,
+	},
+	.probe = loongson2_i2c_probe,
+};
+
+module_platform_driver(loongson2_i2c_driver);
+
+MODULE_DESCRIPTION("Loongson-2K0300 I2C bus driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");
-- 
2.47.3

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko]

On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:
> This I2C module is integrated into the Loongson-2K0300 SoCs.
> 
> It provides multi-master functionality and controls all I2C bus-specific
> timing, protocols, arbitration, and timing. It supports both standard
> and fast modes.

Thanks for the update, but either v1 was not reviewed properly (if reviewed
at all), or this missed comments from there. This driver needs much more work.
Since it misses v6.20 anyway, you have about 2 month to put this into a shape.

...

> +/*
> + * Loongson-2K fast I2C controller driver
> + *
> + * Copyright (C) 2025-2026 Loongson Technology Corporation Limited

> + *

Redundant blank line.

> + */

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>

> +#include <linux/device.h>

Is this being used?

> +#include <linux/iopoll.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>

> +#include <linux/kernel.h>

No way you should include this one.

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

Please, follow IWYU principle, exempli gratia here the types.h is missing.

> +#include <linux/units.h>

...

> +#define LOONGSON2_I2C_CR2_IRQ_MASK	(LOONGSON2_I2C_CR2_ITBUFEN | \
> +					 LOONGSON2_I2C_CR2_ITEVTEN | \
> +					 LOONGSON2_I2C_CR2_ITERREN)

Better to indent differently:

#define LOONGSON2_I2C_CR2_IRQ_MASK	\
	(LOONGSON2_I2C_CR2_ITBUFEN | LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN)


...

> +#define LOONGSON2_I2C_SR1_ITEVTEN_MASK	(LOONGSON2_I2C_SR1_BTF | \
> +					 LOONGSON2_I2C_SR1_ADDR | \
> +					 LOONGSON2_I2C_SR1_SB)
> +#define LOONGSON2_I2C_SR1_ITBUFEN_MASK	(LOONGSON2_I2C_SR1_TXE | LOONGSON2_I2C_SR1_RXNE)
> +#define LOONGSON2_I2C_SR1_ITERREN_MASK	(LOONGSON2_I2C_SR1_AF | \
> +					 LOONGSON2_I2C_SR1_ARLO | \
> +					 LOONGSON2_I2C_SR1_BERR)

As per above.

...

> +#define LOONGSON2_I2C_FREE_SLEEP_US	1000
> +#define LOONGSON2_I2C_FREE_TIMEOUT_US	5000

1 * USEC_PER_MSEC
5 * USEC_PER_MSEC

...

> +#define LOONGSON2_I2C_MIN_STD_FREQ	2U
> +#define LOONGSON2_I2C_MIN_FAST_FREQ	6U
> +#define LOONGSON2_I2C_MAX_FREQ		46U

The three are unused, what are they for?


> +#define HZ_TO_MHZ			1000000

Dup. Use units.h.

...

> +enum loongson2_i2c_speed {
> +	LOONGSON2_I2C_SPEED_STANDARD, /* 100 kHz */
> +	LOONGSON2_I2C_SPEED_FAST, /* 400 kHz */
> +	LOONGSON2_I2C_SPEED_FAST_PLUS, /* 1 MHz */

Use existing speed definitions and drop this enum.

> +	LOONGSON2_I2C_SPEED_END,

Besides comma in the terminator, that has not to be there, this is unused.

> +};

...

> +/*

It's not marked as kernel-doc, but looks very much like that. Why?
Same Q for *all* cases like this.

> + * struct loongson2_i2c_msg - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transferred
> + * @buf: data buffer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + * @result: result of the transfer
> + */
> +struct loongson2_i2c_msg {
> +	u8 addr;
> +	u32 count;
> +	u8 *buf;
> +	bool stop;
> +	int result;

Run `pahole` and amend *all* data types accordingly.

> +};

...

> +struct loongson2_i2c_priv {
> +	struct i2c_adapter adapter;

> +	struct device *dev;

Isn't this a dup? Can't it be derived from regmap and/or adapter?

> +	struct clk *clk;
> +	struct completion complete;
> +	struct regmap *regmap;
> +	int speed;
> +	int parent_rate;
> +	struct loongson2_i2c_msg msg;
> +};

...

> +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> +{
> +	u32 status;
> +	int ret;
> +
> +	ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> +				       !(status & LOONGSON2_I2C_SR2_BUSY),
> +				       LOONGSON2_I2C_FREE_SLEEP_US,
> +				       LOONGSON2_I2C_FREE_TIMEOUT_US);
> +	if (ret) {
> +		dev_dbg(priv->dev, "I2C bus free failed.\n");

> +		ret = -EBUSY;

Why?! What's wrong with the error code returned in ret?

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

...

> +static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
> +{
> +	struct loongson2_i2c_msg *msg = &priv->msg;
> +	bool changed;

> +	int i;

Why signed?

> +	switch (msg->count) {
> +	case 1:
> +		/* only transmit 1 bytes condition */
> +		loongson2_i2c_disable_irq(priv);
> +		loongson2_i2c_read_msg(priv);
> +		complete(&priv->complete);
> +		break;
> +	case 2:
> +		if (flag != 1) {
> +			/* ensure only transmit 2 bytes condition */
> +			regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +					   LOONGSON2_I2C_CR2_ITBUFEN, 0);
> +			break;
> +		}
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
> +				   msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
> +
> +		loongson2_i2c_disable_irq(priv);

> +		for (i = 2; i > 0; i--)
> +			loongson2_i2c_read_msg(priv);

Just unroll the loop and put a comment on top explaining the magic 2. But I
think it's just a msg->count.

> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
> +		complete(&priv->complete);
> +		break;
> +	case 3:
> +		regmap_update_bits_check(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> +					 0, &changed);
> +		if (changed)
> +			break;
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
> +		fallthrough;
> +	default:
> +		loongson2_i2c_read_msg(priv);
> +	}
> +}

...

> +static irqreturn_t loongson2_i2c_isr_error(u32 status, void *data)
> +{
> +	struct loongson2_i2c_priv *priv = data;
> +	struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +	/* Arbitration lost */
> +	if (status & LOONGSON2_I2C_SR1_ARLO) {
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_ARLO, 0);
> +		msg->result = -EAGAIN;
> +	}
> +
> +	/*
> +	 * Acknowledge failure:
> +	 * In master transmitter mode a Stop must be generated by software
> +	 */
> +	if (status & LOONGSON2_I2C_SR1_AF) {
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_STOP,
> +				   LOONGSON2_I2C_CR1_STOP);
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_AF, 0);
> +		msg->result = -EIO;
> +	}
> +
> +	/* Bus error */
> +	if (status & LOONGSON2_I2C_SR1_BERR) {
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_BERR, 0);
> +		msg->result = -EIO;
> +	}

Even if the interrupt is spurious you still Ack it, why?

Ah, it seems it's a helper. Please, return the error code from it as int and
not irqreturn_t, this will make things clearer.

> +	loongson2_i2c_disable_irq(priv);
> +	complete(&priv->complete);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
> +{
> +	u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;

Split assignment...

> +	struct loongson2_i2c_priv *priv = data;
> +	struct loongson2_i2c_msg *msg = &priv->msg;
> +	u32 status, ien, event, cr2;
> +
> +	regmap_read(priv->regmap, LOONGSON2_I2C_SR1, &status);
> +	if (status & LOONGSON2_I2C_SR1_ITERREN_MASK)
> +		return loongson2_i2c_isr_error(status, data);
> +
> +	regmap_read(priv->regmap, LOONGSON2_I2C_CR2, &cr2);
> +	ien = cr2 & LOONGSON2_I2C_CR2_IRQ_MASK;

...to be here, which improves readability (no need to go somewhere up in
the code to see what this is about.

> +	/* Update possible_status if buffer interrupt is enabled */
> +	if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> +		possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
> +
> +	event = status & possible_status;
> +	if (!event) {
> +		dev_dbg(priv->dev, "spurious evt irq (status=0x%08x, ien=0x%08x)\n", status, ien);
> +		return IRQ_NONE;
> +	}
> +
> +	/* Start condition generated */
> +	if (event & LOONGSON2_I2C_SR1_SB)
> +		loongson2_i2c_write_msg(priv, msg->addr);
> +
> +	/* I2C Address sent */
> +	if (event & LOONGSON2_I2C_SR1_ADDR) {
> +		if (msg->addr & I2C_M_RD)
> +			loongson2_i2c_handle_rx_addr(priv);
> +		/* Clear ADDR flag */
> +		regmap_read(priv->regmap, LOONGSON2_I2C_SR2, &status);
> +		/* Enable buffer interrupts for RX/TX not empty events */
> +		regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> +				   LOONGSON2_I2C_CR2_ITBUFEN);
> +	}
> +
> +	if (msg->addr & I2C_M_RD) {
> +		/* RX not empty */
> +		if (event & LOONGSON2_I2C_SR1_RXNE)
> +			loongson2_i2c_handle_read(priv, 0);
> +
> +		if (event & LOONGSON2_I2C_SR1_BTF)
> +			loongson2_i2c_handle_read(priv, 1);
> +	} else {
> +		/* TX empty */
> +		if (event & LOONGSON2_I2C_SR1_TXE)
> +			loongson2_i2c_handle_write(priv);
> +
> +		if (event & LOONGSON2_I2C_SR1_BTF)
> +			loongson2_i2c_handle_write(priv);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int loongson2_i2c_xfer_msg(struct loongson2_i2c_priv *priv, struct i2c_msg *msg,
> +				  bool is_stop)
> +{
> +	struct loongson2_i2c_msg *l_msg = &priv->msg;
> +	unsigned long timeout;

> +	int ret;

Seems unneeded.

> +
> +	l_msg->addr   = i2c_8bit_addr_from_msg(msg);
> +	l_msg->buf    = msg->buf;
> +	l_msg->count  = msg->len;
> +	l_msg->stop   = is_stop;
> +	l_msg->result = 0;
> +
> +	reinit_completion(&priv->complete);
> +
> +	/* Enable events and errors interrupts */
> +	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +			   LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN,
> +			   LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN);

> +	timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
> +	ret = l_msg->result;
> +
> +	if (!timeout)
> +		ret = -ETIMEDOUT;
> +
> +	return ret;

	timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
	if (!timeout)
		return -ETIMEDOUT;

	return l_msg->result;

> +}

> +static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num)
> +{
> +	struct loongson2_i2c_priv *priv = i2c_get_adapdata(i2c_adap);
> +	int ret = 0, i;

Why is 'i' signed?
And redundant assignment for 'ret'.

> +	ret = loongson2_i2c_wait_free_bus(priv);
> +	if (ret)
> +		return ret;
> +
> +	/* START generation */
> +	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_START,
> +			   LOONGSON2_I2C_CR1_START);

> +	for (i = 0; i < num && !ret; i++)
> +		ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
> +
> +	return (ret < 0) ? ret : num;

	for (i = 0; i < num; i++) {
		ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
		if (ret < 0)
			return ret;
	}

	return num;


> +}

...

> +static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
> +{
> +	u32 val, freq, ccr = 0, cr2 = 0;

Redundant assignment(s), see below why.

> +	priv->parent_rate = clk_get_rate(priv->clk);
> +	freq = DIV_ROUND_UP(priv->parent_rate, HZ_TO_MHZ);

Use unit suffix:

	freq_mhz = ...

(if I am not mistaken).

> +	cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);

	cr2 = FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);

> +	regmap_write(priv->regmap, LOONGSON2_I2C_CR2, cr2);
> +
> +	if (priv->speed == LOONGSON2_I2C_SPEED_STANDARD) {
> +		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);

		/* Select Standard mode */
		ccr = 0;

> +	} else {
> +		val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
> +
> +		/* Select Fast mode */
> +		ccr |= LOONGSON2_I2C_CCR_FS;

		ccr = LOONGSON2_I2C_CCR_FS;

> +	}

> +	ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);

FIELD_MODIFY()?

> +	regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
> +
> +	/* reference clock determination the configure val(0x3f) */
> +	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
> +			   LOONGSON2_I2C_CR2_FREQ);
> +	regmap_update_bits(priv->regmap, LOONGSON2_I2C_TRISE, LOONGSON2_I2C_TRISE_SCL,
> +			   LOONGSON2_I2C_TRISE_SCL);
> +
> +	/* Enable I2C */
> +	regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_PE,
> +			   LOONGSON2_I2C_CR1_PE);
> +
> +	return 0;
> +}

...

> +static int loongson2_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct loongson2_i2c_priv *priv;
> +	struct i2c_adapter *adap;
> +	void __iomem *base;
> +	u32 clk_rate;
> +	int irq, ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))

> +		return dev_err_probe(dev, PTR_ERR(base),
> +				     "Failed to ioremap resource.\n");

You utilise 100, put this on a single line.

But, this is a dup, devm_platform_*() call above takes care already about error
message.

> +
> +	priv->regmap = devm_regmap_init_mmio(dev, base,
> +					     &loongson2_i2c_regmap_config);

Single line.

> +	if (IS_ERR(priv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +				     "Failed to init regmap.\n");

Single line.

> +	priv->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(priv->clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->clk),
> +				     "Failed  to enable clock.\n");

Single line.

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return -EINVAL;

Why?! What's wrong with the error code in 'irq'?

> +	priv->dev = dev;
> +
> +	adap = &priv->adapter;
> +	adap->retries = 5;
> +	adap->nr = pdev->id;
> +	adap->dev.parent = dev;
> +	adap->owner = THIS_MODULE;
> +	adap->algo = &loongson2_i2c_algo;
> +	adap->timeout = 2 * HZ;
> +	device_set_node(&adap->dev, dev_fwnode(dev));
> +	i2c_set_adapdata(adap, priv);

> +	strscpy(adap->name, pdev->name, sizeof(adap->name));

2-arguments version is even better.

> +	init_completion(&priv->complete);
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->speed = LOONGSON2_I2C_SPEED_STANDARD;

> +	ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);

device_property_read_u32()

> +	if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
> +		priv->speed = LOONGSON2_I2C_SPEED_FAST;
> +
> +	ret = loongson2_i2c_adjust_bus_speed(priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_request_irq(dev, irq, loongson2_i2c_isr_event, IRQF_SHARED, pdev->name, priv);
> +	if (ret)

> +		return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);

Dup message.

		return ret;

> +	return devm_i2c_add_adapter(dev, adap);
> +}

...

> +static struct platform_driver loongson2_i2c_driver = {
> +	.driver = {
> +		.name = "loongson2-i2c-v2",
> +		.of_match_table = loongson2_i2c_id_table,
> +	},
> +	.probe = loongson2_i2c_probe,
> +};

> +

Remove this blank line.

> +module_platform_driver(loongson2_i2c_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Binbin Zhou]

Hi Andy:

Thanks for your detailed review.

On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:
> > This I2C module is integrated into the Loongson-2K0300 SoCs.
> >
> > It provides multi-master functionality and controls all I2C bus-specific
> > timing, protocols, arbitration, and timing. It supports both standard
> > and fast modes.
>
> Thanks for the update, but either v1 was not reviewed properly (if reviewed
> at all), or this missed comments from there. This driver needs much more work.
> Since it misses v6.20 anyway, you have about 2 month to put this into a shape.
>
> ...
>
> > +/*
> > + * Loongson-2K fast I2C controller driver
> > + *
> > + * Copyright (C) 2025-2026 Loongson Technology Corporation Limited
>
> > + *
>
> Redundant blank line.

ok..
>
> > + */
>
> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
>
> > +#include <linux/device.h>
>
> Is this being used?
>
> > +#include <linux/iopoll.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
>
> > +#include <linux/kernel.h>
>
> No way you should include this one.
>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
>
> Please, follow IWYU principle, exempli gratia here the types.h is missing.

I will check the included header files.
>
> > +#include <linux/units.h>
>
> ...
>
> > +#define LOONGSON2_I2C_CR2_IRQ_MASK   (LOONGSON2_I2C_CR2_ITBUFEN | \
> > +                                      LOONGSON2_I2C_CR2_ITEVTEN | \
> > +                                      LOONGSON2_I2C_CR2_ITERREN)
>
> Better to indent differently:
>
> #define LOONGSON2_I2C_CR2_IRQ_MASK      \
>         (LOONGSON2_I2C_CR2_ITBUFEN | LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN)

ok. I will check.
>
>
> ...
>
> > +#define LOONGSON2_I2C_SR1_ITEVTEN_MASK       (LOONGSON2_I2C_SR1_BTF | \
> > +                                      LOONGSON2_I2C_SR1_ADDR | \
> > +                                      LOONGSON2_I2C_SR1_SB)
> > +#define LOONGSON2_I2C_SR1_ITBUFEN_MASK       (LOONGSON2_I2C_SR1_TXE | LOONGSON2_I2C_SR1_RXNE)
> > +#define LOONGSON2_I2C_SR1_ITERREN_MASK       (LOONGSON2_I2C_SR1_AF | \
> > +                                      LOONGSON2_I2C_SR1_ARLO | \
> > +                                      LOONGSON2_I2C_SR1_BERR)
>
> As per above.
>
> ...
>
> > +#define LOONGSON2_I2C_FREE_SLEEP_US  1000
> > +#define LOONGSON2_I2C_FREE_TIMEOUT_US        5000
>
> 1 * USEC_PER_MSEC
> 5 * USEC_PER_MSEC

ok...
>
> ...
>
> > +#define LOONGSON2_I2C_MIN_STD_FREQ   2U
> > +#define LOONGSON2_I2C_MIN_FAST_FREQ  6U
> > +#define LOONGSON2_I2C_MAX_FREQ               46U
>
> The three are unused, what are they for?

I will drop it.
>
>
> > +#define HZ_TO_MHZ                    1000000
>
> Dup. Use units.h.
>
> ...
>
> > +enum loongson2_i2c_speed {
> > +     LOONGSON2_I2C_SPEED_STANDARD, /* 100 kHz */
> > +     LOONGSON2_I2C_SPEED_FAST, /* 400 kHz */
> > +     LOONGSON2_I2C_SPEED_FAST_PLUS, /* 1 MHz */
>
> Use existing speed definitions and drop this enum.

ok....
>
> > +     LOONGSON2_I2C_SPEED_END,
>
> Besides comma in the terminator, that has not to be there, this is unused.
>
> > +};
>
> ...
>
> > +/*
>
> It's not marked as kernel-doc, but looks very much like that. Why?
> Same Q for *all* cases like this.

I didn't intend to mark it in kernel-doc; I just wanted to comment on
the variable.
Is removing the `@` symbol sufficient?

>
> > + * struct loongson2_i2c_msg - client specific data
> > + * @addr: 8-bit slave addr, including r/w bit
> > + * @count: number of bytes to be transferred
> > + * @buf: data buffer
> > + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> > + * @result: result of the transfer
> > + */
> > +struct loongson2_i2c_msg {
> > +     u8 addr;
> > +     u32 count;
> > +     u8 *buf;
> > +     bool stop;
> > +     int result;
>
> Run `pahole` and amend *all* data types accordingly.

 Moving 'stop' from after 'buf' to after 'addr'.
>
> > +};
>
> ...
>
> > +struct loongson2_i2c_priv {
> > +     struct i2c_adapter adapter;
>
> > +     struct device *dev;
>
> Isn't this a dup? Can't it be derived from regmap and/or adapter?

 it can be converted using `struct device *dev = priv->adapter.dev.parent;`
>
> > +     struct clk *clk;
> > +     struct completion complete;
> > +     struct regmap *regmap;
> > +     int speed;
> > +     int parent_rate;
> > +     struct loongson2_i2c_msg msg;
> > +};
>
> ...

After run `pahole`, the structs are reorganized as follow:

pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
i2c-ls2x-v2.o
struct loongson2_i2c_priv {
        struct i2c_adapter         adapter
__attribute__((__aligned__(8))); /*     0  1064 */
        /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
*/
        struct clk *               clk;                  /*  1064
8 */
        struct completion          complete;             /*  1072
32 */
        /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
*/
        struct regmap *            regmap;               /*  1104
8 */
        int                        speed;                /*  1112
4 */
        int                        parent_rate;          /*  1116
4 */
        struct loongson2_i2c_msg   msg;                  /*  1120
24 */

        /* XXX last struct has 4 bytes of padding */

        /* size: 1144, cachelines: 18, members: 7 */
        /* paddings: 1, sum paddings: 4 */
        /* forced alignments: 1 */
        /* last cacheline: 56 bytes */
} __attribute__((__aligned__(8)));

pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
i2c-ls2x-v2.o
struct loongson2_i2c_msg {
        u8                         addr;                 /*     0
1 */
        bool                       stop;                 /*     1
1 */

        /* XXX 2 bytes hole, try to pack */

        u32                        count;                /*     4
4 */
        u8 *                       buf;                  /*     8
8 */
        int                        result;               /*    16
4 */

        /* size: 24, cachelines: 1, members: 5 */
        /* sum members: 18, holes: 1, sum holes: 2 */
        /* padding: 4 */
        /* last cacheline: 24 bytes */
};

>
> > +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> > +{
> > +     u32 status;
> > +     int ret;
> > +
> > +     ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> > +                                    !(status & LOONGSON2_I2C_SR2_BUSY),
> > +                                    LOONGSON2_I2C_FREE_SLEEP_US,
> > +                                    LOONGSON2_I2C_FREE_TIMEOUT_US);
> > +     if (ret) {
> > +             dev_dbg(priv->dev, "I2C bus free failed.\n");
>
> > +             ret = -EBUSY;
>
> Why?! What's wrong with the error code returned in ret?

I want to indicate the bus busy state if it times out.
>
> > +     }
> > +
> > +     return ret;
> > +}
>
> ...
>
> > +static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
> > +{
> > +     struct loongson2_i2c_msg *msg = &priv->msg;
> > +     bool changed;
>
> > +     int i;
>
> Why signed?

unsigned int i;
>
> > +     switch (msg->count) {
> > +     case 1:
> > +             /* only transmit 1 bytes condition */
> > +             loongson2_i2c_disable_irq(priv);
> > +             loongson2_i2c_read_msg(priv);
> > +             complete(&priv->complete);
> > +             break;
> > +     case 2:
> > +             if (flag != 1) {
> > +                     /* ensure only transmit 2 bytes condition */
> > +                     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> > +                                        LOONGSON2_I2C_CR2_ITBUFEN, 0);
> > +                     break;
> > +             }
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
> > +                                msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
> > +
> > +             loongson2_i2c_disable_irq(priv);
>
> > +             for (i = 2; i > 0; i--)
> > +                     loongson2_i2c_read_msg(priv);
>
> Just unroll the loop and put a comment on top explaining the magic 2. But I
> think it's just a msg->count.

Yes it's  msg->count.
>
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
> > +             complete(&priv->complete);
> > +             break;
> > +     case 3:
> > +             regmap_update_bits_check(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> > +                                      0, &changed);
> > +             if (changed)
> > +                     break;
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
> > +             fallthrough;
> > +     default:
> > +             loongson2_i2c_read_msg(priv);
> > +     }
> > +}
>
> ...
>
> > +static irqreturn_t loongson2_i2c_isr_error(u32 status, void *data)
> > +{
> > +     struct loongson2_i2c_priv *priv = data;
> > +     struct loongson2_i2c_msg *msg = &priv->msg;
> > +
> > +     /* Arbitration lost */
> > +     if (status & LOONGSON2_I2C_SR1_ARLO) {
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_ARLO, 0);
> > +             msg->result = -EAGAIN;
> > +     }
> > +
> > +     /*
> > +      * Acknowledge failure:
> > +      * In master transmitter mode a Stop must be generated by software
> > +      */
> > +     if (status & LOONGSON2_I2C_SR1_AF) {
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_STOP,
> > +                                LOONGSON2_I2C_CR1_STOP);
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_AF, 0);
> > +             msg->result = -EIO;
> > +     }
> > +
> > +     /* Bus error */
> > +     if (status & LOONGSON2_I2C_SR1_BERR) {
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_BERR, 0);
> > +             msg->result = -EIO;
> > +     }
>
> Even if the interrupt is spurious you still Ack it, why?
>
> Ah, it seems it's a helper. Please, return the error code from it as int and
> not irqreturn_t, this will make things clearer.

How about just define it as void:

static void loongson2_i2c_isr_error(u32 status, void *data)
{
.........
        if (status & LOONGSON2_I2C_SR1_ARLO) {
         ........
                msg->result = -EAGAIN;
                goto out;
        }
        if (status & LOONGSON2_I2C_SR1_AF) {
           .......
                msg->result = -EIO;
                goto out;
        }
        if (status & LOONGSON2_I2C_SR1_BERR) {
         .........
                msg->result = -EIO;
                goto out;
        }

out:
        loongson2_i2c_disable_irq(priv);
        complete(&priv->complete);
}

and in loongson2_i2c_isr_event(), I reference it as:

        if (status & LOONGSON2_I2C_SR1_ITERREN_MASK) {
                loongson2_i2c_isr_error(status, data);
                return IRQ_NONE;
        }

Its return value is meaningless for loongson2_i2c_isr_event().

>
> > +     loongson2_i2c_disable_irq(priv);
> > +     complete(&priv->complete);
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
> > +{
> > +     u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
>
> Split assignment...
>
> > +     struct loongson2_i2c_priv *priv = data;
> > +     struct loongson2_i2c_msg *msg = &priv->msg;
> > +     u32 status, ien, event, cr2;
> > +
> > +     regmap_read(priv->regmap, LOONGSON2_I2C_SR1, &status);
> > +     if (status & LOONGSON2_I2C_SR1_ITERREN_MASK)
> > +             return loongson2_i2c_isr_error(status, data);
> > +
> > +     regmap_read(priv->regmap, LOONGSON2_I2C_CR2, &cr2);
> > +     ien = cr2 & LOONGSON2_I2C_CR2_IRQ_MASK;
>
> ...to be here, which improves readability (no need to go somewhere up in
> the code to see what this is about.

ok, I will put  `possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;` here.

>
> > +     /* Update possible_status if buffer interrupt is enabled */
> > +     if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> > +             possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
> > +
> > +     event = status & possible_status;
> > +     if (!event) {
> > +             dev_dbg(priv->dev, "spurious evt irq (status=0x%08x, ien=0x%08x)\n", status, ien);
> > +             return IRQ_NONE;
> > +     }
> > +
> > +     /* Start condition generated */
> > +     if (event & LOONGSON2_I2C_SR1_SB)
> > +             loongson2_i2c_write_msg(priv, msg->addr);
> > +
> > +     /* I2C Address sent */
> > +     if (event & LOONGSON2_I2C_SR1_ADDR) {
> > +             if (msg->addr & I2C_M_RD)
> > +                     loongson2_i2c_handle_rx_addr(priv);
> > +             /* Clear ADDR flag */
> > +             regmap_read(priv->regmap, LOONGSON2_I2C_SR2, &status);
> > +             /* Enable buffer interrupts for RX/TX not empty events */
> > +             regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> > +                                LOONGSON2_I2C_CR2_ITBUFEN);
> > +     }
> > +
> > +     if (msg->addr & I2C_M_RD) {
> > +             /* RX not empty */
> > +             if (event & LOONGSON2_I2C_SR1_RXNE)
> > +                     loongson2_i2c_handle_read(priv, 0);
> > +
> > +             if (event & LOONGSON2_I2C_SR1_BTF)
> > +                     loongson2_i2c_handle_read(priv, 1);
> > +     } else {
> > +             /* TX empty */
> > +             if (event & LOONGSON2_I2C_SR1_TXE)
> > +                     loongson2_i2c_handle_write(priv);
> > +
> > +             if (event & LOONGSON2_I2C_SR1_BTF)
> > +                     loongson2_i2c_handle_write(priv);
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static int loongson2_i2c_xfer_msg(struct loongson2_i2c_priv *priv, struct i2c_msg *msg,
> > +                               bool is_stop)
> > +{
> > +     struct loongson2_i2c_msg *l_msg = &priv->msg;
> > +     unsigned long timeout;
>
> > +     int ret;
>
> Seems unneeded.

emm, I will drop it.
>
> > +
> > +     l_msg->addr   = i2c_8bit_addr_from_msg(msg);
> > +     l_msg->buf    = msg->buf;
> > +     l_msg->count  = msg->len;
> > +     l_msg->stop   = is_stop;
> > +     l_msg->result = 0;
> > +
> > +     reinit_completion(&priv->complete);
> > +
> > +     /* Enable events and errors interrupts */
> > +     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> > +                        LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN,
> > +                        LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN);
>
> > +     timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
> > +     ret = l_msg->result;
> > +
> > +     if (!timeout)
> > +             ret = -ETIMEDOUT;
> > +
> > +     return ret;
>
>         timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
>         if (!timeout)
>                 return -ETIMEDOUT;
>
>         return l_msg->result;
>
> > +}
>
> > +static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num)
> > +{
> > +     struct loongson2_i2c_priv *priv = i2c_get_adapdata(i2c_adap);
> > +     int ret = 0, i;
>
> Why is 'i' signed?
> And redundant assignment for 'ret'.

unsigned int i;
int ret;
>
> > +     ret = loongson2_i2c_wait_free_bus(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* START generation */
> > +     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_START,
> > +                        LOONGSON2_I2C_CR1_START);
>
> > +     for (i = 0; i < num && !ret; i++)
> > +             ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
> > +
> > +     return (ret < 0) ? ret : num;
>
>         for (i = 0; i < num; i++) {
>                 ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
>                 if (ret < 0)
>                         return ret;
>         }
>
>         return num;

Yes, this way will be more readable.
>
>
> > +}
>
> ...
>
> > +static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
> > +{
> > +     u32 val, freq, ccr = 0, cr2 = 0;
>
> Redundant assignment(s), see below why.
>
> > +     priv->parent_rate = clk_get_rate(priv->clk);
> > +     freq = DIV_ROUND_UP(priv->parent_rate, HZ_TO_MHZ);
>
> Use unit suffix:
>
>         freq_mhz = ...
>
> (if I am not mistaken).

Yes, `freq_mhz` is more appropriate.
>
> > +     cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);
>
>         cr2 = FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);

ok..
>
> > +     regmap_write(priv->regmap, LOONGSON2_I2C_CR2, cr2);
> > +
> > +     if (priv->speed == LOONGSON2_I2C_SPEED_STANDARD) {
> > +             val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);
>
>                 /* Select Standard mode */
>                 ccr = 0;
>
> > +     } else {
> > +             val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
> > +
> > +             /* Select Fast mode */
> > +             ccr |= LOONGSON2_I2C_CCR_FS;
>
>                 ccr = LOONGSON2_I2C_CCR_FS;
>
> > +     }
>
> > +     ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);
>
> FIELD_MODIFY()?

FIELD_MODIFY(LOONGSON2_I2C_CCR_CCR, &ccr, val);  instead.
>
> > +     regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
> > +
> > +     /* reference clock determination the configure val(0x3f) */
> > +     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
> > +                        LOONGSON2_I2C_CR2_FREQ);
> > +     regmap_update_bits(priv->regmap, LOONGSON2_I2C_TRISE, LOONGSON2_I2C_TRISE_SCL,
> > +                        LOONGSON2_I2C_TRISE_SCL);
> > +
> > +     /* Enable I2C */
> > +     regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_PE,
> > +                        LOONGSON2_I2C_CR1_PE);
> > +
> > +     return 0;
> > +}
>
> ...
>
> > +static int loongson2_i2c_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct loongson2_i2c_priv *priv;
> > +     struct i2c_adapter *adap;
> > +     void __iomem *base;
> > +     u32 clk_rate;
> > +     int irq, ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(base))
>
> > +             return dev_err_probe(dev, PTR_ERR(base),
> > +                                  "Failed to ioremap resource.\n");
>
> You utilise 100, put this on a single line.

ok. I will check the issue again.
>
> But, this is a dup, devm_platform_*() call above takes care already about error
> message.

emm. I will use `return PTR_ERR(base);` instead.
>
> > +
> > +     priv->regmap = devm_regmap_init_mmio(dev, base,
> > +                                          &loongson2_i2c_regmap_config);
>
> Single line.
>
> > +     if (IS_ERR(priv->regmap))
> > +             return dev_err_probe(dev, PTR_ERR(priv->regmap),
> > +                                  "Failed to init regmap.\n");
>
> Single line.
>
> > +     priv->clk = devm_clk_get_enabled(dev, NULL);
> > +     if (IS_ERR(priv->clk))
> > +             return dev_err_probe(dev, PTR_ERR(priv->clk),
> > +                                  "Failed  to enable clock.\n");
>
> Single line.
>
> > +     irq = platform_get_irq(pdev, 0);
> > +     if (irq < 0)
> > +             return -EINVAL;
>
> Why?! What's wrong with the error code in 'irq'?

I will use `return irq` instead.
>
> > +     priv->dev = dev;
> > +
> > +     adap = &priv->adapter;
> > +     adap->retries = 5;
> > +     adap->nr = pdev->id;
> > +     adap->dev.parent = dev;
> > +     adap->owner = THIS_MODULE;
> > +     adap->algo = &loongson2_i2c_algo;
> > +     adap->timeout = 2 * HZ;
> > +     device_set_node(&adap->dev, dev_fwnode(dev));
> > +     i2c_set_adapdata(adap, priv);
>
> > +     strscpy(adap->name, pdev->name, sizeof(adap->name));
>
> 2-arguments version is even better.

Sorry, I'm not quite sure what you mean by `2-arguments version.`

>
> > +     init_completion(&priv->complete);
> > +     platform_set_drvdata(pdev, priv);
> > +
> > +     priv->speed = LOONGSON2_I2C_SPEED_STANDARD;
>
> > +     ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);
>
> device_property_read_u32()

ok...
>
> > +     if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
> > +             priv->speed = LOONGSON2_I2C_SPEED_FAST;
> > +
> > +     ret = loongson2_i2c_adjust_bus_speed(priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_request_irq(dev, irq, loongson2_i2c_isr_event, IRQF_SHARED, pdev->name, priv);
> > +     if (ret)
>
> > +             return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
>
> Dup message.

ok...
>
>                 return ret;
>
> > +     return devm_i2c_add_adapter(dev, adap);
> > +}
>
> ...
>
> > +static struct platform_driver loongson2_i2c_driver = {
> > +     .driver = {
> > +             .name = "loongson2-i2c-v2",
> > +             .of_match_table = loongson2_i2c_id_table,
> > +     },
> > +     .probe = loongson2_i2c_probe,
> > +};
>
> > +
>
> Remove this blank line.

ok...
>
> > +module_platform_driver(loongson2_i2c_driver);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


--
Thanks.
Binbin

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko]

On Thu, Jan 29, 2026 at 04:07:06PM +0800, Binbin Zhou wrote:
> On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:

...

> > > +/*
> >
> > It's not marked as kernel-doc, but looks very much like that. Why?
> > Same Q for *all* cases like this.
> 
> I didn't intend to mark it in kernel-doc; I just wanted to comment on
> the variable.
> Is removing the `@` symbol sufficient?

But why? If you want it being properly documented, make sure it follows
the regular format.

> > > + * struct loongson2_i2c_msg - client specific data
> > > + * @addr: 8-bit slave addr, including r/w bit
> > > + * @count: number of bytes to be transferred
> > > + * @buf: data buffer
> > > + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> > > + * @result: result of the transfer
> > > + */
> > > +struct loongson2_i2c_msg {
> > > +     u8 addr;
> > > +     u32 count;
> > > +     u8 *buf;
> > > +     bool stop;
> > > +     int result;
> >
> > Run `pahole` and amend *all* data types accordingly.
> 
>  Moving 'stop' from after 'buf' to after 'addr'.
> >
> > > +};

...

> After run `pahole`, the structs are reorganized as follow:
> 
> pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
> i2c-ls2x-v2.o
> struct loongson2_i2c_priv {
>         struct i2c_adapter         adapter
> __attribute__((__aligned__(8))); /*     0  1064 */
>         /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
> */
>         struct clk *               clk;                  /*  1064
> 8 */
>         struct completion          complete;             /*  1072
> 32 */
>         /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
> */
>         struct regmap *            regmap;               /*  1104

It's better to keep pointers like clk and regmap next to each other.
They are semantically coupled as "resources".

> 8 */
>         int                        speed;                /*  1112
> 4 */
>         int                        parent_rate;          /*  1116
> 4 */
>         struct loongson2_i2c_msg   msg;                  /*  1120
> 24 */
> 
>         /* XXX last struct has 4 bytes of padding */
> 
>         /* size: 1144, cachelines: 18, members: 7 */
>         /* paddings: 1, sum paddings: 4 */
>         /* forced alignments: 1 */
>         /* last cacheline: 56 bytes */
> } __attribute__((__aligned__(8)));

...

> pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
> i2c-ls2x-v2.o
> struct loongson2_i2c_msg {
>         u8                         addr;                 /*     0
> 1 */
>         bool                       stop;                 /*     1
> 1 */
> 
>         /* XXX 2 bytes hole, try to pack */

>         u32                        count;                /*     4
> 4 */
>         u8 *                       buf;                  /*     8
> 8 */

Also think about it (don't blindly follow the `pahole` automatic mode).
This is much better if you move the pointer to be the first, followed
by a count, result, and others.

>         int                        result;               /*    16
> 4 */
> 
>         /* size: 24, cachelines: 1, members: 5 */
>         /* sum members: 18, holes: 1, sum holes: 2 */
>         /* padding: 4 */
>         /* last cacheline: 24 bytes */
> };

TL;DR: don't use `pahole` blindly. Use the common sense.

...

> > > +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> > > +{
> > > +     u32 status;
> > > +     int ret;
> > > +
> > > +     ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> > > +                                    !(status & LOONGSON2_I2C_SR2_BUSY),
> > > +                                    LOONGSON2_I2C_FREE_SLEEP_US,
> > > +                                    LOONGSON2_I2C_FREE_TIMEOUT_US);
> > > +     if (ret) {
> > > +             dev_dbg(priv->dev, "I2C bus free failed.\n");
> >
> > > +             ret = -EBUSY;
> >
> > Why?! What's wrong with the error code returned in ret?
> 
> I want to indicate the bus busy state if it times out.

This is not an answer. So, why do you remap error code. What's wrong with
the callee's one?

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

...

> > Ah, it seems it's a helper. Please, return the error code from it as int and
> > not irqreturn_t, this will make things clearer.
> 
> How about just define it as void:
> 
> static void loongson2_i2c_isr_error(u32 status, void *data)
> {
> .........
>         if (status & LOONGSON2_I2C_SR1_ARLO) {
>          ........
>                 msg->result = -EAGAIN;
>                 goto out;
>         }
>         if (status & LOONGSON2_I2C_SR1_AF) {
>            .......
>                 msg->result = -EIO;
>                 goto out;
>         }
>         if (status & LOONGSON2_I2C_SR1_BERR) {
>          .........
>                 msg->result = -EIO;
>                 goto out;
>         }
> 
> out:
>         loongson2_i2c_disable_irq(priv);
>         complete(&priv->complete);
> }
> 
> and in loongson2_i2c_isr_event(), I reference it as:
> 
>         if (status & LOONGSON2_I2C_SR1_ITERREN_MASK) {
>                 loongson2_i2c_isr_error(status, data);
>                 return IRQ_NONE;
>         }

> Its return value is meaningless for loongson2_i2c_isr_event().

Works for me.

...

> > > +     u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
> >
> > Split assignment...

> > ...to be here, which improves readability (no need to go somewhere up in
> > the code to see what this is about.
> 
> ok, I will put  `possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;` here.
> 
> > > +     /* Update possible_status if buffer interrupt is enabled */
> > > +     if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> > > +             possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;

I expect something like this:

		/* Update possible_status if buffer interrupt is enabled */
		possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
		if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
			possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;

...

> > > +     strscpy(adap->name, pdev->name, sizeof(adap->name));
> >
> > 2-arguments version is even better.
> 
> Sorry, I'm not quite sure what you mean by `2-arguments version.`

Use 2 arguments instead of 3. I.o.w. just drop the third argument.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Binbin Zhou]

Hi Andy:

Thanks for your patient reply.

On Thu, Jan 29, 2026 at 10:38 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Jan 29, 2026 at 04:07:06PM +0800, Binbin Zhou wrote:
> > On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
> > > On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:
>
> ...
>
> > > > +/*
> > >
> > > It's not marked as kernel-doc, but looks very much like that. Why?
> > > Same Q for *all* cases like this.
> >
> > I didn't intend to mark it in kernel-doc; I just wanted to comment on
> > the variable.
> > Is removing the `@` symbol sufficient?
>
> But why? If you want it being properly documented, make sure it follows
> the regular format.

I sincerely apologize for not having read `kernel-doc.rst` earlier.
`kernel-doc` comments use `/**` as its opening marker. I will take
this time to learn the documentation.

>
> > > > + * struct loongson2_i2c_msg - client specific data
> > > > + * @addr: 8-bit slave addr, including r/w bit
> > > > + * @count: number of bytes to be transferred
> > > > + * @buf: data buffer
> > > > + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> > > > + * @result: result of the transfer
> > > > + */
> > > > +struct loongson2_i2c_msg {
> > > > +     u8 addr;
> > > > +     u32 count;
> > > > +     u8 *buf;
> > > > +     bool stop;
> > > > +     int result;
> > >
> > > Run `pahole` and amend *all* data types accordingly.
> >
> >  Moving 'stop' from after 'buf' to after 'addr'.
> > >
> > > > +};
>
> ...
>
> > After run `pahole`, the structs are reorganized as follow:
> >
> > pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
> > i2c-ls2x-v2.o
> > struct loongson2_i2c_priv {
> >         struct i2c_adapter         adapter
> > __attribute__((__aligned__(8))); /*     0  1064 */
> >         /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
> > */
> >         struct clk *               clk;                  /*  1064
> > 8 */
> >         struct completion          complete;             /*  1072
> > 32 */
> >         /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
> > */
> >         struct regmap *            regmap;               /*  1104
>
> It's better to keep pointers like clk and regmap next to each other.
> They are semantically coupled as "resources".
>
> > 8 */
> >         int                        speed;                /*  1112
> > 4 */
> >         int                        parent_rate;          /*  1116
> > 4 */
> >         struct loongson2_i2c_msg   msg;                  /*  1120
> > 24 */
> >
> >         /* XXX last struct has 4 bytes of padding */
> >
> >         /* size: 1144, cachelines: 18, members: 7 */
> >         /* paddings: 1, sum paddings: 4 */
> >         /* forced alignments: 1 */
> >         /* last cacheline: 56 bytes */
> > } __attribute__((__aligned__(8)));
>
> ...
>
> > pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
> > i2c-ls2x-v2.o
> > struct loongson2_i2c_msg {
> >         u8                         addr;                 /*     0
> > 1 */
> >         bool                       stop;                 /*     1
> > 1 */
> >
> >         /* XXX 2 bytes hole, try to pack */
>
> >         u32                        count;                /*     4
> > 4 */
> >         u8 *                       buf;                  /*     8
> > 8 */
>
> Also think about it (don't blindly follow the `pahole` automatic mode).
> This is much better if you move the pointer to be the first, followed
> by a count, result, and others.
>
> >         int                        result;               /*    16
> > 4 */
> >
> >         /* size: 24, cachelines: 1, members: 5 */
> >         /* sum members: 18, holes: 1, sum holes: 2 */
> >         /* padding: 4 */
> >         /* last cacheline: 24 bytes */
> > };
>
> TL;DR: don't use `pahole` blindly. Use the common sense.
>

Sorry, I slacked off.
How about the organized data structure as follows:

struct loongson2_i2c_msg {
        u8      *buf;
        u32     count;
        int     result;
        u8      addr;
        bool    stop;
};

struct loongson2_i2c_priv {
        struct i2c_adapter              adapter;
        struct completion               complete;
        struct clk                      *clk;
        struct regmap                   *regmap;
        int                             speed;
        int                             parent_rate;
        struct loongson2_i2c_msg        msg;
};

> ...
>
> > > > +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> > > > +{
> > > > +     u32 status;
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> > > > +                                    !(status & LOONGSON2_I2C_SR2_BUSY),
> > > > +                                    LOONGSON2_I2C_FREE_SLEEP_US,
> > > > +                                    LOONGSON2_I2C_FREE_TIMEOUT_US);
> > > > +     if (ret) {
> > > > +             dev_dbg(priv->dev, "I2C bus free failed.\n");
> > >
> > > > +             ret = -EBUSY;
> > >
> > > Why?! What's wrong with the error code returned in ret?
> >
> > I want to indicate the bus busy state if it times out.
>
> This is not an answer. So, why do you remap error code. What's wrong with
> the callee's one?

I examined the existing callee and found no special checks for
`-EBUSY`. Using the return value of `regmap_read_poll_timeout()`
directly is the more appropriate choice.

Additionally, `loongson2_i2c_wait_free_bus()` is only called by
`loongson2_i2c_xfer()`. Its separate definition does not appear
essential. Calling `regmap_read_poll_timeout()` directly within
`loongson2_i2c_xfer()` seems more concise.

As follows:

static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct
i2c_msg msgs[], int num)
{
............

        /* Wait I2C bus free */
        ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
                                       !(status & LOONGSON2_I2C_SR2_BUSY),
                                       LOONGSON2_I2C_FREE_SLEEP_US,
                                       LOONGSON2_I2C_FREE_TIMEOUT_US);
        if (ret) {
                dev_dbg(dev, "I2C bus free failed.\n");
                return ret;
        }

        /* Start generation */
        regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1,
LOONGSON2_I2C_CR1_START,
                           LOONGSON2_I2C_CR1_START);

.......

        return num;
}

>
> > > > +     }
> > > > +
> > > > +     return ret;
> > > > +}
>
> ...
>
> > > Ah, it seems it's a helper. Please, return the error code from it as int and
> > > not irqreturn_t, this will make things clearer.
> >
> > How about just define it as void:
> >
> > static void loongson2_i2c_isr_error(u32 status, void *data)
> > {
> > .........
> >         if (status & LOONGSON2_I2C_SR1_ARLO) {
> >          ........
> >                 msg->result = -EAGAIN;
> >                 goto out;
> >         }
> >         if (status & LOONGSON2_I2C_SR1_AF) {
> >            .......
> >                 msg->result = -EIO;
> >                 goto out;
> >         }
> >         if (status & LOONGSON2_I2C_SR1_BERR) {
> >          .........
> >                 msg->result = -EIO;
> >                 goto out;
> >         }
> >
> > out:
> >         loongson2_i2c_disable_irq(priv);
> >         complete(&priv->complete);
> > }
> >
> > and in loongson2_i2c_isr_event(), I reference it as:
> >
> >         if (status & LOONGSON2_I2C_SR1_ITERREN_MASK) {
> >                 loongson2_i2c_isr_error(status, data);
> >                 return IRQ_NONE;
> >         }
>
> > Its return value is meaningless for loongson2_i2c_isr_event().
>
> Works for me.
>
> ...
>
> > > > +     u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
> > >
> > > Split assignment...
>
> > > ...to be here, which improves readability (no need to go somewhere up in
> > > the code to see what this is about.
> >
> > ok, I will put  `possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;` here.
> >
> > > > +     /* Update possible_status if buffer interrupt is enabled */
> > > > +     if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> > > > +             possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
>
> I expect something like this:
>
>                 /* Update possible_status if buffer interrupt is enabled */
>                 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
>                 if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
>                         possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;

ok...
>
> ...
>
> > > > +     strscpy(adap->name, pdev->name, sizeof(adap->name));
> > >
> > > 2-arguments version is even better.
> >
> > Sorry, I'm not quite sure what you mean by `2-arguments version.`
>
> Use 2 arguments instead of 3. I.o.w. just drop the third argument.

Ah, I see, the third argument is optional.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

--
Thanks.
Binbin

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko]

On Mon, Feb 02, 2026 at 02:39:28PM +0800, Binbin Zhou wrote:
> On Thu, Jan 29, 2026 at 10:38 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Thu, Jan 29, 2026 at 04:07:06PM +0800, Binbin Zhou wrote:
> > > On Tue, Jan 27, 2026 at 4:18 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:
> > > > On Tue, Jan 27, 2026 at 10:47:57AM +0800, Binbin Zhou wrote:

...

> > > After run `pahole`, the structs are reorganized as follow:
> > >
> > > pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_priv
> > > i2c-ls2x-v2.o
> > > struct loongson2_i2c_priv {
> > >         struct i2c_adapter         adapter
> > > __attribute__((__aligned__(8))); /*     0  1064 */
> > >         /* --- cacheline 16 boundary (1024 bytes) was 40 bytes ago ---
> > > */
> > >         struct clk *               clk;                  /*  1064
> > > 8 */
> > >         struct completion          complete;             /*  1072
> > > 32 */
> > >         /* --- cacheline 17 boundary (1088 bytes) was 16 bytes ago ---
> > > */
> > >         struct regmap *            regmap;               /*  1104
> >
> > It's better to keep pointers like clk and regmap next to each other.
> > They are semantically coupled as "resources".
> >
> > > 8 */
> > >         int                        speed;                /*  1112
> > > 4 */
> > >         int                        parent_rate;          /*  1116
> > > 4 */
> > >         struct loongson2_i2c_msg   msg;                  /*  1120
> > > 24 */
> > >
> > >         /* XXX last struct has 4 bytes of padding */
> > >
> > >         /* size: 1144, cachelines: 18, members: 7 */
> > >         /* paddings: 1, sum paddings: 4 */
> > >         /* forced alignments: 1 */
> > >         /* last cacheline: 56 bytes */
> > > } __attribute__((__aligned__(8)));
> >
> > ...
> >
> > > pahole --show_reorg_steps --reorganize --sort -C loongson2_i2c_msg
> > > i2c-ls2x-v2.o
> > > struct loongson2_i2c_msg {
> > >         u8                         addr;                 /*     0
> > > 1 */
> > >         bool                       stop;                 /*     1
> > > 1 */
> > >
> > >         /* XXX 2 bytes hole, try to pack */
> >
> > >         u32                        count;                /*     4
> > > 4 */
> > >         u8 *                       buf;                  /*     8
> > > 8 */
> >
> > Also think about it (don't blindly follow the `pahole` automatic mode).
> > This is much better if you move the pointer to be the first, followed
> > by a count, result, and others.
> >
> > >         int                        result;               /*    16
> > > 4 */
> > >
> > >         /* size: 24, cachelines: 1, members: 5 */
> > >         /* sum members: 18, holes: 1, sum holes: 2 */
> > >         /* padding: 4 */
> > >         /* last cacheline: 24 bytes */
> > > };
> >
> > TL;DR: don't use `pahole` blindly. Use the common sense.
> 
> Sorry, I slacked off.
> How about the organized data structure as follows:
> 
> struct loongson2_i2c_msg {
>         u8      *buf;
>         u32     count;
>         int     result;
>         u8      addr;
>         bool    stop;
> };
> 
> struct loongson2_i2c_priv {
>         struct i2c_adapter              adapter;
>         struct completion               complete;
>         struct clk                      *clk;
>         struct regmap                   *regmap;
>         int                             speed;
>         int                             parent_rate;
>         struct loongson2_i2c_msg        msg;
> };

At the first glance looks okay to me.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Huacai Chen]

Hi, Binbin,

On Tue, Jan 27, 2026 at 10:48 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:
>
> This I2C module is integrated into the Loongson-2K0300 SoCs.
>
> It provides multi-master functionality and controls all I2C bus-specific
> timing, protocols, arbitration, and timing. It supports both standard
> and fast modes.
>
> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
> ---
>  MAINTAINERS                      |   1 +
>  drivers/i2c/busses/Kconfig       |  10 +
>  drivers/i2c/busses/Makefile      |   1 +
>  drivers/i2c/busses/i2c-ls2x-v2.c | 545 +++++++++++++++++++++++++++++++
>  4 files changed, 557 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-ls2x-v2.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5b11839cba9d..01fd37fdd29b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14783,6 +14783,7 @@ M:      Binbin Zhou <zhoubinbin@loongson.cn>
>  L:     linux-i2c@vger.kernel.org
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> +F:     drivers/i2c/busses/i2c-ls2x-v2.c
I  still think it is better to move this line below. Though it may
cause a checkpatch warning, but for checkpatch only errors must be
fixed, whether warnings need to be eliminated depends on the fact.

Huacai

>  F:     drivers/i2c/busses/i2c-ls2x.c
>
>  LOONGSON PWM DRIVER
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 09ba55bae1fa..d409401c9c94 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -857,6 +857,16 @@ config I2C_LS2X
>           This driver can also be built as a module. If so, the module
>           will be called i2c-ls2x.
>
> +config I2C_LS2X_V2
> +       tristate "Loongson-2 Fast Speed I2C adapter"
> +       depends on LOONGARCH || COMPILE_TEST
> +       help
> +         If you say yes to this option, support will be included for the
> +         I2C interface on the Loongson-2K0300 SoCs.
> +
> +         This driver can also be built as a module. If so, the module
> +         will be called i2c-ls2x-v2.
> +
>  config I2C_MLXBF
>          tristate "Mellanox BlueField I2C controller"
>          depends on (MELLANOX_PLATFORM && ARM64) || COMPILE_TEST
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index fb985769f5ff..8cdfc30b79e9 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_I2C_KEBA)                += i2c-keba.o
>  obj-$(CONFIG_I2C_KEMPLD)       += i2c-kempld.o
>  obj-$(CONFIG_I2C_LPC2K)                += i2c-lpc2k.o
>  obj-$(CONFIG_I2C_LS2X)         += i2c-ls2x.o
> +obj-$(CONFIG_I2C_LS2X_V2)      += i2c-ls2x-v2.o
>  obj-$(CONFIG_I2C_MESON)                += i2c-meson.o
>  obj-$(CONFIG_I2C_MICROCHIP_CORE)       += i2c-microchip-corei2c.o
>  obj-$(CONFIG_I2C_MPC)          += i2c-mpc.o
> diff --git a/drivers/i2c/busses/i2c-ls2x-v2.c b/drivers/i2c/busses/i2c-ls2x-v2.c
> new file mode 100644
> index 000000000000..93d24c9482f0
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ls2x-v2.c
> @@ -0,0 +1,545 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Loongson-2K fast I2C controller driver
> + *
> + * Copyright (C) 2025-2026 Loongson Technology Corporation Limited
> + *
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/iopoll.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +/* Loongson-2 fast I2C offset registers */
> +#define LOONGSON2_I2C_CR1      0x00    /* I2C control 1 register */
> +#define LOONGSON2_I2C_CR2      0x04    /* I2C control 2 register */
> +#define LOONGSON2_I2C_OAR      0x08    /* I2C slave address register */
> +#define LOONGSON2_I2C_DR       0x10    /* I2C data register */
> +#define LOONGSON2_I2C_SR1      0x14    /* I2C status 1 register */
> +#define LOONGSON2_I2C_SR2      0x18    /* I2C status 2 register */
> +#define LOONGSON2_I2C_CCR      0x1C    /* I2C clock control register */
> +#define LOONGSON2_I2C_TRISE    0x20    /* I2C trise register */
> +#define LOONGSON2_I2C_FLTR     0x24
> +
> +/* Bitfields of I2C control 1 register */
> +#define LOONGSON2_I2C_CR1_PE           BIT(0)
> +#define LOONGSON2_I2C_CR1_START                BIT(8)
> +#define LOONGSON2_I2C_CR1_STOP         BIT(9)
> +#define LOONGSON2_I2C_CR1_ACK          BIT(10)
> +#define LOONGSON2_I2C_CR1_POS          BIT(11)
> +
> +#define LOONGSON2_I2C_CR1_OP_MASK      (LOONGSON2_I2C_CR1_START | LOONGSON2_I2C_CR1_STOP)
> +
> +/* Bitfields of I2C control 2 register */
> +#define LOONGSON2_I2C_CR2_FREQ         GENMASK(5, 0)
> +#define LOONGSON2_I2C_CR2_ITERREN      BIT(8)
> +#define LOONGSON2_I2C_CR2_ITEVTEN      BIT(9)
> +#define LOONGSON2_I2C_CR2_ITBUFEN      BIT(10)
> +
> +#define LOONGSON2_I2C_CR2_IRQ_MASK     (LOONGSON2_I2C_CR2_ITBUFEN | \
> +                                        LOONGSON2_I2C_CR2_ITEVTEN | \
> +                                        LOONGSON2_I2C_CR2_ITERREN)
> +
> +/* Bitfields of I2C status 1 register */
> +#define LOONGSON2_I2C_SR1_SB           BIT(0)
> +#define LOONGSON2_I2C_SR1_ADDR         BIT(1)
> +#define LOONGSON2_I2C_SR1_BTF          BIT(2)
> +#define LOONGSON2_I2C_SR1_RXNE         BIT(6)
> +#define LOONGSON2_I2C_SR1_TXE          BIT(7)
> +#define LOONGSON2_I2C_SR1_BERR         BIT(8)
> +#define LOONGSON2_I2C_SR1_ARLO         BIT(9)
> +#define LOONGSON2_I2C_SR1_AF           BIT(10)
> +
> +#define LOONGSON2_I2C_SR1_ITEVTEN_MASK (LOONGSON2_I2C_SR1_BTF | \
> +                                        LOONGSON2_I2C_SR1_ADDR | \
> +                                        LOONGSON2_I2C_SR1_SB)
> +#define LOONGSON2_I2C_SR1_ITBUFEN_MASK (LOONGSON2_I2C_SR1_TXE | LOONGSON2_I2C_SR1_RXNE)
> +#define LOONGSON2_I2C_SR1_ITERREN_MASK (LOONGSON2_I2C_SR1_AF | \
> +                                        LOONGSON2_I2C_SR1_ARLO | \
> +                                        LOONGSON2_I2C_SR1_BERR)
> +
> +/* Bitfields of I2C status 2 register */
> +#define LOONGSON2_I2C_SR2_BUSY         BIT(1)
> +
> +/* Bitfields of I2C clock control register */
> +#define LOONGSON2_I2C_CCR_CCR          GENMASK(11, 0)
> +#define LOONGSON2_I2C_CCR_DUTY         BIT(14)
> +#define LOONGSON2_I2C_CCR_FS           BIT(15)
> +
> +/* Bitfields of I2C trise register */
> +#define LOONGSON2_I2C_TRISE_SCL                GENMASK(5, 0)
> +
> +#define LOONGSON2_I2C_FREE_SLEEP_US    1000
> +#define LOONGSON2_I2C_FREE_TIMEOUT_US  5000
> +
> +#define LOONGSON2_I2C_MIN_STD_FREQ     2U
> +#define LOONGSON2_I2C_MIN_FAST_FREQ    6U
> +#define LOONGSON2_I2C_MAX_FREQ         46U
> +#define HZ_TO_MHZ                      1000000
> +
> +enum loongson2_i2c_speed {
> +       LOONGSON2_I2C_SPEED_STANDARD, /* 100 kHz */
> +       LOONGSON2_I2C_SPEED_FAST, /* 400 kHz */
> +       LOONGSON2_I2C_SPEED_FAST_PLUS, /* 1 MHz */
> +       LOONGSON2_I2C_SPEED_END,
> +};
> +
> +/*
> + * struct loongson2_i2c_msg - client specific data
> + * @addr: 8-bit slave addr, including r/w bit
> + * @count: number of bytes to be transferred
> + * @buf: data buffer
> + * @stop: last I2C msg to be sent, i.e. STOP to be generated
> + * @result: result of the transfer
> + */
> +struct loongson2_i2c_msg {
> +       u8 addr;
> +       u32 count;
> +       u8 *buf;
> +       bool stop;
> +       int result;
> +};
> +
> +/*
> + * struct loongson2_i2c_priv - private data of the controller
> + * @adapter: I2C adapter for this controller
> + * @dev: device for this controller
> + * @clk: hw i2c clock
> + * @complete: completion of I2C message
> + * @regmap: regmap of the I2C device
> + * @speed: I2C clock frequency of the controller. Standard or Fast are supported
> + * @parent_rate: I2C clock parent rate in MHz
> + * @msg: I2C transfer information
> + */
> +struct loongson2_i2c_priv {
> +       struct i2c_adapter adapter;
> +       struct device *dev;
> +       struct clk *clk;
> +       struct completion complete;
> +       struct regmap *regmap;
> +       int speed;
> +       int parent_rate;
> +       struct loongson2_i2c_msg msg;
> +};
> +
> +static void loongson2_i2c_disable_irq(struct loongson2_i2c_priv *priv)
> +{
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_IRQ_MASK, 0);
> +}
> +
> +static int loongson2_i2c_wait_free_bus(struct loongson2_i2c_priv *priv)
> +{
> +       u32 status;
> +       int ret;
> +
> +       ret = regmap_read_poll_timeout(priv->regmap, LOONGSON2_I2C_SR2, status,
> +                                      !(status & LOONGSON2_I2C_SR2_BUSY),
> +                                      LOONGSON2_I2C_FREE_SLEEP_US,
> +                                      LOONGSON2_I2C_FREE_TIMEOUT_US);
> +       if (ret) {
> +               dev_dbg(priv->dev, "I2C bus free failed.\n");
> +               ret = -EBUSY;
> +       }
> +
> +       return ret;
> +}
> +
> +static void loongson2_i2c_read_msg(struct loongson2_i2c_priv *priv)
> +{
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +       u32 rbuf;
> +
> +       regmap_read(priv->regmap, LOONGSON2_I2C_DR, &rbuf);
> +       *msg->buf++ = rbuf;
> +       msg->count--;
> +}
> +
> +static void loongson2_i2c_write_msg(struct loongson2_i2c_priv *priv, u8 byte)
> +{
> +       regmap_write(priv->regmap, LOONGSON2_I2C_DR, byte);
> +}
> +
> +static void loongson2_i2c_terminate_xfer(struct loongson2_i2c_priv *priv)
> +{
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +       loongson2_i2c_disable_irq(priv);
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
> +                          msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
> +       complete(&priv->complete);
> +}
> +
> +static void loongson2_i2c_handle_read(struct loongson2_i2c_priv *priv, int flag)
> +{
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +       bool changed;
> +       int i;
> +
> +       switch (msg->count) {
> +       case 1:
> +               /* only transmit 1 bytes condition */
> +               loongson2_i2c_disable_irq(priv);
> +               loongson2_i2c_read_msg(priv);
> +               complete(&priv->complete);
> +               break;
> +       case 2:
> +               if (flag != 1) {
> +                       /* ensure only transmit 2 bytes condition */
> +                       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +                                          LOONGSON2_I2C_CR2_ITBUFEN, 0);
> +                       break;
> +               }
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
> +                                  msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
> +
> +               loongson2_i2c_disable_irq(priv);
> +
> +               for (i = 2; i > 0; i--)
> +                       loongson2_i2c_read_msg(priv);
> +
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
> +               complete(&priv->complete);
> +               break;
> +       case 3:
> +               regmap_update_bits_check(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> +                                        0, &changed);
> +               if (changed)
> +                       break;
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
> +               fallthrough;
> +       default:
> +               loongson2_i2c_read_msg(priv);
> +       }
> +}
> +
> +static void loongson2_i2c_handle_write(struct loongson2_i2c_priv *priv)
> +{
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +       if (msg->count) {
> +               loongson2_i2c_write_msg(priv, *msg->buf++);
> +               msg->count--;
> +               if (!msg->count)
> +                       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +                                          LOONGSON2_I2C_CR2_ITBUFEN, 0);
> +       } else {
> +               loongson2_i2c_terminate_xfer(priv);
> +       }
> +}
> +
> +static void loongson2_i2c_handle_rx_addr(struct loongson2_i2c_priv *priv)
> +{
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +       switch (msg->count) {
> +       case 0:
> +               loongson2_i2c_terminate_xfer(priv);
> +               break;
> +       case 1:
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1,
> +                                  LOONGSON2_I2C_CR1_ACK | LOONGSON2_I2C_CR1_POS, 0);
> +               /* start or stop */
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_OP_MASK,
> +                                  msg->stop ? LOONGSON2_I2C_CR1_STOP : LOONGSON2_I2C_CR1_START);
> +               break;
> +       case 2:
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK, 0);
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS,
> +                                  LOONGSON2_I2C_CR1_POS);
> +               break;
> +
> +       default:
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_ACK,
> +                                  LOONGSON2_I2C_CR1_ACK);
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_POS, 0);
> +       }
> +}
> +
> +static irqreturn_t loongson2_i2c_isr_error(u32 status, void *data)
> +{
> +       struct loongson2_i2c_priv *priv = data;
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +
> +       /* Arbitration lost */
> +       if (status & LOONGSON2_I2C_SR1_ARLO) {
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_ARLO, 0);
> +               msg->result = -EAGAIN;
> +       }
> +
> +       /*
> +        * Acknowledge failure:
> +        * In master transmitter mode a Stop must be generated by software
> +        */
> +       if (status & LOONGSON2_I2C_SR1_AF) {
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_STOP,
> +                                  LOONGSON2_I2C_CR1_STOP);
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_AF, 0);
> +               msg->result = -EIO;
> +       }
> +
> +       /* Bus error */
> +       if (status & LOONGSON2_I2C_SR1_BERR) {
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_SR1, LOONGSON2_I2C_SR1_BERR, 0);
> +               msg->result = -EIO;
> +       }
> +
> +       loongson2_i2c_disable_irq(priv);
> +       complete(&priv->complete);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t loongson2_i2c_isr_event(int irq, void *data)
> +{
> +       u32 possible_status = LOONGSON2_I2C_SR1_ITEVTEN_MASK;
> +       struct loongson2_i2c_priv *priv = data;
> +       struct loongson2_i2c_msg *msg = &priv->msg;
> +       u32 status, ien, event, cr2;
> +
> +       regmap_read(priv->regmap, LOONGSON2_I2C_SR1, &status);
> +       if (status & LOONGSON2_I2C_SR1_ITERREN_MASK)
> +               return loongson2_i2c_isr_error(status, data);
> +
> +       regmap_read(priv->regmap, LOONGSON2_I2C_CR2, &cr2);
> +       ien = cr2 & LOONGSON2_I2C_CR2_IRQ_MASK;
> +
> +       /* Update possible_status if buffer interrupt is enabled */
> +       if (ien & LOONGSON2_I2C_CR2_ITBUFEN)
> +               possible_status |= LOONGSON2_I2C_SR1_ITBUFEN_MASK;
> +
> +       event = status & possible_status;
> +       if (!event) {
> +               dev_dbg(priv->dev, "spurious evt irq (status=0x%08x, ien=0x%08x)\n", status, ien);
> +               return IRQ_NONE;
> +       }
> +
> +       /* Start condition generated */
> +       if (event & LOONGSON2_I2C_SR1_SB)
> +               loongson2_i2c_write_msg(priv, msg->addr);
> +
> +       /* I2C Address sent */
> +       if (event & LOONGSON2_I2C_SR1_ADDR) {
> +               if (msg->addr & I2C_M_RD)
> +                       loongson2_i2c_handle_rx_addr(priv);
> +               /* Clear ADDR flag */
> +               regmap_read(priv->regmap, LOONGSON2_I2C_SR2, &status);
> +               /* Enable buffer interrupts for RX/TX not empty events */
> +               regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_ITBUFEN,
> +                                  LOONGSON2_I2C_CR2_ITBUFEN);
> +       }
> +
> +       if (msg->addr & I2C_M_RD) {
> +               /* RX not empty */
> +               if (event & LOONGSON2_I2C_SR1_RXNE)
> +                       loongson2_i2c_handle_read(priv, 0);
> +
> +               if (event & LOONGSON2_I2C_SR1_BTF)
> +                       loongson2_i2c_handle_read(priv, 1);
> +       } else {
> +               /* TX empty */
> +               if (event & LOONGSON2_I2C_SR1_TXE)
> +                       loongson2_i2c_handle_write(priv);
> +
> +               if (event & LOONGSON2_I2C_SR1_BTF)
> +                       loongson2_i2c_handle_write(priv);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static int loongson2_i2c_xfer_msg(struct loongson2_i2c_priv *priv, struct i2c_msg *msg,
> +                                 bool is_stop)
> +{
> +       struct loongson2_i2c_msg *l_msg = &priv->msg;
> +       unsigned long timeout;
> +       int ret;
> +
> +       l_msg->addr   = i2c_8bit_addr_from_msg(msg);
> +       l_msg->buf    = msg->buf;
> +       l_msg->count  = msg->len;
> +       l_msg->stop   = is_stop;
> +       l_msg->result = 0;
> +
> +       reinit_completion(&priv->complete);
> +
> +       /* Enable events and errors interrupts */
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2,
> +                          LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN,
> +                          LOONGSON2_I2C_CR2_ITEVTEN | LOONGSON2_I2C_CR2_ITERREN);
> +
> +       timeout = wait_for_completion_timeout(&priv->complete, priv->adapter.timeout);
> +       ret = l_msg->result;
> +
> +       if (!timeout)
> +               ret = -ETIMEDOUT;
> +
> +       return ret;
> +}
> +
> +static int loongson2_i2c_xfer(struct i2c_adapter *i2c_adap, struct i2c_msg msgs[], int num)
> +{
> +       struct loongson2_i2c_priv *priv = i2c_get_adapdata(i2c_adap);
> +       int ret = 0, i;
> +
> +       ret = loongson2_i2c_wait_free_bus(priv);
> +       if (ret)
> +               return ret;
> +
> +       /* START generation */
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_START,
> +                          LOONGSON2_I2C_CR1_START);
> +
> +       for (i = 0; i < num && !ret; i++)
> +               ret = loongson2_i2c_xfer_msg(priv, &msgs[i], i == num - 1);
> +
> +       return (ret < 0) ? ret : num;
> +}
> +
> +static u32 loongson2_i2c_func(struct i2c_adapter *adap)
> +{
> +       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm loongson2_i2c_algo = {
> +       .master_xfer = loongson2_i2c_xfer,
> +       .functionality = loongson2_i2c_func,
> +};
> +
> +static int loongson2_i2c_adjust_bus_speed(struct loongson2_i2c_priv *priv)
> +{
> +       u32 val, freq, ccr = 0, cr2 = 0;
> +
> +       priv->parent_rate = clk_get_rate(priv->clk);
> +       freq = DIV_ROUND_UP(priv->parent_rate, HZ_TO_MHZ);
> +
> +       cr2 |= FIELD_GET(LOONGSON2_I2C_CR2_FREQ, freq);
> +       regmap_write(priv->regmap, LOONGSON2_I2C_CR2, cr2);
> +
> +       if (priv->speed == LOONGSON2_I2C_SPEED_STANDARD) {
> +               val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_STANDARD_MODE_FREQ * 2);
> +       } else {
> +               val = DIV_ROUND_UP(priv->parent_rate, I2C_MAX_FAST_MODE_FREQ * 3);
> +
> +               /* Select Fast mode */
> +               ccr |= LOONGSON2_I2C_CCR_FS;
> +       }
> +
> +       ccr |= FIELD_GET(LOONGSON2_I2C_CCR_CCR, val);
> +       regmap_write(priv->regmap, LOONGSON2_I2C_CCR, ccr);
> +
> +       /* reference clock determination the configure val(0x3f) */
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR2, LOONGSON2_I2C_CR2_FREQ,
> +                          LOONGSON2_I2C_CR2_FREQ);
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_TRISE, LOONGSON2_I2C_TRISE_SCL,
> +                          LOONGSON2_I2C_TRISE_SCL);
> +
> +       /* Enable I2C */
> +       regmap_update_bits(priv->regmap, LOONGSON2_I2C_CR1, LOONGSON2_I2C_CR1_PE,
> +                          LOONGSON2_I2C_CR1_PE);
> +
> +       return 0;
> +}
> +
> +static const struct regmap_config loongson2_i2c_regmap_config = {
> +       .reg_bits = 32,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .max_register = LOONGSON2_I2C_TRISE,
> +};
> +
> +static int loongson2_i2c_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct loongson2_i2c_priv *priv;
> +       struct i2c_adapter *adap;
> +       void __iomem *base;
> +       u32 clk_rate;
> +       int irq, ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(base))
> +               return dev_err_probe(dev, PTR_ERR(base),
> +                                    "Failed to ioremap resource.\n");
> +
> +       priv->regmap = devm_regmap_init_mmio(dev, base,
> +                                            &loongson2_i2c_regmap_config);
> +       if (IS_ERR(priv->regmap))
> +               return dev_err_probe(dev, PTR_ERR(priv->regmap),
> +                                    "Failed to init regmap.\n");
> +
> +       priv->clk = devm_clk_get_enabled(dev, NULL);
> +       if (IS_ERR(priv->clk))
> +               return dev_err_probe(dev, PTR_ERR(priv->clk),
> +                                    "Failed  to enable clock.\n");
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0)
> +               return -EINVAL;
> +
> +       priv->dev = dev;
> +
> +       adap = &priv->adapter;
> +       adap->retries = 5;
> +       adap->nr = pdev->id;
> +       adap->dev.parent = dev;
> +       adap->owner = THIS_MODULE;
> +       adap->algo = &loongson2_i2c_algo;
> +       adap->timeout = 2 * HZ;
> +       device_set_node(&adap->dev, dev_fwnode(dev));
> +       i2c_set_adapdata(adap, priv);
> +       strscpy(adap->name, pdev->name, sizeof(adap->name));
> +       init_completion(&priv->complete);
> +       platform_set_drvdata(pdev, priv);
> +
> +       priv->speed = LOONGSON2_I2C_SPEED_STANDARD;
> +       ret = of_property_read_u32(dev->of_node, "clock-frequency", &clk_rate);
> +       if (!ret && clk_rate >= I2C_MAX_FAST_MODE_FREQ)
> +               priv->speed = LOONGSON2_I2C_SPEED_FAST;
> +
> +       ret = loongson2_i2c_adjust_bus_speed(priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = devm_request_irq(dev, irq, loongson2_i2c_isr_event, IRQF_SHARED, pdev->name, priv);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
> +
> +       return devm_i2c_add_adapter(dev, adap);
> +}
> +
> +static const struct of_device_id loongson2_i2c_id_table[] = {
> +       { .compatible = "loongson,ls2k0300-i2c" },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, loongson2_i2c_id_table);
> +
> +static struct platform_driver loongson2_i2c_driver = {
> +       .driver = {
> +               .name = "loongson2-i2c-v2",
> +               .of_match_table = loongson2_i2c_id_table,
> +       },
> +       .probe = loongson2_i2c_probe,
> +};
> +
> +module_platform_driver(loongson2_i2c_driver);
> +
> +MODULE_DESCRIPTION("Loongson-2K0300 I2C bus driver");
> +MODULE_AUTHOR("Loongson Technology Corporation Limited");
> +MODULE_LICENSE("GPL");
> --
> 2.47.3
>
>

Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller

[Andy Shevchenko]

On Wed, Jan 28, 2026 at 12:15:08PM +0800, Huacai Chen wrote:
> On Tue, Jan 27, 2026 at 10:48 AM Binbin Zhou <zhoubinbin@loongson.cn> wrote:

...

> >  F:     Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> > +F:     drivers/i2c/busses/i2c-ls2x-v2.c
> I  still think it is better to move this line below. Though it may
> cause a checkpatch warning, but for checkpatch only errors must be
> fixed, whether warnings need to be eliminated depends on the fact.
> 
> >  F:     drivers/i2c/busses/i2c-ls2x.c

This should be sorted, and

$ cat << EOF | LC_ALL=C sort
> Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
> drivers/i2c/busses/i2c-ls2x-v2.c
> drivers/i2c/busses/i2c-ls2x.c

Documentation/devicetree/bindings/i2c/loongson,ls2x-i2c.yaml
drivers/i2c/busses/i2c-ls2x-v2.c
drivers/i2c/busses/i2c-ls2x.c

The patch is correct AFAICS.

P.S> Remove the unneeded context when you do not comment on it,
do not make others to waste time on reading that.

-- 
With Best Regards,
Andy Shevchenko