Hi Pragnesh,
On Tue, 1 Dec 2020 at 00:05, Pragnesh Patel <pragnesh.pa...@openfive.com> wrote:
>
> Hi Simon,
>
> >-----Original Message-----
> >From: U-Boot <u-boot-boun...@lists.denx.de> On Behalf Of Pragnesh Patel
> >Sent: 01 December 2020 11:17
> >To: Simon Glass <s...@chromium.org>
> >Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra
> ><atish.pa...@wdc.com>; Palmer Dabbelt <palmerdabb...@google.com>; Bin
> >Meng <bmeng...@gmail.com>; Paul Walmsley ( Sifive)
> ><paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar Kadam
> ><sagar.ka...@openfive.com>; rick <r...@andestech.com>; Naoki Hayama
> ><naoki.hay...@lineo.co.jp>; Marek Vasut <marek.vasut+rene...@gmail.com>;
> >Patrick Delaunay <patrick.delau...@st.com>; Adam Ford
> ><aford...@gmail.com>; Thomas Hebb <tommyh...@gmail.com>; Ramon Fried
> ><rfried....@gmail.com>; Heinrich Schuchardt <xypron.g...@gmx.de>; Bin Meng
> ><bin.m...@windriver.com>; Sam Protsenko <joe.s...@gmail.com>; Miquel
> >Raynal <miquel.ray...@bootlin.com>; Philippe Reynes
> ><philippe.rey...@softathome.com>; Frédéric Danis
> ><frederic.da...@collabora.com>; Patrice Chotard <patrice.chot...@st.com>;
> >Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com>
> >Subject: RE: [PATCH v2] cmd: Add a pwm command
> >
> >Hi Simon,
> >
> >>-----Original Message-----
> >>From: Simon Glass <s...@chromium.org>
> >>Sent: 01 December 2020 01:42
> >>To: Pragnesh Patel <pragnesh.pa...@openfive.com>
> >>Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Atish Patra
> >><atish.pa...@wdc.com>; Palmer Dabbelt <palmerdabb...@google.com>; Bin
> >>Meng <bmeng...@gmail.com>; Paul Walmsley ( Sifive)
> >><paul.walms...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar
> >>Kadam <sagar.ka...@openfive.com>; rick <r...@andestech.com>; Naoki
> >>Hayama <naoki.hay...@lineo.co.jp>; Marek Vasut
> >><marek.vasut+rene...@gmail.com>; Patrick Delaunay
> >><patrick.delau...@st.com>; Adam Ford <aford...@gmail.com>; Thomas Hebb
> >><tommyh...@gmail.com>; Ramon Fried <rfried....@gmail.com>; Heinrich
> >>Schuchardt <xypron.g...@gmx.de>; Bin Meng <bin.m...@windriver.com>;
> >Sam
> >>Protsenko <joe.s...@gmail.com>; Miquel Raynal
> >><miquel.ray...@bootlin.com>; Philippe Reynes
> >><philippe.rey...@softathome.com>; Frédéric Danis
> >><frederic.da...@collabora.com>; Patrice Chotard
> >><patrice.chot...@st.com>; Vladimir Olovyannikov
> >><vladimir.olovyanni...@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.pa...@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.pa...@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 <s...@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.pa...@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.pa...@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
