Re: [PATCH] i2c: i801: Fix handling SMBHSTCNT_PEC_EN

[Heiner Kallweit <hkallweit1@gmail.com>]

On 21.06.2021 23:08, Heiner Kallweit wrote:
> Bit SMBHSTCNT_PEC_EN is used only if software calculates the CRC and
> uses register SMBPEC. This is not supported by the driver, it supports
> hw-calculation of CRC only (using bit SMBAUXSTS_CRCE). The chip spec
> states the following, therefore never set bit SMBHSTCNT_PEC_EN.
> 
> Chapter SMBus CRC Generation and Checking
> If the AAC bit is set in the Auxiliary Control register, the PCH
> automatically calculates and drives CRC at the end of the transmitted
> packet for write cycles, and will check the CRC for read cycles. It will
> not transmit the contents of the PEC register for CRC. The PEC bit must
> not be set in the Host Control register. If this bit is set, unspecified
> behavior will result.
> 
> This patch is based solely on the specification and compile-tested only,
> because I have no PEC-capable devices.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> This patch may be a candidate for stable (with a little bit of fuzz)
> once somebody with a PEC-capable device has tested it.
> ---
>  drivers/i2c/busses/i2c-i801.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 0153a21f4..161176fcd 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -516,12 +516,9 @@ static int i801_transaction(struct i801_priv *priv, int xact)
>  
>  static int i801_block_transaction_by_block(struct i801_priv *priv,
>  					   union i2c_smbus_data *data,
> -					   char read_write, int command,
> -					   int hwpec)
> +					   char read_write, int command)
>  {
> -	int i, len;
> -	int status;
> -	int xact = hwpec ? SMBHSTCNT_PEC_EN : 0;
> +	int i, len, status, xact = 0;
>  
>  	switch (command) {
>  	case I2C_SMBUS_BLOCK_PROC_CALL:
> @@ -678,8 +675,7 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>   */
>  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  					       union i2c_smbus_data *data,
> -					       char read_write, int command,
> -					       int hwpec)
> +					       char read_write, int command)
>  {
>  	int i, len;
>  	int smbcmd;
> @@ -777,9 +773,8 @@ static int i801_set_block_buffer_mode(struct i801_priv *priv)
>  }
>  
>  /* Block transaction function */
> -static int i801_block_transaction(struct i801_priv *priv,
> -				  union i2c_smbus_data *data, char read_write,
> -				  int command, int hwpec)
> +static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> +				  char read_write, int command)
>  {
>  	int result = 0;
>  	unsigned char hostc;
> @@ -815,11 +810,11 @@ static int i801_block_transaction(struct i801_priv *priv,
>  	 && i801_set_block_buffer_mode(priv) == 0)
>  		result = i801_block_transaction_by_block(priv, data,
>  							 read_write,
> -							 command, hwpec);
> +							 command);
>  	else
>  		result = i801_block_transaction_byte_by_byte(priv, data,
>  							     read_write,
> -							     command, hwpec);
> +							     command);
>  
>  	if (command == I2C_SMBUS_I2C_BLOCK_DATA
>  	 && read_write == I2C_SMBUS_WRITE) {
> @@ -936,8 +931,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>  		       SMBAUXCTL(priv));
>  
>  	if (block)
> -		ret = i801_block_transaction(priv, data, read_write, size,
> -					     hwpec);
> +		ret = i801_block_transaction(priv, data, read_write, size);
>  	else
>  		ret = i801_transaction(priv, xact);
>  
> @@ -1625,6 +1619,7 @@ static void i801_setup_hstcfg(struct i801_priv *priv)
>  	unsigned char hstcfg = priv->original_hstcfg;
>  
>  	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
> +	hstcfg &= ~SMBHSTCNT_PEC_EN;
>  	hstcfg |= SMBHSTCFG_HST_EN;
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  
> 

Jean, do you have an opinion on this patch? It's been pending for quite some
time and I have few more patches that I'd like to submit.