| Message ID | 20230423032446.34347-1-lnk_01@hust.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | spi: davinci: Remove dead code in `davinci_spi_probe()` | expand |
On Sun, Apr 23, 2023 at 03:24:46AM +0000, Li Ningke wrote: > Smatch complains that > drivers/spi/spi-davinci.c:915 davinci_spi_probe() warn: > platform_get_irq() does not return zero > > There is no need to check whether the return value is zero as > `platform_get_irq()` only returns non-zero IRQ number on success > or negative error number on failure, removing them to solve this > problem. Is that check valid? 0 was a valid interrupt for some architectures...
On 2023/4/24 19:48, Mark Brown wrote: > On Sun, Apr 23, 2023 at 03:24:46AM +0000, Li Ningke wrote: >> Smatch complains that >> drivers/spi/spi-davinci.c:915 davinci_spi_probe() warn: >> platform_get_irq() does not return zero >> >> There is no need to check whether the return value is zero as >> `platform_get_irq()` only returns non-zero IRQ number on success >> or negative error number on failure, removing them to solve this >> problem. > Is that check valid? 0 was a valid interrupt for some architectures... We just follow the comments of platform_get_irq(). /** * platform_get_irq - get an IRQ for a device * @dev: platform device * @num: IRQ number index * * Gets an IRQ for a platform device and prints an error message if finding the * IRQ fails. Device drivers should check the return value for errors so as to * not pass a negative integer value to the request_irq() APIs. * * For example:: * * int irq = platform_get_irq(pdev, 0); * if (irq < 0) * return irq; * * Return: non-zero IRQ number on success, negative error number on failure. */ int platform_get_irq(struct platform_device *dev, unsigned int num) { int ret; ret = platform_get_irq_optional(dev, num); if (ret < 0) return dev_err_probe(&dev->dev, ret, "IRQ index %u not found\n", num); return ret; }
On Mon, Apr 24, 2023 at 08:03:42PM +0800, Dongliang Mu wrote: > On 2023/4/24 19:48, Mark Brown wrote: > > Is that check valid? 0 was a valid interrupt for some architectures... > We just follow the comments of platform_get_irq(). > * Gets an IRQ for a platform device and prints an error message if finding > the > * IRQ fails. Device drivers should check the return value for errors so as > to > * not pass a negative integer value to the request_irq() APIs. I'm not sure that's universally true yet, though there were some moves to try to get us there. arm, where this driver is used, was one of the platforms with 0 as a valid interrupt.
On 2023/4/24 23:52, Mark Brown wrote: > On Mon, Apr 24, 2023 at 08:03:42PM +0800, Dongliang Mu wrote: >> On 2023/4/24 19:48, Mark Brown wrote: >>> Is that check valid? 0 was a valid interrupt for some architectures... >> We just follow the comments of platform_get_irq(). >> * Gets an IRQ for a platform device and prints an error message if finding >> the >> * IRQ fails. Device drivers should check the return value for errors so as >> to >> * not pass a negative integer value to the request_irq() APIs. > I'm not sure that's universally true yet, though there were some moves > to try to get us there. arm, where this driver is used, was one of the > platforms with 0 as a valid interrupt. Hi Brown, First, we're sorry about the fact that our internal robot(beta) made a mistake and sent our testing message to LKML. We have fixed the incorrect logic. Second, from code review of platform_get_irq / platform_get_irq_optional, it would warn IRQ 0 as an invalid IRQ number. out: if (WARN(!ret, "0 is an invalid IRQ number\n")) return -EINVAL; return ret; Dongliang Mu
On Wed, Apr 26, 2023 at 09:50:26AM +0800, Dongliang Mu wrote: > Second, from code review of platform_get_irq / platform_get_irq_optional, it > would warn IRQ 0 as an invalid IRQ number. > out: > if (WARN(!ret, "0 is an invalid IRQ number\n")) > return -EINVAL; > return ret; Like I say I'm not sure that's actually accurate for all architectures yet.
On 2023/4/26 22:13, Mark Brown wrote: > On Wed, Apr 26, 2023 at 09:50:26AM +0800, Dongliang Mu wrote: > >> Second, from code review of platform_get_irq / platform_get_irq_optional, it >> would warn IRQ 0 as an invalid IRQ number. >> out: >> if (WARN(!ret, "0 is an invalid IRQ number\n")) >> return -EINVAL; >> return ret; > Like I say I'm not sure that's actually accurate for all architectures > yet. I see. Let's wait and see. When it becomes stable and universal for all architectures, we could clean up them all together. Currently our team just works to make Smatch happy :)
diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c index d112c2cac042..fdb241e3a7bf 100644 --- a/drivers/spi/spi-davinci.c +++ b/drivers/spi/spi-davinci.c @@ -912,8 +912,6 @@ static int davinci_spi_probe(struct platform_device *pdev) init_completion(&dspi->done); ret = platform_get_irq(pdev, 0); - if (ret == 0) - ret = -EINVAL; if (ret < 0) goto free_master; dspi->irq = ret;