Hi Przemyslaw,
On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marc...@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.marc...@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.marc...@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 <s...@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.marc...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
