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 6 Oct 2009 08:40 Hi Mark, >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] >On Tue, Oct 06, 2009 at 03:44:31AM -0400, Mike Frysinger wrote: > >> +int adp5520_register_notifier(struct device *dev, struct notifier_block *nb, >> + unsigned int events) >> +{ > >This notifier stuff looks an awful lot like an interrupt controller >driver. Now that it's possible to implement support for an I2C/SPI >driven interrupt controller it'd be good to use that rather than having >a custom API if that's possible. Honestly this notifier chain is a clean approach and serves its purpose here pretty well. IMHO it's much more preferable than pretending there is a virtual GPIO that doesn't exist and a MFD subdev could request. > >> +#define GPIO_R3 (1 << 3) /* LED3 or GPIO3 aka R3 */ >> +#define GPIO_R2 (1 << 2) >> +#define GPIO_R1 (1 << 1) >> +#define GPIO_R0 (1 << 0) > >> +struct adp5520_gpio_platfrom_data { >> + unsigned gpio_start; >> + u8 gpio_en_mask; >> + u8 gpio_pullup_mask; >> +}; > >Since there's platform data in here it'd probably be better to namespace >the #defines or put them in a separate header in case there are >collisions with some other chip used on a board. Will do. > >> +struct adp5520_leds_platfrom_data { > >There's some 'platfrom' rather than 'platform' typos. Good catch - this happens to me all the time > >> + int num_leds; >> + struct led_info *leds; >> + u8 fade_in; /* Backlight Fade-In Timer */ >> + u8 fade_out; /* Backlight Fade-Out Timer */ >> + u8 led_on_time; > >I don't know exactly what the on_time option does but if it controls >hardware-implemented blinking there's actually a callback function the >LED driver can implement to allow triggers to use the hardware assisted >blinking at runtime. If it returns an error or isn't implemented >there's a software fallback. Yes its hardware controlled blinking. I noticed the leds timer trigger driver. People can still use it - the downside with the hardware assisted blinking is that all LEDs share the same on_time. So I decided against using the callback. -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 6 Oct 2009 09:00 >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] >On Tue, Oct 06, 2009 at 01:23:52PM +0100, Hennerich, Michael wrote: >> >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] > >> >This notifier stuff looks an awful lot like an interrupt controller >> >driver. Now that it's possible to implement support for an I2C/SPI >> >driven interrupt controller it'd be good to use that rather than having >> >a custom API if that's possible. > >> Honestly this notifier chain is a clean approach and serves its purpose >> here pretty well. >> IMHO it's much more preferable than pretending there is a virtual GPIO >> that doesn't exist and a MFD subdev could request. > >I'm not sure what the association with virtual gpios is? This is all >separate to gpiolib except in that it would mean that a gpio driver for >the device would be able to export these interrupts to its clients. This is what I meant. So you propose having the MFD Core as well as its subdevs requesting the ADP5520 IRQ (client->irq) IRQF_SHARED? I think we already excluded us from using this option when we were asked to move to the NEW threaded irqs? -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 6 Oct 2009 11:20 >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] >On Tue, Oct 06, 2009 at 03:32:56PM +0100, Hennerich, Michael wrote: > >> This is not an interrupt controller. >> The only ADP5520 subdev that needs to be notified is the adp5520-keys >> input driver, if present. >> Sounds like overshoot, registering a irq_chip using >> set_irq_chip_and_handler() and friends, for exactly one dedicated and >> known consumer. > >According to the datasheet the GPIOs, light sensor and regulator can >also generate interrupts? Right - I know - but none of the subdevs are currently using this functionality. >The GPIOs in particular would benefit from >this since this would mean that their interrupts would be usable by >generic gpiolib based drivers. Right - and I didn't say that I'm not going to add this functionality later. All the ADP5520 subdevs already merged in 2.6.32 - the MFD core is the only remaining part. Time is running short and major changes are going to require another round of full testing. -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 6 Oct 2009 10:40 >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] >On Tue, Oct 06, 2009 at 01:55:45PM +0100, Hennerich, Michael wrote: >> >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] > >> >I'm not sure what the association with virtual gpios is? This is all >> >separate to gpiolib except in that it would mean that a gpio driver for >> >the device would be able to export these interrupts to its clients. > >> This is what I meant. >> So you propose having the MFD Core as well as its subdevs requesting the >> ADP5520 IRQ (client->irq) IRQF_SHARED? I think we already excluded us >> from using this option when we were asked to move to the NEW threaded >> irqs? > >No, I'm suggesting implementing an IRQ controller driver for the device >- 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. This gives access to all the genirq infrastrure >features rather than having to implement a custom IRQ handling stack. > >Like I say, I'm not sure what you meant when you were talking about >virtual gpios. This is not an interrupt controller. The only ADP5520 subdev that needs to be notified is the adp5520-keys input driver, if present. Sounds like overshoot, registering a irq_chip using set_irq_chip_and_handler() and friends, for exactly one dedicated and known consumer. -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 05:00
>From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] >On Tue, Oct 06, 2009 at 04:05:45PM +0100, Hennerich, Michael wrote: >> >From: Mark Brown [mailto:broonie(a)opensource.wolfsonmicro.com] > >> >According to the datasheet the GPIOs, light sensor and regulator can >> >also generate interrupts? > >> Right - I know - but none of the subdevs are currently using this >> functionality. > >You weren't very clear on the difference between the current state of >the drivers and the capabilities of the chip there. 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. There are exactly 8 GP signals which are muxed with Keypad and GPIO. In case you use a 4x4 Keypad there is no GPIO left. In case you use a 3x4 Keypad there is exactly 1 GPIO that can be exposed to the gpiolib. 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? -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/ |