RE: [PATCH v3] mfd: ADP5520 Multifunction LCD Backlight and KeypadInput Device Driver

[Hennerich, Michael]

>From: Hennerich, Michael
>>From: Mark Brown [mailto:broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx]
>>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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/