* [PATCH 0/1 v1] cmd: gpio: Correct do_gpio() return value @ 2020-01-05 19:10 Luka Kovacic 2020-01-05 19:10 ` [PATCH 1/1 " Luka Kovacic 0 siblings, 1 reply; 16+ messages in thread From: Luka Kovacic @ 2020-01-05 19:10 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-05 19:10 [PATCH 0/1 v1] cmd: gpio: Correct do_gpio() return value Luka Kovacic @ 2020-01-05 19:10 ` Luka Kovacic 2020-01-23 12:31 ` Tom Rini 2020-02-08 0:05 ` Tom Rini 0 siblings, 2 replies; 16+ messages in thread From: Luka Kovacic @ 2020-01-05 19:10 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-05 19:10 ` [PATCH 1/1 " Luka Kovacic @ 2020-01-23 12:31 ` Tom Rini 2020-01-23 21:04 ` Luka Kovačič 2020-02-08 0:05 ` Tom Rini 1 sibling, 1 reply; 16+ messages in thread From: Tom Rini @ 2020-01-23 12:31 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-23 12:31 ` Tom Rini @ 2020-01-23 21:04 ` Luka Kovačič 2020-01-23 21:12 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Luka Kovačič @ 2020-01-23 21:04 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-23 21:04 ` Luka Kovačič @ 2020-01-23 21:12 ` Tom Rini 2020-01-27 6:30 ` [PATCH 1/2 v1] cmd: doc: Update command return values luka.kovacic at sartura.hr ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Tom Rini @ 2020-01-23 21:12 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2 v1] cmd: doc: Update command return values 2020-01-23 21:12 ` Tom Rini @ 2020-01-27 6:30 ` luka.kovacic at sartura.hr 2020-01-27 6:30 ` [PATCH 2/2 v1] cmd: gpio: Add the command return value to the help text luka.kovacic at sartura.hr 2020-01-30 2:17 ` [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value Simon Glass 2 siblings, 0 replies; 16+ messages in thread From: luka.kovacic at sartura.hr @ 2020-01-27 6:30 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2 v1] cmd: gpio: Add the command return value to the help text 2020-01-23 21:12 ` Tom Rini 2020-01-27 6:30 ` [PATCH 1/2 v1] cmd: doc: Update command return values luka.kovacic at sartura.hr @ 2020-01-27 6:30 ` luka.kovacic at sartura.hr 2020-01-30 2:17 ` [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value Simon Glass 2 siblings, 0 replies; 16+ messages in thread From: luka.kovacic at sartura.hr @ 2020-01-27 6:30 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-23 21:12 ` Tom Rini 2020-01-27 6:30 ` [PATCH 1/2 v1] cmd: doc: Update command return values luka.kovacic at sartura.hr 2020-01-27 6:30 ` [PATCH 2/2 v1] cmd: gpio: Add the command return value to the help text luka.kovacic at sartura.hr @ 2020-01-30 2:17 ` Simon Glass 2020-01-30 18:52 ` Tom Rini 2 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2020-01-30 2:17 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-30 2:17 ` [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value Simon Glass @ 2020-01-30 18:52 ` Tom Rini 2020-01-31 2:27 ` Simon Glass 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2020-01-30 18:52 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-30 18:52 ` Tom Rini @ 2020-01-31 2:27 ` Simon Glass 2020-01-31 20:59 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Simon Glass @ 2020-01-31 2:27 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-31 2:27 ` Simon Glass @ 2020-01-31 20:59 ` Tom Rini 2020-02-02 22:50 ` Simon Glass 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2020-01-31 20:59 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-31 20:59 ` Tom Rini @ 2020-02-02 22:50 ` Simon Glass 0 siblings, 0 replies; 16+ messages in thread From: Simon Glass @ 2020-02-02 22:50 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-01-05 19:10 ` [PATCH 1/1 " Luka Kovacic 2020-01-23 12:31 ` Tom Rini @ 2020-02-08 0:05 ` Tom Rini 2020-03-10 9:47 ` Alex Kiernan 1 sibling, 1 reply; 16+ messages in thread From: Tom Rini @ 2020-02-08 0:05 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-02-08 0:05 ` Tom Rini @ 2020-03-10 9:47 ` Alex Kiernan 2020-03-10 12:37 ` Tom Rini 0 siblings, 1 reply; 16+ messages in thread From: Alex Kiernan @ 2020-03-10 9:47 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-03-10 9:47 ` Alex Kiernan @ 2020-03-10 12:37 ` Tom Rini 2020-03-10 13:23 ` Alex Kiernan 0 siblings, 1 reply; 16+ messages in thread From: Tom Rini @ 2020-03-10 12:37 UTC (permalink / raw) To: u-boot 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value 2020-03-10 12:37 ` Tom Rini @ 2020-03-10 13:23 ` Alex Kiernan 0 siblings, 0 replies; 16+ messages in thread From: Alex Kiernan @ 2020-03-10 13:23 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-03-10 13:23 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-05 19:10 [PATCH 0/1 v1] cmd: gpio: Correct do_gpio() return value Luka Kovacic 2020-01-05 19:10 ` [PATCH 1/1 " Luka Kovacic 2020-01-23 12:31 ` Tom Rini 2020-01-23 21:04 ` Luka Kovačič 2020-01-23 21:12 ` Tom Rini 2020-01-27 6:30 ` [PATCH 1/2 v1] cmd: doc: Update command return values luka.kovacic at sartura.hr 2020-01-27 6:30 ` [PATCH 2/2 v1] cmd: gpio: Add the command return value to the help text luka.kovacic at sartura.hr 2020-01-30 2:17 ` [PATCH 1/1 v1] cmd: gpio: Correct do_gpio() return value Simon Glass 2020-01-30 18:52 ` Tom Rini 2020-01-31 2:27 ` Simon Glass 2020-01-31 20:59 ` Tom Rini 2020-02-02 22:50 ` Simon Glass 2020-02-08 0:05 ` Tom Rini 2020-03-10 9:47 ` Alex Kiernan 2020-03-10 12:37 ` Tom Rini 2020-03-10 13:23 ` Alex Kiernan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.