Skip to contentSkip to conversation

gpiolib: devres: shrink devm_gpiochip_add_data_with_key()

PATCH In review Bartosz Golaszewski 3 participants 5 messages Nov 10, 2020 – Nov 12, 2020 GPIO

lore
Share & tools
lore

Bartosz Golaszewski

author
#0 lore raw

PATCH 1/1 gpiolib: devres: shrink devm_gpiochip_add_data_with_key()

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>
---
 drivers/gpio/gpiolib-devres.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)
drivers/gpio/gpiolib-devres.c +4 -15
index 7dbce4c4ebdf..0a292dd3e2a6 100644
@@ -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);
Signature · 1 line
2.29.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

Sweet!

Reviewed-by Linus Walleij <[email protected]>

I suppose I will get this with a pull request.

Yours,
Linus Walleij

#2 lore raw
In reply to Bartosz Golaszewski
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#3 lore raw
Quoted 9 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 6 lines

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...

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#4 lore raw
Quoted 15 lines · Andy Shevchenko
Quoted 12 lines · Andy Shevchenko
Quoted 6 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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.

Quoted 8 lines
Quoted 3 lines

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