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