[U-Boot] [PATCH v4 07/16] dm: regulator: add regulator command
Przemyslaw Marczak
p.marczak at samsung.com
Fri Apr 24 14:53:12 CEST 2015
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?
[snip]
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
More information about the U-Boot
mailing list