Re: [PATCH v2 06/12] ARM: dts: apq8064: Add MDP support

[Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 10/04/15 21:21, Stephen Boyd wrote:
> On 04/10/15 12:39, Srinivas Kandagatla wrote:
>>
>>
>> On 10/04/15 18:04, Stephen Boyd wrote:
>>> On 04/10/15 05:34, Srinivas Kandagatla wrote:
>>>> @@ -250,6 +265,18 @@
>>>>                };
>>>>            };
>>>>
>>>> +        ext_3p3v: regulator-fixed@1 {
>>>> +            compatible = "regulator-fixed";
>>>> +            regulator-min-microvolt = <3300000>;
>>>> +            regulator-max-microvolt = <3300000>;
>>>> +            regulator-name = "ext_3p3v";
>>>> +            regulator-type = "voltage";
>>>> +            startup-delay-us = <0>;
>>>> +            gpio = <&tlmm_pinmux 77 GPIO_ACTIVE_HIGH>;
>>>> +            enable-active-high;
>>>> +            regulator-boot-on;
>>>> +        };
>>>
>>> This shouldn't be inside the SoC node because it doesn't have a reg
>>> property. It should be in a 'regulators' node that's in the root of the
>>> tree:
>>
>> Is this new DT requirement/style? I have not noticed such a dt style
>> in the past. I might have missed it. Any advantage of doing this way?
>
> It's a style. I'm not sure if it's new, but I feel like I've seen
> mention of it before more than a year ago (see
> arch/arm/boot/dts/tegra30-beaver.dts for an example). The advantage of
> doing it this way is we can see all the gpio/fixed regulators in one
> place and they're physically placed on a separate bus from the SoC bus.
> Typically nodes have reg properties too, so making up fake reg
> properties for the regulator nodes when they're on the SoC bus would be
> wrong and confusing. If they're under some regulators container node we
> can number them from 0 to N and use that for the reg property.
>
Thanks for explaining.
>>>
>>>> +
>>>> +        hdmi: qcom,hdmi-tx@4a00000 {
>>>> +            compatible = "qcom,hdmi-tx-8960";
>>>> +            reg-names = "core_physical";
>>>> +            reg = <0x04a00000 0x1000>;
>>>> +            interrupts = <GIC_SPI 79 IRQ_TYPE_NONE>;
>>>> +            clock-names =
>>>> +                "core_clk",
>>>> +                "master_iface_clk",
>>>> +                "slave_iface_clk";
>>>> +            clocks =
>>>> +                <&mmcc HDMI_APP_CLK>,
>>>> +                <&mmcc HDMI_M_AHB_CLK>,
>>>> +                <&mmcc HDMI_S_AHB_CLK>;
>>>> +            qcom,hdmi-tx-ddc-clk = <&tlmm_pinmux 70
>>>> +                        GPIO_ACTIVE_HIGH>;
>>>> +            qcom,hdmi-tx-ddc-data = <&tlmm_pinmux 71
>>>> +                        GPIO_ACTIVE_HIGH>;
>>>> +            qcom,hdmi-tx-hpd = <&tlmm_pinmux 72
>>>> +                        GPIO_ACTIVE_HIGH>;
>>>
>>> This should be done via the *-gpios method. i.e. hdmi-tx-ddc-clk-gpios,
>>> hdmi-tx-ddc-data-gpios, etc.
>>>
>> Thanks for the inputs,
>>
>> That's true, These are existing bindings, so I can't change it as part
>> of this patch, However I will make another patch to fix this in both
>> drivers and DT for good reasons. Just noticed that bindings are not
>> consistent with the examples and the driver, which I guess is another
>> issue.
>
> Yes, the driver/binding should be fixed and then this patch can be
> corrected and applied. There are no implementations of the DT for this
> device upstream in the dts directory so there's no breakage or backwards
> incompatibility problem by fixing the driver/binding.
>
Yep, In that case, I should pull this patch out of this series just to 
avoid any delays and create a new patchset for fixing bindings + driver 
+ DT.


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