[PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

platform_get_irq returns -ENXIO on failure, so !irq was probably
always true.  Better use (int)irq <= 0.  Note that a return value of
zero is still handled as error even though this could mean irq0.

This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
changed the return value of platform_get_irq from 0 to -ENXIO on error.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: David Vrabel <dvrabel@arcom.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: spi-devel-general@lists.sourceforge.net
---
 drivers/spi/spi_mpc8xxx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index e9390d7..b13501a 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -1339,7 +1339,7 @@ static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	irq = platform_get_irq(pdev, 0);
-	if (!irq)
+	if ((int)irq <= 0)
 		return -EINVAL;
 
 	master = mpc8xxx_spi_probe(&pdev->dev, mem, irq);
-- 
1.6.5.2

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Dec 16, 2009 at 05:10:08PM +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !irq was probably
> always true.  Better use (int)irq <= 0.  Note that a return value of
> zero is still handled as error even though this could mean irq0.
> 
> This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> changed the return value of platform_get_irq from 0 to -ENXIO on error.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

Noooooo... :-(

Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
and fix platforms to remap HWIRQ0 to something that is not VIRQ0.

IRQ0 is invalid for everything that is outside of arch/*.

http://lkml.org/lkml/2005/11/22/159
http://lkml.org/lkml/2005/11/22/213
http://lkml.org/lkml/2005/11/22/227

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

Hello,

[Note the email address used for David Vrabel isn't valid any more,
this mail uses his last used address.]

On Wed, Dec 16, 2009 at 07:32:29PM +0300, Anton Vorontsov wrote:
> On Wed, Dec 16, 2009 at 05:10:08PM +0100, Uwe Kleine-König wrote:
> > platform_get_irq returns -ENXIO on failure, so !irq was probably
> > always true.  Better use (int)irq <= 0.  Note that a return value of
> > zero is still handled as error even though this could mean irq0.
> > 
> > This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> > changed the return value of platform_get_irq from 0 to -ENXIO on error.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> 
> Noooooo... :-(
> 
> Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
> and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
> 
> IRQ0 is invalid for everything that is outside of arch/*.
> 
> http://lkml.org/lkml/2005/11/22/159
> http://lkml.org/lkml/2005/11/22/213
> http://lkml.org/lkml/2005/11/22/227
First note that my check is safe with both variants (e.g. it does the
right thing independent of the error being signaled by 0 or
-ESOMETHING.)

Then arch/arm/mach-pxa/devices.c has:

static struct resource pxa27x_resource_ssp3[] = {
	...
	[1] = {
		.start  = IRQ_SSP3,
		.end    = IRQ_SSP3,
		.flags  = IORESOURCE_IRQ,
	},
	...
}

with IRQ_SSP3 being zero (sometimes).  The driver is implemented in
arch/arm/mach-pxa/ssp.c and uses platform_get_irq.  So according to your
definition it's allowed (arch/* only).  Still this would break if you
revert 305b3228f9.

Actually I don't care much, but as platform_get_irq returns an int I
think it's fine for it to signal an error using a value < 0 as irqs are
not negative.

My position regarding irq0 is:  If a variable holds either a valid irq
or a value indicating "no irq", then feel free to use 0 as "no irq" and
a value > 0 for a valid irq (without offset).  If you want irq0 here,
you're out of luck.  But if you have a variable holding a valid irq only
(that is, there is no doubt if the value is valid or not) I see no
reason to dogmatically prohibit irq0.

I'm a bit annoyed as this is the third time[1] this month this irq0
discussion pops up for me.  I think people see that irq0 is involved
somehow, start wailing and stop seeing the issues being fixed.

Best regards
Uwe

[1] one is:
	http://thread.gmane.org/gmane.linux.kernel/924739
    the other wasn't on lkml, only mm-commits.  Cannot find it on the
    net now.
-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Dec 16, 2009 at 06:49:04PM +0100, Uwe Kleine-König wrote:
[...]
> > Noooooo... :-(
> > 
> > Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
> > and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
> > 
> > IRQ0 is invalid for everything that is outside of arch/*.
> > 
> > http://lkml.org/lkml/2005/11/22/159
> > http://lkml.org/lkml/2005/11/22/213
> > http://lkml.org/lkml/2005/11/22/227
> First note that my check is safe with both variants (e.g. it does the
> right thing independent of the error being signaled by 0 or
> -ESOMETHING.)
> 
> Then arch/arm/mach-pxa/devices.c has:
> 
> static struct resource pxa27x_resource_ssp3[] = {
> 	...
> 	[1] = {
> 		.start  = IRQ_SSP3,
> 		.end    = IRQ_SSP3,
> 		.flags  = IORESOURCE_IRQ,
> 	},
> 	...
> }
> 
> with IRQ_SSP3 being zero (sometimes).  The driver is implemented in
> arch/arm/mach-pxa/ssp.c and uses platform_get_irq.

So fix this *one* driver? Implement arm-specific platform_get_irq() as
a band-aid. Or better, implement virtual irqs <-> hardware irqs mapping
for ARM.

[...]
> I'm a bit annoyed as this is the third time[1] this month this irq0
> discussion pops up for me.  I think people see that irq0 is involved
> somehow, start wailing and stop seeing the issues being fixed.

For this particular driver, there is NO issue whatsoever. It is
only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
that there still could be HWIRQ0 on PowerPC, but it is *never*
mapped to VIRQ0.

[...]
> [1] one is:
> 	http://thread.gmane.org/gmane.linux.kernel/924739

No wonder the discussion popped up. You're adding some ugly
#ifdef stuff that adds some arch-specific knowledge to a generic
code.

Sure, there's a lot of ugly code even in the kernel/ directly, but
you have to prepare for resistance when you add more of it.

So, if you want to fix the root cause of the issue: revert the
305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0, and try to improve the
ARM land, do not band-aid the whole kernel all over the place.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

Hi Anton,

On Wed, Dec 16, 2009 at 09:20:34PM +0300, Anton Vorontsov wrote:
> On Wed, Dec 16, 2009 at 06:49:04PM +0100, Uwe Kleine-König wrote:
> [...]
> > > Noooooo... :-(
> > > 
> > > Please revert 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 instead,
> > > and fix platforms to remap HWIRQ0 to something that is not VIRQ0.
> > > 
> > > IRQ0 is invalid for everything that is outside of arch/*.
> > > 
> > > http://lkml.org/lkml/2005/11/22/159
> > > http://lkml.org/lkml/2005/11/22/213
> > > http://lkml.org/lkml/2005/11/22/227
> > First note that my check is safe with both variants (e.g. it does the
> > right thing independent of the error being signaled by 0 or
> > -ESOMETHING.)
> > 
> > Then arch/arm/mach-pxa/devices.c has:
> > 
> > static struct resource pxa27x_resource_ssp3[] = {
> > 	...
> > 	[1] = {
> > 		.start  = IRQ_SSP3,
> > 		.end    = IRQ_SSP3,
> > 		.flags  = IORESOURCE_IRQ,
> > 	},
> > 	...
> > }
> > 
> > with IRQ_SSP3 being zero (sometimes).  The driver is implemented in
> > arch/arm/mach-pxa/ssp.c and uses platform_get_irq.
> 
> So fix this *one* driver? Implement arm-specific platform_get_irq() as
> a band-aid. Or better, implement virtual irqs <-> hardware irqs mapping
> for ARM.
> 
> [...]
> > I'm a bit annoyed as this is the third time[1] this month this irq0
> > discussion pops up for me.  I think people see that irq0 is involved
> > somehow, start wailing and stop seeing the issues being fixed.
> 
> For this particular driver, there is NO issue whatsoever. It is
> only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
> that there still could be HWIRQ0 on PowerPC, but it is *never*
> mapped to VIRQ0.
Yes, there is an issue.  If the platform device doesn't have a resource
specifing the irq, platform_get_irq returns -ENXIO.  So in the driver
(unsigned)(-ENXIO) is passed to mpc8xxx_spi_probe as (!(-ENXIO)) is
false and so the error isn't catched.

> [...]
> > [1] one is:
> > 	http://thread.gmane.org/gmane.linux.kernel/924739
> 
> No wonder the discussion popped up. You're adding some ugly
> #ifdef stuff that adds some arch-specific knowledge to a generic
> code.
I wouldn't argue if people objected to the arch-specific #ifdef.  The
arch-specific code is already in there and I don't object to just
removing it.  But answering that irq0 must not be used isn't helpful.
 
Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Dec 16, 2009 at 08:18:39PM +0100, Uwe Kleine-König wrote:
[...]
> > > I'm a bit annoyed as this is the third time[1] this month this irq0
> > > discussion pops up for me.  I think people see that irq0 is involved
> > > somehow, start wailing and stop seeing the issues being fixed.
> > 
> > For this particular driver, there is NO issue whatsoever. It is
> > only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
> > that there still could be HWIRQ0 on PowerPC, but it is *never*
> > mapped to VIRQ0.
> Yes, there is an issue.  If the platform device doesn't have a resource
> specifing the irq, platform_get_irq returns -ENXIO.  So in the driver
> (unsigned)(-ENXIO) is passed to mpc8xxx_spi_probe as (!(-ENXIO)) is
> false and so the error isn't catched.

If you look into how we create the platform device, you'll notice
that -ENXIO isn't possible.
It's in arch/powerpc/platforms/83xx/mpc832x_rdb.c:of_fsl_spi_probe(),
which is a legacy interface that I'd like to be removed anyway.

Though, if you want to fix the inconsistency in the platform device
API, then I'd suggest to fix the platform_get_irq(). The driver itself
is correct.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Dec 16, 2009 at 10:37:45PM +0300, Anton Vorontsov wrote:
[...]
> which is a legacy interface that I'd like to be removed anyway.

...which means I really don't care that much to nack or ack the
patch. Do whatever you feel the best, but you must realize that
what you're doing is just papering over the real problem, and
maybe even spreading the problem further.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

Hi Anton,

On Wed, Dec 16, 2009 at 10:37:45PM +0300, Anton Vorontsov wrote:
> On Wed, Dec 16, 2009 at 08:18:39PM +0100, Uwe Kleine-König wrote:
> [...]
> > > > I'm a bit annoyed as this is the third time[1] this month this irq0
> > > > discussion pops up for me.  I think people see that irq0 is involved
> > > > somehow, start wailing and stop seeing the issues being fixed.
> > > 
> > > For this particular driver, there is NO issue whatsoever. It is
> > > only used for PowerPC, which has VIRQ0 == invalid IRQ. And note
> > > that there still could be HWIRQ0 on PowerPC, but it is *never*
> > > mapped to VIRQ0.
> > Yes, there is an issue.  If the platform device doesn't have a resource
> > specifing the irq, platform_get_irq returns -ENXIO.  So in the driver
> > (unsigned)(-ENXIO) is passed to mpc8xxx_spi_probe as (!(-ENXIO)) is
> > false and so the error isn't catched.
> 
> If you look into how we create the platform device, you'll notice
> that -ENXIO isn't possible.
> It's in arch/powerpc/platforms/83xx/mpc832x_rdb.c:of_fsl_spi_probe(),
> which is a legacy interface that I'd like to be removed anyway.
> 
> Though, if you want to fix the inconsistency in the platform device
> API, then I'd suggest to fix the platform_get_irq(). The driver itself
> is correct.
With platform_get_irq as it is today, the check in

	irq = platform_get_irq(pdev, 0);
	if (!irq)
		return -EINVAL;

is non-sense as !irq always evaluates to 0.  If your argue that the
resources are right, then the logical consequence is to strip down
plat_mpc8xxx_spi_probe to just

	struct spi_master *master;
	master = mpc8xxx_spi_probe(&pdev->dev,
		platform_get_resource(pdev, IORESOURCE_MEM, 0)
		platform_get_irq(pdev, 0));
	if (IS_ERR(master))
		return PTR_ERR(master);
	return 0;

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Thu, Dec 17, 2009 at 02:05:49PM +0100, Uwe Kleine-König wrote:
[...]
> > > Yes, there is an issue.  If the platform device doesn't have a resource
> > > specifing the irq, platform_get_irq returns -ENXIO.  So in the driver
> > > (unsigned)(-ENXIO) is passed to mpc8xxx_spi_probe as (!(-ENXIO)) is
> > > false and so the error isn't catched.
> > 
> > If you look into how we create the platform device, you'll notice
> > that -ENXIO isn't possible.
> > It's in arch/powerpc/platforms/83xx/mpc832x_rdb.c:of_fsl_spi_probe(),
> > which is a legacy interface that I'd like to be removed anyway.
> > 
> > Though, if you want to fix the inconsistency in the platform device
> > API, then I'd suggest to fix the platform_get_irq(). The driver itself
> > is correct.
> With platform_get_irq as it is today, the check in
> 
> 	irq = platform_get_irq(pdev, 0);
> 	if (!irq)
> 		return -EINVAL;

And I see no problem with this piece of code, really. I see the
problem in platform_get_irq() implementation (not that easy to fix,
see below).

> is non-sense as !irq always evaluates to 0.  If your argue that the
> resources are right,

No, I'm arguing that there is no issue. There *could* be an issue in
the future (if someone break the platform code), but the way you're
trying to fix the *possibility* isn't quite correct.

Believe me, I'd have no problem with this particular patch if the
patch could possibly fix any real world issue with this driver.

For example, you may notice the following commit:

commit 2fac6674ddf3164da42a76d62f8912073d629a30
Author: Anton Vorontsov <avorontsov@ru.mvista.com>
Date:   Tue Jan 6 14:42:11 2009 -0800

    rtc: bunch of drivers: fix 'no irq' case handing

    This patch fixes a bunch of irq checking misuses.  Most drivers were
    getting irq via platform_get_irq(), which returns -ENXIO or r->start.

Sounds similar, eh? But you may notice that the unfixed code
was really wrong and didn't work for every sane platform:

        m48t59->irq = platform_get_irq(pdev, 0);
-       if (m48t59->irq < 0)
+       if (m48t59->irq <= 0)

The checks were irq < 0, so the drivers were requesting irq0, and
then were failing to probe.

Unfortunately, I stopped half way, afraid to possibly break current
platforms that use these drivers, so I kept the "<" part. Which is
a shame, because... um, it seems I spread the bad example further.

Anyway, I just did 'grep platform_get_irq -A 2 -r drivers/', and
it looks horrible, most callers check for irq < 0, which means
the code won't work on anything but arches with NO_IRQ == -1
(ARM, parisc, xtensa).

It seems the situation is near to hopeless. Maybe someone needs to
sit down and cleanup this mess once and for ever...

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[David Vrabel]

Uwe Kleine-König wrote:
> Hello,
> 
> [Note the email address used for David Vrabel isn't valid any more,
> this mail uses his last used address.]

I've not been involved in PXA27x stuff for years.  Please drop me from 
the CC, thanks.

David

Re: [PATCH 6/7] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Dec 16, 2009 at 05:10:08PM +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !irq was probably
> always true.  Better use (int)irq <= 0.  Note that a return value of
> zero is still handled as error even though this could mean irq0.
> 
> This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> changed the return value of platform_get_irq from 0 to -ENXIO on error.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: David Vrabel <dvrabel@arcom.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: spi-devel-general@lists.sourceforge.net
> ---
>  drivers/spi/spi_mpc8xxx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> index e9390d7..b13501a 100644
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -1339,7 +1339,7 @@ static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (!irq)
> +	if ((int)irq <= 0)

I tend to think that it's really hopeless to fix the
platform_get_irq() in its current form, so can you get rid
of this ugly cast and just make the irq signed?

And I'll be fine with it. :-(

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

[PATCH] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

platform_get_irq returns -ENXIO on failure, so !irq was probably
always true.  Make irq a signed variable and compare irq <= 0.  Note
that a return value of zero is still handled as error even though this
could mean irq0.

This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
changed the return value of platform_get_irq from 0 to -ENXIO on error.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: David Vrabel <dvrabel@arcom.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: spi-devel-general@lists.sourceforge.net
---
Hello,

as requested by Anton Vorontsov I made irq signed instead of casting to
int when comparing to zero.

Best regards
Uwe

 drivers/spi/spi_mpc8xxx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 1fb2a6e..08065fb 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -1328,7 +1328,7 @@ static struct of_platform_driver of_mpc8xxx_spi_driver = {
 static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
 {
 	struct resource *mem;
-	unsigned int irq;
+	int irq;
 	struct spi_master *master;
 
 	if (!pdev->dev.platform_data)
@@ -1339,7 +1339,7 @@ static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	irq = platform_get_irq(pdev, 0);
-	if (!irq)
+	if (irq <= 0)
 		return -EINVAL;
 
 	master = mpc8xxx_spi_probe(&pdev->dev, mem, irq);
-- 
1.6.5.2

[RESEND PATCH 5/5] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Uwe Kleine-König]

platform_get_irq returns -ENXIO on failure, so !irq was probably
always true.  Make irq a signed variable and compare irq <= 0.  Note
that a return value of zero is still handled as error even though this
could mean irq0.

This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
changed the return value of platform_get_irq from 0 to -ENXIO on error.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: David Vrabel <dvrabel@arcom.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: spi-devel-general@lists.sourceforge.net
---
 drivers/spi/spi_mpc8xxx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index 1fb2a6e..08065fb 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -1328,7 +1328,7 @@ static struct of_platform_driver of_mpc8xxx_spi_driver = {
 static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
 {
 	struct resource *mem;
-	unsigned int irq;
+	int irq;
 	struct spi_master *master;
 
 	if (!pdev->dev.platform_data)
@@ -1339,7 +1339,7 @@ static int __devinit plat_mpc8xxx_spi_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	irq = platform_get_irq(pdev, 0);
-	if (!irq)
+	if (irq <= 0)
 		return -EINVAL;
 
 	master = mpc8xxx_spi_probe(&pdev->dev, mem, irq);
-- 
1.6.6

Re: [RESEND PATCH 5/5] spi/mpc8xxx: don't check platform_get_irq's return value against zero

[Anton Vorontsov]

On Wed, Jan 13, 2010 at 12:05:46PM +0100, Uwe Kleine-König wrote:
> platform_get_irq returns -ENXIO on failure, so !irq was probably
> always true.  Make irq a signed variable and compare irq <= 0.  Note
> that a return value of zero is still handled as error even though this
> could mean irq0.
> 
> This is a followup to 305b3228f9ff4d59f49e6d34a7034d44ee8ce2f0 that
> changed the return value of platform_get_irq from 0 to -ENXIO on error.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: David Vrabel <dvrabel@arcom.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: David Brownell <dbrownell@users.sourceforge.net>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Anton Vorontsov <avorontsov@ru.mvista.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: spi-devel-general@lists.sourceforge.net
> ---

Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Thanks!