| Message ID | 1374834657-29088-1-git-send-email-sachin.kamat@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
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.
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
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 --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 {
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(-)