>From: Hennerich, Michael
>>From: Mark Brown [mailto:[email protected]]
>>Subject: Re: [PATCH v3] mfd: ADP5520 Multifunction LCD Backlight and
KeypadInput Device Driver
>>
>>On Wed, Oct 07, 2009 at 09:50:43AM +0100, Hennerich, Michael wrote:
>>
>>> As you said there are chip internal interrupt sources for I/Os,
keypad
>>> presses and releases, ambient light sensor comparator states, and
>>> overvoltage conditions.
>>> The current state of the driver uses only interrupts for the keypad.
>>> I think you agree that its common practice to only implement
>>> functionality for chip features that are typically used.
>>
>>Yes, but equally well it's good to avoid future issues as the driver
is
>>enhanced. To be honest the main thing that made me notice when
reading
>>the patch was the use of the blocking notifier pattern to implement
the
>>interrupt infrastructure, mostly since it doesn't look like an IRQ
>>driver and the naming hides the fat that that's what it is.
>
>In case I would have done the whole ADP5520 in a single file exposing
functionality to the input,
>backlight, led and gpio infrastructure - I probably wouldn't find a
subtree maintainer that is likely
>to merge this blob.
>But apart from the GPIO interrupt capabilities - I wouldn't need doing
an interrupt controller.
>I think you agree.
>
>So this notifier chain seemed like a good approach to notifiy the
input/keypad/adp5520-keys about
>some work. BTW - this approach is used by other drivers for exactly the
same reason too.
>
>Honestly - I'm not yet convinced that this new irq stuff really works
in combination with my ADP5520
>Low Level IRQ.
>My chained_handler (for demux) as well as irq_desc .mask .unmask .ack
and .set_type need to also be
>allowed to invoke sleeping i2c transfers!!!?
>
>If I understood you right:
>
>Instead of request_threaded_irq() in my MFD core:
>
>I should do following: (unfortunately this is all on the bleeding edge
of technology, with no example
>driver actually using this craft)
>
>
>static struct irq_chip adp5520_irqchip = {
> .name = "ADP5520",
> .mask = adp5520_irq_mask, /* MAYSLEEP */
> .unmask = adp5520_irq_unmask, /* MAYSLEEP */
> .set_type = adp5520_irq_type, /* MAYSLEEP */
>};
>
>static void adp5520_irq_handler(unsigned irq, struct irq_desc *desc)
>{
>
> /* MAYSLEEP */
>
>}
>
>--snip--
>
> set_irq_data(client->irq, chip);
> set_irq_chained_handler(client->irq, adp5520_irq_handler);
>
>Or instead of using the set_irq_chained_handler
>- I should use request_threaded_irq() and do whatever I wanted to do in
adp5520_irq_handler(unsigned
>irq, struct irq_desc *desc), in my irq_thread???
>
>For the additional interrupts exposed by the ADP5520 MFD Core Interrupt
Controller I do this:
>
>
> set_irq_chip_and_handler_name(chip->irq_base +
ADP5520_KEYPAD_IRQ, &adp5520_irqchip,
> handle_nested_irq,
"adp5520-demux");
> set_irq_chip_data(chip->irq_base + ADP5520_KEYPAD_IRQ, chip);
>
> set_irq_nested_thread(chip->irq_base + ADP5520_KEYPAD_IRQ, 1);
>
> for (i = ADP5520_KEYPAD_IRQ; i < ngpio; i++) {
> set_irq_chip_and_handler_name(i + chip->irq_base,
&adp5520_irqchip,
> handle_nested_irq,
"adp5520-demux");
> set_irq_chip_data(i + chip->irq_base, chip);
> set_irq_nested_thread(chip->irq_base + i, 1);
> }
>
>
Mark,
Most architectures define NR_IRQS to exactly the number of on-chip IRQs.
Therefore irq_desc pointers > NR_IRQS will fail on most architectures.
This driver should be usable out of the box - but I guess this may
prevent this.
-Michael
On Wed, Oct 07, 2009 at 02:01:42PM +0100, Hennerich, Michael wrote:
> Most architectures define NR_IRQS to exactly the number of on-chip IRQs.
> Therefore irq_desc pointers > NR_IRQS will fail on most architectures.
> This driver should be usable out of the box - but I guess this may
> prevent this.
Indeed, that's rather unfortunate. This wouldn't be the only driver
with that dependency, though, so I'd expect to see the interesting
architectures getting fixed.
>From: Mark Brown [mailto:[email protected]]
>
>On Wed, Oct 07, 2009 at 02:01:42PM +0100, Hennerich, Michael wrote:
>
>> Most architectures define NR_IRQS to exactly the number of on-chip
IRQs.
>> Therefore irq_desc pointers > NR_IRQS will fail on most
architectures.
>> This driver should be usable out of the box - but I guess this may
>> prevent this.
>
>Indeed, that's rather unfortunate. This wouldn't be the only driver
>with that dependency, though, so I'd expect to see the interesting
>architectures getting fixed.
We have an update patch that fixes the platform typo and namespace
pollution in the header file.
I think we send out this patch as final for mainline inclusion.
For the interrupt controller feature - I think I can wait until the
interesting architectures get fixed.
Thanks for your feedback and help
best regards,
Michael