Re: [PATCH v2] arm64: dts: qcom: qcs6490-rb3gen2: enable hdmi bridge

[Prahlad Valluru <quic_vvalluru@quicinc.com>]

On Mon, May 06, 2024 at 06:14:10PM -0500, Bjorn Andersson wrote:
> On Fri, Apr 26, 2024 at 07:54:42PM GMT, Prahlad Valluru wrote:
> > From: Venkata Prahlad Valluru <quic_vvalluru@quicinc.com>
> > 
> 
> Please don't thread new versions off existing version. b4 helps you with
> getting these things right, please check go/upstream for more details.

My internal gitconfig is not configured correctly. Fixed in v3.

> 
> > Enable lt9611uxc bridge for qcs6490 rb3 gen2 platform.
> > 
> 
> Even if it's clear what this is, I would prefer if you described the
> hardware a little bit in your commit message.
> "Rb3Gen2 has a HDMI connector, connected to DSI via a LT on i2cX.... reset and
> irq pins comes from x and y. Describe this."
> 

Agreed. Updated the commit text to include bridge details.

> > Signed-off-by: Prahlad Valluru <quic_vvalluru@quicinc.com>
> > ---
> > v2: Addressed dtschema errors
> > 	- Fixed lt9611-irq
> > 	- vdd-supply error to be ignored, as it is connected to
> > 	  input supply directly, on rb3gen2
> > ---
> >  arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 86 ++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > index a085ff5b5fb2..c14d4a4bb3ce 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> > @@ -51,6 +51,18 @@
> >  			};
> >  		};
> >  	};
> > +	
> 
> There's a stray tab here, please run checkpatch --strict on your
> patches.

Fixed the checkpatch issues.

> 
> > +	hdmi-connector {
> > +		compatible = "hdmi-connector";
> > +		label = "HDMI";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con: endpoint {
> > +				remote-endpoint = <&lt9611_out>;
> > +			};
> > +		};
> > +	};
> >  
> >  	reserved-memory {
> >  		xbl_mem: xbl@80700000 {
> > @@ -530,6 +542,45 @@
> [..]
> > @@ -602,6 +653,21 @@
> >  	status = "okay";
> >  };
> >  
> > +&mdss_dsi {
> 
> We want to keep nodes sorted (by address if applicable, otherwise
> alphabetically on node names and on labels). "mdss_dsi" < "mdss_edp".
> 
> So please move this up where it belongs.
> 

Moved the dsi nodes on top of edp. Didn't realise this.

> > +        vdda-supply = <&vreg_l6b_1p2>;
> > +        status = "okay";
> > +};
> > +
> > +&mdss_dsi0_out {
> > +        remote-endpoint = <&lt9611_a>;
> > +        data-lanes = <0 1 2 3>;
> > +};
> > +
> > +&mdss_dsi_phy {
> > +        vdds-supply = <&vreg_l10c_0p88>;
> > +        status = "okay";
> > +};
> > +
> >  &qupv3_id_0 {
> >  	status = "okay";
> >  };
> > @@ -711,3 +777,23 @@
> >  	function = "gpio";
> >  	bias-disable;
> >  };
> > +
> > +&pm7250b_gpios {
> > +        lt9611_rst_pin: lt9611-rst-state {
> > +                pins = "gpio2";
> > +                function = "normal";
> > +
> > +                output-high;
> > +                input-disable;
> > +                power-source = <0>;
> > +        };
> > +};
> > +
> > +&tlmm {
> > +        lt9611_irq_pin: lt9611-irq-state {
> > +                pins = "gpio24";
> > +                function = "gpio";
> > +                drive-strength = <8>;
> 
> I'd expect a 2 here, so please document why this is 8.

Accepted. I dont see any reason to have drive strength 8. 

> 
> Regards,
> Bjorn
> 
> > +                bias-disable;
> > +        };
> > +};
> > -- 
> > 2.17.1
> >