[U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command

[Przemyslaw Marczak <p.marczak@samsung.com>]

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:
>>>>
>>>> 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.marczak at samsung.com