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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
