Hi Przemyslaw,
On 3 April 2015 at 10:11, Przemyslaw Marczak p.marczak@samsung.com wrote:
Hello Simon,
On 03/29/2015 03:05 PM, Simon Glass wrote:
Hi Przemyslaw,
On 24 March 2015 at 14:30, Przemyslaw Marczak p.marczak@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.
- 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.
Right, those compatibles don't point to a driver. This is a specific case of use from the kernel.
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.
Yes, I know how it's done. But we haven't the compatibles for each GPIO as for the regulators, and each GPIO driver, bind the childs by its own implementation.
I tried to reuse the existing code. It was nice, but I missed one thing...if there are more, than one driver with the same e.g. "LDO1" compatible, then the first one will bind - and it could be the wrong one...
But, it could be tune-up, to get the right drivers list by arg.
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,
Ok, this could be good.
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.
Yes, it's easy. I made something like this in the first version of this patchset, to parse the nodes and fill the max77686 regulator descriptors in function get_desc_for_compat(), from this patch: http://lists.denx.de/pipermail/u-boot/2014-October/191024.html
Yes I see.
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.
Yes, this is an advantage.
I wonder about the usage of "struct udevice_id", which should contain the data, defined only by the driver, right?
In the method you mentioned, we bind the pmic childs and then we modify the "dev->of_id->data" by putting the regulator number from matched prefix in it.
This is ok, but I think, that the second part of this idea should be done by the driver. I think, that the external function shouldn't modify this driver data on bind.
This could be solved by leave this job for the device driver. For example on max77686, we could leave the dev->of_id as null and use dev->priv for the node prefix id.
So, it's not a big problem.
As I see it the regulator number (e.g. 1 for LDO1, 3 for BUCK3) is picked up from the name string, so it feels like this is common code. You would be calling a helper function from the driver, so yes you can always have it return the regulator number, and then store it in driver_data in the driver.
But you should rebase on dm/usb-working to get the new driver_data member of struct udevice.
My only concern with using driver_data is that this is normally used to specify the device type where the driver supports multiple hardware variants. The regulator number is more correctly considered to be platform data. This leads on to your point below.
Here, I would like mention the problem with the regulator_get() by name, for which we want use the "regulator-name" constraint.
For this version, it requires probing of all regulator devices, since it uses the dev->uclass_priv, which I think, is a good place to provide the framework-specific structure type for regulator constraints and mode descriptors.
What about moving it back to dev->platdata? Then:
- the structure type: "dm_regulator_info", will move to
"dm_regulator_platdata"
- only its .name field could be assigned at regulator bind stage from the
"regulator-name" constraint
the bind could fail, when the constraint name not found
the rest of constraints will assign in .ofdata_to_platdata() function call
as it is at the present version
Then, we also don't need probe each regulator device, when using regulator_get() function.
Here is my understanding on this one:
The regulator name is the device name, so looking up by name should be easy. It just involves scanning through the regulator uclass comparing device names.
Based on what you said above, the regulator number should go in platform data for the regulator as you say. It can be worked out when the PMIC is bound (perhaps in the PMIC uclass' post_bind method).
The idea is that the PMIC is responsible for binding its children so that at start-up we know about all the regulators and can find them, even through none is probed yet.
- Should we put the regulator stuff in drivers/regulator, as with Linux?
I will do this in the next version.
- Can you please bring in the regulator and pmic device tree binding
files, plus max77686?
Right, I forgot about this.
- 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
Ok, I will add sandbox drivers for this all, but first let's prepare the final patchset version of this framework, and then I will send a separate patchset for sandbox before merge of this part.
OK sounds good.
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
Thank you again for the review! I will send the next version probably on 7-8-th April.
Regards, Simon