| Message ID | bd0def53-4e63-61eb-c0bb-9975a308cb1a@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] i2c: i801: Don't clear status flags twice in interrupt mode | expand |
On Sat, 04 Dec 2021 21:04:40 +0100, Heiner Kallweit wrote: > In interrupt mode we clear the status flags twice, in the interrupt > handler and in i801_check_post(). Remove clearing the status flags > from i801_check_post() and handle polling mode by using the > SMBus unlocking write to also clear the status flags if still set. > To be precise: One could still argue that the status flags are > cleared twice in interrupt mode, but it comes for free. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > v2: > - clear status flags also in i801_wait_byte_done() > - remove outdated comment at i801_check_post() > v3: > - merge unlocking SMBus and clearing status flags > - avoid the complexity added with v2 > --- > drivers/i2c/busses/i2c-i801.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 930c6edbe..128a25de7 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -372,11 +372,6 @@ static int i801_check_pre(struct i801_priv *priv) > return 0; > } > > -/* > - * Convert the status register to an error code, and clear it. > - * Note that status only contains the bits we want to clear, not the > - * actual register value. > - */ > static int i801_check_post(struct i801_priv *priv, int status) > { > int result = 0; > @@ -401,7 +396,6 @@ static int i801_check_post(struct i801_priv *priv, int status) > !(status & SMBHSTSTS_FAILED)) > dev_err(&priv->pci_dev->dev, > "Failed terminating the transaction\n"); > - outb_p(STATUS_FLAGS, SMBHSTSTS(priv)); > return -ETIMEDOUT; > } > > @@ -440,9 +434,6 @@ static int i801_check_post(struct i801_priv *priv, int status) > dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n"); > } > > - /* Clear status flags except BYTE_DONE, to be cleared by caller */ > - outb_p(status, SMBHSTSTS(priv)); > - > return result; > } > > @@ -939,8 +930,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > } > > out: > - /* Unlock the SMBus device for use by BIOS/ACPI */ > - outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv)); > + /* > + * Unlock the SMBus device for use by BIOS/ACPI, > + * and clear status flags if not done already. > + */ > + outb_p(SMBHSTSTS_INUSE_STS | STATUS_FLAGS, SMBHSTSTS(priv)); > > pm_runtime_mark_last_busy(&priv->pci_dev->dev); > pm_runtime_put_autosuspend(&priv->pci_dev->dev); Looks good to me. Reviewed-by: Jean Delvare <jdelvare@suse.de> Tested-by: Jean Delvare <jdelvare@suse.de>
On Sat, Dec 04, 2021 at 09:04:40PM +0100, Heiner Kallweit wrote: > In interrupt mode we clear the status flags twice, in the interrupt > handler and in i801_check_post(). Remove clearing the status flags > from i801_check_post() and handle polling mode by using the > SMBus unlocking write to also clear the status flags if still set. > To be precise: One could still argue that the status flags are > cleared twice in interrupt mode, but it comes for free. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 930c6edbe..128a25de7 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -372,11 +372,6 @@ static int i801_check_pre(struct i801_priv *priv) return 0; } -/* - * Convert the status register to an error code, and clear it. - * Note that status only contains the bits we want to clear, not the - * actual register value. - */ static int i801_check_post(struct i801_priv *priv, int status) { int result = 0; @@ -401,7 +396,6 @@ static int i801_check_post(struct i801_priv *priv, int status) !(status & SMBHSTSTS_FAILED)) dev_err(&priv->pci_dev->dev, "Failed terminating the transaction\n"); - outb_p(STATUS_FLAGS, SMBHSTSTS(priv)); return -ETIMEDOUT; } @@ -440,9 +434,6 @@ static int i801_check_post(struct i801_priv *priv, int status) dev_dbg(&priv->pci_dev->dev, "Lost arbitration\n"); } - /* Clear status flags except BYTE_DONE, to be cleared by caller */ - outb_p(status, SMBHSTSTS(priv)); - return result; } @@ -939,8 +930,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, } out: - /* Unlock the SMBus device for use by BIOS/ACPI */ - outb_p(SMBHSTSTS_INUSE_STS, SMBHSTSTS(priv)); + /* + * Unlock the SMBus device for use by BIOS/ACPI, + * and clear status flags if not done already. + */ + outb_p(SMBHSTSTS_INUSE_STS | STATUS_FLAGS, SMBHSTSTS(priv)); pm_runtime_mark_last_busy(&priv->pci_dev->dev); pm_runtime_put_autosuspend(&priv->pci_dev->dev);
In interrupt mode we clear the status flags twice, in the interrupt handler and in i801_check_post(). Remove clearing the status flags from i801_check_post() and handle polling mode by using the SMBus unlocking write to also clear the status flags if still set. To be precise: One could still argue that the status flags are cleared twice in interrupt mode, but it comes for free. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - clear status flags also in i801_wait_byte_done() - remove outdated comment at i801_check_post() v3: - merge unlocking SMBus and clearing status flags - avoid the complexity added with v2 --- drivers/i2c/busses/i2c-i801.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)