| Message ID | 20211026143916.1272115-1-jarkko.nikula@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: i801: Fix incorrect and needless software PEC disabling | expand |
On 26.10.2021 16:39, 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). > Indeed, my bad. Mixed up HSTCFG and HSTCNT here. > 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> > --- > 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. > --- > 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); > } >
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); > }
On Tue, Oct 26, 2021 at 05:39:16PM +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> Applied to for-next, thanks!
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); }
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> --- 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. --- drivers/i2c/busses/i2c-i801.c | 1 - 1 file changed, 1 deletion(-)