* [PATCH] cs5535_gpio: gpio_chip.get should return the input value
@ 2010-02-23 22:55 Ben Gardner
2010-02-24 22:58 ` Andrew Morton
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Ben Gardner @ 2010-02-23 22:55 UTC (permalink / raw)
To: Andres Salomon, linux-kernel
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 <gardner.ben@gmail.com>
---
--- 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)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-23 22:55 [PATCH] cs5535_gpio: gpio_chip.get should return the input value Ben Gardner
@ 2010-02-24 22:58 ` Andrew Morton
2010-02-25 3:52 ` Ben Gardner
2010-02-24 23:42 ` Andres Salomon
2010-02-24 23:45 ` [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Andres Salomon
2 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2010-02-24 22:58 UTC (permalink / raw)
To: Ben Gardner; +Cc: Andres Salomon, linux-kernel
On Tue, 23 Feb 2010 16:55:17 -0600 Ben Gardner <gardner.ben@gmail.com> 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 <gardner.ben@gmail.com>
> ---
> --- 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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-23 22:55 [PATCH] cs5535_gpio: gpio_chip.get should return the input value Ben Gardner
2010-02-24 22:58 ` Andrew Morton
@ 2010-02-24 23:42 ` Andres Salomon
2010-02-24 23:45 ` [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Andres Salomon
2 siblings, 0 replies; 13+ messages in thread
From: Andres Salomon @ 2010-02-24 23:42 UTC (permalink / raw)
To: Ben Gardner; +Cc: linux-kernel, Andrew Morton
On Tue, 23 Feb 2010 16:55:17 -0600
Ben Gardner <gardner.ben@gmail.com> 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 <gardner.ben@gmail.com>
> ---
> --- 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 <dilinger@collabora.co.uk>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-02-23 22:55 [PATCH] cs5535_gpio: gpio_chip.get should return the input value Ben Gardner
2010-02-24 22:58 ` Andrew Morton
2010-02-24 23:42 ` Andres Salomon
@ 2010-02-24 23:45 ` Andres Salomon
2010-02-26 17:47 ` Ben Gardner
2 siblings, 1 reply; 13+ messages in thread
From: Andres Salomon @ 2010-02-24 23:45 UTC (permalink / raw)
To: Ben Gardner; +Cc: linux-kernel, Andrew Morton
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 <dilinger@collabora.co.uk>
---
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-24 22:58 ` Andrew Morton
@ 2010-02-25 3:52 ` Ben Gardner
2010-02-25 4:05 ` Andrew Morton
0 siblings, 1 reply; 13+ messages in thread
From: Ben Gardner @ 2010-02-25 3:52 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andres Salomon, linux-kernel
>> --- 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-25 3:52 ` Ben Gardner
@ 2010-02-25 4:05 ` Andrew Morton
2010-02-25 4:16 ` Ben Gardner
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2010-02-25 4:05 UTC (permalink / raw)
To: Ben Gardner; +Cc: Andres Salomon, linux-kernel, stable
On Wed, 24 Feb 2010 21:52:07 -0600 Ben Gardner <gardner.ben@gmail.com> 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..
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-25 4:05 ` Andrew Morton
@ 2010-02-25 4:16 ` Ben Gardner
2010-02-25 5:04 ` Andres Salomon
0 siblings, 1 reply; 13+ messages in thread
From: Ben Gardner @ 2010-02-25 4:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: Andres Salomon, linux-kernel, stable
>> > 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] cs5535_gpio: gpio_chip.get should return the input value
2010-02-25 4:16 ` Ben Gardner
@ 2010-02-25 5:04 ` Andres Salomon
0 siblings, 0 replies; 13+ messages in thread
From: Andres Salomon @ 2010-02-25 5:04 UTC (permalink / raw)
To: Ben Gardner; +Cc: Andrew Morton, linux-kernel, stable
On Wed, 24 Feb 2010 22:16:43 -0600
Ben Gardner <gardner.ben@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-02-24 23:45 ` [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Andres Salomon
@ 2010-02-26 17:47 ` Ben Gardner
2010-02-26 21:07 ` Andres Salomon
0 siblings, 1 reply; 13+ messages in thread
From: Ben Gardner @ 2010-02-26 17:47 UTC (permalink / raw)
To: Andres Salomon; +Cc: linux-kernel, Andrew Morton
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 <dilinger@collabora.co.uk>
> ---
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-02-26 17:47 ` Ben Gardner
@ 2010-02-26 21:07 ` Andres Salomon
2010-02-26 23:32 ` Ben Gardner
0 siblings, 1 reply; 13+ messages in thread
From: Andres Salomon @ 2010-02-26 21:07 UTC (permalink / raw)
To: Ben Gardner; +Cc: linux-kernel, Andrew Morton
On Fri, 26 Feb 2010 11:47:35 -0600
Ben Gardner <gardner.ben@gmail.com> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-02-26 21:07 ` Andres Salomon
@ 2010-02-26 23:32 ` Ben Gardner
2010-03-01 14:55 ` Ben Gardner
0 siblings, 1 reply; 13+ messages in thread
From: Ben Gardner @ 2010-02-26 23:32 UTC (permalink / raw)
To: Andres Salomon; +Cc: linux-kernel, Andrew Morton
>> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-02-26 23:32 ` Ben Gardner
@ 2010-03-01 14:55 ` Ben Gardner
2010-03-01 15:17 ` Andres Salomon
0 siblings, 1 reply; 13+ messages in thread
From: Ben Gardner @ 2010-03-01 14:55 UTC (permalink / raw)
To: Andres Salomon, Andrew Morton; +Cc: linux-kernel
> Lets see how the proposed changes I just sent off are received and go
> from there.
Based on the feedback from the other gpio patches, I'd like to do the following:
1) drop this patch (olpc-alsa-fix-cs5535audios-mic-gpio-to-enable-input.patch)
2) drop the patch that caused this patch to be created
(cs5535_gpio-gpio_chipget-should-return-the-input-value.patch)
3) submit a new patch that contains the original change + the changes
needed to adjust input/output settings to follow the spirit of
gpiolib.
That way, there is one patch that fully fixes the issue and no window
where the OLPC MIC stuff is broken.
Ben
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input
2010-03-01 14:55 ` Ben Gardner
@ 2010-03-01 15:17 ` Andres Salomon
0 siblings, 0 replies; 13+ messages in thread
From: Andres Salomon @ 2010-03-01 15:17 UTC (permalink / raw)
To: Ben Gardner; +Cc: Andrew Morton, linux-kernel
On Mon, 1 Mar 2010 08:55:44 -0600
Ben Gardner <gardner.ben@gmail.com> wrote:
> > Lets see how the proposed changes I just sent off are received and go
> > from there.
>
> Based on the feedback from the other gpio patches, I'd like to do the following:
> 1) drop this patch (olpc-alsa-fix-cs5535audios-mic-gpio-to-enable-input.patch)
> 2) drop the patch that caused this patch to be created
> (cs5535_gpio-gpio_chipget-should-return-the-input-value.patch)
> 3) submit a new patch that contains the original change + the changes
> needed to adjust input/output settings to follow the spirit of
> gpiolib.
>
> That way, there is one patch that fully fixes the issue and no window
> where the OLPC MIC stuff is broken.
>
That'd be great, thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-03-01 15:18 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-23 22:55 [PATCH] cs5535_gpio: gpio_chip.get should return the input value Ben Gardner
2010-02-24 22:58 ` Andrew Morton
2010-02-25 3:52 ` Ben Gardner
2010-02-25 4:05 ` Andrew Morton
2010-02-25 4:16 ` Ben Gardner
2010-02-25 5:04 ` Andres Salomon
2010-02-24 23:42 ` Andres Salomon
2010-02-24 23:45 ` [PATCH] OLPC: ALSA: fix cs5535audio's MIC GPIO to enable input Andres Salomon
2010-02-26 17:47 ` Ben Gardner
2010-02-26 21:07 ` Andres Salomon
2010-02-26 23:32 ` Ben Gardner
2010-03-01 14:55 ` Ben Gardner
2010-03-01 15:17 ` Andres Salomon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).