Hi Mike,
On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger <vap...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
