* [PATCH 0/3] i2c: Add driver support Amlogic A9 I2C
@ 2026-09-24 8:39 Xianwei Zhao via B4 Relay
2026-09-24 8:39 ` [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 6+ 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
Add a Device Tree binding and driver for the I2C controller found on the
Amlogic A9 SoC.
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Junyi Zhao (1):
i2c: amlogic: Add Amlogic A9 I2C controller driver
Xianwei Zhao (2):
dt-bindings: i2c: add Amlogic A9 I2C controller
MAINTAINERS: Add an entry for Amlogic I2C driver
.../devicetree/bindings/i2c/amlogic,a9-i2c.yaml | 61 +++
MAINTAINERS | 8 +
drivers/i2c/busses/Kconfig | 9 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-amlogic-a9.c | 504 +++++++++++++++++++++
5 files changed, 583 insertions(+)
---
base-commit: e817a366cd24ddc8f986ef43c5d54cc3f60b0c21
change-id: 20260911-a9-i2c-24478e22e683
Best regards,
--
Xianwei Zhao <xianwei.zhao@amlogic.com>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller 2026-09-24 8:39 [PATCH 0/3] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay @ 2026-09-24 8:39 ` Xianwei Zhao via B4 Relay 2026-09-24 8:46 ` sashiko-bot 2026-09-24 8:39 ` [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay 2026-09-24 8:39 ` [PATCH 3/3] MAINTAINERS: Add an entry for Amlogic I2C driver Xianwei Zhao via B4 Relay 2 siblings, 1 reply; 6+ 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: Xianwei Zhao <xianwei.zhao@amlogic.com> Add a Device Tree binding for the I2C controller found on the Amlogic A9 SoC. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- .../devicetree/bindings/i2c/amlogic,a9-i2c.yaml | 61 ++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml new file mode 100644 index 000000000000..681241a52680 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml @@ -0,0 +1,61 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/i2c/amlogic,a9-i2c.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic A9 I2C Controller + +maintainers: + - Xianwei Zhao <xianwei.zhaol@amlogic.com> + - Junyi Zhao <junyi.zhaol@amlogic.com> + +allOf: + - $ref: /schemas/i2c/i2c-controller.yaml# + +properties: + compatible: + const: amlogic,a9-i2c + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 1 + + dmas: + items: + - description: DMA controller phandle and request line for RX + - description: DMA controller phandle and request line for TX + + dma-names: + items: + - const: rx + - const: tx + +required: + - compatible + - reg + - interrupts + - clocks + +unevaluatedProperties: false + +examples: + - | + i2c@3a4400 { + compatible = "amlogic,a9-i2c"; + reg = <0x3a4400 0x3c>; + interrupts = <1 164 1>; + clocks = <&clkc_periphs 11>; + #address-cells = <1>; + #size-cells = <0>; + + eeprom@52 { + compatible = "atmel,24c32"; + reg = <0x52>; + }; + }; -- 2.52.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller 2026-09-24 8:39 ` [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay @ 2026-09-24 8:46 ` sashiko-bot 0 siblings, 0 replies; 6+ messages in thread From: sashiko-bot @ 2026-09-24 8:46 UTC (permalink / raw) To: Xianwei Zhao; +Cc: linux-amlogic, conor+dt, robh, devicetree, neil.armstrong Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] dt-bindings: i2c: amlogic: misspelled maintainer email addresses - [Low] dt-bindings: i2c: amlogic: invalid GIC interrupt type in example -- commit 1ce7d70f157b5cc3e10c7e75c64287c2dbe2c1bb Author: Xianwei Zhao <xianwei.zhao@amlogic.com> dt-bindings: i2c: add Amlogic A9 I2C controller Add a Device Tree binding for the I2C controller found on the Amlogic A9 SoC. > diff --git a/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml > new file mode 100644 > index 0000000000000..681241a526806 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/i2c/amlogic,a9-i2c.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Amlogic A9 I2C Controller > + > +maintainers: > + - Xianwei Zhao <xianwei.zhaol@amlogic.com> > + - Junyi Zhao <junyi.zhaol@amlogic.com> [Severity: Low] This isn't a functional bug, but are these email addresses misspelled? The trailing 'l' in 'zhaol' causes emails to bounce when contributors use get_maintainer.pl to CC the maintainers on patches modifying this binding. [ ... ] > +examples: > + - | > + i2c@3a4400 { > + compatible = "amlogic,a9-i2c"; > + reg = <0x3a4400 0x3c>; > + interrupts = <1 164 1>; [Severity: Low] Is this GIC interrupt specifier in the examples block correct? The first cell value of 1 designates a GIC_PPI (Per-Processor Interrupt), which is limited to the range 0-15. A value of 164 is highly invalid for a PPI. Since I2C controllers are typically shared peripherals, should this be 0 (GIC_SPI) instead? Developers copy-pasting the example will create invalid device trees that fail at runtime or during dtc validation. > + clocks = <&clkc_periphs 11>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + eeprom@52 { > + compatible = "atmel,24c32"; > + reg = <0x52>; > + }; > + }; -- Sashiko AI review · https://sashiko.dev/#/patchset/20260924-a9-i2c-v1-0-b8ad9b46f4a8@amlogic.com?part=1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver 2026-09-24 8:39 [PATCH 0/3] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay 2026-09-24 8:39 ` [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay @ 2026-09-24 8:39 ` Xianwei Zhao via B4 Relay 2026-09-24 8:57 ` sashiko-bot 2026-09-24 8:39 ` [PATCH 3/3] MAINTAINERS: Add an entry for Amlogic I2C driver Xianwei Zhao via B4 Relay 2 siblings, 1 reply; 6+ 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 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver 2026-09-24 8:39 ` [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay @ 2026-09-24 8:57 ` sashiko-bot 0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* [PATCH 3/3] MAINTAINERS: Add an entry for Amlogic I2C driver 2026-09-24 8:39 [PATCH 0/3] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay 2026-09-24 8:39 ` [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay 2026-09-24 8:39 ` [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay @ 2026-09-24 8:39 ` Xianwei Zhao via B4 Relay 2 siblings, 0 replies; 6+ 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: Xianwei Zhao <xianwei.zhao@amlogic.com> Add Amlogic I2C controller entry to MAINTAINERS to clarify the maintainers. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 6215fcb07770..33c39ecf535f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1371,6 +1371,14 @@ F: Documentation/devicetree/bindings/perf/amlogic,g12-ddr-pmu.yaml F: drivers/perf/amlogic/ F: include/soc/amlogic/ +AMLOGIC I2C DRIVER +M: Junyi Zhao <junyi.zhao@amlogic.com> +L: linux-i2c@vger.kernel.org +L: linux-amlogic@lists.infradead.org +S: Maintained +F: Documentation/devicetree/bindings/i2c/amlogic,a9-i2c.yaml +F: drivers/i2c/busses/i2c-amlogic-a9.c + AMLOGIC ISP DRIVER M: Keke Li <keke.li@amlogic.com> L: linux-media@vger.kernel.org -- 2.52.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-24 8:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-24 8:39 [PATCH 0/3] i2c: Add driver support Amlogic A9 I2C Xianwei Zhao via B4 Relay 2026-09-24 8:39 ` [PATCH 1/3] dt-bindings: i2c: add Amlogic A9 I2C controller Xianwei Zhao via B4 Relay 2026-09-24 8:46 ` sashiko-bot 2026-09-24 8:39 ` [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver Xianwei Zhao via B4 Relay 2026-09-24 8:57 ` sashiko-bot 2026-09-24 8:39 ` [PATCH 3/3] MAINTAINERS: Add an entry for Amlogic I2C driver Xianwei Zhao via B4 Relay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®