diff mbox

[v2,2/8] mfd: AXP20x: Add bindings documentation

Message ID 1394898225-28452-3-git-send-email-carlo@caione.org (mailing list archive)
State New, archived
Headers show

Commit Message

Carlo Caione March 15, 2014, 3:43 p.m. UTC
Bindings documentation for the AXP20x driver. In this file also two
sub-nodes (PEK and regulators) are documented.

Signed-off-by: Carlo Caione <carlo@caione.org>
---
 Documentation/devicetree/bindings/mfd/axp20x.txt   | 83 ++++++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt        |  1 +
 2 files changed, 84 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/axp20x.txt

Comments

Maxime Ripard March 18, 2014, 8:45 a.m. UTC | #1
On Sat, Mar 15, 2014 at 04:43:39PM +0100, Carlo Caione wrote:
> Bindings documentation for the AXP20x driver. In this file also two
> sub-nodes (PEK and regulators) are documented.

PEK doesn't look to be documented, either in this patch, or any other.

> Signed-off-by: Carlo Caione <carlo@caione.org>
> ---
>  Documentation/devicetree/bindings/mfd/axp20x.txt   | 83 ++++++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt        |  1 +

I don't really know what the DT maintainers are expecting here, but I
would have done two patches.

>  2 files changed, 84 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/axp20x.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
> new file mode 100644
> index 0000000..982aefe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
> @@ -0,0 +1,83 @@
> +* axp20x device tree bindings
> +
> +The axp20x family current members :-
> +axp202 (X-Powers)
> +axp209 (X-Powers)
> +
> +Required properties:
> +- compatible 			: Should be "x-powers,axp202" or "x-powers,axp209"
> +- interrupt-controller 		: axp20x has its own internal IRQs
> +- #interrupt-cells 		: Should be set to 1
> +- interrupt-parent 		: The parent interrupt controller
> +- interrupts 			: Interrupt specifiers for interrupt sources
> +- reg 				: The I2C slave address for the AXP chip
> +
> +Sub-nodes:
> +* regulators : Contain the regulator nodes. The regulators are bound using
> +	       their name as listed here: dcdc2, dcdc3, ldo1, ldo2, ldo3,
> +	       ldo4, ldo5.
> +	       The bindings details of individual regulator device can be found in:
> +	       Documentation/devicetree/bindings/regulator/regulator.txt with the
> +	       exception of:
> +
> +	- dcdc-freq		: defines the work frequency of DC-DC in KHz
> +				  (range: 750-1875). Default: 1.5MHz
> +	- dcdc-workmode		: Optional. 1 for PWM mode, 0 for AUTO mode
> +				  Default: AUTO mode

Those two are x-powers specific, or would it make sense to have them
in other drivers too?

If the former, please add the x-powers prefix.

> +
> +Example:
> +
> +axp: axp20x@34 {
> +	reg = <0x34>;
> +	interrupt-parent = <&nmi_intc>;
> +	interrupts = <0 8>;
> +
> +	compatible = "x-powers,axp209";
> +	interrupt-controller;
> +	#interrupt-cells = <1>;
> +
> +	regulators {

Do we really need that subnode ? it looks useless, and we already know
that we are defining regulators here.

> +		dcdc-freq = "1500";

That frequency is defined at the same level than the dcdc-workmode
property, yet they both seem to be placed at different levels.

> +
> +		axp_dcdc2: dcdc2 {
> +			regulator-min-microvolt = <700000>;
> +			regulator-max-microvolt = <2275000>;
> +			dcdc-workmode = <0>;
> +			regulator-always-on;
> +		};
> +
> +		axp_dcdc3: dcdc3 {
> +			regulator-min-microvolt = <700000>;
> +			regulator-max-microvolt = <3500000>;
> +			dcdc-workmode = <0>;

It looks like those are at their default values?

Thanks!
Maxime
Carlo Caione March 22, 2014, 2:11 p.m. UTC | #2
On Tue, Mar 18, 2014 at 09:45:05AM +0100, Maxime Ripard wrote:
> On Sat, Mar 15, 2014 at 04:43:39PM +0100, Carlo Caione wrote:
> > Bindings documentation for the AXP20x driver. In this file also two
> > sub-nodes (PEK and regulators) are documented.
> 
> PEK doesn't look to be documented, either in this patch, or any other.

Residue from v1. To be deleted.

> > Signed-off-by: Carlo Caione <carlo@caione.org>
> > ---
> >  Documentation/devicetree/bindings/mfd/axp20x.txt   | 83 ++++++++++++++++++++++
> >  .../devicetree/bindings/vendor-prefixes.txt        |  1 +
> 
> I don't really know what the DT maintainers are expecting here, but I
> would have done two patches.

Uhm, no idea (and no feedback). I'll split it.

> >  2 files changed, 84 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/axp20x.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
> > new file mode 100644
> > index 0000000..982aefe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
> > @@ -0,0 +1,83 @@
> > +* axp20x device tree bindings
> > +
> > +The axp20x family current members :-
> > +axp202 (X-Powers)
> > +axp209 (X-Powers)
> > +
> > +Required properties:
> > +- compatible 			: Should be "x-powers,axp202" or "x-powers,axp209"
> > +- interrupt-controller 		: axp20x has its own internal IRQs
> > +- #interrupt-cells 		: Should be set to 1
> > +- interrupt-parent 		: The parent interrupt controller
> > +- interrupts 			: Interrupt specifiers for interrupt sources
> > +- reg 				: The I2C slave address for the AXP chip
> > +
> > +Sub-nodes:
> > +* regulators : Contain the regulator nodes. The regulators are bound using
> > +	       their name as listed here: dcdc2, dcdc3, ldo1, ldo2, ldo3,
> > +	       ldo4, ldo5.
> > +	       The bindings details of individual regulator device can be found in:
> > +	       Documentation/devicetree/bindings/regulator/regulator.txt with the
> > +	       exception of:
> > +
> > +	- dcdc-freq		: defines the work frequency of DC-DC in KHz
> > +				  (range: 750-1875). Default: 1.5MHz
> > +	- dcdc-workmode		: Optional. 1 for PWM mode, 0 for AUTO mode
> > +				  Default: AUTO mode
> 
> Those two are x-powers specific, or would it make sense to have them
> in other drivers too?
> 
> If the former, please add the x-powers prefix.

AFAIK this is AXP specific. I'll add the prefix.

> > +
> > +Example:
> > +
> > +axp: axp20x@34 {
> > +	reg = <0x34>;
> > +	interrupt-parent = <&nmi_intc>;
> > +	interrupts = <0 8>;
> > +
> > +	compatible = "x-powers,axp209";
> > +	interrupt-controller;
> > +	#interrupt-cells = <1>;
> > +
> > +	regulators {
> 
> Do we really need that subnode ? it looks useless, and we already know
> that we are defining regulators here.

What do you mean? We are defining the MFD and regulators are just one of
the subsystem here. Moveover I'm using the "regulators" node in the
regulators driver (using of_find_node_by_name()) to get the regulators
configuration.

> > +		dcdc-freq = "1500";
> 
> That frequency is defined at the same level than the dcdc-workmode
> property, yet they both seem to be placed at different levels.

They are at different level. dcdc-freq is valid for all the dcdc,
whereas dcdc-workmode is dcdc-specific.
I'll clarify the point.

> > +
> > +		axp_dcdc2: dcdc2 {
> > +			regulator-min-microvolt = <700000>;
> > +			regulator-max-microvolt = <2275000>;
> > +			dcdc-workmode = <0>;
> > +			regulator-always-on;
> > +		};
> > +
> > +		axp_dcdc3: dcdc3 {
> > +			regulator-min-microvolt = <700000>;
> > +			regulator-max-microvolt = <3500000>;
> > +			dcdc-workmode = <0>;
> 
> It looks like those are at their default values?

Yes. To be fixed.

Thanks,
Maxime Ripard March 25, 2014, 10:11 a.m. UTC | #3
On Sat, Mar 22, 2014 at 03:11:57PM +0100, Carlo Caione wrote:
> > > +
> > > +Example:
> > > +
> > > +axp: axp20x@34 {
> > > +	reg = <0x34>;
> > > +	interrupt-parent = <&nmi_intc>;
> > > +	interrupts = <0 8>;
> > > +
> > > +	compatible = "x-powers,axp209";
> > > +	interrupt-controller;
> > > +	#interrupt-cells = <1>;
> > > +
> > > +	regulators {
> > 
> > Do we really need that subnode ? it looks useless, and we already know
> > that we are defining regulators here.
> 
> What do you mean? We are defining the MFD and regulators are just one of
> the subsystem here.

I mean that it's useless.

> Moveover I'm using the "regulators" node in the
> regulators driver (using of_find_node_by_name()) to get the regulators
> configuration.

Can't you just use the of_node field of whatever struct device you
get?
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
new file mode 100644
index 0000000..982aefe
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -0,0 +1,83 @@ 
+* axp20x device tree bindings
+
+The axp20x family current members :-
+axp202 (X-Powers)
+axp209 (X-Powers)
+
+Required properties:
+- compatible 			: Should be "x-powers,axp202" or "x-powers,axp209"
+- interrupt-controller 		: axp20x has its own internal IRQs
+- #interrupt-cells 		: Should be set to 1
+- interrupt-parent 		: The parent interrupt controller
+- interrupts 			: Interrupt specifiers for interrupt sources
+- reg 				: The I2C slave address for the AXP chip
+
+Sub-nodes:
+* regulators : Contain the regulator nodes. The regulators are bound using
+	       their name as listed here: dcdc2, dcdc3, ldo1, ldo2, ldo3,
+	       ldo4, ldo5.
+	       The bindings details of individual regulator device can be found in:
+	       Documentation/devicetree/bindings/regulator/regulator.txt with the
+	       exception of:
+
+	- dcdc-freq		: defines the work frequency of DC-DC in KHz
+				  (range: 750-1875). Default: 1.5MHz
+	- dcdc-workmode		: Optional. 1 for PWM mode, 0 for AUTO mode
+				  Default: AUTO mode
+
+Example:
+
+axp: axp20x@34 {
+	reg = <0x34>;
+	interrupt-parent = <&nmi_intc>;
+	interrupts = <0 8>;
+
+	compatible = "x-powers,axp209";
+	interrupt-controller;
+	#interrupt-cells = <1>;
+
+	regulators {
+		dcdc-freq = "1500";
+
+		axp_dcdc2: dcdc2 {
+			regulator-min-microvolt = <700000>;
+			regulator-max-microvolt = <2275000>;
+			dcdc-workmode = <0>;
+			regulator-always-on;
+		};
+
+		axp_dcdc3: dcdc3 {
+			regulator-min-microvolt = <700000>;
+			regulator-max-microvolt = <3500000>;
+			dcdc-workmode = <0>;
+			regulator-always-on;
+		};
+
+		axp_ldo1: ldo1 {
+			regulator-min-microvolt = <1300000>;
+			regulator-max-microvolt = <1300000>;
+		};
+
+		axp_ldo2: ldo2 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-always-on;
+		};
+
+		axp_ldo3: ldo3 {
+			regulator-min-microvolt = <700000>;
+			regulator-max-microvolt = <3500000>;
+		};
+
+		axp_ldo4: ldo4 {
+			regulator-min-microvolt = <1250000>;
+			regulator-max-microvolt = <3300000>;
+		};
+
+		axp_ldo5: ldo5 {
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <3300000>;
+		};
+	};
+};
+
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 40ce2df..d06ba8c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -95,3 +95,4 @@  winbond Winbond Electronics corp.
 wlf	Wolfson Microelectronics
 wm	Wondermedia Technologies, Inc.
 xlnx	Xilinx
+x-powers	X-Powers