| Message ID | e3d3e704804668d1403f3630c181010b34409c8f.1621577204.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-regmap: Support few custom operations | expand |
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 <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
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 =]
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
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
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 --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; }
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(-)