Hello Simon,
On 04/24/2015 02:34 PM, Simon Glass wrote:
Hi Przemyslaw,
On 24 April 2015 at 06:18, Przemyslaw Marczak <p.marc...@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.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.
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,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
