[PATCH v1 0/4] gpiolib: finish conversion to devm_*_action*() APIs

[PATCH v1 0/4] gpiolib: finish conversion to devm_*_action*() APIs

[Andy Shevchenko]

GPIOLIB has some open coded stuff that can be folded to the devm_*_action*()
calls. This mini-series is for that. The necessary prerequisites are here
as well, namely:
1) moving the respective APIs to the devres.h;
2) adding a couple of simple helpers that GPIOLIB will rely on;
3) finishing the GPIOLIB conversion to the device managed action APIs.

The series is based on another series that's available via immutable tag
devres-iio-input-pinctrl-v6.15 [1]. The idea is to route this via GPIOLIB
tree (or Intel GPIO for the starter) with an immutable tag for the device
core and others if needed. Please, review and acknowledge.

Link: https://lore.kernel.org/r/Z7cqCaME4LxTTBn6@black.fi.intel.com [1]
Cc: Raag Jadav <raag.jadav@intel.com>

Andy Shevchenko (4):
  devres: Move devm_*_action*() APIs to devres.h
  devres: Add devm_is_action_added() helper
  devres: Add devm_remove_action_optional() helper
  gpiolib: devres: Finish the conversion to use devm_add_action()

 drivers/base/devres.c         | 11 ++++
 drivers/gpio/gpiolib-devres.c | 94 ++++++++++-------------------------
 include/linux/device.h        | 38 --------------
 include/linux/device/devres.h | 54 ++++++++++++++++++++
 4 files changed, 90 insertions(+), 107 deletions(-)


base-commit: 9deb15de8ca27cf9cba0d2bac53bbe37c836591b
-- 
2.45.1.3035.g276e886db78b

[PATCH v1 1/4] devres: Move devm_*_action*() APIs to devres.h

[Andy Shevchenko]

We have a newly created header linux/device/devres.h that gathers
device managed APIs, so users won't need to include entire device.h
for only these ones. Move devm_*_action*() APIs to devres.h as well.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/device.h        | 38 ----------------------------------
 include/linux/device/devres.h | 39 +++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 78ca7fd0e625..d6341a05e4fb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -281,44 +281,6 @@ int __must_check device_create_bin_file(struct device *dev,
 void device_remove_bin_file(struct device *dev,
 			    const struct bin_attribute *attr);
 
-/* allows to add/remove a custom action to devres stack */
-int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
-
-/**
- * devm_remove_action() - removes previously added custom action
- * @dev: Device that owns the action
- * @action: Function implementing the action
- * @data: Pointer to data passed to @action implementation
- *
- * Removes instance of @action previously added by devm_add_action().
- * Both action and data should match one of the existing entries.
- */
-static inline
-void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
-{
-	WARN_ON(devm_remove_action_nowarn(dev, action, data));
-}
-
-void devm_release_action(struct device *dev, void (*action)(void *), void *data);
-
-int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
-#define devm_add_action(dev, action, data) \
-	__devm_add_action(dev, action, data, #action)
-
-static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(void *),
-					     void *data, const char *name)
-{
-	int ret;
-
-	ret = __devm_add_action(dev, action, data, name);
-	if (ret)
-		action(data);
-
-	return ret;
-}
-#define devm_add_action_or_reset(dev, action, data) \
-	__devm_add_action_or_reset(dev, action, data, #action)
-
 /**
  * devm_alloc_percpu - Resource-managed alloc_percpu
  * @dev: Device to allocate per-cpu memory for
diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index 9b49f9915850..9cd1787ef28e 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -8,6 +8,7 @@
 #include <linux/overflow.h>
 #include <linux/stdarg.h>
 #include <linux/types.h>
+#include <asm/bug.h>
 
 struct device;
 struct device_node;
@@ -126,4 +127,42 @@ void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int in
 
 #endif
 
+/* allows to add/remove a custom action to devres stack */
+int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
+
+/**
+ * devm_remove_action() - removes previously added custom action
+ * @dev: Device that owns the action
+ * @action: Function implementing the action
+ * @data: Pointer to data passed to @action implementation
+ *
+ * Removes instance of @action previously added by devm_add_action().
+ * Both action and data should match one of the existing entries.
+ */
+static inline
+void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
+{
+	WARN_ON(devm_remove_action_nowarn(dev, action, data));
+}
+
+void devm_release_action(struct device *dev, void (*action)(void *), void *data);
+
+int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
+#define devm_add_action(dev, action, data) \
+	__devm_add_action(dev, action, data, #action)
+
+static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(void *),
+					     void *data, const char *name)
+{
+	int ret;
+
+	ret = __devm_add_action(dev, action, data, name);
+	if (ret)
+		action(data);
+
+	return ret;
+}
+#define devm_add_action_or_reset(dev, action, data) \
+	__devm_add_action_or_reset(dev, action, data, #action)
+
 #endif /* _DEVICE_DEVRES_H_ */
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 1/4] devres: Move devm_*_action*() APIs to devres.h

[Raag Jadav]

On Thu, Feb 20, 2025 at 03:44:57PM +0200, Andy Shevchenko wrote:
> We have a newly created header linux/device/devres.h that gathers
> device managed APIs, so users won't need to include entire device.h
> for only these ones. Move devm_*_action*() APIs to devres.h as well.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Raag Jadav <raag.jadav@intel.com>

[PATCH v1 2/4] devres: Add devm_is_action_added() helper

[Andy Shevchenko]

In some code we would like to know if the action in device managed resources
was added by devm_add_action() family of calls. Introduce a helper for that.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/devres.c         | 11 +++++++++++
 include/linux/device/devres.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 93e7779ef21e..7c2babfa9396 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -749,6 +749,17 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
 }
 EXPORT_SYMBOL_GPL(__devm_add_action);
 
+bool devm_is_action_added(struct device *dev, void (*action)(void *), void *data)
+{
+	struct action_devres devres = {
+		.data = data,
+		.action = action,
+	};
+
+	return devres_find(dev, devm_action_release, devm_action_match, &devres);
+}
+EXPORT_SYMBOL_GPL(devm_is_action_added);
+
 /**
  * devm_remove_action_nowarn() - removes previously added custom action
  * @dev: Device that owns the action
diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index 9cd1787ef28e..ae696d10faff 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -165,4 +165,6 @@ static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(
 #define devm_add_action_or_reset(dev, action, data) \
 	__devm_add_action_or_reset(dev, action, data, #action)
 
+bool devm_is_action_added(struct device *dev, void (*action)(void *), void *data);
+
 #endif /* _DEVICE_DEVRES_H_ */
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 2/4] devres: Add devm_is_action_added() helper

[Raag Jadav]

On Thu, Feb 20, 2025 at 03:44:58PM +0200, Andy Shevchenko wrote:
> In some code we would like to know if the action in device managed resources
> was added by devm_add_action() family of calls. Introduce a helper for that.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Raag Jadav <raag.jadav@intel.com>

[PATCH v1 3/4] devres: Add devm_remove_action_optional() helper

[Andy Shevchenko]

Add a new helper to remove an action that was added via devm_add_action()
family of calls, but not warn in the cases where action wasn't found since
it is optional and wasn't even added.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/device/devres.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index ae696d10faff..4daebbf7f227 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -145,6 +145,19 @@ void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
 	WARN_ON(devm_remove_action_nowarn(dev, action, data));
 }
 
+/* Same as devm_remove_action(), but doesn't WARN() if action wasn't added before */
+static inline
+void devm_remove_action_optional(struct device *dev, void (*action)(void *), void *data)
+{
+	int ret;
+
+	ret = devm_remove_action_nowarn(dev, action, data);
+	if (ret == -ENOENT)
+		return;
+
+	WARN_ON(ret);
+}
+
 void devm_release_action(struct device *dev, void (*action)(void *), void *data);
 
 int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 3/4] devres: Add devm_remove_action_optional() helper

[Raag Jadav]

On Thu, Feb 20, 2025 at 03:44:59PM +0200, Andy Shevchenko wrote:
> Add a new helper to remove an action that was added via devm_add_action()
> family of calls, but not warn in the cases where action wasn't found since
> it is optional and wasn't even added.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/device/devres.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
> index ae696d10faff..4daebbf7f227 100644
> --- a/include/linux/device/devres.h
> +++ b/include/linux/device/devres.h
> @@ -145,6 +145,19 @@ void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
>  	WARN_ON(devm_remove_action_nowarn(dev, action, data));
>  }
>  
> +/* Same as devm_remove_action(), but doesn't WARN() if action wasn't added before */
> +static inline
> +void devm_remove_action_optional(struct device *dev, void (*action)(void *), void *data)
> +{
> +	int ret;
> +
> +	ret = devm_remove_action_nowarn(dev, action, data);
> +	if (ret == -ENOENT)
> +		return;
> +
> +	WARN_ON(ret);
> +}

Trying to wrap my head around this one, can't the user simply do

	if (devm_is_action_added())
		devm_remove_action/_nowarn();

Raag

Re: [PATCH v1 3/4] devres: Add devm_remove_action_optional() helper

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 05:30:07PM +0200, Raag Jadav wrote:
> On Thu, Feb 20, 2025 at 03:44:59PM +0200, Andy Shevchenko wrote:

...

> > +/* Same as devm_remove_action(), but doesn't WARN() if action wasn't added before */
> > +static inline
> > +void devm_remove_action_optional(struct device *dev, void (*action)(void *), void *data)
> > +{
> > +	int ret;
> > +
> > +	ret = devm_remove_action_nowarn(dev, action, data);
> > +	if (ret == -ENOENT)
> > +		return;
> > +
> > +	WARN_ON(ret);
> > +}
> 
> Trying to wrap my head around this one, can't the user simply do
> 
> 	if (devm_is_action_added())
> 		devm_remove_action/_nowarn();

Hmm... Actually it sounds like a good point. I will check
(and I like the idea of dropping this patch).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] devres: Add devm_remove_action_optional() helper

[Raag Jadav]

On Thu, Feb 20, 2025 at 05:40:24PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 05:30:07PM +0200, Raag Jadav wrote:
> > On Thu, Feb 20, 2025 at 03:44:59PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > +/* Same as devm_remove_action(), but doesn't WARN() if action wasn't added before */
> > > +static inline
> > > +void devm_remove_action_optional(struct device *dev, void (*action)(void *), void *data)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = devm_remove_action_nowarn(dev, action, data);
> > > +	if (ret == -ENOENT)
> > > +		return;
> > > +
> > > +	WARN_ON(ret);
> > > +}
> > 
> > Trying to wrap my head around this one, can't the user simply do
> > 
> > 	if (devm_is_action_added())
> > 		devm_remove_action/_nowarn();
> 
> Hmm... Actually it sounds like a good point. I will check
> (and I like the idea of dropping this patch).

And perhaps

s/devm_is_action_added/devm_action_is_added

But whichever you think _is best_ ;)

Raag

Re: [PATCH v1 3/4] devres: Add devm_remove_action_optional() helper

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 05:51:42PM +0200, Raag Jadav wrote:
> On Thu, Feb 20, 2025 at 05:40:24PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2025 at 05:30:07PM +0200, Raag Jadav wrote:
> > > On Thu, Feb 20, 2025 at 03:44:59PM +0200, Andy Shevchenko wrote:

...

> > > > +/* Same as devm_remove_action(), but doesn't WARN() if action wasn't added before */
> > > > +static inline
> > > > +void devm_remove_action_optional(struct device *dev, void (*action)(void *), void *data)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = devm_remove_action_nowarn(dev, action, data);
> > > > +	if (ret == -ENOENT)
> > > > +		return;
> > > > +
> > > > +	WARN_ON(ret);
> > > > +}
> > > 
> > > Trying to wrap my head around this one, can't the user simply do
> > > 
> > > 	if (devm_is_action_added())
> > > 		devm_remove_action/_nowarn();
> > 
> > Hmm... Actually it sounds like a good point. I will check
> > (and I like the idea of dropping this patch).
> 
> And perhaps
> 
> s/devm_is_action_added/devm_action_is_added
> 
> But whichever you think _is best_ ;)

I thought about that and that's why I would like to stick to the my variant.

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/4] gpiolib: devres: Finish the conversion to use devm_add_action()

[Andy Shevchenko]

With new recently added helpers we can complete the conversion
of the gpiolib code to use devm_add_action() in all suitable cases.
So do this.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-devres.c | 94 ++++++++++-------------------------
 1 file changed, 25 insertions(+), 69 deletions(-)

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 08205f355ceb..4338e21c9123 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -19,32 +19,14 @@
 struct fwnode_handle;
 struct lock_class_key;
 
-static void devm_gpiod_release(struct device *dev, void *res)
+static void devm_gpiod_release(void *desc)
 {
-	struct gpio_desc **desc = res;
-
-	gpiod_put(*desc);
+	gpiod_put(desc);
 }
 
-static int devm_gpiod_match(struct device *dev, void *res, void *data)
+static void devm_gpiod_release_array(void *descs)
 {
-	struct gpio_desc **this = res, **gpio = data;
-
-	return *this == *gpio;
-}
-
-static void devm_gpiod_release_array(struct device *dev, void *res)
-{
-	struct gpio_descs **descs = res;
-
-	gpiod_put_array(*descs);
-}
-
-static int devm_gpiod_match_array(struct device *dev, void *res, void *data)
-{
-	struct gpio_descs **this = res, **gpios = data;
-
-	return *this == *gpios;
+	gpiod_put_array(descs);
 }
 
 /**
@@ -114,8 +96,8 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 						    unsigned int idx,
 						    enum gpiod_flags flags)
 {
-	struct gpio_desc **dr;
 	struct gpio_desc *desc;
+	int ret;
 
 	desc = gpiod_get_index(dev, con_id, idx, flags);
 	if (IS_ERR(desc))
@@ -126,23 +108,16 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 	 * already under resource management by this device.
 	 */
 	if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
-		struct devres *dres;
+		bool dres;
 
-		dres = devres_find(dev, devm_gpiod_release,
-				   devm_gpiod_match, &desc);
+		dres = devm_is_action_added(dev, devm_gpiod_release, desc);
 		if (dres)
 			return desc;
 	}
 
-	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
-			  GFP_KERNEL);
-	if (!dr) {
-		gpiod_put(desc);
-		return ERR_PTR(-ENOMEM);
-	}
-
-	*dr = desc;
-	devres_add(dev, dr);
+	ret = devm_add_action_or_reset(dev, devm_gpiod_release, desc);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return desc;
 }
@@ -171,22 +146,16 @@ struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
 					      enum gpiod_flags flags,
 					      const char *label)
 {
-	struct gpio_desc **dr;
 	struct gpio_desc *desc;
-
-	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
-			  GFP_KERNEL);
-	if (!dr)
-		return ERR_PTR(-ENOMEM);
+	int ret;
 
 	desc = gpiod_find_and_request(dev, fwnode, con_id, index, flags, label, false);
-	if (IS_ERR(desc)) {
-		devres_free(dr);
+	if (IS_ERR(desc))
 		return desc;
-	}
 
-	*dr = desc;
-	devres_add(dev, dr);
+	ret = devm_add_action_or_reset(dev, devm_gpiod_release, desc);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return desc;
 }
@@ -244,22 +213,16 @@ struct gpio_descs *__must_check devm_gpiod_get_array(struct device *dev,
 						     const char *con_id,
 						     enum gpiod_flags flags)
 {
-	struct gpio_descs **dr;
 	struct gpio_descs *descs;
-
-	dr = devres_alloc(devm_gpiod_release_array,
-			  sizeof(struct gpio_descs *), GFP_KERNEL);
-	if (!dr)
-		return ERR_PTR(-ENOMEM);
+	int ret;
 
 	descs = gpiod_get_array(dev, con_id, flags);
-	if (IS_ERR(descs)) {
-		devres_free(dr);
+	if (IS_ERR(descs))
 		return descs;
-	}
 
-	*dr = descs;
-	devres_add(dev, dr);
+	ret = devm_add_action_or_reset(dev, devm_gpiod_release_array, descs);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return descs;
 }
@@ -307,8 +270,7 @@ EXPORT_SYMBOL_GPL(devm_gpiod_get_array_optional);
  */
 void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
 {
-	WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
-		&desc));
+	devm_release_action(dev, devm_gpiod_release, desc);
 }
 EXPORT_SYMBOL_GPL(devm_gpiod_put);
 
@@ -324,21 +286,16 @@ EXPORT_SYMBOL_GPL(devm_gpiod_put);
 
 void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
 {
-	int ret;
-
 	if (IS_ERR_OR_NULL(desc))
 		return;
-	ret = devres_destroy(dev, devm_gpiod_release,
-			     devm_gpiod_match, &desc);
+
 	/*
 	 * If the GPIO descriptor is requested as nonexclusive, we
 	 * may call this function several times on the same descriptor
 	 * so it is OK if devres_destroy() returns -ENOENT.
+	 * Anything else we should warn about.
 	 */
-	if (ret == -ENOENT)
-		return;
-	/* Anything else we should warn about */
-	WARN_ON(ret);
+	devm_remove_action_optional(dev, devm_gpiod_release, desc);
 }
 EXPORT_SYMBOL_GPL(devm_gpiod_unhinge);
 
@@ -353,8 +310,7 @@ EXPORT_SYMBOL_GPL(devm_gpiod_unhinge);
  */
 void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs)
 {
-	WARN_ON(devres_release(dev, devm_gpiod_release_array,
-			       devm_gpiod_match_array, &descs));
+	devm_remove_action(dev, devm_gpiod_release_array, descs);
 }
 EXPORT_SYMBOL_GPL(devm_gpiod_put_array);
 
-- 
2.45.1.3035.g276e886db78b

Re: [PATCH v1 0/4] gpiolib: finish conversion to devm_*_action*() APIs

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 03:44:56PM +0200, Andy Shevchenko wrote:
> GPIOLIB has some open coded stuff that can be folded to the devm_*_action*()
> calls. This mini-series is for that. The necessary prerequisites are here
> as well, namely:
> 1) moving the respective APIs to the devres.h;
> 2) adding a couple of simple helpers that GPIOLIB will rely on;
> 3) finishing the GPIOLIB conversion to the device managed action APIs.
> 
> The series is based on another series that's available via immutable tag
> devres-iio-input-pinctrl-v6.15 [1]. The idea is to route this via GPIOLIB
> tree (or Intel GPIO for the starter) with an immutable tag for the device
> core and others if needed. Please, review and acknowledge.

Please, don't waste time on this, I have just sent a v2 which is simpler:
https://lore.kernel.org/r/20250220162238.2738038-1-andriy.shevchenko@linux.intel.com

-- 
With Best Regards,
Andy Shevchenko