Re: [U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Hi Przemyslaw,
On 3 April 2015 at 10:11, Przemyslaw Marczak wrote:
> Hello Simon,
>
> On 03/29/2015 03:05 PM, Simon Glass wrote:
>>
>> Hi Przemyslaw,
>>
>> On 24 March 2015 at 14:30, Przemyslaw Marczak
>> 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
Re: [U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Hello Simon,
On 03/29/2015 03:05 PM, Simon Glass wrote:
Hi Przemyslaw,
On 24 March 2015 at 14:30, Przemyslaw Marczak 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
Re: [U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Hi Przemyslaw,
On 24 March 2015 at 14:30, Przemyslaw Marczak 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/powe
Re: [U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
Hello, On 03/24/2015 09:30 PM, Przemyslaw Marczak 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. 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 This patchset is available at: https://github.com/bobenstein/u-boot branch: dm-pmic-v3 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
[U-Boot] [PATCH v3 00/17] Power(full) framework based on Driver Model
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. 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
