Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
On Tue, Mar 10, 2020 at 12:37 PM Tom Rini wrote: > > On Tue, Mar 10, 2020 at 09:47:33AM +, Alex Kiernan wrote: > > On Sat, Feb 8, 2020 at 12:06 AM Tom Rini 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 > > > > Tested-by: Robert Marko > > > > > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
On Tue, Mar 10, 2020 at 09:47:33AM +, Alex Kiernan wrote: > On Sat, Feb 8, 2020 at 12:06 AM Tom Rini 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 > > > Tested-by: Robert Marko > > > > 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 signature.asc Description: PGP signature
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
On Sat, Feb 8, 2020 at 12:06 AM Tom Rini 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 > > Tested-by: Robert Marko > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 > Tested-by: Robert Marko Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
Hi Tom, On Fri, 31 Jan 2020 at 13:59, Tom Rini 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 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 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 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 > > > > > > > > Tested-by: Robert Marko > > > > > > > > > > > > > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 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 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 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 > > > > > > > Tested-by: Robert Marko > > > > > > > > > > > > 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 signature.asc Description: PGP signature
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
Hi Tom. On Thu, 30 Jan 2020 at 11:52, Tom Rini 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 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 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 > > > > > > Tested-by: Robert Marko > > > > > > > > > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 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 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 > > > > > Tested-by: Robert Marko > > > > > > > > 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 signature.asc Description: PGP signature
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
Hi Tom, On Thu, 23 Jan 2020 at 14:12, Tom Rini 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 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 > > > > Tested-by: Robert Marko > > > > > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 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 > > > Tested-by: Robert Marko > > > > 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 signature.asc Description: PGP signature
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 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 > > Tested-by: Robert Marko > > 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
Re: [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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 > Tested-by: Robert Marko 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 signature.asc Description: PGP signature
[PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value
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
Tested-by: Robert Marko
---
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_SUCCESSThe command was successfully executed.
+CMD_RET_SUCCESSThe command was successfully executed.
-CMD_FAILUREThe command failed.
+CMD_RET_FAILUREThe 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
