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

[Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html