gpiolib: implement gpiochip_reserve

PATCH In review Anton Vorontsov 2 participants 4 messages Mar 11, 2008 – Mar 12, 2008

Subsystems (2)

GPIO Generic include/asm header files

lore

Share & tools

lore

Anton Vorontsov

author

2008-03-11 17:41 UTC #0 lore raw

PATCH 2/3 gpiolib: implement gpiochip_reserve

Function gpiochip_reserve() reserves range of gpios to use with platform code only, that is, this function used to mark specified range of gpios unavailable for the dynamic gpio base allocator.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/gpio/gpiolib.c     |   51 +++++++++++++++++++++++++++++++++++++++++--
 include/asm-generic/gpio.h |    1 +
 2 files changed, 49 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +48 -3

index 81d81c9..2149e0c 100644
@@ -43,6 +43,7 @@ struct gpio_desc {
 /* flag symbols are bit numbers */
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
+#define FLAG_RESERVED	2
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;

@@ -80,6 +81,48 @@ static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
 	return gpio_desc[gpio].chip;
 }
 
+/**
+ * gpiochip_reserve() - reserve range of gpios to use with platform code only
+ * @start: starting gpio number
+ * @ngpio: number of gpios to reserve
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * Returns a negative errno if any gpio within the range already reserved
+ * or registered. Otherwise it returns zero as a success code.
+ * Use this function to mark specified range of gpios unavailable for the
+ * dynamic gpio base allocator.
+ */
+int __must_check gpiochip_reserve(int start, int ngpio)
+{
+	int ret = 0;
+	unsigned long flags;
+	int i;
+
+	if (!gpio_is_valid(start) || !gpio_is_valid(start + ngpio))
+		return -EINVAL;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	for (i = start; i < start + ngpio; i++) {
+		struct gpio_desc *desc = &gpio_desc[i];
+
+		if (desc->chip || test_bit(FLAG_RESERVED, &desc->flags)) {
+			ret = -EBUSY;
+			goto err;
+		}
+
+		set_bit(FLAG_RESERVED, &desc->flags);
+	}
+
+	pr_debug("%s: reserved gpios from %d to %d\n",
+		 __func__, start, start + ngpio - 1);
+err:
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiochip_reserve);
+
 static int gpiochip_find_base(int ngpio)
 {
 	int i;

@@ -87,9 +130,10 @@ static int gpiochip_find_base(int ngpio)
 	int base = -ENOSPC;
 
 	for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) {
-		struct gpio_chip *chip = gpio_desc[i].chip;
+		struct gpio_desc *desc = &gpio_desc[i];
+		struct gpio_chip *chip = desc->chip;
 
-		if (!chip) {
+		if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)) {
 			spare++;
 			if (spare == ngpio) {
 				base = i;

@@ -97,7 +141,8 @@ static int gpiochip_find_base(int ngpio)
 			}
 		} else {
 			spare = 0;
-			i -= chip->ngpio - 1;
+			if (chip)
+				i -= chip->ngpio - 1;
 		}
 	}

include/asm-generic/gpio.h +1 -0

index 464c5b3..20f5d67 100644
@@ -74,6 +74,7 @@ struct gpio_chip {
 
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
+extern int __must_check gpiochip_reserve(int start, int ngpio);
 
 /* add/remove chips */
 extern int gpiochip_add(struct gpio_chip *chip);

Signature · 1 line

1.5.2.2

David Brownell

2008-03-11 21:12 UTC #1 lore raw

Quoted 3 lines · Anton Vorontsov

Function gpiochip_reserve() reserves range of gpios to use with platform code only, that is, this function used to mark specified range of gpios unavailable for the dynamic gpio base allocator.

Since this is for platform code only, it doesn't need to stick around after initcalls have run. So please mark it as __init so that we minimize the footprint cost.

- Dave

Quoted 82 lines

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/gpio/gpiolib.c     |   51 +++++++++++++++++++++++++++++++++++++++++--
 include/asm-generic/gpio.h |    1 +
 2 files changed, 49 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +46 -2

index 81d81c9..2149e0c 100644
@@ -43,6 +43,7 @@ struct gpio_desc {
 /* flag symbols are bit numbers */
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
+#define FLAG_RESERVED	2
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;

@@ -80,6 +81,48 @@ static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
 	return gpio_desc[gpio].chip;
 }
 
+/**
+ * gpiochip_reserve() - reserve range of gpios to use with platform code only
+ * @start: starting gpio number
+ * @ngpio: number of gpios to reserve
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * Returns a negative errno if any gpio within the range already reserved
+ * or registered. Otherwise it returns zero as a success code.
+ * Use this function to mark specified range of gpios unavailable for the
+ * dynamic gpio base allocator.
+ */
+int __must_check gpiochip_reserve(int start, int ngpio)
+{
+	int ret = 0;
+	unsigned long flags;
+	int i;
+
+	if (!gpio_is_valid(start) || !gpio_is_valid(start + ngpio))
+		return -EINVAL;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	for (i = start; i < start + ngpio; i++) {
+		struct gpio_desc *desc = &gpio_desc[i];
+
+		if (desc->chip || test_bit(FLAG_RESERVED, &desc->flags)) {
+			ret = -EBUSY;
+			goto err;
+		}
+
+		set_bit(FLAG_RESERVED, &desc->flags);
+	}
+
+	pr_debug("%s: reserved gpios from %d to %d\n",
+		 __func__, start, start + ngpio - 1);
+err:
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiochip_reserve);
+
 static int gpiochip_find_base(int ngpio)
 {
 	int i;

@@ -87,9 +130,10 @@ static int gpiochip_find_base(int ngpio)
 	int base = -ENOSPC;
 
 	for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) {
-		struct gpio_chip *chip = gpio_desc[i].chip;
+		struct gpio_desc *desc = &gpio_desc[i];
+		struct gpio_chip *chip = desc->chip;
 
-		if (!chip) {
+		if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)) {
 			spare++;
 			if (spare == ngpio) {
 				base = i;

drivers/gpio/gpiolib.c static int gpiochip_find_base(int ngpio) · line 145

Quoted 6 lines

drivers/gpio/gpiolib.c +2 -1

@@ -97,7 +141,8 @@ static int gpiochip_find_base(int ngpio)
 			}
 		} else {
 			spare = 0;
-			i -= chip->ngpio - 1;
+			if (chip)
+				i -= chip->ngpio - 1;

Isn't this little optimization a bit error prone? It presumes the reserved bits and gpio chips are in fairly close synch. I'd just remove it ... it shouldn't be a common path.

Quoted 18 lines

drivers/gpio/gpiolib.c +0 -0

 		}
 	}

include/asm-generic/gpio.h +1 -0

index 464c5b3..20f5d67 100644
@@ -74,6 +74,7 @@ struct gpio_chip {
 
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
+extern int __must_check gpiochip_reserve(int start, int ngpio);
 
 /* add/remove chips */
 extern int gpiochip_add(struct gpio_chip *chip);

Signature · 1 line

1.5.2.2

Anton Vorontsov

author

2008-03-12 12:23 UTC #2 lore raw

Quoted 8 lines · David Brownell

Quoted 3 lines · Anton Vorontsov
Function gpiochip_reserve() reserves range of gpios to use with platform code only, that is, this function used to mark specified range of gpios unavailable for the dynamic gpio base allocator.
Since this is for platform code only, it doesn't need to stick around after initcalls have run. So please mark it as __init so that we minimize the footprint cost.

Good idea, thanks.

[...]

Quoted 5 lines

(quoted diff) +2 -1

 	for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) {
-		struct gpio_chip *chip = gpio_desc[i].chip;
+		struct gpio_desc *desc = &gpio_desc[i];
+		struct gpio_chip *chip = desc->chip;

Quoted 14 lines

Review static int gpiochip_find_base(int ngpio) · line 145

Quoted 12 lines

(quoted diff) +3 -2

-		if (!chip) {
+		if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)) {
 			spare++;
 			if (spare == ngpio) {
 				base = i;

@@ -97,7 +141,8 @@ static int gpiochip_find_base(int ngpio)
 			}
 		} else {
 			spare = 0;
-			i -= chip->ngpio - 1;
+			if (chip)
+				i -= chip->ngpio - 1;

Isn't this little optimization a bit error prone?

So far I don't see how exactly it could be error prone.

Quoted 3 lines

It
presumes the reserved bits and gpio chips are in fairly close synch.

Probably I don't understand the synch part here. The thing is that I'm replacing if (!chip) with if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)), so now "else" code might be executed when gpio is either reserved or the range of gpios is already managed by the chip. In later case we can skip whole range, because we know its size. In case of reserved or spare gpio we can't do that, so we simply try the next one. Anything wrong with this scheme?..

Below the patch with __init attribute added.

- - - -
From: Anton Vorontsov <[email protected]>
Subject: gpiolib: implement gpiochip_reserve

Function gpiochip_reserve() reserves range of gpios to use with platform code only, that is, this function used to mark specified range of gpios unavailable for the dynamic gpio base allocator.

Signed-off-by Anton Vorontsov <[email protected]>

---
 drivers/gpio/gpiolib.c     |   51 +++++++++++++++++++++++++++++++++++++++++--
 include/asm-generic/gpio.h |    1 +
 2 files changed, 49 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib.c +48 -3

index 81d81c9..372bcc5 100644
@@ -43,6 +43,7 @@ struct gpio_desc {
 /* flag symbols are bit numbers */
 #define FLAG_REQUESTED	0
 #define FLAG_IS_OUT	1
+#define FLAG_RESERVED	2
 
 #ifdef CONFIG_DEBUG_FS
 	const char		*label;

@@ -80,6 +81,48 @@ static inline struct gpio_chip *gpio_to_chip(unsigned gpio)
 	return gpio_desc[gpio].chip;
 }
 
+/**
+ * gpiochip_reserve() - reserve range of gpios to use with platform code only
+ * @start: starting gpio number
+ * @ngpio: number of gpios to reserve
+ * Context: potentially before irqs or kmalloc will work
+ *
+ * Returns a negative errno if any gpio within the range already reserved
+ * or registered. Otherwise it returns zero as a success code.
+ * Use this function to mark specified range of gpios unavailable for the
+ * dynamic gpio base allocator.
+ */
+int __init __must_check gpiochip_reserve(int start, int ngpio)
+{
+	int ret = 0;
+	unsigned long flags;
+	int i;
+
+	if (!gpio_is_valid(start) || !gpio_is_valid(start + ngpio))
+		return -EINVAL;
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
+	for (i = start; i < start + ngpio; i++) {
+		struct gpio_desc *desc = &gpio_desc[i];
+
+		if (desc->chip || test_bit(FLAG_RESERVED, &desc->flags)) {
+			ret = -EBUSY;
+			goto err;
+		}
+
+		set_bit(FLAG_RESERVED, &desc->flags);
+	}
+
+	pr_debug("%s: reserved gpios from %d to %d\n",
+		 __func__, start, start + ngpio - 1);
+err:
+	spin_unlock_irqrestore(&gpio_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiochip_reserve);
+
 static int gpiochip_find_base(int ngpio)
 {
 	int i;

@@ -87,9 +130,10 @@ static int gpiochip_find_base(int ngpio)
 	int base = -ENOSPC;
 
 	for (i = ARCH_NR_GPIOS - 1; i >= 0 ; i--) {
-		struct gpio_chip *chip = gpio_desc[i].chip;
+		struct gpio_desc *desc = &gpio_desc[i];
+		struct gpio_chip *chip = desc->chip;
 
-		if (!chip) {
+		if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)) {
 			spare++;
 			if (spare == ngpio) {
 				base = i;

@@ -97,7 +141,8 @@ static int gpiochip_find_base(int ngpio)
 			}
 		} else {
 			spare = 0;
-			i -= chip->ngpio - 1;
+			if (chip)
+				i -= chip->ngpio - 1;
 		}
 	}

include/asm-generic/gpio.h +1 -0

index 464c5b3..ecf675a 100644
@@ -74,6 +74,7 @@ struct gpio_chip {
 
 extern const char *gpiochip_is_requested(struct gpio_chip *chip,
 			unsigned offset);
+extern int __init __must_check gpiochip_reserve(int start, int ngpio);
 
 /* add/remove chips */
 extern int gpiochip_add(struct gpio_chip *chip);

Signature · 1 line

1.5.2.2

David Brownell

Acked-by

2008-03-12 23:03 UTC #3 lore raw

Quoted 8 lines · Anton Vorontsov

Quoted 3 lines
It
presumes the reserved bits and gpio chips are in fairly close synch.
Probably I don't understand the synch part here. The thing is that I'm replacing if (!chip) with if (!chip && !test_bit(FLAG_RESERVED, &desc->flags)), so now "else" code might be executed when gpio is either reserved or the range of gpios is already managed by the chip.

OK then.

Quoted 11 lines

Below the patch with __init attribute added.
- - - -
From: Anton Vorontsov <[email protected]>
Subject: gpiolib: implement gpiochip_reserve
Function gpiochip_reserve() reserves range of gpios to use with platform code only, that is, this function used to mark specified range of gpios unavailable for the dynamic gpio base allocator.
Signed-off-by Anton Vorontsov <[email protected]>

Acked-by David Brownell <[email protected]>

Quoted 2 lines

---
...