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
--
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
