[PATCH 1/2] spi_imx.c: CSPI3 irq is 0 on imx25

[PATCH 1/2] spi_imx.c: CSPI3 irq is 0 on imx25

[Richard Genoud]

On imx25 soc, MX25_INT_CSPI3 is 0
(cf arch/arm/plat-mxc/include/mach/mx25.h).
So, the test (spi_imx->irq <= 0) returned an error
for this platform.
This patch corrects this behaviour.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/spi/spi_imx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
index 55a38e2..793ae99 100644
--- a/drivers/spi/spi_imx.c
+++ b/drivers/spi/spi_imx.c
@@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
 	}
 
 	spi_imx->irq = platform_get_irq(pdev, 0);
-	if (spi_imx->irq <= 0) {
+	if (spi_imx->irq < 0) {
 		ret = -EINVAL;
 		goto out_iounmap;
 	}
-- 
1.7.1

[PATCH 2/2] spi_imx.c: TRIVIAL mx25 & mx35 SPI_VER is 0.7

[Richard Genoud]

The check on cpu_is_mx25 and cpu_is_mx35 was made twice.
This is obviously wrong.
Anyway, this patch won't change the previous behaviour which is
SPI_IMX_VER_0_7 for mx25 and mx35.

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/spi/spi_imx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
index 793ae99..96be9d1 100644
--- a/drivers/spi/spi_imx.c
+++ b/drivers/spi/spi_imx.c
@@ -806,7 +806,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
 		if (cpu_is_mx25() || cpu_is_mx35())
 			spi_imx->devtype_data =
 				spi_imx_devtype_data[SPI_IMX_VER_0_7];
-		else if (cpu_is_mx25() || cpu_is_mx31() || cpu_is_mx35())
+		else if (cpu_is_mx31())
 			spi_imx->devtype_data =
 				spi_imx_devtype_data[SPI_IMX_VER_0_4];
 		else if (cpu_is_mx27() || cpu_is_mx21())
-- 
1.7.1

Re: [PATCH 2/2] spi_imx.c: TRIVIAL mx25 & mx35 SPI_VER is 0.7

[Uwe Kleine-König]

On Thu, Dec 23, 2010 at 12:12:10PM +0100, Richard Genoud wrote:
> The check on cpu_is_mx25 and cpu_is_mx35 was made twice.
> This is obviously wrong.
obviously wrong but no wrong behaviour.  Your patch only makes the code
consistent, but IMHO it doesn't qualify as a bugfix.

> Anyway, this patch won't change the previous behaviour which is
> SPI_IMX_VER_0_7 for mx25 and mx35.
This conflicts with
http://mid.gmane.org/1290589546-21662-1-git-send-email-u.kleine-koenig@pengutronix.de

which I prefer.  The platform IDs have it correct (i.e.

	imx25-cspi -> 0.7
	imx31-cspi -> 0.4
	imx35-cspi -> 0.7

)

Best regards
Uwe

> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/spi/spi_imx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
> index 793ae99..96be9d1 100644
> --- a/drivers/spi/spi_imx.c
> +++ b/drivers/spi/spi_imx.c
> @@ -806,7 +806,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>  		if (cpu_is_mx25() || cpu_is_mx35())
>  			spi_imx->devtype_data =
>  				spi_imx_devtype_data[SPI_IMX_VER_0_7];
> -		else if (cpu_is_mx25() || cpu_is_mx31() || cpu_is_mx35())
> +		else if (cpu_is_mx31())
>  			spi_imx->devtype_data =
>  				spi_imx_devtype_data[SPI_IMX_VER_0_4];
>  		else if (cpu_is_mx27() || cpu_is_mx21())
> -- 
> 1.7.1
> 
> 

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

Re: [PATCH 2/2] spi_imx.c: TRIVIAL mx25 & mx35 SPI_VER is 0.7

[Richard Genoud]

2010/12/23 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Thu, Dec 23, 2010 at 12:12:10PM +0100, Richard Genoud wrote:
>> The check on cpu_is_mx25 and cpu_is_mx35 was made twice.
>> This is obviously wrong.
> obviously wrong but no wrong behaviour.  Your patch only makes the code
> consistent, but IMHO it doesn't qualify as a bugfix.
yes, that's what I said, this patch doesn't change the behaviour, it's
only janitoring (I could have CC the list by the way).

If the whole SPI_IMX_VER_AUTODETECT system is going to be trashed,
sure, my patch is useless...

>
>> Anyway, this patch won't change the previous behaviour which is
>> SPI_IMX_VER_0_7 for mx25 and mx35.
> This conflicts with
> http://mid.gmane.org/1290589546-21662-1-git-send-email-u.kleine-koenig@pengutronix.de
>
> which I prefer.  The platform IDs have it correct (i.e.
>
>        imx25-cspi -> 0.7
>        imx31-cspi -> 0.4
>        imx35-cspi -> 0.7
>
> )
>
> Best regards
> Uwe
>
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/spi/spi_imx.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
>> index 793ae99..96be9d1 100644
>> --- a/drivers/spi/spi_imx.c
>> +++ b/drivers/spi/spi_imx.c
>> @@ -806,7 +806,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>>               if (cpu_is_mx25() || cpu_is_mx35())
>>                       spi_imx->devtype_data =
>>                               spi_imx_devtype_data[SPI_IMX_VER_0_7];
>> -             else if (cpu_is_mx25() || cpu_is_mx31() || cpu_is_mx35())
>> +             else if (cpu_is_mx31())
>>                       spi_imx->devtype_data =
>>                               spi_imx_devtype_data[SPI_IMX_VER_0_4];
>>               else if (cpu_is_mx27() || cpu_is_mx21())
>> --
>> 1.7.1
>>

Richard.

Re: [PATCH 1/2] spi_imx.c: CSPI3 irq is 0 on imx25

[Grant Likely]

On Thu, Dec 23, 2010 at 12:12:09PM +0100, Richard Genoud wrote:
> On imx25 soc, MX25_INT_CSPI3 is 0
> (cf arch/arm/plat-mxc/include/mach/mx25.h).
> So, the test (spi_imx->irq <= 0) returned an error
> for this platform.
> This patch corrects this behaviour.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
> ---
>  drivers/spi/spi_imx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
> index 55a38e2..793ae99 100644
> --- a/drivers/spi/spi_imx.c
> +++ b/drivers/spi/spi_imx.c
> @@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>  	}
>  
>  	spi_imx->irq = platform_get_irq(pdev, 0);
> -	if (spi_imx->irq <= 0) {
> +	if (spi_imx->irq < 0) {

I *really* don't want to apply this.  The kernel is slowly moving
toward NO_IRQ == 0 for all architectures.  This patch is a step in the
wrong direction because it will break with ARM starts using 0 for
NO_IRQ.

I would accept a change to spi_imx->irq == NO_IRQ however.

g.

Re: [PATCH 2/2] spi_imx.c: TRIVIAL mx25 & mx35 SPI_VER is 0.7

[Grant Likely]

On Thu, Dec 23, 2010 at 12:12:10PM +0100, Richard Genoud wrote:
> The check on cpu_is_mx25 and cpu_is_mx35 was made twice.
> This is obviously wrong.
> Anyway, this patch won't change the previous behaviour which is
> SPI_IMX_VER_0_7 for mx25 and mx35.
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

applied, thanks.

g.

> ---
>  drivers/spi/spi_imx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
> index 793ae99..96be9d1 100644
> --- a/drivers/spi/spi_imx.c
> +++ b/drivers/spi/spi_imx.c
> @@ -806,7 +806,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>  		if (cpu_is_mx25() || cpu_is_mx35())
>  			spi_imx->devtype_data =
>  				spi_imx_devtype_data[SPI_IMX_VER_0_7];
> -		else if (cpu_is_mx25() || cpu_is_mx31() || cpu_is_mx35())
> +		else if (cpu_is_mx31())
>  			spi_imx->devtype_data =
>  				spi_imx_devtype_data[SPI_IMX_VER_0_4];
>  		else if (cpu_is_mx27() || cpu_is_mx21())
> -- 
> 1.7.1
> 

Re: [PATCH 2/2] spi_imx.c: TRIVIAL mx25 & mx35 SPI_VER is 0.7

[Grant Likely]

On Thu, Dec 23, 2010 at 03:29:34PM +0100, Uwe Kleine-König wrote:
> On Thu, Dec 23, 2010 at 12:12:10PM +0100, Richard Genoud wrote:
> > The check on cpu_is_mx25 and cpu_is_mx35 was made twice.
> > This is obviously wrong.
> obviously wrong but no wrong behaviour.  Your patch only makes the code
> consistent, but IMHO it doesn't qualify as a bugfix.
> 
> > Anyway, this patch won't change the previous behaviour which is
> > SPI_IMX_VER_0_7 for mx25 and mx35.
> This conflicts with
> http://mid.gmane.org/1290589546-21662-1-git-send-email-u.kleine-koenig@pengutronix.de
> 
> which I prefer.  The platform IDs have it correct (i.e.
> 
> 	imx25-cspi -> 0.7
> 	imx31-cspi -> 0.4
> 	imx35-cspi -> 0.7
> 
> )

Oops, missed you're reply.  Okay, I've dropped Richard's patch and will look at yours.


g.

Re: [PATCH 1/2] spi_imx.c: CSPI3 irq is 0 on imx25

[Richard Genoud]

2010/12/23 Grant Likely <grant.likely@secretlab.ca>:
> On Thu, Dec 23, 2010 at 12:12:09PM +0100, Richard Genoud wrote:
>> On imx25 soc, MX25_INT_CSPI3 is 0
>> (cf arch/arm/plat-mxc/include/mach/mx25.h).
>> So, the test (spi_imx->irq <= 0) returned an error
>> for this platform.
>> This patch corrects this behaviour.
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>> ---
>>  drivers/spi/spi_imx.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
>> index 55a38e2..793ae99 100644
>> --- a/drivers/spi/spi_imx.c
>> +++ b/drivers/spi/spi_imx.c
>> @@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>>       }
>>
>>       spi_imx->irq = platform_get_irq(pdev, 0);
>> -     if (spi_imx->irq <= 0) {
>> +     if (spi_imx->irq < 0) {
>
> I *really* don't want to apply this.  The kernel is slowly moving
> toward NO_IRQ == 0 for all architectures.  This patch is a step in the
> wrong direction because it will break with ARM starts using 0 for
> NO_IRQ.
>
> I would accept a change to spi_imx->irq == NO_IRQ however.
>
> g.
>

Actually, the description of the patch is wrong, but not the patch itself.
The test "if (spi_imx->irq <= 0)" is not testing the IRQ value, but
the return value of platform_get_irq().
And platform_get_irq() can return an error (-ENXIO) or the IRQ value it found.
So, as we are testing if platform_get_irq() failed or not, the test
should be "if (spi_imx->irq < 0)"

If you agree with that, I will resend the patch with a good description.

richard.


-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

[PATCH] spi_imx.c: correct the test on platform_get_irq() return value

[Richard Genoud]

The test "if (spi_imx->irq <= 0)" is not testing the IRQ value, but the return value of platform_get_irq().
As platform_get_irq() can return an error (-ENXIO) or the IRQ value it found, the test should be "if (spi_imx->irq < 0)"

Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/spi/spi_imx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
index 55a38e2..793ae99 100644
--- a/drivers/spi/spi_imx.c
+++ b/drivers/spi/spi_imx.c
@@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
 	}
 
 	spi_imx->irq = platform_get_irq(pdev, 0);
-	if (spi_imx->irq <= 0) {
+	if (spi_imx->irq < 0) {
 		ret = -EINVAL;
 		goto out_iounmap;
 	}
-- 
1.7.1

Re: [PATCH] spi_imx.c: correct the test on platform_get_irq() return value

[Grant Likely]

On Fri, Jan 7, 2011 at 7:26 AM, Richard Genoud <richard.genoud@gmail.com> wrote:
> The test "if (spi_imx->irq <= 0)" is not testing the IRQ value, but the return value of platform_get_irq().
> As platform_get_irq() can return an error (-ENXIO) or the IRQ value it found, the test should be "if (spi_imx->irq < 0)"
>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

For the majority of architectures in Linux, 0 is not a valid IRQ for
devices.  Unless there is an actual imx platform that currently
assigns linux irq 0 to the spi controller then I'd rather not apply
this change.

g.

> ---
>  drivers/spi/spi_imx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
> index 55a38e2..793ae99 100644
> --- a/drivers/spi/spi_imx.c
> +++ b/drivers/spi/spi_imx.c
> @@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>        }
>
>        spi_imx->irq = platform_get_irq(pdev, 0);
> -       if (spi_imx->irq <= 0) {
> +       if (spi_imx->irq < 0) {
>                ret = -EINVAL;
>                goto out_iounmap;
>        }
> --
> 1.7.1
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] spi_imx.c: correct the test on platform_get_irq() return value

[Richard Genoud]

2011/1/7 Grant Likely <grant.likely@secretlab.ca>:
> On Fri, Jan 7, 2011 at 7:26 AM, Richard Genoud <richard.genoud@gmail.com> wrote:
>> The test "if (spi_imx->irq <= 0)" is not testing the IRQ value, but the return value of platform_get_irq().
>> As platform_get_irq() can return an error (-ENXIO) or the IRQ value it found, the test should be "if (spi_imx->irq < 0)"
>>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>
> For the majority of architectures in Linux, 0 is not a valid IRQ for
> devices.  Unless there is an actual imx platform that currently
> assigns linux irq 0 to the spi controller then I'd rather not apply
> this change.
actually, there is the imx25 platform:
in arch/arm/plat-mxc/include/mach/mx25.h
line 61 :
#define MX25_INT_CSPI3          0
this is the interrupt assigned for CSPI3.

I doubled check with the imx25 release manual chapter 3.1 (page 51):
http://cache.freescale.com/files/dsp/doc/ref_manual/IMX25RM.pdf
And I made some spi test on the imx25 3stack PDK, the CSPI3 is working
with irq 0.

richard.

Re: [PATCH] spi_imx.c: correct the test on platform_get_irq() return value

[Grant Likely]

On Fri, Jan 07, 2011 at 03:26:01PM +0100, Richard Genoud wrote:
> The test "if (spi_imx->irq <= 0)" is not testing the IRQ value, but the return value of platform_get_irq().
> As platform_get_irq() can return an error (-ENXIO) or the IRQ value it found, the test should be "if (spi_imx->irq < 0)"
> 
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Alright.  Applied.

g.

> ---
>  drivers/spi/spi_imx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi_imx.c b/drivers/spi/spi_imx.c
> index 55a38e2..793ae99 100644
> --- a/drivers/spi/spi_imx.c
> +++ b/drivers/spi/spi_imx.c
> @@ -847,7 +847,7 @@ static int __devinit spi_imx_probe(struct platform_device *pdev)
>  	}
>  
>  	spi_imx->irq = platform_get_irq(pdev, 0);
> -	if (spi_imx->irq <= 0) {
> +	if (spi_imx->irq < 0) {
>  		ret = -EINVAL;
>  		goto out_iounmap;
>  	}
> -- 
> 1.7.1
>