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

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

Hello Simon,

On 03/06/2015 03:10 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 3 March 2015 at 09:24, Przemyslaw Marczak <p.marczak@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.

Thank you, and also thank you for the review.
I hope to finish this all successfully.

>
> Here are some high-level comments on this series:
>
> 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
> 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).
> c. Things like your 'switch (type)' in max7786_get_state() etc. will go away
> 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
> 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
>
> 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)
>
> 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.
> 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.
>
> 4. Should use Kconfig now.
>
> Regards,
> Simon
>

I quickly read your all comments, and all are very helpful for me, I 
will reply to each in the next week.

I see that this piece of code should be done as e.g. the gpio uclass is.

My previous assumption was, to use the regulator numbers rather than 
names - as it is easy and faster - cause we have one driver instance for 
few regulators - but I agree, need change.

Usually there are numbers in the pmic documentation, and the names are 
in the board schematics which uses both names and numbers.
So I added getting the names from dts just as some useful info for the 
regulator command.

I agree with you, that using the names instead of the numbers allow to 
make some things more easy, e.g. getting the 'udevice' by name.

I made some rework of the soft i2c with dm support, it looks that it is 
working fine. I will need this for work on next pmic devices in my 
Trats2 (charger, muic and fuelgauge).

Probably, I will send the dm soft i2c as independent patch set, and then 
get back to this framework code.

Thank you and best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com