ARM:mfd: fix ezx-pcap build failure
Subsystems (2)
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
Attempting to build the ezx_defconfig we get a build failure:
drivers/mfd/ezx-pcap.c: In function 'pcap_isr_work':drivers/mfd/ezx-pcap.c:205: error: implicit declaration of function 'irq_to_gpio'
Looking at this failure we can find that commit 4929f5a8a99f [ARM:pxa: rename gpio_to_irq and irq_to_gpio] renamed irq_to_gpio() to pxa_irq_to_gpio() but this change was not made in the ezx-pcap driver.
--- drivers/mfd/ezx-pcap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/mfd/ezx-pcap.c +2 -1
@@ -19,6 +19,7 @@#include <linux/spi/spi.h> #include <linux/gpio.h> #include <linux/slab.h> +#include <linux/gpio-pxa.h> #define PCAP_ADC_MAXQ 8struct pcap_adc_request {
@@ -202,7 +203,7 @@ static void pcap_isr_work(struct work_struct *work) }local_irq_enable ();ezx_pcap_write (pcap,PCAP_REG_MSR , pcap->msr); - }while (gpio_get_value (irq_to_gpio (pcap->spi->irq))); + }while (gpio_get_value (pxa_irq_to_gpio (pcap->spi->irq))); }static void pcap_irq_handler (unsigned int irq,struct irq_desc *desc)
Signature · 1 line
1.7.5.4
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
Quoted 9 lines · Mark Asselstine
Attempting to build the
ezx_defconfigwe get a build failure:
drivers/mfd/ezx-pcap.c: In function 'pcap_isr_work':drivers/mfd/ezx-pcap.c:205: error: implicit declaration of function 'irq_to_gpio'Looking at this failure we can find that commit
4929f5a8a99f[ARM:pxa: renamegpio_to_irqandirq_to_gpio] renamedirq_to_gpio()topxa_irq_to_gpio()but this change was not made in the ezx-pcap driver.
That's because drivers shouldn't have knowledge about the SoC they're running on. This driver is not restricted to only being built for PXA, and so with your change, this will cause a link error should this be built on non-PXA.
However, there's a big problem with code using irq_to_gpio() or any variant of that in this manner:
} while (gpio_get_value(irq_to_gpio(pcap->spi->irq)));
What is the effect when the supplied IRQ does not have a mapping to a GPIO - or it _does_ by way of a badly coded irq_to_gpio() function but that GPIO is not the correct one.
There is no prevention against endlessly looping, so it could cause a system lockup.
The real answer is to fix SPI et.al. so that its possible to pass the _GPIO_ number into this driver (that's what the driver wants for its interrupt line after all). To fix that, SPI folk need to get involved (added Grant as a first step.)
In the meantime, I'd suggest making this depend on !ARM || BROKEN until it's probably fixed.
Quoted 35 lines
Signed-off-by Mark Asselstine <[email protected]>--- drivers/mfd/ezx-pcap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/mfd/ezx-pcap.c +2
-1@@ -19,6 +19,7 @@#include <linux/spi/spi.h> #include <linux/gpio.h> #include <linux/slab.h> +#include <linux/gpio-pxa.h> #define PCAP_ADC_MAXQ 8struct pcap_adc_request {@@ -202,7 +203,7 @@ static void pcap_isr_work(struct work_struct *work) }local_irq_enable ();ezx_pcap_write (pcap,PCAP_REG_MSR , pcap->msr); - }while (gpio_get_value (irq_to_gpio (pcap->spi->irq))); + }while (gpio_get_value (pxa_irq_to_gpio (pcap->spi->irq))); }static void pcap_irq_handler (unsigned int irq,struct irq_desc *desc)Signature · 7 lines
1.7.5.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
Quoted 4 lines · Russell King - ARM Linux
The real answer is to fix SPI et.al. so that its possible to pass the _GPIO_ number into this driver (that's what the driver wants for its interrupt line after all). To fix that, SPI folk need to get involved (added Grant as a first step.)
Or, in the short term, add platform data to pass in a GPIO and ignore any IRQ that might get passed in.
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
In reply to Russell King - ARM LinuxQuoted 12 lines · On April 26, 2012 22:52:58 Russell King - ARM Linux wrote:
Quoted 10 lines · Mark Asselstine
Attempting to build the
ezx_defconfigwe get a build failure:
drivers/mfd/ezx-pcap.c: In function 'pcap_isr_work':drivers/mfd/ezx-pcap.c:205: error: implicit declaration of function 'irq_to_gpio'Looking at this failure we can find that commit
4929f5a8a99f[ARM:pxa: renamegpio_to_irqandirq_to_gpio] renamedirq_to_gpio()topxa_irq_to_gpio()but this change was not made in the ezx-pcap driver.
Quoted 4 lines
That's because drivers shouldn't have knowledge about the SoC they're running on. This driver is not restricted to only being built for PXA, and so with your change, this will cause a link error should this be built on non-PXA.
Agreed. I haven't dissected the rest of the file to see if there are other platform bits leaking in but the filename also wraps the platform name.
Quoted 4 lines
However, there's a big problem with code using
irq_to_gpio()or any variant of that in this manner:
Quoted 8 lines
} while (gpio_get_value(irq_to_gpio(pcap->spi->irq)));What is the effect when the supplied IRQ does not have a mapping to a GPIO - or it _does_ by way of a badly coded
irq_to_gpio()function but that GPIO is not the correct one.There is no prevention against endlessly looping, so it could cause a system lockup.
Unfortunately the commit [b1148fd4 mfd: fix pcap irq bottom handler ] which modified things to loop as long as the interrupt is asserted didn't supply much information regarding the behavior they were trying to achieve/fix nor what would be the consequence of bailing earlier.
Quoted 8 lines
The real answer is to fix SPI et.al. so that its possible to pass the _GPIO_ number into this driver (that's what the driver wants for its interrupt line after all). To fix that, SPI folk need to get involved (added Grant as a first step.)
In the meantime, I'd suggest making this depend on !ARM || BROKEN until it's probably fixed.
I will look at having the gpio num passed to the driver one way or another and will take any suggestions from Grant but without access to this hardware I am inclined to do as you suggest and work around things with this patch and a modified Kconfig , possibly making this driver depend on CONFIG_PXA_EZX.
Thanks,
Mark
Quoted 34 lines
Quoted 34 lines
Signed-off-by Mark Asselstine <[email protected]>---
drivers/mfd/ezx-pcap.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)drivers/mfd/ezx-pcap.c +1
-0@@ -19,6 +19,7 @@#include <linux/spi/spi.h> #include <linux/gpio.h> #include <linux/slab.h> +#include <linux/gpio-pxa.h> #define
PCAP_ADC_MAXQ8
structpcap_adc_request{drivers/mfd/ezx-pcap.c +1
-1@@ -202,7 +203,7 @@ static void pcap_isr_work(struct work_struct *work) }local_irq_enable ();ezx_pcap_write (pcap,PCAP_REG_MSR , pcap->msr); - }while (gpio_get_value (irq_to_gpio (pcap->spi->irq))); + }while (gpio_get_value (pxa_irq_to_gpio (pcap->spi->irq)));}
static void
pcap_irq_handler(unsigned int irq, struct irq_desc *desc)
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
Quoted 6 lines · Mark Asselstine
Quoted 3 lines · On April 26, 2012 22:52:58 Russell King - ARM Linux wrote:
What is the effect when the supplied IRQ does not have a mapping to a GPIO - or it _does_ by way of a badly coded
irq_to_gpio()function but that GPIO is not the correct one.
Quoted 7 lines
Quoted 2 lines
There is no prevention against endlessly looping, so it could cause a system lockup.
Unfortunately the commit [
b1148fd4mfd: fix pcap irq bottom handler ] which modified things to loop as long as the interrupt is asserted didn't supply much information regarding the behavior they were trying to achieve/fix nor what would be the consequence of bailing earlier.
The usual reason for this pattern is to simulate level triggered IRQs on an edge triggered interrupt controller.
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
Quoted 11 lines · On April 27, 2012 19:02:35 Mark Brown wrote:
Quoted 10 lines · Mark Asselstine
Quoted 8 lines · On April 26, 2012 22:52:58 Russell King - ARM Linux wrote:
What is the effect when the supplied IRQ does not have a mapping to a
GPIO - or it _does_ by way of a badly codedirq_to_gpio()function but that GPIO is not the correct one.There is no prevention against endlessly looping, so it could cause a
system lockup.
Quoted 7 lines
Quoted 4 lines
Unfortunately the commit [
b1148fd4mfd: fix pcap irq bottom handler ] which modified things to loop as long as the interrupt is asserted didn't supply much information regarding the behavior they were trying to achieve/fix nor what would be the consequence of bailing earlier.The usual reason for this pattern is to simulate level triggered IRQs on an edge triggered interrupt controller.
Makes sense. OK. I want to help here with a complete fix but it has been a while since I have waded into SPI and ARM initialization so here are some stumbling blocks for me.
If I first fix the build issue by adding 'int gpio_num' to the pcap_platform_data and made use of pdata->gpio_num in place of irq_to_gpio() in pcap_isr_work(). Now to set gpio_num.
In the board .c file I would assume the goal would be to create the necessary structs to set gpio_num for "ezx-pcap" and pass them to spi_register_board_info(). Right? But should I expect to already see some of this infrastructure in place already? Doesn't the "ezx-pcap" device already have to be defined somewhere, to have irq_base set for example? Or is this coming from firmware?
Thanks,
Mark A.
PATCH 1/1 ARM:mfd: fix ezx-pcap build failure
In reply to Russell King - ARM LinuxQuoted 16 lines · Russell King - ARM Linux
Quoted 9 lines · Mark Asselstine
Attempting to build the
ezx_defconfigwe get a build failure:
drivers/mfd/ezx-pcap.c: In function 'pcap_isr_work':drivers/mfd/ezx-pcap.c:205: error: implicit declaration of function 'irq_to_gpio'Looking at this failure we can find that commit
4929f5a8a99f[ARM:pxa: renamegpio_to_irqandirq_to_gpio] renamedirq_to_gpio()topxa_irq_to_gpio()but this change was not made in the ezx-pcap driver.That's because drivers shouldn't have knowledge about the SoC they're running on. This driver is not restricted to only being built for PXA, and so with your change, this will cause a link error should this be built on non-PXA.
Quoted 8 lines
However, there's a big problem with code using
irq_to_gpio()or any variant of that in this manner:} while (gpio_get_value(irq_to_gpio(pcap->spi->irq)));What is the effect when the supplied IRQ does not have a mapping to a GPIO - or it _does_ by way of a badly coded
irq_to_gpio()function but that GPIO is not the correct one.
... like if multiple gpios map to the same IRQ.
Quoted 7 lines
There is no prevention against endlessly looping, so it could cause a system lockup.
The real answer is to fix SPI et.al. so that its possible to pass the _GPIO_ number into this driver (that's what the driver wants for its interrupt line after all). To fix that, SPI folk need to get involved (added Grant as a first step.)
This doesn't look like a core SPI problem to me. This specific driver needs a gpio connection to work and therefore it should be passed in via either the DT binding or platform_data as Mark suggests.
g.