| Message ID | 20201110093921.3731-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: devres: shrink devm_gpiochip_add_data_with_key() | expand |
On Tue, Nov 10, 2020 at 10:39 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > If all we want to manage is a single pointer, there's no need to > manually allocate and add a new devres. We can simply use > devm_add_action_or_reset() and shrink the code by a good bit. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Sweet! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I suppose I will get this with a pull request. Yours, Linus Walleij
On Tue, Nov 10, 2020 at 11:42 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > If all we want to manage is a single pointer, there's no need to > manually allocate and add a new devres. We can simply use > devm_add_action_or_reset() and shrink the code by a good bit. Yes, it is possible to convert all one-function-based devm_*() wrappers to use this approach. The problem is, it will call the release() function on error which is new (and probably undesired) behaviour. I suppose you meant devm_add_action() here. -- With Best Regards, Andy Shevchenko
On Wed, Nov 11, 2020 at 5:02 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Nov 10, 2020 at 11:42 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > If all we want to manage is a single pointer, there's no need to > > manually allocate and add a new devres. We can simply use > > devm_add_action_or_reset() and shrink the code by a good bit. > > Yes, it is possible to convert all one-function-based devm_*() > wrappers to use this approach. > > The problem is, it will call the release() function on error which is > new (and probably undesired) behaviour. > I suppose you meant devm_add_action() here. Ah, now it seems I got it. You need to release the chip in case if devm_add_action() fail. Dunno if devm_add_action() can somehow change the logic to be clearer here... -- With Best Regards, Andy Shevchenko
On Wed, Nov 11, 2020 at 4:04 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Nov 11, 2020 at 5:02 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Nov 10, 2020 at 11:42 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > If all we want to manage is a single pointer, there's no need to > > > manually allocate and add a new devres. We can simply use > > > devm_add_action_or_reset() and shrink the code by a good bit. > > > > Yes, it is possible to convert all one-function-based devm_*() > > wrappers to use this approach. > > > > The problem is, it will call the release() function on error which is > > new (and probably undesired) behaviour. > > I suppose you meant devm_add_action() here. > > Ah, now it seems I got it. You need to release the chip in case if > devm_add_action() fail. > Dunno if devm_add_action() can somehow change the logic to be clearer here... > devm_add_action_or_reset() is correct here - it undos the previous chip registration on error. Bartosz
diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c index 7dbce4c4ebdf..0a292dd3e2a6 100644 --- a/drivers/gpio/gpiolib-devres.c +++ b/drivers/gpio/gpiolib-devres.c @@ -479,9 +479,9 @@ void devm_gpio_free(struct device *dev, unsigned int gpio) } EXPORT_SYMBOL_GPL(devm_gpio_free); -static void devm_gpio_chip_release(struct device *dev, void *res) +static void devm_gpio_chip_release(void *data) { - struct gpio_chip *gc = *(struct gpio_chip **)res; + struct gpio_chip *gc = data; gpiochip_remove(gc); } @@ -507,23 +507,12 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, vo struct lock_class_key *lock_key, struct lock_class_key *request_key) { - struct gpio_chip **ptr; int ret; - ptr = devres_alloc(devm_gpio_chip_release, sizeof(*ptr), - GFP_KERNEL); - if (!ptr) - return -ENOMEM; - ret = gpiochip_add_data_with_key(gc, data, lock_key, request_key); - if (ret < 0) { - devres_free(ptr); + if (ret < 0) return ret; - } - *ptr = gc; - devres_add(dev, ptr); - - return 0; + return devm_add_action_or_reset(dev, devm_gpio_chip_release, gc); } EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key);