Message ID | 20201028100640.13876-1-patrick.delaunay@st.com |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
Series | [1/2] cmd: pinmux: update result of do_status | expand |
Hi Patrick -----Original Message----- From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On Behalf Of Patrick DELAUNAY Sent: mercredi 28 octobre 2020 11:07 To: u-boot@lists.denx.de Cc: U-Boot STM32 <uboot-stm32@st-md-mailman.stormreply.com>; Simon Glass <sjg@chromium.org>; Patrick DELAUNAY <patrick.delaunay@st.com>; Sean Anderson <seanga2@gmail.com> Subject: [Uboot-stm32] [PATCH 1/2] cmd: pinmux: update result of do_status Update the result of do_status and alway returns a CMD_RET_ value (-ENOSYS was a possible result of show_pinmux). This patch also adds pincontrol name in error messages (dev->name) and treats correctly the status sub command when pin-controller device is not selected. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- cmd/pinmux.c | 44 +++++++++++++++++++----------------- test/py/tests/test_pinmux.py | 4 ++-- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/cmd/pinmux.c b/cmd/pinmux.c index 9942b15419..af04c95a46 100644 --- a/cmd/pinmux.c +++ b/cmd/pinmux.c @@ -41,7 +41,7 @@ static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; } -static int show_pinmux(struct udevice *dev) +static void show_pinmux(struct udevice *dev) { char pin_name[PINNAME_SIZE]; char pin_mux[PINMUX_SIZE]; @@ -51,54 +51,56 @@ static int show_pinmux(struct udevice *dev) pins_count = pinctrl_get_pins_count(dev); - if (pins_count == -ENOSYS) { - printf("Ops get_pins_count not supported\n"); - return pins_count; + if (pins_count < 0) { + printf("Ops get_pins_count not supported by %s\n", dev->name); + return; } for (i = 0; i < pins_count; i++) { ret = pinctrl_get_pin_name(dev, i, pin_name, PINNAME_SIZE); - if (ret == -ENOSYS) { - printf("Ops get_pin_name not supported\n"); - return ret; + if (ret) { + printf("Ops get_pin_name error (%d) by %s\n", + ret, dev->name); + return; } ret = pinctrl_get_pin_muxing(dev, i, pin_mux, PINMUX_SIZE); if (ret) { - printf("Ops get_pin_muxing error (%d)\n", ret); - return ret; + printf("Ops get_pin_muxing error (%d) by %s in %s\n", + ret, pin_name, dev->name); + return; } printf("%-*s: %-*s\n", PINNAME_SIZE, pin_name, PINMUX_SIZE, pin_mux); } - - return 0; } static int do_status(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct udevice *dev; - int ret = CMD_RET_USAGE; - if (currdev && (argc < 2 || strcmp(argv[1], "-a"))) - return show_pinmux(currdev); + if (argc < 2) { + if (!currdev) { + printf("pin-controller device not selected\n"); + return CMD_RET_FAILURE; + } + show_pinmux(currdev); + return CMD_RET_SUCCESS; + } - if (argc < 2 || strcmp(argv[1], "-a")) - return ret; + if (strcmp(argv[1], "-a")) + return CMD_RET_USAGE; uclass_foreach_dev_probe(UCLASS_PINCTRL, dev) { /* insert a separator between each pin-controller display */ printf("--------------------------\n"); printf("%s:\n", dev->name); - ret = show_pinmux(dev); - if (ret < 0) - printf("Can't display pin muxing for %s\n", - dev->name); + show_pinmux(dev); } - return ret; + return CMD_RET_SUCCESS; } static int do_list(struct cmd_tbl *cmdtp, int flag, int argc, diff --git a/test/py/tests/test_pinmux.py b/test/py/tests/test_pinmux.py index 0cbbae000c..b3ae2ab024 100644 --- a/test/py/tests/test_pinmux.py +++ b/test/py/tests/test_pinmux.py @@ -13,9 +13,9 @@ def test_pinmux_usage_1(u_boot_console): @pytest.mark.buildconfigspec('cmd_pinmux') def test_pinmux_usage_2(u_boot_console): """Test that 'pinmux status' executed without previous "pinmux dev" - command displays pinmux usage.""" + command displays error message.""" output = u_boot_console.run_command('pinmux status') - assert 'Usage:' in output + assert 'pin-controller device not selected' in output @pytest.mark.buildconfigspec('cmd_pinmux') @pytest.mark.boardspec('sandbox') Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> Thanks Patrice -- 2.17.1
Hi Patrick, On Tue, 20 Apr 2021 at 03:21, Patrice CHOTARD <patrice.chotard@st.com> wrote: > > Hi Patrick > > -----Original Message----- > From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On Behalf Of Patrick DELAUNAY > Sent: mercredi 28 octobre 2020 11:07 > To: u-boot@lists.denx.de > Cc: U-Boot STM32 <uboot-stm32@st-md-mailman.stormreply.com>; Simon Glass <sjg@chromium.org>; Patrick DELAUNAY <patrick.delaunay@st.com>; Sean Anderson <seanga2@gmail.com> > Subject: [Uboot-stm32] [PATCH 1/2] cmd: pinmux: update result of do_status > > Update the result of do_status and alway returns a CMD_RET_ value (-ENOSYS was a possible result of show_pinmux). > > This patch also adds pincontrol name in error messages (dev->name) and treats correctly the status sub command when pin-controller device is not selected. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > > cmd/pinmux.c | 44 +++++++++++++++++++----------------- > test/py/tests/test_pinmux.py | 4 ++-- > 2 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/cmd/pinmux.c b/cmd/pinmux.c index 9942b15419..af04c95a46 100644 > --- a/cmd/pinmux.c > +++ b/cmd/pinmux.c > @@ -41,7 +41,7 @@ static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, > return CMD_RET_SUCCESS; > } > > -static int show_pinmux(struct udevice *dev) I think it is better to return the error code and let the caller ignore it, If we later want to report the error code, we can. > +static void show_pinmux(struct udevice *dev) > { > char pin_name[PINNAME_SIZE]; > char pin_mux[PINMUX_SIZE]; > @@ -51,54 +51,56 @@ static int show_pinmux(struct udevice *dev) > > pins_count = pinctrl_get_pins_count(dev); > > - if (pins_count == -ENOSYS) { > - printf("Ops get_pins_count not supported\n"); > - return pins_count; > + if (pins_count < 0) { Why change this to < 0 ? I believe that -ENOSYS is the only valid error. We should update the get_pins_count() API function to indicate that. > + printf("Ops get_pins_count not supported by %s\n", dev->name); > + return; > } > > for (i = 0; i < pins_count; i++) { > ret = pinctrl_get_pin_name(dev, i, pin_name, PINNAME_SIZE); > - if (ret == -ENOSYS) { > - printf("Ops get_pin_name not supported\n"); > - return ret; > + if (ret) { > + printf("Ops get_pin_name error (%d) by %s\n", > + ret, dev->name); > + return; > } > > ret = pinctrl_get_pin_muxing(dev, i, pin_mux, PINMUX_SIZE); > if (ret) { > - printf("Ops get_pin_muxing error (%d)\n", ret); > - return ret; > + printf("Ops get_pin_muxing error (%d) by %s in %s\n", > + ret, pin_name, dev->name); > + return; > } > > printf("%-*s: %-*s\n", PINNAME_SIZE, pin_name, > PINMUX_SIZE, pin_mux); > } > - > - return 0; > } > > static int do_status(struct cmd_tbl *cmdtp, int flag, int argc, > char *const argv[]) > { > struct udevice *dev; > - int ret = CMD_RET_USAGE; > > - if (currdev && (argc < 2 || strcmp(argv[1], "-a"))) > - return show_pinmux(currdev); > + if (argc < 2) { > + if (!currdev) { > + printf("pin-controller device not selected\n"); > + return CMD_RET_FAILURE; > + } > + show_pinmux(currdev); > + return CMD_RET_SUCCESS; > + } > > - if (argc < 2 || strcmp(argv[1], "-a")) > - return ret; > + if (strcmp(argv[1], "-a")) > + return CMD_RET_USAGE; > > uclass_foreach_dev_probe(UCLASS_PINCTRL, dev) { > /* insert a separator between each pin-controller display */ > printf("--------------------------\n"); > printf("%s:\n", dev->name); > - ret = show_pinmux(dev); > - if (ret < 0) > - printf("Can't display pin muxing for %s\n", > - dev->name); > + show_pinmux(dev); > } > > - return ret; > + return CMD_RET_SUCCESS; > } > > static int do_list(struct cmd_tbl *cmdtp, int flag, int argc, diff --git a/test/py/tests/test_pinmux.py b/test/py/tests/test_pinmux.py index 0cbbae000c..b3ae2ab024 100644 > --- a/test/py/tests/test_pinmux.py > +++ b/test/py/tests/test_pinmux.py > @@ -13,9 +13,9 @@ def test_pinmux_usage_1(u_boot_console): > @pytest.mark.buildconfigspec('cmd_pinmux') > def test_pinmux_usage_2(u_boot_console): > """Test that 'pinmux status' executed without previous "pinmux dev" > - command displays pinmux usage.""" > + command displays error message.""" > output = u_boot_console.run_command('pinmux status') > - assert 'Usage:' in output > + assert 'pin-controller device not selected' in output > > @pytest.mark.buildconfigspec('cmd_pinmux') > @pytest.mark.boardspec('sandbox') > > Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> > > Thanks > Patrice Regards, Simon
Hi Simon, On 4/29/21 6:10 PM, Simon Glass wrote: > Hi Patrick, > > On Tue, 20 Apr 2021 at 03:21, Patrice CHOTARD <patrice.chotard@st.com> wrote: >> Hi Patrick >> >> -----Original Message----- >> From: Uboot-stm32 <uboot-stm32-bounces@st-md-mailman.stormreply.com> On Behalf Of Patrick DELAUNAY >> Sent: mercredi 28 octobre 2020 11:07 >> To: u-boot@lists.denx.de >> Cc: U-Boot STM32 <uboot-stm32@st-md-mailman.stormreply.com>; Simon Glass <sjg@chromium.org>; Patrick DELAUNAY <patrick.delaunay@st.com>; Sean Anderson <seanga2@gmail.com> >> Subject: [Uboot-stm32] [PATCH 1/2] cmd: pinmux: update result of do_status >> >> Update the result of do_status and alway returns a CMD_RET_ value (-ENOSYS was a possible result of show_pinmux). >> >> This patch also adds pincontrol name in error messages (dev->name) and treats correctly the status sub command when pin-controller device is not selected. >> >> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> >> --- >> >> cmd/pinmux.c | 44 +++++++++++++++++++----------------- >> test/py/tests/test_pinmux.py | 4 ++-- >> 2 files changed, 25 insertions(+), 23 deletions(-) >> >> diff --git a/cmd/pinmux.c b/cmd/pinmux.c index 9942b15419..af04c95a46 100644 >> --- a/cmd/pinmux.c >> +++ b/cmd/pinmux.c >> @@ -41,7 +41,7 @@ static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, >> return CMD_RET_SUCCESS; >> } >> >> -static int show_pinmux(struct udevice *dev) > I think it is better to return the error code and let the caller > ignore it, If we later want to report the error code, we can. ok even if this static is only a help of do_status I will continue to return the result. >> +static void show_pinmux(struct udevice *dev) >> { >> char pin_name[PINNAME_SIZE]; >> char pin_mux[PINMUX_SIZE]; >> @@ -51,54 +51,56 @@ static int show_pinmux(struct udevice *dev) >> >> pins_count = pinctrl_get_pins_count(dev); >> >> - if (pins_count == -ENOSYS) { >> - printf("Ops get_pins_count not supported\n"); >> - return pins_count; >> + if (pins_count < 0) { > Why change this to < 0 ? > > I believe that -ENOSYS is the only valid error. We should update the > get_pins_count() API function to indicate that. I think that any value < 0 was allowed ... /** * pinctrl_get_pins_count() - Display pin-controller pins number * @dev:Pinctrl device to use * * This allows to know the number of pins owned by a given pin-controller * * Return: Number of pins if OK, or negative error code on failure */ intpinctrl_get_pins_count(structudevice*dev); with intpinctrl_get_pins_count(structudevice*dev) { struct pinctrl_ops *ops = pinctrl_get_ops(dev); if (!ops->get_pins_count) return -ENOSYS; returnops->get_pins_count(dev); } But after check you are right: the ops don't allow negative value for error /** * @get_pins_count:Get the number of selectable pins * * @dev:Pinctrl device to use * * This function is necessary to parse the "pins" property in DTS. * * @Return: * number of selectable named pins available in this driver */ int(*get_pins_count)(structudevice*dev); I will update the API doc and this check. >> + printf("Ops get_pins_count not supported by %s\n", dev->name); >> + return; >> } >> >> (...) >> >> static int do_list(struct cmd_tbl *cmdtp, int flag, int argc, diff --git a/test/py/tests/test_pinmux.py b/test/py/tests/test_pinmux.py index 0cbbae000c..b3ae2ab024 100644 >> --- a/test/py/tests/test_pinmux.py >> +++ b/test/py/tests/test_pinmux.py >> @@ -13,9 +13,9 @@ def test_pinmux_usage_1(u_boot_console): >> @pytest.mark.buildconfigspec('cmd_pinmux') >> def test_pinmux_usage_2(u_boot_console): >> """Test that 'pinmux status' executed without previous "pinmux dev" >> - command displays pinmux usage.""" >> + command displays error message.""" >> output = u_boot_console.run_command('pinmux status') >> - assert 'Usage:' in output >> + assert 'pin-controller device not selected' in output >> >> @pytest.mark.buildconfigspec('cmd_pinmux') >> @pytest.mark.boardspec('sandbox') >> >> Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com> >> >> Thanks >> Patrice > Regards, > Simon > _______________________________________________ > Uboot-stm32 mailing list > Uboot-stm32@st-md-mailman.stormreply.com > https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32 Regards Patrick
diff --git a/cmd/pinmux.c b/cmd/pinmux.c index 9942b15419..af04c95a46 100644 --- a/cmd/pinmux.c +++ b/cmd/pinmux.c @@ -41,7 +41,7 @@ static int do_dev(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; } -static int show_pinmux(struct udevice *dev) +static void show_pinmux(struct udevice *dev) { char pin_name[PINNAME_SIZE]; char pin_mux[PINMUX_SIZE]; @@ -51,54 +51,56 @@ static int show_pinmux(struct udevice *dev) pins_count = pinctrl_get_pins_count(dev); - if (pins_count == -ENOSYS) { - printf("Ops get_pins_count not supported\n"); - return pins_count; + if (pins_count < 0) { + printf("Ops get_pins_count not supported by %s\n", dev->name); + return; } for (i = 0; i < pins_count; i++) { ret = pinctrl_get_pin_name(dev, i, pin_name, PINNAME_SIZE); - if (ret == -ENOSYS) { - printf("Ops get_pin_name not supported\n"); - return ret; + if (ret) { + printf("Ops get_pin_name error (%d) by %s\n", + ret, dev->name); + return; } ret = pinctrl_get_pin_muxing(dev, i, pin_mux, PINMUX_SIZE); if (ret) { - printf("Ops get_pin_muxing error (%d)\n", ret); - return ret; + printf("Ops get_pin_muxing error (%d) by %s in %s\n", + ret, pin_name, dev->name); + return; } printf("%-*s: %-*s\n", PINNAME_SIZE, pin_name, PINMUX_SIZE, pin_mux); } - - return 0; } static int do_status(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) { struct udevice *dev; - int ret = CMD_RET_USAGE; - if (currdev && (argc < 2 || strcmp(argv[1], "-a"))) - return show_pinmux(currdev); + if (argc < 2) { + if (!currdev) { + printf("pin-controller device not selected\n"); + return CMD_RET_FAILURE; + } + show_pinmux(currdev); + return CMD_RET_SUCCESS; + } - if (argc < 2 || strcmp(argv[1], "-a")) - return ret; + if (strcmp(argv[1], "-a")) + return CMD_RET_USAGE; uclass_foreach_dev_probe(UCLASS_PINCTRL, dev) { /* insert a separator between each pin-controller display */ printf("--------------------------\n"); printf("%s:\n", dev->name); - ret = show_pinmux(dev); - if (ret < 0) - printf("Can't display pin muxing for %s\n", - dev->name); + show_pinmux(dev); } - return ret; + return CMD_RET_SUCCESS; } static int do_list(struct cmd_tbl *cmdtp, int flag, int argc, diff --git a/test/py/tests/test_pinmux.py b/test/py/tests/test_pinmux.py index 0cbbae000c..b3ae2ab024 100644 --- a/test/py/tests/test_pinmux.py +++ b/test/py/tests/test_pinmux.py @@ -13,9 +13,9 @@ def test_pinmux_usage_1(u_boot_console): @pytest.mark.buildconfigspec('cmd_pinmux') def test_pinmux_usage_2(u_boot_console): """Test that 'pinmux status' executed without previous "pinmux dev" - command displays pinmux usage.""" + command displays error message.""" output = u_boot_console.run_command('pinmux status') - assert 'Usage:' in output + assert 'pin-controller device not selected' in output @pytest.mark.buildconfigspec('cmd_pinmux') @pytest.mark.boardspec('sandbox')
Update the result of do_status and alway returns a CMD_RET_ value (-ENOSYS was a possible result of show_pinmux). This patch also adds pincontrol name in error messages (dev->name) and treats correctly the status sub command when pin-controller device is not selected. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- cmd/pinmux.c | 44 +++++++++++++++++++----------------- test/py/tests/test_pinmux.py | 4 ++-- 2 files changed, 25 insertions(+), 23 deletions(-)