[PATCH v2 00/10] i2c: i801: Series with minor improvements

[PATCH v2 00/10] i2c: i801: Series with minor improvements

[Heiner Kallweit]

This series includes a number of minor improvements, partially it's
a re-send of patches submitted in December last year already.

Changes in v2:
patch 1
- change one expression for generating less binary code
patch 2
- add a commit message part as comment to the code
- make changed code cleaner
patch 3
- add a commit message part as comment to the code
original patch 4
- dropped
patch 4 (previously patch 5)
- change expressions for generating less binary code
patch 5  (previously patch 6)
- rename new function to i801_simple_transaction
- code style fixes
patch 6
- added
patch 7
- added based on a suggestion in  a review comment
patch 8
- added based on a suggestion in  a review comment

Heiner Kallweit (10):
  i2c: i801: improve interrupt handler
  i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ
  i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER
  i2c: i801: add helper i801_set_hstadd()
  i2c: i801: add i801_simple_transaction(), complementing
    i801_block_transaction()
  i2c: i801: handle SMBAUXCTL_E32B in i801_block_transaction_by_block
    only
  i2c: i801: centralize configuring non-block commands in
    i801_simple_transaction
  i2c: i801: centralize configuring block commands in
    i801_block_transaction
  i2c: i801: call i801_check_pre() from i801_access()
  i2c: i801: call i801_check_post() from i801_access()

 drivers/i2c/busses/i2c-i801.c | 311 +++++++++++++++++-----------------
 1 file changed, 153 insertions(+), 158 deletions(-)

-- 
2.39.0

[PATCH v2 01/10] i2c: i801: improve interrupt handler

[Heiner Kallweit]

Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
and an error flag is set, then don't trust the result and skip calling
i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
in the main interrupt handler, this allows to simplify the code a
little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- change one expression for generating less binary code
---
 drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1fda1eaa6..da773c563 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -558,9 +558,6 @@ static void i801_isr_byte_done(struct i801_priv *priv)
 		/* Write next byte, except for IRQ after last byte */
 		outb_p(priv->data[++priv->count], SMBBLKDAT(priv));
 	}
-
-	/* Clear BYTE_DONE to continue with next byte */
-	outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 }
 
 static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
@@ -590,7 +587,6 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
  *      BUS_ERR - SMI# transaction collision
  *      FAILED - transaction was canceled due to a KILL request
  *    When any of these occur, update ->status and signal completion.
- *    ->status must be cleared before kicking off the next transaction.
  *
  * 2) For byte-by-byte (I2C read/write) transactions, one BYTE_DONE interrupt
  *    occurs for each byte of a byte-by-byte to prepare the next byte.
@@ -615,23 +611,18 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 	}
 
 	status = inb_p(SMBHSTSTS(priv));
-	if (status & SMBHSTSTS_BYTE_DONE)
+	if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)
 		i801_isr_byte_done(priv);
 
 	/*
-	 * Clear remaining IRQ sources: Completion of last command, errors
-	 * and the SMB_ALERT signal. SMB_ALERT status is set after signal
-	 * assertion independently of the interrupt generation being blocked
-	 * or not so clear it always when the status is set.
-	 */
-	status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS | SMBHSTSTS_SMBALERT_STS;
-	if (status)
-		outb_p(status, SMBHSTSTS(priv));
-	status &= ~SMBHSTSTS_SMBALERT_STS; /* SMB_ALERT not reported */
-	/*
-	 * Report transaction result.
-	 * ->status must be cleared before the next transaction is started.
+	 * Clear IRQ sources: SMB_ALERT status is set after signal assertion
+	 * independently of the interrupt generation being blocked or not
+	 * so clear it always when the status is set.
 	 */
+	status &= STATUS_FLAGS | SMBHSTSTS_SMBALERT_STS;
+	outb_p(status, SMBHSTSTS(priv));
+
+	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
 	if (status) {
 		priv->status = status;
 		complete(&priv->done);
-- 
2.39.0

Re: [PATCH v2 01/10] i2c: i801: improve interrupt handler

[Jean Delvare]

On Mon, 19 Dec 2022 19:13:44 +0100, Heiner Kallweit wrote:
> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
> and an error flag is set, then don't trust the result and skip calling
> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
> in the main interrupt handler, this allows to simplify the code a
> little.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - change one expression for generating less binary code
> ---
>  drivers/i2c/busses/i2c-i801.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 01/10] i2c: i801: improve interrupt handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Mon, Dec 19, 2022 at 07:13:44PM +0100, Heiner Kallweit wrote:
> Not sure if it can happen, but better play safe: If SMBHSTSTS_BYTE_DONE
> and an error flag is set, then don't trust the result and skip calling
> i801_isr_byte_done(). In addition clear status bit SMBHSTSTS_BYTE_DONE
> in the main interrupt handler, this allows to simplify the code a
> little.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 02/10] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Heiner Kallweit]

Host notification uses an interrupt, therefore it makes sense only if
interrupts are enabled. Make this dependency explicit by removing
FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- add a commit message part as comment to the code
- make changed code cleaner
---
 drivers/i2c/busses/i2c-i801.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index da773c563..45c2ebe40 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1705,11 +1705,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));
 
-	/* Remember original Interrupt and Host Notify settings */
-	priv->original_hstcnt = inb_p(SMBHSTCNT(priv)) & ~SMBHSTCNT_KILL;
-	if (priv->features & FEATURE_HOST_NOTIFY)
-		priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
-
 	/* Default timeout in interrupt mode: 200 ms */
 	priv->adapter.timeout = HZ / 5;
 
@@ -1739,6 +1734,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	dev_info(&dev->dev, "SMBus using %s\n",
 		 priv->features & FEATURE_IRQ ? "PCI interrupt" : "polling");
 
+	/* Host notification uses an interrupt */
+	if (!(priv->features & FEATURE_IRQ))
+		priv->features &= ~FEATURE_HOST_NOTIFY;
+
+	/* Remember original Interrupt and Host Notify settings */
+	priv->original_hstcnt = inb_p(SMBHSTCNT(priv)) & ~SMBHSTCNT_KILL;
+	if (priv->features & FEATURE_HOST_NOTIFY)
+		priv->original_slvcmd = inb_p(SMBSLVCMD(priv));
+
 	i801_add_tco(priv);
 
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
-- 
2.39.0

Re: [PATCH v2 02/10] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Jean Delvare]

On Mon, 19 Dec 2022 19:14:43 +0100, Heiner Kallweit wrote:
> Host notification uses an interrupt, therefore it makes sense only if
> interrupts are enabled. Make this dependency explicit by removing
> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - add a commit message part as comment to the code
> - make changed code cleaner
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 02/10] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Mon, Dec 19, 2022 at 07:14:43PM +0100, Heiner Kallweit wrote:
> Host notification uses an interrupt, therefore it makes sense only if
> interrupts are enabled. Make this dependency explicit by removing
> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 03/10] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Heiner Kallweit]

According to the datasheet the block process call requires block
buffer mode. The user may disable block buffer mode by module
parameter disable_features, in such a case we have to clear
FEATURE_BLOCK_PROC.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- add a commit message part as comment to the code
---
 drivers/i2c/busses/i2c-i801.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 45c2ebe40..2e9c5856a 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1658,6 +1658,10 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 	priv->features &= ~disable_features;
 
+	/* The block process call uses block buffer mode */
+	if (!(priv->features & FEATURE_BLOCK_BUFFER))
+		priv->features &= ~FEATURE_BLOCK_PROC;
+
 	err = pcim_enable_device(dev);
 	if (err) {
 		dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
-- 
2.39.0

Re: [PATCH v2 03/10] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Jean Delvare]

On Mon, 19 Dec 2022 19:15:25 +0100, Heiner Kallweit wrote:
> According to the datasheet the block process call requires block
> buffer mode. The user may disable block buffer mode by module
> parameter disable_features, in such a case we have to clear
> FEATURE_BLOCK_PROC.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - add a commit message part as comment to the code
> ---
>  drivers/i2c/busses/i2c-i801.c | 4 ++++
>  1 file changed, 4 insertions(+)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 03/10] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

On Mon, Dec 19, 2022 at 07:15:25PM +0100, Heiner Kallweit wrote:
> According to the datasheet the block process call requires block
> buffer mode. The user may disable block buffer mode by module
> parameter disable_features, in such a case we have to clear
> FEATURE_BLOCK_PROC.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 04/10] i2c: i801: add helper i801_set_hstadd()

[Heiner Kallweit]

Factor out setting SMBHSTADD to a helper. The current code makes the
assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
Therefore let the new helper explicitly check for I2C_SMBUS_READ.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 2e9c5856a..882cf5135 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -727,6 +727,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	return i801_check_post(priv, status);
 }
 
+static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
+{
+	outb_p((addr << 1) | (read_write & 0x01), SMBHSTADD(priv));
+}
+
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
 				  char read_write, int command)
@@ -797,28 +802,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
-		       SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, read_write);
 		xact = I801_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
-		       SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, read_write);
 		if (read_write == I2C_SMBUS_WRITE)
 			outb_p(command, SMBHSTCMD(priv));
 		xact = I801_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
-		       SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, read_write);
 		outb_p(command, SMBHSTCMD(priv));
 		if (read_write == I2C_SMBUS_WRITE)
 			outb_p(data->byte, SMBHSTDAT0(priv));
 		xact = I801_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
-		       SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, read_write);
 		outb_p(command, SMBHSTCMD(priv));
 		if (read_write == I2C_SMBUS_WRITE) {
 			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
@@ -827,7 +828,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		xact = I801_WORD_DATA;
 		break;
 	case I2C_SMBUS_PROC_CALL:
-		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
 		outb_p(command, SMBHSTCMD(priv));
 		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
@@ -835,8 +836,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		read_write = I2C_SMBUS_READ;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
-		       SMBHSTADD(priv));
+		i801_set_hstadd(priv, addr, read_write);
 		outb_p(command, SMBHSTCMD(priv));
 		block = 1;
 		break;
@@ -847,10 +847,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		 * However if SPD Write Disable is set (Lynx Point and later),
 		 * the read will fail if we don't set the R/#W bit.
 		 */
-		outb_p(((addr & 0x7f) << 1) |
-		       ((priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
-			(read_write & 0x01) : 0),
-		       SMBHSTADD(priv));
+		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 */
@@ -860,11 +859,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		block = 1;
 		break;
 	case I2C_SMBUS_BLOCK_PROC_CALL:
-		/*
-		 * Bit 0 of the slave address register always indicate a write
-		 * command.
-		 */
-		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
+		/* Needs to be flagged as write transaction */
+		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
 		outb_p(command, SMBHSTCMD(priv));
 		block = 1;
 		break;
-- 
2.39.0

Re: [PATCH v2 04/10] i2c: i801: add helper i801_set_hstadd()

[Jean Delvare]

On Mon, 19 Dec 2022 19:16:08 +0100, Heiner Kallweit wrote:
> Factor out setting SMBHSTADD to a helper. The current code makes the
> assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - change expressions for generating less binary code
> ---
>  drivers/i2c/busses/i2c-i801.c | 36 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 04/10] i2c: i801: add helper i801_set_hstadd()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]

On Mon, Dec 19, 2022 at 07:16:08PM +0100, Heiner Kallweit wrote:
> Factor out setting SMBHSTADD to a helper. The current code makes the
> assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 05/10] i2c: i801: add i801_simple_transaction(), complementing i801_block_transaction()

[Heiner Kallweit]

This patch factors out non-block pre/post processing to a new function
i801_simple_transaction(), complementing existing function
i801_block_transaction(). This makes i801_access() better readable.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rename new function to i801_simple_transaction
- code style fixes
---
 drivers/i2c/busses/i2c-i801.c | 92 +++++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 37 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 882cf5135..d934d410b 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -732,6 +732,59 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
 	outb_p((addr << 1) | (read_write & 0x01), SMBHSTADD(priv));
 }
 
+/* Single value transaction function */
+static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
+				   char read_write, int command)
+{
+	int xact, ret;
+
+	switch (command) {
+	case I2C_SMBUS_QUICK:
+		xact = I801_QUICK;
+		break;
+	case I2C_SMBUS_BYTE:
+		xact = I801_BYTE;
+		break;
+	case I2C_SMBUS_BYTE_DATA:
+		if (read_write == I2C_SMBUS_WRITE)
+			outb_p(data->byte, SMBHSTDAT0(priv));
+		xact = I801_BYTE_DATA;
+		break;
+	case I2C_SMBUS_WORD_DATA:
+		if (read_write == I2C_SMBUS_WRITE) {
+			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
+			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+		}
+		xact = I801_WORD_DATA;
+		break;
+	case I2C_SMBUS_PROC_CALL:
+		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
+		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+		xact = I801_PROC_CALL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = i801_transaction(priv, xact);
+	if (ret || read_write == I2C_SMBUS_WRITE)
+		return ret;
+
+	switch (command) {
+	case I2C_SMBUS_BYTE:
+	case I2C_SMBUS_BYTE_DATA:
+		data->byte = inb_p(SMBHSTDAT0(priv));
+		break;
+	case I2C_SMBUS_WORD_DATA:
+	case I2C_SMBUS_PROC_CALL:
+		data->word = inb_p(SMBHSTDAT0(priv)) +
+			     (inb_p(SMBHSTDAT1(priv)) << 8);
+		break;
+	}
+
+	return 0;
+}
+
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
 				  char read_write, int command)
@@ -783,9 +836,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data *data)
 {
-	int hwpec;
-	int block = 0;
-	int ret, xact;
+	int hwpec, ret, block = 0;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
 	mutex_lock(&priv->acpi_lock);
@@ -803,36 +854,23 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		i801_set_hstadd(priv, addr, read_write);
-		xact = I801_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
 		i801_set_hstadd(priv, addr, read_write);
 		if (read_write == I2C_SMBUS_WRITE)
 			outb_p(command, SMBHSTCMD(priv));
-		xact = I801_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
 		i801_set_hstadd(priv, addr, read_write);
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE)
-			outb_p(data->byte, SMBHSTDAT0(priv));
-		xact = I801_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
 		i801_set_hstadd(priv, addr, read_write);
 		outb_p(command, SMBHSTCMD(priv));
-		if (read_write == I2C_SMBUS_WRITE) {
-			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
-			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
-		}
-		xact = I801_WORD_DATA;
 		break;
 	case I2C_SMBUS_PROC_CALL:
 		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
 		outb_p(command, SMBHSTCMD(priv));
-		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
-		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
-		xact = I801_PROC_CALL;
 		read_write = I2C_SMBUS_READ;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
@@ -880,7 +918,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	if (block)
 		ret = i801_block_transaction(priv, data, read_write, size);
 	else
-		ret = i801_transaction(priv, xact);
+		ret = i801_simple_transaction(priv, data, read_write, size);
 
 	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
 	   time, so we forcibly disable it after every transaction. Turn off
@@ -888,26 +926,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	if (hwpec || block)
 		outb_p(inb_p(SMBAUXCTL(priv)) &
 		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
-
-	if (block)
-		goto out;
-	if (ret)
-		goto out;
-	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
-		goto out;
-
-	switch (xact) {
-	case I801_BYTE:	/* Result put in SMBHSTDAT0 */
-	case I801_BYTE_DATA:
-		data->byte = inb_p(SMBHSTDAT0(priv));
-		break;
-	case I801_WORD_DATA:
-	case I801_PROC_CALL:
-		data->word = inb_p(SMBHSTDAT0(priv)) +
-			     (inb_p(SMBHSTDAT1(priv)) << 8);
-		break;
-	}
-
 out:
 	/*
 	 * Unlock the SMBus device for use by BIOS/ACPI,
-- 
2.39.0

Re: [PATCH v2 05/10] i2c: i801: add i801_simple_transaction(), complementing i801_block_transaction()

[Jean Delvare]

On Mon, 19 Dec 2022 19:17:03 +0100, Heiner Kallweit wrote:
> This patch factors out non-block pre/post processing to a new function
> i801_simple_transaction(), complementing existing function
> i801_block_transaction(). This makes i801_access() better readable.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rename new function to i801_simple_transaction
> - code style fixes
> ---
>  drivers/i2c/busses/i2c-i801.c | 92 +++++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 882cf5135..d934d410b 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -732,6 +732,59 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>  	outb_p((addr << 1) | (read_write & 0x01), SMBHSTADD(priv));
>  }
>  
> +/* Single value transaction function */
> +static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> +				   char read_write, int command)
> +{
> +	int xact, ret;
> +
> +	switch (command) {

I did not notice during my initial review, but this parameter name is
confusing, and this becomes even more obvious with patch 7 later. What
you name "command" here is named "size" in function i801_access(). And
what is named "command" there, you will name "hstcmd" in this function
in patch 7. I would prefer to keep the same names (and parameter order)
throughout the function stack for consistency.

Rest is OK.

-- 
Jean Delvare
SUSE L3 Support

[PATCH v2 06/10] i2c: i801: handle SMBAUXCTL_E32B in i801_block_transaction_by_block only

[Heiner Kallweit]

Currently we touch SMBAUXCTL even if not needed. That's the case for block
commands that don't use block buffer mode, either because block buffer
mode isn't available or because it's not supported for the respective
command (e.g. I2C block transfer). Improve this by setting/resetting
SMBAUXCTL_E32B in i801_block_transaction_by_block() only.

Small downside is that we know access SMBAUXCTL twice for transactions
that use PEC and block buffer mode. But this should a rather rare case
and the impact is negligible.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d934d410b..d7182f7c8 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -511,19 +511,23 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
 
 	status = i801_transaction(priv, xact);
 	if (status)
-		return status;
+		goto out;
 
 	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)
-			return -EPROTO;
+		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
+			status = -EPROTO;
+			goto out;
+		}
 
 		data->block[0] = len;
 		for (i = 0; i < len; i++)
 			data->block[i + 1] = inb_p(SMBBLKDAT(priv));
 	}
-	return 0;
+out:
+	outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_E32B, SMBAUXCTL(priv));
+	return status;
 }
 
 static void i801_isr_byte_done(struct i801_priv *priv)
@@ -921,11 +925,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		ret = i801_simple_transaction(priv, data, read_write, size);
 
 	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
-	   time, so we forcibly disable it after every transaction. Turn off
-	   E32B for the same reason. */
-	if (hwpec || block)
-		outb_p(inb_p(SMBAUXCTL(priv)) &
-		       ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv));
+	 * time, so we forcibly disable it after every transaction.
+	 */
+	if (hwpec)
+		outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_CRC, SMBAUXCTL(priv));
 out:
 	/*
 	 * Unlock the SMBus device for use by BIOS/ACPI,
-- 
2.39.0

Re: [PATCH v2 06/10] i2c: i801: handle SMBAUXCTL_E32B in i801_block_transaction_by_block only

[Jean Delvare]

On Mon, 19 Dec 2022 19:20:10 +0100, Heiner Kallweit wrote:
> Currently we touch SMBAUXCTL even if not needed. That's the case for block
> commands that don't use block buffer mode, either because block buffer
> mode isn't available or because it's not supported for the respective
> command (e.g. I2C block transfer). Improve this by setting/resetting
> SMBAUXCTL_E32B in i801_block_transaction_by_block() only.
> 
> Small downside is that we know access SMBAUXCTL twice for transactions

Typo: know -> now.

> that use PEC and block buffer mode. But this should a rather rare case
> and the impact is negligible.

I agree, and the new way also makes things symmetric and thus more
obviously correct.

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

Reviewed-by: Jean Delvare <jdelvare@suse.de>

One possible further improvement step, performance-wise, would be to
store the original value of SMBAUXCTL so that we can skip the inb_p()
at the end of the function. What do you think?

-- 
Jean Delvare
SUSE L3 Support

[PATCH v2 07/10] i2c: i801: centralize configuring non-block commands in i801_simple_transaction

[Heiner Kallweit]

Currently configuring command register settings is disributed over multiple
functions. At first cetralize this for non-block commands in
i801_simple_transaction().

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d7182f7c8..0d49e9587 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -738,35 +738,47 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
 
 /* Single value transaction function */
 static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
-				   char read_write, int command)
+				   u8 addr, u8 hstcmd, char read_write, int command)
 {
 	int xact, ret;
 
 	switch (command) {
 	case I2C_SMBUS_QUICK:
+		i801_set_hstadd(priv, addr, read_write);
 		xact = I801_QUICK;
 		break;
 	case I2C_SMBUS_BYTE:
+		i801_set_hstadd(priv, addr, read_write);
+		if (read_write == I2C_SMBUS_WRITE)
+			outb_p(hstcmd, SMBHSTCMD(priv));
 		xact = I801_BYTE;
 		break;
 	case I2C_SMBUS_BYTE_DATA:
+		i801_set_hstadd(priv, addr, read_write);
 		if (read_write == I2C_SMBUS_WRITE)
 			outb_p(data->byte, SMBHSTDAT0(priv));
+		outb_p(hstcmd, SMBHSTCMD(priv));
 		xact = I801_BYTE_DATA;
 		break;
 	case I2C_SMBUS_WORD_DATA:
+		i801_set_hstadd(priv, addr, read_write);
 		if (read_write == I2C_SMBUS_WRITE) {
 			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
 			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
 		}
+		outb_p(hstcmd, SMBHSTCMD(priv));
 		xact = I801_WORD_DATA;
 		break;
 	case I2C_SMBUS_PROC_CALL:
+		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
 		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
 		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
+		outb_p(hstcmd, SMBHSTCMD(priv));
+		read_write = I2C_SMBUS_READ;
 		xact = I801_PROC_CALL;
 		break;
 	default:
+		pci_err(priv->pci_dev, "Unsupported transaction %d\n", command);
 		return -EOPNOTSUPP;
 	}
 
@@ -857,25 +869,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
-		i801_set_hstadd(priv, addr, read_write);
-		break;
 	case I2C_SMBUS_BYTE:
-		i801_set_hstadd(priv, addr, read_write);
-		if (read_write == I2C_SMBUS_WRITE)
-			outb_p(command, SMBHSTCMD(priv));
-		break;
 	case I2C_SMBUS_BYTE_DATA:
-		i801_set_hstadd(priv, addr, read_write);
-		outb_p(command, SMBHSTCMD(priv));
-		break;
 	case I2C_SMBUS_WORD_DATA:
-		i801_set_hstadd(priv, addr, read_write);
-		outb_p(command, SMBHSTCMD(priv));
-		break;
 	case I2C_SMBUS_PROC_CALL:
-		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
-		outb_p(command, SMBHSTCMD(priv));
-		read_write = I2C_SMBUS_READ;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
 		i801_set_hstadd(priv, addr, read_write);
@@ -922,7 +919,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	if (block)
 		ret = i801_block_transaction(priv, data, read_write, size);
 	else
-		ret = i801_simple_transaction(priv, data, read_write, size);
+		ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
 
 	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
 	 * time, so we forcibly disable it after every transaction.
-- 
2.39.0

Re: [PATCH v2 07/10] i2c: i801: centralize configuring non-block commands in i801_simple_transaction

[Jean Delvare]

Hi Heiner,

On Mon, 19 Dec 2022 19:20:55 +0100, Heiner Kallweit wrote:
> Currently configuring command register settings is disributed over multiple
> functions. At first cetralize this for non-block commands in

Typo: cetralize -> centralize.

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

I'm happy with the idea (well I kind of suggested it in the first
place, so...), and pleased to see that the diffstats confirm it was a
good idea.

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d7182f7c8..0d49e9587 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -738,35 +738,47 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>  
>  /* Single value transaction function */
>  static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> -				   char read_write, int command)
> +				   u8 addr, u8 hstcmd, char read_write, int command)

See my review of patch 5 about how I don't like some of the parameter
names. But I realize now that this problem predates your patches,
functions i801_block_transaction_by_block() and
i801_block_transaction_byte_by_byte() already carry the confusion. So
I'm fine leaving it as is for now, maybe we can clean it up later
driver-wide.

>  {
>  	int xact, ret;
>  
>  	switch (command) {
>  	case I2C_SMBUS_QUICK:
> +		i801_set_hstadd(priv, addr, read_write);
>  		xact = I801_QUICK;
>  		break;
>  	case I2C_SMBUS_BYTE:
> +		i801_set_hstadd(priv, addr, read_write);
> +		if (read_write == I2C_SMBUS_WRITE)
> +			outb_p(hstcmd, SMBHSTCMD(priv));
>  		xact = I801_BYTE;
>  		break;
>  	case I2C_SMBUS_BYTE_DATA:
> +		i801_set_hstadd(priv, addr, read_write);
>  		if (read_write == I2C_SMBUS_WRITE)
>  			outb_p(data->byte, SMBHSTDAT0(priv));
> +		outb_p(hstcmd, SMBHSTCMD(priv));
>  		xact = I801_BYTE_DATA;
>  		break;
>  	case I2C_SMBUS_WORD_DATA:
> +		i801_set_hstadd(priv, addr, read_write);
>  		if (read_write == I2C_SMBUS_WRITE) {
>  			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>  			outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>  		}
> +		outb_p(hstcmd, SMBHSTCMD(priv));
>  		xact = I801_WORD_DATA;
>  		break;
>  	case I2C_SMBUS_PROC_CALL:
> +		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>  		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>  		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> +		outb_p(hstcmd, SMBHSTCMD(priv));
> +		read_write = I2C_SMBUS_READ;
>  		xact = I801_PROC_CALL;
>  		break;
>  	default:
> +		pci_err(priv->pci_dev, "Unsupported transaction %d\n", command);
>  		return -EOPNOTSUPP;
>  	}

The split between patches 7 and 8 isn't really clean, as you have dead
code here at this step, which will only become active with the next
patch. I'm willing to let it go though.

>  
> @@ -857,25 +869,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> -		i801_set_hstadd(priv, addr, read_write);
> -		break;
>  	case I2C_SMBUS_BYTE:
> -		i801_set_hstadd(priv, addr, read_write);
> -		if (read_write == I2C_SMBUS_WRITE)
> -			outb_p(command, SMBHSTCMD(priv));
> -		break;
>  	case I2C_SMBUS_BYTE_DATA:
> -		i801_set_hstadd(priv, addr, read_write);
> -		outb_p(command, SMBHSTCMD(priv));
> -		break;
>  	case I2C_SMBUS_WORD_DATA:
> -		i801_set_hstadd(priv, addr, read_write);
> -		outb_p(command, SMBHSTCMD(priv));
> -		break;
>  	case I2C_SMBUS_PROC_CALL:
> -		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> -		outb_p(command, SMBHSTCMD(priv));
> -		read_write = I2C_SMBUS_READ;
>  		break;
>  	case I2C_SMBUS_BLOCK_DATA:
>  		i801_set_hstadd(priv, addr, read_write);
> @@ -922,7 +919,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  	if (block)
>  		ret = i801_block_transaction(priv, data, read_write, size);
>  	else
> -		ret = i801_simple_transaction(priv, data, read_write, size);
> +		ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
>  
>  	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
>  	 * time, so we forcibly disable it after every transaction.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

[PATCH v2 08/10] i2c: i801: centralize configuring block commands in i801_block_transaction

[Heiner Kallweit]

Similar to the patch for non-block commands, centralize block command
register settings in i801_block_transaction().

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 0d49e9587..78663d8df 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -803,7 +803,7 @@ static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data
 
 /* Block transaction function */
 static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
-				  char read_write, int command)
+				  u8 addr, u8 hstcmd, char read_write, int command)
 {
 	int result = 0;
 	unsigned char hostc;
@@ -813,7 +813,29 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 		return -EPROTO;
 
-	if (command == I2C_SMBUS_I2C_BLOCK_DATA) {
+	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);
@@ -824,6 +846,13 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 				"I2C block read is unsupported!\n");
 			return -EOPNOTSUPP;
 		}
+
+		break;
+	case I2C_SMBUS_BLOCK_PROC_CALL:
+		/* Needs to be flagged as write transaction */
+		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
+		outb_p(hstcmd, SMBHSTCMD(priv));
+		break;
 	}
 
 	/* Experience has shown that the block buffer can only be used for
@@ -852,7 +881,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data *data)
 {
-	int hwpec, ret, block = 0;
+	int hwpec, ret;
 	struct i801_priv *priv = i2c_get_adapdata(adap);
 
 	mutex_lock(&priv->acpi_lock);
@@ -867,57 +896,16 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		&& size != I2C_SMBUS_QUICK
 		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
 
-	switch (size) {
-	case I2C_SMBUS_QUICK:
-	case I2C_SMBUS_BYTE:
-	case I2C_SMBUS_BYTE_DATA:
-	case I2C_SMBUS_WORD_DATA:
-	case I2C_SMBUS_PROC_CALL:
-		break;
-	case I2C_SMBUS_BLOCK_DATA:
-		i801_set_hstadd(priv, addr, read_write);
-		outb_p(command, SMBHSTCMD(priv));
-		block = 1;
-		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(command, SMBHSTDAT1(priv));
-		} else
-			outb_p(command, SMBHSTCMD(priv));
-		block = 1;
-		break;
-	case I2C_SMBUS_BLOCK_PROC_CALL:
-		/* Needs to be flagged as write transaction */
-		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
-		outb_p(command, SMBHSTCMD(priv));
-		block = 1;
-		break;
-	default:
-		dev_err(&priv->pci_dev->dev, "Unsupported transaction %d\n",
-			size);
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
 	if (hwpec)	/* enable/disable hardware PEC */
 		outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_CRC, SMBAUXCTL(priv));
 	else
 		outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC),
 		       SMBAUXCTL(priv));
 
-	if (block)
-		ret = i801_block_transaction(priv, data, read_write, size);
+	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);
 	else
 		ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
 
@@ -926,7 +914,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	 */
 	if (hwpec)
 		outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_CRC, SMBAUXCTL(priv));
-out:
 	/*
 	 * Unlock the SMBus device for use by BIOS/ACPI,
 	 * and clear status flags if not done already.
-- 
2.39.0

Re: [PATCH v2 08/10] i2c: i801: centralize configuring block commands in i801_block_transaction

[Jean Delvare]

On Mon, 19 Dec 2022 19:21:33 +0100, Heiner Kallweit wrote:
> Similar to the patch for non-block commands, centralize block command
> register settings in i801_block_transaction().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 85 +++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 0d49e9587..78663d8df 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> (...)
> @@ -824,6 +846,13 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  				"I2C block read is unsupported!\n");
>  			return -EOPNOTSUPP;
>  		}
> +

No blank line here.

> +		break;
> +	case I2C_SMBUS_BLOCK_PROC_CALL:
> +		/* Needs to be flagged as write transaction */
> +		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> +		outb_p(hstcmd, SMBHSTCMD(priv));
> +		break;
>  	}
>  
>  	/* Experience has shown that the block buffer can only be used for

Other that this minor style issue, looks good, nice clean-up.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

[PATCH v2 09/10] i2c: i801: call i801_check_pre() from i801_access()

[Heiner Kallweit]

This avoids code duplication, in a next step we'll call
i801_check_post() from i801_transaction() as well.

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 78663d8df..4b5fb2c61 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -462,10 +462,6 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 	unsigned long result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
-	status = i801_check_pre(priv);
-	if (status < 0)
-		return status;
-
 	if (priv->features & FEATURE_IRQ) {
 		reinit_completion(&priv->done);
 		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
@@ -653,10 +649,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	if (command == I2C_SMBUS_BLOCK_PROC_CALL)
 		return -EOPNOTSUPP;
 
-	status = i801_check_pre(priv);
-	if (status < 0)
-		return status;
-
 	len = data->block[0];
 
 	if (read_write == I2C_SMBUS_WRITE) {
@@ -892,6 +884,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 
 	pm_runtime_get_sync(&priv->pci_dev->dev);
 
+	ret = i801_check_pre(priv);
+	if (ret)
+		goto out;
+
 	hwpec = (priv->features & FEATURE_SMBUS_PEC) && (flags & I2C_CLIENT_PEC)
 		&& size != I2C_SMBUS_QUICK
 		&& size != I2C_SMBUS_I2C_BLOCK_DATA;
@@ -914,6 +910,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	 */
 	if (hwpec)
 		outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_CRC, SMBAUXCTL(priv));
+out:
 	/*
 	 * Unlock the SMBus device for use by BIOS/ACPI,
 	 * and clear status flags if not done already.
-- 
2.39.0

[PATCH v2 10/10] i2c: i801: call i801_check_post() from i801_access()

[Heiner Kallweit]

Avoid code duplication by calling i801_check_post() from i801_access().

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 4b5fb2c61..7313efadf 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -434,7 +434,7 @@ static int i801_wait_intr(struct i801_priv *priv)
 		busy = status & SMBHSTSTS_HOST_BUSY;
 		status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
 		if (!busy && status)
-			return status;
+			return status & STATUS_ERROR_FLAGS;
 	} while (time_is_after_eq_jiffies(timeout));
 
 	return -ETIMEDOUT;
@@ -458,7 +458,6 @@ static int i801_wait_byte_done(struct i801_priv *priv)
 
 static int i801_transaction(struct i801_priv *priv, int xact)
 {
-	int status;
 	unsigned long result;
 	const struct i2c_adapter *adap = &priv->adapter;
 
@@ -467,13 +466,12 @@ static int i801_transaction(struct i801_priv *priv, int xact)
 		outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
 		       SMBHSTCNT(priv));
 		result = wait_for_completion_timeout(&priv->done, adap->timeout);
-		return i801_check_post(priv, result ? priv->status : -ETIMEDOUT);
+		return result ? priv->status : -ETIMEDOUT;
 	}
 
 	outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
 
-	status = i801_wait_intr(priv);
-	return i801_check_post(priv, status);
+	return i801_wait_intr(priv);
 }
 
 static int i801_block_transaction_by_block(struct i801_priv *priv,
@@ -624,7 +622,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
 
 	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
 	if (status) {
-		priv->status = status;
+		priv->status = status & STATUS_ERROR_FLAGS;
 		complete(&priv->done);
 	}
 
@@ -674,7 +672,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		reinit_completion(&priv->done);
 		outb_p(priv->cmd | SMBHSTCNT_START, SMBHSTCNT(priv));
 		result = wait_for_completion_timeout(&priv->done, adap->timeout);
-		return i801_check_post(priv, result ? priv->status : -ETIMEDOUT);
+		return result ? priv->status : -ETIMEDOUT;
 	}
 
 	for (i = 1; i <= len; i++) {
@@ -688,7 +686,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 
 		status = i801_wait_byte_done(priv);
 		if (status)
-			goto exit;
+			return status;
 
 		if (i == 1 && read_write == I2C_SMBUS_READ
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
@@ -718,9 +716,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
 	}
 
-	status = i801_wait_intr(priv);
-exit:
-	return i801_check_post(priv, status);
+	return i801_wait_intr(priv);
 }
 
 static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
@@ -905,6 +901,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	else
 		ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
 
+	ret = i801_check_post(priv, ret);
+
 	/* Some BIOSes don't like it when PEC is enabled at reboot or resume
 	 * time, so we forcibly disable it after every transaction.
 	 */
-- 
2.39.0

Re: [PATCH v2 10/10] i2c: i801: call i801_check_post() from i801_access()

[Jean Delvare]

Hi Heiner,

On Mon, 19 Dec 2022 19:22:42 +0100, Heiner Kallweit wrote:
> Avoid code duplication by calling i801_check_post() from i801_access().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> (...)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

So I'm done reviewing the series. I have also tested the result,
successfully, albeit my coverage is limited.

Patches 1-4 have been committed by Wolfram already. Patches 6, 7 and 8
have minor issues. I can take care of resubmitting if you want.

My suggested changes (renaming function parameters and storing the
original value of SMBAUXCTL) are better implemented on top of that
later (if you agree with them) so as to not delay this series even
further.

Thanks for your work,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 10/10] i2c: i801: call i801_check_post() from i801_access()

[Heiner Kallweit]

On 15.02.2023 18:09, Jean Delvare wrote:
> Hi Heiner,
> 
> On Mon, 19 Dec 2022 19:22:42 +0100, Heiner Kallweit wrote:
>> Avoid code duplication by calling i801_check_post() from i801_access().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>> (...)
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> So I'm done reviewing the series. I have also tested the result,
> successfully, albeit my coverage is limited.
> 
> Patches 1-4 have been committed by Wolfram already. Patches 6, 7 and 8
> have minor issues. I can take care of resubmitting if you want.
> 
This would be great, thanks!

> My suggested changes (renaming function parameters and storing the
> original value of SMBAUXCTL) are better implemented on top of that
> later (if you agree with them) so as to not delay this series even
> further.
> 
> Thanks for your work,