* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-26 21:52 ` Russell King - ARM Linux
0 siblings, 0 replies; 14+ messages in thread
From: Russell King - ARM Linux @ 2012-04-26 21:52 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] ARM:mfd: fix ezx-pcap build failure
2012-04-26 21:52 ` Russell King - ARM Linux
@ 2012-04-26 22:04 ` Mark Brown
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2012-04-26 22:04 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Mark Asselstine, Grant Likely, openezx-devel, linux-next, sameo,
linux-arm-kernel, haojian.zhuang
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-26 22:04 ` Mark Brown
0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2012-04-26 22:04 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM:mfd: fix ezx-pcap build failure
2012-04-26 21:52 ` Russell King - ARM Linux
@ 2012-04-27 15:28 ` Mark Asselstine
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Asselstine @ 2012-04-27 15:28 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Grant Likely, linux-next, openezx-devel, sameo, linux-arm-kernel,
haojian.zhuang
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)
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-27 15:28 ` Mark Asselstine
0 siblings, 0 replies; 14+ messages in thread
From: Mark Asselstine @ 2012-04-27 15:28 UTC (permalink / raw)
To: linux-arm-kernel
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)
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] ARM:mfd: fix ezx-pcap build failure
2012-04-27 15:28 ` Mark Asselstine
@ 2012-04-27 18:02 ` Mark Brown
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2012-04-27 18:02 UTC (permalink / raw)
To: Mark Asselstine
Cc: Russell King - ARM Linux, openezx-devel, sameo, haojian.zhuang,
Grant Likely, linux-next, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-27 18:02 ` Mark Brown
0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2012-04-27 18:02 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM:mfd: fix ezx-pcap build failure
2012-04-27 18:02 ` Mark Brown
@ 2012-04-30 17:26 ` Mark Asselstine
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Asselstine @ 2012-04-30 17:26 UTC (permalink / raw)
To: Mark Brown
Cc: Russell King - ARM Linux, openezx-devel, sameo, haojian.zhuang,
Grant Likely, linux-next, linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-30 17:26 ` Mark Asselstine
0 siblings, 0 replies; 14+ messages in thread
From: Mark Asselstine @ 2012-04-30 17:26 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ARM:mfd: fix ezx-pcap build failure
2012-04-26 21:52 ` Russell King - ARM Linux
@ 2012-04-27 16:09 ` Grant Likely
-1 siblings, 0 replies; 14+ messages in thread
From: Grant Likely @ 2012-04-27 16:09 UTC (permalink / raw)
To: Russell King - ARM Linux, Mark Asselstine
Cc: linux-next, openezx-devel, sameo, linux-arm-kernel,
haojian.zhuang
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ARM:mfd: fix ezx-pcap build failure
@ 2012-04-27 16:09 ` Grant Likely
0 siblings, 0 replies; 14+ messages in thread
From: Grant Likely @ 2012-04-27 16:09 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread