Devicetree
 help / color / mirror / Atom feed
* [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs.
  2026-06-04  6:04 SP_ISW1_AT
@ 2026-06-04  6:04 ` SP_ISW1_AT
  2026-06-04  6:22   ` sashiko-bot
  2026-06-04  6:38   ` Wolfram Sang
  0 siblings, 2 replies; 9+ messages in thread
From: SP_ISW1_AT @ 2026-06-04  6:04 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, linux-i2c, devicetree,
	linux-kernel
  Cc: SP_ISW1_AT, ben_huang, toby_chui, shihpei_hsu

[-- Attachment #1: Type: text/html, Size: 1596 bytes --]

[-- Attachment #2: Type: text/plain, Size: 22224 bytes --]

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

This patch introduce the support of i2c bus controller 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>
---
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-nt726xx.c | 699 +++++++++++++++++++++++++++++++
 3 files changed, 710 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-nt726xx.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8c935f867a37..61daeac6b042 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -950,6 +950,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 547123ab351f..bfcd29203b35 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -89,6 +89,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..b0e79722a85b
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nt726xx.c
@@ -0,0 +1,699 @@
+// 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                              "NT72xxx 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_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_PASSWORD                         0xFC040204
+#define STBC_PASSWORD_DATA1                   0x72682
+#define STBC_PASSWORD_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_enable;
+	unsigned int bus_clk_rate;
+	unsigned int 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_PASSWORD, 4);
+		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
+		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);
+
+		reg_tmp = ioremap(STBC_KEYPASS, 4);
+		writel(readl(reg_tmp) | 0x1, reg_tmp);
+		iounmap(reg_tmp);
+	}
+}
+
+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) / 1000;
+	writel(clk_div << 1, i2c->base + I2C_REG_CLK);
+
+	duty = (clk_div * 9 + 10) / 20;
+	writel(duty << 16, i2c->base + I2C_REG_DUTY);
+
+	writel(0x1e0, 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;
+}
+
+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(nvt,hwmods) and set bus number */
+	ret = of_property_read_string(np, "nvt,hwmods", &hwmods_val);
+	if (ret == 0) {
+		i2c->adapter.nr = nvt_i2c_get_hwmods(hwmods_val);
+		if (i2c->adapter.nr >= 0) {
+			I2C_INFO_LOG("Get nvt,hwmods:i2c%d\n", i2c->adapter.nr);
+		} else {
+			I2C_ERR_LOG("Invalid nvt,hwmods value = %d\n", i2c->adapter.nr);
+			return -EINVAL;
+		}
+	} else {
+		I2C_ERR_LOG("Can't get nvt,hwmods\n");
+		return ret;
+	}
+
+	/* read DTS(bus-enable) */
+	ret = of_property_read_u32(np, "bus-enable", &i2c->bus_enable);
+	if (ret != 0)
+		I2C_INFO_LOG("Not set dtb bus-enable, set default disable\n");
+	else if (!i2c->bus_enable)
+		I2C_INFO_LOG("%s.%d is disabled, skipping initialization.\n",
+			     dev_name(i2c->dev), i2c->adapter.nr);
+	if (!i2c->bus_enable)
+		return 0;
+
+	i2c->comp_data = of_device_get_match_data(dev);
+
+	/* read DTS(stbc) */
+	ret = of_property_read_u32(np, "stbc", &i2c->stbc_i2c);
+	if (ret) {
+		I2C_INFO_LOG("Not set dtb stbc, set default false\n");
+		i2c->stbc_i2c = 0;
+	}
+
+	/* read DTS(clock-frequency) */
+	ret = of_property_read_u32(np, "clock-frequency", &i2c->bus_clk_rate);
+	if (ret) {
+		I2C_INFO_LOG("Not set dtb clock-frequency, set default 100kHz\n");
+		i2c->bus_clk_rate = 100;
+	}
+
+	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,nt726xx_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 || !i2c->bus_enable)
+		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);
+
+	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_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 NT72xxx SoC I2C Bus Driver");
+MODULE_AUTHOR("Ben Huang <ben_huang@novatek.com.tw>");
+MODULE_LICENSE("GPL");
-- 
2.40.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs.
  2026-06-04  6:04 ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
@ 2026-06-04  6:22   ` sashiko-bot
  2026-06-04  6:38   ` Wolfram Sang
  1 sibling, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-04  6:22 UTC (permalink / raw)
  To: SP_ISW1_AT; +Cc: conor+dt, devicetree, robh

Thank you for your contribution! Sashiko AI review found 16 potential issue(s) to consider:
- [Critical] Timeout handling in I2C read/write lacks synchronization, leaving a dangling pointer accessible by the interrupt handler.
- [Critical] Unbinding the driver causes a NULL pointer dereference if `bus-enable` is false or missing.
- [Critical] The `clock-frequency` DT property expects kHz and causes a divide-by-zero panic if correctly configured.
- [Critical] Control register fields are bitwise ORed without clearing previous state, allowing I2C target isolation bypass.
- [High] The `master_xfer` implementation unilaterally drops or misroutes the first message of any 2-message transfer.
- [High] Virtual memory mapping leaked during initialization due to missing `iounmap()`.
- [High] The driver advertises SMBus Quick support but explicitly rejects 0-length messages.
- [High] The ISR returns `IRQ_NONE` without clearing device interrupts if no transfer is active, causing an unhandled interrupt storm.
- [High] Device interrupts are not disabled during driver removal, risking a shared IRQ storm.
- [High] The driver interacts with hardware registers without ensuring clocks and power domains are enabled.
- [Medium] The driver hardcodes physical memory addresses to access system-level control registers.
- [Medium] The driver parses custom, non-standard Device Tree properties to handle core subsystem state.
- [Medium] Unconditional assignment of PM callbacks causes dead code when PM features are disabled.
- [Medium] The commit message contains a corporate confidentiality notice.
- [Medium] Device tree node reference acquired via `of_node_get()` is never released.
- [Medium] The driver uses `writel()`/`readl()` with manual byte-packing for FIFO access instead of stream accessors.
--

commit 57029ad0ef0636d7917520387b5868dcfc696f11
Author: SP_ISW1_AT@novatek.com.tw <SP_ISW1_AT@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 basic read/write functions in Standard and Fast modes.

> CONFIDENTIALITY NOTICE: This message, including attachments, contains information which may be confidential and privileged, and is intended only for use by the addressees designated above. Unless you are the intended recipient, any use, copying, disclosure, or distribution is prohibited. If you have received the message in error, please immediately delete the message and destroy all copies thereof and notify the sender by reply email. 本郵件及任何附件均屬機密,僅供其上指定地址之收件人使用。除非您是指定之收件人,否則請勿使用、複製、揭露或散布本郵件之任何部份。若您錯誤地收到此郵件,請立即回覆電子郵件通知寄件人,並請完全刪除且銷毀本郵件及其複本。

This isn't a regression, but the commit message contains a corporate
confidentiality notice.

> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..b0e79722a85b0
> --- /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_PASSWORD, 4);

[Severity: Medium]
Is it safe to hardcode physical memory addresses like STBC_PASSWORD to access
system-level control registers, rather than using standard Device Tree
bindings or resource management?

> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);
> +
> +		reg_tmp = ioremap(STBC_KEYPASS, 4);

[Severity: High]
Does this code leak the first virtual memory mapping? The pointer assigned
to reg_tmp from ioremap is overwritten here without a corresponding iounmap(),
creating a persistent leak.

[ ... ]
> +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) / 1000;

[Severity: Critical]
Does this assume the clock-frequency property is in kHz?

The device tree standard mandates it is specified in Hertz. If a standard
value like 400000 is provided, could this integer division result in 0,
locking up the hardware or causing a divide-by-zero kernel panic?

[ ... ]
> +static void nvt_i2c_set_subaddr(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +		for (i = 0; i < msg->len; i++)
> +			subaddr |= msg->buf[i] << (8 * (msg->len - 1 - i));

[Severity: High]
Could this read from uninitialized memory if the first message in a
transfer is a read command?

[ ... ]
> +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);

[Severity: Medium]
Would it be better to use stream accessors like writesl() instead of manually
byte-packing into a writel() call? The standard register accessors perform
implicit byte-swapping which may be unintended for FIFO data streams.

[ ... ]
> +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 is active but i2c->current_msg is NULL, could returning
IRQ_NONE without clearing the hardware interrupt flag cause an unhandled
interrupt storm on a shared, level-triggered line?

[ ... ]
> +	} else if (status & I2C_IRQ_TX_EMPTY) {
> +		if (i2c->remaining > 0)
> +			nvt_i2c_handle(i2c, msg, 0);

[Severity: Critical]
Can this result in a use-after-free regression if the wait_for_completion
expires?

If wait_for_completion_timeout() times out in nvt_i2c_write(), it sets
i2c->current_msg to NULL and returns, leaving the stack-allocated msg
to be freed. If an interrupt fires at that exact moment, the asynchronous
ISR might have already copied i2c->current_msg to the local msg variable
before it was cleared, leading to nvt_i2c_handle() accessing freed
stack memory.

[ ... ]
> +static int nvt_i2c_check_msg(const struct i2c_msg *msg)
> +{
> +	if (!msg || !msg->buf || msg->len == 0 || msg->len > 4096)
> +		return -EINVAL;

[Severity: High]
Will this reject 0-length messages explicitly? The adapter advertises
I2C_FUNC_SMBUS_EMUL later on, which implies support for SMBus Quick
commands that rely on 0-length transactions.

[ ... ]
> +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: Critical]
Does this bitwise-OR the target address without first clearing the existing
address bits in the control mask?

During a sequence of multiple messages in a single transfer, the previous
address might not be cleared, causing subsequent messages to transmit a
corrupted, logically-combined address on the bus. This could potentially
bypass I2C target isolation checks.

[ ... ]
> +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];

[Severity: High]
Does this unilaterally drop the first message of any 2-message transfer and
treat it as a hardware subaddress? This could break standard 2-message I2C
sequences, such as interacting with two separate targets or consecutive writes.

[ ... ]
> +static int nvt_i2c_parse_dts(struct nvt_i2c_bus *i2c)
> +{
[ ... ]
> +	/* read DTS(nvt,hwmods) and set bus number */
> +	ret = of_property_read_string(np, "nvt,hwmods", &hwmods_val);
[ ... ]
> +	/* read DTS(bus-enable) */
> +	ret = of_property_read_u32(np, "bus-enable", &i2c->bus_enable);

[Severity: Medium]
Is there a reason to parse custom properties like nvt,hwmods and bus-enable
instead of relying on standard I2C core bindings like aliases and the
standard status = "disabled" flag?

[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = nvt_i2c_parse_dts(i2c);
> +	if (ret || !i2c->bus_enable)
> +		return ret;

[Severity: Critical]
If bus-enable is false, this returns 0 (success) before calling
platform_set_drvdata(). Could this cause a NULL pointer dereference
later during unbind when nvt_i2c_remove() fetches the empty drvdata?

> +
> +	nvt_i2c_init(i2c);

[Severity: High]
Does nvt_i2c_init() access hardware registers before ensuring the system bus
clocks and I2C power domain are enabled? Accessing a clock-gated or
powered-down peripheral can trigger an imprecise external abort.

[ ... ]
> +	i2c->adapter.dev.of_node = of_node_get(pdev->dev.of_node);

[Severity: Medium]
Is this of_node reference ever released? There doesn't appear to be a
matching of_node_put() in the teardown paths, which would cause a memory
leak across bind/unbind cycles.

[ ... ]
> +static void nvt_i2c_remove(struct platform_device *pdev)
> +{
> +	struct nvt_i2c_bus *i2c = platform_get_drvdata(pdev);
> +
> +	i2c_del_adapter(&i2c->adapter);
> +}

[Severity: High]
Should device hardware interrupts be disabled before unregistering the
adapter and removing the driver? Since a shared IRQ line is requested,
an interrupt fired during or after teardown could cause an unhandled
interrupt storm.

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

[Severity: Medium]
Would it be better to wrap these callback assignments with pm_sleep_ptr()
to avoid generating dead code when PM features are disabled?

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

[Severity: Medium]
Similarly, should the pm structure pointer be wrapped with pm_ptr()?

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs.
  2026-06-04  6:04 ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
  2026-06-04  6:22   ` sashiko-bot
@ 2026-06-04  6:38   ` Wolfram Sang
  1 sibling, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2026-06-04  6:38 UTC (permalink / raw)
  To: SP_ISW1_AT
  Cc: andi.shyti, robh, krzk+dt, conor+dt, linux-i2c, devicetree,
	linux-kernel, ben_huang, toby_chui, shihpei_hsu

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

Hi,

thank you for your contribution! Sadly, I have to mention a few formal
issues first.

>  NOTICE: This message, including attachments, contains information which may
>  be confidential and privileged, and is intended only for use by the
>  addressees designated above. Unless you are the intended recipient, any
>  use, copying, disclosure, or distribution is prohibited. If you have
>  received the message in error, please immediately delete the message and
>  destroy all copies thereof and notify the sender by reply
>  email.</span><span

a) Please, no HTML messages.
b) we can't apply patches with a header like this because we distribute
the code to the whole world.
c) the sender e-mail address doesn't match the author or signed-off
e-mail address. Who is the sender?

All the best,

   Wolfram


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC i2c driver
@ 2026-06-05  3:55 SP_ISW1_AT
  2026-06-05  3:56 ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller SP_ISW1_AT
  0 siblings, 1 reply; 9+ messages in thread
From: SP_ISW1_AT @ 2026-06-05  3:55 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, linux-i2c, devicetree,
	linux-kernel
  Cc: SP_ISW1_AT, ben_huang, toby_chui, shihpei_hsu

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

Add entry for maintenance of Novatek NT726xx SoC i2c driver.

Signed-off-by: Ben Huang <Ben_Huang@novatek.com.tw>
Signed-off-by: Novatek i2c <SP_ISW1_AT@novatek.com.tw>
---
v2:
- Add Novatek i2c to Signed-off-by email list.
- Remove confidential related statements and HTML messages.
- Fix the potential issues in the device tree document and
  i2c driver source codes.
- Fix typos.

v1:
https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#t
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e035a3be797c..d42f644a596d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19014,6 +19014,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller
  2026-06-05  3:55 [PATCH 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC i2c driver SP_ISW1_AT
@ 2026-06-05  3:56 ` SP_ISW1_AT
  2026-06-05  3:56   ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
                     ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: SP_ISW1_AT @ 2026-06-05  3:56 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, linux-i2c, devicetree,
	linux-kernel
  Cc: SP_ISW1_AT, ben_huang, toby_chui, shihpei_hsu

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: Novatek i2c <SP_ISW1_AT@novatek.com.tw>
---
v2:
- Add Novatek i2c to Signed-off-by email list.
- Remove confidential related statements and HTML messages.
- Fix the potential issues in the device tree document and
  i2c driver source codes.
- Fix typos.

v1:
https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#t
---
 .../bindings/i2c/novatek,nt726xx-i2c.yaml     | 48 +++++++++++++++++++
 1 file changed, 48 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..d9dfdaaec205
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
@@ -0,0 +1,48 @@
+# 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 SoC I2C master controller
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    const: novatek,nt726xx-i2c
+
+  reg:
+    maxItems: 4
+
+  interrupts:
+    maxItems: 3
+
+  clock-frequency:
+    description: Operation clock frequency of i2c in Hz.
+    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)
+    minItems: 1
+
+  novatek,stbc:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Set if this i2c master controlled by stbc.
+    minItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - novatek,hwmods
+
+unevaluatedProperties: false
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs
  2026-06-05  3:56 ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller SP_ISW1_AT
@ 2026-06-05  3:56   ` SP_ISW1_AT
  2026-06-05  4:08     ` sashiko-bot
  2026-06-05  4:04   ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller sashiko-bot
  2026-06-05  5:25   ` Rob Herring (Arm)
  2 siblings, 1 reply; 9+ messages in thread
From: SP_ISW1_AT @ 2026-06-05  3:56 UTC (permalink / raw)
  To: andi.shyti, robh, krzk+dt, conor+dt, linux-i2c, devicetree,
	linux-kernel
  Cc: SP_ISW1_AT, ben_huang, toby_chui, shihpei_hsu

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

This patch introduce the support of i2c bus controller 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: Novatek i2c <SP_ISW1_AT@novatek.com.tw>
---
v2:
- Add Novatek i2c to Signed-off-by email list.
- Remove confidential related statements and HTML messages.
- Fix the potential issues in the device tree document and
  i2c driver source codes.
- Fix typos.

v1:
https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#t
---
 drivers/i2c/busses/Kconfig       |  10 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-nt726xx.c | 694 +++++++++++++++++++++++++++++++
 3 files changed, 705 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-nt726xx.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8c935f867a37..61daeac6b042 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -950,6 +950,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 547123ab351f..bfcd29203b35 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -89,6 +89,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..921f47b7dd55
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nt726xx.c
@@ -0,0 +1,694 @@
+// 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_PASSWORD                         0xFC040204
+#define STBC_PASSWORD_DATA1                   0x72682
+#define STBC_PASSWORD_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;
+	unsigned int 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_PASSWORD, 4);
+		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
+		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);
+		iounmap(reg_tmp);
+
+		reg_tmp = ioremap(STBC_KEYPASS, 4);
+		writel(readl(reg_tmp) | 0x1, reg_tmp);
+		iounmap(reg_tmp);
+	}
+}
+
+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) */
+	ret = of_property_read_u32(np, "novatek,stbc", &i2c->stbc_i2c);
+	if (ret) {
+		I2C_INFO_LOG("Not set dtb novatek,stbc, set default false\n");
+		i2c->stbc_i2c = 0;
+	}
+
+	/* 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,nt726xx-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_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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller
  2026-06-05  3:56 ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller SP_ISW1_AT
  2026-06-05  3:56   ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
@ 2026-06-05  4:04   ` sashiko-bot
  2026-06-05  5:25   ` Rob Herring (Arm)
  2 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-05  4:04 UTC (permalink / raw)
  To: SP_ISW1_AT; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] The device tree binding schema is entirely missing the mandatory `examples` section.
- [Low] The `novatek,hwmods` property uses incorrect type constraints, lacks pattern enforcement, and has ambiguous semantics.
- [Low] The `novatek,stbc` property is defined as a `uint32` but its description indicates it is a boolean flag.
- [Low] The `reg` and `interrupts` properties permit multiple items without documenting their meaning or expected order.
--

commit 1cba013f2a2d800236543516887a86edb943712e
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..d9dfdaaec205e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> @@ -0,0 +1,48 @@

[ ... ]

> +properties:
> +  compatible:
> +    const: novatek,nt726xx-i2c
> +
> +  reg:
> +    maxItems: 4
> +
> +  interrupts:
> +    maxItems: 3

[Severity: Low]
These properties permit multiple items without documenting their meaning or
expected order. Should we use an items list with descriptions so that the
purpose and expected order of each entry is defined?

> +
> +  clock-frequency:
> +    description: Operation clock frequency of i2c in Hz.
> +    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)
> +    minItems: 1

[Severity: Low]
This property references a scalar string definition, but minItems: 1 is an
array constraint, and the description "Name of each i2c pin" suggests it
accepts multiple names. Should this be defined as a string-array instead?

Also, should we add a pattern regex (like ^i2c[0-9]+$) to enforce the
documented naming rule?

> +
> +  novatek,stbc:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Set if this i2c master controlled by stbc.
> +    minItems: 1

[Severity: Low]
The description "Set if..." outlines a boolean state, but it is defined as a
uint32. Should this be declared using /schemas/types.yaml#/definitions/flag
instead?

Applying minItems: 1 is also redundant for a single scalar type.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - novatek,hwmods
> +
> +unevaluatedProperties: false

[Severity: Low]
Is the mandatory examples block missing from the end of this file?

All new hardware device tree binding YAML schemas require an examples block
to demonstrate how the device tree node should be written and to allow
dt_binding_check validation testing.

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs
  2026-06-05  3:56   ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
@ 2026-06-05  4:08     ` sashiko-bot
  0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-06-05  4:08 UTC (permalink / raw)
  To: SP_ISW1_AT; +Cc: robh, conor+dt, devicetree

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Hardcoded physical addresses mapped via ioremap() without NULL checks, and unprotected read-modify-write on shared global registers.
- [High] Driver breaks generic I2C transfers by unconditionally hijacking any 2-message array.
- [High] The ISR returns IRQ_NONE without clearing the hardware interrupt status when current_msg is NULL.
- [High] Missing synchronization between the transfer timeout path and the ISR leads to a use-after-free.
- [High] Interrupts are enabled in hardware before the IRQ handler is registered, and are leaked enabled on probe failure paths.
- [High] The I2C hardware and interrupts are disabled before the adapter is unregistered in the remove function.
- [Medium] Using a custom Device Tree property to statically assign the I2C bus number.
- [Medium] Device Tree node reference is permanently leaked on the adapter registration error path.
- [Low] Unconditional assignment of PM callbacks causes dead code when PM is disabled.
--

commit 2b999343fc99d6d31a3c1ef33d691ee2f0f37f51
Author: Ben Huang <Ben_Huang@novatek.com.tw>

i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs

This patch introduces support for the I2C bus controller of Novatek
NT726xx SoCs. It performs fundamental read/write functions and supports
Standard-mode and Fast-mode.

> diff --git a/drivers/i2c/busses/i2c-nt726xx.c b/drivers/i2c/busses/i2c-nt726xx.c
> new file mode 100644
> index 0000000000000..921f47b7dd551
> --- /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_PASSWORD, 4);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA1, reg_tmp);
> +		writel(readl(reg_tmp) | STBC_PASSWORD_DATA2, reg_tmp);

[Severity: High]
Does this code map a hardcoded physical address without checking the return
value of ioremap()? If the mapping fails, readl() will dereference a NULL
pointer. Additionally, is it safe to perform an unprotected read-modify-write
on these shared global registers, or could it corrupt state if other drivers
concurrently access the Standby Controller?

> +		iounmap(reg_tmp);
[ ... ]
> +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 the hardware asserts an interrupt and i2c->current_msg is NULL, does
this early return skip clearing the hardware interrupt status? The write
to clear the interrupt at the end of the ISR is bypassed, which could leave
the IRQ line asserted and cause an infinite interrupt loop.

> +
> +	if (status & I2C_IRQ_NACK) {
[ ... ]
> +static int nvt_i2c_write(struct nvt_i2c_bus *i2c, struct i2c_msg *msg)
> +{
[ ... ]
> +	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;

[Severity: High]
Can a use-after-free happen here if the timeout path races with the ISR?
If a transfer times out, nvt_i2c_reset() is called and current_msg is set
to NULL, but there is no synchronization (like a spinlock or
synchronize_irq()) to wait for a concurrently running ISR to finish. If the
ISR checks current_msg before it is set to NULL, the thread could return and
free msg->buf while the ISR continues to execute nvt_i2c_handle().

> +
> +	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];

[Severity: High]
Does this code unconditionally intercept all 2-message arrays? By consuming
msgs[0] as a subaddress and only transmitting msgs[1], this breaks standard
I2C semantics if a user-space application or generic kernel driver issues a
regular 2-message transfer. If the first message is a read, could it lead
to reading from an uninitialized buffer and writing it to the register?

> +
> +		if (msg->flags & I2C_M_RD)
[ ... ]
> +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 it appropriate to use a custom Device Tree property like novatek,hwmods
to statically assign the I2C adapter number? The standard Linux mechanism
for assigning bus numbers relies on the aliases node in the Device Tree
rather than custom properties.

> +		if (i2c->adapter.nr >= 0) {
[ ... ]
> +static int nvt_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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);

[Severity: High]
Does nvt_i2c_init() enable hardware interrupts before devm_request_irq()
registers the software handler? If the hardware has a pending interrupt,
the kernel might receive it prematurely. Furthermore, if probe fails on
subsequent calls like devm_request_irq(), are these interrupts left enabled
in the hardware, potentially causing an unhandled interrupt storm on a
shared line?

> +	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;

[Severity: Medium]
If i2c_add_numbered_adapter() fails, does this return path leak the OF node
reference? The node was acquired just above with of_node_get(), and
returning directly here skips calling of_node_put().

> +	}
> +
> +	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);

[Severity: High]
Is the teardown order reversed here? The code disables the I2C hardware and
interrupts before calling i2c_del_adapter(). If an active transaction is
running, this will forcefully break it. Additionally, i2c_del_adapter()
ensures synchronization by draining and blocking transfers, so it should
be called first before shutting down the hardware.

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

[Severity: Low]
Are the pm_sleep_ptr() wrappers missing here? Without wrapping the callbacks
in the dev_pm_ops structure, this will result in dead code being compiled
into the kernel when CONFIG_PM_SLEEP is disabled.

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

[Severity: Low]
Does this assignment need a pm_ptr() wrapper to avoid dead code when PM is
disabled?

> +		.of_match_table = of_match_ptr(nvt_i2c_of_match),
> +	},
> +};

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller
  2026-06-05  3:56 ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller SP_ISW1_AT
  2026-06-05  3:56   ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
  2026-06-05  4:04   ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller sashiko-bot
@ 2026-06-05  5:25   ` Rob Herring (Arm)
  2 siblings, 0 replies; 9+ messages in thread
From: Rob Herring (Arm) @ 2026-06-05  5:25 UTC (permalink / raw)
  To: SP_ISW1_AT
  Cc: shihpei_hsu, linux-kernel, devicetree, toby_chui, andi.shyti,
	krzk+dt, conor+dt, ben_huang, linux-i2c


On Fri, 05 Jun 2026 11:56:14 +0800, SP_ISW1_AT@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: Novatek i2c <SP_ISW1_AT@novatek.com.tw>
> ---
> v2:
> - Add Novatek i2c to Signed-off-by email list.
> - Remove confidential related statements and HTML messages.
> - Fix the potential issues in the device tree document and
>   i2c driver source codes.
> - Fix typos.
> 
> v1:
> https://lore.kernel.org/lkml/20260604060411.355675-1-SP_ISW1_AT@novatek.com.tw/T/#t
> ---
>  .../bindings/i2c/novatek,nt726xx-i2c.yaml     | 48 +++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/novatek,nt726xx-i2c.yaml: properties:novatek,stbc: False schema does not allow 1
	hint: Scalar properties should not have array keywords
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/20260605035614.383387-1-SP_ISW1_AT@novatek.com.tw

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-06-05  5:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-05  3:55 [PATCH 1/3] MAINTAINERS: Add entry for Novatek NT726xx SoC i2c driver SP_ISW1_AT
2026-06-05  3:56 ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller SP_ISW1_AT
2026-06-05  3:56   ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
2026-06-05  4:08     ` sashiko-bot
2026-06-05  4:04   ` [PATCH 2/3] dt-bindings: i2c: add Novatek NT726xx SoC i2c controller sashiko-bot
2026-06-05  5:25   ` Rob Herring (Arm)
  -- strict thread matches above, loose matches on Subject: below --
2026-06-04  6:04 SP_ISW1_AT
2026-06-04  6:04 ` [PATCH 3/3] i2c: Add i2c-nt726xx.c i2c driver for Novatek NT726xx SoCs SP_ISW1_AT
2026-06-04  6:22   ` sashiko-bot
2026-06-04  6:38   ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox