diff mbox series

[v2,2/3] gpio: gpio-regmap: Use devm_add_action()

Message ID e3d3e704804668d1403f3630c181010b34409c8f.1621577204.git.matti.vaittinen@fi.rohmeurope.com
State Superseded
Headers show
Series gpio: gpio-regmap: Support few custom operations | expand

Commit Message

Vaittinen, Matti May 21, 2021, 6:28 a.m. UTC
Slightly simplify the devm_gpio_regmap_register() by using the
devm_add_action().

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
Changelog:
  - New patch at v2

 drivers/gpio/gpio-regmap.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

Comments

Michael Walle May 21, 2021, 8:10 a.m. UTC | #1
Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> Slightly simplify the devm_gpio_regmap_register() by using the
> devm_add_action().

Hm, nice, but what bothers me a bit is that no other subsystem
does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
They also store just one pointer, thus could be simplified in the
same way. What I don't know is if devm_add_action() was intended
to be used this way. So I can't say much for this patch ;)

-michael
Bjørn Mork May 21, 2021, 8:38 a.m. UTC | #2
Michael Walle <michael@walle.cc> writes:

> Am 2021-05-21 08:28, schrieb Matti Vaittinen:
>> Slightly simplify the devm_gpio_regmap_register() by using the
>> devm_add_action().
>
> Hm, nice, but what bothers me a bit is that no other subsystem
> does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> They also store just one pointer, thus could be simplified in the
> same way. What I don't know is if devm_add_action() was intended
> to be used this way. So I can't say much for this patch ;)

There are some examples.  Like:

int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
{
        int ret;

        ret = i2c_add_adapter(adapter);
        if (ret)
                return ret;

        return devm_add_action_or_reset(dev, devm_i2c_del_adapter, adapter);
}


You should probably use the devm_add_action_or_reset() wrapper here too,
catching the unlikely devm_add_action() alloc failure.


Bjørn
Andy Shevchenko May 21, 2021, 10:10 a.m. UTC | #3
On Fri, May 21, 2021 at 12:54 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Slightly simplify the devm_gpio_regmap_register() by using the
> devm_add_action().

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> Changelog:
>   - New patch at v2
>
>  drivers/gpio/gpio-regmap.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index c05370e984b9..4555e59f916e 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -341,9 +341,9 @@ void gpio_regmap_unregister(struct gpio_regmap *gpio)
>  }
>  EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
>
> -static void devm_gpio_regmap_unregister(struct device *dev, void *res)
> +static void devm_gpio_regmap_unregister(void *res)
>  {
> -       gpio_regmap_unregister(*(struct gpio_regmap **)res);
> +       gpio_regmap_unregister(res);
>  }
>
>  /**
> @@ -360,20 +360,12 @@ static void devm_gpio_regmap_unregister(struct device *dev, void *res)
>  struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
>                                               const struct gpio_regmap_config *config)
>  {
> -       struct gpio_regmap **ptr, *gpio;
> -
> -       ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return ERR_PTR(-ENOMEM);
> +       struct gpio_regmap *gpio;
>
>         gpio = gpio_regmap_register(config);
> -       if (!IS_ERR(gpio)) {
> -               *ptr = gpio;
> -               devres_add(dev, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
> +
> +       if (!IS_ERR(gpio))
> +               devm_add_action(dev, devm_gpio_regmap_unregister, gpio);
>
>         return gpio;
>  }
> --
> 2.25.4
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]
Vaittinen, Matti May 21, 2021, 10:30 a.m. UTC | #4
On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> Michael Walle <michael@walle.cc> writes:

> 

> > Am 2021-05-21 08:28, schrieb Matti Vaittinen:

> > > Slightly simplify the devm_gpio_regmap_register() by using the

> > > devm_add_action().

> > 

> > Hm, nice, but what bothers me a bit is that no other subsystem

> > does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.

> > They also store just one pointer, thus could be simplified in the

> > same way. What I don't know is if devm_add_action() was intended

> > to be used this way. So I can't say much for this patch ;)

> 

> There are some examples.  Like:

> 

> int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter

> *adapter)

> {

>         int ret;

> 

>         ret = i2c_add_adapter(adapter);

>         if (ret)

>                 return ret;

> 

>         return devm_add_action_or_reset(dev, devm_i2c_del_adapter,

> adapter);

> }

> 

> 

> You should probably use the devm_add_action_or_reset() wrapper here

> too,

> catching the unlikely devm_add_action() alloc failure.

> 


I was thinking of it but as the gpio registration succeeded I was
thinking that we could go on with it - (which means we can proceed but
the gpio is never released.)

I am not sure how much difference it makes in the case of small alloc
failure ;)

But as it seems I am in any case re-spinning this I can change this to
the devm_add_action_or_reset() and fail the gpio_regmap registration if
alloc fails.

Best Regards
	Matti Vaittinen
Bartosz Golaszewski May 21, 2021, 4:35 p.m. UTC | #5
On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
>
> On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> > Michael Walle <michael@walle.cc> writes:
> >
> > > Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> > > > Slightly simplify the devm_gpio_regmap_register() by using the
> > > > devm_add_action().
> > >
> > > Hm, nice, but what bothers me a bit is that no other subsystem
> > > does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> > > They also store just one pointer, thus could be simplified in the
> > > same way. What I don't know is if devm_add_action() was intended
> > > to be used this way. So I can't say much for this patch ;)
> >
> > There are some examples.  Like:
> >
> > int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter
> > *adapter)
> > {
> >         int ret;
> >
> >         ret = i2c_add_adapter(adapter);
> >         if (ret)
> >                 return ret;
> >
> >         return devm_add_action_or_reset(dev, devm_i2c_del_adapter,
> > adapter);
> > }
> >
> >
> > You should probably use the devm_add_action_or_reset() wrapper here
> > too,
> > catching the unlikely devm_add_action() alloc failure.
> >
>
> I was thinking of it but as the gpio registration succeeded I was
> thinking that we could go on with it - (which means we can proceed but
> the gpio is never released.)
>
> I am not sure how much difference it makes in the case of small alloc
> failure ;)
>
> But as it seems I am in any case re-spinning this I can change this to
> the devm_add_action_or_reset() and fail the gpio_regmap registration if
> alloc fails.
>
> Best Regards
>         Matti Vaittinen

Hi Matti,

Please use the reset variant. We always want to roll-back the changes
done in a function before the failure and propagate the error code.

Bart
Vaittinen, Matti May 24, 2021, 5:01 a.m. UTC | #6
Morning folks!

On Fri, 2021-05-21 at 18:35 +0200, Bartosz Golaszewski wrote:
> On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti

> <Matti.Vaittinen@fi.rohmeurope.com> wrote:

> > 

> > On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:

> > > Michael Walle <michael@walle.cc> writes:

> > > 

> > > > Am 2021-05-21 08:28, schrieb Matti Vaittinen:

> > > > > Slightly simplify the devm_gpio_regmap_register() by using

> > > > > the

> > > > > devm_add_action().

> > > > 

> > > > 

> > > You should probably use the devm_add_action_or_reset() wrapper

> > > here

> > > too,

> > > catching the unlikely devm_add_action() alloc failure.

> > > 

> > 

> > I was thinking of it but as the gpio registration succeeded I was

> > thinking that we could go on with it - (which means we can proceed

> > but

> > the gpio is never released.)

> > 

> > I am not sure how much difference it makes in the case of small

> > alloc

> > failure ;)

> > 

> > But as it seems I am in any case re-spinning this I can change this

> > to

> > the devm_add_action_or_reset() and fail the gpio_regmap

> > registration if

> > alloc fails.

> > 

> > Best Regards

> >         Matti Vaittinen

> 

> Hi Matti,

> 

> Please use the reset variant. We always want to roll-back the changes

> done in a function before the failure and propagate the error code.


Right. I'll do that. I hope to be able to re-spin this today.

Best Regards
	-- Matti
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index c05370e984b9..4555e59f916e 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -341,9 +341,9 @@  void gpio_regmap_unregister(struct gpio_regmap *gpio)
 }
 EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
 
-static void devm_gpio_regmap_unregister(struct device *dev, void *res)
+static void devm_gpio_regmap_unregister(void *res)
 {
-	gpio_regmap_unregister(*(struct gpio_regmap **)res);
+	gpio_regmap_unregister(res);
 }
 
 /**
@@ -360,20 +360,12 @@  static void devm_gpio_regmap_unregister(struct device *dev, void *res)
 struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 					      const struct gpio_regmap_config *config)
 {
-	struct gpio_regmap **ptr, *gpio;
-
-	ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct gpio_regmap *gpio;
 
 	gpio = gpio_regmap_register(config);
-	if (!IS_ERR(gpio)) {
-		*ptr = gpio;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+
+	if (!IS_ERR(gpio))
+		devm_add_action(dev, devm_gpio_regmap_unregister, gpio);
 
 	return gpio;
 }