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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy:

Sorry for my late reply.

On Wed, Dec 28, 2022 at 5:57 AM Andy Shevchenko <andy@xxxxxxxxxx> wrote:
>
> On Fri, Dec 23, 2022 at 05:00:51PM +0800, Binbin Zhou wrote:
> > This I2C module is integrated into the Loongson-2K SoCs and Loongson
> > LS7A bridge chip.
>
> ...
>
> > +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.

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?

Thanks.

Binbin



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



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux