[U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model

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

Hello Simon,

On 03/29/2015 03:05 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 24 March 2015 at 14:30, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> Hello,
>> Here is the third RFC version of the new PMIC framework.Big thanks to
>> Simon Glass, your comments were really helpful, and I think, that this
>> version is much more better to discuss, than the previous. The changes
>> made in this version are described below each commit. Sorry that I didn't
>> reply to each patch, I agreed with most and just started the work.
>
> This is looking really good. Here are a few overall comments.
>
> 1. There is one oddity that I'd like to address before merging.
>
> I don't think the fdt_node_check_prop_compatible() is a good idea, nor
> necessary. I don't think we should consider the regulator-compatible
> property to be a compatible string. It has values like LDO8, LDO9 and
> these don't look like compatible strings, which are normally unique
> and point to a driver. Here they point to a particular device.
>

Right, those compatibles don't point to a driver. This is a specific 
case of use from the kernel.

> A similar problem is faced in pinctrl and if you look at
> gpio_exynos_bind() you will see that it works through the sub-nodes,
> creating devices as needed.
>
> I don't think using udevice_id is right here either.
>

Yes, I know how it's done. But we haven't the compatibles for each GPIO 
as for the regulators, and each GPIO driver, bind the childs by its own 
implementation.

I tried to reuse the existing code. It was nice, but I missed one 
thing...if there are more, than one driver with the same e.g. "LDO1" 
compatible, then the first one will bind - and it could be the wrong one...

But, it could be tune-up, to get the right drivers list by arg.

> Here is my suggestion:
>
> a. Create a new structure like this:
>
> struct pmic_child_info {
>     const char *prefix;   // "LDO" or "BUCK"
>     const char *driver_name;   // "max77686_ldo" or "max77686_buck"
> };
>
> b. Pass a list of these to pmic_child_node_scan(). In your case there
> will be three entries, one for LDO, one for BUCK, plus a NULL
> termination entry,
>

Ok, this could be good.

> c. It can work through the subnodes looking for the given prefixes. It
> then calls device_bind_driver() on each. Then it changes the returned
> device's of_data to hold the correct value (obtained with strtol() on
> the part of the name that follows the prefix - e.g. 17 for "LDO17").
> This will be easier if you rebase on u-boot-dm/usb-working, where the
> data is just a long, not a device tree pointer.

Yes, it's easy. I made something like this in the first version of this 
patchset, to parse the nodes and fill the max77686 regulator descriptors 
in function get_desc_for_compat(), from this patch:
http://lists.denx.de/pipermail/u-boot/2014-October/191024.html

>
> d. Now you have the same effect as before, but you can drop the tables
> like max77686_ldo_ids[] and avoid misappropriating driver model's
> device lookup.

Yes, this is an advantage.

I wonder about the usage of "struct udevice_id", which should contain 
the data, defined only by the driver, right?

In the method you mentioned, we bind the pmic childs and then
we modify the "dev->of_id->data" by putting the regulator number from 
matched prefix in it.

This is ok, but I think, that the second part of this idea should be 
done by the driver. I think, that the external function shouldn't modify 
this driver data on bind.

This could be solved by leave this job for the device driver. For 
example on max77686, we could leave the dev->of_id as null and use 
dev->priv for the node prefix id.

So, it's not a big problem.

Here, I would like mention the problem with the regulator_get() by name, 
for which we want use the "regulator-name" constraint.

For this version, it requires probing of all regulator devices,
since it uses the dev->uclass_priv, which I think, is a good place to 
provide the framework-specific structure type for regulator constraints 
and mode descriptors.

What about moving it back to dev->platdata?
Then:
- the structure type: "dm_regulator_info", will move to 
"dm_regulator_platdata"

- only its .name field could be assigned at regulator bind stage from 
the "regulator-name" constraint

- the bind could fail, when the constraint name not found

- the rest of constraints will assign in .ofdata_to_platdata() function 
call as it is at the present version

Then, we also don't need probe each regulator device, when using 
regulator_get() function.

>
> 2. Should we put the regulator stuff in drivers/regulator, as with Linux?

I will do this in the next version.

>
> 3. Can you please bring in the regulator and pmic device tree binding
> files, plus max77686?

Right, I forgot about this.

>
> 4. We really do need tests! I suspect that you could create a sandbox
> I2C pmic that has a few registers and regulators. See
> i2c_eeprom_emul.c for a basic example. Then you can write some tests
> that find the pmi,c find the regulator, read and write a few
> registers, and read and write a few regulators. That would be enough
> test coverage to get us started. I know this is different from
> previous U-Boot policy, but tests are a big win during development and
> also for years to come (as people can change the framework and have
> some confidence that they did not break anything).
>
> It can be a follow-on patch separate from your series but I'm really
> not keen on bringing in a major new driver model framework with no
> tests. If you are struggling for time, let me know and I can try to
> help by writing a sandbox I2C PMIC for example.
>
> Regards,
> Simon
>

Ok, I will add sandbox drivers for this all, but first let's prepare the 
final patchset version of this framework, and then I will send a 
separate patchset for sandbox before merge of this part.

>>
>> Best regards
>>
>> Przemyslaw Marczak (17):
>>    exynos5: fix build break by adding CONFIG_POWER
>>    fdt_ro.c: add new function: fdt_node_check_prop_compatible()
>>    dm: core: lists.c: add new function lists_bind_fdt_by_prop()
>>    lib: Kconfig: add entry for errno_str() function
>>    dm: pmic: add implementation of driver model pmic uclass
>>    dm: regulator: add implementation of driver model regulator uclass
>>    dm: pmic: add pmic command
>>    dm: regulator: add regulator command
>>    pmic: max77686 set the same compatible as in the kernel
>>    dm: pmic: add max77686 pmic driver
>>    dm: regulator: add max77686 regulator driver
>>    dm: regulator: add fixed voltage regulator driver
>>    doc: driver-model: pmic and regulator uclass documentation
>>    dm: board:samsung: power_init_board: add requirement of CONFIG_DM_PMIC
>>    odroid: board: add support to dm pmic api
>>    odroid: dts: add 'voltage-regulators' description to max77686 node
>>    odroid: config: enable dm pmic, dm regulator and max77686 driver
>>
>>   Makefile                             |   1 +
>>   arch/arm/dts/exynos4412-odroid.dts   | 249 +++++++++-
>>   arch/arm/dts/exynos4412-trats2.dts   |   2 +-
>>   arch/arm/dts/exynos5250-smdk5250.dts |   2 +-
>>   arch/arm/dts/exynos5250-snow.dts     |   2 +-
>>   board/samsung/common/board.c         |   4 +-
>>   board/samsung/common/misc.c          |   1 +
>>   board/samsung/odroid/odroid.c        | 113 ++++-
>>   common/Kconfig                       |  36 ++
>>   common/Makefile                      |   4 +
>>   common/cmd_pmic.c                    | 210 +++++++++
>>   common/cmd_regulator.c               | 385 +++++++++++++++
>>   configs/odroid_defconfig             |   8 +-
>>   doc/driver-model/pmic-framework.txt  | 350 ++++++++++++++
>>   drivers/core/lists.c                 |  28 +-
>>   drivers/power/Kconfig                | 124 ++++-
>>   drivers/power/Makefile               |   3 +-
>>   drivers/power/pmic-uclass.c          | 130 ++++++
>>   drivers/power/pmic/Makefile          |   1 +
>>   drivers/power/pmic/max77686.c        |  76 +++
>>   drivers/power/pmic/pmic_max77686.c   |   2 +-
>>   drivers/power/regulator-uclass.c     | 219 +++++++++
>>   drivers/power/regulator/Makefile     |   9 +
>>   drivers/power/regulator/fixed.c      | 124 +++++
>>   drivers/power/regulator/max77686.c   | 876 +++++++++++++++++++++++++++++++++++
>>   include/configs/exynos5-common.h     |   4 +
>>   include/configs/odroid.h             |   5 -
>>   include/dm/lists.h                   |  18 +
>>   include/dm/uclass-id.h               |   4 +
>>   include/libfdt.h                     |  27 ++
>>   include/power/max77686_pmic.h        |  26 +-
>>   include/power/pmic.h                 | 210 +++++++++
>>   include/power/regulator.h            | 259 +++++++++++
>>   lib/Kconfig                          |   8 +
>>   lib/fdtdec.c                         |   2 +-
>>   lib/libfdt/fdt_ro.c                  |  14 +-
>>   36 files changed, 3481 insertions(+), 55 deletions(-)
>>   create mode 100644 common/cmd_pmic.c
>>   create mode 100644 common/cmd_regulator.c
>>   create mode 100644 doc/driver-model/pmic-framework.txt
>>   create mode 100644 drivers/power/pmic-uclass.c
>>   create mode 100644 drivers/power/pmic/max77686.c
>>   create mode 100644 drivers/power/regulator-uclass.c
>>   create mode 100644 drivers/power/regulator/Makefile
>>   create mode 100644 drivers/power/regulator/fixed.c
>>   create mode 100644 drivers/power/regulator/max77686.c
>>   create mode 100644 include/power/regulator.h
>>
>> --
>> 1.9.1
>>
>

Thank you again for the review!
I will send the next version probably on 7-8-th April.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com