| Message ID | 987ac156e9590cbe2f667f9782b56515d7a991a4.1511194239.git.arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 20, 2017 at 09:56:21PM +0530, Arvind Yadav wrote: > The platform_get_irq() function returns negative if an error occurs. > zero or positive number on success. platform_get_irq() error checking > for zero is not correct. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > changes in v2 : > Return keypad->irq insted of -ENXIO. > changes in v3 : > Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid. > changes ib v4 : > Return -ENXIO insted of keypad->irq. Which was not correct in v3. > > drivers/input/keyboard/ep93xx_keypad.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index f77b295..c6eb31a 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c > @@ -257,7 +257,7 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > } > > keypad->irq = platform_get_irq(pdev, 0); > - if (!keypad->irq) { > + if (keypad->irq <= 0) { > err = -ENXIO; Still not right. err = keypad->irq < 0 ? keypad->irq : -ENXIO; would have the right effect. However, I disagree with the idea of making platform_get_irq() return an error if there is a zero irq specified as suggested in a previous review - zero means "not present" and that is not necessarily an error - it's up to the driver to decide whether it can proceed without an interrupt, or whether it should error out on the probe function.
On Mon, Nov 20, 2017 at 8:29 AM, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Mon, Nov 20, 2017 at 09:56:21PM +0530, Arvind Yadav wrote: >> The platform_get_irq() function returns negative if an error occurs. >> zero or positive number on success. platform_get_irq() error checking >> for zero is not correct. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> --- >> changes in v2 : >> Return keypad->irq insted of -ENXIO. >> changes in v3 : >> Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid. >> changes ib v4 : >> Return -ENXIO insted of keypad->irq. Which was not correct in v3. >> >> drivers/input/keyboard/ep93xx_keypad.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c >> index f77b295..c6eb31a 100644 >> --- a/drivers/input/keyboard/ep93xx_keypad.c >> +++ b/drivers/input/keyboard/ep93xx_keypad.c >> @@ -257,7 +257,7 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) >> } >> >> keypad->irq = platform_get_irq(pdev, 0); >> - if (!keypad->irq) { >> + if (keypad->irq <= 0) { >> err = -ENXIO; > > Still not right. > > err = keypad->irq < 0 ? keypad->irq : -ENXIO; > > would have the right effect. > > However, I disagree with the idea of making platform_get_irq() return > an error if there is a zero irq specified as suggested in a previous > review - zero means "not present" and that is not necessarily an error - > it's up to the driver to decide whether it can proceed without an > interrupt, or whether it should error out on the probe function. We used to return 0 from platform_get_irq() in early 2.6 days on errors but not anymore and I do not think keeping treating 0 there as special makes much sense. We already return -ENXIO for missing resources, and I do not think that anyone actually defines IRQ resource with 0 number and attaches it to devices in home that drivers would recognize it as missing interrupt and work without it. Also, in case of DT we do not recognize IRQ 0 as valid and try to fetch it from other sources (static board data), I think ACPI is similar. I think we should also return -ENXIO in case of res->start == 0 and leave it to the driver to check and decide if it wants to deal with missing interrupt. Most of them do not. Thanks.
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index f77b295..c6eb31a 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -257,7 +257,7 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) } keypad->irq = platform_get_irq(pdev, 0); - if (!keypad->irq) { + if (keypad->irq <= 0) { err = -ENXIO; goto failed_free; }
The platform_get_irq() function returns negative if an error occurs. zero or positive number on success. platform_get_irq() error checking for zero is not correct. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- changes in v2 : Return keypad->irq insted of -ENXIO. changes in v3 : Add failure case '<= 0' instead of '< 0'. IRQ0 is not valid. changes ib v4 : Return -ENXIO insted of keypad->irq. Which was not correct in v3. drivers/input/keyboard/ep93xx_keypad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)