* [PATCH 0/3] gpio: add gpio_set_direction
@ 2010-02-26 23:26 Ben Gardner
2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Ben Gardner @ 2010-02-26 23:26 UTC (permalink / raw)
To: linux-kernel, Andres Salomon, Andrew Morton; +Cc: 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(-)
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-26 23:26 [PATCH 0/3] gpio: add gpio_set_direction Ben Gardner @ 2010-02-26 23:26 ` Ben Gardner 2010-02-26 23:39 ` Andres Salomon ` (3 more replies) 2010-02-26 23:26 ` [PATCH 2/3] cs5535-gpio: Use set_direction Ben Gardner 2010-02-26 23:26 ` [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Ben Gardner 2 siblings, 4 replies; 21+ messages in thread From: Ben Gardner @ 2010-02-26 23:26 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Andrew Morton Cc: Ben Gardner, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner @ 2010-02-26 23:39 ` Andres Salomon 2010-02-27 5:11 ` Ben Gardner 2010-02-27 7:02 ` David Brownell ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Andres Salomon @ 2010-02-26 23:39 UTC (permalink / raw) To: Ben Gardner; +Cc: linux-kernel, Andrew Morton, David Brownell, Jani Nikula 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-26 23:39 ` Andres Salomon @ 2010-02-27 5:11 ` Ben Gardner 0 siblings, 0 replies; 21+ messages in thread From: Ben Gardner @ 2010-02-27 5:11 UTC (permalink / raw) To: Andres Salomon; +Cc: linux-kernel, Andrew Morton, David Brownell, Jani Nikula > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner 2010-02-26 23:39 ` Andres Salomon @ 2010-02-27 7:02 ` David Brownell 2010-02-27 10:26 ` Mark Brown 2010-02-27 18:36 ` Ben Gardner 2010-02-27 18:36 ` [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations Ben Gardner 2010-02-27 18:56 ` [PATCH v2] " Ben Gardner 3 siblings, 2 replies; 21+ messages in thread From: David Brownell @ 2010-02-27 7:02 UTC (permalink / raw) To: Ben Gardner; +Cc: linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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.) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 7:02 ` David Brownell @ 2010-02-27 10:26 ` Mark Brown 2010-02-27 16:24 ` David Brownell 2010-02-27 18:36 ` Ben Gardner 1 sibling, 1 reply; 21+ messages in thread From: Mark Brown @ 2010-02-27 10:26 UTC (permalink / raw) To: David Brownell Cc: Ben Gardner, linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 10:26 ` Mark Brown @ 2010-02-27 16:24 ` David Brownell 2010-02-27 17:20 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: David Brownell @ 2010-02-27 16:24 UTC (permalink / raw) To: Mark Brown Cc: Ben Gardner, linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 16:24 ` David Brownell @ 2010-02-27 17:20 ` Mark Brown 2010-02-27 17:51 ` David Brownell 0 siblings, 1 reply; 21+ messages in thread From: Mark Brown @ 2010-02-27 17:20 UTC (permalink / raw) To: David Brownell Cc: Ben Gardner, linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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). ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 17:20 ` Mark Brown @ 2010-02-27 17:51 ` David Brownell 2010-03-01 10:35 ` Mark Brown 0 siblings, 1 reply; 21+ messages in thread From: David Brownell @ 2010-02-27 17:51 UTC (permalink / raw) To: Mark Brown Cc: Ben Gardner, linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 17:51 ` David Brownell @ 2010-03-01 10:35 ` Mark Brown 0 siblings, 0 replies; 21+ messages in thread From: Mark Brown @ 2010-03-01 10:35 UTC (permalink / raw) To: David Brownell Cc: Ben Gardner, linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 7:02 ` David Brownell 2010-02-27 10:26 ` Mark Brown @ 2010-02-27 18:36 ` Ben Gardner 2010-02-27 19:23 ` David Brownell 1 sibling, 1 reply; 21+ messages in thread From: Ben Gardner @ 2010-02-27 18:36 UTC (permalink / raw) To: David Brownell; +Cc: linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] gpiolib: add gpio_set_direction() 2010-02-27 18:36 ` Ben Gardner @ 2010-02-27 19:23 ` David Brownell 0 siblings, 0 replies; 21+ messages in thread From: David Brownell @ 2010-02-27 19:23 UTC (permalink / raw) To: Ben Gardner; +Cc: linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner 2010-02-26 23:39 ` Andres Salomon 2010-02-27 7:02 ` David Brownell @ 2010-02-27 18:36 ` Ben Gardner 2010-02-27 18:40 ` Ben Gardner 2010-02-27 18:56 ` [PATCH v2] " Ben Gardner 3 siblings, 1 reply; 21+ messages in thread From: Ben Gardner @ 2010-02-27 18:36 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Mark Brown, David Brownell Cc: Ben Gardner, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations 2010-02-27 18:36 ` [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations Ben Gardner @ 2010-02-27 18:40 ` Ben Gardner 0 siblings, 0 replies; 21+ messages in thread From: Ben Gardner @ 2010-02-27 18:40 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Mark Brown, David Brownell Cc: Ben Gardner, Andrew Morton, David Brownell, Jani Nikula 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 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] cs5535-gpio: change input/output enable to match gpiolib expectations 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner ` (2 preceding siblings ...) 2010-02-27 18:36 ` [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations Ben Gardner @ 2010-02-27 18:56 ` Ben Gardner 2010-02-27 19:18 ` David Brownell 3 siblings, 1 reply; 21+ messages in thread From: Ben Gardner @ 2010-02-27 18:56 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Mark Brown, David Brownell Cc: Ben Gardner, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] cs5535-gpio: change input/output enable to match gpiolib expectations 2010-02-27 18:56 ` [PATCH v2] " Ben Gardner @ 2010-02-27 19:18 ` David Brownell 0 siblings, 0 replies; 21+ messages in thread From: David Brownell @ 2010-02-27 19:18 UTC (permalink / raw) To: Ben Gardner Cc: linux-kernel, Andres Salomon, Mark Brown, Andrew Morton, Jani Nikula 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] cs5535-gpio: Use set_direction 2010-02-26 23:26 [PATCH 0/3] gpio: add gpio_set_direction Ben Gardner 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner @ 2010-02-26 23:26 ` Ben Gardner 2010-02-27 7:04 ` David Brownell 2010-02-26 23:26 ` [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Ben Gardner 2 siblings, 1 reply; 21+ messages in thread From: Ben Gardner @ 2010-02-26 23:26 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Andrew Morton Cc: Ben Gardner, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] cs5535-gpio: Use set_direction 2010-02-26 23:26 ` [PATCH 2/3] cs5535-gpio: Use set_direction Ben Gardner @ 2010-02-27 7:04 ` David Brownell 2010-02-27 18:18 ` Ben Gardner 0 siblings, 1 reply; 21+ messages in thread From: David Brownell @ 2010-02-27 7:04 UTC (permalink / raw) To: Ben Gardner Cc: linux-kernel, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/3] cs5535-gpio: Use set_direction 2010-02-27 7:04 ` David Brownell @ 2010-02-27 18:18 ` Ben Gardner 0 siblings, 0 replies; 21+ messages in thread From: Ben Gardner @ 2010-02-27 18:18 UTC (permalink / raw) To: David Brownell Cc: linux-kernel, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula >> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input 2010-02-26 23:26 [PATCH 0/3] gpio: add gpio_set_direction Ben Gardner 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner 2010-02-26 23:26 ` [PATCH 2/3] cs5535-gpio: Use set_direction Ben Gardner @ 2010-02-26 23:26 ` Ben Gardner 2010-02-27 7:14 ` David Brownell 2 siblings, 1 reply; 21+ messages in thread From: Ben Gardner @ 2010-02-26 23:26 UTC (permalink / raw) To: linux-kernel, Andres Salomon, Andrew Morton Cc: Ben Gardner, Andres Salomon, Andrew Morton, David Brownell, Jani Nikula 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input 2010-02-26 23:26 ` [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Ben Gardner @ 2010-02-27 7:14 ` David Brownell 0 siblings, 0 replies; 21+ messages in thread From: David Brownell @ 2010-02-27 7:14 UTC (permalink / raw) To: Ben Gardner; +Cc: linux-kernel, Andres Salomon, Andrew Morton, Jani Nikula 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2010-03-01 10:35 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-26 23:26 [PATCH 0/3] gpio: add gpio_set_direction Ben Gardner 2010-02-26 23:26 ` [PATCH 1/3] gpiolib: add gpio_set_direction() Ben Gardner 2010-02-26 23:39 ` Andres Salomon 2010-02-27 5:11 ` Ben Gardner 2010-02-27 7:02 ` David Brownell 2010-02-27 10:26 ` Mark Brown 2010-02-27 16:24 ` David Brownell 2010-02-27 17:20 ` Mark Brown 2010-02-27 17:51 ` David Brownell 2010-03-01 10:35 ` Mark Brown 2010-02-27 18:36 ` Ben Gardner 2010-02-27 19:23 ` David Brownell 2010-02-27 18:36 ` [PATCH] cs5535-gpio: change input/output enable to match gpiolib expectations Ben Gardner 2010-02-27 18:40 ` Ben Gardner 2010-02-27 18:56 ` [PATCH v2] " Ben Gardner 2010-02-27 19:18 ` David Brownell 2010-02-26 23:26 ` [PATCH 2/3] cs5535-gpio: Use set_direction Ben Gardner 2010-02-27 7:04 ` David Brownell 2010-02-27 18:18 ` Ben Gardner 2010-02-26 23:26 ` [PATCH 3/3] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Ben Gardner 2010-02-27 7:14 ` David Brownell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.