[PATCH v3 0/3] i2c: Add Novatek NT726xx SoC I2C controller

[PATCH v3 0/3] i2c: Add Novatek NT726xx SoC I2C controller

[Nina_Kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>


These patch series adds support for the I2C bus controllers on
Novatek NT726xx SoCs.

The controller driver implements the fundamental I2C read/write
operations and supports Standard-mode and Fast-mode.

Some I2C bus controllers on Novatek NT726xx SoCs are controllable by
stbc (Standby controller) and named as `stbc-i2c`s. These `stbc-i2c`s
are driven by stbc clock (12 MHz) and require the authentication to
stbc before any control.

The patch is tested with concurrent read/write operations on
2 different I2C busses for 1000000 times on Novatek NT72676 SoC.
Neither error nor data corruption is detected under this test.

---
v3:
  - Only remove attached HTML messages, no code is modified.

v2:
  https://lore.kernel.org/lkml/20260714094145.84387-1-Nina_Kuo@novatek.com.tw/T/#t
  From Krzysztof's review of novatek,nt726xx-i2c.yaml:
  - Explicitly specify the unique compatibles "novatek,nt72600-i2c" for
    Novatek NT726xx SoCs
  - Modify `maxItems` as 1 for both `reg` and `interrupts` properties
  - Modify description and allowed values for `clock-frequency`
    property
  - Remove `minItems` for `novatek,hwmods` custom property
  - Rename, fix wrong type definition and add more description for
    `novatek,stbc-controllable` custom property
  - Remove `bus-enable` custom property
  - Add an example for dtsi declaration

  From Sashiko AI's review of i2c-nt726xx.c:
  - Remove flows related to `bus-enable` custom property
  - Add default value 100000 (100 kHz) of `clock-frequency` property
    to prevent from devide-by-zero panic
  - Add flows for handling ioremap() in nvt_i2c_use_case_feature()
  - Remove I2C_FUNC_SMBUS_QUICK functionality due to unsupported handling
    of 0-byte messages
  - Add flows of disabling IRQs in nvt_i2c_suspend() and nvt_i2c_remove()
  - Add of_node_get() to release the reference to device tree node in
    nvt_i2c_remove()

v1:
  https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#t

Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>


Ben Huang (3):
  MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver
  dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller
  i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

 .../bindings/i2c/novatek,nt726xx-i2c.yaml     |  74 ++
 MAINTAINERS                                   |   7 +
 drivers/i2c/busses/Kconfig                    |  10 +
 drivers/i2c/busses/Makefile                   |   1 +
 drivers/i2c/busses/i2c-nt726xx.c              | 698 ++++++++++++++++++
 5 files changed, 790 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
 create mode 100644 drivers/i2c/busses/i2c-nt726xx.c

-- 
2.40.1

[PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[Nina_Kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

Add entry for maintenance of Novatek NT726xx SoC I2C controller driver.

Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 806bd2d80d15..15df47f9e81f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19234,6 +19234,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc.git
 F:	tools/include/nolibc/
 F:	tools/testing/selftests/nolibc/
 
+NOVATEK NT726XX I2C CONTROLLER DRIVER
+M:	Ben Huang <ben_huang@novatek.com.tw>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
+F:	drivers/i2c/busses/i2c-nt726xx.c
+
 NOVATEK NVT-TS I2C TOUCHSCREEN DRIVER
 M:	Hans de Goede <hansg@kernel.org>
 L:	linux-input@vger.kernel.org
-- 
2.40.1

[PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller

[Nina_Kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

Add device tree documentation for Novatek NT726xx SoC I2C controller.

Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
---
 .../bindings/i2c/novatek,nt726xx-i2c.yaml     | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
new file mode 100644
index 000000000000..866589c5ae51
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/novatek,nt726xx-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+maintainers:
+  - Ben Huang <ben_huang@novatek.com.tw>
+  - Jason JJ Wu <jason_jj_wu@novatek.com.tw>
+
+title: Novatek NT726xx Series SoC I2C master controller
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    const: novatek,nt72600-i2c
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clock-frequency:
+    default: 100000
+    enum: [ 100000, 400000 ]
+
+  novatek,hwmods:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Name of each i2c pin, must be named with "i2cX". (X is
+                 an integer starting from 0, must be unique)
+
+  novatek,stbc-controllable:
+    type: boolean
+    description: Set if this i2c master, named as `stbc-i2c`, is
+      controllable by stbc (Standby controller, actually is an 8051
+      micro-processor) on Novatek NT726xx SoCs.
+      For these `stbc-i2c`s, the driven clock is only 12 MHz and the
+      authentication to stbc must be executed before controlling the
+      registers.
+
+    default: 0
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - novatek,hwmods
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c0: i2c@10000000 {
+      compatible = "novatek,nt72600-i2c";
+      reg = <0x0 0x10000000 0x0 0x100>;
+      interrupts = <0 67 4>;
+      clock-frequency = <400000>;
+      novatek,hwmods = "i2c0";
+      #address-cells = <2>;
+      #size-cells = <2>;
+    };
+
+    stbc-i2c1: i2c@20000000 {
+      compatible = "novatek,nt72600-i2c";
+      reg = <0x0 0x20000000 0x0 0x100>;
+      interrupts = <0 77 4>;
+      novatek,hwmods = "i2c1";
+      novatek,stbc-controllable;
+      #address-cells = <2>;
+      #size-cells = <2>;
+    };
-- 
2.40.1

[PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

[Nina_Kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

This patch introduce the support of I2C bus controller driver of
Novatek NT726xx SoCs.

This driver performs the fundamental read/write functions as
an I2C controller and supports Standard-mode and Fast-mode.
Default operation is Stardard-mode.

Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
---
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-nt726xx.c | 698 +++++++++++++++++++++++++++++++
 3 files changed, 709 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-nt726xx.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d35456994280..52ee2c82e9d5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -962,6 +962,16 @@ config I2C_NPCM
 	  controllers.
 	  Driver can also support slave mode (select I2C_SLAVE).
 
+config I2C_NT726XX
+	tristate "Novatek NT726xx Driver"
+	default n
+	help
+	  Say Y here if you want to enable I2C bus controller on
+	  Novatek NT726xx SoCs.
+	  This driver performs fundamental read/write functions
+	  as an I2C bus controller and supports Standard-mode and
+	  Fast-mode. Default operation is Standard-mode.
+
 config I2C_OCORES
 	tristate "OpenCores I2C Controller"
 	help
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3755c54b3d82..e82646919916 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
 obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_NT726XX)	+= i2c-nt726xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
new file mode 100644
index 000000000000..ac3fd7ccc68c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nt726xx.c
@@ -0,0 +1,698 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Novatek Microelectronics Corp.
+ * Author: Ben Huang <ben_huang@novatek.com.tw>
+ */
+
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/version.h>
+
+#define I2C_INFO_LOG(fmt, ...)		\
+	pr_info("[I/SOC_I2C] " fmt, ##__VA_ARGS__)
+
+#define I2C_ERR_LOG(fmt, ...)		\
+	pr_err("[E/SOC_I2C] " fmt, ##__VA_ARGS__)
+
+#define I2C_NAME                              "NT726xx I2C adapter"
+#define I2C_REG_CTRL                          0xC0
+#define I2C_REG_CLK                           0xC4
+#define I2C_REG_ACK                           0xC8
+#define I2C_REG_SIZE                          0xCC
+#define I2C_REG_FIFO1                         0xD0
+#define I2C_REG_SUBADDR                       0xE0
+#define I2C_REG_PINGPONG                      0xE4
+#define I2C_REG_INTR                          0xE8
+#define I2C_REG_FIFO2                         0xEC
+#define I2C_REG_DUTY                          0xFC
+#define I2C_CLR_FIFO1                         0x80
+#define I2C_CLR_FIFO2                         0x800000
+#define I2C_BUF_LITTLE_ENDIAN                 0x00002000
+#define I2C_BUSY                              0x00000002
+#define I2C_ENABLE                            0x00000004
+#define I2C_REPEAT_ENABLE                     0x00000080
+#define I2C_READ_OPERATION                    0x00000100
+#define I2C_NACK                              0x01000000
+#define I2C_CLOCK_DUTY_ENABLE                 0x00200000
+#define I2C_CLOCK_STRETCH_ENABLE              0x08000000
+#define I2C_MASTER_CLK_STRETCH_ENABLE         0x10000000
+#define I2C_TRIGGER                           0x00000001
+#define I2C_IRQ_FLAG                          0x0000FF00
+#define I2C_IRQ_ARBI_LOSS                     0x00008000
+#define I2C_IRQ_SUS                           0x00004000
+#define I2C_IRQ_ALERT                         0x00002000
+#define I2C_IRQ_CLK_STR_TIMEOUT               0x00001000
+#define I2C_IRQ_NACK                          0x00000800
+#define I2C_IRQ_RX_FULL                       0x00000400
+#define I2C_IRQ_TX_EMPTY                      0x00000200
+#define I2C_IRQ_FINISH                        0x00000100
+#define I2C_IRQ_TX_SETTING                    0x001F001B
+#define I2C_IRQ_RX_SETTING                    0x001F001D
+#define I2C_IRQ_ENABLE_SETTING                0x001F001F
+#define I2C_IRQ_DISABLE_SETTING               0x00000000
+#define I2C_SUBADDR_ENABLE                    0x00000040
+#define I2C_16BITSUBADDR_ENABLE               0x00010000
+#define I2C_24BITSUBADDR_ENABLE               0x00020000
+#define I2C_32BITSUBADDR_ENABLE               0x00040000
+#define I2C_ACK_CTRL_COUNTER                  0x1E0
+#define I2C_TX_EMPTY_FIFO1                    0x40
+#define I2C_TX_EMPTY_FIFO2                    0x400000
+#define I2C_RX_FULL_FIFO1                     0x20
+#define I2C_RX_FULL_FIFO2                     0x200000
+#define FIFO_1                                0
+#define FIFO_2                                1
+#define FIFO_ALL                              2
+#define FIFO_CHUNK_SIZE                       16
+#define FIFO_WORD_BYTES                       4
+#define STBC_MASTER                           0xFC040000
+#define STBC_PSWD                             0xFC040204
+#define STBC_PSWD_DATA1                       0x72682
+#define STBC_PSWD_DATA2                       0x78627
+#define STBC_KEYPASS                          0xFC040208
+#define STBC_I2C_SWITCH                       0xFC040220
+
+enum {
+	SUBADDR_DISABLE,
+	SUBADDR_8BITS,
+	SUBADDR_16BITS,
+	SUBADDR_24BITS,
+	SUBADDR_32BITS,
+	SUBADDR_40BITS,
+	SUBADDR_48BITS,
+	SUBADDR_56BITS,
+	SUBADDR_64BITS
+};
+
+struct nvt_i2c_compatible_data {
+	unsigned int sys_clock; // Unit: Hz
+	unsigned int stbc_clock; // Unit: Hz
+};
+
+struct nvt_i2c_bus {
+	void __iomem *base;
+	struct i2c_adapter adapter;
+	struct device *dev;
+	struct completion msg_complete;
+	struct i2c_msg *msg;
+	unsigned int bus_clk_rate;
+	bool stbc_i2c;
+	const struct nvt_i2c_compatible_data *comp_data;
+	int irq;
+	/* used for xfer */
+	struct i2c_msg *current_msg;
+	int remaining;
+	int write_ptr;
+	int read_ptr;
+	int fifo_idx;
+	int error_code;
+};
+
+static void nvt_i2c_use_case_feature(struct nvt_i2c_bus *i2c)
+{
+	void __iomem *reg_tmp;
+
+	if (i2c->stbc_i2c) {
+		reg_tmp = ioremap(STBC_PSWD, 4);
+		if (reg_tmp) {
+			writel(readl(reg_tmp) | STBC_PSWD_DATA1, reg_tmp);
+			writel(readl(reg_tmp) | STBC_PSWD_DATA2, reg_tmp);
+			iounmap(reg_tmp);
+		} else {
+			I2C_ERR_LOG("stbc pswd ioremap failed\n");
+		}
+
+		reg_tmp = ioremap(STBC_KEYPASS, 4);
+		if (reg_tmp) {
+			writel(readl(reg_tmp) | 0x1, reg_tmp);
+			iounmap(reg_tmp);
+		} else {
+			I2C_ERR_LOG("stbc keypass ioremap failed\n");
+		}
+	}
+}
+
+static void nvt_i2c_reset(struct nvt_i2c_bus *i2c)
+{
+	writel(readl(i2c->base + I2C_REG_CTRL) & ~I2C_ENABLE,
+	       i2c->base + I2C_REG_CTRL);
+	writel(readl(i2c->base + I2C_REG_CTRL) | I2C_ENABLE,
+	       i2c->base + I2C_REG_CTRL);
+}
+
+static void nvt_i2c_set_clk(struct nvt_i2c_bus *i2c)
+{
+	unsigned int duty = 0;
+	unsigned int clk_div = 0;
+	unsigned int source_speed = i2c->stbc_i2c ?
+		i2c->comp_data->stbc_clock : i2c->comp_data->sys_clock;
+
+	clk_div = source_speed / i2c->bus_clk_rate;
+	writel(clk_div << 1, i2c->base + I2C_REG_CLK);
+
+	duty = (clk_div * 9 + 10) / 20;
+	writel(duty << 16, i2c->base + I2C_REG_DUTY);
+
+	writel(I2C_ACK_CTRL_COUNTER, i2c->base + I2C_REG_ACK);
+}
+
+static void nvt_i2c_set_subaddr(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
+{
+	unsigned int reg_ctrl;
+	unsigned int subaddr = 0;
+	int i = 0;
+
+	reg_ctrl = readl(i2c->base + I2C_REG_CTRL);
+	reg_ctrl &= ~(I2C_16BITSUBADDR_ENABLE |
+		      I2C_24BITSUBADDR_ENABLE |
+		      I2C_32BITSUBADDR_ENABLE);
+
+	if (msg && msg->len > 0 && msg->len <= 4 && msg->buf) {
+		reg_ctrl |= I2C_SUBADDR_ENABLE;
+
+		switch (msg->len) {
+		case SUBADDR_8BITS:
+			break;
+		case SUBADDR_16BITS:
+			reg_ctrl |= I2C_16BITSUBADDR_ENABLE;
+			break;
+		case SUBADDR_24BITS:
+			reg_ctrl |= I2C_24BITSUBADDR_ENABLE;
+			break;
+		case SUBADDR_32BITS:
+			reg_ctrl |= I2C_32BITSUBADDR_ENABLE;
+			break;
+		default:
+			return;
+		}
+
+		for (i = 0; i < msg->len; i++)
+			subaddr |= msg->buf[i] << (8 * (msg->len - 1 - i));
+		writel(subaddr, i2c->base + I2C_REG_SUBADDR);
+	} else {
+		reg_ctrl &= ~I2C_SUBADDR_ENABLE;
+	}
+
+	writel(reg_ctrl, i2c->base + I2C_REG_CTRL);
+}
+
+static void nvt_i2c_init(struct nvt_i2c_bus *i2c)
+{
+	nvt_i2c_use_case_feature(i2c);
+	nvt_i2c_set_clk(i2c);
+	writel(I2C_BUF_LITTLE_ENDIAN, i2c->base + I2C_REG_PINGPONG);
+	writel(I2C_IRQ_ENABLE_SETTING, i2c->base + I2C_REG_INTR);
+}
+
+static int nvt_i2c_suspend(struct device *dev)
+{
+	struct nvt_i2c_bus *i2c = dev_get_drvdata(dev);
+
+	if (i2c) {
+		i2c_mark_adapter_suspended(&i2c->adapter);
+		i2c->current_msg = NULL;
+		writel(I2C_IRQ_DISABLE_SETTING, i2c->base + I2C_REG_INTR);
+	}
+
+	return 0;
+}
+
+static int nvt_i2c_resume(struct device *dev)
+{
+	struct nvt_i2c_bus *i2c = dev_get_drvdata(dev);
+
+	if (i2c) {
+		nvt_i2c_init(i2c);
+		i2c_mark_adapter_resumed(&i2c->adapter);
+	}
+
+	return 0;
+}
+
+static void nvt_i2c_clear_fifo(struct nvt_i2c_bus *i2c, unsigned int which)
+{
+	unsigned int regval = readl(i2c->base + I2C_REG_PINGPONG);
+
+	switch (which) {
+	case FIFO_1:
+		regval |= I2C_CLR_FIFO1;
+		break;
+	case FIFO_2:
+		regval |= I2C_CLR_FIFO2;
+		break;
+	case FIFO_ALL:
+		regval |= I2C_CLR_FIFO1 | I2C_CLR_FIFO2;
+		break;
+	default:
+		break;
+	}
+	writel(regval, i2c->base + I2C_REG_PINGPONG);
+}
+
+static void nvt_i2c_write_fifo(struct nvt_i2c_bus *i2c,
+			       unsigned int fifo_reg,
+			       const unsigned char *buf,
+			       unsigned int buf_offset,
+			       unsigned int length)
+{
+	unsigned int reg_idx = 0, copy_bytes = 0, j = 0, value = 0;
+
+	while (length > 0) {
+		value = 0;
+		copy_bytes = length >= FIFO_WORD_BYTES ? FIFO_WORD_BYTES : length;
+		for (j = 0; j < copy_bytes; j++)
+			value |= ((unsigned int)buf[buf_offset + j]) << (j * 8);
+
+		writel(value, i2c->base + fifo_reg + reg_idx * 4);
+		buf_offset += copy_bytes;
+		length -= copy_bytes;
+		reg_idx++;
+	}
+}
+
+static void nvt_i2c_read_fifo(struct nvt_i2c_bus *i2c,
+			      unsigned int fifo_reg,
+			      unsigned char *buf,
+			      unsigned int buf_offset,
+			      unsigned int length)
+{
+	unsigned int reg_idx = 0, copy_bytes = 0, j = 0, value = 0;
+
+	while (length > 0) {
+		value = readl(i2c->base + fifo_reg + reg_idx * 4);
+		copy_bytes = length >= FIFO_WORD_BYTES ? FIFO_WORD_BYTES : length;
+		for (j = 0; j < copy_bytes; j++)
+			buf[buf_offset + j] = (unsigned char)(value >> (j * 8));
+
+		buf_offset += copy_bytes;
+		length -= copy_bytes;
+		reg_idx++;
+	}
+}
+
+static void nvt_i2c_handle(struct nvt_i2c_bus *i2c, struct i2c_msg *msg, int is_read)
+{
+	unsigned int bytes, fiforeg;
+
+	if (!i2c || !msg || !msg->buf || msg->len == 0 || msg->len > 4096) {
+		I2C_ERR_LOG("I2C invalid msg: i2c=%p, msg=%p, buf=%p, len=%d\n",
+			    i2c, msg, msg ? msg->buf : NULL, msg ? msg->len : 0);
+		if (i2c) {
+			I2C_ERR_LOG("[%s.%d]: i2c_handle\n", dev_name(i2c->dev), i2c->adapter.nr);
+			i2c->error_code = -EINVAL;
+		}
+
+		return;
+	}
+
+	bytes = i2c->remaining > FIFO_CHUNK_SIZE ? FIFO_CHUNK_SIZE : i2c->remaining;
+	fiforeg = i2c->fifo_idx == 0 ? I2C_REG_FIFO1 : I2C_REG_FIFO2;
+
+	if (is_read) {
+		nvt_i2c_read_fifo(i2c, fiforeg, msg->buf, i2c->read_ptr, bytes);
+		i2c->read_ptr += bytes;
+	} else {
+		nvt_i2c_write_fifo(i2c, fiforeg, msg->buf, i2c->write_ptr, bytes);
+		i2c->write_ptr += bytes;
+	}
+	nvt_i2c_clear_fifo(i2c, i2c->fifo_idx);
+	i2c->remaining -= bytes;
+	i2c->fifo_idx ^= 1;
+}
+
+static irqreturn_t nvt_i2c_isr(int irq, void *dev_id)
+{
+	struct nvt_i2c_bus *i2c = dev_id;
+	struct i2c_msg *msg = i2c->current_msg;
+	unsigned int status = readl(i2c->base + I2C_REG_INTR);
+	unsigned int clr = 0;
+	int do_complete = 0;
+
+	if (!(status & I2C_IRQ_FLAG) || !i2c->current_msg)
+		return IRQ_NONE;
+
+	if (status & I2C_IRQ_NACK) {
+		i2c->error_code = -ENXIO;
+		clr |= I2C_IRQ_NACK << 8;
+	} else if (status & I2C_IRQ_RX_FULL) {
+		if (i2c->remaining > 0)
+			nvt_i2c_handle(i2c, msg, 1);
+		clr |= I2C_IRQ_RX_FULL << 8;
+	} else if (status & I2C_IRQ_TX_EMPTY) {
+		if (i2c->remaining > 0)
+			nvt_i2c_handle(i2c, msg, 0);
+		clr |= I2C_IRQ_TX_EMPTY << 8;
+	} else if (status & I2C_IRQ_FINISH) {
+		if (i2c->remaining > 0 && (msg->flags & I2C_M_RD))
+			nvt_i2c_handle(i2c, msg, 1);
+		clr |= I2C_IRQ_FINISH << 8;
+		do_complete = 1;
+	}
+	if (i2c->error_code)
+		do_complete = 1;
+
+	writel(status | clr, i2c->base + I2C_REG_INTR);
+	if (do_complete)
+		complete(&i2c->msg_complete);
+
+	return IRQ_HANDLED;
+}
+
+static void nvt_i2c_ctrl_init(struct nvt_i2c_bus *i2c)
+{
+	int i = 0;
+
+	writel(0, i2c->base + I2C_REG_CTRL);
+	for (i = 0; i < 4; i++) {
+		writel(0, i2c->base + I2C_REG_FIFO1 + i * 4);
+		writel(0, i2c->base + I2C_REG_FIFO2 + i * 4);
+	}
+	nvt_i2c_clear_fifo(i2c, FIFO_ALL);
+	reinit_completion(&i2c->msg_complete);
+}
+
+static int nvt_i2c_check_msg(const struct i2c_msg *msg)
+{
+	if (!msg || !msg->buf || msg->len == 0 || msg->len > 4096)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void nvt_i2c_prepare_xfer(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
+{
+	i2c->remaining   = msg->len;
+	i2c->current_msg = msg;
+	i2c->write_ptr   = 0;
+	i2c->read_ptr    = 0;
+	i2c->error_code  = 0;
+	i2c->fifo_idx    = 0;
+}
+
+static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
+{
+	const unsigned char *buf = msg->buf;
+	int ret, offset = 0, write_bytes, fifo_num;
+	unsigned int ctrl_mask;
+
+	ret = nvt_i2c_check_msg(msg);
+	if (ret)
+		return ret;
+
+	nvt_i2c_prepare_xfer(i2c, msg);
+
+	writel((msg->len * 8) << 8, i2c->base + I2C_REG_SIZE);
+
+	/*  Write FIFO data first */
+	for (fifo_num = 0; fifo_num < FIFO_ALL && offset < msg->len; fifo_num++) {
+		write_bytes = msg->len - offset > FIFO_CHUNK_SIZE ?
+				FIFO_CHUNK_SIZE : msg->len - offset;
+		nvt_i2c_write_fifo(i2c, fifo_num == 0 ? I2C_REG_FIFO1 : I2C_REG_FIFO2,
+				   buf, offset, write_bytes);
+		offset += write_bytes;
+	}
+	i2c->write_ptr = offset;
+	i2c->remaining = msg->len - offset;
+	i2c->fifo_idx = 0;
+
+	ctrl_mask = readl(i2c->base + I2C_REG_CTRL);
+	ctrl_mask |= (((msg->addr << 1) << 8) | I2C_ENABLE |
+			I2C_CLOCK_DUTY_ENABLE | I2C_CLOCK_STRETCH_ENABLE |
+			I2C_MASTER_CLK_STRETCH_ENABLE | I2C_TRIGGER);
+	writel(ctrl_mask, i2c->base + I2C_REG_CTRL);
+
+	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
+	if (ret == 0) {
+		i2c->error_code = -ETIMEDOUT;
+		nvt_i2c_reset(i2c);
+	}
+	if (i2c->error_code)
+		I2C_ERR_LOG("[%s.%d]: write failed (err:%d);"
+			    " SA[0x%X]\n",
+			    dev_name(i2c->dev), i2c->adapter.nr, i2c->error_code,
+			    msg->addr);
+
+	i2c->current_msg = NULL;
+
+	return i2c->error_code;
+}
+
+static int nvt_i2c_read(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
+{
+	unsigned int ctrl_mask;
+	int ret;
+
+	ret = nvt_i2c_check_msg(msg);
+	if (ret)
+		return ret;
+
+	nvt_i2c_prepare_xfer(i2c, msg);
+
+	writel((msg->len * 8) << 8, i2c->base + I2C_REG_SIZE);
+
+	ctrl_mask = readl(i2c->base + I2C_REG_CTRL);
+	ctrl_mask |= (((msg->addr << 1) << 8) | I2C_ENABLE |
+			I2C_REPEAT_ENABLE | I2C_READ_OPERATION |
+			I2C_CLOCK_DUTY_ENABLE | I2C_CLOCK_STRETCH_ENABLE |
+			I2C_MASTER_CLK_STRETCH_ENABLE | I2C_TRIGGER);
+	writel(ctrl_mask, i2c->base + I2C_REG_CTRL);
+
+	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
+	if (ret == 0) {
+		i2c->error_code = -ETIMEDOUT;
+		nvt_i2c_reset(i2c);
+	}
+	if (i2c->error_code)
+		I2C_ERR_LOG("[%s.%d]: read failed (err:%d);"
+			    " SA[0x%X]\n",
+			    dev_name(i2c->dev), i2c->adapter.nr, i2c->error_code,
+			    msg->addr);
+
+	i2c->current_msg = NULL;
+
+	return i2c->error_code;
+}
+
+static int nvt_i2c_xfer(struct i2c_adapter *adap,
+			struct i2c_msg msgs[],
+			int num)
+{
+	struct nvt_i2c_bus *i2c = i2c_get_adapdata(adap);
+	int ret = 0, i = 0;
+	struct i2c_msg *msg = NULL;
+
+	nvt_i2c_ctrl_init(i2c);
+
+	if (num == 2) {
+		nvt_i2c_set_subaddr(i2c, &msgs[0]);
+		msg = &msgs[1];
+
+		if (msg->flags & I2C_M_RD)
+			ret = nvt_i2c_read(i2c, msg);
+		else
+			ret = nvt_i2c_write(i2c, msg);
+	} else {
+		nvt_i2c_set_subaddr(i2c, NULL);
+
+		for (i = 0; i < num; i++) {
+			msg = &msgs[i];
+			if (msg->flags & I2C_M_RD)
+				ret = nvt_i2c_read(i2c, msg);
+			else
+				ret = nvt_i2c_write(i2c, msg);
+			if (ret < 0)
+				break;
+		}
+	}
+
+	if (ret < 0)
+		return ret;
+	return num;
+}
+
+static u32 nvt_i2c_func(struct i2c_adapter *adap)
+{
+	return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL) & ~I2C_FUNC_SMBUS_QUICK;
+}
+
+static int nvt_i2c_get_hwmods(const char *hwmods_name)
+{
+	long val;
+
+	if (!hwmods_name || strncmp(hwmods_name, "i2c", 3) != 0)
+		return -EINVAL;
+
+	if (kstrtol(hwmods_name + 3, 10, &val) == 0 && val >= 0)
+		return (int)val;
+
+	return -ENODEV;
+}
+
+static struct i2c_algorithm nvt_i2c_algo = {
+	.master_xfer = nvt_i2c_xfer,
+	.functionality = nvt_i2c_func,
+};
+
+static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
+{
+	int ret;
+	struct device *dev = i2c->dev;
+	struct device_node *np = dev->of_node;
+	const char *hwmods_val = NULL;
+
+	/* read DTS(novatek,hwmods) and set bus number */
+	ret = of_property_read_string(np, "novatek,hwmods", &hwmods_val);
+	if (ret == 0) {
+		i2c->adapter.nr = nvt_i2c_get_hwmods(hwmods_val);
+		if (i2c->adapter.nr >= 0) {
+			I2C_INFO_LOG("Get novatek,hwmods:i2c%d\n", i2c->adapter.nr);
+		} else {
+			I2C_ERR_LOG("Invalid novatek,hwmods value = %d\n", i2c->adapter.nr);
+			return -EINVAL;
+		}
+	} else {
+		I2C_ERR_LOG("Can't get novatek,hwmods\n");
+		return ret;
+	}
+
+	i2c->comp_data = of_device_get_match_data(dev);
+
+	/* read DTS(novatek,stbc-controllable) */
+	i2c->stbc_i2c = of_property_read_bool(np, "novatek,stbc-controllable");
+
+	/* read DTS(clock-frequency) */
+	ret = of_property_read_u32(np, "clock-frequency", &i2c->bus_clk_rate);
+	if (ret || !i2c->bus_clk_rate) {
+		I2C_INFO_LOG("Not set dtb clock-frequency, set default 100kHz\n");
+		i2c->bus_clk_rate = 100000;
+	}
+
+	return 0;
+}
+
+static const struct nvt_i2c_compatible_data nt726xx_data = {
+	.sys_clock = 96000000,
+	.stbc_clock = 12000000,
+};
+
+static const struct of_device_id nvt_i2c_of_match[] = {
+	{ .compatible = "novatek,nt72600-i2c", .data = &nt726xx_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, nvt_i2c_of_match);
+
+static int nvt_i2c_probe(struct platform_device *pdev)
+{
+	struct nvt_i2c_bus *i2c;
+	struct resource *res;
+	int ret, irq;
+
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	i2c->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(i2c->base)) {
+		I2C_ERR_LOG("failed to map controller\n");
+		return PTR_ERR(i2c->base);
+	}
+
+	init_completion(&i2c->msg_complete);
+	i2c->dev = &pdev->dev;
+
+	ret = nvt_i2c_parse_dts(i2c);
+	if (ret)
+		return ret;
+
+	nvt_i2c_init(i2c);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		I2C_ERR_LOG("No IRQ resource\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, nvt_i2c_isr,
+			       IRQF_SHARED | IRQF_TRIGGER_HIGH, I2C_NAME, i2c);
+	if (ret) {
+		I2C_ERR_LOG("devm_request_irq fail\n");
+		return ret;
+	}
+
+	/* Setup I2C adapter */
+	i2c->adapter.owner = THIS_MODULE;
+	i2c->adapter.class = I2C_CLASS_HWMON;
+	i2c->adapter.algo = &nvt_i2c_algo;
+	i2c->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
+	i2c->adapter.dev.parent = &pdev->dev;
+	i2c->adapter.timeout = 3 * HZ;
+	strscpy(i2c->adapter.name, I2C_NAME, sizeof(i2c->adapter.name));
+	i2c_set_adapdata(&i2c->adapter, i2c);
+
+	ret = i2c_add_numbered_adapter(&i2c->adapter);
+	if (ret) {
+		I2C_ERR_LOG("[%s] failed to add adapter\n", dev_name(&pdev->dev));
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, i2c);
+
+	return 0;
+}
+
+static void nvt_i2c_remove(struct platform_device *pdev)
+{
+	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
+
+	writel(I2C_IRQ_DISABLE_SETTING, i2c->base + I2C_REG_INTR);
+	writel(readl(i2c->base + I2C_REG_CTRL) & ~I2C_ENABLE,
+	       i2c->base + I2C_REG_CTRL);
+	of_node_put(i2c->adapter.dev.of_node);
+	i2c_del_adapter(&i2c->adapter);
+}
+
+static const struct dev_pm_ops nvt_i2c_pm_ops = {
+	.resume_early = nvt_i2c_resume,
+	.suspend_late = nvt_i2c_suspend,
+};
+
+static struct platform_driver nvt_i2c_driver = {
+	.probe = nvt_i2c_probe,
+	.remove = nvt_i2c_remove,
+	.driver = {
+		.name = "nvt_nt726xx_i2c",
+		.owner = THIS_MODULE,
+		.pm = &nvt_i2c_pm_ops,
+		.of_match_table = of_match_ptr(nvt_i2c_of_match),
+	},
+};
+
+static int __init nvt_i2c_platform_init(void)
+{
+	return platform_driver_register(&nvt_i2c_driver);
+}
+postcore_initcall(nvt_i2c_platform_init);
+
+static void __exit nvt_i2c_platform_exit(void)
+{
+	platform_driver_unregister(&nvt_i2c_driver);
+}
+module_exit(nvt_i2c_platform_exit);
+
+MODULE_DESCRIPTION("Novatek NT726xx SoC I2C Bus Driver");
+MODULE_AUTHOR("Ben Huang <ben_huang@novatek.com.tw>");
+MODULE_LICENSE("GPL");
-- 
2.40.1

Re: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] DT Binding Anti-Pattern: Usage of custom `novatek,hwmods` string property to assign unique instance names.
- [Low] Schema Type Violation: The boolean property `novatek,stbc-controllable` incorrectly specifies an integer default value of `0`.
- [Low] DT Schema Validation Failure: I2C controller examples use invalid `#address-cells` and `#size-cells` values.
--

--- Patch [2]: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller ---
commit fc4c1ba789209e1e7e8f1fe88e1d6a2a1a7eec66
Author: Ben Huang <Ben_Huang@novatek.com.tw>

dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller
    
Add device tree documentation for Novatek NT726xx SoC I2C controller.

> diff --git a/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> new file mode 100644
> index 0000000000000..866589c5ae51a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
[ ... ]
> +  novatek,hwmods:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Name of each i2c pin, must be named with "i2cX". (X is
> +                 an integer starting from 0, must be unique)

[Severity: Low]
Does this violate standard Device Tree schema practices? 

Hardware nodes should typically not encode software logical indices. Instances
are usually mapped dynamically via the /aliases node (e.g., i2c0 = &i2c0;) or
derived from the reg base address rather than using a custom string property
like novatek,hwmods.

> +
> +  novatek,stbc-controllable:
> +    type: boolean
> +    description: Set if this i2c master, named as `stbc-i2c`, is
> +      controllable by stbc (Standby controller, actually is an 8051
> +      micro-processor) on Novatek NT726xx SoCs.
> +      For these `stbc-i2c`s, the driven clock is only 12 MHz and the
> +      authentication to stbc must be executed before controlling the
> +      registers.
> +
> +    default: 0

[Severity: Low]
Is this default value valid for a boolean property? 

Boolean properties in DT schema are evaluated by their presence or absence
and shouldn't accept an integer value. This will likely fail schema validation
during dt_binding_check. If a default is needed, it should be false.

[ ... ]
> +examples:
> +  - |
> +    i2c0: i2c@10000000 {
> +      compatible = "novatek,nt72600-i2c";
> +      reg = <0x0 0x10000000 0x0 0x100>;
> +      interrupts = <0 67 4>;
> +      clock-frequency = <400000>;
> +      novatek,hwmods = "i2c0";
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +    };
> +
> +    stbc-i2c1: i2c@20000000 {
> +      compatible = "novatek,nt72600-i2c";
> +      reg = <0x0 0x20000000 0x0 0x100>;
> +      interrupts = <0 77 4>;
> +      novatek,hwmods = "i2c1";
> +      novatek,stbc-controllable;
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +    };

[Severity: Low]
Do these cell counts pass core device tree schema validation for I2C controllers?

I2C controllers dictate the cell sizes for their child devices and normally
enforce #address-cells = <1> (for the I2C address) and #size-cells = <0> (since
child devices are not memory mapped). Both examples seem to be using <2>.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715073521.147610-1-Nina_Kuo@novatek.com.tw?part=2

Re: [PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] The driver bypasses Device Tree abstractions by mapping hardcoded physical MMIO addresses, and triggers a sleep-in-atomic panic during system resume.
- [High] The master transfer function improperly intercepts 2-message sequences, silently dropping the first message and potentially leaking uninitialized kernel memory to hardware.
- [High] The interrupt handler returns IRQ_NONE without clearing asserted IRQ flags on unhandled events, leading to a hard lockup.
- [High] A race condition between the transfer timeout path and the ISR leads to a use-after-free of the I2C message buffer.
- [High] Hardware interrupts are enabled before the driver state is initialized or the adapter is registered.
- [High] Hardware and interrupts are disabled before unregistering the I2C adapter in `remove`, forcefully stalling ongoing transfers.
- [High] `I2C_REG_CTRL` accumulates target addresses and operation flags across multiple messages without clearing previous states.
- [Medium] Device tree node reference leak in the probe error path.
- [Medium] The driver uses a custom string property 'novatek,hwmods' to assign the I2C adapter number instead of standard DT aliases.
- [Low] PM callbacks and `dev_pm_ops` are unconditionally assigned, producing dead code when PM is disabled.
- [Low] Missing Kconfig OF dependency leads to unused variable warnings.
--

--- Patch [3]: [PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs ---
commit fffdbc67de89a90f8a40ad053da0475e89a835d6
Author: Ben Huang <Ben_Huang@novatek.com.tw>
Subject: i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

This patch introduces the I2C bus controller driver for Novatek NT726xx SoCs,
supporting standard and fast modes.

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index d354569942803..52ee2c82e9d5e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -962,6 +962,16 @@ config I2C_NPCM
>  	  controllers.
>  	  Driver can also support slave mode (select I2C_SLAVE).
>  
> +config I2C_NT726XX
> +	tristate "Novatek NT726xx Driver"
> +	default n

[Severity: Low]
Does this need a "depends on OF" directive? Without it, of_match_ptr() can
evaluate to NULL when CONFIG_OF is disabled, causing a compiler warning for
an unused nvt_i2c_of_match variable.

[ ... ]
> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..ac3fd7ccc68cb
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt726xx.c
[ ... ]
> +static void nvt_i2c_use_case_feature(struct nvt_i2c_bus *i2c)
> +{
> +	void __iomem *reg_tmp;
> +
> +	if (i2c->stbc_i2c) {
> +		reg_tmp = ioremap(STBC_PSWD, 4);

[Severity: High]
Is it safe to call ioremap() on a hardcoded physical address here? Since
this is called from nvt_i2c_resume() via the resume_early PM hook,
interrupts might be disabled. ioremap() can sleep, which could trigger a
scheduling-while-atomic panic during early resume.

Also, should this physical address be managed via standard device tree
resources instead?

[ ... ]
> +static irqreturn_t nvt_i2c_isr(int irq, void *dev_id)
> +{
> +	struct nvt_i2c_bus *i2c = dev_id;
> +	struct i2c_msg *msg = i2c->current_msg;
> +	unsigned int status = readl(i2c->base + I2C_REG_INTR);
> +	unsigned int clr = 0;
> +	int do_complete = 0;
> +
> +	if (!(status & I2C_IRQ_FLAG) || !i2c->current_msg)
> +		return IRQ_NONE;

[Severity: High]
If an interrupt fires while current_msg is NULL, this returns IRQ_NONE
without clearing the hardware interrupt status in I2C_REG_INTR. Since the
IRQ is requested as IRQF_TRIGGER_HIGH (level-triggered), won't the
uncleared interrupt keep the IRQ line asserted and lead to an interrupt
storm?

[ ... ]
> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	ctrl_mask = readl(i2c->base + I2C_REG_CTRL);
> +	ctrl_mask |= (((msg->addr << 1) << 8) | I2C_ENABLE |
> +			I2C_CLOCK_DUTY_ENABLE | I2C_CLOCK_STRETCH_ENABLE |
> +			I2C_MASTER_CLK_STRETCH_ENABLE | I2C_TRIGGER);
> +	writel(ctrl_mask, i2c->base + I2C_REG_CTRL);

[Severity: High]
Since this uses a bitwise OR to set the address and flags (like
I2C_READ_OPERATION) into ctrl_mask, and the previous state is not cleared,
won't subsequent messages bitwise OR their target address with the previous
message's address? Could this also cause a write message to be executed as
a read if the previous message was a read?

> +
> +	ret = wait_for_completion_timeout(&i2c->msg_complete, i2c->adapter.timeout);
> +	if (ret == 0) {
> +		i2c->error_code = -ETIMEDOUT;
> +		nvt_i2c_reset(i2c);
> +	}
> +	if (i2c->error_code)
> +		I2C_ERR_LOG("[%s.%d]: write failed (err:%d);"
> +			    " SA[0x%X]\n",
> +			    dev_name(i2c->dev), i2c->adapter.nr, i2c->error_code,
> +			    msg->addr);
> +
> +	i2c->current_msg = NULL;
> +
> +	return i2c->error_code;
> +}

[Severity: High]
When a transfer times out, this sets i2c->current_msg to NULL and returns,
which allows the caller to free the message buffer. What happens if the ISR
is running concurrently on another CPU and has already cached current_msg?

Could the ISR proceed to access the freed msg->buf, causing a
use-after-free?

[ ... ]
> +static int nvt_i2c_xfer(struct i2c_adapter *adap,
> +			struct i2c_msg msgs[],
> +			int num)
> +{
> +	struct nvt_i2c_bus *i2c = i2c_get_adapdata(adap);
> +	int ret = 0, i = 0;
> +	struct i2c_msg *msg = NULL;
> +
> +	nvt_i2c_ctrl_init(i2c);
> +
> +	if (num == 2) {
> +		nvt_i2c_set_subaddr(i2c, &msgs[0]);
> +		msg = &msgs[1];
> +
> +		if (msg->flags & I2C_M_RD)
> +			ret = nvt_i2c_read(i2c, msg);
> +		else
> +			ret = nvt_i2c_write(i2c, msg);

[Severity: High]
Is it intentional to unconditionally pass msgs[0] to nvt_i2c_set_subaddr()
and only execute the transfer for msgs[1]?

If msgs[0] is actually a read request, its buffer could be uninitialized
memory, which nvt_i2c_set_subaddr() might then read and write to the
hardware register, potentially leaking kernel memory. Additionally, wouldn't
this break the API contract by silently dropping the first message of a
valid 2-message sequence?

[ ... ]
> +static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
> +{
> +	int ret;
> +	struct device *dev = i2c->dev;
> +	struct device_node *np = dev->of_node;
> +	const char *hwmods_val = NULL;
> +
> +	/* read DTS(novatek,hwmods) and set bus number */
> +	ret = of_property_read_string(np, "novatek,hwmods", &hwmods_val);
> +	if (ret == 0) {
> +		i2c->adapter.nr = nvt_i2c_get_hwmods(hwmods_val);

[Severity: Medium]
Is there a specific reason to use a custom property like 'novatek,hwmods'
for assigning the I2C bus number? Typically, fixed indices for I2C
controllers are assigned using standard device tree aliases, like this:
aliases { i2c0 = &i2c_node; };

[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	nvt_i2c_init(i2c);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		I2C_ERR_LOG("No IRQ resource\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, nvt_i2c_isr,

[Severity: High]
The nvt_i2c_init() function enables hardware interrupts before the handler
is registered by devm_request_irq(). If the controller already has a
pending interrupt on boot, won't this trigger an immediate interrupt storm,
especially since the ISR returns IRQ_NONE when current_msg is NULL?

[ ... ]
> +	ret = i2c_add_numbered_adapter(&i2c->adapter);
> +	if (ret) {
> +		I2C_ERR_LOG("[%s] failed to add adapter\n", dev_name(&pdev->dev));
> +		return ret;
> +	}

[Severity: Medium]
If i2c_add_numbered_adapter() fails, does this error path need to call
of_node_put() to release the reference to i2c->adapter.dev.of_node
acquired earlier via of_node_get()?

[ ... ]
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	writel(I2C_IRQ_DISABLE_SETTING, i2c->base + I2C_REG_INTR);
> +	writel(readl(i2c->base + I2C_REG_CTRL) & ~I2C_ENABLE,
> +	       i2c->base + I2C_REG_CTRL);
> +	of_node_put(i2c->adapter.dev.of_node);
> +	i2c_del_adapter(&i2c->adapter);
> +}

[Severity: High]
By disabling the I2C controller and its interrupts before calling
i2c_del_adapter(), couldn't we interrupt active I2C operations?

If a transfer is ongoing concurrently, disabling the hardware out from under
it will cause it to stall and time out. Should i2c_del_adapter() be called
first to cleanly block new transfers and wait for active ones to finish?

> +
> +static const struct dev_pm_ops nvt_i2c_pm_ops = {
> +	.resume_early = nvt_i2c_resume,
> +	.suspend_late = nvt_i2c_suspend,
> +};

[Severity: Low]
Should these callbacks be wrapped with pm_sleep_ptr()? When CONFIG_PM_SLEEP
is disabled, these functions and the struct assignment below will become
dead code and might trigger compiler warnings.

> +
> +static struct platform_driver nvt_i2c_driver = {
> +	.probe = nvt_i2c_probe,
> +	.remove = nvt_i2c_remove,
> +	.driver = {
> +		.name = "nvt_nt726xx_i2c",
> +		.owner = THIS_MODULE,
> +		.pm = &nvt_i2c_pm_ops,

[Severity: Low]
Can we wrap the pm_ops pointer with pm_ptr() to ensure it compiles away cleanly
when CONFIG_PM is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715073521.147610-1-Nina_Kuo@novatek.com.tw?part=3

Re: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller

[Krzysztof Kozlowski]

On Wed, Jul 15, 2026 at 03:35:21PM +0800, Nina_Kuo@novatek.com.tw wrote:
> From: Ben Huang <Ben_Huang@novatek.com.tw>
> 
> Add device tree documentation for Novatek NT726xx SoC I2C controller.
> 
> Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
> Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
> ---
>  .../bindings/i2c/novatek,nt726xx-i2c.yaml     | 74 +++++++++++++++++++
>  1 file changed, 74 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> new file mode 100644
> index 000000000000..866589c5ae51
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml

Filename must match compatible.

> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/novatek,nt726xx-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +maintainers:
> +  - Ben Huang <ben_huang@novatek.com.tw>
> +  - Jason JJ Wu <jason_jj_wu@novatek.com.tw>

Why are you changing the order of entries in the schema file? Do not
come with own style. Please read and follow writing-bindings and
example-schema.

> +
> +title: Novatek NT726xx Series SoC I2C master controller

So 726xx or 72600? Confusing.

> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: novatek,nt72600-i2c
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clock-frequency:
> +    default: 100000
> +    enum: [ 100000, 400000 ]
> +
> +  novatek,hwmods:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Name of each i2c pin, must be named with "i2cX". (X is
> +                 an integer starting from 0, must be unique)

I do not understand the purpose of this property. How in one string can
you name each pin? And why would even need to name each pin - why would
that be a board setup? Pins have FIXED names.

Drop property.

> +
> +  novatek,stbc-controllable:
> +    type: boolean
> +    description: Set if this i2c master, named as `stbc-i2c`, is
> +      controllable by stbc (Standby controller, actually is an 8051
> +      micro-processor) on Novatek NT726xx SoCs.
> +      For these `stbc-i2c`s, the driven clock is only 12 MHz and the
> +      authentication to stbc must be executed before controlling the
> +      registers.

Compatible implies that, drop.

> +
> +    default: 0

Don't vibe code stuff.



> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - novatek,hwmods
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    i2c0: i2c@10000000 {

Drop label

> +      compatible = "novatek,nt72600-i2c";
> +      reg = <0x0 0x10000000 0x0 0x100>;
> +      interrupts = <0 67 4>;

Use proper defines.

> +      clock-frequency = <400000>;
> +      novatek,hwmods = "i2c0";
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +    };
> +
> +    stbc-i2c1: i2c@20000000 {

Drop example, almost the same, so pretty pointless.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[Krzysztof Kozlowski]

On Wed, Jul 15, 2026 at 03:35:03PM +0800, Nina_Kuo@novatek.com.tw wrote:
> From: Ben Huang <Ben_Huang@novatek.com.tw>
> 
> Add entry for maintenance of Novatek NT726xx SoC I2C controller driver.
> 
> Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
> Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 806bd2d80d15..15df47f9e81f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19234,6 +19234,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc.git
>  F:	tools/include/nolibc/
>  F:	tools/testing/selftests/nolibc/
>  
> +NOVATEK NT726XX I2C CONTROLLER DRIVER
> +M:	Ben Huang <ben_huang@novatek.com.tw>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml

There is no such file.

> +F:	drivers/i2c/busses/i2c-nt726xx.c

Neither this one.

Apply this patch (important: this patch!) and check yourself.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

[Krzysztof Kozlowski]

On Wed, Jul 15, 2026 at 03:35:34PM +0800, Nina_Kuo@novatek.com.tw wrote:
> From: Ben Huang <Ben_Huang@novatek.com.tw>
> 
> This patch introduce the support of I2C bus controller driver of
> Novatek NT726xx SoCs.
> 
> This driver performs the fundamental read/write functions as
> an I2C controller and supports Standard-mode and Fast-mode.
> Default operation is Stardard-mode.
> 
> Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
> Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
> ---
>  drivers/i2c/busses/Kconfig       |  10 +
>  drivers/i2c/busses/Makefile      |   1 +
>  drivers/i2c/busses/i2c-nt726xx.c | 698 +++++++++++++++++++++++++++++++
>  3 files changed, 709 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-nt726xx.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index d35456994280..52ee2c82e9d5 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -962,6 +962,16 @@ config I2C_NPCM
>  	  controllers.
>  	  Driver can also support slave mode (select I2C_SLAVE).
>  
> +config I2C_NT726XX
> +	tristate "Novatek NT726xx Driver"
> +	default n
> +	help
> +	  Say Y here if you want to enable I2C bus controller on
> +	  Novatek NT726xx SoCs.
> +	  This driver performs fundamental read/write functions
> +	  as an I2C bus controller and supports Standard-mode and
> +	  Fast-mode. Default operation is Standard-mode.
> +
>  config I2C_OCORES
>  	tristate "OpenCores I2C Controller"
>  	help
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 3755c54b3d82..e82646919916 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
>  obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
>  obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
>  obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
> +obj-$(CONFIG_I2C_NT726XX)	+= i2c-nt726xx.o
>  obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
>  obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
>  obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 000000000000..ac3fd7ccc68c
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nt726xx.c
> @@ -0,0 +1,698 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Novatek Microelectronics Corp.
> + * Author: Ben Huang <ben_huang@novatek.com.tw>
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/version.h>
> +
> +#define I2C_INFO_LOG(fmt, ...)		\
> +	pr_info("[I/SOC_I2C] " fmt, ##__VA_ARGS__)
> +
> +#define I2C_ERR_LOG(fmt, ...)		\
> +	pr_err("[E/SOC_I2C] " fmt, ##__VA_ARGS__)

No, you do not get own printing stuff.

This is not Linux coding style. Learn how Linux treats your own new
abstraction layers.

...

> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c;
> +	struct resource *res;
> +	int ret, irq;
> +
> +	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	i2c->base = devm_ioremap_resource(&pdev->dev, res);

Look at other drivers

> +	if (IS_ERR(i2c->base)) {
> +		I2C_ERR_LOG("failed to map controller\n");
> +		return PTR_ERR(i2c->base);

Look at other drivers how they do it

> +	}
> +
> +	init_completion(&i2c->msg_complete);
> +	i2c->dev = &pdev->dev;
> +
> +	ret = nvt_i2c_parse_dts(i2c);
> +	if (ret)
> +		return ret;
> +
> +	nvt_i2c_init(i2c);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		I2C_ERR_LOG("No IRQ resource\n");

Look at other drivers how they do it

> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, nvt_i2c_isr,
> +			       IRQF_SHARED | IRQF_TRIGGER_HIGH, I2C_NAME, i2c);
> +	if (ret) {
> +		I2C_ERR_LOG("devm_request_irq fail\n");

Look at other drivers how they do it

> +		return ret;
> +	}
> +
> +	/* Setup I2C adapter */
> +	i2c->adapter.owner = THIS_MODULE;
> +	i2c->adapter.class = I2C_CLASS_HWMON;
> +	i2c->adapter.algo = &nvt_i2c_algo;
> +	i2c->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> +	i2c->adapter.dev.parent = &pdev->dev;
> +	i2c->adapter.timeout = 3 * HZ;
> +	strscpy(i2c->adapter.name, I2C_NAME, sizeof(i2c->adapter.name));
> +	i2c_set_adapdata(&i2c->adapter, i2c);
> +
> +	ret = i2c_add_numbered_adapter(&i2c->adapter);
> +	if (ret) {
> +		I2C_ERR_LOG("[%s] failed to add adapter\n", dev_name(&pdev->dev));
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, i2c);
> +
> +	return 0;
> +}
> +
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	writel(I2C_IRQ_DISABLE_SETTING, i2c->base + I2C_REG_INTR);
> +	writel(readl(i2c->base + I2C_REG_CTRL) & ~I2C_ENABLE,
> +	       i2c->base + I2C_REG_CTRL);
> +	of_node_put(i2c->adapter.dev.of_node);
> +	i2c_del_adapter(&i2c->adapter);
> +}
> +
> +static const struct dev_pm_ops nvt_i2c_pm_ops = {
> +	.resume_early = nvt_i2c_resume,
> +	.suspend_late = nvt_i2c_suspend,
> +};
> +
> +static struct platform_driver nvt_i2c_driver = {
> +	.probe = nvt_i2c_probe,
> +	.remove = nvt_i2c_remove,
> +	.driver = {
> +		.name = "nvt_nt726xx_i2c",
> +		.owner = THIS_MODULE,

NAK, you just sent us 13 year old downstream junk code. This is not
acceptable, because by doing that you ignore all the bugs or issues we
fixed over last 13 years.


> +		.pm = &nvt_i2c_pm_ops,
> +		.of_match_table = of_match_ptr(nvt_i2c_of_match),
> +	},
> +};
> +
> +static int __init nvt_i2c_platform_init(void)
> +{
> +	return platform_driver_register(&nvt_i2c_driver);
> +}
> +postcore_initcall(nvt_i2c_platform_init);

NAK, this is not postcore!

You have standard module. Full stop.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[Krzysztof Kozlowski]

On 21/07/2026 09:11, Krzysztof Kozlowski wrote:
> On Wed, Jul 15, 2026 at 03:35:03PM +0800, Nina_Kuo@novatek.com.tw wrote:
>> From: Ben Huang <Ben_Huang@novatek.com.tw>
>>
>> Add entry for maintenance of Novatek NT726xx SoC I2C controller driver.
>>
>> Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
>> Signed-off-by: Nina Kuo <Nina_Kuo@novatek.com.tw>
>> ---
>>  MAINTAINERS | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 806bd2d80d15..15df47f9e81f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19234,6 +19234,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/nolibc/linux-nolibc.git
>>  F:	tools/include/nolibc/
>>  F:	tools/testing/selftests/nolibc/
>>  
>> +NOVATEK NT726XX I2C CONTROLLER DRIVER
>> +M:	Ben Huang <ben_huang@novatek.com.tw>
>> +L:	linux-i2c@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> 
> There is no such file.
> 
>> +F:	drivers/i2c/busses/i2c-nt726xx.c
> 
> Neither this one.
> 
> Apply this patch (important: this patch!) and check yourself.

And I already told you that here:

https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#m1589bef5c6da621ba79f46fc87c0d39844f17534

which you just ignored.

Please respond - how did you fix that issue? If you keep ignoring
comments, why shall we keep reviewing this patch?

Best regards,
Krzysztof

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[Nina_Kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

Hi,

Thanks for your reviewing.

Sorry about the same mistake. This is the first time for
submitting kernel patches.
To fix this problem, shall I put all modifications into 1 patch
file? Please correct me if I'm wrong.

In the patch series, MAINTAINERS is in the first patch (1/3),
related .yaml (2/3) and .c (3/3) files are in the subsequent
patches.
If only MAINTAINTERS patch is applied, neither .yaml nor .c can
be found under kernel source codes. I think this is the question
you mentioned. Please correct me if I'm wrong.

Sincerely,
Ben Huang

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[Krzysztof Kozlowski]

On 21/07/2026 10:00, Nina_Kuo@novatek.com.tw wrote:
> From: Ben Huang <Ben_Huang@novatek.com.tw>
> 
> Hi,
> 
> Thanks for your reviewing.
> 
> Sorry about the same mistake. This is the first time for
> submitting kernel patches.
> To fix this problem, shall I put all modifications into 1 patch
> file? Please correct me if I'm wrong.
> 
> In the patch series, MAINTAINERS is in the first patch (1/3),
> related .yaml (2/3) and .c (3/3) files are in the subsequent
> patches.
> If only MAINTAINTERS patch is applied, neither .yaml nor .c can
> be found under kernel source codes. I think this is the question
> you mentioned. Please correct me if I'm wrong.

Please do not top post or remove context you are replying to. I have
like 200+ emails per day, so what is this about?

Please learn how Git works, how its history is organized, how commits
link to each other. This is nothing specific to submitting patches or
kernel development - this is basics of Git with combination of
bisectability of software development cycles.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[SP_ISW1_AT]

From: Ben Huang <Ben_Huang@novatek.com.tw>

On Tue, July 21, 2026 at 4:09 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:

> Please do not top post or remove context you are replying to. I have
> like 200+ emails per day, so what is this about?

Apologies for removing the contents of reply mails.
Quotation of replied contents would be added in further reply and patch
mails.

> Please learn how Git works, how its history is organized, how commits
> link to each other. This is nothing specific to submitting patches or
> kernel development - this is basics of Git with combination of
> bisectability of software development cycles.

The organization of these patch series will be adjusted for next
submission.

Sincerely,
Ben Huang

Re: [PATCH v3 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC I2C controller driver

[nina_kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

On Tue, July 21, 2026 at 4:09 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:

> Please do not top post or remove context you are replying to. I have
> like 200+ emails per day, so what is this about?

Apologies for removing the contents of reply mails.
Quotation of replied contents would be added in further reply and patch
mails.

> Please learn how Git works, how its history is organized, how commits
> link to each other. This is nothing specific to submitting patches or
> kernel development - this is basics of Git with combination of
> bisectability of software development cycles.

The organization of these patch series will be adjusted for next
submission.

Sincerely,
Ben Huang

Re: [PATCH v3 2/3] dt-bindings: i2c: Add Novatek NT726xx SoC I2C controller

[nina_kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

On Tue, July 21, 2026 at 3:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Wed, Jul 15, 2026 at 03:35:21PM +0800, Nina_Kuo@novatek.com.tw wrote:
>>  
>> diff --git a/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
>> new file mode 100644
>> index 000000000000..866589c5ae51
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> 
> Filename must match compatible.

Compatible shall be "novatek,nt726xx-i2c" for common i2c use of Novatek
NT726xx SoC platforms. Modify on next submission.

>
>> +
>> +maintainers:
>> +  - Ben Huang <ben_huang@novatek.com.tw>
>> +  - Jason JJ Wu <jason_jj_wu@novatek.com.tw>
>
> Why are you changing the order of entries in the schema file? Do not
> come with own style. Please read and follow writing-bindings and
> example-schema.

The order of title and maintainers will be corrected on next
submission.

>
>> +
>> +title: Novatek NT726xx Series SoC I2C master controller
>
> So 726xx or 72600? Confusing.

726xx is proper for common i2c use of Novatek NT726xx SoC platforms.

>
>> +  novatek,hwmods:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Name of each i2c pin, must be named with "i2cX". (X is
>> +                 an integer starting from 0, must be unique)
>
> I do not understand the purpose of this property. How in one string can
> you name each pin? And why would even need to name each pin - why would
> that be a board setup? Pins have FIXED names.
>
> Drop property.

This property is to determine the number of i2c adapter (nr).
I will evaluate any alternatives to this property.

>
>> +
>> +  novatek,stbc-controllable:
>> +    type: boolean
>> +    description: Set if this i2c master, named as `stbc-i2c`, is
>> +      controllable by stbc (Standby controller, actually is an 8051
>> +      micro-processor) on Novatek NT726xx SoCs.
>> +      For these `stbc-i2c`s, the driven clock is only 12 MHz and the
>> +      authentication to stbc must be executed before controlling the
>> +      registers.
>
> Compatible implies that, drop.

Additional compatible "novatek,nt726xx-i2c-stbc" will be applied for
i2c master controllable by the standby controller (stbc-i2c).

>
>> +
>> +    default: 0
>
> Don't vibe code stuff.

This will be removed as moving stbc-i2c to compatible.

>
>> +examples:
>> +  - |
>> +    i2c0: i2c@10000000 {
>
> Drop label

Label will be removed on next submission.

>
>> +      compatible = "novatek,nt72600-i2c";
>> +      reg = <0x0 0x10000000 0x0 0x100>;
>> +      interrupts = <0 67 4>;
>
> Use proper defines.

Below will be applied to examples on next submission:

#include <dt-bindings/interrupt-controller/arm-gic.h>

Interrupt type: 0 => GIC_SPI
Interrupt number: 67 (normal i2c), 77 (stbc-i2c)
Flags: 4 => IRQ_TYPE_LEVEL_HIGH

>
>> +      clock-frequency = <400000>;
>> +      novatek,hwmods = "i2c0";
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +    };
>> +
>> +    stbc-i2c1: i2c@20000000 {
>
> Drop example, almost the same, so pretty pointless.

This example will be modified for stbc-i2c instance on next submission.

Sincerely,
Ben Huang

Re: [PATCH v3 3/3] i2c: Add i2c-nt726xx.c I2C driver for Novatek NT726xx SoCs

[nina_kuo]

From: Ben Huang <Ben_Huang@novatek.com.tw>

On Tuesday, July 21, 2026 at 3:14 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Wed, Jul 15, 2026 at 03:35:34PM +0800, Nina_Kuo@novatek.com.tw wrote:
>> +#define I2C_INFO_LOG(fmt, ...)\
>> +pr_info("[I/SOC_I2C] " fmt, ##__VA_ARGS__)
>> +
>> +#define I2C_ERR_LOG(fmt, ...)\
>> +pr_err("[E/SOC_I2C] " fmt, ##__VA_ARGS__)
>
> No, you do not get own printing stuff.
>
> This is not Linux coding style. Learn how Linux treats your own new
> abstraction layers.
>
> ...
>
>> +static int nvt_i2c_probe(struct platform_device *pdev)
>> +{
>> +struct nvt_i2c_bus *i2c;
>> +struct resource *res;
>> +int ret, irq;
>> +
>> +i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
>> +if (!i2c)
>> +return -ENOMEM;
>> +
>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +i2c->base = devm_ioremap_resource(&pdev->dev, res);
>
> Look at other drivers
>
>> +if (IS_ERR(i2c->base)) {
>> +I2C_ERR_LOG("failed to map controller\n");
>> +return PTR_ERR(i2c->base);
>
> Look at other drivers how they do it
>
>> +}
>> +
>> +init_completion(&i2c->msg_complete);
>> +i2c->dev = &pdev->dev;
>> +
>> +ret = nvt_i2c_parse_dts(i2c);
>> +if (ret)
>> +return ret;
>> +
>> +nvt_i2c_init(i2c);
>> +
>> +irq = platform_get_irq(pdev, 0);
>> +if (irq < 0) {
>> +I2C_ERR_LOG("No IRQ resource\n");
>
> Look at other drivers how they do it
>
>> +return irq;
>> +}
>> +
>> +ret = devm_request_irq(&pdev->dev, irq, nvt_i2c_isr,
>> +       IRQF_SHARED | IRQF_TRIGGER_HIGH, I2C_NAME, i2c);
>> +if (ret) {
>> +I2C_ERR_LOG("devm_request_irq fail\n");
>
> Look at other drivers how they do it
>

Log functions will be changed to dev_err() for error and dev_info()
for info use.

>> +static struct platform_driver nvt_i2c_driver = {
>> +.probe = nvt_i2c_probe,
>> +.remove = nvt_i2c_remove,
>> +.driver = {
>> +.name = "nvt_nt726xx_i2c",
>> +.owner = THIS_MODULE,
>
> NAK, you just sent us 13 year old downstream junk code. This is not
> acceptable, because by doing that you ignore all the bugs or issues we
> fixed over last 13 years.

owner will be removed.

>> +.pm = &nvt_i2c_pm_ops,
>> +.of_match_table = of_match_ptr(nvt_i2c_of_match),
>> +},
>> +};
>> +
>> +static int __init nvt_i2c_platform_init(void)
>> +{
>> +return platform_driver_register(&nvt_i2c_driver);
>> +}
>> +postcore_initcall(nvt_i2c_platform_init);
>
> NAK, this is not postcore!
>
> You have standard module. Full stop.

This will be replaced by module_init().

Sincerely,
Ben Huang