| Message ID | 20240911154820.2846187-9-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: isch: Put the driver into shape | expand |
Hi Andy, ... > @@ -83,7 +80,6 @@ static int sch_transaction(struct i2c_adapter *adap) > struct sch_i2c *priv = container_of(adap, struct sch_i2c, adapter); > int temp; > int result = 0; > - int retries = 0; > > dev_dbg(&adap->dev, > "Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n", > @@ -112,15 +108,11 @@ static int sch_transaction(struct i2c_adapter *adap) > temp |= 0x10; > sch_io_wr8(priv, SMBHSTCNT, temp); > > - do { > - usleep_range(100, 200); > - temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f; > - } while ((temp & 0x08) && (retries++ < MAX_RETRIES)); > - > + result = read_poll_timeout(sch_io_rd8, temp, !(temp & 0x08), 200, 500000, true, > + priv, SMBHSTSTS); > /* If the SMBus is still busy, we give up */ > - if (retries > MAX_RETRIES) { > + if (result) { > dev_err(&adap->dev, "SMBus Timeout!\n"); > - result = -ETIMEDOUT; > } else if (temp & 0x04) { > result = -EIO; > dev_dbg(&adap->dev, "Bus collision! SMBus may be locked until next hard reset. (sorry!)\n"); > @@ -130,7 +122,7 @@ static int sch_transaction(struct i2c_adapter *adap) > dev_err(&adap->dev, "Error: no response!\n"); > } else if (temp & 0x01) { > dev_dbg(&adap->dev, "Post complete!\n"); > - sch_io_wr8(priv, SMBHSTSTS, temp); > + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); there is still a dev_dbg() using temp. To be on the safe side, do we want to do a "temp &= 0x0f" after the read_poll_timeout? Andi > temp = sch_io_rd8(priv, SMBHSTSTS) & 0x07; > if (temp & 0x06) { > /* Completion clear failed */ > -- > 2.43.0.rc1.1336.g36b5255a03ac >
Thu, Sep 12, 2024 at 09:29:38AM +0200, Andi Shyti kirjoitti: ... > > - sch_io_wr8(priv, SMBHSTSTS, temp); > > + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); > > there is still a dev_dbg() using temp. To be on the safe side, do > we want to do a "temp &= 0x0f" after the read_poll_timeout? Isn't it even better that we have more information in the debug output?
On Thu, Sep 12, 2024 at 06:35:46PM GMT, Andy Shevchenko wrote: > Thu, Sep 12, 2024 at 09:29:38AM +0200, Andi Shyti kirjoitti: > > ... > > > > - sch_io_wr8(priv, SMBHSTSTS, temp); > > > + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); > > > > there is still a dev_dbg() using temp. To be on the safe side, do > > we want to do a "temp &= 0x0f" after the read_poll_timeout? > > Isn't it even better that we have more information in the debug output? I think not, because: 1. It's information that we don't need, and we intentionally discard in every check. If we print a value we ignore, we risk providing incorrect information. 2. It’s more future-proof. In future development, cleanups, refactorings, or copy-paste, temp can be used as is without needing to continuously & it with 0xf. This avoids unnecessary operations being repeated later on. 3. It maintains the original design. In any case, these are small details, and the patch is already good as it is. Thanks, Andi
On Thu, Sep 12, 2024 at 6:55 PM Andi Shyti <andi.shyti@kernel.org> wrote: > On Thu, Sep 12, 2024 at 06:35:46PM GMT, Andy Shevchenko wrote: > > Thu, Sep 12, 2024 at 09:29:38AM +0200, Andi Shyti kirjoitti: ... > > > > - sch_io_wr8(priv, SMBHSTSTS, temp); > > > > + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); > > > > > > there is still a dev_dbg() using temp. To be on the safe side, do > > > we want to do a "temp &= 0x0f" after the read_poll_timeout? > > > > Isn't it even better that we have more information in the debug output? > > I think not, because: > > 1. It's information that we don't need, and we intentionally > discard in every check. If we print a value we ignore, we > risk providing incorrect information. > > 2. It’s more future-proof. In future development, cleanups, > refactorings, or copy-paste, temp can be used as is > without needing to continuously & it with 0xf. This > avoids unnecessary operations being repeated later on. > > 3. It maintains the original design. Okay, but where do you see this debug message? I looked again into the code and do not see that _this_ value of temp is used in the messaging. What did I miss? > In any case, these are small details, and the patch is already > good as it is. Thank you for the review!
On Thu, Sep 12, 2024 at 07:06:19PM GMT, Andy Shevchenko wrote: > On Thu, Sep 12, 2024 at 6:55 PM Andi Shyti <andi.shyti@kernel.org> wrote: > > On Thu, Sep 12, 2024 at 06:35:46PM GMT, Andy Shevchenko wrote: > > > Thu, Sep 12, 2024 at 09:29:38AM +0200, Andi Shyti kirjoitti: > > ... > > > > > > - sch_io_wr8(priv, SMBHSTSTS, temp); > > > > > + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); > > > > > > > > there is still a dev_dbg() using temp. To be on the safe side, do > > > > we want to do a "temp &= 0x0f" after the read_poll_timeout? > > > > > > Isn't it even better that we have more information in the debug output? > > > > I think not, because: > > > > 1. It's information that we don't need, and we intentionally > > discard in every check. If we print a value we ignore, we > > risk providing incorrect information. > > > > 2. It’s more future-proof. In future development, cleanups, > > refactorings, or copy-paste, temp can be used as is > > without needing to continuously & it with 0xf. This > > avoids unnecessary operations being repeated later on. > > > > 3. It maintains the original design. > > Okay, but where do you see this debug message? I looked again into the > code and do not see that _this_ value of temp is used in the > messaging. What did I miss? Indeed nowhere :-) 'temp' is re-read right after and &-ed with 0x0f. Nevermind! Andi
diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c index bbcfa3218a81..3a8cf7efb592 100644 --- a/drivers/i2c/busses/i2c-isch.c +++ b/drivers/i2c/busses/i2c-isch.c @@ -17,9 +17,9 @@ #include <linux/container_of.h> #include <linux/delay.h> #include <linux/device.h> -#include <linux/ioport.h> #include <linux/i2c.h> -#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/ioport.h> #include <linux/stddef.h> #include <linux/string_choices.h> #include <linux/types.h> @@ -34,9 +34,6 @@ #define SMBHSTDAT1 0x07 #define SMBBLKDAT 0x20 -/* Other settings */ -#define MAX_RETRIES 5000 - /* I2C constants */ #define SCH_QUICK 0x00 #define SCH_BYTE 0x01 @@ -83,7 +80,6 @@ static int sch_transaction(struct i2c_adapter *adap) struct sch_i2c *priv = container_of(adap, struct sch_i2c, adapter); int temp; int result = 0; - int retries = 0; dev_dbg(&adap->dev, "Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n", @@ -112,15 +108,11 @@ static int sch_transaction(struct i2c_adapter *adap) temp |= 0x10; sch_io_wr8(priv, SMBHSTCNT, temp); - do { - usleep_range(100, 200); - temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f; - } while ((temp & 0x08) && (retries++ < MAX_RETRIES)); - + result = read_poll_timeout(sch_io_rd8, temp, !(temp & 0x08), 200, 500000, true, + priv, SMBHSTSTS); /* If the SMBus is still busy, we give up */ - if (retries > MAX_RETRIES) { + if (result) { dev_err(&adap->dev, "SMBus Timeout!\n"); - result = -ETIMEDOUT; } else if (temp & 0x04) { result = -EIO; dev_dbg(&adap->dev, "Bus collision! SMBus may be locked until next hard reset. (sorry!)\n"); @@ -130,7 +122,7 @@ static int sch_transaction(struct i2c_adapter *adap) dev_err(&adap->dev, "Error: no response!\n"); } else if (temp & 0x01) { dev_dbg(&adap->dev, "Post complete!\n"); - sch_io_wr8(priv, SMBHSTSTS, temp); + sch_io_wr8(priv, SMBHSTSTS, temp & 0x0f); temp = sch_io_rd8(priv, SMBHSTSTS) & 0x07; if (temp & 0x06) { /* Completion clear failed */
Simplify the code by using read_poll_timeout(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/busses/i2c-isch.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)