2014-07-31 16:10:57

by Andreas Färber

[permalink] [raw]
Subject: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Adds initial support for the HP Chromebook 11.

Cc: Vincent Palatin <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Stephan van Schaik <[email protected]>
Signed-off-by: Andreas Färber <[email protected]>
---
v3 -> v4:
* Fixed samsung,pin-function 1 -> 0 for dp-hpd-gpio
* Replaced dp-hpd-gpio with existing dp_hpd, overriding it

v2 -> v3:
* Use GPIO_ACTIVE_{LOW,HIGH} (Doug Anderson)
* Use symbolic KEY_POWER instead of comment
* Moved hsic_reset to new USB3503 node's reset-gpios (Vincent Palatin)
* Use dp_hpd_gpio for dp-controller (Doug Anderson, Ajay Kumar)
* Override sd1_{clk,cmd,cd,bus4} pinctrl similar to Snow (Doug Anderson)
* Added ec_irq pinctrl for cros_ec (Doug Anderson)
* Reordered nodes to minimize diff against Snow (Doug Anderson)
* Dropped obsolete mmc_2 override (Doug Anderson)
* Added lid-switch node (Doug Anderson)
* Added gpio-keys pinctrl (Doug Anderson)
* Added bootargs to avoid empty /chosen node and to document console setting
* Renamed s5m8767_pmic node to avoid underscore
* Use new style for overriding inherited pinctrl nodes, too
* Enable i2s0 node

v1 -> v2:
* Use label-based overriding/extension of nodes. (Doug Anderson)
* Dropped tps65090 for now, until we know where to place it.
* Dropped non-Spring nodes from -cros-common.dtsi rather than disabling them.
* Enabled a missing MMC node for access to internal storage.
* Dropped display-timings from dp-controller node. (Ajay Kumar)

arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/exynos5250-spring.dts | 539 ++++++++++++++++++++++++++++++++
2 files changed, 540 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 80a781f76e88..dec4c292f63d 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -76,6 +76,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 000000000000..1fdfa04182fc
--- /dev/null
+++ b/arch/arm/boot/dts/exynos5250-spring.dts
@@ -0,0 +1,539 @@
+/*
+ * 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 <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include "exynos5250.dtsi"
+
+/ {
+ model = "Google Spring";
+ compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
+
+ memory {
+ reg = <0x40000000 0x80000000>;
+ };
+
+ chosen {
+ bootargs = "console=tty1";
+ };
+
+ gpio-keys {
+ compatible = "gpio-keys";
+ pinctrl-names = "default";
+ pinctrl-0 = <&power_key_irq>, <&lid_irq>;
+
+ power {
+ label = "Power";
+ gpios = <&gpx1 3 GPIO_ACTIVE_LOW>;
+ linux,code = <KEY_POWER>;
+ gpio-key,wakeup;
+ };
+
+ lid-switch {
+ label = "Lid";
+ gpios = <&gpx3 5 GPIO_ACTIVE_LOW>;
+ linux,input-type = <5>; /* EV_SW */
+ linux,code = <0>; /* SW_LID */
+ debounce-interval = <1>;
+ gpio-key,wakeup;
+ };
+ };
+
+ usb3_vbus_reg: regulator-usb3 {
+ compatible = "regulator-fixed";
+ regulator-name = "P5.0V_USB3CON";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
+ enable-active-high;
+ };
+
+ usb@12110000 {
+ samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
+ };
+
+ usb-hub {
+ compatible = "smsc,usb3503a";
+ reset-gpios = <&hsic_reset>;
+ };
+
+ fixed-rate-clocks {
+ xxti {
+ compatible = "samsung,clock-xxti";
+ clock-frequency = <24000000>;
+ };
+ };
+
+ hdmi {
+ hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&hdmi_hpd_irq>;
+ phy = <&hdmiphy>;
+ ddc = <&i2c_2>;
+ 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 GPIO_ACTIVE_HIGH>;
+ };
+};
+
+&dp_hpd {
+ samsung,pins = "gpc3-0";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <3>;
+ samsung,pin-drv = <0>;
+};
+
+&i2c_0 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+
+ 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 GPIO_ACTIVE_LOW>, /* DVS1 */
+ <&gpd1 1 GPIO_ACTIVE_LOW>, /* DVS2 */
+ <&gpd1 2 GPIO_ACTIVE_LOW>; /* DVS3 */
+
+ s5m8767,pmic-buck-ds-gpios = <&gpx2 3 GPIO_ACTIVE_LOW>, /* SET1 */
+ <&gpx2 4 GPIO_ACTIVE_LOW>, /* SET2 */
+ <&gpx2 5 GPIO_ACTIVE_LOW>; /* 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_1 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+};
+
+/*
+ * Disabled pullups since external part has its own pullups and
+ * double-pulling gets us out of spec in some cases.
+ */
+&i2c2_bus {
+ samsung,pin-pud = <0>;
+};
+
+&i2c_2 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+
+ hdmiddc@50 {
+ compatible = "samsung,exynos4210-hdmiddc";
+ reg = <0x50>;
+ };
+};
+
+&i2c_3 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_4 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+
+ cros_ec: embedded-controller {
+ compatible = "google,cros-ec-i2c";
+ reg = <0x1e>;
+ interrupts = <6 0>;
+ interrupt-parent = <&gpx1>;
+ wakeup-source;
+ pinctrl-names = "default";
+ pinctrl-0 = <&ec_irq>;
+ };
+};
+
+&i2c_5 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_7 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <66000>;
+};
+
+&i2c_8 {
+ status = "okay";
+ samsung,i2c-sda-delay = <100>;
+ samsung,i2c-max-bus-freq = <378000>;
+
+ hdmiphy: hdmiphy@38 {
+ compatible = "samsung,exynos4212-hdmiphy";
+ reg = <0x38>;
+ };
+};
+
+&i2s0 {
+ status = "okay";
+};
+
+&mmc_0 {
+ status = "okay";
+ num-slots = <1>;
+ supports-highspeed;
+ broken-cd;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-ciu-div = <3>;
+ samsung,dw-mshc-sdr-timing = <2 3>;
+ samsung,dw-mshc-ddr-timing = <1 2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
+
+ slot@0 {
+ reg = <0>;
+ bus-width = <8>;
+ };
+};
+
+&mmc_1 {
+ status = "okay";
+ num-slots = <1>;
+ supports-highspeed;
+ broken-cd;
+ card-detect-delay = <200>;
+ samsung,dw-mshc-ciu-div = <3>;
+ samsung,dw-mshc-sdr-timing = <2 3>;
+ samsung,dw-mshc-ddr-timing = <1 2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_cd &sd1_bus4>;
+
+ slot@0 {
+ reg = <0>;
+ bus-width = <4>;
+ };
+};
+
+&pinctrl_0 {
+ s5m8767_dvs: s5m8767-dvs {
+ samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <1>;
+ samsung,pin-drv = <0>;
+ };
+
+ power_key_irq: power-key-irq {
+ samsung,pins = "gpx1-3";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
+ ec_irq: ec-irq {
+ samsung,pins = "gpx1-6";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ 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>;
+ };
+
+ s5m8767_irq: s5m8767-irq {
+ samsung,pins = "gpx3-2";
+ samsung,pin-function = <0>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+
+ lid_irq: lid-irq {
+ samsung,pins = "gpx3-5";
+ 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_1 {
+ hsic_reset: hsic-reset {
+ samsung,pins = "gpe1-0";
+ samsung,pin-function = <1>;
+ samsung,pin-pud = <0>;
+ samsung,pin-drv = <0>;
+ };
+};
+
+&sd1_clk {
+ samsung,pin-drv = <0>;
+};
+
+&sd1_cmd {
+ samsung,pin-pud = <3>;
+ samsung,pin-drv = <0>;
+};
+
+&sd1_cd {
+ samsung,pin-drv = <0>;
+};
+
+&sd1_bus4 {
+ samsung,pin-drv = <0>;
+};
+
+&spi_1 {
+ status = "okay";
+ samsung,spi-src-clk = <0>;
+ num-cs = <1>;
+};
+
+&usbdrd_phy {
+ vbus-supply = <&usb3_vbus_reg>;
+};
+
+#include "cros-ec-keyboard.dtsi"
--
1.9.3


2014-07-31 17:00:37

by Vincent Palatin

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Always a bit late to the game.
One small comment inline.

Reviewed-by: Vincent Palatin <[email protected]>

On Thu, Jul 31, 2014 at 9:08 AM, Andreas Färber <[email protected]> wrote:
> Adds initial support for the HP Chromebook 11.
>
> Cc: Vincent Palatin <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Stephan van Schaik <[email protected]>
> Signed-off-by: Andreas Färber <[email protected]>
> ---
> v3 -> v4:
> * Fixed samsung,pin-function 1 -> 0 for dp-hpd-gpio
> * Replaced dp-hpd-gpio with existing dp_hpd, overriding it
>
> v2 -> v3:
> * Use GPIO_ACTIVE_{LOW,HIGH} (Doug Anderson)
> * Use symbolic KEY_POWER instead of comment
> * Moved hsic_reset to new USB3503 node's reset-gpios (Vincent Palatin)
> * Use dp_hpd_gpio for dp-controller (Doug Anderson, Ajay Kumar)
> * Override sd1_{clk,cmd,cd,bus4} pinctrl similar to Snow (Doug Anderson)
> * Added ec_irq pinctrl for cros_ec (Doug Anderson)
> * Reordered nodes to minimize diff against Snow (Doug Anderson)
> * Dropped obsolete mmc_2 override (Doug Anderson)
> * Added lid-switch node (Doug Anderson)
> * Added gpio-keys pinctrl (Doug Anderson)
> * Added bootargs to avoid empty /chosen node and to document console setting
> * Renamed s5m8767_pmic node to avoid underscore
> * Use new style for overriding inherited pinctrl nodes, too
> * Enable i2s0 node
>
> v1 -> v2:
> * Use label-based overriding/extension of nodes. (Doug Anderson)
> * Dropped tps65090 for now, until we know where to place it.
> * Dropped non-Spring nodes from -cros-common.dtsi rather than disabling them.
> * Enabled a missing MMC node for access to internal storage.
> * Dropped display-timings from dp-controller node. (Ajay Kumar)
>
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/exynos5250-spring.dts | 539 ++++++++++++++++++++++++++++++++
> 2 files changed, 540 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 80a781f76e88..dec4c292f63d 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -76,6 +76,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 000000000000..1fdfa04182fc
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5250-spring.dts
> @@ -0,0 +1,539 @@
> +/*
> + * 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 <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include "exynos5250.dtsi"
> +
> +/ {
> + model = "Google Spring";
> + compatible = "google,spring", "samsung,exynos5250", "samsung,exynos5";
> +
> + memory {
> + reg = <0x40000000 0x80000000>;
> + };
> +
> + chosen {
> + bootargs = "console=tty1";
> + };
> +
> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-names = "default";
> + pinctrl-0 = <&power_key_irq>, <&lid_irq>;
> +
> + power {
> + label = "Power";
> + gpios = <&gpx1 3 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_POWER>;
> + gpio-key,wakeup;
> + };
> +
> + lid-switch {
> + label = "Lid";
> + gpios = <&gpx3 5 GPIO_ACTIVE_LOW>;
> + linux,input-type = <5>; /* EV_SW */
> + linux,code = <0>; /* SW_LID */
> + debounce-interval = <1>;
> + gpio-key,wakeup;
> + };
> + };
> +
> + usb3_vbus_reg: regulator-usb3 {
> + compatible = "regulator-fixed";
> + regulator-name = "P5.0V_USB3CON";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
> + enable-active-high;
> + };

GPE1_0 GPIO is the HSIC hub (SMSC 3503) reset# line (already defined
below afaik).
On this design there is no external USB3 port, so no VBUS reg/load
switch for USB3.



> +
> + usb@12110000 {
> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + usb-hub {
> + compatible = "smsc,usb3503a";
> + reset-gpios = <&hsic_reset>;
> + };
> +
> + fixed-rate-clocks {
> + xxti {
> + compatible = "samsung,clock-xxti";
> + clock-frequency = <24000000>;
> + };
> + };
> +
> + hdmi {
> + hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hdmi_hpd_irq>;
> + phy = <&hdmiphy>;
> + ddc = <&i2c_2>;
> + 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 GPIO_ACTIVE_HIGH>;
> + };
> +};
> +
> +&dp_hpd {
> + samsung,pins = "gpc3-0";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <3>;
> + samsung,pin-drv = <0>;
> +};
> +
> +&i2c_0 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +
> + 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 GPIO_ACTIVE_LOW>, /* DVS1 */
> + <&gpd1 1 GPIO_ACTIVE_LOW>, /* DVS2 */
> + <&gpd1 2 GPIO_ACTIVE_LOW>; /* DVS3 */
> +
> + s5m8767,pmic-buck-ds-gpios = <&gpx2 3 GPIO_ACTIVE_LOW>, /* SET1 */
> + <&gpx2 4 GPIO_ACTIVE_LOW>, /* SET2 */
> + <&gpx2 5 GPIO_ACTIVE_LOW>; /* 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_1 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +};
> +
> +/*
> + * Disabled pullups since external part has its own pullups and
> + * double-pulling gets us out of spec in some cases.
> + */
> +&i2c2_bus {
> + samsung,pin-pud = <0>;
> +};
> +
> +&i2c_2 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +
> + hdmiddc@50 {
> + compatible = "samsung,exynos4210-hdmiddc";
> + reg = <0x50>;
> + };
> +};
> +
> +&i2c_3 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_4 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +
> + cros_ec: embedded-controller {
> + compatible = "google,cros-ec-i2c";
> + reg = <0x1e>;
> + interrupts = <6 0>;
> + interrupt-parent = <&gpx1>;
> + wakeup-source;
> + pinctrl-names = "default";
> + pinctrl-0 = <&ec_irq>;
> + };
> +};
> +
> +&i2c_5 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_7 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};
> +
> +&i2c_8 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;
> +
> + hdmiphy: hdmiphy@38 {
> + compatible = "samsung,exynos4212-hdmiphy";
> + reg = <0x38>;
> + };
> +};
> +
> +&i2s0 {
> + status = "okay";
> +};
> +
> +&mmc_0 {
> + status = "okay";
> + num-slots = <1>;
> + supports-highspeed;
> + broken-cd;
> + card-detect-delay = <200>;
> + samsung,dw-mshc-ciu-div = <3>;
> + samsung,dw-mshc-sdr-timing = <2 3>;
> + samsung,dw-mshc-ddr-timing = <1 2>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sd0_clk &sd0_cmd &sd0_cd &sd0_bus4 &sd0_bus8>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <8>;
> + };
> +};
> +
> +&mmc_1 {
> + status = "okay";
> + num-slots = <1>;
> + supports-highspeed;
> + broken-cd;
> + card-detect-delay = <200>;
> + samsung,dw-mshc-ciu-div = <3>;
> + samsung,dw-mshc-sdr-timing = <2 3>;
> + samsung,dw-mshc-ddr-timing = <1 2>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_cd &sd1_bus4>;
> +
> + slot@0 {
> + reg = <0>;
> + bus-width = <4>;
> + };
> +};
> +
> +&pinctrl_0 {
> + s5m8767_dvs: s5m8767-dvs {
> + samsung,pins = "gpd1-0", "gpd1-1", "gpd1-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <1>;
> + samsung,pin-drv = <0>;
> + };
> +
> + power_key_irq: power-key-irq {
> + samsung,pins = "gpx1-3";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + ec_irq: ec-irq {
> + samsung,pins = "gpx1-6";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + 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>;
> + };
> +
> + s5m8767_irq: s5m8767-irq {
> + samsung,pins = "gpx3-2";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +
> + lid_irq: lid-irq {
> + samsung,pins = "gpx3-5";
> + 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_1 {
> + hsic_reset: hsic-reset {
> + samsung,pins = "gpe1-0";
> + samsung,pin-function = <1>;
> + samsung,pin-pud = <0>;
> + samsung,pin-drv = <0>;
> + };
> +};
> +
> +&sd1_clk {
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_cmd {
> + samsung,pin-pud = <3>;
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_cd {
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_bus4 {
> + samsung,pin-drv = <0>;
> +};
> +
> +&spi_1 {
> + status = "okay";
> + samsung,spi-src-clk = <0>;
> + num-cs = <1>;
> +};
> +
> +&usbdrd_phy {
> + vbus-supply = <&usb3_vbus_reg>;
> +};
> +
> +#include "cros-ec-keyboard.dtsi"
> --
> 1.9.3
>

2014-07-31 17:15:08

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Hi,

Am 31.07.2014 19:00, schrieb Vincent Palatin:
> Always a bit late to the game.
> One small comment inline.
>
> Reviewed-by: Vincent Palatin <[email protected]>

Thanks,

>
> On Thu, Jul 31, 2014 at 9:08 AM, Andreas Färber <[email protected]> wrote:
>> + usb3_vbus_reg: regulator-usb3 {
>> + compatible = "regulator-fixed";
>> + regulator-name = "P5.0V_USB3CON";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
>> + enable-active-high;
>> + };
>
> GPE1_0 GPIO is the HSIC hub (SMSC 3503) reset# line (already defined
> below afaik).

Yes, that was a suggestion you made on v1.

> On this design there is no external USB3 port, so no VBUS reg/load
> switch for USB3.

Could you be a little clearer? Are you suggesting to drop the gpio
property? I just re-tested that without the regulator node plus the
vbus-supply below I don't get any USB2 (so maybe rename the regulator?).

Regards,
Andreas

>> +
>> + usb@12110000 {
>> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
>> + };
>> +
>> + usb-hub {
>> + compatible = "smsc,usb3503a";
>> + reset-gpios = <&hsic_reset>;
>> + };
[...]
>> +&usbdrd_phy {
>> + vbus-supply = <&usb3_vbus_reg>;
>> +};
>> +
>> +#include "cros-ec-keyboard.dtsi"
>> --
>> 1.9.3

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-31 17:39:06

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Am 31.07.2014 19:14, schrieb Andreas Färber:
> Hi,
>
> Am 31.07.2014 19:00, schrieb Vincent Palatin:
>> Always a bit late to the game.
>> One small comment inline.
>>
>> Reviewed-by: Vincent Palatin <[email protected]>
>
> Thanks,
>
>>
>> On Thu, Jul 31, 2014 at 9:08 AM, Andreas Färber <[email protected]> wrote:
>>> + usb3_vbus_reg: regulator-usb3 {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "P5.0V_USB3CON";
>>> + regulator-min-microvolt = <5000000>;
>>> + regulator-max-microvolt = <5000000>;
>>> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
>>> + enable-active-high;
>>> + };
>>
>> GPE1_0 GPIO is the HSIC hub (SMSC 3503) reset# line (already defined
>> below afaik).
>
> Yes, that was a suggestion you made on v1.
>
>> On this design there is no external USB3 port, so no VBUS reg/load
>> switch for USB3.
>
> Could you be a little clearer? Are you suggesting to drop the gpio
> property?

Nah, doesn't work. Do we need a pinctrl on the usb-hub instead?

Andreas

> I just re-tested that without the regulator node plus the
> vbus-supply below I don't get any USB2 (so maybe rename the regulator?).
>
> Regards,
> Andreas
>
>>> +
>>> + usb@12110000 {
>>> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
>>> + };
>>> +
>>> + usb-hub {
>>> + compatible = "smsc,usb3503a";
>>> + reset-gpios = <&hsic_reset>;
>>> + };
> [...]
>>> +&usbdrd_phy {
>>> + vbus-supply = <&usb3_vbus_reg>;
>>> +};
>>> +
>>> +#include "cros-ec-keyboard.dtsi"
>>> --
>>> 1.9.3
>


--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-31 18:52:24

by Vincent Palatin

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

On Thu, Jul 31, 2014 at 10:14 AM, Andreas Färber <[email protected]> wrote:
> Hi,
>
> Am 31.07.2014 19:00, schrieb Vincent Palatin:
>> Always a bit late to the game.
>> One small comment inline.
>>
>> Reviewed-by: Vincent Palatin <[email protected]>
>
> Thanks,
>
>>
>> On Thu, Jul 31, 2014 at 9:08 AM, Andreas Färber <[email protected]> wrote:
>>> + usb3_vbus_reg: regulator-usb3 {
>>> + compatible = "regulator-fixed";
>>> + regulator-name = "P5.0V_USB3CON";
>>> + regulator-min-microvolt = <5000000>;
>>> + regulator-max-microvolt = <5000000>;
>>> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
>>> + enable-active-high;
>>> + };
>>
>> GPE1_0 GPIO is the HSIC hub (SMSC 3503) reset# line (already defined
>> below afaik).
>
> Yes, that was a suggestion you made on v1.
>
>> On this design there is no external USB3 port, so no VBUS reg/load
>> switch for USB3.
>
> Could you be a little clearer? Are you suggesting to drop the gpio
> property? I just re-tested that without the regulator node plus the
> vbus-supply below I don't get any USB2 (so maybe rename the regulator?).

The 3503 PHY driver is not fully correct, so we probably need to keep
this to get the right init timings when the bootloader has initiliazed
it before.
but yes renaming the regulator to mention that's actually the hsic hub
reset would make it clearer.


>>> +
>>> + usb@12110000 {
>>> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
>>> + };
>>> +
>>> + usb-hub {
>>> + compatible = "smsc,usb3503a";
>>> + reset-gpios = <&hsic_reset>;
>>> + };
> [...]
>>> +&usbdrd_phy {
>>> + vbus-supply = <&usb3_vbus_reg>;
>>> +};
>>> +
>>> +#include "cros-ec-keyboard.dtsi"
>>> --
>>> 1.9.3
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-31 19:05:25

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Hi Andreas,

Sorry for joining the party a bit late, but there were patches with less
people involved so I preferred to review them first.

You can find my comments inline.

On 31.07.2014 18:08, Andreas Färber wrote:
> Adds initial support for the HP Chromebook 11.

[snip]

> + gpio-keys {
> + compatible = "gpio-keys";
> + pinctrl-names = "default";
> + pinctrl-0 = <&power_key_irq>, <&lid_irq>;
> +
> + power {
> + label = "Power";
> + gpios = <&gpx1 3 GPIO_ACTIVE_LOW>;
> + linux,code = <KEY_POWER>;

I assume the key is debounced in hardware, so there is no need for
debounce-interval here. Is this correct?

> + gpio-key,wakeup;
> + };
> +
> + lid-switch {
> + label = "Lid";
> + gpios = <&gpx3 5 GPIO_ACTIVE_LOW>;
> + linux,input-type = <5>; /* EV_SW */
> + linux,code = <0>; /* SW_LID */
> + debounce-interval = <1>;
> + gpio-key,wakeup;
> + };
> + };
> +
> + usb3_vbus_reg: regulator-usb3 {
> + compatible = "regulator-fixed";
> + regulator-name = "P5.0V_USB3CON";
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
> + enable-active-high;
> + };
> +
> + usb@12110000 {

Since this is a brand new dts file, it should use the reference based
syntax, which would be something like

&usbhost {
...
};

below the / { ... }; block.

> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
> + };
> +
> + usb-hub {
> + compatible = "smsc,usb3503a";
> + reset-gpios = <&hsic_reset>;

Hmm, why a -gpios property points to a pinctrl node? Shouldn't there be
a phandle to GPIO bank + GPIO specifier instead?

> + };
> +
> + fixed-rate-clocks {
> + xxti {
> + compatible = "samsung,clock-xxti";
> + clock-frequency = <24000000>;
> + };
> + };

This is also referencing a node from higher level, so it should be done
using a reference.

> +
> + hdmi {
> + hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&hdmi_hpd_irq>;
> + phy = <&hdmiphy>;
> + ddc = <&i2c_2>;
> + hdmi-en-supply = <&s5m_ldo8_reg>;
> + vdd-supply = <&s5m_ldo8_reg>;
> + vdd_osc-supply = <&s5m_ldo10_reg>;
> + vdd_pll-supply = <&s5m_ldo8_reg>;
> + };

Ditto.

> +
> + fimd@14400000 {
> + status = "okay";
> + samsung,invert-vclk;
> + };

Ditto.

> +
> + 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 GPIO_ACTIVE_HIGH>;
> + };

Ditto.

> +};
> +
> +&dp_hpd {
> + samsung,pins = "gpc3-0";
> + samsung,pin-function = <0>;
> + samsung,pin-pud = <3>;
> + samsung,pin-drv = <0>;
> +};

Hmm, what node is this referencing? I believe this should rather
reference the pin controller and add a new board-specific pinconf/pinmux
group instead....

> +
> +&i2c_0 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <378000>;

[snip]

> +/*
> + * Disabled pullups since external part has its own pullups and
> + * double-pulling gets us out of spec in some cases.
> + */
> +&i2c2_bus {
> + samsung,pin-pud = <0>;
> +};

OK, here overriding a generic pinconf group is justified and nicely
explained by a comment.

> +
> +&i2c_2 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +
> + hdmiddc@50 {
> + compatible = "samsung,exynos4210-hdmiddc";
> + reg = <0x50>;
> + };

I don't think this matches current Exynos HDMI bindings, which I believe
have been changed to just take a phandle to i2c bus instead.

> +};
> +
> +&i2c_3 {
> + status = "okay";
> + samsung,i2c-sda-delay = <100>;
> + samsung,i2c-max-bus-freq = <66000>;
> +};

[snip]

> +&sd1_clk {
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_cmd {
> + samsung,pin-pud = <3>;
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_cd {
> + samsung,pin-drv = <0>;
> +};
> +
> +&sd1_bus4 {
> + samsung,pin-drv = <0>;
> +};

Here generic settings are being overridden, so it might be a good idea
to explain why, like with i2c pull-up above.

Best regards,
Tomasz

2014-07-31 19:20:51

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Hi Tomasz,

Am 31.07.2014 21:05, schrieb Tomasz Figa:
> Hi Andreas,
>
> Sorry for joining the party a bit late, but there were patches with less
> people involved so I preferred to review them first.
>
> You can find my comments inline.
>
> On 31.07.2014 18:08, Andreas Färber wrote:
>> Adds initial support for the HP Chromebook 11.
>
> [snip]
>
>> + gpio-keys {
>> + compatible = "gpio-keys";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&power_key_irq>, <&lid_irq>;
>> +
>> + power {
>> + label = "Power";
>> + gpios = <&gpx1 3 GPIO_ACTIVE_LOW>;
>> + linux,code = <KEY_POWER>;
>
> I assume the key is debounced in hardware, so there is no need for
> debounce-interval here. Is this correct?

You're asking the wrong person... This is copied from
-cros-common/-snow. Downstream 3.8 does not have a debounce-interval
property.

>
>> + gpio-key,wakeup;
>> + };
>> +
>> + lid-switch {
>> + label = "Lid";
>> + gpios = <&gpx3 5 GPIO_ACTIVE_LOW>;
>> + linux,input-type = <5>; /* EV_SW */
>> + linux,code = <0>; /* SW_LID */
>> + debounce-interval = <1>;
>> + gpio-key,wakeup;
>> + };
>> + };
>> +
>> + usb3_vbus_reg: regulator-usb3 {
>> + compatible = "regulator-fixed";
>> + regulator-name = "P5.0V_USB3CON";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + gpio = <&gpe1 0 GPIO_ACTIVE_LOW>;
>> + enable-active-high;
>> + };
>> +
>> + usb@12110000 {
>
> Since this is a brand new dts file, it should use the reference based
> syntax, which would be something like
>
> &usbhost {
> ...
> };
>
> below the / { ... }; block.

You will find that I already did that for all nodes that have a label.
Since there are lots of usb nodes, please suggest specific label names.

I originally tried to stay out of existing code, then I was asked to fix
-cros-common, clean up -snow too, now the SoC, ... ;)

>> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
>> + };
>> +
>> + usb-hub {
>> + compatible = "smsc,usb3503a";
>> + reset-gpios = <&hsic_reset>;
>
> Hmm, why a -gpios property points to a pinctrl node? Shouldn't there be
> a phandle to GPIO bank + GPIO specifier instead?

Dunno, can change it. Can I just copy the gpio property from the
regulator above?

>> + };
>> +
>> + fixed-rate-clocks {
>> + xxti {
>> + compatible = "samsung,clock-xxti";
>> + clock-frequency = <24000000>;
>> + };
>> + };
>
> This is also referencing a node from higher level, so it should be done
> using a reference.
>
>> +
>> + hdmi {
>> + hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&hdmi_hpd_irq>;
>> + phy = <&hdmiphy>;
>> + ddc = <&i2c_2>;
>> + hdmi-en-supply = <&s5m_ldo8_reg>;
>> + vdd-supply = <&s5m_ldo8_reg>;
>> + vdd_osc-supply = <&s5m_ldo10_reg>;
>> + vdd_pll-supply = <&s5m_ldo8_reg>;
>> + };
>
> Ditto.

hdmi?

>
>> +
>> + fimd@14400000 {
>> + status = "okay";
>> + samsung,invert-vclk;
>> + };
>
> Ditto.

fimd?

>
>> +
>> + 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 GPIO_ACTIVE_HIGH>;
>> + };
>
> Ditto.

dp_controller? display_port_controller?

>
>> +};
>> +
>> +&dp_hpd {
>> + samsung,pins = "gpc3-0";
>> + samsung,pin-function = <0>;
>> + samsung,pin-pud = <3>;
>> + samsung,pin-drv = <0>;
>> +};
>
> Hmm, what node is this referencing? I believe this should rather
> reference the pin controller and add a new board-specific pinconf/pinmux
> group instead....

It's a -pinctrl node. See v3->v4 change log and discussion on v3.

>> +
>> +&i2c_0 {
>> + status = "okay";
>> + samsung,i2c-sda-delay = <100>;
>> + samsung,i2c-max-bus-freq = <378000>;
>
> [snip]
>
>> +/*
>> + * Disabled pullups since external part has its own pullups and
>> + * double-pulling gets us out of spec in some cases.
>> + */
>> +&i2c2_bus {
>> + samsung,pin-pud = <0>;
>> +};
>
> OK, here overriding a generic pinconf group is justified and nicely
> explained by a comment.

You seem to assume that I actually understand these things. ;)
Just copied from -cros-common/-snow.

>> +
>> +&i2c_2 {
>> + status = "okay";
>> + samsung,i2c-sda-delay = <100>;
>> + samsung,i2c-max-bus-freq = <66000>;
>> +
>> + hdmiddc@50 {
>> + compatible = "samsung,exynos4210-hdmiddc";
>> + reg = <0x50>;
>> + };
>
> I don't think this matches current Exynos HDMI bindings, which I believe
> have been changed to just take a phandle to i2c bus instead.

Copied from -cros-common/-snow.

>> +};
>> +
>> +&i2c_3 {
>> + status = "okay";
>> + samsung,i2c-sda-delay = <100>;
>> + samsung,i2c-max-bus-freq = <66000>;
>> +};
>
> [snip]
>
>> +&sd1_clk {
>> + samsung,pin-drv = <0>;
>> +};
>> +
>> +&sd1_cmd {
>> + samsung,pin-pud = <3>;
>> + samsung,pin-drv = <0>;
>> +};
>> +
>> +&sd1_cd {
>> + samsung,pin-drv = <0>;
>> +};
>> +
>> +&sd1_bus4 {
>> + samsung,pin-drv = <0>;
>> +};
>
> Here generic settings are being overridden, so it might be a good idea
> to explain why, like with i2c pull-up above.

Snow does not have an explanation either, so please suggest what comment
you'd like to see. Consider me just a user with no specs. :)

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

2014-07-31 19:40:33

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Andreas,

On 31.07.2014 21:20, Andreas Färber wrote:
> Am 31.07.2014 21:05, schrieb Tomasz Figa:
>> On 31.07.2014 18:08, Andreas Färber wrote:
>>> Adds initial support for the HP Chromebook 11.
>>
>> [snip]
>>
>>> + gpio-keys {
>>> + compatible = "gpio-keys";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&power_key_irq>, <&lid_irq>;
>>> +
>>> + power {
>>> + label = "Power";
>>> + gpios = <&gpx1 3 GPIO_ACTIVE_LOW>;
>>> + linux,code = <KEY_POWER>;
>>
>> I assume the key is debounced in hardware, so there is no need for
>> debounce-interval here. Is this correct?
>
> You're asking the wrong person... This is copied from
> -cros-common/-snow. Downstream 3.8 does not have a debounce-interval
> property.

Doug, Vincent?

>
>>
>>> + gpio-key,wakeup;
>>> + };

[snip]

>>> +
>>> + usb@12110000 {
>>
>> Since this is a brand new dts file, it should use the reference based
>> syntax, which would be something like
>>
>> &usbhost {
>> ...
>> };
>>
>> below the / { ... }; block.
>
> You will find that I already did that for all nodes that have a label.
> Since there are lots of usb nodes, please suggest specific label names.
>
> I originally tried to stay out of existing code, then I was asked to fix
> -cros-common, clean up -snow too, now the SoC, ... ;)

Well, adding labels should be pretty trivial and IMHO could be squashed
into this patch. For usb@1211000 the right label would be "ehci".

>
>>> + samsung,vbus-gpio = <&gpx1 1 GPIO_ACTIVE_HIGH>;
>>> + };
>>> +
>>> + usb-hub {
>>> + compatible = "smsc,usb3503a";
>>> + reset-gpios = <&hsic_reset>;
>>
>> Hmm, why a -gpios property points to a pinctrl node? Shouldn't there be
>> a phandle to GPIO bank + GPIO specifier instead?
>
> Dunno, can change it. Can I just copy the gpio property from the
> regulator above?

Reading what Vincent posted earlier I would consider this as the right
thing to do and it might even let you remove the fake regulator node.

>
>>> + };
>>> +
>>> + fixed-rate-clocks {
>>> + xxti {
>>> + compatible = "samsung,clock-xxti";
>>> + clock-frequency = <24000000>;
>>> + };
>>> + };
>>
>> This is also referencing a node from higher level, so it should be done
>> using a reference.
>>
>>> +
>>> + hdmi {
>>> + hpd-gpio = <&gpx3 7 GPIO_ACTIVE_HIGH>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&hdmi_hpd_irq>;
>>> + phy = <&hdmiphy>;
>>> + ddc = <&i2c_2>;
>>> + hdmi-en-supply = <&s5m_ldo8_reg>;
>>> + vdd-supply = <&s5m_ldo8_reg>;
>>> + vdd_osc-supply = <&s5m_ldo10_reg>;
>>> + vdd_pll-supply = <&s5m_ldo8_reg>;
>>> + };
>>
>> Ditto.
>
> hdmi?

Sounds good.

>
>>
>>> +
>>> + fimd@14400000 {
>>> + status = "okay";
>>> + samsung,invert-vclk;
>>> + };
>>
>> Ditto.
>
> fimd?

OK.

>
>>
>>> +
>>> + 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 GPIO_ACTIVE_HIGH>;
>>> + };
>>
>> Ditto.
>
> dp_controller? display_port_controller?

dp?

>
>>
>>> +};
>>> +
>>> +&dp_hpd {
>>> + samsung,pins = "gpc3-0";
>>> + samsung,pin-function = <0>;
>>> + samsung,pin-pud = <3>;
>>> + samsung,pin-drv = <0>;
>>> +};
>>
>> Hmm, what node is this referencing? I believe this should rather
>> reference the pin controller and add a new board-specific pinconf/pinmux
>> group instead....
>
> It's a -pinctrl node. See v3->v4 change log and discussion on v3.
>

Well, this is clearly a board specific node anyway, because it does not
refer to a special function, but simply an input/interrupt GPIO. If it
somehow has landed in generic pinctrl dtsi then it should be removed
from there and this patch should simply introduce its own instance of
dp_hpd node, so you did the right thing in v3.

>>> +
>>> +&i2c_0 {
>>> + status = "okay";
>>> + samsung,i2c-sda-delay = <100>;
>>> + samsung,i2c-max-bus-freq = <378000>;
>>
>> [snip]
>>
>>> +/*
>>> + * Disabled pullups since external part has its own pullups and
>>> + * double-pulling gets us out of spec in some cases.
>>> + */
>>> +&i2c2_bus {
>>> + samsung,pin-pud = <0>;
>>> +};
>>
>> OK, here overriding a generic pinconf group is justified and nicely
>> explained by a comment.
>
> You seem to assume that I actually understand these things. ;)
> Just copied from -cros-common/-snow.
>

It is good if those things are being done with some level of
understanding. The DT mechanics are quite well documented in
Documentation/devicetree/bindings, while for HW-specific bits I believe
Chromium guys could give you a hand.

>>> +
>>> +&i2c_2 {
>>> + status = "okay";
>>> + samsung,i2c-sda-delay = <100>;
>>> + samsung,i2c-max-bus-freq = <66000>;
>>> +
>>> + hdmiddc@50 {
>>> + compatible = "samsung,exynos4210-hdmiddc";
>>> + reg = <0x50>;
>>> + };
>>
>> I don't think this matches current Exynos HDMI bindings, which I believe
>> have been changed to just take a phandle to i2c bus instead.
>
> Copied from -cros-common/-snow.
>

This means that somebody probably forgot to update -cros-common/-snow
dts(i). Not the first and I guess probably not the last time...

>>> +};
>>> +
>>> +&i2c_3 {
>>> + status = "okay";
>>> + samsung,i2c-sda-delay = <100>;
>>> + samsung,i2c-max-bus-freq = <66000>;
>>> +};
>>
>> [snip]
>>
>>> +&sd1_clk {
>>> + samsung,pin-drv = <0>;
>>> +};
>>> +
>>> +&sd1_cmd {
>>> + samsung,pin-pud = <3>;
>>> + samsung,pin-drv = <0>;
>>> +};
>>> +
>>> +&sd1_cd {
>>> + samsung,pin-drv = <0>;
>>> +};
>>> +
>>> +&sd1_bus4 {
>>> + samsung,pin-drv = <0>;
>>> +};
>>
>> Here generic settings are being overridden, so it might be a good idea
>> to explain why, like with i2c pull-up above.
>
> Snow does not have an explanation either, so please suggest what comment
> you'd like to see. Consider me just a user with no specs. :)

Doug, Vincent, someone else?

Best regards,
Tomasz

2014-07-31 20:37:05

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Am 31.07.2014 21:05, schrieb Tomasz Figa:
>> + };
>> +
>> + fixed-rate-clocks {
>> + xxti {
>> + compatible = "samsung,clock-xxti";
>> + clock-frequency = <24000000>;
>> + };
>> + };
>
> This is also referencing a node from higher level, so it should be done
> using a reference.

Actually this one isn't! Neither fixed-rate-clocks nor xxti exists in
the .dtsi.

Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-07-31 21:10:07

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree



On 31.07.2014 22:36, Andreas Färber wrote:
> Am 31.07.2014 21:05, schrieb Tomasz Figa:
>>> + };
>>> +
>>> + fixed-rate-clocks {
>>> + xxti {
>>> + compatible = "samsung,clock-xxti";
>>> + clock-frequency = <24000000>;
>>> + };
>>> + };
>>
>> This is also referencing a node from higher level, so it should be done
>> using a reference.
>
> Actually this one isn't! Neither fixed-rate-clocks nor xxti exists in
> the .dtsi.

You've got me here. :)

In new dts I actually suggest to specify those on SoC level, since they
are generic SoC inputs and just override their frequencies on board
level, but apparently Exynos5250 doesn't follow this fashion. So this is
fine as is, sorry for noise.

Best regards,
Tomasz

2014-07-31 23:17:23

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Am 31.07.2014 21:40, schrieb Tomasz Figa:
> On 31.07.2014 21:20, Andreas Färber wrote:
>> Am 31.07.2014 21:05, schrieb Tomasz Figa:
>>> On 31.07.2014 18:08, Andreas Färber wrote:
>>>> + usb-hub {
>>>> + compatible = "smsc,usb3503a";
>>>> + reset-gpios = <&hsic_reset>;
>>>
>>> Hmm, why a -gpios property points to a pinctrl node? Shouldn't there be
>>> a phandle to GPIO bank + GPIO specifier instead?
>>
>> Dunno, can change it. Can I just copy the gpio property from the
>> regulator above?
>
> Reading what Vincent posted earlier I would consider this as the right
> thing to do and it might even let you remove the fake regulator node.

Indeed it does, thanks for spotting this!

[...]
>>>> +&dp_hpd {
>>>> + samsung,pins = "gpc3-0";
>>>> + samsung,pin-function = <0>;
>>>> + samsung,pin-pud = <3>;
>>>> + samsung,pin-drv = <0>;
>>>> +};
>>>
>>> Hmm, what node is this referencing? I believe this should rather
>>> reference the pin controller and add a new board-specific pinconf/pinmux
>>> group instead....
>>
>> It's a -pinctrl node. See v3->v4 change log and discussion on v3.
>>
>
> Well, this is clearly a board specific node anyway, because it does not
> refer to a special function, but simply an input/interrupt GPIO. If it
> somehow has landed in generic pinctrl dtsi then it should be removed
> from there and this patch should simply introduce its own instance of
> dp_hpd node, so you did the right thing in v3.

Well, my point was that the 3.8 tree contains only one dp-hpd node, not
two as we would get by adding a new node here.

Apart from Spring, it's used in Snow and SMDK5250, so moving it there
seems feasible and the cleanest solution to me.

>>> [snip]
>>>
>>>> +/*
>>>> + * Disabled pullups since external part has its own pullups and
>>>> + * double-pulling gets us out of spec in some cases.
>>>> + */
>>>> +&i2c2_bus {
>>>> + samsung,pin-pud = <0>;
>>>> +};
>>>
>>> OK, here overriding a generic pinconf group is justified and nicely
>>> explained by a comment.
>>
>> You seem to assume that I actually understand these things. ;)
>> Just copied from -cros-common/-snow.
>>
>
> It is good if those things are being done with some level of
> understanding. The DT mechanics are quite well documented in
> Documentation/devicetree/bindings, while for HW-specific bits I believe
> Chromium guys could give you a hand.

I did read and even fix documentation for those bindings that I added
myself in Spring, just not for those that were already in common code,
like this one here.

A tps65090 patch has been ignored since being asked to extend the commit
message, v3 was recently sent. Help getting that in appreciated.

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

2014-07-31 23:26:42

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

On 01.08.2014 01:17, Andreas Färber wrote:
> Am 31.07.2014 21:40, schrieb Tomasz Figa:
>> On 31.07.2014 21:20, Andreas Färber wrote:
>>> Am 31.07.2014 21:05, schrieb Tomasz Figa:
>>>> On 31.07.2014 18:08, Andreas Färber wrote:
[snip]
>>>>> +&dp_hpd {
>>>>> + samsung,pins = "gpc3-0";
>>>>> + samsung,pin-function = <0>;
>>>>> + samsung,pin-pud = <3>;
>>>>> + samsung,pin-drv = <0>;
>>>>> +};
>>>>
>>>> Hmm, what node is this referencing? I believe this should rather
>>>> reference the pin controller and add a new board-specific pinconf/pinmux
>>>> group instead....
>>>
>>> It's a -pinctrl node. See v3->v4 change log and discussion on v3.
>>>
>>
>> Well, this is clearly a board specific node anyway, because it does not
>> refer to a special function, but simply an input/interrupt GPIO. If it
>> somehow has landed in generic pinctrl dtsi then it should be removed
>> from there and this patch should simply introduce its own instance of
>> dp_hpd node, so you did the right thing in v3.
>
> Well, my point was that the 3.8 tree contains only one dp-hpd node, not
> two as we would get by adding a new node here.
>
> Apart from Spring, it's used in Snow and SMDK5250, so moving it there
> seems feasible and the cleanest solution to me.
>

What I mean is that in exynos5250-pinctrl.dtsi only generic SoC pin
groups should be defined and those more or less correspond to groups
with samsung,pin-function set to something other than 0 (input) or 1
(output). Now here hpd_gpio is just a normal GPIO input used as
interrupt source to detect when a cable is plugged or unplugged. This is
by no means generic to the SoC, because any GPIO with interrupt
capability can be used for this purpose. This means that the whole
pin{conf,mux} group should be defined on board level.

Best regards,
Tomasz

>>>> [snip]
>>>>
>>>>> +/*
>>>>> + * Disabled pullups since external part has its own pullups and
>>>>> + * double-pulling gets us out of spec in some cases.
>>>>> + */
>>>>> +&i2c2_bus {
>>>>> + samsung,pin-pud = <0>;
>>>>> +};
>>>>
>>>> OK, here overriding a generic pinconf group is justified and nicely
>>>> explained by a comment.
>>>
>>> You seem to assume that I actually understand these things. ;)
>>> Just copied from -cros-common/-snow.
>>>
>>
>> It is good if those things are being done with some level of
>> understanding. The DT mechanics are quite well documented in
>> Documentation/devicetree/bindings, while for HW-specific bits I believe
>> Chromium guys could give you a hand.
>
> I did read and even fix documentation for those bindings that I added
> myself in Spring, just not for those that were already in common code,
> like this one here.

My intention was that if there is something not clear, rather than
blindly copy-pasting it, it might be worth to ask people that might
know. Although any issues should generally be found at review stage, so
I don't really have any objections, assuming that Chromium people assure
that this patch adds valid data indeed.

>
> A tps65090 patch has been ignored since being asked to extend the commit
> message, v3 was recently sent. Help getting that in appreciated.

Will take a look if time allows. Thanks for your work on this.

Best regards,
Tomasz

2014-07-31 23:31:36

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Am 01.08.2014 01:26, schrieb Tomasz Figa:
> On 01.08.2014 01:17, Andreas Färber wrote:
>> Am 31.07.2014 21:40, schrieb Tomasz Figa:
>>> On 31.07.2014 21:20, Andreas Färber wrote:
>>>> Am 31.07.2014 21:05, schrieb Tomasz Figa:
>>>>> On 31.07.2014 18:08, Andreas Färber wrote:
> [snip]
>>>>>> +&dp_hpd {
>>>>>> + samsung,pins = "gpc3-0";
>>>>>> + samsung,pin-function = <0>;
>>>>>> + samsung,pin-pud = <3>;
>>>>>> + samsung,pin-drv = <0>;
>>>>>> +};
>>>>>
>>>>> Hmm, what node is this referencing? I believe this should rather
>>>>> reference the pin controller and add a new board-specific pinconf/pinmux
>>>>> group instead....
>>>>
>>>> It's a -pinctrl node. See v3->v4 change log and discussion on v3.
>>>>
>>>
>>> Well, this is clearly a board specific node anyway, because it does not
>>> refer to a special function, but simply an input/interrupt GPIO. If it
>>> somehow has landed in generic pinctrl dtsi then it should be removed
>>> from there and this patch should simply introduce its own instance of
>>> dp_hpd node, so you did the right thing in v3.
>>
>> Well, my point was that the 3.8 tree contains only one dp-hpd node, not
>> two as we would get by adding a new node here.
>>
>> Apart from Spring, it's used in Snow and SMDK5250, so moving it there
>> seems feasible and the cleanest solution to me.
>>
>
> What I mean is that in exynos5250-pinctrl.dtsi only generic SoC pin
> groups should be defined and those more or less correspond to groups
> with samsung,pin-function set to something other than 0 (input) or 1
> (output). Now here hpd_gpio is just a normal GPIO input used as
> interrupt source to detect when a cable is plugged or unplugged. This is
> by no means generic to the SoC, because any GPIO with interrupt
> capability can be used for this purpose. This means that the whole
> pin{conf,mux} group should be defined on board level.

Exactly what I meant! :)

Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

2014-08-01 03:17:51

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH v4 4/4] ARM: dts: Add exynos5250-spring device tree

Am 31.07.2014 21:05, schrieb Tomasz Figa:
>> +
>> +&i2c_2 {
>> + status = "okay";
>> + samsung,i2c-sda-delay = <100>;
>> + samsung,i2c-max-bus-freq = <66000>;
>> +
>> + hdmiddc@50 {
>> + compatible = "samsung,exynos4210-hdmiddc";
>> + reg = <0x50>;
>> + };
>
> I don't think this matches current Exynos HDMI bindings, which I believe
> have been changed to just take a phandle to i2c bus instead.

Looks correct to me:

http://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/tree/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt?h=for-next

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt

Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg