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

[Bjorn Andersson]

On Fri, Apr 26, 2024 at 07:54:42PM GMT, Prahlad Valluru wrote:
> From: Venkata Prahlad Valluru <quic_vvalluru@xxxxxxxxxxx>
> 

Please don't thread new versions off existing version. b4 helps you with
getting these things right, please check go/upstream for more details.

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

> Signed-off-by: Prahlad Valluru <quic_vvalluru@xxxxxxxxxxx>
> ---
> 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.

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

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

Regards,
Bjorn

> +                bias-disable;
> +        };
> +};
> -- 
> 2.17.1
>