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

[Srinivas Kandagatla]

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