| Message ID | 1284581577-2217-1-git-send-email-agust@denx.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Grant Likely |
| Headers | show |
On Wed, Sep 15, 2010 at 10:12:57PM +0200, Anatolij Gustschin wrote: > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension > breaks touch screen support on this board since the GPIO > interrupt will be mapped to 8xxx GPIO irq host resulting in > a not requestable interrupt in the touch screen driver. Fix > it by mapping the touch interrupt on 8xxx GPIO irq host. This looks wrong to me. The touchscreen code should not go mucking about in the GPIO controller registers; that is the job of the gpio driver. What is the reason that the touch interrupt isn't requestable? It looks like the 8xxx gpio driver is designed to hand out a separate virq number for each gpio pin (I've not had time to dig into details, so you'll need to educate me on the problem details) g. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > arch/powerpc/platforms/512x/pdm360ng.c | 26 ++++++++++++++++++++++---- > 1 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/platforms/512x/pdm360ng.c b/arch/powerpc/platforms/512x/pdm360ng.c > index 0575e85..558eb9e 100644 > --- a/arch/powerpc/platforms/512x/pdm360ng.c > +++ b/arch/powerpc/platforms/512x/pdm360ng.c > @@ -27,6 +27,7 @@ > #include <linux/spi/ads7846.h> > #include <linux/spi/spi.h> > #include <linux/notifier.h> > +#include <asm/gpio.h> > > static void *pdm360ng_gpio_base; > > @@ -50,7 +51,7 @@ static struct ads7846_platform_data pdm360ng_ads7846_pdata = { > .irq_flags = IRQF_TRIGGER_LOW, > }; > > -static int __init pdm360ng_penirq_init(void) > +static int pdm360ng_penirq_init(void) > { > struct device_node *np; > > @@ -73,6 +74,9 @@ static int __init pdm360ng_penirq_init(void) > return 0; > } > > +#define GPIO_NR(x) (ARCH_NR_GPIOS - 32 + (x)) > +#define PENDOWN_GPIO GPIO_NR(25) > + > static int pdm360ng_touchscreen_notifier_call(struct notifier_block *nb, > unsigned long event, void *__dev) > { > @@ -80,7 +84,24 @@ static int pdm360ng_touchscreen_notifier_call(struct notifier_block *nb, > > if ((event == BUS_NOTIFY_ADD_DEVICE) && > of_device_is_compatible(dev->of_node, "ti,ads7846")) { > + struct spi_device *spi = to_spi_device(dev); > + int gpio = PENDOWN_GPIO; > + > dev->platform_data = &pdm360ng_ads7846_pdata; > + if (pdm360ng_penirq_init()) > + return NOTIFY_DONE; > + > + if (gpio_request_one(gpio, GPIOF_IN, "ads7845_pen_down") < 0) { > + pr_err("Failed to request GPIO %d for " > + "ads7845 pen down IRQ\n", gpio); > + return NOTIFY_DONE; > + } > + spi->irq = gpio_to_irq(gpio); > + if (spi->irq < 0) { > + pr_err("Can't map GPIO IRQ\n"); > + gpio_free(gpio); > + return NOTIFY_DONE; > + } > return NOTIFY_OK; > } > return NOTIFY_DONE; > @@ -92,9 +113,6 @@ static struct notifier_block pdm360ng_touchscreen_nb = { > > static void __init pdm360ng_touchscreen_init(void) > { > - if (pdm360ng_penirq_init()) > - return; > - > bus_register_notifier(&spi_bus_type, &pdm360ng_touchscreen_nb); > } > #else > -- > 1.7.0.4 >
On Wed, 15 Sep 2010 20:38:23 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, Sep 15, 2010 at 10:12:57PM +0200, Anatolij Gustschin wrote: > > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension > > breaks touch screen support on this board since the GPIO > > interrupt will be mapped to 8xxx GPIO irq host resulting in > > a not requestable interrupt in the touch screen driver. Fix > > it by mapping the touch interrupt on 8xxx GPIO irq host. > > This looks wrong to me. The touchscreen code should not go mucking > about in the GPIO controller registers; that is the job of the gpio > driver. But if there is no GPIO driver (as it was the case before adding mpc512x support in the 8xxx gpio driver) or if the driver is not enabled in the kernel configuration? Then the platform specific callback (called from touchscreen driver) returns the pin state and acknowlegdes the interrupt. > What is the reason that the touch interrupt isn't > requestable? The 8xxx gpio driver sets up gpio irq host and installs the chained irq handler for GPIO interrupt 78 using set_irq_chained_handler() which sets the status field of the irq_desc structure to IRQ_NOREQUEST | IRQ_NOPROBE. Other drivers can't request this GPIO interrupt any more, request_threaded_irq() checks the IRQ_NOREQUEST status flag and returns -EINVAL if it is set. The gpio interrupts for each gpio pin are now handled by the mpc8xxx_gpio_irq_cascade() handler as they should. > It looks like the 8xxx gpio driver is designed to hand > out a separate virq number for each gpio pin (I've not had time to dig > into details, so you'll need to educate me on the problem details) Yes, exactly. This patch adds code to request the board's pen_down gpio pin and to use it's virq number in the touchscreen driver. The touchscreen driver can request this virq interrupt and it is now properly handled by the chained handler in the gpio driver. Anatolij
Hi Grant, On Wed, 15 Sep 2010 22:12:57 +0200 Anatolij Gustschin <agust@denx.de> wrote: > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension > breaks touch screen support on this board since the GPIO > interrupt will be mapped to 8xxx GPIO irq host resulting in > a not requestable interrupt in the touch screen driver. Fix > it by mapping the touch interrupt on 8xxx GPIO irq host. > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > --- > arch/powerpc/platforms/512x/pdm360ng.c | 26 ++++++++++++++++++++++---- > 1 files changed, 22 insertions(+), 4 deletions(-) Can this patch go in for 2.6.37 ? Thanks, Anatolij
On Thu, Oct 14, 2010 at 04:55:49PM +0200, Anatolij Gustschin wrote: > Hi Grant, > > On Wed, 15 Sep 2010 22:12:57 +0200 > Anatolij Gustschin <agust@denx.de> wrote: > > > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension > > breaks touch screen support on this board since the GPIO > > interrupt will be mapped to 8xxx GPIO irq host resulting in > > a not requestable interrupt in the touch screen driver. Fix > > it by mapping the touch interrupt on 8xxx GPIO irq host. > > > > Signed-off-by: Anatolij Gustschin <agust@denx.de> > > --- > > arch/powerpc/platforms/512x/pdm360ng.c | 26 ++++++++++++++++++++++---- > > 1 files changed, 22 insertions(+), 4 deletions(-) > > Can this patch go in for 2.6.37 ? I hope so, but I haven't looked at it properly yet and I'm trying to dig myself out of a patch backlog. g.
On Sat, Sep 25, 2010 at 10:22:44PM +0200, Anatolij Gustschin wrote: > On Wed, 15 Sep 2010 20:38:23 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > > > On Wed, Sep 15, 2010 at 10:12:57PM +0200, Anatolij Gustschin wrote: > > > Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension > > > breaks touch screen support on this board since the GPIO > > > interrupt will be mapped to 8xxx GPIO irq host resulting in > > > a not requestable interrupt in the touch screen driver. Fix > > > it by mapping the touch interrupt on 8xxx GPIO irq host. > > > > This looks wrong to me. The touchscreen code should not go mucking > > about in the GPIO controller registers; that is the job of the gpio > > driver. > > But if there is no GPIO driver (as it was the case before adding > mpc512x support in the 8xxx gpio driver) or if the driver is not > enabled in the kernel configuration? Then the platform specific > callback (called from touchscreen driver) returns the pin state > and acknowlegdes the interrupt. So, basically the touchscreen device node has an interrupts property which does not use the gpio controller as the interrupt controller, but instead points directly and the interrupt controller that the gpio controller is cascaded from. Really it sounds like the device tree data is broken. The preferred solution is be to fix the device tree to declare the gpio node as an interrupt controller. > > > What is the reason that the touch interrupt isn't > > requestable? > > The 8xxx gpio driver sets up gpio irq host and installs > the chained irq handler for GPIO interrupt 78 using > set_irq_chained_handler() which sets the status field of > the irq_desc structure to IRQ_NOREQUEST | IRQ_NOPROBE. > Other drivers can't request this GPIO interrupt any more, > request_threaded_irq() checks the IRQ_NOREQUEST status > flag and returns -EINVAL if it is set. The gpio interrupts > for each gpio pin are now handled by the > mpc8xxx_gpio_irq_cascade() handler as they should. > > > It looks like the 8xxx gpio driver is designed to hand > > out a separate virq number for each gpio pin (I've not had time to dig > > into details, so you'll need to educate me on the problem details) > > Yes, exactly. This patch adds code to request the > board's pen_down gpio pin and to use it's virq number in > the touchscreen driver. The touchscreen driver can > request this virq interrupt and it is now properly handled > by the chained handler in the gpio driver. ...but it does so by hard coding the irq number (via the GPIO number) into the board code; a situation which I've tried very hard to avoid. This is what I'm not okay with. Another solution is to modify the 8xxx gpio driver to cascade off the normal request_irq() path instead of via set_irq_chained_handler(), but that *might* have unacceptable performance impact for other users. Unfortunately, I've been slow on this patch, so I cannot get anything into 2.6.37 (sorry). However, I've not asked Linus to pull the 8xxx gpio driver changes either so nothing in mainline will get broken. g.
diff --git a/arch/powerpc/platforms/512x/pdm360ng.c b/arch/powerpc/platforms/512x/pdm360ng.c index 0575e85..558eb9e 100644 --- a/arch/powerpc/platforms/512x/pdm360ng.c +++ b/arch/powerpc/platforms/512x/pdm360ng.c @@ -27,6 +27,7 @@ #include <linux/spi/ads7846.h> #include <linux/spi/spi.h> #include <linux/notifier.h> +#include <asm/gpio.h> static void *pdm360ng_gpio_base; @@ -50,7 +51,7 @@ static struct ads7846_platform_data pdm360ng_ads7846_pdata = { .irq_flags = IRQF_TRIGGER_LOW, }; -static int __init pdm360ng_penirq_init(void) +static int pdm360ng_penirq_init(void) { struct device_node *np; @@ -73,6 +74,9 @@ static int __init pdm360ng_penirq_init(void) return 0; } +#define GPIO_NR(x) (ARCH_NR_GPIOS - 32 + (x)) +#define PENDOWN_GPIO GPIO_NR(25) + static int pdm360ng_touchscreen_notifier_call(struct notifier_block *nb, unsigned long event, void *__dev) { @@ -80,7 +84,24 @@ static int pdm360ng_touchscreen_notifier_call(struct notifier_block *nb, if ((event == BUS_NOTIFY_ADD_DEVICE) && of_device_is_compatible(dev->of_node, "ti,ads7846")) { + struct spi_device *spi = to_spi_device(dev); + int gpio = PENDOWN_GPIO; + dev->platform_data = &pdm360ng_ads7846_pdata; + if (pdm360ng_penirq_init()) + return NOTIFY_DONE; + + if (gpio_request_one(gpio, GPIOF_IN, "ads7845_pen_down") < 0) { + pr_err("Failed to request GPIO %d for " + "ads7845 pen down IRQ\n", gpio); + return NOTIFY_DONE; + } + spi->irq = gpio_to_irq(gpio); + if (spi->irq < 0) { + pr_err("Can't map GPIO IRQ\n"); + gpio_free(gpio); + return NOTIFY_DONE; + } return NOTIFY_OK; } return NOTIFY_DONE; @@ -92,9 +113,6 @@ static struct notifier_block pdm360ng_touchscreen_nb = { static void __init pdm360ng_touchscreen_init(void) { - if (pdm360ng_penirq_init()) - return; - bus_register_notifier(&spi_bus_type, &pdm360ng_touchscreen_nb); } #else
Enabling the MPC8xxx GPIO driver with MPC512x GPIO extension breaks touch screen support on this board since the GPIO interrupt will be mapped to 8xxx GPIO irq host resulting in a not requestable interrupt in the touch screen driver. Fix it by mapping the touch interrupt on 8xxx GPIO irq host. Signed-off-by: Anatolij Gustschin <agust@denx.de> --- arch/powerpc/platforms/512x/pdm360ng.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)