[RFC PATCH] gpiolib: introduce gpio_set_multiple function

[RFC PATCH] gpiolib: introduce gpio_set_multiple function

[Rojhalat Ibrahim]

This patch introduces a new function gpio_set_multiple which allows setting
multiple GPIO lines on the same chip with just one function call. If a
corresponding set_multi function is defined for the chip performance can be
significantly improved. Otherwise gpio_set_multiple uses the chip's set
function and the performance stays the same as without the new function.
For my application, where I have to frequently set 9 GPIO lines in order to
configure an FPGA, configuration time goes down from 48 s to 15 s when I use
this new function. (Tested with a modified gpio-mpc8xxx module.)

Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
---
 drivers/gpio/gpiolib.c        |   59 ++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h    |   17 ++++++++++++
 include/linux/gpio.h          |   20 ++++++++++++++
 include/linux/gpio/consumer.h |   14 +++++++++
 include/linux/gpio/driver.h   |    3 ++
 5 files changed, 113 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 50c4922..8355acb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2099,6 +2099,44 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
 }
 EXPORT_SYMBOL_GPL(gpiod_set_value);
 
+static void _gpiod_set_raw_multiple(struct gpio_chip *chip,
+				    u64 mask, u64 value)
+{
+	if (chip->set_multi)
+		chip->set_multi(chip, mask, value);
+	else {
+		int i;
+		for (i=0; i<64; i++) {
+			if (i > chip->ngpio - 1)
+				break;
+			if (mask & ((u64)(1) << i))
+				chip->set(chip, i, (value >> i) & 1);
+		}
+	}
+}
+
+/**
+ * gpiod_set_raw_multiple() - assign values to multiple GPIOs
+ * @chip: chip the GPIOs belong to
+ * @mask: only GPIOs with corresponding bits in mask set are modified
+ * @value: value bits to assign to GPIOs
+ *
+ * Set the raw values of multiple GPIOs, i.e. physical line values without
+ * regard for their ACTIVE_LOW status.
+ *
+ * This function should be called from contexts where we cannot sleep, and will
+ * complain if the GPIO chip functions potentially sleep.
+ */
+void gpiod_set_raw_multiple(struct gpio_chip *chip, u64 mask, u64 value)
+{
+	if (!chip)
+		return;
+	/* Should be using gpio_set_multiple_cansleep() */
+	WARN_ON(chip->can_sleep);
+	_gpiod_set_raw_multiple(chip, mask, value);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_multiple);
+
 /**
  * gpiod_cansleep() - report whether gpio value access may sleep
  * @desc: gpio to check
@@ -2272,6 +2310,27 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
 
 /**
+ * gpiod_set_raw_multiple_cansleep() - assign values to multiple GPIOs
+ * @chip: chip the GPIOs belong to
+ * @mask: only GPIOs with corresponding bits in mask set are modified
+ * @value: value bits to assign to GPIOs
+ *
+ * Set the raw values of multiple GPIOs, i.e. physical line values without
+ * regard for their ACTIVE_LOW status.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+void gpiod_set_raw_multiple_cansleep(struct gpio_chip *chip,
+				     u64 mask, u64 value)
+{
+	might_sleep_if(extra_checks);
+	if (!chip)
+		return;
+	_gpiod_set_raw_multiple(chip, mask, value);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_multiple_cansleep);
+
+/**
  * gpiod_add_lookup_table() - register GPIO device consumers
  * @table: table of consumers to register
  */
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index a5f56a0..c53d8ac 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -85,6 +85,11 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 {
 	return gpiod_set_raw_value_cansleep(gpio_to_desc(gpio), value);
 }
+static inline void gpio_set_multiple_cansleep(struct gpio_chip *chip, u64 mask,
+					      u64 value)
+{
+	return gpiod_set_raw_multiple_cansleep(chip, mask, value);
+}
 
 
 /* A platform's <asm/gpio.h> code may want to inline the I/O calls when
@@ -99,6 +104,11 @@ static inline void __gpio_set_value(unsigned gpio, int value)
 {
 	return gpiod_set_raw_value(gpio_to_desc(gpio), value);
 }
+static inline void __gpio_set_multiple(struct gpio_chip *chip, u64 mask,
+				       u64 value)
+{
+	return gpiod_set_raw_multiple(chip, mask, value);
+}
 
 static inline int __gpio_cansleep(unsigned gpio)
 {
@@ -218,6 +228,13 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	__gpio_set_value(gpio, value);
 }
 
+static inline void gpio_set_multiple_cansleep(struct gpio_chip *chip, u64 mask,
+					      u64 value)
+{
+	might_sleep();
+	__gpio_set_multiple(chip, mask, value);
+}
+
 #endif /* !CONFIG_GPIOLIB */
 
 #endif /* _ASM_GENERIC_GPIO_H */
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index b581b13..cbb4725 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -60,6 +60,12 @@ static inline void gpio_set_value(unsigned int gpio, int value)
 	__gpio_set_value(gpio, value);
 }
 
+static inline void gpio_set_multiple(struct gpio_chip *chip, u64 mask,
+				     u64 value)
+{
+	__gpio_set_multiple(chip, mask, value);
+}
+
 static inline int gpio_cansleep(unsigned int gpio)
 {
 	return __gpio_cansleep(gpio);
@@ -161,6 +167,13 @@ static inline void gpio_set_value(unsigned gpio, int value)
 	WARN_ON(1);
 }
 
+static inline void gpio_set_multiple(struct gpio_chip *chip, u64 mask,
+				     u64 value)
+{
+	/* GPIO can never have been requested or set as output */
+	WARN_ON(1);
+}
+
 static inline int gpio_cansleep(unsigned gpio)
 {
 	/* GPIO can never have been requested or set as {in,out}put */
@@ -181,6 +194,13 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	WARN_ON(1);
 }
 
+static inline void gpio_set_multiple_cansleep(struct gpio_chip *chip,
+					      u64 mask, u64 value)
+{
+	/* GPIO can never have been requested or set as output */
+	WARN_ON(1);
+}
+
 static inline int gpio_export(unsigned gpio, bool direction_may_change)
 {
 	/* GPIO can never have been requested or set as {in,out}put */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 4d34dbb..5ffdbd0 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -42,12 +42,14 @@ int gpiod_get_value(const struct gpio_desc *desc);
 void gpiod_set_value(struct gpio_desc *desc, int value);
 int gpiod_get_raw_value(const struct gpio_desc *desc);
 void gpiod_set_raw_value(struct gpio_desc *desc, int value);
+void gpiod_set_raw_multiple(struct gpio_chip *chip, u64 mask, u64 value);
 
 /* Value get/set from sleeping context */
 int gpiod_get_value_cansleep(const struct gpio_desc *desc);
 void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
 int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc);
 void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
+void gpiod_set_raw_multiple_cansleep(struct gpio_chip *chip, u64 mask, u64 value);
 
 int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
 
@@ -145,6 +147,12 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
+static inline void gpiod_set_raw_multiple(struct gpio_chip *chip, u64 mask,
+					  u64 value)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
 
 static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 {
@@ -169,6 +177,12 @@ static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
+static inline void gpiod_set_raw_multiple_cansleep(struct gpio_chip *chip,
+						   u64 mask, u64 value)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
 
 static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a3e181e..571ad82 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -27,6 +27,7 @@ struct seq_file;
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
  * @set: assigns output value for signal "offset"
+ * @set_multi: assigns output values for multiple signals defined by "mask"
  * @set_debounce: optional hook for setting debounce time for specified gpio in
  *      interrupt triggered gpio chips
  * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
@@ -79,6 +80,8 @@ struct gpio_chip {
 						unsigned offset);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	void			(*set_multi)(struct gpio_chip *chip,
+					        u64 mask, u64 value);
 	int			(*set_debounce)(struct gpio_chip *chip,
 						unsigned offset,
 						unsigned debounce);
--
1.8.3.2

Re: [RFC PATCH] gpiolib: introduce gpio_set_multiple function

[Alexandre Courbot]

On Thu, Jan 23, 2014 at 9:19 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> This patch introduces a new function gpio_set_multiple which allows setting
> multiple GPIO lines on the same chip with just one function call. If a
> corresponding set_multi function is defined for the chip performance can be
> significantly improved. Otherwise gpio_set_multiple uses the chip's set
> function and the performance stays the same as without the new function.
> For my application, where I have to frequently set 9 GPIO lines in order to
> configure an FPGA, configuration time goes down from 48 s to 15 s when I use
> this new function. (Tested with a modified gpio-mpc8xxx module.)

You seem to have a real use-case so it is probably ok to include
something like this. However there are a few points/limitations I'd
like to discuss:

- Right now your 64-bit mask means this function is limited to the
first 64 GPIOs of the chip. How would one handle chips with more
GPIOs?

- How do you guarantee that the GPIOs have been properly acquired?
This is something that we really want to enforce with the gpiod API,
and your new functions allow to completely bypass this security.

I wonder if both issues could not be addressed by extending gpiolib
(yay, more GPIO functions!) to be able to acquire a previously-defined
"group" of GPIOs (this group could be defined as a chain of
platform-defined GPIOs, or within the DT). Success upon this would
return a GPIO group descriptor that would be used with
gpiod_*_multiple(). That way it stays in the spirit of this API and
ensures bad guys cannot steal our GPIOs. *Or*, just allow a GPIO
descriptor to also describe a GPIO group, that way we don't need the
gpiod_*_multiple() functions. We will need to find a way to honor the
active-low, open drain and other flags though (gpiolib could maybe
split the group into sub-groups of 64 bits and use masks to handle
that quickly).

With his experience on pinctrl, Linus can probably come with more ideas. ;)

>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> ---
>  drivers/gpio/gpiolib.c        |   59 ++++++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h    |   17 ++++++++++++
>  include/linux/gpio.h          |   20 ++++++++++++++
>  include/linux/gpio/consumer.h |   14 +++++++++
>  include/linux/gpio/driver.h   |    3 ++
>  5 files changed, 113 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 50c4922..8355acb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2099,6 +2099,44 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_value);
>
> +static void _gpiod_set_raw_multiple(struct gpio_chip *chip,
> +                                   u64 mask, u64 value)
> +{
> +       if (chip->set_multi)
> +               chip->set_multi(chip, mask, value);
> +       else {
> +               int i;
> +               for (i=0; i<64; i++) {
> +                       if (i > chip->ngpio - 1)
> +                               break;

You will be looping over the 64 possible entries each and every time,
even if only the first 10 GPIOs are used. How about something like:

for (i = 0; mask != 0 && i < chip->ngpio; i++) {
    ... do stuff ...
    mask &= ~(((u64)1) << i);
}

That way you will stop as soon as all the GPIOs captured by the mask
have been processed.

I think this definitely needs some more discussion. But I find the
idea interesting.

Alex.

Re: [RFC PATCH] gpiolib: introduce gpio_set_multiple function

[Gerhard Sittig]

[ added Cc: to Roland Stigge ]

On Thu, Jan 23, 2014 at 13:19 +0100, Rojhalat Ibrahim wrote:
> 
> This patch introduces a new function gpio_set_multiple which allows setting
> multiple GPIO lines on the same chip with just one function call. If a
> corresponding set_multi function is defined for the chip performance can be
> significantly improved. Otherwise gpio_set_multiple uses the chip's set
> function and the performance stays the same as without the new function.
> For my application, where I have to frequently set 9 GPIO lines in order to
> configure an FPGA, configuration time goes down from 48 s to 15 s when I use
> this new function. (Tested with a modified gpio-mpc8xxx module.)

It's worth noting that performance is not the only concern, it
may be even more important to avoid glitches when adjusting the
levels of multiple pins which form some kind of bus (think
"parport").  Improved throughput may just be a byproduct.  How
you see it depends on your use case.

ISTR that Roland suggested some "block GPIO" approach in the
past, work on it has continued for quite long a period of time.
Apparently it has not yet gone into mainline, as the subject is
more complex than one might expect at first glance.

There are several concerns that need to get addressed in the
design, before an implementation could follow:
- do you want to restrict the use for groups of pins which all
  reside on the same chip?
- do you expect callers to know or make sure that these
  constraints are met?
- do you want to operate in a "best effort" manner, or will you
  reject requests that cannot get serviced "at the same time"?
  (see above, what is the motivation or use case?)

Current implementation of the Linux GPIO subsystem suggests
- that callers don't expect gpio_set_value() could ever fail
- that callers need not care, and should not care, which specific
  pin they are controlling, as long as they get a handle to it

Which suggests that the best effort approach would be most
appropriate:  Allow callers to specify any arbitrary group of
pins and which values to drive to them, try to process the
request with as few glitches as possible, still transparently
adjust individual pins and/or individual banks if nothing better
is available.  This results in
- callers still not caring about how the pins are attached in
  hardware (configuration is not encoded but data driven these
  days)
- best possible performance, automatically taking benefit from
  optional support for advanced features
- gpio chip drivers can get adjusted as they want to, or remain
  as they are and still keep working
- glitch free signal adjustment in "bus style" for those setups
  where the pins reside on the same bank and the chip driver
  supports parallel manipulation -- it's under control of the
  board designers whether they need or want this feature

None of the above says anything about the implementation, whether
a "parallel" code path is required to communicate multi-pin
set-operations, or whether some kind of "prepare and commit"
transaction style gets implemented (and what to do about
concurrent requests then).


There is not much one could say about your specific patch, as it
only introduces one callback for a GPIO chip, while it neither
provides an example implementation within a driver nor provides
an example caller demonstrating how the API is meant to get used,
and completely lacks any kind of documentation.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [RFC PATCH] gpiolib: introduce gpio_set_multiple function

[Rojhalat Ibrahim]

Thanks for your replies. I'll try to come up with a new patch which addresses
at least some of your points when I get around to it.

   Rojhalat


On Friday 24 January 2014 13:36:55 Gerhard Sittig wrote:
> [ added Cc: to Roland Stigge ]
> 
> On Thu, Jan 23, 2014 at 13:19 +0100, Rojhalat Ibrahim wrote:
> > 
> > This patch introduces a new function gpio_set_multiple which allows setting
> > multiple GPIO lines on the same chip with just one function call. If a
> > corresponding set_multi function is defined for the chip performance can be
> > significantly improved. Otherwise gpio_set_multiple uses the chip's set
> > function and the performance stays the same as without the new function.
> > For my application, where I have to frequently set 9 GPIO lines in order to
> > configure an FPGA, configuration time goes down from 48 s to 15 s when I use
> > this new function. (Tested with a modified gpio-mpc8xxx module.)
> 
> It's worth noting that performance is not the only concern, it
> may be even more important to avoid glitches when adjusting the
> levels of multiple pins which form some kind of bus (think
> "parport").  Improved throughput may just be a byproduct.  How
> you see it depends on your use case.
> 
> ISTR that Roland suggested some "block GPIO" approach in the
> past, work on it has continued for quite long a period of time.
> Apparently it has not yet gone into mainline, as the subject is
> more complex than one might expect at first glance.
> 
> There are several concerns that need to get addressed in the
> design, before an implementation could follow:
> - do you want to restrict the use for groups of pins which all
>   reside on the same chip?
> - do you expect callers to know or make sure that these
>   constraints are met?
> - do you want to operate in a "best effort" manner, or will you
>   reject requests that cannot get serviced "at the same time"?
>   (see above, what is the motivation or use case?)
> 
> Current implementation of the Linux GPIO subsystem suggests
> - that callers don't expect gpio_set_value() could ever fail
> - that callers need not care, and should not care, which specific
>   pin they are controlling, as long as they get a handle to it
> 
> Which suggests that the best effort approach would be most
> appropriate:  Allow callers to specify any arbitrary group of
> pins and which values to drive to them, try to process the
> request with as few glitches as possible, still transparently
> adjust individual pins and/or individual banks if nothing better
> is available.  This results in
> - callers still not caring about how the pins are attached in
>   hardware (configuration is not encoded but data driven these
>   days)
> - best possible performance, automatically taking benefit from
>   optional support for advanced features
> - gpio chip drivers can get adjusted as they want to, or remain
>   as they are and still keep working
> - glitch free signal adjustment in "bus style" for those setups
>   where the pins reside on the same bank and the chip driver
>   supports parallel manipulation -- it's under control of the
>   board designers whether they need or want this feature
> 
> None of the above says anything about the implementation, whether
> a "parallel" code path is required to communicate multi-pin
> set-operations, or whether some kind of "prepare and commit"
> transaction style gets implemented (and what to do about
> concurrent requests then).
> 
> 
> There is not much one could say about your specific patch, as it
> only introduces one callback for a GPIO chip, while it neither
> provides an example implementation within a driver nor provides
> an example caller demonstrating how the API is meant to get used,
> and completely lacks any kind of documentation.
> 
> 
> virtually yours
> Gerhard Sittig
>