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

From: Simon Glass <sjg@chromium.org>
To: u-boot@lists.denx.de
Subject: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
Date: Sun, 2 Feb 2020 15:50:05 -0700	[thread overview]
Message-ID: <CAPnjgZ0Xgw6c=7__Hd9TxooWGusA8UuZ0ANx=eSx-cLwAyajWw@mail.gmail.com> (raw)
In-Reply-To: <20200131205916.GC13379@bill-the-cat>

Hi Tom,

On Fri, 31 Jan 2020 at 13:59, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jan 30, 2020 at 07:27:57PM -0700, Simon Glass wrote:
> > Hi Tom.
> >
> > On Thu, 30 Jan 2020 at 11:52, Tom Rini <trini@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@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@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@sartura.hr>
> > > > > > > > Tested-by: Robert Marko <robert.marko@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?
>
> What about the socfpga example?  Thanks!

This is also using 'gpio input' so we should be OK.

Regards,
SImon