| Message ID | 20200520144821.8069-1-zhengdejin5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: cadence: Add an error handling for platform_get_irq() | expand |
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
> 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?
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
> 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/
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
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
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 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(+)