Re: [PATCH] i2c: i801: Fix incorrect and needless software PEC disabling

[Jean Delvare <jdelvare@suse.de>]

On Tue, 26 Oct 2021 17:39:16 +0300, Jarkko Nikula wrote:
> Commit a6b8bb6a813a ("i2c: i801: Fix handling SMBHSTCNT_PEC_EN")
> attempts to disable software PEC by clearing the SMBHSTCNT_PEC_EN (bit 7)
> in the SMBus Host Control register (I/O SMBHSTCNT) but incorrectly
> clears it in the PCI Host Configuration register (PCI SMBHSTCFG).
> 
> This clearing is actually needless since after above commit the
> SMBHSTCNT_PEC_EN is never set and the register is initialized with known
> values.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Oops, sorry for missing this and duplicating your work. I noticed the
bug while reviewing your other change and wrote a fix without realizing
you were already working on it.

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

> ---
> I didn't add Fixes tag and Cc stable@vger.kernel.org since I don't see
> this causes any real issues. A few platforms I checked the PCI Host
> Configuration register bit 7 was reserved 0.

I checked the datasheets too and came to the same conclusion, but added
the Fixes tag still, because the new fix is definitely safe and we have
no idea if the broken fix was actually harmless as it must have seen
very little deployment at this point (kernel 5.15 isn't out yet).
Better safe than sorry.

> ---
>  drivers/i2c/busses/i2c-i801.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 115660dce722..b6048a571543 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1657,7 +1657,6 @@ 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;	/* Disable software PEC */
>  	hstcfg |= SMBHSTCFG_HST_EN;
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hstcfg);
>  }


-- 
Jean Delvare
SUSE L3 Support