* [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-09-24 8:39 ` Xianwei Zhao via B4 Relay
0 siblings, 0 replies; 24+ messages in thread
From: Xianwei Zhao @ 2026-09-24 8:39 UTC (permalink / raw)
To: Junyi Zhao, Andi Shyti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Xianwei Zhao
Cc: Junyi Zhao, linux-i2c, linux-amlogic, devicetree, linux-kernel,
Xianwei Zhao
From: Junyi Zhao <junyi.zhao@amlogic.com>
Add support for the new I2C controller used by Amlogic A9 SoCs.
Add a dedicated Kconfig option and Makefile entry for the A9 I2C
controller driver.
Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
drivers/i2c/busses/Kconfig | 9 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-amlogic-a9.c | 504 ++++++++++++++++++++++++++++++++++++
3 files changed, 514 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d7b89508311f..5595a5166569 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -885,6 +885,15 @@ config I2C_MESON
If you say yes to this option, support will be included for the
I2C interface on the Amlogic Meson family of SoCs.
+config I2C_AMLOGIC_A9
+ tristate "Amlogic new I2C controller"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ If you say yes to this option, support will be included for the
+ I2C interface on the new Amlogic family of SoCs.
+
+
config I2C_MICROCHIP_CORE
tristate "Microchip FPGA I2C controller"
depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3755c54b3d82..9ffb1082b80b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o
obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o
obj-$(CONFIG_I2C_LS2X_V2) += i2c-ls2x-v2.o
obj-$(CONFIG_I2C_MESON) += i2c-meson.o
+obj-$(CONFIG_I2C_AMLOGIC_A9) += i2c-amlogic-a9.o
obj-$(CONFIG_I2C_MICROCHIP_CORE) += i2c-microchip-corei2c.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o
diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
new file mode 100644
index 000000000000..2fcac7ee0e72
--- /dev/null
+++ b/drivers/i2c/busses/i2c-amlogic-a9.c
@@ -0,0 +1,504 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2026 Amlogic, Inc. All rights reserved
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/delay.h>
+
+/* Amlogic I2C register map */
+#define REG_CFG_RDY 0x00
+#define REG_CFG_I2C 0x04
+#define REG_CFG_START 0x08
+#define REG_CFG_BUS 0x0c
+#define REG_TX_RD_ADDR 0x10
+#define REG_TX_WR_ADDR 0x14
+#define REG_RX_RD_ADDR 0x18
+#define REG_RX_WR_ADDR 0x1c
+#define REG_CGF_TX 0x20
+#define REG_CGF_RX 0x24
+#define REG_CGF_IRQ_STATE 0x30
+#define REG_CGF_IRQ_ENABLE 0x34
+#define REG_SHAKE_BLK_CNT 0x38
+
+/* CFG RDY fields */
+#define RDY_TEE_ONLY BIT(1)
+#define RDY_IF BIT(0)
+
+/* CFG I2C fields */
+#define I2C_RX_THR GENMASK(23, 16)
+#define I2C_TX_THR GENMASK(15, 8)
+
+/* CFG BUS fields */
+#define BUS_NO_STOP BIT(18)
+#define BUS_SPEED_MODE BIT(17)
+#define BUS_SLAVE_MODE BIT(16)
+#define BUS_FILTER_MASK GENMASK(15, 12)
+/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
+#define BUS_RATIO_MASK GENMASK(11, 0)
+
+/* CFG START fields */
+#define START_START BIT(31)
+#define START_LEN GENMASK(23, 12)
+#define START_LEN_SHIFT 12
+#define START_SLAVE_ADDR GENMASK(10, 1)
+#define START_READ BIT(0)
+
+/* CFG TX_RX fields */
+#define TX_RX_EMPTY BIT(9)
+#define TX_RX_FULL BIT(8)
+#define TX_RX_DATA GENMASK(7, 0)
+
+/*CGF IRQ fields */
+#define IRQ_ALL_MASK 0xffff
+#define A9_NCK_ERROR BIT(0)
+#define A9_RX_EMPTY BIT(1)
+#define A9_RX_FULL BIT(2)
+#define A9_TX_EMPTY BIT(3)
+#define A9_TX_FULL BIT(4)
+#define A9_RX_THRESH_READ BIT(5)
+#define A9_TX_THRESH_WRITE BIT(6)
+#define A9_PHY_DONE BIT(7)
+#define A9_TASK_DONE BIT(8)
+#define A9_ALL_DONE BIT(9)
+
+#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
+#define A9_TRANS_ERROR (A9_NCK_ERROR)
+#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
+#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
+
+#define A9_I2C_FIFO_DEPTH 32
+#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
+
+#define I2C_TIMEOUT_MS 500
+
+enum {
+ STATE_IDLE,
+ STATE_READ,
+ STATE_WRITE,
+};
+
+enum fifo_fill_mode {
+ FIFO_FILL_FULL,
+ FIFO_FILL_HALF,
+};
+
+/**
+ * struct aml_i2c - Amlogic I2C device private data
+ *
+ * @adap: I2C adapter instance
+ * @dev: Pointer to device structure
+ * @regs: Base address of the device memory mapped registers
+ * @clk: Pointer to clock structure
+ * @msg: Pointer to the current I2C message
+ * @state: Current state in the driver state machine
+ * @count: Number of bytes to be sent/received in current transfer
+ * @pos: Current position in the send/receive buffer
+ * @error: Flag set when an error is received
+ * @lock: To avoid race conditions between irq handler and xfer code
+ * @done: Completion used to wait for transfer termination
+ * @clk_rate: Clock source rate of I2C module
+ * @frequency: Operating frequency of I2C bus clock
+ */
+struct aml_i2c {
+ struct i2c_adapter adap;
+ struct device *dev;
+ void __iomem *regs;
+ struct clk *clk;
+
+ struct i2c_msg *msg;
+ int state;
+ int count;
+ int pos;
+ int error;
+ /* protect reg access */
+ spinlock_t lock;
+ struct completion done;
+ unsigned long clk_rate;
+ unsigned int frequency;
+};
+
+static void aml_i2c_set_mask(struct aml_i2c *i2c, int reg, u32 mask, u32 val)
+{
+ u32 data;
+
+ data = readl(i2c->regs + reg);
+ data &= ~mask;
+ data |= val & mask;
+ writel(data, i2c->regs + reg);
+}
+
+/*
+ * SCL = clk/b_ratio. if b_ratio<=8, SCL = clk/8
+ */
+static int aml_i2c_set_clk_div(struct aml_i2c *i2c)
+{
+ unsigned long clk_rate = i2c->clk_rate;
+ unsigned int div;
+
+ div = DIV_ROUND_UP(clk_rate, i2c->frequency);
+
+ /* clock divider has 12 bits */
+ if (div >= (1 << 12)) {
+ dev_err(i2c->dev, "requested bus frequency too low\n");
+ return -EINVAL;
+ } else if (div <= 8) {
+ dev_err(i2c->dev, "requested bus frequency too high\n");
+ return -EINVAL;
+ }
+ /* CFG_BUS reg:11 - 0 bits
+ */
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_RATIO_MASK, div);
+
+ return 0;
+}
+
+static void reset_fifo_pos(struct aml_i2c *i2c)
+{
+ writel(0x00, i2c->regs + REG_TX_RD_ADDR);
+ writel(0x00, i2c->regs + REG_TX_WR_ADDR);
+ writel(0x00, i2c->regs + REG_RX_WR_ADDR);
+ writel(0x00, i2c->regs + REG_RX_RD_ADDR);
+}
+
+static int controller_is_error(struct aml_i2c *i2c)
+{
+ int ret = 0;
+
+ if (readl(i2c->regs + REG_CGF_IRQ_STATE) & A9_TRANS_ERROR)
+ ret = -ENXIO;
+
+ return ret;
+}
+
+static int controller_is_done(struct aml_i2c *i2c)
+{
+ return readl(i2c->regs + REG_CGF_IRQ_STATE) & A9_TRANS_DONE;
+}
+
+static void clear_irq_status(struct aml_i2c *i2c, u32 irq_bits)
+{
+ writel(irq_bits, i2c->regs + REG_CGF_IRQ_STATE);
+}
+
+static int aml_i2c_init(struct aml_i2c *i2c)
+{
+ int ret;
+
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ writel(A9_ENABLE_IRQ_BIT, i2c->regs + REG_CGF_IRQ_ENABLE);
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP);
+ aml_i2c_set_mask(i2c, REG_CFG_I2C, I2C_RX_THR | I2C_TX_THR,
+ A9_I2C_FIFO_DEPTH << 16 | A9_I2C_HALF_FIFO << 8);
+ reset_fifo_pos(i2c);
+ ret = aml_i2c_set_clk_div(i2c);
+
+ return ret;
+}
+
+static void aml_i2c_add_stop(struct aml_i2c *i2c)
+{
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0);
+}
+
+static void aml_i2c_cpy_data_to_user(struct aml_i2c *i2c, u8 data)
+{
+ u8 *buf = i2c->msg->buf + i2c->pos;
+
+ i2c->count = 1;
+ *buf = data;
+}
+
+static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
+{
+ int i;
+
+ /* this i2c module when trans 0 byte, must put at least 1.
+ */
+ if (!i2c->msg->len) {
+ writel(0x00, i2c->regs + REG_CGF_TX);
+ return;
+ }
+
+ for (i = 0; i < len; i++, buf++)
+ writel(*buf, i2c->regs + REG_CGF_TX);
+}
+
+static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
+{
+ bool write = !(i2c->msg->flags & I2C_M_RD);
+
+ if (write) {
+ if (mode == FIFO_FILL_FULL)
+ i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
+ else
+ i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
+ aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
+ }
+}
+
+static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
+{
+ struct aml_i2c *i2c = dev_id;
+ int ret;
+ u32 rd_len, i;
+ u32 rd_val;
+
+ spin_lock(&i2c->lock);
+ ret = controller_is_error(i2c);
+ if (ret) {
+ i2c->error = ret;
+ goto complete_irq_out;
+ }
+
+ switch (i2c->state) {
+ case STATE_READ:
+ rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
+ for (i = 0; i < rd_len; i++) {
+ rd_val = readl(i2c->regs + REG_CGF_RX);
+ aml_i2c_cpy_data_to_user(i2c, rd_val & TX_RX_DATA);
+ i2c->pos += i2c->count;
+ }
+ /* rx must clear thresh after read fifo */
+ clear_irq_status(i2c, A9_RX_THRESH_READ);
+ break;
+ case STATE_WRITE:
+ aml_i2c_prepare_xfer(i2c, FIFO_FILL_HALF);
+ if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len)
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
+ /* tx must clear thresh after write fifo */
+ clear_irq_status(i2c, A9_TX_THRESH_WRITE);
+ i2c->pos += i2c->count;
+ break;
+ case STATE_IDLE:
+ dev_warn(i2c->dev, "i2c irq triggered abnormally!!!, irq_state:0x%x,irq_en:0x%x\n",
+ readl(i2c->regs + REG_CGF_IRQ_STATE),
+ readl(i2c->regs + REG_CGF_IRQ_ENABLE));
+ break;
+ default:
+ break;
+ }
+
+ if (!controller_is_done(i2c)) {
+ goto irq_trans_out;
+ } else {
+ if (i2c->state == STATE_READ && (i2c->msg->len - i2c->pos))
+ goto irq_trans_out;
+ }
+
+complete_irq_out:
+ reset_fifo_pos(i2c);
+ complete(&i2c->done);
+ i2c->state = STATE_IDLE;
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+irq_trans_out:
+ spin_unlock(&i2c->lock);
+
+ return IRQ_HANDLED;
+}
+
+static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg)
+{
+ unsigned int reg_val;
+ unsigned int trans_len;
+
+ trans_len = msg->len;
+ reg_val = (unsigned int)((msg->addr << 1) |
+ ((msg->flags & I2C_M_RD) ? START_READ : 0) |
+ ((trans_len & 0xfff) << 12) | START_START);
+ aml_i2c_set_mask(i2c, REG_CFG_START,
+ START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val);
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ /* enable thresh mode */
+ if (trans_len > A9_I2C_FIFO_DEPTH) {
+ reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE,
+ A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
+ }
+}
+
+static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
+{
+ unsigned long time_left, flags;
+ int ret = 0;
+
+ i2c->msg = msg;
+ i2c->pos = 0;
+ i2c->count = 0;
+ i2c->error = 0;
+
+ i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
+ aml_i2c_prepare_xfer(i2c, FIFO_FILL_FULL);
+ i2c->pos += i2c->count;
+ reinit_completion(&i2c->done);
+
+ if (last)
+ aml_i2c_add_stop(i2c);
+ /* Start the transfer */
+ aml_i2c_do_start(i2c, msg);
+
+ time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
+ time_left = wait_for_completion_timeout(&i2c->done, time_left);
+
+ /*
+ * Protect access to i2c struct and registers from interrupt
+ * handlers triggered by a transfer terminated after the
+ * timeout period
+ */
+ spin_lock_irqsave(&i2c->lock, flags);
+
+ if (!time_left) {
+ i2c->state = STATE_IDLE;
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ reset_fifo_pos(i2c);
+ ret = -ETIMEDOUT;
+ }
+
+ if (i2c->error)
+ ret = i2c->error;
+
+ spin_unlock_irqrestore(&i2c->lock, flags);
+
+ return ret;
+}
+
+static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct aml_i2c *i2c = adap->algo_data;
+ int i, ret = 0;
+
+ for (i = 0; i < num; i++) {
+ ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
+ if (ret)
+ break;
+ }
+
+ return ret ?: i;
+}
+
+static u32 aml_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm aml_i2c_algorithm = {
+ .master_xfer = aml_i2c_xfer,
+ .functionality = aml_i2c_func,
+};
+
+static int aml_i2c_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct aml_i2c *i2c;
+ struct i2c_timings timings;
+ int irq, ret;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(struct aml_i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ i2c_parse_fw_timings(&pdev->dev, &timings, true);
+ i2c->frequency = timings.bus_freq_hz;
+
+ i2c->dev = &pdev->dev;
+ platform_set_drvdata(pdev, i2c);
+
+ spin_lock_init(&i2c->lock);
+ init_completion(&i2c->done);
+
+ i2c->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(i2c->regs))
+ return PTR_ERR(i2c->regs);
+
+ i2c->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(i2c->clk)) {
+ dev_err(&pdev->dev, "can't get device clock\n");
+ return PTR_ERR(i2c->clk);
+ }
+
+ i2c->clk_rate = clk_get_rate(i2c->clk);
+ if (i2c->clk_rate == 0) {
+ dev_err(&pdev->dev, "failed to get clk rate\n");
+ return -EINVAL;
+ }
+
+ ret = aml_i2c_init(i2c);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "fail init i2c\n");
+ return ret;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't find IRQ\n");
+ return irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "can't request IRQ\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(i2c->clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "can't prepare clock\n");
+ return ret;
+ }
+
+ strscpy(i2c->adap.name, "Amlogic I2C adapter",
+ sizeof(i2c->adap.name));
+ i2c->adap.owner = THIS_MODULE;
+ i2c->adap.algo = &aml_i2c_algorithm;
+ i2c->adap.dev.parent = &pdev->dev;
+ i2c->adap.dev.of_node = np;
+ i2c->adap.algo_data = i2c;
+
+ ret = i2c_add_adapter(&i2c->adap);
+ if (ret < 0) {
+ clk_disable_unprepare(i2c->clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void aml_i2c_remove(struct platform_device *pdev)
+{
+ struct aml_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&i2c->adap);
+ clk_disable_unprepare(i2c->clk);
+}
+
+static const struct of_device_id aml_i2c_match[] = {
+ { .compatible = "amlogic,a9-i2c",},
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, aml_i2c_match);
+
+static struct platform_driver aml_i2c_driver = {
+ .probe = aml_i2c_probe,
+ .remove = aml_i2c_remove,
+ .driver = {
+ .name = "amlogic-i2c",
+ .of_match_table = aml_i2c_match,
+ },
+};
+
+module_platform_driver(aml_i2c_driver);
+
+MODULE_DESCRIPTION("Amlogic I2C Bus driver");
+MODULE_AUTHOR("Junyi Zhao <junyi.zhao@amlogic.com>");
+MODULE_LICENSE("GPL");
--
2.52.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-09-24 8:39 ` Xianwei Zhao via B4 Relay
0 siblings, 0 replies; 24+ messages in thread
From: Xianwei Zhao via B4 Relay @ 2026-09-24 8:39 UTC (permalink / raw)
To: Junyi Zhao, Andi Shyti, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Xianwei Zhao
Cc: Junyi Zhao, linux-i2c, linux-amlogic, devicetree, linux-kernel,
Xianwei Zhao
From: Junyi Zhao <junyi.zhao@amlogic.com>
Add support for the new I2C controller used by Amlogic A9 SoCs.
Add a dedicated Kconfig option and Makefile entry for the A9 I2C
controller driver.
Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
drivers/i2c/busses/Kconfig | 9 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-amlogic-a9.c | 504 ++++++++++++++++++++++++++++++++++++
3 files changed, 514 insertions(+)
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d7b89508311f..5595a5166569 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -885,6 +885,15 @@ config I2C_MESON
If you say yes to this option, support will be included for the
I2C interface on the Amlogic Meson family of SoCs.
+config I2C_AMLOGIC_A9
+ tristate "Amlogic new I2C controller"
+ depends on ARCH_MESON || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ If you say yes to this option, support will be included for the
+ I2C interface on the new Amlogic family of SoCs.
+
+
config I2C_MICROCHIP_CORE
tristate "Microchip FPGA I2C controller"
depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3755c54b3d82..9ffb1082b80b 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_I2C_LPC2K) += i2c-lpc2k.o
obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o
obj-$(CONFIG_I2C_LS2X_V2) += i2c-ls2x-v2.o
obj-$(CONFIG_I2C_MESON) += i2c-meson.o
+obj-$(CONFIG_I2C_AMLOGIC_A9) += i2c-amlogic-a9.o
obj-$(CONFIG_I2C_MICROCHIP_CORE) += i2c-microchip-corei2c.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
obj-$(CONFIG_I2C_MT65XX) += i2c-mt65xx.o
diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
new file mode 100644
index 000000000000..2fcac7ee0e72
--- /dev/null
+++ b/drivers/i2c/busses/i2c-amlogic-a9.c
@@ -0,0 +1,504 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2026 Amlogic, Inc. All rights reserved
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/delay.h>
+
+/* Amlogic I2C register map */
+#define REG_CFG_RDY 0x00
+#define REG_CFG_I2C 0x04
+#define REG_CFG_START 0x08
+#define REG_CFG_BUS 0x0c
+#define REG_TX_RD_ADDR 0x10
+#define REG_TX_WR_ADDR 0x14
+#define REG_RX_RD_ADDR 0x18
+#define REG_RX_WR_ADDR 0x1c
+#define REG_CGF_TX 0x20
+#define REG_CGF_RX 0x24
+#define REG_CGF_IRQ_STATE 0x30
+#define REG_CGF_IRQ_ENABLE 0x34
+#define REG_SHAKE_BLK_CNT 0x38
+
+/* CFG RDY fields */
+#define RDY_TEE_ONLY BIT(1)
+#define RDY_IF BIT(0)
+
+/* CFG I2C fields */
+#define I2C_RX_THR GENMASK(23, 16)
+#define I2C_TX_THR GENMASK(15, 8)
+
+/* CFG BUS fields */
+#define BUS_NO_STOP BIT(18)
+#define BUS_SPEED_MODE BIT(17)
+#define BUS_SLAVE_MODE BIT(16)
+#define BUS_FILTER_MASK GENMASK(15, 12)
+/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
+#define BUS_RATIO_MASK GENMASK(11, 0)
+
+/* CFG START fields */
+#define START_START BIT(31)
+#define START_LEN GENMASK(23, 12)
+#define START_LEN_SHIFT 12
+#define START_SLAVE_ADDR GENMASK(10, 1)
+#define START_READ BIT(0)
+
+/* CFG TX_RX fields */
+#define TX_RX_EMPTY BIT(9)
+#define TX_RX_FULL BIT(8)
+#define TX_RX_DATA GENMASK(7, 0)
+
+/*CGF IRQ fields */
+#define IRQ_ALL_MASK 0xffff
+#define A9_NCK_ERROR BIT(0)
+#define A9_RX_EMPTY BIT(1)
+#define A9_RX_FULL BIT(2)
+#define A9_TX_EMPTY BIT(3)
+#define A9_TX_FULL BIT(4)
+#define A9_RX_THRESH_READ BIT(5)
+#define A9_TX_THRESH_WRITE BIT(6)
+#define A9_PHY_DONE BIT(7)
+#define A9_TASK_DONE BIT(8)
+#define A9_ALL_DONE BIT(9)
+
+#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
+#define A9_TRANS_ERROR (A9_NCK_ERROR)
+#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
+#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
+
+#define A9_I2C_FIFO_DEPTH 32
+#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
+
+#define I2C_TIMEOUT_MS 500
+
+enum {
+ STATE_IDLE,
+ STATE_READ,
+ STATE_WRITE,
+};
+
+enum fifo_fill_mode {
+ FIFO_FILL_FULL,
+ FIFO_FILL_HALF,
+};
+
+/**
+ * struct aml_i2c - Amlogic I2C device private data
+ *
+ * @adap: I2C adapter instance
+ * @dev: Pointer to device structure
+ * @regs: Base address of the device memory mapped registers
+ * @clk: Pointer to clock structure
+ * @msg: Pointer to the current I2C message
+ * @state: Current state in the driver state machine
+ * @count: Number of bytes to be sent/received in current transfer
+ * @pos: Current position in the send/receive buffer
+ * @error: Flag set when an error is received
+ * @lock: To avoid race conditions between irq handler and xfer code
+ * @done: Completion used to wait for transfer termination
+ * @clk_rate: Clock source rate of I2C module
+ * @frequency: Operating frequency of I2C bus clock
+ */
+struct aml_i2c {
+ struct i2c_adapter adap;
+ struct device *dev;
+ void __iomem *regs;
+ struct clk *clk;
+
+ struct i2c_msg *msg;
+ int state;
+ int count;
+ int pos;
+ int error;
+ /* protect reg access */
+ spinlock_t lock;
+ struct completion done;
+ unsigned long clk_rate;
+ unsigned int frequency;
+};
+
+static void aml_i2c_set_mask(struct aml_i2c *i2c, int reg, u32 mask, u32 val)
+{
+ u32 data;
+
+ data = readl(i2c->regs + reg);
+ data &= ~mask;
+ data |= val & mask;
+ writel(data, i2c->regs + reg);
+}
+
+/*
+ * SCL = clk/b_ratio. if b_ratio<=8, SCL = clk/8
+ */
+static int aml_i2c_set_clk_div(struct aml_i2c *i2c)
+{
+ unsigned long clk_rate = i2c->clk_rate;
+ unsigned int div;
+
+ div = DIV_ROUND_UP(clk_rate, i2c->frequency);
+
+ /* clock divider has 12 bits */
+ if (div >= (1 << 12)) {
+ dev_err(i2c->dev, "requested bus frequency too low\n");
+ return -EINVAL;
+ } else if (div <= 8) {
+ dev_err(i2c->dev, "requested bus frequency too high\n");
+ return -EINVAL;
+ }
+ /* CFG_BUS reg:11 - 0 bits
+ */
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_RATIO_MASK, div);
+
+ return 0;
+}
+
+static void reset_fifo_pos(struct aml_i2c *i2c)
+{
+ writel(0x00, i2c->regs + REG_TX_RD_ADDR);
+ writel(0x00, i2c->regs + REG_TX_WR_ADDR);
+ writel(0x00, i2c->regs + REG_RX_WR_ADDR);
+ writel(0x00, i2c->regs + REG_RX_RD_ADDR);
+}
+
+static int controller_is_error(struct aml_i2c *i2c)
+{
+ int ret = 0;
+
+ if (readl(i2c->regs + REG_CGF_IRQ_STATE) & A9_TRANS_ERROR)
+ ret = -ENXIO;
+
+ return ret;
+}
+
+static int controller_is_done(struct aml_i2c *i2c)
+{
+ return readl(i2c->regs + REG_CGF_IRQ_STATE) & A9_TRANS_DONE;
+}
+
+static void clear_irq_status(struct aml_i2c *i2c, u32 irq_bits)
+{
+ writel(irq_bits, i2c->regs + REG_CGF_IRQ_STATE);
+}
+
+static int aml_i2c_init(struct aml_i2c *i2c)
+{
+ int ret;
+
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ writel(A9_ENABLE_IRQ_BIT, i2c->regs + REG_CGF_IRQ_ENABLE);
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, BUS_NO_STOP);
+ aml_i2c_set_mask(i2c, REG_CFG_I2C, I2C_RX_THR | I2C_TX_THR,
+ A9_I2C_FIFO_DEPTH << 16 | A9_I2C_HALF_FIFO << 8);
+ reset_fifo_pos(i2c);
+ ret = aml_i2c_set_clk_div(i2c);
+
+ return ret;
+}
+
+static void aml_i2c_add_stop(struct aml_i2c *i2c)
+{
+ aml_i2c_set_mask(i2c, REG_CFG_BUS, BUS_NO_STOP, 0);
+}
+
+static void aml_i2c_cpy_data_to_user(struct aml_i2c *i2c, u8 data)
+{
+ u8 *buf = i2c->msg->buf + i2c->pos;
+
+ i2c->count = 1;
+ *buf = data;
+}
+
+static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
+{
+ int i;
+
+ /* this i2c module when trans 0 byte, must put at least 1.
+ */
+ if (!i2c->msg->len) {
+ writel(0x00, i2c->regs + REG_CGF_TX);
+ return;
+ }
+
+ for (i = 0; i < len; i++, buf++)
+ writel(*buf, i2c->regs + REG_CGF_TX);
+}
+
+static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
+{
+ bool write = !(i2c->msg->flags & I2C_M_RD);
+
+ if (write) {
+ if (mode == FIFO_FILL_FULL)
+ i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
+ else
+ i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
+ aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
+ }
+}
+
+static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
+{
+ struct aml_i2c *i2c = dev_id;
+ int ret;
+ u32 rd_len, i;
+ u32 rd_val;
+
+ spin_lock(&i2c->lock);
+ ret = controller_is_error(i2c);
+ if (ret) {
+ i2c->error = ret;
+ goto complete_irq_out;
+ }
+
+ switch (i2c->state) {
+ case STATE_READ:
+ rd_len = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
+ for (i = 0; i < rd_len; i++) {
+ rd_val = readl(i2c->regs + REG_CGF_RX);
+ aml_i2c_cpy_data_to_user(i2c, rd_val & TX_RX_DATA);
+ i2c->pos += i2c->count;
+ }
+ /* rx must clear thresh after read fifo */
+ clear_irq_status(i2c, A9_RX_THRESH_READ);
+ break;
+ case STATE_WRITE:
+ aml_i2c_prepare_xfer(i2c, FIFO_FILL_HALF);
+ if ((i2c->pos + A9_I2C_HALF_FIFO) >= i2c->msg->len)
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE, A9_TX_THRESH_WRITE, 0);
+ /* tx must clear thresh after write fifo */
+ clear_irq_status(i2c, A9_TX_THRESH_WRITE);
+ i2c->pos += i2c->count;
+ break;
+ case STATE_IDLE:
+ dev_warn(i2c->dev, "i2c irq triggered abnormally!!!, irq_state:0x%x,irq_en:0x%x\n",
+ readl(i2c->regs + REG_CGF_IRQ_STATE),
+ readl(i2c->regs + REG_CGF_IRQ_ENABLE));
+ break;
+ default:
+ break;
+ }
+
+ if (!controller_is_done(i2c)) {
+ goto irq_trans_out;
+ } else {
+ if (i2c->state == STATE_READ && (i2c->msg->len - i2c->pos))
+ goto irq_trans_out;
+ }
+
+complete_irq_out:
+ reset_fifo_pos(i2c);
+ complete(&i2c->done);
+ i2c->state = STATE_IDLE;
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE, A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, 0);
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+irq_trans_out:
+ spin_unlock(&i2c->lock);
+
+ return IRQ_HANDLED;
+}
+
+static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg)
+{
+ unsigned int reg_val;
+ unsigned int trans_len;
+
+ trans_len = msg->len;
+ reg_val = (unsigned int)((msg->addr << 1) |
+ ((msg->flags & I2C_M_RD) ? START_READ : 0) |
+ ((trans_len & 0xfff) << 12) | START_START);
+ aml_i2c_set_mask(i2c, REG_CFG_START,
+ START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val);
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ /* enable thresh mode */
+ if (trans_len > A9_I2C_FIFO_DEPTH) {
+ reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
+ aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE,
+ A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
+ }
+}
+
+static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
+{
+ unsigned long time_left, flags;
+ int ret = 0;
+
+ i2c->msg = msg;
+ i2c->pos = 0;
+ i2c->count = 0;
+ i2c->error = 0;
+
+ i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
+ aml_i2c_prepare_xfer(i2c, FIFO_FILL_FULL);
+ i2c->pos += i2c->count;
+ reinit_completion(&i2c->done);
+
+ if (last)
+ aml_i2c_add_stop(i2c);
+ /* Start the transfer */
+ aml_i2c_do_start(i2c, msg);
+
+ time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
+ time_left = wait_for_completion_timeout(&i2c->done, time_left);
+
+ /*
+ * Protect access to i2c struct and registers from interrupt
+ * handlers triggered by a transfer terminated after the
+ * timeout period
+ */
+ spin_lock_irqsave(&i2c->lock, flags);
+
+ if (!time_left) {
+ i2c->state = STATE_IDLE;
+ clear_irq_status(i2c, IRQ_ALL_MASK);
+ reset_fifo_pos(i2c);
+ ret = -ETIMEDOUT;
+ }
+
+ if (i2c->error)
+ ret = i2c->error;
+
+ spin_unlock_irqrestore(&i2c->lock, flags);
+
+ return ret;
+}
+
+static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+ struct aml_i2c *i2c = adap->algo_data;
+ int i, ret = 0;
+
+ for (i = 0; i < num; i++) {
+ ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
+ if (ret)
+ break;
+ }
+
+ return ret ?: i;
+}
+
+static u32 aml_i2c_func(struct i2c_adapter *adap)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm aml_i2c_algorithm = {
+ .master_xfer = aml_i2c_xfer,
+ .functionality = aml_i2c_func,
+};
+
+static int aml_i2c_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct aml_i2c *i2c;
+ struct i2c_timings timings;
+ int irq, ret;
+
+ i2c = devm_kzalloc(&pdev->dev, sizeof(struct aml_i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ i2c_parse_fw_timings(&pdev->dev, &timings, true);
+ i2c->frequency = timings.bus_freq_hz;
+
+ i2c->dev = &pdev->dev;
+ platform_set_drvdata(pdev, i2c);
+
+ spin_lock_init(&i2c->lock);
+ init_completion(&i2c->done);
+
+ i2c->regs = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(i2c->regs))
+ return PTR_ERR(i2c->regs);
+
+ i2c->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(i2c->clk)) {
+ dev_err(&pdev->dev, "can't get device clock\n");
+ return PTR_ERR(i2c->clk);
+ }
+
+ i2c->clk_rate = clk_get_rate(i2c->clk);
+ if (i2c->clk_rate == 0) {
+ dev_err(&pdev->dev, "failed to get clk rate\n");
+ return -EINVAL;
+ }
+
+ ret = aml_i2c_init(i2c);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "fail init i2c\n");
+ return ret;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't find IRQ\n");
+ return irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "can't request IRQ\n");
+ return ret;
+ }
+
+ ret = clk_prepare_enable(i2c->clk);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "can't prepare clock\n");
+ return ret;
+ }
+
+ strscpy(i2c->adap.name, "Amlogic I2C adapter",
+ sizeof(i2c->adap.name));
+ i2c->adap.owner = THIS_MODULE;
+ i2c->adap.algo = &aml_i2c_algorithm;
+ i2c->adap.dev.parent = &pdev->dev;
+ i2c->adap.dev.of_node = np;
+ i2c->adap.algo_data = i2c;
+
+ ret = i2c_add_adapter(&i2c->adap);
+ if (ret < 0) {
+ clk_disable_unprepare(i2c->clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void aml_i2c_remove(struct platform_device *pdev)
+{
+ struct aml_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&i2c->adap);
+ clk_disable_unprepare(i2c->clk);
+}
+
+static const struct of_device_id aml_i2c_match[] = {
+ { .compatible = "amlogic,a9-i2c",},
+ { }
+};
+
+MODULE_DEVICE_TABLE(of, aml_i2c_match);
+
+static struct platform_driver aml_i2c_driver = {
+ .probe = aml_i2c_probe,
+ .remove = aml_i2c_remove,
+ .driver = {
+ .name = "amlogic-i2c",
+ .of_match_table = aml_i2c_match,
+ },
+};
+
+module_platform_driver(aml_i2c_driver);
+
+MODULE_DESCRIPTION("Amlogic I2C Bus driver");
+MODULE_AUTHOR("Junyi Zhao <junyi.zhao@amlogic.com>");
+MODULE_LICENSE("GPL");
--
2.52.0
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-09-24 8:39 ` Xianwei Zhao via B4 Relay
@ 2026-09-24 8:57 ` sashiko-bot
-1 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-09-24 8:57 UTC (permalink / raw)
To: Xianwei Zhao; +Cc: devicetree, neil.armstrong, linux-amlogic, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] i2c: amlogic: hardware registers accessed before clock is enabled
- [High] i2c: amlogic: race condition drops completion interrupts
- [High] i2c: amlogic: silent truncation of large transfers
- [High] i2c: amlogic: broken Repeated START for subsequent transfers
- [High] i2c: amlogic: data race on REG_CGF_IRQ_ENABLE
- [High] i2c: amlogic: IRQ storm on transfer timeout
- [High] i2c: amlogic: race in lockless state initialization
- [Medium] i2c: amlogic: crash due to early clock disable in remove
--
commit 5e779e1911cc216efb8179fa9e2910973c79bb74
Author: Junyi Zhao <junyi.zhao@amlogic.com>
i2c: amlogic: Add Amlogic A9 I2C controller driver
Add support for the new I2C controller used by Amlogic A9 SoCs.
> diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
[ ... ]
> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
> +{
> + struct aml_i2c *i2c = dev_id;
> + int ret;
> + u32 rd_len, i;
> + u32 rd_val;
> +
> + spin_lock(&i2c->lock);
> + ret = controller_is_error(i2c);
> + if (ret) {
> + i2c->error = ret;
> + goto complete_irq_out;
> + }
> +
> + switch (i2c->state) {
> + case STATE_READ:
[ ... ]
> + case STATE_WRITE:
[ ... ]
> + case STATE_IDLE:
> + dev_warn(i2c->dev, "i2c irq triggered abnormally!!!, irq_state:0x%x,irq_en:0x%x\n",
> + readl(i2c->regs + REG_CGF_IRQ_STATE),
> + readl(i2c->regs + REG_CGF_IRQ_ENABLE));
> + break;
> + default:
> + break;
> + }
> +
> + if (!controller_is_done(i2c)) {
> + goto irq_trans_out;
> + } else {
[Severity: High]
Could an unhandled interrupt cause an infinite IRQ storm and CPU lockup here?
When a transfer times out, aml_i2c_xfer_msg() resets the state to STATE_IDLE
but doesn't appear to disable the hardware threshold interrupts. If the
delayed interrupt fires, this handler logs a warning and returns IRQ_HANDLED
without clearing the interrupt source.
[ ... ]
> +static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg)
> +{
> + unsigned int reg_val;
> + unsigned int trans_len;
> +
> + trans_len = msg->len;
> + reg_val = (unsigned int)((msg->addr << 1) |
> + ((msg->flags & I2C_M_RD) ? START_READ : 0) |
> + ((trans_len & 0xfff) << 12) | START_START);
[Severity: High]
Does this silently truncate I2C message lengths greater than 4095 bytes?
The length is masked with 0xfff without returning an error to the caller,
which could result in silent data corruption for large transfers.
> + aml_i2c_set_mask(i2c, REG_CFG_START,
> + START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val);
> + clear_irq_status(i2c, IRQ_ALL_MASK);
[Severity: High]
Could clear_irq_status() inadvertently drop a valid completion interrupt?
If the hardware responds extremely fast (for example, generating a NACK on
the address byte) immediately after START_START is written, the completion
interrupt might get cleared before the handler has a chance to run, causing
the transfer to spuriously time out.
> + /* enable thresh mode */
> + if (trans_len > A9_I2C_FIFO_DEPTH) {
> + reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
> + aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE,
> + A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
[Severity: High]
Is it safe to perform a read-modify-write on REG_CGF_IRQ_ENABLE here without
holding i2c->lock? If a hardware interrupt asserts immediately after the
transfer starts, a concurrent aml_i2c_irq() running on another CPU might
also modify REG_CGF_IRQ_ENABLE, causing a data race that loses updates.
> + }
> +}
> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
> + unsigned long time_left, flags;
> + int ret = 0;
> +
> + i2c->msg = msg;
> + i2c->pos = 0;
> + i2c->count = 0;
> + i2c->error = 0;
> +
> + i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
> + aml_i2c_prepare_xfer(i2c, FIFO_FILL_FULL);
> + i2c->pos += i2c->count;
> + reinit_completion(&i2c->done);
[Severity: High]
Are these transfer state variables reinitialized without holding i2c->lock?
If a delayed interrupt from a previous timed-out transfer fires concurrently,
it might read this inconsistent state or falsely complete this new transfer,
potentially causing memory corruption or an invalid state machine transition.
> +
> + if (last)
> + aml_i2c_add_stop(i2c);
[Severity: High]
While this clears the BUS_NO_STOP bit for the final message in a transfer,
is there missing logic to restore it to 1 for non-last messages in new
transfers? Without setting it back, subsequent multi-message transfers might
incorrectly emit a STOP between messages, causing devices requiring Repeated
START to fail.
> + /* Start the transfer */
> + aml_i2c_do_start(i2c, msg);
> +
> + time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
> + time_left = wait_for_completion_timeout(&i2c->done, time_left);
> +
> + /*
> + * Protect access to i2c struct and registers from interrupt
> + * handlers triggered by a transfer terminated after the
> + * timeout period
> + */
> + spin_lock_irqsave(&i2c->lock, flags);
> +
> + if (!time_left) {
> + i2c->state = STATE_IDLE;
> + clear_irq_status(i2c, IRQ_ALL_MASK);
> + reset_fifo_pos(i2c);
> + ret = -ETIMEDOUT;
> + }
[Severity: High]
When a transfer times out, is it necessary to also disable the hardware
interrupts in REG_CGF_IRQ_ENABLE? As currently written, the hardware might
still assert a delayed interrupt once this function returns.
[ ... ]
> +static int aml_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct aml_i2c *i2c;
> + struct i2c_timings timings;
> + int irq, ret;
[ ... ]
> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "can't get device clock\n");
> + return PTR_ERR(i2c->clk);
> + }
> +
> + i2c->clk_rate = clk_get_rate(i2c->clk);
> + if (i2c->clk_rate == 0) {
> + dev_err(&pdev->dev, "failed to get clk rate\n");
> + return -EINVAL;
> + }
> +
> + ret = aml_i2c_init(i2c);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "fail init i2c\n");
> + return ret;
> + }
[Severity: High]
Does aml_i2c_init() attempt to write to hardware registers before the clock
is enabled? clk_prepare_enable() appears to be called further down in this
function. Accessing unclocked registers could trigger a synchronous external
abort on ARM and crash the system.
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "can't find IRQ\n");
> + return irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c);
[Severity: Medium]
Since the interrupt is requested using devres here, it will remain active
until the devres cleanup runs after aml_i2c_remove() returns. Could this
leave a vulnerable window during removal where a spurious interrupt might
attempt to read hardware registers after the clock has been disabled?
> + if (ret < 0) {
> + dev_err(&pdev->dev, "can't request IRQ\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(i2c->clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "can't prepare clock\n");
> + return ret;
> + }
> +
> + strscpy(i2c->adap.name, "Amlogic I2C adapter",
> + sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &aml_i2c_algorithm;
> + i2c->adap.dev.parent = &pdev->dev;
> + i2c->adap.dev.of_node = np;
> + i2c->adap.algo_data = i2c;
> +
> + ret = i2c_add_adapter(&i2c->adap);
[Severity: High]
Could the silent truncation of large transfers observed in aml_i2c_do_start()
be avoided by setting the quirks pointer on i2c->adap here? Populating the
quirks would allow the I2C core to properly enforce the 4095-byte hardware
limit and return an error to the caller natively.
[ ... ]
> +static void aml_i2c_remove(struct platform_device *pdev)
> +{
> + struct aml_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + clk_disable_unprepare(i2c->clk);
> +}
[Severity: Medium]
Is the clock being manually disabled here before the interrupt handler is
cleaned up? As mentioned in aml_i2c_probe(), the devm-managed IRQ is still
active at this point. A spurious interrupt arriving after this line could
crash the system by accessing unclocked registers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-a9-i2c-v1-0-b8ad9b46f4a8@amlogic.com?part=2
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-09-24 8:57 ` sashiko-bot
0 siblings, 0 replies; 24+ messages in thread
From: sashiko-bot @ 2026-09-24 8:57 UTC (permalink / raw)
To: Xianwei Zhao; +Cc: devicetree, neil.armstrong, linux-amlogic, robh, conor+dt
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] i2c: amlogic: hardware registers accessed before clock is enabled
- [High] i2c: amlogic: race condition drops completion interrupts
- [High] i2c: amlogic: silent truncation of large transfers
- [High] i2c: amlogic: broken Repeated START for subsequent transfers
- [High] i2c: amlogic: data race on REG_CGF_IRQ_ENABLE
- [High] i2c: amlogic: IRQ storm on transfer timeout
- [High] i2c: amlogic: race in lockless state initialization
- [Medium] i2c: amlogic: crash due to early clock disable in remove
--
commit 5e779e1911cc216efb8179fa9e2910973c79bb74
Author: Junyi Zhao <junyi.zhao@amlogic.com>
i2c: amlogic: Add Amlogic A9 I2C controller driver
Add support for the new I2C controller used by Amlogic A9 SoCs.
> diff --git a/drivers/i2c/busses/i2c-amlogic-a9.c b/drivers/i2c/busses/i2c-amlogic-a9.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-amlogic-a9.c
[ ... ]
> +static irqreturn_t aml_i2c_irq(int irqno, void *dev_id)
> +{
> + struct aml_i2c *i2c = dev_id;
> + int ret;
> + u32 rd_len, i;
> + u32 rd_val;
> +
> + spin_lock(&i2c->lock);
> + ret = controller_is_error(i2c);
> + if (ret) {
> + i2c->error = ret;
> + goto complete_irq_out;
> + }
> +
> + switch (i2c->state) {
> + case STATE_READ:
[ ... ]
> + case STATE_WRITE:
[ ... ]
> + case STATE_IDLE:
> + dev_warn(i2c->dev, "i2c irq triggered abnormally!!!, irq_state:0x%x,irq_en:0x%x\n",
> + readl(i2c->regs + REG_CGF_IRQ_STATE),
> + readl(i2c->regs + REG_CGF_IRQ_ENABLE));
> + break;
> + default:
> + break;
> + }
> +
> + if (!controller_is_done(i2c)) {
> + goto irq_trans_out;
> + } else {
[Severity: High]
Could an unhandled interrupt cause an infinite IRQ storm and CPU lockup here?
When a transfer times out, aml_i2c_xfer_msg() resets the state to STATE_IDLE
but doesn't appear to disable the hardware threshold interrupts. If the
delayed interrupt fires, this handler logs a warning and returns IRQ_HANDLED
without clearing the interrupt source.
[ ... ]
> +static void aml_i2c_do_start(struct aml_i2c *i2c, struct i2c_msg *msg)
> +{
> + unsigned int reg_val;
> + unsigned int trans_len;
> +
> + trans_len = msg->len;
> + reg_val = (unsigned int)((msg->addr << 1) |
> + ((msg->flags & I2C_M_RD) ? START_READ : 0) |
> + ((trans_len & 0xfff) << 12) | START_START);
[Severity: High]
Does this silently truncate I2C message lengths greater than 4095 bytes?
The length is masked with 0xfff without returning an error to the caller,
which could result in silent data corruption for large transfers.
> + aml_i2c_set_mask(i2c, REG_CFG_START,
> + START_START | START_SLAVE_ADDR | START_READ | START_LEN, reg_val);
> + clear_irq_status(i2c, IRQ_ALL_MASK);
[Severity: High]
Could clear_irq_status() inadvertently drop a valid completion interrupt?
If the hardware responds extremely fast (for example, generating a NACK on
the address byte) immediately after START_START is written, the completion
interrupt might get cleared before the handler has a chance to run, causing
the transfer to spuriously time out.
> + /* enable thresh mode */
> + if (trans_len > A9_I2C_FIFO_DEPTH) {
> + reg_val = (msg->flags & I2C_M_RD) ? A9_RX_THRESH_READ : A9_TX_THRESH_WRITE;
> + aml_i2c_set_mask(i2c, REG_CGF_IRQ_ENABLE,
> + A9_RX_THRESH_READ | A9_TX_THRESH_WRITE, reg_val);
[Severity: High]
Is it safe to perform a read-modify-write on REG_CGF_IRQ_ENABLE here without
holding i2c->lock? If a hardware interrupt asserts immediately after the
transfer starts, a concurrent aml_i2c_irq() running on another CPU might
also modify REG_CGF_IRQ_ENABLE, causing a data race that loses updates.
> + }
> +}
> +
> +static int aml_i2c_xfer_msg(struct aml_i2c *i2c, struct i2c_msg *msg, bool last)
> +{
> + unsigned long time_left, flags;
> + int ret = 0;
> +
> + i2c->msg = msg;
> + i2c->pos = 0;
> + i2c->count = 0;
> + i2c->error = 0;
> +
> + i2c->state = (msg->flags & I2C_M_RD) ? STATE_READ : STATE_WRITE;
> + aml_i2c_prepare_xfer(i2c, FIFO_FILL_FULL);
> + i2c->pos += i2c->count;
> + reinit_completion(&i2c->done);
[Severity: High]
Are these transfer state variables reinitialized without holding i2c->lock?
If a delayed interrupt from a previous timed-out transfer fires concurrently,
it might read this inconsistent state or falsely complete this new transfer,
potentially causing memory corruption or an invalid state machine transition.
> +
> + if (last)
> + aml_i2c_add_stop(i2c);
[Severity: High]
While this clears the BUS_NO_STOP bit for the final message in a transfer,
is there missing logic to restore it to 1 for non-last messages in new
transfers? Without setting it back, subsequent multi-message transfers might
incorrectly emit a STOP between messages, causing devices requiring Repeated
START to fail.
> + /* Start the transfer */
> + aml_i2c_do_start(i2c, msg);
> +
> + time_left = msecs_to_jiffies(I2C_TIMEOUT_MS);
> + time_left = wait_for_completion_timeout(&i2c->done, time_left);
> +
> + /*
> + * Protect access to i2c struct and registers from interrupt
> + * handlers triggered by a transfer terminated after the
> + * timeout period
> + */
> + spin_lock_irqsave(&i2c->lock, flags);
> +
> + if (!time_left) {
> + i2c->state = STATE_IDLE;
> + clear_irq_status(i2c, IRQ_ALL_MASK);
> + reset_fifo_pos(i2c);
> + ret = -ETIMEDOUT;
> + }
[Severity: High]
When a transfer times out, is it necessary to also disable the hardware
interrupts in REG_CGF_IRQ_ENABLE? As currently written, the hardware might
still assert a delayed interrupt once this function returns.
[ ... ]
> +static int aml_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct aml_i2c *i2c;
> + struct i2c_timings timings;
> + int irq, ret;
[ ... ]
> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "can't get device clock\n");
> + return PTR_ERR(i2c->clk);
> + }
> +
> + i2c->clk_rate = clk_get_rate(i2c->clk);
> + if (i2c->clk_rate == 0) {
> + dev_err(&pdev->dev, "failed to get clk rate\n");
> + return -EINVAL;
> + }
> +
> + ret = aml_i2c_init(i2c);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "fail init i2c\n");
> + return ret;
> + }
[Severity: High]
Does aml_i2c_init() attempt to write to hardware registers before the clock
is enabled? clk_prepare_enable() appears to be called further down in this
function. Accessing unclocked registers could trigger a synchronous external
abort on ARM and crash the system.
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "can't find IRQ\n");
> + return irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, irq, aml_i2c_irq, 0, dev_name(&pdev->dev), i2c);
[Severity: Medium]
Since the interrupt is requested using devres here, it will remain active
until the devres cleanup runs after aml_i2c_remove() returns. Could this
leave a vulnerable window during removal where a spurious interrupt might
attempt to read hardware registers after the clock has been disabled?
> + if (ret < 0) {
> + dev_err(&pdev->dev, "can't request IRQ\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(i2c->clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "can't prepare clock\n");
> + return ret;
> + }
> +
> + strscpy(i2c->adap.name, "Amlogic I2C adapter",
> + sizeof(i2c->adap.name));
> + i2c->adap.owner = THIS_MODULE;
> + i2c->adap.algo = &aml_i2c_algorithm;
> + i2c->adap.dev.parent = &pdev->dev;
> + i2c->adap.dev.of_node = np;
> + i2c->adap.algo_data = i2c;
> +
> + ret = i2c_add_adapter(&i2c->adap);
[Severity: High]
Could the silent truncation of large transfers observed in aml_i2c_do_start()
be avoided by setting the quirks pointer on i2c->adap here? Populating the
quirks would allow the I2C core to properly enforce the 4095-byte hardware
limit and return an error to the caller natively.
[ ... ]
> +static void aml_i2c_remove(struct platform_device *pdev)
> +{
> + struct aml_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> + clk_disable_unprepare(i2c->clk);
> +}
[Severity: Medium]
Is the clock being manually disabled here before the interrupt handler is
cleaned up? As mentioned in aml_i2c_probe(), the devm-managed IRQ is still
active at this point. A spurious interrupt arriving after this line could
crash the system by accessing unclocked registers.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-a9-i2c-v1-0-b8ad9b46f4a8@amlogic.com?part=2
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-09-24 8:39 ` Xianwei Zhao via B4 Relay
@ 2026-09-27 12:08 ` Andi Shyti
-1 siblings, 0 replies; 24+ messages in thread
From: Andi Shyti @ 2026-09-27 12:08 UTC (permalink / raw)
To: xianwei.zhao
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Xianwei,
...
> +config I2C_AMLOGIC_A9
> + tristate "Amlogic new I2C controller"
> + depends on ARCH_MESON || COMPILE_TEST
> + depends on COMMON_CLK
> + help
> + If you say yes to this option, support will be included for the
> + I2C interface on the new Amlogic family of SoCs.
> +
> +
Please, remove this extra line
> config I2C_MICROCHIP_CORE
> tristate "Microchip FPGA I2C controller"
> depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
...
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/delay.h>
Please sort the above in alphabetic order.
> +/* Amlogic I2C register map */
> +#define REG_CFG_RDY 0x00
> +#define REG_CFG_I2C 0x04
> +#define REG_CFG_START 0x08
> +#define REG_CFG_BUS 0x0c
> +#define REG_TX_RD_ADDR 0x10
> +#define REG_TX_WR_ADDR 0x14
> +#define REG_RX_RD_ADDR 0x18
> +#define REG_RX_WR_ADDR 0x1c
> +#define REG_CGF_TX 0x20
> +#define REG_CGF_RX 0x24
> +#define REG_CGF_IRQ_STATE 0x30
> +#define REG_CGF_IRQ_ENABLE 0x34
> +#define REG_SHAKE_BLK_CNT 0x38
> +
> +/* CFG RDY fields */
> +#define RDY_TEE_ONLY BIT(1)
> +#define RDY_IF BIT(0)
> +
> +/* CFG I2C fields */
> +#define I2C_RX_THR GENMASK(23, 16)
> +#define I2C_TX_THR GENMASK(15, 8)
> +
> +/* CFG BUS fields */
> +#define BUS_NO_STOP BIT(18)
> +#define BUS_SPEED_MODE BIT(17)
> +#define BUS_SLAVE_MODE BIT(16)
> +#define BUS_FILTER_MASK GENMASK(15, 12)
> +/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
> +#define BUS_RATIO_MASK GENMASK(11, 0)
> +
> +/* CFG START fields */
> +#define START_START BIT(31)
> +#define START_LEN GENMASK(23, 12)
> +#define START_LEN_SHIFT 12
> +#define START_SLAVE_ADDR GENMASK(10, 1)
> +#define START_READ BIT(0)
> +
> +/* CFG TX_RX fields */
> +#define TX_RX_EMPTY BIT(9)
> +#define TX_RX_FULL BIT(8)
> +#define TX_RX_DATA GENMASK(7, 0)
> +
> +/*CGF IRQ fields */
> +#define IRQ_ALL_MASK 0xffff
> +#define A9_NCK_ERROR BIT(0)
> +#define A9_RX_EMPTY BIT(1)
> +#define A9_RX_FULL BIT(2)
> +#define A9_TX_EMPTY BIT(3)
> +#define A9_TX_FULL BIT(4)
> +#define A9_RX_THRESH_READ BIT(5)
> +#define A9_TX_THRESH_WRITE BIT(6)
> +#define A9_PHY_DONE BIT(7)
> +#define A9_TASK_DONE BIT(8)
> +#define A9_ALL_DONE BIT(9)
> +
> +#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
> +#define A9_TRANS_ERROR (A9_NCK_ERROR)
> +#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
> +#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
> +
> +#define A9_I2C_FIFO_DEPTH 32
> +#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
> +
> +#define I2C_TIMEOUT_MS 500
> +
> +enum {
> + STATE_IDLE,
> + STATE_READ,
> + STATE_WRITE,
> +};
> +
> +enum fifo_fill_mode {
> + FIFO_FILL_FULL,
> + FIFO_FILL_HALF,
> +};
For all the enums and defines above, please use the prefix of the
driver name, A9, I guess.
...
> +static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
> +{
> + int i;
> +
> + /* this i2c module when trans 0 byte, must put at least 1.
> + */
Please use the kernel style commenting format, I think checkpatch
would have had raised this.
> + if (!i2c->msg->len) {
> + writel(0x00, i2c->regs + REG_CGF_TX);
> + return;
> + }
> +
> + for (i = 0; i < len; i++, buf++)
> + writel(*buf, i2c->regs + REG_CGF_TX);
> +}
> +
> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
> +{
> + bool write = !(i2c->msg->flags & I2C_M_RD);
> +
> + if (write) {
you can revert the logic here with if (!write) return; to save a
level of indentation.
> + if (mode == FIFO_FILL_FULL)
> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
> + else
> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
> + aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
> + }
> +}
...
> +static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct aml_i2c *i2c = adap->algo_data;
> + int i, ret = 0;
> +
> + for (i = 0; i < num; i++) {
> + ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
> + if (ret)
> + break;
you can save some code here by doing
for (...) {
int ret;
ret = aml_i2c_xfer_msg(...);
if (ret)
return ret;
}
return i;
> + }
> +
> + return ret ?: i;
> +}
...
> +static int aml_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct aml_i2c *i2c;
...
> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "can't get device clock\n");
> + return PTR_ERR(i2c->clk);
> + }
Please use return dev_err_probe(...);
Andi
> +
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-09-27 12:08 ` Andi Shyti
0 siblings, 0 replies; 24+ messages in thread
From: Andi Shyti @ 2026-09-27 12:08 UTC (permalink / raw)
To: xianwei.zhao
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Xianwei,
...
> +config I2C_AMLOGIC_A9
> + tristate "Amlogic new I2C controller"
> + depends on ARCH_MESON || COMPILE_TEST
> + depends on COMMON_CLK
> + help
> + If you say yes to this option, support will be included for the
> + I2C interface on the new Amlogic family of SoCs.
> +
> +
Please, remove this extra line
> config I2C_MICROCHIP_CORE
> tristate "Microchip FPGA I2C controller"
> depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
...
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/delay.h>
Please sort the above in alphabetic order.
> +/* Amlogic I2C register map */
> +#define REG_CFG_RDY 0x00
> +#define REG_CFG_I2C 0x04
> +#define REG_CFG_START 0x08
> +#define REG_CFG_BUS 0x0c
> +#define REG_TX_RD_ADDR 0x10
> +#define REG_TX_WR_ADDR 0x14
> +#define REG_RX_RD_ADDR 0x18
> +#define REG_RX_WR_ADDR 0x1c
> +#define REG_CGF_TX 0x20
> +#define REG_CGF_RX 0x24
> +#define REG_CGF_IRQ_STATE 0x30
> +#define REG_CGF_IRQ_ENABLE 0x34
> +#define REG_SHAKE_BLK_CNT 0x38
> +
> +/* CFG RDY fields */
> +#define RDY_TEE_ONLY BIT(1)
> +#define RDY_IF BIT(0)
> +
> +/* CFG I2C fields */
> +#define I2C_RX_THR GENMASK(23, 16)
> +#define I2C_TX_THR GENMASK(15, 8)
> +
> +/* CFG BUS fields */
> +#define BUS_NO_STOP BIT(18)
> +#define BUS_SPEED_MODE BIT(17)
> +#define BUS_SLAVE_MODE BIT(16)
> +#define BUS_FILTER_MASK GENMASK(15, 12)
> +/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
> +#define BUS_RATIO_MASK GENMASK(11, 0)
> +
> +/* CFG START fields */
> +#define START_START BIT(31)
> +#define START_LEN GENMASK(23, 12)
> +#define START_LEN_SHIFT 12
> +#define START_SLAVE_ADDR GENMASK(10, 1)
> +#define START_READ BIT(0)
> +
> +/* CFG TX_RX fields */
> +#define TX_RX_EMPTY BIT(9)
> +#define TX_RX_FULL BIT(8)
> +#define TX_RX_DATA GENMASK(7, 0)
> +
> +/*CGF IRQ fields */
> +#define IRQ_ALL_MASK 0xffff
> +#define A9_NCK_ERROR BIT(0)
> +#define A9_RX_EMPTY BIT(1)
> +#define A9_RX_FULL BIT(2)
> +#define A9_TX_EMPTY BIT(3)
> +#define A9_TX_FULL BIT(4)
> +#define A9_RX_THRESH_READ BIT(5)
> +#define A9_TX_THRESH_WRITE BIT(6)
> +#define A9_PHY_DONE BIT(7)
> +#define A9_TASK_DONE BIT(8)
> +#define A9_ALL_DONE BIT(9)
> +
> +#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
> +#define A9_TRANS_ERROR (A9_NCK_ERROR)
> +#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
> +#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
> +
> +#define A9_I2C_FIFO_DEPTH 32
> +#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
> +
> +#define I2C_TIMEOUT_MS 500
> +
> +enum {
> + STATE_IDLE,
> + STATE_READ,
> + STATE_WRITE,
> +};
> +
> +enum fifo_fill_mode {
> + FIFO_FILL_FULL,
> + FIFO_FILL_HALF,
> +};
For all the enums and defines above, please use the prefix of the
driver name, A9, I guess.
...
> +static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
> +{
> + int i;
> +
> + /* this i2c module when trans 0 byte, must put at least 1.
> + */
Please use the kernel style commenting format, I think checkpatch
would have had raised this.
> + if (!i2c->msg->len) {
> + writel(0x00, i2c->regs + REG_CGF_TX);
> + return;
> + }
> +
> + for (i = 0; i < len; i++, buf++)
> + writel(*buf, i2c->regs + REG_CGF_TX);
> +}
> +
> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
> +{
> + bool write = !(i2c->msg->flags & I2C_M_RD);
> +
> + if (write) {
you can revert the logic here with if (!write) return; to save a
level of indentation.
> + if (mode == FIFO_FILL_FULL)
> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
> + else
> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
> + aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
> + }
> +}
...
> +static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> + struct aml_i2c *i2c = adap->algo_data;
> + int i, ret = 0;
> +
> + for (i = 0; i < num; i++) {
> + ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
> + if (ret)
> + break;
you can save some code here by doing
for (...) {
int ret;
ret = aml_i2c_xfer_msg(...);
if (ret)
return ret;
}
return i;
> + }
> +
> + return ret ?: i;
> +}
...
> +static int aml_i2c_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct aml_i2c *i2c;
...
> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(i2c->clk)) {
> + dev_err(&pdev->dev, "can't get device clock\n");
> + return PTR_ERR(i2c->clk);
> + }
Please use return dev_err_probe(...);
Andi
> +
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
2026-09-27 12:08 ` Andi Shyti
@ 2026-09-29 5:53 ` Xianwei Zhao
-1 siblings, 0 replies; 24+ messages in thread
From: Xianwei Zhao @ 2026-09-29 5:53 UTC (permalink / raw)
To: Andi Shyti
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Andi,
Thanks for your review.
On 2026/9/27 20:08, Andi Shyti wrote:
> Hi Xianwei,
>
> ...
>
>> +config I2C_AMLOGIC_A9
>> + tristate "Amlogic new I2C controller"
>> + depends on ARCH_MESON || COMPILE_TEST
>> + depends on COMMON_CLK
>> + help
>> + If you say yes to this option, support will be included for the
>> + I2C interface on the new Amlogic family of SoCs.
>> +
>> +
> Please, remove this extra line
>
Will do.
>> config I2C_MICROCHIP_CORE
>> tristate "Microchip FPGA I2C controller"
>> depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
> ...
>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/delay.h>
> Please sort the above in alphabetic order.
>
Will do.
>> +/* Amlogic I2C register map */
>> +#define REG_CFG_RDY 0x00
>> +#define REG_CFG_I2C 0x04
>> +#define REG_CFG_START 0x08
>> +#define REG_CFG_BUS 0x0c
>> +#define REG_TX_RD_ADDR 0x10
>> +#define REG_TX_WR_ADDR 0x14
>> +#define REG_RX_RD_ADDR 0x18
>> +#define REG_RX_WR_ADDR 0x1c
>> +#define REG_CGF_TX 0x20
>> +#define REG_CGF_RX 0x24
>> +#define REG_CGF_IRQ_STATE 0x30
>> +#define REG_CGF_IRQ_ENABLE 0x34
>> +#define REG_SHAKE_BLK_CNT 0x38
>> +
>> +/* CFG RDY fields */
>> +#define RDY_TEE_ONLY BIT(1)
>> +#define RDY_IF BIT(0)
>> +
>> +/* CFG I2C fields */
>> +#define I2C_RX_THR GENMASK(23, 16)
>> +#define I2C_TX_THR GENMASK(15, 8)
>> +
>> +/* CFG BUS fields */
>> +#define BUS_NO_STOP BIT(18)
>> +#define BUS_SPEED_MODE BIT(17)
>> +#define BUS_SLAVE_MODE BIT(16)
>> +#define BUS_FILTER_MASK GENMASK(15, 12)
>> +/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
>> +#define BUS_RATIO_MASK GENMASK(11, 0)
>> +
>> +/* CFG START fields */
>> +#define START_START BIT(31)
>> +#define START_LEN GENMASK(23, 12)
>> +#define START_LEN_SHIFT 12
>> +#define START_SLAVE_ADDR GENMASK(10, 1)
>> +#define START_READ BIT(0)
>> +
>> +/* CFG TX_RX fields */
>> +#define TX_RX_EMPTY BIT(9)
>> +#define TX_RX_FULL BIT(8)
>> +#define TX_RX_DATA GENMASK(7, 0)
>> +
>> +/*CGF IRQ fields */
>> +#define IRQ_ALL_MASK 0xffff
>> +#define A9_NCK_ERROR BIT(0)
>> +#define A9_RX_EMPTY BIT(1)
>> +#define A9_RX_FULL BIT(2)
>> +#define A9_TX_EMPTY BIT(3)
>> +#define A9_TX_FULL BIT(4)
>> +#define A9_RX_THRESH_READ BIT(5)
>> +#define A9_TX_THRESH_WRITE BIT(6)
>> +#define A9_PHY_DONE BIT(7)
>> +#define A9_TASK_DONE BIT(8)
>> +#define A9_ALL_DONE BIT(9)
>> +
>> +#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
>> +#define A9_TRANS_ERROR (A9_NCK_ERROR)
>> +#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
>> +#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
>> +
>> +#define A9_I2C_FIFO_DEPTH 32
>> +#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
>> +
>> +#define I2C_TIMEOUT_MS 500
>> +
>> +enum {
>> + STATE_IDLE,
>> + STATE_READ,
>> + STATE_WRITE,
>> +};
>> +
>> +enum fifo_fill_mode {
>> + FIFO_FILL_FULL,
>> + FIFO_FILL_HALF,
>> +};
> For all the enums and defines above, please use the prefix of the
> driver name, A9, I guess.
>
Will add prefix A9
> ...
>
>> +static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
>> +{
>> + int i;
>> +
>> + /* this i2c module when trans 0 byte, must put at least 1.
>> + */
> Please use the kernel style commenting format, I think checkpatch
> would have had raised this.
Will do.
>
>> + if (!i2c->msg->len) {
>> + writel(0x00, i2c->regs + REG_CGF_TX);
>> + return;
>> + }
>> +
>> + for (i = 0; i < len; i++, buf++)
>> + writel(*buf, i2c->regs + REG_CGF_TX);
>> +}
>> +
>> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
>> +{
>> + bool write = !(i2c->msg->flags & I2C_M_RD);
>> +
>> + if (write) {
> you can revert the logic here with if (!write) return; to save a
> level of indentation.
>
Will do.
>> + if (mode == FIFO_FILL_FULL)
>> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
>> + else
>> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
>> + aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>> + }
>> +}
> ...
>
>> +static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> + struct aml_i2c *i2c = adap->algo_data;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i < num; i++) {
>> + ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
>> + if (ret)
>> + break;
> you can save some code here by doing
>
> for (...) {
> int ret;
>
> ret = aml_i2c_xfer_msg(...);
> if (ret)
> return ret;
> }
>
> return i;
>
Will do.
>> + }
>> +
>> + return ret ?: i;
>> +}
> ...
>
>> +static int aml_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct aml_i2c *i2c;
> ...
>
>> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(i2c->clk)) {
>> + dev_err(&pdev->dev, "can't get device clock\n");
>> + return PTR_ERR(i2c->clk);
>> + }
> Please use return dev_err_probe(...);
>
Will do.
> Andi
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver
@ 2026-09-29 5:53 ` Xianwei Zhao
0 siblings, 0 replies; 24+ messages in thread
From: Xianwei Zhao @ 2026-09-29 5:53 UTC (permalink / raw)
To: Andi Shyti
Cc: Junyi Zhao, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Xianwei Zhao, Junyi Zhao, linux-i2c, linux-amlogic, devicetree,
linux-kernel
Hi Andi,
Thanks for your review.
On 2026/9/27 20:08, Andi Shyti wrote:
> Hi Xianwei,
>
> ...
>
>> +config I2C_AMLOGIC_A9
>> + tristate "Amlogic new I2C controller"
>> + depends on ARCH_MESON || COMPILE_TEST
>> + depends on COMMON_CLK
>> + help
>> + If you say yes to this option, support will be included for the
>> + I2C interface on the new Amlogic family of SoCs.
>> +
>> +
> Please, remove this extra line
>
Will do.
>> config I2C_MICROCHIP_CORE
>> tristate "Microchip FPGA I2C controller"
>> depends on ARCH_MICROCHIP_POLARFIRE || COMPILE_TEST
> ...
>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/delay.h>
> Please sort the above in alphabetic order.
>
Will do.
>> +/* Amlogic I2C register map */
>> +#define REG_CFG_RDY 0x00
>> +#define REG_CFG_I2C 0x04
>> +#define REG_CFG_START 0x08
>> +#define REG_CFG_BUS 0x0c
>> +#define REG_TX_RD_ADDR 0x10
>> +#define REG_TX_WR_ADDR 0x14
>> +#define REG_RX_RD_ADDR 0x18
>> +#define REG_RX_WR_ADDR 0x1c
>> +#define REG_CGF_TX 0x20
>> +#define REG_CGF_RX 0x24
>> +#define REG_CGF_IRQ_STATE 0x30
>> +#define REG_CGF_IRQ_ENABLE 0x34
>> +#define REG_SHAKE_BLK_CNT 0x38
>> +
>> +/* CFG RDY fields */
>> +#define RDY_TEE_ONLY BIT(1)
>> +#define RDY_IF BIT(0)
>> +
>> +/* CFG I2C fields */
>> +#define I2C_RX_THR GENMASK(23, 16)
>> +#define I2C_TX_THR GENMASK(15, 8)
>> +
>> +/* CFG BUS fields */
>> +#define BUS_NO_STOP BIT(18)
>> +#define BUS_SPEED_MODE BIT(17)
>> +#define BUS_SLAVE_MODE BIT(16)
>> +#define BUS_FILTER_MASK GENMASK(15, 12)
>> +/* SCL = clk/b_ratio if b_ratio<=8, SCL = clk/8 */
>> +#define BUS_RATIO_MASK GENMASK(11, 0)
>> +
>> +/* CFG START fields */
>> +#define START_START BIT(31)
>> +#define START_LEN GENMASK(23, 12)
>> +#define START_LEN_SHIFT 12
>> +#define START_SLAVE_ADDR GENMASK(10, 1)
>> +#define START_READ BIT(0)
>> +
>> +/* CFG TX_RX fields */
>> +#define TX_RX_EMPTY BIT(9)
>> +#define TX_RX_FULL BIT(8)
>> +#define TX_RX_DATA GENMASK(7, 0)
>> +
>> +/*CGF IRQ fields */
>> +#define IRQ_ALL_MASK 0xffff
>> +#define A9_NCK_ERROR BIT(0)
>> +#define A9_RX_EMPTY BIT(1)
>> +#define A9_RX_FULL BIT(2)
>> +#define A9_TX_EMPTY BIT(3)
>> +#define A9_TX_FULL BIT(4)
>> +#define A9_RX_THRESH_READ BIT(5)
>> +#define A9_TX_THRESH_WRITE BIT(6)
>> +#define A9_PHY_DONE BIT(7)
>> +#define A9_TASK_DONE BIT(8)
>> +#define A9_ALL_DONE BIT(9)
>> +
>> +#define A9_TRANS_DONE (A9_PHY_DONE | A9_NCK_ERROR)
>> +#define A9_TRANS_ERROR (A9_NCK_ERROR)
>> +#define A9_ENABLE_IRQ_BIT (A9_NCK_ERROR | A9_PHY_DONE)
>> +#define THRESH_MODE (A9_RX_THRESH_READ | A9_TX_THRESH_WRITE)
>> +
>> +#define A9_I2C_FIFO_DEPTH 32
>> +#define A9_I2C_HALF_FIFO (A9_I2C_FIFO_DEPTH >> 1)
>> +
>> +#define I2C_TIMEOUT_MS 500
>> +
>> +enum {
>> + STATE_IDLE,
>> + STATE_READ,
>> + STATE_WRITE,
>> +};
>> +
>> +enum fifo_fill_mode {
>> + FIFO_FILL_FULL,
>> + FIFO_FILL_HALF,
>> +};
> For all the enums and defines above, please use the prefix of the
> driver name, A9, I guess.
>
Will add prefix A9
> ...
>
>> +static void aml_i2c_put_data(struct aml_i2c *i2c, char *buf, int len)
>> +{
>> + int i;
>> +
>> + /* this i2c module when trans 0 byte, must put at least 1.
>> + */
> Please use the kernel style commenting format, I think checkpatch
> would have had raised this.
Will do.
>
>> + if (!i2c->msg->len) {
>> + writel(0x00, i2c->regs + REG_CGF_TX);
>> + return;
>> + }
>> +
>> + for (i = 0; i < len; i++, buf++)
>> + writel(*buf, i2c->regs + REG_CGF_TX);
>> +}
>> +
>> +static void aml_i2c_prepare_xfer(struct aml_i2c *i2c, enum fifo_fill_mode mode)
>> +{
>> + bool write = !(i2c->msg->flags & I2C_M_RD);
>> +
>> + if (write) {
> you can revert the logic here with if (!write) return; to save a
> level of indentation.
>
Will do.
>> + if (mode == FIFO_FILL_FULL)
>> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_FIFO_DEPTH);
>> + else
>> + i2c->count = min(i2c->msg->len - i2c->pos, A9_I2C_HALF_FIFO);
>> + aml_i2c_put_data(i2c, i2c->msg->buf + i2c->pos, i2c->count);
>> + }
>> +}
> ...
>
>> +static int aml_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> +{
>> + struct aml_i2c *i2c = adap->algo_data;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i < num; i++) {
>> + ret = aml_i2c_xfer_msg(i2c, msgs + i, i == num - 1);
>> + if (ret)
>> + break;
> you can save some code here by doing
>
> for (...) {
> int ret;
>
> ret = aml_i2c_xfer_msg(...);
> if (ret)
> return ret;
> }
>
> return i;
>
Will do.
>> + }
>> +
>> + return ret ?: i;
>> +}
> ...
>
>> +static int aml_i2c_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct aml_i2c *i2c;
> ...
>
>> + i2c->clk = devm_clk_get(&pdev->dev, NULL);
>> + if (IS_ERR(i2c->clk)) {
>> + dev_err(&pdev->dev, "can't get device clock\n");
>> + return PTR_ERR(i2c->clk);
>> + }
> Please use return dev_err_probe(...);
>
Will do.
> Andi
^ permalink raw reply [flat|nested] 24+ messages in thread