diff mbox

[1/2] ARM: dts: exynos4210-origen: Add fixed voltage regulator to simple bus

Message ID 1374834657-29088-1-git-send-email-sachin.kamat@linaro.org
State Accepted
Headers show

Commit Message

Sachin Kamat July 26, 2013, 10:30 a.m. UTC
This has been done for Arndale board vide commit aa3edb65
("ARM: dts: Put Arndale fixed voltage regulators on a simple-bus").
Replicate here for consistency and correctness.

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 arch/arm/boot/dts/exynos4210-origen.dts |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Sachin Kamat Sept. 26, 2013, 5:02 a.m. UTC | #1
Hi Kukjin,

On 26 July 2013 16:00, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> This has been done for Arndale board vide commit aa3edb65
> ("ARM: dts: Put Arndale fixed voltage regulators on a simple-bus").
> Replicate here for consistency and correctness.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  arch/arm/boot/dts/exynos4210-origen.dts |   20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
> index 5537af8..ae2653e 100644
> --- a/arch/arm/boot/dts/exynos4210-origen.dts
> +++ b/arch/arm/boot/dts/exynos4210-origen.dts
> @@ -32,13 +32,19 @@
>                 bootargs ="root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC2,115200 init=/linuxrc";
>         };
>
> -       mmc_reg: voltage-regulator {
> -               compatible = "regulator-fixed";
> -               regulator-name = "VMEM_VDD_2.8V";
> -               regulator-min-microvolt = <2800000>;
> -               regulator-max-microvolt = <2800000>;
> -               gpio = <&gpx1 1 0>;
> -               enable-active-high;
> +       regulators {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               mmc_reg: voltage-regulator {
> +                       compatible = "regulator-fixed";
> +                       regulator-name = "VMEM_VDD_2.8V";
> +                       regulator-min-microvolt = <2800000>;
> +                       regulator-max-microvolt = <2800000>;
> +                       gpio = <&gpx1 1 0>;
> +                       enable-active-high;
> +               };
>         };
>
>         tmu@100C0000 {
> --
> 1.7.9.5
>

These 2 patches pending since long time. Can you please check.
Tomasz Figa Sept. 26, 2013, 8:20 a.m. UTC | #2
Hi Sachin,

I must have missed this patch so better later than never ;).
Please see my comments inline.

On Thursday 26 of September 2013 10:32:01 Sachin Kamat wrote:
> > +       regulators {
> > +               compatible = "simple-bus";
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               mmc_reg: voltage-regulator {

For consistency and correctness, since this is a bus, even if not 
physical, it is worth to add reg property and unit-address to subnodes. 
Since this bus is not physical, the reg property would not be anything 
more than purely an index.

Also for consistency I'd use "regulator" as node name here, as this is 
what is used most commonly across all the dts files in arch/arm/boot/dts.

So you could have then (in case of more than one regulator)

	regulators {
		compatible = "simple-bus";
		#address-cells = <1>;
		#size-cells = <0>;

		mmc_reg: regulator@0 {
			/* ... */
			reg = <0>;
		};

		xxx_reg: regulator@1 {
			/* ... */
			reg = <1>;
		};

		/* ... */
	};

Best regards,
Tomasz
Sachin Kamat Sept. 27, 2013, 6:50 a.m. UTC | #3
Hi Tomasz,

On 26 September 2013 13:50, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sachin,
>
> I must have missed this patch so better later than never ;).

Since you are quite prompt at reviewing I thought you did not have any
comments :)

> Please see my comments inline.
>
> On Thursday 26 of September 2013 10:32:01 Sachin Kamat wrote:
>> > +       regulators {
>> > +               compatible = "simple-bus";
>> > +               #address-cells = <1>;
>> > +               #size-cells = <0>;
>> > +
>> > +               mmc_reg: voltage-regulator {
>
> For consistency and correctness, since this is a bus, even if not
> physical, it is worth to add reg property and unit-address to subnodes.
> Since this bus is not physical, the reg property would not be anything
> more than purely an index.
>
> Also for consistency I'd use "regulator" as node name here, as this is
> what is used most commonly across all the dts files in arch/arm/boot/dts.

Sounds good. Will update accordingly. While at it will also update the
Arndale dts
file to reflect your suggestions for a similar node there.

Thank you for your review and feedback.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/exynos4210-origen.dts b/arch/arm/boot/dts/exynos4210-origen.dts
index 5537af8..ae2653e 100644
--- a/arch/arm/boot/dts/exynos4210-origen.dts
+++ b/arch/arm/boot/dts/exynos4210-origen.dts
@@ -32,13 +32,19 @@ 
 		bootargs ="root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC2,115200 init=/linuxrc";
 	};
 
-	mmc_reg: voltage-regulator {
-	        compatible = "regulator-fixed";
-		regulator-name = "VMEM_VDD_2.8V";
-		regulator-min-microvolt = <2800000>;
-		regulator-max-microvolt = <2800000>;
-		gpio = <&gpx1 1 0>;
-		enable-active-high;
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mmc_reg: voltage-regulator {
+			compatible = "regulator-fixed";
+			regulator-name = "VMEM_VDD_2.8V";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <2800000>;
+			gpio = <&gpx1 1 0>;
+			enable-active-high;
+		};
 	};
 
 	tmu@100C0000 {