| Message ID | 1326242752-9981-3-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Frysinger |
| Headers | show |
On Tuesday 10 January 2012 19:45:47 Simon Glass wrote: > This provides a way of simulating GPIOs by setting values which are seen > by the normal gpio_get/set_value() calls. seems to be a desync in types ... all "gpio" fields should be "unsigned" and not "int" > --- /dev/null > +++ b/arch/sandbox/include/asm/gpio.h > > +int sandbox_gpio_get_value(int gp); why bother with parallel sandbox gpio API ? why can't we just implement the gpio API directly and throw away sandbox_gpio_xxx ? then we can also stub out sandbox/include/asm/gpio.h ... also, missing gpio_status() define to gpio_info() > --- /dev/null > +++ b/drivers/gpio/sandbox.c > > +enum { > + CMD_INFO, > + CMD_PORT, > + CMD_OUTPUT, > + CMD_INPUT, > +}; CMD_XXX enums are unused -> punt > +enum { > + GPIOF_OUTPUT = 1 << 1, > + GPIOF_HIGH = 1 << 2, > +}; turn enum bit flags into defines also, add a "reserved" bit flag and check it in gpio_request() > +/* read GPIO IN value of port 'gp' */ > +int gpio_get_value(int gp) > ... > + if (get_gpio_flag(gp, GPIOF_OUTPUT)) > ... > +int gpio_set_value(int gp, int value) > ... > + if (get_gpio_flag(gp, GPIOF_OUTPUT)) { drop valid gpio checking in these funcs ... only the request func should do this > +int gpio_request(unsigned gpio, const char *label) > +{ > + /* for now, do nothing */ > + return 0; > +} add (gpio <= CONFIG_SANDBOX_GPIO_COUNT) check to verify the gpio is valid > +int gpio_info(int gp) > +{ > + printf("Sandbox GPIOs\n"); > + return 0; > +} implement it ? -mike
Hi Mike, On Fri, Jan 20, 2012 at 10:59 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Tuesday 10 January 2012 19:45:47 Simon Glass wrote: >> This provides a way of simulating GPIOs by setting values which are seen >> by the normal gpio_get/set_value() calls. > > seems to be a desync in types ... all "gpio" fields should be "unsigned" and > not "int" OK I see that changed in November. I will update. > >> --- /dev/null >> +++ b/arch/sandbox/include/asm/gpio.h >> >> +int sandbox_gpio_get_value(int gp); > > why bother with parallel sandbox gpio API ? why can't we just implement the > gpio API directly and throw away sandbox_gpio_xxx ? then we can also stub out > sandbox/include/asm/gpio.h ... Because the current state of the GPIOs needs to be stored somewhere. Test code which wants a GPIO to appear to be high to U-Boot can call sandbox_gpio_set_value() and that value will be recorded and provided to future gpio_get_value() calls. Without this virtualisation, the driver would have no purpose. > > also, missing gpio_status() define to gpio_info() I see gpio_info() there at the bottom - gpio_status() is optional I think. Do we need to bring this in? > >> --- /dev/null >> +++ b/drivers/gpio/sandbox.c >> >> +enum { >> + CMD_INFO, >> + CMD_PORT, >> + CMD_OUTPUT, >> + CMD_INPUT, >> +}; > > CMD_XXX enums are unused -> punt Done > >> +enum { >> + GPIOF_OUTPUT = 1 << 1, >> + GPIOF_HIGH = 1 << 2, >> +}; > > turn enum bit flags into defines Done > > also, add a "reserved" bit flag and check it in gpio_request() Oh ok then, might as well do it now. I will also complain if someone uses a GPIO before requesting it. > >> +/* read GPIO IN value of port 'gp' */ >> +int gpio_get_value(int gp) >> ... >> + if (get_gpio_flag(gp, GPIOF_OUTPUT)) >> ... >> +int gpio_set_value(int gp, int value) >> ... >> + if (get_gpio_flag(gp, GPIOF_OUTPUT)) { > > drop valid gpio checking in these funcs ... only the request func should do > this What if someone passes in garbage value? Don't we want to catch this? > >> +int gpio_request(unsigned gpio, const char *label) >> +{ >> + /* for now, do nothing */ >> + return 0; >> +} > > add (gpio <= CONFIG_SANDBOX_GPIO_COUNT) check to verify the gpio is valid Done as part of implementing this function. > >> +int gpio_info(int gp) >> +{ >> + printf("Sandbox GPIOs\n"); >> + return 0; >> +} > > implement it ? Yes OK. > -mike Regards, Simon
On Monday 23 January 2012 01:20:16 Simon Glass wrote: > On Fri, Jan 20, 2012 at 10:59 AM, Mike Frysinger wrote: > > On Tuesday 10 January 2012 19:45:47 Simon Glass wrote: > >> --- /dev/null > >> +++ b/arch/sandbox/include/asm/gpio.h > >> > >> +int sandbox_gpio_get_value(int gp); > > > > why bother with parallel sandbox gpio API ? why can't we just implement > > the gpio API directly and throw away sandbox_gpio_xxx ? then we can > > also stub out sandbox/include/asm/gpio.h ... > > Because the current state of the GPIOs needs to be stored somewhere. > Test code which wants a GPIO to appear to be high to U-Boot can call > sandbox_gpio_set_value() and that value will be recorded and provided > to future gpio_get_value() calls. > > Without this virtualisation, the driver would have no purpose. i'm not seeing it. why does external code need to reach into the guts at all when we have a gpio API for them to use ? > > also, missing gpio_status() define to gpio_info() > > I see gpio_info() there at the bottom - gpio_status() is optional I > think. Do we need to bring this in? gpio_info is pointless without gpio_status. honestly, i don't even know why that symbol exists at all ... the public API is gpio_status(). might as well just call it that. > >> +/* read GPIO IN value of port 'gp' */ > >> +int gpio_get_value(int gp) > >> ... > >> + if (get_gpio_flag(gp, GPIOF_OUTPUT)) > >> ... > >> +int gpio_set_value(int gp, int value) > >> ... > >> + if (get_gpio_flag(gp, GPIOF_OUTPUT)) { > > > > drop valid gpio checking in these funcs ... only the request func should > > do this > > What if someone passes in garbage value? Don't we want to catch this? normally, no. in the sandbox world, i guess we do, but there should be verbose dump messages ... perhaps an assert() ? -mike
On Tuesday 24 January 2012 16:35:00 Mike Frysinger wrote: > On Monday 23 January 2012 01:20:16 Simon Glass wrote: > > On Fri, Jan 20, 2012 at 10:59 AM, Mike Frysinger wrote: > > > On Tuesday 10 January 2012 19:45:47 Simon Glass wrote: > > >> --- /dev/null > > >> +++ b/arch/sandbox/include/asm/gpio.h > > >> > > >> +int sandbox_gpio_get_value(int gp); > > > > > > why bother with parallel sandbox gpio API ? why can't we just > > > implement the gpio API directly and throw away sandbox_gpio_xxx ? > > > then we can also stub out sandbox/include/asm/gpio.h ... > > > > Because the current state of the GPIOs needs to be stored somewhere. > > Test code which wants a GPIO to appear to be high to U-Boot can call > > sandbox_gpio_set_value() and that value will be recorded and provided > > to future gpio_get_value() calls. > > > > Without this virtualisation, the driver would have no purpose. > > i'm not seeing it. why does external code need to reach into the guts at > all when we have a gpio API for them to use ? ok, you clarified it for me, so now we just need some comments in asm/gpio.h explaining the expected users of the internal API -mike
Hi Mike, On Tue, Jan 24, 2012 at 3:06 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Tuesday 24 January 2012 16:35:00 Mike Frysinger wrote: >> On Monday 23 January 2012 01:20:16 Simon Glass wrote: >> > On Fri, Jan 20, 2012 at 10:59 AM, Mike Frysinger wrote: >> > > On Tuesday 10 January 2012 19:45:47 Simon Glass wrote: >> > >> --- /dev/null >> > >> +++ b/arch/sandbox/include/asm/gpio.h >> > >> >> > >> +int sandbox_gpio_get_value(int gp); >> > > >> > > why bother with parallel sandbox gpio API ? why can't we just >> > > implement the gpio API directly and throw away sandbox_gpio_xxx ? >> > > then we can also stub out sandbox/include/asm/gpio.h ... >> > >> > Because the current state of the GPIOs needs to be stored somewhere. >> > Test code which wants a GPIO to appear to be high to U-Boot can call >> > sandbox_gpio_set_value() and that value will be recorded and provided >> > to future gpio_get_value() calls. >> > >> > Without this virtualisation, the driver would have no purpose. >> >> i'm not seeing it. why does external code need to reach into the guts at >> all when we have a gpio API for them to use ? > > ok, you clarified it for me, so now we just need some comments in asm/gpio.h > explaining the expected users of the internal API > -mike OK I will add a few more comments. Regards, Simon
diff --git a/arch/sandbox/include/asm/gpio.h b/arch/sandbox/include/asm/gpio.h new file mode 100644 index 0000000..405d875 --- /dev/null +++ b/arch/sandbox/include/asm/gpio.h @@ -0,0 +1,61 @@ +/* + * This is the interface to the sandbox GPIO driver for test code which + * wants to change the GPIO values reported to U-Boot. + * + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __ASM_SANDBOX_GPIO_H +#define __ASM_SANDBOX_GPIO_H + +/* We use the generic interface, and add a back channel */ +#include <asm-generic/gpio.h> + +/** + * Return the value of a GPIO + * + * @param gp GPIO number + * @return -1 on error, 0 if GPIO is low, >0 if high + */ +int sandbox_gpio_get_value(int gp); + +/** + * @param gp GPIO number + * @param value value to set (0 for low, non-zero for high) + * @return -1 on error, 0 if ok + */ +int sandbox_gpio_set_value(int gp, int value); + +/** + * Return the direction of a GPIO + * + * @param gp GPIO number + * @return -1 on error, 0 if GPIO is input, >0 if output + */ +int sandbox_gpio_get_direction(int gp); + +/** + * @param gp GPIO number + * @param output 0 to set as input, 1 to set as output + * @return -1 on error, 0 if ok + */ +int sandbox_gpio_set_direction(int gp, int output); + +#endif diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index e22c096..7315cfc 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MXS_GPIO) += mxs_gpio.o COBJS-$(CONFIG_PCA953X) += pca953x.o COBJS-$(CONFIG_PCA9698) += pca9698.o COBJS-$(CONFIG_S5P) += s5p_gpio.o +COBJS-$(CONFIG_SANDBOX_GPIO) += sandbox.o COBJS-$(CONFIG_TEGRA2_GPIO) += tegra2_gpio.o COBJS-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o COBJS-$(CONFIG_ALTERA_PIO) += altera_pio.o diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c new file mode 100644 index 0000000..2c36b89 --- /dev/null +++ b/drivers/gpio/sandbox.c @@ -0,0 +1,152 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <asm/io.h> +#include <asm/bitops.h> +#include <asm-generic/gpio.h> +#include <asm/gpio.h> + +enum { + CMD_INFO, + CMD_PORT, + CMD_OUTPUT, + CMD_INPUT, +}; + +/* Flags for each GPIO */ +enum { + GPIOF_OUTPUT = 1 << 1, + GPIOF_HIGH = 1 << 2, +}; + +/* TODO: Put this into sandbox state */ +static u8 state[CONFIG_SANDBOX_GPIO_COUNT]; /* State of GPIOs */ + + +/* Access routines for GPIO state */ +static u8 *get_gpio(int gp) +{ + assert(gp >= 0 && gp < CONFIG_SANDBOX_GPIO_COUNT); + return &state[gp]; +} + +static int get_gpio_flag(int gp, int flag) +{ + return *get_gpio(gp) & flag; +} + +static void set_gpio_flag(int gp, int flag, int value) +{ + u8 *gpio = get_gpio(gp); + + if (value) + *gpio |= flag; + else + *gpio &= ~flag; +} + +int sandbox_gpio_get_value(int gp) +{ + if (get_gpio_flag(gp, GPIOF_OUTPUT)) + printf("sandbox_gpio: get_value on output GPIO %d\n", gp); + return *get_gpio(gp) & GPIOF_HIGH; +} + +int sandbox_gpio_set_value(int gp, int value) +{ + set_gpio_flag(gp, GPIOF_HIGH, value); + return 0; +} + +int sandbox_gpio_get_direction(int gp) +{ + return get_gpio_flag(gp, GPIOF_OUTPUT); +} + +int sandbox_gpio_set_direction(int gp, int output) +{ + set_gpio_flag(gp, GPIOF_OUTPUT, output); + return 0; +} + + +/* These functions implement the public interface within U-Boot */ + +/* set GPIO port 'gp' as an input */ +int gpio_direction_input(int gp) +{ + debug("gpio_direction_input: gp = %d\n", gp); + set_gpio_flag(gp, GPIOF_OUTPUT, 0); + return 0; +} + +/* set GPIO port 'gp' as an output, with polarity 'value' */ +int gpio_direction_output(int gp, int value) +{ + debug("gpio_direction_output: gp = %d, value = %d\n", + gp, value); + set_gpio_flag(gp, GPIOF_OUTPUT, 1); + set_gpio_flag(gp, GPIOF_HIGH, value); + return 0; +} + +/* read GPIO IN value of port 'gp' */ +int gpio_get_value(int gp) +{ + debug("gpio_get_value: gp = %d\n", gp); + if (get_gpio_flag(gp, GPIOF_OUTPUT)) + printf("sandbox_gpio: get_value on output GPIO %d\n", gp); + return *get_gpio(gp) & GPIOF_HIGH; +} + +/* write GPIO OUT value to port 'gp' */ +int gpio_set_value(int gp, int value) +{ + debug("gpio_set_value: gp = %d, value = %d\n", gp, value); + if (get_gpio_flag(gp, GPIOF_OUTPUT)) { + set_gpio_flag(gp, GPIOF_HIGH, value); + } else { + printf("sandbox_gpio: set_value on input GPIO %d\n", gp); + return -1; + } + + return 0; +} + +int gpio_request(unsigned gpio, const char *label) +{ + /* for now, do nothing */ + return 0; +} + +int gpio_free(unsigned gpio) +{ + /* for now, do nothing */ + return 0; +} + +/* Display GPIO information */ +int gpio_info(int gp) +{ + printf("Sandbox GPIOs\n"); + return 0; +}
This provides a way of simulating GPIOs by setting values which are seen by the normal gpio_get/set_value() calls. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: - Use generic GPIO command and interface - Fix gpio_direction_output() to actually set the value arch/sandbox/include/asm/gpio.h | 61 ++++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/sandbox.c | 152 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 0 deletions(-) create mode 100644 arch/sandbox/include/asm/gpio.h create mode 100644 drivers/gpio/sandbox.c