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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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/[email protected]/
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/[email protected]/
>
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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
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/[email protected]/
Abandon this patch and Thanks again!
BR,
Dejin
> Thanks,
> Michal