Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Nov 28, 2022 at 09:00:24PM +0800, Yinbo Zhu wrote:
> This bus driver supports the Loongson i2c hardware controller in the
> Loongson platforms and supports to use DTS and ACPI framework to
> register i2c adapter device resources.
> 
> The Loongson i2c controller supports operating frequencty is 50MHZ
> and supports the maximum transmission rate is 400kbps.

...

> +static inline u8 i2c_readb(struct loongson_i2c_dev *dev, u8 offset)
> +{
> +	return readb(dev->base + offset);
> +}
> +
> +static inline void i2c_writeb(struct loongson_i2c_dev *dev, u8 val,
> +			      u8 offset)

For this you may turn parameters to be in more intuitive order, i.e.

static inline void i2c_writeb(struct loongson_i2c_dev *dev, u8 offset, u8 val)

(Also, why not on one line? Even with strict 80 it still fits).

> +{
> +	writeb(val, dev->base + offset);
> +}

...

> +static int loongson_i2c_start(struct loongson_i2c_dev *dev, int dev_addr,
> +			      int flags)
> +{
> +	int ret;
> +	unsigned long time_left;
> +	int retry = 5;

> +	unsigned char addr = LOONGSON_I2C_ADDR_A7(dev_addr) << 1;
> +
> +	addr |= (flags & I2C_M_RD) ? 1 : 0;

Why not i2c_8bit_addr_from_msg()?

> +	do {

> +		mdelay(1);

Needs an explanation why.

> +		i2c_writeb(dev, addr, LOONGSON_I2C_TXR_REG);
> +		i2c_writeb(dev, (CR_START | CR_WRITE | CR_IACK),
> +			   LOONGSON_I2C_CR_REG);
> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
> +			ret = loongson_i2c_stop(dev);
> +			if (ret)
> +				return ret;
> +		} else
> +			break;
> +	} while (retry--);
> +
> +	return 0;
> +}

...

> +	i2c_writeb(dev, 0xa0, LOONGSON_I2C_CTR_REG);

Magic number.

...

> +	if (!dev->speed_hz) {


Why not positive conditional?

> +		prer_val = 0x12c;
> +	} else {
> +		pclk = 50000000;

50 * HZ_PER_MHZ?

> +		prer_val = pclk / (5 * dev->speed_hz) - 1;
> +	}
> +
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CR_REG) |
> +		   0x01, LOONGSON_I2C_CR_REG);
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) & ~0x80,
> +		   LOONGSON_I2C_CTR_REG);

> +	i2c_writeb(dev, prer_val & GENMASK(7, 0), LOONGSON_I2C_PRER_LO_REG);
> +	i2c_writeb(dev, (prer_val & GENMASK(15, 8)) >> 8,

Why do you need GENMASK() parts?

> +		   LOONGSON_I2C_PRER_HI_REG);
> +	i2c_writeb(dev, i2c_readb(dev, LOONGSON_I2C_CTR_REG) |
> +		   0xe0, LOONGSON_I2C_CTR_REG);

A lot of magic numbers...

...

> +static int loongson_i2c_read(struct loongson_i2c_dev *dev, unsigned char *buf,
> +			     int count)
> +{
> +	int i;
> +	unsigned long time_left;
> +
> +	for (i = 0; i < count; i++) {

> +		i2c_writeb(dev, (i == count - 1) ?
> +			(CR_READ | CR_IACK | CR_ACK) : (CR_READ | CR_IACK),
> +			LOONGSON_I2C_CR_REG);

With temporary variable this will look better.

	u8 val = CR_READ | CR_IACK;
	...

		i2c_writeb(dev, (i == count - 1) ? val | CR_ACK : val,
			   LOONGSON_I2C_CR_REG);


Also fix wrong indentation.

> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		buf[i] = i2c_readb(dev, LOONGSON_I2C_RXR_REG);
> +	}
> +
> +	return i;
> +}

...

> +static int loongson_i2c_write(struct loongson_i2c_dev *dev, unsigned char *buf,
> +			      int count)
> +{
> +	int i;
> +	int ret;
> +	unsigned long time_left;
> +
> +	for (i = 0; i < count; i++) {
> +		i2c_writeb(dev, buf[i], LOONGSON_I2C_TXR_REG);
> +		i2c_writeb(dev, CR_WRITE | CR_IACK, LOONGSON_I2C_CR_REG);
> +		time_left = wait_for_completion_timeout(&dev->cmd_complete,
> +							dev->adapter.timeout);
> +		if (!time_left)
> +			return -ETIMEDOUT;
> +
> +		if (i2c_readb(dev, LOONGSON_I2C_SR_REG) & SR_NOACK) {
> +			ret = loongson_i2c_stop(dev);
> +			if (ret)
> +				return ret;
> +			return 0;
> +		}
> +	}
> +
> +	return i;

Can i be not equal to count here?

> +}

...

> +static int loongson_i2c_doxfer(struct loongson_i2c_dev *dev, struct i2c_msg *msgs,
> +			       int num)
> +{
> +	int i, ret;
> +	struct i2c_msg *m = msgs;
> +
> +	for (i = 0; i < num; i++) {
> +		reinit_completion(&dev->cmd_complete);
> +		ret = loongson_i2c_start(dev, m->addr, m->flags);
> +		if (ret)
> +			return ret;
> +
> +		if (m->flags & I2C_M_RD) {
> +			ret = loongson_i2c_read(dev, m->buf, m->len);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (!(m->flags & I2C_M_RD)) {
> +			ret = loongson_i2c_write(dev, m->buf, m->len);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		++m;
> +	}
> +
> +	ret = loongson_i2c_stop(dev);
> +	if (ret)
> +		return ret;
> +
> +	return i;

Can i be not equal to num here?

> +}

...

> +static int loongson_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> +			     int num)
> +{
> +	int ret;
> +	int retry;
> +	struct loongson_i2c_dev *dev;
> +
> +	dev = i2c_get_adapdata(adap);
> +	for (retry = 0; retry < adap->retries; retry++) {
> +		ret = loongson_i2c_doxfer(dev, msgs, num);
> +		if (ret != -EAGAIN)
> +			return ret;
> +
> +		udelay(100);
> +	}

Why udelay() and not usleep_range() ?
Why so long?

All these at least have to be explained.

> +	return -EREMOTEIO;
> +}

Why not utilizing something from iopoll.h?

...

> +	if (dev->slave_state == LOONGSON_I2C_SLAVE_START)
> +		if (stat & SR_SLAVE_RW)
> +			dev->slave_state =
> +				LOONGSON_I2C_SLAVE_READ_REQUESTED;
> +		else
> +			dev->slave_state =
> +				LOONGSON_I2C_SLAVE_WRITE_REQUESTED;

Even with strict 80 rule, these are fine to be on one line.

I suggest you to go through the code and shrink it by 20-30 LoCs. It seems
feasible taking into account this kind of indentation.

...

> +static irqreturn_t loongson_i2c_isr(int this_irq, void *dev_id)
> +{
> +	unsigned char iflag;
> +	struct loongson_i2c_dev *dev = dev_id;
> +
> +	iflag = i2c_readb(dev, LOONGSON_I2C_SR_REG);

> +	if (iflag & SR_IF) {


Why not using the usual pattern, i.e.

	if (!(...))
		return IRQ_NONE;

?

It seems you ignored some of my comments...
I stopped here, please check what was given against v1 and try again.

-- 
With Best Regards,
Andy Shevchenko