From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Date: Tue, 21 Feb 2012 17:13:52 -0500 Subject: [U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs In-Reply-To: References: <1329349878-16664-1-git-send-email-sjg@chromium.org> <201202211104.13950.vapier@gentoo.org> Message-ID: <201202211713.53108.vapier@gentoo.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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: