Re: [PATCH 6/6] gpio: macsmc: Add IRQ support

[Hector Martin <marcan@xxxxxxxxx>]

On 06/09/2022 16.47, Russell King (Oracle) wrote:
> On Tue, Sep 06, 2022 at 04:00:31PM +0900, Hector Martin wrote:
>> On 02/09/2022 22.21, Linus Walleij wrote:
>>>> +       switch (type & IRQ_TYPE_SENSE_MASK) {
>>>> +       case IRQ_TYPE_LEVEL_HIGH:
>>>> +               mode = IRQ_MODE_HIGH;
>>>> +               break;
>>>> +       case IRQ_TYPE_LEVEL_LOW:
>>>> +               mode = IRQ_MODE_LOW;
>>>> +               break;
>>>> +       case IRQ_TYPE_EDGE_RISING:
>>>> +               mode = IRQ_MODE_RISING;
>>>> +               break;
>>>> +       case IRQ_TYPE_EDGE_FALLING:
>>>> +               mode = IRQ_MODE_FALLING;
>>>> +               break;
>>>> +       case IRQ_TYPE_EDGE_BOTH:
>>>> +               mode = IRQ_MODE_BOTH;
>>>> +               break;
>>>> +       default:
>>>> +               return -EINVAL;
>>>
>>> I don't know how level IRQs would work on this essentially
>>> message-passing process context interrupt. Maybe I am getting
>>> it all wrong, but for level the line should be held low/high until
>>> the IRQ is serviced, it would be possible to test if this actually
>>> works by *not* servicing an IRQ and see if the SMC then sends
>>> another message notifier for the same IRQ.
>>>
>>> I strongly suspect that actually only edges are supported, but
>>> there might be semantics I don't understand here.
>>
>> IIRC that is exactly what happens - the SMC will re-fire the IRQ after
>> the ACK if it is set to level mode and still at the active level.
>>
>> I do remember testing all the modes carefully when implementing this to
>> figure out what the precise semantics are, and I *think* I agonized over
>> the flow handlers quite a bit and decided this way would work properly
>> for all the modes, but it's been a while so I'd have to take a look
>> again to convince myself again :)
> 
> Thanks for the clarification - I think it would be useful to put some
> of that as comments before the CMD_IRQ_ACK write to head off any
> questions about this in the future. Something like this maybe?
> 
>         /*
>          * This is not an "ack" int he i8253 PIC sense - it is used for level
>          * interrupts as well. The SMC will re-fire the interrupt event after
>          * this ACK if the level interrupt is still active.
>          */
>         if (apple_smc_write_u32(smcgp->smc, key, CMD_IRQ_ACK | 1) < 0)
>                 dev_err(smcgp->dev, "GPIO IRQ ack failed for %p4ch\n", &key);
> 

Sounds good to me! (nit: typo on "int he")

- Hector