[PATCH] ARM:mfd: fix ezx-pcap build failure

[PATCH] ARM:mfd: fix ezx-pcap build failure

[Mark Asselstine]

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.

Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
---
 drivers/mfd/ezx-pcap.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index 43a76c4..c49c52a 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -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		8
 struct 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)
-- 
1.7.5.4

[PATCH] ARM:mfd: fix ezx-pcap build failure

[Russell King - ARM Linux]

On Wed, Apr 25, 2012 at 11:02:43AM -0400, Mark Asselstine wrote:
> 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.

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.

> 
> Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> ---
>  drivers/mfd/ezx-pcap.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
> index 43a76c4..c49c52a 100644
> --- a/drivers/mfd/ezx-pcap.c
> +++ b/drivers/mfd/ezx-pcap.c
> @@ -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		8
>  struct 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)
> -- 
> 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] ARM:mfd: fix ezx-pcap build failure

[Mark Brown]

On Thu, Apr 26, 2012 at 10:52:58PM +0100, Russell King - ARM Linux wrote:

> 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] ARM:mfd: fix ezx-pcap build failure

[Mark Asselstine]

On April 26, 2012 22:52:58 Russell King - ARM Linux wrote:
> On Wed, Apr 25, 2012 at 11:02:43AM -0400, Mark Asselstine wrote:
> > 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.
> 
> 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.

> 
> 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.

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.

> 
> 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

> 
> > Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> > ---
> > 
> >  drivers/mfd/ezx-pcap.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
> > index 43a76c4..c49c52a 100644
> > --- a/drivers/mfd/ezx-pcap.c
> > +++ b/drivers/mfd/ezx-pcap.c
> > @@ -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		8
> >  struct 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)

[PATCH] ARM:mfd: fix ezx-pcap build failure

[Mark Brown]

On Fri, Apr 27, 2012 at 11:28:28AM -0400, Mark Asselstine wrote:
> 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.

> > 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.

The usual reason for this pattern is to simulate level triggered IRQs on
an edge triggered interrupt controller.

[PATCH] ARM:mfd: fix ezx-pcap build failure

[Mark Asselstine]

On April 27, 2012 19:02:35 Mark Brown wrote:
> On Fri, Apr 27, 2012 at 11:28:28AM -0400, Mark Asselstine wrote:
> > 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.
> > > 
> > > 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.
> 
> 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] ARM:mfd: fix ezx-pcap build failure

[Grant Likely]

On Thu, 26 Apr 2012 22:52:58 +0100, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Wed, Apr 25, 2012 at 11:02:43AM -0400, Mark Asselstine wrote:
> > 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.
> 
> 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.

... like if multiple gpios map to the same IRQ.

> 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.