[U-Boot] [PATCH v2 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

Hi Mike,

On Fri, Jan 20, 2012 at 10:59 AM, Mike Frysinger <vapier at 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