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

From: Tomasz Figa
Date: Thu Jul 31 2014 - 15:05:34 EST


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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/