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