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