[PATCH 0/2] regulator: add support for mxs regulator

[PATCH 0/2] regulator: add support for mxs regulator

[Stefan Wahren]

This patch series adds support for Freescale i.MX23, i.xM28
on-chip regulators: vddd, vdda, vddio

This driver based on the Freescale high level [1] and low level 
driver [2], but contains the following changes:

* devicetree support
* fix for regulator modes
* drop support for overall_current and brown out
* replace udelay() with schedule()
* code cleanup

The code has been tested on a I2SE Duckbill.

[1] - 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
[2] - 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 (2):
  DT: add binding for mxs regulator
  regulator: add mxs regulator driver

 .../bindings/regulator/mxs-regulator.txt           |   36 ++
 drivers/regulator/Kconfig                          |   11 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/mxs-regulator.c                  |  395 ++++++++++++++++++++
 4 files changed, 443 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
 create mode 100644 drivers/regulator/mxs-regulator.c

-- 
1.7.9.5

[PATCH 1/2] DT: add binding for mxs regulator

[Stefan Wahren]

This patch adds the Device tree bindings for the Freescale MXS
on-chip regulators.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../bindings/regulator/mxs-regulator.txt           |   36 ++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
new file mode 100644
index 0000000..e3133a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
@@ -0,0 +1,36 @@
+MXS regulators
+
+Required node properties:
+- compatible: Should be "simple-bus"
+- #address-cells: Number of cells required to define regulator register,
+  must be 1
+- #size-cells: Number of cells required to define register size, must be 1
+- reg: Absolute physical address and size of the register set for the device
+
+Required regulator properties:
+- compatible: Must be "fsl,mxs-regulator"
+- reg: Absolute physical address of the register set for the regulator
+
+Any regulator property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+	power: power@80044000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80044000 0x2000>;
+		ranges;
+
+		reg_vddd: regulator@80044040 {
+			reg = <0x80044040 0x10>;
+			compatible = "fsl,mxs-regulator";
+			regulator-name = "vddd";
+			regulator-min-microvolt = <800000>;
+			regulator-max-microvolt = <1575000>;
+			regulator-boot-on;
+			vddd-supply = <&reg_vdda>;
+		};
+	};
+
-- 
1.7.9.5

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Sat, Sep 27, 2014 at 12:59:47AM +0000, Stefan Wahren wrote:
> This patch adds the Device tree bindings for the Freescale MXS
> on-chip regulators.

Use subject lines matching the style for the subsystem.

> +Required regulator properties:
> +- compatible: Must be "fsl,mxs-regulator"
> +- reg: Absolute physical address of the register set for the regulator
> +
> +Any regulator property defined as part of the core regulator
> +binding, defined in regulator.txt, can also be used.

While this should be using compatibles to identify which regulator is
being supported note that the binding doesn't document the fact that the
code makes regulator-name mandatory or what values are required.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Stefan Wahren]

Hi Mark,

Am 28.09.2014 um 12:22 schrieb Mark Brown:
> On Sat, Sep 27, 2014 at 12:59:47AM +0000, Stefan Wahren wrote:
>> This patch adds the Device tree bindings for the Freescale MXS
>> on-chip regulators.
> Use subject lines matching the style for the subsystem.

sorry i'm not sure what's wrong with the subject lines.

Did you expect "[PATCH 1/2] regulator: add binding for mxs regulator"?

>
>> +Required regulator properties:
>> +- compatible: Must be "fsl,mxs-regulator"
>> +- reg: Absolute physical address of the register set for the regulator
>> +
>> +Any regulator property defined as part of the core regulator
>> +binding, defined in regulator.txt, can also be used.
> While this should be using compatibles to identify which regulator is
> being supported note that the binding doesn't document the fact that the
> code makes regulator-name mandatory or what values are required.

Is the following better?

- fsl,mxs-regulator-vddd
- fsl,mxs-regulator-vdda
- fsl,mxs-regulator-vddio

Thanks

Stefan

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On Mon, Sep 29, 2014 at 08:00:50AM +0200, Stefan Wahren wrote:
> Am 28.09.2014 um 12:22 schrieb Mark Brown:

> > Use subject lines matching the style for the subsystem.

> sorry i'm not sure what's wrong with the subject lines.

> Did you expect "[PATCH 1/2] regulator: add binding for mxs regulator"?

Yes.

> > While this should be using compatibles to identify which regulator is
> > being supported note that the binding doesn't document the fact that the
> > code makes regulator-name mandatory or what values are required.

> Is the following better?

> - fsl,mxs-regulator-vddd
> - fsl,mxs-regulator-vdda
> - fsl,mxs-regulator-vddio

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Mark Rutland]

On Sat, Sep 27, 2014 at 01:59:47AM +0100, Stefan Wahren wrote:
> This patch adds the Device tree bindings for the Freescale MXS
> on-chip regulators.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  .../bindings/regulator/mxs-regulator.txt           |   36 ++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> new file mode 100644
> index 0000000..e3133a4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> @@ -0,0 +1,36 @@
> +MXS regulators
> +
> +Required node properties:
> +- compatible: Should be "simple-bus"

This does not look like an appropriate use of simple-bus.

Why do you want the parent node to be a simple-bus?

> +- #address-cells: Number of cells required to define regulator register,
> +  must be 1
> +- #size-cells: Number of cells required to define register size, must be 1

Why must this be the case, given that the child node expects an absolute
physical address?

What's wrong with #address-cells = <2>, for example?

> +- reg: Absolute physical address and size of the register set for the device

Why is this here _and_ in the child node(s)?

What is the difference between this node and its children?

Can there be more than one sub-node?

Mark.

> +
> +Required regulator properties:
> +- compatible: Must be "fsl,mxs-regulator"
> +- reg: Absolute physical address of the register set for the regulator
> +
> +Any regulator property defined as part of the core regulator
> +binding, defined in regulator.txt, can also be used.
> +
> +Example:
> +
> +	power: power@80044000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80044000 0x2000>;
> +		ranges;
> +
> +		reg_vddd: regulator@80044040 {
> +			reg = <0x80044040 0x10>;
> +			compatible = "fsl,mxs-regulator";
> +			regulator-name = "vddd";
> +			regulator-min-microvolt = <800000>;
> +			regulator-max-microvolt = <1575000>;
> +			regulator-boot-on;
> +			vddd-supply = <&reg_vdda>;
> +		};
> +	};
> +
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Stefan Wahren]

Hi Mark,

Am 29.09.2014 um 13:09 schrieb Mark Rutland:
> On Sat, Sep 27, 2014 at 01:59:47AM +0100, Stefan Wahren wrote:
>> This patch adds the Device tree bindings for the Freescale MXS
>> on-chip regulators.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  .../bindings/regulator/mxs-regulator.txt           |   36 ++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>> new file mode 100644
>> index 0000000..e3133a4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>> @@ -0,0 +1,36 @@
>> +MXS regulators
>> +
>> +Required node properties:
>> +- compatible: Should be "simple-bus"
> This does not look like an appropriate use of simple-bus.
>
> Why do you want the parent node to be a simple-bus?

the current parent node in imx28.dtsi looks like a placeholder for the
power sub system:

power: power@80044000 {
	reg = <0x80044000 0x2000>;
	status = "disabled";
};

I want to trigger the probing of the child nodes (regulators) without
writing a driver for the complete power sub system. The simple-bus
avoids that.

Do we need a extra driver?

>
>> +- #address-cells: Number of cells required to define regulator register,
>> +  must be 1
>> +- #size-cells: Number of cells required to define register size, must be 1
> Why must this be the case, given that the child node expects an absolute
> physical address?

I need a property to define the control register for the regulators
without defining vendor specific properties like "fsl,mxs-control-reg"
or something.

> What's wrong with #address-cells = <2>, for example?

Nothing

>
>> +- reg: Absolute physical address and size of the register set for the device
> Why is this here _and_ in the child node(s)?

The parent of the power node is also a simple bus. I use this to
calculate the power status register per offset.

> What is the difference between this node and its children?

The parent node represent the power sub system and the regulators are
part of this sub system.

> Can there be more than one sub-node?

In the i.MX28 are at least 4 voltage regulators, 1 current regulator and
many more. At first, the driver should implement only 3 voltage
regulators (vddd, vdda, vddio).

> Mark.
>

Best regards

Stefan

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Mark Rutland]

On Mon, Sep 29, 2014 at 12:53:22PM +0100, Stefan Wahren wrote:
> Hi Mark,
> 
> Am 29.09.2014 um 13:09 schrieb Mark Rutland:
> > On Sat, Sep 27, 2014 at 01:59:47AM +0100, Stefan Wahren wrote:
> >> This patch adds the Device tree bindings for the Freescale MXS
> >> on-chip regulators.
> >>
> >> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >> ---
> >>  .../bindings/regulator/mxs-regulator.txt           |   36 ++++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> >> new file mode 100644
> >> index 0000000..e3133a4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> >> @@ -0,0 +1,36 @@
> >> +MXS regulators
> >> +
> >> +Required node properties:
> >> +- compatible: Should be "simple-bus"
> > This does not look like an appropriate use of simple-bus.
> >
> > Why do you want the parent node to be a simple-bus?
> 
> the current parent node in imx28.dtsi looks like a placeholder for the
> power sub system:
> 
> power: power@80044000 {
> 	reg = <0x80044000 0x2000>;
> 	status = "disabled";
> };
> 
> I want to trigger the probing of the child nodes (regulators) without
> writing a driver for the complete power sub system. The simple-bus
> avoids that.

Well, the simple-bus will cause the children to be probed. But it looks
like you care about properties of the parent. I don't think that
simple-bus is appropriate because it's not being handled as a
transparent bridge from the PoV of the children.

> 
> Do we need a extra driver?

Perhaps, but it's relatively simple to match on a compatible string and
probe children if you just wantto start small for now.

> >
> >> +- #address-cells: Number of cells required to define regulator register,
> >> +  must be 1
> >> +- #size-cells: Number of cells required to define register size, must be 1
> > Why must this be the case, given that the child node expects an absolute
> > physical address?
> 
> I need a property to define the control register for the regulators
> without defining vendor specific properties like "fsl,mxs-control-reg"
> or something.

You misunderstand me. I was querying the "must be 1" rather than the
proeprties themselves.

> 
> > What's wrong with #address-cells = <2>, for example?
> 
> Nothing

Then we shouldn't specify "must be 1", no?

> 
> >
> >> +- reg: Absolute physical address and size of the register set for the device
> > Why is this here _and_ in the child node(s)?
> 
> The parent of the power node is also a simple bus. I use this to
> calculate the power status register per offset.
> 
> > What is the difference between this node and its children?
> 
> The parent node represent the power sub system and the regulators are
> part of this sub system.
> 
> > Can there be more than one sub-node?
> 
> In the i.MX28 are at least 4 voltage regulators, 1 current regulator and
> many more. At first, the driver should implement only 3 voltage
> regulators (vddd, vdda, vddio).

Ok.

I think you need a binding for the power subsystem, and a trivial driver
that can match on that and probe the child regulators. Are there
components other than voltage or current regulators in the sub system?

Mark.

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Stefan Wahren]

Hi,

Am 29.09.2014 um 14:41 schrieb Mark Rutland:
> Well, the simple-bus will cause the children to be probed. But it looks
> like you care about properties of the parent. I don't think that
> simple-bus is appropriate because it's not being handled as a
> transparent bridge from the PoV of the children.

actually i need the address of the power status register. In this
version i get the base address from the parent node add an offset.

Do you prefer to define the address of the power status register like a
second address cell:

reg_vddd: regulator@80044040 {
	reg = <0x80044040 0x10
	       0x800440c0 0x01>;
        ...
};

or do i need special properties like this:

reg_vddd: regulator@80044040 {
	reg = <0x80044040 0x10>;
	fsl,mxs-status-reg = <0x800440c0>;
        ...
};

>> Do we need a extra driver?
> Perhaps, but it's relatively simple to match on a compatible string and
> probe children if you just wantto start small for now.

Okay. Would be great if someone has a good example. At first, i thought
of power/anatop.

>
>>>> +- #address-cells: Number of cells required to define regulator register,
>>>> +  must be 1
>>>> +- #size-cells: Number of cells required to define register size, must be 1
>>> Why must this be the case, given that the child node expects an absolute
>>> physical address?
>> I need a property to define the control register for the regulators
>> without defining vendor specific properties like "fsl,mxs-control-reg"
>> or something.
> You misunderstand me. I was querying the "must be 1" rather than the
> proeprties themselves.
>
>>> What's wrong with #address-cells = <2>, for example?
>> Nothing
> Then we shouldn't specify "must be 1", no?

Right, must be at least 1.

>>>> +- reg: Absolute physical address and size of the register set for the device
>>> Why is this here _and_ in the child node(s)?
>> The parent of the power node is also a simple bus. I use this to
>> calculate the power status register per offset.
>>
>>> What is the difference between this node and its children?
>> The parent node represent the power sub system and the regulators are
>> part of this sub system.
>>
>>> Can there be more than one sub-node?
>> In the i.MX28 are at least 4 voltage regulators, 1 current regulator and
>> many more. At first, the driver should implement only 3 voltage
>> regulators (vddd, vdda, vddio).
> Ok.
>
> I think you need a binding for the power subsystem, and a trivial driver
> that can match on that and probe the child regulators. Are there
> components other than voltage or current regulators in the sub system?

Yes, according to the reference manual there is a dc-dc converter, a
battery charger, battery monitor, ...

In short a lot of developing time ;-)

> Mark.

Best regards

Stefan

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Mark Rutland]

On Mon, Sep 29, 2014 at 02:10:07PM +0100, Stefan Wahren wrote:
> Hi,
> 
> Am 29.09.2014 um 14:41 schrieb Mark Rutland:
> > Well, the simple-bus will cause the children to be probed. But it looks
> > like you care about properties of the parent. I don't think that
> > simple-bus is appropriate because it's not being handled as a
> > transparent bridge from the PoV of the children.
> 
> actually i need the address of the power status register. In this
> version i get the base address from the parent node add an offset.
> 
> Do you prefer to define the address of the power status register like a
> second address cell:
> 
> reg_vddd: regulator@80044040 {
> 	reg = <0x80044040 0x10
> 	       0x800440c0 0x01>;
>         ...
> };
> 
> or do i need special properties like this:
> 
> reg_vddd: regulator@80044040 {
> 	reg = <0x80044040 0x10>;
> 	fsl,mxs-status-reg = <0x800440c0>;
>         ...
> };

I would prefer a top level node for the subsystem that is not a
simple-bus.

Give it a compatible string and a well-defined set of base properties
(looks like you just need the reg for now). Match that and probe the
child nodes as appropriate.

> >> Do we need a extra driver?
> > Perhaps, but it's relatively simple to match on a compatible string and
> > probe children if you just wantto start small for now.
> 
> Okay. Would be great if someone has a good example. At first, i thought
> of power/anatop.

While I believe there are examples in the kernel, I can't think
immediately of any instances.

> >>>> +- #address-cells: Number of cells required to define regulator register,
> >>>> +  must be 1
> >>>> +- #size-cells: Number of cells required to define register size, must be 1
> >>> Why must this be the case, given that the child node expects an absolute
> >>> physical address?
> >> I need a property to define the control register for the regulators
> >> without defining vendor specific properties like "fsl,mxs-control-reg"
> >> or something.
> > You misunderstand me. I was querying the "must be 1" rather than the
> > proeprties themselves.
> >
> >>> What's wrong with #address-cells = <2>, for example?
> >> Nothing
> > Then we shouldn't specify "must be 1", no?
> 
> Right, must be at least 1.

Why not just say that #address-cells, #size-cells and ranges must be
present as appropriate to map children?

> 
> >>>> +- reg: Absolute physical address and size of the register set for the device
> >>> Why is this here _and_ in the child node(s)?
> >> The parent of the power node is also a simple bus. I use this to
> >> calculate the power status register per offset.
> >>
> >>> What is the difference between this node and its children?
> >> The parent node represent the power sub system and the regulators are
> >> part of this sub system.
> >>
> >>> Can there be more than one sub-node?
> >> In the i.MX28 are at least 4 voltage regulators, 1 current regulator and
> >> many more. At first, the driver should implement only 3 voltage
> >> regulators (vddd, vdda, vddio).
> > Ok.
> >
> > I think you need a binding for the power subsystem, and a trivial driver
> > that can match on that and probe the child regulators. Are there
> > components other than voltage or current regulators in the sub system?
> 
> Yes, according to the reference manual there is a dc-dc converter, a
> battery charger, battery monitor, ...
> 
> In short a lot of developing time ;-)

Sure, but not everything needs to be supported fomr the outset.

Mark.

Re: [PATCH 1/2] DT: add binding for mxs regulator

[Stefan Wahren]

Hi Mark,
hi Fabio,

> Mark Rutland <mark.rutland@arm.com> hat am 29. September 2014 um 15:23
> geschrieben:
>
> I would prefer a top level node for the subsystem that is not a
> simple-bus.
>
> Give it a compatible string and a well-defined set of base properties
> (looks like you just need the reg for now). Match that and probe the
> child nodes as appropriate.
>
> > >> Do we need a extra driver?
> > > Perhaps, but it's relatively simple to match on a compatible string and
> > > probe children if you just wantto start small for now.
>
> Mark.

i would name the driver for the power subsystem "mxs_power.c" and use the
compatibel string "fsl,imx28-power" for i.MX28.

Now the question: where should i take?

arch/arm/mach-mxs/ or drivers/power/

Regards

Stefan

[PATCH 2/2] regulator: add mxs regulator driver

[Stefan Wahren]

This patch adds driver support for Freescale i.MX23, i.MX28
on-chip regulators. The driver supports the following regulators:
vddd, vdda and vddio.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/regulator/Kconfig         |   11 ++
 drivers/regulator/Makefile        |    1 +
 drivers/regulator/mxs-regulator.c |  395 +++++++++++++++++++++++++++++++++++++
 3 files changed, 407 insertions(+)
 create mode 100644 drivers/regulator/mxs-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index a353011..f353a2b 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -433,6 +433,17 @@ config REGULATOR_MC13892
 	  Say y here to support the regulators found on the Freescale MC13892
 	  PMIC.
 
+config REGULATOR_MXS
+	tristate "Freescale MXS on-chip regulators"
+	depends on ARCH_MXS
+	default y if ARCH_MXS
+	help
+	  Say y here to support Freescale MXS on-chip regulators.
+
+	  The driver currently support the following voltage regulators:
+          vddd, vdda and vddio. It is recommended that this option be
+	  enabled on i.MX23, i.MX28 platform.
+
 config REGULATOR_PALMAS
 	tristate "TI Palmas PMIC Regulators"
 	depends on MFD_PALMAS
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 25271f8..0f5b66c 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_REGULATOR_MAX77802) += max77802.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
+obj-$(CONFIG_REGULATOR_MXS) += mxs-regulator.o
 obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
 obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
 obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
diff --git a/drivers/regulator/mxs-regulator.c b/drivers/regulator/mxs-regulator.c
new file mode 100644
index 0000000..63c09cc
--- /dev/null
+++ b/drivers/regulator/mxs-regulator.c
@@ -0,0 +1,395 @@
+/*
+ * Freescale STMP378X voltage regulators
+ *
+ * Embedded Alley Solutions, Inc <source@embeddedalley.com>
+ *
+ * Copyright (C) 2014 Stefan Wahren
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
+ * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+#define HW_POWER_STS			(0x000000c0)
+
+#define BM_POWER_STS_DC_OK		BIT(9)
+
+#define MXS_VDDIO	1
+#define MXS_VDDA	2
+#define MXS_VDDD	3
+
+struct mxs_regulator {
+	struct regulator_desc rdesc;
+	struct regulator_init_data *initdata;
+
+	const char *name;
+	void __iomem *base_addr;
+	void __iomem *power_addr;
+	unsigned int mode_mask;
+};
+
+static int mxs_set_voltage(struct regulator_dev *reg, int min_uV, int max_uV,
+			   unsigned *selector)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	struct regulation_constraints *con = &sreg->initdata->constraints;
+	void __iomem *power_sts = sreg->power_addr + HW_POWER_STS;
+	unsigned long start;
+	u32 val, regs, i;
+
+	pr_debug("%s: min_uV %d, max_uV %d, min %d, max %d\n", __func__,
+		 min_uV, max_uV, con->min_uV, con->max_uV);
+
+	if (max_uV < con->min_uV || max_uV > con->max_uV)
+		return -EINVAL;
+
+	val = (max_uV - con->min_uV) * sreg->rdesc.n_voltages /
+			(con->max_uV - con->min_uV);
+
+	regs = (readl(sreg->base_addr) & ~sreg->rdesc.vsel_mask);
+
+	pr_debug("%s: %s calculated val %d\n", __func__, sreg->name, val);
+
+	writel(val | regs, sreg->base_addr);
+	for (i = 20; i; i--) {
+		/* delay for fast mode */
+		if (readl(power_sts) & BM_POWER_STS_DC_OK)
+			return 0;
+
+		udelay(1);
+	}
+
+	writel(val | regs, sreg->base_addr);
+	start = jiffies;
+	while (1) {
+		/* delay for normal mode */
+		if (readl(power_sts) & BM_POWER_STS_DC_OK)
+			return 0;
+
+		if (time_after(jiffies, start +	msecs_to_jiffies(80)))
+			break;
+
+		schedule();
+	}
+
+	return -ETIMEDOUT;
+}
+
+
+static int mxs_get_voltage(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	struct regulation_constraints *con = &sreg->initdata->constraints;
+	int uV;
+	u32 val = readl(sreg->base_addr) & sreg->rdesc.vsel_mask;
+
+	pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
+
+	if (val > sreg->rdesc.n_voltages)
+		val = sreg->rdesc.n_voltages;
+
+	uV = con->min_uV + val *
+		(con->max_uV - con->min_uV) / sreg->rdesc.n_voltages;
+
+	return uV;
+}
+
+static int mxs_is_enabled(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	u32 val;
+
+	if (sreg->rdesc.id == MXS_VDDIO)
+		return 1;
+
+	val = readl(sreg->base_addr) & sreg->rdesc.enable_mask;
+
+	pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
+
+	switch (sreg->rdesc.id) {
+	case MXS_VDDA:
+		val >>= 16;
+		break;
+	case MXS_VDDD:
+		val >>= 20;
+		break;
+	}
+
+	return val ? 1 : 0;
+}
+
+static int mxs_set_mode(struct regulator_dev *reg, unsigned int mode)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	int ret = 0;
+	u32 val;
+
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		val = readl(sreg->base_addr);
+		/* Disable stepping */
+		writel(val | sreg->mode_mask, sreg->base_addr);
+		break;
+
+	case REGULATOR_MODE_NORMAL:
+		val = readl(sreg->base_addr);
+		/* Enable stepping */
+		writel(val & ~sreg->mode_mask, sreg->base_addr);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static unsigned int mxs_get_mode(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	u32 val = readl(sreg->base_addr) & sreg->mode_mask;
+
+	return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
+}
+
+static struct regulator_ops mxs_rops = {
+	.set_voltage	= mxs_set_voltage,
+	.get_voltage	= mxs_get_voltage,
+	.set_mode	= mxs_set_mode,
+	.get_mode	= mxs_get_mode,
+};
+
+static struct regulator_desc mxs_reg_desc[] = {
+	{
+		.name = "vddio",
+		.id = MXS_VDDIO,
+		.type = REGULATOR_VOLTAGE,
+		.n_voltages = 0x10,
+		.uV_step = 50000,
+		.linear_min_sel = 0,
+		.vsel_mask = 0x1f,
+		.enable_mask = 0,
+	},
+	{
+		.name = "vdda",
+		.id = MXS_VDDA,
+		.type = REGULATOR_VOLTAGE,
+		.n_voltages = 0x1f,
+		.uV_step = 25000,
+		.linear_min_sel = 0,
+		.vsel_mask = 0x1f,
+		.enable_mask = (3 << 16),
+	},
+	{
+		.name = "vddd",
+		.id = MXS_VDDD,
+		.type = REGULATOR_VOLTAGE,
+		.n_voltages = 0x1f,
+		.uV_step = 25000,
+		.linear_min_sel = 0,
+		.vsel_mask = 0x1f,
+		.enable_mask = (3 << 20),
+	},
+};
+
+static int mxs_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *parent;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev = NULL;
+	struct mxs_regulator *sreg;
+	struct regulator_init_data *initdata = NULL;
+	struct regulation_constraints *con;
+	struct regulator_config config = { };
+	void __iomem *base_addr = NULL;
+	void __iomem *power_addr = NULL;
+	int ret = 0;
+	const char *name;
+	unsigned int i;
+
+	if (!np) {
+		dev_err(dev, "missing device tree\n");
+		return -EINVAL;
+	}
+
+	initdata = of_get_regulator_init_data(dev, np);
+	if (!initdata) {
+		dev_err(dev, "missing regulator init data\n");
+		return -EINVAL;
+	}
+
+	if (of_property_read_string(np, "regulator-name", &name)) {
+		dev_err(dev, "missing property regulator-name\n");
+		return -EINVAL;
+	}
+
+	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
+	if (!sreg)
+		return -EINVAL;
+
+	sreg->initdata = initdata;
+	rdesc = &sreg->rdesc;
+	memset(rdesc, 0, sizeof(*rdesc));
+
+	for (i = 0; i < ARRAY_SIZE(mxs_reg_desc); i++) {
+		if (!strcmp(mxs_reg_desc[i].name, name))
+			break;
+	}
+
+	if (i >= ARRAY_SIZE(mxs_reg_desc)) {
+		dev_err(dev, "unknown regulator %s\n", name);
+		return -EINVAL;
+	}
+
+	sreg->name = name;
+	rdesc->name = sreg->name;
+	rdesc->owner = THIS_MODULE;
+	rdesc->id = mxs_reg_desc[i].id;
+	rdesc->type = REGULATOR_VOLTAGE;
+	rdesc->linear_min_sel = mxs_reg_desc[i].linear_min_sel;
+	rdesc->n_voltages = mxs_reg_desc[i].n_voltages;
+	rdesc->uV_step = mxs_reg_desc[i].uV_step;
+	rdesc->vsel_mask = mxs_reg_desc[i].vsel_mask;
+	rdesc->enable_mask = mxs_reg_desc[i].enable_mask;
+	rdesc->enable_is_inverted = mxs_reg_desc[i].enable_is_inverted;
+	rdesc->ops = &mxs_rops;
+
+	switch (sreg->rdesc.id) {
+	case MXS_VDDIO:
+		sreg->mode_mask = BIT(17);
+		break;
+	case MXS_VDDA:
+		sreg->mode_mask = BIT(18);
+		break;
+	case MXS_VDDD:
+		sreg->mode_mask = BIT(22);
+		break;
+	}
+
+	/* get device base address */
+	base_addr = of_iomap(np, 0);
+	if (!base_addr) {
+		dev_err(dev, "unable to map base addr\n");
+		return -ENXIO;
+	}
+
+	parent = of_get_parent(np);
+	if (!parent) {
+		dev_err(dev, "unable to get power controller node\n");
+		ret = -ENXIO;
+		goto fail1;
+	}
+
+	/* get base address of power controller */
+	power_addr = of_iomap(parent, 0);
+	of_node_put(parent);
+	if (!power_addr) {
+		dev_err(dev, "unable to map power controller addr\n");
+		ret = -ENXIO;
+		goto fail1;
+	}
+
+	dev_info(dev, "%s found\n", name);
+
+	sreg->base_addr = base_addr;
+	sreg->power_addr = power_addr;
+
+	con = &initdata->constraints;
+
+	config.dev = &pdev->dev;
+	config.init_data = initdata;
+	config.driver_data = sreg;
+	config.of_node = np;
+
+	pr_debug("probing regulator %s\n", name);
+
+	rdev = devm_regulator_register(dev, rdesc, &config);
+
+	if (IS_ERR(rdev)) {
+		dev_err(dev, "failed to register %s\n", name);
+		ret = PTR_ERR(rdev);
+		goto fail2;
+	}
+
+	platform_set_drvdata(pdev, rdev);
+
+	return 0;
+fail2:
+	iounmap(power_addr);
+fail1:
+	iounmap(base_addr);
+
+	return ret;
+}
+
+static int mxs_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+	struct mxs_regulator *sreg = rdev_get_drvdata(rdev);
+	void __iomem *base_addr = sreg->base_addr;
+	void __iomem *power_addr = sreg->power_addr;
+
+	regulator_unregister(rdev);
+	iounmap(power_addr);
+	iounmap(base_addr);
+
+	return 0;
+}
+
+static const struct of_device_id of_mxs_regulator_match[] = {
+	{ .compatible = "fsl,mxs-regulator" },
+	{ /* end */ }
+};
+MODULE_DEVICE_TABLE(of, of_mxs_regulator_match);
+
+static struct platform_driver mxs_regulator_driver = {
+	.driver = {
+		.name	= "mxs_regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_mxs_regulator_match,
+	},
+	.probe	= mxs_regulator_probe,
+	.remove = mxs_regulator_remove,
+};
+
+static int __init mxs_regulator_init(void)
+{
+	return platform_driver_register(&mxs_regulator_driver);
+}
+postcore_initcall(mxs_regulator_init);
+
+static void __exit mxs_regulator_exit(void)
+{
+	platform_driver_unregister(&mxs_regulator_driver);
+}
+module_exit(mxs_regulator_exit);
+
+MODULE_AUTHOR("Embedded Alley Solutions <source@embeddedalley.com>");
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Freescale STMP378X voltage regulators");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:mxs_regulator");
-- 
1.7.9.5

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3109 bytes --]

On Sat, Sep 27, 2014 at 12:59:48AM +0000, Stefan Wahren wrote:

> +	pr_debug("%s: min_uV %d, max_uV %d, min %d, max %d\n", __func__,
> +		 min_uV, max_uV, con->min_uV, con->max_uV);
> +
> +	if (max_uV < con->min_uV || max_uV > con->max_uV)
> +		return -EINVAL;

This is replicating checks done by the core.

> +	val = (max_uV - con->min_uV) * sreg->rdesc.n_voltages /
> +			(con->max_uV - con->min_uV);

Drivers should never look at their constraints, they should let the core
do that and just do what they're asked.  In this case it is probably
best to implement a get_voltage_sel() operation and have the conversion
to voltage done by regulator_map_voltage_linear(), this will both make
the code look better and mean the driver gets the benefit of all the
error checking done by the core.

> +	writel(val | regs, sreg->base_addr);
> +	for (i = 20; i; i--) {
> +		/* delay for fast mode */
> +		if (readl(power_sts) & BM_POWER_STS_DC_OK)
> +			return 0;
> +
> +		udelay(1);
> +	}
> +
> +	writel(val | regs, sreg->base_addr);
> +	start = jiffies;
> +	while (1) {
> +		/* delay for normal mode */
> +		if (readl(power_sts) & BM_POWER_STS_DC_OK)
> +			return 0;

This really needs a comment to explain what on earth is going on here -
the whole thing with writing the same thing twice with two delays is
more than a little odd.  It looks like the driver is trying to busy wait
in cases where the change happens quickly but the comments about "fast"
and "normal" mode make this unclear.

> +	pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
> +
> +	switch (sreg->rdesc.id) {
> +	case MXS_VDDA:
> +		val >>= 16;
> +		break;
> +	case MXS_VDDD:
> +		val >>= 20;
> +		break;
> +	}
> +
> +	return val ? 1 : 0;
> +}

This seems buggy - it'll always return true for MXS_VDDD if MXS_VDDA
enabled won't it?

> +static unsigned int mxs_get_mode(struct regulator_dev *reg)
> +{
> +	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
> +	u32 val = readl(sreg->base_addr) & sreg->mode_mask;
> +
> +	return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
> +}

Please try to avoid the ternery operator, it does nothing for
legibility.

> +	if (of_property_read_string(np, "regulator-name", &name)) {
> +		dev_err(dev, "missing property regulator-name\n");
> +		return -EINVAL;
> +	}

Use different compatibles to identify the regulators, regulator-name
should never be mandatory.

> +	switch (sreg->rdesc.id) {
> +	case MXS_VDDIO:
> +		sreg->mode_mask = BIT(17);
> +		break;
> +	case MXS_VDDA:
> +		sreg->mode_mask = BIT(18);
> +		break;
> +	case MXS_VDDD:
> +		sreg->mode_mask = BIT(22);
> +		break;
> +	}

Why is this not looked up from the data structure like the rest of the
data?

> +	dev_info(dev, "%s found\n", name);

Don't add noise like this to the boot log, it provides no useful
information.

> +	regulator_unregister(rdev);
> +	iounmap(power_addr);
> +	iounmap(base_addr);

Use devm_ versions of functions.

> +static int __init mxs_regulator_init(void)
> +{
> +	return platform_driver_register(&mxs_regulator_driver);
> +}
> +postcore_initcall(mxs_regulator_init);

module_platform_driver().

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Stefan Wahren]

Hi Mark,

thanks for your comments. Now it looks to me, that i try to reinvent the
wheel.

I'm searching for a good regulator implementation example.

Does it apply to ti-abb-regulator.c and twl-regulator.c?

Am 28.09.2014 um 12:16 schrieb Mark Brown:
> On Sat, Sep 27, 2014 at 12:59:48AM +0000, Stefan Wahren wrote:
>
>> +	pr_debug("%s: min_uV %d, max_uV %d, min %d, max %d\n", __func__,
>> +		 min_uV, max_uV, con->min_uV, con->max_uV);
>> +
>> +	if (max_uV < con->min_uV || max_uV > con->max_uV)
>> +		return -EINVAL;
> This is replicating checks done by the core.
>
>> +	val = (max_uV - con->min_uV) * sreg->rdesc.n_voltages /
>> +			(con->max_uV - con->min_uV);
> Drivers should never look at their constraints, they should let the core
> do that and just do what they're asked.  In this case it is probably
> best to implement a get_voltage_sel() operation and have the conversion
> to voltage done by regulator_map_voltage_linear(), this will both make
> the code look better and mean the driver gets the benefit of all the
> error checking done by the core.

Okay, i will do that. For the list_voltage operation
regulator_list_voltage_linear()
should be the perfect candidate.

>> +	writel(val | regs, sreg->base_addr);
>> +	for (i = 20; i; i--) {
>> +		/* delay for fast mode */
>> +		if (readl(power_sts) & BM_POWER_STS_DC_OK)
>> +			return 0;
>> +
>> +		udelay(1);
>> +	}
>> +
>> +	writel(val | regs, sreg->base_addr);
>> +	start = jiffies;
>> +	while (1) {
>> +		/* delay for normal mode */
>> +		if (readl(power_sts) & BM_POWER_STS_DC_OK)
>> +			return 0;
> This really needs a comment to explain what on earth is going on here -
> the whole thing with writing the same thing twice with two delays is
> more than a little odd.  It looks like the driver is trying to busy wait
> in cases where the change happens quickly but the comments about "fast"
> and "normal" mode make this unclear.

The regulator driver polls for the DC_OK bit in the power status register.

Quote for reference manual (p. 935): "High when switching DC-DC
converter control loop has stabilized after a voltage target change."

The two loops comes from the different regulator modes
(REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).

In REGULATOR_MODE_FAST the voltage steping is disabled and changing
voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
enabled and it's take a while for reaching the target voltage.

Do you see more a problem with the two different loops or the redundant
register write?

Is it acceptable that the function blocks here?

>> +	pr_debug("%s: %s register val %d\n", __func__, sreg->name, val);
>> +
>> +	switch (sreg->rdesc.id) {
>> +	case MXS_VDDA:
>> +		val >>= 16;
>> +		break;
>> +	case MXS_VDDD:
>> +		val >>= 20;
>> +		break;
>> +	}
>> +
>> +	return val ? 1 : 0;
>> +}
> This seems buggy - it'll always return true for MXS_VDDD if MXS_VDDA
> enabled won't it?

Shame on me, i forgot to remove this function. mxs_is_enabled is never used.

>> +static unsigned int mxs_get_mode(struct regulator_dev *reg)
>> +{
>> +	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
>> +	u32 val = readl(sreg->base_addr) & sreg->mode_mask;
>> +
>> +	return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
>> +}
> Please try to avoid the ternery operator, it does nothing for
> legibility.

if (readl(sreg->base_addr) & sreg->mode_mask)
    return REGULATOR_MODE_FAST;

return REGULATOR_MODE_NORMAL;

Better?

>> +	if (of_property_read_string(np, "regulator-name", &name)) {
>> +		dev_err(dev, "missing property regulator-name\n");
>> +		return -EINVAL;
>> +	}
> Use different compatibles to identify the regulators, regulator-name
> should never be mandatory.

I will remove this and use the compatibles.

>> +	switch (sreg->rdesc.id) {
>> +	case MXS_VDDIO:
>> +		sreg->mode_mask = BIT(17);
>> +		break;
>> +	case MXS_VDDA:
>> +		sreg->mode_mask = BIT(18);
>> +		break;
>> +	case MXS_VDDD:
>> +		sreg->mode_mask = BIT(22);
>> +		break;
>> +	}
> Why is this not looked up from the data structure like the rest of the
> data?

I was a little bit confused why there wasn't a mode_mask in the struct
regulator_desc. I will do it like the ti-abb-regulator in the matching
table.

>
>> +	dev_info(dev, "%s found\n", name);
> Don't add noise like this to the boot log, it provides no useful
> information.

Okay, i will remove this.

>> +	regulator_unregister(rdev);
>> +	iounmap(power_addr);
>> +	iounmap(base_addr);
> Use devm_ versions of functions.

As a result the remove function for the drivers becomes unnecessary. Am
i right?

>
>> +static int __init mxs_regulator_init(void)
>> +{
>> +	return platform_driver_register(&mxs_regulator_driver);
>> +}
>> +postcore_initcall(mxs_regulator_init);
> module_platform_driver().

I wasn't sure because of the postcore stuff. I will fix it.

Best regards

Stefan

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On Mon, Sep 29, 2014 at 08:38:51AM +0200, Stefan Wahren wrote:

> I'm searching for a good regulator implementation example.

> Does it apply to ti-abb-regulator.c and twl-regulator.c?

Possibly.  But bear in mind that it's important to understand the
hardware you're trying to support.

> > This really needs a comment to explain what on earth is going on here -
> > the whole thing with writing the same thing twice with two delays is
> > more than a little odd.  It looks like the driver is trying to busy wait
> > in cases where the change happens quickly but the comments about "fast"
> > and "normal" mode make this unclear.

> The regulator driver polls for the DC_OK bit in the power status register.

> Quote for reference manual (p. 935): "High when switching DC-DC
> converter control loop has stabilized after a voltage target change."

> The two loops comes from the different regulator modes
> (REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).

> In REGULATOR_MODE_FAST the voltage steping is disabled and changing
> voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
> enabled and it's take a while for reaching the target voltage.

I don't think you've fully understood what the different modes mean
here, that's not normally how a buck convertor works.  The different
modes would typically control the ability of the regulator to respond
quickly to changes in load without drifting off regulation, fast mode
makes the regulator less efficient but more responsive to load changes
(probably marginally with modern regulators).  It should have relatively
little to do with the ability to ramp the voltage and certainly not on
the scale there.

> Do you see more a problem with the two different loops or the redundant
> register write?

Both.  The code right now just looks really obscure.

> if (readl(sreg->base_addr) & sreg->mode_mask)
>     return REGULATOR_MODE_FAST;

> return REGULATOR_MODE_NORMAL;

> Better?

Yes.

> > Why is this not looked up from the data structure like the rest of the
> > data?

> I was a little bit confused why there wasn't a mode_mask in the struct
> regulator_desc. I will do it like the ti-abb-regulator in the matching
> table.

There's no helpers for setting modes.

> >> +	regulator_unregister(rdev);
> >> +	iounmap(power_addr);
> >> +	iounmap(base_addr);

> > Use devm_ versions of functions.

> As a result the remove function for the drivers becomes unnecessary. Am
> i right?

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Stefan Wahren]

Am 29.09.2014 um 19:13 schrieb Mark Brown:
> On Mon, Sep 29, 2014 at 08:38:51AM +0200, Stefan Wahren wrote:
>
>> I'm searching for a good regulator implementation example.
>> Does it apply to ti-abb-regulator.c and twl-regulator.c?
> Possibly.  But bear in mind that it's important to understand the
> hardware you're trying to support.

The question refer more to the devicetree binding and it's implementation.

>
>>> This really needs a comment to explain what on earth is going on here -
>>> the whole thing with writing the same thing twice with two delays is
>>> more than a little odd.  It looks like the driver is trying to busy wait
>>> in cases where the change happens quickly but the comments about "fast"
>>> and "normal" mode make this unclear.
>> The regulator driver polls for the DC_OK bit in the power status register.
>> Quote for reference manual (p. 935): "High when switching DC-DC
>> converter control loop has stabilized after a voltage target change."
>> The two loops comes from the different regulator modes
>> (REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).
>> In REGULATOR_MODE_FAST the voltage steping is disabled and changing
>> voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
>> enabled and it's take a while for reaching the target voltage.
> I don't think you've fully understood what the different modes mean
> here, that's not normally how a buck convertor works.  The different
> modes would typically control the ability of the regulator to respond
> quickly to changes in load without drifting off regulation, fast mode
> makes the regulator less efficient but more responsive to load changes
> (probably marginally with modern regulators).  It should have relatively
> little to do with the ability to ramp the voltage and certainly not on
> the scale there.
>
>> Do you see more a problem with the two different loops or the redundant
>> register write?
> Both.  The code right now just looks really obscure.

That leads me to the conclusion to drop both mode functions. My
intention is to get the cpufreq-cpu0 aka cpufreq-dt working on i.MX28,
not to build up the complete power system.

@Fabio, @Shawn: What is your opinion?

Best regards

Stefan

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Stefan Wahren]

Hi Mark,

> Mark Brown <broonie@kernel.org> hat am 29. September 2014 um 19:13
> geschrieben:
>
> > > This really needs a comment to explain what on earth is going on here -
> > > the whole thing with writing the same thing twice with two delays is
> > > more than a little odd. It looks like the driver is trying to busy wait
> > > in cases where the change happens quickly but the comments about "fast"
> > > and "normal" mode make this unclear.
>
> > The regulator driver polls for the DC_OK bit in the power status register.
>
> > Quote for reference manual (p. 935): "High when switching DC-DC
> > converter control loop has stabilized after a voltage target change."
>
> > The two loops comes from the different regulator modes
> > (REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).
>
> > In REGULATOR_MODE_FAST the voltage steping is disabled and changing
> > voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
> > enabled and it's take a while for reaching the target voltage.
>
> I don't think you've fully understood what the different modes mean
> here, that's not normally how a buck convertor works. The different
> modes would typically control the ability of the regulator to respond
> quickly to changes in load without drifting off regulation, fast mode
> makes the regulator less efficient but more responsive to load changes
> (probably marginally with modern regulators). It should have relatively
> little to do with the ability to ramp the voltage and certainly not on
> the scale there.
>

thanks for your explanation.

Is it better to let the core handles the ramp delay instead of set_voltage_sel
with a busy wait?

I've found that polling DC_OK is only reliable for increasing voltages. So i
think about defining ramp delay in the regulator description.

Stefan

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Wed, Oct 01, 2014 at 03:23:44PM +0200, Stefan Wahren wrote:

> Is it better to let the core handles the ramp delay instead of set_voltage_sel
> with a busy wait?

> I've found that polling DC_OK is only reliable for increasing voltages. So i
> think about defining ramp delay in the regulator description.

It's better to use the notification from the device if it is reliable;
dead reckoning isn't going to work well either for voltage reduction
since the time taken to implement a drop in voltage is more a function
of the loading than anything else, the regulator can't really control
it.  For most DC-DC loads it'll be quick enough though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/2] regulator: add mxs regulator driver

[Fabio Estevam]

On Fri, Sep 26, 2014 at 9:59 PM, Stefan Wahren <stefan.wahren@i2se.com> wrote:

> +config REGULATOR_MXS
> +       tristate "Freescale MXS on-chip regulators"
> +       depends on ARCH_MXS
> +       default y if ARCH_MXS

I would suggest removing this 'default y if ARCH_MXS' line. Thanks