Re: [PATCH V7 3/4] i2c: ls2x: Add driver for Loongson-2K/LS7A I2C controller

[Andy Shevchenko <andy@kernel.org>]

On Tue, Dec 20, 2022 at 07:47:25PM +0800, Binbin Zhou wrote:
> On Mon, Dec 19, 2022 at 9:04 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Mon, Dec 19, 2022 at 08:28:33PM +0800, Binbin Zhou wrote:

...

> > > +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.

This is not obvious from the code above. Maybe the comment is missing, maybe as
you said the function should be amended and the code is refactored accordingly.

> 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.

You're welcome!

> > > +     else if (ret < 0)
> > > +             ls2x_i2c_init(priv);
> > > +
> > > +     return ret;
> > > +}

-- 
With Best Regards,
Andy Shevchenko