| Message ID | 20201126104812.14619-1-pragnesh.patel@sifive.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] cmd: Add a pwm command | expand |
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_PWM > + help > + Control 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. > + } > + > + if (IS_ENABLED(CONFIG_DM_PWM)) { You don't need this because the command is only enabled if DM_PWM > + 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: ... > + 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) > + return CMD_RET_FAILURE; > + } > + > + printf("success\n"); Drop this. Success is assumed unless an error is printed > + 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. > 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? > + 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
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_PWM >> + help >> + Control 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 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
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_PWM >>> + help >>> + Control 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
Hi Pragnesh, On Tue, 1 Dec 2020 at 00:05, Pragnesh Patel <pragnesh.patel@openfive.com> wrote: > > 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_PWM > >>> + help > >>> + Control 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. How about adding ut_assert_nextline_empty() ? > > > > >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
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_PWM + help + Control 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; + } + + if (IS_ENABLED(CONFIG_DM_PWM)) { + 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); + 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"); + return CMD_RET_FAILURE; + } + + printf("success\n"); + 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>"); 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)); + 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);
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