| Message ID | 1236677181-16601-1-git-send-email-ext-jani.1.nikula@nokia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 > -- 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
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 > > -- 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
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 -- 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
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; + 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;
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(-)