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

Wed Mar 25 17:09:12 CET 2015

Hello Simon,

On 03/10/2015 03:12 AM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 6 March 2015 at 07:10, Simon Glass <sjg at chromium.org> wrote:
>> Hi Przemyslaw,
>>
>> On 3 March 2015 at 09:24, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
>>> Hello,
>>> Here is the second RFC version of the new PMIC framework.
>>> The changes made in this version are described below each commit.
>>>
>>> So again, a quick summary of:
>>> Framework:
>>> - Add new uclass types:
>>>   -- UCLASS_PMIC(for device I/O)
>>>   -- UCLASS_PMIC_REGULATOR (for common regulator ops)
>>> - Two uclass drivers for the above types
>>> - A common regulator operations - will easy cover the real devices design
>>> - V2: pmic: add read/write ops
>>> - V2: regulator: use regulator type as an argument - not as function name
>>>
>>>
>>> Drivers:
>>> - Introduce new PMIC API for drivers - now everything base on "struct udevice"
>>> - Introduce Regulator Voltage descriptors and Operation Mode descriptors
>>>    which are usually taken from the device tree (board dependent data)
>>> - Two uclass device drivers for MAX77686(PMIC+REGULATOR)
>>> - V2: don't use the 'hw union' from old pmic
>>> - V2: remove the files: pmic_i2c.c/pmic_spi.c - now using bus drivers
>>> - V2: cleanup the pmic_get() functions
>>> - V2: add pmic_io_dev() function for getting the proper I/O dev for devices
>>> - V2: add function calls for getting pmic devices platdata
>>> - V2: remove regulator type from regulator operations function calls,
>>>        use type as an argument
>>>
>>> User Interface:
>>> - command pmic, unchanged functionality and ported to the driver model
>>> - command regulator(NEW) for safe regulator setup from commandline,
>>>    - now can check output Voltage and operation mode of the regulators,
>>>    - also can check the board Voltage limits and driver available modes
>>> - V2: simplify the code after remove the regulator type from function naming
>>> - V2: add on/off command
>>>
>>> Supported boards:
>>> - Odroid U3
>>> - V2: drop the commits for Trats2 - wait for charger and muic uclass types
>>>
>>> The assumptions of this work is:
>>> - Add new code to independent files
>>> - Keep two Frameworks as independent and without conflicts
>>> - Don't mix OLD/NEW Framework code - for the readability
>>>
>>> The future plans:
>>> - Add additional uclass types: MUIC, CHARGER, BATTERY, MFD and maybe more.
>>> - Port all U-Boot drivers to the new Framework
>>> - Remove the old drivers and the old PMIC Framework code
>>>
>>> Need help:
>>> - After merge this, it is welcome to help with driver porting
>>> - Every new driver should be tested on real hardware
>>>
>>> Best regards
>>>
>>> Przemyslaw Marczak (12):
>>>    exynos5: fix build break by adding CONFIG_POWER
>>>    dm: device: add function device_get_first_child_by_uclass_id()
>>>    dm: pmic: add implementation of driver model pmic uclass
>>>    dm: pmic: add implementation of driver model regulator uclass
>>>    dm: pmic: new commands: pmic and regulator
>>>    dm: pmic: add max77686 pmic driver
>>>    dm: regulator: add max77686 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 ++++++++-
>>>   board/samsung/common/board.c           |   4 +-
>>>   board/samsung/common/misc.c            |   1 +
>>>   board/samsung/odroid/odroid.c          |  52 +-
>>>   configs/odroid_defconfig               |   1 -
>>>   doc/driver-model/dm-pmic-framework.txt | 367 +++++++++++++
>>>   drivers/core/device.c                  |  15 +
>>>   drivers/power/Makefile                 |   5 +-
>>>   drivers/power/cmd_pmic.c               | 820 +++++++++++++++++++++++++++++
>>>   drivers/power/pmic-uclass.c            | 191 +++++++
>>>   drivers/power/pmic/Makefile            |   1 +
>>>   drivers/power/pmic/max77686.c          | 102 ++++
>>>   drivers/power/pmic/pmic_max77686.c     |   2 +-
>>>   drivers/power/regulator-uclass.c       | 227 ++++++++
>>>   drivers/power/regulator/Makefile       |   8 +
>>>   drivers/power/regulator/max77686.c     | 926 +++++++++++++++++++++++++++++++++
>>>   include/configs/exynos5-common.h       |   4 +
>>>   include/configs/odroid.h               |   9 +-
>>>   include/dm/device.h                    |  16 +
>>>   include/dm/uclass-id.h                 |   4 +
>>>   include/power/max77686_pmic.h          |  26 +-
>>>   include/power/pmic.h                   | 265 ++++++++++
>>>   include/power/regulator.h              | 310 +++++++++++
>>>   24 files changed, 3573 insertions(+), 33 deletions(-)
>>>   create mode 100644 doc/driver-model/dm-pmic-framework.txt
>>>   create mode 100644 drivers/power/cmd_pmic.c
>>>   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/max77686.c
>>>   create mode 100644 include/power/regulator.h
>>
>> This is an impressive pieces of work! It will be great to get these in.
>>
>> Here are some high-level comments on this series:
>>

So, just as a summary of V3 rework.

>> 1. I think regulator LDOs and bucks should be proper devices (struct
>> udevice), bound by the pmic when it is probed. The advantages are
>>
>> a. You can see them in the device list - the pmic will end up with a
>> lot more children
This is done.

>> b. You can use the same regulator uclass for each, but have different
>> operations for each driver (e.g. max77686 might provide two different
>> drivers, one for LDO, one for buck).
This is done.

>> c. Things like your 'switch (type)' in max7786_get_state() etc. will go away
This is done.

>> d. You can perform operations on them without having to specify their
>> parent and number - e.g. regulator_set_mode(struct udevice *ldo, int
>> mode) which is much more natural for users
This is done.

>> e. You avoid needing your own list of regulators and bucks - struct
>> max7786_regulator_info. After all, keeping track of child devices is
>> something that driver model can do
This is done. Now, each regulator device keeps the common regulator 
constraints as uclass private data (dev->uclass_priv) and the additional 
constraints for the fixed-regulator (gpio constraints) are kept as the 
device private data (dev->priv).

>>
>> 2. I see device tree support, but the Linux device tree bindings are
>> not fully supported, e.g. the regulators sub-node uses
>> regulator-compatible instead of regulator-name. I think it should be
>> exactly the same (and we should copy the device tree files, only
>> leaving out what we don't support)
This is done. Since the compatible is stored in a different property 
than just 'compatible', I added the function to bind devices by custom 
compatible property name. So regulators are bind by 
'regulator-compatible' property compatibles

>>
>> 3. The I2C/SPI difference is a bit clunky. We should try to hide this
>> away. The most obvious problem is in getting the device. Instead of
>> pmic_i2c_get() we should use the "power-supply" property in the device
>> tree, so we need a function which can find the regulator given the
>> device node (a bit like gpio_request_by_name() but for PMICs). The
>> pmic_get() function is OK and will be needed also, as I am sure we
>> will use names in some places. We should remove any mention of the bus
>> type from the API I think. Also regulator number seems and odd concept
>> - better to use the name/device tree link to find the right device.
This is done partially. My time was limited. The bus type and address is 
now removed from the api. I think that the 'pmic_get()' by the name 
should be ok, when we have different device-tree names for each pmic 
intarface.
 From the other side this is not so needed, since we are going to 
provide the device features by driver-model uclass drivers.
This will take some time, so I add some extended function for getting 
the pmic in the next version.
The 'power-supply' seem to be right for all devices, I will work on it 
in the next version.

>> One way to avoid I2C/SPI problems is to create a helper file which
>> allows you to read and write registers given a struct udevice. It
>> could look at whether the device is I2C or SPI and do the right thing.
>> This could be generally useful, not just for PMICs.
Yes, this would be useful, and actually will make something like the 
relationship for the pmic device and it's regulator child.

>>
>> 4. Should use Kconfig now.
This is done.

>
> Sorry I don't know how I forgot to mention this:
>
> 5. A test PMIC device for sandbox so that the tests can run (test/dm/pmic.c).
This one, will be task for the future.

>
> Regards,
> Simon
>

Thank you again for the helpful suggestions!

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