Re: [PATCH v3 1/5] i2c: i2c-davinci: switch to use platform_get_irq
On Mon, Dec 01, 2014 at 05:34:03PM +0200, Grygorii Strashko wrote: > Switch Davinci I2C driver to use platform_get_irq(), because > it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..) > for requesting IRQ resources any more, as they can be not ready yet > in case of DT-boot. > > CC: Sekhar Nori > CC: Kevin Hilman > CC: Santosh Shilimkar > CC: Murali Karicheri > Acked-by: Uwe Kleine-König > Signed-off-by: Grygorii Strashko Applied to for-next, thanks! signature.asc Description: Digital signature
[PATCH v3 1/5] i2c: i2c-davinci: switch to use platform_get_irq
Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Acked-by: Uwe Kleine-König
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..25e8e25 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
- int r;
-
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ struct resource *mem;
+ int r, irq;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ if (!irq)
+ irq = -ENXIO;
+ if (irq != -EPROBE_DEFER)
+ dev_err(&pdev->dev,
+ "can't get irq resource ret=%d\n", irq);
+ return irq;
}
dev = devm_kzalloc(&pdev->dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +665,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(&dev->xfr_complete);
#endif
dev->dev = &pdev->dev;
- dev->irq = irq->start;
+ dev->irq = irq;
dev->pdata = dev_get_platdata(&pdev->dev);
platform_set_drvdata(pdev, dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq
On 11/26/2014 05:54 PM, Uwe Kleine-König wrote:
Hello Grygorii,
On Wed, Nov 26, 2014 at 03:59:49PM +0200, Grygorii Strashko wrote:
Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..7f54903 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
- int r;
-
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ struct resource *mem;
+ int r, irq;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ if (irq == -EPROBE_DEFER)
+ return irq;
+ if (!irq)
+ irq = -ENXIO;
+ dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
+ return irq;
The simpler and I think also more usual logic is:
if (!irq)
irq = -ENXIO;
if (irq != -EPROBE_DEFER)
dev_err(...);
return irq;
Other than that the patch looks fine.
Ok. will change and add your ack.
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq
Hello Grygorii,
On Wed, Nov 26, 2014 at 03:59:49PM +0200, Grygorii Strashko wrote:
> Switch Davinci I2C driver to use platform_get_irq(), because
> it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
> for requesting IRQ resources any more, as they can be not ready yet
> in case of DT-boot.
>
> CC: Sekhar Nori
> CC: Kevin Hilman
> CC: Santosh Shilimkar
> CC: Murali Karicheri
> Signed-off-by: Grygorii Strashko
> ---
> drivers/i2c/busses/i2c-davinci.c | 20
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c
> b/drivers/i2c/busses/i2c-davinci.c
> index 4d96147..7f54903 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device
> *pdev)
> {
> struct davinci_i2c_dev *dev;
> struct i2c_adapter *adap;
> - struct resource *mem, *irq;
> - int r;
> -
> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!irq) {
> - dev_err(&pdev->dev, "no irq resource?\n");
> - return -ENODEV;
> + struct resource *mem;
> + int r, irq;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0) {
> + if (irq == -EPROBE_DEFER)
> + return irq;
> + if (!irq)
> + irq = -ENXIO;
> + dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
> + return irq;
The simpler and I think also more usual logic is:
if (!irq)
irq = -ENXIO;
if (irq != -EPROBE_DEFER)
dev_err(...);
return irq;
Other than that the patch looks fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq
Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..7f54903 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
- int r;
-
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ struct resource *mem;
+ int r, irq;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ if (irq == -EPROBE_DEFER)
+ return irq;
+ if (!irq)
+ irq = -ENXIO;
+ dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
+ return irq;
}
dev = devm_kzalloc(&pdev->dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +665,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(&dev->xfr_complete);
#endif
dev->dev = &pdev->dev;
- dev->irq = irq->start;
+ dev->irq = irq;
dev->pdata = dev_get_platdata(&pdev->dev);
platform_set_drvdata(pdev, dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On 11/21/2014 04:03 PM, Rob Herring wrote:
On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
wrote:
On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
Hello Grygorii,
On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
-struct resource *mem, *irq;
-int r;
+struct resource *mem;
+int r, irq;
-irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-if (!irq) {
-dev_err(&pdev->dev, "no irq resource?\n");
-return -ENODEV;
+irq = platform_get_irq(pdev, 0);
One bad thing about platform_get_irq is its unusual handling of irq=0.
I'm pretty sure you don't want to use this value, so adding something
like:
if (!irq)
irq = -ENXIO
I'll add this check in driver.
would be welcome because the usual value for "invalid irq" is 0 and not
-ESOMETHING. platform_get_irq is one of the very few functions that
don't adhere to this convention. With handling <= 0 as error your code
is immune to changes in this area. Although I notice that
platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
Apart from your change I wonder if platform_get_irq should handle
of_irq_get returning 0 as an error.
I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.
Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.
Thanks for your comment.
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On Fri, Nov 21, 2014 at 5:01 AM, Grygorii Strashko
wrote:
> On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
>> Hello Grygorii,
>>
>> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>>> Switch Davinci I2C driver to use platform_get_irq(), because
>>> - it is not recommened to use
>>>platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>>resources any more, as they can be not ready yet in case of DT-booting.
>>> - it makes code simpler
>>>
>>> CC: Sekhar Nori
>>> CC: Kevin Hilman
>>> CC: Santosh Shilimkar
>>> CC: Murali Karicheri
>>> Signed-off-by: Grygorii Strashko
>>> ---
>>> drivers/i2c/busses/i2c-davinci.c | 14 +++---
>>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>>> b/drivers/i2c/busses/i2c-davinci.c
>>> index 4d96147..9bbfb8f 100644
>>> --- a/drivers/i2c/busses/i2c-davinci.c
>>> +++ b/drivers/i2c/busses/i2c-davinci.c
>>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device
>>> *pdev)
>>> {
>>> struct davinci_i2c_dev *dev;
>>> struct i2c_adapter *adap;
>>> -struct resource *mem, *irq;
>>> -int r;
>>> +struct resource *mem;
>>> +int r, irq;
>>>
>>> -irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>> -if (!irq) {
>>> -dev_err(&pdev->dev, "no irq resource?\n");
>>> -return -ENODEV;
>>> +irq = platform_get_irq(pdev, 0);
>> One bad thing about platform_get_irq is its unusual handling of irq=0.
>> I'm pretty sure you don't want to use this value, so adding something
>> like:
>>
>> if (!irq)
>> irq = -ENXIO
>>
>> would be welcome because the usual value for "invalid irq" is 0 and not
>> -ESOMETHING. platform_get_irq is one of the very few functions that
>> don't adhere to this convention. With handling <= 0 as error your code
>> is immune to changes in this area. Although I notice that
>> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>>
>> Apart from your change I wonder if platform_get_irq should handle
>> of_irq_get returning 0 as an error.
>
> I think you are right and It seems like, the check for !irq should
> be added/restored for OF case in platform_get_irq() too.
Changing the return values of platform_get_irq is tricky as it would
potentially break drivers because NO_IRQ can be 0 or -1 depending on
the arch. Drivers checking against specific values of NO_IRQ would
break. We've done some clean-up in this area, but I suspect more is
needed.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
On 11/20/2014 11:48 PM, Uwe Kleine-König wrote:
> Hello Grygorii,
>
> On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
>> Switch Davinci I2C driver to use platform_get_irq(), because
>> - it is not recommened to use
>>platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
>>resources any more, as they can be not ready yet in case of DT-booting.
>> - it makes code simpler
>>
>> CC: Sekhar Nori
>> CC: Kevin Hilman
>> CC: Santosh Shilimkar
>> CC: Murali Karicheri
>> Signed-off-by: Grygorii Strashko
>> ---
>> drivers/i2c/busses/i2c-davinci.c | 14 +++---
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c
>> b/drivers/i2c/busses/i2c-davinci.c
>> index 4d96147..9bbfb8f 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device
>> *pdev)
>> {
>> struct davinci_i2c_dev *dev;
>> struct i2c_adapter *adap;
>> -struct resource *mem, *irq;
>> -int r;
>> +struct resource *mem;
>> +int r, irq;
>>
>> -irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> -if (!irq) {
>> -dev_err(&pdev->dev, "no irq resource?\n");
>> -return -ENODEV;
>> +irq = platform_get_irq(pdev, 0);
> One bad thing about platform_get_irq is its unusual handling of irq=0.
> I'm pretty sure you don't want to use this value, so adding something
> like:
>
> if (!irq)
> irq = -ENXIO
>
> would be welcome because the usual value for "invalid irq" is 0 and not
> -ESOMETHING. platform_get_irq is one of the very few functions that
> don't adhere to this convention. With handling <= 0 as error your code
> is immune to changes in this area. Although I notice that
> platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
>
> Apart from your change I wonder if platform_get_irq should handle
> of_irq_get returning 0 as an error.
I think you are right and It seems like, the check for !irq should
be added/restored for OF case in platform_get_irq() too.
Also, I've simulated irq == 0 case - the .probe() failed with error -EINVAL
which is returned by request_threaded_irq() because of
!irq_settings_can_request(desc).
i2c_davinci 253.i2c: failure requesting irq 0
i2c_davinci: probe of 253.i2c failed with error -22
I'm not sure that above will work for everyone because it depends on
ARCH_IRQ_INIT_FLAGS
and ARCH_IRQ_INIT_FLAGS = (IRQ_NOREQUEST | IRQ_NOPROBE) for ARM.
>
>> +if (irq < 0) {
>> +dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
> Please don't print an error if irq=-EPROBE_DEFER.
ok.
regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [1/5] i2c: i2c-davinci: switch to use platform_get_irq
Hello Grygorii,
On Thu, Nov 20, 2014 at 12:03:04PM +0200, Grygorii Strashko wrote:
> Switch Davinci I2C driver to use platform_get_irq(), because
> - it is not recommened to use
> platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
> resources any more, as they can be not ready yet in case of DT-booting.
> - it makes code simpler
>
> CC: Sekhar Nori
> CC: Kevin Hilman
> CC: Santosh Shilimkar
> CC: Murali Karicheri
> Signed-off-by: Grygorii Strashko
> ---
> drivers/i2c/busses/i2c-davinci.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c
> b/drivers/i2c/busses/i2c-davinci.c
> index 4d96147..9bbfb8f 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device
> *pdev)
> {
> struct davinci_i2c_dev *dev;
> struct i2c_adapter *adap;
> - struct resource *mem, *irq;
> - int r;
> + struct resource *mem;
> + int r, irq;
>
> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!irq) {
> - dev_err(&pdev->dev, "no irq resource?\n");
> - return -ENODEV;
> + irq = platform_get_irq(pdev, 0);
One bad thing about platform_get_irq is its unusual handling of irq=0.
I'm pretty sure you don't want to use this value, so adding something
like:
if (!irq)
irq = -ENXIO
would be welcome because the usual value for "invalid irq" is 0 and not
-ESOMETHING. platform_get_irq is one of the very few functions that
don't adhere to this convention. With handling <= 0 as error your code
is immune to changes in this area. Although I notice that
platform_get_irq got worse in this respect to handle -EPROBE_DEFER. hmm.
Apart from your change I wonder if platform_get_irq should handle
of_irq_get returning 0 as an error.
> + if (irq < 0) {
> + dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
Please don't print an error if irq=-EPROBE_DEFER.
> + return irq;
> }
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/5] i2c: i2c-davinci: switch to use platform_get_irq
Switch Davinci I2C driver to use platform_get_irq(), because
- it is not recommened to use
platform_get_resource(pdev, IORESOURCE_IRQ, ..) for requesting IRQ's
resources any more, as they can be not ready yet in case of DT-booting.
- it makes code simpler
CC: Sekhar Nori
CC: Kevin Hilman
CC: Santosh Shilimkar
CC: Murali Karicheri
Signed-off-by: Grygorii Strashko
---
drivers/i2c/busses/i2c-davinci.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..9bbfb8f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,13 @@ static int davinci_i2c_probe(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev;
struct i2c_adapter *adap;
- struct resource *mem, *irq;
- int r;
+ struct resource *mem;
+ int r, irq;
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
+ return irq;
}
dev = devm_kzalloc(&pdev->dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +661,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
init_completion(&dev->xfr_complete);
#endif
dev->dev = &pdev->dev;
- dev->irq = irq->start;
+ dev->irq = irq;
dev->pdata = dev_get_platdata(&pdev->dev);
platform_set_drvdata(pdev, dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
