Re: [PATCH v2 1/2] i2c: loongson: add bus driver for the loongson i2c controller
From: Andy Shevchenko
Date: Mon Nov 28 2022 - 09:02:53 EST
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