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

[Simon Glass]

Hi Przemyslaw,

On 6 March 2015 at 08:08, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> 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 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.
>
>
> 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.

Sounds good! It's a lot of work but it will be nice to have all this done.

Regards,
Simon