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

[Simon Glass]

Hi Mike,

On Tue, Feb 21, 2012 at 2:13 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> 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

OK

>
>> 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()

Yes. Later, maybe.

>
>> >> >  /* 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.

Well yes GPIO state could go into state.h one day and be accessed from
there as I think we previously discussed when I was motivating
state.h. Your change makes more sense in that case than it does now. I
didn't do that because we don't yet have a way to load/save state so
the need for centralising it isn't there (yet).

Let's go with your approach until we get to that point.
>
>> 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.

If you add the assert() then I'm happy with this.

>
>> - 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

You mention above that assert() is a nop when DEBUG is not defined -
that's what I meant by that comment. In other words the dev chooses
whether to abort or not, but it is definitely flagged as an error.

> -mike

Regards,
Simon