[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240508102202.GA28609@hu-vvalluru-hyd.qualcomm.com>
Date: Wed, 8 May 2024 15:52:02 +0530
From: Prahlad Valluru <quic_vvalluru@...cinc.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: <robh@...nel.org>, <conor+dt@...nel.org>, <devicetree@...r.kernel.org>,
<konrad.dybcio@...aro.org>, <krzysztof.kozlowski+dt@...aro.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<quic_abhinavk@...cinc.com>, <quic_nankam@...cinc.com>
Subject: Re: [PATCH v2] arm64: dts: qcom: qcs6490-rb3gen2: enable hdmi bridge
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@...cinc.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@...cinc.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 = <<9611_out>;
> > + };
> > + };
> > + };
> >
> > reserved-memory {
> > xbl_mem: xbl@...00000 {
> > @@ -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 = <<9611_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
> >
Powered by blists - more mailing lists