[U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command
Simon Glass
sjg at chromium.org
Fri Apr 24 15:00:15 CEST 2015
Hi Przemyslaw,
On 24 April 2015 at 06:53, Przemyslaw Marczak <p.marczak at 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 at 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 at 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 at 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 at 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 at 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 at 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?
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
More information about the U-Boot
mailing list