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_REGULATORhelpThis command is based on driver model regulator's API.User interface features:- list - list regulator devices- regulator dev <id> - show or [set] operating regulatordevice
- 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 outputThe '-f' (force) option can be used for set the value whichexceeds
the limits, which are found in device-tree and are kept inregulator'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:
- 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.
- 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;elsereturn -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);elseprintf(" %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]);elseforce = 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 constraintlimits\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 regulatordevice\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