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

[Andi Shyti <andi.shyti@kernel.org>]

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

> +