From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Fri, 24 Apr 2015 14:53:12 +0200 Subject: [U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command In-Reply-To: References: <1427229051-20170-1-git-send-email-p.marczak@samsung.com> <1429553273-6453-1-git-send-email-p.marczak@samsung.com> <1429553273-6453-8-git-send-email-p.marczak@samsung.com> <5538D884.1070605@samsung.com> <553A348E.7090001@samsung.com> Message-ID: <553A3CB8.2060603@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 04/24/2015 02:34 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 24 April 2015 at 06:18, Przemyslaw Marczak wrote: >> Hello Simon, >> >> >> On 04/24/2015 06:51 AM, Simon Glass wrote: >>> >>> Hi Przemyslaw, >>> >>> On 23 April 2015 at 05:33, Przemyslaw Marczak >>> wrote: >>>> >>>> Hello Simon, >>>> >>>> >>>> On 04/22/2015 06:30 PM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Przemyslaw, >>>>> >>>>> On 20 April 2015 at 12:07, Przemyslaw Marczak >>>>> 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 >>>>>> --- >>>>>> 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 >>>>> >>>>> 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 - show or [set] operating regulator >>>>>> device >>>>>> + - regulator info - print constraints info >>>>>> + - regulator status - print operating status >>>>>> + - regulator value - print/[set] voltage value [uV] >>>>>> + - regulator current - print/[set] current value [uA] >>>>>> + - regulator mode - 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 >>>>>> + * >>>>>> + * SPDX-License-Identifier: GPL-2.0+ >>>>>> + */ >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +#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 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.marczak at samsung.com