| Message ID | 1429553273-6453-8-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Przemyslaw, On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> 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 <p.marczak@samsung.com> > --- > 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 <sjg@chromium.org> I have a few nits that could be dealt with by a follow-on patch. > > 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 <id> - show or [set] operating regulator device > + - regulator info - print constraints info > + - regulator status - print operating status > + - regulator value <val] <-f> - print/[set] voltage value [uV] > + - regulator current <val> - print/[set] current value [uA] > + - regulator mode <id> - 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 <p.marczak@samsung.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > +#include <common.h> > +#include <errno.h> > +#include <dm.h> > +#include <dm/uclass-internal.h> > +#include <power/regulator.h> > + > +#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 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. > + 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 > + 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. > + 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
On 22 April 2015 at 10:30, Simon Glass <sjg@chromium.org> wrote: > Hi Przemyslaw, > > On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> 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 <p.marczak@samsung.com> >> --- >> 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 <sjg@chromium.org> > > I have a few nits that could be dealt with by a follow-on patch. Applied to u-boot-dm/next, thanks!
Hello Simon, On 04/22/2015 06:30 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> 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 <p.marczak@samsung.com> >> --- >> 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 <sjg@chromium.org> > > 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 <id> - show or [set] operating regulator device >> + - regulator info - print constraints info >> + - regulator status - print operating status >> + - regulator value <val] <-f> - print/[set] voltage value [uV] >> + - regulator current <val> - print/[set] current value [uA] >> + - regulator mode <id> - 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 <p.marczak@samsung.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> +#include <common.h> >> +#include <errno.h> >> +#include <dm.h> >> +#include <dm/uclass-internal.h> >> +#include <power/regulator.h> >> + >> +#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,
Hi Przemyslaw, On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marczak@samsung.com> wrote: > Hello Simon, > > > On 04/22/2015 06:30 PM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> >> 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 <p.marczak@samsung.com> >>> --- >>> 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 <sjg@chromium.org> >> >> 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 <id> - show or [set] operating regulator device >>> + - regulator info - print constraints info >>> + - regulator status - print operating status >>> + - regulator value <val] <-f> - print/[set] voltage value [uV] >>> + - regulator current <val> - print/[set] current value [uA] >>> + - regulator mode <id> - 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 <p.marczak@samsung.com> >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> +#include <common.h> >>> +#include <errno.h> >>> +#include <dm.h> >>> +#include <dm/uclass-internal.h> >>> +#include <power/regulator.h> >>> + >>> +#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. Odd, there seem to be two blank lines there, and we only need one. > >> >> 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. git grep -i failed |wc -l 2089 Is there some way to know it is a PMIC error message, and find it that way? > >>> + 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. But you can use req_seq, or if you have platform data, check that. > > 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. We could probably refuse to bind regulators with the same name. That's just going to lead to confusion. > > 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? I suspect it would be better as two completely separate functions, and probably not much more code size. > > >>> + 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
Hello Simon, On 04/24/2015 06:51 AM, Simon Glass wrote: > Hi Przemyslaw, > > On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marczak@samsung.com> wrote: >> Hello Simon, >> >> >> On 04/22/2015 06:30 PM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> >>> 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 <p.marczak@samsung.com> >>>> --- >>>> 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 <sjg@chromium.org> >>> >>> 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 <id> - show or [set] operating regulator device >>>> + - regulator info - print constraints info >>>> + - regulator status - print operating status >>>> + - regulator value <val] <-f> - print/[set] voltage value [uV] >>>> + - regulator current <val> - print/[set] current value [uA] >>>> + - regulator mode <id> - 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 <p.marczak@samsung.com> >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0+ >>>> + */ >>>> +#include <common.h> >>>> +#include <errno.h> >>>> +#include <dm.h> >>>> +#include <dm/uclass-internal.h> >>>> +#include <power/regulator.h> >>>> + >>>> +#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. > > Odd, there seem to be two blank lines there, and we only need one. > Ah, sorry. You mean, that there should be added a blank line. Ok, will add one. >> >>> >>> 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. > > git grep -i failed |wc -l > 2089 > > Is there some way to know it is a PMIC error message, and find it that way? > Ok, I assumed that you know which command you called, and where to find it, so you could use: grep -i "failed" common/cmd_regulator.c | wc -l 15 But this was only the function name, not a useful text for grep. Now I see that this should use the printf instead of the helper funcion. >> >>>> + 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. > > But you can use req_seq, or if you have platform data, check that. > Ok, we could use the req_seq for the PMIC uclass, it's natural that interface, has its address and <reg> property - but this can repeat, if we have two PMICs on a different busses. This is probably possible. We also shouldn't set the req_seq as the number found in node name, because those numbers can repeat: ldo1 {}; buck1 {}; regulator1 { }. I think that, using the req_seq is bad idea, since we can't be sure that those values are unique. I understand that, the probe is not ideal here? But from the other side, if we call "pmic list", then we are sure, that listed devices are ready to use. Shouldn't we expect this? >> >> 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. > > We could probably refuse to bind regulators with the same name. That's > just going to lead to confusion. > This could be good, because at present we can get only the first matching device, for the given "regulator-name" constraint. Ok, I need add the checking routine to regulator's post_bind() method. And then refuse the bind of the repeated one. >> >> 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? > > I suspect it would be better as two completely separate functions, and > probably not much more code size. > Ok. >> >> >>>> + 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,
Hi Przemyslaw, On 24 April 2015 at 06:18, Przemyslaw Marczak <p.marczak@samsung.com> wrote: > Hello Simon, > > > On 04/24/2015 06:51 AM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marczak@samsung.com> >> wrote: >>> >>> Hello Simon, >>> >>> >>> On 04/22/2015 06:30 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Przemyslaw, >>>> >>>> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> >>>> 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 <p.marczak@samsung.com> >>>>> --- >>>>> 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 <sjg@chromium.org> >>>> >>>> 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 <id> - show or [set] operating regulator >>>>> device >>>>> + - regulator info - print constraints info >>>>> + - regulator status - print operating status >>>>> + - regulator value <val] <-f> - print/[set] voltage value [uV] >>>>> + - regulator current <val> - print/[set] current value [uA] >>>>> + - regulator mode <id> - 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 <p.marczak@samsung.com> >>>>> + * >>>>> + * SPDX-License-Identifier: GPL-2.0+ >>>>> + */ >>>>> +#include <common.h> >>>>> +#include <errno.h> >>>>> +#include <dm.h> >>>>> +#include <dm/uclass-internal.h> >>>>> +#include <power/regulator.h> >>>>> + >>>>> +#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. >> >> >> Odd, there seem to be two blank lines there, and we only need one. >> > > Ah, sorry. You mean, that there should be added a blank line. > Ok, will add one. > >>> >>>> >>>> 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. >> >> >> git grep -i failed |wc -l >> 2089 >> >> Is there some way to know it is a PMIC error message, and find it that >> way? >> > > Ok, I assumed that you know which command you called, and where to find it, > so you could use: > grep -i "failed" common/cmd_regulator.c | wc -l > 15 > > But this was only the function name, not a useful text for grep. > Now I see that this should use the printf instead of the helper funcion. > >>> >>>>> + 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. >> >> >> But you can use req_seq, or if you have platform data, check that. >> > > Ok, we could use the req_seq for the PMIC uclass, it's natural that > interface, has its address and <reg> property - but this can repeat, > if we have two PMICs on a different busses. This is probably possible. > > We also shouldn't set the req_seq as the number found in node name, because > those numbers can repeat: ldo1 {}; buck1 {}; regulator1 { }. > > I think that, using the req_seq is bad idea, since we can't be sure that > those values are unique. > > I understand that, the probe is not ideal here? But from the other side, > if we call "pmic list", then we are sure, that listed devices are ready to > use. Shouldn't we expect this? I was hoping that we would not probe devices until they are actually used, and that listing them would not constitute 'use'. In the case of listing, you should not need to worry about ->seq or ->req_seq. If you avoid 'getting' the device you will not probe it. In the case of getting a device ready for use, yes, it must be probed. But I am only commenting on your 'list' function. > >>> >>> 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. >> >> >> We could probably refuse to bind regulators with the same name. That's >> just going to lead to confusion. >> > > This could be good, because at present we can get only the first matching > device, for the given "regulator-name" constraint. > Ok, I need add the checking routine to regulator's post_bind() method. > And then refuse the bind of the repeated one. > >>> >>> 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? >> >> >> I suspect it would be better as two completely separate functions, and >> probably not much more code size. >> > > Ok. > > >>> >>> >>>>> + 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 Regards, Simon
Hello Simon, On 04/24/2015 02:34 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 24 April 2015 at 06:18, Przemyslaw Marczak <p.marczak@samsung.com> wrote: >> Hello Simon, >> >> >> On 04/24/2015 06:51 AM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marczak@samsung.com> >>> wrote: >>>> >>>> Hello Simon, >>>> >>>> >>>> On 04/22/2015 06:30 PM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Przemyslaw, >>>>> >>>>> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> >>>>> 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 <p.marczak@samsung.com> >>>>>> --- >>>>>> 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 <sjg@chromium.org> >>>>> >>>>> 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 <id> - show or [set] operating regulator >>>>>> device >>>>>> + - regulator info - print constraints info >>>>>> + - regulator status - print operating status >>>>>> + - regulator value <val] <-f> - print/[set] voltage value [uV] >>>>>> + - regulator current <val> - print/[set] current value [uA] >>>>>> + - regulator mode <id> - 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 <p.marczak@samsung.com> >>>>>> + * >>>>>> + * SPDX-License-Identifier: GPL-2.0+ >>>>>> + */ >>>>>> +#include <common.h> >>>>>> +#include <errno.h> >>>>>> +#include <dm.h> >>>>>> +#include <dm/uclass-internal.h> >>>>>> +#include <power/regulator.h> >>>>>> + >>>>>> +#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. >>> >>> >>> Odd, there seem to be two blank lines there, and we only need one. >>> >> >> Ah, sorry. You mean, that there should be added a blank line. >> Ok, will add one. >> >>>> >>>>> >>>>> 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. >>> >>> >>> git grep -i failed |wc -l >>> 2089 >>> >>> Is there some way to know it is a PMIC error message, and find it that >>> way? >>> >> >> Ok, I assumed that you know which command you called, and where to find it, >> so you could use: >> grep -i "failed" common/cmd_regulator.c | wc -l >> 15 >> >> But this was only the function name, not a useful text for grep. >> Now I see that this should use the printf instead of the helper funcion. >> >>>> >>>>>> + 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. >>> >>> >>> But you can use req_seq, or if you have platform data, check that. >>> >> >> Ok, we could use the req_seq for the PMIC uclass, it's natural that >> interface, has its address and <reg> property - but this can repeat, >> if we have two PMICs on a different busses. This is probably possible. >> >> We also shouldn't set the req_seq as the number found in node name, because >> those numbers can repeat: ldo1 {}; buck1 {}; regulator1 { }. >> >> I think that, using the req_seq is bad idea, since we can't be sure that >> those values are unique. >> >> I understand that, the probe is not ideal here? But from the other side, >> if we call "pmic list", then we are sure, that listed devices are ready to >> use. Shouldn't we expect this? > > I was hoping that we would not probe devices until they are actually > used, and that listing them would not constitute 'use'. > > In the case of listing, you should not need to worry about ->seq or > ->req_seq. If you avoid 'getting' the device you will not probe it. Yes I know, that I can use the uclass_find_first/next_device() functions here. But only after moving the "regulator dev" command to getting the regulator by it's "name" constraint as will do in the fixup patches. > > In the case of getting a device ready for use, yes, it must be probed. > But I am only commenting on your 'list' function. > Yes this is clean for me. I'm only wonder now, what to do with the "pmic list/dev" commands. Actually, for the multi interface PMIC IC, we can be sure, that for each interface device will have a different name (dev->name). But even if the nodes are inside a different parent bus nodes, and have the same names, we probably could assume, that each PMIC's interface has a different address. To be sure we could put some note into the documentation, that for the PMICs, each node name should be unique. Then I can use: - uclass_find_first/next_device() for listing PMIC devices - uclass_get_device_by_name() for getting the required PMIC Is that correct, for you? [snip] Best regards,
Hi Przemyslaw, On 24 April 2015 at 06:53, Przemyslaw Marczak <p.marczak@samsung.com> wrote: > Hello Simon, > > > On 04/24/2015 02:34 PM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 24 April 2015 at 06:18, Przemyslaw Marczak <p.marczak@samsung.com> >> wrote: >>> >>> Hello Simon, >>> >>> >>> On 04/24/2015 06:51 AM, Simon Glass wrote: >>>> >>>> >>>> Hi Przemyslaw, >>>> >>>> On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marczak@samsung.com> >>>> wrote: >>>>> >>>>> >>>>> Hello Simon, >>>>> >>>>> >>>>> On 04/22/2015 06:30 PM, Simon Glass wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Przemyslaw, >>>>>> >>>>>> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> >>>>>> 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 <p.marczak@samsung.com> >>>>>>> --- >>>>>>> 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 <sjg@chromium.org> >>>>>> >>>>>> 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 <id> - show or [set] operating regulator >>>>>>> device >>>>>>> + - regulator info - print constraints info >>>>>>> + - regulator status - print operating status >>>>>>> + - regulator value <val] <-f> - print/[set] voltage value >>>>>>> [uV] >>>>>>> + - regulator current <val> - print/[set] current value >>>>>>> [uA] >>>>>>> + - regulator mode <id> - 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 <p.marczak@samsung.com> >>>>>>> + * >>>>>>> + * SPDX-License-Identifier: GPL-2.0+ >>>>>>> + */ >>>>>>> +#include <common.h> >>>>>>> +#include <errno.h> >>>>>>> +#include <dm.h> >>>>>>> +#include <dm/uclass-internal.h> >>>>>>> +#include <power/regulator.h> >>>>>>> + >>>>>>> +#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. >>>> >>>> >>>> >>>> Odd, there seem to be two blank lines there, and we only need one. >>>> >>> >>> Ah, sorry. You mean, that there should be added a blank line. >>> Ok, will add one. >>> >>>>> >>>>>> >>>>>> 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. >>>> >>>> >>>> >>>> git grep -i failed |wc -l >>>> 2089 >>>> >>>> Is there some way to know it is a PMIC error message, and find it that >>>> way? >>>> >>> >>> Ok, I assumed that you know which command you called, and where to find >>> it, >>> so you could use: >>> grep -i "failed" common/cmd_regulator.c | wc -l >>> 15 >>> >>> But this was only the function name, not a useful text for grep. >>> Now I see that this should use the printf instead of the helper funcion. >>> >>>>> >>>>>>> + 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. >>>> >>>> >>>> >>>> But you can use req_seq, or if you have platform data, check that. >>>> >>> >>> Ok, we could use the req_seq for the PMIC uclass, it's natural that >>> interface, has its address and <reg> property - but this can repeat, >>> if we have two PMICs on a different busses. This is probably possible. >>> >>> We also shouldn't set the req_seq as the number found in node name, >>> because >>> those numbers can repeat: ldo1 {}; buck1 {}; regulator1 { }. >>> >>> I think that, using the req_seq is bad idea, since we can't be sure that >>> those values are unique. >>> >>> I understand that, the probe is not ideal here? But from the other side, >>> if we call "pmic list", then we are sure, that listed devices are ready >>> to >>> use. Shouldn't we expect this? >> >> >> I was hoping that we would not probe devices until they are actually >> used, and that listing them would not constitute 'use'. >> >> In the case of listing, you should not need to worry about ->seq or >> ->req_seq. If you avoid 'getting' the device you will not probe it. > > > Yes I know, that I can use the uclass_find_first/next_device() functions > here. But only after moving the "regulator dev" command to getting the > regulator by it's "name" constraint as will do in the fixup patches. > >> >> In the case of getting a device ready for use, yes, it must be probed. >> But I am only commenting on your 'list' function. >> > > Yes this is clean for me. > > I'm only wonder now, what to do with the "pmic list/dev" commands. > > Actually, for the multi interface PMIC IC, we can be sure, that for each > interface device will have a different name (dev->name). > > But even if the nodes are inside a different parent bus nodes, and have the > same names, we probably could assume, that each PMIC's interface has a > different address. > To be sure we could put some note into the documentation, that for the > PMICs, each node name should be unique. > > Then I can use: > - uclass_find_first/next_device() for listing PMIC devices > - uclass_get_device_by_name() for getting the required PMIC > > Is that correct, for you? Yes, I think that is good. It will just confuse everyone if we try to handle two PMICs with the same name (or two regulators for that matter). Adding a note to the doc sounds good. Regards, Simon
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 <id> - show or [set] operating regulator device + - regulator info - print constraints info + - regulator status - print operating status + - regulator value <val] <-f> - print/[set] voltage value [uV] + - regulator current <val> - print/[set] current value [uA] + - regulator mode <id> - 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 <p.marczak@samsung.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#include <common.h> +#include <errno.h> +#include <dm.h> +#include <dm/uclass-internal.h> +#include <power/regulator.h> + +#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)); + return CMD_RET_FAILURE; +} + +static int regulator_get(bool list_only, int get_seq, struct udevice **devp) +{ + 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)) { + 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, + 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" +);
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 <p.marczak@samsung.com> --- 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