The gpio_chip.get() function for the CS5535 GPIO driver currently returns the
output value instead of the input value.
This patch changes it to return the input value.
Signed-off-by: Ben Gardner <[email protected]>
---
--- linux-2.6.33-rc8.orig/drivers/gpio/cs5535-gpio.c
+++ linux-2.6.33-rc8/drivers/gpio/cs5535-gpio.c
@@ -154,7 +154,7 @@
static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
{
- return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
+ return cs5535_gpio_isset(offset, GPIO_READ_BACK);
}
static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
On Tue, 23 Feb 2010 16:55:17 -0600 Ben Gardner <[email protected]> wrote:
> The gpio_chip.get() function for the CS5535 GPIO driver currently returns the
> output value instead of the input value.
> This patch changes it to return the input value.
>
> Signed-off-by: Ben Gardner <[email protected]>
> ---
> --- linux-2.6.33-rc8.orig/drivers/gpio/cs5535-gpio.c
> +++ linux-2.6.33-rc8/drivers/gpio/cs5535-gpio.c
> @@ -154,7 +154,7 @@
>
> static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> - return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
> + return cs5535_gpio_isset(offset, GPIO_READ_BACK);
> }
>
> static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
<presses F10>
What were the user-visible effects of this bug?
On Tue, 23 Feb 2010 16:55:17 -0600
Ben Gardner <[email protected]> wrote:
> The gpio_chip.get() function for the CS5535 GPIO driver currently
> returns the output value instead of the input value.
> This patch changes it to return the input value.
>
> Signed-off-by: Ben Gardner <[email protected]>
> ---
> --- linux-2.6.33-rc8.orig/drivers/gpio/cs5535-gpio.c
> +++ linux-2.6.33-rc8/drivers/gpio/cs5535-gpio.c
> @@ -154,7 +154,7 @@
>
> static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
> - return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
> + return cs5535_gpio_isset(offset, GPIO_READ_BACK);
> }
>
> static void chip_gpio_set(struct gpio_chip *chip, unsigned offset,
> int val)
Hm, you're probably right. Of course this breaks cs5535audio_olpc.c,
since the GPIO isn't marked bidirectional (only output is enabled).
I'll follow up w/ a patch.
Acked-by: Andres Salomon <[email protected]>
Previously the MIC GPIO was set to output mode, and when checking the status
after setting it we were checking OUTPUT_VAL. This worked, though I'm not
quite sure why. Instead, if we actually check the READ_BACK value, it
doesn't work unless the GPIO is in bidirectional mode. Thus, enable
input mode as well.
Signed-off-by: Andres Salomon <[email protected]>
---
sound/pci/cs5535audio/cs5535audio_olpc.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/pci/cs5535audio/cs5535audio_olpc.c b/sound/pci/cs5535audio/cs5535audio_olpc.c
index 50da49b..f5574f2 100644
--- a/sound/pci/cs5535audio/cs5535audio_olpc.c
+++ b/sound/pci/cs5535audio/cs5535audio_olpc.c
@@ -157,6 +157,7 @@ int __devinit olpc_quirks(struct snd_card *card, struct snd_ac97 *ac97)
return -EIO;
}
gpio_direction_output(OLPC_GPIO_MIC_AC, 0);
+ gpio_direction_input(OLPC_GPIO_MIC_AC);
/* drop the original AD1888 HPF control */
memset(&elem, 0, sizeof(elem));
--
1.5.6.5
>> --- linux-2.6.33-rc8.orig/drivers/gpio/cs5535-gpio.c
>> +++ linux-2.6.33-rc8/drivers/gpio/cs5535-gpio.c
>> @@ -154,7 +154,7 @@
>>
>> ?static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
>> ?{
>> - ? ? return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
>> + ? ? return cs5535_gpio_isset(offset, GPIO_READ_BACK);
>> ?}
>>
>> ?static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
>
> <presses F10>
>
> What were the user-visible effects of this bug?
The user-visible effects were that the input didn't work.
I use the CS5535 chip in a custom board that uses GPIO 25 and 26 as
voltage monitoring inputs (external voltage comparators).
Since the char-based cs5535-gpio is now deprecated, I'm trying out the
gpio driver.
I export the GPIO pins via sysfs to access them from user-space.
Reading the 'value' member didn't reflect the input.
Personally, I would prefer a separate sysfs entry for the output and
the input, since the GPIO pins can be both outputs and inputs, but
that would require a gpiolib change.
Since there doesn't appear to be a gpio maintainer, any change to that
might be tricky.
Ben
On Wed, 24 Feb 2010 21:52:07 -0600 Ben Gardner <[email protected]> wrote:
> >> --- linux-2.6.33-rc8.orig/drivers/gpio/cs5535-gpio.c
> >> +++ linux-2.6.33-rc8/drivers/gpio/cs5535-gpio.c
> >> @@ -154,7 +154,7 @@
> >>
> >> __static int chip_gpio_get(struct gpio_chip *chip, unsigned offset)
> >> __{
> >> - __ __ return cs5535_gpio_isset(offset, GPIO_OUTPUT_VAL);
> >> + __ __ return cs5535_gpio_isset(offset, GPIO_READ_BACK);
> >> __}
> >>
> >> __static void chip_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> >
> > <presses F10>
> >
> > What were the user-visible effects of this bug?
>
> The user-visible effects were that the input didn't work.
> I use the CS5535 chip in a custom board that uses GPIO 25 and 26 as
> voltage monitoring inputs (external voltage comparators).
> Since the char-based cs5535-gpio is now deprecated, I'm trying out the
> gpio driver.
> I export the GPIO pins via sysfs to access them from user-space.
> Reading the 'value' member didn't reflect the input.
OK, then in that case we'd want to backport this into 2.6.33.x and
perhaps earlier?
> Personally, I would prefer a separate sysfs entry for the output and
> the input, since the GPIO pins can be both outputs and inputs, but
> that would require a gpiolib change.
> Since there doesn't appear to be a gpio maintainer, any change to that
> might be tricky.
Eh, we can cope. Propose a patch and cc lots of people..
>> > What were the user-visible effects of this bug?
>>
>> The user-visible effects were that the input didn't work.
>> I use the CS5535 chip in a custom board that uses GPIO 25 and 26 as
>> voltage monitoring inputs (external voltage comparators).
>> Since the char-based cs5535-gpio is now deprecated, I'm trying out the
>> gpio driver.
>> I export the GPIO pins via sysfs to access them from user-space.
>> Reading the 'value' member didn't reflect the input.
>
> OK, then in that case we'd want to backport this into 2.6.33.x and
> perhaps earlier?
It looks like the driver was added 15 Dec 2009, so it doesn't look
like it existed in an earlier kernel.
>> Personally, I would prefer a separate sysfs entry for the output and
>> the input, since the GPIO pins can be both outputs and inputs, but
>> that would require a gpiolib change.
>> Since there doesn't appear to be a gpio maintainer, any change to that
>> might be tricky.
>
> Eh, we can cope. ?Propose a patch and cc lots of people..
Will do.
Thanks,
Ben
On Wed, 24 Feb 2010 22:16:43 -0600
Ben Gardner <[email protected]> wrote:
> >> > What were the user-visible effects of this bug?
> >>
> >> The user-visible effects were that the input didn't work.
> >> I use the CS5535 chip in a custom board that uses GPIO 25 and 26 as
> >> voltage monitoring inputs (external voltage comparators).
> >> Since the char-based cs5535-gpio is now deprecated, I'm trying out
> >> the gpio driver.
> >> I export the GPIO pins via sysfs to access them from user-space.
> >> Reading the 'value' member didn't reflect the input.
> >
> > OK, then in that case we'd want to backport this into 2.6.33.x and
> > perhaps earlier?
>
> It looks like the driver was added 15 Dec 2009, so it doesn't look
> like it existed in an earlier kernel.
But it should definitely be added to 2.6.33.y.
Hi Andres,
> Previously the MIC GPIO was set to output mode, and when checking the status
> after setting it we were checking OUTPUT_VAL. ?This worked, though I'm not
> quite sure why. Instead, if we actually check the READ_BACK value, it
> doesn't work unless the GPIO is in bidirectional mode. ?Thus, enable
> input mode as well.
The cs5535-gpio driver was reading output value, not the sensed input value.
When that was fixed, this OLPC code failed because the input wasn't
enabled, so the gpio_get_value() call didn't return anything useful.
At reset the CS5535/6 GPIOs have both input and output disabled.
> Signed-off-by: Andres Salomon <[email protected]>
> ---
> ?sound/pci/cs5535audio/cs5535audio_olpc.c | ? ?1 +
> ?1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/sound/pci/cs5535audio/cs5535audio_olpc.c b/sound/pci/cs5535audio/cs5535audio_olpc.c
> index 50da49b..f5574f2 100644
> --- a/sound/pci/cs5535audio/cs5535audio_olpc.c
> +++ b/sound/pci/cs5535audio/cs5535audio_olpc.c
> @@ -157,6 +157,7 @@ int __devinit olpc_quirks(struct snd_card *card, struct snd_ac97 *ac97)
> ? ? ? ? ? ? ? ?return -EIO;
> ? ? ? ?}
> ? ? ? ?gpio_direction_output(OLPC_GPIO_MIC_AC, 0);
> + ? ? ? gpio_direction_input(OLPC_GPIO_MIC_AC);
>
> ? ? ? ?/* drop the original AD1888 HPF control */
> ? ? ? ?memset(&elem, 0, sizeof(elem));
This will only work because the cs5535-gpio driver and gpiolib are
both 'broken'.
The problem with gpiolib is that it only allows a GPIO pin to be
either an input or output, but not both.
It has two separate functions to configure the direction
(gpio_direction_input/gpio_direction_output), where one should do (ie,
gpio_set_direction).
>From what I can tell, when direction_input() is called, the GPIO chip
is expected to disable the output and enable the input. If that really
occurred, then the above code would still be broken.
The cs5535-gpio driver doesn't follow that input-or-output convention.
It never disables the direction that wasn't requested.
I'm working on a gpiolib patch that will combine gpio_direction_output
and gpio_direction_input into one function. That would enable the
above driver to be 'obviously correct'.
Ben
On Fri, 26 Feb 2010 11:47:35 -0600
Ben Gardner <[email protected]> wrote:
[...]
>
> I'm working on a gpiolib patch that will combine gpio_direction_output
> and gpio_direction_input into one function. That would enable the
> above driver to be 'obviously correct'.
>
Cool. I'm happy to help convert drivers if you want the help.
>> I'm working on a gpiolib patch that will combine gpio_direction_output
>> and gpio_direction_input into one function. ?That would enable the
>> above driver to be 'obviously correct'.
>
> Cool. ?I'm happy to help convert drivers if you want the help.
I'm not one to turn down help.
Lets see how the proposed changes I just sent off are received and go
from there.
Ben