diff mbox

[v2] gpiolib: rewrite gpiodev_add_to_list

Message ID 1456497434-11656-1-git-send-email-bamv2005@gmail.com
State New
Headers show

Commit Message

Bamvor Zhang Feb. 26, 2016, 2:37 p.m. UTC 
From: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>


The original code of gpiodev_add_to_list is not very clear which
lead to bugs or compiling warning, reference the following patches:
Bugs:
1.  Commit ef7c7553039b ("gpiolib: improve overlap check of range of
    gpio").
2.  Commit 96098df125c0 ("gpiolib: fix chip order in gpio list")

Warning:
1.  Commit e28ecca6eac4 ("gpio: fix warning about iterator").
of gpio").

There is a off-list discussion about how to improve it consequently.
This commit try to follow this by rewriting the whole functions.

Tested pass with my gpio mockup driver and test scripts[1].

[1] http://www.spinics.net/lists/linux-gpio/msg09598.html

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

---
 drivers/gpio/gpiolib.c | 65 +++++++++++++++++++-------------------------------
 1 file changed, 25 insertions(+), 40 deletions(-)

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Walleij March 7, 2016, 5:04 a.m. UTC | #1 
On Fri, Feb 26, 2016 at 9:37 PM, Bamvor Jian Zhang <bamv2005@gmail.com> wrote:

> From: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

>

> The original code of gpiodev_add_to_list is not very clear which

> lead to bugs or compiling warning, reference the following patches:

> Bugs:

> 1.  Commit ef7c7553039b ("gpiolib: improve overlap check of range of

>     gpio").

> 2.  Commit 96098df125c0 ("gpiolib: fix chip order in gpio list")

>

> Warning:

> 1.  Commit e28ecca6eac4 ("gpio: fix warning about iterator").

> of gpio").

>

> There is a off-list discussion about how to improve it consequently.

> This commit try to follow this by rewriting the whole functions.

>

> Tested pass with my gpio mockup driver and test scripts[1].

>

> [1] http://www.spinics.net/lists/linux-gpio/msg09598.html

>

> Suggested-by: Arnd Bergmann <arnd@arndb.de>

> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>


Patch applied. It surely looks much better like this!

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bc788b9..1c10bd8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -206,58 +206,43 @@  EXPORT_SYMBOL_GPL(gpiod_get_direction);
  */
 static int gpiodev_add_to_list(struct gpio_device *gdev)
 {
-	struct gpio_device *iterator;
-	struct gpio_device *previous = NULL;
-
-	if (!gdev->chip)
-		return -EINVAL;
+	struct gpio_device *prev, *next;
 
 	if (list_empty(&gpio_devices)) {
+		/* initial entry in list */
 		list_add_tail(&gdev->list, &gpio_devices);
 		return 0;
 	}
 
-	list_for_each_entry(iterator, &gpio_devices, list) {
-		if (iterator->base >= gdev->base + gdev->ngpio) {
-			/*
-			 * Iterator is the first GPIO chip so there is no
-			 * previous one
-			 */
-			if (!previous) {
-				goto found;
-			} else {
-				/*
-				 * We found a valid range(means
-				 * [base, base + ngpio - 1]) between previous
-				 * and iterator chip.
-				 */
-				if (previous->base + previous->ngpio
-				    <= gdev->base)
-					goto found;
-			}
-		}
-		previous = iterator;
+	next = list_entry(gpio_devices.next, struct gpio_device, list);
+	if (gdev->base + gdev->ngpio <= next->base) {
+		/* add before first entry */
+		list_add(&gdev->list, &gpio_devices);
+		return 0;
 	}
 
-	/*
-	 * We are beyond the last chip in the list and iterator now
-	 * points to the head.
-	 * Let iterator point to the last chip in the list.
-	 */
-
-	iterator = list_last_entry(&gpio_devices, struct gpio_device, list);
-	if (iterator->base + iterator->ngpio <= gdev->base) {
-		list_add(&gdev->list, &iterator->list);
+	prev = list_entry(gpio_devices.prev, struct gpio_device, list);
+	if (prev->base + prev->ngpio <= gdev->base) {
+		/* add behind last entry */
+		list_add_tail(&gdev->list, &gpio_devices);
 		return 0;
 	}
 
-	dev_err(&gdev->dev,
-	       "GPIO integer space overlap, cannot add chip\n");
-	return -EBUSY;
+	list_for_each_entry_safe(prev, next, &gpio_devices, list) {
+		/* at the end of the list */
+		if (&next->list == &gpio_devices)
+			break;
 
-found:
-	list_add_tail(&gdev->list, &iterator->list);
-	return 0;
+		/* add between prev and next */
+		if (prev->base + prev->ngpio <= gdev->base
+				&& gdev->base + gdev->ngpio <= next->base) {
+			list_add(&gdev->list, &prev->list);
+			return 0;
+		}
+	}
+
+	dev_err(&gdev->dev, "GPIO integer space overlap, cannot add chip\n");
+	return -EBUSY;
 }
 
 /**