diff mbox series

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

Message ID 9d3e4997519fd9ecebae6fd241148cc22d3fe04f.1671688961.git.zhoubinbin@loongson.cn
State Superseded
Headers show
Series i2c: ls2x: Add support for the Loongson-2K/LS7A I2C controller | expand

Commit Message

Binbin Zhou Dec. 23, 2022, 9 a.m. UTC
This I2C module is integrated into the Loongson-2K SoCs and Loongson
LS7A bridge chip.

Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn>
---
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ls2x.c | 366 ++++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ls2x.c

Comments

Andy Shevchenko Dec. 27, 2022, 9:57 p.m. UTC | #1
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
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;
Binbin Zhou Dec. 29, 2022, 7:31 a.m. UTC | #2
Hi Andy:

Sorry for my late reply.

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:
> > 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
>
>
>
Andy Shevchenko Dec. 29, 2022, 9:20 a.m. UTC | #3
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;
Andy Shevchenko Dec. 29, 2022, 9:23 a.m. UTC | #4
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.

> >                 }
> >         }
> > 
> >         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;
Binbin Zhou Dec. 30, 2022, 1:46 a.m. UTC | #5
Hi Andy:

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:

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)
                ret = ls2x_i2c_rx(priv, msg);
        else
                ret = ls2x_i2c_tx(priv, msg);

        if (ret < 0) {
                if (ret == -ETIMEDOUT) /* Fatel error. Needs reinit. */
                        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?

Thanks.
Binbin


>
> > >                 }
> > >         }
> > >
> > >         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
>
>
Andy Shevchenko Dec. 30, 2022, 8:26 a.m. UTC | #6
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;
Binbin Zhou Dec. 30, 2022, 8:49 a.m. UTC | #7
On Fri, Dec 30, 2022 at 4:27 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> 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.

Well, is_read is a bit redundant, I would use 'msg->flags & I2C_M_RD'
directly in the if judgment condition.

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

Got it.

In the meantime I'll double check the rest!

Thanks.
Binbin
>
> >                         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
>
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e50f9603d189..d80c938142f5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -761,6 +761,17 @@  config I2C_LPC2K
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-lpc2k.
 
+config I2C_LS2X
+	tristate "Loongson LS2X I2C adapter"
+	depends on MACH_LOONGSON64 || COMPILE_TEST
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C interface on the Loongson-2K SoCs and Loongson LS7A bridge
+	  chip.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called i2c-ls2x.
+
 config I2C_MLXBF
         tristate "Mellanox BlueField I2C controller"
         depends on MELLANOX_PLATFORM && ARM64
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e73cdb1d2b5a..0584fe160824 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -77,6 +77,7 @@  obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_JZ4780)	+= i2c-jz4780.o
 obj-$(CONFIG_I2C_KEMPLD)	+= i2c-kempld.o
 obj-$(CONFIG_I2C_LPC2K)		+= i2c-lpc2k.o
+obj-$(CONFIG_I2C_LS2X)		+= i2c-ls2x.o
 obj-$(CONFIG_I2C_MESON)		+= i2c-meson.o
 obj-$(CONFIG_I2C_MICROCHIP_CORE)	+= i2c-microchip-corei2c.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
diff --git a/drivers/i2c/busses/i2c-ls2x.c b/drivers/i2c/busses/i2c-ls2x.c
new file mode 100644
index 000000000000..665faa009c7c
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ls2x.c
@@ -0,0 +1,366 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson-2K/Loongson LS7A I2C master mode driver
+ *
+ * Copyright (C) 2013 Loongson Technology Corporation Limited.
+ * Copyright (C) 2014-2017 Lemote, Inc.
+ * Copyright (C) 2018-2022 Loongson Technology Corporation Limited.
+ *
+ * Originally written by liushaozong
+ * Rewritten for mainline by Binbin Zhou <zhoubinbin@loongson.cn>
+ */
+
+#include <linux/bits.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/iopoll.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/units.h>
+
+/* I2C Registers */
+#define I2C_LS2X_PRER		0x0 /* Freq Division Register(16 bits) */
+#define I2C_LS2X_CTR		0x2 /* Control Register */
+#define I2C_LS2X_TXR		0x3 /* Transport Data Register */
+#define I2C_LS2X_RXR		0x3 /* Receive Data Register */
+#define I2C_LS2X_CR		0x4 /* Command Control Register */
+#define I2C_LS2X_SR		0x4 /* State Register */
+
+/* Command Control Register Bit */
+#define LS2X_CR_START		BIT(7) /* Start signal */
+#define LS2X_CR_STOP		BIT(6) /* Stop signal */
+#define LS2X_CR_READ		BIT(5) /* Read signal */
+#define LS2X_CR_WRITE		BIT(4) /* Write signal */
+#define LS2X_CR_ACK		BIT(3) /* Response signal */
+#define LS2X_CR_IACK		BIT(0) /* Interrupt response signal */
+
+/* State Register Bit */
+#define LS2X_SR_NOACK		BIT(7) /* Receive NACK */
+#define LS2X_SR_BUSY		BIT(6) /* Bus busy state */
+#define LS2X_SR_AL		BIT(5) /* Arbitration lost */
+#define LS2X_SR_TIP		BIT(1) /* Transmission state */
+#define LS2X_SR_IF		BIT(0) /* Interrupt flag */
+
+/* Control Register Bit */
+#define LS2X_CTR_EN		BIT(7) /* 0: I2c frequency setting 1: Normal */
+#define LS2X_CTR_IEN		BIT(6) /* Enable i2c interrupt */
+#define LS2X_CTR_MST		BIT(5) /* 0: Slave mode 1: Master mode */
+#define CTR_FREQ_MASK		GENMASK(7, 6)
+#define CTR_READY_MASK		GENMASK(7, 5)
+
+/* The PCLK frequency from LPB */
+#define LS2X_I2C_PCLK_FREQ	(50 * HZ_PER_MHZ)
+
+/* The default bus frequency, which is an empirical value */
+#define LS2X_I2C_FREQ_STD	(33 * HZ_PER_KHZ)
+
+struct ls2x_i2c_priv {
+	struct i2c_adapter	adapter;
+	void __iomem		*base;
+	struct i2c_timings	i2c_t;
+	struct completion	cmd_complete;
+};
+
+/*
+ * Interrupt service routine.
+ * This gets called whenever an I2C interrupt occurs.
+ */
+static irqreturn_t ls2x_i2c_isr(int this_irq, void *dev_id)
+{
+	struct ls2x_i2c_priv *priv = dev_id;
+
+	if (!(readb(priv->base + I2C_LS2X_SR) & LS2X_SR_IF))
+		return IRQ_NONE;
+
+	writeb(LS2X_CR_IACK, priv->base + I2C_LS2X_CR);
+	complete(&priv->cmd_complete);
+	return IRQ_HANDLED;
+}
+
+/*
+ * The ls2x i2c controller supports standard mode and fast mode, so the
+ * maximum bus frequency is '400kHz'.
+ * The bus frequency is set to the empirical value of '33KHz' by default,
+ * but it can also be taken from ACPI or FDT for compatibility with more
+ * devices.
+ */
+static void ls2x_i2c_adjust_bus_speed(struct ls2x_i2c_priv *priv)
+{
+	struct i2c_timings *t = &priv->i2c_t;
+	struct device *dev = priv->adapter.dev.parent;
+	u32 acpi_speed = i2c_acpi_find_bus_speed(dev);
+
+	i2c_parse_fw_timings(dev, t, false);
+
+	if (acpi_speed || t->bus_freq_hz)
+		t->bus_freq_hz = max(t->bus_freq_hz, acpi_speed);
+	else
+		t->bus_freq_hz = LS2X_I2C_FREQ_STD;
+
+	/* Calculate and set i2c frequency */
+	writew(LS2X_I2C_PCLK_FREQ / (5 * t->bus_freq_hz) - 1,
+	       priv->base + I2C_LS2X_PRER);
+}
+
+static void ls2x_i2c_init(struct ls2x_i2c_priv *priv)
+{
+	/* Set i2c frequency setting mode and disable interrupts */
+	writeb(readb(priv->base + I2C_LS2X_CTR) & ~CTR_FREQ_MASK,
+	       priv->base + I2C_LS2X_CTR);
+
+	ls2x_i2c_adjust_bus_speed(priv);
+
+	/* Set i2c normal operating mode and enable interrupts */
+	writeb(readb(priv->base + I2C_LS2X_CTR) | CTR_READY_MASK,
+	       priv->base + I2C_LS2X_CTR);
+}
+
+static int ls2x_i2c_xfer_byte(struct ls2x_i2c_priv *priv, u8 txdata, u8 *rxdatap)
+{
+	u8 rxdata;
+	unsigned long time_left;
+
+	writeb(txdata, priv->base + I2C_LS2X_CR);
+
+	time_left = wait_for_completion_timeout(&priv->cmd_complete,
+						priv->adapter.timeout);
+	if (!time_left)
+		return -ETIMEDOUT;
+
+	rxdata = readb(priv->base + I2C_LS2X_SR);
+	if (rxdatap)
+		*rxdatap = rxdata;
+
+	return 0;
+}
+
+static int ls2x_i2c_send_byte(struct ls2x_i2c_priv *priv, u8 txdata)
+{
+	int ret;
+	u8 rxdata;
+
+	ret = ls2x_i2c_xfer_byte(priv, txdata, &rxdata);
+	if (ret)
+		return ret;
+
+	if (rxdata & LS2X_SR_AL)
+		return -EAGAIN;
+
+	if (rxdata & LS2X_SR_NOACK)
+		return -ENXIO;
+
+	return 0;
+}
+
+static int ls2x_i2c_stop(struct ls2x_i2c_priv *priv)
+{
+	u8 value;
+
+	writeb(LS2X_CR_STOP, priv->base + I2C_LS2X_CR);
+	return readb_poll_timeout(priv->base + I2C_LS2X_SR, value,
+				  !(value & LS2X_SR_BUSY), 100,
+				  jiffies_to_usecs(priv->adapter.timeout));
+}
+
+static int ls2x_i2c_start(struct ls2x_i2c_priv *priv, struct i2c_msg *msgs)
+{
+	reinit_completion(&priv->cmd_complete);
+
+	writeb(i2c_8bit_addr_from_msg(msgs), priv->base + I2C_LS2X_TXR);
+	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)
+{
+	int ret;
+	u8 rxdata;
+
+	while (len--) {
+		ret = ls2x_i2c_xfer_byte(priv,
+					 LS2X_CR_READ | (len ? 0 : LS2X_CR_ACK),
+					 &rxdata);
+		if (ret)
+			return ret;
+
+		*buf++ = readb(priv->base + I2C_LS2X_RXR);
+	}
+
+	return 0;
+}
+
+static int ls2x_i2c_tx(struct ls2x_i2c_priv *priv, u8 *buf, u16 len)
+{
+	int ret;
+
+	while (len--) {
+		writeb(*buf++, priv->base + I2C_LS2X_TXR);
+
+		ret = ls2x_i2c_send_byte(priv, LS2X_CR_WRITE);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+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;
+}
+
+static int ls2x_i2c_master_xfer(struct i2c_adapter *adap,
+				struct i2c_msg *msgs, int num)
+{
+	int ret;
+	struct i2c_msg *msg, *emsg = msgs + num;
+	struct ls2x_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	for (msg = msgs; msg < emsg; msg++) {
+		ret = ls2x_i2c_xfer_one(priv, msg, msg == emsg - 1);
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static unsigned int ls2x_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ls2x_i2c_algo = {
+	.master_xfer	= ls2x_i2c_master_xfer,
+	.functionality	= ls2x_i2c_func,
+};
+
+static int ls2x_i2c_probe(struct platform_device *pdev)
+{
+	int ret, irq;
+	struct i2c_adapter *adap;
+	struct ls2x_i2c_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* Map hardware registers */
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	/* Add the i2c adapter */
+	adap = &priv->adapter;
+	adap->nr = pdev->id;
+	adap->owner = THIS_MODULE;
+	adap->retries = 5;
+	adap->algo = &ls2x_i2c_algo;
+	adap->dev.parent = dev;
+	device_set_node(&adap->dev, dev_fwnode(dev));
+	i2c_set_adapdata(adap, priv);
+	strscpy(adap->name, pdev->name, sizeof(adap->name));
+	init_completion(&priv->cmd_complete);
+	platform_set_drvdata(pdev, priv);
+
+	ls2x_i2c_init(priv);
+
+	ret = devm_request_irq(dev, irq, ls2x_i2c_isr, IRQF_SHARED, "ls2x-i2c",
+			       priv);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Unable to request irq %d\n", irq);
+
+	return devm_i2c_add_adapter(dev, adap);
+}
+
+static int ls2x_i2c_suspend(struct device *dev)
+{
+	struct ls2x_i2c_priv *priv = dev_get_drvdata(dev);
+
+	/* Disable interrupts */
+	writeb(readb(priv->base + I2C_LS2X_CTR) & ~LS2X_CTR_IEN,
+	       priv->base + I2C_LS2X_CTR);
+
+	return 0;
+}
+
+static int ls2x_i2c_resume(struct device *dev)
+{
+	ls2x_i2c_init(dev_get_drvdata(dev));
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ls2x_i2c_pm_ops,
+				 ls2x_i2c_suspend, ls2x_i2c_resume, NULL);
+
+static const struct of_device_id ls2x_i2c_id_table[] = {
+	{ .compatible = "loongson,ls2k-i2c" },
+	{ .compatible = "loongson,ls7a-i2c" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ls2x_i2c_id_table);
+
+static const struct acpi_device_id ls2x_i2c_acpi_match[] = {
+	{ "LOON0004" }, /* Loongson LS7A */
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, ls2x_i2c_acpi_match);
+
+static struct platform_driver ls2x_i2c_driver = {
+	.probe		= ls2x_i2c_probe,
+	.driver		= {
+		.name	= "ls2x-i2c",
+		.pm	= pm_sleep_ptr(&ls2x_i2c_pm_ops),
+		.of_match_table = ls2x_i2c_id_table,
+		.acpi_match_table = ls2x_i2c_acpi_match,
+	},
+};
+
+/* The DC subsystem depends on it, we should initialize it earlier */
+static int __init ls2x_i2c_init_driver(void)
+{
+	return platform_driver_register(&ls2x_i2c_driver);
+}
+subsys_initcall(ls2x_i2c_init_driver);
+
+static void __exit ls2x_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&ls2x_i2c_driver);
+}
+module_exit(ls2x_i2c_exit_driver);
+
+MODULE_DESCRIPTION("Loongson LS2X I2C Bus driver");
+MODULE_AUTHOR("Loongson Technology Corporation Limited");
+MODULE_LICENSE("GPL");