Re: [PATCH 7/7] ARM: dts: qcom: msm8226: Add mdss nodes

[Dmitry Baryshkov]

On 29/05/2023 15:10, Konrad Dybcio wrote:
> On 29.05.2023 11:44, Luca Weiss wrote:
> > Add the nodes that describe the mdss so that display can work on
> > MSM8226.
> >
> > Signed-off-by: Luca Weiss <luca@xxxxxxxxx>
> > ---
> >   arch/arm/boot/dts/qcom-msm8226.dtsi | 118 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 118 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/qcom-msm8226.dtsi b/arch/arm/boot/dts/qcom-msm8226.dtsi
> > index 42acb9ddb8cc..182d6405032f 100644
> > --- a/arch/arm/boot/dts/qcom-msm8226.dtsi
> > +++ b/arch/arm/boot/dts/qcom-msm8226.dtsi
> > @@ -636,6 +636,124 @@ smd-edge {
> >   				label = "lpass";
> >   			};
> >   		};
> > +
> > +		mdss: display-subsystem@fd900000 {
> > +			compatible = "qcom,mdss";
> > +			reg = <0xfd900000 0x100>, <0xfd924000 0x1000>;
> > +			reg-names = "mdss_phys", "vbif_phys";
> > +
> > +			power-domains = <&mmcc MDSS_GDSC>;
> > +
> > +			clocks = <&mmcc MDSS_AHB_CLK>,
> > +				 <&mmcc MDSS_AXI_CLK>,
> > +				 <&mmcc MDSS_VSYNC_CLK>;
> > +			clock-names = "iface", "bus", "vsync";
> One per line, please
>
> > +
> > +			interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > +
> > +			interrupt-controller;
> > +			#interrupt-cells = <1>;
> We're not using the irq cell, is that necessary/should that be 0?

No. With 0 it would mean that there is a single interrupt for mdss 
source, which clearly is not the case.

> > +
> > +			status = "disabled";
> status should go last
>
> > +
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges;
> > +
> > +			mdp: display-controller@fd900000 {
> > +				compatible = "qcom,msm8226-mdp5", "qcom,mdp5";
> > +				reg = <0xfd900100 0x22000>;
> > +				reg-names = "mdp_phys";
> > +
> > +				interrupt-parent = <&mdss>;
> > +				interrupts = <0>;
> > +
> > +				clocks = <&mmcc MDSS_AHB_CLK>,
> > +					 <&mmcc MDSS_AXI_CLK>,
> > +					 <&mmcc MDSS_MDP_CLK>,
> > +					 <&mmcc MDSS_VSYNC_CLK>;
> > +				clock-names = "iface", "bus", "core", "vsync";
> One per line, please
>
> > +
> > +				ports {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> Would port { work here? I remember one mdss component's bindings
> didn't allow it but don't recall which one

Let's use ports /port@0 for uniformity even if there is just a single 
port always.

> > +
> > +					port@0 {
> > +						reg = <0>;
> > +						mdp5_intf1_out: endpoint {
> > +							remote-endpoint = <&dsi0_in>;
> > +						};
> > +					};
> > +				};
> > +			};
> > +-- 
With best wishes
Dmitry