Slightly simplify the devm_gpio_regmap_register() by using the
devm_add_action().
Signed-off-by: Matti Vaittinen <[email protected]>
---
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 =]
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
Michael Walle <[email protected]> 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
On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> Michael Walle <[email protected]> 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
On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti
<[email protected]> wrote:
>
>
> On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> > Michael Walle <[email protected]> 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
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
> <[email protected]> wrote:
> >
> > On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> > > Michael Walle <[email protected]> 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