From: Stefan Wahren <hidden> Date: 2014-09-07 11:38:31
This patch series draft should adds support for Freescale i.MX23, i.MX28
on-chip regulators. I'm new to regulator drivers and need early feedback for
the driver's stage before i go in the wrong direction.
The information about the i.MX28 regulators are from chapter 11 of the
reference manual [1].
This driver based on the Freescale high level [2] and low level driver [3],
but contains the following changes:
* devictree support
* code cleanup
Please bear in mind that the code is not tested against real hardware and
it's not expected to work.
I'm not sure if it's correct, but these series based on the for-next branch
of Mark Brown's regulator repository.
Any comments about the code are welcome.
[1] - http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
[2] - http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/regulator/mxs-regulator.c?h=imx_2.6.35_maintain
[3] - http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx28/power.c?h=imx_2.6.35_maintain
Stefan Wahren (3):
DT: add binding for MXS regulator
ARM: regulator: add Freescale MXS regulator driver
DT: ARM: mxs: enable regulator support for i.MX28
.../bindings/regulator/mxs-regulator.txt | 38 ++
arch/arm/boot/dts/imx28.dtsi | 50 ++-
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile | 1 +
drivers/regulator/mxs-regulator.c | 411 +++++++++++++++++++++
5 files changed, 508 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
create mode 100644 drivers/regulator/mxs-regulator.c
--
1.8.1.4
@@ -0,0 +1,38 @@+MXS regulators++Required node properties:+- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device++Required regulator properties:+- compatible: Must be "fsl,mxs-regulator"+- reg: Offset of the register set for the regulator+- mxs-max-reg-val: Maximum value of this register++Optional regulator properties:+- mxs-default-microvolt: initial voltage of the regulator++Any regulator property defined as part of the core regulator+binding, defined in regulator.txt, can also be used.++Example:++ power: power at 80044000 {+ compatible = "fsl,imx28-power";+ #address-cells = <1>;+ #size-cells = <0>;+ reg = <0x80044000 0x2000>;++ reg_vddio: regulator-vddio at 80044060 {+ reg = <0x80044060>;+ compatible = "fsl,mxs-regulator";+ regulator-name = "vddio";+ regulator-min-microvolt = <2800000>;+ regulator-max-microvolt = <3600000>;+ regulator-microvolt-offset = <80000>;+ regulator-always-on;+ mxs-max-reg-val = <0x10>;+ mxs-default-microvolt = <3300000>;+ };+ };+
@@ -0,0 +1,38 @@+MXS regulators++Required node properties:+- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device++Required regulator properties:+- compatible: Must be "fsl,mxs-regulator"+- reg: Offset of the register set for the regulator+- mxs-max-reg-val: Maximum value of this register++Optional regulator properties:+- mxs-default-microvolt: initial voltage of the regulator
Vendor specific property names should be prefixed with the vendor name and
comma.
WBR, Sergei
@@ -0,0 +1,38 @@+MXS regulators++Required node properties:+- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device
We require #address-cells and #size-cells if the child nodes have reg
entries.
+
+Required regulator properties:
+- compatible: Must be "fsl,mxs-regulator"
+- reg: Offset of the register set for the regulator
Is this the offset or the absolute physical address?
The example seems to be absolute.
+- mxs-max-reg-val: Maximum value of this register
What does this even mean? What's the format?
Is this not implied by standard properties like regulator-max-microvolt?
+Optional regulator properties:
+- mxs-default-microvolt: initial voltage of the regulator
Why is this necessary?
Mark.
+
+Any regulator property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+ power: power at 80044000 {
+ compatible = "fsl,imx28-power";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x80044000 0x2000>;
+
+ reg_vddio: regulator-vddio at 80044060 {
+ reg = <0x80044060>;
+ compatible = "fsl,mxs-regulator";
+ regulator-name = "vddio";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <3600000>;
+ regulator-microvolt-offset = <80000>;
+ regulator-always-on;
+ mxs-max-reg-val = <0x10>;
+ mxs-default-microvolt = <3300000>;
+ };
+ };
+
--
1.8.1.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -0,0 +1,38 @@+MXS regulators++Required node properties:+- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"+- reg: Offset and length of the register set for the device
We require #address-cells and #size-cells if the child nodes have reg
entries.
okay.
quoted
+
+Required regulator properties:
+- compatible: Must be "fsl,mxs-regulator"
+- reg: Offset of the register set for the regulator
Is this the offset or the absolute physical address?
The example seems to be absolute.
You are right, the description is wrong.
quoted
+- mxs-max-reg-val: Maximum value of this register
What does this even mean? What's the format?
Is this not implied by standard properties like regulator-max-microvolt?
Unfortunately not as long the step size in microvolt isn't defined. The
parameter defines the register value in hex corresponding to
regulator-max-microvolt.
step size = (regulator-max-microvolt - regulator-min-microvolt) /
mxs-max-reg-val
May be i missed or understand something wrong?
quoted
+Optional regulator properties:
+- mxs-default-microvolt: initial voltage of the regulator
Why is this necessary?
The original driver from Freescale set the MX28 regulator vddio to 3.38
V during registering. I think that is ugly to implement in the driver
part, so i add a more general property in the devicetree binding.
From: Stefan Wahren <hidden> Date: 2014-09-07 11:38:30
This patch enables the regulator support for i.MX28.
Signed-off-by: Stefan Wahren <redacted>
---
arch/arm/boot/dts/imx28.dtsi | 50 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 49 insertions(+), 1 deletion(-)
As I mentioned in my comments on the binding, I'd like to know why this
is necessary and if it is why it shouldn't be a standardised property.
From my understanding the standardised properties only defines a range,
but no default state of the regulators. If the initialization from the
bootloader or a hardcoded initialization in the driver is okay then the
property is not necessary.
Wouldn't it make more sense to explicitly match the names you expect?
Okay, i make "regulator-name" a required property and use a white list
of all possible regulators.
I'm not sure if regulator-name is the right way to go. I believe the
thing to do is match on the node name of the subnodes (as you were doing
here).
The only thing I didn't like was assuming the nodes were voltage
regulators if they weren't called "overall_current". I'd prefer explicit
matching, or something more general.
[...]
As I mentioned in my comments on the binding, I'd like to know why this
is necessary and if it is why it shouldn't be a standardised property.
From my understanding the standardised properties only defines a range,
but no default state of the regulators. If the initialization from the
bootloader or a hardcoded initialization in the driver is okay then the
property is not necessary.
Sure. My questions was why it is necessary to preconfigure the
regulators at all rather than why it is necessary to do so in this
manner.
Mark.
From: Mark Brown <broonie@kernel.org> Date: 2014-09-10 15:14:06
On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.
I'm not keen on using of_get_property here. We have no idea if
regulator-name is even a string (it should be, but we have no
guarantee).
quoted
Better using of_property_read_string?
Yes. That will check the value is NUL-terminated, at least.
Or just remove the property entirely... without having seen the
bindings if we're specifying the name of the device via the device tree
something seems wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140910/de331256/attachment-0001.sig>
From: Stefan Wahren <hidden> Date: 2014-09-10 17:10:56
Hi Mark,
Am 10.09.2014 17:13, schrieb Mark Brown:
On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
quoted
On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.
sorry about the confusion, i will remove ARM in the next version.
Changelog? I didn't send a changelog because it was my first version.
Should i resend this version only to you?
I'm not keen on using of_get_property here. We have no idea if
regulator-name is even a string (it should be, but we have no
guarantee).
quoted
quoted
Better using of_property_read_string?
quoted
Yes. That will check the value is NUL-terminated, at least.
Or just remove the property entirely... without having seen the
bindings if we're specifying the name of the device via the device tree
something seems wrong.
Hi Stefan,
On Wed, Sep 10, 2014 at 2:32 PM, Stefan Wahren [off-list ref] wrote:
Hi Mark,
Am 10.09.2014 17:13, schrieb Mark Brown:
quoted
On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
quoted
On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.
sorry about the confusion, i will remove ARM in the next version.
Changelog? I didn't send a changelog because it was my first version.
Should i resend this version only to you?
In the cover letter of this RFC series you mentioned that this has not
been tested on real hardware.
What about sending a new version of this series (with the RFC prefix
removed and with Mark Rutland's suggestion implemented) tested on a
mx28 board and also with the Subject of the regulator patch changed to
'regulator: add support for mxs regulator" with Mark Brown on Cc?
From: Stefan Wahren <hidden> Date: 2014-09-11 06:30:19
Hi Fabio,
Am 10.09.2014 20:54, schrieb Fabio Estevam:
Hi Stefan,
On Wed, Sep 10, 2014 at 2:32 PM, Stefan Wahren [off-list ref] wrote:
quoted
Hi Mark,
Am 10.09.2014 17:13, schrieb Mark Brown:
quoted
On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
quoted
On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.
sorry about the confusion, i will remove ARM in the next version.
Changelog? I didn't send a changelog because it was my first version.
Should i resend this version only to you?
In the cover letter of this RFC series you mentioned that this has not
been tested on real hardware.
What about sending a new version of this series (with the RFC prefix
removed and with Mark Rutland's suggestion implemented) tested on a
mx28 board and also with the Subject of the regulator patch changed to
'regulator: add support for mxs regulator" with Mark Brown on Cc?
that's the same idea i had about the first real version of my patch.
Unfortunately i can do the porting only in my spare time. So i try to
avoid with the RFC series the situation, that i spend many days in
development and testing, but after it the regulator guys says it went in
the complete wrong direction. The advice about the anatop regulator was
helpful, but there is still some unsureness.
I will try to get a Duckbill, so i can do testing at home.
Thanks
Stefan
From: Mark Brown <broonie@kernel.org> Date: 2014-09-10 19:51:33
On Wed, Sep 10, 2014 at 07:32:36PM +0200, Stefan Wahren wrote:
Am 10.09.2014 17:13, schrieb Mark Brown:
quoted
Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.
sorry about the confusion, i will remove ARM in the next version.
Changelog? I didn't send a changelog because it was my first version.
The changelog is the text describing the change you are making, not
something describing differences between revisions of the patch set.
As I mentioned in my comments on the binding, I'd like to know why this
is necessary and if it is why it shouldn't be a standardised property.
From my understanding the standardised properties only defines a range,
but no default state of the regulators. If the initialization from the
bootloader or a hardcoded initialization in the driver is okay then the
property is not necessary.
Sure. My questions was why it is necessary to preconfigure the
regulators at all rather than why it is necessary to do so in this
manner.
Mark.
sorry i don't have a clue. In the original code there isn't a comment
about the reason. Currently there is no init of the vddio regulator by
the kernel and everything works fine.
@Fabio: Do you have any doubts?
Stefan
On Wed, Sep 10, 2014 at 2:24 PM, Stefan Wahren [off-list ref] wrote:
sorry i don't have a clue. In the original code there isn't a comment about
the reason. Currently there is no init of the vddio regulator by the kernel
and everything works fine.
@Fabio: Do you have any doubts?
I would say, just use and report whatever value comes from the
bootloader. No need to force a particular regulator output.