[PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Anton Vorontsov]

On a PowerPC board with ds1374 RTC I'm getting this error while
RTC tries to probe:

rtc-ds1374 0-0068: unable to request IRQ

This happens because I2C probing code (drivers/of/of_i2c.c) is
specifying IRQ0 for 'no irq' case, which is correct.

The driver handles this incorrectly, though. This patch fixes it.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/rtc/rtc-ds1374.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 640acd2..a150418 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -173,7 +173,7 @@ static int ds1374_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	int cr, sr;
 	int ret = 0;
 
-	if (client->irq < 0)
+	if (client->irq <= 0)
 		return -EINVAL;
 
 	mutex_lock(&ds1374->mutex);
@@ -212,7 +212,7 @@ static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	int cr;
 	int ret = 0;
 
-	if (client->irq < 0)
+	if (client->irq <= 0)
 		return -EINVAL;
 
 	ret = ds1374_read_time(dev, &now);
@@ -381,7 +381,7 @@ static int ds1374_probe(struct i2c_client *client,
 	if (ret)
 		goto out_free;
 
-	if (client->irq >= 0) {
+	if (client->irq > 0) {
 		ret = request_irq(client->irq, ds1374_irq, 0,
 		                  "ds1374", client);
 		if (ret) {
@@ -401,7 +401,7 @@ static int ds1374_probe(struct i2c_client *client,
 	return 0;
 
 out_irq:
-	if (client->irq >= 0)
+	if (client->irq > 0)
 		free_irq(client->irq, client);
 
 out_free:
@@ -414,7 +414,7 @@ static int __devexit ds1374_remove(struct i2c_client *client)
 {
 	struct ds1374 *ds1374 = i2c_get_clientdata(client);
 
-	if (client->irq >= 0) {
+	if (client->irq > 0) {
 		mutex_lock(&ds1374->mutex);
 		ds1374->exiting = 1;
 		mutex_unlock(&ds1374->mutex);
-- 
1.5.6.3

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Peter Korsgaard]

>>>>> "Anton" == Anton Vorontsov <avorontsov@ru.mvista.com> writes:

 Anton> On a PowerPC board with ds1374 RTC I'm getting this error while
 Anton> RTC tries to probe:

 Anton> rtc-ds1374 0-0068: unable to request IRQ

 Anton> This happens because I2C probing code (drivers/of/of_i2c.c) is
 Anton> specifying IRQ0 for 'no irq' case, which is correct.

 Anton> The driver handles this incorrectly, though. This patch fixes it.

Great, I was just about to send a similar patch. Another advantage of
using 0 for 'no irq' is for I2C_BOARD_INFO(). With that you can simply
not assign anything to .irq instead of having to set it to -1.

Acked-by: Peter Korsgaard <jacmet@sunsite.dk>

-- 
Bye, Peter Korsgaard

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Alan Cox]

> Shouldn't this be
> 
> >  -       if (client->irq <= NO_IRQ)
> 
> instead of

NO_IRQ is obsolete. client->irq != is the test and IRQ numbers are
unsigned.

Alan

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Jon Smirl]

On 8/12/08, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> On a PowerPC board with ds1374 RTC I'm getting this error while
>  RTC tries to probe:
>
>  rtc-ds1374 0-0068: unable to request IRQ
>
>  This happens because I2C probing code (drivers/of/of_i2c.c) is
>  specifying IRQ0 for 'no irq' case, which is correct.

Shouldn't this be

>  -       if (client->irq <= NO_IRQ)

instead of

>  -       if (client->irq < 0)
>  +       if (client->irq <= 0)

Since NO_IRQ can vary by platform (0 or -1)? Work is underway to get
everyone onto NO_IRQ=0 but I don't know if it is finished yet. It is
much cleaner to use the NO_IRQ define.

In of_i2c.c shouldn't there be an error check?

		info.irq = irq_of_parse_and_map(node, 0);

if (info.irq < NO_IRQ) {report error; continue }


>
>  The driver handles this incorrectly, though. This patch fixes it.
>
>  Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
>  ---
>   drivers/rtc/rtc-ds1374.c |   10 +++++-----
>   1 files changed, 5 insertions(+), 5 deletions(-)
>
>  diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
>  index 640acd2..a150418 100644
>  --- a/drivers/rtc/rtc-ds1374.c
>  +++ b/drivers/rtc/rtc-ds1374.c
>  @@ -173,7 +173,7 @@ static int ds1374_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>         int cr, sr;
>         int ret = 0;
>
>  -       if (client->irq < 0)
>  +       if (client->irq <= 0)
>                 return -EINVAL;
>
>         mutex_lock(&ds1374->mutex);
>  @@ -212,7 +212,7 @@ static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>         int cr;
>         int ret = 0;
>
>  -       if (client->irq < 0)
>  +       if (client->irq <= 0)
>                 return -EINVAL;
>
>         ret = ds1374_read_time(dev, &now);
>  @@ -381,7 +381,7 @@ static int ds1374_probe(struct i2c_client *client,
>         if (ret)
>                 goto out_free;
>
>  -       if (client->irq >= 0) {
>  +       if (client->irq > 0) {
>                 ret = request_irq(client->irq, ds1374_irq, 0,
>                                   "ds1374", client);
>                 if (ret) {
>  @@ -401,7 +401,7 @@ static int ds1374_probe(struct i2c_client *client,
>         return 0;
>
>   out_irq:
>  -       if (client->irq >= 0)
>  +       if (client->irq > 0)
>                 free_irq(client->irq, client);
>
>   out_free:
>  @@ -414,7 +414,7 @@ static int __devexit ds1374_remove(struct i2c_client *client)
>   {
>         struct ds1374 *ds1374 = i2c_get_clientdata(client);
>
>  -       if (client->irq >= 0) {
>  +       if (client->irq > 0) {
>                 mutex_lock(&ds1374->mutex);
>                 ds1374->exiting = 1;
>                 mutex_unlock(&ds1374->mutex);
>
> --
>  1.5.6.3
>
>  _______________________________________________
>  Linuxppc-dev mailing list
>  Linuxppc-dev@ozlabs.org
>  https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Anton Vorontsov]

On Tue, Aug 19, 2008 at 04:39:09PM -0400, Jon Smirl wrote:
> On 8/12/08, Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > On a PowerPC board with ds1374 RTC I'm getting this error while
> >  RTC tries to probe:
> >
> >  rtc-ds1374 0-0068: unable to request IRQ
> >
> >  This happens because I2C probing code (drivers/of/of_i2c.c) is
> >  specifying IRQ0 for 'no irq' case, which is correct.
> 
> Shouldn't this be
> 
> >  -       if (client->irq <= NO_IRQ)
> 
> instead of
> 
> >  -       if (client->irq < 0)
> >  +       if (client->irq <= 0)
> 
> Since NO_IRQ can vary by platform (0 or -1)?

First of all, NO_IRQ is not defined for every architecture. You can't
use it for truly cross-platform drivers.

Secondly, "<= 0" will work for both NO_IRQ == 0 and NO_IRQ == -1,
since client->irq is signed type.

As for false positives, I don't believe that there is any platform
that use IRQ0 for external interrupts.

[...]
> In of_i2c.c shouldn't there be an error check?
> 
> 		info.irq = irq_of_parse_and_map(node, 0);
> 
> if (info.irq < NO_IRQ) {report error; continue }

irq_of_parse_and_map() returns unsigned type, plus it is defined
only for PowerPC, and for PowerPC NO_IRQ is always 0.

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

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Jon Smirl]

On 8/19/08, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > Shouldn't this be
>  >
>  > >  -       if (client->irq <= NO_IRQ)
>  >
>  > instead of
>
>
> NO_IRQ is obsolete. client->irq != is the test and IRQ numbers are
>  unsigned.

I don't know the status of these platforms....

asm-blackfin/irq.h:#define NO_IRQ ((unsigned int)(-1))
asm-mn10300/irq.h:#define NO_IRQ		INT_MAX
asm-parisc/irq.h:#define NO_IRQ		(-1)

-- 
Jon Smirl
jonsmirl@gmail.com

Re: [PATCH 1/2] rtc: rtc-ds1374: fix 'no irq' case handling

[Alan Cox]

> I don't know the status of these platforms....
> 
> asm-blackfin/irq.h:#define NO_IRQ ((unsigned int)(-1))
> asm-mn10300/irq.h:#define NO_IRQ		INT_MAX
> asm-parisc/irq.h:#define NO_IRQ		(-1)

In need of fixing, assuming they actually use NO_IRQ for anything - don't
be mislead by the fact they may have old defines lying around.

Alan