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

[Andy Shevchenko <andy@kernel.org>]

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;
>         }
> 
>         if (stop) {
>                 ret2 = ls2x_i2c_stop(priv);
> 
>                 if (ret2) {
>                         /* Failed to issue STOP. Needs reinit. */
>                         reinit = true;
>                         ret = ret ?: ret2;
>                 }
>         }
> 
>         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