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

[Mike Frysinger]

On Tuesday 21 February 2012 16:55:39 Simon Glass wrote:
> On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote:
> > 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".
> 
> I think the opposite though - it makes more sense to me that the test
> fails and reports failure, than continues with bogus results.

any test framework worth using will catch the error message that is displayed, 
so i don't think that's a big deal

> How about you leave the assert in as well - then I will be happy enough.

i'm OK with that

> Later we could change assert to always bail on sandbox, or make
> sandbox always build with DEBUG (although we would need to introduce a
> way of skipping the output).

we'd have to split the knobs so we could do assert() and not debug()

> >> >  /* 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.
> 
> What I find confusing is that you implement the external API using the
> test API - I mean confusing for people reading the code. It would be
> better (if you want functions to access all the state) if there were
> an internal access functions which the two sets use. I was trying to
> keep them as separate as possible.

i thought when we discussed this before that sandbox_gpio_xxx isn't a test 
api.  it *could* be used to seed the initial test state, or to fake out a 
simple device, but it's more than that.  if i was writing a proper simulation 
of a device connected over gpio lines, that device would need direct access to 
the pin state and thus would utilize sandbox_gpio_xxx.  i wouldn't label this 
simulated device as "test" code.

so once i have it in my mind that that we've got hardware state, and 
sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on 
top of that.  it's no different from having memory mapped registers that 
represent a block of gpio's -- in one case i'm doing readl(foo) and in the 
other i'm doing sandbox_gpio_xxx().

if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to 
a dedicated file in arch/sandbox/cpu/gpio.c.  then in order to access the 
"hardware", you'd have to call the sandbox_gpio_xxx funcs.

> Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO
> and doesn't stop.

well, it issues an error message for the developer to see, but there's no 
arbitrary memory access going on.

> - the test API should fault an invalid access and stop
> - the external API should assert() and continue.

"assert() and continue" doesn't make sense ... an assert(), by definition, will 
halt termination when things fail
-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/c0a0ce77/attachment.pgp>