[PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct.

Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
---

 drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index bbe787b..d20959d 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
 	u32 x;
 	int result = 0;
 
-	if (i2c->irq == 0)
+	if (i2c->irq == NO_IRQ)
 	{
 		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
 			schedule();
@@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
+	if (i2c->irq < NO_IRQ) {
 		result = -ENXIO;
 		goto fail_get_irq;
 	}
@@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		goto fail_map;
 	}
 
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
 					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
 			printk(KERN_ERR
@@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 	return result;
 
       fail_add:
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
       fail_irq:
 	iounmap(i2c->base);
@@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 	platform_set_drvdata(pdev, NULL);
 
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
 
 	iounmap(i2c->base);

Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

Ben, do you approve of this? How should error be checked for, is
<NO_IRQ right? The current code in the kernel looks to be broken
because of these checks, the ppc build is wrong and powerpc polled
mode doesn't work.

On 1/21/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct.
>
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
>
>  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index bbe787b..d20959d 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
>         u32 x;
>         int result = 0;
>
> -       if (i2c->irq == 0)
> +       if (i2c->irq == NO_IRQ)
>         {
>                 while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
>                         schedule();
> @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         i2c->irq = platform_get_irq(pdev, 0);
> -       if (i2c->irq < 0) {
> +       if (i2c->irq < NO_IRQ) {
>                 result = -ENXIO;
>                 goto fail_get_irq;
>         }
> @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>                 goto fail_map;
>         }
>
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>                                           IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>                         printk(KERN_ERR
> @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>         return result;
>
>        fail_add:
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 free_irq(i2c->irq, i2c);
>        fail_irq:
>         iounmap(i2c->base);
> @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>         i2c_del_adapter(&i2c->adap);
>         platform_set_drvdata(pdev, NULL);
>
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 free_irq(i2c->irq, i2c);
>
>         iounmap(i2c->base);
>
>
> _______________________________________________
> i2c mailing list
> i2c@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Benjamin Herrenschmidt]

On Thu, 2008-01-24 at 17:32 -0500, Jon Smirl wrote:
> Ben, do you approve of this? How should error be checked for, is
> <NO_IRQ right? The current code in the kernel looks to be broken
> because of these checks, the ppc build is wrong and powerpc polled
> mode doesn't work.

 == 0 should work on powerpc since NO_IRQ is defined to be 0 there no ?

Anyway, using the symbolic constant is always nicer I suppose.

Ben.

> On 1/21/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> > Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct.
> >
> > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> > ---
> >
> >  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> >
> >
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index bbe787b..d20959d 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> >         u32 x;
> >         int result = 0;
> >
> > -       if (i2c->irq == 0)
> > +       if (i2c->irq == NO_IRQ)
> >         {
> >                 while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
> >                         schedule();
> > @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >
> >         i2c->irq = platform_get_irq(pdev, 0);
> > -       if (i2c->irq < 0) {
> > +       if (i2c->irq < NO_IRQ) {
> >                 result = -ENXIO;
> >                 goto fail_get_irq;
> >         }
> > @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >                 goto fail_map;
> >         }
> >
> > -       if (i2c->irq != 0)
> > +       if (i2c->irq != NO_IRQ)
> >                 if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> >                                           IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> >                         printk(KERN_ERR
> > @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >         return result;
> >
> >        fail_add:
> > -       if (i2c->irq != 0)
> > +       if (i2c->irq != NO_IRQ)
> >                 free_irq(i2c->irq, i2c);
> >        fail_irq:
> >         iounmap(i2c->base);
> > @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
> >         i2c_del_adapter(&i2c->adap);
> >         platform_set_drvdata(pdev, NULL);
> >
> > -       if (i2c->irq != 0)
> > +       if (i2c->irq != NO_IRQ)
> >                 free_irq(i2c->irq, i2c);
> >
> >         iounmap(i2c->base);
> >
> >
> > _______________________________________________
> > i2c mailing list
> > i2c@lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/i2c
> >
> 
> 

Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

On 1/24/08, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> On Thu, 2008-01-24 at 17:32 -0500, Jon Smirl wrote:
> > Ben, do you approve of this? How should error be checked for, is
> > <NO_IRQ right? The current code in the kernel looks to be broken
> > because of these checks, the ppc build is wrong and powerpc polled
> > mode doesn't work.
>
>  == 0 should work on powerpc since NO_IRQ is defined to be 0 there no ?

The driver being patched is used in both the powerpc and ppc builds.

>
> Anyway, using the symbolic constant is always nicer I suppose.
>
> Ben.
>
> > On 1/21/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> > > Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct.
> > >
> > > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> > > ---
> > >
> > >  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
> > >  1 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > >
> > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > > index bbe787b..d20959d 100644
> > > --- a/drivers/i2c/busses/i2c-mpc.c
> > > +++ b/drivers/i2c/busses/i2c-mpc.c
> > > @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> > >         u32 x;
> > >         int result = 0;
> > >
> > > -       if (i2c->irq == 0)
> > > +       if (i2c->irq == NO_IRQ)
> > >         {
> > >                 while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
> > >                         schedule();
> > > @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> > >                 return -ENOMEM;
> > >
> > >         i2c->irq = platform_get_irq(pdev, 0);
> > > -       if (i2c->irq < 0) {
> > > +       if (i2c->irq < NO_IRQ) {
> > >                 result = -ENXIO;
> > >                 goto fail_get_irq;
> > >         }
> > > @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> > >                 goto fail_map;
> > >         }
> > >
> > > -       if (i2c->irq != 0)
> > > +       if (i2c->irq != NO_IRQ)
> > >                 if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> > >                                           IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> > >                         printk(KERN_ERR
> > > @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> > >         return result;
> > >
> > >        fail_add:
> > > -       if (i2c->irq != 0)
> > > +       if (i2c->irq != NO_IRQ)
> > >                 free_irq(i2c->irq, i2c);
> > >        fail_irq:
> > >         iounmap(i2c->base);
> > > @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
> > >         i2c_del_adapter(&i2c->adap);
> > >         platform_set_drvdata(pdev, NULL);
> > >
> > > -       if (i2c->irq != 0)
> > > +       if (i2c->irq != NO_IRQ)
> > >                 free_irq(i2c->irq, i2c);
> > >
> > >         iounmap(i2c->base);
> > >
> > >
> > > _______________________________________________
> > > i2c mailing list
> > > i2c@lm-sensors.org
> > > http://lists.lm-sensors.org/mailman/listinfo/i2c
> > >
> >
> >
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

Any final objections to this patch? When these were changed to 0
instead of NO_IRQ it should have broken polling mode on ppc. ppc would
treat polling mode, NO_IRQ=-1, as an error.

On powerpc this change is a NOP since NO_IRQ=0.

On 1/21/08, Jon Smirl <jonsmirl@gmail.com> wrote:
> Alter the mpc i2c driver to use the NO_IRQ symbol instead of the constant zero when checking for valid interrupts. NO_IRQ=-1 on ppc and NO_IRQ=0 on powerpc so the checks against zero are not correct.
>
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
>
>  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index bbe787b..d20959d 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
>         u32 x;
>         int result = 0;
>
> -       if (i2c->irq == 0)
> +       if (i2c->irq == NO_IRQ)
>         {
>                 while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
>                         schedule();
> @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>
>         i2c->irq = platform_get_irq(pdev, 0);
> -       if (i2c->irq < 0) {
> +       if (i2c->irq < NO_IRQ) {
>                 result = -ENXIO;
>                 goto fail_get_irq;
>         }
> @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>                 goto fail_map;
>         }
>
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>                                           IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>                         printk(KERN_ERR
> @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>         return result;
>
>        fail_add:
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 free_irq(i2c->irq, i2c);
>        fail_irq:
>         iounmap(i2c->base);
> @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>         i2c_del_adapter(&i2c->adap);
>         platform_set_drvdata(pdev, NULL);
>
> -       if (i2c->irq != 0)
> +       if (i2c->irq != NO_IRQ)
>                 free_irq(i2c->irq, i2c);
>
>         iounmap(i2c->base);
>
>
> _______________________________________________
> i2c mailing list
> i2c@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jean Delvare]

Hi Jon,

On Mon, 21 Jan 2008 15:07:40 -0500, Jon Smirl wrote:
> Alter the mpc i2c driver to use the NO_IRQ symbol instead of
> the constant zero when checking for valid interrupts. NO_IRQ=-1
> on ppc and NO_IRQ=0 on powerpc so the checks against zero are
> not correct.

Using NO_IRQ sounds good, just one question:

> 
> Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> ---
> 
>  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index bbe787b..d20959d 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
>  	u32 x;
>  	int result = 0;
>  
> -	if (i2c->irq == 0)
> +	if (i2c->irq == NO_IRQ)
>  	{
>  		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
>  			schedule();
> @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	i2c->irq = platform_get_irq(pdev, 0);
> -	if (i2c->irq < 0) {
> +	if (i2c->irq < NO_IRQ) {

I am skeptical about this one. Can platform_get_irq() really return
NO_IRQ? I thought that the IRQ resource would be plain missing if the
device has no IRQ, so I would expect:

 	i2c->irq = platform_get_irq(pdev, 0);
	if (i2c->irq < 0)
		i2c->irq = NO_IRQ; /* Use polling */

Testing against NO_IRQ suggests that devices with no IRQ would still
have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird
to me. Can you please clarify this point?

For what it's worth, no other kernel driver checks for irq < NO_IRQ.
They all check for irq < 0 after calling platform_get_irq().

>  		result = -ENXIO;
>  		goto fail_get_irq;
>  	}
> @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  		goto fail_map;
>  	}
>  
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>  					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>  			printk(KERN_ERR
> @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  	return result;
>  
>        fail_add:
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		free_irq(i2c->irq, i2c);
>        fail_irq:
>  	iounmap(i2c->base);
> @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>  	i2c_del_adapter(&i2c->adap);
>  	platform_set_drvdata(pdev, NULL);
>  
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		free_irq(i2c->irq, i2c);
>  
>  	iounmap(i2c->base);

The rest looks good.

-- 
Jean Delvare

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jean Delvare]

Hi Jon,

On Tue, 19 Feb 2008 17:42:21 +0100, Jean Delvare wrote:
> On Mon, 21 Jan 2008 15:07:40 -0500, Jon Smirl wrote:
> > Alter the mpc i2c driver to use the NO_IRQ symbol instead of
> > the constant zero when checking for valid interrupts. NO_IRQ=-1
> > on ppc and NO_IRQ=0 on powerpc so the checks against zero are
> > not correct.
> 
> Using NO_IRQ sounds good, just one question:
> 
> > 
> > Signed-off-by: Jon Smirl <jonsmirl@gmail.com>
> > ---
> > 
> >  drivers/i2c/busses/i2c-mpc.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index bbe787b..d20959d 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
> >  	u32 x;
> >  	int result = 0;
> >  
> > -	if (i2c->irq == 0)
> > +	if (i2c->irq == NO_IRQ)
> >  	{
> >  		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
> >  			schedule();
> > @@ -329,7 +329,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	i2c->irq = platform_get_irq(pdev, 0);
> > -	if (i2c->irq < 0) {
> > +	if (i2c->irq < NO_IRQ) {
> 
> I am skeptical about this one. Can platform_get_irq() really return
> NO_IRQ? I thought that the IRQ resource would be plain missing if the
> device has no IRQ, so I would expect:
> 
>  	i2c->irq = platform_get_irq(pdev, 0);
> 	if (i2c->irq < 0)
> 		i2c->irq = NO_IRQ; /* Use polling */
> 
> Testing against NO_IRQ suggests that devices with no IRQ would still
> have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird
> to me. Can you please clarify this point?
> 
> For what it's worth, no other kernel driver checks for irq < NO_IRQ.
> They all check for irq < 0 after calling platform_get_irq().
> 
> >  		result = -ENXIO;
> >  		goto fail_get_irq;
> >  	}
> > @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >  		goto fail_map;
> >  	}
> >  
> > -	if (i2c->irq != 0)
> > +	if (i2c->irq != NO_IRQ)
> >  		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> >  					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> >  			printk(KERN_ERR
> > @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
> >  	return result;
> >  
> >        fail_add:
> > -	if (i2c->irq != 0)
> > +	if (i2c->irq != NO_IRQ)
> >  		free_irq(i2c->irq, i2c);
> >        fail_irq:
> >  	iounmap(i2c->base);
> > @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
> >  	i2c_del_adapter(&i2c->adap);
> >  	platform_set_drvdata(pdev, NULL);
> >  
> > -	if (i2c->irq != 0)
> > +	if (i2c->irq != NO_IRQ)
> >  		free_irq(i2c->irq, i2c);
> >  
> >  	iounmap(i2c->base);
> 
> The rest looks good.

Any news about this patch? I had a question above which is left
unanswered. If you want this patch merged in 2.6.26 you'll have to be
quick.

-- 
Jean Delvare

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

On 2/19/08, Jean Delvare <khali@linux-fr.org> wrote:
>  >       i2c->irq = platform_get_irq(pdev, 0);
>  > -     if (i2c->irq < 0) {
>  > +     if (i2c->irq < NO_IRQ) {
>
>
> I am skeptical about this one. Can platform_get_irq() really return
>  NO_IRQ? I thought that the IRQ resource would be plain missing if the
>  device has no IRQ, so I would expect:
>
>
>         i2c->irq = platform_get_irq(pdev, 0);
>         if (i2c->irq < 0)
>
>                 i2c->irq = NO_IRQ; /* Use polling */
>
>  Testing against NO_IRQ suggests that devices with no IRQ would still
>  have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird
>  to me. Can you please clarify this point?

Your fix is correct. I'm not sure polling worked in the original driver.

>  For what it's worth, no other kernel driver checks for irq < NO_IRQ.
>  They all check for irq < 0 after calling platform_get_irq().
>
>
>  >               result = -ENXIO;
>  >               goto fail_get_irq;
>  >       }
>  > @@ -344,7 +344,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  >               goto fail_map;
>  >       }
>  >
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>  >                                         IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>  >                       printk(KERN_ERR
>  > @@ -367,7 +367,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  >       return result;
>  >
>  >        fail_add:
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               free_irq(i2c->irq, i2c);
>  >        fail_irq:
>  >       iounmap(i2c->base);
>  > @@ -384,7 +384,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>  >       i2c_del_adapter(&i2c->adap);
>  >       platform_set_drvdata(pdev, NULL);
>  >
>  > -     if (i2c->irq != 0)
>  > +     if (i2c->irq != NO_IRQ)
>  >               free_irq(i2c->irq, i2c);
>  >
>  >       iounmap(i2c->base);
>
>
> The rest looks good.
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jean Delvare]

Hi Jon,

On Fri, 2 May 2008 10:23:01 -0400, Jon Smirl wrote:
> On 2/19/08, Jean Delvare <khali@linux-fr.org> wrote:
> >  >       i2c->irq = platform_get_irq(pdev, 0);
> >  > -     if (i2c->irq < 0) {
> >  > +     if (i2c->irq < NO_IRQ) {
> >
> >
> > I am skeptical about this one. Can platform_get_irq() really return
> >  NO_IRQ? I thought that the IRQ resource would be plain missing if the
> >  device has no IRQ, so I would expect:
> >
> >
> >         i2c->irq = platform_get_irq(pdev, 0);
> >         if (i2c->irq < 0)
> >
> >                 i2c->irq = NO_IRQ; /* Use polling */
> >
> >  Testing against NO_IRQ suggests that devices with no IRQ would still
> >  have an IRQ resource defined and explicitly set to NO_IRQ. Sounds weird
> >  to me. Can you please clarify this point?
> 
> Your fix is correct. I'm not sure polling worked in the original driver.

OK, can you send an updated patch then?

Thanks.

> >  For what it's worth, no other kernel driver checks for irq < NO_IRQ.
> >  They all check for irq < 0 after calling platform_get_irq().
> >
> >
> >  >               result = -ENXIO;
> >  >               goto fail_get_irq;
> >  >       }

-- 
Jean Delvare

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

New version with your fix.

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index bbe787b..b141057 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned
timeout, int writing)
 	u32 x;
 	int result = 0;

-	if (i2c->irq == 0)
+	if (i2c->irq == NO_IRQ)
 	{
 		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
 			schedule();
@@ -329,10 +329,9 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;

 	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
-		result = -ENXIO;
-		goto fail_get_irq;
-	}
+	if (i2c->irq < 0)
+		i2c->irq = NO_IRQ; /* Use polling */
+
 	i2c->flags = pdata->device_flags;
 	init_waitqueue_head(&i2c->queue);

@@ -344,7 +343,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		goto fail_map;
 	}

-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
 					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
 			printk(KERN_ERR
@@ -367,7 +366,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 	return result;

       fail_add:
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
       fail_irq:
 	iounmap(i2c->base);
@@ -384,7 +383,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 	platform_set_drvdata(pdev, NULL);

-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);

 	iounmap(i2c->base);


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jean Delvare]

Hi Jon,

On Fri, 2 May 2008 12:02:27 -0400, Jon Smirl wrote:
> New version with your fix.
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index bbe787b..b141057 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned
> timeout, int writing)

Long lines folded, patch doesn't apply...

>  	u32 x;
>  	int result = 0;
> 
> -	if (i2c->irq == 0)
> +	if (i2c->irq == NO_IRQ)
>  	{
>  		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
>  			schedule();
> @@ -329,10 +329,9 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  		return -ENOMEM;
> 
>  	i2c->irq = platform_get_irq(pdev, 0);
> -	if (i2c->irq < 0) {
> -		result = -ENXIO;
> -		goto fail_get_irq;
> -	}
> +	if (i2c->irq < 0)
> +		i2c->irq = NO_IRQ; /* Use polling */
> +

After this change, label fail_get_irq is unused so you should remove
it. gcc should have told you, didn't it?

>  	i2c->flags = pdata->device_flags;
>  	init_waitqueue_head(&i2c->queue);
> 
> @@ -344,7 +343,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  		goto fail_map;
>  	}
> 
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
>  					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
>  			printk(KERN_ERR
> @@ -367,7 +366,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
>  	return result;
> 
>        fail_add:
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		free_irq(i2c->irq, i2c);
>        fail_irq:
>  	iounmap(i2c->base);
> @@ -384,7 +383,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
>  	i2c_del_adapter(&i2c->adap);
>  	platform_set_drvdata(pdev, NULL);
> 
> -	if (i2c->irq != 0)
> +	if (i2c->irq != NO_IRQ)
>  		free_irq(i2c->irq, i2c);
> 
>  	iounmap(i2c->base);
> 
> 


-- 
Jean Delvare

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jon Smirl]

[-- Attachment #1: Type: text/plain, Size: 237 bytes --]

I attached the diff file. I had forgot that I renamed the file so it
wasn't getting compiled. I compiled it this time. I've made too many
other changes to it to test this version on my current hardware.

-- 
Jon Smirl
jonsmirl@gmail.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: p2.diff --]
[-- Type: text/x-diff; name=p2.diff, Size: 1563 bytes --]

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index bbe787b..d73edef 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
 	u32 x;
 	int result = 0;
 
-	if (i2c->irq == 0)
+	if (i2c->irq == NO_IRQ)
 	{
 		while (!(readb(i2c->base + MPC_I2C_SR) & CSR_MIF)) {
 			schedule();
@@ -329,10 +329,9 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
-		result = -ENXIO;
-		goto fail_get_irq;
-	}
+	if (i2c->irq < 0)
+		i2c->irq = NO_IRQ; /* Use polling */
+
 	i2c->flags = pdata->device_flags;
 	init_waitqueue_head(&i2c->queue);
 
@@ -344,7 +343,7 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 		goto fail_map;
 	}
 
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
 					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
 			printk(KERN_ERR
@@ -367,12 +366,11 @@ static int fsl_i2c_probe(struct platform_device *pdev)
 	return result;
 
       fail_add:
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
       fail_irq:
 	iounmap(i2c->base);
       fail_map:
-      fail_get_irq:
 	kfree(i2c);
 	return result;
 };
@@ -384,7 +382,7 @@ static int fsl_i2c_remove(struct platform_device *pdev)
 	i2c_del_adapter(&i2c->adap);
 	platform_set_drvdata(pdev, NULL);
 
-	if (i2c->irq != 0)
+	if (i2c->irq != NO_IRQ)
 		free_irq(i2c->irq, i2c);
 
 	iounmap(i2c->base);

Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero

[Jean Delvare]

On Fri, 2 May 2008 13:19:44 -0400, Jon Smirl wrote:
> I attached the diff file. I had forgot that I renamed the file so it
> wasn't getting compiled. I compiled it this time. I've made too many
> other changes to it to test this version on my current hardware.

Applied, thanks.

-- 
Jean Delvare