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

[Andy Shevchenko <andy@kernel.org>]

On Fri, Dec 30, 2022 at 09:46:25AM +0800, Binbin Zhou wrote:
> On Thu, Dec 29, 2022 at 5:23 PM Andy Shevchenko <andy@kernel.org> wrote:
> >
> > On Thu, Dec 29, 2022 at 11:20:01AM +0200, Andy Shevchenko wrote:
> > > On Thu, Dec 29, 2022 at 03:31:47PM +0800, Binbin Zhou wrote:
> > > > On Wed, Dec 28, 2022 at 5:57 AM Andy Shevchenko <andy@kernel.org> wrote:
> > > > > On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote:

...

> > > > > > +static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv,
> > > > > > +                          struct i2c_msg *msg, bool stop)
> > > > > > +{
> > > > > > +     int ret;
> > > > > > +     bool is_read = msg->flags & I2C_M_RD;
> > > > > > +
> > > > > > +     /* Contains steps to send start condition and address */
> > > > > > +     ret = ls2x_i2c_start(priv, msg);
> > > > > > +     if (!ret) {
> > > > > > +             if (is_read)
> > > > > > +                     ret = ls2x_i2c_rx(priv, msg->buf, msg->len);
> > > > > > +             else
> > > > > > +                     ret = ls2x_i2c_tx(priv, msg->buf, msg->len);
> > > > > > +
> > > > > > +             if (!ret && stop)
> > > > > > +                     ret = ls2x_i2c_stop(priv);
> > > > > > +     }
> > > > > > +
> > > > > > +     if (ret == -ENXIO)
> > > > > > +             ls2x_i2c_stop(priv);
> > > > > > +     else if (ret < 0)
> > > > > > +             ls2x_i2c_init(priv);
> > > > > > +
> > > > > > +     return ret;
> > > > > > +}
> > > > >
> > > > > Still this code is odd from reader's perspective. It's in particular not clear
> > > > > if the stop can be called twice in a row. I recommend to split it to two
> > > >
> > > > Sorry,
> > > > Actually, I don't quite understand why you keep thinking that the stop
> > > > can be called twice in a row.
> > >
> > > Because nothing in the code suggests otherwise. You need deeply understand
> > > the flow to ensure that it won't. This means that the code is fragile and
> > > needs refactoring (even comment, which you can do a least won't help, because
> > > changing code in the other parts may break all this and you won't notice it).
> > >
> > > > As I said in my last email, the logic here should be:
> > > > 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 pointer is the stop function will only return a TIMEOUT error
> > > > or 0 for success, so if the stop function above is failed, the stop
> > > > function below will never be called twice.
> > > >
> > > > Anyway, I also admit that this part of the code may not be concise and
> > > > clear enough, and I have tried the following changes:
> > > >
> > > > 1. put the start function into the rx/tx function respectively. As followers:
> > > >
> > > > @@ -177,10 +177,16 @@ static int ls2x_i2c_start(struct ls2x_i2c_priv
> > > > *priv, struct i2c_msg *msgs)
> > > >         return ls2x_i2c_send_byte(priv, LS2X_CR_START | LS2X_CR_WRITE);
> > > >  }
> > > >
> > > > -static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len)
> > > > +static int ls2x_i2c_rx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg)
> > > >  {
> > > >         int ret;
> > > > -       u8 rxdata;
> > > > +       u8 rxdata, *buf = msg->buf;
> > > > +       u16 len = msg->len;
> > > > +
> > > > +       /* Contains steps to send start condition and address */
> > > > +       ret = ls2x_i2c_start(priv, msg);
> > > > +       if (ret)
> > > > +               return ret;
> > > >
> > > >         while (len--) {
> > > >                 ret = ls2x_i2c_xfer_byte(priv,
> > > > @@ -195,9 +201,16 @@ static int ls2x_i2c_rx(struct ls2x_i2c_priv
> > > > *priv, u8 *buf, u16 len)
> > > >         return 0;
> > > >  }
> > > >
> > > > -static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len)
> > > > +static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, struct i2c_msg *msg)
> > > >  {
> > > >         int ret;
> > > > +       u8 *buf = msg->buf;
> > > > +       u16 len = msg->len;
> > > > +
> > > > +       /* Contains steps to send start condition and address */
> > > > +       ret = ls2x_i2c_start(priv, msg);
> > > > +       if (ret)
> > > > +               return ret;
> > > >
> > > >         while (len--) {
> > > >                 writeb(*buf++, priv->base + I2C_LS2X_TXR);
> > > >
> > > > 2. define the variable 'reinit' in the xfer_one function to mark the
> > > > cases where reinit is needed. As follows:
> > > >
> > > > static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv,
> > > >                              struct i2c_msg *msg, bool stop)
> > > > {
> > > >         int ret, ret2;
> > > >         bool reinit = false;
> > > >         bool is_read = msg->flags & I2C_M_RD;
> > > >
> > > >         if (is_read)
> > > >                 ret = ls2x_i2c_rx(priv, msg);
> > > >         else
> > > >                 ret = ls2x_i2c_tx(priv, msg);
> > > >
> > > >         if (ret == -EAGAIN) /* could not acquire bus. bail out without STOP */
> > > >                 return ret;
> > > >
> > > >         if (ret == -ETIMEDOUT) {
> > > >                 /* Fatal error. Needs reinit. */
> > > >                 stop = false;
> > > >                 reinit = true;
> >
> > Why do you need to initialize stop here?
> > Why not to call reinit here and bailout?
> >
> > > >         }
> > > >
> > > >         if (stop) {
> > > >                 ret2 = ls2x_i2c_stop(priv);
> > > >
> > > >                 if (ret2) {
> > > >                         /* Failed to issue STOP. Needs reinit. */
> > > >                         reinit = true;
> > > >                         ret = ret ?: ret2;
> >
> > All the same, try to be less verbose with unneeded variables.
> 
> Ok, the reinit and ret2 variables seem to be a bit redundant, I will
> remove them.
> 
>  I will divide the whole thing into two parts:
> The first part is to handle errors: if ret < 0, return ret directly.
> One of the special handling is the fatal error timeout, which requires
> reinit.
> 
>         if (ret < 0) {
>                 if (ret == -ETIMEDOUT) /* Fatel error. Needs reinit. */
>                         ls2x_i2c_init(priv);
>                 return ret;
>         }
> 
> The second part is to handle the final stop command: it should be
> noted that if the stop command fails, reinit is also required.
> 
>         if (stop) {
>                 /* Failed to issue STOP. Needs reinit. */
>                 ret = ls2x_i2c_stop(priv);
>                 if (ret)
>                         ls2x_i2c_init(priv);
>         }
> 
> The complete code is as follows:

This looks much better!
See a couple of nit-picks below.

> static int ls2x_i2c_xfer_one(struct ls2x_i2c_priv *priv,
>                              struct i2c_msg *msg, bool stop)
> {
>         int ret;

>         bool is_read = msg->flags & I2C_M_RD;
> 
>         if (is_read)

With this you don't need to have is_read variable.

>                 ret = ls2x_i2c_rx(priv, msg);
>         else
>                 ret = ls2x_i2c_tx(priv, msg);
> 
>         if (ret < 0) {
>                 if (ret == -ETIMEDOUT) /* Fatel error. Needs reinit. */

Split comment and code, so the comment is followed by the code.

>                         ls2x_i2c_init(priv);
>                 return ret;
>         }
> 
>         if (stop) {
>                 /* Failed to issue STOP. Needs reinit. */
>                 ret = ls2x_i2c_stop(priv);
>                 if (ret)
>                         ls2x_i2c_init(priv);
>         }
> 
>         return ret;
> }
> 
> Do you think this is better?

> > > >                 }
> > > >         }
> > > >
> > > >         if (reinit)
> > > >                 ls2x_i2c_init(priv);
> > > >
> > > >         return ret;
> > > > }
> > > >
> > > > Do you think this is better?
> > >
> > > Slightly, but still twisted at the end with the play of error codes. Try to
> > > make it even more clear.
> > >
> > > > > functions and then do something like
> > > > >
> > > > > _read_one()
> > > > > {
> > > > >         ret = start();
> > > > >         if (ret)
> > > > >                 goto _stop; // Do we really need this?
> > > > >
> > > > >                 ret = rx();
> > > > >                 if (ret)
> > > > >                         goto _stop; // Do we need this?
> > > > >
> > > > >                 /* By setting this call the stop */
> > > > >                 if (stop)
> > > > >                         ret = -ENXIO;
> > > > >
> > > > >         out_send_stop:
> > > > >                 if (ret == ...)
> > > > >                         return _stop();
> > > > >                 // I don't like above, so this error checking/setting parts
> > > > >                 // also can be rethought and refactored accordingly
> > > > >
> > > > >                 return ret;
> > > > > }
> > > > >
> > > > >
> > > > >         if (is_read)
> > > > >                 ret = _read_one();
> > > > >         else
> > > > >                 ret = _write_one();
> > > > >
> > > > >         if (ret)
> > > > >                 _init();
> > > > >
> > > > >         return ret;

-- 
With Best Regards,
Andy Shevchenko