Re: [PATCH 07/11] x86, olpc-xo1-sci: Add GPE handler and ebook switch functionality

[Daniel Drake <dsd@laptop.org>]

On 16 May 2011 10:08, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>>+
>>+      /* Enable EC SCI events */
>>+      cs5535_gpio_set(OLPC_GPIO_ECSCI, GPIO_EVENTS_ENABLE);
>>+
>>+      /* Set the SCI to cause a PME event on group 7 */
>>+      cs5535_gpio_setup_event(OLPC_GPIO_ECSCI, 7, 1);
>>+
>>+      /* And have group 7 also fire the SCI interrupt */
>>+      cs5535_gpio_set_irq(7, sci_irq);
>
> What do you do here? Could this be hidden behind a gpio irq_chip or is
> this too olpc specific?

Thanks for looking at these patches.

I looked into irq_chip alongside the CS5535 docs. Initially this
looked like a good suggestion - any CS5535 GPIO can be mapped to one
of 8 PIC-level interrupts, which seems like a good match for irq_chip.

However, this isn't what we're doing here.

CS5535 GPIOs can either be mapped to interrupts, *or* to a Power
Management Event (PME). A PME is a CS5535-specific concept; its an
event that will bring the system out of suspend if it is sleeping. In
this case, we are mapping a GPIO to a PME. I'm not aware of any
appropriate Linux abstraction for this. Thats all done in
cs5535_gpio_setup_event().

The CS5535 also has a highly programmable PIC, with loads of possible
input and mapping options. The call to cs5535_gpio_set_irq() then
programs the PIC to map PME input to an IRQ line. This function is
both misnamed and misplaced; it is unrelated to GPIOs and is simply
one of the many generic knobs on the PIC. If there was one, this
function should belong in a cs5535-pic driver, named something
according to "IRQ Mapper Unrestricted Z Select High".

So, I'm left feeling that irq_chip isn't an appropriate abstraction,
because the mapping made to the IRQ line is indirect via PME events,
and that is significant in the context of this driver (where wakeup
control is a big thing). However, it may be worthwhile moving
cs5535_gpio_set_irq() into cs5535.h as a static inline with a more
appropriate name. I could also add some explanatory comments in this
part of the code.

What do you think?

Thanks,
Daniel