| Message ID | 20200105191056.12571-2-luka.kovacic@sartura.hr |
|---|---|
| State | Accepted |
| Commit | 4dbc107f4683e45749045b97f0b529d5eb5178a9 |
| Headers | show |
| Series | cmd: gpio: Correct do_gpio() return value | expand |
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.
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 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. > > -- > Tom
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. > > > > -- > > Tom
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. Regards, Simon
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? > > Regards, > Simon
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
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 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? What about the socfpga example? Thanks!
On Sat, Feb 8, 2020 at 12:06 AM 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> > > 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.
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 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> > > > > 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!
On Tue, Mar 10, 2020 at 12:37 PM Tom Rini <trini at 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 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> > > > > > > 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/
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.