Hello Simon,
On 03/29/2015 03:05 PM, Simon Glass wrote:
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.
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
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.
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.
2. Should we put the regulator stuff in drivers/regulator, as with Linux?
I will do this in the next version.
3. Can you please bring in the regulator and pmic device tree binding
files, plus max77686?
Right, I forgot about this.
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
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.
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.
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
