[PATCH v4 0/2] i2c: Add Cortina Systems CS75xx I2C controller driver

[PATCH v4 0/2] i2c: Add Cortina Systems CS75xx I2C controller driver

[Fil Dunsky via B4 Relay]

The I2C ("Bi-Wire") controller of the Cortina Systems CS75xx
("Goldengate G2") SoCs transfers one byte per command, with a command
register much like the OpenCores one, and raises an interrupt when a
command has completed. This series adds the binding and a driver for
plain I2C and emulated SMBus, without quick commands (every message
carries at least one data byte) and without 10-bit addresses.

The driver used to be part of a series with the interrupt controller,
GPIO and watchdog drivers and the device tree changes. That series is
now split per subsystem: the driver builds with COMPILE_TEST and does
not depend on the platform series [1], which is under review separately.
The device tree nodes will follow with the other CS75xx dts changes once
the platform is merged.

There is no public documentation; the register layout comes from the GPL
vendor kernels. Tested on a Securifi Almond+ (CS7542) with the SX8650
touchscreen controller at 0x48: reads are ACKed, an empty address returns
-ENXIO without timeouts. Repeated START between messages has not been
exercised on the board.

[1] https://lore.kernel.org/all/20261114-b4-cs75xx-v7-0-8f17f38821b4@gmail.com/

---
Changes in v4:
- rebase on v7.4-rc1
- split out of "Cortina Systems CS75xx: interrupt controller, GPIO, I2C
  and TWD watchdog" so that each driver can go through its own tree; no
  longer based on the platform series
- binding: limit clock-frequency to 400 kHz instead of listing 100 and
  400 kHz, which is what the driver accepts; I kept Rob's Reviewed-by
  from v3 for this one-line change
- collect Reviewed-by tags
- use fsleep() and device_set_node(); sort the Kconfig and Makefile
  entries
- Link to v3: https://patch.msgid.link/20261013-b4-cs75xx-s2-v3-0-181bb0d23f4c@gmail.com

Changes in v3:
- rebase on v6 of the platform series
- reset the controller with its interrupt disabled, check the prescaler
  against the field width exactly, wait for a bus held by another master
  up to the adapter timeout (sashiko-bot)
- Link to v2: https://patch.msgid.link/20261013-b4-cs75xx-s2-v2-0-94f8c0a90cda@gmail.com

Changes in v2:
- rebase on v5 of the platform series
- do not include <linux/mod_devicetable.h> (Uwe)
- complete a failed transfer only once its stop condition is done, so
  that the stop interrupt cannot race with the next transfer; take the
  lock over the whole interrupt handler; report a NACK of the last byte
  of a write; enable the interrupt again after a reset on timeout; fail
  with -EBUSY when the bus stays busy (sashiko-bot)
- Link to v1: https://patch.msgid.link/20261012-b4-cs75xx-s2-v1-0-8a7641b7b79c@gmail.com

To: Andi Shyti <andi.shyti@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org

---
Fil Dunsky (2):
      dt-bindings: i2c: Add Cortina Systems CS7542 I2C controller
      i2c: Add Cortina Systems CS75xx I2C controller driver

 .../bindings/i2c/cortina,cs7542-i2c.yaml           |  56 +++
 drivers/i2c/busses/Kconfig                         |  11 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-cs75xx.c                    | 392 +++++++++++++++++++++
 4 files changed, 460 insertions(+)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261015-cs75xx-i2c-944d27231b4c

Best regards,
--  
Fil Dunsky <filipp.dunsky@gmail.com>

[PATCH v4 1/2] dt-bindings: i2c: Add Cortina Systems CS7542 I2C controller

[Fil Dunsky via B4 Relay]

From: Fil Dunsky <filipp.dunsky@gmail.com>

Add a binding for the I2C ("Bi-Wire") controller of the Cortina Systems
CS75xx SoCs.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Fil Dunsky <filipp.dunsky@gmail.com>
---
 .../bindings/i2c/cortina,cs7542-i2c.yaml           | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/cortina,cs7542-i2c.yaml b/Documentation/devicetree/bindings/i2c/cortina,cs7542-i2c.yaml
new file mode 100644
index 000000000..095e24379
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/cortina,cs7542-i2c.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/cortina,cs7542-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cortina Systems CS7542 I2C controller
+
+maintainers:
+  - Fil Dunsky <filipp.dunsky@gmail.com>
+
+description:
+  The I2C ("Bi-Wire") controller of the Cortina Systems CS75xx (Goldengate
+  G2) SoCs. It transfers one byte per command and runs from the peripheral
+  (APB) clock through a 16-bit prescaler.
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    const: cortina,cs7542-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-frequency:
+    maximum: 400000
+    default: 100000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c@f00700e8 {
+        compatible = "cortina,cs7542-i2c";
+        reg = <0xf00700e8 0x28>;
+        interrupt-parent = <&intc>;
+        interrupts = <11>;
+        clocks = <&apb_clk>;
+        clock-frequency = <400000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+    };

-- 
2.53.0

[PATCH v4 2/2] i2c: Add Cortina Systems CS75xx I2C controller driver

[Fil Dunsky via B4 Relay]

From: Fil Dunsky <filipp.dunsky@gmail.com>

The I2C ("Bi-Wire") controller of the Cortina Systems CS75xx SoCs
transfers one byte per command, with a command register much like the
OpenCores one (start, stop, read, write, acknowledge), and raises an
interrupt when a command has completed.

Messages are sent with a repeated start between them and a stop after
the last one. Every message needs at least one data byte, so SMBus quick
commands are not supported. Arbitration loss is reported as -EAGAIN and
retried by the core.

Signed-off-by: Fil Dunsky <filipp.dunsky@gmail.com>
---
 drivers/i2c/busses/Kconfig      |  11 ++
 drivers/i2c/busses/Makefile     |   1 +
 drivers/i2c/busses/i2c-cs75xx.c | 392 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 404 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 275aaa7f3..db1b218a6 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -550,6 +550,17 @@ config I2C_CPM
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-cpm.
 
+config I2C_CS75XX
+	tristate "Cortina Systems CS75xx I2C controller"
+	depends on ARCH_CORTINA_ACCESS || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Say yes here to support the I2C ("Bi-Wire") controller of the
+	  Cortina Systems CS75xx SoCs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-cs75xx.
+
 config I2C_DAVINCI
 	tristate "DaVinci I2C driver"
 	depends on ARCH_DAVINCI || ARCH_KEYSTONE || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e5440de0f..f19df25eb 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_I2C_CADENCE)	+= i2c-cadence.o
 obj-$(CONFIG_I2C_CBUS_GPIO)	+= i2c-cbus-gpio.o
 obj-$(CONFIG_I2C_CGBC)		+= i2c-cgbc.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
+obj-$(CONFIG_I2C_CS75XX)	+= i2c-cs75xx.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
 obj-$(CONFIG_I2C_DESIGNWARE_CORE)			+= i2c-designware-core.o
 i2c-designware-core-y					:= i2c-designware-common.o
diff --git a/drivers/i2c/busses/i2c-cs75xx.c b/drivers/i2c/busses/i2c-cs75xx.c
new file mode 100644
index 000000000..a7aec20ea
--- /dev/null
+++ b/drivers/i2c/busses/i2c-cs75xx.c
@@ -0,0 +1,392 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Cortina Systems CS75xx I2C ("Bi-Wire") controller
+ *
+ * The controller works one byte at a time: software writes the byte to
+ * transmit and a command (start, stop, read, write, acknowledge) and gets
+ * an interrupt when the command has completed.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/spinlock.h>
+
+#define CS75XX_I2C_CFG		0x00
+#define CS75XX_I2C_CTRL		0x04
+#define CS75XX_I2C_TXR		0x08
+#define CS75XX_I2C_RXR		0x0c
+#define CS75XX_I2C_ACK		0x10
+#define CS75XX_I2C_IE		0x14
+#define CS75XX_I2C_INT		0x18
+
+#define CS75XX_I2C_CFG_EN	BIT(0)
+#define CS75XX_I2C_CFG_PRER	GENMASK(31, 16)
+
+#define CS75XX_I2C_CTRL_DONE	BIT(0)
+#define CS75XX_I2C_CTRL_NACK	BIT(3)
+#define CS75XX_I2C_CTRL_WRITE	BIT(4)
+#define CS75XX_I2C_CTRL_READ	BIT(5)
+#define CS75XX_I2C_CTRL_STOP	BIT(6)
+#define CS75XX_I2C_CTRL_START	BIT(7)
+
+#define CS75XX_I2C_ACK_AL	BIT(0)
+#define CS75XX_I2C_ACK_BUSY	BIT(1)
+#define CS75XX_I2C_ACK_NACK	BIT(2)
+
+#define CS75XX_I2C_INT_DONE	BIT(0)
+
+enum cs75xx_i2c_state {
+	CS75XX_I2C_IDLE,
+	CS75XX_I2C_ADDR,
+	CS75XX_I2C_WRITE,
+	CS75XX_I2C_READ,
+	CS75XX_I2C_STOP,
+};
+
+struct cs75xx_i2c {
+	struct i2c_adapter adap;
+	struct device *dev;
+	void __iomem *base;
+	int irq;
+	u32 prer;
+
+	spinlock_t lock;	/* protects the transfer state */
+	struct completion done;
+	enum cs75xx_i2c_state state;
+	struct i2c_msg *msg;
+	unsigned int pos;
+	bool last;
+	int err;
+};
+
+static void cs75xx_i2c_hw_init(struct cs75xx_i2c *i2c)
+{
+	if (readl(i2c->base + CS75XX_I2C_CFG) & CS75XX_I2C_CFG_EN) {
+		writel(0, i2c->base + CS75XX_I2C_CFG);
+		fsleep(5000);
+	}
+
+	writel(FIELD_PREP(CS75XX_I2C_CFG_PRER, i2c->prer) | CS75XX_I2C_CFG_EN,
+	       i2c->base + CS75XX_I2C_CFG);
+	fsleep(5000);
+}
+
+/*
+ * Reset the controller and enable its interrupt again, with the handler
+ * kept away from the registers meanwhile
+ */
+static void cs75xx_i2c_reset(struct cs75xx_i2c *i2c)
+{
+	disable_irq(i2c->irq);
+	cs75xx_i2c_hw_init(i2c);
+	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
+	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_IE);
+	enable_irq(i2c->irq);
+}
+
+static void cs75xx_i2c_finish(struct cs75xx_i2c *i2c, int err)
+{
+	i2c->err = err;
+	i2c->state = CS75XX_I2C_IDLE;
+	complete(&i2c->done);
+}
+
+/*
+ * End the transfer with a stop condition and complete it only once the
+ * stop is done, so that its interrupt cannot be taken for the next one
+ */
+static void cs75xx_i2c_abort(struct cs75xx_i2c *i2c, int err)
+{
+	i2c->err = err;
+	i2c->state = CS75XX_I2C_STOP;
+	writel(CS75XX_I2C_CTRL_STOP, i2c->base + CS75XX_I2C_CTRL);
+}
+
+/* Start the transfer of the next data byte of the message */
+static void cs75xx_i2c_next_byte(struct cs75xx_i2c *i2c)
+{
+	struct i2c_msg *msg = i2c->msg;
+	bool last_byte = i2c->pos == msg->len - 1;
+	u32 cmd;
+
+	if (msg->flags & I2C_M_RD) {
+		i2c->state = CS75XX_I2C_READ;
+		cmd = CS75XX_I2C_CTRL_READ;
+		/* Do not acknowledge the last byte of a read */
+		if (last_byte)
+			cmd |= CS75XX_I2C_CTRL_NACK;
+	} else {
+		i2c->state = CS75XX_I2C_WRITE;
+		writel(msg->buf[i2c->pos++], i2c->base + CS75XX_I2C_TXR);
+		cmd = CS75XX_I2C_CTRL_WRITE;
+	}
+
+	if (last_byte && i2c->last)
+		cmd |= CS75XX_I2C_CTRL_STOP;
+
+	writel(cmd, i2c->base + CS75XX_I2C_CTRL);
+}
+
+static void cs75xx_i2c_process(struct cs75xx_i2c *i2c, u32 ack)
+{
+	struct i2c_msg *msg = i2c->msg;
+
+	if (i2c->state == CS75XX_I2C_STOP) {
+		cs75xx_i2c_finish(i2c, i2c->err);
+		return;
+	}
+
+	if (ack & CS75XX_I2C_ACK_AL) {
+		cs75xx_i2c_abort(i2c, -EAGAIN);
+		return;
+	}
+
+	switch (i2c->state) {
+	case CS75XX_I2C_ADDR:
+		if (ack & CS75XX_I2C_ACK_NACK) {
+			cs75xx_i2c_abort(i2c, -ENXIO);
+			return;
+		}
+		cs75xx_i2c_next_byte(i2c);
+		break;
+
+	case CS75XX_I2C_WRITE:
+		if (ack & CS75XX_I2C_ACK_NACK) {
+			/* The last byte of the last message carried the stop */
+			if (i2c->pos == msg->len && i2c->last)
+				cs75xx_i2c_finish(i2c, -EIO);
+			else
+				cs75xx_i2c_abort(i2c, -EIO);
+			return;
+		}
+		if (i2c->pos == msg->len) {
+			cs75xx_i2c_finish(i2c, 0);
+			return;
+		}
+		cs75xx_i2c_next_byte(i2c);
+		break;
+
+	case CS75XX_I2C_READ:
+		msg->buf[i2c->pos++] = readl(i2c->base + CS75XX_I2C_RXR);
+		if (i2c->pos == msg->len) {
+			cs75xx_i2c_finish(i2c, 0);
+			return;
+		}
+		cs75xx_i2c_next_byte(i2c);
+		break;
+
+	case CS75XX_I2C_IDLE:
+	case CS75XX_I2C_STOP:
+		/* Late interrupt of a transfer that timed out */
+		break;
+	}
+}
+
+static irqreturn_t cs75xx_i2c_isr(int irq, void *dev_id)
+{
+	struct cs75xx_i2c *i2c = dev_id;
+	u32 ctrl, ack;
+
+	guard(spinlock)(&i2c->lock);
+
+	if (!(readl(i2c->base + CS75XX_I2C_INT) & CS75XX_I2C_INT_DONE))
+		return IRQ_NONE;
+
+	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
+
+	/* The done flag is cleared by writing it back */
+	ctrl = readl(i2c->base + CS75XX_I2C_CTRL);
+	writel(ctrl, i2c->base + CS75XX_I2C_CTRL);
+
+	ack = readl(i2c->base + CS75XX_I2C_ACK);
+	cs75xx_i2c_process(i2c, ack);
+
+	return IRQ_HANDLED;
+}
+
+static int cs75xx_i2c_xfer_msg(struct cs75xx_i2c *i2c, struct i2c_msg *msg,
+			       bool last)
+{
+	unsigned long time_left;
+
+	spin_lock_irq(&i2c->lock);
+	reinit_completion(&i2c->done);
+	i2c->msg = msg;
+	i2c->pos = 0;
+	i2c->last = last;
+	i2c->err = 0;
+	i2c->state = CS75XX_I2C_ADDR;
+	writel(i2c_8bit_addr_from_msg(msg), i2c->base + CS75XX_I2C_TXR);
+	writel(CS75XX_I2C_CTRL_START | CS75XX_I2C_CTRL_WRITE,
+	       i2c->base + CS75XX_I2C_CTRL);
+	spin_unlock_irq(&i2c->lock);
+
+	time_left = wait_for_completion_timeout(&i2c->done, i2c->adap.timeout);
+	if (!time_left) {
+		spin_lock_irq(&i2c->lock);
+		i2c->state = CS75XX_I2C_IDLE;
+		spin_unlock_irq(&i2c->lock);
+		dev_err(i2c->dev, "transfer timed out, resetting the controller\n");
+		cs75xx_i2c_reset(i2c);
+		return -ETIMEDOUT;
+	}
+
+	return i2c->err;
+}
+
+static int cs75xx_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			   int num)
+{
+	struct cs75xx_i2c *i2c = i2c_get_adapdata(adap);
+	u32 val;
+	int i, ret;
+
+	/* Another master may hold the bus for a whole transfer */
+	if (readl_poll_timeout(i2c->base + CS75XX_I2C_ACK, val,
+			       !(val & CS75XX_I2C_ACK_BUSY), 100,
+			       jiffies_to_usecs(adap->timeout))) {
+		cs75xx_i2c_reset(i2c);
+		if (readl(i2c->base + CS75XX_I2C_ACK) & CS75XX_I2C_ACK_BUSY)
+			return -EBUSY;
+	}
+
+	for (i = 0; i < num; i++) {
+		ret = cs75xx_i2c_xfer_msg(i2c, &msgs[i], i == num - 1);
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static u32 cs75xx_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+}
+
+static const struct i2c_algorithm cs75xx_i2c_algo = {
+	.xfer = cs75xx_i2c_xfer,
+	.functionality = cs75xx_i2c_func,
+};
+
+/* A message always carries at least one data byte after the address */
+static const struct i2c_adapter_quirks cs75xx_i2c_quirks = {
+	.flags = I2C_AQ_NO_ZERO_LEN,
+};
+
+static void cs75xx_i2c_disable(void *data)
+{
+	struct cs75xx_i2c *i2c = data;
+
+	writel(0, i2c->base + CS75XX_I2C_IE);
+	writel(0, i2c->base + CS75XX_I2C_CFG);
+}
+
+static int cs75xx_i2c_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cs75xx_i2c *i2c;
+	unsigned long ref_rate;
+	struct clk *clk;
+	u32 bus_rate;
+	u64 div;
+	int ret;
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->dev = dev;
+	spin_lock_init(&i2c->lock);
+	init_completion(&i2c->done);
+
+	i2c->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(i2c->base))
+		return PTR_ERR(i2c->base);
+
+	i2c->irq = platform_get_irq(pdev, 0);
+	if (i2c->irq < 0)
+		return i2c->irq;
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
+
+	ref_rate = clk_get_rate(clk);
+	if (device_property_read_u32(dev, "clock-frequency", &bus_rate))
+		bus_rate = I2C_MAX_STANDARD_MODE_FREQ;
+	if (!bus_rate || bus_rate > I2C_MAX_FAST_MODE_FREQ)
+		return dev_err_probe(dev, -EINVAL,
+				     "unsupported bus frequency %u Hz\n",
+				     bus_rate);
+
+	div = DIV_ROUND_UP_ULL(ref_rate, 5ULL * bus_rate);
+	if (!div || div - 1 > FIELD_MAX(CS75XX_I2C_CFG_PRER))
+		return dev_err_probe(dev, -EINVAL,
+				     "cannot derive %u Hz from %lu Hz\n",
+				     bus_rate, ref_rate);
+	i2c->prer = div - 1;
+
+	cs75xx_i2c_hw_init(i2c);
+
+	writel(0, i2c->base + CS75XX_I2C_IE);
+	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
+
+	ret = devm_add_action_or_reset(dev, cs75xx_i2c_disable, i2c);
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(dev, i2c->irq, cs75xx_i2c_isr, 0, dev_name(dev),
+			       i2c);
+	if (ret)
+		return ret;
+
+	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_IE);
+
+	i2c->adap.owner = THIS_MODULE;
+	i2c->adap.algo = &cs75xx_i2c_algo;
+	i2c->adap.quirks = &cs75xx_i2c_quirks;
+	i2c->adap.retries = 3;
+	i2c->adap.dev.parent = dev;
+	device_set_node(&i2c->adap.dev, dev_fwnode(dev));
+	strscpy(i2c->adap.name, dev_name(dev), sizeof(i2c->adap.name));
+	i2c_set_adapdata(&i2c->adap, i2c);
+
+	ret = devm_i2c_add_adapter(dev, &i2c->adap);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "bus %u Hz, reference %lu Hz\n", bus_rate,
+		ref_rate);
+
+	return 0;
+}
+
+static const struct of_device_id cs75xx_i2c_of_match[] = {
+	{ .compatible = "cortina,cs7542-i2c" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cs75xx_i2c_of_match);
+
+static struct platform_driver cs75xx_i2c_driver = {
+	.probe = cs75xx_i2c_probe,
+	.driver = {
+		.name = "cs75xx-i2c",
+		.of_match_table = cs75xx_i2c_of_match,
+	},
+};
+module_platform_driver(cs75xx_i2c_driver);
+
+MODULE_DESCRIPTION("Cortina Systems CS75xx I2C controller driver");
+MODULE_LICENSE("GPL");

-- 
2.53.0

Re: [PATCH v4 2/2] i2c: Add Cortina Systems CS75xx I2C controller driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] i2c: cs75xx: CS75XX_I2C_ACK_AL handled before CS75XX_I2C_IDLE
  in cs75xx_i2c_process()
- [Medium] i2c: cs75xx: Kconfig depends on undefined symbol
  ARCH_CORTINA_ACCESS
- [Low] i2c: cs75xx: unused CS75XX_I2C_CTRL_DONE macro in
  cs75xx_i2c_isr()
--

commit a796a9d6a4def2db9ac47c7036d59a29ac7bf6d7
Author: Fil Dunsky <filipp.dunsky@gmail.com>

i2c: Add Cortina Systems CS75xx I2C controller driver

This commit adds an I2C controller driver for the Cortina Systems CS75xx
("Bi-Wire") controller.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 275aaa7f35833..db1b218a66a23 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -550,6 +550,17 @@ config I2C_CPM
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-cpm.
>  
> +config I2C_CS75XX
> +	tristate "Cortina Systems CS75xx I2C controller"
> +	depends on ARCH_CORTINA_ACCESS || COMPILE_TEST

[Severity: Medium]
Does ARCH_CORTINA_ACCESS exist in Kconfig?  No Kconfig file in the tree
defines this symbol, so CONFIG_I2C_CS75XX cannot be enabled unless
CONFIG_COMPILE_TEST is set.

[ ... ]

> diff --git a/drivers/i2c/busses/i2c-cs75xx.c b/drivers/i2c/busses/i2c-cs75xx.c
> new file mode 100644
> index 0000000000000..a7aec20ea5cf1
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-cs75xx.c

[ ... ]

> +static void cs75xx_i2c_process(struct cs75xx_i2c *i2c, u32 ack)
> +{
> +	struct i2c_msg *msg = i2c->msg;
> +
> +	if (i2c->state == CS75XX_I2C_STOP) {
> +		cs75xx_i2c_finish(i2c, i2c->err);
> +		return;
> +	}
> +
> +	if (ack & CS75XX_I2C_ACK_AL) {
> +		cs75xx_i2c_abort(i2c, -EAGAIN);
> +		return;
> +	}
> +
> +	switch (i2c->state) {
> +	case CS75XX_I2C_ADDR:
> +		if (ack & CS75XX_I2C_ACK_NACK) {
> +			cs75xx_i2c_abort(i2c, -ENXIO);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_WRITE:
> +		if (ack & CS75XX_I2C_ACK_NACK) {
> +			/* The last byte of the last message carried the stop */
> +			if (i2c->pos == msg->len && i2c->last)
> +				cs75xx_i2c_finish(i2c, -EIO);
> +			else
> +				cs75xx_i2c_abort(i2c, -EIO);
> +			return;
> +		}
> +		if (i2c->pos == msg->len) {
> +			cs75xx_i2c_finish(i2c, 0);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_READ:
> +		msg->buf[i2c->pos++] = readl(i2c->base + CS75XX_I2C_RXR);
> +		if (i2c->pos == msg->len) {
> +			cs75xx_i2c_finish(i2c, 0);
> +			return;
> +		}
> +		cs75xx_i2c_next_byte(i2c);
> +		break;
> +
> +	case CS75XX_I2C_IDLE:
> +	case CS75XX_I2C_STOP:
> +		/* Late interrupt of a transfer that timed out */
> +		break;
> +	}
> +}

[Severity: Medium]
Can a late interrupt with CS75XX_I2C_ACK_AL set trigger cs75xx_i2c_abort()
while i2c->state is CS75XX_I2C_IDLE?

When cs75xx_i2c_xfer_msg() times out, it sets i2c->state to CS75XX_I2C_IDLE
and drops i2c->lock before cs75xx_i2c_reset() calls disable_irq().  If a
late interrupt arrives in that window with CS75XX_I2C_ACK_AL set,
cs75xx_i2c_process() checks CS75XX_I2C_ACK_AL before checking for
CS75XX_I2C_IDLE, writes CS75XX_I2C_CTRL_STOP to the hardware and leaves
i2c->state in CS75XX_I2C_STOP across cs75xx_i2c_reset().  In addition, case
CS75XX_I2C_STOP in the switch statement is unreachable because
cs75xx_i2c_process() already returns early for CS75XX_I2C_STOP.

> +
> +static irqreturn_t cs75xx_i2c_isr(int irq, void *dev_id)
> +{
> +	struct cs75xx_i2c *i2c = dev_id;
> +	u32 ctrl, ack;
> +
> +	guard(spinlock)(&i2c->lock);
> +
> +	if (!(readl(i2c->base + CS75XX_I2C_INT) & CS75XX_I2C_INT_DONE))
> +		return IRQ_NONE;
> +
> +	writel(CS75XX_I2C_INT_DONE, i2c->base + CS75XX_I2C_INT);
> +
> +	/* The done flag is cleared by writing it back */
> +	ctrl = readl(i2c->base + CS75XX_I2C_CTRL);
> +	writel(ctrl, i2c->base + CS75XX_I2C_CTRL);

[Severity: Low]
Should cs75xx_i2c_isr() write CS75XX_I2C_CTRL_DONE instead of writing back
the entire CS75XX_I2C_CTRL register?  CS75XX_I2C_CTRL_DONE is defined at the
top of the file but never used, and writing back all of ctrl risks
re-asserting any non-self-clearing control bits in CS75XX_I2C_CTRL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-cs75xx-i2c-v4-0-4b16eea98a78@gmail.com?part=2