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

From: Bjorn Andersson
Date: Mon May 06 2024 - 19:14:23 EST


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
>