* [PATCH] staging: greybus: Use gpio_is_valid()
@ 2018-04-27 10:52 Arvind Yadav
2018-04-27 12:17 ` [greybus-dev] " Alex Elder
0 siblings, 1 reply; 5+ messages in thread
From: Arvind Yadav @ 2018-04-27 10:52 UTC (permalink / raw)
To: hvaibhav.linux, johan, elder, gregkh; +Cc: linux-kernel, greybus-dev, devel
Replace the manual validity checks for the GPIO with the
gpio_is_valid().
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
drivers/staging/greybus/arche-platform.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index 83254a7..fc6bf60 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device *pdev)
arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
"svc,reset-gpio",
0);
- if (arche_pdata->svc_reset_gpio < 0) {
+ if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
dev_err(dev, "failed to get reset-gpio\n");
- return arche_pdata->svc_reset_gpio;
+ return -ENODEV;
}
ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
if (ret) {
@@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device *pdev)
arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
"svc,sysboot-gpio",
0);
- if (arche_pdata->svc_sysboot_gpio < 0) {
+ if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
dev_err(dev, "failed to get sysboot gpio\n");
- return arche_pdata->svc_sysboot_gpio;
+ return -ENODEV;
}
ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
if (ret) {
@@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device *pdev)
arche_pdata->svc_refclk_req = of_get_named_gpio(np,
"svc,refclk-req-gpio",
0);
- if (arche_pdata->svc_refclk_req < 0) {
+ if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
dev_err(dev, "failed to get svc clock-req gpio\n");
- return arche_pdata->svc_refclk_req;
+ return -ENODEV;
}
ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
"svc-clk-req");
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
2018-04-27 10:52 [PATCH] staging: greybus: Use gpio_is_valid() Arvind Yadav
@ 2018-04-27 12:17 ` Alex Elder
2018-04-27 12:50 ` Arvind Yadav
0 siblings, 1 reply; 5+ messages in thread
From: Alex Elder @ 2018-04-27 12:17 UTC (permalink / raw)
To: Arvind Yadav, hvaibhav.linux, johan, elder, gregkh
Cc: devel, greybus-dev, linux-kernel
On 04/27/2018 05:52 AM, Arvind Yadav wrote:
> Replace the manual validity checks for the GPIO with the
> gpio_is_valid().
I haven't looked through the code paths very closely, but I
think that get_named_gpio() might return -EPROBE_DEFER, which
would be something we want to pass to the caller.
So rather than returning -ENODEV and hiding the reason the
call to of_get_named_gpio() failed, you should continue
returning the errno it supplies (if not a valid gpio number).
-Alex
> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
> ---
> drivers/staging/greybus/arche-platform.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index 83254a7..fc6bf60 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device *pdev)
> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> "svc,reset-gpio",
> 0);
> - if (arche_pdata->svc_reset_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> dev_err(dev, "failed to get reset-gpio\n");
> - return arche_pdata->svc_reset_gpio;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> if (ret) {
> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device *pdev)
> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
> "svc,sysboot-gpio",
> 0);
> - if (arche_pdata->svc_sysboot_gpio < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
> dev_err(dev, "failed to get sysboot gpio\n");
> - return arche_pdata->svc_sysboot_gpio;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
> if (ret) {
> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device *pdev)
> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
> "svc,refclk-req-gpio",
> 0);
> - if (arche_pdata->svc_refclk_req < 0) {
> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
> dev_err(dev, "failed to get svc clock-req gpio\n");
> - return arche_pdata->svc_refclk_req;
> + return -ENODEV;
> }
> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
> "svc-clk-req");
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
2018-04-27 12:17 ` [greybus-dev] " Alex Elder
@ 2018-04-27 12:50 ` Arvind Yadav
2018-04-27 13:02 ` Alex Elder
0 siblings, 1 reply; 5+ messages in thread
From: Arvind Yadav @ 2018-04-27 12:50 UTC (permalink / raw)
To: Alex Elder, hvaibhav.linux, johan, elder, gregkh
Cc: devel, greybus-dev, linux-kernel
On Friday 27 April 2018 05:47 PM, Alex Elder wrote:
> On 04/27/2018 05:52 AM, Arvind Yadav wrote:
>> Replace the manual validity checks for the GPIO with the
>> gpio_is_valid().
> I haven't looked through the code paths very closely, but I
> think that get_named_gpio() might return -EPROBE_DEFER, which
> would be something we want to pass to the caller.
Yes of_get_name_gpio() can return other error value apart from
-EPROBE_DEFER.
> So rather than returning -ENODEV and hiding the reason the
> call to of_get_named_gpio() failed, you should continue
> returning the errno it supplies (if not a valid gpio number).
>
> -Alex
I have return -ENODEV because invalid gpio pin can be positive.
static inline bool gpio_is_valid(int number)
{
return number >= 0 && number < ARCH_NR_GPIOS;
}
Here if number > ARCH_NR_GPIOS then it's invalid but return value
will be positive.
We can return like this
" return (gpio > 0) ? -ENODEV: gpio;"
But not sure this is worth to handle this.
~arvind
>
>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>> ---
>> drivers/staging/greybus/arche-platform.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
>> index 83254a7..fc6bf60 100644
>> --- a/drivers/staging/greybus/arche-platform.c
>> +++ b/drivers/staging/greybus/arche-platform.c
>> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
>> "svc,reset-gpio",
>> 0);
>> - if (arche_pdata->svc_reset_gpio < 0) {
>> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
>> dev_err(dev, "failed to get reset-gpio\n");
>> - return arche_pdata->svc_reset_gpio;
>> + return -ENODEV;
>> }
>> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
>> if (ret) {
>> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
>> "svc,sysboot-gpio",
>> 0);
>> - if (arche_pdata->svc_sysboot_gpio < 0) {
>> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
>> dev_err(dev, "failed to get sysboot gpio\n");
>> - return arche_pdata->svc_sysboot_gpio;
>> + return -ENODEV;
>> }
>> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
>> if (ret) {
>> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
>> "svc,refclk-req-gpio",
>> 0);
>> - if (arche_pdata->svc_refclk_req < 0) {
>> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
>> dev_err(dev, "failed to get svc clock-req gpio\n");
>> - return arche_pdata->svc_refclk_req;
>> + return -ENODEV;
>> }
>> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
>> "svc-clk-req");
>>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
2018-04-27 12:50 ` Arvind Yadav
@ 2018-04-27 13:02 ` Alex Elder
2018-04-28 4:30 ` arvindY
0 siblings, 1 reply; 5+ messages in thread
From: Alex Elder @ 2018-04-27 13:02 UTC (permalink / raw)
To: Arvind Yadav, hvaibhav.linux, johan, elder, gregkh
Cc: devel, greybus-dev, linux-kernel
On 04/27/2018 07:50 AM, Arvind Yadav wrote:
>
>
> On Friday 27 April 2018 05:47 PM, Alex Elder wrote:
>> On 04/27/2018 05:52 AM, Arvind Yadav wrote:
>>> Replace the manual validity checks for the GPIO with the
>>> gpio_is_valid().
>> I haven't looked through the code paths very closely, but I
>> think that get_named_gpio() might return -EPROBE_DEFER, which
>> would be something we want to pass to the caller.
> Yes of_get_name_gpio() can return other error value apart from
> -EPROBE_DEFER.
>> So rather than returning -ENODEV and hiding the reason the
>> call to of_get_named_gpio() failed, you should continue
>> returning the errno it supplies (if not a valid gpio number).
>>
>> -Alex
> I have return -ENODEV because invalid gpio pin can be positive.
> static inline bool gpio_is_valid(int number)
> {
> return number >= 0 && number < ARCH_NR_GPIOS;
> }
> Here if number > ARCH_NR_GPIOS then it's invalid but return value
> will be positive.
Your reasoning is good. However in all three of these cases,
the GPIO number you're checking is the value returned from
of_get_named_gpio(). The return value is a "GPIO number to
use with Linux generic GPIO API, or one of the errno value."
So unless the API of of_get_named_gpio() changes, you can be
sure that if the value returned is invalid, it is a negative
errno. (And if the API did change, the person making that
change would be responsible for fixing all callers to ensure
the change didn't break them.)
This distinction may be why the code you're changing was only
testing for negative, rather than using gpio_is_valid() (you'll
see it's used elsewhere in the Greybus code--even in the same
source files.)
Anyway, changing the code to use gpio_is_valid() is fine. But
you should avoid obscuring the reason for the error that the
return value from of_get_named_gpio() provides.
-Alex
> We can return like this
> " return (gpio > 0) ? -ENODEV: gpio;"
>
> But not sure this is worth to handle this.
>
> ~arvind
>>
>>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>> ---
>>> drivers/staging/greybus/arche-platform.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
>>> index 83254a7..fc6bf60 100644
>>> --- a/drivers/staging/greybus/arche-platform.c
>>> +++ b/drivers/staging/greybus/arche-platform.c
>>> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
>>> "svc,reset-gpio",
>>> 0);
>>> - if (arche_pdata->svc_reset_gpio < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
>>> dev_err(dev, "failed to get reset-gpio\n");
>>> - return arche_pdata->svc_reset_gpio;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
>>> if (ret) {
>>> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
>>> "svc,sysboot-gpio",
>>> 0);
>>> - if (arche_pdata->svc_sysboot_gpio < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
>>> dev_err(dev, "failed to get sysboot gpio\n");
>>> - return arche_pdata->svc_sysboot_gpio;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
>>> if (ret) {
>>> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
>>> "svc,refclk-req-gpio",
>>> 0);
>>> - if (arche_pdata->svc_refclk_req < 0) {
>>> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
>>> dev_err(dev, "failed to get svc clock-req gpio\n");
>>> - return arche_pdata->svc_refclk_req;
>>> + return -ENODEV;
>>> }
>>> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
>>> "svc-clk-req");
>>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [greybus-dev] [PATCH] staging: greybus: Use gpio_is_valid()
2018-04-27 13:02 ` Alex Elder
@ 2018-04-28 4:30 ` arvindY
0 siblings, 0 replies; 5+ messages in thread
From: arvindY @ 2018-04-28 4:30 UTC (permalink / raw)
To: Alex Elder, hvaibhav.linux, johan, elder, gregkh
Cc: devel, greybus-dev, linux-kernel
On Friday 27 April 2018 06:32 PM, Alex Elder wrote:
> On 04/27/2018 07:50 AM, Arvind Yadav wrote:
>>
>> On Friday 27 April 2018 05:47 PM, Alex Elder wrote:
>>> On 04/27/2018 05:52 AM, Arvind Yadav wrote:
>>>> Replace the manual validity checks for the GPIO with the
>>>> gpio_is_valid().
>>> I haven't looked through the code paths very closely, but I
>>> think that get_named_gpio() might return -EPROBE_DEFER, which
>>> would be something we want to pass to the caller.
>> Yes of_get_name_gpio() can return other error value apart from
>> -EPROBE_DEFER.
>>> So rather than returning -ENODEV and hiding the reason the
>>> call to of_get_named_gpio() failed, you should continue
>>> returning the errno it supplies (if not a valid gpio number).
>>>
>>> -Alex
>> I have return -ENODEV because invalid gpio pin can be positive.
>> static inline bool gpio_is_valid(int number)
>> {
>> return number >= 0 && number < ARCH_NR_GPIOS;
>> }
>> Here if number > ARCH_NR_GPIOS then it's invalid but return value
>> will be positive.
> Your reasoning is good. However in all three of these cases,
> the GPIO number you're checking is the value returned from
> of_get_named_gpio(). The return value is a "GPIO number to
> use with Linux generic GPIO API, or one of the errno value."
>
> So unless the API of of_get_named_gpio() changes, you can be
> sure that if the value returned is invalid, it is a negative
> errno. (And if the API did change, the person making that
> change would be responsible for fixing all callers to ensure
> the change didn't break them.)
>
> This distinction may be why the code you're changing was only
> testing for negative, rather than using gpio_is_valid() (you'll
> see it's used elsewhere in the Greybus code--even in the same
> source files.)
>
> Anyway, changing the code to use gpio_is_valid() is fine. But
> you should avoid obscuring the reason for the error that the
> return value from of_get_named_gpio() provides.
>
> -Alex
Yes, It'll be fine to return a invalid gpio as error instead of
-ENODEV. I will send an updated patch.
~arvind
>
>> We can return like this
>> " return (gpio > 0) ? -ENODEV: gpio;"
>>
>> But not sure this is worth to handle this.
>>
>> ~arvind
>>>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
>>>> ---
>>>> drivers/staging/greybus/arche-platform.c | 12 ++++++------
>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
>>>> index 83254a7..fc6bf60 100644
>>>> --- a/drivers/staging/greybus/arche-platform.c
>>>> +++ b/drivers/staging/greybus/arche-platform.c
>>>> @@ -448,9 +448,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>>> arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
>>>> "svc,reset-gpio",
>>>> 0);
>>>> - if (arche_pdata->svc_reset_gpio < 0) {
>>>> + if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
>>>> dev_err(dev, "failed to get reset-gpio\n");
>>>> - return arche_pdata->svc_reset_gpio;
>>>> + return -ENODEV;
>>>> }
>>>> ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
>>>> if (ret) {
>>>> @@ -468,9 +468,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>>> arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
>>>> "svc,sysboot-gpio",
>>>> 0);
>>>> - if (arche_pdata->svc_sysboot_gpio < 0) {
>>>> + if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
>>>> dev_err(dev, "failed to get sysboot gpio\n");
>>>> - return arche_pdata->svc_sysboot_gpio;
>>>> + return -ENODEV;
>>>> }
>>>> ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
>>>> if (ret) {
>>>> @@ -487,9 +487,9 @@ static int arche_platform_probe(struct platform_device *pdev)
>>>> arche_pdata->svc_refclk_req = of_get_named_gpio(np,
>>>> "svc,refclk-req-gpio",
>>>> 0);
>>>> - if (arche_pdata->svc_refclk_req < 0) {
>>>> + if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
>>>> dev_err(dev, "failed to get svc clock-req gpio\n");
>>>> - return arche_pdata->svc_refclk_req;
>>>> + return -ENODEV;
>>>> }
>>>> ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
>>>> "svc-clk-req");
>>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-28 4:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-27 10:52 [PATCH] staging: greybus: Use gpio_is_valid() Arvind Yadav
2018-04-27 12:17 ` [greybus-dev] " Alex Elder
2018-04-27 12:50 ` Arvind Yadav
2018-04-27 13:02 ` Alex Elder
2018-04-28 4:30 ` arvindY
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.