[PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value

[Simon Glass]

Hi Tom.

On Thu, 30 Jan 2020 at 11:52, Tom Rini <trini at konsulko.com> wrote:
>
> On Wed, Jan 29, 2020 at 07:17:09PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 23 Jan 2020 at 14:12, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Thu, Jan 23, 2020 at 10:04:05PM +0100, Luka Kovačič wrote:
> > >
> > > > Hello Tom,
> > > >
> > > > thank you for feedback and review. I understand the implications.
> > > > Would it make sense to document this somewhere to avoid any future confusion?
> > >
> > > Yes, along with a standalone patch to update the document to use
> > > CMD_RET_SUCCESS NOT CMD_SUCCESS.  Updating the gpio help text even to be
> > > clear what the return value is would be nice.  Thanks!
> > >
> > > >
> > > > Thanks,
> > > > Luka
> > > >
> > > > On Thu, Jan 23, 2020 at 1:31 PM Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sun, Jan 05, 2020 at 08:10:56PM +0100, Luka Kovacic wrote:
> > > > >
> > > > > > Use the correct return value in function do_gpio() and update
> > > > > > commands documentation with the return values from command_ret_t enum.
> > > > > >
> > > > > > CMD_RET_SUCCESS is returned on command success and CMD_RET_FAILURE is
> > > > > > returned on command failure.
> > > > > >
> > > > > > The command was returning the pin value, which caused confusion when
> > > > > > debugging (#define DEBUG).
> > > > > >
> > > > > > Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > > > > Tested-by: Robert Marko <robert.marko at sartura.hr>
> > > > >
> > > > > So, I think the problem is that despite this not being an optimal user
> > > > > interface, it's what we've had here for "forever".  We can't just go
> > > > > change it now as there's scripts out in the world (and even
> > > > > include/configs/) that depend on the current behavior.  Sorry, nak.
> >
> > The command is effectively returning a negative value on failure,
> > which causes the calling shell to try to exit!
> >
> > Also 'gpio set' will return failure if you enable a GPIO. I really
> > can't see that people could be relying too much on the current
> > behaviour.
> >
> > GIven our policy on upstream, if we fix the in-tree scripts do you
> > think we could fix this problem?
> >
> > The 'return -1' is definitely a bug BTW.
>
> My first comment is to look at configs/socfpga_vining_fpga_defconfig and
> include/configs/omap3_beagle.h around 'if gpio' and tell me if I'm
> simply misunderstanding how things are being used.
>
> But if I'm not then I'm not sure just changing the users is OK because
> it's baked into saved environments.  Now I can say that for the Beagle
> case it might be OK in the end.  But I'm not so sure about the socfpga
> case.  Marek?

The omap3 code looks like it is checking if the GPIO is set or not.

Oddly 'if gpio input xx' is true if the GPIO is 0, so it might be
confusing. Arguably this should be inverted.

So how about we leave the behaviour for 'gpio input' alone, and 'fix'
the other bits?

Regards,
Simon