* [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos
[not found] <1403486483-4063-1-git-send-email-afaerber@suse.de>
@ 2014-06-23 1:21 ` Andreas Färber
2014-06-23 3:21 ` Sachin Kamat
2014-06-23 8:15 ` Lee Jones
2014-06-23 1:21 ` [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax Andreas Färber
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 1:21 UTC (permalink / raw)
To: linux-samsung-soc
Cc: Stephan van Schaik, Vincent Palatin, Doug Anderson,
Andreas Färber, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Yadwinder Singh Brar, Tomasz Figa,
Sachin Kamat, open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
It's LDO2, not LD02.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
index d81ba30..55ab4f4 100644
--- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
+++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
@@ -81,7 +81,7 @@ as per the datasheet of s2mps11.
- valid values for n are:
- S2MPS11: 1 to 38
- S2MPS14: 1 to 25
- - Example: LDO1, LD02, LDO28
+ - Example: LDO1, LDO2, LDO28
- BUCKn
- valid values for n are:
- S2MPS11: 1 to 10
diff --git a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
index d290988..2019131 100644
--- a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
@@ -86,7 +86,7 @@ as per the datasheet of s5m8767.
- LDOn
- valid values for n are 1 to 28
- - Example: LDO1, LD02, LDO28
+ - Example: LDO1, LDO2, LDO28
- BUCKn
- valid values for n are 1 to 9.
- Example: BUCK1, BUCK2, BUCK9
--
1.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax
[not found] <1403486483-4063-1-git-send-email-afaerber@suse.de>
2014-06-23 1:21 ` [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos Andreas Färber
@ 2014-06-23 1:21 ` Andreas Färber
2014-06-23 3:23 ` Sachin Kamat
2014-06-23 8:15 ` Lee Jones
2014-06-23 1:21 ` [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos Andreas Färber
2014-06-23 1:21 ` [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree Andreas Färber
3 siblings, 2 replies; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 1:21 UTC (permalink / raw)
To: linux-samsung-soc
Cc: Stephan van Schaik, Vincent Palatin, Doug Anderson,
Andreas Färber, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Sachin Kamat, Yadwinder Singh Brar,
open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
It's <1>, not 1.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
index 55ab4f4..99a0c52 100644
--- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
+++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
@@ -96,7 +96,7 @@ Example:
s2m_osc: clocks {
compatible = "samsung,s2mps11-clk";
- #clock-cells = 1;
+ #clock-cells = <1>;
clock-output-names = "xx", "yy", "zz";
};
--
1.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos
[not found] <1403486483-4063-1-git-send-email-afaerber@suse.de>
2014-06-23 1:21 ` [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos Andreas Färber
2014-06-23 1:21 ` [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax Andreas Färber
@ 2014-06-23 1:21 ` Andreas Färber
2014-06-23 17:27 ` Doug Anderson
2014-06-23 1:21 ` [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree Andreas Färber
3 siblings, 1 reply; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 1:21 UTC (permalink / raw)
To: linux-samsung-soc
Cc: Stephan van Schaik, Vincent Palatin, Doug Anderson,
Andreas Färber, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Mark Brown, Simon Glass,
Michael Spang, open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
It's vsys-l{1,2}-supply, not vsys_l{1,2}-supply.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
Documentation/devicetree/bindings/regulator/tps65090.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
index 34098023..ca69f5e 100644
--- a/Documentation/devicetree/bindings/regulator/tps65090.txt
+++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
@@ -45,8 +45,8 @@ Example:
infet5-supply = <&some_reg>;
infet6-supply = <&some_reg>;
infet7-supply = <&some_reg>;
- vsys_l1-supply = <&some_reg>;
- vsys_l2-supply = <&some_reg>;
+ vsys-l1-supply = <&some_reg>;
+ vsys-l2-supply = <&some_reg>;
regulators {
dcdc1 {
--
1.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
[not found] <1403486483-4063-1-git-send-email-afaerber@suse.de>
` (2 preceding siblings ...)
2014-06-23 1:21 ` [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos Andreas Färber
@ 2014-06-23 1:21 ` Andreas Färber
2014-06-23 19:47 ` Doug Anderson
3 siblings, 1 reply; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 1:21 UTC (permalink / raw)
To: linux-samsung-soc
Cc: Stephan van Schaik, Vincent Palatin, Doug Anderson,
Andreas Färber, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Ben Dooks, Kukjin Kim,
open list:OPEN FIRMWARE AND...,
moderated list:ARM PORT, open list
Adds initial support for the HP Chromebook 11.
Cc: Vincent Palatin <vpalatin@chromium.org>
Cc: Doug Anderson <dianders@chromium.org>
Cc: Stephan van Schaik <stephan@synkhronix.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/exynos5250-spring.dts | 556 ++++++++++++++++++++++++++++++++
2 files changed, 557 insertions(+)
create mode 100644 arch/arm/boot/dts/exynos5250-spring.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 5986ff6..dc2c5aa 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -74,6 +74,7 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \
exynos5250-arndale.dtb \
exynos5250-smdk5250.dtb \
exynos5250-snow.dtb \
+ exynos5250-spring.dtb \
exynos5260-xyref5260.dtb \
exynos5410-smdk5410.dtb \
exynos5420-arndale-octa.dtb \
diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
new file mode 100644
index 0000000..e857d44
--- /dev/null
+++ b/arch/arm/boot/dts/exynos5250-spring.dts
@@ -0,0 +1,556 @@
+/*
+ * Google Spring board device tree source
+ *
+ * Copyright (c) 2013 Google, Inc
+ * Copyright (c) 2014 SUSE LINUX Products GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/dts-v1/;
+#include "exynos5250.dtsi"
+#include "exynos5250-cros-common.dtsi"
+
+/ {
+ model = "Google Spring";
+ compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
+
+ pinctrl@11400000 {
+ s5m8767_dvs: s5m8767-dvs {
+ samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <1>;
+ samsung,pin-drv = <0>;
+ };
+
+ s5m8767_ds: s5m8767-ds {
+ samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <1>;
+ samsung,pin-drv = <0>;
+ };
+
+ tps65090_irq: tps65090-irq {
+ samsung,pins = "gpx2-6";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
+ s5m8767_irq: s5m8767-irq {
+ samsung,pins = "gpx3-2";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
+ hdmi_hpd_irq: hdmi-hpd-irq {
+ samsung,pins = "gpx3-7";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <1>;
+ samsung,pin-drv = <0>;
+ };
+ };
+
+ pinctrl@13400000 {
+ hsic_reset: hsic-reset {
+ samsung,pins = "gpe1-0";
+ samsung,pin-function = <1>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+ };
+
+ vbat: vbat-fixed-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "vbat-supply";
+ regulator-boot-on;
+ };
+
+ usb@12000000 {
+ status = "okay";
+ };
+
+ usb3_vbus_reg: regulator-usb3 {
+ compatible = "regulator-fixed";
+ regulator-name = "P5.0V_USB3CON";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ gpio = <&gpe1 0 1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&hsic_reset>;
+ enable-active-high;
+ };
+
+ phy@12100000 {
+ vbus-supply = <&usb3_vbus_reg>;
+ };
+
+ usb@12110000 {
+ samsung,vbus-gpio = <&gpx1 1 0>;
+ status = "okay";
+ };
+
+ usb@12120000 {
+ status = "okay";
+ };
+
+ mmc@12220000 {
+ /* MMC2 pins are used as GPIO for eDP bridge control. */
+ status = "disabled";
+ };
+
+ mmc@12230000 {
+ status = "disabled";
+ };
+
+ i2c@12C60000 {
+ max77686@09 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+
+ rtc {
+ reg = <0x6>;
+ };
+ };
+
+ s5m8767_pmic@66 {
+ compatible = "samsung,s5m8767-pmic";
+ reg = <0x66>;
+ interrupt-parent = <&gpx3>;
+ interrupts = <2 0>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
+ wakeup-source;
+
+ s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
+ <&gpd1 1 1>, /* DVS2 */
+ <&gpd1 2 1>; /* DVS3 */
+
+ s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
+ <&gpx2 4 1>, /* SET2 */
+ <&gpx2 5 1>; /* SET3 */
+
+ /*
+ * The following arrays of DVS voltages are not used, since we are
+ * not using GPIOs to control PMIC bucks, but they must be defined
+ * to please the driver.
+ */
+ s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
+ <1250000>, <1200000>,
+ <1150000>, <1100000>,
+ <1000000>, <950000>;
+
+ s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
+ <1100000>, <1100000>,
+ <1000000>, <1000000>,
+ <1000000>, <1000000>;
+
+ s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
+ <1200000>, <1200000>,
+ <1200000>, <1200000>,
+ <1200000>, <1200000>;
+
+ clocks {
+ compatible = "samsung,s5m8767-clk";
+ #clock-cells = <1>;
+ clock-output-names = "en32khz_ap",
+ "en32khz_cp",
+ "en32khz_bt";
+ };
+
+ regulators {
+ s5m_ldo4_reg: LDO4 {
+ regulator-name = "P1.0V_LDO_OUT4";
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ op_mode = <0>;
+ };
+
+ s5m_ldo5_reg: LDO5 {
+ regulator-name = "P1.0V_LDO_OUT5";
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ op_mode = <0>;
+ };
+
+ s5m_ldo6_reg: LDO6 {
+ regulator-name = "vdd_mydp";
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo7_reg: LDO7 {
+ regulator-name = "P1.1V_LDO_OUT7";
+ regulator-min-microvolt = <1100000>;
+ regulator-max-microvolt = <1100000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo8_reg: LDO8 {
+ regulator-name = "P1.0V_LDO_OUT8";
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo10_reg: LDO10 {
+ regulator-name = "P1.8V_LDO_OUT10";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo11_reg: LDO11 {
+ regulator-name = "P1.8V_LDO_OUT11";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ op_mode = <0>;
+ };
+
+ s5m_ldo12_reg: LDO12 {
+ regulator-name = "P3.0V_LDO_OUT12";
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo13_reg: LDO13 {
+ regulator-name = "P1.8V_LDO_OUT13";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ op_mode = <0>;
+ };
+
+ s5m_ldo14_reg: LDO14 {
+ regulator-name = "P1.8V_LDO_OUT14";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo15_reg: LDO15 {
+ regulator-name = "P1.0V_LDO_OUT15";
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo16_reg: LDO16 {
+ regulator-name = "P1.8V_LDO_OUT16";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ op_mode = <3>;
+ };
+
+ s5m_ldo17_reg: LDO17 {
+ regulator-name = "P2.8V_LDO_OUT17";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ regulator-always-on;
+ op_mode = <0>;
+ };
+
+ s5m_ldo25_reg: LDO25 {
+ regulator-name = "vdd_bridge";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-always-on;
+ op_mode = <1>;
+ };
+
+ BUCK1 {
+ regulator-name = "vdd_mif";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <3>;
+ };
+
+ BUCK2 {
+ regulator-name = "vdd_arm";
+ regulator-min-microvolt = <850000>;
+ regulator-max-microvolt = <1350000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <3>;
+ };
+
+ BUCK3 {
+ regulator-name = "vdd_int";
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <3>;
+ };
+
+ BUCK4 {
+ regulator-name = "vdd_g3d";
+ regulator-min-microvolt = <850000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-boot-on;
+ op_mode = <3>;
+ };
+
+ BUCK5 {
+ regulator-name = "P1.8V_BUCK_OUT5";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <1>;
+ };
+
+ BUCK6 {
+ regulator-name = "P1.2V_BUCK_OUT6";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <0>;
+ };
+
+ BUCK9 {
+ regulator-name = "vdd_ummc";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-always-on;
+ regulator-boot-on;
+ op_mode = <3>;
+ };
+ };
+ };
+ };
+
+ i2c@12C70000 {
+ trackpad {
+ status = "disabled";
+ };
+ };
+
+ i2c@12CA0000 {
+ embedded-controller {
+ compatible = "google,cros-ec-i2c";
+ reg = <0x1e>;
+ interrupts = <6 0>;
+ interrupt-parent = <&gpx1>;
+ wakeup-source;
+
+ keyboard-controller {
+ compatible = "google,cros-ec-keyb";
+ keypad,num-rows = <8>;
+ keypad,num-columns = <13>;
+ google,needs-ghost-filter;
+ linux,keymap = <
+ 0x0001007d /* L_META */
+ 0x0002003b /* F1 */
+ 0x00030030 /* B */
+ 0x00040044 /* F10 */
+ 0x00060031 /* N */
+ 0x0008000d /* = */
+ 0x000a0064 /* R_ALT */
+
+ 0x01010001 /* ESC */
+ 0x0102003e /* F4 */
+ 0x01030022 /* G */
+ 0x01040041 /* F7 */
+ 0x01060023 /* H */
+ 0x01080028 /* ' */
+ 0x01090043 /* F9 */
+ 0x010b000e /* BKSPACE */
+
+ 0x0200001d /* L_CTRL */
+ 0x0201000f /* TAB */
+ 0x0202003d /* F3 */
+ 0x02030014 /* T */
+ 0x02040040 /* F6 */
+ 0x0205001b /* ] */
+ 0x02060015 /* Y */
+ 0x02070056 /* 102ND */
+ 0x0208001a /* [ */
+ 0x02090042 /* F8 */
+
+ 0x03010029 /* GRAVE */
+ 0x0302003c /* F2 */
+ 0x03030006 /* 5 */
+ 0x0304003f /* F5 */
+ 0x03060007 /* 6 */
+ 0x0308000c /* - */
+ 0x030b002b /* \ */
+
+ 0x04000061 /* R_CTRL */
+ 0x0401001e /* A */
+ 0x04020020 /* D */
+ 0x04030021 /* F */
+ 0x0404001f /* S */
+ 0x04050025 /* K */
+ 0x04060024 /* J */
+ 0x04080027 /* ; */
+ 0x04090026 /* L */
+ 0x040a002b /* \ */
+ 0x040b001c /* ENTER */
+
+ 0x0501002c /* Z */
+ 0x0502002e /* C */
+ 0x0503002f /* V */
+ 0x0504002d /* X */
+ 0x05050033 /* , */
+ 0x05060032 /* M */
+ 0x0507002a /* L_SHIFT */
+ 0x05080035 /* / */
+ 0x05090034 /* . */
+ 0x050B0039 /* SPACE */
+
+ 0x06010002 /* 1 */
+ 0x06020004 /* 3 */
+ 0x06030005 /* 4 */
+ 0x06040003 /* 2 */
+ 0x06050009 /* 8 */
+ 0x06060008 /* 7 */
+ 0x0608000b /* 0 */
+ 0x0609000a /* 9 */
+ 0x060a0038 /* L_ALT */
+ 0x060b006c /* DOWN */
+ 0x060c006a /* RIGHT */
+
+ 0x07010010 /* Q */
+ 0x07020012 /* E */
+ 0x07030013 /* R */
+ 0x07040011 /* W */
+ 0x07050017 /* I */
+ 0x07060016 /* U */
+ 0x07070036 /* R_SHIFT */
+ 0x07080019 /* P */
+ 0x07090018 /* O */
+ 0x070b0067 /* UP */
+ 0x070c0069 /* LEFT */
+ >;
+ };
+ };
+
+ power-regulator {
+ compatible = "ti,tps65090";
+ reg = <0x48>;
+
+ /*
+ * Config irq to disable internal pulls
+ * even though we run in polling mode.
+ */
+ pinctrl-names = "default";
+ pinctrl-0 = <&tps65090_irq>;
+
+ vsys1-supply = <&vbat>;
+ vsys2-supply = <&vbat>;
+ vsys3-supply = <&vbat>;
+ infet1-supply = <&vbat>;
+ infet2-supply = <&vbat>;
+ infet3-supply = <&vbat>;
+ infet4-supply = <&vbat>;
+ infet5-supply = <&vbat>;
+ infet6-supply = <&vbat>;
+ infet7-supply = <&vbat>;
+ vsys-l1-supply = <&vbat>;
+ vsys-l2-supply = <&vbat>;
+
+ regulators {
+ fet1 {
+ regulator-name = "vcd_led";
+ regulator-min-microvolt = <12000000>;
+ regulator-max-microvolt = <12000000>;
+ };
+ fet3 {
+ regulator-name = "wwan_r";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+ fet5 {
+ regulator-name = "cam";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ };
+ fet6 {
+ regulator-name = "lcd_vdd";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+ fet7 {
+ regulator-name = "ts";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ };
+ };
+
+ charger {
+ compatible = "ti,tps65090-charger";
+ };
+ };
+ };
+
+ hdmi {
+ hdmi-en-supply = <&s5m_ldo8_reg>;
+ vdd-supply = <&s5m_ldo8_reg>;
+ vdd_osc-supply = <&s5m_ldo10_reg>;
+ vdd_pll-supply = <&s5m_ldo8_reg>;
+ };
+
+ fimd@14400000 {
+ status = "okay";
+ samsung,invert-vclk;
+ };
+
+ dp-controller@145B0000 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&dp_hpd>;
+ samsung,color-space = <0>;
+ samsung,dynamic-range = <0>;
+ samsung,ycbcr-coeff = <0>;
+ samsung,color-depth = <1>;
+ samsung,link-rate = <0x0a>;
+ samsung,lane-count = <1>;
+ samsung,hpd-gpio = <&gpc3 0 0>;
+
+ display-timings {
+ native-mode = <&timing1>;
+
+ timing1: timing@1 {
+ clock-frequency = <70589280>;
+ hactive = <1366>;
+ vactive = <768>;
+ hfront-porch = <40>;
+ hback-porch = <40>;
+ hsync-len = <32>;
+ vback-porch = <10>;
+ vfront-porch = <12>;
+ vsync-len = <6>;
+ };
+ };
+ };
+
+ fixed-rate-clocks {
+ xxti {
+ compatible = "samsung,clock-xxti";
+ clock-frequency = <24000000>;
+ };
+ };
+};
--
1.9.3
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos
2014-06-23 1:21 ` [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos Andreas Färber
@ 2014-06-23 3:21 ` Sachin Kamat
2014-06-23 23:06 ` Andreas Färber
2014-06-23 8:15 ` Lee Jones
1 sibling, 1 reply; 20+ messages in thread
From: Sachin Kamat @ 2014-06-23 3:21 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Doug Anderson, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Yadwinder Singh Brar, Tomasz Figa,
Sachin Kamat, open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
On Mon, Jun 23, 2014 at 6:51 AM, Andreas Färber <afaerber@suse.de> wrote:
> It's LDO2, not LD02.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
> Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> index d81ba30..55ab4f4 100644
> --- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> @@ -81,7 +81,7 @@ as per the datasheet of s2mps11.
> - valid values for n are:
> - S2MPS11: 1 to 38
> - S2MPS14: 1 to 25
> - - Example: LDO1, LD02, LDO28
> + - Example: LDO1, LDO2, LDO28
> - BUCKn
> - valid values for n are:
> - S2MPS11: 1 to 10
> diff --git a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> index d290988..2019131 100644
> --- a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> @@ -86,7 +86,7 @@ as per the datasheet of s5m8767.
>
> - LDOn
> - valid values for n are 1 to 28
> - - Example: LDO1, LD02, LDO28
> + - Example: LDO1, LDO2, LDO28
> - BUCKn
> - valid values for n are 1 to 9.
> - Example: BUCK1, BUCK2, BUCK9
> --
Very keen observation :)
Reviewed-by: Sachin Kamat <sachin.kamat@samsung.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax
2014-06-23 1:21 ` [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax Andreas Färber
@ 2014-06-23 3:23 ` Sachin Kamat
2014-06-23 8:15 ` Lee Jones
1 sibling, 0 replies; 20+ messages in thread
From: Sachin Kamat @ 2014-06-23 3:23 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Doug Anderson, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Sachin Kamat, Yadwinder Singh Brar,
open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
On Mon, Jun 23, 2014 at 6:51 AM, Andreas Färber <afaerber@suse.de> wrote:
> It's <1>, not 1.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> index 55ab4f4..99a0c52 100644
> --- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> @@ -96,7 +96,7 @@ Example:
>
> s2m_osc: clocks {
> compatible = "samsung,s2mps11-clk";
> - #clock-cells = 1;
> + #clock-cells = <1>;
> clock-output-names = "xx", "yy", "zz";
> };
>
> --
> 1.9.3
>
Reviewed-by: Sachin Kamat <sachin.kamat@samsung.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos
2014-06-23 1:21 ` [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos Andreas Färber
2014-06-23 3:21 ` Sachin Kamat
@ 2014-06-23 8:15 ` Lee Jones
1 sibling, 0 replies; 20+ messages in thread
From: Lee Jones @ 2014-06-23 8:15 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Doug Anderson, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Mark Brown,
Krzysztof Kozlowski, Yadwinder Singh Brar, Tomasz Figa,
Sachin Kamat, open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
On Mon, 23 Jun 2014, Andreas Färber wrote:
> It's LDO2, not LD02.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
> Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Applied, thanks.
> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> index d81ba30..55ab4f4 100644
> --- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> @@ -81,7 +81,7 @@ as per the datasheet of s2mps11.
> - valid values for n are:
> - S2MPS11: 1 to 38
> - S2MPS14: 1 to 25
> - - Example: LDO1, LD02, LDO28
> + - Example: LDO1, LDO2, LDO28
> - BUCKn
> - valid values for n are:
> - S2MPS11: 1 to 10
> diff --git a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> index d290988..2019131 100644
> --- a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
> @@ -86,7 +86,7 @@ as per the datasheet of s5m8767.
>
> - LDOn
> - valid values for n are 1 to 28
> - - Example: LDO1, LD02, LDO28
> + - Example: LDO1, LDO2, LDO28
> - BUCKn
> - valid values for n are 1 to 9.
> - Example: BUCK1, BUCK2, BUCK9
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax
2014-06-23 1:21 ` [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax Andreas Färber
2014-06-23 3:23 ` Sachin Kamat
@ 2014-06-23 8:15 ` Lee Jones
1 sibling, 0 replies; 20+ messages in thread
From: Lee Jones @ 2014-06-23 8:15 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Doug Anderson, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Mark Brown,
Krzysztof Kozlowski, Sachin Kamat, Yadwinder Singh Brar,
open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
On Mon, 23 Jun 2014, Andreas Färber wrote:
> It's <1>, not 1.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> index 55ab4f4..99a0c52 100644
> --- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
> @@ -96,7 +96,7 @@ Example:
>
> s2m_osc: clocks {
> compatible = "samsung,s2mps11-clk";
> - #clock-cells = 1;
> + #clock-cells = <1>;
> clock-output-names = "xx", "yy", "zz";
> };
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos
2014-06-23 1:21 ` [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos Andreas Färber
@ 2014-06-23 17:27 ` Doug Anderson
2014-06-25 10:47 ` Mark Rutland
0 siblings, 1 reply; 20+ messages in thread
From: Doug Anderson @ 2014-06-23 17:27 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Randy Dunlap, Mark Brown, Simon Glass, Michael Spang,
open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
Andreas,
On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
> It's vsys-l{1,2}-supply, not vsys_l{1,2}-supply.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/regulator/tps65090.txt | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
> index 34098023..ca69f5e 100644
> --- a/Documentation/devicetree/bindings/regulator/tps65090.txt
> +++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
> @@ -45,8 +45,8 @@ Example:
> infet5-supply = <&some_reg>;
> infet6-supply = <&some_reg>;
> infet7-supply = <&some_reg>;
> - vsys_l1-supply = <&some_reg>;
> - vsys_l2-supply = <&some_reg>;
> + vsys-l1-supply = <&some_reg>;
> + vsys-l2-supply = <&some_reg>;
Your change matches the code and all existing device trees in the
Linux kernel. I also see plenty of other bindings with dashes, so
this seems reasonable.
Reviewed-by: Doug Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-23 1:21 ` [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree Andreas Färber
@ 2014-06-23 19:47 ` Doug Anderson
2014-06-23 22:46 ` Andreas Färber
0 siblings, 1 reply; 20+ messages in thread
From: Doug Anderson @ 2014-06-23 19:47 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Ben Dooks, Kukjin Kim,
open list:OPEN FIRMWARE AND...,
moderated list:ARM PORT, open list, Olof Johansson,
Javier Martinez Canillas, tbroch, Tomasz Figa
Andreas,
Thanks for posting! A first pass on this is below...
On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
> Adds initial support for the HP Chromebook 11.
>
> Cc: Vincent Palatin <vpalatin@chromium.org>
> Cc: Doug Anderson <dianders@chromium.org>
> Cc: Stephan van Schaik <stephan@synkhronix.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/exynos5250-spring.dts | 556 ++++++++++++++++++++++++++++++++
> 2 files changed, 557 insertions(+)
> create mode 100644 arch/arm/boot/dts/exynos5250-spring.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 5986ff6..dc2c5aa 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -74,6 +74,7 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \
> exynos5250-arndale.dtb \
> exynos5250-smdk5250.dtb \
> exynos5250-snow.dtb \
> + exynos5250-spring.dtb \
> exynos5260-xyref5260.dtb \
> exynos5410-smdk5410.dtb \
> exynos5420-arndale-octa.dtb \
> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
> new file mode 100644
> index 0000000..e857d44
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
> @@ -0,0 +1,556 @@
> +/*
> + * Google Spring board device tree source
> + *
> + * Copyright (c) 2013 Google, Inc
> + * Copyright (c) 2014 SUSE LINUX Products GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/dts-v1/;
> +#include "exynos5250.dtsi"
> +#include "exynos5250-cros-common.dtsi"
It is possible we may want to backpedal on the use of
"exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
wasn't a fan of how it turned out.
The original idea was that it should include the arbitrary set of
things that are common between a chunk of Chrome OS boards. As more
boards were introduced things would need to migrate from the "common"
file to the board files.
At the moment the current conventional wisdom is that some duplication
is better than the confusing movement of everything back and forth.
See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.
> +/ {
> + model = "Google Spring";
> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
> +
> + pinctrl@11400000 {
The new best way to do things is to put this down at the bottom. See
exynos5420-peach-pit as an example:
&pinctrl_0 {
...
}
Note that I believe it was decided that top-level references like that
should be sorted alphabetically.
If you wanted to apply that run to exynos5250-snow I don't think it
would be a terrible idea.
> + s5m8767_dvs: s5m8767-dvs {
> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> +
> + s5m8767_ds: s5m8767-ds {
> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> +
> + tps65090_irq: tps65090-irq {
> + samsung,pins = "gpx2-6";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + s5m8767_irq: s5m8767-irq {
> + samsung,pins = "gpx3-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + hdmi_hpd_irq: hdmi-hpd-irq {
> + samsung,pins = "gpx3-7";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> + };
> +
> + pinctrl@13400000 {
> + hsic_reset: hsic-reset {
> + samsung,pins = "gpe1-0";
> + samsung,pin-function = <1>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
I'm pretty sure that the HSIC reset needed some funky code to make it
work and I'm not sure what the status of that is upstream
> + };
> +
> + vbat: vbat-fixed-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "vbat-supply";
> + regulator-boot-on;
> + };
> +
> + usb@12000000 {
> + status = "okay";
> + };
> +
> + usb3_vbus_reg: regulator-usb3 {
> + compatible = "regulator-fixed";
> + regulator-name = "P5.0V_USB3CON";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpe1 0 1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hsic_reset>;
> + enable-active-high;
> + };
> +
> + phy@12100000 {
> + vbus-supply = <&usb3_vbus_reg>;
> + };
> +
> + usb@12110000 {
> + samsung,vbus-gpio = <&gpx1 1 0>;
> + status = "okay";
> + };
> +
> + usb@12120000 {
> + status = "okay";
> + };
> +
> + mmc@12220000 {
> + /* MMC2 pins are used as GPIO for eDP bridge control. */
> + status = "disabled";
> + };
> +
> + mmc@12230000 {
> + status = "disabled";
> + };
> +
> + i2c@12C60000 {
> + max77686@09 {
There is no max77686 on spring. It uses s5m8767 in the place of max77686.
...you've got "status = disabled", just remove it.
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "disabled";
> +
> + rtc {
> + reg = <0x6>;
> + };
> + };
> +
> + s5m8767_pmic@66 {
> + compatible = "samsung,s5m8767-pmic";
> + reg = <0x66>;
> + interrupt-parent = <&gpx3>;
> + interrupts = <2 0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
> + wakeup-source;
> +
> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
> + <&gpd1 1 1>, /* DVS2 */
> + <&gpd1 2 1>; /* DVS3 */
> +
> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
> + <&gpx2 4 1>, /* SET2 */
> + <&gpx2 5 1>; /* SET3 */
The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.
> +
> + /*
> + * The following arrays of DVS voltages are not used, since we are
> + * not using GPIOs to control PMIC bucks, but they must be defined
> + * to please the driver.
> + */
> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
> + <1250000>, <1200000>,
> + <1150000>, <1100000>,
> + <1000000>, <950000>;
> +
> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
> + <1100000>, <1100000>,
> + <1000000>, <1000000>,
> + <1000000>, <1000000>;
> +
> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
> + <1200000>, <1200000>,
> + <1200000>, <1200000>,
> + <1200000>, <1200000>;
> +
> + clocks {
> + compatible = "samsung,s5m8767-clk";
> + #clock-cells = <1>;
> + clock-output-names = "en32khz_ap",
> + "en32khz_cp",
> + "en32khz_bt";
> + };
> +
> + regulators {
> + s5m_ldo4_reg: LDO4 {
> + regulator-name = "P1.0V_LDO_OUT4";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <0>;
I think that "op_mode" here is questionable. Adding Javier to the
thread who was looking at this for max77802 and possibly max77686.
> + };
> +
> + s5m_ldo5_reg: LDO5 {
> + regulator-name = "P1.0V_LDO_OUT5";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo6_reg: LDO6 {
> + regulator-name = "vdd_mydp";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo7_reg: LDO7 {
> + regulator-name = "P1.1V_LDO_OUT7";
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo8_reg: LDO8 {
> + regulator-name = "P1.0V_LDO_OUT8";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo10_reg: LDO10 {
> + regulator-name = "P1.8V_LDO_OUT10";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo11_reg: LDO11 {
> + regulator-name = "P1.8V_LDO_OUT11";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo12_reg: LDO12 {
> + regulator-name = "P3.0V_LDO_OUT12";
> + regulator-min-microvolt = <3000000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo13_reg: LDO13 {
> + regulator-name = "P1.8V_LDO_OUT13";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo14_reg: LDO14 {
> + regulator-name = "P1.8V_LDO_OUT14";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo15_reg: LDO15 {
> + regulator-name = "P1.0V_LDO_OUT15";
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo16_reg: LDO16 {
> + regulator-name = "P1.8V_LDO_OUT16";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + op_mode = <3>;
> + };
> +
> + s5m_ldo17_reg: LDO17 {
> + regulator-name = "P2.8V_LDO_OUT17";
> + regulator-min-microvolt = <2800000>;
> + regulator-max-microvolt = <2800000>;
> + regulator-always-on;
> + op_mode = <0>;
> + };
> +
> + s5m_ldo25_reg: LDO25 {
> + regulator-name = "vdd_bridge";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + op_mode = <1>;
> + };
> +
> + BUCK1 {
> + regulator-name = "vdd_mif";
> + regulator-min-microvolt = <950000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK2 {
> + regulator-name = "vdd_arm";
> + regulator-min-microvolt = <850000>;
> + regulator-max-microvolt = <1350000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK3 {
> + regulator-name = "vdd_int";
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK4 {
> + regulator-name = "vdd_g3d";
> + regulator-min-microvolt = <850000>;
> + regulator-max-microvolt = <1300000>;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> +
> + BUCK5 {
> + regulator-name = "P1.8V_BUCK_OUT5";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <1>;
> + };
> +
> + BUCK6 {
> + regulator-name = "P1.2V_BUCK_OUT6";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <0>;
> + };
> +
> + BUCK9 {
> + regulator-name = "vdd_ummc";
> + regulator-min-microvolt = <950000>;
> + regulator-max-microvolt = <3000000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <3>;
> + };
> + };
> + };
> + };
> +
> + i2c@12C70000 {
> + trackpad {
> + status = "disabled";
Having this bogus entry here doesn't add anything. Remove it until
the trackpad should be added. See http://crbug.com/371114 for a
slightly stale bug about trackpad.
> + };
> + };
> +
> + i2c@12CA0000 {
> + embedded-controller {
Add "cros_ec" alias like snow.
> + compatible = "google,cros-ec-i2c";
> + reg = <0x1e>;
> + interrupts = <6 0>;
> + interrupt-parent = <&gpx1>;
> + wakeup-source;
> +
> + keyboard-controller {
Don't include keyboard-controller here. Add:
#include "cros-ec-keyboard.dtsi"
...at the bottom. Note that I think that the spring EC has a special
"charger" key that it uses to indicate when a charger was plugged in
and unplugged. I'm not sure how that will end up getting supported
upstream but you could just leave it out for now.
> + compatible = "google,cros-ec-keyb";
> + keypad,num-rows = <8>;
> + keypad,num-columns = <13>;
Don't you need pinctrl here?
> + google,needs-ghost-filter;
> + linux,keymap = <
> + 0x0001007d /* L_META */
> + 0x0002003b /* F1 */
> + 0x00030030 /* B */
> + 0x00040044 /* F10 */
> + 0x00060031 /* N */
> + 0x0008000d /* = */
> + 0x000a0064 /* R_ALT */
> +
> + 0x01010001 /* ESC */
> + 0x0102003e /* F4 */
> + 0x01030022 /* G */
> + 0x01040041 /* F7 */
> + 0x01060023 /* H */
> + 0x01080028 /* ' */
> + 0x01090043 /* F9 */
> + 0x010b000e /* BKSPACE */
> +
> + 0x0200001d /* L_CTRL */
> + 0x0201000f /* TAB */
> + 0x0202003d /* F3 */
> + 0x02030014 /* T */
> + 0x02040040 /* F6 */
> + 0x0205001b /* ] */
> + 0x02060015 /* Y */
> + 0x02070056 /* 102ND */
> + 0x0208001a /* [ */
> + 0x02090042 /* F8 */
> +
> + 0x03010029 /* GRAVE */
> + 0x0302003c /* F2 */
> + 0x03030006 /* 5 */
> + 0x0304003f /* F5 */
> + 0x03060007 /* 6 */
> + 0x0308000c /* - */
> + 0x030b002b /* \ */
> +
> + 0x04000061 /* R_CTRL */
> + 0x0401001e /* A */
> + 0x04020020 /* D */
> + 0x04030021 /* F */
> + 0x0404001f /* S */
> + 0x04050025 /* K */
> + 0x04060024 /* J */
> + 0x04080027 /* ; */
> + 0x04090026 /* L */
> + 0x040a002b /* \ */
> + 0x040b001c /* ENTER */
> +
> + 0x0501002c /* Z */
> + 0x0502002e /* C */
> + 0x0503002f /* V */
> + 0x0504002d /* X */
> + 0x05050033 /* , */
> + 0x05060032 /* M */
> + 0x0507002a /* L_SHIFT */
> + 0x05080035 /* / */
> + 0x05090034 /* . */
> + 0x050B0039 /* SPACE */
> +
> + 0x06010002 /* 1 */
> + 0x06020004 /* 3 */
> + 0x06030005 /* 4 */
> + 0x06040003 /* 2 */
> + 0x06050009 /* 8 */
> + 0x06060008 /* 7 */
> + 0x0608000b /* 0 */
> + 0x0609000a /* 9 */
> + 0x060a0038 /* L_ALT */
> + 0x060b006c /* DOWN */
> + 0x060c006a /* RIGHT */
> +
> + 0x07010010 /* Q */
> + 0x07020012 /* E */
> + 0x07030013 /* R */
> + 0x07040011 /* W */
> + 0x07050017 /* I */
> + 0x07060016 /* U */
> + 0x07070036 /* R_SHIFT */
> + 0x07080019 /* P */
> + 0x07090018 /* O */
> + 0x070b0067 /* UP */
> + 0x070c0069 /* LEFT */
> + >;
> + };
> + };
> +
> + power-regulator {
> + compatible = "ti,tps65090";
I doubt tps65090 will "just work". Does it?
On spring the tps65090 is not directly on the same i2c bus as the EC.
It's actually hidden behind the EC.
Locally in the ChromeOS tree there appears to be a forked copy of the
65090 regulator driver that's in use just for spring. See this from
the ChromeOS 3.8 tree:
./drivers/regulator/tps65090-regulator.c
./drivers/regulator/cros_ec-tps65090.c
The Spring version of the driver sends EC commands directly to access
the tps65090.
It is possible (untested) that you could also talk to tps65090 over an
i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
tunnel, but you may be able to extend the tunnel to export an i2c
tunnel for spring using something like
<https://chromium-review.googlesource.com/66116>
> + reg = <0x48>;
> +
> + /*
> + * Config irq to disable internal pulls
> + * even though we run in polling mode.
> + */
> + pinctrl-names = "default";
> + pinctrl-0 = <&tps65090_irq>;
> +
> + vsys1-supply = <&vbat>;
> + vsys2-supply = <&vbat>;
> + vsys3-supply = <&vbat>;
> + infet1-supply = <&vbat>;
> + infet2-supply = <&vbat>;
> + infet3-supply = <&vbat>;
> + infet4-supply = <&vbat>;
> + infet5-supply = <&vbat>;
> + infet6-supply = <&vbat>;
> + infet7-supply = <&vbat>;
> + vsys-l1-supply = <&vbat>;
> + vsys-l2-supply = <&vbat>;
> +
> + regulators {
> + fet1 {
> + regulator-name = "vcd_led";
> + regulator-min-microvolt = <12000000>;
> + regulator-max-microvolt = <12000000>;
> + };
> + fet3 {
> + regulator-name = "wwan_r";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
> + fet5 {
> + regulator-name = "cam";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-always-on;
> + };
> + fet6 {
> + regulator-name = "lcd_vdd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> + fet7 {
> + regulator-name = "ts";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> + };
> +
> + charger {
> + compatible = "ti,tps65090-charger";
> + };
> + };
> + };
> +
> + hdmi {
> + hdmi-en-supply = <&s5m_ldo8_reg>;
> + vdd-supply = <&s5m_ldo8_reg>;
> + vdd_osc-supply = <&s5m_ldo10_reg>;
> + vdd_pll-supply = <&s5m_ldo8_reg>;
> + };
> +
> + fimd@14400000 {
> + status = "okay";
> + samsung,invert-vclk;
> + };
> +
> + dp-controller@145B0000 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&dp_hpd>;
This is probably not right. It looks as if spring uses gpc3-0 for
display port HPD (as a GPIO). The upstream has this in the
exynos5250-pinctrl.dtsi as a different pin.
I think you'll need to define your own pinctrl here.
> + samsung,color-space = <0>;
> + samsung,dynamic-range = <0>;
> + samsung,ycbcr-coeff = <0>;
> + samsung,color-depth = <1>;
> + samsung,link-rate = <0x0a>;
> + samsung,lane-count = <1>;
> + samsung,hpd-gpio = <&gpc3 0 0>;
> +
> + display-timings {
> + native-mode = <&timing1>;
> +
> + timing1: timing@1 {
> + clock-frequency = <70589280>;
> + hactive = <1366>;
> + vactive = <768>;
> + hfront-porch = <40>;
> + hback-porch = <40>;
> + hsync-len = <32>;
> + vback-porch = <10>;
> + vfront-porch = <12>;
> + vsync-len = <6>;
> + };
> + };
> + };
> +
> + fixed-rate-clocks {
> + xxti {
> + compatible = "samsung,clock-xxti";
> + clock-frequency = <24000000>;
> + };
> + };
> +};
-Doug
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-23 19:47 ` Doug Anderson
@ 2014-06-23 22:46 ` Andreas Färber
2014-06-24 4:05 ` Doug Anderson
0 siblings, 1 reply; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 22:46 UTC (permalink / raw)
To: Doug Anderson
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Ben Dooks, Kukjin Kim, OPEN FIRMWARE AND...,
ARM PORT, LKML, Olof Johansson, Javier Martinez Canillas, tbroch,
Tomasz Figa
Hi Doug,
Am 23.06.2014 21:47, schrieb Doug Anderson:
> Thanks for posting! A first pass on this is below...
Thanks a lot for your quick review! My first big .dts patch, and no
datasheets for the hardware at hand as a user.
A first pass of replies to my defense. ;)
> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
[...]
>> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
>> new file mode 100644
>> index 0000000..e857d44
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
>> @@ -0,0 +1,556 @@
>> +/*
>> + * Google Spring board device tree source
>> + *
>> + * Copyright (c) 2013 Google, Inc
>> + * Copyright (c) 2014 SUSE LINUX Products GmbH
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/dts-v1/;
>> +#include "exynos5250.dtsi"
>> +#include "exynos5250-cros-common.dtsi"
>
> It is possible we may want to backpedal on the use of
> "exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
> wasn't a fan of how it turned out.
>
> The original idea was that it should include the arbitrary set of
> things that are common between a chunk of Chrome OS boards. As more
> boards were introduced things would need to migrate from the "common"
> file to the board files.
>
> At the moment the current conventional wisdom is that some duplication
> is better than the confusing movement of everything back and forth.
> See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.
>
>
>> +/ {
>> + model = "Google Spring";
>> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
>> +
>> + pinctrl@11400000 {
>
> The new best way to do things is to put this down at the bottom. See
> exynos5420-peach-pit as an example:
>
> &pinctrl_0 {
> ...
> }
>
> Note that I believe it was decided that top-level references like that
> should be sorted alphabetically.
Thanks for the hint. (My chosen sort order here was by address.)
> If you wanted to apply that run to exynos5250-snow I don't think it
> would be a terrible idea.
I can of course apply changes to Snow, but I cannot test them myself.
>> + s5m8767_dvs: s5m8767-dvs {
>> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <1>;
>> + samsung,pin-drv = <0>;
>> + };
>> +
>> + s5m8767_ds: s5m8767-ds {
>> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <1>;
>> + samsung,pin-drv = <0>;
>> + };
>> +
>> + tps65090_irq: tps65090-irq {
>> + samsung,pins = "gpx2-6";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <0>;
>> + samsung,pin-drv = <0>;
>> + };
>> +
>> + s5m8767_irq: s5m8767-irq {
>> + samsung,pins = "gpx3-2";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <0>;
>> + samsung,pin-drv = <0>;
>> + };
>> +
>> + hdmi_hpd_irq: hdmi-hpd-irq {
>> + samsung,pins = "gpx3-7";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <1>;
>> + samsung,pin-drv = <0>;
>> + };
>> + };
>> +
>> + pinctrl@13400000 {
>> + hsic_reset: hsic-reset {
>> + samsung,pins = "gpe1-0";
>> + samsung,pin-function = <1>;
>> + samsung,pin-pud = <0>;
>> + samsung,pin-drv = <0>;
>> + };
>
> I'm pretty sure that the HSIC reset needed some funky code to make it
> work and I'm not sure what the status of that is upstream
Yeah, you mentioned something along those lines. However it's the
equivalent of the usb3-vbus-en in -snow.dts. Rename or drop?
>> + };
>> +
>> + vbat: vbat-fixed-regulator {
>> + compatible = "regulator-fixed";
>> + regulator-name = "vbat-supply";
>> + regulator-boot-on;
>> + };
>> +
>> + usb@12000000 {
>> + status = "okay";
>> + };
>> +
>> + usb3_vbus_reg: regulator-usb3 {
>> + compatible = "regulator-fixed";
>> + regulator-name = "P5.0V_USB3CON";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + gpio = <&gpe1 0 1>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&hsic_reset>;
>> + enable-active-high;
>> + };
>> +
>> + phy@12100000 {
>> + vbus-supply = <&usb3_vbus_reg>;
>> + };
>> +
>> + usb@12110000 {
>> + samsung,vbus-gpio = <&gpx1 1 0>;
>> + status = "okay";
>> + };
>> +
>> + usb@12120000 {
>> + status = "okay";
>> + };
>> +
>> + mmc@12220000 {
>> + /* MMC2 pins are used as GPIO for eDP bridge control. */
>> + status = "disabled";
>> + };
>> +
>> + mmc@12230000 {
>> + status = "disabled";
>> + };
>> +
>> + i2c@12C60000 {
>> + max77686@09 {
>
> There is no max77686 on spring. It uses s5m8767 in the place of max77686.
>
> ...you've got "status = disabled", just remove it.
That's because it's inherited from exynos5250-cros-common.dtsi.
The rtc that the system successfully uses is "s5m-rtc", which I guess is
on the s5m8767 then? (Confusing that 3.8 Spring had this rtc node
despite Snow's max77686 not having it.)
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + status = "disabled";
>> +
>> + rtc {
>> + reg = <0x6>;
>> + };
>> + };
>> +
>> + s5m8767_pmic@66 {
>> + compatible = "samsung,s5m8767-pmic";
>> + reg = <0x66>;
>> + interrupt-parent = <&gpx3>;
>> + interrupts = <2 0>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
>> + wakeup-source;
>> +
>> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
>> + <&gpd1 1 1>, /* DVS2 */
>> + <&gpd1 2 1>; /* DVS3 */
>> +
>> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
>> + <&gpx2 4 1>, /* SET2 */
>> + <&gpx2 5 1>; /* SET3 */
>
> The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.
>
>
>> +
>> + /*
>> + * The following arrays of DVS voltages are not used, since we are
>> + * not using GPIOs to control PMIC bucks, but they must be defined
>> + * to please the driver.
>> + */
>> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
>> + <1250000>, <1200000>,
>> + <1150000>, <1100000>,
>> + <1000000>, <950000>;
>> +
>> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
>> + <1100000>, <1100000>,
>> + <1000000>, <1000000>,
>> + <1000000>, <1000000>;
>> +
>> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
>> + <1200000>, <1200000>,
>> + <1200000>, <1200000>,
>> + <1200000>, <1200000>;
>> +
>> + clocks {
>> + compatible = "samsung,s5m8767-clk";
>> + #clock-cells = <1>;
>> + clock-output-names = "en32khz_ap",
>> + "en32khz_cp",
>> + "en32khz_bt";
>> + };
>> +
>> + regulators {
>> + s5m_ldo4_reg: LDO4 {
>> + regulator-name = "P1.0V_LDO_OUT4";
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1000000>;
>> + regulator-always-on;
>> + op_mode = <0>;
>
> I think that "op_mode" here is questionable. Adding Javier to the
> thread who was looking at this for max77802 and possibly max77686.
Confirming that this op_mode is present in the original 3.8 device tree.
(I used dtc to compile my /proc/device-tree tarball back to .dts, so it
has hexadecimal <0x0> but that shouldn't matter to dtc AFAIU.)
>> + };
>> +
>> + s5m_ldo5_reg: LDO5 {
>> + regulator-name = "P1.0V_LDO_OUT5";
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1000000>;
>> + regulator-always-on;
>> + op_mode = <0>;
>> + };
>> +
>> + s5m_ldo6_reg: LDO6 {
>> + regulator-name = "vdd_mydp";
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1000000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo7_reg: LDO7 {
>> + regulator-name = "P1.1V_LDO_OUT7";
>> + regulator-min-microvolt = <1100000>;
>> + regulator-max-microvolt = <1100000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo8_reg: LDO8 {
>> + regulator-name = "P1.0V_LDO_OUT8";
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1000000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo10_reg: LDO10 {
>> + regulator-name = "P1.8V_LDO_OUT10";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo11_reg: LDO11 {
>> + regulator-name = "P1.8V_LDO_OUT11";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + op_mode = <0>;
>> + };
>> +
>> + s5m_ldo12_reg: LDO12 {
>> + regulator-name = "P3.0V_LDO_OUT12";
>> + regulator-min-microvolt = <3000000>;
>> + regulator-max-microvolt = <3000000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo13_reg: LDO13 {
>> + regulator-name = "P1.8V_LDO_OUT13";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + op_mode = <0>;
>> + };
>> +
>> + s5m_ldo14_reg: LDO14 {
>> + regulator-name = "P1.8V_LDO_OUT14";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo15_reg: LDO15 {
>> + regulator-name = "P1.0V_LDO_OUT15";
>> + regulator-min-microvolt = <1000000>;
>> + regulator-max-microvolt = <1000000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo16_reg: LDO16 {
>> + regulator-name = "P1.8V_LDO_OUT16";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + op_mode = <3>;
>> + };
>> +
>> + s5m_ldo17_reg: LDO17 {
>> + regulator-name = "P2.8V_LDO_OUT17";
>> + regulator-min-microvolt = <2800000>;
>> + regulator-max-microvolt = <2800000>;
>> + regulator-always-on;
>> + op_mode = <0>;
>> + };
>> +
>> + s5m_ldo25_reg: LDO25 {
>> + regulator-name = "vdd_bridge";
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + regulator-always-on;
>> + op_mode = <1>;
>> + };
>> +
>> + BUCK1 {
>> + regulator-name = "vdd_mif";
>> + regulator-min-microvolt = <950000>;
>> + regulator-max-microvolt = <1300000>;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + op_mode = <3>;
>> + };
>> +
>> + BUCK2 {
>> + regulator-name = "vdd_arm";
>> + regulator-min-microvolt = <850000>;
>> + regulator-max-microvolt = <1350000>;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + op_mode = <3>;
>> + };
>> +
>> + BUCK3 {
>> + regulator-name = "vdd_int";
>> + regulator-min-microvolt = <900000>;
>> + regulator-max-microvolt = <1200000>;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + op_mode = <3>;
>> + };
>> +
>> + BUCK4 {
>> + regulator-name = "vdd_g3d";
>> + regulator-min-microvolt = <850000>;
>> + regulator-max-microvolt = <1300000>;
>> + regulator-boot-on;
>> + op_mode = <3>;
>> + };
>> +
>> + BUCK5 {
>> + regulator-name = "P1.8V_BUCK_OUT5";
>> + regulator-min-microvolt = <1800000>;
>> + regulator-max-microvolt = <1800000>;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + op_mode = <1>;
>> + };
>> +
>> + BUCK6 {
>> + regulator-name = "P1.2V_BUCK_OUT6";
>> + regulator-min-microvolt = <1200000>;
>> + regulator-max-microvolt = <1200000>;
>> + regulator-always-on;
>> + regulator-boot>
>
-on;
>> + op_mode = <0>;
>> + };
>> +
>> + BUCK9 {
>> + regulator-name = "vdd_ummc";
>> + regulator-min-microvolt = <950000>;
>> + regulator-max-microvolt = <3000000>;
>> + regulator-always-on;
>> + regulator-boot-on;
>> + op_mode = <3>;
>> + };
>> + };
>> + };
>> + };
>> +
>> + i2c@12C70000 {
>> + trackpad {
>> + status = "disabled";
>
> Having this bogus entry here doesn't add anything. Remove it until
> the trackpad should be added. See http://crbug.com/371114 for a
> slightly stale bug about trackpad.
You misunderstand: This resolves an error about the cypress,cyapa
inherited from -cros-common.dtsi. Spring uses a different device and two
different I2C addresses.
Nodes named trackpad-bootloader and trackpad-alt are prepared here:
https://github.com/afaerber/linux/commits/spring-next
>> + };
>> + };
>> +
>> + i2c@12CA0000 {
>> + embedded-controller {
>
> Add "cros_ec" alias like snow.
>
>
>> + compatible = "google,cros-ec-i2c";
>> + reg = <0x1e>;
>> + interrupts = <6 0>;
>> + interrupt-parent = <&gpx1>;
>> + wakeup-source;
>> +
>> + keyboard-controller {
>
> Don't include keyboard-controller here. Add:
>
> #include "cros-ec-keyboard.dtsi"
>
> ...at the bottom. Note that I think that the spring EC has a special
> "charger" key that it uses to indicate when a charger was plugged in
> and unplugged. I'm not sure how that will end up getting supported
> upstream but you could just leave it out for now.
Is there such a pending patch for snow? That's what I modeled after.
Where is cros-ec-keyboard.dtsi? Don't see it in
http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/arch/arm/boot/dts?h=for-next
or
http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/include/dt-bindings?h=for-next
Are you proposing I factor it out?
>> + compatible = "google,cros-ec-keyb";
>> + keypad,num-rows = <8>;
>> + keypad,num-columns = <13>;
>
> Don't you need pinctrl here?
The keyboard is usable; what would pinctrl be needed for? There's none
in the 3.8 device tree.
>> + google,needs-ghost-filter;
>> + linux,keymap = <
>> + 0x0001007d /* L_META */
>> + 0x0002003b /* F1 */
>> + 0x00030030 /* B */
>> + 0x00040044 /* F10 */
>> + 0x00060031 /* N */
>> + 0x0008000d /* = */
>> + 0x000a0064 /* R_ALT */
>> +
>> + 0x01010001 /* ESC */
>> + 0x0102003e /* F4 */
>> + 0x01030022 /* G */
>> + 0x01040041 /* F7 */
>> + 0x01060023 /* H */
>> + 0x01080028 /* ' */
>> + 0x01090043 /* F9 */
>> + 0x010b000e /* BKSPACE */
>> +
>> + 0x0200001d /* L_CTRL */
>> + 0x0201000f /* TAB */
>> + 0x0202003d /* F3 */
>> + 0x02030014 /* T */
>> + 0x02040040 /* F6 */
>> + 0x0205001b /* ] */
>> + 0x02060015 /* Y */
>> + 0x02070056 /* 102ND */
>> + 0x0208001a /* [ */
>> + 0x02090042 /* F8 */
>> +
>> + 0x03010029 /* GRAVE */
>> + 0x0302003c /* F2 */
>> + 0x03030006 /* 5 */
>> + 0x0304003f /* F5 */
>> + 0x03060007 /* 6 */
>> + 0x0308000c /* - */
>> + 0x030b002b /* \ */
>> +
>> + 0x04000061 /* R_CTRL */
>> + 0x0401001e /* A */
>> + 0x04020020 /* D */
>> + 0x04030021 /* F */
>> + 0x0404001f /* S */
>> + 0x04050025 /* K */
>> + 0x04060024 /* J */
>> + 0x04080027 /* ; */
>> + 0x04090026 /* L */
>> + 0x040a002b /* \ */
>> + 0x040b001c /* ENTER */
>> +
>> + 0x0501002c /* Z */
>> + 0x0502002e /* C */
>> + 0x0503002f /* V */
>> + 0x0504002d /* X */
>> + 0x05050033 /* , */
>> + 0x05060032 /* M */
>> + 0x0507002a /* L_SHIFT */
>> + 0x05080035 /* / */
>> + 0x05090034 /* . */
>> + 0x050B0039 /* SPACE */
>> +
>> + 0x06010002 /* 1 */
>> + 0x06020004 /* 3 */
>> + 0x06030005 /* 4 */
>> + 0x06040003 /* 2 */
>> + 0x06050009 /* 8 */
>> + 0x06060008 /* 7 */
>> + 0x0608000b /* 0 */
>> + 0x0609000a /* 9 */
>> + 0x060a0038 /* L_ALT */
>> + 0x060b006c /* DOWN */
>> + 0x060c006a /* RIGHT */
>> +
>> + 0x07010010 /* Q */
>> + 0x07020012 /* E */
>> + 0x07030013 /* R */
>> + 0x07040011 /* W */
>> + 0x07050017 /* I */
>> + 0x07060016 /* U */
>> + 0x07070036 /* R_SHIFT */
>> + 0x07080019 /* P */
>> + 0x07090018 /* O */
>> + 0x070b0067 /* UP */
>> + 0x070c0069 /* LEFT */
>> + >;
>> + };
>> + };
>> +
>> + power-regulator {
>> + compatible = "ti,tps65090";
>
> I doubt tps65090 will "just work". Does it?
Good question. How to tell? :) Not familiar with clocks and regulators.
I don't see the nodes referenced anywhere, so I could just try to drop
(as I would, as per my cover letter, propose for anything non-essential
that turns controversial).
If we drop tps65090, can we safely drop vbat-fixed-regulator as well?
> On spring the tps65090 is not directly on the same i2c bus as the EC.
> It's actually hidden behind the EC.
>
> Locally in the ChromeOS tree there appears to be a forked copy of the
> 65090 regulator driver that's in use just for spring. See this from
> the ChromeOS 3.8 tree:
>
> ./drivers/regulator/tps65090-regulator.c
> ./drivers/regulator/cros_ec-tps65090.c
>
> The Spring version of the driver sends EC commands directly to access
> the tps65090.
>
> It is possible (untested) that you could also talk to tps65090 over an
> i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
> tunnel, but you may be able to extend the tunnel to export an i2c
> tunnel for spring using something like
> <https://chromium-review.googlesource.com/66116>
The SBS battery thingy (not in this patch) should also be behind some
tunnel. There's none defined in -cros-common.dtsi or in my patch, and
still it shows up in dmesg to my surprise, complaining "Couldn't read
remote-bus property".
>> + reg = <0x48>;
>> +
>> + /*
>> + * Config irq to disable internal pulls
>> + * even though we run in polling mode.
>> + */
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&tps65090_irq>;
>> +
>> + vsys1-supply = <&vbat>;
>> + vsys2-supply = <&vbat>;
>> + vsys3-supply = <&vbat>;
>> + infet1-supply = <&vbat>;
>> + infet2-supply = <&vbat>;
>> + infet3-supply = <&vbat>;
>> + infet4-supply = <&vbat>;
>> + infet5-supply = <&vbat>;
>> + infet6-supply = <&vbat>;
>> + infet7-supply = <&vbat>;
>> + vsys-l1-supply = <&vbat>;
>> + vsys-l2-supply = <&vbat>;
>> +
>> + regulators {
>> + fet1 {
>> + regulator-name = "vcd_led";
>> + regulator-min-microvolt = <12000000>;
>> + regulator-max-microvolt = <12000000>;
>> + };
>> + fet3 {
>> + regulator-name = "wwan_r";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-always-on;
>> + };
>> + fet5 {
>> + regulator-name = "cam";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + regulator-always-on;
>> + };
>> + fet6 {
>> + regulator-name = "lcd_vdd";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + };
>> + fet7 {
>> + regulator-name = "ts";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + };
>> + };
>> +
>> + charger {
>> + compatible = "ti,tps65090-charger";
>> + };
>> + };
>> + };
>> +
>> + hdmi {
>> + hdmi-en-supply = <&s5m_ldo8_reg>;
>> + vdd-supply = <&s5m_ldo8_reg>;
>> + vdd_osc-supply = <&s5m_ldo10_reg>;
>> + vdd_pll-supply = <&s5m_ldo8_reg>;
>> + };
>> +
>> + fimd@14400000 {
>> + status = "okay";
>> + samsung,invert-vclk;
>> + };
>> +
>> + dp-controller@145B0000 {
>> + status = "okay";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&dp_hpd>;
>
> This is probably not right. It looks as if spring uses gpc3-0 for
> display port HPD (as a GPIO). The upstream has this in the
> exynos5250-pinctrl.dtsi as a different pin.
>
> I think you'll need to define your own pinctrl here.
>
>
>> + samsung,color-space = <0>;
>> + samsung,dynamic-range = <0>;
>> + samsung,ycbcr-coeff = <0>;
>> + samsung,color-depth = <1>;
>> + samsung,link-rate = <0x0a>;
>> + samsung,lane-count = <1>;
>> + samsung,hpd-gpio = <&gpc3 0 0>;
>> +
>> + display-timings {
>> + native-mode = <&timing1>;
>> +
>> + timing1: timing@1 {
>> + clock-frequency = <70589280>;
>> + hactive = <1366>;
>> + vactive = <768>;
>> + hfront-porch = <40>;
>> + hback-porch = <40>;
>> + hsync-len = <32>;
>> + vback-porch = <10>;
>> + vfront-porch = <12>;
>> + vsync-len = <6>;
>> + };
>> + };
>> + };
>> +
>> + fixed-rate-clocks {
>> + xxti {
>> + compatible = "samsung,clock-xxti";
>> + clock-frequency = <24000000>;
>> + };
>> + };
>> +};
Will check on the other suggestions.
Thanks,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos
2014-06-23 3:21 ` Sachin Kamat
@ 2014-06-23 23:06 ` Andreas Färber
2014-06-23 23:20 ` Doug Anderson
0 siblings, 1 reply; 20+ messages in thread
From: Andreas Färber @ 2014-06-23 23:06 UTC (permalink / raw)
To: Sachin Kamat
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Doug Anderson, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Yadwinder Singh Brar, Tomasz Figa,
Sachin Kamat, OPEN FIRMWARE AND...,
DOCUMENTATION, LKML
Am 23.06.2014 05:21, schrieb Sachin Kamat:
> On Mon, Jun 23, 2014 at 6:51 AM, Andreas Färber <afaerber@suse.de> wrote:
>> It's LDO2, not LD02.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> Documentation/devicetree/bindings/mfd/s2mps11.txt | 2 +-
>> Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/s2mps11.txt b/Documentation/devicetree/bindings/mfd/s2mps11.txt
>> index d81ba30..55ab4f4 100644
>> --- a/Documentation/devicetree/bindings/mfd/s2mps11.txt
>> +++ b/Documentation/devicetree/bindings/mfd/s2mps11.txt
>> @@ -81,7 +81,7 @@ as per the datasheet of s2mps11.
>> - valid values for n are:
>> - S2MPS11: 1 to 38
>> - S2MPS14: 1 to 25
>> - - Example: LDO1, LD02, LDO28
>> + - Example: LDO1, LDO2, LDO28
>> - BUCKn
>> - valid values for n are:
>> - S2MPS11: 1 to 10
>> diff --git a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
>> index d290988..2019131 100644
>> --- a/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
>> +++ b/Documentation/devicetree/bindings/regulator/s5m8767-regulator.txt
>> @@ -86,7 +86,7 @@ as per the datasheet of s5m8767.
>>
>> - LDOn
>> - valid values for n are 1 to 28
>> - - Example: LDO1, LD02, LDO28
>> + - Example: LDO1, LDO2, LDO28
>> - BUCKn
>> - valid values for n are 1 to 9.
>> - Example: BUCK1, BUCK2, BUCK9
>> --
>
> Very keen observation :)
>
> Reviewed-by: Sachin Kamat <sachin.kamat@samsung.com>
A font that distinguishes the zero with a dot or dash helps! :)
I was wondering which character to type, and found two undocumented
s5m8767_pmic properties downstream (s5m-core,enable-low-jitter and
s5m-core,device_type = <0x2>), which I then left out.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos
2014-06-23 23:06 ` Andreas Färber
@ 2014-06-23 23:20 ` Doug Anderson
0 siblings, 0 replies; 20+ messages in thread
From: Doug Anderson @ 2014-06-23 23:20 UTC (permalink / raw)
To: Andreas Färber
Cc: Sachin Kamat, linux-samsung-soc, Stephan van Schaik,
Vincent Palatin, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Randy Dunlap, Lee Jones, Mark Brown,
Krzysztof Kozlowski, Yadwinder Singh Brar, Tomasz Figa,
Sachin Kamat, OPEN FIRMWARE AND...,
DOCUMENTATION, LKML
Andreas,
On Mon, Jun 23, 2014 at 4:06 PM, Andreas Färber <afaerber@suse.de> wrote:
> I was wondering which character to type, and found two undocumented
> s5m8767_pmic properties downstream (s5m-core,enable-low-jitter and
> s5m-core,device_type = <0x2>), which I then left out.
I don't know much about "s5m-core,device_type", but I doubt it's
needed. You can see <http://crosreview.com/42202> for details. I
haven't looked but I'd bet that we just get this from the compatible
string now.
I did do a (very!) quick look and I see that low-jitter was originally
implemented in the local 3.4 kernel at <http://crosreview.com/43624>.
...and the local 3.8 kernel at <http://crosreview.com/66037>.
NOTE: it's pretty important to make sure low-jitter is turned on for
Chromebooks if you actually want full functionality. At least on
exynos5250-snow (with the max77686 PMIC) you'd get occasional (and
very strange and very hard to debug) TPM errors if you didn't have
low-jitter. The TPM is part of the security model on Chromebooks and
you might have a hard time accessing the encrypted parts of the disk
without it.
-Doug
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-23 22:46 ` Andreas Färber
@ 2014-06-24 4:05 ` Doug Anderson
2014-06-24 10:06 ` Javier Martinez Canillas
2014-06-24 15:06 ` Vincent Palatin
0 siblings, 2 replies; 20+ messages in thread
From: Doug Anderson @ 2014-06-24 4:05 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Ben Dooks, Kukjin Kim, OPEN FIRMWARE AND...,
ARM PORT, LKML, Olof Johansson, Javier Martinez Canillas, tbroch,
Tomasz Figa, jwerner
Andreas,
On Mon, Jun 23, 2014 at 3:46 PM, Andreas Färber <afaerber@suse.de> wrote:
> Hi Doug,
>
> Am 23.06.2014 21:47, schrieb Doug Anderson:
>> Thanks for posting! A first pass on this is below...
>
> Thanks a lot for your quick review! My first big .dts patch, and no
> datasheets for the hardware at hand as a user.
>
> A first pass of replies to my defense. ;)
>
>> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
> [...]
>>> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
>>> new file mode 100644
>>> index 0000000..e857d44
>>> --- /dev/null
>>> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
>>> @@ -0,0 +1,556 @@
>>> +/*
>>> + * Google Spring board device tree source
>>> + *
>>> + * Copyright (c) 2013 Google, Inc
>>> + * Copyright (c) 2014 SUSE LINUX Products GmbH
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +/dts-v1/;
>>> +#include "exynos5250.dtsi"
>>> +#include "exynos5250-cros-common.dtsi"
>>
>> It is possible we may want to backpedal on the use of
>> "exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
>> wasn't a fan of how it turned out.
>>
>> The original idea was that it should include the arbitrary set of
>> things that are common between a chunk of Chrome OS boards. As more
>> boards were introduced things would need to migrate from the "common"
>> file to the board files.
>>
>> At the moment the current conventional wisdom is that some duplication
>> is better than the confusing movement of everything back and forth.
>> See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.
>>
>>
>>> +/ {
>>> + model = "Google Spring";
>>> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
>>> +
>>> + pinctrl@11400000 {
>>
>> The new best way to do things is to put this down at the bottom. See
>> exynos5420-peach-pit as an example:
>>
>> &pinctrl_0 {
>> ...
>> }
>>
>> Note that I believe it was decided that top-level references like that
>> should be sorted alphabetically.
>
> Thanks for the hint. (My chosen sort order here was by address.)
>
>> If you wanted to apply that run to exynos5250-snow I don't think it
>> would be a terrible idea.
>
> I can of course apply changes to Snow, but I cannot test them myself.
If you want to send up a patch like that I'm happy to give it a once
over and also to test it. ...but don't feel obligated
>>> + s5m8767_dvs: s5m8767-dvs {
>>> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <1>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>> +
>>> + s5m8767_ds: s5m8767-ds {
>>> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <1>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>> +
>>> + tps65090_irq: tps65090-irq {
>>> + samsung,pins = "gpx2-6";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <0>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>> +
>>> + s5m8767_irq: s5m8767-irq {
>>> + samsung,pins = "gpx3-2";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <0>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>> +
>>> + hdmi_hpd_irq: hdmi-hpd-irq {
>>> + samsung,pins = "gpx3-7";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <1>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>> + };
>>> +
>>> + pinctrl@13400000 {
>>> + hsic_reset: hsic-reset {
>>> + samsung,pins = "gpe1-0";
>>> + samsung,pin-function = <1>;
>>> + samsung,pin-pud = <0>;
>>> + samsung,pin-drv = <0>;
>>> + };
>>
>> I'm pretty sure that the HSIC reset needed some funky code to make it
>> work and I'm not sure what the status of that is upstream
>
> Yeah, you mentioned something along those lines. However it's the
> equivalent of the usb3-vbus-en in -snow.dts. Rename or drop?
On snow locally I see USB2 vbus is gpx1-1 and USB3 vbus is gpx2-7. I
don't see that in spring.
This will take more time than I have right now to track down. I added
Julius to the thread in case he has time to answer and can suggest
what to do for upstream purposes. I may be able to look more
tomorrow.
You can always send up the next version and include this and we'll
look at it again.
>>> + };
>>> +
>>> + vbat: vbat-fixed-regulator {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "vbat-supply";
>>> + regulator-boot-on;
>>> + };
>>> +
>>> + usb@12000000 {
>>> + status = "okay";
>>> + };
>>> +
>>> + usb3_vbus_reg: regulator-usb3 {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "P5.0V_USB3CON";
>>> + regulator-min-microvolt = <5000000>;
>>> + regulator-max-microvolt = <5000000>;
>>> + gpio = <&gpe1 0 1>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&hsic_reset>;
>>> + enable-active-high;
>>> + };
>>> +
>>> + phy@12100000 {
>>> + vbus-supply = <&usb3_vbus_reg>;
>>> + };
>>> +
>>> + usb@12110000 {
>>> + samsung,vbus-gpio = <&gpx1 1 0>;
>>> + status = "okay";
>>> + };
>>> +
>>> + usb@12120000 {
>>> + status = "okay";
>>> + };
>>> +
>>> + mmc@12220000 {
>>> + /* MMC2 pins are used as GPIO for eDP bridge control. */
>>> + status = "disabled";
>>> + };
>>> +
>>> + mmc@12230000 {
>>> + status = "disabled";
>>> + };
>>> +
>>> + i2c@12C60000 {
>>> + max77686@09 {
>>
>> There is no max77686 on spring. It uses s5m8767 in the place of max77686.
>>
>> ...you've got "status = disabled", just remove it.
>
> That's because it's inherited from exynos5250-cros-common.dtsi.
Ah, right. So if we're keeping cros-common the proper thing to do is
to move it out of cros-common in a patch before this one. ...but I
think people might be happier if you simply don't include cros-common.
> The rtc that the system successfully uses is "s5m-rtc", which I guess is
> on the s5m8767 then? (Confusing that 3.8 Spring had this rtc node
> despite Snow's max77686 not having it.)
I think it's not really supposed to. It was a bit of a hack to handle
the fact that the RTC has two different i2c addresses on both the
77686 and this PMIC. The max77686 driver upstream handles it all in
code and doesn't ask the device tree to list both addresses.
I think that _upstream_ snow doesn't have the node but _downstream_
spring might have the node (though having a child of a disabled node
isn't particularly useful).
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + status = "disabled";
>>> +
>>> + rtc {
>>> + reg = <0x6>;
>>> + };
>>> + };
>>> +
>>> + s5m8767_pmic@66 {
>>> + compatible = "samsung,s5m8767-pmic";
>>> + reg = <0x66>;
>>> + interrupt-parent = <&gpx3>;
>>> + interrupts = <2 0>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
>>> + wakeup-source;
>>> +
>>> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
>>> + <&gpd1 1 1>, /* DVS2 */
>>> + <&gpd1 2 1>; /* DVS3 */
>>> +
>>> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
>>> + <&gpx2 4 1>, /* SET2 */
>>> + <&gpx2 5 1>; /* SET3 */
>>
>> The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.
>>
>>
>>> +
>>> + /*
>>> + * The following arrays of DVS voltages are not used, since we are
>>> + * not using GPIOs to control PMIC bucks, but they must be defined
>>> + * to please the driver.
>>> + */
>>> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
>>> + <1250000>, <1200000>,
>>> + <1150000>, <1100000>,
>>> + <1000000>, <950000>;
>>> +
>>> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
>>> + <1100000>, <1100000>,
>>> + <1000000>, <1000000>,
>>> + <1000000>, <1000000>;
>>> +
>>> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
>>> + <1200000>, <1200000>,
>>> + <1200000>, <1200000>,
>>> + <1200000>, <1200000>;
>>> +
>>> + clocks {
>>> + compatible = "samsung,s5m8767-clk";
>>> + #clock-cells = <1>;
>>> + clock-output-names = "en32khz_ap",
>>> + "en32khz_cp",
>>> + "en32khz_bt";
>>> + };
>>> +
>>> + regulators {
>>> + s5m_ldo4_reg: LDO4 {
>>> + regulator-name = "P1.0V_LDO_OUT4";
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + regulator-always-on;
>>> + op_mode = <0>;
>>
>> I think that "op_mode" here is questionable. Adding Javier to the
>> thread who was looking at this for max77802 and possibly max77686.
>
> Confirming that this op_mode is present in the original 3.8 device tree.
Yes and it needs to be handled eventually. It makes suspend/resume
work properly. ...but I think it needs to be thought out better.
...but given that other users of this PMIC have it I guess it makes
sense to leave it in. Javier was going to try to think it through
better for max77686 and max77802.
> (I used dtc to compile my /proc/device-tree tarball back to .dts, so it
> has hexadecimal <0x0> but that shouldn't matter to dtc AFAIU.)
>
>>> + };
>>> +
>>> + s5m_ldo5_reg: LDO5 {
>>> + regulator-name = "P1.0V_LDO_OUT5";
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + regulator-always-on;
>>> + op_mode = <0>;
>>> + };
>>> +
>>> + s5m_ldo6_reg: LDO6 {
>>> + regulator-name = "vdd_mydp";
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo7_reg: LDO7 {
>>> + regulator-name = "P1.1V_LDO_OUT7";
>>> + regulator-min-microvolt = <1100000>;
>>> + regulator-max-microvolt = <1100000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo8_reg: LDO8 {
>>> + regulator-name = "P1.0V_LDO_OUT8";
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo10_reg: LDO10 {
>>> + regulator-name = "P1.8V_LDO_OUT10";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo11_reg: LDO11 {
>>> + regulator-name = "P1.8V_LDO_OUT11";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + op_mode = <0>;
>>> + };
>>> +
>>> + s5m_ldo12_reg: LDO12 {
>>> + regulator-name = "P3.0V_LDO_OUT12";
>>> + regulator-min-microvolt = <3000000>;
>>> + regulator-max-microvolt = <3000000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo13_reg: LDO13 {
>>> + regulator-name = "P1.8V_LDO_OUT13";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + op_mode = <0>;
>>> + };
>>> +
>>> + s5m_ldo14_reg: LDO14 {
>>> + regulator-name = "P1.8V_LDO_OUT14";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo15_reg: LDO15 {
>>> + regulator-name = "P1.0V_LDO_OUT15";
>>> + regulator-min-microvolt = <1000000>;
>>> + regulator-max-microvolt = <1000000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo16_reg: LDO16 {
>>> + regulator-name = "P1.8V_LDO_OUT16";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + s5m_ldo17_reg: LDO17 {
>>> + regulator-name = "P2.8V_LDO_OUT17";
>>> + regulator-min-microvolt = <2800000>;
>>> + regulator-max-microvolt = <2800000>;
>>> + regulator-always-on;
>>> + op_mode = <0>;
>>> + };
>>> +
>>> + s5m_ldo25_reg: LDO25 {
>>> + regulator-name = "vdd_bridge";
>>> + regulator-min-microvolt = <1200000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + regulator-always-on;
>>> + op_mode = <1>;
>>> + };
>>> +
>>> + BUCK1 {
>>> + regulator-name = "vdd_mif";
>>> + regulator-min-microvolt = <950000>;
>>> + regulator-max-microvolt = <1300000>;
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + BUCK2 {
>>> + regulator-name = "vdd_arm";
>>> + regulator-min-microvolt = <850000>;
>>> + regulator-max-microvolt = <1350000>;
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + BUCK3 {
>>> + regulator-name = "vdd_int";
>>> + regulator-min-microvolt = <900000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + BUCK4 {
>>> + regulator-name = "vdd_g3d";
>>> + regulator-min-microvolt = <850000>;
>>> + regulator-max-microvolt = <1300000>;
>>> + regulator-boot-on;
>>> + op_mode = <3>;
>>> + };
>>> +
>>> + BUCK5 {
>>> + regulator-name = "P1.8V_BUCK_OUT5";
>>> + regulator-min-microvolt = <1800000>;
>>> + regulator-max-microvolt = <1800000>;
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + op_mode = <1>;
>>> + };
>>> +
>>> + BUCK6 {
>>> + regulator-name = "P1.2V_BUCK_OUT6";
>>> + regulator-min-microvolt = <1200000>;
>>> + regulator-max-microvolt = <1200000>;
>>> + regulator-always-on;
>>> + regulator-boot>
>>
> -on;
>>> + op_mode = <0>;
>>> + };
>>> +
>>> + BUCK9 {
>>> + regulator-name = "vdd_ummc";
>>> + regulator-min-microvolt = <950000>;
>>> + regulator-max-microvolt = <3000000>;
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>> + op_mode = <3>;
>>> + };
>>> + };
>>> + };
>>> + };
>>> +
>>> + i2c@12C70000 {
>>> + trackpad {
>>> + status = "disabled";
>>
>> Having this bogus entry here doesn't add anything. Remove it until
>> the trackpad should be added. See http://crbug.com/371114 for a
>> slightly stale bug about trackpad.
>
> You misunderstand: This resolves an error about the cypress,cyapa
> inherited from -cros-common.dtsi. Spring uses a different device and two
> different I2C addresses.
Ah, right. I forgot about cros5250-common.
> Nodes named trackpad-bootloader and trackpad-alt are prepared here:
> https://github.com/afaerber/linux/commits/spring-next
It might work, but you run into pinctrl problems. You need to put the
pinctrl node _somewhere_ and if you put it with the trackpad entries
themselves then you get conflicts. ...and it doesn't belong in the
i2c bus (though that's what we have locally in our tree)
See <http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/30953>
for some background.
>>> + };
>>> + };
>>> +
>>> + i2c@12CA0000 {
>>> + embedded-controller {
>>
>> Add "cros_ec" alias like snow.
>>
>>
>>> + compatible = "google,cros-ec-i2c";
>>> + reg = <0x1e>;
>>> + interrupts = <6 0>;
>>> + interrupt-parent = <&gpx1>;
>>> + wakeup-source;
>>> +
>>> + keyboard-controller {
>>
>> Don't include keyboard-controller here. Add:
>>
>> #include "cros-ec-keyboard.dtsi"
>>
>> ...at the bottom. Note that I think that the spring EC has a special
>> "charger" key that it uses to indicate when a charger was plugged in
>> and unplugged. I'm not sure how that will end up getting supported
>> upstream but you could just leave it out for now.
>
> Is there such a pending patch for snow? That's what I modeled after.
Yeah, and it appears to be in linux-next. (1a395e3 ARM: dts: Use the
cros-ec-keyboard fragment in exynos5250-snow). It actually ended up
going through the tegra tree since it was in a series with a similar
tegra change. I asked Kukjin to pick it up via merge but he didn't
appear to.
> Where is cros-ec-keyboard.dtsi? Don't see it in
> http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/arch/arm/boot/dts?h=for-next
> or
> http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/include/dt-bindings?h=for-next
>
> Are you proposing I factor it out?
>
>>> + compatible = "google,cros-ec-keyb";
>>> + keypad,num-rows = <8>;
>>> + keypad,num-columns = <13>;
>>
>> Don't you need pinctrl here?
>
> The keyboard is usable; what would pinctrl be needed for? There's none
> in the 3.8 device tree.
Generally it's good to have the pinctrl listed to configure the IRQ
properly (get the pulls right). It might work without it but it's a
little less ideal I think.
I see this, right?
pinctrl-names = "default";
pinctrl-0 = <&ec_irq>;
...in the downstream "arch/arm/boot/dts/exynos5250-spring.dts"
>>> + google,needs-ghost-filter;
>>> + linux,keymap = <
>>> + 0x0001007d /* L_META */
>>> + 0x0002003b /* F1 */
>>> + 0x00030030 /* B */
>>> + 0x00040044 /* F10 */
>>> + 0x00060031 /* N */
>>> + 0x0008000d /* = */
>>> + 0x000a0064 /* R_ALT */
>>> +
>>> + 0x01010001 /* ESC */
>>> + 0x0102003e /* F4 */
>>> + 0x01030022 /* G */
>>> + 0x01040041 /* F7 */
>>> + 0x01060023 /* H */
>>> + 0x01080028 /* ' */
>>> + 0x01090043 /* F9 */
>>> + 0x010b000e /* BKSPACE */
>>> +
>>> + 0x0200001d /* L_CTRL */
>>> + 0x0201000f /* TAB */
>>> + 0x0202003d /* F3 */
>>> + 0x02030014 /* T */
>>> + 0x02040040 /* F6 */
>>> + 0x0205001b /* ] */
>>> + 0x02060015 /* Y */
>>> + 0x02070056 /* 102ND */
>>> + 0x0208001a /* [ */
>>> + 0x02090042 /* F8 */
>>> +
>>> + 0x03010029 /* GRAVE */
>>> + 0x0302003c /* F2 */
>>> + 0x03030006 /* 5 */
>>> + 0x0304003f /* F5 */
>>> + 0x03060007 /* 6 */
>>> + 0x0308000c /* - */
>>> + 0x030b002b /* \ */
>>> +
>>> + 0x04000061 /* R_CTRL */
>>> + 0x0401001e /* A */
>>> + 0x04020020 /* D */
>>> + 0x04030021 /* F */
>>> + 0x0404001f /* S */
>>> + 0x04050025 /* K */
>>> + 0x04060024 /* J */
>>> + 0x04080027 /* ; */
>>> + 0x04090026 /* L */
>>> + 0x040a002b /* \ */
>>> + 0x040b001c /* ENTER */
>>> +
>>> + 0x0501002c /* Z */
>>> + 0x0502002e /* C */
>>> + 0x0503002f /* V */
>>> + 0x0504002d /* X */
>>> + 0x05050033 /* , */
>>> + 0x05060032 /* M */
>>> + 0x0507002a /* L_SHIFT */
>>> + 0x05080035 /* / */
>>> + 0x05090034 /* . */
>>> + 0x050B0039 /* SPACE */
>>> +
>>> + 0x06010002 /* 1 */
>>> + 0x06020004 /* 3 */
>>> + 0x06030005 /* 4 */
>>> + 0x06040003 /* 2 */
>>> + 0x06050009 /* 8 */
>>> + 0x06060008 /* 7 */
>>> + 0x0608000b /* 0 */
>>> + 0x0609000a /* 9 */
>>> + 0x060a0038 /* L_ALT */
>>> + 0x060b006c /* DOWN */
>>> + 0x060c006a /* RIGHT */
>>> +
>>> + 0x07010010 /* Q */
>>> + 0x07020012 /* E */
>>> + 0x07030013 /* R */
>>> + 0x07040011 /* W */
>>> + 0x07050017 /* I */
>>> + 0x07060016 /* U */
>>> + 0x07070036 /* R_SHIFT */
>>> + 0x07080019 /* P */
>>> + 0x07090018 /* O */
>>> + 0x070b0067 /* UP */
>>> + 0x070c0069 /* LEFT */
>>> + >;
>>> + };
>>> + };
>>> +
>>> + power-regulator {
>>> + compatible = "ti,tps65090";
>>
>> I doubt tps65090 will "just work". Does it?
>
> Good question. How to tell? :) Not familiar with clocks and regulators.
> I don't see the nodes referenced anywhere, so I could just try to drop
> (as I would, as per my cover letter, propose for anything non-essential
> that turns controversial).
What does "dmesg | grep tps65090" say?
What does this show:
grep "" /sys/class/regulator/*/name
> If we drop tps65090, can we safely drop vbat-fixed-regulator as well?
Yes
>> On spring the tps65090 is not directly on the same i2c bus as the EC.
>> It's actually hidden behind the EC.
>>
>> Locally in the ChromeOS tree there appears to be a forked copy of the
>> 65090 regulator driver that's in use just for spring. See this from
>> the ChromeOS 3.8 tree:
>>
>> ./drivers/regulator/tps65090-regulator.c
>> ./drivers/regulator/cros_ec-tps65090.c
>>
>> The Spring version of the driver sends EC commands directly to access
>> the tps65090.
>>
>> It is possible (untested) that you could also talk to tps65090 over an
>> i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
>> tunnel, but you may be able to extend the tunnel to export an i2c
>> tunnel for spring using something like
>> <https://chromium-review.googlesource.com/66116>
>
> The SBS battery thingy (not in this patch) should also be behind some
> tunnel. There's none defined in -cros-common.dtsi or in my patch, and
> still it shows up in dmesg to my surprise, complaining "Couldn't read
> remote-bus property".
Might be due to the cros5250-common. ...but yes, the battery should
be behind the tunnel too and the patches needed for Spring haven't
been posted by anyone yet. I picked up a whole bunch of cros_ec
cleanup patches and thought that the tunnel patches for spring could
possible go up after those land.
>>> + reg = <0x48>;
>>> +
>>> + /*
>>> + * Config irq to disable internal pulls
>>> + * even though we run in polling mode.
>>> + */
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&tps65090_irq>;
>>> +
>>> + vsys1-supply = <&vbat>;
>>> + vsys2-supply = <&vbat>;
>>> + vsys3-supply = <&vbat>;
>>> + infet1-supply = <&vbat>;
>>> + infet2-supply = <&vbat>;
>>> + infet3-supply = <&vbat>;
>>> + infet4-supply = <&vbat>;
>>> + infet5-supply = <&vbat>;
>>> + infet6-supply = <&vbat>;
>>> + infet7-supply = <&vbat>;
>>> + vsys-l1-supply = <&vbat>;
>>> + vsys-l2-supply = <&vbat>;
>>> +
>>> + regulators {
>>> + fet1 {
>>> + regulator-name = "vcd_led";
>>> + regulator-min-microvolt = <12000000>;
>>> + regulator-max-microvolt = <12000000>;
>>> + };
>>> + fet3 {
>>> + regulator-name = "wwan_r";
>>> + regulator-min-microvolt = <3300000>;
>>> + regulator-max-microvolt = <3300000>;
>>> + regulator-always-on;
>>> + };
>>> + fet5 {
>>> + regulator-name = "cam";
>>> + regulator-min-microvolt = <3300000>;
>>> + regulator-max-microvolt = <3300000>;
>>> + regulator-always-on;
>>> + };
>>> + fet6 {
>>> + regulator-name = "lcd_vdd";
>>> + regulator-min-microvolt = <3300000>;
>>> + regulator-max-microvolt = <3300000>;
>>> + };
>>> + fet7 {
>>> + regulator-name = "ts";
>>> + regulator-min-microvolt = <5000000>;
>>> + regulator-max-microvolt = <5000000>;
>>> + };
>>> + };
>>> +
>>> + charger {
>>> + compatible = "ti,tps65090-charger";
>>> + };
>>> + };
>>> + };
>>> +
>>> + hdmi {
>>> + hdmi-en-supply = <&s5m_ldo8_reg>;
>>> + vdd-supply = <&s5m_ldo8_reg>;
>>> + vdd_osc-supply = <&s5m_ldo10_reg>;
>>> + vdd_pll-supply = <&s5m_ldo8_reg>;
>>> + };
>>> +
>>> + fimd@14400000 {
>>> + status = "okay";
>>> + samsung,invert-vclk;
>>> + };
>>> +
>>> + dp-controller@145B0000 {
>>> + status = "okay";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&dp_hpd>;
>>
>> This is probably not right. It looks as if spring uses gpc3-0 for
>> display port HPD (as a GPIO). The upstream has this in the
>> exynos5250-pinctrl.dtsi as a different pin.
>>
>> I think you'll need to define your own pinctrl here.
>>
>>
>>> + samsung,color-space = <0>;
>>> + samsung,dynamic-range = <0>;
>>> + samsung,ycbcr-coeff = <0>;
>>> + samsung,color-depth = <1>;
>>> + samsung,link-rate = <0x0a>;
>>> + samsung,lane-count = <1>;
>>> + samsung,hpd-gpio = <&gpc3 0 0>;
>>> +
>>> + display-timings {
>>> + native-mode = <&timing1>;
>>> +
>>> + timing1: timing@1 {
>>> + clock-frequency = <70589280>;
>>> + hactive = <1366>;
>>> + vactive = <768>;
>>> + hfront-porch = <40>;
>>> + hback-porch = <40>;
>>> + hsync-len = <32>;
>>> + vback-porch = <10>;
>>> + vfront-porch = <12>;
>>> + vsync-len = <6>;
>>> + };
>>> + };
>>> + };
>>> +
>>> + fixed-rate-clocks {
>>> + xxti {
>>> + compatible = "samsung,clock-xxti";
>>> + clock-frequency = <24000000>;
>>> + };
>>> + };
>>> +};
>
> Will check on the other suggestions.
-Doug
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-24 4:05 ` Doug Anderson
@ 2014-06-24 10:06 ` Javier Martinez Canillas
2014-06-24 15:20 ` Doug Anderson
2014-06-24 15:06 ` Vincent Palatin
1 sibling, 1 reply; 20+ messages in thread
From: Javier Martinez Canillas @ 2014-06-24 10:06 UTC (permalink / raw)
To: Doug Anderson, Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Ben Dooks, Kukjin Kim, OPEN FIRMWARE AND...,
ARM PORT, LKML, Olof Johansson, tbroch, Tomasz Figa, jwerner
Hello Doug,
On 06/24/2014 06:05 AM, Doug Anderson wrote:
> Andreas,
>
> On Mon, Jun 23, 2014 at 3:46 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Hi Doug,
>>
>> Am 23.06.2014 21:47, schrieb Doug Anderson:
>>> Thanks for posting! A first pass on this is below...
>>
>> Thanks a lot for your quick review! My first big .dts patch, and no
>> datasheets for the hardware at hand as a user.
>>
>> A first pass of replies to my defense. ;)
>>
>>> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
>> [...]
>>>> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
>>>> new file mode 100644
>>>> index 0000000..e857d44
>>>> --- /dev/null
>>>> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
>>>> @@ -0,0 +1,556 @@
>>>> +/*
>>>> + * Google Spring board device tree source
>>>> + *
>>>> + * Copyright (c) 2013 Google, Inc
>>>> + * Copyright (c) 2014 SUSE LINUX Products GmbH
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +/dts-v1/;
>>>> +#include "exynos5250.dtsi"
>>>> +#include "exynos5250-cros-common.dtsi"
>>>
>>> It is possible we may want to backpedal on the use of
>>> "exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
>>> wasn't a fan of how it turned out.
>>>
>>> The original idea was that it should include the arbitrary set of
>>> things that are common between a chunk of Chrome OS boards. As more
>>> boards were introduced things would need to migrate from the "common"
>>> file to the board files.
>>>
>>> At the moment the current conventional wisdom is that some duplication
>>> is better than the confusing movement of everything back and forth.
>>> See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.
>>>
>>>
Another option is to identify DTS fragments that are common across boards and
create .dtsi files for these specific chunks instead of trying to group all set
of common things on a single .dtsi file.
For example, a quite common design for OMAP2+ based boards is to use a SMSC LAN
chip connected to OMAP's General-Purpose Memory Controller (GPMC). So the
following files were created to reduce DTS duplication:
arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi
arch/arm/boot/dts/omap-gpmc-smsc9221.dtsi
Now that I think about it, is the same that what you did for
arm/boot/dts/cros-ec-keyboard.dtsi.
Maybe splitting exynos5250-cros-common.dtsi in a set of .dtsi files will make it
more flexible/reusable?
>>>> +/ {
>>>> + model = "Google Spring";
>>>> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
>>>> +
>>>> + pinctrl@11400000 {
>>>
>>> The new best way to do things is to put this down at the bottom. See
>>> exynos5420-peach-pit as an example:
>>>
>>> &pinctrl_0 {
>>> ...
>>> }
>>>
>>> Note that I believe it was decided that top-level references like that
>>> should be sorted alphabetically.
>>
>> Thanks for the hint. (My chosen sort order here was by address.)
>>
>>> If you wanted to apply that run to exynos5250-snow I don't think it
>>> would be a terrible idea.
>>
>> I can of course apply changes to Snow, but I cannot test them myself.
>
> If you want to send up a patch like that I'm happy to give it a once
> over and also to test it. ...but don't feel obligated
>
>
>>>> + s5m8767_dvs: s5m8767-dvs {
>>>> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <1>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>> +
>>>> + s5m8767_ds: s5m8767-ds {
>>>> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <1>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>> +
>>>> + tps65090_irq: tps65090-irq {
>>>> + samsung,pins = "gpx2-6";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <0>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>> +
>>>> + s5m8767_irq: s5m8767-irq {
>>>> + samsung,pins = "gpx3-2";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <0>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>> +
>>>> + hdmi_hpd_irq: hdmi-hpd-irq {
>>>> + samsung,pins = "gpx3-7";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <1>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>> + };
>>>> +
>>>> + pinctrl@13400000 {
>>>> + hsic_reset: hsic-reset {
>>>> + samsung,pins = "gpe1-0";
>>>> + samsung,pin-function = <1>;
>>>> + samsung,pin-pud = <0>;
>>>> + samsung,pin-drv = <0>;
>>>> + };
>>>
>>> I'm pretty sure that the HSIC reset needed some funky code to make it
>>> work and I'm not sure what the status of that is upstream
>>
>> Yeah, you mentioned something along those lines. However it's the
>> equivalent of the usb3-vbus-en in -snow.dts. Rename or drop?
>
> On snow locally I see USB2 vbus is gpx1-1 and USB3 vbus is gpx2-7. I
> don't see that in spring.
>
> This will take more time than I have right now to track down. I added
> Julius to the thread in case he has time to answer and can suggest
> what to do for upstream purposes. I may be able to look more
> tomorrow.
>
> You can always send up the next version and include this and we'll
> look at it again.
>
>
>>>> + };
>>>> +
>>>> + vbat: vbat-fixed-regulator {
>>>> + compatible = "regulator-fixed";
>>>> + regulator-name = "vbat-supply";
>>>> + regulator-boot-on;
>>>> + };
>>>> +
>>>> + usb@12000000 {
>>>> + status = "okay";
>>>> + };
>>>> +
>>>> + usb3_vbus_reg: regulator-usb3 {
>>>> + compatible = "regulator-fixed";
>>>> + regulator-name = "P5.0V_USB3CON";
>>>> + regulator-min-microvolt = <5000000>;
>>>> + regulator-max-microvolt = <5000000>;
>>>> + gpio = <&gpe1 0 1>;
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&hsic_reset>;
>>>> + enable-active-high;
>>>> + };
>>>> +
>>>> + phy@12100000 {
>>>> + vbus-supply = <&usb3_vbus_reg>;
>>>> + };
>>>> +
>>>> + usb@12110000 {
>>>> + samsung,vbus-gpio = <&gpx1 1 0>;
>>>> + status = "okay";
>>>> + };
>>>> +
>>>> + usb@12120000 {
>>>> + status = "okay";
>>>> + };
>>>> +
>>>> + mmc@12220000 {
>>>> + /* MMC2 pins are used as GPIO for eDP bridge control. */
>>>> + status = "disabled";
>>>> + };
>>>> +
>>>> + mmc@12230000 {
>>>> + status = "disabled";
>>>> + };
>>>> +
>>>> + i2c@12C60000 {
>>>> + max77686@09 {
>>>
>>> There is no max77686 on spring. It uses s5m8767 in the place of max77686.
>>>
>>> ...you've got "status = disabled", just remove it.
>>
>> That's because it's inherited from exynos5250-cros-common.dtsi.
>
> Ah, right. So if we're keeping cros-common the proper thing to do is
> to move it out of cros-common in a patch before this one. ...but I
> think people might be happier if you simply don't include cros-common.
>
>
>> The rtc that the system successfully uses is "s5m-rtc", which I guess is
>> on the s5m8767 then? (Confusing that 3.8 Spring had this rtc node
>> despite Snow's max77686 not having it.)
>
> I think it's not really supposed to. It was a bit of a hack to handle
> the fact that the RTC has two different i2c addresses on both the
> 77686 and this PMIC. The max77686 driver upstream handles it all in
> code and doesn't ask the device tree to list both addresses.
>
> I think that _upstream_ snow doesn't have the node but _downstream_
> spring might have the node (though having a child of a disabled node
> isn't particularly useful).
>
>
Personally I think that "status = [enabled | disabled]" only makes sense for IP
blocks that are part of the SoC but may or may not be used by a board (e.g: i2c
and spi buses, sdhci and usb host controllers, etc).
DTS should be a description of the hardware so I agree that having a disabled
node for a device that is not present in the board is not right.
>>>> + #address-cells = <1>;
>>>> + #size-cells = <0>;
>>>> + status = "disabled";
>>>> +
>>>> + rtc {
>>>> + reg = <0x6>;
>>>> + };
>>>> + };
>>>> +
>>>> + s5m8767_pmic@66 {
>>>> + compatible = "samsung,s5m8767-pmic";
>>>> + reg = <0x66>;
>>>> + interrupt-parent = <&gpx3>;
>>>> + interrupts = <2 0>;
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
>>>> + wakeup-source;
>>>> +
>>>> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
>>>> + <&gpd1 1 1>, /* DVS2 */
>>>> + <&gpd1 2 1>; /* DVS3 */
>>>> +
>>>> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
>>>> + <&gpx2 4 1>, /* SET2 */
>>>> + <&gpx2 5 1>; /* SET3 */
>>>
>>> The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.
>>>
>>>
>>>> +
>>>> + /*
>>>> + * The following arrays of DVS voltages are not used, since we are
>>>> + * not using GPIOs to control PMIC bucks, but they must be defined
>>>> + * to please the driver.
>>>> + */
>>>> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
>>>> + <1250000>, <1200000>,
>>>> + <1150000>, <1100000>,
>>>> + <1000000>, <950000>;
>>>> +
>>>> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
>>>> + <1100000>, <1100000>,
>>>> + <1000000>, <1000000>,
>>>> + <1000000>, <1000000>;
>>>> +
>>>> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
>>>> + <1200000>, <1200000>,
>>>> + <1200000>, <1200000>,
>>>> + <1200000>, <1200000>;
>>>> +
>>>> + clocks {
>>>> + compatible = "samsung,s5m8767-clk";
>>>> + #clock-cells = <1>;
>>>> + clock-output-names = "en32khz_ap",
>>>> + "en32khz_cp",
>>>> + "en32khz_bt";
>>>> + };
>>>> +
>>>> + regulators {
>>>> + s5m_ldo4_reg: LDO4 {
>>>> + regulator-name = "P1.0V_LDO_OUT4";
>>>> + regulator-min-microvolt = <1000000>;
>>>> + regulator-max-microvolt = <1000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <0>;
>>>
>>> I think that "op_mode" here is questionable. Adding Javier to the
>>> thread who was looking at this for max77802 and possibly max77686.
>>
>> Confirming that this op_mode is present in the original 3.8 device tree.
>
> Yes and it needs to be handled eventually. It makes suspend/resume
> work properly. ...but I think it needs to be thought out better.
>
> ...but given that other users of this PMIC have it I guess it makes
> sense to leave it in. Javier was going to try to think it through
> better for max77686 and max77802.
>
>
The op_mode property is supported in mainline for this driver. I think that is
not a great binding tbh since the property has a generic name while it's
specific to the s5m8767 PMIC. But the driver uses it so makes sense to have it
in the DTS.
I've on my TODO list after the Max 77802 PMIC driver is merged to propose a
property for the generic regulator binding along the lines of what you proposed
in [0].
If a generic property is added to the regulator binding, the
exynos5250-spring.dts can later be changed to use this new property or keep
using the s5m8767 specific one since the driver has to keep supporting it for
backward compatibility.
>> (I used dtc to compile my /proc/device-tree tarball back to .dts, so it
>> has hexadecimal <0x0> but that shouldn't matter to dtc AFAIU.)
>>
>>>> + };
>>>> +
>>>> + s5m_ldo5_reg: LDO5 {
>>>> + regulator-name = "P1.0V_LDO_OUT5";
>>>> + regulator-min-microvolt = <1000000>;
>>>> + regulator-max-microvolt = <1000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <0>;
>>>> + };
>>>> +
>>>> + s5m_ldo6_reg: LDO6 {
>>>> + regulator-name = "vdd_mydp";
>>>> + regulator-min-microvolt = <1000000>;
>>>> + regulator-max-microvolt = <1000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo7_reg: LDO7 {
>>>> + regulator-name = "P1.1V_LDO_OUT7";
>>>> + regulator-min-microvolt = <1100000>;
>>>> + regulator-max-microvolt = <1100000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo8_reg: LDO8 {
>>>> + regulator-name = "P1.0V_LDO_OUT8";
>>>> + regulator-min-microvolt = <1000000>;
>>>> + regulator-max-microvolt = <1000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo10_reg: LDO10 {
>>>> + regulator-name = "P1.8V_LDO_OUT10";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo11_reg: LDO11 {
>>>> + regulator-name = "P1.8V_LDO_OUT11";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <0>;
>>>> + };
>>>> +
>>>> + s5m_ldo12_reg: LDO12 {
>>>> + regulator-name = "P3.0V_LDO_OUT12";
>>>> + regulator-min-microvolt = <3000000>;
>>>> + regulator-max-microvolt = <3000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo13_reg: LDO13 {
>>>> + regulator-name = "P1.8V_LDO_OUT13";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <0>;
>>>> + };
>>>> +
>>>> + s5m_ldo14_reg: LDO14 {
>>>> + regulator-name = "P1.8V_LDO_OUT14";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo15_reg: LDO15 {
>>>> + regulator-name = "P1.0V_LDO_OUT15";
>>>> + regulator-min-microvolt = <1000000>;
>>>> + regulator-max-microvolt = <1000000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo16_reg: LDO16 {
>>>> + regulator-name = "P1.8V_LDO_OUT16";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + s5m_ldo17_reg: LDO17 {
>>>> + regulator-name = "P2.8V_LDO_OUT17";
>>>> + regulator-min-microvolt = <2800000>;
>>>> + regulator-max-microvolt = <2800000>;
>>>> + regulator-always-on;
>>>> + op_mode = <0>;
>>>> + };
>>>> +
>>>> + s5m_ldo25_reg: LDO25 {
>>>> + regulator-name = "vdd_bridge";
>>>> + regulator-min-microvolt = <1200000>;
>>>> + regulator-max-microvolt = <1200000>;
>>>> + regulator-always-on;
>>>> + op_mode = <1>;
>>>> + };
>>>> +
>>>> + BUCK1 {
>>>> + regulator-name = "vdd_mif";
>>>> + regulator-min-microvolt = <950000>;
>>>> + regulator-max-microvolt = <1300000>;
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + BUCK2 {
>>>> + regulator-name = "vdd_arm";
>>>> + regulator-min-microvolt = <850000>;
>>>> + regulator-max-microvolt = <1350000>;
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + BUCK3 {
>>>> + regulator-name = "vdd_int";
>>>> + regulator-min-microvolt = <900000>;
>>>> + regulator-max-microvolt = <1200000>;
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + BUCK4 {
>>>> + regulator-name = "vdd_g3d";
>>>> + regulator-min-microvolt = <850000>;
>>>> + regulator-max-microvolt = <1300000>;
>>>> + regulator-boot-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> +
>>>> + BUCK5 {
>>>> + regulator-name = "P1.8V_BUCK_OUT5";
>>>> + regulator-min-microvolt = <1800000>;
>>>> + regulator-max-microvolt = <1800000>;
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + op_mode = <1>;
>>>> + };
>>>> +
>>>> + BUCK6 {
>>>> + regulator-name = "P1.2V_BUCK_OUT6";
>>>> + regulator-min-microvolt = <1200000>;
>>>> + regulator-max-microvolt = <1200000>;
>>>> + regulator-always-on;
>>>> + regulator-boot>
>>>
>> -on;
>>>> + op_mode = <0>;
>>>> + };
>>>> +
>>>> + BUCK9 {
>>>> + regulator-name = "vdd_ummc";
>>>> + regulator-min-microvolt = <950000>;
>>>> + regulator-max-microvolt = <3000000>;
>>>> + regulator-always-on;
>>>> + regulator-boot-on;
>>>> + op_mode = <3>;
>>>> + };
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + i2c@12C70000 {
>>>> + trackpad {
>>>> + status = "disabled";
>>>
>>> Having this bogus entry here doesn't add anything. Remove it until
>>> the trackpad should be added. See http://crbug.com/371114 for a
>>> slightly stale bug about trackpad.
>>
>> You misunderstand: This resolves an error about the cypress,cyapa
>> inherited from -cros-common.dtsi. Spring uses a different device and two
>> different I2C addresses.
>
> Ah, right. I forgot about cros5250-common.
>
>
>> Nodes named trackpad-bootloader and trackpad-alt are prepared here:
>> https://github.com/afaerber/linux/commits/spring-next
>
> It might work, but you run into pinctrl problems. You need to put the
> pinctrl node _somewhere_ and if you put it with the trackpad entries
> themselves then you get conflicts. ...and it doesn't belong in the
> i2c bus (though that's what we have locally in our tree)
>
> See <http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/30953>
> for some background.
>
>
>>>> + };
>>>> + };
>>>> +
>>>> + i2c@12CA0000 {
>>>> + embedded-controller {
>>>
>>> Add "cros_ec" alias like snow.
>>>
>>>
>>>> + compatible = "google,cros-ec-i2c";
>>>> + reg = <0x1e>;
>>>> + interrupts = <6 0>;
>>>> + interrupt-parent = <&gpx1>;
>>>> + wakeup-source;
>>>> +
>>>> + keyboard-controller {
>>>
>>> Don't include keyboard-controller here. Add:
>>>
>>> #include "cros-ec-keyboard.dtsi"
>>>
>>> ...at the bottom. Note that I think that the spring EC has a special
>>> "charger" key that it uses to indicate when a charger was plugged in
>>> and unplugged. I'm not sure how that will end up getting supported
>>> upstream but you could just leave it out for now.
>>
>> Is there such a pending patch for snow? That's what I modeled after.
>
> Yeah, and it appears to be in linux-next. (1a395e3 ARM: dts: Use the
> cros-ec-keyboard fragment in exynos5250-snow). It actually ended up
> going through the tegra tree since it was in a series with a similar
> tegra change. I asked Kukjin to pick it up via merge but he didn't
> appear to.
>
>
>> Where is cros-ec-keyboard.dtsi? Don't see it in
>> http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/arch/arm/boot/dts?h=for-next
>> or
>> http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/include/dt-bindings?h=for-next
>>
>> Are you proposing I factor it out?
>>
>>>> + compatible = "google,cros-ec-keyb";
>>>> + keypad,num-rows = <8>;
>>>> + keypad,num-columns = <13>;
>>>
>>> Don't you need pinctrl here?
>>
>> The keyboard is usable; what would pinctrl be needed for? There's none
>> in the 3.8 device tree.
>
> Generally it's good to have the pinctrl listed to configure the IRQ
> properly (get the pulls right). It might work without it but it's a
> little less ideal I think.
>
> I see this, right?
>
> pinctrl-names = "default";
> pinctrl-0 = <&ec_irq>;
>
> ...in the downstream "arch/arm/boot/dts/exynos5250-spring.dts"
>
>
>>>> + google,needs-ghost-filter;
>>>> + linux,keymap = <
>>>> + 0x0001007d /* L_META */
>>>> + 0x0002003b /* F1 */
>>>> + 0x00030030 /* B */
>>>> + 0x00040044 /* F10 */
>>>> + 0x00060031 /* N */
>>>> + 0x0008000d /* = */
>>>> + 0x000a0064 /* R_ALT */
>>>> +
>>>> + 0x01010001 /* ESC */
>>>> + 0x0102003e /* F4 */
>>>> + 0x01030022 /* G */
>>>> + 0x01040041 /* F7 */
>>>> + 0x01060023 /* H */
>>>> + 0x01080028 /* ' */
>>>> + 0x01090043 /* F9 */
>>>> + 0x010b000e /* BKSPACE */
>>>> +
>>>> + 0x0200001d /* L_CTRL */
>>>> + 0x0201000f /* TAB */
>>>> + 0x0202003d /* F3 */
>>>> + 0x02030014 /* T */
>>>> + 0x02040040 /* F6 */
>>>> + 0x0205001b /* ] */
>>>> + 0x02060015 /* Y */
>>>> + 0x02070056 /* 102ND */
>>>> + 0x0208001a /* [ */
>>>> + 0x02090042 /* F8 */
>>>> +
>>>> + 0x03010029 /* GRAVE */
>>>> + 0x0302003c /* F2 */
>>>> + 0x03030006 /* 5 */
>>>> + 0x0304003f /* F5 */
>>>> + 0x03060007 /* 6 */
>>>> + 0x0308000c /* - */
>>>> + 0x030b002b /* \ */
>>>> +
>>>> + 0x04000061 /* R_CTRL */
>>>> + 0x0401001e /* A */
>>>> + 0x04020020 /* D */
>>>> + 0x04030021 /* F */
>>>> + 0x0404001f /* S */
>>>> + 0x04050025 /* K */
>>>> + 0x04060024 /* J */
>>>> + 0x04080027 /* ; */
>>>> + 0x04090026 /* L */
>>>> + 0x040a002b /* \ */
>>>> + 0x040b001c /* ENTER */
>>>> +
>>>> + 0x0501002c /* Z */
>>>> + 0x0502002e /* C */
>>>> + 0x0503002f /* V */
>>>> + 0x0504002d /* X */
>>>> + 0x05050033 /* , */
>>>> + 0x05060032 /* M */
>>>> + 0x0507002a /* L_SHIFT */
>>>> + 0x05080035 /* / */
>>>> + 0x05090034 /* . */
>>>> + 0x050B0039 /* SPACE */
>>>> +
>>>> + 0x06010002 /* 1 */
>>>> + 0x06020004 /* 3 */
>>>> + 0x06030005 /* 4 */
>>>> + 0x06040003 /* 2 */
>>>> + 0x06050009 /* 8 */
>>>> + 0x06060008 /* 7 */
>>>> + 0x0608000b /* 0 */
>>>> + 0x0609000a /* 9 */
>>>> + 0x060a0038 /* L_ALT */
>>>> + 0x060b006c /* DOWN */
>>>> + 0x060c006a /* RIGHT */
>>>> +
>>>> + 0x07010010 /* Q */
>>>> + 0x07020012 /* E */
>>>> + 0x07030013 /* R */
>>>> + 0x07040011 /* W */
>>>> + 0x07050017 /* I */
>>>> + 0x07060016 /* U */
>>>> + 0x07070036 /* R_SHIFT */
>>>> + 0x07080019 /* P */
>>>> + 0x07090018 /* O */
>>>> + 0x070b0067 /* UP */
>>>> + 0x070c0069 /* LEFT */
>>>> + >;
>>>> + };
>>>> + };
>>>> +
>>>> + power-regulator {
>>>> + compatible = "ti,tps65090";
>>>
>>> I doubt tps65090 will "just work". Does it?
>>
>> Good question. How to tell? :) Not familiar with clocks and regulators.
>> I don't see the nodes referenced anywhere, so I could just try to drop
>> (as I would, as per my cover letter, propose for anything non-essential
>> that turns controversial).
>
> What does "dmesg | grep tps65090" say?
>
> What does this show:
> grep "" /sys/class/regulator/*/name
>
>
>> If we drop tps65090, can we safely drop vbat-fixed-regulator as well?
>
> Yes
>
>
>>> On spring the tps65090 is not directly on the same i2c bus as the EC.
>>> It's actually hidden behind the EC.
>>>
>>> Locally in the ChromeOS tree there appears to be a forked copy of the
>>> 65090 regulator driver that's in use just for spring. See this from
>>> the ChromeOS 3.8 tree:
>>>
>>> ./drivers/regulator/tps65090-regulator.c
>>> ./drivers/regulator/cros_ec-tps65090.c
>>>
>>> The Spring version of the driver sends EC commands directly to access
>>> the tps65090.
>>>
>>> It is possible (untested) that you could also talk to tps65090 over an
>>> i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
>>> tunnel, but you may be able to extend the tunnel to export an i2c
>>> tunnel for spring using something like
>>> <https://chromium-review.googlesource.com/66116>
>>
>> The SBS battery thingy (not in this patch) should also be behind some
>> tunnel. There's none defined in -cros-common.dtsi or in my patch, and
>> still it shows up in dmesg to my surprise, complaining "Couldn't read
>> remote-bus property".
>
> Might be due to the cros5250-common. ...but yes, the battery should
> be behind the tunnel too and the patches needed for Spring haven't
> been posted by anyone yet. I picked up a whole bunch of cros_ec
> cleanup patches and thought that the tunnel patches for spring could
> possible go up after those land.
>
>
>>>> + reg = <0x48>;
>>>> +
>>>> + /*
>>>> + * Config irq to disable internal pulls
>>>> + * even though we run in polling mode.
>>>> + */
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&tps65090_irq>;
>>>> +
>>>> + vsys1-supply = <&vbat>;
>>>> + vsys2-supply = <&vbat>;
>>>> + vsys3-supply = <&vbat>;
>>>> + infet1-supply = <&vbat>;
>>>> + infet2-supply = <&vbat>;
>>>> + infet3-supply = <&vbat>;
>>>> + infet4-supply = <&vbat>;
>>>> + infet5-supply = <&vbat>;
>>>> + infet6-supply = <&vbat>;
>>>> + infet7-supply = <&vbat>;
>>>> + vsys-l1-supply = <&vbat>;
>>>> + vsys-l2-supply = <&vbat>;
>>>> +
>>>> + regulators {
>>>> + fet1 {
>>>> + regulator-name = "vcd_led";
>>>> + regulator-min-microvolt = <12000000>;
>>>> + regulator-max-microvolt = <12000000>;
>>>> + };
>>>> + fet3 {
>>>> + regulator-name = "wwan_r";
>>>> + regulator-min-microvolt = <3300000>;
>>>> + regulator-max-microvolt = <3300000>;
>>>> + regulator-always-on;
>>>> + };
>>>> + fet5 {
>>>> + regulator-name = "cam";
>>>> + regulator-min-microvolt = <3300000>;
>>>> + regulator-max-microvolt = <3300000>;
>>>> + regulator-always-on;
>>>> + };
>>>> + fet6 {
>>>> + regulator-name = "lcd_vdd";
>>>> + regulator-min-microvolt = <3300000>;
>>>> + regulator-max-microvolt = <3300000>;
>>>> + };
>>>> + fet7 {
>>>> + regulator-name = "ts";
>>>> + regulator-min-microvolt = <5000000>;
>>>> + regulator-max-microvolt = <5000000>;
>>>> + };
>>>> + };
>>>> +
>>>> + charger {
>>>> + compatible = "ti,tps65090-charger";
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + hdmi {
>>>> + hdmi-en-supply = <&s5m_ldo8_reg>;
>>>> + vdd-supply = <&s5m_ldo8_reg>;
>>>> + vdd_osc-supply = <&s5m_ldo10_reg>;
>>>> + vdd_pll-supply = <&s5m_ldo8_reg>;
>>>> + };
>>>> +
>>>> + fimd@14400000 {
>>>> + status = "okay";
>>>> + samsung,invert-vclk;
>>>> + };
>>>> +
>>>> + dp-controller@145B0000 {
>>>> + status = "okay";
>>>> + pinctrl-names = "default";
>>>> + pinctrl-0 = <&dp_hpd>;
>>>
>>> This is probably not right. It looks as if spring uses gpc3-0 for
>>> display port HPD (as a GPIO). The upstream has this in the
>>> exynos5250-pinctrl.dtsi as a different pin.
>>>
>>> I think you'll need to define your own pinctrl here.
>>>
>>>
>>>> + samsung,color-space = <0>;
>>>> + samsung,dynamic-range = <0>;
>>>> + samsung,ycbcr-coeff = <0>;
>>>> + samsung,color-depth = <1>;
>>>> + samsung,link-rate = <0x0a>;
>>>> + samsung,lane-count = <1>;
>>>> + samsung,hpd-gpio = <&gpc3 0 0>;
>>>> +
>>>> + display-timings {
>>>> + native-mode = <&timing1>;
>>>> +
>>>> + timing1: timing@1 {
>>>> + clock-frequency = <70589280>;
>>>> + hactive = <1366>;
>>>> + vactive = <768>;
>>>> + hfront-porch = <40>;
>>>> + hback-porch = <40>;
>>>> + hsync-len = <32>;
>>>> + vback-porch = <10>;
>>>> + vfront-porch = <12>;
>>>> + vsync-len = <6>;
>>>> + };
>>>> + };
>>>> + };
>>>> +
>>>> + fixed-rate-clocks {
>>>> + xxti {
>>>> + compatible = "samsung,clock-xxti";
>>>> + clock-frequency = <24000000>;
>>>> + };
>>>> + };
>>>> +};
>>
>> Will check on the other suggestions.
>
> -Doug
>
Best regards,
Javier
[0]: https://patchwork.kernel.org/patch/1855331/
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-24 4:05 ` Doug Anderson
2014-06-24 10:06 ` Javier Martinez Canillas
@ 2014-06-24 15:06 ` Vincent Palatin
1 sibling, 0 replies; 20+ messages in thread
From: Vincent Palatin @ 2014-06-24 15:06 UTC (permalink / raw)
To: Doug Anderson
Cc: Andreas Färber, linux-samsung-soc, Stephan van Schaik,
Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Russell King, Ben Dooks, Kukjin Kim, OPEN FIRMWARE AND...,
ARM PORT, LKML, Olof Johansson, Javier Martinez Canillas,
Todd Broch, Tomasz Figa, Julius Werner
Re-sending ... the text-only encoding was not properly turned on on
the previous one and irritated the mailing lists.
On Mon, Jun 23, 2014 at 9:05 PM, Doug Anderson <dianders@chromium.org> wrote:
>
> Andreas,
>
> On Mon, Jun 23, 2014 at 3:46 PM, Andreas Färber <afaerber@suse.de> wrote:
> > Hi Doug,
> >
> > Am 23.06.2014 21:47, schrieb Doug Anderson:
> >> Thanks for posting! A first pass on this is below...
> >
> > Thanks a lot for your quick review! My first big .dts patch, and no
> > datasheets for the hardware at hand as a user.
> >
> > A first pass of replies to my defense. ;)
> >
> >> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
> > [...]
> >>> diff --git a/arch/arm/boot/dts/exynos5250-spring.dts b/arch/arm/boot/dts/exynos5250-spring.dts
> >>> new file mode 100644
> >>> index 0000000..e857d44
> >>> --- /dev/null
> >>> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
> >>> @@ -0,0 +1,556 @@
> >>> +/*
> >>> + * Google Spring board device tree source
> >>> + *
> >>> + * Copyright (c) 2013 Google, Inc
> >>> + * Copyright (c) 2014 SUSE LINUX Products GmbH
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify
> >>> + * it under the terms of the GNU General Public License version 2 as
> >>> + * published by the Free Software Foundation.
> >>> + */
> >>> +
> >>> +/dts-v1/;
> >>> +#include "exynos5250.dtsi"
> >>> +#include "exynos5250-cros-common.dtsi"
> >>
> >> It is possible we may want to backpedal on the use of
> >> "exynos5250-cros-common.dtsi". I know that Olof (now CCed) said he
> >> wasn't a fan of how it turned out.
> >>
> >> The original idea was that it should include the arbitrary set of
> >> things that are common between a chunk of Chrome OS boards. As more
> >> boards were introduced things would need to migrate from the "common"
> >> file to the board files.
> >>
> >> At the moment the current conventional wisdom is that some duplication
> >> is better than the confusing movement of everything back and forth.
> >> See exynos5420-peach-pit and exynos5800-peach-pi in ToT linux-next.
> >>
> >>
> >>> +/ {
> >>> + model = "Google Spring";
> >>> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
> >>> +
> >>> + pinctrl@11400000 {
> >>
> >> The new best way to do things is to put this down at the bottom. See
> >> exynos5420-peach-pit as an example:
> >>
> >> &pinctrl_0 {
> >> ...
> >> }
> >>
> >> Note that I believe it was decided that top-level references like that
> >> should be sorted alphabetically.
> >
> > Thanks for the hint. (My chosen sort order here was by address.)
> >
> >> If you wanted to apply that run to exynos5250-snow I don't think it
> >> would be a terrible idea.
> >
> > I can of course apply changes to Snow, but I cannot test them myself.
>
> If you want to send up a patch like that I'm happy to give it a once
> over and also to test it. ...but don't feel obligated
>
>
> >>> + s5m8767_dvs: s5m8767-dvs {
> >>> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
> >>> + samsung,pin-function = <0>;
> >>> + samsung,pin-pud = <1>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>> +
> >>> + s5m8767_ds: s5m8767-ds {
> >>> + samsung,pins = "gpx2-3", "gpx2-4", "gpx2-5";
> >>> + samsung,pin-function = <0>;
> >>> + samsung,pin-pud = <1>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>> +
> >>> + tps65090_irq: tps65090-irq {
> >>> + samsung,pins = "gpx2-6";
> >>> + samsung,pin-function = <0>;
> >>> + samsung,pin-pud = <0>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>> +
> >>> + s5m8767_irq: s5m8767-irq {
> >>> + samsung,pins = "gpx3-2";
> >>> + samsung,pin-function = <0>;
> >>> + samsung,pin-pud = <0>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>> +
> >>> + hdmi_hpd_irq: hdmi-hpd-irq {
> >>> + samsung,pins = "gpx3-7";
> >>> + samsung,pin-function = <0>;
> >>> + samsung,pin-pud = <1>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>> + };
> >>> +
> >>> + pinctrl@13400000 {
> >>> + hsic_reset: hsic-reset {
> >>> + samsung,pins = "gpe1-0";
> >>> + samsung,pin-function = <1>;
> >>> + samsung,pin-pud = <0>;
> >>> + samsung,pin-drv = <0>;
> >>> + };
> >>
> >> I'm pretty sure that the HSIC reset needed some funky code to make it
> >> work and I'm not sure what the status of that is upstream
> >
> > Yeah, you mentioned something along those lines. However it's the
> > equivalent of the usb3-vbus-en in -snow.dts. Rename or drop?
It's clearly not equivalent, usb3-vbus-en just turns on/off the load
switch for VBUS
while the HSIC chip (USB3503) is an external PHY (and hub) for the
USB2 port, the reset of the PHY side of the HSIC needs to be done
within the proper timing window compared to the SoC HSIC controller to
get a working link.
As the current kernel has a binding for the SMSC USB3503 and a PHY
driver (which is in driver/usb/misc probably because it is an external
PHY), the hsic-reset should be connected there (as "reset-gpios").
On top of that the HSIC link is a bit finicky and it needs a few
quirks if we want it to survive all kind of events :
eg s5p_hub_control() in
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.8/drivers/usb/host/ehci-s5p.c
>
> On snow locally I see USB2 vbus is gpx1-1 and USB3 vbus is gpx2-7. I
> don't see that in spring.
On Spring, the USB3 root hub is just connected to the internal camera
(which is powered by one of the TPS65090 LDOs)
On the external USB2 ports, the port enable going to the VBUS load
switch is controlled directly the USB3503 (which has an internal
mechanism, configured by default as an automatic control of
overcurrent protection, it can be bypass by using the USB3503 I2C
registers).
> This will take more time than I have right now to track down. I added
> Julius to the thread in case he has time to answer and can suggest
> what to do for upstream purposes. I may be able to look more
> tomorrow.
>
> You can always send up the next version and include this and we'll
> look at it again.
>
>
> >>> + };
> >>> +
> >>> + vbat: vbat-fixed-regulator {
> >>> + compatible = "regulator-fixed";
> >>> + regulator-name = "vbat-supply";
> >>> + regulator-boot-on;
> >>> + };
> >>> +
> >>> + usb@12000000 {
> >>> + status = "okay";
> >>> + };
> >>> +
> >>> + usb3_vbus_reg: regulator-usb3 {
> >>> + compatible = "regulator-fixed";
> >>> + regulator-name = "P5.0V_USB3CON";
> >>> + regulator-min-microvolt = <5000000>;
> >>> + regulator-max-microvolt = <5000000>;
> >>> + gpio = <&gpe1 0 1>;
> >>> + pinctrl-names = "default";
> >>> + pinctrl-0 = <&hsic_reset>;
> >>> + enable-active-high;
> >>> + };
> >>> +
> >>> + phy@12100000 {
> >>> + vbus-supply = <&usb3_vbus_reg>;
> >>> + };
> >>> +
> >>> + usb@12110000 {
> >>> + samsung,vbus-gpio = <&gpx1 1 0>;
> >>> + status = "okay";
> >>> + };
> >>> +
> >>> + usb@12120000 {
> >>> + status = "okay";
> >>> + };
> >>> +
> >>> + mmc@12220000 {
> >>> + /* MMC2 pins are used as GPIO for eDP bridge control. */
> >>> + status = "disabled";
> >>> + };
> >>> +
> >>> + mmc@12230000 {
> >>> + status = "disabled";
> >>> + };
> >>> +
> >>> + i2c@12C60000 {
> >>> + max77686@09 {
> >>
> >> There is no max77686 on spring. It uses s5m8767 in the place of max77686.
> >>
> >> ...you've got "status = disabled", just remove it.
> >
> > That's because it's inherited from exynos5250-cros-common.dtsi.
>
> Ah, right. So if we're keeping cros-common the proper thing to do is
> to move it out of cros-common in a patch before this one. ...but I
> think people might be happier if you simply don't include cros-common.
>
>
> > The rtc that the system successfully uses is "s5m-rtc", which I guess is
> > on the s5m8767 then? (Confusing that 3.8 Spring had this rtc node
> > despite Snow's max77686 not having it.)
>
> I think it's not really supposed to. It was a bit of a hack to handle
> the fact that the RTC has two different i2c addresses on both the
> 77686 and this PMIC. The max77686 driver upstream handles it all in
> code and doesn't ask the device tree to list both addresses.
>
> I think that _upstream_ snow doesn't have the node but _downstream_
> spring might have the node (though having a child of a disabled node
> isn't particularly useful).
>
>
> >>> + #address-cells = <1>;
> >>> + #size-cells = <0>;
> >>> + status = "disabled";
> >>> +
> >>> + rtc {
> >>> + reg = <0x6>;
> >>> + };
> >>> + };
> >>> +
> >>> + s5m8767_pmic@66 {
> >>> + compatible = "samsung,s5m8767-pmic";
> >>> + reg = <0x66>;
> >>> + interrupt-parent = <&gpx3>;
> >>> + interrupts = <2 0>;
> >>> + pinctrl-names = "default";
> >>> + pinctrl-0 = <&s5m8767_irq &s5m8767_dvs &s5m8767_ds>;
> >>> + wakeup-source;
> >>> +
> >>> + s5m8767,pmic-buck-dvs-gpios = <&gpd1 0 1>, /* DVS1 */
> >>> + <&gpd1 1 1>, /* DVS2 */
> >>> + <&gpd1 2 1>; /* DVS3 */
> >>> +
> >>> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 1>, /* SET1 */
> >>> + <&gpx2 4 1>, /* SET2 */
> >>> + <&gpx2 5 1>; /* SET3 */
> >>
> >> The final "1" in each of the GPIO specifiers above should be GPIO_ACTIVE_LOW.
> >>
> >>
> >>> +
> >>> + /*
> >>> + * The following arrays of DVS voltages are not used, since we are
> >>> + * not using GPIOs to control PMIC bucks, but they must be defined
> >>> + * to please the driver.
> >>> + */
> >>> + s5m8767,pmic-buck2-dvs-voltage = <1350000>, <1300000>,
> >>> + <1250000>, <1200000>,
> >>> + <1150000>, <1100000>,
> >>> + <1000000>, <950000>;
> >>> +
> >>> + s5m8767,pmic-buck3-dvs-voltage = <1100000>, <1100000>,
> >>> + <1100000>, <1100000>,
> >>> + <1000000>, <1000000>,
> >>> + <1000000>, <1000000>;
> >>> +
> >>> + s5m8767,pmic-buck4-dvs-voltage = <1200000>, <1200000>,
> >>> + <1200000>, <1200000>,
> >>> + <1200000>, <1200000>,
> >>> + <1200000>, <1200000>;
> >>> +
> >>> + clocks {
> >>> + compatible = "samsung,s5m8767-clk";
> >>> + #clock-cells = <1>;
> >>> + clock-output-names = "en32khz_ap",
> >>> + "en32khz_cp",
> >>> + "en32khz_bt";
> >>> + };
> >>> +
> >>> + regulators {
> >>> + s5m_ldo4_reg: LDO4 {
> >>> + regulator-name = "P1.0V_LDO_OUT4";
> >>> + regulator-min-microvolt = <1000000>;
> >>> + regulator-max-microvolt = <1000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <0>;
> >>
> >> I think that "op_mode" here is questionable. Adding Javier to the
> >> thread who was looking at this for max77802 and possibly max77686.
> >
> > Confirming that this op_mode is present in the original 3.8 device tree.
>
> Yes and it needs to be handled eventually. It makes suspend/resume
> work properly. ...but I think it needs to be thought out better.
>
> ...but given that other users of this PMIC have it I guess it makes
> sense to leave it in. Javier was going to try to think it through
> better for max77686 and max77802.
>
>
> > (I used dtc to compile my /proc/device-tree tarball back to .dts, so it
> > has hexadecimal <0x0> but that shouldn't matter to dtc AFAIU.)
> >
> >>> + };
> >>> +
> >>> + s5m_ldo5_reg: LDO5 {
> >>> + regulator-name = "P1.0V_LDO_OUT5";
> >>> + regulator-min-microvolt = <1000000>;
> >>> + regulator-max-microvolt = <1000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <0>;
> >>> + };
> >>> +
> >>> + s5m_ldo6_reg: LDO6 {
> >>> + regulator-name = "vdd_mydp";
> >>> + regulator-min-microvolt = <1000000>;
> >>> + regulator-max-microvolt = <1000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo7_reg: LDO7 {
> >>> + regulator-name = "P1.1V_LDO_OUT7";
> >>> + regulator-min-microvolt = <1100000>;
> >>> + regulator-max-microvolt = <1100000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo8_reg: LDO8 {
> >>> + regulator-name = "P1.0V_LDO_OUT8";
> >>> + regulator-min-microvolt = <1000000>;
> >>> + regulator-max-microvolt = <1000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo10_reg: LDO10 {
> >>> + regulator-name = "P1.8V_LDO_OUT10";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo11_reg: LDO11 {
> >>> + regulator-name = "P1.8V_LDO_OUT11";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <0>;
> >>> + };
> >>> +
> >>> + s5m_ldo12_reg: LDO12 {
> >>> + regulator-name = "P3.0V_LDO_OUT12";
> >>> + regulator-min-microvolt = <3000000>;
> >>> + regulator-max-microvolt = <3000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo13_reg: LDO13 {
> >>> + regulator-name = "P1.8V_LDO_OUT13";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <0>;
> >>> + };
> >>> +
> >>> + s5m_ldo14_reg: LDO14 {
> >>> + regulator-name = "P1.8V_LDO_OUT14";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo15_reg: LDO15 {
> >>> + regulator-name = "P1.0V_LDO_OUT15";
> >>> + regulator-min-microvolt = <1000000>;
> >>> + regulator-max-microvolt = <1000000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo16_reg: LDO16 {
> >>> + regulator-name = "P1.8V_LDO_OUT16";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + s5m_ldo17_reg: LDO17 {
> >>> + regulator-name = "P2.8V_LDO_OUT17";
> >>> + regulator-min-microvolt = <2800000>;
> >>> + regulator-max-microvolt = <2800000>;
> >>> + regulator-always-on;
> >>> + op_mode = <0>;
> >>> + };
> >>> +
> >>> + s5m_ldo25_reg: LDO25 {
> >>> + regulator-name = "vdd_bridge";
> >>> + regulator-min-microvolt = <1200000>;
> >>> + regulator-max-microvolt = <1200000>;
> >>> + regulator-always-on;
> >>> + op_mode = <1>;
> >>> + };
> >>> +
> >>> + BUCK1 {
> >>> + regulator-name = "vdd_mif";
> >>> + regulator-min-microvolt = <950000>;
> >>> + regulator-max-microvolt = <1300000>;
> >>> + regulator-always-on;
> >>> + regulator-boot-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + BUCK2 {
> >>> + regulator-name = "vdd_arm";
> >>> + regulator-min-microvolt = <850000>;
> >>> + regulator-max-microvolt = <1350000>;
> >>> + regulator-always-on;
> >>> + regulator-boot-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + BUCK3 {
> >>> + regulator-name = "vdd_int";
> >>> + regulator-min-microvolt = <900000>;
> >>> + regulator-max-microvolt = <1200000>;
> >>> + regulator-always-on;
> >>> + regulator-boot-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + BUCK4 {
> >>> + regulator-name = "vdd_g3d";
> >>> + regulator-min-microvolt = <850000>;
> >>> + regulator-max-microvolt = <1300000>;
> >>> + regulator-boot-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> +
> >>> + BUCK5 {
> >>> + regulator-name = "P1.8V_BUCK_OUT5";
> >>> + regulator-min-microvolt = <1800000>;
> >>> + regulator-max-microvolt = <1800000>;
> >>> + regulator-always-on;
> >>> + regulator-boot-on;
> >>> + op_mode = <1>;
> >>> + };
> >>> +
> >>> + BUCK6 {
> >>> + regulator-name = "P1.2V_BUCK_OUT6";
> >>> + regulator-min-microvolt = <1200000>;
> >>> + regulator-max-microvolt = <1200000>;
> >>> + regulator-always-on;
> >>> + regulator-boot>
> >>
> > -on;
> >>> + op_mode = <0>;
> >>> + };
> >>> +
> >>> + BUCK9 {
> >>> + regulator-name = "vdd_ummc";
> >>> + regulator-min-microvolt = <950000>;
> >>> + regulator-max-microvolt = <3000000>;
> >>> + regulator-always-on;
> >>> + regulator-boot-on;
> >>> + op_mode = <3>;
> >>> + };
> >>> + };
> >>> + };
> >>> + };
> >>> +
> >>> + i2c@12C70000 {
> >>> + trackpad {
> >>> + status = "disabled";
> >>
> >> Having this bogus entry here doesn't add anything. Remove it until
> >> the trackpad should be added. See http://crbug.com/371114 for a
> >> slightly stale bug about trackpad.
> >
> > You misunderstand: This resolves an error about the cypress,cyapa
> > inherited from -cros-common.dtsi. Spring uses a different device and two
> > different I2C addresses.
>
> Ah, right. I forgot about cros5250-common.
>
>
> > Nodes named trackpad-bootloader and trackpad-alt are prepared here:
> > https://github.com/afaerber/linux/commits/spring-next
>
> It might work, but you run into pinctrl problems. You need to put the
> pinctrl node _somewhere_ and if you put it with the trackpad entries
> themselves then you get conflicts. ...and it doesn't belong in the
> i2c bus (though that's what we have locally in our tree)
>
> See <http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/30953>
> for some background.
>
>
> >>> + };
> >>> + };
> >>> +
> >>> + i2c@12CA0000 {
> >>> + embedded-controller {
> >>
> >> Add "cros_ec" alias like snow.
> >>
> >>
> >>> + compatible = "google,cros-ec-i2c";
> >>> + reg = <0x1e>;
> >>> + interrupts = <6 0>;
> >>> + interrupt-parent = <&gpx1>;
> >>> + wakeup-source;
> >>> +
> >>> + keyboard-controller {
> >>
> >> Don't include keyboard-controller here. Add:
> >>
> >> #include "cros-ec-keyboard.dtsi"
> >>
> >> ...at the bottom. Note that I think that the spring EC has a special
> >> "charger" key that it uses to indicate when a charger was plugged in
> >> and unplugged. I'm not sure how that will end up getting supported
> >> upstream but you could just leave it out for now.
> >
> > Is there such a pending patch for snow? That's what I modeled after.
>
> Yeah, and it appears to be in linux-next. (1a395e3 ARM: dts: Use the
> cros-ec-keyboard fragment in exynos5250-snow). It actually ended up
> going through the tegra tree since it was in a series with a similar
> tegra change. I asked Kukjin to pick it up via merge but he didn't
> appear to.
>
>
> > Where is cros-ec-keyboard.dtsi? Don't see it in
> > http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/arch/arm/boot/dts?h=for-next
> > or
> > http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/include/dt-bindings?h=for-next
> >
> > Are you proposing I factor it out?
> >
> >>> + compatible = "google,cros-ec-keyb";
> >>> + keypad,num-rows = <8>;
> >>> + keypad,num-columns = <13>;
> >>
> >> Don't you need pinctrl here?
> >
> > The keyboard is usable; what would pinctrl be needed for? There's none
> > in the 3.8 device tree.
>
> Generally it's good to have the pinctrl listed to configure the IRQ
> properly (get the pulls right). It might work without it but it's a
> little less ideal I think.
>
> I see this, right?
>
> pinctrl-names = "default";
> pinctrl-0 = <&ec_irq>;
>
> ...in the downstream "arch/arm/boot/dts/exynos5250-spring.dts"
>
>
> >>> + google,needs-ghost-filter;
> >>> + linux,keymap = <
> >>> + 0x0001007d /* L_META */
> >>> + 0x0002003b /* F1 */
> >>> + 0x00030030 /* B */
> >>> + 0x00040044 /* F10 */
> >>> + 0x00060031 /* N */
> >>> + 0x0008000d /* = */
> >>> + 0x000a0064 /* R_ALT */
> >>> +
> >>> + 0x01010001 /* ESC */
> >>> + 0x0102003e /* F4 */
> >>> + 0x01030022 /* G */
> >>> + 0x01040041 /* F7 */
> >>> + 0x01060023 /* H */
> >>> + 0x01080028 /* ' */
> >>> + 0x01090043 /* F9 */
> >>> + 0x010b000e /* BKSPACE */
> >>> +
> >>> + 0x0200001d /* L_CTRL */
> >>> + 0x0201000f /* TAB */
> >>> + 0x0202003d /* F3 */
> >>> + 0x02030014 /* T */
> >>> + 0x02040040 /* F6 */
> >>> + 0x0205001b /* ] */
> >>> + 0x02060015 /* Y */
> >>> + 0x02070056 /* 102ND */
> >>> + 0x0208001a /* [ */
> >>> + 0x02090042 /* F8 */
> >>> +
> >>> + 0x03010029 /* GRAVE */
> >>> + 0x0302003c /* F2 */
> >>> + 0x03030006 /* 5 */
> >>> + 0x0304003f /* F5 */
> >>> + 0x03060007 /* 6 */
> >>> + 0x0308000c /* - */
> >>> + 0x030b002b /* \ */
> >>> +
> >>> + 0x04000061 /* R_CTRL */
> >>> + 0x0401001e /* A */
> >>> + 0x04020020 /* D */
> >>> + 0x04030021 /* F */
> >>> + 0x0404001f /* S */
> >>> + 0x04050025 /* K */
> >>> + 0x04060024 /* J */
> >>> + 0x04080027 /* ; */
> >>> + 0x04090026 /* L */
> >>> + 0x040a002b /* \ */
> >>> + 0x040b001c /* ENTER */
> >>> +
> >>> + 0x0501002c /* Z */
> >>> + 0x0502002e /* C */
> >>> + 0x0503002f /* V */
> >>> + 0x0504002d /* X */
> >>> + 0x05050033 /* , */
> >>> + 0x05060032 /* M */
> >>> + 0x0507002a /* L_SHIFT */
> >>> + 0x05080035 /* / */
> >>> + 0x05090034 /* . */
> >>> + 0x050B0039 /* SPACE */
> >>> +
> >>> + 0x06010002 /* 1 */
> >>> + 0x06020004 /* 3 */
> >>> + 0x06030005 /* 4 */
> >>> + 0x06040003 /* 2 */
> >>> + 0x06050009 /* 8 */
> >>> + 0x06060008 /* 7 */
> >>> + 0x0608000b /* 0 */
> >>> + 0x0609000a /* 9 */
> >>> + 0x060a0038 /* L_ALT */
> >>> + 0x060b006c /* DOWN */
> >>> + 0x060c006a /* RIGHT */
> >>> +
> >>> + 0x07010010 /* Q */
> >>> + 0x07020012 /* E */
> >>> + 0x07030013 /* R */
> >>> + 0x07040011 /* W */
> >>> + 0x07050017 /* I */
> >>> + 0x07060016 /* U */
> >>> + 0x07070036 /* R_SHIFT */
> >>> + 0x07080019 /* P */
> >>> + 0x07090018 /* O */
> >>> + 0x070b0067 /* UP */
> >>> + 0x070c0069 /* LEFT */
> >>> + >;
> >>> + };
> >>> + };
> >>> +
> >>> + power-regulator {
> >>> + compatible = "ti,tps65090";
> >>
> >> I doubt tps65090 will "just work". Does it?
> >
> > Good question. How to tell? :) Not familiar with clocks and regulators.
> > I don't see the nodes referenced anywhere, so I could just try to drop
> > (as I would, as per my cover letter, propose for anything non-essential
> > that turns controversial).
>
> What does "dmesg | grep tps65090" say?
>
> What does this show:
> grep "" /sys/class/regulator/*/name
>
>
> > If we drop tps65090, can we safely drop vbat-fixed-regulator as well?
>
> Yes
>
>
> >> On spring the tps65090 is not directly on the same i2c bus as the EC.
> >> It's actually hidden behind the EC.
> >>
> >> Locally in the ChromeOS tree there appears to be a forked copy of the
> >> 65090 regulator driver that's in use just for spring. See this from
> >> the ChromeOS 3.8 tree:
> >>
> >> ./drivers/regulator/tps65090-regulator.c
> >> ./drivers/regulator/cros_ec-tps65090.c
> >>
> >> The Spring version of the driver sends EC commands directly to access
> >> the tps65090.
> >>
> >> It is possible (untested) that you could also talk to tps65090 over an
> >> i2c tunnel. On exynos5420-peach-pit we have a full fledged i2c
> >> tunnel, but you may be able to extend the tunnel to export an i2c
> >> tunnel for spring using something like
> >> <https://chromium-review.googlesource.com/66116>
> >
> > The SBS battery thingy (not in this patch) should also be behind some
> > tunnel. There's none defined in -cros-common.dtsi or in my patch, and
> > still it shows up in dmesg to my surprise, complaining "Couldn't read
> > remote-bus property".
>
> Might be due to the cros5250-common. ...but yes, the battery should
> be behind the tunnel too and the patches needed for Spring haven't
> been posted by anyone yet. I picked up a whole bunch of cros_ec
> cleanup patches and thought that the tunnel patches for spring could
> possible go up after those land.
>
>
> >>> + reg = <0x48>;
> >>> +
> >>> + /*
> >>> + * Config irq to disable internal pulls
> >>> + * even though we run in polling mode.
> >>> + */
> >>> + pinctrl-names = "default";
> >>> + pinctrl-0 = <&tps65090_irq>;
> >>> +
> >>> + vsys1-supply = <&vbat>;
> >>> + vsys2-supply = <&vbat>;
> >>> + vsys3-supply = <&vbat>;
> >>> + infet1-supply = <&vbat>;
> >>> + infet2-supply = <&vbat>;
> >>> + infet3-supply = <&vbat>;
> >>> + infet4-supply = <&vbat>;
> >>> + infet5-supply = <&vbat>;
> >>> + infet6-supply = <&vbat>;
> >>> + infet7-supply = <&vbat>;
> >>> + vsys-l1-supply = <&vbat>;
> >>> + vsys-l2-supply = <&vbat>;
> >>> +
> >>> + regulators {
> >>> + fet1 {
> >>> + regulator-name = "vcd_led";
> >>> + regulator-min-microvolt = <12000000>;
> >>> + regulator-max-microvolt = <12000000>;
> >>> + };
> >>> + fet3 {
> >>> + regulator-name = "wwan_r";
> >>> + regulator-min-microvolt = <3300000>;
> >>> + regulator-max-microvolt = <3300000>;
> >>> + regulator-always-on;
> >>> + };
> >>> + fet5 {
> >>> + regulator-name = "cam";
> >>> + regulator-min-microvolt = <3300000>;
> >>> + regulator-max-microvolt = <3300000>;
> >>> + regulator-always-on;
> >>> + };
> >>> + fet6 {
> >>> + regulator-name = "lcd_vdd";
> >>> + regulator-min-microvolt = <3300000>;
> >>> + regulator-max-microvolt = <3300000>;
> >>> + };
> >>> + fet7 {
> >>> + regulator-name = "ts";
> >>> + regulator-min-microvolt = <5000000>;
> >>> + regulator-max-microvolt = <5000000>;
> >>> + };
> >>> + };
> >>> +
> >>> + charger {
> >>> + compatible = "ti,tps65090-charger";
> >>> + };
> >>> + };
> >>> + };
> >>> +
> >>> + hdmi {
> >>> + hdmi-en-supply = <&s5m_ldo8_reg>;
> >>> + vdd-supply = <&s5m_ldo8_reg>;
> >>> + vdd_osc-supply = <&s5m_ldo10_reg>;
> >>> + vdd_pll-supply = <&s5m_ldo8_reg>;
> >>> + };
> >>> +
> >>> + fimd@14400000 {
> >>> + status = "okay";
> >>> + samsung,invert-vclk;
> >>> + };
> >>> +
> >>> + dp-controller@145B0000 {
> >>> + status = "okay";
> >>> + pinctrl-names = "default";
> >>> + pinctrl-0 = <&dp_hpd>;
> >>
> >> This is probably not right. It looks as if spring uses gpc3-0 for
> >> display port HPD (as a GPIO). The upstream has this in the
> >> exynos5250-pinctrl.dtsi as a different pin.
> >>
> >> I think you'll need to define your own pinctrl here.
> >>
> >>
> >>> + samsung,color-space = <0>;
> >>> + samsung,dynamic-range = <0>;
> >>> + samsung,ycbcr-coeff = <0>;
> >>> + samsung,color-depth = <1>;
> >>> + samsung,link-rate = <0x0a>;
> >>> + samsung,lane-count = <1>;
> >>> + samsung,hpd-gpio = <&gpc3 0 0>;
> >>> +
> >>> + display-timings {
> >>> + native-mode = <&timing1>;
> >>> +
> >>> + timing1: timing@1 {
> >>> + clock-frequency = <70589280>;
> >>> + hactive = <1366>;
> >>> + vactive = <768>;
> >>> + hfront-porch = <40>;
> >>> + hback-porch = <40>;
> >>> + hsync-len = <32>;
> >>> + vback-porch = <10>;
> >>> + vfront-porch = <12>;
> >>> + vsync-len = <6>;
> >>> + };
> >>> + };
> >>> + };
> >>> +
> >>> + fixed-rate-clocks {
> >>> + xxti {
> >>> + compatible = "samsung,clock-xxti";
> >>> + clock-frequency = <24000000>;
> >>> + };
> >>> + };
> >>> +};
> >
> > Will check on the other suggestions.
>
> -Doug
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree
2014-06-24 10:06 ` Javier Martinez Canillas
@ 2014-06-24 15:20 ` Doug Anderson
0 siblings, 0 replies; 20+ messages in thread
From: Doug Anderson @ 2014-06-24 15:20 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Andreas Färber, linux-samsung-soc, Stephan van Schaik,
Vincent Palatin, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Ben Dooks, Kukjin Kim,
OPEN FIRMWARE AND...,
ARM PORT, LKML, Olof Johansson, Todd Broch, Tomasz Figa, jwerner
Javier,
On Tue, Jun 24, 2014 at 3:06 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello Doug,
>
> On 06/24/2014 06:05 AM, Doug Anderson wrote:
> Another option is to identify DTS fragments that are common across boards and
> create .dtsi files for these specific chunks instead of trying to group all set
> of common things on a single .dtsi file.
>
> For example, a quite common design for OMAP2+ based boards is to use a SMSC LAN
> chip connected to OMAP's General-Purpose Memory Controller (GPMC). So the
> following files were created to reduce DTS duplication:
>
> arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi
> arch/arm/boot/dts/omap-gpmc-smsc9221.dtsi
>
> Now that I think about it, is the same that what you did for
> arm/boot/dts/cros-ec-keyboard.dtsi.
>
> Maybe splitting exynos5250-cros-common.dtsi in a set of .dtsi files will make it
> more flexible/reusable?
Yes, I think the config fragments can be cleaner but I think we have
to be judicious about using them. There are definitely tradeoffs
involved. The keyboard was such an excessively large thing and
totally duplicated, so moving it out made sense. Other bits are less
obvious (to me) because there are so many interactions / combinations
and you end up with a bit of spaghetti in terms of which labels are
used by and provided by each fragment. I guess possibly you could
codify that better...
A few thoughts looking at exynos5420-peach-pit:
* backlight: seems (?) too board specific
* samsung,exynos5420-oscclk: could totally be a fragment, but very small.
* power key: could be a fragment for all boards that happen to use
gpx1-2 for this
* sound: could be a fragment for all devices using
"google,snow-audio-max98090", possibly.
> Personally I think that "status = [enabled | disabled]" only makes sense for IP
> blocks that are part of the SoC but may or may not be used by a board (e.g: i2c
> and spi buses, sdhci and usb host controllers, etc).
>
> DTS should be a description of the hardware so I agree that having a disabled
> node for a device that is not present in the board is not right.
Right. We'll take a look again in v2 when cros-common isn't used. I
think this could go in steps:
1. Don't use cros-common for spring
2. Don't use cros-common for snow (fold stuff in)
3. Introduce some fragments.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos
2014-06-23 17:27 ` Doug Anderson
@ 2014-06-25 10:47 ` Mark Rutland
2014-06-25 11:43 ` Andreas Färber
0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2014-06-25 10:47 UTC (permalink / raw)
To: Doug Anderson, Andreas Färber
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Ian Campbell, Kumar Gala, Randy Dunlap,
Mark Brown, Simon Glass, Michael Spang,
open list:OPEN FIRMWARE AND...,
open list:DOCUMENTATION, open list
On Mon, Jun 23, 2014 at 06:27:04PM +0100, Doug Anderson wrote:
> Andreas,
>
> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
> > It's vsys-l{1,2}-supply, not vsys_l{1,2}-supply.
> >
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > ---
> > Documentation/devicetree/bindings/regulator/tps65090.txt | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
> > index 34098023..ca69f5e 100644
> > --- a/Documentation/devicetree/bindings/regulator/tps65090.txt
> > +++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
> > @@ -45,8 +45,8 @@ Example:
> > infet5-supply = <&some_reg>;
> > infet6-supply = <&some_reg>;
> > infet7-supply = <&some_reg>;
> > - vsys_l1-supply = <&some_reg>;
> > - vsys_l2-supply = <&some_reg>;
> > + vsys-l1-supply = <&some_reg>;
> > + vsys-l2-supply = <&some_reg>;
>
> Your change matches the code and all existing device trees in the
> Linux kernel.
Could this fact please be mentioned in the commit message?
Given that:
Acked-by: Mark Rutland <mark.rutland@arm.com>
> I also see plenty of other bindings with dashes, so this seems
> reasonable.
Dashes rather than underscores are preferred/correct for property names
and compatible strings. Given no-one can possibly be using the
bad/incorrect form with underscores, fixing the documentation to use
dashes makes sense.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos
2014-06-25 10:47 ` Mark Rutland
@ 2014-06-25 11:43 ` Andreas Färber
2014-06-25 12:23 ` Rob Herring
0 siblings, 1 reply; 20+ messages in thread
From: Andreas Färber @ 2014-06-25 11:43 UTC (permalink / raw)
To: Mark Rutland, Doug Anderson
Cc: linux-samsung-soc, Stephan van Schaik, Vincent Palatin,
Rob Herring, Pawel Moll, Ian Campbell, Kumar Gala, Randy Dunlap,
Mark Brown, Simon Glass, Michael Spang, OPEN FIRMWARE AND...,
DOCUMENTATION, LKML
Am 25.06.2014 12:47, schrieb Mark Rutland:
> On Mon, Jun 23, 2014 at 06:27:04PM +0100, Doug Anderson wrote:
>> Andreas,
>>
>> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> It's vsys-l{1,2}-supply, not vsys_l{1,2}-supply.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> Documentation/devicetree/bindings/regulator/tps65090.txt | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
>>> index 34098023..ca69f5e 100644
>>> --- a/Documentation/devicetree/bindings/regulator/tps65090.txt
>>> +++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
>>> @@ -45,8 +45,8 @@ Example:
>>> infet5-supply = <&some_reg>;
>>> infet6-supply = <&some_reg>;
>>> infet7-supply = <&some_reg>;
>>> - vsys_l1-supply = <&some_reg>;
>>> - vsys_l2-supply = <&some_reg>;
>>> + vsys-l1-supply = <&some_reg>;
>>> + vsys-l2-supply = <&some_reg>;
>>
>> Your change matches the code and all existing device trees in the
>> Linux kernel.
>
> Could this fact please be mentioned in the commit message?
Yes, I admit the commit message could've been clearer in stating that
only the example is modified, not the actual specification. What about:
"Specification and existing device trees use vsys-l{1,2}-supply, not
vsys_l{1,2}-supply. Fix the example to match the specification."
Maybe also "... typos in example" in the subject.
Let me know whether I should send a v2 or let maintainers fix it up.
Regards,
Andreas
> Given that:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
>
>> I also see plenty of other bindings with dashes, so this seems
>> reasonable.
>
> Dashes rather than underscores are preferred/correct for property names
> and compatible strings. Given no-one can possibly be using the
> bad/incorrect form with underscores, fixing the documentation to use
> dashes makes sense.
>
> Thanks,
> Mark.
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos
2014-06-25 11:43 ` Andreas Färber
@ 2014-06-25 12:23 ` Rob Herring
0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2014-06-25 12:23 UTC (permalink / raw)
To: Andreas Färber
Cc: Mark Rutland, Doug Anderson, linux-samsung-soc,
Stephan van Schaik, Vincent Palatin, Rob Herring, Pawel Moll,
Ian Campbell, Kumar Gala, Randy Dunlap, Mark Brown, Simon Glass,
Michael Spang, OPEN FIRMWARE AND...,
DOCUMENTATION, LKML
On Wed, Jun 25, 2014 at 6:43 AM, Andreas Färber <afaerber@suse.de> wrote:
> Am 25.06.2014 12:47, schrieb Mark Rutland:
>> On Mon, Jun 23, 2014 at 06:27:04PM +0100, Doug Anderson wrote:
>>> Andreas,
>>>
>>> On Sun, Jun 22, 2014 at 6:21 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>> It's vsys-l{1,2}-supply, not vsys_l{1,2}-supply.
>>>>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> ---
>>>> Documentation/devicetree/bindings/regulator/tps65090.txt | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/regulator/tps65090.txt b/Documentation/devicetree/bindings/regulator/tps65090.txt
>>>> index 34098023..ca69f5e 100644
>>>> --- a/Documentation/devicetree/bindings/regulator/tps65090.txt
>>>> +++ b/Documentation/devicetree/bindings/regulator/tps65090.txt
>>>> @@ -45,8 +45,8 @@ Example:
>>>> infet5-supply = <&some_reg>;
>>>> infet6-supply = <&some_reg>;
>>>> infet7-supply = <&some_reg>;
>>>> - vsys_l1-supply = <&some_reg>;
>>>> - vsys_l2-supply = <&some_reg>;
>>>> + vsys-l1-supply = <&some_reg>;
>>>> + vsys-l2-supply = <&some_reg>;
>>>
>>> Your change matches the code and all existing device trees in the
>>> Linux kernel.
>>
>> Could this fact please be mentioned in the commit message?
>
> Yes, I admit the commit message could've been clearer in stating that
> only the example is modified, not the actual specification. What about:
>
> "Specification and existing device trees use vsys-l{1,2}-supply, not
> vsys_l{1,2}-supply. Fix the example to match the specification."
>
> Maybe also "... typos in example" in the subject.
>
> Let me know whether I should send a v2 or let maintainers fix it up.
Please send v2.
Rob
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-06-25 12:23 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1403486483-4063-1-git-send-email-afaerber@suse.de>
2014-06-23 1:21 ` [PATCH 1/4] Documentation: devicetree: Fix s2mps11 and s5m8767 typos Andreas Färber
2014-06-23 3:21 ` Sachin Kamat
2014-06-23 23:06 ` Andreas Färber
2014-06-23 23:20 ` Doug Anderson
2014-06-23 8:15 ` Lee Jones
2014-06-23 1:21 ` [PATCH 2/4] Documentation: devicetree: Fix s2mps11 example syntax Andreas Färber
2014-06-23 3:23 ` Sachin Kamat
2014-06-23 8:15 ` Lee Jones
2014-06-23 1:21 ` [PATCH 3/4] Documentation: devicetree: Fix tps65090 typos Andreas Färber
2014-06-23 17:27 ` Doug Anderson
2014-06-25 10:47 ` Mark Rutland
2014-06-25 11:43 ` Andreas Färber
2014-06-25 12:23 ` Rob Herring
2014-06-23 1:21 ` [RFC 4/4] ARM: dts: exynos5250: Add Spring device tree Andreas Färber
2014-06-23 19:47 ` Doug Anderson
2014-06-23 22:46 ` Andreas Färber
2014-06-24 4:05 ` Doug Anderson
2014-06-24 10:06 ` Javier Martinez Canillas
2014-06-24 15:20 ` Doug Anderson
2014-06-24 15:06 ` Vincent Palatin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).