[PATCH v2 0/8] gpiolib: Some cleanups

[PATCH v2 0/8] gpiolib: Some cleanups

[Andy Shevchenko]

Just a three groups of cleanups (some of them may be dependent to the previous
ones compile and logically wise, but I am only aware about couple of patches,
i.e. 3 & 4).

In v2:
- split one patch to two (Linus)
- added tags (Linus)

Andy Shevchenko (8):
  gpiolib: Make taking gpio_lookup_lock consistent
  gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex
  gpiolib: Print actual error when descriptor contains an error pointer
  gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"
  gpiolib: Move validate_desc() and Co upper in the code
  gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used
  gpiolib: Reuse return variable in gpiod_to_irq()
  gpiolib: Remove redundant assignment of return variable

 drivers/gpio/gpiolib.c | 123 ++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 70 deletions(-)

-- 
2.47.2

[PATCH v2 1/8] gpiolib: Make taking gpio_lookup_lock consistent

[Andy Shevchenko]

There are two ways to take a lock: plain call to the mutex_lock()
or using guard()() / scoped_guard(). The driver inconsistently uses
both. Make taking gpio_lookup_lock consistent.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b8993d2d31e1..7d9cc1a9c535 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4363,12 +4363,10 @@ void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n)
 {
 	unsigned int i;
 
-	mutex_lock(&gpio_lookup_lock);
+	guard(mutex)(&gpio_lookup_lock);
 
 	for (i = 0; i < n; i++)
 		list_add_tail(&tables[i]->list, &gpio_lookup_list);
-
-	mutex_unlock(&gpio_lookup_lock);
 }
 
 /**
@@ -4427,11 +4425,9 @@ void gpiod_remove_lookup_table(struct gpiod_lookup_table *table)
 	if (!table)
 		return;
 
-	mutex_lock(&gpio_lookup_lock);
+	guard(mutex)(&gpio_lookup_lock);
 
 	list_del(&table->list);
-
-	mutex_unlock(&gpio_lookup_lock);
 }
 EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table);
 
-- 
2.47.2

[PATCH v2 2/8] gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex

[Andy Shevchenko]

The driver uses guard()()/scoped_guard() for the rest of the synchronisation
calls. Convert to use the same for gpio_machine_hogs_mutex.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7d9cc1a9c535..d6a161dd737d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -888,14 +888,12 @@ static void machine_gpiochip_add(struct gpio_chip *gc)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
 
 	list_for_each_entry(hog, &gpio_machine_hogs, list) {
 		if (!strcmp(gc->label, hog->chip_label))
 			gpiochip_machine_hog(gc, hog);
 	}
-
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 
 static void gpiochip_setup_devs(void)
@@ -4439,7 +4437,7 @@ void gpiod_add_hogs(struct gpiod_hog *hogs)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
 
 	for (hog = &hogs[0]; hog->chip_label; hog++) {
 		list_add_tail(&hog->list, &gpio_machine_hogs);
@@ -4453,8 +4451,6 @@ void gpiod_add_hogs(struct gpiod_hog *hogs)
 		if (gdev)
 			gpiochip_machine_hog(gpio_device_get_chip(gdev), hog);
 	}
-
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 EXPORT_SYMBOL_GPL(gpiod_add_hogs);
 
@@ -4462,10 +4458,10 @@ void gpiod_remove_hogs(struct gpiod_hog *hogs)
 {
 	struct gpiod_hog *hog;
 
-	mutex_lock(&gpio_machine_hogs_mutex);
+	guard(mutex)(&gpio_machine_hogs_mutex);
+
 	for (hog = &hogs[0]; hog->chip_label; hog++)
 		list_del(&hog->list);
-	mutex_unlock(&gpio_machine_hogs_mutex);
 }
 EXPORT_SYMBOL_GPL(gpiod_remove_hogs);
 
-- 
2.47.2

[PATCH v2 3/8] gpiolib: Print actual error when descriptor contains an error pointer

[Andy Shevchenko]

Print the actual error when descriptor contains an error pointer.
This might help debugging those rare cases.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d6a161dd737d..7f8e8c33207e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2443,7 +2443,7 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
 		return 0;
 
 	if (IS_ERR(desc)) {
-		pr_warn("%s: invalid GPIO (errorpointer)\n", func);
+		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
 		return PTR_ERR(desc);
 	}
 
-- 
2.47.2

[PATCH v2 4/8] gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"

[Andy Shevchenko]

No need to double check the pointer for NULL since gpiod_free()
is using VALIDATE_DESC_VOID() which simply returns in that case.

This reverts commit 1d7765ba15aca68f3bc52f59434c1c34855bbb54.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7f8e8c33207e..4485dc8b1e42 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -5140,8 +5140,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_array_optional);
  */
 void gpiod_put(struct gpio_desc *desc)
 {
-	if (desc)
-		gpiod_free(desc);
+	gpiod_free(desc);
 }
 EXPORT_SYMBOL_GPL(gpiod_put);
 
-- 
2.47.2

[PATCH v2 5/8] gpiolib: Move validate_desc() and Co upper in the code

[Andy Shevchenko]

Move validate_desc() and Co upper in the code to be able to use
in the further changes.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 62 +++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4485dc8b1e42..ad2e68f66500 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -356,6 +356,37 @@ static int gpiochip_find_base_unlocked(u16 ngpio)
 	}
 }
 
+/*
+ * This descriptor validation needs to be inserted verbatim into each
+ * function taking a descriptor, so we need to use a preprocessor
+ * macro to avoid endless duplication. If the desc is NULL it is an
+ * optional GPIO and calls should just bail out.
+ */
+static int validate_desc(const struct gpio_desc *desc, const char *func)
+{
+	if (!desc)
+		return 0;
+
+	if (IS_ERR(desc)) {
+		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
+		return PTR_ERR(desc);
+	}
+
+	return 1;
+}
+
+#define VALIDATE_DESC(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid <= 0) \
+		return __valid; \
+	} while (0)
+
+#define VALIDATE_DESC_VOID(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid <= 0) \
+		return; \
+	} while (0)
+
 static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	int ret;
@@ -2431,37 +2462,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	return ret;
 }
 
-/*
- * This descriptor validation needs to be inserted verbatim into each
- * function taking a descriptor, so we need to use a preprocessor
- * macro to avoid endless duplication. If the desc is NULL it is an
- * optional GPIO and calls should just bail out.
- */
-static int validate_desc(const struct gpio_desc *desc, const char *func)
-{
-	if (!desc)
-		return 0;
-
-	if (IS_ERR(desc)) {
-		pr_warn("%s: invalid GPIO (errorpointer: %pe)\n", func, desc);
-		return PTR_ERR(desc);
-	}
-
-	return 1;
-}
-
-#define VALIDATE_DESC(desc) do { \
-	int __valid = validate_desc(desc, __func__); \
-	if (__valid <= 0) \
-		return __valid; \
-	} while (0)
-
-#define VALIDATE_DESC_VOID(desc) do { \
-	int __valid = validate_desc(desc, __func__); \
-	if (__valid <= 0) \
-		return; \
-	} while (0)
-
 int gpiod_request(struct gpio_desc *desc, const char *label)
 {
 	int ret = -EPROBE_DEFER;
-- 
2.47.2

[PATCH v2 6/8] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used

[Andy Shevchenko]

Call validate_desc() directly when VALIDATE_DESC() can't be used.
It will print additional information useful for debugging.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ad2e68f66500..3f3371e427fd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -421,11 +421,8 @@ int gpiod_get_direction(struct gpio_desc *desc)
 	unsigned int offset;
 	int ret;
 
-	/*
-	 * We cannot use VALIDATE_DESC() as we must not return 0 for a NULL
-	 * descriptor like we usually do.
-	 */
-	if (IS_ERR_OR_NULL(desc))
+	ret = validate_desc(desc, __func__);
+	if (ret <= 0)
 		return -EINVAL;
 
 	CLASS(gpio_chip_guard, guard)(desc);
@@ -3982,13 +3979,10 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 	struct gpio_device *gdev;
 	struct gpio_chip *gc;
 	int offset;
+	int ret;
 
-	/*
-	 * Cannot VALIDATE_DESC() here as gpiod_to_irq() consumer semantics
-	 * requires this function to not return zero on an invalid descriptor
-	 * but rather a negative error number.
-	 */
-	if (IS_ERR_OR_NULL(desc))
+	ret = validate_desc(desc, __func__);
+	if (ret <= 0)
 		return -EINVAL;
 
 	gdev = desc->gdev;
-- 
2.47.2

[PATCH v2 7/8] gpiolib: Reuse return variable in gpiod_to_irq()

[Andy Shevchenko]

There are two variables for the same used in the gpiod_to_irq().
Replace the second by reusing the function top-level one.
While at it, refactor the branch to have less lines of code.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3f3371e427fd..7a669218e42c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3994,13 +3994,12 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 
 	offset = gpio_chip_hwgpio(desc);
 	if (gc->to_irq) {
-		int retirq = gc->to_irq(gc, offset);
+		ret = gc->to_irq(gc, offset);
+		if (ret)
+			return ret;
 
 		/* Zero means NO_IRQ */
-		if (!retirq)
-			return -ENXIO;
-
-		return retirq;
+		return -ENXIO;
 	}
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 	if (gc->irq.chip) {
-- 
2.47.2

[PATCH v2 8/8] gpiolib: Remove redundant assignment of return variable

[Andy Shevchenko]

In some functions the returned variable is assigned to 0 and then
reassigned to the actual value. Remove redundant assignments.

In one case make it more clear that the assignment is not needed.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7a669218e42c..c787c9310e85 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1015,7 +1015,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	struct gpio_device *gdev;
 	unsigned int desc_index;
 	int base = 0;
-	int ret = 0;
+	int ret;
 
 	/* Only allow one set() and one set_multiple(). */
 	if ((gc->set && gc->set_rv) ||
@@ -1040,11 +1040,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	device_set_node(&gdev->dev, gpiochip_choose_fwnode(gc));
 
-	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
-	if (gdev->id < 0) {
-		ret = gdev->id;
+	ret = ida_alloc(&gpio_ida, GFP_KERNEL);
+	if (ret < 0)
 		goto err_free_gdev;
-	}
+	gdev->id = ret;
 
 	ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
 	if (ret)
@@ -3068,7 +3067,7 @@ int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
  */
 int gpiod_enable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 
 	VALIDATE_DESC(desc);
 
@@ -3101,7 +3100,7 @@ EXPORT_SYMBOL_GPL(gpiod_enable_hw_timestamp_ns);
  */
 int gpiod_disable_hw_timestamp_ns(struct gpio_desc *desc, unsigned long flags)
 {
-	int ret = 0;
+	int ret;
 
 	VALIDATE_DESC(desc);
 
-- 
2.47.2

Re: [PATCH v2 0/8] gpiolib: Some cleanups

[Bartosz Golaszewski]

On Wed, 16 Apr 2025 12:55:08 +0300, Andy Shevchenko wrote:
> Just a three groups of cleanups (some of them may be dependent to the previous
> ones compile and logically wise, but I am only aware about couple of patches,
> i.e. 3 & 4).
> 
> In v2:
> - split one patch to two (Linus)
> - added tags (Linus)
> 
> [...]

Applied, thanks!

[1/8] gpiolib: Make taking gpio_lookup_lock consistent
      https://git.kernel.org/brgl/linux/c/6a40e6c5be2a22d724ed195393442653a2cf9a1f
[2/8] gpiolib: Convert to use guard()() for gpio_machine_hogs_mutex
      https://git.kernel.org/brgl/linux/c/4ccbf7bc68cad35061e1f1b4c280fd736855a2b0
[3/8] gpiolib: Print actual error when descriptor contains an error pointer
      https://git.kernel.org/brgl/linux/c/c240ccd62764e036bd5e1f426fcdebe69e15d7df
[4/8] gpiolib: Revert "Don't WARN on gpiod_put() for optional GPIO"
      https://git.kernel.org/brgl/linux/c/960460b7b43957f3fb6b7856d5273f370e20ac9a
[5/8] gpiolib: Move validate_desc() and Co upper in the code
      https://git.kernel.org/brgl/linux/c/dc5881036291c412d673d11b19d392f25349a70f
[6/8] gpiolib: Call validate_desc() when VALIDATE_DESC() can't be used
      https://git.kernel.org/brgl/linux/c/197184384759ce0b7ae5fa17c9a6e272a3d28626
[7/8] gpiolib: Reuse return variable in gpiod_to_irq()
      https://git.kernel.org/brgl/linux/c/78998fa9887af032a2fc0f9ad0269af62439d848
[8/8] gpiolib: Remove redundant assignment of return variable
      https://git.kernel.org/brgl/linux/c/bfe489117ab2a5c16e4f8ab6863e61726338558d

Best regards,
-- 
Bartosz Golaszewski <brgl@bgdev.pl>