| Message ID | 201202210111.48891.vapier@gentoo.org |
|---|---|
| State | Accepted |
| Delegated to: | Mike Frysinger |
| Headers | show |
Hi Mike, On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger <vapier@gentoo.org> wrote: > here's the incremental/full diff against your v4 showing the direction i think > things should end up at ... incremental inline below while full is attached. > -mike Hmmm I'm fine with the get_gpio_flags() rename, printf() changes, but please see below. > > diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c > index 17e601d..aa2aa4c 100644 > --- a/drivers/gpio/sandbox.c > +++ b/drivers/gpio/sandbox.c > @@ -20,9 +20,6 @@ > */ > > #include <common.h> > -#include <asm/io.h> > -#include <asm/bitops.h> > -#include <asm-generic/gpio.h> > #include <asm/gpio.h> > > /* Flags for each GPIO */ > @@ -42,38 +39,59 @@ struct gpio_state { > static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT]; > > /* Access routines for GPIO state */ > -static u8 *get_gpio(unsigned gp) > +static u8 *get_gpio_flags(unsigned gp) > { > - assert(gp < CONFIG_SANDBOX_GPIO_COUNT); > + if (gp >= ARRAY_SIZE(state)) { > + static u8 invalid_flags; > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); > + return &invalid_flags; > + } > + I think we want to die / fail the test here, but since we don't have any tests I suppose this is ok for now. I like assert() because it halts. > return &state[gp].flags; > } > > static int get_gpio_flag(unsigned gp, int flag) > { > - return (*get_gpio(gp) & flag) != 0; > + return (*get_gpio_flags(gp) & flag) != 0; > } > > -static void set_gpio_flag(unsigned gp, int flag, int value) > +static int set_gpio_flag(unsigned gp, int flag, int value) > { > - u8 *gpio = get_gpio(gp); > + u8 *gpio = get_gpio_flags(gp); > > if (value) > *gpio |= flag; > else > *gpio &= ~flag; > + > + return 0; > } > > +static int check_reserved(unsigned gpio, const char *func) > +{ > + if (!get_gpio_flag(gpio, GPIOF_RESERVED)) { > + printf("sandbox_gpio: %s: error: gpio %u not reserved\n", > + func, gpio); > + return -1; > + } > + > + return 0; > +} > + > +/* > + * Back-channel sandbox-internal-only access to GPIO state > + */ > + > int sandbox_gpio_get_value(unsigned 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; > + debug("sandbox_gpio: get_value on output gpio %u\n", gp); > + return get_gpio_flag(gp, GPIOF_HIGH); > } > > int sandbox_gpio_set_value(unsigned gp, int value) > { > - set_gpio_flag(gp, GPIOF_HIGH, value); > - return 0; > + return set_gpio_flag(gp, GPIOF_HIGH, value); > } > > int sandbox_gpio_get_direction(unsigned gp) > @@ -83,95 +101,90 @@ int sandbox_gpio_get_direction(unsigned gp) > > int sandbox_gpio_set_direction(unsigned gp, int output) > { > - set_gpio_flag(gp, GPIOF_OUTPUT, output); > - return 0; > + return set_gpio_flag(gp, GPIOF_OUTPUT, output); > } > > -static int check_reserved(unsigned gpio, const char *op_name) > -{ > - if (!get_gpio_flag(gpio, GPIOF_RESERVED)) { > - printf("sandbox_gpio: '%s' on unreserved GPIO\n", op_name); > - return -1; > - } > - > - return 0; > -} > - > -/* These functions implement the public interface within U-Boot */ > +/* > + * These functions implement the public interface within U-Boot > + */ > > /* set GPIO port 'gp' as an input */ > int gpio_direction_input(unsigned gp) > { > - debug("%s: gp = %d\n", __func__, gp); > + debug("%s: gp:%u\n", __func__, gp); > + > if (check_reserved(gp, __func__)) > return -1; > - set_gpio_flag(gp, GPIOF_OUTPUT, 0); > > - return 0; > + return sandbox_gpio_set_direction(gp, 0); Ick, we shouldn't call that function here - it is in the test code. Same below. The idea is that this state has two completely separate sides to it - by calling the 'test' functions from the 'U-Boot' functions I think you are going to confuse people a lot. > } > > /* set GPIO port 'gp' as an output, with polarity 'value' */ > int gpio_direction_output(unsigned gp, int value) > { > - debug("%s: gp = %d, value = %d\n", __func__, gp, value); > + debug("%s: gp:%u, value = %d\n", __func__, gp, value); > + > if (check_reserved(gp, __func__)) > return -1; > - set_gpio_flag(gp, GPIOF_OUTPUT, 1); > - set_gpio_flag(gp, GPIOF_HIGH, value); > > - return 0; > + return sandbox_gpio_set_direction(gp, 1) | > + sandbox_gpio_set_value(gp, value); > } > > /* read GPIO IN value of port 'gp' */ > int gpio_get_value(unsigned gp) > { > - debug("%s: gp = %d\n", __func__, gp); > + debug("%s: gp:%u\n", __func__, gp); > + > if (check_reserved(gp, __func__)) > return -1; > - if (get_gpio_flag(gp, GPIOF_OUTPUT)) > - printf("sandbox_gpio: get_value on output GPIO %d\n", gp); > > - return get_gpio_flag(gp, GPIOF_HIGH); > + return sandbox_gpio_get_value(gp); > } > > /* write GPIO OUT value to port 'gp' */ > int gpio_set_value(unsigned gp, int value) > { > - debug("%s: gp = %d, value = %d\n", __func__, gp, value); > + debug("%s: gp:%u, value = %d\n", __func__, gp, value); > + > if (check_reserved(gp, __func__)) > return -1; > - 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); > + > + if (!sandbox_gpio_get_direction(gp)) { > + printf("sandbox_gpio: error: set_value on input gpio %u\n", gp); > return -1; > } > > - return 0; > + return sandbox_gpio_set_value(gp, value); > } > > int gpio_request(unsigned gp, const char *label) > { > - debug("%s: gp = %d, label= %s\n", __func__, gp, label); > + debug("%s: gp:%u, label:%s\n", __func__, gp, label); > + > + if (gp >= ARRAY_SIZE(state)) { > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); > + return -1; > + } > + > if (get_gpio_flag(gp, GPIOF_RESERVED)) { > - printf("sandbox_gpio: request on reserved GPIO\n"); > + printf("sandbox_gpio: error: gpio %u already reserved\n", gp); > return -1; > } > - set_gpio_flag(gp, GPIOF_RESERVED, 1); > - state[gp].label = label; > > - return 0; > + state[gp].label = label; > + return set_gpio_flag(gp, GPIOF_RESERVED, 1); > } > > int gpio_free(unsigned gp) > { > - debug("%s: gp = %d\n", __func__, gp); > + debug("%s: gp:%u\n", __func__, gp); > + > if (check_reserved(gp, __func__)) > return -1; > - set_gpio_flag(gp, GPIOF_RESERVED, 0); > - state[gp].label = NULL; > > - return 0; > + state[gp].label = NULL; > + return set_gpio_flag(gp, GPIOF_RESERVED, 0); > } > > /* Display GPIO information */ > @@ -179,15 +192,15 @@ void gpio_info(void) > { > unsigned gpio; > > - printf("Sandbox GPIOs\n"); > + puts("Sandbox GPIOs\n"); > > - for (gpio = 0; gpio < CONFIG_SANDBOX_GPIO_COUNT; ++gpio) { > + for (gpio = 0; gpio < ARRAY_SIZE(state); ++gpio) { > const char *label = state[gpio].label; > > printf("%4d: %s: %d [%c] %s\n", > gpio, > - get_gpio_flag(gpio, GPIOF_OUTPUT) ? "out" : " in", > - get_gpio_flag(gpio, GPIOF_HIGH), > + sandbox_gpio_get_direction(gpio) ? "out" : " in", > + sandbox_gpio_get_value(gpio), > get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ', > label ? label : ""); > } Regards, Simon
On Tuesday 21 February 2012 01:27:31 Simon Glass wrote: > On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote: > > --- a/drivers/gpio/sandbox.c > > +++ b/drivers/gpio/sandbox.c > > > > /* Access routines for GPIO state */ > > -static u8 *get_gpio(unsigned gp) > > +static u8 *get_gpio_flags(unsigned gp) > > { > > - assert(gp < CONFIG_SANDBOX_GPIO_COUNT); > > + if (gp >= ARRAY_SIZE(state)) { > > + static u8 invalid_flags; > > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); > > + return &invalid_flags; > > + } > > + > > I think we want to die / fail the test here, but since we don't have > any tests I suppose this is ok for now. I like assert() because it > halts. the problem is that assert() is disabled by default, so by default, we get memory corruption :). i tend to agree with your previous statements (in another thread) that the sandbox should, by default, do arg checking since the sandbox env is expected to be tested/developed under. extending that logic, i think it makes more sense to get output that includes errors but "works" so people can play around more on the command line without interrupting things. after all, i'd rather see an error message if i was in the middle of typing "gpio ..." on the command line but fat fingered the gpio number and typed "gpio set 199" instead of "gpio set 19". > > /* set GPIO port 'gp' as an input */ > > int gpio_direction_input(unsigned gp) > > { > > - debug("%s: gp = %d\n", __func__, gp); > > + debug("%s: gp:%u\n", __func__, gp); > > + > > if (check_reserved(gp, __func__)) > > return -1; > > - set_gpio_flag(gp, GPIOF_OUTPUT, 0); > > > > - return 0; > > + return sandbox_gpio_set_direction(gp, 0); > > Ick, we shouldn't call that function here - it is in the test code. Same > below. > > The idea is that this state has two completely separate sides to it - > by calling the 'test' functions from the 'U-Boot' functions I think > you are going to confuse people a lot. the way i see it is we have the pin state ("state"), we have direct accessor functions with no error checking so other things can directly manipulate that state (sandbox_gpio_xxx), and we have the generic gpio api (gpio_xxx). i don't think both API's should get to directly manipulate the state ... it's more logical (to me) that the generic gpio api be built off the hardware state api rather than grubbin' around directly. the only place that gets confusing is when we have one structure that ends up storing the hardware state (pin direction/levels) along side the generic gpio state (pin reservation and friendly label names). although, thinking a little more, we should be able to split that out easily enough -- have an array of labels and if a gpio's label is NULL, we know the pin is not reserved. -mike
Hi Mike, On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger <vapier@gentoo.org> wrote: > On Tuesday 21 February 2012 01:27:31 Simon Glass wrote: >> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote: >> > --- a/drivers/gpio/sandbox.c >> > +++ b/drivers/gpio/sandbox.c >> > >> > /* Access routines for GPIO state */ >> > -static u8 *get_gpio(unsigned gp) >> > +static u8 *get_gpio_flags(unsigned gp) >> > { >> > - assert(gp < CONFIG_SANDBOX_GPIO_COUNT); >> > + if (gp >= ARRAY_SIZE(state)) { >> > + static u8 invalid_flags; >> > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); >> > + return &invalid_flags; >> > + } >> > + >> >> I think we want to die / fail the test here, but since we don't have >> any tests I suppose this is ok for now. I like assert() because it >> halts. > > the problem is that assert() is disabled by default, so by default, we get > memory corruption :). i tend to agree with your previous statements (in > another thread) that the sandbox should, by default, do arg checking since the > sandbox env is expected to be tested/developed under. > > extending that logic, i think it makes more sense to get output that includes > errors but "works" so people can play around more on the command line without > interrupting things. after all, i'd rather see an error message if i was in > the middle of typing "gpio ..." on the command line but fat fingered the gpio > number and typed "gpio set 199" instead of "gpio set 19". I think the opposite though - it makes more sense to me that the test fails and reports failure, than continues with bogus results. How about you leave the assert in as well - then I will be happy enough. Later we could change assert to always bail on sandbox, or make sandbox always build with DEBUG (although we would need to introduce a way of skipping the output). > >> > /* set GPIO port 'gp' as an input */ >> > int gpio_direction_input(unsigned gp) >> > { >> > - debug("%s: gp = %d\n", __func__, gp); >> > + debug("%s: gp:%u\n", __func__, gp); >> > + >> > if (check_reserved(gp, __func__)) >> > return -1; >> > - set_gpio_flag(gp, GPIOF_OUTPUT, 0); >> > >> > - return 0; >> > + return sandbox_gpio_set_direction(gp, 0); >> >> Ick, we shouldn't call that function here - it is in the test code. Same >> below. >> >> The idea is that this state has two completely separate sides to it - >> by calling the 'test' functions from the 'U-Boot' functions I think >> you are going to confuse people a lot. > > the way i see it is we have the pin state ("state"), we have direct accessor > functions with no error checking so other things can directly manipulate that > state (sandbox_gpio_xxx), and we have the generic gpio api (gpio_xxx). i > don't think both API's should get to directly manipulate the state ... it's > more logical (to me) that the generic gpio api be built off the hardware state > api rather than grubbin' around directly. > > the only place that gets confusing is when we have one structure that ends up > storing the hardware state (pin direction/levels) along side the generic gpio > state (pin reservation and friendly label names). although, thinking a little > more, we should be able to split that out easily enough -- have an array of > labels and if a gpio's label is NULL, we know the pin is not reserved. What I find confusing is that you implement the external API using the test API - I mean confusing for people reading the code. It would be better (if you want functions to access all the state) if there were an internal access functions which the two sets use. I was trying to keep them as separate as possible. Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO and doesn't stop. IMO - the test API should fault an invalid access and stop - the external API should assert() and continue. I agree that assert() is currently skipped and will not cause a test to fail, but of course we can address that if you like. Anyway, this GPIO implementation is better than what we currently have...so if you really think this is the right thing to do, then let's go with it and address it later when we have some tests. > -mike Regards, Simon
On Tuesday 21 February 2012 16:55:39 Simon Glass wrote: > On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote: > > On Tuesday 21 February 2012 01:27:31 Simon Glass wrote: > >> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote: > >> > --- a/drivers/gpio/sandbox.c > >> > +++ b/drivers/gpio/sandbox.c > >> > > >> > /* Access routines for GPIO state */ > >> > -static u8 *get_gpio(unsigned gp) > >> > +static u8 *get_gpio_flags(unsigned gp) > >> > { > >> > - assert(gp < CONFIG_SANDBOX_GPIO_COUNT); > >> > + if (gp >= ARRAY_SIZE(state)) { > >> > + static u8 invalid_flags; > >> > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); > >> > + return &invalid_flags; > >> > + } > >> > + > >> > >> I think we want to die / fail the test here, but since we don't have > >> any tests I suppose this is ok for now. I like assert() because it > >> halts. > > > > the problem is that assert() is disabled by default, so by default, we > > get memory corruption :). i tend to agree with your previous statements > > (in another thread) that the sandbox should, by default, do arg checking > > since the sandbox env is expected to be tested/developed under. > > > > extending that logic, i think it makes more sense to get output that > > includes errors but "works" so people can play around more on the > > command line without interrupting things. after all, i'd rather see an > > error message if i was in the middle of typing "gpio ..." on the command > > line but fat fingered the gpio number and typed "gpio set 199" instead > > of "gpio set 19". > > I think the opposite though - it makes more sense to me that the test > fails and reports failure, than continues with bogus results. any test framework worth using will catch the error message that is displayed, so i don't think that's a big deal > How about you leave the assert in as well - then I will be happy enough. i'm OK with that > Later we could change assert to always bail on sandbox, or make > sandbox always build with DEBUG (although we would need to introduce a > way of skipping the output). we'd have to split the knobs so we could do assert() and not debug() > >> > /* set GPIO port 'gp' as an input */ > >> > int gpio_direction_input(unsigned gp) > >> > { > >> > - debug("%s: gp = %d\n", __func__, gp); > >> > + debug("%s: gp:%u\n", __func__, gp); > >> > + > >> > if (check_reserved(gp, __func__)) > >> > return -1; > >> > - set_gpio_flag(gp, GPIOF_OUTPUT, 0); > >> > > >> > - return 0; > >> > + return sandbox_gpio_set_direction(gp, 0); > >> > >> Ick, we shouldn't call that function here - it is in the test code. Same > >> below. > >> > >> The idea is that this state has two completely separate sides to it - > >> by calling the 'test' functions from the 'U-Boot' functions I think > >> you are going to confuse people a lot. > > > > the way i see it is we have the pin state ("state"), we have direct > > accessor functions with no error checking so other things can directly > > manipulate that state (sandbox_gpio_xxx), and we have the generic gpio > > api (gpio_xxx). i don't think both API's should get to directly > > manipulate the state ... it's more logical (to me) that the generic gpio > > api be built off the hardware state api rather than grubbin' around > > directly. > > > > the only place that gets confusing is when we have one structure that > > ends up storing the hardware state (pin direction/levels) along side the > > generic gpio state (pin reservation and friendly label names). > > although, thinking a little more, we should be able to split that out > > easily enough -- have an array of labels and if a gpio's label is NULL, > > we know the pin is not reserved. > > What I find confusing is that you implement the external API using the > test API - I mean confusing for people reading the code. It would be > better (if you want functions to access all the state) if there were > an internal access functions which the two sets use. I was trying to > keep them as separate as possible. i thought when we discussed this before that sandbox_gpio_xxx isn't a test api. it *could* be used to seed the initial test state, or to fake out a simple device, but it's more than that. if i was writing a proper simulation of a device connected over gpio lines, that device would need direct access to the pin state and thus would utilize sandbox_gpio_xxx. i wouldn't label this simulated device as "test" code. so once i have it in my mind that that we've got hardware state, and sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on top of that. it's no different from having memory mapped registers that represent a block of gpio's -- in one case i'm doing readl(foo) and in the other i'm doing sandbox_gpio_xxx(). if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to a dedicated file in arch/sandbox/cpu/gpio.c. then in order to access the "hardware", you'd have to call the sandbox_gpio_xxx funcs. > Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO > and doesn't stop. well, it issues an error message for the developer to see, but there's no arbitrary memory access going on. > - the test API should fault an invalid access and stop > - the external API should assert() and continue. "assert() and continue" doesn't make sense ... an assert(), by definition, will halt termination when things fail -mike
Hi Mike, On Tue, Feb 21, 2012 at 2:13 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Tuesday 21 February 2012 16:55:39 Simon Glass wrote: >> On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote: >> > On Tuesday 21 February 2012 01:27:31 Simon Glass wrote: >> >> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote: >> >> > --- a/drivers/gpio/sandbox.c >> >> > +++ b/drivers/gpio/sandbox.c >> >> > >> >> > /* Access routines for GPIO state */ >> >> > -static u8 *get_gpio(unsigned gp) >> >> > +static u8 *get_gpio_flags(unsigned gp) >> >> > { >> >> > - assert(gp < CONFIG_SANDBOX_GPIO_COUNT); >> >> > + if (gp >= ARRAY_SIZE(state)) { >> >> > + static u8 invalid_flags; >> >> > + printf("sandbox_gpio: error: invalid gpio %u\n", gp); >> >> > + return &invalid_flags; >> >> > + } >> >> > + >> >> >> >> I think we want to die / fail the test here, but since we don't have >> >> any tests I suppose this is ok for now. I like assert() because it >> >> halts. >> > >> > the problem is that assert() is disabled by default, so by default, we >> > get memory corruption :). i tend to agree with your previous statements >> > (in another thread) that the sandbox should, by default, do arg checking >> > since the sandbox env is expected to be tested/developed under. >> > >> > extending that logic, i think it makes more sense to get output that >> > includes errors but "works" so people can play around more on the >> > command line without interrupting things. after all, i'd rather see an >> > error message if i was in the middle of typing "gpio ..." on the command >> > line but fat fingered the gpio number and typed "gpio set 199" instead >> > of "gpio set 19". >> >> I think the opposite though - it makes more sense to me that the test >> fails and reports failure, than continues with bogus results. > > any test framework worth using will catch the error message that is displayed, > so i don't think that's a big deal > >> How about you leave the assert in as well - then I will be happy enough. > > i'm OK with that OK > >> Later we could change assert to always bail on sandbox, or make >> sandbox always build with DEBUG (although we would need to introduce a >> way of skipping the output). > > we'd have to split the knobs so we could do assert() and not debug() Yes. Later, maybe. > >> >> > /* set GPIO port 'gp' as an input */ >> >> > int gpio_direction_input(unsigned gp) >> >> > { >> >> > - debug("%s: gp = %d\n", __func__, gp); >> >> > + debug("%s: gp:%u\n", __func__, gp); >> >> > + >> >> > if (check_reserved(gp, __func__)) >> >> > return -1; >> >> > - set_gpio_flag(gp, GPIOF_OUTPUT, 0); >> >> > >> >> > - return 0; >> >> > + return sandbox_gpio_set_direction(gp, 0); >> >> >> >> Ick, we shouldn't call that function here - it is in the test code. Same >> >> below. >> >> >> >> The idea is that this state has two completely separate sides to it - >> >> by calling the 'test' functions from the 'U-Boot' functions I think >> >> you are going to confuse people a lot. >> > >> > the way i see it is we have the pin state ("state"), we have direct >> > accessor functions with no error checking so other things can directly >> > manipulate that state (sandbox_gpio_xxx), and we have the generic gpio >> > api (gpio_xxx). i don't think both API's should get to directly >> > manipulate the state ... it's more logical (to me) that the generic gpio >> > api be built off the hardware state api rather than grubbin' around >> > directly. >> > >> > the only place that gets confusing is when we have one structure that >> > ends up storing the hardware state (pin direction/levels) along side the >> > generic gpio state (pin reservation and friendly label names). >> > although, thinking a little more, we should be able to split that out >> > easily enough -- have an array of labels and if a gpio's label is NULL, >> > we know the pin is not reserved. >> >> What I find confusing is that you implement the external API using the >> test API - I mean confusing for people reading the code. It would be >> better (if you want functions to access all the state) if there were >> an internal access functions which the two sets use. I was trying to >> keep them as separate as possible. > > i thought when we discussed this before that sandbox_gpio_xxx isn't a test > api. it *could* be used to seed the initial test state, or to fake out a > simple device, but it's more than that. if i was writing a proper simulation > of a device connected over gpio lines, that device would need direct access to > the pin state and thus would utilize sandbox_gpio_xxx. i wouldn't label this > simulated device as "test" code. > > so once i have it in my mind that that we've got hardware state, and > sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on > top of that. it's no different from having memory mapped registers that > represent a block of gpio's -- in one case i'm doing readl(foo) and in the > other i'm doing sandbox_gpio_xxx(). > > if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to > a dedicated file in arch/sandbox/cpu/gpio.c. then in order to access the > "hardware", you'd have to call the sandbox_gpio_xxx funcs. Well yes GPIO state could go into state.h one day and be accessed from there as I think we previously discussed when I was motivating state.h. Your change makes more sense in that case than it does now. I didn't do that because we don't yet have a way to load/save state so the need for centralising it isn't there (yet). Let's go with your approach until we get to that point. > >> Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO >> and doesn't stop. > > well, it issues an error message for the developer to see, but there's no > arbitrary memory access going on. If you add the assert() then I'm happy with this. > >> - the test API should fault an invalid access and stop >> - the external API should assert() and continue. > > "assert() and continue" doesn't make sense ... an assert(), by definition, will > halt termination when things fail You mention above that assert() is a nop when DEBUG is not defined - that's what I meant by that comment. In other words the dev chooses whether to abort or not, but it is definitely flagged as an error. > -mike Regards, Simon
From 5252fab2c89ba05035d022c9a5a50253dc053cd7 Mon Sep 17 00:00:00 2001 From: Simon Glass <sjg@chromium.org> Date: Wed, 15 Feb 2012 15:51:13 -0800 Subject: [PATCH] sandbox: gpio: Add basic driver for simulating GPIOs 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> Signed-off-by: Mike Frysinger <vapier@gentoo.org> --- arch/sandbox/include/asm/gpio.h | 81 +++++++++++++++ drivers/gpio/Makefile | 1 + drivers/gpio/sandbox.c | 207 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+), 0 deletions(-) create mode 100644 arch/sandbox/include/asm/gpio.h create mode 100644 drivers/gpio/sandbox.c diff --git a/arch/sandbox/include/asm/gpio.h b/arch/sandbox/include/asm/gpio.h new file mode 100644 index 0000000..0500c53 --- /dev/null +++ b/arch/sandbox/include/asm/gpio.h @@ -0,0 +1,81 @@ +/* + * 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. + * + * The back-channel functions are declared in this file. They should not be used + * except in test code. + * + * Test code can, for example, call sandbox_gpio_set_value() to set the value of + * a simulated GPIO. From then on, normal code in U-Boot will see this new + * value when it calls gpio_get_value(). + * + * NOTE: DO NOT use the functions in this file except in test code! + */ +#include <asm-generic/gpio.h> + +/** + * Return the simulated value of a GPIO (used only in sandbox test code) + * + * @param gp GPIO number + * @return -1 on error, 0 if GPIO is low, >0 if high + */ +int sandbox_gpio_get_value(unsigned gp); + +/** + * Set the simulated value of a GPIO (used only in sandbox test code) + * + * @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(unsigned gp, int value); + +/** + * Return the simulated direction of a GPIO (used only in sandbox test code) + * + * @param gp GPIO number + * @return -1 on error, 0 if GPIO is input, >0 if output + */ +int sandbox_gpio_get_direction(unsigned gp); + +/** + * Set the simulated direction of a GPIO (used only in sandbox test code) + * + * @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(unsigned gp, int output); + +/* Display information about each GPIO */ +void gpio_info(void); + +#define gpio_status() gpio_info() + +#endif diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 4375a55..fb3b09a 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..aa2aa4c --- /dev/null +++ b/drivers/gpio/sandbox.c @@ -0,0 +1,207 @@ +/* + * 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/gpio.h> + +/* Flags for each GPIO */ +#define GPIOF_OUTPUT (1 << 0) /* Currently set as an output */ +#define GPIOF_HIGH (1 << 1) /* Currently set high */ +#define GPIOF_RESERVED (1 << 2) /* Is in use / requested */ + +struct gpio_state { + const char *label; /* label given by requester */ + u8 flags; /* flags (GPIOF_...) */ +}; + +/* + * State of GPIOs + * TODO: Put this into sandbox state + */ +static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT]; + +/* Access routines for GPIO state */ +static u8 *get_gpio_flags(unsigned gp) +{ + if (gp >= ARRAY_SIZE(state)) { + static u8 invalid_flags; + printf("sandbox_gpio: error: invalid gpio %u\n", gp); + return &invalid_flags; + } + + return &state[gp].flags; +} + +static int get_gpio_flag(unsigned gp, int flag) +{ + return (*get_gpio_flags(gp) & flag) != 0; +} + +static int set_gpio_flag(unsigned gp, int flag, int value) +{ + u8 *gpio = get_gpio_flags(gp); + + if (value) + *gpio |= flag; + else + *gpio &= ~flag; + + return 0; +} + +static int check_reserved(unsigned gpio, const char *func) +{ + if (!get_gpio_flag(gpio, GPIOF_RESERVED)) { + printf("sandbox_gpio: %s: error: gpio %u not reserved\n", + func, gpio); + return -1; + } + + return 0; +} + +/* + * Back-channel sandbox-internal-only access to GPIO state + */ + +int sandbox_gpio_get_value(unsigned gp) +{ + if (get_gpio_flag(gp, GPIOF_OUTPUT)) + debug("sandbox_gpio: get_value on output gpio %u\n", gp); + return get_gpio_flag(gp, GPIOF_HIGH); +} + +int sandbox_gpio_set_value(unsigned gp, int value) +{ + return set_gpio_flag(gp, GPIOF_HIGH, value); +} + +int sandbox_gpio_get_direction(unsigned gp) +{ + return get_gpio_flag(gp, GPIOF_OUTPUT); +} + +int sandbox_gpio_set_direction(unsigned gp, int output) +{ + return set_gpio_flag(gp, GPIOF_OUTPUT, output); +} + +/* + * These functions implement the public interface within U-Boot + */ + +/* set GPIO port 'gp' as an input */ +int gpio_direction_input(unsigned gp) +{ + debug("%s: gp:%u\n", __func__, gp); + + if (check_reserved(gp, __func__)) + return -1; + + return sandbox_gpio_set_direction(gp, 0); +} + +/* set GPIO port 'gp' as an output, with polarity 'value' */ +int gpio_direction_output(unsigned gp, int value) +{ + debug("%s: gp:%u, value = %d\n", __func__, gp, value); + + if (check_reserved(gp, __func__)) + return -1; + + return sandbox_gpio_set_direction(gp, 1) | + sandbox_gpio_set_value(gp, value); +} + +/* read GPIO IN value of port 'gp' */ +int gpio_get_value(unsigned gp) +{ + debug("%s: gp:%u\n", __func__, gp); + + if (check_reserved(gp, __func__)) + return -1; + + return sandbox_gpio_get_value(gp); +} + +/* write GPIO OUT value to port 'gp' */ +int gpio_set_value(unsigned gp, int value) +{ + debug("%s: gp:%u, value = %d\n", __func__, gp, value); + + if (check_reserved(gp, __func__)) + return -1; + + if (!sandbox_gpio_get_direction(gp)) { + printf("sandbox_gpio: error: set_value on input gpio %u\n", gp); + return -1; + } + + return sandbox_gpio_set_value(gp, value); +} + +int gpio_request(unsigned gp, const char *label) +{ + debug("%s: gp:%u, label:%s\n", __func__, gp, label); + + if (gp >= ARRAY_SIZE(state)) { + printf("sandbox_gpio: error: invalid gpio %u\n", gp); + return -1; + } + + if (get_gpio_flag(gp, GPIOF_RESERVED)) { + printf("sandbox_gpio: error: gpio %u already reserved\n", gp); + return -1; + } + + state[gp].label = label; + return set_gpio_flag(gp, GPIOF_RESERVED, 1); +} + +int gpio_free(unsigned gp) +{ + debug("%s: gp:%u\n", __func__, gp); + + if (check_reserved(gp, __func__)) + return -1; + + state[gp].label = NULL; + return set_gpio_flag(gp, GPIOF_RESERVED, 0); +} + +/* Display GPIO information */ +void gpio_info(void) +{ + unsigned gpio; + + puts("Sandbox GPIOs\n"); + + for (gpio = 0; gpio < ARRAY_SIZE(state); ++gpio) { + const char *label = state[gpio].label; + + printf("%4d: %s: %d [%c] %s\n", + gpio, + sandbox_gpio_get_direction(gpio) ? "out" : " in", + sandbox_gpio_get_value(gpio), + get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ', + label ? label : ""); + } +} -- 1.7.8.4