diff mbox

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

Message ID 1411779588-22031-2-git-send-email-stefan.wahren@i2se.com
State New, archived
Headers show

Commit Message

Stefan Wahren Sept. 27, 2014, 12:59 a.m. UTC
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

Comments

Mark Brown Sept. 28, 2014, 10:22 a.m. UTC | #1
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.
Stefan Wahren Sept. 29, 2014, 6 a.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Sept. 29, 2014, 10:23 a.m. UTC | #3
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.
Mark Rutland Sept. 29, 2014, 11:09 a.m. UTC | #4
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
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren Sept. 29, 2014, 11:53 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Sept. 29, 2014, 12:41 p.m. UTC | #6
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren Sept. 29, 2014, 1:10 p.m. UTC | #7
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Rutland Sept. 29, 2014, 1:23 p.m. UTC | #8
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Wahren Oct. 3, 2014, 11:46 a.m. UTC | #9
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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>;
+		};
+	};
+