[PATCH v1 00/12] i2c: isch: Put the driver into shape

[PATCH v1 00/12] i2c: isch: Put the driver into shape

[Andy Shevchenko]

Driver code is quite outdated WRT modern in-kernel APIs and
also has one non-critival bug.

The series is to address the above. Has been tested on
Minnowboard (Intel Tunnel Creek platform) with connected
LM95245 HW monitor chip.

Andy Shevchenko (12):
  i2c: isch: Add missed 'else'
  i2c: isch: Pass pointer to struct i2c_adapter down
  i2c: isch: Use string_choices API instead of ternary operator
  i2c: isch: Switch to memory mapped IO accessors
  i2c: isch: Use custom private data structure
  i2c: isch: switch i2c registration to devm functions
  i2c: isch: Utilize temporary variable to hold device pointer
  i2c: isch: Use read_poll_timeout()
  i2c: isch: Unify the name of the variable to hold an error code
  i2c: isch: Don't use "proxy" headers
  i2c: isch: Prefer to use octal permission
  i2c: isch: Convert to kernel-doc

 drivers/i2c/busses/i2c-isch.c | 324 +++++++++++++++++-----------------
 1 file changed, 165 insertions(+), 159 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 01/12] i2c: isch: Add missed 'else'

[Andy Shevchenko]

In accordance with the existing comment and code analysis
it is quite likely that there is a missed 'else' when adapter
times out. Add it.

Fixes: 5bc1200852c3 ("i2c: Add Intel SCH SMBus support")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 33dbc19d3848..f59158489ad9 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -99,8 +99,7 @@ static int sch_transaction(void)
 	if (retries > MAX_RETRIES) {
 		dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
-	}
-	if (temp & 0x04) {
+	} else if (temp & 0x04) {
 		result = -EIO;
 		dev_dbg(&sch_adapter.dev, "Bus collision! SMBus may be "
 			"locked until next hard reset. (sorry!)\n");
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 01/12] i2c: isch: Add missed 'else'

[Andi Shyti]

Hi Andy,

On Wed, Sep 11, 2024 at 06:39:14PM GMT, Andy Shevchenko wrote:
> In accordance with the existing comment and code analysis
> it is quite likely that there is a missed 'else' when adapter
> times out. Add it.

Good catch! Very likely this was an oversight.

> Fixes: 5bc1200852c3 ("i2c: Add Intel SCH SMBus support")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I merged just this patch to i2c/i2c-host-fixes.

I will take the rest of the series after the merge window.

Thanks,
Andi

Re: [PATCH v1 01/12] i2c: isch: Add missed 'else'

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 11:35:01PM +0200, Andi Shyti wrote:
> Hi Andy,
> 
> On Wed, Sep 11, 2024 at 06:39:14PM GMT, Andy Shevchenko wrote:
> > In accordance with the existing comment and code analysis
> > it is quite likely that there is a missed 'else' when adapter
> > times out. Add it.
> 
> Good catch! Very likely this was an oversight.
> 
> > Fixes: 5bc1200852c3 ("i2c: Add Intel SCH SMBus support")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I merged just this patch to i2c/i2c-host-fixes.
> 
> I will take the rest of the series after the merge window.

Fine with me, thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 02/12] i2c: isch: Pass pointer to struct i2c_adapter down

[Andy Shevchenko]

There are a lot of messaging calls that use global variable of
struct i2c_adapter. Instead, to make code better and flexible
for further improvements, pass the pointer to the actual adapter
used for transfers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 53 +++++++++++++++--------------------
 1 file changed, 23 insertions(+), 30 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index f59158489ad9..96ee73fe6e81 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -55,14 +55,15 @@ MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)");
  * and this function will execute it.
  * return 0 for success and others for failure.
  */
-static int sch_transaction(void)
+static int sch_transaction(struct i2c_adapter *adap)
 {
 	int temp;
 	int result = 0;
 	int retries = 0;
 
-	dev_dbg(&sch_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
-		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
+	dev_dbg(&adap->dev,
+		"Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
+		inb(SMBHSTCNT),
 		inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
 		inb(SMBHSTDAT1));
 
@@ -70,19 +71,14 @@ static int sch_transaction(void)
 	temp = inb(SMBHSTSTS) & 0x0f;
 	if (temp) {
 		/* Can not be busy since we checked it in sch_access */
-		if (temp & 0x01) {
-			dev_dbg(&sch_adapter.dev, "Completion (%02x). "
-				"Clear...\n", temp);
-		}
-		if (temp & 0x06) {
-			dev_dbg(&sch_adapter.dev, "SMBus error (%02x). "
-				"Resetting...\n", temp);
-		}
+		if (temp & 0x01)
+			dev_dbg(&adap->dev, "Completion (%02x). Clear...\n", temp);
+		if (temp & 0x06)
+			dev_dbg(&adap->dev, "SMBus error (%02x). Resetting...\n", temp);
 		outb(temp, SMBHSTSTS);
 		temp = inb(SMBHSTSTS) & 0x0f;
 		if (temp) {
-			dev_err(&sch_adapter.dev,
-				"SMBus is not ready: (%02x)\n", temp);
+			dev_err(&adap->dev, "SMBus is not ready: (%02x)\n", temp);
 			return -EAGAIN;
 		}
 	}
@@ -97,31 +93,30 @@ static int sch_transaction(void)
 
 	/* If the SMBus is still busy, we give up */
 	if (retries > MAX_RETRIES) {
-		dev_err(&sch_adapter.dev, "SMBus Timeout!\n");
+		dev_err(&adap->dev, "SMBus Timeout!\n");
 		result = -ETIMEDOUT;
 	} else if (temp & 0x04) {
 		result = -EIO;
-		dev_dbg(&sch_adapter.dev, "Bus collision! SMBus may be "
-			"locked until next hard reset. (sorry!)\n");
+		dev_dbg(&adap->dev, "Bus collision! SMBus may be locked until next hard reset. (sorry!)\n");
 		/* Clock stops and target is stuck in mid-transmission */
 	} else if (temp & 0x02) {
 		result = -EIO;
-		dev_err(&sch_adapter.dev, "Error: no response!\n");
+		dev_err(&adap->dev, "Error: no response!\n");
 	} else if (temp & 0x01) {
-		dev_dbg(&sch_adapter.dev, "Post complete!\n");
+		dev_dbg(&adap->dev, "Post complete!\n");
 		outb(temp, SMBHSTSTS);
 		temp = inb(SMBHSTSTS) & 0x07;
 		if (temp & 0x06) {
 			/* Completion clear failed */
-			dev_dbg(&sch_adapter.dev, "Failed reset at end of "
-				"transaction (%02x), Bus error!\n", temp);
+			dev_dbg(&adap->dev,
+				"Failed reset at end of transaction (%02x), Bus error!\n", temp);
 		}
 	} else {
 		result = -ENXIO;
-		dev_dbg(&sch_adapter.dev, "No such address.\n");
+		dev_dbg(&adap->dev, "No such address.\n");
 	}
-	dev_dbg(&sch_adapter.dev, "Transaction (post): CNT=%02x, CMD=%02x, "
-		"ADD=%02x, DAT0=%02x, DAT1=%02x\n", inb(SMBHSTCNT),
+	dev_dbg(&adap->dev, "Transaction (post): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
+		inb(SMBHSTCNT),
 		inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
 		inb(SMBHSTDAT1));
 	return result;
@@ -143,7 +138,7 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 	/* Make sure the SMBus host is not busy */
 	temp = inb(SMBHSTSTS) & 0x0f;
 	if (temp & 0x08) {
-		dev_dbg(&sch_adapter.dev, "SMBus busy (%02x)\n", temp);
+		dev_dbg(&adap->dev, "SMBus busy (%02x)\n", temp);
 		return -EAGAIN;
 	}
 	temp = inw(SMBHSTCLK);
@@ -154,13 +149,11 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		 * 100 kHz. If we actually run at 25 MHz the bus will be
 		 * run ~75 kHz instead which should do no harm.
 		 */
-		dev_notice(&sch_adapter.dev,
-			"Clock divider uninitialized. Setting defaults\n");
+		dev_notice(&adap->dev, "Clock divider uninitialized. Setting defaults\n");
 		outw(backbone_speed / (4 * 100), SMBHSTCLK);
 	}
 
-	dev_dbg(&sch_adapter.dev, "access size: %d %s\n", size,
-		(read_write)?"READ":"WRITE");
+	dev_dbg(&adap->dev, "access size: %d %s\n", size, (read_write)?"READ":"WRITE");
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		outb((addr << 1) | read_write, SMBHSTADD);
@@ -205,10 +198,10 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
 		return -EOPNOTSUPP;
 	}
-	dev_dbg(&sch_adapter.dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
+	dev_dbg(&adap->dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
 	outb((inb(SMBHSTCNT) & 0xb0) | (size & 0x7), SMBHSTCNT);
 
-	rc = sch_transaction();
+	rc = sch_transaction(adap);
 	if (rc)	/* Error in transaction */
 		return rc;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 03/12] i2c: isch: Use string_choices API instead of ternary operator

[Andy Shevchenko]

Use modern string_choices API instead of manually determining the
output using ternary operator.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 96ee73fe6e81..f44c5fa276dc 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -19,10 +19,11 @@
 #include <linux/platform_device.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
-#include <linux/stddef.h>
 #include <linux/ioport.h>
 #include <linux/i2c.h>
 #include <linux/io.h>
+#include <linux/stddef.h>
+#include <linux/string_choices.h>
 
 /* SCH SMBus address offsets */
 #define SMBHSTCNT	(0 + sch_smba)
@@ -153,7 +154,7 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		outw(backbone_speed / (4 * 100), SMBHSTCLK);
 	}
 
-	dev_dbg(&adap->dev, "access size: %d %s\n", size, (read_write)?"READ":"WRITE");
+	dev_dbg(&adap->dev, "access size: %d %s\n", size, str_read_write(read_write));
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		outb((addr << 1) | read_write, SMBHSTADD);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andy Shevchenko]

Convert driver to use memory mapped IO accessors.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 133 ++++++++++++++++++++--------------
 1 file changed, 78 insertions(+), 55 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index f44c5fa276dc..8d34d4398f9b 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -24,16 +24,17 @@
 #include <linux/io.h>
 #include <linux/stddef.h>
 #include <linux/string_choices.h>
+#include <linux/types.h>
 
 /* SCH SMBus address offsets */
-#define SMBHSTCNT	(0 + sch_smba)
-#define SMBHSTSTS	(1 + sch_smba)
-#define SMBHSTCLK	(2 + sch_smba)
-#define SMBHSTADD	(4 + sch_smba) /* TSA */
-#define SMBHSTCMD	(5 + sch_smba)
-#define SMBHSTDAT0	(6 + sch_smba)
-#define SMBHSTDAT1	(7 + sch_smba)
-#define SMBBLKDAT	(0x20 + sch_smba)
+#define SMBHSTCNT	0x00
+#define SMBHSTSTS	0x01
+#define SMBHSTCLK	0x02
+#define SMBHSTADD	0x04	/* TSA */
+#define SMBHSTCMD	0x05
+#define SMBHSTDAT0	0x06
+#define SMBHSTDAT1	0x07
+#define SMBBLKDAT	0x20
 
 /* Other settings */
 #define MAX_RETRIES	5000
@@ -45,12 +46,33 @@
 #define SCH_WORD_DATA		0x03
 #define SCH_BLOCK_DATA		0x05
 
-static unsigned short sch_smba;
 static struct i2c_adapter sch_adapter;
+static void __iomem *sch_smba;
+
 static int backbone_speed = 33000; /* backbone speed in kHz */
 module_param(backbone_speed, int, S_IRUSR | S_IWUSR);
 MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)");
 
+static inline u8 sch_io_rd8(void __iomem *smba, unsigned int offset)
+{
+	return ioread8(smba + offset);
+}
+
+static inline void sch_io_wr8(void __iomem *smba, unsigned int offset, u8 value)
+{
+	iowrite8(value, smba + offset);
+}
+
+static inline u16 sch_io_rd16(void __iomem *smba, unsigned int offset)
+{
+	return ioread16(smba + offset);
+}
+
+static inline void sch_io_wr16(void __iomem *smba, unsigned int offset, u16 value)
+{
+	iowrite16(value, smba + offset);
+}
+
 /*
  * Start the i2c transaction -- the i2c_access will prepare the transaction
  * and this function will execute it.
@@ -64,20 +86,20 @@ static int sch_transaction(struct i2c_adapter *adap)
 
 	dev_dbg(&adap->dev,
 		"Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
-		inb(SMBHSTCNT),
-		inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
-		inb(SMBHSTDAT1));
+		sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD),
+		sch_io_rd8(sch_smba, SMBHSTADD),
+		sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1));
 
 	/* Make sure the SMBus host is ready to start transmitting */
-	temp = inb(SMBHSTSTS) & 0x0f;
+	temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
 	if (temp) {
 		/* Can not be busy since we checked it in sch_access */
 		if (temp & 0x01)
 			dev_dbg(&adap->dev, "Completion (%02x). Clear...\n", temp);
 		if (temp & 0x06)
 			dev_dbg(&adap->dev, "SMBus error (%02x). Resetting...\n", temp);
-		outb(temp, SMBHSTSTS);
-		temp = inb(SMBHSTSTS) & 0x0f;
+		sch_io_wr8(sch_smba, SMBHSTSTS, temp);
+		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
 		if (temp) {
 			dev_err(&adap->dev, "SMBus is not ready: (%02x)\n", temp);
 			return -EAGAIN;
@@ -85,11 +107,13 @@ static int sch_transaction(struct i2c_adapter *adap)
 	}
 
 	/* start the transaction by setting bit 4 */
-	outb(inb(SMBHSTCNT) | 0x10, SMBHSTCNT);
+	temp = sch_io_rd8(sch_smba, SMBHSTCNT);
+	temp |= 0x10;
+	sch_io_wr8(sch_smba, SMBHSTCNT, temp);
 
 	do {
 		usleep_range(100, 200);
-		temp = inb(SMBHSTSTS) & 0x0f;
+		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
 	} while ((temp & 0x08) && (retries++ < MAX_RETRIES));
 
 	/* If the SMBus is still busy, we give up */
@@ -105,8 +129,8 @@ 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");
-		outb(temp, SMBHSTSTS);
-		temp = inb(SMBHSTSTS) & 0x07;
+		sch_io_wr8(sch_smba, SMBHSTSTS, temp);
+		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x07;
 		if (temp & 0x06) {
 			/* Completion clear failed */
 			dev_dbg(&adap->dev,
@@ -117,9 +141,9 @@ static int sch_transaction(struct i2c_adapter *adap)
 		dev_dbg(&adap->dev, "No such address.\n");
 	}
 	dev_dbg(&adap->dev, "Transaction (post): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
-		inb(SMBHSTCNT),
-		inb(SMBHSTCMD), inb(SMBHSTADD), inb(SMBHSTDAT0),
-		inb(SMBHSTDAT1));
+		sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD),
+		sch_io_rd8(sch_smba, SMBHSTADD),
+		sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1));
 	return result;
 }
 
@@ -137,12 +161,12 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 	int i, len, temp, rc;
 
 	/* Make sure the SMBus host is not busy */
-	temp = inb(SMBHSTSTS) & 0x0f;
+	temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
 	if (temp & 0x08) {
 		dev_dbg(&adap->dev, "SMBus busy (%02x)\n", temp);
 		return -EAGAIN;
 	}
-	temp = inw(SMBHSTCLK);
+	temp = sch_io_rd16(sch_smba, SMBHSTCLK);
 	if (!temp) {
 		/*
 		 * We can't determine if we have 33 or 25 MHz clock for
@@ -151,47 +175,47 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		 * run ~75 kHz instead which should do no harm.
 		 */
 		dev_notice(&adap->dev, "Clock divider uninitialized. Setting defaults\n");
-		outw(backbone_speed / (4 * 100), SMBHSTCLK);
+		sch_io_wr16(sch_smba, SMBHSTCLK, backbone_speed / (4 * 100));
 	}
 
 	dev_dbg(&adap->dev, "access size: %d %s\n", size, str_read_write(read_write));
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		outb((addr << 1) | read_write, SMBHSTADD);
+		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
 		size = SCH_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
-		outb((addr << 1) | read_write, SMBHSTADD);
+		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
 		if (read_write == I2C_SMBUS_WRITE)
-			outb(command, SMBHSTCMD);
+			sch_io_wr8(sch_smba, SMBHSTCMD, command);
 		size = SCH_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		outb((addr << 1) | read_write, SMBHSTADD);
-		outb(command, SMBHSTCMD);
+		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(sch_smba, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE)
-			outb(data->byte, SMBHSTDAT0);
+			sch_io_wr8(sch_smba, SMBHSTDAT0, data->byte);
 		size = SCH_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		outb((addr << 1) | read_write, SMBHSTADD);
-		outb(command, SMBHSTCMD);
+		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(sch_smba, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE) {
-			outb(data->word & 0xff, SMBHSTDAT0);
-			outb((data->word & 0xff00) >> 8, SMBHSTDAT1);
+			sch_io_wr8(sch_smba, SMBHSTDAT0, data->word >> 0);
+			sch_io_wr8(sch_smba, SMBHSTDAT1, data->word >> 8);
 		}
 		size = SCH_WORD_DATA;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		outb((addr << 1) | read_write, SMBHSTADD);
-		outb(command, SMBHSTCMD);
+		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(sch_smba, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE) {
 			len = data->block[0];
 			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
 				return -EINVAL;
-			outb(len, SMBHSTDAT0);
+			sch_io_wr8(sch_smba, SMBHSTDAT0, len);
 			for (i = 1; i <= len; i++)
-				outb(data->block[i], SMBBLKDAT+i-1);
+				sch_io_wr8(sch_smba, SMBBLKDAT + i - 1, data->block[i]);
 		}
 		size = SCH_BLOCK_DATA;
 		break;
@@ -200,7 +224,10 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		return -EOPNOTSUPP;
 	}
 	dev_dbg(&adap->dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
-	outb((inb(SMBHSTCNT) & 0xb0) | (size & 0x7), SMBHSTCNT);
+
+	temp = sch_io_rd8(sch_smba, SMBHSTCNT);
+	temp = (temp & 0xb0) | (size & 0x7);
+	sch_io_wr8(sch_smba, SMBHSTCNT, temp);
 
 	rc = sch_transaction(adap);
 	if (rc)	/* Error in transaction */
@@ -212,17 +239,18 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 	switch (size) {
 	case SCH_BYTE:
 	case SCH_BYTE_DATA:
-		data->byte = inb(SMBHSTDAT0);
+		data->byte = sch_io_rd8(sch_smba, SMBHSTDAT0);
 		break;
 	case SCH_WORD_DATA:
-		data->word = inb(SMBHSTDAT0) + (inb(SMBHSTDAT1) << 8);
+		data->word = (sch_io_rd8(sch_smba, SMBHSTDAT0) << 0) +
+			     (sch_io_rd8(sch_smba, SMBHSTDAT1) << 8);
 		break;
 	case SCH_BLOCK_DATA:
-		data->block[0] = inb(SMBHSTDAT0);
+		data->block[0] = sch_io_rd8(sch_smba, SMBHSTDAT0);
 		if (data->block[0] == 0 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 			return -EPROTO;
 		for (i = 1; i <= data->block[0]; i++)
-			data->block[i] = inb(SMBBLKDAT+i-1);
+			data->block[i] = sch_io_rd8(sch_smba, SMBBLKDAT + i - 1);
 		break;
 	}
 	return 0;
@@ -255,26 +283,21 @@ static int smbus_sch_probe(struct platform_device *dev)
 	if (!res)
 		return -EBUSY;
 
-	if (!devm_request_region(&dev->dev, res->start, resource_size(res),
-				 dev->name)) {
-		dev_err(&dev->dev, "SMBus region 0x%x already in use!\n",
-			sch_smba);
+	sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
+	if (!sch_smba) {
+		dev_err(&dev->dev, "SMBus region %pR already in use!\n", res);
 		return -EBUSY;
 	}
 
-	sch_smba = res->start;
-
-	dev_dbg(&dev->dev, "SMBA = 0x%X\n", sch_smba);
-
 	/* set up the sysfs linkage to our parent device */
 	sch_adapter.dev.parent = &dev->dev;
 
 	snprintf(sch_adapter.name, sizeof(sch_adapter.name),
-		"SMBus SCH adapter at %04x", sch_smba);
+		"SMBus SCH adapter at %04x", res->start);
 
 	retval = i2c_add_adapter(&sch_adapter);
 	if (retval)
-		sch_smba = 0;
+		sch_smba = NULL;
 
 	return retval;
 }
@@ -283,7 +306,7 @@ static void smbus_sch_remove(struct platform_device *pdev)
 {
 	if (sch_smba) {
 		i2c_del_adapter(&sch_adapter);
-		sch_smba = 0;
+		sch_smba = NULL;
 	}
 }
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.11-rc7]
[cannot apply to next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors
config: i386-buildonly-randconfig-001-20240913 (https://download.01.org/0day-ci/archive/20240914/202409140743.kKVc8T3C-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140743.kKVc8T3C-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409140743.kKVc8T3C-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-isch.c:296:32: warning: format specifies type 'unsigned int' but the argument has type 'resource_size_t' (aka 'unsigned long long') [-Wformat]
     296 |                 "SMBus SCH adapter at %04x", res->start);
         |                                       ~~~~   ^~~~~~~~~~
         |                                       %04llx
   1 warning generated.


vim +296 drivers/i2c/busses/i2c-isch.c

   276	
   277	static int smbus_sch_probe(struct platform_device *dev)
   278	{
   279		struct resource *res;
   280		int retval;
   281	
   282		res = platform_get_resource(dev, IORESOURCE_IO, 0);
   283		if (!res)
   284			return -EBUSY;
   285	
   286		sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
   287		if (!sch_smba) {
   288			dev_err(&dev->dev, "SMBus region %pR already in use!\n", res);
   289			return -EBUSY;
   290		}
   291	
   292		/* set up the sysfs linkage to our parent device */
   293		sch_adapter.dev.parent = &dev->dev;
   294	
   295		snprintf(sch_adapter.name, sizeof(sch_adapter.name),
 > 296			"SMBus SCH adapter at %04x", res->start);
   297	
   298		retval = i2c_add_adapter(&sch_adapter);
   299		if (retval)
   300			sch_smba = NULL;
   301	
   302		return retval;
   303	}
   304	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[kernel test robot]

Hi Andy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.11-rc7]
[cannot apply to next-20240913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409141436.QFCDQrRF-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe':
>> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=]
     296 |                 "SMBus SCH adapter at %04x", res->start);
         |                                       ~~~^   ~~~~~~~~~~
         |                                          |      |
         |                                          |      resource_size_t {aka long long unsigned int}
         |                                          unsigned int
         |                                       %04llx


vim +296 drivers/i2c/busses/i2c-isch.c

   276	
   277	static int smbus_sch_probe(struct platform_device *dev)
   278	{
   279		struct resource *res;
   280		int retval;
   281	
   282		res = platform_get_resource(dev, IORESOURCE_IO, 0);
   283		if (!res)
   284			return -EBUSY;
   285	
   286		sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
   287		if (!sch_smba) {
   288			dev_err(&dev->dev, "SMBus region %pR already in use!\n", res);
   289			return -EBUSY;
   290		}
   291	
   292		/* set up the sysfs linkage to our parent device */
   293		sch_adapter.dev.parent = &dev->dev;
   294	
   295		snprintf(sch_adapter.name, sizeof(sch_adapter.name),
 > 296			"SMBus SCH adapter at %04x", res->start);
   297	
   298		retval = i2c_add_adapter(&sch_adapter);
   299		if (retval)
   300			sch_smba = NULL;
   301	
   302		return retval;
   303	}
   304	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andy Shevchenko]

On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote:
> Hi Andy,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on andi-shyti/i2c/i2c-host]
> [also build test WARNING on linus/master v6.11-rc7]
> [cannot apply to next-20240913]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> patch link:    https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com
> patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors
> config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/config)
> compiler: alpha-linux-gcc (GCC) 13.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409141436.QFCDQrRF-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe':
> >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=]
>      296 |                 "SMBus SCH adapter at %04x", res->start);
>          |                                       ~~~^   ~~~~~~~~~~
>          |                                          |      |
>          |                                          |      resource_size_t {aka long long unsigned int}
>          |                                          unsigned int
>          |                                       %04llx

Yeah, this should be something like %pa, but the problem with that that it
always uses the same, fixed-width format with a prefix. We don't want this. But
to make sure we have proper specifier we need to introduce a temporary variable
and assign the resource start address to it and then use that variable in here.
I'll update this in v2 and send it after we have v6.12-rc1 is out.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andi Shyti]

Hi Andy,

On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote:
> On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote:
> > Hi Andy,
> > 
> > kernel test robot noticed the following build warnings:
> > 
> > [auto build test WARNING on andi-shyti/i2c/i2c-host]
> > [also build test WARNING on linus/master v6.11-rc7]
> > [cannot apply to next-20240913]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/i2c-isch-Add-missed-else/20240912-002224
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
> > patch link:    https://lore.kernel.org/r/20240911154820.2846187-5-andriy.shevchenko%40linux.intel.com
> > patch subject: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors
> > config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/config)
> > compiler: alpha-linux-gcc (GCC) 13.3.0
> > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141436.QFCDQrRF-lkp@intel.com/reproduce)
> > 
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409141436.QFCDQrRF-lkp@intel.com/
> > 
> > All warnings (new ones prefixed by >>):
> > 
> >    drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe':
> > >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=]
> >      296 |                 "SMBus SCH adapter at %04x", res->start);
> >          |                                       ~~~^   ~~~~~~~~~~
> >          |                                          |      |
> >          |                                          |      resource_size_t {aka long long unsigned int}
> >          |                                          unsigned int
> >          |                                       %04llx
> 
> Yeah, this should be something like %pa, but the problem with that that it
> always uses the same, fixed-width format with a prefix. We don't want this. But
> to make sure we have proper specifier we need to introduce a temporary variable
> and assign the resource start address to it and then use that variable in here.
> I'll update this in v2 and send it after we have v6.12-rc1 is out.

Feel free to send it, I will apply it in i2c/i2c-host-for-6.12,
that's where I'm collecting the next patches.

Andi

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 12:10:32PM +0200, Andi Shyti wrote:
> On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote:
> > On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote:

...

> > >    drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe':
> > > >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=]
> > >      296 |                 "SMBus SCH adapter at %04x", res->start);
> > >          |                                       ~~~^   ~~~~~~~~~~
> > >          |                                          |      |
> > >          |                                          |      resource_size_t {aka long long unsigned int}
> > >          |                                          unsigned int
> > >          |                                       %04llx
> > 
> > Yeah, this should be something like %pa, but the problem with that that it
> > always uses the same, fixed-width format with a prefix. We don't want this. But
> > to make sure we have proper specifier we need to introduce a temporary variable
> > and assign the resource start address to it and then use that variable in here.
> > I'll update this in v2 and send it after we have v6.12-rc1 is out.
> 
> Feel free to send it, I will apply it in i2c/i2c-host-for-6.12,
> that's where I'm collecting the next patches.

But I believe it's a material for v6.13, no?
From the whole series the first patch is only a fix, the rest is pure
refactoring and cleanup.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andi Shyti]

On Mon, Sep 16, 2024 at 01:30:48PM GMT, Andy Shevchenko wrote:
> On Mon, Sep 16, 2024 at 12:10:32PM +0200, Andi Shyti wrote:
> > On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote:
> > > On Sat, Sep 14, 2024 at 02:56:19PM +0800, kernel test robot wrote:
> 
> ...
> 
> > > >    drivers/i2c/busses/i2c-isch.c: In function 'smbus_sch_probe':
> > > > >> drivers/i2c/busses/i2c-isch.c:296:42: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t' {aka 'long long unsigned int'} [-Wformat=]
> > > >      296 |                 "SMBus SCH adapter at %04x", res->start);
> > > >          |                                       ~~~^   ~~~~~~~~~~
> > > >          |                                          |      |
> > > >          |                                          |      resource_size_t {aka long long unsigned int}
> > > >          |                                          unsigned int
> > > >          |                                       %04llx
> > > 
> > > Yeah, this should be something like %pa, but the problem with that that it
> > > always uses the same, fixed-width format with a prefix. We don't want this. But
> > > to make sure we have proper specifier we need to introduce a temporary variable
> > > and assign the resource start address to it and then use that variable in here.
> > > I'll update this in v2 and send it after we have v6.12-rc1 is out.
> > 
> > Feel free to send it, I will apply it in i2c/i2c-host-for-6.12,
> > that's where I'm collecting the next patches.
> 
> But I believe it's a material for v6.13, no?

yes, I gave it the wrong name :-)
I renamed it now to i2c/i2c-host-for-6.13(*).

But it doesn't matter. It will become the next i2c/i2c-host after
Linus has taken the actual for-6.12 patches into mainline.

> From the whole series the first patch is only a fix, the rest is pure
> refactoring and cleanup.

Yes, you can omit the first patch. It has already been sent in
the fixes pull request.

Andi

(*) https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git/log/?h=i2c/i2c-host-for-6.13

Re: [PATCH v1 04/12] i2c: isch: Switch to memory mapped IO accessors

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 01:58:06PM +0200, Andi Shyti wrote:
> On Mon, Sep 16, 2024 at 01:30:48PM GMT, Andy Shevchenko wrote:
> > On Mon, Sep 16, 2024 at 12:10:32PM +0200, Andi Shyti wrote:
> > > On Mon, Sep 16, 2024 at 12:07:28PM GMT, Andy Shevchenko wrote:

...

> > > Feel free to send it, I will apply it in i2c/i2c-host-for-6.12,
> > > that's where I'm collecting the next patches.
> > 
> > But I believe it's a material for v6.13, no?
> 
> yes, I gave it the wrong name :-)
> I renamed it now to i2c/i2c-host-for-6.13(*).
> 
> But it doesn't matter. It will become the next i2c/i2c-host after
> Linus has taken the actual for-6.12 patches into mainline.

v2 has just been sent out.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 05/12] i2c: isch: Use custom private data structure

[Andy Shevchenko]

Use custom private data structure instead of global variables.
With that, remove not anymore true comment.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 145 ++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 70 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 8d34d4398f9b..4ec6c0a66a96 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -9,16 +9,14 @@
 
 */
 
-/*
-   Supports:
-	Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L)
-   Note: we assume there can only be one device, with one SMBus interface.
-*/
+/* Supports: Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) */
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/kernel.h>
+#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>
@@ -46,31 +44,33 @@
 #define SCH_WORD_DATA		0x03
 #define SCH_BLOCK_DATA		0x05
 
-static struct i2c_adapter sch_adapter;
-static void __iomem *sch_smba;
+struct sch_i2c {
+	struct i2c_adapter adapter;
+	void __iomem *smba;
+};
 
 static int backbone_speed = 33000; /* backbone speed in kHz */
 module_param(backbone_speed, int, S_IRUSR | S_IWUSR);
 MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)");
 
-static inline u8 sch_io_rd8(void __iomem *smba, unsigned int offset)
+static inline u8 sch_io_rd8(struct sch_i2c *priv, unsigned int offset)
 {
-	return ioread8(smba + offset);
+	return ioread8(priv->smba + offset);
 }
 
-static inline void sch_io_wr8(void __iomem *smba, unsigned int offset, u8 value)
+static inline void sch_io_wr8(struct sch_i2c *priv, unsigned int offset, u8 value)
 {
-	iowrite8(value, smba + offset);
+	iowrite8(value, priv->smba + offset);
 }
 
-static inline u16 sch_io_rd16(void __iomem *smba, unsigned int offset)
+static inline u16 sch_io_rd16(struct sch_i2c *priv, unsigned int offset)
 {
-	return ioread16(smba + offset);
+	return ioread16(priv->smba + offset);
 }
 
-static inline void sch_io_wr16(void __iomem *smba, unsigned int offset, u16 value)
+static inline void sch_io_wr16(struct sch_i2c *priv, unsigned int offset, u16 value)
 {
-	iowrite16(value, smba + offset);
+	iowrite16(value, priv->smba + offset);
 }
 
 /*
@@ -80,26 +80,27 @@ static inline void sch_io_wr16(void __iomem *smba, unsigned int offset, u16 valu
  */
 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",
-		sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD),
-		sch_io_rd8(sch_smba, SMBHSTADD),
-		sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1));
+		sch_io_rd8(priv, SMBHSTCNT), sch_io_rd8(priv, SMBHSTCMD),
+		sch_io_rd8(priv, SMBHSTADD),
+		sch_io_rd8(priv, SMBHSTDAT0), sch_io_rd8(priv, SMBHSTDAT1));
 
 	/* Make sure the SMBus host is ready to start transmitting */
-	temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
+	temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f;
 	if (temp) {
 		/* Can not be busy since we checked it in sch_access */
 		if (temp & 0x01)
 			dev_dbg(&adap->dev, "Completion (%02x). Clear...\n", temp);
 		if (temp & 0x06)
 			dev_dbg(&adap->dev, "SMBus error (%02x). Resetting...\n", temp);
-		sch_io_wr8(sch_smba, SMBHSTSTS, temp);
-		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
+		sch_io_wr8(priv, SMBHSTSTS, temp);
+		temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f;
 		if (temp) {
 			dev_err(&adap->dev, "SMBus is not ready: (%02x)\n", temp);
 			return -EAGAIN;
@@ -107,13 +108,13 @@ static int sch_transaction(struct i2c_adapter *adap)
 	}
 
 	/* start the transaction by setting bit 4 */
-	temp = sch_io_rd8(sch_smba, SMBHSTCNT);
+	temp = sch_io_rd8(priv, SMBHSTCNT);
 	temp |= 0x10;
-	sch_io_wr8(sch_smba, SMBHSTCNT, temp);
+	sch_io_wr8(priv, SMBHSTCNT, temp);
 
 	do {
 		usleep_range(100, 200);
-		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
+		temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f;
 	} while ((temp & 0x08) && (retries++ < MAX_RETRIES));
 
 	/* If the SMBus is still busy, we give up */
@@ -129,8 +130,8 @@ 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(sch_smba, SMBHSTSTS, temp);
-		temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x07;
+		sch_io_wr8(priv, SMBHSTSTS, temp);
+		temp = sch_io_rd8(priv, SMBHSTSTS) & 0x07;
 		if (temp & 0x06) {
 			/* Completion clear failed */
 			dev_dbg(&adap->dev,
@@ -141,9 +142,9 @@ static int sch_transaction(struct i2c_adapter *adap)
 		dev_dbg(&adap->dev, "No such address.\n");
 	}
 	dev_dbg(&adap->dev, "Transaction (post): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
-		sch_io_rd8(sch_smba, SMBHSTCNT), sch_io_rd8(sch_smba, SMBHSTCMD),
-		sch_io_rd8(sch_smba, SMBHSTADD),
-		sch_io_rd8(sch_smba, SMBHSTDAT0), sch_io_rd8(sch_smba, SMBHSTDAT1));
+		sch_io_rd8(priv, SMBHSTCNT), sch_io_rd8(priv, SMBHSTCMD),
+		sch_io_rd8(priv, SMBHSTADD),
+		sch_io_rd8(priv, SMBHSTDAT0), sch_io_rd8(priv, SMBHSTDAT1));
 	return result;
 }
 
@@ -158,15 +159,16 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		 unsigned short flags, char read_write,
 		 u8 command, int size, union i2c_smbus_data *data)
 {
+	struct sch_i2c *priv = container_of(adap, struct sch_i2c, adapter);
 	int i, len, temp, rc;
 
 	/* Make sure the SMBus host is not busy */
-	temp = sch_io_rd8(sch_smba, SMBHSTSTS) & 0x0f;
+	temp = sch_io_rd8(priv, SMBHSTSTS) & 0x0f;
 	if (temp & 0x08) {
 		dev_dbg(&adap->dev, "SMBus busy (%02x)\n", temp);
 		return -EAGAIN;
 	}
-	temp = sch_io_rd16(sch_smba, SMBHSTCLK);
+	temp = sch_io_rd16(priv, SMBHSTCLK);
 	if (!temp) {
 		/*
 		 * We can't determine if we have 33 or 25 MHz clock for
@@ -175,47 +177,47 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		 * run ~75 kHz instead which should do no harm.
 		 */
 		dev_notice(&adap->dev, "Clock divider uninitialized. Setting defaults\n");
-		sch_io_wr16(sch_smba, SMBHSTCLK, backbone_speed / (4 * 100));
+		sch_io_wr16(priv, SMBHSTCLK, backbone_speed / (4 * 100));
 	}
 
 	dev_dbg(&adap->dev, "access size: %d %s\n", size, str_read_write(read_write));
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(priv, SMBHSTADD, (addr << 1) | read_write);
 		size = SCH_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
-		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(priv, SMBHSTADD, (addr << 1) | read_write);
 		if (read_write == I2C_SMBUS_WRITE)
-			sch_io_wr8(sch_smba, SMBHSTCMD, command);
+			sch_io_wr8(priv, SMBHSTCMD, command);
 		size = SCH_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
-		sch_io_wr8(sch_smba, SMBHSTCMD, command);
+		sch_io_wr8(priv, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(priv, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE)
-			sch_io_wr8(sch_smba, SMBHSTDAT0, data->byte);
+			sch_io_wr8(priv, SMBHSTDAT0, data->byte);
 		size = SCH_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
-		sch_io_wr8(sch_smba, SMBHSTCMD, command);
+		sch_io_wr8(priv, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(priv, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE) {
-			sch_io_wr8(sch_smba, SMBHSTDAT0, data->word >> 0);
-			sch_io_wr8(sch_smba, SMBHSTDAT1, data->word >> 8);
+			sch_io_wr8(priv, SMBHSTDAT0, data->word >> 0);
+			sch_io_wr8(priv, SMBHSTDAT1, data->word >> 8);
 		}
 		size = SCH_WORD_DATA;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		sch_io_wr8(sch_smba, SMBHSTADD, (addr << 1) | read_write);
-		sch_io_wr8(sch_smba, SMBHSTCMD, command);
+		sch_io_wr8(priv, SMBHSTADD, (addr << 1) | read_write);
+		sch_io_wr8(priv, SMBHSTCMD, command);
 		if (read_write == I2C_SMBUS_WRITE) {
 			len = data->block[0];
 			if (len == 0 || len > I2C_SMBUS_BLOCK_MAX)
 				return -EINVAL;
-			sch_io_wr8(sch_smba, SMBHSTDAT0, len);
+			sch_io_wr8(priv, SMBHSTDAT0, len);
 			for (i = 1; i <= len; i++)
-				sch_io_wr8(sch_smba, SMBBLKDAT + i - 1, data->block[i]);
+				sch_io_wr8(priv, SMBBLKDAT + i - 1, data->block[i]);
 		}
 		size = SCH_BLOCK_DATA;
 		break;
@@ -225,9 +227,9 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 	}
 	dev_dbg(&adap->dev, "write size %d to 0x%04x\n", size, SMBHSTCNT);
 
-	temp = sch_io_rd8(sch_smba, SMBHSTCNT);
+	temp = sch_io_rd8(priv, SMBHSTCNT);
 	temp = (temp & 0xb0) | (size & 0x7);
-	sch_io_wr8(sch_smba, SMBHSTCNT, temp);
+	sch_io_wr8(priv, SMBHSTCNT, temp);
 
 	rc = sch_transaction(adap);
 	if (rc)	/* Error in transaction */
@@ -239,18 +241,18 @@ static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 	switch (size) {
 	case SCH_BYTE:
 	case SCH_BYTE_DATA:
-		data->byte = sch_io_rd8(sch_smba, SMBHSTDAT0);
+		data->byte = sch_io_rd8(priv, SMBHSTDAT0);
 		break;
 	case SCH_WORD_DATA:
-		data->word = (sch_io_rd8(sch_smba, SMBHSTDAT0) << 0) +
-			     (sch_io_rd8(sch_smba, SMBHSTDAT1) << 8);
+		data->word = (sch_io_rd8(priv, SMBHSTDAT0) << 0) +
+			     (sch_io_rd8(priv, SMBHSTDAT1) << 8);
 		break;
 	case SCH_BLOCK_DATA:
-		data->block[0] = sch_io_rd8(sch_smba, SMBHSTDAT0);
+		data->block[0] = sch_io_rd8(priv, SMBHSTDAT0);
 		if (data->block[0] == 0 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 			return -EPROTO;
 		for (i = 1; i <= data->block[0]; i++)
-			data->block[i] = sch_io_rd8(sch_smba, SMBBLKDAT + i - 1);
+			data->block[i] = sch_io_rd8(priv, SMBBLKDAT + i - 1);
 		break;
 	}
 	return 0;
@@ -268,46 +270,49 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= sch_func,
 };
 
-static struct i2c_adapter sch_adapter = {
-	.owner		= THIS_MODULE,
-	.class		= I2C_CLASS_HWMON,
-	.algo		= &smbus_algorithm,
-};
-
 static int smbus_sch_probe(struct platform_device *dev)
 {
+	struct sch_i2c *priv;
 	struct resource *res;
 	int retval;
 
+	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	res = platform_get_resource(dev, IORESOURCE_IO, 0);
 	if (!res)
 		return -EBUSY;
 
-	sch_smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
-	if (!sch_smba) {
+	priv->smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
+	if (!priv->smba) {
 		dev_err(&dev->dev, "SMBus region %pR already in use!\n", res);
 		return -EBUSY;
 	}
 
 	/* set up the sysfs linkage to our parent device */
-	sch_adapter.dev.parent = &dev->dev;
+	priv->adapter.dev.parent = &dev->dev;
+	priv->adapter.owner = THIS_MODULE,
+	priv->adapter.class = I2C_CLASS_HWMON,
+	priv->adapter.algo = &smbus_algorithm,
 
-	snprintf(sch_adapter.name, sizeof(sch_adapter.name),
+	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
 		"SMBus SCH adapter at %04x", res->start);
 
-	retval = i2c_add_adapter(&sch_adapter);
+	retval = i2c_add_adapter(&priv->adapter);
 	if (retval)
-		sch_smba = NULL;
+		return retval;
 
-	return retval;
+	platform_set_drvdata(dev, priv);
+
+	return 0;
 }
 
 static void smbus_sch_remove(struct platform_device *pdev)
 {
-	if (sch_smba) {
-		i2c_del_adapter(&sch_adapter);
-		sch_smba = NULL;
-	}
+	struct sch_i2c *priv = platform_get_drvdata(pdev);
+
+	i2c_del_adapter(&priv->adapter);
 }
 
 static struct platform_driver smbus_sch_driver = {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 06/12] i2c: isch: switch i2c registration to devm functions

[Andy Shevchenko]

Switch from i2c_add_adapter() to resource managed devm_i2c_add_adapter()
for matching rest of driver initialization, and more concise code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 4ec6c0a66a96..679fe3049299 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -274,7 +274,6 @@ static int smbus_sch_probe(struct platform_device *dev)
 {
 	struct sch_i2c *priv;
 	struct resource *res;
-	int retval;
 
 	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -299,20 +298,7 @@ static int smbus_sch_probe(struct platform_device *dev)
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
 		"SMBus SCH adapter at %04x", res->start);
 
-	retval = i2c_add_adapter(&priv->adapter);
-	if (retval)
-		return retval;
-
-	platform_set_drvdata(dev, priv);
-
-	return 0;
-}
-
-static void smbus_sch_remove(struct platform_device *pdev)
-{
-	struct sch_i2c *priv = platform_get_drvdata(pdev);
-
-	i2c_del_adapter(&priv->adapter);
+	return devm_i2c_add_adapter(&dev->dev, &priv->adapter);
 }
 
 static struct platform_driver smbus_sch_driver = {
@@ -320,7 +306,6 @@ static struct platform_driver smbus_sch_driver = {
 		.name = "isch_smbus",
 	},
 	.probe		= smbus_sch_probe,
-	.remove_new	= smbus_sch_remove,
 };
 
 module_platform_driver(smbus_sch_driver);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 07/12] i2c: isch: Utilize temporary variable to hold device pointer

[Andy Shevchenko]

Introduce a temporary variable to hold a device pointer.
It can be utilized in the ->probe() and save a bit of LoCs.
To make it consistent, rename currently used dev to pdev.

While at it, convert the only error message to dev_err_probe().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 679fe3049299..bbcfa3218a81 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -270,27 +270,26 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= sch_func,
 };
 
-static int smbus_sch_probe(struct platform_device *dev)
+static int smbus_sch_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct sch_i2c *priv;
 	struct resource *res;
 
-	priv = devm_kzalloc(&dev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	res = platform_get_resource(dev, IORESOURCE_IO, 0);
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	if (!res)
 		return -EBUSY;
 
-	priv->smba = devm_ioport_map(&dev->dev, res->start, resource_size(res));
-	if (!priv->smba) {
-		dev_err(&dev->dev, "SMBus region %pR already in use!\n", res);
-		return -EBUSY;
-	}
+	priv->smba = devm_ioport_map(dev, res->start, resource_size(res));
+	if (!priv->smba)
+		return dev_err_probe(dev, -EBUSY, "SMBus region %pR already in use!\n", res);
 
 	/* set up the sysfs linkage to our parent device */
-	priv->adapter.dev.parent = &dev->dev;
+	priv->adapter.dev.parent = dev;
 	priv->adapter.owner = THIS_MODULE,
 	priv->adapter.class = I2C_CLASS_HWMON,
 	priv->adapter.algo = &smbus_algorithm,
@@ -298,7 +297,7 @@ static int smbus_sch_probe(struct platform_device *dev)
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
 		"SMBus SCH adapter at %04x", res->start);
 
-	return devm_i2c_add_adapter(&dev->dev, &priv->adapter);
+	return devm_i2c_add_adapter(dev, &priv->adapter);
 }
 
 static struct platform_driver smbus_sch_driver = {
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andy Shevchenko]

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

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 */
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andi Shyti]

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
> 

Re: [PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andi Shyti]

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

Re: [PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 08/12] i2c: isch: Use read_poll_timeout()

[Andi Shyti]

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

[PATCH v1 09/12] i2c: isch: Unify the name of the variable to hold an error code

[Andy Shevchenko]

There are two different names used for the variable that holds
an error code. Unify to use one variant in all cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 3a8cf7efb592..bb5e09feea61 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -79,7 +79,7 @@ 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 rc;
 
 	dev_dbg(&adap->dev,
 		"Transaction (pre): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
@@ -108,17 +108,16 @@ static int sch_transaction(struct i2c_adapter *adap)
 	temp |= 0x10;
 	sch_io_wr8(priv, SMBHSTCNT, temp);
 
-	result = read_poll_timeout(sch_io_rd8, temp, !(temp & 0x08), 200, 500000, true,
-				   priv, SMBHSTSTS);
+	rc = read_poll_timeout(sch_io_rd8, temp, !(temp & 0x08), 200, 500000, true, priv, SMBHSTSTS);
 	/* If the SMBus is still busy, we give up */
-	if (result) {
+	if (rc) {
 		dev_err(&adap->dev, "SMBus Timeout!\n");
 	} else if (temp & 0x04) {
-		result = -EIO;
+		rc = -EIO;
 		dev_dbg(&adap->dev, "Bus collision! SMBus may be locked until next hard reset. (sorry!)\n");
 		/* Clock stops and target is stuck in mid-transmission */
 	} else if (temp & 0x02) {
-		result = -EIO;
+		rc = -EIO;
 		dev_err(&adap->dev, "Error: no response!\n");
 	} else if (temp & 0x01) {
 		dev_dbg(&adap->dev, "Post complete!\n");
@@ -130,14 +129,14 @@ static int sch_transaction(struct i2c_adapter *adap)
 				"Failed reset at end of transaction (%02x), Bus error!\n", temp);
 		}
 	} else {
-		result = -ENXIO;
+		rc = -ENXIO;
 		dev_dbg(&adap->dev, "No such address.\n");
 	}
 	dev_dbg(&adap->dev, "Transaction (post): CNT=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
 		sch_io_rd8(priv, SMBHSTCNT), sch_io_rd8(priv, SMBHSTCMD),
 		sch_io_rd8(priv, SMBHSTADD),
 		sch_io_rd8(priv, SMBHSTDAT0), sch_io_rd8(priv, SMBHSTDAT1));
-	return result;
+	return rc;
 }
 
 /*
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 10/12] i2c: isch: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index bb5e09feea61..8fa48a346e12 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -11,15 +11,17 @@
 
 /* Supports: Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) */
 
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/kernel.h>
 #include <linux/container_of.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gfp_types.h>
 #include <linux/i2c.h>
 #include <linux/iopoll.h>
 #include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/sprintf.h>
 #include <linux/stddef.h>
 #include <linux/string_choices.h>
 #include <linux/types.h>
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 11/12] i2c: isch: Prefer to use octal permission

[Andy Shevchenko]

Octal permissions are preferred over the symbolics ones
for readbility. This ceases warning message pointed by checkpatch.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index 8fa48a346e12..a6aa28000568 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -49,7 +49,7 @@ struct sch_i2c {
 };
 
 static int backbone_speed = 33000; /* backbone speed in kHz */
-module_param(backbone_speed, int, S_IRUSR | S_IWUSR);
+module_param(backbone_speed, int, 0600);
 MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)");
 
 static inline u8 sch_io_rd8(struct sch_i2c *priv, unsigned int offset)
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 11/12] i2c: isch: Prefer to use octal permission

[Jesper Juhl]

Personally I find this to be *less* readable, but maybe that's just me.

On Wed, 11 Sept 2024 at 17:51, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Octal permissions are preferred over the symbolics ones
> for readbility. This ceases warning message pointed by checkpatch.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-isch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
> index 8fa48a346e12..a6aa28000568 100644
> --- a/drivers/i2c/busses/i2c-isch.c
> +++ b/drivers/i2c/busses/i2c-isch.c
> @@ -49,7 +49,7 @@ struct sch_i2c {
>  };
>
>  static int backbone_speed = 33000; /* backbone speed in kHz */
> -module_param(backbone_speed, int, S_IRUSR | S_IWUSR);
> +module_param(backbone_speed, int, 0600);
>  MODULE_PARM_DESC(backbone_speed, "Backbone speed in kHz, (default = 33000)");
>
>  static inline u8 sch_io_rd8(struct sch_i2c *priv, unsigned int offset)
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>
>

Re: [PATCH v1 11/12] i2c: isch: Prefer to use octal permission

[Andy Shevchenko]

On Wed, Sep 11, 2024 at 05:53:44PM +0200, Jesper Juhl wrote:
> Personally I find this to be *less* readable, but maybe that's just me.

It's just you :-)

checkpatch should complain nowadays about non-octal permissions.
It is documented here Documentation/dev-tools/checkpatch.rst.
IIRC it's added after Linus' rant on them.

But nonetheless thanks for the review!

> On Wed, 11 Sept 2024 at 17:51, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Octal permissions are preferred over the symbolics ones
> > for readbility. This ceases warning message pointed by checkpatch.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 12/12] i2c: isch: Convert to kernel-doc

[Andy Shevchenko]

Convert existing descriptions to kernel-doc format and unify
the rest of the comments to follow the modern style.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-isch.c | 48 ++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-isch.c b/drivers/i2c/busses/i2c-isch.c
index a6aa28000568..333312a50deb 100644
--- a/drivers/i2c/busses/i2c-isch.c
+++ b/drivers/i2c/busses/i2c-isch.c
@@ -1,13 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
-    i2c-isch.c - Linux kernel driver for Intel SCH chipset SMBus
-    - Based on i2c-piix4.c
-    Copyright (c) 1998 - 2002 Frodo Looijaard <frodol@dds.nl> and
-    Philip Edelbrock <phil@netroedge.com>
-    - Intel SCH support
-    Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan@intel.com>
-
-*/
+ *  Linux kernel driver for Intel SCH chipset SMBus
+ *  - Based on i2c-piix4.c
+ *  Copyright (c) 1998 - 2002 Frodo Looijaard <frodol@dds.nl> and
+ *  Philip Edelbrock <phil@netroedge.com>
+ *  - Intel SCH support
+ *  Copyright (c) 2007 - 2008 Jacob Jun Pan <jacob.jun.pan@intel.com>
+ */
 
 /* Supports: Intel SCH chipsets (AF82US15W, AF82US15L, AF82UL11L) */
 
@@ -72,10 +71,14 @@ static inline void sch_io_wr16(struct sch_i2c *priv, unsigned int offset, u16 va
 	iowrite16(value, priv->smba + offset);
 }
 
-/*
- * Start the i2c transaction -- the i2c_access will prepare the transaction
- * and this function will execute it.
- * return 0 for success and others for failure.
+/**
+ * sch_transaction - Start the i2c transaction
+ * @adap: the i2c adapter pointer
+ *
+ * The sch_access() will prepare the transaction and
+ * this function will execute it.
+ *
+ * Return: 0 for success and others for failure.
  */
 static int sch_transaction(struct i2c_adapter *adap)
 {
@@ -105,7 +108,7 @@ static int sch_transaction(struct i2c_adapter *adap)
 		}
 	}
 
-	/* start the transaction by setting bit 4 */
+	/* Start the transaction by setting bit 4 */
 	temp = sch_io_rd8(priv, SMBHSTCNT);
 	temp |= 0x10;
 	sch_io_wr8(priv, SMBHSTCNT, temp);
@@ -141,12 +144,17 @@ static int sch_transaction(struct i2c_adapter *adap)
 	return rc;
 }
 
-/*
- * This is the main access entry for i2c-sch access
- * adap is i2c_adapter pointer, addr is the i2c device bus address, read_write
- * (0 for read and 1 for write), size is i2c transaction type and data is the
- * union of transaction for data to be transferred or data read from bus.
- * return 0 for success and others for failure.
+/**
+ * sch_access - the main access entry for i2c-sch access
+ * @adap: the i2c adapter pointer
+ * @addr: the i2c device bus address
+ * @flags: I2C_CLIENT_* flags (usually zero or I2C_CLIENT_PEC)
+ * @read_write: 0 for read and 1 for write
+ * @command: Byte interpreted by slave, for protocols which use such bytes
+ * @size: the i2c transaction type
+ * @data: the union of transaction for data to be transferred or data read from bus
+ *
+ * Return: 0 for success and others for failure.
  */
 static s32 sch_access(struct i2c_adapter *adap, u16 addr,
 		 unsigned short flags, char read_write,
@@ -281,7 +289,7 @@ static int smbus_sch_probe(struct platform_device *pdev)
 	if (!priv->smba)
 		return dev_err_probe(dev, -EBUSY, "SMBus region %pR already in use!\n", res);
 
-	/* set up the sysfs linkage to our parent device */
+	/* Set up the sysfs linkage to our parent device */
 	priv->adapter.dev.parent = dev;
 	priv->adapter.owner = THIS_MODULE,
 	priv->adapter.class = I2C_CLASS_HWMON,
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 00/12] i2c: isch: Put the driver into shape

[Andi Shyti]

Hi Andy,

> Andy Shevchenko (12):
>   i2c: isch: Add missed 'else'
>   i2c: isch: Pass pointer to struct i2c_adapter down
>   i2c: isch: Use string_choices API instead of ternary operator
>   i2c: isch: Switch to memory mapped IO accessors
>   i2c: isch: Use custom private data structure
>   i2c: isch: switch i2c registration to devm functions
>   i2c: isch: Utilize temporary variable to hold device pointer
>   i2c: isch: Use read_poll_timeout()
>   i2c: isch: Unify the name of the variable to hold an error code
>   i2c: isch: Don't use "proxy" headers
>   i2c: isch: Prefer to use octal permission
>   i2c: isch: Convert to kernel-doc

very nice cleanup! Now this driver has a nice modern look.
Thanks!

I will leave this patch for some time to give it some more time
for review and then I will take it in.

Thanks,
Andi