[PATCH v2] cmd: Add a pwm command
Simon Glass
sjg at chromium.org
Mon Nov 30 21:11:44 CET 2020
Hi Pragnesh,
On Thu, 26 Nov 2020 at 03:48, Pragnesh Patel <pragnesh.patel at 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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list