Re: [PATCH v3] arm64: dts: qcom: sdm845-xiaomi-beryllium: Add DSI and panel bits

[Amit Pundir <amit.pundir@linaro.org>]

Hi,

On Thu, 11 Feb 2021 at 00:25, AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
>
> Il 10/02/21 09:18, Amit Pundir ha scritto:
> > From: Sumit Semwal <sumit.semwal@linaro.org>
> >
> > Enabling the Display panel for beryllium requires DSI
> > labibb regulators and panel dts nodes to be added.
> > It is also required to keep some of the regulators as
> > always-on.
> >
> > Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> > Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
> > ---
>
> Hello!
> Your patch looks good, however, I have a few concerns...
>
> > v3: Addressed Konrad's concerns. Configured labibb regulators
> >      explicitly based on downstream microvolt values. Display
> >      comes up fine with default discharge-resistor-kohms and
> >      soft-start-us properties, so didn't touch them.
> >      Smoke tested on next-20210209.
> > v2: Rebased to mainline (v5.11-rc6) and fixed build warnings.
> >
> >   .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts      | 64 ++++++++++++++++++++++
> >   1 file changed, 64 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> > index 86cbae63eaf7..5ac049a247e1 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> > +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> > @@ -157,6 +157,14 @@
> >                       regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> >               };
> >
> > +             vreg_l14a_1p8: ldo14 {
> > +                     regulator-min-microvolt = <1800000>;
> > +                     regulator-max-microvolt = <1800000>;
> > +                     regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > +                     regulator-boot-on;
> > +                     regulator-always-on;
> > +             };
> > +
> >               vreg_l17a_1p3: ldo17 {
> >                       regulator-min-microvolt = <1304000>;
> >                       regulator-max-microvolt = <1304000>;
> > @@ -191,6 +199,7 @@
> >                       regulator-min-microvolt = <1200000>;
> >                       regulator-max-microvolt = <1200000>;
> >                       regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > +                     regulator-boot-on;
> >               };
> >       };
> >   };
> > @@ -200,6 +209,43 @@
> >       firmware-name = "qcom/sdm845/cdsp.mdt";
> >   };
> >
> > +&dsi0 {
> > +     status = "okay";
> > +     vdda-supply = <&vreg_l26a_1p2>;
> > +
> > +     #address-cells = <1>;
> > +     #size-cells = <0>;
> > +
> > +     panel@0 {
> > +             compatible = "tianma,fhd-video";
> > +             reg = <0>;
> > +             vddi0-supply = <&vreg_l14a_1p8>;
> > +             vddpos-supply = <&lab>;
> > +             vddneg-supply = <&ibb>;
> > +
> > +             #address-cells = <1>;
> > +             #size-cells = <0>;
> > +
> > +             reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
> > +
> > +             port {
> > +                     tianma_nt36672a_in_0: endpoint {
> > +                             remote-endpoint = <&dsi0_out>;
> > +                     };
> > +             };
> > +     };
> > +};
> > +
> > +&dsi0_out {
> > +     remote-endpoint = <&tianma_nt36672a_in_0>;
> > +     data-lanes = <0 1 2 3>;
> > +};
> > +
> > +&dsi0_phy {
> > +     status = "okay";
> > +     vdds-supply = <&vreg_l1a_0p875>;
> > +};
> > +
> >   &gcc {
> >       protected-clocks = <GCC_QSPI_CORE_CLK>,
> >                          <GCC_QSPI_CORE_CLK_SRC>,
> > @@ -215,6 +261,24 @@
> >       };
> >   };
> >
> > +&ibb {
> > +     regulator-min-microvolt = <4600000>;
> > +     regulator-max-microvolt = <6000000>;
> > +};
> > +
>
> I think you want to also configure overvoltage and overcurrent
> protection values for both LAB and IBB, as these regulators may be a bit
> dangerous if used without.

Can you point me to the relevant DT properties please. I didn't find
any DT properties which set the over voltage/current protection
properties explicitly in upstream as well as in downstream kernel.
Plus I also do not see "regulator-min/max-microamp" values set
downstream, otherwise I'd have used that as well atleast.

Regards,
Amit Pundir

> Besides that, even if it wouldn't be that dangerous, since the
> protection features are present, it would be nice to configure them
> properly as in the rare event that something bad happens, you would be
> able to save the hardware (or at least have a chance to!).
>
> > +&lab {
> > +     regulator-min-microvolt = <4600000>;
> > +     regulator-max-microvolt = <6000000>;
> > +};
> > +
>
> Same here.
>
> Yours,
> -- Angelo
>
> > +&mdss {
> > +     status = "okay";
> > +};
> > +
> > +&mdss_mdp {
> > +     status = "okay";
> > +};
> > +
> >   &mss_pil {
> >       status = "okay";
> >       firmware-name = "qcom/sdm845/mba.mbn", "qcom/sdm845/modem.mdt";
> >
>