[U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Simon Glass
sjg at chromium.org
Sun Mar 29 15:05:30 CEST 2015
Hi Przemyslaw,
On 24 March 2015 at 14:30, Przemyslaw Marczak <p.marczak at 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.
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.
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,
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.
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.
2. Should we put the regulator stuff in drivers/regulator, as with Linux?
3. Can you please bring in the regulator and pmic device tree binding
files, plus max77686?
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
>
> 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
>
More information about the U-Boot
mailing list