| Message ID | 20250228141802.1344453-2-jarkko.nikula@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates | expand |
On Fri, Feb 28, 2025 at 04:18:00PM +0200, Jarkko Nikula wrote: > Return IRQ_HANDLED from the i3c_hci_irq_handler() only if some > INTR_STATUS bit was set or if DMA/PIO handler handled it. > > Currently it returns IRQ_HANDLED in case INTR_STATUS is zero and IO > handler returns false. Which could be the case if interrupt comes from > other device or is spurious. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > drivers/i3c/master/mipi-i3c-hci/core.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c > index e139d7e4d252..e5593b6e897e 100644 > --- a/drivers/i3c/master/mipi-i3c-hci/core.c > +++ b/drivers/i3c/master/mipi-i3c-hci/core.c > @@ -594,6 +594,7 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id) > > if (val) { > reg_write(INTR_STATUS, val); > + result = IRQ_HANDLED; Not related this patch. reg_write(INTR_STATUS, val); should be unconditional since INTR_STATUS is w1c. I am strange why need return IRQ_NONE case. I suppose it should always return IRQ_HANDLED. Frank > } > > if (val & INTR_HC_RESET_CANCEL) { > @@ -605,12 +606,11 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id) > val &= ~INTR_HC_INTERNAL_ERR; > } > > - hci->io->irq_handler(hci); > + if (hci->io->irq_handler(hci)) > + result = IRQ_HANDLED; > > if (val) > dev_err(&hci->master.dev, "unexpected INTR_STATUS %#x\n", val); > - else > - result = IRQ_HANDLED; > > return result; > } > -- > 2.47.2 > > > -- > linux-i3c mailing list > linux-i3c@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi On 2/28/25 6:14 PM, Frank Li wrote: > On Fri, Feb 28, 2025 at 04:18:00PM +0200, Jarkko Nikula wrote: >> Return IRQ_HANDLED from the i3c_hci_irq_handler() only if some >> INTR_STATUS bit was set or if DMA/PIO handler handled it. >> >> Currently it returns IRQ_HANDLED in case INTR_STATUS is zero and IO >> handler returns false. Which could be the case if interrupt comes from >> other device or is spurious. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> drivers/i3c/master/mipi-i3c-hci/core.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c >> index e139d7e4d252..e5593b6e897e 100644 >> --- a/drivers/i3c/master/mipi-i3c-hci/core.c >> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c >> @@ -594,6 +594,7 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id) >> >> if (val) { >> reg_write(INTR_STATUS, val); >> + result = IRQ_HANDLED; > > Not related this patch. > reg_write(INTR_STATUS, val); should be unconditional since INTR_STATUS is > w1c. > > I am strange why need return IRQ_NONE case. I suppose it should always > return IRQ_HANDLED. > This makes driver more ready for shared interrupts and let kernel interrupt code be able to disable bad behaving interrupt (misrouted, wrong polarity, etc) by returning IRQ_NONE when we know interrupt wasn't generated by this device. Currently driver doesn't support shared interrupts since IRQF_SHARED flag is not passed to the devm_request_irq() call in i3c_hci_probe(). I didn't change that since I don't have a setup to test.
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c index e139d7e4d252..e5593b6e897e 100644 --- a/drivers/i3c/master/mipi-i3c-hci/core.c +++ b/drivers/i3c/master/mipi-i3c-hci/core.c @@ -594,6 +594,7 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id) if (val) { reg_write(INTR_STATUS, val); + result = IRQ_HANDLED; } if (val & INTR_HC_RESET_CANCEL) { @@ -605,12 +606,11 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id) val &= ~INTR_HC_INTERNAL_ERR; } - hci->io->irq_handler(hci); + if (hci->io->irq_handler(hci)) + result = IRQ_HANDLED; if (val) dev_err(&hci->master.dev, "unexpected INTR_STATUS %#x\n", val); - else - result = IRQ_HANDLED; return result; }
Return IRQ_HANDLED from the i3c_hci_irq_handler() only if some INTR_STATUS bit was set or if DMA/PIO handler handled it. Currently it returns IRQ_HANDLED in case INTR_STATUS is zero and IO handler returns false. Which could be the case if interrupt comes from other device or is spurious. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/i3c/master/mipi-i3c-hci/core.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)