Hi Przemyslaw, On 24 March 2015 at 14:30, Przemyslaw Marczak <p.marc...@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 > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot