diff mbox series

[v3,19/24] gpio: remove unnecessary checks from gpiod_to_chip()

Message ID 20240208095920.8035-20-brgl@bgdev.pl
State New
Headers show
Series gpio: rework locking and object life-time control | expand

Commit Message

Bartosz Golaszewski Feb. 8, 2024, 9:59 a.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We don't need to check the gdev pointer in struct gpio_desc - it's
always assigned and never cleared. It's also pointless to check
gdev->chip before we actually serialize access to it.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Feb. 8, 2024, 5:39 p.m. UTC | #1 
On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We don't need to check the gdev pointer in struct gpio_desc - it's
> always assigned and never cleared. It's also pointless to check
> gdev->chip before we actually serialize access to it.

...

>  struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
>  {
> -	if (!desc || !desc->gdev)
> +	if (!desc)

Wondering if it makes sense to align with the below and use IS_ERR_OR_NULL() check.

>  		return NULL;
>  	return desc->gdev->chip;

...

> -	if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> +	if (!desc || IS_ERR(desc))

IS_ERR_OR_NULL()

>  		return -EINVAL;
>  
>  	gc = desc->gdev->chip;
Bartosz Golaszewski Feb. 8, 2024, 7:17 p.m. UTC | #2 
On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We don't need to check the gdev pointer in struct gpio_desc - it's
> > always assigned and never cleared. It's also pointless to check
> > gdev->chip before we actually serialize access to it.
>
> ...
>
> >  struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
> >  {
> > -     if (!desc || !desc->gdev)
> > +     if (!desc)
>
> Wondering if it makes sense to align with the below and use IS_ERR_OR_NULL() check.
>

Nah, it's not supposed to be used with optional GPIOs anyway as it's
not a consumer facing API.

> >               return NULL;
> >       return desc->gdev->chip;
>
> ...
>
> > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > +     if (!desc || IS_ERR(desc))
>
> IS_ERR_OR_NULL()
>

Ah, good point. It's a small nit though so I'll fix it when applying
barring some major objections for the rest.

Bart

> >               return -EINVAL;
> >
> >       gc = desc->gdev->chip;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko Feb. 8, 2024, 7:24 p.m. UTC | #3 
On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:

...

> > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > +     if (!desc || IS_ERR(desc))
> >
> > IS_ERR_OR_NULL()
> 
> Ah, good point. It's a small nit though so I'll fix it when applying
> barring some major objections for the rest.
> 
> > >               return -EINVAL;

thinking more about it, shouldn't we return an actual error to the caller which
is in desc?

     if (!desc)
               return -EINVAL;
     if (IS_ERR(desc))
	return PTR_ERR(desc);

?
Bartosz Golaszewski Feb. 8, 2024, 7:34 p.m. UTC | #4 
On Thu, Feb 8, 2024 at 8:24 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:
>
> ...
>
> > > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > > +     if (!desc || IS_ERR(desc))
> > >
> > > IS_ERR_OR_NULL()
> >
> > Ah, good point. It's a small nit though so I'll fix it when applying
> > barring some major objections for the rest.
> >
> > > >               return -EINVAL;
>
> thinking more about it, shouldn't we return an actual error to the caller which
> is in desc?
>
>      if (!desc)
>                return -EINVAL;
>      if (IS_ERR(desc))
>         return PTR_ERR(desc);
>
> ?

Hmm... maybe but that's out of the scope of this series.

Bart

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko Feb. 9, 2024, 1:59 p.m. UTC | #5 
On Thu, Feb 08, 2024 at 08:34:56PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 8, 2024 at 8:24 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 08:17:14PM +0100, Bartosz Golaszewski wrote:
> > > On Thu, Feb 8, 2024 at 6:39 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Thu, Feb 08, 2024 at 10:59:15AM +0100, Bartosz Golaszewski wrote:

...

> > > > > -     if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
> > > > > +     if (!desc || IS_ERR(desc))
> > > >
> > > > IS_ERR_OR_NULL()
> > >
> > > Ah, good point. It's a small nit though so I'll fix it when applying
> > > barring some major objections for the rest.
> > >
> > > > >               return -EINVAL;
> >
> > thinking more about it, shouldn't we return an actual error to the caller which
> > is in desc?
> >
> >      if (!desc)
> >                return -EINVAL;
> >      if (IS_ERR(desc))
> >         return PTR_ERR(desc);
> >
> > ?
> 
> Hmm... maybe but that's out of the scope of this series.

Yeah, but just think about it.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9be7ec470cc0..140a44dec0be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -214,7 +214,7 @@  EXPORT_SYMBOL_GPL(desc_to_gpio);
  */
 struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
-	if (!desc || !desc->gdev)
+	if (!desc)
 		return NULL;
 	return desc->gdev->chip;
 }
@@ -3505,7 +3505,7 @@  int gpiod_to_irq(const struct gpio_desc *desc)
 	 * requires this function to not return zero on an invalid descriptor
 	 * but rather a negative error number.
 	 */
-	if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
+	if (!desc || IS_ERR(desc))
 		return -EINVAL;
 
 	gc = desc->gdev->chip;