* [PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support
@ 2026-01-27 2:47 Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
0 siblings, 2 replies; 10+ messages in thread
From: Binbin Zhou @ 2026-01-27 2:47 UTC (permalink / raw)
To: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c
Cc: Huacai Chen, Xuerui Wang, loongarch, devicetree, 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible
2026-01-27 2:47 [PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support Binbin Zhou
@ 2026-01-27 2:47 ` Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
1 sibling, 0 replies; 10+ messages in thread
From: Binbin Zhou @ 2026-01-27 2:47 UTC (permalink / raw)
To: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c
Cc: Huacai Chen, Xuerui Wang, loongarch, devicetree, Binbin Zhou,
Conor Dooley
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-27 2:47 [PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible Binbin Zhou
@ 2026-01-27 2:47 ` Binbin Zhou
2026-01-27 8:18 ` Andy Shevchenko
2026-01-28 4:15 ` Huacai Chen
1 sibling, 2 replies; 10+ messages in thread
From: Binbin Zhou @ 2026-01-27 2:47 UTC (permalink / raw)
To: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c
Cc: Huacai Chen, Xuerui Wang, loongarch, devicetree, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
@ 2026-01-27 8:18 ` Andy Shevchenko
2026-01-29 8:07 ` Binbin Zhou
2026-01-28 4:15 ` Huacai Chen
1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2026-01-27 8:18 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Huacai Chen, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
2026-01-27 8:18 ` Andy Shevchenko
@ 2026-01-28 4:15 ` Huacai Chen
2026-01-28 9:08 ` Andy Shevchenko
1 sibling, 1 reply; 10+ messages in thread
From: Huacai Chen @ 2026-01-28 4:15 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Xuerui Wang, loongarch, devicetree
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-28 4:15 ` Huacai Chen
@ 2026-01-28 9:08 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2026-01-28 9:08 UTC (permalink / raw)
To: Huacai Chen
Cc: Binbin Zhou, Binbin Zhou, Huacai Chen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Andi Shyti, Wolfram Sang,
Andy Shevchenko, linux-i2c, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-27 8:18 ` Andy Shevchenko
@ 2026-01-29 8:07 ` Binbin Zhou
2026-01-29 14:38 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Binbin Zhou @ 2026-01-29 8:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Huacai Chen, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-29 8:07 ` Binbin Zhou
@ 2026-01-29 14:38 ` Andy Shevchenko
2026-02-02 6:39 ` Binbin Zhou
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2026-01-29 14:38 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Huacai Chen, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-01-29 14:38 ` Andy Shevchenko
@ 2026-02-02 6:39 ` Binbin Zhou
2026-02-03 9:53 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Binbin Zhou @ 2026-02-02 6:39 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Huacai Chen, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller
2026-02-02 6:39 ` Binbin Zhou
@ 2026-02-03 9:53 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2026-02-03 9:53 UTC (permalink / raw)
To: Binbin Zhou
Cc: Binbin Zhou, Huacai Chen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Andi Shyti, Wolfram Sang, Andy Shevchenko,
linux-i2c, Huacai Chen, Xuerui Wang, loongarch, devicetree
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-02-03 9:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-27 2:47 [PATCH v2 0/2] i2c: Add Loongson-2K0300 I2C controller support Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 1/2] dt-bindings: i2c: loongson,ls2x: Add ls2k0300-i2c compatible Binbin Zhou
2026-01-27 2:47 ` [PATCH v2 2/2] i2c: ls2x-v2: Add driver for Loongson-2K0300 I2C controller Binbin Zhou
2026-01-27 8:18 ` Andy Shevchenko
2026-01-29 8:07 ` Binbin Zhou
2026-01-29 14:38 ` Andy Shevchenko
2026-02-02 6:39 ` Binbin Zhou
2026-02-03 9:53 ` Andy Shevchenko
2026-01-28 4:15 ` Huacai Chen
2026-01-28 9:08 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox