lkml.org 
[lkml]   [2022]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] pinctrl: ocelot: Fix interrupt controller
The 10/13/2022 09:30, Michael Walle wrote:

Hi Michael,

> > We lose the interrupt here, as the HW will not generate another one
> > but at later point we read again the line status. And if the line is
> > active then we kick again the interrupt handler again.
>
> Ahh, thanks for explaining. That also explains the read below.
>
> Will you send a proper patch?

No worries. Yes, I will do that.

>
> -michael
>
> >
> > >
> > > > +
> > > > /* Enable the interrupt now */
> > > > gpiochip_enable_irq(chip, gpio);
> > > > regmap_update_bits(info->map, REG(OCELOT_GPIO_INTR_ENA, info, gpio),
> > > > bit, bit);
> > > >
> > > > /*
> > > > - * In case the interrupt line is still active and the interrupt
> > > > - * controller has not seen any changes in the interrupt line, then it
> > > > - * means that there happen another interrupt while the line was
> > > > active.
> > > > + * In case the interrupt line is still active then it means that
> > > > + * there happen another interrupt while the line was active.
> > > > * So we missed that one, so we need to kick the interrupt again
> > > > * handler.
> > > > */
> > > > - if (active && !ack) {
> > > > + regmap_read(info->map, REG(OCELOT_GPIO_IN, info, gpio), &val);
> > > > + if ((!(val & bit) && trigger_level == IRQ_TYPE_LEVEL_LOW) ||
> > > > + (val & bit && trigger_level == IRQ_TYPE_LEVEL_HIGH))
> > > > + active = true;
> > >
> > > Why do you read the line state twice? What happens if the line state
> > > changes right after you've read it?
> >
> > Here we need to read again the status because we might have clear the
> > ack of interrupt.
> > If the line becomes active right after this read, then the HW will
> > generate another interrupt as the interrupt is enabled and ack is
> > cleared.
> >
> > >
> > > > +
> > > > + if (active) {
> > > > struct ocelot_irq_work *work;
> > > >
> > > > work = kmalloc(sizeof(*work), GFP_ATOMIC);
> > >
> > > So yes, maybe the trade-off that there will be two interrupts are
> > > better than this additional patch. But it should be documented
> > > somewhere, even if it's just a comment in this driver.
> > >
> > > -michael

--
/Horatiu

\
 
 \ /
  Last update: 2022-10-13 16:08    [W:0.697 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site