From: Jon Smirl <hidden> Date: 2008-01-21 21:09:08
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 <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Jon Smirl <hidden> Date: 2008-01-24 22:32:09
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 [off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2008-01-24 22:36:40
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 [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Jon Smirl <hidden> Date: 2008-01-24 23:07:40
On 1/24/08, Benjamin Herrenschmidt [off-list ref] wrote:
On Thu, 2008-01-24 at 17:32 -0500, Jon Smirl wrote:
quoted
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.
quoted
On 1/21/08, Jon Smirl [off-list ref] wrote:
quoted
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 <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Jon Smirl <hidden> Date: 2008-01-26 00:35:38
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 [off-list ref] wrote:
quoted hunk
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 <redacted>
---
drivers/i2c/busses/i2c-mpc.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
From: Jean Delvare <hidden> Date: 2008-02-19 16:42:23
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.
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)u32x;intresult=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().
quoted hunk
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);
From: Jean Delvare <hidden> Date: 2008-04-25 09:43:30
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:
quoted
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.
@@ -99,7 +99,7 @@ static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)u32x;intresult=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().
quoted
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
From: Jon Smirl <hidden> Date: 2008-05-02 14:23:03
On 2/19/08, Jean Delvare [off-list ref] 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
From: Jean Delvare <hidden> Date: 2008-05-02 14:46:23
Hi Jon,
On Fri, 2 May 2008 10:23:01 -0400, Jon Smirl wrote:
On 2/19/08, Jean Delvare [off-list ref] wrote:
quoted
> 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.
quoted
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;
> }
@@ -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);
From: Jon Smirl <hidden> Date: 2008-05-02 17:19:47
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
From: Jean Delvare <hidden> Date: 2008-05-02 20:27:29
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.