Hi,

Just a few minor comments...

On 12/06/2013 06:47 AM, Leela Krishna Amudala wrote:
This patch adds pmusysreg node to exynos5250 and exynos5420 dtsi files to

s/pmusysreg/PMU sysreg ? Similarly I would capitalize it in the subject
line as well.

handle PMU register accesses in a centralized way using syscon driver

Signed-off-by: Leela Krishna Amudala<[email protected]>
Reviewed-by: Tomasz Figa<[email protected]>
Reviewed-by: Doug Anderson<[email protected]>
Tested-by: Doug Anderson<[email protected]>
---
  Documentation/devicetree/bindings/arm/samsung/pmu.txt |   15 +++++++++++++++
  arch/arm/boot/dts/exynos5250.dtsi                     |    5 +++++
  arch/arm/boot/dts/exynos5420.dtsi                     |    5 +++++
  3 files changed, 25 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/arm/samsung/pmu.txt

diff --git a/Documentation/devicetree/bindings/arm/samsung/pmu.txt 
b/Documentation/devicetree/bindings/arm/samsung/pmu.txt
new file mode 100644
index 0000000..f1f1552
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/samsung/pmu.txt

I might be easy to confuse this with ARM Performance Monitoring Unit.
So perhaps we should rename this file to, e.g. power-management-unit.txt ?

@@ -0,0 +1,15 @@
+SAMSUNG Exynos SoC series PMU Registers

s/PMU/Power Management Unit ?

+
+Properties:
+ - compatible : should contain two values. First value must be one from 
following list:
+                  - "samsung,exynos5250-pmu" - for Exynos5250 SoC,
+                  - "samsung,exynos5420-pmu" - for Exynos5420 SoC.

s/./; ?

+               second value must be always "syscon".

It might be more safe to specify it as the last value, so something along
the lines of:

        The last value should be "syscon".

+
+ - reg : offset and length of the register set.
+
+Example :
+pmu_system_controller: system-controller@10040000 {

Might be more sensible to use 'power_management_unit' for the label.

+       compatible = "samsung,exynos5250-pmu", "syscon";
+       reg =<0x10040000 0x5000>;
+};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi 
b/arch/arm/boot/dts/exynos5250.dtsi
index b98ffc3..62f9e36 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -163,6 +163,11 @@
                interrupts =<0 47 0>;
        };

+       pmu_system_controller: system-controller@10040000 {

s/pmu_system_controller/power_management_unit ? So it describes the subsystem
better in terms used in the SoCs User Manual ?

+               compatible = "samsung,exynos5250-pmu", "syscon";
+               reg =<0x10040000 0x5000>;
+       };
+
        watchdog {
                clocks =<&clock 336>;
                clock-names = "watchdog";
diff --git a/arch/arm/boot/dts/exynos5420.dtsi 
b/arch/arm/boot/dts/exynos5420.dtsi
index b1fa334..cd47db0 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -402,4 +402,9 @@
                clock-names = "gscl";
                samsung,power-domain =<&gsc_pd>;
        };
+
+       pmu_system_controller: system-controller@10040000 {

s/pmu_system_controller/power_management_unit ?

+               compatible = "samsung,exynos5420-pmu", "syscon";
+               reg =<0x10040000 0x5000>;
+       };
  };

Otherwise looks good.

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to