* [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
@ 2014-05-27 11:14 Rojhalat Ibrahim
2014-05-29 10:10 ` Linus Walleij
2014-06-01 9:36 ` Alexandre Courbot
0 siblings, 2 replies; 6+ messages in thread
From: Rojhalat Ibrahim @ 2014-05-27 11:14 UTC (permalink / raw)
To: linux-gpio; +Cc: Alexandre Courbot, Gerhard Sittig, Linus Walleij
Introduce a new function gpiod_set_raw_array to the consumer interface which
allows setting multiple outputs with just one function call. Also add an
optional set_multiple function to the driver interface. Without an
implementation of that function in the chip driver outputs are set
sequentially.
Implementing the set_multiple function in a chip driver allows for:
- Improved performance for certain use cases. The original motivation for this
was the task of configuring an FPGA. In that specific case, where 9 GPIO
lines have to be set many times, configuration time goes down from 48 s to
19 s when using the new function.
- Simultaneous glitch-free setting of multiple pins on any kind of parallel
bus attached to GPIOs provided they all reside on the same chip and bank.
Limitations:
- Performance is only improved for normal high-low outputs. Open drain and
open source outputs are always set separately from each other. Those kinds
of outputs could probably be accelerated in a similar way if we could
forgo the error checking when setting GPIO directions.
- There is no gpiod_set_array function that regards the ACTIVE_LOW bits.
The raw function should be sufficient for many use cases. So I avoided
the code duplication the other functions would require.
Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
---
Change log:
v3: - add documentation
- change commit message
v2: - use descriptor interface
- allow arbitrary groups of GPIOs spanning multiple chips
Documentation/gpio/consumer.txt | 17 ++++++
drivers/gpio/gpiolib.c | 116 ++++++++++++++++++++++++++++++++++++++++
include/linux/gpio/consumer.h | 18 +++++++
include/linux/gpio/driver.h | 3 ++
4 files changed, 154 insertions(+)
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index 09854fe..affcd9b 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -163,6 +163,23 @@ The active-low state of a GPIO can also be queried using the following call:
Note that these functions should only be used with great moderation ; a driver
should not have to care about the physical line level.
+Set multiple GPIO outputs with a single function call
+-----------------------------------------------------
+The following two functions set the raw output values of an array of GPIOs:
+
+ void gpiod_set_raw_array(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array)
+ void gpiod_set_raw_array_cansleep(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array)
+
+The array can be an arbitrary set of GPIOs. The functions will try to set
+GPIOs belonging to the same bank or chip simultaneously if supported by the
+corresponding chip driver. In that case a significantly improved performance
+can be expected. If simultaneous setting is not possible the GPIOs will be set
+sequentially.
+
GPIOs mapped to IRQs
--------------------
GPIO lines can quite often be used as IRQs. You can get the IRQ number
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f48817d..ee67ef1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2345,6 +2345,77 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
chip->set(chip, gpio_chip_hwgpio(desc), value);
}
+static void _gpio_chip_set_multiple(struct gpio_chip *chip,
+ u32 mask[ARCH_NR_GPIOS/32],
+ u32 bits[ARCH_NR_GPIOS/32])
+{
+ if (chip->set_multiple) {
+ chip->set_multiple(chip, mask, bits);
+ } else {
+ int i;
+ for (i = 0; i < ARCH_NR_GPIOS; i++) {
+ if (i > chip->ngpio - 1)
+ break;
+ if (mask[i/32] == 0) {
+ /* skip ahead */
+ i = (i/32 + 1) * 32 - 1;
+ continue;
+ }
+ if (mask[i/32] & (1 << (i % 32))) {
+ chip->set(chip, i, (bits[i/32] >> (i % 32)) & 1);
+ mask[i/32] &= ~(1 << (i % 32));
+ }
+ }
+ }
+}
+
+static void _gpiod_set_raw_array(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array)
+{
+ struct gpio_chip *chip = desc_array[0]->chip;
+ u32 mask[ARCH_NR_GPIOS/32];
+ u32 bits[ARCH_NR_GPIOS/32];
+ int count = 0;
+ int i;
+
+ memset(mask, 0, sizeof(mask));
+ for (i = 0; i < array_size; i++) {
+ struct gpio_desc *desc = desc_array[i];
+ int hwgpio = gpio_chip_hwgpio(desc);
+ int value = value_array[i];
+
+ /* another chip; push collected bits to outputs */
+ if (desc->chip != chip) {
+ if (count != 0) {
+ _gpio_chip_set_multiple(chip, mask, bits);
+ memset(mask, 0, sizeof(mask));
+ count = 0;
+ }
+ chip = desc->chip;
+ }
+ /* collect all normal outputs belonging to the same chip */
+ /* open drain and open source outputs are set individually */
+ trace_gpio_value(desc_to_gpio(desc), 0, value);
+ if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
+ _gpio_set_open_drain_value(desc, value);
+ } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
+ _gpio_set_open_source_value(desc, value);
+ } else {
+ mask[hwgpio/32] |= (1 << (hwgpio % 32));
+ if (value) {
+ bits[hwgpio/32] |= (1 << (hwgpio % 32));
+ } else {
+ bits[hwgpio/32] &= ~(1 << (hwgpio % 32));
+ }
+ count++;
+ }
+ }
+ if (count != 0) {
+ _gpio_chip_set_multiple(chip, mask, bits);
+ }
+}
+
/**
* gpiod_set_raw_value() - assign a gpio's raw value
* @desc: gpio whose value will be assigned
@@ -2390,6 +2461,30 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
EXPORT_SYMBOL_GPL(gpiod_set_value);
/**
+ * gpiod_set_raw_array() - assign values to an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be assigned
+ * @value_array: array of values to assign
+ *
+ * Set the raw values of the GPIOs, i.e. the values of the physical lines
+ * 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_array(unsigned int array_size,
+ struct gpio_desc **desc_array, int *value_array)
+{
+ if (!desc_array)
+ return;
+ if (!desc_array[0])
+ return;
+ /* Should be using gpiod_set_raw_array_cansleep() */
+ WARN_ON(desc_array[0]->chip->can_sleep);
+ _gpiod_set_raw_array(array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array);
+
+/**
* gpiod_cansleep() - report whether gpio value access may sleep
* @desc: gpio to check
*
@@ -2559,6 +2654,27 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
/**
+ * gpiod_set_raw_array_cansleep() - assign values to an array of GPIOs
+ * @desc_array: array of GPIO descriptors whose values will be assigned
+ * @value_array: array of values to assign
+ *
+ * Set the raw values of the GPIOs, i.e. the values of the physical lines
+ * without regard for their ACTIVE_LOW status.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+void gpiod_set_raw_array_cansleep(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array)
+{
+ might_sleep_if(extra_checks);
+ if (!desc_array)
+ return;
+ _gpiod_set_raw_array(array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array_cansleep);
+
+/**
* gpiod_add_lookup_table() - register GPIO device consumers
* @table: table of consumers to register
*/
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index bed128e..1d0bab3 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -42,12 +42,17 @@ 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_array(unsigned int array_size,
+ struct gpio_desc **desc_array, int *value_array);
/* 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_array_cansleep(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array);
int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
@@ -150,6 +155,12 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
/* GPIO can never have been requested */
WARN_ON(1);
}
+void gpiod_set_raw_array(unsigned int array_size,
+ struct gpio_desc **desc_array, int *value_array)
+{
+ /* GPIO can never have been requested */
+ WARN_ON(1);
+}
static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
{
@@ -174,6 +185,13 @@ static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
/* GPIO can never have been requested */
WARN_ON(1);
}
+void gpiod_set_raw_array_cansleep(unsigned int array_size,
+ struct gpio_desc **desc_array,
+ int *value_array)
+{
+ /* 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 1827b43..d7968c8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -32,6 +32,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_multiple: 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;
@@ -84,6 +85,8 @@ struct gpio_chip {
unsigned offset);
void (*set)(struct gpio_chip *chip,
unsigned offset, int value);
+ void (*set_multiple)(struct gpio_chip *chip,
+ u32 *mask, u32 *bits);
int (*set_debounce)(struct gpio_chip *chip,
unsigned offset,
unsigned debounce);
--
1.8.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
2014-05-27 11:14 [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs Rojhalat Ibrahim
@ 2014-05-29 10:10 ` Linus Walleij
2014-06-01 19:31 ` Mark Brown
2014-06-02 8:33 ` Rojhalat Ibrahim
2014-06-01 9:36 ` Alexandre Courbot
1 sibling, 2 replies; 6+ messages in thread
From: Linus Walleij @ 2014-05-29 10:10 UTC (permalink / raw)
To: Rojhalat Ibrahim, Grant Likely
Cc: linux-gpio@vger.kernel.org, Alexandre Courbot, Gerhard Sittig,
Mark Brown
On Tue, May 27, 2014 at 1:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Introduce a new function gpiod_set_raw_array to the consumer interface which
> allows setting multiple outputs with just one function call. Also add an
> optional set_multiple function to the driver interface. Without an
> implementation of that function in the chip driver outputs are set
> sequentially.
>
> Implementing the set_multiple function in a chip driver allows for:
> - Improved performance for certain use cases. The original motivation for this
> was the task of configuring an FPGA. In that specific case, where 9 GPIO
> lines have to be set many times, configuration time goes down from 48 s to
> 19 s when using the new function.
> - Simultaneous glitch-free setting of multiple pins on any kind of parallel
> bus attached to GPIOs provided they all reside on the same chip and bank.
>
> Limitations:
> - Performance is only improved for normal high-low outputs. Open drain and
> open source outputs are always set separately from each other. Those kinds
> of outputs could probably be accelerated in a similar way if we could
> forgo the error checking when setting GPIO directions.
> - There is no gpiod_set_array function that regards the ACTIVE_LOW bits.
> The raw function should be sufficient for many use cases. So I avoided
> the code duplication the other functions would require.
>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> ---
> Change log:
> v3: - add documentation
> - change commit message
> v2: - use descriptor interface
> - allow arbitrary groups of GPIOs spanning multiple chips
This concept has been brought up before. Please keep Grant Likley in
the loop on these postings as he has some history with this.
As this is a speed optimization basically, do you have performance
numbers? Like how much quicker is your code in some critical
usecase as measured by perf or throughput whatever before and
after doing this?
Notice that the SPI bitbang driver in drivers/spi/spi-gpio.c was one
of the most important consumers of this API IIRC. So I'm CC:ing
Mark Brown.
> +The following two functions set the raw output values of an array of GPIOs:
> +
> + void gpiod_set_raw_array(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> + void gpiod_set_raw_array_cansleep(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
So this interface looks quite nice.
I don't see why the complex loop in the raw setter
below cannot actually also handle non-raw (cooked? hehe)
values with some single-liner to invert the value if it
is active low.
> +static void _gpio_chip_set_multiple(struct gpio_chip *chip,
I hate _underscored function names, just call it
gpio_chip_set_multiple_offsets() or whatever.
> + u32 mask[ARCH_NR_GPIOS/32],
> + u32 bits[ARCH_NR_GPIOS/32])
> +{
First: NO, you NEVER tie any code into ARCH_NR_GPIOS
because that is something we want to get rid of. Just use
regular pointers, add a count argument if need be.
> + if (chip->set_multiple) {
> + chip->set_multiple(chip, mask, bits);
Simple and quick OK. But even this is making a quite
nasty assumption...
> + } else {
> + int i;
> + for (i = 0; i < ARCH_NR_GPIOS; i++) {
As I said you cannot depend on ARCH_NR_GPIOS and not
introduce more dependencies to it. Figure out something else.
Like using chip->ngpio to iterate.
> + if (i > chip->ngpio - 1)
> + break;
> + if (mask[i/32] == 0) {
> + /* skip ahead */
> + i = (i/32 + 1) * 32 - 1;
> + continue;
> + }
> + if (mask[i/32] & (1 << (i % 32))) {
> + chip->set(chip, i, (bits[i/32] >> (i % 32)) & 1);
> + mask[i/32] &= ~(1 << (i % 32));
> + }
> + }
> + }
> +}
This code is a bit convoluted but I *guess* the idea is to iterate
over the 32 bits in the argument word by word. Add some
comments describing what's going on.
Some chips have 8bit or 16bit registers actually but AFAICT
that is not what this is about, rather about the fact that the
function takes 32bit arguments.
> +static void _gpiod_set_raw_array(unsigned int array_size,
No underscored function name.
> + struct gpio_desc **desc_array,
> + int *value_array)
> +{
> + struct gpio_chip *chip = desc_array[0]->chip;
> + u32 mask[ARCH_NR_GPIOS/32];
> + u32 bits[ARCH_NR_GPIOS/32];
Again dependency to ARCH_NR_GPIOS. Don't do it.
> + int count = 0;
> + int i;
> +
> + memset(mask, 0, sizeof(mask));
> + for (i = 0; i < array_size; i++) {
> + struct gpio_desc *desc = desc_array[i];
> + int hwgpio = gpio_chip_hwgpio(desc);
> + int value = value_array[i];
> +
> + /* another chip; push collected bits to outputs */
> + if (desc->chip != chip) {
Clever logic!
> + if (count != 0) {
> + _gpio_chip_set_multiple(chip, mask, bits);
> + memset(mask, 0, sizeof(mask));
> + count = 0;
> + }
> + chip = desc->chip;
> + }
> + /* collect all normal outputs belonging to the same chip */
> + /* open drain and open source outputs are set individually */
> + trace_gpio_value(desc_to_gpio(desc), 0, value);
> + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
> + _gpio_set_open_drain_value(desc, value);
> + } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
> + _gpio_set_open_source_value(desc, value);
> + } else {
> + mask[hwgpio/32] |= (1 << (hwgpio % 32));
> + if (value) {
> + bits[hwgpio/32] |= (1 << (hwgpio % 32));
> + } else {
> + bits[hwgpio/32] &= ~(1 << (hwgpio % 32));
> + }
> + count++;
> + }
This is also real smart and nice code.
The assumption is that this iteration hotloop will be quicker
to run, and the driver can take shortcuts improving performance.
As mentione above we need some proof of that as it complicates
the code a lot for this performance improvement.
> + }
> + if (count != 0) {
> + _gpio_chip_set_multiple(chip, mask, bits);
> + }
> +}
> +
(...)
> 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 1827b43..d7968c8 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -32,6 +32,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_multiple: 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;
> @@ -84,6 +85,8 @@ struct gpio_chip {
> unsigned offset);
> void (*set)(struct gpio_chip *chip,
> unsigned offset, int value);
> + void (*set_multiple)(struct gpio_chip *chip,
> + u32 *mask, u32 *bits);
This sets the chunk size to 32bits which means that any GPIO
controller with say 8bit registers will get a complex demuxing
scheme.
But thinking of it I think it's a reasonable compromise given that
so many GPIO drivers are actually 32bit.
However some of the real slow I2C expander GPIOs are 8bit.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
2014-05-27 11:14 [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs Rojhalat Ibrahim
2014-05-29 10:10 ` Linus Walleij
@ 2014-06-01 9:36 ` Alexandre Courbot
2014-06-02 9:00 ` Rojhalat Ibrahim
1 sibling, 1 reply; 6+ messages in thread
From: Alexandre Courbot @ 2014-06-01 9:36 UTC (permalink / raw)
To: Rojhalat Ibrahim
Cc: linux-gpio@vger.kernel.org, Gerhard Sittig, Linus Walleij,
Grant Likely
On Tue, May 27, 2014 at 8:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Introduce a new function gpiod_set_raw_array to the consumer interface which
> allows setting multiple outputs with just one function call. Also add an
> optional set_multiple function to the driver interface. Without an
> implementation of that function in the chip driver outputs are set
> sequentially.
>
> Implementing the set_multiple function in a chip driver allows for:
> - Improved performance for certain use cases. The original motivation for this
> was the task of configuring an FPGA. In that specific case, where 9 GPIO
> lines have to be set many times, configuration time goes down from 48 s to
> 19 s when using the new function.
> - Simultaneous glitch-free setting of multiple pins on any kind of parallel
> bus attached to GPIOs provided they all reside on the same chip and bank.
>
> Limitations:
> - Performance is only improved for normal high-low outputs. Open drain and
> open source outputs are always set separately from each other. Those kinds
> of outputs could probably be accelerated in a similar way if we could
> forgo the error checking when setting GPIO directions.
> - There is no gpiod_set_array function that regards the ACTIVE_LOW bits.
> The raw function should be sufficient for many use cases. So I avoided
> the code duplication the other functions would require.
How much effort would it be to also implement this function? IIUC it
should just require one extra pass of the values array prior to
calling gpiod_set_raw_array().
>
> Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> ---
> Change log:
> v3: - add documentation
> - change commit message
> v2: - use descriptor interface
> - allow arbitrary groups of GPIOs spanning multiple chips
>
> Documentation/gpio/consumer.txt | 17 ++++++
> drivers/gpio/gpiolib.c | 116 ++++++++++++++++++++++++++++++++++++++++
> include/linux/gpio/consumer.h | 18 +++++++
> include/linux/gpio/driver.h | 3 ++
> 4 files changed, 154 insertions(+)
>
> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index 09854fe..affcd9b 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -163,6 +163,23 @@ The active-low state of a GPIO can also be queried using the following call:
> Note that these functions should only be used with great moderation ; a driver
> should not have to care about the physical line level.
>
> +Set multiple GPIO outputs with a single function call
> +-----------------------------------------------------
> +The following two functions set the raw output values of an array of GPIOs:
> +
> + void gpiod_set_raw_array(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> + void gpiod_set_raw_array_cansleep(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> +
> +The array can be an arbitrary set of GPIOs. The functions will try to set
> +GPIOs belonging to the same bank or chip simultaneously if supported by the
> +corresponding chip driver. In that case a significantly improved performance
> +can be expected. If simultaneous setting is not possible the GPIOs will be set
> +sequentially.
Nice and clean interface, taking advantage of the descriptor interface.
> +
> GPIOs mapped to IRQs
> --------------------
> GPIO lines can quite often be used as IRQs. You can get the IRQ number
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f48817d..ee67ef1 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2345,6 +2345,77 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
> chip->set(chip, gpio_chip_hwgpio(desc), value);
> }
>
> +static void _gpio_chip_set_multiple(struct gpio_chip *chip,
> + u32 mask[ARCH_NR_GPIOS/32],
> + u32 bits[ARCH_NR_GPIOS/32])
> +{
> + if (chip->set_multiple) {
> + chip->set_multiple(chip, mask, bits);
> + } else {
> + int i;
> + for (i = 0; i < ARCH_NR_GPIOS; i++) {
As Linus already said, please avoid using the deprecated
ARCH_NR_GPIOS. chip->ngpio should be just as usable in your case...
> + if (i > chip->ngpio - 1)
> + break;
... and it would allow you to remove that test.
> + if (mask[i/32] == 0) {
> + /* skip ahead */
> + i = (i/32 + 1) * 32 - 1;
> + continue;
> + }
> + if (mask[i/32] & (1 << (i % 32))) {
BIT(i % 32)? (and everywhere else where it applies)
> + chip->set(chip, i, (bits[i/32] >> (i % 32)) & 1);
Why are you putting a space around the operator in "i % 32" but not in "i/32" ?
> + mask[i/32] &= ~(1 << (i % 32));
There are lots of mask[i/32] and bits[i/32] here. I'm sure the
compiler will optimize them away, but for code clarity it might be
worth to have each set of u32 be processed by another (inline)
function.
> + }
> + }
> + }
> +}
> +
> +static void _gpiod_set_raw_array(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> +{
> + struct gpio_chip *chip = desc_array[0]->chip;
> + u32 mask[ARCH_NR_GPIOS/32];
> + u32 bits[ARCH_NR_GPIOS/32];
If ARCH_NR_GPIOS is not a multiple of 32, your array will miss one
element. You should use DIV_ROUND_UP(ARCH_NR_GPIOS, 32), but even
better, let's not use ARCH_NR_GPIOS at all.
Maybe someone will kill me for suggesting this, but I wonder if you
could not use C99's VLA-on-stack feature here? Please rap my knuckles
if this is taboo in the kernel, but something like this may work
(untested though):
i = 0;
while (i < array_size) {
struct gpio_chip *chip = desc_array[i]->chip;
u32 mask[DIV_ROUND_UP(chip->ngpios, 32)];
....
do {
/* Process values for GPIOs of the same chip */
} while (++i < array_size && desc_array[i]->chip != chip);
_gpio_chip_set_multiple(...);
}
}
Something like this. It should also avoid some of the repetitions in
your code. That is, *if* the stack VLAs are usable at all and someone
does not fall on us shouting how evil this is.
> + int count = 0;
> + int i;
> +
> + memset(mask, 0, sizeof(mask));
> + for (i = 0; i < array_size; i++) {
> + struct gpio_desc *desc = desc_array[i];
> + int hwgpio = gpio_chip_hwgpio(desc);
> + int value = value_array[i];
> +
> + /* another chip; push collected bits to outputs */
> + if (desc->chip != chip) {
> + if (count != 0) {
> + _gpio_chip_set_multiple(chip, mask, bits);
> + memset(mask, 0, sizeof(mask));
> + count = 0;
> + }
> + chip = desc->chip;
> + }
> + /* collect all normal outputs belonging to the same chip */
> + /* open drain and open source outputs are set individually */
> + trace_gpio_value(desc_to_gpio(desc), 0, value);
> + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
> + _gpio_set_open_drain_value(desc, value);
> + } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
> + _gpio_set_open_source_value(desc, value);
> + } else {
> + mask[hwgpio/32] |= (1 << (hwgpio % 32));
__set_bit(&mask[hwgpio / 32], BIT(hwgpio % 32))
> + if (value) {
> + bits[hwgpio/32] |= (1 << (hwgpio % 32));
__set_bit(&bits[hwgpio / 32], BIT(hwgpio % 32))
> + } else {
> + bits[hwgpio/32] &= ~(1 << (hwgpio % 32));
__clear_bit(&bits[hwgpio / 32], BIT(hwgpio % 32));
> + }
> + count++;
> + }
> + }
> + if (count != 0) {
> + _gpio_chip_set_multiple(chip, mask, bits);
> + }
> +}
> +
> /**
> * gpiod_set_raw_value() - assign a gpio's raw value
> * @desc: gpio whose value will be assigned
> @@ -2390,6 +2461,30 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
> EXPORT_SYMBOL_GPL(gpiod_set_value);
>
> /**
> + * gpiod_set_raw_array() - assign values to an array of GPIOs
Miss documentation for array_size here.
> + * @desc_array: array of GPIO descriptors whose values will be assigned
> + * @value_array: array of values to assign
> + *
> + * Set the raw values of the GPIOs, i.e. the values of the physical lines
> + * 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_array(unsigned int array_size,
> + struct gpio_desc **desc_array, int *value_array)
> +{
> + if (!desc_array)
> + return;
> + if (!desc_array[0])
> + return;
What if array_size == 0? This seems to be more important to check that
the value of desc_array[0].
> + /* Should be using gpiod_set_raw_array_cansleep() */
> + WARN_ON(desc_array[0]->chip->can_sleep);
> + _gpiod_set_raw_array(array_size, desc_array, value_array);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_set_raw_array);
> +
> +/**
> * gpiod_cansleep() - report whether gpio value access may sleep
> * @desc: gpio to check
> *
> @@ -2559,6 +2654,27 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
> EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
>
> /**
> + * gpiod_set_raw_array_cansleep() - assign values to an array of GPIOs
array_size documentation missing here as well.
> + * @desc_array: array of GPIO descriptors whose values will be assigned
> + * @value_array: array of values to assign
> + *
> + * Set the raw values of the GPIOs, i.e. the values of the physical lines
> + * without regard for their ACTIVE_LOW status.
> + *
> + * This function is to be called from contexts that can sleep.
> + */
> +void gpiod_set_raw_array_cansleep(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> +{
> + might_sleep_if(extra_checks);
> + if (!desc_array)
> + return;
What happened to the other test here?
> + _gpiod_set_raw_array(array_size, desc_array, value_array);
> +}
> +EXPORT_SYMBOL_GPL(gpiod_set_raw_array_cansleep);
> +
> +/**
> * gpiod_add_lookup_table() - register GPIO device consumers
> * @table: table of consumers to register
> */
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index bed128e..1d0bab3 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -42,12 +42,17 @@ 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_array(unsigned int array_size,
> + struct gpio_desc **desc_array, int *value_array);
>
> /* 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_array_cansleep(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array);
>
> int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
>
> @@ -150,6 +155,12 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
> /* GPIO can never have been requested */
> WARN_ON(1);
> }
> +void gpiod_set_raw_array(unsigned int array_size,
> + struct gpio_desc **desc_array, int *value_array)
> +{
> + /* GPIO can never have been requested */
> + WARN_ON(1);
> +}
>
> static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
> {
> @@ -174,6 +185,13 @@ static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
> /* GPIO can never have been requested */
> WARN_ON(1);
> }
> +void gpiod_set_raw_array_cansleep(unsigned int array_size,
> + struct gpio_desc **desc_array,
> + int *value_array)
> +{
> + /* 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 1827b43..d7968c8 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -32,6 +32,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_multiple: 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;
> @@ -84,6 +85,8 @@ struct gpio_chip {
> unsigned offset);
> void (*set)(struct gpio_chip *chip,
> unsigned offset, int value);
> + void (*set_multiple)(struct gpio_chip *chip,
> + u32 *mask, u32 *bits);
> int (*set_debounce)(struct gpio_chip *chip,
> unsigned offset,
> unsigned debounce);
>
All in all I think this has good chances to be merged, once the issues
are addressed. The issue of setting multiple GPIOs simulatenously
comes regularly, and the performance improvement is undeniable here.
Alex.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
2014-05-29 10:10 ` Linus Walleij
@ 2014-06-01 19:31 ` Mark Brown
2014-06-02 8:33 ` Rojhalat Ibrahim
1 sibling, 0 replies; 6+ messages in thread
From: Mark Brown @ 2014-06-01 19:31 UTC (permalink / raw)
To: Linus Walleij
Cc: Rojhalat Ibrahim, Grant Likely, linux-gpio@vger.kernel.org,
Alexandre Courbot, Gerhard Sittig
[-- Attachment #1: Type: text/plain, Size: 904 bytes --]
On Thu, May 29, 2014 at 12:10:02PM +0200, Linus Walleij wrote:
> On Tue, May 27, 2014 at 1:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> > + void (*set_multiple)(struct gpio_chip *chip,
> > + u32 *mask, u32 *bits);
> This sets the chunk size to 32bits which means that any GPIO
> controller with say 8bit registers will get a complex demuxing
> scheme.
> But thinking of it I think it's a reasonable compromise given that
> so many GPIO drivers are actually 32bit.
> However some of the real slow I2C expander GPIOs are 8bit.
We can probably librify the demuxing code if we need it, though a lot of
those devices only have 8 GPIOs in the first place which makes life a
lot simpler since everything will always be in the same register anyway.
Overall the interface here looks good modulo your concerns about
ARCH_NR_GPIOs.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
2014-05-29 10:10 ` Linus Walleij
2014-06-01 19:31 ` Mark Brown
@ 2014-06-02 8:33 ` Rojhalat Ibrahim
1 sibling, 0 replies; 6+ messages in thread
From: Rojhalat Ibrahim @ 2014-06-02 8:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, linux-gpio@vger.kernel.org, Alexandre Courbot,
Gerhard Sittig, Mark Brown
On Thursday 29 May 2014 12:10:02 Linus Walleij wrote:
> On Tue, May 27, 2014 at 1:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
>
> > Introduce a new function gpiod_set_raw_array to the consumer interface which
> > allows setting multiple outputs with just one function call. Also add an
> > optional set_multiple function to the driver interface. Without an
> > implementation of that function in the chip driver outputs are set
> > sequentially.
> >
> > Implementing the set_multiple function in a chip driver allows for:
> > - Improved performance for certain use cases. The original motivation for this
> > was the task of configuring an FPGA. In that specific case, where 9 GPIO
> > lines have to be set many times, configuration time goes down from 48 s to
> > 19 s when using the new function.
> > - Simultaneous glitch-free setting of multiple pins on any kind of parallel
> > bus attached to GPIOs provided they all reside on the same chip and bank.
> >
> > Limitations:
> > - Performance is only improved for normal high-low outputs. Open drain and
> > open source outputs are always set separately from each other. Those kinds
> > of outputs could probably be accelerated in a similar way if we could
> > forgo the error checking when setting GPIO directions.
> > - There is no gpiod_set_array function that regards the ACTIVE_LOW bits.
> > The raw function should be sufficient for many use cases. So I avoided
> > the code duplication the other functions would require.
> >
> > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> > ---
> > Change log:
> > v3: - add documentation
> > - change commit message
> > v2: - use descriptor interface
> > - allow arbitrary groups of GPIOs spanning multiple chips
>
> This concept has been brought up before. Please keep Grant Likley in
> the loop on these postings as he has some history with this.
>
Will do.
> As this is a speed optimization basically, do you have performance
> numbers? Like how much quicker is your code in some critical
> usecase as measured by perf or throughput whatever before and
> after doing this?
>
As mentioned in the commit message my use case is the configuration of an
FPGA. Using the new function configuration time goes down from 48 s to 19 s.
> Notice that the SPI bitbang driver in drivers/spi/spi-gpio.c was one
> of the most important consumers of this API IIRC. So I'm CC:ing
> Mark Brown.
>
> > +The following two functions set the raw output values of an array of GPIOs:
> > +
> > + void gpiod_set_raw_array(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > + void gpiod_set_raw_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
>
> So this interface looks quite nice.
>
Thanks.
> I don't see why the complex loop in the raw setter
> below cannot actually also handle non-raw (cooked? hehe)
> values with some single-liner to invert the value if it
> is active low.
>
I will look into that. I did not want to invert the active-low values in
the array first and then pass the array of partly inverted values to the
_gpiod_set_raw_array function because that would change the array values
passed by the user.
Another way to handle the non-raw values would of course be to do it in the
same function as the raw values. That would require additional checks and
would possibly degrade performance for the raw case. But the performance
impact could also be negligible. So as I said I will look into it.
> > +static void _gpio_chip_set_multiple(struct gpio_chip *chip,
>
> I hate _underscored function names, just call it
> gpio_chip_set_multiple_offsets() or whatever.
>
Ok.
> > + u32 mask[ARCH_NR_GPIOS/32],
> > + u32 bits[ARCH_NR_GPIOS/32])
> > +{
>
> First: NO, you NEVER tie any code into ARCH_NR_GPIOS
> because that is something we want to get rid of. Just use
> regular pointers, add a count argument if need be.
>
Will do.
> > + if (chip->set_multiple) {
> > + chip->set_multiple(chip, mask, bits);
>
> Simple and quick OK. But even this is making a quite
> nasty assumption...
>
Could you elaborate? What's the nasty assumption?
> > + } else {
> > + int i;
> > + for (i = 0; i < ARCH_NR_GPIOS; i++) {
>
> As I said you cannot depend on ARCH_NR_GPIOS and not
> introduce more dependencies to it. Figure out something else.
> Like using chip->ngpio to iterate.
>
Ok.
> > + if (i > chip->ngpio - 1)
> > + break;
> > + if (mask[i/32] == 0) {
> > + /* skip ahead */
> > + i = (i/32 + 1) * 32 - 1;
> > + continue;
> > + }
> > + if (mask[i/32] & (1 << (i % 32))) {
> > + chip->set(chip, i, (bits[i/32] >> (i % 32)) & 1);
> > + mask[i/32] &= ~(1 << (i % 32));
> > + }
> > + }
> > + }
> > +}
>
> This code is a bit convoluted but I *guess* the idea is to iterate
> over the 32 bits in the argument word by word. Add some
> comments describing what's going on.
>
Will do.
> Some chips have 8bit or 16bit registers actually but AFAICT
> that is not what this is about, rather about the fact that the
> function takes 32bit arguments.
>
Right.
> > +static void _gpiod_set_raw_array(unsigned int array_size,
>
> No underscored function name.
>
Ok.
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +{
> > + struct gpio_chip *chip = desc_array[0]->chip;
> > + u32 mask[ARCH_NR_GPIOS/32];
> > + u32 bits[ARCH_NR_GPIOS/32];
>
> Again dependency to ARCH_NR_GPIOS. Don't do it.
>
Ok.
> > + int count = 0;
> > + int i;
> > +
> > + memset(mask, 0, sizeof(mask));
> > + for (i = 0; i < array_size; i++) {
> > + struct gpio_desc *desc = desc_array[i];
> > + int hwgpio = gpio_chip_hwgpio(desc);
> > + int value = value_array[i];
> > +
> > + /* another chip; push collected bits to outputs */
> > + if (desc->chip != chip) {
>
> Clever logic!
>
Thanks.
> > + if (count != 0) {
> > + _gpio_chip_set_multiple(chip, mask, bits);
> > + memset(mask, 0, sizeof(mask));
> > + count = 0;
> > + }
> > + chip = desc->chip;
> > + }
> > + /* collect all normal outputs belonging to the same chip */
> > + /* open drain and open source outputs are set individually */
> > + trace_gpio_value(desc_to_gpio(desc), 0, value);
> > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
> > + _gpio_set_open_drain_value(desc, value);
> > + } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
> > + _gpio_set_open_source_value(desc, value);
> > + } else {
> > + mask[hwgpio/32] |= (1 << (hwgpio % 32));
> > + if (value) {
> > + bits[hwgpio/32] |= (1 << (hwgpio % 32));
> > + } else {
> > + bits[hwgpio/32] &= ~(1 << (hwgpio % 32));
> > + }
> > + count++;
> > + }
>
> This is also real smart and nice code.
>
Thanks again.
> The assumption is that this iteration hotloop will be quicker
> to run, and the driver can take shortcuts improving performance.
> As mentione above we need some proof of that as it complicates
> the code a lot for this performance improvement.
>
See above.
> > + }
> > + if (count != 0) {
> > + _gpio_chip_set_multiple(chip, mask, bits);
> > + }
> > +}
> > +
>
> (...)
>
> > 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 1827b43..d7968c8 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -32,6 +32,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_multiple: 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;
> > @@ -84,6 +85,8 @@ struct gpio_chip {
> > unsigned offset);
> > void (*set)(struct gpio_chip *chip,
> > unsigned offset, int value);
> > + void (*set_multiple)(struct gpio_chip *chip,
> > + u32 *mask, u32 *bits);
>
> This sets the chunk size to 32bits which means that any GPIO
> controller with say 8bit registers will get a complex demuxing
> scheme.
>
> But thinking of it I think it's a reasonable compromise given that
> so many GPIO drivers are actually 32bit.
>
> However some of the real slow I2C expander GPIOs are 8bit.
>
So I'll leave the chunk size at 32 bits.
> Yours,
> Linus Walleij
>
Thanks for the review. I'll send a new revision of the patch soon.
Rojhalat
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs
2014-06-01 9:36 ` Alexandre Courbot
@ 2014-06-02 9:00 ` Rojhalat Ibrahim
0 siblings, 0 replies; 6+ messages in thread
From: Rojhalat Ibrahim @ 2014-06-02 9:00 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-gpio@vger.kernel.org, Gerhard Sittig, Linus Walleij,
Grant Likely
On Sunday 01 June 2014 18:36:32 Alexandre Courbot wrote:
> On Tue, May 27, 2014 at 8:14 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> > Introduce a new function gpiod_set_raw_array to the consumer interface which
> > allows setting multiple outputs with just one function call. Also add an
> > optional set_multiple function to the driver interface. Without an
> > implementation of that function in the chip driver outputs are set
> > sequentially.
> >
> > Implementing the set_multiple function in a chip driver allows for:
> > - Improved performance for certain use cases. The original motivation for this
> > was the task of configuring an FPGA. In that specific case, where 9 GPIO
> > lines have to be set many times, configuration time goes down from 48 s to
> > 19 s when using the new function.
> > - Simultaneous glitch-free setting of multiple pins on any kind of parallel
> > bus attached to GPIOs provided they all reside on the same chip and bank.
> >
> > Limitations:
> > - Performance is only improved for normal high-low outputs. Open drain and
> > open source outputs are always set separately from each other. Those kinds
> > of outputs could probably be accelerated in a similar way if we could
> > forgo the error checking when setting GPIO directions.
> > - There is no gpiod_set_array function that regards the ACTIVE_LOW bits.
> > The raw function should be sufficient for many use cases. So I avoided
> > the code duplication the other functions would require.
>
> How much effort would it be to also implement this function? IIUC it
> should just require one extra pass of the values array prior to
> calling gpiod_set_raw_array().
>
I did not want to do that because that would change the array values
passed by the user. Or would that be acceptable?
> >
> > Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
> > ---
> > Change log:
> > v3: - add documentation
> > - change commit message
> > v2: - use descriptor interface
> > - allow arbitrary groups of GPIOs spanning multiple chips
> >
> > Documentation/gpio/consumer.txt | 17 ++++++
> > drivers/gpio/gpiolib.c | 116 ++++++++++++++++++++++++++++++++++++++++
> > include/linux/gpio/consumer.h | 18 +++++++
> > include/linux/gpio/driver.h | 3 ++
> > 4 files changed, 154 insertions(+)
> >
> > diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> > index 09854fe..affcd9b 100644
> > --- a/Documentation/gpio/consumer.txt
> > +++ b/Documentation/gpio/consumer.txt
> > @@ -163,6 +163,23 @@ The active-low state of a GPIO can also be queried using the following call:
> > Note that these functions should only be used with great moderation ; a driver
> > should not have to care about the physical line level.
> >
> > +Set multiple GPIO outputs with a single function call
> > +-----------------------------------------------------
> > +The following two functions set the raw output values of an array of GPIOs:
> > +
> > + void gpiod_set_raw_array(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > + void gpiod_set_raw_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +
> > +The array can be an arbitrary set of GPIOs. The functions will try to set
> > +GPIOs belonging to the same bank or chip simultaneously if supported by the
> > +corresponding chip driver. In that case a significantly improved performance
> > +can be expected. If simultaneous setting is not possible the GPIOs will be set
> > +sequentially.
>
> Nice and clean interface, taking advantage of the descriptor interface.
>
> > +
> > GPIOs mapped to IRQs
> > --------------------
> > GPIO lines can quite often be used as IRQs. You can get the IRQ number
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index f48817d..ee67ef1 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2345,6 +2345,77 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
> > chip->set(chip, gpio_chip_hwgpio(desc), value);
> > }
> >
> > +static void _gpio_chip_set_multiple(struct gpio_chip *chip,
> > + u32 mask[ARCH_NR_GPIOS/32],
> > + u32 bits[ARCH_NR_GPIOS/32])
> > +{
> > + if (chip->set_multiple) {
> > + chip->set_multiple(chip, mask, bits);
> > + } else {
> > + int i;
> > + for (i = 0; i < ARCH_NR_GPIOS; i++) {
>
> As Linus already said, please avoid using the deprecated
> ARCH_NR_GPIOS. chip->ngpio should be just as usable in your case...
>
> > + if (i > chip->ngpio - 1)
> > + break;
>
> ... and it would allow you to remove that test.
>
Ok.
> > + if (mask[i/32] == 0) {
> > + /* skip ahead */
> > + i = (i/32 + 1) * 32 - 1;
> > + continue;
> > + }
> > + if (mask[i/32] & (1 << (i % 32))) {
>
> BIT(i % 32)? (and everywhere else where it applies)
>
Will do.
> > + chip->set(chip, i, (bits[i/32] >> (i % 32)) & 1);
>
> Why are you putting a space around the operator in "i % 32" but not in "i/32" ?
>
I will do it consistently in the next revision.
> > + mask[i/32] &= ~(1 << (i % 32));
>
> There are lots of mask[i/32] and bits[i/32] here. I'm sure the
> compiler will optimize them away, but for code clarity it might be
> worth to have each set of u32 be processed by another (inline)
> function.
>
I'll look into that.
> > + }
> > + }
> > + }
> > +}
> > +
> > +static void _gpiod_set_raw_array(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +{
> > + struct gpio_chip *chip = desc_array[0]->chip;
> > + u32 mask[ARCH_NR_GPIOS/32];
> > + u32 bits[ARCH_NR_GPIOS/32];
>
> If ARCH_NR_GPIOS is not a multiple of 32, your array will miss one
> element. You should use DIV_ROUND_UP(ARCH_NR_GPIOS, 32), but even
> better, let's not use ARCH_NR_GPIOS at all.
>
> Maybe someone will kill me for suggesting this, but I wonder if you
> could not use C99's VLA-on-stack feature here? Please rap my knuckles
> if this is taboo in the kernel, but something like this may work
> (untested though):
>
> i = 0;
>
> while (i < array_size) {
> struct gpio_chip *chip = desc_array[i]->chip;
> u32 mask[DIV_ROUND_UP(chip->ngpios, 32)];
> ....
>
> do {
> /* Process values for GPIOs of the same chip */
> } while (++i < array_size && desc_array[i]->chip != chip);
>
> _gpio_chip_set_multiple(...);
> }
> }
>
> Something like this. It should also avoid some of the repetitions in
> your code. That is, *if* the stack VLAs are usable at all and someone
> does not fall on us shouting how evil this is.
>
I'll look into that, too.
> > + int count = 0;
> > + int i;
> > +
> > + memset(mask, 0, sizeof(mask));
> > + for (i = 0; i < array_size; i++) {
> > + struct gpio_desc *desc = desc_array[i];
> > + int hwgpio = gpio_chip_hwgpio(desc);
> > + int value = value_array[i];
> > +
> > + /* another chip; push collected bits to outputs */
> > + if (desc->chip != chip) {
> > + if (count != 0) {
> > + _gpio_chip_set_multiple(chip, mask, bits);
> > + memset(mask, 0, sizeof(mask));
> > + count = 0;
> > + }
> > + chip = desc->chip;
> > + }
> > + /* collect all normal outputs belonging to the same chip */
> > + /* open drain and open source outputs are set individually */
> > + trace_gpio_value(desc_to_gpio(desc), 0, value);
> > + if (test_bit(FLAG_OPEN_DRAIN, &desc->flags)) {
> > + _gpio_set_open_drain_value(desc, value);
> > + } else if (test_bit(FLAG_OPEN_SOURCE, &desc->flags)) {
> > + _gpio_set_open_source_value(desc, value);
> > + } else {
> > + mask[hwgpio/32] |= (1 << (hwgpio % 32));
>
> __set_bit(&mask[hwgpio / 32], BIT(hwgpio % 32))
>
Ok.
> > + if (value) {
> > + bits[hwgpio/32] |= (1 << (hwgpio % 32));
>
> __set_bit(&bits[hwgpio / 32], BIT(hwgpio % 32))
>
Ok.
> > + } else {
> > + bits[hwgpio/32] &= ~(1 << (hwgpio % 32));
>
> __clear_bit(&bits[hwgpio / 32], BIT(hwgpio % 32));
>
Ok.
> > + }
> > + count++;
> > + }
> > + }
> > + if (count != 0) {
> > + _gpio_chip_set_multiple(chip, mask, bits);
> > + }
> > +}
> > +
> > /**
> > * gpiod_set_raw_value() - assign a gpio's raw value
> > * @desc: gpio whose value will be assigned
> > @@ -2390,6 +2461,30 @@ void gpiod_set_value(struct gpio_desc *desc, int value)
> > EXPORT_SYMBOL_GPL(gpiod_set_value);
> >
> > /**
> > + * gpiod_set_raw_array() - assign values to an array of GPIOs
>
> Miss documentation for array_size here.
>
Will fix that in the next revision.
> > + * @desc_array: array of GPIO descriptors whose values will be assigned
> > + * @value_array: array of values to assign
> > + *
> > + * Set the raw values of the GPIOs, i.e. the values of the physical lines
> > + * 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_array(unsigned int array_size,
> > + struct gpio_desc **desc_array, int *value_array)
> > +{
> > + if (!desc_array)
> > + return;
> > + if (!desc_array[0])
> > + return;
>
> What if array_size == 0? This seems to be more important to check that
> the value of desc_array[0].
>
Will add a check in the next revision.
desc_array[0] is checked because it is used here:
WARN_ON(desc_array[0]->chip->can_sleep)
> > + /* Should be using gpiod_set_raw_array_cansleep() */
> > + WARN_ON(desc_array[0]->chip->can_sleep);
> > + _gpiod_set_raw_array(array_size, desc_array, value_array);
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_set_raw_array);
> > +
> > +/**
> > * gpiod_cansleep() - report whether gpio value access may sleep
> > * @desc: gpio to check
> > *
> > @@ -2559,6 +2654,27 @@ void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
> > EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
> >
> > /**
> > + * gpiod_set_raw_array_cansleep() - assign values to an array of GPIOs
>
> array_size documentation missing here as well.
>
Will fix that in the next revision.
> > + * @desc_array: array of GPIO descriptors whose values will be assigned
> > + * @value_array: array of values to assign
> > + *
> > + * Set the raw values of the GPIOs, i.e. the values of the physical lines
> > + * without regard for their ACTIVE_LOW status.
> > + *
> > + * This function is to be called from contexts that can sleep.
> > + */
> > +void gpiod_set_raw_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +{
> > + might_sleep_if(extra_checks);
> > + if (!desc_array)
> > + return;
>
> What happened to the other test here?
>
desc_array[0] is not checked here because it is not used in that function.
I will add a check for array_size == 0 in the next revision.
> > + _gpiod_set_raw_array(array_size, desc_array, value_array);
> > +}
> > +EXPORT_SYMBOL_GPL(gpiod_set_raw_array_cansleep);
> > +
> > +/**
> > * gpiod_add_lookup_table() - register GPIO device consumers
> > * @table: table of consumers to register
> > */
> > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> > index bed128e..1d0bab3 100644
> > --- a/include/linux/gpio/consumer.h
> > +++ b/include/linux/gpio/consumer.h
> > @@ -42,12 +42,17 @@ 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_array(unsigned int array_size,
> > + struct gpio_desc **desc_array, int *value_array);
> >
> > /* 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_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array);
> >
> > int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
> >
> > @@ -150,6 +155,12 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
> > /* GPIO can never have been requested */
> > WARN_ON(1);
> > }
> > +void gpiod_set_raw_array(unsigned int array_size,
> > + struct gpio_desc **desc_array, int *value_array)
> > +{
> > + /* GPIO can never have been requested */
> > + WARN_ON(1);
> > +}
> >
> > static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
> > {
> > @@ -174,6 +185,13 @@ static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
> > /* GPIO can never have been requested */
> > WARN_ON(1);
> > }
> > +void gpiod_set_raw_array_cansleep(unsigned int array_size,
> > + struct gpio_desc **desc_array,
> > + int *value_array)
> > +{
> > + /* 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 1827b43..d7968c8 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -32,6 +32,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_multiple: 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;
> > @@ -84,6 +85,8 @@ struct gpio_chip {
> > unsigned offset);
> > void (*set)(struct gpio_chip *chip,
> > unsigned offset, int value);
> > + void (*set_multiple)(struct gpio_chip *chip,
> > + u32 *mask, u32 *bits);
> > int (*set_debounce)(struct gpio_chip *chip,
> > unsigned offset,
> > unsigned debounce);
> >
>
> All in all I think this has good chances to be merged, once the issues
> are addressed. The issue of setting multiple GPIOs simulatenously
> comes regularly, and the performance improvement is undeniable here.
>
> Alex.
>
Thanks for the review. I'll send a new revision of the patch soon.
Rojhalat
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-06-02 9:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-27 11:14 [PATCH 1/2][v3] gpiolib: allow simultaneous setting of multiple GPIO outputs Rojhalat Ibrahim
2014-05-29 10:10 ` Linus Walleij
2014-06-01 19:31 ` Mark Brown
2014-06-02 8:33 ` Rojhalat Ibrahim
2014-06-01 9:36 ` Alexandre Courbot
2014-06-02 9:00 ` Rojhalat Ibrahim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).