| Message ID | 0ef3e6f9-471f-ff05-0cf0-046d79a4c820@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i2c: i801: Series with minor improvements | expand |
On Mon, 19 Dec 2022 19:20:10 +0100, Heiner Kallweit wrote: > Currently we touch SMBAUXCTL even if not needed. That's the case for block > commands that don't use block buffer mode, either because block buffer > mode isn't available or because it's not supported for the respective > command (e.g. I2C block transfer). Improve this by setting/resetting > SMBAUXCTL_E32B in i801_block_transaction_by_block() only. > > Small downside is that we know access SMBAUXCTL twice for transactions Typo: know -> now. > that use PEC and block buffer mode. But this should a rather rare case > and the impact is negligible. I agree, and the new way also makes things symmetric and thus more obviously correct. > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > (...) Reviewed-by: Jean Delvare <jdelvare@suse.de> One possible further improvement step, performance-wise, would be to store the original value of SMBAUXCTL so that we can skip the inb_p() at the end of the function. What do you think?
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index d934d410b..d7182f7c8 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -511,19 +511,23 @@ static int i801_block_transaction_by_block(struct i801_priv *priv, status = i801_transaction(priv, xact); if (status) - return status; + goto out; if (read_write == I2C_SMBUS_READ || command == I2C_SMBUS_BLOCK_PROC_CALL) { len = inb_p(SMBHSTDAT0(priv)); - if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) - return -EPROTO; + if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) { + status = -EPROTO; + goto out; + } data->block[0] = len; for (i = 0; i < len; i++) data->block[i + 1] = inb_p(SMBBLKDAT(priv)); } - return 0; +out: + outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_E32B, SMBAUXCTL(priv)); + return status; } static void i801_isr_byte_done(struct i801_priv *priv) @@ -921,11 +925,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, ret = i801_simple_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 - E32B for the same reason. */ - if (hwpec || block) - outb_p(inb_p(SMBAUXCTL(priv)) & - ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv)); + * time, so we forcibly disable it after every transaction. + */ + if (hwpec) + outb_p(inb_p(SMBAUXCTL(priv)) & ~SMBAUXCTL_CRC, SMBAUXCTL(priv)); out: /* * Unlock the SMBus device for use by BIOS/ACPI,
Currently we touch SMBAUXCTL even if not needed. That's the case for block commands that don't use block buffer mode, either because block buffer mode isn't available or because it's not supported for the respective command (e.g. I2C block transfer). Improve this by setting/resetting SMBAUXCTL_E32B in i801_block_transaction_by_block() only. Small downside is that we know access SMBAUXCTL twice for transactions that use PEC and block buffer mode. But this should a rather rare case and the impact is negligible. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)