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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

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