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

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

[Luka Kovacic]

Hi,
The U-Boot gpio command always returns the value of the pin, which 
is confusing if you are debugging, since calling the command
gpio set pin always results in failure. 

The patch modifies the GPIO command to return CMD_RET_SUCCESS 
on success and CMD_RET_FAILURE on command failure and doesn't
use the pin value as a return value. 

Should this be changed, since users may rely on this in scripts?
What would be the best workaround?

Luka Kovacic (1):
  cmd: gpio: Correct do_gpio() return value

 cmd/gpio.c          | 22 +++++++++++++++++-----
 doc/README.commands |  4 ++--
 2 files changed, 19 insertions(+), 7 deletions(-)

-- 
2.20.1

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

[Luka Kovacic]

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>
---
 cmd/gpio.c          | 22 +++++++++++++++++-----
 doc/README.commands |  4 ++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/cmd/gpio.c b/cmd/gpio.c
index eff36ab2af..67eef83c95 100644
--- a/cmd/gpio.c
+++ b/cmd/gpio.c
@@ -223,23 +223,35 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		gpio_direction_output(gpio, value);
 	}
 	printf("gpio: pin %s (gpio %u) value is ", str_gpio, gpio);
-	if (IS_ERR_VALUE(value))
+
+	if (IS_ERR_VALUE(value)) {
 		printf("unknown (ret=%d)\n", value);
-	else
+		goto err;
+	} else {
 		printf("%d\n", value);
+	}
+
 	if (sub_cmd != GPIOC_INPUT && !IS_ERR_VALUE(value)) {
 		int nval = gpio_get_value(gpio);
 
-		if (IS_ERR_VALUE(nval))
+		if (IS_ERR_VALUE(nval)) {
 			printf("   Warning: no access to GPIO output value\n");
-		else if (nval != value)
+			goto err;
+		} else if (nval != value) {
 			printf("   Warning: value of pin is still %d\n", nval);
+			goto err;
+		}
 	}
 
 	if (ret != -EBUSY)
 		gpio_free(gpio);
 
-	return value;
+	return CMD_RET_SUCCESS;
+
+err:
+	if (ret != -EBUSY)
+		gpio_free(gpio);
+	return CMD_RET_FAILURE;
 }
 
 U_BOOT_CMD(gpio, 4, 0, do_gpio,
diff --git a/doc/README.commands b/doc/README.commands
index e03eb44187..4e9e8098fa 100644
--- a/doc/README.commands
+++ b/doc/README.commands
@@ -83,9 +83,9 @@ argv:		Arguments.
 
 Allowable return value are:
 
-CMD_SUCCESS	The command was successfully executed.
+CMD_RET_SUCCESS	The command was successfully executed.
 
-CMD_FAILURE	The command failed.
+CMD_RET_FAILURE	The command failed.
 
 CMD_RET_USAGE	The command was called with invalid parameters. This value
 		leads to the display of the usage string.
-- 
2.20.1

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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200123/30c34a37/attachment.sig>

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

[Luka Kovačič]

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?

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

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

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200123/85527a31/attachment.sig>

[PATCH 1/2 v1] cmd: doc: Update command return values

[luka.kovacic at sartura.hr]

From: Luka Kovacic <luka.kovacic@sartura.hr>

Update the command return values to CMD_RET_SUCCESS and
CMD_RET_FAILURE in commands documentation.

Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
---
 doc/README.commands | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/README.commands b/doc/README.commands
index e03eb44187..4e9e8098fa 100644
--- a/doc/README.commands
+++ b/doc/README.commands
@@ -83,9 +83,9 @@ argv:		Arguments.
 
 Allowable return value are:
 
-CMD_SUCCESS	The command was successfully executed.
+CMD_RET_SUCCESS	The command was successfully executed.
 
-CMD_FAILURE	The command failed.
+CMD_RET_FAILURE	The command failed.
 
 CMD_RET_USAGE	The command was called with invalid parameters. This value
 		leads to the display of the usage string.
-- 
2.20.1

[PATCH 2/2 v1] cmd: gpio: Add the command return value to the help text

[luka.kovacic at sartura.hr]

From: Luka Kovacic <luka.kovacic@sartura.hr>

Adds the command return value to the help text, since pin value
is returned.

Cc: Luka Perkov <luka.perkov@sartura.hr>
Signed-off-by: Luka Kovacic <luka.kovacic@sartura.hr>
---
 cmd/gpio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cmd/gpio.c b/cmd/gpio.c
index eff36ab2af..e57d3fb638 100644
--- a/cmd/gpio.c
+++ b/cmd/gpio.c
@@ -245,5 +245,6 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 U_BOOT_CMD(gpio, 4, 0, do_gpio,
 	   "query and control gpio pins",
 	   "<input|set|clear|toggle> <pin>\n"
-	   "    - input/set/clear/toggle the specified pin\n"
+	   "    - input/set/clear/toggle the specified pin; pin value is\n"
+	   "      returned.\n"
 	   "gpio status [-a] [<bank> | <pin>]  - show [all/claimed] GPIOs");
-- 
2.20.1

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

[Simon Glass]

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.

Regards,
Simon

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

[Tom Rini]

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?

> 
> Regards,
> Simon

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200130/8bfd6646/attachment.sig>

[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@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?

Regards,
Simon

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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200131/7c989d87/attachment.sig>

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

[Simon Glass]

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

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

[Tom Rini]

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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200207/4ac0fd55/attachment.sig>

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

[Alex Kiernan]

On Sat, Feb 8, 2020 at 12:06 AM 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>
>
> Applied to u-boot/master, thanks!
>

I just pulled in HEAD for a test build and our boot scripts are broken
with this gpio change - I don't see a way to get the value of a gpio
pin in a script now?

Whilst I agree what's there was wrong, I'm really not sure we can
change an existing interface like this.

-- 
Alex Kiernan

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

[Tom Rini]

On Tue, Mar 10, 2020 at 09:47:33AM +0000, Alex Kiernan wrote:
> On Sat, Feb 8, 2020 at 12:06 AM 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>
> >
> > Applied to u-boot/master, thanks!
> >
> 
> I just pulled in HEAD for a test build and our boot scripts are broken
> with this gpio change - I don't see a way to get the value of a gpio
> pin in a script now?
> 
> Whilst I agree what's there was wrong, I'm really not sure we can
> change an existing interface like this.

Sigh, this is what I was worried about.  If folks don't have a
suggestion on how to correct things again I'm going to revert this
change, sorry for the noise, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200310/77ea319a/attachment.sig>

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

[Alex Kiernan]

On Tue, Mar 10, 2020 at 12:37 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 10, 2020 at 09:47:33AM +0000, Alex Kiernan wrote:
> > On Sat, Feb 8, 2020 at 12:06 AM 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>
> > >
> > > Applied to u-boot/master, thanks!
> > >
> >
> > I just pulled in HEAD for a test build and our boot scripts are broken
> > with this gpio change - I don't see a way to get the value of a gpio
> > pin in a script now?
> >
> > Whilst I agree what's there was wrong, I'm really not sure we can
> > change an existing interface like this.
>
> Sigh, this is what I was worried about.  If folks don't have a
> suggestion on how to correct things again I'm going to revert this
> change, sorry for the noise, thanks!
>

There's a one-liner which fixes it for me (implementing the suggestion
of retaining the behaviour for gpio input):

https://patchwork.ozlabs.org/patch/1252077/

-- 
Alex Kiernan