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:
- 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