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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
