* Re: [PATCH] ARM: OMAP: Fix GPIO switch initial output state handling
[not found] <1236677181-16601-1-git-send-email-ext-jani.1.nikula@nokia.com>
@ 2009-03-10 14:25 ` Kainan Cha
2009-03-11 9:03 ` Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Kainan Cha @ 2009-03-10 14:25 UTC (permalink / raw)
To: Jani Nikula; +Cc: linux-omap
Jani,
Please see my comments below.
On Tue, Mar 10, 2009 at 4:26 AM, Jani Nikula
<ext-jani.1.nikula@nokia.com> wrote:
> The switchover to cross-platform GPIO interface unexpectedly caused all
> output GPIO switches to be set to high state by default, unlike the
> original OMAP code. Introduce a new GPIO switch flag to define the
> initial state. Unless the flag is set, the default is now low state.
>
> Also store the state of output switches directly into the switch struct
> instead of trying to read an output GPIO.
>
> Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
> ---
> arch/arm/plat-omap/gpio-switch.c | 13 +++++++++----
> arch/arm/plat-omap/include/mach/gpio-switch.h | 1 +
> 2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio-switch.c b/arch/arm/plat-omap/gpio-switch.c
> index 2b5665d..b278024 100644
> --- a/arch/arm/plat-omap/gpio-switch.c
> +++ b/arch/arm/plat-omap/gpio-switch.c
> @@ -286,12 +286,17 @@ static int __init new_switch(struct gpio_switch *sw)
>
> /* input: 1, output: 0 */
> direction = !(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT);
> - if (direction)
> + if (direction) {
> gpio_direction_input(sw->gpio);
> - else
> - gpio_direction_output(sw->gpio, true);
> + sw->state = gpio_sw_get_state(sw);
> + } else {
> + int state = sw->state =
> + !!(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH);
>
> - sw->state = gpio_sw_get_state(sw);
> + if (sw->flags & OMAP_GPIO_SWITCH_FLAG_INVERTED)
> + state = !state;
Using OMAP_GPIO_SWITCH_FLAG_INVERTED along with
OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH here is somewhat confusing to
me.
INIT_HIGH indicates the state of the gpio, not the state of the
switch. Why not ignore the OMAP_GPIO_SWITCH_FLAG_INVERTED all together
when setting up the switch. This way, the user does not have to think
twice. :)
> + gpio_direction_output(sw->gpio, state);
> + }
>
> r = 0;
> r |= device_create_file(&sw->pdev.dev, &dev_attr_state);
> diff --git a/arch/arm/plat-omap/include/mach/gpio-switch.h b/arch/arm/plat-omap/include/mach/gpio-switch.h
> index a143253..107d23f 100644
> --- a/arch/arm/plat-omap/include/mach/gpio-switch.h
> +++ b/arch/arm/plat-omap/include/mach/gpio-switch.h
> @@ -29,6 +29,7 @@
> #define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
> #define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
> #define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
> +#define OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH 0x0004
>
> struct omap_gpio_switch {
> const char *name;
> --
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: OMAP: Fix GPIO switch initial output state handling
2009-03-10 14:25 ` [PATCH] ARM: OMAP: Fix GPIO switch initial output state handling Kainan Cha
@ 2009-03-11 9:03 ` Jani Nikula
2009-03-11 11:35 ` Juha Yrjola
0 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2009-03-11 9:03 UTC (permalink / raw)
To: ext Kainan Cha; +Cc: linux-omap@vger.kernel.org, juha.yrjola
On Tue, 2009-03-10 at 15:25 +0100, ext Kainan Cha wrote:
> Jani,
>
> Please see my comments below.
>
> On Tue, Mar 10, 2009 at 4:26 AM, Jani Nikula
> <ext-jani.1.nikula@nokia.com> wrote:
> > The switchover to cross-platform GPIO interface unexpectedly caused all
> > output GPIO switches to be set to high state by default, unlike the
> > original OMAP code. Introduce a new GPIO switch flag to define the
> > initial state. Unless the flag is set, the default is now low state.
> >
> > Also store the state of output switches directly into the switch struct
> > instead of trying to read an output GPIO.
> >
> > Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
> > ---
> > arch/arm/plat-omap/gpio-switch.c | 13 +++++++++----
> > arch/arm/plat-omap/include/mach/gpio-switch.h | 1 +
> > 2 files changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/gpio-switch.c b/arch/arm/plat-omap/gpio-switch.c
> > index 2b5665d..b278024 100644
> > --- a/arch/arm/plat-omap/gpio-switch.c
> > +++ b/arch/arm/plat-omap/gpio-switch.c
> > @@ -286,12 +286,17 @@ static int __init new_switch(struct gpio_switch *sw)
> >
> > /* input: 1, output: 0 */
> > direction = !(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT);
> > - if (direction)
> > + if (direction) {
> > gpio_direction_input(sw->gpio);
> > - else
> > - gpio_direction_output(sw->gpio, true);
> > + sw->state = gpio_sw_get_state(sw);
> > + } else {
> > + int state = sw->state =
> > + !!(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH);
> >
> > - sw->state = gpio_sw_get_state(sw);
> > + if (sw->flags & OMAP_GPIO_SWITCH_FLAG_INVERTED)
> > + state = !state;
>
> Using OMAP_GPIO_SWITCH_FLAG_INVERTED along with
> OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH here is somewhat confusing to
> me.
>
> INIT_HIGH indicates the state of the gpio, not the state of the
> switch. Why not ignore the OMAP_GPIO_SWITCH_FLAG_INVERTED all together
> when setting up the switch. This way, the user does not have to think
> twice. :)
The INVERTED flag can't be ignored, since sw->state must be the opposite
of the GPIO if the flag is set. The only question is, should INIT_HIGH
flag refer to the GPIO or the switch.
I did think about this, and the patch is as I intended it to be, i.e.
INIT_HIGH refers to the switch. I thought this would be less confusing.
If INVERTED is set, it's inverted *everywhere* - why not also when
setting the initial value?
BR,
Jani.
>
> > + gpio_direction_output(sw->gpio, state);
> > + }
> >
> > r = 0;
> > r |= device_create_file(&sw->pdev.dev, &dev_attr_state);
> > diff --git a/arch/arm/plat-omap/include/mach/gpio-switch.h b/arch/arm/plat-omap/include/mach/gpio-switch.h
> > index a143253..107d23f 100644
> > --- a/arch/arm/plat-omap/include/mach/gpio-switch.h
> > +++ b/arch/arm/plat-omap/include/mach/gpio-switch.h
> > @@ -29,6 +29,7 @@
> > #define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
> > #define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
> > #define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
> > +#define OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_HIGH 0x0004
> >
> > struct omap_gpio_switch {
> > const char *name;
> > --
> > 1.6.0.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ARM: OMAP: Fix GPIO switch initial output state handling
2009-03-11 9:03 ` Jani Nikula
@ 2009-03-11 11:35 ` Juha Yrjola
2009-03-11 13:07 ` [PATCH v2] " Jani Nikula
0 siblings, 1 reply; 6+ messages in thread
From: Juha Yrjola @ 2009-03-11 11:35 UTC (permalink / raw)
To: Jani Nikula; +Cc: ext Kainan Cha, linux-omap@vger.kernel.org
Jani Nikula wrote:
>> INIT_HIGH indicates the state of the gpio, not the state of the
>> switch. Why not ignore the OMAP_GPIO_SWITCH_FLAG_INVERTED all together
>> when setting up the switch. This way, the user does not have to think
>> twice. :)
>
> The INVERTED flag can't be ignored, since sw->state must be the opposite
> of the GPIO if the flag is set. The only question is, should INIT_HIGH
> flag refer to the GPIO or the switch.
>
> I did think about this, and the patch is as I intended it to be, i.e.
> INIT_HIGH refers to the switch. I thought this would be less confusing.
> If INVERTED is set, it's inverted *everywhere* - why not also when
> setting the initial value?
How about INIT_ACTIVE to reduce possible confusion? HIGH does imply an
electrical level, as opposed to a more abstract activation level.
Cheers,
Juha
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] ARM: OMAP: Fix GPIO switch initial output state handling
2009-03-11 11:35 ` Juha Yrjola
@ 2009-03-11 13:07 ` Jani Nikula
2009-03-11 13:35 ` Kainan Cha
2009-03-11 16:38 ` [APPLIED] " Tony Lindgren
0 siblings, 2 replies; 6+ messages in thread
From: Jani Nikula @ 2009-03-11 13:07 UTC (permalink / raw)
To: linux-omap; +Cc: juha.yrjola, ext-jani.1.nikula
The switchover to cross-platform GPIO interface unexpectedly caused all
output GPIO switches to be set to high state by default, unlike the
original OMAP code. Introduce a new GPIO switch flag to define the
initial state of the switch. Unless the flag is set, the default is now
inactive state of the switch.
Also store the state of output switches directly into the switch struct
instead of trying to read an output GPIO.
Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
---
arch/arm/plat-omap/gpio-switch.c | 13 +++++++++----
arch/arm/plat-omap/include/mach/gpio-switch.h | 11 ++++++-----
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/arch/arm/plat-omap/gpio-switch.c b/arch/arm/plat-omap/gpio-switch.c
index 2b5665d..9053ea0 100644
--- a/arch/arm/plat-omap/gpio-switch.c
+++ b/arch/arm/plat-omap/gpio-switch.c
@@ -286,12 +286,17 @@ static int __init new_switch(struct gpio_switch *sw)
/* input: 1, output: 0 */
direction = !(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT);
- if (direction)
+ if (direction) {
gpio_direction_input(sw->gpio);
- else
- gpio_direction_output(sw->gpio, true);
+ sw->state = gpio_sw_get_state(sw);
+ } else {
+ int state = sw->state = !!(sw->flags &
+ OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_ACTIVE);
- sw->state = gpio_sw_get_state(sw);
+ if (sw->flags & OMAP_GPIO_SWITCH_FLAG_INVERTED)
+ state = !state;
+ gpio_direction_output(sw->gpio, state);
+ }
r = 0;
r |= device_create_file(&sw->pdev.dev, &dev_attr_state);
diff --git a/arch/arm/plat-omap/include/mach/gpio-switch.h b/arch/arm/plat-omap/include/mach/gpio-switch.h
index a143253..2096780 100644
--- a/arch/arm/plat-omap/include/mach/gpio-switch.h
+++ b/arch/arm/plat-omap/include/mach/gpio-switch.h
@@ -24,11 +24,12 @@
* low -> inactive
*
*/
-#define OMAP_GPIO_SWITCH_TYPE_COVER 0x0000
-#define OMAP_GPIO_SWITCH_TYPE_CONNECTION 0x0001
-#define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
-#define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
-#define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
+#define OMAP_GPIO_SWITCH_TYPE_COVER 0x0000
+#define OMAP_GPIO_SWITCH_TYPE_CONNECTION 0x0001
+#define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
+#define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
+#define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
+#define OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_ACTIVE 0x0004
struct omap_gpio_switch {
const char *name;
--
1.6.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ARM: OMAP: Fix GPIO switch initial output state handling
2009-03-11 13:07 ` [PATCH v2] " Jani Nikula
@ 2009-03-11 13:35 ` Kainan Cha
2009-03-11 16:38 ` [APPLIED] " Tony Lindgren
1 sibling, 0 replies; 6+ messages in thread
From: Kainan Cha @ 2009-03-11 13:35 UTC (permalink / raw)
To: Jani Nikula; +Cc: linux-omap, juha.yrjola
I am okay with this. Thanks for the patch.
On Wed, Mar 11, 2009 at 8:07 AM, Jani Nikula
<ext-jani.1.nikula@nokia.com> wrote:
> The switchover to cross-platform GPIO interface unexpectedly caused all
> output GPIO switches to be set to high state by default, unlike the
> original OMAP code. Introduce a new GPIO switch flag to define the
> initial state of the switch. Unless the flag is set, the default is now
> inactive state of the switch.
>
> Also store the state of output switches directly into the switch struct
> instead of trying to read an output GPIO.
>
> Signed-off-by: Jani Nikula <ext-jani.1.nikula@nokia.com>
> ---
> arch/arm/plat-omap/gpio-switch.c | 13 +++++++++----
> arch/arm/plat-omap/include/mach/gpio-switch.h | 11 ++++++-----
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/plat-omap/gpio-switch.c b/arch/arm/plat-omap/gpio-switch.c
> index 2b5665d..9053ea0 100644
> --- a/arch/arm/plat-omap/gpio-switch.c
> +++ b/arch/arm/plat-omap/gpio-switch.c
> @@ -286,12 +286,17 @@ static int __init new_switch(struct gpio_switch *sw)
>
> /* input: 1, output: 0 */
> direction = !(sw->flags & OMAP_GPIO_SWITCH_FLAG_OUTPUT);
> - if (direction)
> + if (direction) {
> gpio_direction_input(sw->gpio);
> - else
> - gpio_direction_output(sw->gpio, true);
> + sw->state = gpio_sw_get_state(sw);
> + } else {
> + int state = sw->state = !!(sw->flags &
> + OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_ACTIVE);
>
> - sw->state = gpio_sw_get_state(sw);
> + if (sw->flags & OMAP_GPIO_SWITCH_FLAG_INVERTED)
> + state = !state;
> + gpio_direction_output(sw->gpio, state);
> + }
>
> r = 0;
> r |= device_create_file(&sw->pdev.dev, &dev_attr_state);
> diff --git a/arch/arm/plat-omap/include/mach/gpio-switch.h b/arch/arm/plat-omap/include/mach/gpio-switch.h
> index a143253..2096780 100644
> --- a/arch/arm/plat-omap/include/mach/gpio-switch.h
> +++ b/arch/arm/plat-omap/include/mach/gpio-switch.h
> @@ -24,11 +24,12 @@
> * low -> inactive
> *
> */
> -#define OMAP_GPIO_SWITCH_TYPE_COVER 0x0000
> -#define OMAP_GPIO_SWITCH_TYPE_CONNECTION 0x0001
> -#define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
> -#define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
> -#define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
> +#define OMAP_GPIO_SWITCH_TYPE_COVER 0x0000
> +#define OMAP_GPIO_SWITCH_TYPE_CONNECTION 0x0001
> +#define OMAP_GPIO_SWITCH_TYPE_ACTIVITY 0x0002
> +#define OMAP_GPIO_SWITCH_FLAG_INVERTED 0x0001
> +#define OMAP_GPIO_SWITCH_FLAG_OUTPUT 0x0002
> +#define OMAP_GPIO_SWITCH_FLAG_OUTPUT_INIT_ACTIVE 0x0004
>
> struct omap_gpio_switch {
> const char *name;
> --
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [APPLIED] [PATCH v2] ARM: OMAP: Fix GPIO switch initial output state handling
2009-03-11 13:07 ` [PATCH v2] " Jani Nikula
2009-03-11 13:35 ` Kainan Cha
@ 2009-03-11 16:38 ` Tony Lindgren
1 sibling, 0 replies; 6+ messages in thread
From: Tony Lindgren @ 2009-03-11 16:38 UTC (permalink / raw)
To: linux-omap
This patch has been applied to the linux-omap
by youw fwiendly patch wobot.
Commit: cc2b459c066361098704c586f3134c3c3ac13be3
PatchWorks
http://patchwork.kernel.org/patch/11128/
Git
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=cc2b459c066361098704c586f3134c3c3ac13be3
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-11 16:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1236677181-16601-1-git-send-email-ext-jani.1.nikula@nokia.com>
2009-03-10 14:25 ` [PATCH] ARM: OMAP: Fix GPIO switch initial output state handling Kainan Cha
2009-03-11 9:03 ` Jani Nikula
2009-03-11 11:35 ` Juha Yrjola
2009-03-11 13:07 ` [PATCH v2] " Jani Nikula
2009-03-11 13:35 ` Kainan Cha
2009-03-11 16:38 ` [APPLIED] " Tony Lindgren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox