Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Sep 2022 09:48:06 -0400
From:   =?utf-8?B?TsOtY29sYXMgRi4gUi4gQS4=?= Prado 
        <nfraprado@collabora.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Chen-Yu Tsai <wenst@chromium.org>, kernel@collabora.com,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 1/3] arm64: dts: mediatek: asurada: Add display regulators
Message-ID: <20220921134806.lbg5meuy4fn6pifm@notapiano>
References: <20220908171153.670762-1-nfraprado@collabora.com>
 <20220908171153.670762-2-nfraprado@collabora.com>
 <ab2027b9-17e8-4fe8-3847-84c54d6f9d58@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ab2027b9-17e8-4fe8-3847-84c54d6f9d58@collabora.com>
Precedence: bulk

On Fri, Sep 09, 2022 at 09:46:33AM +0200, AngeloGioacchino Del Regno wrote:
> Il 08/09/22 19:11, N?colas F. R. A. Prado ha scritto:
> > Add the regulators present on the Asurada platform that are used to
> > power the internal and external displays.
> > 
> > Signed-off-by: N?colas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > 
> >   .../boot/dts/mediatek/mt8192-asurada.dtsi     | 114 ++++++++++++++++++
> >   1 file changed, 114 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> > index 4b314435f8fd..1d99e470ea1a 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
> 
> ..snip..
> 
> > @@ -56,6 +116,18 @@ pp3300_ldo_z: regulator-3v3-z {
> >   		vin-supply = <&ppvar_sys>;
> >   	};
> 
> Can you please add a comment here advertising that this regulator
> will not only provide power to the MIPI bridge, but *also* to the
> display panel itself?
> 
> This is to make sure that everyone understands what's going on, and
> also that we ourselves don't forget about that.
> 
> Probably something like:
> /* pp3300_mipibrdg also enables pp3300_panel */
> 
> I would then propose to add a "regulator-fixed" that has no GPIO
> but with vin-supply as this one.
> 
> pp3300_panel: regulator-3v3-panel {
> 	compatible = "regulator-fixed";
> 	regulator-name = "pp3300_panel";
> 	regulator-min-microvolt = <3300000>;
> 	regulator-max-microvolt = <3300000>;
> 
> 	vin-supply = <&pp3300_mipibrdg>;
> };
> 
> I would also test assigning this regulator to the panel node, as this
> will make sure to cover future corner cases (think about PM suspend/resume).
> 
> P.S.: If you add the pp3300_panel regulator-fixed with that vin-supply,
>       maybe the proposed comment would become a bit overkill. Your choice!

Hi Angelo,

thanks for the feedback.

I think the current layout makes more sense based on my understanding of the
power routing here: a single power line output by the pp3300_mipibrdg regulator
powers both the ANX chip as well as the panel. So I'm going to keep it the way
it is for now. If there are any other concerns please let me know.

Thanks,
N?colas

> 
> Cheers,
> Angelo
> 
> > +	pp3300_mipibrdg: regulator-3v3-mipibrdg {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "pp3300_mipibrdg";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pp3300_mipibrdg_en_pins>;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		enable-active-high;
> > +		regulator-boot-on;
> > +		gpio = <&pio 127 GPIO_ACTIVE_HIGH>;
> > +	};
> > +
> >   	/* separately switched 3.3V power rail */
> >   	pp3300_u: regulator-3v3-u {
> >   		compatible = "regulator-fixed";
>