* [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() @ 2020-05-20 14:48 Dejin Zheng 2020-05-22 11:24 ` Michal Simek 0 siblings, 1 reply; 7+ messages in thread From: Dejin Zheng @ 2020-05-20 14:48 UTC (permalink / raw) To: michal.simek, wsa, harinik, soren.brinkmann, linux-i2c Cc: linux-kernel, Dejin Zheng The driver initialization should be end immediately after found the platform_get_irq() function return an error. Fixes: df8eb5691c48d3b0 ("i2c: Add driver for Cadence I2C controller") Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- v1 -> v2: - add Fixes tag. drivers/i2c/busses/i2c-cadence.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c index 89d58f7d2a25..0e8debe32cea 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -912,6 +912,8 @@ static int cdns_i2c_probe(struct platform_device *pdev) return PTR_ERR(id->membase); id->irq = platform_get_irq(pdev, 0); + if (id->irq < 0) + return id->irq; id->adap.owner = THIS_MODULE; id->adap.dev.of_node = pdev->dev.of_node; -- 2.25.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-20 14:48 [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() Dejin Zheng @ 2020-05-22 11:24 ` Michal Simek 2020-05-22 15:14 ` Wolfram Sang 2020-05-23 15:26 ` Dejin Zheng 0 siblings, 2 replies; 7+ messages in thread From: Michal Simek @ 2020-05-22 11:24 UTC (permalink / raw) To: Dejin Zheng, michal.simek, wsa, harinik, soren.brinkmann, linux-i2c Cc: linux-kernel On 20. 05. 20 16:48, Dejin Zheng wrote: > The driver initialization should be end immediately after found > the platform_get_irq() function return an error. > > Fixes: df8eb5691c48d3b0 ("i2c: Add driver for Cadence I2C controller") I wouldn't really consider this as bug. Driver is likely not failing when irq is not defined. It should just fail later on when devm_request_irq is called. Or is there any other issue with it? > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > v1 -> v2: > - add Fixes tag. > > drivers/i2c/busses/i2c-cadence.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > index 89d58f7d2a25..0e8debe32cea 100644 > --- a/drivers/i2c/busses/i2c-cadence.c > +++ b/drivers/i2c/busses/i2c-cadence.c > @@ -912,6 +912,8 @@ static int cdns_i2c_probe(struct platform_device *pdev) > return PTR_ERR(id->membase); > > id->irq = platform_get_irq(pdev, 0); > + if (id->irq < 0) > + return id->irq; > > id->adap.owner = THIS_MODULE; > id->adap.dev.of_node = pdev->dev.of_node; > The change is valid but the question is if make sense to do it in this way. Some drivers are using devm_request_irq to do do job. For example: id->irq = platform_get_irq(pdev, 0); ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0, DRIVER_NAME, id); if (ret) return ret; But I am also fine with solution above where you fail in quickest way. Without that Fixed tag Acked-by: Michal Simek <michal.simek@xilinx.com> Thanks, Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-22 11:24 ` Michal Simek @ 2020-05-22 15:14 ` Wolfram Sang 2020-05-22 15:26 ` Michal Simek 2020-05-23 15:26 ` Dejin Zheng 1 sibling, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2020-05-22 15:14 UTC (permalink / raw) To: Michal Simek Cc: Dejin Zheng, harinik, soren.brinkmann, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 486 bytes --] > The change is valid but the question is if make sense to do it in this > way. Some drivers are using devm_request_irq to do do job. > > For example: > id->irq = platform_get_irq(pdev, 0); > ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0, > DRIVER_NAME, id); > if (ret) > return ret; I like this version better. Maybe we should simply move the platform_get_irq() line to the devm_request_irq() call? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-22 15:14 ` Wolfram Sang @ 2020-05-22 15:26 ` Michal Simek 2020-05-22 20:19 ` Wolfram Sang 0 siblings, 1 reply; 7+ messages in thread From: Michal Simek @ 2020-05-22 15:26 UTC (permalink / raw) To: Wolfram Sang, Michal Simek Cc: Dejin Zheng, harinik, soren.brinkmann, linux-i2c, linux-kernel On 22. 05. 20 17:14, Wolfram Sang wrote: > >> The change is valid but the question is if make sense to do it in this >> way. Some drivers are using devm_request_irq to do do job. >> >> For example: >> id->irq = platform_get_irq(pdev, 0); >> ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0, >> DRIVER_NAME, id); >> if (ret) >> return ret; > > I like this version better. Maybe we should simply move the > platform_get_irq() line to the devm_request_irq() call? You know about devm_platform_get_and_ioremap_resource() usage. Maybe that's the way to go. Because as of today there is no way to pass position of irq resource. But I expect it will come in near future. Thanks, Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-22 15:26 ` Michal Simek @ 2020-05-22 20:19 ` Wolfram Sang 2020-05-23 13:36 ` Dejin Zheng 0 siblings, 1 reply; 7+ messages in thread From: Wolfram Sang @ 2020-05-22 20:19 UTC (permalink / raw) To: Michal Simek Cc: Dejin Zheng, harinik, soren.brinkmann, linux-i2c, linux-kernel [-- Attachment #1: Type: text/plain, Size: 367 bytes --] > You know about > devm_platform_get_and_ioremap_resource() > usage. > Maybe that's the way to go. Because as of today there is no way to pass > position of irq resource. > > But I expect it will come in near future. Has been tried, has been nacked: http://patchwork.ozlabs.org/project/linux-i2c/patch/20200518155304.28639-3-zhengdejin5@gmail.com/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-22 20:19 ` Wolfram Sang @ 2020-05-23 13:36 ` Dejin Zheng 0 siblings, 0 replies; 7+ messages in thread From: Dejin Zheng @ 2020-05-23 13:36 UTC (permalink / raw) To: Wolfram Sang Cc: Michal Simek, harinik, soren.brinkmann, linux-i2c, linux-kernel On Fri, May 22, 2020 at 10:19:56PM +0200, Wolfram Sang wrote: > > > You know about > > devm_platform_get_and_ioremap_resource() > > usage. > > Maybe that's the way to go. Because as of today there is no way to pass > > position of irq resource. > > > > But I expect it will come in near future. > > Has been tried, has been nacked: > > http://patchwork.ozlabs.org/project/linux-i2c/patch/20200518155304.28639-3-zhengdejin5@gmail.com/ > Wolfram and Michal, Thanks very much for your comments, I will resend devm_platform_request_irq() again. BR, Dejin ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() 2020-05-22 11:24 ` Michal Simek 2020-05-22 15:14 ` Wolfram Sang @ 2020-05-23 15:26 ` Dejin Zheng 1 sibling, 0 replies; 7+ messages in thread From: Dejin Zheng @ 2020-05-23 15:26 UTC (permalink / raw) To: Michal Simek; +Cc: wsa, harinik, soren.brinkmann, linux-i2c, linux-kernel On Fri, May 22, 2020 at 01:24:57PM +0200, Michal Simek wrote: > On 20. 05. 20 16:48, Dejin Zheng wrote: > > The driver initialization should be end immediately after found > > the platform_get_irq() function return an error. > > > > Fixes: df8eb5691c48d3b0 ("i2c: Add driver for Cadence I2C controller") > > I wouldn't really consider this as bug. Driver is likely not failing > when irq is not defined. It should just fail later on when > devm_request_irq is called. > Or is there any other issue with it? > > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > > --- > > v1 -> v2: > > - add Fixes tag. > > > > drivers/i2c/busses/i2c-cadence.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c > > index 89d58f7d2a25..0e8debe32cea 100644 > > --- a/drivers/i2c/busses/i2c-cadence.c > > +++ b/drivers/i2c/busses/i2c-cadence.c > > @@ -912,6 +912,8 @@ static int cdns_i2c_probe(struct platform_device *pdev) > > return PTR_ERR(id->membase); > > > > id->irq = platform_get_irq(pdev, 0); > > + if (id->irq < 0) > > + return id->irq; > > > > id->adap.owner = THIS_MODULE; > > id->adap.dev.of_node = pdev->dev.of_node; > > > > The change is valid but the question is if make sense to do it in this > way. Some drivers are using devm_request_irq to do do job. > > For example: > id->irq = platform_get_irq(pdev, 0); > ret = devm_request_irq(&pdev->dev, id->irq, cdns_i2c_isr, 0, > DRIVER_NAME, id); > if (ret) > return ret; > > But I am also fine with solution above where you fail in quickest way. > > Without that Fixed tag > Acked-by: Michal Simek <michal.simek@xilinx.com> > Michal, Thanks very much for review my patch, As you said, maybe the better way is provide a function like the devm_platform_get_and_ioremap_resource(). So I resend a patch of I gave up before, It's here now: https://patchwork.ozlabs.org/project/linux-i2c/patch/20200523145157.16257-3-zhengdejin5@gmail.com/ Abandon this patch and Thanks again! BR, Dejin > Thanks, > Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-23 15:26 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-20 14:48 [PATCH v2] i2c: cadence: Add an error handling for platform_get_irq() Dejin Zheng 2020-05-22 11:24 ` Michal Simek 2020-05-22 15:14 ` Wolfram Sang 2020-05-22 15:26 ` Michal Simek 2020-05-22 20:19 ` Wolfram Sang 2020-05-23 13:36 ` Dejin Zheng 2020-05-23 15:26 ` Dejin Zheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).