Hi Andy:
On Mon, Dec 19, 2022 at 9:04 PM Andy Shevchenko <andy@xxxxxxxxxx> wrote:
>
> On Mon, Dec 19, 2022 at 08:28:33PM +0800, Binbin Zhou wrote:
> > This I2C module is integrated into the Loongson-2K SoCs and Loongson
> > LS7A bridge chip.
>
> Almost there, see my comments below (note, you have ~1w of time before this
> can be applied anyway, so take you time for carefully addressing comments,
> (re-)testing, etc.).
>
> ...
>
> > @@ -888,6 +888,17 @@ config I2C_OWL
> > Say Y here if you want to use the I2C bus controller on
> > the Actions Semiconductor Owl SoC's.
> >
> > +config I2C_LS2X
>
> I believe in Latin alphabet L goes before O...
Sorry, I will fix it.
>
> > + tristate "Loongson LS2X I2C adapter"
> > + depends on MACH_LOONGSON64 || COMPILE_TEST
> > + help
> > + If you say yes to this option, support will be included for the
> > + I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
> > + chip.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called i2c-ls2x.
> > +
> > config I2C_PASEMI
> > tristate "PA Semi SMBus interface"
> > depends on PPC_PASEMI && PCI
>
> ...
>
> > obj-$(CONFIG_I2C_MXS) += i2c-mxs.o
> > obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o
> > obj-$(CONFIG_I2C_NPCM) += i2c-npcm7xx.o
> > +obj-$(CONFIG_I2C_LS2X) += i2c-ls2x.o
>
> ...and even before n and m.
>
> > obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o
> > obj-$(CONFIG_I2C_OMAP) += i2c-omap.o
> > obj-$(CONFIG_I2C_OWL) += i2c-owl.o
>
> ...
>
> > +static int ls2x_i2c_xfer_one(struct i2c_adapter *adap,
> > + struct i2c_msg *msg, bool stop)
> > +{
> > + int ret;
> > + bool is_read = msg->flags & I2C_M_RD;
> > + struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
> > +
> > + /* Contains steps to send start condition and address */
> > + ret = ls2x_i2c_start(adap, msg);
> > + if (!ret) {
> > + if (is_read)
> > + ret = ls2x_i2c_rx(adap, msg->buf, msg->len);
> > + else
> > + ret = ls2x_i2c_tx(adap, msg->buf, msg->len);
>
> > + if (!ret && stop)
> > + ret = ls2x_i2c_stop(adap);
>
> So, we will send stop here...
>
> > + }
>
> > + if (ret == -ENXIO)
> > + ls2x_i2c_stop(adap);
>
> ...and if it fails, we send it again here. Is it okay?
First of all, I think the logic here is fine:
In the first case, stop is called when the last msg is transmitted successfully;
In the second case, stop is called when there is a NOACK during the
transmission;
In the third case, init is called when other errors occur during the
transmission, such as TIMEOUT.
The key point is that the current implementation of the stop function
is probably imperfect, it directly ignores the NOACK. The more
reasonable method would be to judge by LS2X_SR_BUSY(bus busy state).
The basic idea: if the i2c bus is detected as idle within the timeout
range of the stop command issued, the stop command is considered
successful, otherwise -ETIMEDOUT is returned.
As follows:
static int ls2x_i2c_stop(struct i2c_adapter *adap)
{
- int ret = ls2x_i2c_send_byte(adap, LS2X_CR_STOP);
+ u8 value;
+ struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
- return (ret == -ENXIO) ? 0 : ret;
+ writeb(LS2X_CR_STOP, priv->base + I2C_LS2X_CR);
+ return readl_poll_timeout(priv->base + I2C_LS2X_SR,
+ value, !(value & LS2X_SR_BUSY),
+ 100, jiffies_to_usecs(adap->timeout));
}
Now, in the case you mentioned above, the last msg transmission is
completed and the stop command is sent. If the stop command fails, it
will only return -ETIMEDOUT and call ls2x_i2c_init().
Of course, I will use tools like i2c-tools to do more tests.
Thanks for the detailed review.
Binbin
>
> > + else if (ret < 0)
> > + ls2x_i2c_init(priv);
> > +
> > + return ret;
> > +}
>
> ...
>
> > + ret = ls2x_i2c_xfer_one(adap, msg, msg == (emsg - 1));
>
> Too many parentheses, isn't it?
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + r = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c",
> > + priv);
>
> Everywhere else you use 'ret', why is 'r' here?
>
> > + if (r < 0)
> > + return dev_err_probe(dev, r, "Unable to request irq %d\n", irq);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
