| Message ID | 1411779588-22031-2-git-send-email-stefan.wahren@i2se.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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.
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
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.
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 = <®_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
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
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
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
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
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 --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 = <®_vdda>; + }; + }; +
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