[PATCH 0/2] gpiolib: minor optimization

[PATCH 0/2] gpiolib: minor optimization

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Hello,

while working on gpiolib I noticed that gpio_request_one() repeatedly
checks the validity of the gpio_info obtained by gpio_to_desc().

This series introduces some helper functions (prefixed gpioinfo_) that
take an gpio_info instead of a plain gpio number and assume this pointer
to be valid.

The saved runtime is probably too small to measure, so I didn't even
try. Semantically there is no change intended.

Best regards
Uwe

Uwe Kleine-König (2):
  gpiolib: reorder functions
  gpiolib: introduce helper functions working on gpio_info structs

 drivers/gpio/gpiolib.c | 243 +++++++++++++++++++++++++----------------
 1 file changed, 150 insertions(+), 93 deletions(-)

-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/2] gpiolib: reorder functions

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

gpio_request_one() and gpio_request_array() make use of
gpio_direction_*, so it is natural to define the former them further low
in the file. It doesn't matter for the compiler here as all functions
are declared in a header, but the next commit adds a few more functions
and it benefits from this reordering by not needing forward
declarations.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 152 ++++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 76 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9764ddf0f062..6789ffd218b8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -145,82 +145,6 @@ void gpio_free(unsigned gpio)
 	gi->label = NULL;
 }
 
-/**
- * gpio_request_one - request a single GPIO with initial configuration
- * @gpio:	the GPIO number
- * @flags:	GPIO configuration as specified by GPIOF_*
- * @label:	a literal description string of this GPIO
- */
-int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
-{
-	int err;
-	struct gpio_info *gi = gpio_to_desc(gpio);
-
-	/*
-	 * Not all of the flags below are mulit-bit, but, for the sake
-	 * of consistency, the code is written as if all of them were.
-	 */
-	const bool active_low  = (flags & GPIOF_ACTIVE_LOW) == GPIOF_ACTIVE_LOW;
-	const bool dir_in      = (flags & GPIOF_DIR_IN) == GPIOF_DIR_IN;
-	const bool logical     = (flags & GPIOF_LOGICAL) == GPIOF_LOGICAL;
-	const bool init_active = (flags & GPIOF_INIT_ACTIVE) == GPIOF_INIT_ACTIVE;
-	const bool init_high   = (flags & GPIOF_INIT_HIGH) == GPIOF_INIT_HIGH;
-
-	err = gpio_request(gpio, label);
-	if (err)
-		return err;
-
-	gi->active_low = active_low;
-
-	if (dir_in)
-		err = gpio_direction_input(gpio);
-	else if (logical)
-		err = gpio_direction_active(gpio, init_active);
-	else
-		err = gpio_direction_output(gpio, init_high);
-
-	if (err)
-		gpio_free(gpio);
-
-	return err;
-}
-EXPORT_SYMBOL_GPL(gpio_request_one);
-
-/**
- * gpio_request_array - request multiple GPIOs in a single call
- * @array:	array of the 'struct gpio'
- * @num:	how many GPIOs in the array
- */
-int gpio_request_array(const struct gpio *array, size_t num)
-{
-	int i, err;
-
-	for (i = 0; i < num; i++, array++) {
-		err = gpio_request_one(array->gpio, array->flags, array->label);
-		if (err)
-			goto err_free;
-	}
-	return 0;
-
-err_free:
-	while (i--)
-		gpio_free((--array)->gpio);
-	return err;
-}
-EXPORT_SYMBOL_GPL(gpio_request_array);
-
-/**
- * gpio_free_array - release multiple GPIOs in a single call
- * @array:	array of the 'struct gpio'
- * @num:	how many GPIOs in the array
- */
-void gpio_free_array(const struct gpio *array, size_t num)
-{
-	while (num--)
-		gpio_free((array++)->gpio);
-}
-EXPORT_SYMBOL_GPL(gpio_free_array);
-
 void gpio_set_value(unsigned gpio, int value)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -324,6 +248,82 @@ int gpio_direction_input(unsigned gpio)
 }
 EXPORT_SYMBOL(gpio_direction_input);
 
+/**
+ * gpio_request_one - request a single GPIO with initial configuration
+ * @gpio:	the GPIO number
+ * @flags:	GPIO configuration as specified by GPIOF_*
+ * @label:	a literal description string of this GPIO
+ */
+int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
+{
+	int err;
+	struct gpio_info *gi = gpio_to_desc(gpio);
+
+	/*
+	 * Not all of the flags below are mulit-bit, but, for the sake
+	 * of consistency, the code is written as if all of them were.
+	 */
+	const bool active_low  = (flags & GPIOF_ACTIVE_LOW) == GPIOF_ACTIVE_LOW;
+	const bool dir_in      = (flags & GPIOF_DIR_IN) == GPIOF_DIR_IN;
+	const bool logical     = (flags & GPIOF_LOGICAL) == GPIOF_LOGICAL;
+	const bool init_active = (flags & GPIOF_INIT_ACTIVE) == GPIOF_INIT_ACTIVE;
+	const bool init_high   = (flags & GPIOF_INIT_HIGH) == GPIOF_INIT_HIGH;
+
+	err = gpio_request(gpio, label);
+	if (err)
+		return err;
+
+	gi->active_low = active_low;
+
+	if (dir_in)
+		err = gpio_direction_input(gpio);
+	else if (logical)
+		err = gpio_direction_active(gpio, init_active);
+	else
+		err = gpio_direction_output(gpio, init_high);
+
+	if (err)
+		gpio_free(gpio);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(gpio_request_one);
+
+/**
+ * gpio_request_array - request multiple GPIOs in a single call
+ * @array:	array of the 'struct gpio'
+ * @num:	how many GPIOs in the array
+ */
+int gpio_request_array(const struct gpio *array, size_t num)
+{
+	int i, err;
+
+	for (i = 0; i < num; i++, array++) {
+		err = gpio_request_one(array->gpio, array->flags, array->label);
+		if (err)
+			goto err_free;
+	}
+	return 0;
+
+err_free:
+	while (i--)
+		gpio_free((--array)->gpio);
+	return err;
+}
+EXPORT_SYMBOL_GPL(gpio_request_array);
+
+/**
+ * gpio_free_array - release multiple GPIOs in a single call
+ * @array:	array of the 'struct gpio'
+ * @num:	how many GPIOs in the array
+ */
+void gpio_free_array(const struct gpio *array, size_t num)
+{
+	while (num--)
+		gpio_free((array++)->gpio);
+}
+EXPORT_SYMBOL_GPL(gpio_free_array);
+
 static int gpiochip_find_base(int start, int ngpio)
 {
 	int i;
-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/2] gpiolib: introduce helper functions working on gpio_info structs

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

gpioinfo_*() assume their gpio_info pointer parameter to be valid and
don't ensure the gpio to be requested.

This drops several checks for being requested and allows further
extensions to work with unrequested gpios.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpio/gpiolib.c | 135 +++++++++++++++++++++++++++++------------
 1 file changed, 96 insertions(+), 39 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6789ffd218b8..53e7fbe7de49 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -48,6 +48,11 @@ static struct gpio_info *gpio_to_desc(unsigned gpio)
 	return NULL;
 }
 
+static unsigned gpioinfo_chip_offset(struct gpio_info *gi)
+{
+	return (gi - gpio_desc) - gi->chip->base;
+}
+
 static int gpio_adjust_value(struct gpio_info *gi,
 			     int value)
 {
@@ -57,16 +62,10 @@ static int gpio_adjust_value(struct gpio_info *gi,
 	return !!value ^ gi->active_low;
 }
 
-int gpio_request(unsigned gpio, const char *label)
+static int gpioinfo_request(struct gpio_info *gi, const char *label)
 {
-	struct gpio_info *gi = gpio_to_desc(gpio);
 	int ret;
 
-	if (!gi) {
-		ret = -ENODEV;
-		goto done;
-	}
-
 	if (gi->requested) {
 		ret = -EBUSY;
 		goto done;
@@ -75,7 +74,8 @@ int gpio_request(unsigned gpio, const char *label)
 	ret = 0;
 
 	if (gi->chip->ops->request) {
-		ret = gi->chip->ops->request(gi->chip, gpio - gi->chip->base);
+		ret = gi->chip->ops->request(gi->chip,
+					     gpioinfo_chip_offset(gi));
 		if (ret)
 			goto done;
 	}
@@ -87,7 +87,7 @@ int gpio_request(unsigned gpio, const char *label)
 done:
 	if (ret)
 		pr_err("_gpio_request: gpio-%d (%s) status %d\n",
-			 gpio, label ? : "?", ret);
+		       gi - gpio_desc, label ? : "?", ret);
 
 	return ret;
 }
@@ -126,18 +126,26 @@ int gpio_find_by_name(const char *name)
 	return -ENOENT;
 }
 
-void gpio_free(unsigned gpio)
+int gpio_request(unsigned gpio, const char *label)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
 
-	if (!gi)
-		return;
+	if (!gi) {
+		pr_err("_gpio_request: gpio-%d (%s) status %d\n",
+			 gpio, label ? : "?", -ENODEV);
+		return -ENODEV;
+	}
+
+	return gpioinfo_request(gi, label);
+}
 
+static void gpioinfo_free(struct gpio_info *gi)
+{
 	if (!gi->requested)
 		return;
 
 	if (gi->chip->ops->free)
-		gi->chip->ops->free(gi->chip, gpio - gi->chip->base);
+		gi->chip->ops->free(gi->chip, gpioinfo_chip_offset(gi));
 
 	gi->requested = false;
 	gi->active_low = false;
@@ -145,6 +153,22 @@ void gpio_free(unsigned gpio)
 	gi->label = NULL;
 }
 
+void gpio_free(unsigned gpio)
+{
+	struct gpio_info *gi = gpio_to_desc(gpio);
+
+	if (!gi)
+		return;
+
+	gpioinfo_free(gi);
+}
+
+static void gpioinfo_set_value(struct gpio_info *gi, int value)
+{
+	if (gi->chip->ops->set)
+		gi->chip->ops->set(gi->chip, gpioinfo_chip_offset(gi), value);
+}
+
 void gpio_set_value(unsigned gpio, int value)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -155,8 +179,7 @@ void gpio_set_value(unsigned gpio, int value)
 	if (gpio_ensure_requested(gi, gpio))
 		return;
 
-	if (gi->chip->ops->set)
-		gi->chip->ops->set(gi->chip, gpio - gi->chip->base, value);
+	gpioinfo_set_value(gi, value);
 }
 EXPORT_SYMBOL(gpio_set_value);
 
@@ -171,6 +194,14 @@ void gpio_set_active(unsigned gpio, bool value)
 }
 EXPORT_SYMBOL(gpio_set_active);
 
+static int gpioinfo_get_value(struct gpio_info *gi)
+{
+	if (!gi->chip->ops->get)
+		return -ENOSYS;
+
+	return gi->chip->ops->get(gi->chip, gpioinfo_chip_offset(gi));
+}
+
 int gpio_get_value(unsigned gpio)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -183,9 +214,7 @@ int gpio_get_value(unsigned gpio)
 	if (ret)
 		return ret;
 
-	if (!gi->chip->ops->get)
-		return -ENOSYS;
-	return gi->chip->ops->get(gi->chip, gpio - gi->chip->base);
+	return gpioinfo_get_value(gi);
 }
 EXPORT_SYMBOL(gpio_get_value);
 
@@ -200,6 +229,15 @@ int gpio_is_active(unsigned gpio)
 }
 EXPORT_SYMBOL(gpio_is_active);
 
+static int gpioinfo_direction_output(struct gpio_info *gi, int value)
+{
+	if (!gi->chip->ops->direction_output)
+		return -ENOSYS;
+
+	return gi->chip->ops->direction_output(gi->chip,
+					       gpioinfo_chip_offset(gi), value);
+}
+
 int gpio_direction_output(unsigned gpio, int value)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -212,13 +250,15 @@ int gpio_direction_output(unsigned gpio, int value)
 	if (ret)
 		return ret;
 
-	if (!gi->chip->ops->direction_output)
-		return -ENOSYS;
-	return gi->chip->ops->direction_output(gi->chip, gpio - gi->chip->base,
-					       value);
+	return gpioinfo_direction_output(gi, value);
 }
 EXPORT_SYMBOL(gpio_direction_output);
 
+static int gpioinfo_direction_active(struct gpio_info *gi, bool value)
+{
+	return gpioinfo_direction_output(gi, gpio_adjust_value(gi, value));
+}
+
 int gpio_direction_active(unsigned gpio, bool value)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -226,10 +266,19 @@ int gpio_direction_active(unsigned gpio, bool value)
 	if (!gi)
 		return -ENODEV;
 
-	return gpio_direction_output(gpio, gpio_adjust_value(gi, value));
+	return gpioinfo_direction_active(gi, value);
 }
 EXPORT_SYMBOL(gpio_direction_active);
 
+static int gpioinfo_direction_input(struct gpio_info *gi)
+{
+	if (!gi->chip->ops->direction_input)
+		return -ENOSYS;
+
+	return gi->chip->ops->direction_input(gi->chip,
+					      gpioinfo_chip_offset(gi));
+}
+
 int gpio_direction_input(unsigned gpio)
 {
 	struct gpio_info *gi = gpio_to_desc(gpio);
@@ -242,22 +291,14 @@ int gpio_direction_input(unsigned gpio)
 	if (ret)
 		return ret;
 
-	if (!gi->chip->ops->direction_input)
-		return -ENOSYS;
-	return gi->chip->ops->direction_input(gi->chip, gpio - gi->chip->base);
+	return gpioinfo_direction_input(gi);
 }
 EXPORT_SYMBOL(gpio_direction_input);
 
-/**
- * gpio_request_one - request a single GPIO with initial configuration
- * @gpio:	the GPIO number
- * @flags:	GPIO configuration as specified by GPIOF_*
- * @label:	a literal description string of this GPIO
- */
-int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
+static int gpioinfo_request_one(struct gpio_info *gi, unsigned long flags,
+				const char *label)
 {
 	int err;
-	struct gpio_info *gi = gpio_to_desc(gpio);
 
 	/*
 	 * Not all of the flags below are mulit-bit, but, for the sake
@@ -269,24 +310,40 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	const bool init_active = (flags & GPIOF_INIT_ACTIVE) == GPIOF_INIT_ACTIVE;
 	const bool init_high   = (flags & GPIOF_INIT_HIGH) == GPIOF_INIT_HIGH;
 
-	err = gpio_request(gpio, label);
+	err = gpioinfo_request(gi, label);
 	if (err)
 		return err;
 
 	gi->active_low = active_low;
 
 	if (dir_in)
-		err = gpio_direction_input(gpio);
+		err = gpioinfo_direction_input(gi);
 	else if (logical)
-		err = gpio_direction_active(gpio, init_active);
+		err = gpioinfo_direction_active(gi, init_active);
 	else
-		err = gpio_direction_output(gpio, init_high);
+		err = gpioinfo_direction_output(gi, init_high);
 
 	if (err)
-		gpio_free(gpio);
+		gpioinfo_free(gi);
 
 	return err;
 }
+
+/**
+ * gpio_request_one - request a single GPIO with initial configuration
+ * @gpio:	the GPIO number
+ * @flags:	GPIO configuration as specified by GPIOF_*
+ * @label:	a literal description string of this GPIO
+ */
+int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
+{
+	struct gpio_info *gi = gpio_to_desc(gpio);
+
+	if (!gi)
+		return -ENODEV;
+
+	return gpioinfo_request_one(gi, flags, label);
+}
 EXPORT_SYMBOL_GPL(gpio_request_one);
 
 /**
-- 
2.24.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/2] gpiolib: minor optimization

[Sascha Hauer]

On Mon, Jan 27, 2020 at 12:44:51PM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Hello,
> 
> while working on gpiolib I noticed that gpio_request_one() repeatedly
> checks the validity of the gpio_info obtained by gpio_to_desc().
> 
> This series introduces some helper functions (prefixed gpioinfo_) that
> take an gpio_info instead of a plain gpio number and assume this pointer
> to be valid.
> 
> The saved runtime is probably too small to measure, so I didn't even
> try. Semantically there is no change intended.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   gpiolib: reorder functions
>   gpiolib: introduce helper functions working on gpio_info structs
> 
>  drivers/gpio/gpiolib.c | 243 +++++++++++++++++++++++++----------------
>  1 file changed, 150 insertions(+), 93 deletions(-)

Applied, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox