Hi Jean,
After our local test, the action clear SMBALERT status bit is needed only here
for the HW interrupt will be cleared by slave chip.
So I change the patch as follow.
From: Lingyan Xu
In current i801 driver, SMBALERT interrupt is allowed (Slave Command Register
bit2 is 0).
But these is no handler for SMBALERT interrupt in i801_isr, if there is
SMBALERT interrupt asserted and deasserted,
i801 will have an irq flood for the related status bit is setted.
So SMBALERT status clear is needed.
About the solution,
please see http://www.farnell.com/datasheets/1581967.pdf
Page632 P640 for more.
Signed-off-by: Lingyan Xu
---
drivers/i2c/busses/i2c-i801.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index f295693..29c90f0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -661,6 +661,11 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
* Clear irq sources and report transaction result.
* ->status must be cleared before the next transaction is started.
*/
+
+ if (status & SMBHSTSTS_SMBALERT_STS) {
+ outb_p(status & SMBHSTSTS_SMBALERT_STS, SMBHSTSTS(priv));
+ }
+
status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
if (status) {
outb_p(status, SMBHSTSTS(priv));
--
2.6.2
Best Regards!
Lingyan xu
-Original Message-
From: Xu, Lingyan (NSB - CN/Hangzhou)
Sent: 2019年9月3日 10:16
To: 'Jean Delvare'
Cc: Adamski, Krzysztof (Nokia - PL/Wroclaw) ;
Wiebe, Wladislav (Nokia - DE/Ulm) ;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT
interrupt
Hi Jean,
Thanks a lot for your comments. And, yes, it is dangerous that clear all
interrupt bit here based my local test. And about the interrupt flood, I will
show you in attached file. And I agree with you that add SMBALERT interrupt
handler if possible, but I have no idea about what action is need in this
handler because that it seams that i801 can not clear salve chip's status bit
directly.
Best Regards!
Lingyan xu
-Original Message-
From: Jean Delvare
Sent: 2019年8月28日 21:58
To: Xu, Lingyan (NSB - CN/Hangzhou)
Cc: Adamski, Krzysztof (Nokia - PL/Wroclaw) ;
Wiebe, Wladislav (Nokia - DE/Ulm) ;
linux-...@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i801_smbus: clear SMBALERT status bit and disable SMBALERT
interrupt
Hi Lingyan,
On Mon, 12 Aug 2019 10:40:34 +0800, lingyxu wrote:
> From: Lingyan Xu
>
> In current i801 driver, SMBALERT interrupt is allowed (Slave Command
> Register bit2 is 0).
> But these is no handler for SMBALERT interrupt in i801_isr, if there
> is SMBALERT interrupt asserted and deasserted,
> i801 will have an irq flood for the related status bit is setted.
>
> So SMBALERT interrupt handler is needed, and also, SMBALERT interrupt
> will be generated from time to time if slave chip have some fault.
> So disable SMBALERT interrupt is also needed.
>
> About the solution,
> please see http://www.farnell.com/datasheets/1581967.pdf
> Page632 P640 for more.
>
> Signed-off-by: Lingyan Xu
> ---
> drivers/i2c/busses/i2c-i801.c |7 ++-
> 1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c
> b/drivers/i2c/busses/i2c-i801.c index f295693..033bafe 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -661,9 +661,11 @@ static irqreturn_t i801_isr(int irq, void *dev_id)
>* Clear irq sources and report transaction result.
>* ->status must be cleared before the next transaction is started.
>*/
> +
> + outb_p(status, SMBHSTSTS(priv));
> +
> status &= SMBHSTSTS_INTR | STATUS_ERROR_FLAGS;
> if (status) {
> - outb_p(status, SMBHSTSTS(priv));
> priv->status = status;
> wake_up(>waitq);
> }
Looks scary. Writing the whole value of SMBHSTSTS back to itself without
selecting which bits you write is dangerous. Specifically, writing back
SMBHSTSTS_BYTE_DONE, SMBHSTSTS_INUSE_STS and SMBHSTSTS_HOST_BUSY could have
unexpected consequences. I would feel much better if you would just explicitly
add SMBHSTSTS_SMBALERT_STS to the list.
> @@ -1810,6 +1812,9 @@ static int i801_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> /* Default timeout in interrupt mode: 200 ms */
> priv->adapter.timeout = HZ / 5;
>
> + /* Disable SMBALERT interrupt */
> + outb_p(inb_p(SMBSLVCMD(priv)) | BIT(2), SMBSLVCMD(priv));
Please give SMBSLVCMD's BIT(2) a name and define it after
SMBSLVCMD_HST_NTFY_INTREN.
Also it is mandatory to restore the value of SMBSLVCMD before returning the
control
