[rt2x00-users] [RFC 2/9] rt2x00: convert to threaded interrupts

[Helmut Schaa]

Am Montag 05 Juli 2010 schrieb Ivo Van Doorn:
> On Mon, Jul 5, 2010 at 8:10 PM, Helmut Schaa
> <helmut.schaa at googlemail.com> wrote:
> > Am Montag 05 Juli 2010 schrieb Ivo Van Doorn:
> >> On Mon, Jul 5, 2010 at 6:20 PM, Helmut Schaa
> >> <helmut.schaa at googlemail.com> wrote:
> >
> > [...]
> >
> >> > +       /* Enable interrupts again. */
> >> > +       rt2x00pci_register_read(rt2x00dev, CSR8, &reg);
> >> > +       reg &= ~rt2x00dev->irqmask[0];
> >> > +       rt2x00pci_register_write(rt2x00dev, CSR8, reg);
> >> > +
> >>
> >> Cant this be replaced with:
> >> rt2400pci_toggle_irq(rt2x00dev, STATE_RADIO_IRQ_ON)
> >> That way we keep the IRQ changing code in one location,
> >> and we keep the code clearer compared to using masks which
> >> obscure the real action.
> >
> > [...]
> >> > +       /* Disable interrupts, will be enabled again in the interrupt thread. */
> >> > +       rt2x00pci_register_read(rt2x00dev, CSR8, &reg);
> >> > +       reg |= rt2x00dev->irqmask[0];
> >> > +       rt2x00pci_register_write(rt2x00dev, CSR8, reg);
> >>
> >> Cant this be replaced with:
> >> rt2400pci_toggle_irq(rt2x00dev, STATE_RADIO_IRQ_OFF)
> >
> > Yeah, I also had the same thought but during my tests the device got stuck
> > when using rt2800pci_toggle_irq (only tested rt2800pci).
> >
> > The trigger was clearly the following:
> >
> >  428         /*
> >  429          * When interrupts are being enabled, the interrupt registers
> >  430          * should clear the register to assure a clean state.
> >  431          */
> >  432         if (state == STATE_RADIO_IRQ_ON) {
> >  433                 rt2800_register_read(rt2x00dev, INT_SOURCE_CSR, &reg);
> >  434                 rt2800_register_write(rt2x00dev, INT_SOURCE_CSR, reg);
> >  435         }
> >
> > So, reenabling the interrupts at the end of the interrupt thread handler
> > didn't work correctly. However, I don't have an explanation why that would
> > make the device stuck.
> >
> > Of course we could also extend rt2800pci_toggle_irq with a parameter to not
> > force the flush of the INT_SOURCE_CSR register.
> 
> The easiest way to get around this (without an extra parameter) is to introduce
> a special state value, similar to the way I got around with the
> STATE_RADIO_RX_ON
> and STATE_RADIO_RX_OFF problem during link tuning by postfixing those two
> with _LINK.
> 
> So you can postfix the IRQ names with INTERRUPT or something, and update
> the check which sets the mask. The new names should be added to the switch
> in .set_device_state callback function.

Sounds reasonable, I'll look into this soon.

Helmut