[U-Boot] [RFC] gpio command: return value on write, additional actions

[Andreas Pretzsch]

Am Dienstag, den 05.07.2011, 13:44 -0400 schrieb Mike Frysinger:
> On Tuesday, July 05, 2011 12:59:16 Andreas Pretzsch wrote:
> > As of today (2011.06), the generic gpio command (common/cmd_gpio.c)
> > 	gpio <input|set|clear|toggle> <port><pin>
> > 	    - input/set/clear/toggle the specified pin (e.g. PF10)
> > always returns the value read or set.
> > 
> > While this is sensible for read (input) and maybe (questionable) for
> > toggle, I don't see an usage for write (set/clear).
> 
> the trouble with toggle is that there's no way to get the value without 
> changing it to the input.  we'd have to add another field that has no 

True, at least on devices without hardware toggle support.
And you never know if you read back the real pin state, the output latch
or something invalid. And a "short" switch to input isn't always legal
wrt to the attached hardware. Great when setting 1 bit is worth 10 bit
of problems ;-)

Personally, I'd shift the gpio_toggle to the underlying hardware
specific code. And print an error if it's not supported.

> unintentional side effects like "gpio value".  then we could change all the 
> others to return 0/1 based on whether they succeeded, not based on the level 
> of the gpio pin.

Didn't quite get that. In terms of "gpio value" = "give me the current
(output latch) value without setting it to input if it's an output" ?

We can't change the return value of "gpio input", as it's out in the
wild and would break existing scripts.

I'd say clear/set/toggle are changeable, don't see any legit
return-value-usage here. For 100% backward compatibility, one could
leave them as they are and use 0|1 as new actions with return 0, as
proposed.

So these variants:
  gpio clear|0 => set to output, write 0, return success
  gpio set|1   => set to output, write 1, return success
  gpio toggle  => (set to output), toggle output, return success
  gpio input   => set to input, return pin value
  gpio value   => return current pin/latch/whatever value
or
  gpio clear   => set to output, write 0, return 0
  gpio set     => set to output, write 1, return 1
  gpio 0       => set to output, write 0, return success
  gpio 1       => set to output, write 1, return success
  gpio toggle  => (set to output), toggle output, return new_val
  gpio input   => set to input, return pin value
  gpio value   => return current pin/latch/whatever value

Not perfectly symmetric, but the best way out I can think of.
Maybe "get" instead of "value", as it's more usual. OTOH, get (to some
people) implies "set to input", so value is more explicit.


> > Also, this leads to unexpected side effects with complex constructs,
> > e.g. consider this environment:
> > 	setA=gpio set PF1
> > 	setB=gpio clear PF2
> > 	setAB_separate=env run setA ; env run setB
> > 	setAB_concatenated=env run setA setB
> > 	setBA_concatenated=env run setB setA
> > 
> > While executing "setAB_separate" and "setBA_concatenated" work as
> > expected, "setAB_concatenated" will only run setA, but not setB, as setA
> > "failed" (ret=1). [1]
> > The same would apply to e.g. && constructs.
> 
> ive never used the shell in u-boot, but couldnt the first logic be:
> 	setA=gpio set PF1 || :
> 	setB=gpio clear PF2 || :

Not fully, you'd need "gpio set PF1 || true". Not that this makes it
less ugly...
BTW, nobody would mind that without notice. Stumbled over it by pure
accident myself.

> > As it works this way in the release and hence the behaviour is
> > essentially fixed, changing the return value is probably not an option.
> 
> not really.  poor behavior can be adjusted.
> -mike

Fine from my side, but I'm not the one to decide.


-- 

carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch          Tel. +49-(0)731-5521572
Hahnengasse 3                             Fax: +49-(0)731-5521573
89073 Ulm, Germany                        email: apr at cn-eng.de