Re: [PATCH 2/3] i2c: amlogic: Add Amlogic A9 I2C controller driver

[Xianwei Zhao]

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