mfd: ADP5520 Multifunction LCD Backlight and KeypadInput Device Driver
in [Kernel]
|
Prev: AW: AW: AW: clock_nanosleep: cyclic POSIX thread freezes during time setting
Next: PCI PM: Read device power state from register after updating it
From: Hennerich, Michael on 7 Oct 2009 08:20 >From: Mark Brown [mailto:broonie(a)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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Hennerich, Michael on 7 Oct 2009 09:10 >From: Hennerich, Michael >>From: Mark Brown [mailto:broonie(a)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); > } > > 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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Hennerich, Michael on 7 Oct 2009 09:50
>From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] > >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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |