[PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
interface which allow 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
  20 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.

Signed-off-by: Rojhalat Ibrahim <imr@rtschenk.de>
---
Change log:
  v5: - check can_sleep property per chip
      - remove superfluous checks
      - supplement documentation
  v4: - add gpiod_set_array function for setting logical values
      - change interface of the set_multiple driver function to use
        unsigned long as type for the bit fields
      - use generic bitops (which also use unsigned long for bit fields)
      - do not use ARCH_NR_GPIOS any more
  v3: - add documentation
      - change commit message
  v2: - use descriptor interface
      - allow arbitrary groups of GPIOs spanning multiple chips

 Documentation/gpio/consumer.txt |  25 ++++++
 drivers/gpio/gpiolib.c          | 168 ++++++++++++++++++++++++++++++++++++++++
 include/linux/gpio/consumer.h   |  38 +++++++++
 include/linux/gpio/driver.h     |   4 +
 4 files changed, 235 insertions(+)

diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index d8abfc3..369e663 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -163,6 +163,31 @@ 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 functions set the output values of an array of GPIOs:
+
+	void gpiod_set_array(unsigned int array_size,
+			     struct gpio_desc **desc_array,
+			     int *value_array)
+	void gpiod_set_raw_array(unsigned int array_size,
+				 struct gpio_desc **desc_array,
+				 int *value_array)
+	void gpiod_set_array_cansleep(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.
+Note that for optimal performance GPIOs belonging to the same chip should be
+contiguous within the array of descriptors.
+
 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 2ebc907..455e098 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2370,6 +2370,88 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, bool value)
 		chip->set(chip, gpio_chip_hwgpio(desc), value);
 }
 
+/*
+ * set multiple outputs on the same chip;
+ * use the chip's set_multiple function if available;
+ * otherwise set the outputs sequentially;
+ * @mask: bit mask array; one bit per output; BITS_PER_LONG bits per word
+ *        defines which outputs are to be changed
+ * @bits: bit value array; one bit per output; BITS_PER_LONG bits per word
+ *        defines the values the outputs specified by mask are to be set to
+ */
+static void gpio_chip_set_multiple(struct gpio_chip *chip,
+				   unsigned long *mask, unsigned long *bits)
+{
+	if (chip->set_multiple) {
+		chip->set_multiple(chip, mask, bits);
+	} else {
+		int i;
+		for (i = 0; i < chip->ngpio; i++) {
+			if (mask[BIT_WORD(i)] == 0) {
+				/* no more set bits in this mask word;
+				 * skip ahead to the next word */
+				i = (BIT_WORD(i) + 1) * BITS_PER_LONG - 1;
+				continue;
+			}
+			/* set outputs if the corresponding mask bit is set */
+			if (__test_and_clear_bit(i, mask)) {
+				chip->set(chip, i, test_bit(i, bits));
+			}
+		}
+	}
+}
+
+static void gpiod_set_array_priv(bool raw, bool can_sleep,
+				 unsigned int array_size,
+				 struct gpio_desc **desc_array,
+				 int *value_array)
+{
+	int i = 0;
+
+	while (i < array_size) {
+		struct gpio_chip *chip = desc_array[i]->chip;
+		unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
+		unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+		int count = 0;
+
+		if (!can_sleep) {
+			WARN_ON(chip->can_sleep);
+		}
+		memset(mask, 0, sizeof(mask));
+		do {
+			struct gpio_desc *desc = desc_array[i];
+			int hwgpio = gpio_chip_hwgpio(desc);
+			int value = value_array[i];
+
+			if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags))
+				value = !value;
+			trace_gpio_value(desc_to_gpio(desc), 0, value);
+			/*
+			 * collect all normal outputs belonging to the same chip
+			 * open drain and open source outputs are set individually
+			 */
+			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 {
+				__set_bit(hwgpio, mask);
+				if (value) {
+					__set_bit(hwgpio, bits);
+				} else {
+					__clear_bit(hwgpio, bits);
+				}
+				count++;
+			}
+			i++;
+		} while ((i < array_size) && (desc_array[i]->chip == chip));
+		/* push collected bits to outputs */
+		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
@@ -2415,6 +2497,48 @@ 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
+ * @array_size: number of elements in the descriptor / value arrays
+ * @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;
+	gpiod_set_array_priv(true, false, array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array);
+
+/**
+ * gpiod_set_array() - assign values to an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be assigned
+ * @value_array: array of values to assign
+ *
+ * Set the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.
+ *
+ * This function should be called from contexts where we cannot sleep, and will
+ * complain if the GPIO chip functions potentially sleep.
+ */
+void gpiod_set_array(unsigned int array_size,
+		     struct gpio_desc **desc_array, int *value_array)
+{
+	if (!desc_array)
+		return;
+	gpiod_set_array_priv(false, false, array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_array);
+
+/**
  * gpiod_cansleep() - report whether gpio value access may sleep
  * @desc: gpio to check
  *
@@ -2584,6 +2708,50 @@ 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: number of elements in the descriptor / value arrays
+ * @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_array_priv(true, true, array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_raw_array_cansleep);
+
+/**
+ * gpiod_set_array_cansleep() - assign values to an array of GPIOs
+ * @array_size: number of elements in the descriptor / value arrays
+ * @desc_array: array of GPIO descriptors whose values will be assigned
+ * @value_array: array of values to assign
+ *
+ * Set the logical values of the GPIOs, i.e. taking their ACTIVE_LOW status
+ * into account.
+ *
+ * This function is to be called from contexts that can sleep.
+ */
+void gpiod_set_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_array_priv(false, true, array_size, desc_array, value_array);
+}
+EXPORT_SYMBOL_GPL(gpiod_set_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 05e53cc..e51abe3 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -53,14 +53,24 @@ int gpiod_direction_output_raw(struct gpio_desc *desc, int value);
 /* Value get/set from non-sleeping context */
 int gpiod_get_value(const struct gpio_desc *desc);
 void gpiod_set_value(struct gpio_desc *desc, int value);
+void gpiod_set_array(unsigned int array_size,
+		     struct gpio_desc **desc_array, int *value_array);
 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);
+void gpiod_set_array_cansleep(unsigned int array_size,
+			      struct gpio_desc **desc_array,
+			      int *value_array);
 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);
 
@@ -180,6 +190,13 @@ static inline void gpiod_set_value(struct gpio_desc *desc, int value)
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
+static inline void gpiod_set_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_raw_value(const struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
@@ -191,6 +208,13 @@ 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_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)
 {
@@ -203,6 +227,13 @@ static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
+static inline void gpiod_set_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_get_raw_value_cansleep(const struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
@@ -215,6 +246,13 @@ 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_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 573e4f3..4814250 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;
@@ -87,6 +88,9 @@ struct gpio_chip {
 						unsigned offset);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	void			(*set_multiple)(struct gpio_chip *chip,
+						unsigned long *mask,
+						unsigned long *bits);
 	int			(*set_debounce)(struct gpio_chip *chip,
 						unsigned offset,
 						unsigned debounce);

--
1.8.5.5

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Alexandre Courbot]

On Mon, Jun 16, 2014 at 10:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
> interface which allow 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
>   20 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.

Tried to find something more to say, but each and every concern I had
has been addressed through the various revisions of this series.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

As far as I can judge, this is a very nicely crafted patch.

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Mon, Jun 16, 2014 at 03:51:23PM +0200, Rojhalat Ibrahim wrote:
> Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
> interface which allow 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.

I'm pretty sure I reviewed the external interfaces last time and they
don't seem to have changed so:

Reviwed-by: Mark Brown <broonie@linaro.org>

Please carry forward tags that still apply so people don't need to
duplicate review.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Linus Walleij]

On Mon, Jun 16, 2014 at 3:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:

> Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
> interface which allow 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.

Yes this looks good.

Except for one thing I mentioned quite early  I think:

no users!

How am I supposed to test this?

I wanted to see a patch also switching the one in-kernel thing that
really needs this switched over to using this functionality.

That means, a patch to drivers/i2c/busses/i2c-gpio.c
to set scl+sda in one go, for example.

Or a similar patch to drivers/spi/spi-gpio.c I guess?

Something that makes sense. As a bonus you get to rewrite
one of these drivers to use GPIO descriptors ;)

This is not a hard wall, but I highly suspect that there must be
some driver using this if you want to implement it, right? And the
two mentioned drivers will be easy to use to test the feature on
a lot of platforms.

Yours,
Linus Walleij

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

On Monday 07 July 2014 17:23:34 Linus Walleij wrote:
> On Mon, Jun 16, 2014 at 3:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> 
> > Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
> > interface which allow 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.
> 
> Yes this looks good.
> 
> Except for one thing I mentioned quite early  I think:
> 
> no users!
> 

I'm pretty sure you did _not_ mention this before.

> How am I supposed to test this?
> 
> I wanted to see a patch also switching the one in-kernel thing that
> really needs this switched over to using this functionality.
> 
> That means, a patch to drivers/i2c/busses/i2c-gpio.c
> to set scl+sda in one go, for example.
> 
> Or a similar patch to drivers/spi/spi-gpio.c I guess?
> 
> Something that makes sense. As a bonus you get to rewrite
> one of these drivers to use GPIO descriptors ;)
> 
> This is not a hard wall, but I highly suspect that there must be
> some driver using this if you want to implement it, right? And the
> two mentioned drivers will be easy to use to test the feature on
> a lot of platforms.
> 

I'll take a look at those drivers and hopefully come back with an extended
patch set.

   Rojhalat

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Linus Walleij]

On Mon, Jul 7, 2014 at 6:12 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> On Monday 07 July 2014 17:23:34 Linus Walleij wrote:
>> On Mon, Jun 16, 2014 at 3:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
>>
>> > Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
>> > interface which allow 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.
>>
>> Yes this looks good.
>>
>> Except for one thing I mentioned quite early  I think:
>>
>> no users!
>
> I'm pretty sure you did _not_ mention this before.

I'm not referring to your patch set specifically. The idea of array handling
has been proposed something like three times, I cannot tell these discussion
threads apart in my head.

But it's a reasonable request don't you think?

> I'll take a look at those drivers and hopefully come back with an extended
> patch set.

Thanks.

Yours,
Linus Walleij

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

On Wednesday 09 July 2014 12:17:13 Linus Walleij wrote:
> On Mon, Jul 7, 2014 at 6:12 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> > On Monday 07 July 2014 17:23:34 Linus Walleij wrote:
> >> On Mon, Jun 16, 2014 at 3:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> >>
> >> > Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
> >> > interface which allow 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.
> >>
> >> Yes this looks good.
> >>
> >> Except for one thing I mentioned quite early  I think:
> >>
> >> no users!
> >
> > I'm pretty sure you did _not_ mention this before.
> 
> I'm not referring to your patch set specifically. The idea of array handling
> has been proposed something like three times, I cannot tell these discussion
> threads apart in my head.
> 
> But it's a reasonable request don't you think?
> 

The request might be reasonable.

But after taking a look at the i2c-gpio and spi-gpio drivers, I do not think they
would benefit from the new feature. In both cases there are only two GPIO lines.
And they have to be set separately at least half of the time.

I could not find any other in-kernel driver that might benefit from the new feature, either.

So I guess you are right: There are no users for this.

For my use-case the feature is very beneficial, but of course only in combination
with an out-of-kernel driver module for configuring the FPGA on my custom board.
I posted the patch because I expected similar use-cases to exist for other people
out there. If you think that's not the case, there is nothing I can do about it.

   Rojhalat

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Wed, Jul 30, 2014 at 11:12:30AM +0200, Rojhalat Ibrahim wrote:

Please fix your mailer to word wrap within paragraphs.

> I could not find any other in-kernel driver that might benefit from the new feature, either.

> So I guess you are right: There are no users for this.

The usual users for this are devices like LCD panels or other parallel
buses.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

On Wednesday 30 July 2014 11:41:30 Mark Brown wrote:
> On Wed, Jul 30, 2014 at 11:12:30AM +0200, Rojhalat Ibrahim wrote:
> 
> Please fix your mailer to word wrap within paragraphs.
> 
> > I could not find any other in-kernel driver that might benefit from the new feature, either.
> 
> > So I guess you are right: There are no users for this.
> 
> The usual users for this are devices like LCD panels or other parallel
> buses.

Can you point me to any in-kernel driver that uses GPIOs to drive an LCD panel
or a parallel bus?

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Alexandre Courbot]

On Wed, Jul 30, 2014 at 6:12 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> On Wednesday 09 July 2014 12:17:13 Linus Walleij wrote:
>> On Mon, Jul 7, 2014 at 6:12 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
>> > On Monday 07 July 2014 17:23:34 Linus Walleij wrote:
>> >> On Mon, Jun 16, 2014 at 3:51 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
>> >>
>> >> > Introduce new functions gpiod_set_array & gpiod_set_raw_array to the consumer
>> >> > interface which allow 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.
>> >>
>> >> Yes this looks good.
>> >>
>> >> Except for one thing I mentioned quite early  I think:
>> >>
>> >> no users!
>> >
>> > I'm pretty sure you did _not_ mention this before.
>>
>> I'm not referring to your patch set specifically. The idea of array handling
>> has been proposed something like three times, I cannot tell these discussion
>> threads apart in my head.
>>
>> But it's a reasonable request don't you think?
>>
>
> The request might be reasonable.
>
> But after taking a look at the i2c-gpio and spi-gpio drivers, I do not think they
> would benefit from the new feature. In both cases there are only two GPIO lines.
> And they have to be set separately at least half of the time.
>
> I could not find any other in-kernel driver that might benefit from the new feature, either.
>
> So I guess you are right: There are no users for this.
>
> For my use-case the feature is very beneficial, but of course only in combination
> with an out-of-kernel driver module for configuring the FPGA on my custom board.
> I posted the patch because I expected similar use-cases to exist for other people
> out there. If you think that's not the case, there is nothing I can do about it.

Personally I would be sad if this series is not merged. It is a
well-written solution to a problem that comes regularly. It is
understandable that the upstream kernel API serves, well, upstream
users - but we also need to recognize that most of the drivers using
lots of GPIOs do not get merged upstream because using GPIOs that way
is often considered a hack.

I'd be surprised if at least *one* such driver was not in mainline
though. Had a quick look but could not find anything that would be an
obvious candidate to use these new functions.

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Gerhard Sittig]

On Thu, 2014-07-31 at 11:35 +0900, Alexandre Courbot wrote:
> 
> I'd be surprised if at least *one* such driver was not in mainline
> though. Had a quick look but could not find anything that would be an
> obvious candidate to use these new functions.

What about drivers/mtd/nand/gpio.c, the GPIO bitbanged NAND
controller driver?  Most of the latch, control, and data lines
groups are set at the same time.  It should be a perfect example.


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: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

On Thursday 31 July 2014 09:12:35 Gerhard Sittig wrote:
> On Thu, 2014-07-31 at 11:35 +0900, Alexandre Courbot wrote:
> > 
> > I'd be surprised if at least *one* such driver was not in mainline
> > though. Had a quick look but could not find anything that would be an
> > obvious candidate to use these new functions.
> 
> What about drivers/mtd/nand/gpio.c, the GPIO bitbanged NAND
> controller driver?  Most of the latch, control, and data lines
> groups are set at the same time.  It should be a perfect example.
> 

Well, not so perfect, since AFAIUI only a few control signals are managed
by GPIOs (Chip Enable, Command Latch Enable, Address Latch Enable), but not
the actual data lines. But granted, at least at one place in the driver three
lines are set at the same time.

There are a few other drivers which also do that:
- drivers/tty/serial/serial_mctrl_gpio.c controls multiple modem lines via GPIO
- drivers/net/phy/mdio-mux-gpio.c a GPIO controlled MDIO bus multiplexer driver
- drivers/regulator/gpio-regulator.c controls voltage regulators

All of the above do set multiple GPIOs at the same time, but in all cases
performance does not really matter. Moreover they all seem a bit obscure to me.

Nevertheless, IMHO they are still better candidates for using the new functions
than the i2c-gpio or spi-gpio drivers.

So, Linus, would a patched version of one of the mentioned drivers qualify as
a user that would allow you to merge my patch?

   Rojhalat

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Gerhard Sittig]

On Thu, 2014-07-31 at 10:42 +0200, Rojhalat Ibrahim wrote:
> 
> On Thursday 31 July 2014 09:12:35 Gerhard Sittig wrote:
> > On Thu, 2014-07-31 at 11:35 +0900, Alexandre Courbot wrote:
> > > 
> > > I'd be surprised if at least *one* such driver was not in mainline
> > > though. Had a quick look but could not find anything that would be an
> > > obvious candidate to use these new functions.
> > 
> > What about drivers/mtd/nand/gpio.c, the GPIO bitbanged NAND
> > controller driver?  Most of the latch, control, and data lines
> > groups are set at the same time.  It should be a perfect example.
> > 
> 
> Well, not so perfect, since AFAIUI only a few control signals are managed
> by GPIOs (Chip Enable, Command Latch Enable, Address Latch Enable), but not
> the actual data lines. But granted, at least at one place in the driver three
> lines are set at the same time.

For some reason I mistook the driver's description and was under
the impression that it would completely bitbang the NAND
protocol.  But I was wrong, it's a "GPIO _assisted_ NAND flash"
driver.  I am sorry for the confusion.  Have sent patches to the
MTD list to have the documentation updated for clarity.


> There are a few other drivers which also do that:
> - drivers/tty/serial/serial_mctrl_gpio.c controls multiple modem lines via GPIO
> - drivers/net/phy/mdio-mux-gpio.c a GPIO controlled MDIO bus multiplexer driver
> - drivers/regulator/gpio-regulator.c controls voltage regulators
> 
> All of the above do set multiple GPIOs at the same time, but in all cases
> performance does not really matter. Moreover they all seem a bit obscure to me.
> 
> Nevertheless, IMHO they are still better candidates for using the new functions
> than the i2c-gpio or spi-gpio drivers.

Could you spot the LCD driver mentioned by Mark?  There are
displays that implement a typically four or optionally eight bit
data bus plus some three control lines (E/RW/RS), usually GPIO
bitbanged -- so in total it's 7 or 11 software controlled pins.
One of the controller chips is HD44780, compatibles have KS...
and SED... names.

It's a pity that drivers/auxdisplay/ and drivers/staging/panel/
only support parport(4) and not GPIO.


Why is your FPGA netlist download driver not acceptable for
mainline?  Because it uses many GPIOs instead of a parallel or
serial bus?  Doesn't your code submission make parallel busses
done by many GPIOs "more acceptable"?  If your netlist download
follows the ususal passive parallel approach, there should not be
an issue.  There are some FPGA netlist download drivers in
drivers/misc/ and you might follow that pattern.


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: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Wed, Aug 06, 2014 at 02:45:44PM +0200, Gerhard Sittig wrote:

> Could you spot the LCD driver mentioned by Mark?  There are
> displays that implement a typically four or optionally eight bit
> data bus plus some three control lines (E/RW/RS), usually GPIO
> bitbanged -- so in total it's 7 or 11 software controlled pins.
> One of the controller chips is HD44780, compatibles have KS...
> and SED... names.

I think the users just stayed out of tree due to not being able to get
stuff merged.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Rojhalat Ibrahim]

On Wednesday 06 August 2014 14:45:44 Gerhard Sittig wrote:
> 
> Could you spot the LCD driver mentioned by Mark?  There are
> displays that implement a typically four or optionally eight bit
> data bus plus some three control lines (E/RW/RS), usually GPIO
> bitbanged -- so in total it's 7 or 11 software controlled pins.
> One of the controller chips is HD44780, compatibles have KS...
> and SED... names.
> 

I found a few display drivers using GPIOs for enable and/or reset lines,
but none using GPIOs for the data lines.

> It's a pity that drivers/auxdisplay/ and drivers/staging/panel/
> only support parport(4) and not GPIO.
> 

That could be an interesting application.

> 
> Why is your FPGA netlist download driver not acceptable for
> mainline?  Because it uses many GPIOs instead of a parallel or
> serial bus?  Doesn't your code submission make parallel busses
> done by many GPIOs "more acceptable"?  If your netlist download
> follows the ususal passive parallel approach, there should not be
> an issue.  There are some FPGA netlist download drivers in
> drivers/misc/ and you might follow that pattern.
> 
> 

Thanks for the suggestion. I have no problem with submitting that driver.
Right now it's probably a little too board-specific for mainline, but
I think I could improve on that. The result would be a driver to configure
an Altera FPGA via the passive parallel configuration scheme using a number
of GPIOs specified via DT. The driver would depend on the gpiod_set_array
function and for now it would be the only user.

Do you think this has a chance to get merged? Should I post an extended
patch set including the FPGA configuration driver?

   Rojhalat

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Linus Walleij]

On Thu, Jul 31, 2014 at 10:42 AM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> On Thursday 31 July 2014 09:12:35 Gerhard Sittig wrote:
>> On Thu, 2014-07-31 at 11:35 +0900, Alexandre Courbot wrote:
>> >
>> > I'd be surprised if at least *one* such driver was not in mainline
>> > though. Had a quick look but could not find anything that would be an
>> > obvious candidate to use these new functions.
>>
>> What about drivers/mtd/nand/gpio.c, the GPIO bitbanged NAND
>> controller driver?  Most of the latch, control, and data lines
>> groups are set at the same time.  It should be a perfect example.
>>
>
> Well, not so perfect, since AFAIUI only a few control signals are managed
> by GPIOs (Chip Enable, Command Latch Enable, Address Latch Enable), but not
> the actual data lines. But granted, at least at one place in the driver three
> lines are set at the same time.
>
> There are a few other drivers which also do that:
> - drivers/tty/serial/serial_mctrl_gpio.c controls multiple modem lines via GPIO
> - drivers/net/phy/mdio-mux-gpio.c a GPIO controlled MDIO bus multiplexer driver
> - drivers/regulator/gpio-regulator.c controls voltage regulators
>
> All of the above do set multiple GPIOs at the same time, but in all cases
> performance does not really matter. Moreover they all seem a bit obscure to me.
>
> Nevertheless, IMHO they are still better candidates for using the new functions
> than the i2c-gpio or spi-gpio drivers.
>
> So, Linus, would a patched version of one of the mentioned drivers qualify as
> a user that would allow you to merge my patch?

I think so. My concern is with code that doesn't get exercised, anywhere.

Yours,
Linus Walleij

Re: [PATCH 1/2][v5] gpiolib: allow simultaneous setting of multiple GPIO outputs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 480 bytes --]


> > So, Linus, would a patched version of one of the mentioned drivers qualify as
> > a user that would allow you to merge my patch?
> 
> I think so. My concern is with code that doesn't get exercised, anywhere.

If we have a gpio-chardev somewhen, I know a bunch of industrial
customers who will love it. It always feels a bit awkward to tell them
they have to fiddle with bits individually if they want to read out an
ID byte or have paired GPIO for other reasons.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]