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(-)
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 <[email protected]>
CC: Andres Salomon <[email protected]>
CC: Andrew Morton <[email protected]>
CC: David Brownell <[email protected]>
CC: Jani Nikula <[email protected]>
---
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
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 <[email protected]>
CC: Andres Salomon <[email protected]>
CC: Andrew Morton <[email protected]>
CC: David Brownell <[email protected]>
CC: Jani Nikula <[email protected]>
---
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
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 <[email protected]>
CC: Andres Salomon <[email protected]>
CC: Andrew Morton <[email protected]>
CC: David Brownell <[email protected]>
CC: Jani Nikula <[email protected]>
---
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
On Fri, 26 Feb 2010 17:26:24 -0600
Ben Gardner <[email protected]> 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 <[email protected]>
> CC: Andres Salomon <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: David Brownell <[email protected]>
> CC: Jani Nikula <[email protected]>
> ---
> 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)
> 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
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.)
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?
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.
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.
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
On 27 Feb 2010, at 16:24, David Brownell <[email protected]> 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).
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
>> 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
> 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
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 <[email protected]>
CC: Andres Salomon <[email protected]>
CC: Andrew Morton <[email protected]>
CC: David Brownell <[email protected]>
CC: Jani Nikula <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]>
> CC: Andres Salomon <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: David Brownell <[email protected]>
> CC: Jani Nikula <[email protected]>
> ---
> ?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
>
>
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 <[email protected]>
CC: Andres Salomon <[email protected]>
CC: Andrew Morton <[email protected]>
CC: David Brownell <[email protected]>
CC: Jani Nikula <[email protected]>
---
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
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
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