Content-Class: urn:content-classes:message
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
Subject: RE: [PATCH v3] mfd: ADP5520 Multifunction LCD Backlight and KeypadInput Device Driver
Date: Wed, 7 Oct 2009 13:11:40 +0100
Message-ID: <8A42379416420646B9BFAC9682273B6D0E442F71@limkexm3.ad.analog.com>
In-Reply-To: <20091007100638.GB3169@rakim.wolfsonmicro.main>
thread-topic: [PATCH v3] mfd: ADP5520 Multifunction LCD Backlight and KeypadInput Device Driver
thread-index: AcpHNeGj/gwW09AJQUS2/H1MYJjuPgABC5og
References: <20091006115543.GH27168@sirena.org.uk> <8A42379416420646B9BFAC9682273B6D0E3F4348@limkexm3.ad.analog.com> <20091006123658.GA31079@rakim.wolfsonmicro.main> <8A42379416420646B9BFAC9682273B6D0E3F4420@limkexm3.ad.analog.com> <20091006135836.GA3926@rakim.wolfsonmicro.main> <8A42379416420646B9BFAC9682273B6D0E3F46A7@limkexm3.ad.analog.com> <20091006144805.GA10987@rakim.wolfsonmicro.main> <8A42379416420646B9BFAC9682273B6D0E3F4769@limkexm3.ad.analog.com> <20091006160549.GA8881@rakim.wolfsonmicro.main> <8A42379416420646B9BFAC9682273B6D0E3F4CEE@limkexm3.ad.analog.com> <20091007100638.GB3169@rakim.wolfsonmicro.main>
From: "Hennerich, Michael" <Michael.Hennerich@analog.com>
To: "Mark Brown" <broonie@opensource.wolfsonmicro.com>
CC: "Mike Frysinger" <vapier@gentoo.org>,
       "Samuel Ortiz" <sameo@linux.intel.com>,
       <uclinux-dist-devel@blackfin.uclinux.org>,
       <linux-kernel@vger.kernel.org>, "Bryan Wu" <cooloney@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4419
Lines: 126

>From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
>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);
	}


>
>I think I forgot to mention it previously but there's some work on
>getting a standard ALS interface in the kernel too.  I'd really expect
>the GPIOs to end up being used as GPIOs in some designs as well.

This is really interesting. Do you know where this discussion currently
takes place, and who is taking the lead (came up with a proposal)?

-Michael

>
>> In one of your earlier posts you mentioned: "register an irq_chip for
>> the interrupt controller on it.  Support for doing this on I2C
devices
>> was added at pretty much the same time as the IRQ_ONESHOT support."
>
>> Can you point me to what exactly was added to support this on I2C/SPI
>> devices?
>
>These commits (which were merged during the merge window):
>
>4dbc9ca genirq: Do not mask oneshot edge type interrupts
>399b5da genirq: Support nested threaded irq handling
>70aedd2 genirq: Add buslock support
>
>There may be some other fixup patches afterwards too, but the buslock
>and nested threaded handers are the key ones.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/