[U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Simon Glass
sjg at chromium.org
Sun Apr 5 20:30:34 CEST 2015
Hi Przemyslaw,
On 3 April 2015 at 10:11, Przemyslaw Marczak <p.marczak at 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 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.
>>
>
> 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.
>
>>
>> 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.
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
More information about the U-Boot
mailing list