[PATCH 0/8] i2c: i801: collection of further improvements / refactorings

[PATCH 0/8] i2c: i801: collection of further improvements / refactorings

[Heiner Kallweit]

This series contains further improvements and refactorings.

Heiner Kallweit (8):
  i2c: i801: Replace magic value with constant in
    dmi_check_onboard_devices
  i2c: i801: Remove unused argument from tco functions
  i2c: i801: Use i2c core default adapter timeout
  i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4
  i2c: i801: Add helper i801_check_and_clear_pec_error
  i2c: i801: Split i801_block_transaction
  i2c: i801: Add SMBUS_LEN_SENTINEL
  i2c: i801: Add helper i801_get_block_len

 drivers/i2c/busses/i2c-i801.c | 216 +++++++++++++++++-----------------
 1 file changed, 106 insertions(+), 110 deletions(-)

-- 
2.42.0

[PATCH 1/8] i2c: i801: Replace magic value with constant in dmi_check_onboard_devices

[Heiner Kallweit]

Replace magic number 10 with the appropriate constant.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a485dc84d..880e98734 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1115,7 +1115,7 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
 {
 	int i, count;
 
-	if (dm->type != 10)
+	if (dm->type != DMI_ENTRY_ONBOARD_DEVICE)
 		return;
 
 	count = (dm->length - sizeof(struct dmi_header)) / 2;
-- 
2.42.0

[PATCH 2/8] i2c: i801: Remove unused argument from tco functions

[Heiner Kallweit]

Argument priv isn't used, so remove it.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 880e98734..cea8aaba0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1464,8 +1464,7 @@ static inline unsigned int i801_get_adapter_class(struct i801_priv *priv)
 #endif
 
 static struct platform_device *
-i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
-		 struct resource *tco_res)
+i801_add_tco_spt(struct pci_dev *pci_dev, struct resource *tco_res)
 {
 	static const struct itco_wdt_platform_data pldata = {
 		.name = "Intel PCH",
@@ -1496,8 +1495,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev,
 }
 
 static struct platform_device *
-i801_add_tco_cnl(struct i801_priv *priv, struct pci_dev *pci_dev,
-		 struct resource *tco_res)
+i801_add_tco_cnl(struct pci_dev *pci_dev, struct resource *tco_res)
 {
 	static const struct itco_wdt_platform_data pldata = {
 		.name = "Intel PCH",
@@ -1537,9 +1535,9 @@ static void i801_add_tco(struct i801_priv *priv)
 	res->flags = IORESOURCE_IO;
 
 	if (priv->features & FEATURE_TCO_CNL)
-		priv->tco_pdev = i801_add_tco_cnl(priv, pci_dev, tco_res);
+		priv->tco_pdev = i801_add_tco_cnl(pci_dev, tco_res);
 	else
-		priv->tco_pdev = i801_add_tco_spt(priv, pci_dev, tco_res);
+		priv->tco_pdev = i801_add_tco_spt(pci_dev, tco_res);
 
 	if (IS_ERR(priv->tco_pdev))
 		dev_warn(&pci_dev->dev, "failed to create iTCO device\n");
-- 
2.42.0

[PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Heiner Kallweit]

I see no need for a driver-specific timeout value, therefore let's go
with the i2c core default timeout of 1s set by i2c_register_adapter().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index cea8aaba0..344544d1f 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1707,9 +1707,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
 
-	/* Default timeout in interrupt mode: 200 ms */
-	priv->adapter.timeout = HZ / 5;
-
 	if (dev->irq == IRQ_NOTCONNECTED)
 		priv->features &= ~FEATURE_IRQ;
 
-- 
2.42.0

[PATCH 4/8] i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4

[Heiner Kallweit]

This change simplifies the code a little and makes clearer that the
ICH5 feature set is an extension of the ICH4 feature set.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 344544d1f..a300c66b4 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -968,11 +968,10 @@ static const struct i2c_algorithm smbus_algorithm = {
 	.functionality	= i801_func,
 };
 
-#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ	| \
-			 FEATURE_IRQ | FEATURE_SMBUS_PEC		| \
-			 FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
 #define FEATURES_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \
 			 FEATURE_HOST_NOTIFY)
+#define FEATURES_ICH5	(FEATURES_ICH4 | FEATURE_BLOCK_PROC | \
+			 FEATURE_I2C_BLOCK_READ | FEATURE_IRQ)
 
 static const struct pci_device_id i801_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, 82801AA_3,			0)				 },
-- 
2.42.0

[PATCH 5/8] i2c: i801: Add helper i801_check_and_clear_pec_error

[Heiner Kallweit]

Avoid code duplication and factor out checking and clearing PEC error
bit to new helper i801_check_and_clear_pec_error().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 38 ++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a300c66b4..915dd07e1 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -327,11 +327,27 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
 	"\t\t  0x10  don't use interrupts\n"
 	"\t\t  0x20  disable SMBus Host Notify ");
 
+static int i801_check_and_clear_pec_error(struct i801_priv *priv)
+{
+	u8 status;
+
+	if (!(priv->features & FEATURE_SMBUS_PEC))
+		return 0;
+
+	status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE;
+	if (status) {
+		outb_p(status, SMBAUXSTS(priv));
+		return -EBADMSG;
+	}
+
+	return 0;
+}
+
 /* Make sure the SMBus host is ready to start transmitting.
    Return 0 if it is, -EBUSY if it is not. */
 static int i801_check_pre(struct i801_priv *priv)
 {
-	int status;
+	int status, result;
 
 	status = inb_p(SMBHSTSTS(priv));
 	if (status & SMBHSTSTS_HOST_BUSY) {
@@ -352,13 +368,9 @@ static int i801_check_pre(struct i801_priv *priv)
 	 * the hardware was already in this state when the driver
 	 * started.
 	 */
-	if (priv->features & FEATURE_SMBUS_PEC) {
-		status = inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE;
-		if (status) {
-			pci_dbg(priv->pci_dev, "Clearing aux status flags (%02x)\n", status);
-			outb_p(status, SMBAUXSTS(priv));
-		}
-	}
+	result = i801_check_and_clear_pec_error(priv);
+	if (result)
+		pci_dbg(priv->pci_dev, "Clearing aux status flag CRCE\n");
 
 	return 0;
 }
@@ -407,14 +419,12 @@ static int i801_check_post(struct i801_priv *priv, int status)
 		 * bit is harmless as long as it's cleared before
 		 * the next operation.
 		 */
-		if ((priv->features & FEATURE_SMBUS_PEC) &&
-		    (inb_p(SMBAUXSTS(priv)) & SMBAUXSTS_CRCE)) {
-			outb_p(SMBAUXSTS_CRCE, SMBAUXSTS(priv));
-			result = -EBADMSG;
-			dev_dbg(&priv->pci_dev->dev, "PEC error\n");
+		result = i801_check_and_clear_pec_error(priv);
+		if (result) {
+			pci_dbg(priv->pci_dev, "PEC error\n");
 		} else {
 			result = -ENXIO;
-			dev_dbg(&priv->pci_dev->dev, "No response\n");
+			pci_dbg(priv->pci_dev, "No response\n");
 		}
 	}
 	if (status & SMBHSTSTS_BUS_ERR) {
-- 
2.42.0

[PATCH 6/8] i2c: i801: Split i801_block_transaction

[Heiner Kallweit]

i2c and smbus block transaction handling have little in common,
therefore split this function to improve code readability.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 112 +++++++++++++++-------------------
 1 file changed, 50 insertions(+), 62 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 915dd07e1..a9d3dfd9e 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -801,77 +801,65 @@ static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data
 	return 0;
 }
 
-/* Block transaction function */
-static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
-				  u8 addr, u8 hstcmd, char read_write, int command)
+static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
+					u8 addr, u8 hstcmd, char read_write, int command)
 {
-	int result = 0;
-	unsigned char hostc;
-
 	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
 		data->block[0] = I2C_SMBUS_BLOCK_MAX;
 	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 		return -EPROTO;
 
-	switch (command) {
-	case I2C_SMBUS_BLOCK_DATA:
-		i801_set_hstadd(priv, addr, read_write);
-		outb_p(hstcmd, SMBHSTCMD(priv));
-		break;
-	case I2C_SMBUS_I2C_BLOCK_DATA:
-		/*
-		 * NB: page 240 of ICH5 datasheet shows that the R/#W
-		 * bit should be cleared here, even when reading.
-		 * However if SPD Write Disable is set (Lynx Point and later),
-		 * the read will fail if we don't set the R/#W bit.
-		 */
-		i801_set_hstadd(priv, addr,
-				priv->original_hstcfg & SMBHSTCFG_SPD_WD ?
-				read_write : I2C_SMBUS_WRITE);
-		if (read_write == I2C_SMBUS_READ) {
-			/* NB: page 240 of ICH5 datasheet also shows
-			 * that DATA1 is the cmd field when reading
-			 */
-			outb_p(hstcmd, SMBHSTDAT1(priv));
-		} else
-			outb_p(hstcmd, SMBHSTCMD(priv));
-
-		if (read_write == I2C_SMBUS_WRITE) {
-			/* set I2C_EN bit in configuration register */
-			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
-			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
-					      hostc | SMBHSTCFG_I2C_EN);
-		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
-			dev_err(&priv->pci_dev->dev,
-				"I2C block read is unsupported!\n");
-			return -EOPNOTSUPP;
-		}
-		break;
-	case I2C_SMBUS_BLOCK_PROC_CALL:
+	if (command == I2C_SMBUS_BLOCK_PROC_CALL)
 		/* Needs to be flagged as write transaction */
 		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
+	else
+		i801_set_hstadd(priv, addr, read_write);
+	outb_p(hstcmd, SMBHSTCMD(priv));
+
+	if (priv->features & FEATURE_BLOCK_BUFFER)
+		return i801_block_transaction_by_block(priv, data, read_write, command);
+	else
+		return i801_block_transaction_byte_by_byte(priv, data, read_write, command);
+}
+
+static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
+				      u8 addr, u8 hstcmd, char read_write, int command)
+{
+	int result;
+	u8 hostc;
+
+	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
+		return -EPROTO;
+	/*
+	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
+	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
+	 * the read will fail if we don't set the R/#W bit.
+	 */
+	i801_set_hstadd(priv, addr,
+			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
+
+	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
+	if (read_write == I2C_SMBUS_READ)
+		outb_p(hstcmd, SMBHSTDAT1(priv));
+	else
 		outb_p(hstcmd, SMBHSTCMD(priv));
-		break;
+
+	if (read_write == I2C_SMBUS_WRITE) {
+		/* set I2C_EN bit in configuration register */
+		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
+		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
+	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
+		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
+		return -EOPNOTSUPP;
 	}
 
-	/* Experience has shown that the block buffer can only be used for
-	   SMBus (not I2C) block transactions, even though the datasheet
-	   doesn't mention this limitation. */
-	if ((priv->features & FEATURE_BLOCK_BUFFER) &&
-	    command != I2C_SMBUS_I2C_BLOCK_DATA)
-		result = i801_block_transaction_by_block(priv, data,
-							 read_write,
-							 command);
-	else
-		result = i801_block_transaction_byte_by_byte(priv, data,
-							     read_write,
-							     command);
+	/* Block buffer isn't supported for I2C block transactions */
+	result = i801_block_transaction_byte_by_byte(priv, data, read_write, command);
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA
-	 && read_write == I2C_SMBUS_WRITE) {
-		/* restore saved configuration register value */
+	/* restore saved configuration register value */
+	if (read_write == I2C_SMBUS_WRITE)
 		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc);
-	}
+
 	return result;
 }
 
@@ -902,10 +890,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
 
-	if (size == I2C_SMBUS_BLOCK_DATA ||
-	    size == I2C_SMBUS_I2C_BLOCK_DATA ||
-	    size == I2C_SMBUS_BLOCK_PROC_CALL)
-		ret = i801_block_transaction(priv, data, addr, command, read_write, size);
+	if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_BLOCK_PROC_CALL)
+		ret = i801_smbus_block_transaction(priv, data, addr, command, read_write, size);
+	else if (size == I2C_SMBUS_I2C_BLOCK_DATA)
+		ret = i801_i2c_block_transaction(priv, data, addr, command, read_write, size);
 	else
 		ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
 
-- 
2.42.0

[PATCH 7/8] i2c: i801: Add SMBUS_LEN_SENTINEL

[Heiner Kallweit]

Add a sentinel length value that is used to check whether we should
read and use the length value provided by the slave device.
This simplifies the currently used checks.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9d3dfd9e..30a2f9268 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -204,6 +204,8 @@
 #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
 				 STATUS_ERROR_FLAGS)
 
+#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
+
 /* Older devices have their ID defined in <linux/pci_ids.h> */
 #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
 #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
@@ -541,8 +543,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 {
 	if (priv->is_read) {
 		/* For SMBus block reads, length is received with first byte */
-		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
-		    (priv->count == 0)) {
+		if (priv->len == SMBUS_LEN_SENTINEL) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
@@ -697,8 +698,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		if (status)
 			return status;
 
-		if (i == 1 && read_write == I2C_SMBUS_READ
-		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
+		if (len == SMBUS_LEN_SENTINEL) {
 			len = inb_p(SMBHSTDAT0(priv));
 			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
@@ -805,7 +805,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
 					u8 addr, u8 hstcmd, char read_write, int command)
 {
 	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
-		data->block[0] = I2C_SMBUS_BLOCK_MAX;
+		data->block[0] = SMBUS_LEN_SENTINEL;
 	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 		return -EPROTO;
 
-- 
2.42.0

[PATCH 8/8] i2c: i801: Add helper i801_get_block_len

[Heiner Kallweit]

Avoid code duplication and factor out retrieving and checking the
block length value to new helper i801_get_block_len().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 36 +++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 30a2f9268..55bfb4d18 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -329,6 +329,18 @@ MODULE_PARM_DESC(disable_features, "Disable selected driver features:\n"
 	"\t\t  0x10  don't use interrupts\n"
 	"\t\t  0x20  disable SMBus Host Notify ");
 
+static int i801_get_block_len(struct i801_priv *priv)
+{
+	u8 len = inb_p(SMBHSTDAT0(priv));
+
+	if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
+		pci_err(priv->pci_dev, "Illegal SMBus block read size %u\n", len);
+		return -EPROTO;
+	}
+
+	return len;
+}
+
 static int i801_check_and_clear_pec_error(struct i801_priv *priv)
 {
 	u8 status;
@@ -524,11 +536,9 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 
 	if (read_write == I2C_SMBUS_READ ||
 	    command == I2C_SMBUS_BLOCK_PROC_CALL) {
-		len = inb_p(SMBHSTDAT0(priv));
-		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
-			status = -EPROTO;
-			goto out;
-		}
+		len = i801_get_block_len(priv);
+		if (len < 0)
+			return len;
 
 		data->block[0] = len;
 		for (i = 0; i < len; i++)
@@ -544,14 +554,11 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 	if (priv->is_read) {
 		/* For SMBus block reads, length is received with first byte */
 		if (priv->len == SMBUS_LEN_SENTINEL) {
-			priv->len = inb_p(SMBHSTDAT0(priv));
-			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
-				dev_err(&priv->pci_dev->dev,
-					"Illegal SMBus block read size %d\n",
-					priv->len);
+			priv->len = i801_get_block_len(priv);
+			if (priv->len < 0)
 				/* FIXME: Recover */
 				priv->len = I2C_SMBUS_BLOCK_MAX;
-			}
+
 			priv->data[-1] = priv->len;
 		}
 
@@ -699,11 +706,8 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 			return status;
 
 		if (len == SMBUS_LEN_SENTINEL) {
-			len = inb_p(SMBHSTDAT0(priv));
-			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
-				dev_err(&priv->pci_dev->dev,
-					"Illegal SMBus block read size %d\n",
-					len);
+			len = i801_get_block_len(priv);
+			if (len < 0) {
 				/* Recover */
 				while (inb_p(SMBHSTSTS(priv)) &
 				       SMBHSTSTS_HOST_BUSY)
-- 
2.42.0

Re: [PATCH 0/8] i2c: i801: collection of further improvements / refactorings

[Heiner Kallweit]

On 22.09.2023 21:32, Heiner Kallweit wrote:
> This series contains further improvements and refactorings.
> 
> Heiner Kallweit (8):
>   i2c: i801: Replace magic value with constant in
>     dmi_check_onboard_devices
>   i2c: i801: Remove unused argument from tco functions
>   i2c: i801: Use i2c core default adapter timeout
>   i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4
>   i2c: i801: Add helper i801_check_and_clear_pec_error
>   i2c: i801: Split i801_block_transaction
>   i2c: i801: Add SMBUS_LEN_SENTINEL
>   i2c: i801: Add helper i801_get_block_len
> 
>  drivers/i2c/busses/i2c-i801.c | 216 +++++++++++++++++-----------------
>  1 file changed, 106 insertions(+), 110 deletions(-)
> 
This series has been sitting idle for quite some time.
Any chance to get some review feedback?

Re: [PATCH 1/8] i2c: i801: Replace magic value with constant in dmi_check_onboard_devices

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:34:13PM +0200, Heiner Kallweit wrote:
> Replace magic number 10 with the appropriate constant.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 2/8] i2c: i801: Remove unused argument from tco functions

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:35:00PM +0200, Heiner Kallweit wrote:
> Argument priv isn't used, so remove it.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
> I see no need for a driver-specific timeout value, therefore let's go
> with the i2c core default timeout of 1s set by i2c_register_adapter().

Why is the timeout value not needed in your opinion? Is the
datasheet specifying anything here?

Jean any opinion here?

Thanks,
Andi

Re: [PATCH 4/8] i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:36:45PM +0200, Heiner Kallweit wrote:
> This change simplifies the code a little and makes clearer that the
> ICH5 feature set is an extension of the ICH4 feature set.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 5/8] i2c: i801: Add helper i801_check_and_clear_pec_error

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:37:35PM +0200, Heiner Kallweit wrote:
> Avoid code duplication and factor out checking and clearing PEC error
> bit to new helper i801_check_and_clear_pec_error().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 6/8] i2c: i801: Split i801_block_transaction

[Andi Shyti]

Hi Heiner,

...

> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> +				      u8 addr, u8 hstcmd, char read_write, int command)
> +{
> +	int result;
> +	u8 hostc;
> +
> +	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
> +		return -EPROTO;
> +	/*
> +	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
> +	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
> +	 * the read will fail if we don't set the R/#W bit.
> +	 */
> +	i801_set_hstadd(priv, addr,
> +			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
> +
> +	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
> +	if (read_write == I2C_SMBUS_READ)
> +		outb_p(hstcmd, SMBHSTDAT1(priv));
> +	else
>  		outb_p(hstcmd, SMBHSTCMD(priv));
> -		break;
> +
> +	if (read_write == I2C_SMBUS_WRITE) {
> +		/* set I2C_EN bit in configuration register */
> +		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
> +	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> +		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
> +		return -EOPNOTSUPP;
>  	}

These two if...else blocks can be merged.

But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing
something different from the original code. E.g. if command =
I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a
functional change. Or am I getting confused?

Thanks,
Andi

Re: [PATCH 7/8] i2c: i801: Add SMBUS_LEN_SENTINEL

[Andi Shyti]

On Fri, Sep 22, 2023 at 09:40:25PM +0200, Heiner Kallweit wrote:
> Add a sentinel length value that is used to check whether we should
> read and use the length value provided by the slave device.
> This simplifies the currently used checks.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a9d3dfd9e..30a2f9268 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -204,6 +204,8 @@
>  #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
>  				 STATUS_ERROR_FLAGS)
>  
> +#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
> +
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
> @@ -541,8 +543,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  {
>  	if (priv->is_read) {
>  		/* For SMBus block reads, length is received with first byte */
> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
> -		    (priv->count == 0)) {
> +		if (priv->len == SMBUS_LEN_SENTINEL) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
>  				dev_err(&priv->pci_dev->dev,
> @@ -697,8 +698,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		if (status)
>  			return status;
>  
> -		if (i == 1 && read_write == I2C_SMBUS_READ
> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> +		if (len == SMBUS_LEN_SENTINEL) {
>  			len = inb_p(SMBHSTDAT0(priv));
>  			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
>  				dev_err(&priv->pci_dev->dev,
> @@ -805,7 +805,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
>  					u8 addr, u8 hstcmd, char read_write, int command)
>  {
>  	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
> -		data->block[0] = I2C_SMBUS_BLOCK_MAX;
> +		data->block[0] = SMBUS_LEN_SENTINEL;

This patch is good, but a few comments for each change to tell
where the sentinel will be used and where the sentinel was set
would help to better understand the use of the sentinel.

Thanks,
Andi

>  	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>  		return -EPROTO;
>  
> -- 
> 2.42.0
> 
> 

Re: [PATCH 8/8] i2c: i801: Add helper i801_get_block_len

[Andi Shyti]

Hi Heiner,

On Fri, Sep 22, 2023 at 09:41:41PM +0200, Heiner Kallweit wrote:
> Avoid code duplication and factor out retrieving and checking the
> block length value to new helper i801_get_block_len().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thanks,
Andi

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Heiner Kallweit]

On 30.01.2024 00:46, Andi Shyti wrote:
> Hi Heiner,
> 
> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
>> I see no need for a driver-specific timeout value, therefore let's go
>> with the i2c core default timeout of 1s set by i2c_register_adapter().
> 
> Why is the timeout value not needed in your opinion? Is the
> datasheet specifying anything here?
> 
I2C core sets a timeout of 1s if driver doesn't set a timeout value.
So for me the question is: Is there an actual need or benefit of
setting a lower timeout value? And that's something I don't see.

> Jean any opinion here?
> 
> Thanks,
> Andi

Heiner

Re: [PATCH 7/8] i2c: i801: Add SMBUS_LEN_SENTINEL

[Heiner Kallweit]

On 30.01.2024 01:23, Andi Shyti wrote:
> On Fri, Sep 22, 2023 at 09:40:25PM +0200, Heiner Kallweit wrote:
>> Add a sentinel length value that is used to check whether we should
>> read and use the length value provided by the slave device.
>> This simplifies the currently used checks.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a9d3dfd9e..30a2f9268 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -204,6 +204,8 @@
>>  #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
>>  				 STATUS_ERROR_FLAGS)
>>  
>> +#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
>> +
>>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
>>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
>> @@ -541,8 +543,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>>  {
>>  	if (priv->is_read) {
>>  		/* For SMBus block reads, length is received with first byte */
>> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
>> -		    (priv->count == 0)) {
>> +		if (priv->len == SMBUS_LEN_SENTINEL) {
>>  			priv->len = inb_p(SMBHSTDAT0(priv));
>>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
>>  				dev_err(&priv->pci_dev->dev,
>> @@ -697,8 +698,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>  		if (status)
>>  			return status;
>>  
>> -		if (i == 1 && read_write == I2C_SMBUS_READ
>> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>> +		if (len == SMBUS_LEN_SENTINEL) {
>>  			len = inb_p(SMBHSTDAT0(priv));
>>  			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
>>  				dev_err(&priv->pci_dev->dev,
>> @@ -805,7 +805,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
>>  					u8 addr, u8 hstcmd, char read_write, int command)
>>  {
>>  	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
>> -		data->block[0] = I2C_SMBUS_BLOCK_MAX;
>> +		data->block[0] = SMBUS_LEN_SENTINEL;
> 
> This patch is good, but a few comments for each change to tell
> where the sentinel will be used and where the sentinel was set
> would help to better understand the use of the sentinel.
> 

OK

> Thanks,
> Andi
> 
>>  	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>>  		return -EPROTO;
>>  
>> -- 
>> 2.42.0
>>
>>

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Andi Shyti]

Hi Heiner,

> > On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
> >> I see no need for a driver-specific timeout value, therefore let's go
> >> with the i2c core default timeout of 1s set by i2c_register_adapter().
> > 
> > Why is the timeout value not needed in your opinion? Is the
> > datasheet specifying anything here?
> > 
> I2C core sets a timeout of 1s if driver doesn't set a timeout value.
> So for me the question is: Is there an actual need or benefit of
> setting a lower timeout value? And that's something I don't see.

yes, that's why I am asking and I would like to have an opinion
from Jean. I will try to get hold of the datasheets, as well and
see if there is any constraint on the timeout.

Thanks,
Andi

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Heiner Kallweit]

On 30.01.2024 11:00, Andi Shyti wrote:
> Hi Heiner,
> 
>>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
>>>> I see no need for a driver-specific timeout value, therefore let's go
>>>> with the i2c core default timeout of 1s set by i2c_register_adapter().
>>>
>>> Why is the timeout value not needed in your opinion? Is the
>>> datasheet specifying anything here?
>>>
>> I2C core sets a timeout of 1s if driver doesn't set a timeout value.
>> So for me the question is: Is there an actual need or benefit of
>> setting a lower timeout value? And that's something I don't see.
> 
> yes, that's why I am asking and I would like to have an opinion
> from Jean. I will try to get hold of the datasheets, as well and
> see if there is any constraint on the timeout.
> 
The datasheet for the 7-series (doc# 326776-003) states:

5.21.3.2 Bus Time Out (The PCH as SMBus Master)
If there is an error in the transaction, such that an SMBus device does not signal an
acknowledge, or holds the clock lower than the allowed time-out time, the transaction
will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out
minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start
after the last bit of data is transferred by the PCH and it is waiting for a response.
The 25-ms time-out counter will not count under the following conditions:
1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set
2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that
the system has not locked up).

It's my understanding that the chip will signal timeout after 25ms. So we should never
have the case that the host timeout kicks in (as long as it's >25ms).
So the host timeout value doesn't really matter.

> Thanks,
> Andi

Heiner

Re: [PATCH 6/8] i2c: i801: Split i801_block_transaction

[Heiner Kallweit]

On 30.01.2024 01:09, Andi Shyti wrote:
> Hi Heiner,
> 
> ...
> 
>> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>> +				      u8 addr, u8 hstcmd, char read_write, int command)
>> +{
>> +	int result;
>> +	u8 hostc;
>> +
>> +	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>> +		return -EPROTO;
>> +	/*
>> +	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
>> +	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
>> +	 * the read will fail if we don't set the R/#W bit.
>> +	 */
>> +	i801_set_hstadd(priv, addr,
>> +			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
>> +
>> +	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
>> +	if (read_write == I2C_SMBUS_READ)
>> +		outb_p(hstcmd, SMBHSTDAT1(priv));
>> +	else
>>  		outb_p(hstcmd, SMBHSTCMD(priv));
>> -		break;
>> +
>> +	if (read_write == I2C_SMBUS_WRITE) {
>> +		/* set I2C_EN bit in configuration register */
>> +		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
>> +	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
>> +		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
>> +		return -EOPNOTSUPP;
>>  	}
> 
> These two if...else blocks can be merged.
> 
Yes, but I didn't do it because they cover different functionality.
IMO it's better readable this way.

> But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing
> something different from the original code. E.g. if command =
> I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a
> functional change. Or am I getting confused?
> 

At least there's no intentional functional change.
Can you describe the functional change you see?
Then it's easier to comment.

And yes: All the strange and misleading function argument naming
makes it quite confusing. This starts in I2C core:

smbus_xfer() has an argument "command", which is typically
a data value. See i2c_smbus_write_byte()
Argument "size" however is actually the command.

> Thanks,
> Andi

Heiner

Re: [PATCH 0/8] i2c: i801: collection of further improvements / refactorings

[Heiner Kallweit]

On 22.09.2023 21:32, Heiner Kallweit wrote:
> This series contains further improvements and refactorings.
> 
> Heiner Kallweit (8):
>   i2c: i801: Replace magic value with constant in
>     dmi_check_onboard_devices
>   i2c: i801: Remove unused argument from tco functions
>   i2c: i801: Use i2c core default adapter timeout
>   i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4
>   i2c: i801: Add helper i801_check_and_clear_pec_error
>   i2c: i801: Split i801_block_transaction
>   i2c: i801: Add SMBUS_LEN_SENTINEL
>   i2c: i801: Add helper i801_get_block_len
> 
>  drivers/i2c/busses/i2c-i801.c | 216 +++++++++++++++++-----------------
>  1 file changed, 106 insertions(+), 110 deletions(-)
> 
Thanks for the prompt review. Let's see what Jean thinks
about patch 3. Then I can incorporate the feedback and
provide a v2 of the series.

Re: [PATCH 6/8] i2c: i801: Split i801_block_transaction

[Heiner Kallweit]

On 30.01.2024 01:09, Andi Shyti wrote:
> Hi Heiner,
> 
> ...
> 
>> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>> +				      u8 addr, u8 hstcmd, char read_write, int command)
>> +{
>> +	int result;
>> +	u8 hostc;
>> +
>> +	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>> +		return -EPROTO;
>> +	/*
>> +	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
>> +	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
>> +	 * the read will fail if we don't set the R/#W bit.
>> +	 */
>> +	i801_set_hstadd(priv, addr,
>> +			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
>> +
>> +	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
>> +	if (read_write == I2C_SMBUS_READ)
>> +		outb_p(hstcmd, SMBHSTDAT1(priv));
>> +	else
>>  		outb_p(hstcmd, SMBHSTCMD(priv));
>> -		break;
>> +
>> +	if (read_write == I2C_SMBUS_WRITE) {
>> +		/* set I2C_EN bit in configuration register */
>> +		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
>> +	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
>> +		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
>> +		return -EOPNOTSUPP;
>>  	}
> 
> These two if...else blocks can be merged.
> 
> But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing
> something different from the original code. E.g. if command =
> I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a
> functional change. Or am I getting confused?
> 
I2C_SMBUS_BLOCK_DATA is handled by the new function
i801_smbus_block_transaction(). What may contribute to the confusion is that
there's also the command I2C_SMBUS_I2C_BLOCK_DATA, which is handled by
i801_i2c_block_transaction() now.

> Thanks,
> Andi

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Andi Shyti]

Hi Heiner,

On Tue, Jan 30, 2024 at 11:25:33AM +0100, Heiner Kallweit wrote:
> On 30.01.2024 11:00, Andi Shyti wrote:
> >>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
> >>>> I see no need for a driver-specific timeout value, therefore let's go
> >>>> with the i2c core default timeout of 1s set by i2c_register_adapter().
> >>>
> >>> Why is the timeout value not needed in your opinion? Is the
> >>> datasheet specifying anything here?
> >>>
> >> I2C core sets a timeout of 1s if driver doesn't set a timeout value.
> >> So for me the question is: Is there an actual need or benefit of
> >> setting a lower timeout value? And that's something I don't see.
> > 
> > yes, that's why I am asking and I would like to have an opinion
> > from Jean. I will try to get hold of the datasheets, as well and
> > see if there is any constraint on the timeout.
> > 
> The datasheet for the 7-series (doc# 326776-003) states:
> 
> 5.21.3.2 Bus Time Out (The PCH as SMBus Master)
> If there is an error in the transaction, such that an SMBus device does not signal an
> acknowledge, or holds the clock lower than the allowed time-out time, the transaction
> will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out
> minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start
> after the last bit of data is transferred by the PCH and it is waiting for a response.
> The 25-ms time-out counter will not count under the following conditions:
> 1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set
> 2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that
> the system has not locked up).
> 
> It's my understanding that the chip will signal timeout after 25ms. So we should never
> have the case that the host timeout kicks in (as long as it's >25ms).
> So the host timeout value doesn't really matter.

This driver is used by many platforms. I scrolled through the
datasheets of few of them and they differ from each other.

This was set by Jean[*] that's why I need to hear from him from
where this 200 ms comes from. 

As this change doesn't change much to the economy of the driver,
I would take it out from this series or place it at the end.

As of now, I'm going to take patch 1 and 2 in and you can
resubmit a v2 without the first two patches.

Thanks,
Andi

[*] b3b8df97723d ("i2c: i801: Use wait_event_timeout to wait for interrupts")

Re: [PATCH 6/8] i2c: i801: Split i801_block_transaction

[Andi Shyti]

Hi Heiner,

On Tue, Jan 30, 2024 at 12:20:26PM +0100, Heiner Kallweit wrote:
> On 30.01.2024 01:09, Andi Shyti wrote:
> >> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> >> +				      u8 addr, u8 hstcmd, char read_write, int command)
> >> +{
> >> +	int result;
> >> +	u8 hostc;
> >> +
> >> +	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
> >> +		return -EPROTO;
> >> +	/*
> >> +	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
> >> +	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
> >> +	 * the read will fail if we don't set the R/#W bit.
> >> +	 */
> >> +	i801_set_hstadd(priv, addr,
> >> +			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
> >> +
> >> +	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
> >> +	if (read_write == I2C_SMBUS_READ)
> >> +		outb_p(hstcmd, SMBHSTDAT1(priv));
> >> +	else
> >>  		outb_p(hstcmd, SMBHSTCMD(priv));
> >> -		break;
> >> +
> >> +	if (read_write == I2C_SMBUS_WRITE) {
> >> +		/* set I2C_EN bit in configuration register */
> >> +		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
> >> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
> >> +	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> >> +		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
> >> +		return -EOPNOTSUPP;
> >>  	}
> > 
> > These two if...else blocks can be merged.
> > 
> Yes, but I didn't do it because they cover different functionality.
> IMO it's better readable this way.

it's OK, this is a matter of taste.

> > But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing
> > something different from the original code. E.g. if command =
> > I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a
> > functional change. Or am I getting confused?
> > 
> 
> At least there's no intentional functional change.
> Can you describe the functional change you see?
> Then it's easier to comment.

I wrote it :-)

when command is I2C_SMBUS_BLOCK_DATA, before it was simply doing:

	i801_set_hstadd(priv, addr, read_write);
	outb_p(hstcmd, SMBHSTCMD(priv));

while now it does:

	i801_set_hstadd(priv, addr,
			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
	if (read_write == I2C_SMBUS_READ)
		outb_p(hstcmd, SMBHSTDAT1(priv));
	else
		outb_p(hstcmd, SMBHSTCMD(priv));

> And yes: All the strange and misleading function argument naming
> makes it quite confusing. This starts in I2C core:

you could try to play around with different diff algorithms when
generating the patch. Some of them perform better when renaming
functions.

Andi

PS. I'm not sure, though, this patch is improving readability,
    but I will check it again.


> smbus_xfer() has an argument "command", which is typically
> a data value. See i2c_smbus_write_byte()
> Argument "size" however is actually the command.
> 
> > Thanks,
> > Andi
> 
> Heiner

Re: (subset) [PATCH 0/8] i2c: i801: collection of further improvements / refactorings

[Andi Shyti]

Hi Heiner,

On Fri, 22 Sep 2023 21:32:57 +0200, Heiner Kallweit wrote:
> This series contains further improvements and refactorings.
> 
> Heiner Kallweit (8):
>   i2c: i801: Replace magic value with constant in
>     dmi_check_onboard_devices
>   i2c: i801: Remove unused argument from tco functions
>   i2c: i801: Use i2c core default adapter timeout
>   i2c: i801: Define FEATURES_ICH5 as an extension of FEATURES_ICH4
>   i2c: i801: Add helper i801_check_and_clear_pec_error
>   i2c: i801: Split i801_block_transaction
>   i2c: i801: Add SMBUS_LEN_SENTINEL
>   i2c: i801: Add helper i801_get_block_len
> 
> [...]

Applied the first to patches to i2c/i2c-host on

git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Thank you,
Andi

Patches applied
===============
[1/8] i2c: i801: Replace magic value with constant in dmi_check_onboard_devices
      commit: 9f14f46a276521c92cdffb0fc36f907e868d3888
[2/8] i2c: i801: Remove unused argument from tco functions
      commit: 96b125361866d998471c1380f809f2a2b4db60c0

Re: [PATCH 3/8] i2c: i801: Use i2c core default adapter timeout

[Heiner Kallweit]

On 30.01.2024 22:58, Andi Shyti wrote:
> Hi Heiner,
> 
> On Tue, Jan 30, 2024 at 11:25:33AM +0100, Heiner Kallweit wrote:
>> On 30.01.2024 11:00, Andi Shyti wrote:
>>>>> On Fri, Sep 22, 2023 at 09:35:55PM +0200, Heiner Kallweit wrote:
>>>>>> I see no need for a driver-specific timeout value, therefore let's go
>>>>>> with the i2c core default timeout of 1s set by i2c_register_adapter().
>>>>>
>>>>> Why is the timeout value not needed in your opinion? Is the
>>>>> datasheet specifying anything here?
>>>>>
>>>> I2C core sets a timeout of 1s if driver doesn't set a timeout value.
>>>> So for me the question is: Is there an actual need or benefit of
>>>> setting a lower timeout value? And that's something I don't see.
>>>
>>> yes, that's why I am asking and I would like to have an opinion
>>> from Jean. I will try to get hold of the datasheets, as well and
>>> see if there is any constraint on the timeout.
>>>
>> The datasheet for the 7-series (doc# 326776-003) states:
>>
>> 5.21.3.2 Bus Time Out (The PCH as SMBus Master)
>> If there is an error in the transaction, such that an SMBus device does not signal an
>> acknowledge, or holds the clock lower than the allowed time-out time, the transaction
>> will time out. The PCH will discard the cycle and set the DEV_ERR bit. The time out
>> minimum is 25 ms (800 RTC clocks). The time-out counter inside the PCH will start
>> after the last bit of data is transferred by the PCH and it is waiting for a response.
>> The 25-ms time-out counter will not count under the following conditions:
>> 1. BYTE_DONE_STATUS bit (SMBus I/O Offset 00h, Bit 7) is set
>> 2. The SECOND_TO_STS bit (TCO I/O Offset 06h, Bit 1) is not set (this indicates that
>> the system has not locked up).
>>
>> It's my understanding that the chip will signal timeout after 25ms. So we should never
>> have the case that the host timeout kicks in (as long as it's >25ms).
>> So the host timeout value doesn't really matter.
> 
> This driver is used by many platforms. I scrolled through the
> datasheets of few of them and they differ from each other.
> 
> This was set by Jean[*] that's why I need to hear from him from
> where this 200 ms comes from. 
> 
> As this change doesn't change much to the economy of the driver,
> I would take it out from this series or place it at the end.
> 
> As of now, I'm going to take patch 1 and 2 in and you can
> resubmit a v2 without the first two patches.
> 
Sounds good. Thanks!

> Thanks,
> Andi
> 
> [*] b3b8df97723d ("i2c: i801: Use wait_event_timeout to wait for interrupts")

Heiner

Re: [PATCH 6/8] i2c: i801: Split i801_block_transaction

[Heiner Kallweit]

On 30.01.2024 23:07, Andi Shyti wrote:
> Hi Heiner,
> 
> On Tue, Jan 30, 2024 at 12:20:26PM +0100, Heiner Kallweit wrote:
>> On 30.01.2024 01:09, Andi Shyti wrote:
>>>> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>>>> +				      u8 addr, u8 hstcmd, char read_write, int command)
>>>> +{
>>>> +	int result;
>>>> +	u8 hostc;
>>>> +
>>>> +	if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>>>> +		return -EPROTO;
>>>> +	/*
>>>> +	 * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here,
>>>> +	 * even when reading. However if SPD Write Disable is set (Lynx Point and later),
>>>> +	 * the read will fail if we don't set the R/#W bit.
>>>> +	 */
>>>> +	i801_set_hstadd(priv, addr,
>>>> +			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
>>>> +
>>>> +	/* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */
>>>> +	if (read_write == I2C_SMBUS_READ)
>>>> +		outb_p(hstcmd, SMBHSTDAT1(priv));
>>>> +	else
>>>>  		outb_p(hstcmd, SMBHSTCMD(priv));
>>>> -		break;
>>>> +
>>>> +	if (read_write == I2C_SMBUS_WRITE) {
>>>> +		/* set I2C_EN bit in configuration register */
>>>> +		pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>>>> +		pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN);
>>>> +	} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
>>>> +		pci_err(priv->pci_dev, "I2C block read is unsupported!\n");
>>>> +		return -EOPNOTSUPP;
>>>>  	}
>>>
>>> These two if...else blocks can be merged.
>>>
>> Yes, but I didn't do it because they cover different functionality.
>> IMO it's better readable this way.
> 
> it's OK, this is a matter of taste.
> 
>>> But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing
>>> something different from the original code. E.g. if command =
>>> I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a
>>> functional change. Or am I getting confused?
>>>
>>
>> At least there's no intentional functional change.
>> Can you describe the functional change you see?
>> Then it's easier to comment.
> 
> I wrote it :-)
> 
> when command is I2C_SMBUS_BLOCK_DATA, before it was simply doing:
> 
> 	i801_set_hstadd(priv, addr, read_write);
> 	outb_p(hstcmd, SMBHSTCMD(priv));
> 
> while now it does:
> 
> 	i801_set_hstadd(priv, addr,
> 			priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE);
> 	if (read_write == I2C_SMBUS_READ)
> 		outb_p(hstcmd, SMBHSTDAT1(priv));
> 	else
> 		outb_p(hstcmd, SMBHSTCMD(priv));
> 

That's a code snippet from new function i801_i2c_block_transaction() and not
the path taken in case of I2C_SMBUS_BLOCK_DATA. I think the diff is
hard to read. It's easier to look at new function i801_smbus_block_transaction()
after applying the patch.

Due to the change in i801_access() now i801_smbus_block_transaction() is called
in case of I2C_SMBUS_BLOCK_DATA. Because of the split this function became quite
simple. It does the same as before for I2C_SMBUS_BLOCK_DATA.

static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
                                        u8 addr, u8 hstcmd, char read_write, int command)
{
        if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
                data->block[0] = I2C_SMBUS_BLOCK_MAX;
        else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
                return -EPROTO;

        if (command == I2C_SMBUS_BLOCK_PROC_CALL)
                /* Needs to be flagged as write transaction */
                i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
        else
                i801_set_hstadd(priv, addr, read_write);
        outb_p(hstcmd, SMBHSTCMD(priv));

        if (priv->features & FEATURE_BLOCK_BUFFER)
                return i801_block_transaction_by_block(priv, data, read_write, command);
        else
                return i801_block_transaction_byte_by_byte(priv, data, read_write, command);
}


>> And yes: All the strange and misleading function argument naming
>> makes it quite confusing. This starts in I2C core:
> 
> you could try to play around with different diff algorithms when
> generating the patch. Some of them perform better when renaming
> functions.
> 
> Andi
> 
> PS. I'm not sure, though, this patch is improving readability,
>     but I will check it again.
> 
> 
>> smbus_xfer() has an argument "command", which is typically
>> a data value. See i2c_smbus_write_byte()
>> Argument "size" however is actually the command.
>>
>>> Thanks,
>>> Andi
>>
>> Heiner