From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Fri, 24 Apr 2015 07:00:15 -0600
Subject: [U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command
In-Reply-To: <553A3CB8.2060603@samsung.com>
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>
 <CAPnjgZ2a5fAt7X=cb=HPUXwS-PxRk=wRVvCFUrtMDyT+dyRjSg@mail.gmail.com>
 <5538D884.1070605@samsung.com>
 <CAPnjgZ3UxxysjJqKszHAKYCCJTanjOSa8dNAP64qXSd_yPqzQQ@mail.gmail.com>
 <553A348E.7090001@samsung.com>
 <CAPnjgZ1gNPLFnFEM8qvWatBn=5uPr9gJ=y7AMyzN-koVL7OK5g@mail.gmail.com>
 <553A3CB8.2060603@samsung.com>
Message-ID: <CAPnjgZ0xXat4MMyNOTDko2UzV=y=QWC9E__uBXhwyA4nX9Z+hw@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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