Hi Simon,
-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Pragnesh Patel Sent: 01 December 2020 11:17 To: Simon Glass sjg@chromium.org Cc: U-Boot Mailing List u-boot@lists.denx.de; Atish Patra atish.patra@wdc.com; Palmer Dabbelt palmerdabbelt@google.com; Bin Meng bmeng.cn@gmail.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Anup Patel anup.patel@wdc.com; Sagar Kadam sagar.kadam@openfive.com; rick rick@andestech.com; Naoki Hayama naoki.hayama@lineo.co.jp; Marek Vasut marek.vasut+renesas@gmail.com; Patrick Delaunay patrick.delaunay@st.com; Adam Ford aford173@gmail.com; Thomas Hebb tommyhebb@gmail.com; Ramon Fried rfried.dev@gmail.com; Heinrich Schuchardt xypron.glpk@gmx.de; Bin Meng bin.meng@windriver.com; Sam Protsenko joe.skb7@gmail.com; Miquel Raynal miquel.raynal@bootlin.com; Philippe Reynes philippe.reynes@softathome.com; Frédéric Danis frederic.danis@collabora.com; Patrice Chotard patrice.chotard@st.com; Vladimir Olovyannikov vladimir.olovyannikov@broadcom.com Subject: RE: [PATCH v2] cmd: Add a pwm command
Hi Simon,
-----Original Message----- From: Simon Glass sjg@chromium.org Sent: 01 December 2020 01:42 To: Pragnesh Patel pragnesh.patel@openfive.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Atish Patra atish.patra@wdc.com; Palmer Dabbelt palmerdabbelt@google.com; Bin Meng bmeng.cn@gmail.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Anup Patel anup.patel@wdc.com; Sagar Kadam sagar.kadam@openfive.com; rick rick@andestech.com; Naoki Hayama naoki.hayama@lineo.co.jp; Marek Vasut marek.vasut+renesas@gmail.com; Patrick Delaunay patrick.delaunay@st.com; Adam Ford aford173@gmail.com; Thomas Hebb tommyhebb@gmail.com; Ramon Fried rfried.dev@gmail.com; Heinrich Schuchardt xypron.glpk@gmx.de; Bin Meng bin.meng@windriver.com;
Sam
Protsenko joe.skb7@gmail.com; Miquel Raynal miquel.raynal@bootlin.com; Philippe Reynes philippe.reynes@softathome.com; Frédéric Danis frederic.danis@collabora.com; Patrice Chotard patrice.chotard@st.com; Vladimir Olovyannikov vladimir.olovyannikov@broadcom.com Subject: Re: [PATCH v2] cmd: Add a pwm command
[External Email] Do not click links or attachments unless you recognize the sender and know the content is safe
Hi Pragnesh,
On Thu, 26 Nov 2020 at 03:48, Pragnesh Patel pragnesh.patel@sifive.com wrote:
Add the command "pwm" for controlling the pwm channels. This command provides pwm invert/config/enable/disable functionalities via PWM uclass drivers
Signed-off-by: Pragnesh Patel pragnesh.patel@sifive.com
Changes in v2:
- Add test for pwm command
README | 1 + cmd/Kconfig | 6 ++ cmd/Makefile | 1 + cmd/pwm.c | 120 ++++++++++++++++++++++++++++++++++++++ configs/sandbox_defconfig | 1 + test/cmd/Makefile | 1 + test/cmd/pwm.c | 54 +++++++++++++++++ 7 files changed, 184 insertions(+) create mode 100644 cmd/pwm.c create mode 100644 test/cmd/pwm.c
Reviewed-by: Simon Glass sjg@chromium.org
Minor nits below
diff --git a/README b/README index cb49aa15da..dab291e0d0 100644 --- a/README +++ b/README @@ -3160,6 +3160,7 @@ i2c - I2C sub-system sspi - SPI utility commands base - print or set address offset printenv- print environment variables +pwm - control pwm channels setenv - set environment variables saveenv - save environment variables to persistent storage protect
- enable or disable FLASH write protection diff --git a/cmd/Kconfig
b/cmd/Kconfig index 1595de999b..0d085108f4 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -918,6 +918,12 @@ config CMD_GPIO help GPIO support.
+config CMD_PWM
bool "pwm"depends on DM_PWMhelpControl PWM channels, this allows+invert/config/enable/disable PWM
channels.
config CMD_GPT bool "GPT (GUID Partition Table) command" select EFI_PARTITION diff --git a/cmd/Makefile b/cmd/Makefile index dd86675bf2..75df3c136c 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -120,6 +120,7 @@ endif obj-$(CONFIG_CMD_PINMUX) += pinmux.o obj-$(CONFIG_CMD_PMC) += pmc.o obj-$(CONFIG_CMD_PSTORE) += pstore.o +obj-$(CONFIG_CMD_PWM) += pwm.o obj-$(CONFIG_CMD_PXE) += pxe.o pxe_utils.o obj-$(CONFIG_CMD_WOL) += wol.o obj-$(CONFIG_CMD_QFW) += qfw.o diff --git a/cmd/pwm.c b/cmd/pwm.c new file mode 100644 index 0000000000..f704c7a755 --- /dev/null +++ b/cmd/pwm.c @@ -0,0 +1,120 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Control PWM channels
- Copyright (c) 2020 SiFive, Inc
- author: Pragnesh Patel pragnesh.patel@sifive.com */
+#include <command.h> +#include <dm.h> +#include <pwm.h>
+enum pwm_cmd {
PWM_SET_INVERT,PWM_SET_CONFIG,PWM_SET_ENABLE,PWM_SET_DISABLE,+};
+static int do_pwm(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[]) {const char *str_cmd, *str_channel = NULL, *str_enable = NULL;const char *str_pwm = NULL, *str_period = NULL, *str_duty = NULL;enum pwm_cmd sub_cmd;struct udevice *dev;u32 channel, pwm_enable, pwm_dev, period_ns = 0, duty_ns = 0;int ret;if (argc < 4)- show_usage:
return CMD_RET_USAGE;str_cmd = argv[1];argc -= 2;argv += 2;if (argc > 0) {str_pwm = *argv;argc--;argv++;}if (!str_pwm)goto show_usage;switch (*str_cmd) {case 'i':sub_cmd = PWM_SET_INVERT;break;case 'c':sub_cmd = PWM_SET_CONFIG;break;case 'e':sub_cmd = PWM_SET_ENABLE;break;case 'd':sub_cmd = PWM_SET_DISABLE;break;default:goto show_usage;I think it is better to use 'return CMD_RET_USAGE' at each place rather than use a goto.
Okay, will replace in v3.
}if (IS_ENABLED(CONFIG_DM_PWM)) {You don't need this because the command is only enabled if DM_PWM
Will remove.
pwm_dev = simple_strtoul(str_pwm, NULL, 10);ret = uclass_get_device(UCLASS_PWM, pwm_dev, &dev);if (ret) {printf("PWM: '%s' not found\n", str_pwm);printf("pwm: ...
Will update.
return cmd_process_error(cmdtp, ret);}}if (argc > 0) {str_channel = *argv;channel = simple_strtoul(str_channel, NULL, 10);argc--;argv++;} else {goto show_usage;}if (sub_cmd == PWM_SET_INVERT && argc > 0) {str_enable = *argv;pwm_enable = simple_strtoul(str_enable, NULL, 10);ret = pwm_set_invert(dev, channel, pwm_enable);} else if (sub_cmd == PWM_SET_CONFIG && argc == 2) {str_period = *argv;argc--;argv++;period_ns = simple_strtoul(str_period, NULL, 10);if (argc > 0) {str_duty = *argv;duty_ns = simple_strtoul(str_duty, NULL, 10);}ret = pwm_set_config(dev, channel, period_ns, duty_ns);} else if (sub_cmd == PWM_SET_ENABLE) {ret = pwm_set_enable(dev, channel, 1);} else if (sub_cmd == PWM_SET_DISABLE) {ret = pwm_set_enable(dev, channel, 0);} else {printf("PWM arguments missing\n");return CMD_RET_FAILURE;}if (ret) {printf("error\n");error (%d)
(so you print ret)
Okay, will update.
return CMD_RET_FAILURE;}printf("success\n");Drop this. Success is assumed unless an error is printed
Better to print success here because in test/cmd/pwm.c
ut_assert_nextline(""); will generate a warning like below,
In file included from test/cmd/pwm.c:14:0: test/cmd/pwm.c: In function âdm_test_pwm_cmdâ: test/cmd/pwm.c:28:21: warning: zero-length gnu_printf format string [-Wformat-zero-length] ut_assert_nextline(""); ^ include/test/ut.h:282:33: note: in definition of macro âut_assert_nextlineâ if (ut_check_console_line(uts, fmt, ##args)) { \ ^~~
Let me know if you have any other idea here.
Okay, will drop in v3.
return CMD_RET_SUCCESS;+}
+U_BOOT_CMD(pwm, 6, 0, do_pwm,
"control pwm channels","pwm <invert> <pwm_dev_num> <channel> <polarity>\n""pwm <config> <pwm_dev_num> <channel> <period_ns> <duty_ns>\n""pwm <enable/disable> <pwm_dev_num> <channel>");Should note that values are in decimal, since normally for U-Boot they are hex.
Okay, will add a note here.
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index f2a767a4cd..7a16603461 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -58,6 +58,7 @@ CONFIG_CMD_LSBLK=y CONFIG_CMD_MUX=y CONFIG_CMD_OSD=y CONFIG_CMD_PCI=y +CONFIG_CMD_PWM=y CONFIG_CMD_READ=y CONFIG_CMD_REMOTEPROC=y CONFIG_CMD_SPI=y diff --git a/test/cmd/Makefile b/test/cmd/Makefile index 859dcda239..dde98dd371 100644 --- a/test/cmd/Makefile +++ b/test/cmd/Makefile @@ -4,3 +4,4 @@
obj-y += mem.o obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o +obj-$(CONFIG_CMD_PWM) += pwm.o diff --git a/test/cmd/pwm.c b/test/cmd/pwm.c new file mode 100644 index 0000000000..987cdd1115 --- /dev/null +++ b/test/cmd/pwm.c @@ -0,0 +1,54 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Test for pwm command
- Copyright 2020 SiFive, Inc
- Authors:
- Pragnesh Patel pragnesh.patel@sifive.com
- */
+#include <dm.h> +#include <dm/test.h> +#include <test/test.h> +#include <test/ut.h>
+/* Basic test of 'pwm' command */ +static int dm_test_pwm_cmd(struct unit_test_state *uts) {
struct udevice *dev;ut_assertok(uclass_get_device(UCLASS_PWM, 0, &dev));ut_assertnonnull(dev);ut_assertok(console_record_reset_enable());/* pwm <invert> <pwm_dev_num> <channel> <polarity> */run_command("pwm invert 0 0 1", 0);console_record_readline(uts->actual_str,- sizeof(uts->actual_str));
Can you use ut_assert_nextline() for these?
Will update in v3.
ut_asserteq_str("success", uts->actual_str);run_command("pwm invert 0 0 0", 0);console_record_readline(uts->actual_str, sizeof(uts->actual_str));ut_asserteq_str("success", uts->actual_str);/* pwm <config> <pwm_dev_num> <channel> <period_ns> <duty_ns> */run_command("pwm config 0 0 period_ns duty_ns", 0);console_record_readline(uts->actual_str, sizeof(uts->actual_str));ut_asserteq_str("success", uts->actual_str);/* pwm <enable/disable> <pwm_dev_num> <channel> */run_command("pwm enable 0 0", 0);console_record_readline(uts->actual_str, sizeof(uts->actual_str));ut_asserteq_str("success", uts->actual_str);run_command("pwm disable 0 0", 0);console_record_readline(uts->actual_str, sizeof(uts->actual_str));ut_asserteq_str("success", uts->actual_str);ut_assert_console_end();return 0;+}
+DM_TEST(dm_test_pwm_cmd, UT_TESTF_SCAN_PDATA |
UT_TESTF_SCAN_FDT |
+UT_TESTF_CONSOLE_REC);
2.17.1
Regards, Simon