From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 23 Apr 2015 13:33:24 +0200 Subject: [U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command In-Reply-To: References: <1427229051-20170-1-git-send-email-p.marczak@samsung.com> <1429553273-6453-1-git-send-email-p.marczak@samsung.com> <1429553273-6453-8-git-send-email-p.marczak@samsung.com> Message-ID: <5538D884.1070605@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 04/22/2015 06:30 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 20 April 2015 at 12:07, Przemyslaw Marczak wrote: >> This command is based on driver model regulator's API. >> The user interface provides: >> - list UCLASS regulator devices >> - show or [set] operating regulator device >> - print constraints info >> - print operating status >> - print/[set] voltage value [uV] (force) >> - print/[set] current value [uA] >> - print/[set] operating mode id >> - enable the regulator output >> - disable the regulator output >> >> The 'force' option can be used for setting the value which exceeds >> the constraints min/max limits. >> >> Signed-off-by: Przemyslaw Marczak >> --- >> Changes v3: >> - new file >> - Kconfig entry >> >> Changes V4: >> - cmd regulator: move platdata to uc pdata >> - cmd_regulator: includes cleanup >> - cmd_regulator: add get_curr_dev_and_pl() check type >> - move config name: CONFIG_DM_REGULATOR_CMD to CONFIG_CMD_REGULATOR >> - common/Kconfig - cleanup >> --- >> common/Kconfig | 22 +++ >> common/Makefile | 1 + >> common/cmd_regulator.c | 403 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 426 insertions(+) >> create mode 100644 common/cmd_regulator.c > > Acked-by: Simon Glass > > I have a few nits that could be dealt with by a follow-on patch. > Ok. >> >> diff --git a/common/Kconfig b/common/Kconfig >> index 4666f8e..52f8bb1 100644 >> --- a/common/Kconfig >> +++ b/common/Kconfig >> @@ -470,5 +470,27 @@ config CMD_PMIC >> - pmic read address - read byte of register at address >> - pmic write address - write byte to register at address >> The only one change for this command is 'dev' subcommand. >> + >> +config CMD_REGULATOR >> + bool "Enable Driver Model REGULATOR command" >> + depends on DM_REGULATOR >> + help >> + This command is based on driver model regulator's API. >> + User interface features: >> + - list - list regulator devices >> + - regulator dev - show or [set] operating regulator device >> + - regulator info - print constraints info >> + - regulator status - print operating status >> + - regulator value - print/[set] voltage value [uV] >> + - regulator current - print/[set] current value [uA] >> + - regulator mode - print/[set] operating mode id >> + - regulator enable - enable the regulator output >> + - regulator disable - disable the regulator output >> + >> + The '-f' (force) option can be used for set the value which exceeds >> + the limits, which are found in device-tree and are kept in regulator's >> + uclass platdata structure. >> + >> endmenu >> + >> endmenu >> diff --git a/common/Makefile b/common/Makefile >> index 87a3efe..93bded3 100644 >> --- a/common/Makefile >> +++ b/common/Makefile >> @@ -212,6 +212,7 @@ obj-$(CONFIG_CMD_GPT) += cmd_gpt.o >> >> # Power >> obj-$(CONFIG_CMD_PMIC) += cmd_pmic.o >> +obj-$(CONFIG_CMD_REGULATOR) += cmd_regulator.o >> endif >> >> ifdef CONFIG_SPL_BUILD >> diff --git a/common/cmd_regulator.c b/common/cmd_regulator.c >> new file mode 100644 >> index 0000000..b1b9e87 >> --- /dev/null >> +++ b/common/cmd_regulator.c >> @@ -0,0 +1,403 @@ >> +/* >> + * Copyright (C) 2014-2015 Samsung Electronics >> + * Przemyslaw Marczak >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define LIMIT_SEQ 3 >> +#define LIMIT_DEVNAME 20 >> +#define LIMIT_OFNAME 20 >> +#define LIMIT_INFO 16 >> + >> +static struct udevice *currdev; >> + >> +static int failed(const char *getset, const char *thing, >> + const char *for_dev, int ret) >> +{ >> + printf("Can't %s %s %s.\nError: %d (%s)\n", getset, thing, for_dev, >> + ret, errno_str(ret)); > > blank line here. I don't see the blank line here in the patch, which I send. > > I worry that if someone gets one of these messages they will not be > able to find it in the source code. How about passing in the full > printf() string in each case, or just using printf() in situ? I don't > think the code space saving is significant. > It's not a debug message. And each one is different, and easy to grep "failed". The code is a little cleaner with this. Also the command code is not complicated. >> + return CMD_RET_FAILURE; >> +} >> + >> +static int regulator_get(bool list_only, int get_seq, struct udevice **devp) > > This function seems to do multiple things (find and list). Should we > split it into two? > >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + struct udevice *dev; >> + int ret; >> + >> + if (devp) >> + *devp = NULL; >> + >> + for (ret = uclass_first_device(UCLASS_REGULATOR, &dev); dev; >> + ret = uclass_next_device(&dev)) { > > This will probe all regulators that it checks. I think it should avoid > that. Do you mean to use > Regarding the two above comments, we have two problems: 1. Getting the regulator by sequencial number (dev->seq). I think it's required, because only this method returns the right device. Disadvantage: need to probe all devices. 2. Getting the regulator by "regulator-name" (regulator_uclass_platdata->name). This would be clean, but unreliable if we have few regulators with the same name - I think we should keep this in mind. Advantage: can use for non-probed devices. And about the doing multiple things by the regulator_get(). Following your comments about avoiding the code duplication, I put those things into one function, since both actually do the same - loops over the uclass's devices. So we can threat it as a subcommands: - regulator_get list - regulator_get dev Maybe the enum { GET_LIST, GET_DEV } would be better, than the bool. Is that really bad? >> + if (list_only) { >> + uc_pdata = dev_get_uclass_platdata(dev); >> + printf("|%*d | %*.*s @ %-*.*s| %s @ %s\n", >> + LIMIT_SEQ, dev->seq, >> + LIMIT_DEVNAME, LIMIT_DEVNAME, dev->name, >> + LIMIT_OFNAME, LIMIT_OFNAME, uc_pdata->name, >> + dev->parent->name, >> + dev_get_uclass_name(dev->parent)); >> + continue; >> + } >> + >> + if (dev->seq == get_seq) { >> + if (devp) >> + *devp = dev; >> + else >> + return -EINVAL; >> + >> + return 0; >> + } >> + } >> + >> + if (list_only) >> + return ret; >> + >> + return -ENODEV; >> +} >> + >> +static int do_dev(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int seq, ret = -ENXIO; >> + >> + switch (argc) { >> + case 2: >> + seq = simple_strtoul(argv[1], NULL, 0); >> + ret = uclass_get_device_by_seq(UCLASS_REGULATOR, seq, &currdev); >> + if (ret && (ret = regulator_get(false, seq, &currdev))) >> + goto failed; >> + case 1: >> + uc_pdata = dev_get_uclass_platdata(currdev); >> + if (!uc_pdata) >> + goto failed; >> + >> + printf("dev: %d @ %s\n", currdev->seq, uc_pdata->name); >> + } >> + >> + return CMD_RET_SUCCESS; >> +failed: >> + return failed("get", "the", "device", ret); >> +} >> + >> +static int get_curr_dev_and_pl(struct udevice **devp, > > What is pl? The name does not seem very meaningful to me. > The platdata, ok I will tune it. >> + struct dm_regulator_uclass_platdata **uc_pdata, >> + bool allow_type_fixed) >> +{ >> + *devp = NULL; >> + *uc_pdata = NULL; >> + >> + if (!currdev) >> + return failed("get", "current", "device", -ENODEV); >> + >> + *devp = currdev; >> + >> + *uc_pdata = dev_get_uclass_platdata(*devp); >> + if (!*uc_pdata) >> + return failed("get", "regulator", "platdata", -ENXIO); >> + >> + if (!allow_type_fixed && (*uc_pdata)->type == REGULATOR_TYPE_FIXED) { >> + printf("Operation not allowed for fixed regulator!\n"); >> + return CMD_RET_FAILURE; >> + } >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_list(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + int ret; >> + >> + printf("|%*s | %*.*s @ %-*.*s| %s @ %s\n", >> + LIMIT_SEQ, "Seq", >> + LIMIT_DEVNAME, LIMIT_DEVNAME, "Name", >> + LIMIT_OFNAME, LIMIT_OFNAME, "fdtname", >> + "Parent", "uclass"); >> + >> + ret = regulator_get(true, 0, NULL); >> + if (ret) >> + return CMD_RET_FAILURE; >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int constraint(const char *name, int val, const char *val_name) >> +{ >> + printf("%-*s", LIMIT_INFO, name); >> + if (val < 0) { >> + printf(" %s (err: %d)\n", errno_str(val), val); >> + return val; >> + } >> + >> + if (val_name) >> + printf(" %d (%s)\n", val, val_name); >> + else >> + printf(" %d\n", val); >> + >> + return 0; >> +} >> + >> +static const char *get_mode_name(struct dm_regulator_mode *mode, >> + int mode_count, >> + int mode_id) >> +{ >> + while (mode_count--) { >> + if (mode->id == mode_id) >> + return mode->name; >> + mode++; >> + } >> + >> + return NULL; >> +} >> + >> +static int do_info(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + struct dm_regulator_mode *modes; >> + const char *parent_uc; >> + int mode_count; >> + int ret; >> + int i; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, true); >> + if (ret) >> + return ret; >> + >> + parent_uc = dev_get_uclass_name(dev->parent); >> + >> + printf("Uclass regulator dev %d info:\n", dev->seq); >> + printf("%-*s %s @ %s\n%-*s %s\n%-*s %s\n%-*s\n", >> + LIMIT_INFO, "* parent:", dev->parent->name, parent_uc, >> + LIMIT_INFO, "* dev name:", dev->name, >> + LIMIT_INFO, "* fdt name:", uc_pdata->name, >> + LIMIT_INFO, "* constraints:"); >> + >> + constraint(" - min uV:", uc_pdata->min_uV, NULL); >> + constraint(" - max uV:", uc_pdata->max_uV, NULL); >> + constraint(" - min uA:", uc_pdata->min_uA, NULL); >> + constraint(" - max uA:", uc_pdata->max_uA, NULL); >> + constraint(" - always on:", uc_pdata->always_on, >> + uc_pdata->always_on ? "true" : "false"); >> + constraint(" - boot on:", uc_pdata->boot_on, >> + uc_pdata->boot_on ? "true" : "false"); >> + >> + mode_count = regulator_mode(dev, &modes); >> + constraint("* op modes:", mode_count, NULL); >> + >> + for (i = 0; i < mode_count; i++, modes++) >> + constraint(" - mode id:", modes->id, modes->name); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_status(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int current, value, mode, ret; >> + const char *mode_name = NULL; >> + struct udevice *dev; >> + bool enabled; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, true); >> + if (ret) >> + return ret; >> + >> + enabled = regulator_get_enable(dev); >> + constraint(" * enable:", enabled, enabled ? "true" : "false"); >> + >> + value = regulator_get_value(dev); >> + constraint(" * value uV:", value, NULL); >> + >> + current = regulator_get_current(dev); >> + constraint(" * current uA:", current, NULL); >> + >> + mode = regulator_get_mode(dev); >> + mode_name = get_mode_name(uc_pdata->mode, uc_pdata->mode_count, mode); >> + constraint(" * mode id:", mode, mode_name); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_value(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int value; >> + int force; >> + int ret; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, argc == 1); >> + if (ret) >> + return ret; >> + >> + if (argc == 1) { >> + value = regulator_get_value(dev); >> + if (value < 0) >> + return failed("get", uc_pdata->name, "voltage", value); >> + >> + printf("%d uV\n", value); >> + return CMD_RET_SUCCESS; >> + } >> + >> + if (argc == 3) >> + force = !strcmp("-f", argv[2]); >> + else >> + force = 0; >> + >> + value = simple_strtoul(argv[1], NULL, 0); >> + if ((value < uc_pdata->min_uV || value > uc_pdata->max_uV) && !force) { >> + printf("Value exceeds regulator constraint limits\n"); >> + return CMD_RET_FAILURE; >> + } >> + >> + ret = regulator_set_value(dev, value); >> + if (ret) >> + return failed("set", uc_pdata->name, "voltage value", ret); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_current(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int current; >> + int ret; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, argc == 1); >> + if (ret) >> + return ret; >> + >> + if (argc == 1) { >> + current = regulator_get_current(dev); >> + if (current < 0) >> + return failed("get", uc_pdata->name, "current", current); >> + >> + printf("%d uA\n", current); >> + return CMD_RET_SUCCESS; >> + } >> + >> + current = simple_strtoul(argv[1], NULL, 0); >> + if (current < uc_pdata->min_uA || current > uc_pdata->max_uA) { >> + printf("Current exceeds regulator constraint limits\n"); >> + return CMD_RET_FAILURE; >> + } >> + >> + ret = regulator_set_current(dev, current); >> + if (ret) >> + return failed("set", uc_pdata->name, "current value", ret); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_mode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int new_mode; >> + int mode; >> + int ret; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, false); >> + if (ret) >> + return ret; >> + >> + if (argc == 1) { >> + mode = regulator_get_mode(dev); >> + if (mode < 0) >> + return failed("get", uc_pdata->name, "mode", mode); >> + >> + printf("mode id: %d\n", mode); >> + return CMD_RET_SUCCESS; >> + } >> + >> + new_mode = simple_strtoul(argv[1], NULL, 0); >> + >> + ret = regulator_set_mode(dev, new_mode); >> + if (ret) >> + return failed("set", uc_pdata->name, "mode", ret); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_enable(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int ret; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, true); >> + if (ret) >> + return ret; >> + >> + ret = regulator_set_enable(dev, true); >> + if (ret) >> + return failed("enable", "regulator", uc_pdata->name, ret); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static int do_disable(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> +{ >> + struct udevice *dev; >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + int ret; >> + >> + ret = get_curr_dev_and_pl(&dev, &uc_pdata, true); >> + if (ret) >> + return ret; >> + >> + ret = regulator_set_enable(dev, false); >> + if (ret) >> + return failed("disable", "regulator", uc_pdata->name, ret); >> + >> + return CMD_RET_SUCCESS; >> +} >> + >> +static cmd_tbl_t subcmd[] = { >> + U_BOOT_CMD_MKENT(dev, 2, 1, do_dev, "", ""), >> + U_BOOT_CMD_MKENT(list, 1, 1, do_list, "", ""), >> + U_BOOT_CMD_MKENT(info, 2, 1, do_info, "", ""), >> + U_BOOT_CMD_MKENT(status, 2, 1, do_status, "", ""), >> + U_BOOT_CMD_MKENT(value, 3, 1, do_value, "", ""), >> + U_BOOT_CMD_MKENT(current, 3, 1, do_current, "", ""), >> + U_BOOT_CMD_MKENT(mode, 2, 1, do_mode, "", ""), >> + U_BOOT_CMD_MKENT(enable, 1, 1, do_enable, "", ""), >> + U_BOOT_CMD_MKENT(disable, 1, 1, do_disable, "", ""), >> +}; >> + >> +static int do_regulator(cmd_tbl_t *cmdtp, int flag, int argc, >> + char * const argv[]) >> +{ >> + cmd_tbl_t *cmd; >> + >> + argc--; >> + argv++; >> + >> + cmd = find_cmd_tbl(argv[0], subcmd, ARRAY_SIZE(subcmd)); >> + if (cmd == NULL || argc > cmd->maxargs) >> + return CMD_RET_USAGE; >> + >> + return cmd->cmd(cmdtp, flag, argc, argv); >> +} >> + >> +U_BOOT_CMD(regulator, CONFIG_SYS_MAXARGS, 1, do_regulator, >> + "uclass operations", >> + "list - list UCLASS regulator devices\n" >> + "regulator dev [id] - show or [set] operating regulator device\n" >> + "regulator [info] - print constraints info\n" >> + "regulator [status] - print operating status\n" >> + "regulator [value] [-f] - print/[set] voltage value [uV] (force)\n" >> + "regulator [current] - print/[set] current value [uA]\n" >> + "regulator [mode_id] - print/[set] operating mode id\n" >> + "regulator [enable] - enable the regulator output\n" >> + "regulator [disable] - disable the regulator output\n" >> +); >> -- >> 1.9.1 >> > > Regards, > Simon > Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com