* Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-01-21 20:07 [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero Jon Smirl
@ 2008-01-24 22:32 ` Jon Smirl
2008-01-24 22:35 ` Benjamin Herrenschmidt
2008-01-25 17:13 ` Jon Smirl
2008-02-19 16:42 ` Jean Delvare
2 siblings, 1 reply; 13+ messages in thread
From: Jon Smirl @ 2008-01-24 22:32 UTC (permalink / raw)
To: i2c, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-01-24 22:32 ` [i2c] " Jon Smirl
@ 2008-01-24 22:35 ` Benjamin Herrenschmidt
2008-01-24 23:07 ` Jon Smirl
0 siblings, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2008-01-24 22:35 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, i2c
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
> >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-01-24 22:35 ` Benjamin Herrenschmidt
@ 2008-01-24 23:07 ` Jon Smirl
0 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2008-01-24 23:07 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [i2c] [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-01-21 20:07 [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero Jon Smirl
2008-01-24 22:32 ` [i2c] " Jon Smirl
@ 2008-01-25 17:13 ` Jon Smirl
2008-02-19 16:42 ` Jean Delvare
2 siblings, 0 replies; 13+ messages in thread
From: Jon Smirl @ 2008-01-25 17:13 UTC (permalink / raw)
To: i2c, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-01-21 20:07 [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero Jon Smirl
2008-01-24 22:32 ` [i2c] " Jon Smirl
2008-01-25 17:13 ` Jon Smirl
@ 2008-02-19 16:42 ` Jean Delvare
2008-04-25 9:43 ` Jean Delvare
2008-05-02 14:23 ` Jon Smirl
2 siblings, 2 replies; 13+ messages in thread
From: Jean Delvare @ 2008-02-19 16:42 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-02-19 16:42 ` Jean Delvare
@ 2008-04-25 9:43 ` Jean Delvare
2008-05-02 14:23 ` Jon Smirl
1 sibling, 0 replies; 13+ messages in thread
From: Jean Delvare @ 2008-04-25 9:43 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-02-19 16:42 ` Jean Delvare
2008-04-25 9:43 ` Jean Delvare
@ 2008-05-02 14:23 ` Jon Smirl
2008-05-02 14:46 ` Jean Delvare
1 sibling, 1 reply; 13+ messages in thread
From: Jon Smirl @ 2008-05-02 14:23 UTC (permalink / raw)
To: Jean Delvare; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-05-02 14:23 ` Jon Smirl
@ 2008-05-02 14:46 ` Jean Delvare
2008-05-02 16:02 ` Jon Smirl
0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2008-05-02 14:46 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-05-02 14:46 ` Jean Delvare
@ 2008-05-02 16:02 ` Jon Smirl
2008-05-02 16:29 ` Jean Delvare
0 siblings, 1 reply; 13+ messages in thread
From: Jon Smirl @ 2008-05-02 16:02 UTC (permalink / raw)
To: Jean Delvare; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-05-02 16:02 ` Jon Smirl
@ 2008-05-02 16:29 ` Jean Delvare
2008-05-02 17:19 ` Jon Smirl
0 siblings, 1 reply; 13+ messages in thread
From: Jean Delvare @ 2008-05-02 16:29 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, i2c
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mpc i2c driver, compare to NO_IRQ instead of zero
2008-05-02 16:29 ` Jean Delvare
@ 2008-05-02 17:19 ` Jon Smirl
2008-05-02 20:27 ` Jean Delvare
0 siblings, 1 reply; 13+ messages in thread
From: Jon Smirl @ 2008-05-02 17:19 UTC (permalink / raw)
To: Jean Delvare; +Cc: linuxppc-dev, i2c
[-- 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);
^ permalink raw reply related [flat|nested] 13+ messages in thread