[PATCH 0/8] i2c: i801: Series with minor improvements

[PATCH 0/8] 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.

Heiner Kallweit (8):
  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: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip
    versions
  i2c: i801: add helper i801_set_hstadd
  i2c: i801: add i801_single_transaction(), complementing
    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 | 339 ++++++++++++++++++----------------
 1 file changed, 175 insertions(+), 164 deletions(-)

-- 
2.35.3

[PATCH 1/8] 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>
---
 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 ff706349b..c481f121d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -556,9 +556,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)
@@ -588,7 +585,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.
@@ -613,23 +609,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 & STATUS_ERROR_FLAGS))
 		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.35.3

Re: [PATCH 1/8] i2c: i801: improve interrupt handler

[Jean Delvare]

Hi Heiner, 

On Fri, 15 Apr 2022 18:54:32 +0200, 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>
> ---
>  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 ff706349b..c481f121d 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -556,9 +556,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)
> @@ -588,7 +585,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.
> @@ -613,23 +609,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 & STATUS_ERROR_FLAGS))

Isn't this better written

	if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)

? At least my compiler generates smaller binary code.

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

You are making the call to outb_p() unconditional. Is this under the
assumption that at least one of the bits must be set, so the condition
was always met?

> +
> +	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>  	if (status) {
>  		priv->status = status;
>  		complete(&priv->done);

Tested OK on my system, looks good overall, nice simplification.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/8] i2c: i801: improve interrupt handler

[Heiner Kallweit]

On 07.06.2022 14:34, Jean Delvare wrote:
> Hi Heiner, 
> 
> On Fri, 15 Apr 2022 18:54:32 +0200, 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>
>> ---
>>  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 ff706349b..c481f121d 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -556,9 +556,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)
>> @@ -588,7 +585,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.
>> @@ -613,23 +609,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 & STATUS_ERROR_FLAGS))
> 
> Isn't this better written
> 
> 	if ((status & (SMBHSTSTS_BYTE_DONE | STATUS_ERROR_FLAGS)) == SMBHSTSTS_BYTE_DONE)
> 
> ? At least my compiler generates smaller binary code.
> 
OK. Meanwhile I prefer readability over saving few bytes, but here IMO
the readability doesn't suffer.

>>  		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));
> 
> You are making the call to outb_p() unconditional. Is this under the
> assumption that at least one of the bits must be set, so the condition
> was always met?
> 
So far we exclude here the case where SMBHSTSTS is written in
i801_isr_byte_done(). The patch allows to write SMBHSTSTS here only,
so we don't need the condition any longer.

>> +
>> +	status &= STATUS_ERROR_FLAGS | SMBHSTSTS_INTR;
>>  	if (status) {
>>  		priv->status = status;
>>  		complete(&priv->done);
> 
> Tested OK on my system, looks good overall, nice simplification.
> 

[PATCH 2/8] 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>
---
 drivers/i2c/busses/i2c-i801.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index c481f121d..eccdc7035 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1756,6 +1756,9 @@ 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");
 
+	if (!(priv->features & FEATURE_IRQ))
+		priv->features &= ~FEATURE_HOST_NOTIFY;
+
 	i801_add_tco(priv);
 
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
-- 
2.35.3

Re: [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote:
> Host notification uses an interrupt, therefore it makes sense only if
> interrupts are enabled.

It would be nice to have this comment in the code itself.

> Make this dependency explicit by removing
> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index c481f121d..eccdc7035 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1756,6 +1756,9 @@ 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");
>  
> +	if (!(priv->features & FEATURE_IRQ))
> +		priv->features &= ~FEATURE_HOST_NOTIFY;

Earlier in this function, there's an action which depends on the
FEATURE_HOST_NOTIFY flag being set. While this will only result in a
useless action and wouldn't cause a bug as far as I can see, wouldn't
it be cleaner to move that piece of code *after* the check you're
adding?

> +
>  	i801_add_tco(priv);
>  
>  	snprintf(priv->adapter.name, sizeof(priv->adapter.name),

Looks good, tested OK on my system (non-regression only, I'm not using
the Host Notify feature).

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/8] i2c: i801: make FEATURE_HOST_NOTIFY dependent on FEATURE_IRQ

[Heiner Kallweit]

On 07.06.2022 14:48, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:55:10 +0200, Heiner Kallweit wrote:
>> Host notification uses an interrupt, therefore it makes sense only if
>> interrupts are enabled.
> 
> It would be nice to have this comment in the code itself.
> 
OK

>> Make this dependency explicit by removing
>> FEATURE_HOST_NOTIFY if FEATURE_IRQ isn't set.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index c481f121d..eccdc7035 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1756,6 +1756,9 @@ 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");
>>  
>> +	if (!(priv->features & FEATURE_IRQ))
>> +		priv->features &= ~FEATURE_HOST_NOTIFY;
> 
> Earlier in this function, there's an action which depends on the
> FEATURE_HOST_NOTIFY flag being set. While this will only result in a
> useless action and wouldn't cause a bug as far as I can see, wouldn't
> it be cleaner to move that piece of code *after* the check you're
> adding?
> 
Yes, this would be better. Will be part of v2.

>> +
>>  	i801_add_tco(priv);
>>  
>>  	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> 
> Looks good, tested OK on my system (non-regression only, I'm not using
> the Host Notify feature).
> 

[PATCH 3/8] 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>
---
 drivers/i2c/busses/i2c-i801.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eccdc7035..1d8182901 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	}
 	priv->features &= ~disable_features;
 
+	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.35.3

Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:55:46 +0200, 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.

In which datasheet are you seeing this? Can you point me to the
specific section and/or quote the statement? I can't find it in the
datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
huge and I may just be missing it.

Also, same request as previous patch, I'd like a comment in the code,
so that developers don't have to read the git log to figure out why this
piece of code is there.

Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
affects the value returned by i801_func(). i801_access() does not
verify whether this flag is set before processing a command where size
== I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
only partial (will work if the device driver calls .functionality as it
is supposed to, will fail with - I suppose - unpredictable results if
the device driver calls .smbus_xfer directly).

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eccdc7035..1d8182901 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	}
>  	priv->features &= ~disable_features;
>  
> +	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",

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Heiner Kallweit]

On 07.06.2022 16:13, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:55:46 +0200, 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.
> 
> In which datasheet are you seeing this? Can you point me to the
> specific section and/or quote the statement? I can't find it in the
> datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
> huge and I may just be missing it.
> 

I used the following datasheet:
Intel 9 Series Chipset Family PCH
330550-002

On page 211 the block process call is described.
There's a note: E32B bit in the Auxiliary Control register must be set when using this protocol.
The same note can be found on page 663.

> Also, same request as previous patch, I'd like a comment in the code,
> so that developers don't have to read the git log to figure out why this
> piece of code is there.
> 
OK

> Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
> affects the value returned by i801_func(). i801_access() does not
> verify whether this flag is set before processing a command where size
> == I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
> only partial (will work if the device driver calls .functionality as it
> is supposed to, will fail with - I suppose - unpredictable results if
> the device driver calls .smbus_xfer directly).
> 
If FEATURE_BLOCK_PROC isn't set then we would call
i801_block_transaction_byte_by_byte() according to the following code
in i801_block_transaction().

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

And i801_block_transaction_byte_by_byte() immediately returns
-EOPNOTSUPP in case of command == I2C_SMBUS_BLOCK_PROC_CALL.

Having said that the requested check is there, it's just executed
a little bit later.

>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index eccdc7035..1d8182901 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  	}
>>  	priv->features &= ~disable_features;
>>  
>> +	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",
> 
> Thanks,

[PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Heiner Kallweit]

According to the datasheets interrupt mode and i2c block read are
supported on all chip versions. Therefore set both feature flags for
all chip versions.
Note: Don't remove the two feature flags as such (at least for now),
so that in case of a problem users can use the disable_features
module parameter to disable a problematic feature.

Patch is based solely on the datasheets. I don't have old enough
test hw, therefore the patch is compile-tested only.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 1d8182901..a9737f14d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -12,70 +12,70 @@
 /*
  * Supports the following Intel I/O Controller Hubs (ICH):
  *
- *					I/O			Block	I2C
- *					region	SMBus	Block	proc.	block
- * Chip name			PCI ID	size	PEC	buffer	call	read
- * ---------------------------------------------------------------------------
- * 82801AA (ICH)		0x2413	16	no	no	no	no
- * 82801AB (ICH0)		0x2423	16	no	no	no	no
- * 82801BA (ICH2)		0x2443	16	no	no	no	no
- * 82801CA (ICH3)		0x2483	32	soft	no	no	no
- * 82801DB (ICH4)		0x24c3	32	hard	yes	no	no
- * 82801E (ICH5)		0x24d3	32	hard	yes	yes	yes
- * 6300ESB			0x25a4	32	hard	yes	yes	yes
- * 82801F (ICH6)		0x266a	32	hard	yes	yes	yes
- * 6310ESB/6320ESB		0x269b	32	hard	yes	yes	yes
- * 82801G (ICH7)		0x27da	32	hard	yes	yes	yes
- * 82801H (ICH8)		0x283e	32	hard	yes	yes	yes
- * 82801I (ICH9)		0x2930	32	hard	yes	yes	yes
- * EP80579 (Tolapai)		0x5032	32	hard	yes	yes	yes
- * ICH10			0x3a30	32	hard	yes	yes	yes
- * ICH10			0x3a60	32	hard	yes	yes	yes
- * 5/3400 Series (PCH)		0x3b30	32	hard	yes	yes	yes
- * 6 Series (PCH)		0x1c22	32	hard	yes	yes	yes
- * Patsburg (PCH)		0x1d22	32	hard	yes	yes	yes
- * Patsburg (PCH) IDF		0x1d70	32	hard	yes	yes	yes
- * Patsburg (PCH) IDF		0x1d71	32	hard	yes	yes	yes
- * Patsburg (PCH) IDF		0x1d72	32	hard	yes	yes	yes
- * DH89xxCC (PCH)		0x2330	32	hard	yes	yes	yes
- * Panther Point (PCH)		0x1e22	32	hard	yes	yes	yes
- * Lynx Point (PCH)		0x8c22	32	hard	yes	yes	yes
- * Lynx Point-LP (PCH)		0x9c22	32	hard	yes	yes	yes
- * Avoton (SOC)			0x1f3c	32	hard	yes	yes	yes
- * Wellsburg (PCH)		0x8d22	32	hard	yes	yes	yes
- * Wellsburg (PCH) MS		0x8d7d	32	hard	yes	yes	yes
- * Wellsburg (PCH) MS		0x8d7e	32	hard	yes	yes	yes
- * Wellsburg (PCH) MS		0x8d7f	32	hard	yes	yes	yes
- * Coleto Creek (PCH)		0x23b0	32	hard	yes	yes	yes
- * Wildcat Point (PCH)		0x8ca2	32	hard	yes	yes	yes
- * Wildcat Point-LP (PCH)	0x9ca2	32	hard	yes	yes	yes
- * BayTrail (SOC)		0x0f12	32	hard	yes	yes	yes
- * Braswell (SOC)		0x2292	32	hard	yes	yes	yes
- * Sunrise Point-H (PCH) 	0xa123  32	hard	yes	yes	yes
- * Sunrise Point-LP (PCH)	0x9d23	32	hard	yes	yes	yes
- * DNV (SOC)			0x19df	32	hard	yes	yes	yes
- * Emmitsburg (PCH)		0x1bc9	32	hard	yes	yes	yes
- * Broxton (SOC)		0x5ad4	32	hard	yes	yes	yes
- * Lewisburg (PCH)		0xa1a3	32	hard	yes	yes	yes
- * Lewisburg Supersku (PCH)	0xa223	32	hard	yes	yes	yes
- * Kaby Lake PCH-H (PCH)	0xa2a3	32	hard	yes	yes	yes
- * Gemini Lake (SOC)		0x31d4	32	hard	yes	yes	yes
- * Cannon Lake-H (PCH)		0xa323	32	hard	yes	yes	yes
- * Cannon Lake-LP (PCH)		0x9da3	32	hard	yes	yes	yes
- * Cedar Fork (PCH)		0x18df	32	hard	yes	yes	yes
- * Ice Lake-LP (PCH)		0x34a3	32	hard	yes	yes	yes
- * Ice Lake-N (PCH)		0x38a3	32	hard	yes	yes	yes
- * Comet Lake (PCH)		0x02a3	32	hard	yes	yes	yes
- * Comet Lake-H (PCH)		0x06a3	32	hard	yes	yes	yes
- * Elkhart Lake (PCH)		0x4b23	32	hard	yes	yes	yes
- * Tiger Lake-LP (PCH)		0xa0a3	32	hard	yes	yes	yes
- * Tiger Lake-H (PCH)		0x43a3	32	hard	yes	yes	yes
- * Jasper Lake (SOC)		0x4da3	32	hard	yes	yes	yes
- * Comet Lake-V (PCH)		0xa3a3	32	hard	yes	yes	yes
- * Alder Lake-S (PCH)		0x7aa3	32	hard	yes	yes	yes
- * Alder Lake-P (PCH)		0x51a3	32	hard	yes	yes	yes
- * Alder Lake-M (PCH)		0x54a3	32	hard	yes	yes	yes
- * Raptor Lake-S (PCH)		0x7a23	32	hard	yes	yes	yes
+ *					I/O			Block
+ *					region	SMBus	Block	proc.
+ * Chip name			PCI ID	size	PEC	buffer	call
+ * -------------------------------------------------------------------
+ * 82801AA (ICH)		0x2413	16	no	no	no
+ * 82801AB (ICH0)		0x2423	16	no	no	no
+ * 82801BA (ICH2)		0x2443	16	no	no	no
+ * 82801CA (ICH3)		0x2483	32	soft	no	no
+ * 82801DB (ICH4)		0x24c3	32	hard	yes	no
+ * 82801E (ICH5)		0x24d3	32	hard	yes	yes
+ * 6300ESB			0x25a4	32	hard	yes	yes
+ * 82801F (ICH6)		0x266a	32	hard	yes	yes
+ * 6310ESB/6320ESB		0x269b	32	hard	yes	yes
+ * 82801G (ICH7)		0x27da	32	hard	yes	yes
+ * 82801H (ICH8)		0x283e	32	hard	yes	yes
+ * 82801I (ICH9)		0x2930	32	hard	yes	yes
+ * EP80579 (Tolapai)		0x5032	32	hard	yes	yes
+ * ICH10			0x3a30	32	hard	yes	yes
+ * ICH10			0x3a60	32	hard	yes	yes
+ * 5/3400 Series (PCH)		0x3b30	32	hard	yes	yes
+ * 6 Series (PCH)		0x1c22	32	hard	yes	yes
+ * Patsburg (PCH)		0x1d22	32	hard	yes	yes
+ * Patsburg (PCH) IDF		0x1d70	32	hard	yes	yes
+ * Patsburg (PCH) IDF		0x1d71	32	hard	yes	yes
+ * Patsburg (PCH) IDF		0x1d72	32	hard	yes	yes
+ * DH89xxCC (PCH)		0x2330	32	hard	yes	yes
+ * Panther Point (PCH)		0x1e22	32	hard	yes	yes
+ * Lynx Point (PCH)		0x8c22	32	hard	yes	yes
+ * Lynx Point-LP (PCH)		0x9c22	32	hard	yes	yes
+ * Avoton (SOC)			0x1f3c	32	hard	yes	yes
+ * Wellsburg (PCH)		0x8d22	32	hard	yes	yes
+ * Wellsburg (PCH) MS		0x8d7d	32	hard	yes	yes
+ * Wellsburg (PCH) MS		0x8d7e	32	hard	yes	yes
+ * Wellsburg (PCH) MS		0x8d7f	32	hard	yes	yes
+ * Coleto Creek (PCH)		0x23b0	32	hard	yes	yes
+ * Wildcat Point (PCH)		0x8ca2	32	hard	yes	yes
+ * Wildcat Point-LP (PCH)	0x9ca2	32	hard	yes	yes
+ * BayTrail (SOC)		0x0f12	32	hard	yes	yes
+ * Braswell (SOC)		0x2292	32	hard	yes	yes
+ * Sunrise Point-H (PCH)	0xa123  32	hard	yes	yes
+ * Sunrise Point-LP (PCH)	0x9d23	32	hard	yes	yes
+ * DNV (SOC)			0x19df	32	hard	yes	yes
+ * Emmitsburg (PCH)		0x1bc9	32	hard	yes	yes
+ * Broxton (SOC)		0x5ad4	32	hard	yes	yes
+ * Lewisburg (PCH)		0xa1a3	32	hard	yes	yes
+ * Lewisburg Supersku (PCH)	0xa223	32	hard	yes	yes
+ * Kaby Lake PCH-H (PCH)	0xa2a3	32	hard	yes	yes
+ * Gemini Lake (SOC)		0x31d4	32	hard	yes	yes
+ * Cannon Lake-H (PCH)		0xa323	32	hard	yes	yes
+ * Cannon Lake-LP (PCH)		0x9da3	32	hard	yes	yes
+ * Cedar Fork (PCH)		0x18df	32	hard	yes	yes
+ * Ice Lake-LP (PCH)		0x34a3	32	hard	yes	yes
+ * Ice Lake-N (PCH)		0x38a3	32	hard	yes	yes
+ * Comet Lake (PCH)		0x02a3	32	hard	yes	yes
+ * Comet Lake-H (PCH)		0x06a3	32	hard	yes	yes
+ * Elkhart Lake (PCH)		0x4b23	32	hard	yes	yes
+ * Tiger Lake-LP (PCH)		0xa0a3	32	hard	yes	yes
+ * Tiger Lake-H (PCH)		0x43a3	32	hard	yes	yes
+ * Jasper Lake (SOC)		0x4da3	32	hard	yes	yes
+ * Comet Lake-V (PCH)		0xa3a3	32	hard	yes	yes
+ * Alder Lake-S (PCH)		0x7aa3	32	hard	yes	yes
+ * Alder Lake-P (PCH)		0x51a3	32	hard	yes	yes
+ * Alder Lake-M (PCH)		0x54a3	32	hard	yes	yes
+ * Raptor Lake-S (PCH)		0x7a23	32	hard	yes	yes
  *
  * Features supported by this driver:
  * Software PEC				no
@@ -168,7 +168,7 @@
 #define I801_WORD_DATA		0x0C
 #define I801_PROC_CALL		0x10
 #define I801_BLOCK_DATA		0x14
-#define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
+#define I801_I2C_BLOCK_DATA	0x18
 #define I801_BLOCK_PROC_CALL	0x1C
 
 /* I801 Host Control register bits */
@@ -973,11 +973,8 @@ 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_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
+#define FEATURES_ICH5	(FEATURES_ICH4 | FEATURE_BLOCK_PROC)
 
 static const struct pci_device_id i801_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, 82801AA_3,		0)				 },
@@ -1665,7 +1662,8 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	mutex_init(&priv->acpi_lock);
 
 	priv->pci_dev = dev;
-	priv->features = id->driver_data;
+	priv->features = FEATURE_IRQ | FEATURE_I2C_BLOCK_READ;
+	priv->features |= id->driver_data;
 
 	/* Disable features on user request */
 	for (i = 0; i < ARRAY_SIZE(i801_feature_names); i++) {
-- 
2.35.3

Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:56:30 +0200, Heiner Kallweit wrote:
> According to the datasheets interrupt mode and i2c block read are
> supported on all chip versions. Therefore set both feature flags for
> all chip versions.

While the datasheets do match your claims (I checked the 82801CAM aka
ICH3-M datasheet), I have a hard time believing we would have made the
feature device-dependent without a good reason (and I have vague
memories that there was a problem, although I can't find any proof of
that).

So I'll try to resurrect my old ICH3-M-based laptop and test these
changes on it. If I manage to get a Linux distribution to install on
that 20-year-old system...

> Note: Don't remove the two feature flags as such (at least for now),
> so that in case of a problem users can use the disable_features
> module parameter to disable a problematic feature.

Agreed.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Jean Delvare]

Hi Heiner,

I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
work. I get the following error in the kernel log:

[13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
[13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
[13565.434728] Pid: 321, comm: udevd Tainted: G           O 3.4.63-2.44-default #1
[13565.434734] Call Trace:
[13565.434773]  [<c0205349>] try_stack_unwind+0x199/0x1b0
[13565.434793]  [<c02041c7>] dump_trace+0x47/0xf0
[13565.434808]  [<c02053ab>] show_trace_log_lvl+0x4b/0x60
[13565.434820]  [<c02053d8>] show_trace+0x18/0x20
[13565.434837]  [<c0682e81>] dump_stack+0x6d/0x72
[13565.434855]  [<c02adad1>] __report_bad_irq+0x21/0xc0
[13565.434870]  [<c02adef1>] note_interrupt+0x181/0x1d0
[13565.434887]  [<c02abe9e>] handle_irq_event_percpu+0x9e/0x1d0
[13565.434901]  [<c02abffe>] handle_irq_event+0x2e/0x50
[13565.434915]  [<c02ae3e6>] handle_level_irq+0x56/0x90
[13565.434928]  [<c0204168>] handle_irq+0x88/0xa0
[13565.434939] handlers:
[13565.434949] [<c04bbf6c>] acpi_irq
[13565.435027] [<e0baad00>] usb_hcd_irq
[13565.435054] [<e0baad00>] usb_hcd_irq
[13565.435079] [<e0baad00>] usb_hcd_irq
[13565.435193] [<e0d6fb80>] radeon_driver_irq_handler_kms
[13565.435206] [<e0af5c60>] yenta_interrupt
[13565.435214] [<e0af5c60>] yenta_interrupt
[13565.435227] [<e0b514c0>] irq_handler
[13565.435238] [<e0aeec90>] snd_intel8x0m_interrupt
[13565.435251] [<e0c514a0>] snd_intel8x0_interrupt
[13565.435264] [<e0ab4060>] e100_intr
[13565.435278] [<e09465d0>] i801_isr
[13565.435283] Disabling IRQ #9
[13565.437114] i801_smbus 0000:00:1f.3: Transaction timeout
[13565.437125] i801_smbus 0000:00:1f.3: Terminating the current operation
[13565.439189] i801_smbus 0000:00:1f.3: Failed terminating the transaction

If it matters, and as seen in the list of handlers above, IRQ9 is
heavily shared on this laptop (ACPI, USB, graphics, PCMCIA, sound,
network and SMBus).


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Jean Delvare]

Hi Heiner,

On Mon, 13 Jun 2022 19:08:41 +0200, Jean Delvare wrote:
> I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
> on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
> work. I get the following error in the kernel log:
> 
> [13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
> [13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)

And now it's all coming back to me. The reason why we did not enable
interrupts on chipsets older than the ICH5 is because they lack bit
INTS in PCI register PCISTS. When the IRQ line is shared, we don't know
whether the interrupt was caused by our device or by another device.
Specifically, the following piece of code fails (it returns IRQ_NONE
unconditionally):

static irqreturn_t i801_isr(int irq, void *dev_id)
{
	(...)
	/* Confirm this is our interrupt */
	pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
	if (!(pcists & PCI_STATUS_INTERRUPT))
		return IRQ_NONE;

I tried replacing the code above by a check on SMBHSTSTS instead:

	status = inb_p(SMBHSTSTS(priv));
	if (!(status & STATUS_FLAGS))
		return IRQ_NONE;

It seems to work, however my testing is limited and I don't know how
reliable that would be if the other devices sharing the interrupt line
were used heavily at the same time (the laptop is idling in text mode
at the moment so the fact that the interrupt line is heavily shared
does not really get exercised).

Then I loaded the driver with interrupts disabled
(disable_features=0x10) to see if I2C block read was working. It is NOT
working, as can be seen from the following dumps from the memory SPD
EEPROM:

linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 b
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 80 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01    ??????@.?uT.??.?
10: 8f 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20    ?????.??`..???, 
20: 15 08 15 08 00 00 00 00 00 00 00 00 00 00 00 00    ????............
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 b9    ..............??
40: 7f da 00 00 00 00 00 00 53 53 50 31 33 33 2d 30    ??......SSP133-0
50: 36 34 33 32 33 4a 00 00 00 00 00 00 00 01 02 00    64323J.......??.
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 64 cd    ..............d?
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................

linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 i
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
10: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
20: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
30: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
40: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
50: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
60: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
70: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
80: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
90: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
a0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
b0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
c0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
d0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
e0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
f0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?

As you can see, the requested offset of the I2C block read (0x00, then
0x20, then 0x40 etc.) is being ignored, and instead every I2C block
read starts at EEPROM offset 0x01.

If you compare the datasheets of the ICH5 (Intel doc 252516-001) and
ICH3-M (Intel doc 290716-001), section "I2C Read", you will see that
the description of the command is different. The format described for
the ICH5 (table 114 in the datasheet) does match what the Linux i2c
stack calls an I2C block read, while the format described for the
ICH3-M (table 5-87 in the datasheet) does NOT. Apparently the original
implementation was aimed at specific devices using 10-bit I2C
addressing. As no other SMBus host device implemented that, we did not
even allocate an SMBus command constant to it (and the fact that Intel
changed the format in later hardware iterations proves that this was the
right thing to do).

Looking at the ICH3-M implementation of the "I2C Block Read" transfer,
I feel very lucky that I did not trash my memory SPD EEPROM while
running the command. Because the format really starts with a WRITE of 2
bytes to the EEPROM before reading from it.

So at least the I2C block read part of the patch is never going to
work. The interrupt part could work if we change the test as described
above, however I would question the relevance of making that change
considering that it is no longer the obvious clean-up you originally
proposed, and would then impact recent hardware too. I would hate to
introduce a regression for the sole purpose of enabling interrupts on
20-year old hardware which I doubt many people are still using.

I am available to perform any test you want me to on this old laptop.
As long as it runs...

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/8] i2c: i801: enable FEATURE_IRQ and FEATURE_I2C_BLOCK_READ on all chip versions

[Heiner Kallweit]

On 14.06.2022 14:59, Jean Delvare wrote:
> Hi Heiner,
> 
> On Mon, 13 Jun 2022 19:08:41 +0200, Jean Delvare wrote:
>> I was able to resurrect my Sony Vaio GR214EP laptop to test this patch
>> on an Intel 82801CAM (ICH3-M) chipset. And as I suspected, it does not
>> work. I get the following error in the kernel log:
>>
>> [13563.411101] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
>> [13565.434712] irq 9: nobody cared (try booting with the "irqpoll" option)
> 
> And now it's all coming back to me. The reason why we did not enable
> interrupts on chipsets older than the ICH5 is because they lack bit
> INTS in PCI register PCISTS. When the IRQ line is shared, we don't know
> whether the interrupt was caused by our device or by another device.
> Specifically, the following piece of code fails (it returns IRQ_NONE
> unconditionally):
> 
> static irqreturn_t i801_isr(int irq, void *dev_id)
> {
> 	(...)
> 	/* Confirm this is our interrupt */
> 	pci_read_config_word(priv->pci_dev, PCI_STATUS, &pcists);
> 	if (!(pcists & PCI_STATUS_INTERRUPT))
> 		return IRQ_NONE;
> 
> I tried replacing the code above by a check on SMBHSTSTS instead:
> 
> 	status = inb_p(SMBHSTSTS(priv));
> 	if (!(status & STATUS_FLAGS))
> 		return IRQ_NONE;
> 
> It seems to work, however my testing is limited and I don't know how
> reliable that would be if the other devices sharing the interrupt line
> were used heavily at the same time (the laptop is idling in text mode
> at the moment so the fact that the interrupt line is heavily shared
> does not really get exercised).
> 
> Then I loaded the driver with interrupts disabled
> (disable_features=0x10) to see if I2C block read was working. It is NOT
> working, as can be seen from the following dumps from the memory SPD
> EEPROM:
> 
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 b
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 80 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01    ??????@.?uT.??.?
> 10: 8f 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20    ?????.??`..???, 
> 20: 15 08 15 08 00 00 00 00 00 00 00 00 00 00 00 00    ????............
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 12 b9    ..............??
> 40: 7f da 00 00 00 00 00 00 53 53 50 31 33 33 2d 30    ??......SSP133-0
> 50: 36 34 33 32 33 4a 00 00 00 00 00 00 00 01 02 00    64323J.......??.
> 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
> 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 64 cd    ..............d?
> 80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> 90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
> 
> linux-w6i1:/home/jdelvare/src/i2c-i801 # i2cdump -y 4 0x50 i
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
> 00: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 10: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 20: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 30: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 40: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 50: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 60: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 70: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 80: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> 90: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> a0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> b0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> c0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> d0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> e0: 08 04 0d 09 02 40 00 01 75 54 00 82 10 00 01 8f    ?????@.?uT.??.??
> f0: 04 06 01 01 00 0e a0 60 00 00 14 0f 14 2c 20 15    ????.??`..???, ?
> 
> As you can see, the requested offset of the I2C block read (0x00, then
> 0x20, then 0x40 etc.) is being ignored, and instead every I2C block
> read starts at EEPROM offset 0x01.
> 
> If you compare the datasheets of the ICH5 (Intel doc 252516-001) and
> ICH3-M (Intel doc 290716-001), section "I2C Read", you will see that
> the description of the command is different. The format described for
> the ICH5 (table 114 in the datasheet) does match what the Linux i2c
> stack calls an I2C block read, while the format described for the
> ICH3-M (table 5-87 in the datasheet) does NOT. Apparently the original
> implementation was aimed at specific devices using 10-bit I2C
> addressing. As no other SMBus host device implemented that, we did not
> even allocate an SMBus command constant to it (and the fact that Intel
> changed the format in later hardware iterations proves that this was the
> right thing to do).
> 
> Looking at the ICH3-M implementation of the "I2C Block Read" transfer,
> I feel very lucky that I did not trash my memory SPD EEPROM while
> running the command. Because the format really starts with a WRITE of 2
> bytes to the EEPROM before reading from it.
> 
> So at least the I2C block read part of the patch is never going to
> work. The interrupt part could work if we change the test as described
> above, however I would question the relevance of making that change
> considering that it is no longer the obvious clean-up you originally
> proposed, and would then impact recent hardware too. I would hate to
> introduce a regression for the sole purpose of enabling interrupts on
> 20-year old hardware which I doubt many people are still using.
> 
> I am available to perform any test you want me to on this old laptop.
> As long as it runs...
> 
Thanks for testing! So I'll drop this patch from the series.

[PATCH 5/8] 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>
---
 drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9737f14d..bf77f8640 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 	return result;
 }
 
+static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
+{
+	addr <<= 1;
+	if (read_write == I2C_SMBUS_READ)
+		addr |= 0x01;
+	outb_p(addr, SMBHSTADD(priv));
+}
+
 /* Return negative errno on error. */
 static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
@@ -795,28 +803,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));
@@ -825,7 +829,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));
@@ -833,8 +837,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;
@@ -845,10 +848,11 @@ 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));
+		if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
+			i801_set_hstadd(priv, addr, read_write);
+		else
+			i801_set_hstadd(priv, addr, 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 */
@@ -858,11 +862,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.35.3

Re: [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:57:21 +0200, 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.

This isn't an "assumption". The values of I2C_SMBUS_WRITE and
I2C_SMBUS_READ were chosen to match the bit position and values in the
I2C protocol. Maybe it should have been made clearer by defining them
as hexadecimal values instead of decimal values. Nevertheless, I find
it unfortunate to not use this fact to optimize the code, see below.

> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a9737f14d..bf77f8640 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  	return result;
>  }
>  
> +static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
> +{
> +	addr <<= 1;
> +	if (read_write == I2C_SMBUS_READ)
> +		addr |= 0x01;
> +	outb_p(addr, SMBHSTADD(priv));

This can be written:

	outb_p((addr << 1) | read_write, SMBHSTADD(priv));

Net result -48 bytes of (x86_64) binary code. That's basically what the
original code was doing, minus the useless masking.

> +}
> +
>  /* Return negative errno on error. */
>  static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		       unsigned short flags, char read_write, u8 command,
> @@ -795,28 +803,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));
> @@ -825,7 +829,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));
> @@ -833,8 +837,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;
> @@ -845,10 +848,11 @@ 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));
> +		if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
> +			i801_set_hstadd(priv, addr, read_write);
> +		else
> +			i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);

Preserving the use of the ternary operator makes the generated binary
smaller once again:

		i801_set_hstadd(priv, addr,
				(priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
				read_write : I2C_SMBUS_WRITE);

Net result -11 bytes of (x86_64) binary code.

> +
>  		if (read_write == I2C_SMBUS_READ) {
>  			/* NB: page 240 of ICH5 datasheet also shows
>  			 * that DATA1 is the cmd field when reading */
> @@ -858,11 +862,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;


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()

[Heiner Kallweit]

On 09.06.2022 15:53, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:57:21 +0200, 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.
> 
> This isn't an "assumption". The values of I2C_SMBUS_WRITE and
> I2C_SMBUS_READ were chosen to match the bit position and values in the
> I2C protocol. Maybe it should have been made clearer by defining them
> as hexadecimal values instead of decimal values. Nevertheless, I find
> it unfortunate to not use this fact to optimize the code, see below.
> 
>> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
>>  1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a9737f14d..bf77f8640 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>>  	return result;
>>  }
>>  
>> +static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>> +{
>> +	addr <<= 1;
>> +	if (read_write == I2C_SMBUS_READ)
>> +		addr |= 0x01;
>> +	outb_p(addr, SMBHSTADD(priv));
> 
> This can be written:
> 
> 	outb_p((addr << 1) | read_write, SMBHSTADD(priv));
> 
> Net result -48 bytes of (x86_64) binary code. That's basically what the
> original code was doing, minus the useless masking.
> 
OK

>> +}
>> +
>>  /* Return negative errno on error. */
>>  static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		       unsigned short flags, char read_write, u8 command,
>> @@ -795,28 +803,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));
>> @@ -825,7 +829,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));
>> @@ -833,8 +837,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;
>> @@ -845,10 +848,11 @@ 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));
>> +		if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
>> +			i801_set_hstadd(priv, addr, read_write);
>> +		else
>> +			i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> 
> Preserving the use of the ternary operator makes the generated binary
> smaller once again:
> 
> 		i801_set_hstadd(priv, addr,
> 				(priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
> 				read_write : I2C_SMBUS_WRITE);
> 
> Net result -11 bytes of (x86_64) binary code.
> 
OK

>> +
>>  		if (read_write == I2C_SMBUS_READ) {
>>  			/* NB: page 240 of ICH5 datasheet also shows
>>  			 * that DATA1 is the cmd field when reading */
>> @@ -858,11 +862,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;
> 
> 

[PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()

[Heiner Kallweit]

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

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index bf77f8640..8c2245f38 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
 	return result;
 }
 
+/* Single transaction function */
+static int i801_single_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;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
 {
 	addr <<= 1;
@@ -784,9 +840,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);
@@ -804,36 +858,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:
@@ -883,7 +924,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_single_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
@@ -891,26 +932,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.35.3

Re: [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:58:03 +0200, Heiner Kallweit wrote:
> This patch factors out non-block pre/post processing to a new function
> i801_single_transaction(), complementing existing function
> i801_block_transaction(). This makes i801_access() better readable.

I like the idea, but I have objections about some implementation
details, see below.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 95 +++++++++++++++++++++--------------
>  1 file changed, 58 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index bf77f8640..8c2245f38 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  	return result;
>  }
>  
> +/* Single transaction function */

The term "single transaction" is a bit misleading. Block transactions
are also single transactions, in the sense that there's one start
condition at the beginning and one stop condition at the end. I'd
rather call non-block transactions "single value transactions" or
"simple transactions".

> +static int i801_single_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;

Previous 2 lines are indented with spaces instead of tabs.

> +	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;

That's never going to happen.

Generally speaking, I'm worried about having the same switch/case
construct here that we already have in i801_access. Looks to me like we
are doing half of the work here and the other half there and I fail to
see the rationale for splitting the work like that. I mean, I see how
it solves the asymmetry between the block and non-block code paths, but
the result doesn't look appealing. From a performance perspective it's
questionable too.

What prevents us from doing all the work on either side? Maybe we
should move more code into i801_single_transaction (possibly in a
subsequent patch)?

> +	}
> +
> +	ret = i801_transaction(priv, xact);
> +

Traditionally no blank line here.

> +	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;
> +	default:
> +		break;

Default case is not needed.

> +	}
> +
> +	return 0;
> +}
> +
>  static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>  {
>  	addr <<= 1;
> @@ -784,9 +840,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);
> @@ -804,36 +858,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:
> @@ -883,7 +924,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_single_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
> @@ -891,26 +932,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,


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 6/8] i2c: i801: add i801_single_transaction(), complementing i801_block_transaction()

[Heiner Kallweit]

On 10.06.2022 13:03, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:58:03 +0200, Heiner Kallweit wrote:
>> This patch factors out non-block pre/post processing to a new function
>> i801_single_transaction(), complementing existing function
>> i801_block_transaction(). This makes i801_access() better readable.
> 
> I like the idea, but I have objections about some implementation
> details, see below.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 95 +++++++++++++++++++++--------------
>>  1 file changed, 58 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index bf77f8640..8c2245f38 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -771,6 +771,62 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>>  	return result;
>>  }
>>  
>> +/* Single transaction function */
> 
> The term "single transaction" is a bit misleading. Block transactions
> are also single transactions, in the sense that there's one start
> condition at the beginning and one stop condition at the end. I'd
> rather call non-block transactions "single value transactions" or
> "simple transactions".
> 
OK

>> +static int i801_single_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;
> 
> Previous 2 lines are indented with spaces instead of tabs.
> 
OK

>> +	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;
> 
> That's never going to happen.
> 
> Generally speaking, I'm worried about having the same switch/case
> construct here that we already have in i801_access. Looks to me like we
> are doing half of the work here and the other half there and I fail to
> see the rationale for splitting the work like that. I mean, I see how
> it solves the asymmetry between the block and non-block code paths, but
> the result doesn't look appealing. From a performance perspective it's
> questionable too.
> 
> What prevents us from doing all the work on either side? Maybe we
> should move more code into i801_single_transaction (possibly in a
> subsequent patch)?
> 
Makes sense. Ill add this in v2.

>> +	}
>> +
>> +	ret = i801_transaction(priv, xact);
>> +
> 
> Traditionally no blank line here.
> 
OK

>> +	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;
>> +	default:
>> +		break;
> 
> Default case is not needed.
> 
OK

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>>  {
>>  	addr <<= 1;
>> @@ -784,9 +840,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);
>> @@ -804,36 +858,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:
>> @@ -883,7 +924,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_single_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
>> @@ -891,26 +932,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,
> 
> 

[PATCH 7/8] 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_access() as well.

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

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 8c2245f38..9061333f2 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -460,10 +460,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,
@@ -647,10 +643,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) {
@@ -851,6 +843,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;
-- 
2.35.3

Re: [PATCH 7/8] i2c: i801: call i801_check_pre() from i801_access()

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:58:40 +0200, Heiner Kallweit wrote:
> This avoids code duplication, in a next step we'll call
> i801_check_post() from i801_access() as well.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 8c2245f38..9061333f2 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -460,10 +460,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,
> @@ -647,10 +643,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) {
> @@ -851,6 +843,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;

Makes sense, thanks for the clean-up.

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

-- 
Jean Delvare
SUSE L3 Support

[PATCH 8/8] 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 9061333f2..ecec7a3a8 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -432,7 +432,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;
@@ -456,7 +456,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;
 
@@ -465,13 +464,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,
@@ -618,7 +616,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);
 	}
 
@@ -668,7 +666,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++) {
@@ -682,7 +680,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) {
@@ -712,9 +710,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);
 }
 
 /* Block transaction function */
@@ -922,6 +918,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
 	else
 		ret = i801_single_transaction(priv, data, 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. Turn off
 	   E32B for the same reason. */
-- 
2.35.3

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

[Jean Delvare]

Hi Heiner,

On Fri, 15 Apr 2022 18:59:46 +0200, 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(-)

Overall I like the idea. I only have one question to make sure I'm not
missing something.

> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 9061333f2..ecec7a3a8 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -432,7 +432,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));

Do I understand correctly that this change isn't really related to the
rest of the patch, and could have been done independently?

You are filtering out SMBHSTSTS_INTR simply because i801_check_post()
will never check it anyway, right? If so, I wonder if that's really
something we want to do, as ultimately this adds code with no
functional benefit just to be "cleaner". But please correct me if I'm
wrong.

-- 
Jean Delvare
SUSE L3 Support

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

[Heiner Kallweit]

On 10.06.2022 16:31, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:59:46 +0200, 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(-)
> 
> Overall I like the idea. I only have one question to make sure I'm not
> missing something.
> 
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index 9061333f2..ecec7a3a8 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -432,7 +432,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));
> 
> Do I understand correctly that this change isn't really related to the
> rest of the patch, and could have been done independently?
> 
> You are filtering out SMBHSTSTS_INTR simply because i801_check_post()
> will never check it anyway, right? If so, I wonder if that's really
> something we want to do, as ultimately this adds code with no
> functional benefit just to be "cleaner". But please correct me if I'm
> wrong.
> 
Reason is that in few places we check whether return value of
i801_wait_intr() is zero, this would fail if not filtering out SMBHSTSTS_INTR.
Example:
i801_transaction() returns the return value of i801_wait_intr() now.
And in i801_block_transaction_by_block() we check whether return value of
i801_transaction() is zero.