diff mbox

[v2,1/3] gpio: Use __gpiod_request directly

Message ID 1440920686-6892-2-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann Aug. 30, 2015, 7:44 a.m. UTC
There is no reason to find out chip and hwnum to use to request a gpio
and get another gpio descriptor. We already have the descriptor we want
to use so we can directly use it.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Comments

Linus Walleij Sept. 21, 2015, 9:41 p.m. UTC | #1
On Sun, Aug 30, 2015 at 12:44 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

> There is no reason to find out chip and hwnum to use to request a gpio
> and get another gpio descriptor. We already have the descriptor we want
> to use so we can directly use it.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

Patch applied with Uwe's ACK.

Yours,
Linus Walleij
Alexandre Courbot Sept. 23, 2015, 4:25 a.m. UTC | #2
On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> There is no reason to find out chip and hwnum to use to request a gpio
> and get another gpio descriptor. We already have the descriptor we want
> to use so we can directly use it.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/gpio/gpiolib.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 79a0b41ce57b..872fdd3617c1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>               unsigned long lflags, enum gpiod_flags dflags)
>  {
> -       struct gpio_chip *chip;
> -       struct gpio_desc *local_desc;
> -       int hwnum;
>         int status;
>
> -       chip = gpiod_to_chip(desc);
> -       hwnum = gpio_chip_hwgpio(desc);
> -
> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> -       if (IS_ERR(local_desc)) {
> +       status = __gpiod_request(desc, name);
> +       if (status) {
>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> -                      name, chip->label, hwnum);
> -               return PTR_ERR(local_desc);
> +                      name, gpiod_to_chip(desc)->label,
> +                      gpio_chip_hwgpio(desc));
> +               return status;
>         }
>
>         status = gpiod_configure_flags(desc, name, lflags, dflags);
>         if (status < 0) {
>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -                      name, chip->label, hwnum);
> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
>                 gpiochip_free_own_desc(desc);

Mmm I should have reviewed this patch earlier, but what bothers me a
bit is that it breaks the symetry that we had by calling
request_own_desc() and free_own_desc() in the failing case (as well as
in gpiochip_free_hogs). And in the end you still need to call
gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become
slightly harder to understand. Semantically speaking
"request_own_desc()" is exactly the action we want to convey.
__gpiod_request() is more ambiguous.

Note that this is not a reject, I just wanted to stress that "less
code" is not necessarily the same as "easier to read".
Markus Pargmann Sept. 24, 2015, 7:02 a.m. UTC | #3
Hi,

On Wed, Sep 23, 2015 at 01:25:28PM +0900, Alexandre Courbot wrote:
> On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > There is no reason to find out chip and hwnum to use to request a gpio
> > and get another gpio descriptor. We already have the descriptor we want
> > to use so we can directly use it.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/gpio/gpiolib.c | 17 ++++++-----------
> >  1 file changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 79a0b41ce57b..872fdd3617c1 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >  int gpiod_hog(struct gpio_desc *desc, const char *name,
> >               unsigned long lflags, enum gpiod_flags dflags)
> >  {
> > -       struct gpio_chip *chip;
> > -       struct gpio_desc *local_desc;
> > -       int hwnum;
> >         int status;
> >
> > -       chip = gpiod_to_chip(desc);
> > -       hwnum = gpio_chip_hwgpio(desc);
> > -
> > -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> > -       if (IS_ERR(local_desc)) {
> > +       status = __gpiod_request(desc, name);
> > +       if (status) {
> >                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> > -                      name, chip->label, hwnum);
> > -               return PTR_ERR(local_desc);
> > +                      name, gpiod_to_chip(desc)->label,
> > +                      gpio_chip_hwgpio(desc));
> > +               return status;
> >         }
> >
> >         status = gpiod_configure_flags(desc, name, lflags, dflags);
> >         if (status < 0) {
> >                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> > -                      name, chip->label, hwnum);
> > +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> >                 gpiochip_free_own_desc(desc);
> 
> Mmm I should have reviewed this patch earlier, but what bothers me a
> bit is that it breaks the symetry that we had by calling
> request_own_desc() and free_own_desc() in the failing case (as well as
> in gpiochip_free_hogs). And in the end you still need to call
> gpiod_to_chip() so I am not sure what the benefit is.

gpiod_to_chip() is only called for errors after this patch. It just
seemed to me as random reader of the code that using
gpiochip_request_own_desc() could be simplified by using __gpiod_request().

> 
> Sure, the code is less verbose, but at the same time it has become
> slightly harder to understand. Semantically speaking
> "request_own_desc()" is exactly the action we want to convey.
> __gpiod_request() is more ambiguous.

At least for me this is slightly better to read as it removes some
unnecessary lines. But that is probably subjective and depends on the way
someone reads code.

Best Regards,

Markus

> 
> Note that this is not a reject, I just wanted to stress that "less
> code" is not necessarily the same as "easier to read".
Linus Walleij Sept. 24, 2015, 5:49 p.m. UTC | #4
On Tue, Sep 22, 2015 at 9:25 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
>> There is no reason to find out chip and hwnum to use to request a gpio
>> and get another gpio descriptor. We already have the descriptor we want
>> to use so we can directly use it.
>>
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>> ---
>>  drivers/gpio/gpiolib.c | 17 ++++++-----------
>>  1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 79a0b41ce57b..872fdd3617c1 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
>>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>>               unsigned long lflags, enum gpiod_flags dflags)
>>  {
>> -       struct gpio_chip *chip;
>> -       struct gpio_desc *local_desc;
>> -       int hwnum;
>>         int status;
>>
>> -       chip = gpiod_to_chip(desc);
>> -       hwnum = gpio_chip_hwgpio(desc);
>> -
>> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>> -       if (IS_ERR(local_desc)) {
>> +       status = __gpiod_request(desc, name);
>> +       if (status) {
>>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
>> -                      name, chip->label, hwnum);
>> -               return PTR_ERR(local_desc);
>> +                      name, gpiod_to_chip(desc)->label,
>> +                      gpio_chip_hwgpio(desc));
>> +               return status;
>>         }
>>
>>         status = gpiod_configure_flags(desc, name, lflags, dflags);
>>         if (status < 0) {
>>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
>> -                      name, chip->label, hwnum);
>> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
>>                 gpiochip_free_own_desc(desc);
>
> Mmm I should have reviewed this patch earlier, but what bothers me a
> bit is that it breaks the symetry that we had by calling
> request_own_desc() and free_own_desc() in the failing case (as well as
> in gpiochip_free_hogs). And in the end you still need to call
> gpiod_to_chip() so I am not sure what the benefit is.
>
> Sure, the code is less verbose, but at the same time it has become
> slightly harder to understand. Semantically speaking
> "request_own_desc()" is exactly the action we want to convey.
> __gpiod_request() is more ambiguous.
>
> Note that this is not a reject, I just wanted to stress that "less
> code" is not necessarily the same as "easier to read".

OK I dropped this patch for now.

Markus can you live without this patch for 2/3 and 3/3?

Yours,
Linus Walleij
Markus Pargmann Sept. 27, 2015, 2:32 p.m. UTC | #5
Hi,

On Thu, Sep 24, 2015 at 10:49:57AM -0700, Linus Walleij wrote:
> On Tue, Sep 22, 2015 at 9:25 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Sun, Aug 30, 2015 at 4:44 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> >> There is no reason to find out chip and hwnum to use to request a gpio
> >> and get another gpio descriptor. We already have the descriptor we want
> >> to use so we can directly use it.
> >>
> >> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> >> ---
> >>  drivers/gpio/gpiolib.c | 17 ++++++-----------
> >>  1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 79a0b41ce57b..872fdd3617c1 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >>  int gpiod_hog(struct gpio_desc *desc, const char *name,
> >>               unsigned long lflags, enum gpiod_flags dflags)
> >>  {
> >> -       struct gpio_chip *chip;
> >> -       struct gpio_desc *local_desc;
> >> -       int hwnum;
> >>         int status;
> >>
> >> -       chip = gpiod_to_chip(desc);
> >> -       hwnum = gpio_chip_hwgpio(desc);
> >> -
> >> -       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
> >> -       if (IS_ERR(local_desc)) {
> >> +       status = __gpiod_request(desc, name);
> >> +       if (status) {
> >>                 pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> >> -                      name, chip->label, hwnum);
> >> -               return PTR_ERR(local_desc);
> >> +                      name, gpiod_to_chip(desc)->label,
> >> +                      gpio_chip_hwgpio(desc));
> >> +               return status;
> >>         }
> >>
> >>         status = gpiod_configure_flags(desc, name, lflags, dflags);
> >>         if (status < 0) {
> >>                 pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> >> -                      name, chip->label, hwnum);
> >> +                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> >>                 gpiochip_free_own_desc(desc);
> >
> > Mmm I should have reviewed this patch earlier, but what bothers me a
> > bit is that it breaks the symetry that we had by calling
> > request_own_desc() and free_own_desc() in the failing case (as well as
> > in gpiochip_free_hogs). And in the end you still need to call
> > gpiod_to_chip() so I am not sure what the benefit is.
> >
> > Sure, the code is less verbose, but at the same time it has become
> > slightly harder to understand. Semantically speaking
> > "request_own_desc()" is exactly the action we want to convey.
> > __gpiod_request() is more ambiguous.
> >
> > Note that this is not a reject, I just wanted to stress that "less
> > code" is not necessarily the same as "easier to read".
> 
> OK I dropped this patch for now.
> 
> Markus can you live without this patch for 2/3 and 3/3?

Yes, that's fine. I will remove it and rebase the others.

Best Regards,

Markus

> 
> Yours,
> Linus Walleij
>
diff mbox

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 79a0b41ce57b..872fdd3617c1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2189,25 +2189,20 @@  EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 	      unsigned long lflags, enum gpiod_flags dflags)
 {
-	struct gpio_chip *chip;
-	struct gpio_desc *local_desc;
-	int hwnum;
 	int status;
 
-	chip = gpiod_to_chip(desc);
-	hwnum = gpio_chip_hwgpio(desc);
-
-	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-	if (IS_ERR(local_desc)) {
+	status = __gpiod_request(desc, name);
+	if (status) {
 		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
-		return PTR_ERR(local_desc);
+		       name, gpiod_to_chip(desc)->label,
+		       gpio_chip_hwgpio(desc));
+		return status;
 	}
 
 	status = gpiod_configure_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
+		       name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
 		gpiochip_free_own_desc(desc);
 		return status;
 	}