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

[Mike Frysinger <vapier@gentoo.org>]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120221/109902bf/attachment.pgp>