Date:   Wed, 8 Feb 2023 18:09:48 +0000
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Douglas Anderson <dianders@chromium.org>
Cc:     Bjorn Andersson <andersson@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm64: dts: qcom: sc7280: Power herobrine's 3.3 eDP/TS
 rail more properly
Message-ID: <Y+PlbAVDLoSUKJ7U@google.com>
References: <20230207163550.1.I5ff72b7746d5fca8f10ea61351bde4150ed1a7f8@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230207163550.1.I5ff72b7746d5fca8f10ea61351bde4150ed1a7f8@changeid>
Precedence: bulk

On Tue, Feb 07, 2023 at 04:36:10PM -0800, Douglas Anderson wrote:
> This is the equivalent of commit f5b4811e8758 ("arm64: dts: qcom:
> sc7180: Add trogdor eDP/touchscreen regulator off-on-time") and commit
> 23ff866987de ("arm64: dts: qcom: sc7180: Start the trogdor
> eDP/touchscreen regulator on"), but for herobrine instead of trogdor.
> 
> The motivations for herobrine are the same as for trogdor.
> 
> NOTES:
> * Currently for herobrine all boards are eDP, not MIPI. If/when we
>   have herobrine derivatives that are MIPI they we can evaluate
>   whether the same off-on-delay makes sense for them. For trogdor we
>   didn't add the delay to MIPI panels because the problem was found
>   late and nobody had complained about it. For herobrine defaulting to
>   assuming the same 500ms makes sense and if we find we need to
>   optimize later we can.
> * Currently there are no oddball herobrine boards like homestar where
>   the panel really likes to be power cycled. If we have an oddball
>   board it will need to split the eDP and touchscreen rail anyway
>   (like homestar did) and we'll have to delete the "regulator-boot-on"
>   from that board.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This patch should be applied atop my recent series adjusting the
> herobrine touchscreen rails [1]. If I need to send a v2 of that series
> I will add this at the end of it as patch #8.
> 
> [1] https://lore.kernel.org/all/20230207024816.525938-1-dianders@chromium.org/
> 
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> index ded36b5d28c7..312cc0e1cbc7 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> @@ -110,6 +110,22 @@ pp3300_left_in_mlb: pp3300-left-in-mlb-regulator {
>  
>  		regulator-enable-ramp-delay = <3000>;
>  
> +		/*
> +		 * eDP panel specs nearly always have a spec that says you

uber-nit: you could replace the second 'spec' with 'requirement' to avoid
the repetition.

> +		 * shouldn't turn them off an on again without waiting 500ms.
> +		 * Add this as a board constraint since this rail is shared
> +		 * between the panel and touchscreen.
> +		 */
> +		off-on-delay-us = <500000>;
> +
> +		/*
> +		 * Stat the regulator on. This has the advantage of starting

s/Stat/Start/ ?

> +		 * the slow process of powering the panel on as soon as we
> +		 * probe the regulator. It also avoids tripping the
> +		 * off-on-delay immediately on every bootup.
> +		 */
> +		regulator-boot-on;
> +
>  		vin-supply = <&pp3300_z1>;
>  	};

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>