[PATCH 0/3] gpio: add gpio_set_direction

[PATCH 0/3] gpio: add gpio_set_direction

[Ben Gardner]

The OLPC uses a GPIO output to enable/disable the MIC input.
The code uses gpio_get_value() to retrive the current MIC enabled status.
Due to a recent fix to cs5535-gpio, that function returns the input value.
Since the input is not enabled, the input value does not match the output.
gpiolib doesn't currently have the ability to enable both input and output. 
Nor does gpiolib have the ability to retrieve the current output value.

There are several ways to fix this MIC problem.
1) keep track of the output value in the audio driver and do not use
gpio_get_value().

2) add a new gpiolib function to retrieve the output value.                  

3) add a way to set the GPIO bidirectional, so the read retrieves the real
value that the MIC circuitry would see.

Since the CS5535 GPIO pins has independant control of the input and output,
I went with the last option.

This patchset merges the gpio_direction_input() and gpio_direction_output()
function into gpio_set_direction().                                 
The direction parameter is a bit field, so the direction can be one of
none, in, out, inout.
The cs5535-gpio driver is updated to support set_direction() and 
cs5535audio_olpc is updated to set the GPIO in bidirectional mode.

Ben Gardner (3):
  gpiolib: add gpio_set_direction()
  cs5535-gpio: Use set_direction
  OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input

 drivers/gpio/cs5535-gpio.c               |   34 ++++-----
 drivers/gpio/gpiolib.c                   |  123 +++++++++++++++---------------
 include/asm-generic/gpio.h               |    6 ++
 include/linux/gpio.h                     |    5 +
 sound/pci/cs5535audio/cs5535audio_olpc.c |    2 +-
 5 files changed, 89 insertions(+), 81 deletions(-)

[PATCH 1/3] gpiolib: add gpio_set_direction()

[Ben Gardner]

Combine gpio_direction_input() and gpio_direction_output() into
gpio_set_direction().
Add 'none' and 'inout' directions to the sysfs interface.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
CC: Andres Salomon <dilinger@collabora.co.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Brownell <dbrownell@users.sourceforge.net>
CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/gpiolib.c     |  123 ++++++++++++++++++++++----------------------
 include/asm-generic/gpio.h |    6 ++
 include/linux/gpio.h       |    5 ++
 3 files changed, 73 insertions(+), 61 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 350842a..4dd6e44 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -47,13 +47,14 @@ struct gpio_desc {
 	unsigned long		flags;
 /* flag symbols are bit numbers */
 #define FLAG_REQUESTED	0
-#define FLAG_IS_OUT	1
+#define FLAG_IS_OUT	1	/* output is enabled */
 #define FLAG_RESERVED	2
 #define FLAG_EXPORT	3	/* protected by sysfs_lock */
 #define FLAG_SYSFS	4	/* exported via /sys/class/gpio/control */
 #define FLAG_TRIG_FALL	5	/* trigger on falling edge */
 #define FLAG_TRIG_RISE	6	/* trigger on rising edge */
 #define FLAG_ACTIVE_LOW	7	/* sysfs value has active low */
+#define FLAG_IS_IN	8	/* input is enabled */
 
 #define PDESC_ID_SHIFT	16	/* add new flags before this one */
 
@@ -228,10 +229,14 @@ static ssize_t gpio_direction_show(struct device *dev,
 
 	if (!test_bit(FLAG_EXPORT, &desc->flags))
 		status = -EIO;
-	else
-		status = sprintf(buf, "%s\n",
-			test_bit(FLAG_IS_OUT, &desc->flags)
-				? "out" : "in");
+	else {
+		status = ((test_bit(FLAG_IS_IN, &desc->flags) ? 1 : 0) |
+			  (test_bit(FLAG_IS_OUT, &desc->flags) ? 2 : 0));
+		status = sprintf(buf, "%s%s%s\n",
+				 (status == 0) ? "none" : "",
+				 (status & 1) ? "in" : "",
+				 (status & 2) ? "out" : "");
+	}
 
 	mutex_unlock(&sysfs_lock);
 	return status;
@@ -249,11 +254,15 @@ static ssize_t gpio_direction_store(struct device *dev,
 	if (!test_bit(FLAG_EXPORT, &desc->flags))
 		status = -EIO;
 	else if (sysfs_streq(buf, "high"))
-		status = gpio_direction_output(gpio, 1);
+		status = gpio_set_direction(gpio, 2, 1);
 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
-		status = gpio_direction_output(gpio, 0);
+		status = gpio_set_direction(gpio, 2, 0);
 	else if (sysfs_streq(buf, "in"))
-		status = gpio_direction_input(gpio);
+		status = gpio_set_direction(gpio, 1, 0);
+	else if (sysfs_streq(buf, "inout"))
+		status = gpio_set_direction(gpio, 3, 0);
+	else if (sysfs_streq(buf, "none"))
+		status = gpio_set_direction(gpio, 0, 0);
 	else
 		status = -EINVAL;
 
@@ -1277,7 +1286,7 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * rely on gpio_request() having been called beforehand.
  */
 
-int gpio_direction_input(unsigned gpio)
+int gpio_set_direction(unsigned gpio, int direction, int value)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
@@ -1289,7 +1298,13 @@ int gpio_direction_input(unsigned gpio)
 	if (!gpio_is_valid(gpio))
 		goto fail;
 	chip = desc->chip;
-	if (!chip || !chip->get || !chip->direction_input)
+	if (!chip)
+		goto fail;
+	if ((direction & 1) &&
+	    (!chip->get || (!chip->direction_input && !chip->set_direction)))
+		goto fail;
+	if ((direction & 2) &&
+	    (!chip->set || (!chip->direction_output && !chip->set_direction)))
 		goto fail;
 	gpio -= chip->base;
 	if (gpio >= chip->ngpio)
@@ -1316,9 +1331,36 @@ int gpio_direction_input(unsigned gpio)
 		}
 	}
 
-	status = chip->direction_input(chip, gpio);
-	if (status == 0)
-		clear_bit(FLAG_IS_OUT, &desc->flags);
+	if (chip->set_direction) {
+		status = chip->set_direction(chip, gpio, direction, value);
+		if (status == 0) {
+			if (direction & 1)
+				set_bit(FLAG_IS_IN, &desc->flags);
+			else
+				clear_bit(FLAG_IS_IN, &desc->flags);
+			if (direction & 2)
+				set_bit(FLAG_IS_OUT, &desc->flags);
+			else
+				clear_bit(FLAG_IS_OUT, &desc->flags);
+		}
+	}
+	else if (direction == 1) {
+		status = chip->direction_input(chip, gpio);
+		if (status == 0) {
+			clear_bit(FLAG_IS_OUT, &desc->flags);
+			set_bit(FLAG_IS_IN, &desc->flags);
+		}
+	}
+	else if (direction == 2) {
+		status = chip->direction_output(chip, gpio, value);
+		if (status == 0) {
+			clear_bit(FLAG_IS_IN, &desc->flags);
+			set_bit(FLAG_IS_OUT, &desc->flags);
+		}
+	}
+	else
+		/* cannot do 'none' or 'inout' without chip->set_direction */
+		status = -ENOTSUPP;
 lose:
 	return status;
 fail:
@@ -1328,58 +1370,17 @@ fail:
 			__func__, gpio, status);
 	return status;
 }
+EXPORT_SYMBOL_GPL(gpio_set_direction);
+
+int gpio_direction_input(unsigned gpio)
+{
+	return gpio_set_direction(gpio, 1, 0);
+}
 EXPORT_SYMBOL_GPL(gpio_direction_input);
 
 int gpio_direction_output(unsigned gpio, int value)
 {
-	unsigned long		flags;
-	struct gpio_chip	*chip;
-	struct gpio_desc	*desc = &gpio_desc[gpio];
-	int			status = -EINVAL;
-
-	spin_lock_irqsave(&gpio_lock, flags);
-
-	if (!gpio_is_valid(gpio))
-		goto fail;
-	chip = desc->chip;
-	if (!chip || !chip->set || !chip->direction_output)
-		goto fail;
-	gpio -= chip->base;
-	if (gpio >= chip->ngpio)
-		goto fail;
-	status = gpio_ensure_requested(desc, gpio);
-	if (status < 0)
-		goto fail;
-
-	/* now we know the gpio is valid and chip won't vanish */
-
-	spin_unlock_irqrestore(&gpio_lock, flags);
-
-	might_sleep_if(extra_checks && chip->can_sleep);
-
-	if (status) {
-		status = chip->request(chip, gpio);
-		if (status < 0) {
-			pr_debug("GPIO-%d: chip request fail, %d\n",
-				chip->base + gpio, status);
-			/* and it's not available to anyone else ...
-			 * gpio_request() is the fully clean solution.
-			 */
-			goto lose;
-		}
-	}
-
-	status = chip->direction_output(chip, gpio, value);
-	if (status == 0)
-		set_bit(FLAG_IS_OUT, &desc->flags);
-lose:
-	return status;
-fail:
-	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status)
-		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	return status;
+	return gpio_set_direction(gpio, 2, value);
 }
 EXPORT_SYMBOL_GPL(gpio_direction_output);
 
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 485eeb6..17c7642 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -41,6 +41,8 @@ struct module;
  *	enabling module power and clock; may sleep
  * @free: optional hook for chip-specific deactivation, such as
  *	disabling module power and clock; may sleep
+ * @set_direction: configures signal "offset" as "direction" (0-3, bit0=input,
+ *	bit1=output) or returns error
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -82,6 +84,9 @@ struct gpio_chip {
 	void			(*free)(struct gpio_chip *chip,
 						unsigned offset);
 
+	int			(*set_direction)(struct gpio_chip *chip,
+						unsigned offset,
+						int direction, int value);
 	int			(*direction_input)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get)(struct gpio_chip *chip,
@@ -118,6 +123,7 @@ extern int __must_check gpiochip_remove(struct gpio_chip *chip);
 extern int gpio_request(unsigned gpio, const char *label);
 extern void gpio_free(unsigned gpio);
 
+extern int gpio_set_direction(unsigned gpio, int direction, int value);
 extern int gpio_direction_input(unsigned gpio);
 extern int gpio_direction_output(unsigned gpio, int value);
 
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 4e949a5..cdc2117 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -41,6 +41,11 @@ static inline void gpio_free(unsigned gpio)
 	WARN_ON(1);
 }
 
+static inline int gpio_set_direction(unsigned gpio, int direction, int value)
+{
+	return -ENOSYS;
+}
+
 static inline int gpio_direction_input(unsigned gpio)
 {
 	return -ENOSYS;
-- 
1.7.0

[PATCH 2/3] cs5535-gpio: Use set_direction

[Ben Gardner]

The CS5535 GPIO has independant controls for input and output enable.
Use the set_direction method instead of direction_input and direction_output
to enable use of the bidirectional mode.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
CC: Andres Salomon <dilinger@collabora.co.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Brownell <dbrownell@users.sourceforge.net>
CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/cs5535-gpio.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 0fdbe94..0cb1462 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -165,30 +165,27 @@ static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
 		cs5535_gpio_clear(offset, GPIO_OUTPUT_VAL);
 }
 
-static int chip_direction_input(struct gpio_chip *c, unsigned offset)
-{
-	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
-	unsigned long flags;
-
-	spin_lock_irqsave(&chip->lock, flags);
-	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
-	spin_unlock_irqrestore(&chip->lock, flags);
-
-	return 0;
-}
-
-static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
+static int chip_set_direction(struct gpio_chip *c, unsigned offset,
+		int direction, int value)
 {
 	struct cs5535_gpio_chip *chip = (struct cs5535_gpio_chip *) c;
 	unsigned long flags;
 
 	spin_lock_irqsave(&chip->lock, flags);
 
-	__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
-	if (val)
-		__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
+	if (direction & 1)
+		__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
 	else
-		__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_VAL);
+		__cs5535_gpio_clear(chip, offset, GPIO_INPUT_ENABLE);
+
+	if (direction & 2) {
+		if (value)
+			__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
+		else
+			__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_VAL);
+		__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
+	} else
+		__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
 
 	spin_unlock_irqrestore(&chip->lock, flags);
 
@@ -219,8 +216,7 @@ static struct cs5535_gpio_chip cs5535_gpio_chip = {
 		.get = chip_gpio_get,
 		.set = chip_gpio_set,
 
-		.direction_input = chip_direction_input,
-		.direction_output = chip_direction_output,
+		.set_direction = chip_set_direction,
 	},
 };
 
-- 
1.7.0

[PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input

[Ben Gardner]

We need to read back the value written to the GPIO pin to control the
MIC input enable.
Use gpio_set_direction() to set the GPIO in bidirectional mode.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
CC: Andres Salomon <dilinger@collabora.co.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Brownell <dbrownell@users.sourceforge.net>
CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 sound/pci/cs5535audio/cs5535audio_olpc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/pci/cs5535audio/cs5535audio_olpc.c b/sound/pci/cs5535audio/cs5535audio_olpc.c
index 50da49b..0c80e67 100644
--- a/sound/pci/cs5535audio/cs5535audio_olpc.c
+++ b/sound/pci/cs5535audio/cs5535audio_olpc.c
@@ -156,7 +156,7 @@ int __devinit olpc_quirks(struct snd_card *card, struct snd_ac97 *ac97)
 		printk(KERN_ERR DRV_NAME ": unable to allocate MIC GPIO\n");
 		return -EIO;
 	}
-	gpio_direction_output(OLPC_GPIO_MIC_AC, 0);
+	gpio_set_direction(OLPC_GPIO_MIC_AC, 3, 0);
 
 	/* drop the original AD1888 HPF control */
 	memset(&elem, 0, sizeof(elem));
-- 
1.7.0

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Andres Salomon]

On Fri, 26 Feb 2010 17:26:24 -0600
Ben Gardner <gardner.ben@gmail.com> wrote:

> Combine gpio_direction_input() and gpio_direction_output() into
> gpio_set_direction().
> Add 'none' and 'inout' directions to the sysfs interface.
> 
> Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
> CC: Andres Salomon <dilinger@collabora.co.uk>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: David Brownell <dbrownell@users.sourceforge.net>
> CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
> ---
>  drivers/gpio/gpiolib.c     |  123 ++++++++++++++++++++++----------------------
>  include/asm-generic/gpio.h |    6 ++
>  include/linux/gpio.h       |    5 ++
>  3 files changed, 73 insertions(+), 61 deletions(-)
> 
[...]
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index 485eeb6..17c7642 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -41,6 +41,8 @@ struct module;
>   *	enabling module power and clock; may sleep
>   * @free: optional hook for chip-specific deactivation, such as
>   *	disabling module power and clock; may sleep
> + * @set_direction: configures signal "offset" as "direction" (0-3, bit0=input,
> + *	bit1=output) or returns error


How about something like the following for set_direction, so we're not
comparing magic bits?

/* gpio direction flags */
#define GPIO_DIR_NONE 0 
#define	GPIO_DIR_INPUT (1 << 0)
#define	GPIO_DIR_OUTPUT (1 << 1)

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Ben Gardner]

> How about something like the following for set_direction, so we're not
> comparing magic bits?
>
> /* gpio direction flags */
> #define GPIO_DIR_NONE 0
> #define GPIO_DIR_INPUT (1 << 0)
> #define GPIO_DIR_OUTPUT (1 << 1)

Sounds good and it looks like those macros aren't used anywhere else.
Maybe also add a define for inout?
#define GPIO_DIR_INOUT (GPIO_DIR_INPUT | GPIO_DIR_OUTPUT)

Ben

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[David Brownell]

On Friday 26 February 2010, Ben Gardner wrote:
> Combine gpio_direction_input() and gpio_direction_output() into
> gpio_set_direction().

NAK. 

When we discussed the programming interface originally, having the
direction be part of the function name was explicitly requested as
a way to reduce programming errors.

I recall that  a gpio_set_direction() was in the original proposal...
removing that was one of the handful of fixes that went into the final
set of calls in this programming interface.

And in fact  ... it *was* very easy to make errors with a few GPIO
interfaces which worked like that.  With even fewer parameters to
create confusion than in your proposal.

Let's have no retrograde motion...


> Add 'none' and 'inout' directions to the sysfs interface.

Both of those seem nonsensical:

 "none" ... since it's not even a GPIO, why would it show
	up through the GPIO subsystem???

 "inout" ... is not a direction.  But if you want to do this,
	you really ought to bite the bullet and come up with a
	way the implementation can pass up its capabilities.

	Did you read the definition of gpio_get_value()?  It says
	"When reading the value of an output pin, the value returned
	should be what's seen on the pin ... that won't always match
	the specified output value, because of issues including
	open-drain signaling and output latencies."  Also:  "note that
	not all platforms can read the value of output pins; those that
	can't should always return zero."

	Another way to say that is that "output" means 'inout" except
	when the platform can't do that.  So you'd need to address the
	case of hardware that's truly output-only ... instead of
	ignoring that, as done here.  (That is:  you'd need to have a
	way to reject "inout" mode ... or for that matter, "output-only".)


	Doing that well might be a Good Thing ... if for example it
	lets the initial mode of a GPIO show up properly ... there's
	currently an assumption in sysfs that they start up as "input",
	which of course makes sense for any power-sensitive system
	(you don't enable output drivers unless that power consumption
	is for some reason required).

	I've never, for example, seen GPIO hardware that would support
	the equivalent of that "open in <bitmask> mode".   It's either
	unidirectional (rarely), or (normally) the only real option is
	whether the output drivers are disabled.  So you always get the
	"inout" semantics described above, or input-only ones.  Asking
	for output-only would need to fail.  (Or in the less typical
	cases, it's asking for "inout" that would need to fail.)

Re: [PATCH 2/3] cs5535-gpio: Use set_direction

[David Brownell]

On Friday 26 February 2010, Ben Gardner wrote:
> The CS5535 GPIO has independant controls for input and output enable.

Unusual, but not causing any conceptual problem.


> Use the set_direction method instead of direction_input and direction_output
> to enable use of the bidirectional mode.

Any reason you aren't making the standard behavior be:

	input ... input enabled
	output ... both enabled

That would make this driver behave like most other GPIO hardware, which
would help you avoid subtle problems.

Would there be any technical downside to that approach?

Re: [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input

[David Brownell]

On Friday 26 February 2010, Ben Gardner wrote:
> We need to read back the value written to the GPIO pin to control the
> MIC input enable.

There are two potential values associated with GPIO outputs:

	- The value you wrote to it, which you should just remember.
	  This value should *NEVER* be returned through the GPIO calls.

	- The value at the pin, which will *often* be the same as
	  what you wrote to it ... except for open drain signals
	  or other "multi-drive" cases.   Or if you try to read the
	  value back before it gets latched.  (It's common to have
	  the "latch value" step be clocked by something ... and to
	  have multiple clock domains, so latching might not sync
	  up with what your CPU is doing.)

>From other patches recently landing in my mailbox, I'm thinking
that your GPIO driver isn't behaving properly ... it should return
the second value.


> Use gpio_set_direction() to set the GPIO in bidirectional mode.

NAK. THere is no such call, and should never be one.

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Mark Brown]

On Fri, Feb 26, 2010 at 11:02:18PM -0800, David Brownell wrote:
> On Friday 26 February 2010, Ben Gardner wrote:

> > Add 'none' and 'inout' directions to the sysfs interface.

> Both of those seem nonsensical:

>  "none" ... since it's not even a GPIO, why would it show
> 	up through the GPIO subsystem???

I suspect this is intended to be tristated, which might be useful to add.

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[David Brownell]

On Saturday 27 February 2010, Mark Brown wrote:
> >  "none" ... since it's not even a GPIO, why would it show
> >       up through the GPIO subsystem???
> 
> I suspect this is intended to be tristated, which might be useful to add.

That's what "input" means, as a rule:  no output driver
is active with a GPIO configured as "iput".  "Tristate"
is an option that's relevant for outputs ... low, high,
or not-driven.

- Dave

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Mark Brown]

On 27 Feb 2010, at 16:24, David Brownell <david-b@pacbell.net> wrote:

> On Saturday 27 February 2010, Mark Brown wrote:
>>>  "none" ... since it's not even a GPIO, why would it show
>>>       up through the GPIO subsystem???
>>
>> I suspect this is intended to be tristated, which might be useful  
>> to add.
>
> That's what "input" means, as a rule:  no output driver
> is active with a GPIO configured as "iput".  "Tristate"
> is an option that's relevant for outputs ... low, high,
> or not-driven.

Indeed, but some devices do implement a distinct tristate state for  
input mode pins (disabling interrupt generation logic and so on for  
example).

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[David Brownell]

On Saturday 27 February 2010, Mark Brown wrote:
> >>>  "none" ... since it's not even a GPIO, why would it show
> >>>       up through the GPIO subsystem???
> >>
> >> I suspect this is intended to be tristated, which might be useful  
> >> to add.
> >
> > That's what "input" means, as a rule:  no output driver
> > is active with a GPIO configured as "input".  "Tristate"
> > is an option that's relevant for outputs ... low, high,
> > or not-driven.
> 
> Indeed, but some devices do implement a distinct tristate state for  
> input mode pins (disabling interrupt generation logic and so on for  
> example).

That's a pretty sloppy usage of the term "tristate" ... yeah, there
are people who take glee in abusing terminology to introduce confusion,
and some of them write technical manuals with little regard to normal
usage of terms (or trademarks, which do exist for "tristate").

IRQ generation logic should be disabled until request_irq() code paths
report otherwise.   And regardless, whether a GPIO triggers an IRQ has
nothing at all to do with its "direction".

- Dave

Re: [PATCH 2/3] cs5535-gpio: Use set_direction

[Ben Gardner]

>> Use the set_direction method instead of direction_input and direction_output
>> to enable use of the bidirectional mode.
>
> Any reason you aren't making the standard behavior be:
>
>        input ... input enabled
>        output ... both enabled
>
> That would make this driver behave like most other GPIO hardware, which
> would help you avoid subtle problems.
>
> Would there be any technical downside to that approach?

That sounds like a good approach.

Ben

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Ben Gardner]

> NAK.
>
> When we discussed the programming interface originally, having the
> direction be part of the function name was explicitly requested as
> a way to reduce programming errors.
>
> I recall that  a gpio_set_direction() was in the original proposal...
> removing that was one of the handful of fixes that went into the final
> set of calls in this programming interface.
>
> And in fact  ... it *was* very easy to make errors with a few GPIO
> interfaces which worked like that.  With even fewer parameters to
> create confusion than in your proposal.
>
> Let's have no retrograde motion...
>
>
>> Add 'none' and 'inout' directions to the sysfs interface.
>
> Both of those seem nonsensical:
>
>  "none" ... since it's not even a GPIO, why would it show
>        up through the GPIO subsystem???
>
>  "inout" ... is not a direction.  But if you want to do this,
>        you really ought to bite the bullet and come up with a
>        way the implementation can pass up its capabilities.
>
>        Did you read the definition of gpio_get_value()?  It says
>        "When reading the value of an output pin, the value returned
>        should be what's seen on the pin ... that won't always match
>        the specified output value, because of issues including
>        open-drain signaling and output latencies."  Also:  "note that
>        not all platforms can read the value of output pins; those that
>        can't should always return zero."
>
>        Another way to say that is that "output" means 'inout" except
>        when the platform can't do that.  So you'd need to address the
>        case of hardware that's truly output-only ... instead of
>        ignoring that, as done here.  (That is:  you'd need to have a
>        way to reject "inout" mode ... or for that matter, "output-only".)
>
>
>        Doing that well might be a Good Thing ... if for example it
>        lets the initial mode of a GPIO show up properly ... there's
>        currently an assumption in sysfs that they start up as "input",
>        which of course makes sense for any power-sensitive system
>        (you don't enable output drivers unless that power consumption
>        is for some reason required).
>
>        I've never, for example, seen GPIO hardware that would support
>        the equivalent of that "open in <bitmask> mode".   It's either
>        unidirectional (rarely), or (normally) the only real option is
>        whether the output drivers are disabled.  So you always get the
>        "inout" semantics described above, or input-only ones.  Asking
>        for output-only would need to fail.  (Or in the less typical
>        cases, it's asking for "inout" that would need to fail.)

OK. Lets dump this patch series and I'll send off a patch that fixes
the cs5535-gpio driver to behave as indicated.

Ben

[PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations

[Ben Gardner]

The intent of the gpiolib set_direction_xxx functions is as follows:
output: enable both input and output
input: disable output, enable input

Change the cs5535 driver to do that.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
CC: Andres Salomon <dilinger@collabora.co.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Brownell <dbrownell@users.sourceforge.net>
CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/cs5535-gpio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 0fdbe94..59ab30e 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -171,6 +171,7 @@ static int chip_direction_input(struct gpio_chip *c, unsigned offset)
 	unsigned long flags;
 
 	spin_lock_irqsave(&chip->lock, flags);
+	__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_DISABLE);
 	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
 	spin_unlock_irqrestore(&chip->lock, flags);
 
@@ -185,6 +186,7 @@ static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
 	spin_lock_irqsave(&chip->lock, flags);
 
 	__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
+	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
 	if (val)
 		__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
 	else
-- 
1.7.0

Re: [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations

[Ben Gardner]

Please ignore this patch.
It seems you have to 'commit' in git before 'format-patch'...

On Sat, Feb 27, 2010 at 12:36 PM, Ben Gardner <gardner.ben@gmail.com> wrote:
> The intent of the gpiolib set_direction_xxx functions is as follows:
> output: enable both input and output
> input: disable output, enable input
>
> Change the cs5535 driver to do that.
>
> Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
> CC: Andres Salomon <dilinger@collabora.co.uk>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: David Brownell <dbrownell@users.sourceforge.net>
> CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
> ---
>  drivers/gpio/cs5535-gpio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
> index 0fdbe94..59ab30e 100644
> --- a/drivers/gpio/cs5535-gpio.c
> +++ b/drivers/gpio/cs5535-gpio.c
> @@ -171,6 +171,7 @@ static int chip_direction_input(struct gpio_chip *c, unsigned offset)
>        unsigned long flags;
>
>        spin_lock_irqsave(&chip->lock, flags);
> +       __cs5535_gpio_set(chip, offset, GPIO_OUTPUT_DISABLE);
>        __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>        spin_unlock_irqrestore(&chip->lock, flags);
>
> @@ -185,6 +186,7 @@ static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
>        spin_lock_irqsave(&chip->lock, flags);
>
>        __cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
> +       __cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
>        if (val)
>                __cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
>        else
> --
> 1.7.0
>
>

[PATCH v2] cs5535-gpio: change input/output enable to match gpiolib expectations

[Ben Gardner]

The intent of the gpiolib set_direction_xxx functions is as follows:
output: enable both input and output
input: disable output, enable input

Change the cs5535 driver to do that.

Signed-off-by: Ben Gardner <gardner.ben@gmail.com>
CC: Andres Salomon <dilinger@collabora.co.uk>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: David Brownell <dbrownell@users.sourceforge.net>
CC: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
 drivers/gpio/cs5535-gpio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpio/cs5535-gpio.c b/drivers/gpio/cs5535-gpio.c
index 0fdbe94..de064c9 100644
--- a/drivers/gpio/cs5535-gpio.c
+++ b/drivers/gpio/cs5535-gpio.c
@@ -171,6 +171,7 @@ static int chip_direction_input(struct gpio_chip *c, unsigned offset)
 	unsigned long flags;
 
 	spin_lock_irqsave(&chip->lock, flags);
+	__cs5535_gpio_clear(chip, offset, GPIO_OUTPUT_ENABLE);
 	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
 	spin_unlock_irqrestore(&chip->lock, flags);
 
@@ -185,6 +186,7 @@ static int chip_direction_output(struct gpio_chip *c, unsigned offset, int val)
 	spin_lock_irqsave(&chip->lock, flags);
 
 	__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_ENABLE);
+	__cs5535_gpio_set(chip, offset, GPIO_INPUT_ENABLE);
 	if (val)
 		__cs5535_gpio_set(chip, offset, GPIO_OUTPUT_VAL);
 	else
-- 
1.7.0

Re: [PATCH v2] cs5535-gpio: change input/output enable to match gpiolib expectations

[David Brownell]

On Saturday 27 February 2010, Ben Gardner wrote:

Patch seems OK, but the comment could stand correction:


> The intent of the gpiolib set_direction_xxx functions is as follows:

Clarification:  it's not "gpiolib" which expects that; "gpiolib"
is just an optional implementation infrastructure for the GPIO
programming interface.

That behavior is better described as a "weak expectation" than an
intent.  It comes from the GPIO programming interface, as described
in Documentation/gpio.txt ...

And the reason it describes that weak expectation is that it's how
most GPIO hardware behaves ... in particular, how the GPIO banks of
most System-on-Chip (SoC) processors behave.  (Even on boards with
external GPIO controllers, the SoC GPIOs generally outnumber the
external GPIOs by one or more orders of magnitude.)

Since the behavior of reading an "output" GPIO's value needs to
be specified ... this issue comes up.


> output: enable both input and output
> input: disable output, enable input

... the expectation is "weak" in that output-only is very
much allowed.  However, if a gpio is going to provide that
model, (a) it needs to report its value as 0/low/false when
asked, which (b) may well trigger some odd behavior in
some other driver code that expects more typical behavior

(You *could* also return 0/low/false for output-only GPIOs.
But this behavior is more typical, and much more useful.)

So a better explanation of this patch would emphasize that
it's providing "more typical behavior" to reduce surprises.


- Dave

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[David Brownell]

On Saturday 27 February 2010, Ben Gardner wrote:
> OK. Lets dump this patch series and I'll send off a patch that fixes
> the cs5535-gpio driver to behave as indicated.

much better, thanks.  All other things being equal ... it's best to
choose implementation options (as in the cs5535 code) in ways that
make systems act similar, instead of emphasizing quirkage.

- Dave

Re: [PATCH 1/3] gpiolib: add gpio_set_direction()

[Mark Brown]

On Sat, Feb 27, 2010 at 09:51:08AM -0800, David Brownell wrote:
> On Saturday 27 February 2010, Mark Brown wrote:

> > Indeed, but some devices do implement a distinct tristate state for  
> > input mode pins (disabling interrupt generation logic and so on for  
> > example).

> That's a pretty sloppy usage of the term "tristate" ... yeah, there
> are people who take glee in abusing terminology to introduce confusion,
> and some of them write technical manuals with little regard to normal
> usage of terms (or trademarks, which do exist for "tristate").

It's a bit clearer when the pins have multiple functions of which GPIO
is only one - normally the tristate is something that applies over all
the functions of the GPIO.

> IRQ generation logic should be disabled until request_irq() code paths
> report otherwise.   And regardless, whether a GPIO triggers an IRQ has
> nothing at all to do with its "direction".

This is an internal Linux implementation issue rather than a hardware
one - that was just an example one of the things that tristating the pin
rather than just putting it into input mode does.