Hi,
On Thu, 11 Feb 2021 at 00:25, AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxxx> wrote:
>
> Il 10/02/21 09:18, Amit Pundir ha scritto:
> > From: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Amit Pundir <amit.pundir@xxxxxxxxxx>
> > ---
>
> 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";
> >
>
