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

Ivo Van Doorn ivdoorn at gmail.com
Tue Jul 6 04:20:34 AEST 2010


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.

Ivo




More information about the users mailing list