Re: i801: I2C block read should be usable also on ICH/ICH0/ICH2/ICH3/ICH4

[Heiner Kallweit <hkallweit1@gmail.com>]

On 06.12.2021 22:18, Heiner Kallweit wrote:
> Currently we use the following feature definitions. However, according to
> the respective datasheets, also ICH/ICH0/ICH2/ICH3/ICH4 support I2C block
> read. The implementation we have should work also on these chip versions.
> 
Basically the same I found for interrupt support. It's available on all
chip versions. Any specific reason why driver uses interrupts from ICH5
only?


> The commit message of 6342064cad7a ("i2c-i801: Implement I2C block read
> support") states that i2c block read is supported from ICH5 only.
> This doesn't seem to be true. Or is this feature broken on older chip
> versions? 
> 
> To me it seems we could remove FEATURE_I2C_BLOCK_READ because all chip
> versions support this feature. Below is an experimental patch, for the
> ones with test hw. A test case could be to use decode-dimms that
> uses i2c block read to read the EEPROM content.
> 
> * 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
> 
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 930c6edbe..fd9a19a80 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -294,7 +294,6 @@ struct i801_priv {
>  #define FEATURE_SMBUS_PEC	BIT(0)
>  #define FEATURE_BLOCK_BUFFER	BIT(1)
>  #define FEATURE_BLOCK_PROC	BIT(2)
> -#define FEATURE_I2C_BLOCK_READ	BIT(3)
>  #define FEATURE_IRQ		BIT(4)
>  #define FEATURE_HOST_NOTIFY	BIT(5)
>  /* Not really a feature, but it's convenient to handle it as such */
> @@ -780,10 +779,6 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  			pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc);
>  			pci_write_config_byte(priv->pci_dev, SMBHSTCFG,
>  					      hostc | SMBHSTCFG_I2C_EN);
> -		} else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) {
> -			dev_err(&priv->pci_dev->dev,
> -				"I2C block read is unsupported!\n");
> -			return -EOPNOTSUPP;
>  		}
>  	}
>  
> @@ -956,11 +951,10 @@ static u32 i801_func(struct i2c_adapter *adapter)
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
>  	       I2C_FUNC_SMBUS_BLOCK_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK |
> +	       I2C_FUNC_SMBUS_READ_I2C_BLOCK |
>  	       ((priv->features & FEATURE_SMBUS_PEC) ? I2C_FUNC_SMBUS_PEC : 0) |
>  	       ((priv->features & FEATURE_BLOCK_PROC) ?
>  		I2C_FUNC_SMBUS_BLOCK_PROC_CALL : 0) |
> -	       ((priv->features & FEATURE_I2C_BLOCK_READ) ?
> -		I2C_FUNC_SMBUS_READ_I2C_BLOCK : 0) |
>  	       ((priv->features & FEATURE_HOST_NOTIFY) ?
>  		I2C_FUNC_SMBUS_HOST_NOTIFY : 0);
>  }
> @@ -997,7 +991,7 @@ static const struct i2c_algorithm smbus_algorithm = {
>  	.functionality	= i801_func,
>  };
>  
> -#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | FEATURE_I2C_BLOCK_READ	| \
> +#define FEATURES_ICH5	(FEATURE_BLOCK_PROC | 				  \
>  			 FEATURE_IRQ | FEATURE_SMBUS_PEC		| \
>  			 FEATURE_BLOCK_BUFFER | FEATURE_HOST_NOTIFY)
>  #define FEATURES_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \