From patchwork Fri Apr 26 12:22:59 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Luca Ceresoli X-Patchwork-Id: 13644717 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8507EC4345F for ; Fri, 26 Apr 2024 12:23:19 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C03C610EA8A; Fri, 26 Apr 2024 12:23:18 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=bootlin.com header.i=@bootlin.com header.b="SwK1fSt3"; dkim-atps=neutral Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by gabe.freedesktop.org (Postfix) with ESMTPS id A8EA210EA8A for ; Fri, 26 Apr 2024 12:23:16 +0000 (UTC) Received: by mail.gandi.net (Postfix) with ESMTPSA id 85CDAC0002; Fri, 26 Apr 2024 12:23:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1714134194; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=25H8oLPo2hioXDZa3XWQH6r/1CP1DR1BEzXpdCWRmVg=; b=SwK1fSt3RJW3evyI512a5q9PmIwv5tsY1bymwqWDRRDtAGvPSDL2/h9KMq+9OfynodgMPU Gvb2n7wT/uglFzdrOm/Fk6g1AszGMWrQlleJ5JD1LQSp36p9CPB6DSQrIDw42uNSh/fFfa OC5jEfW5VEtOBl8CFeH606tTYdFxia6zCiICxr+9xCsNcXpNLzQ4GR986y52wBn4ssw61E jbKgI0WejgtL0EO1X5Fa9h4hhZY20Q7uNOcai3bNCEa7m3Apc4G5nGJaJ4M5JGhPVakDwr jQKYR5846+ximv+zpUlTXiCI9aU49sWMIm9lTYaLQHx4Og3cEsrgZKQTYIZ+cA== From: Luca Ceresoli To: Andrzej Hajda , Neil Armstrong , Robert Foss , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Alexander Stein Cc: Luca Ceresoli , Thomas Petazzoni , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: [PATCH RESEND] Revert "drm/bridge: ti-sn65dsi83: Fix enable error path" Date: Fri, 26 Apr 2024 14:22:59 +0200 Message-Id: <20240426122259.46808-1-luca.ceresoli@bootlin.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-GND-Sasl: luca.ceresoli@bootlin.com X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" This reverts commit 8a91b29f1f50ce7742cdbe5cf11d17f128511f3f. The regulator_disable() added by the original commit solves one kind of regulator imbalance but adds another one as it allows the regulator to be disabled one more time than it is enabled in the following scenario: 1. Start video pipeline -> sn65dsi83_atomic_pre_enable -> regulator_enable 2. PLL lock fails -> regulator_disable 3. Stop video pipeline -> sn65dsi83_atomic_disable -> regulator_disable The reason is clear from the code flow, which looks like this (after removing unrelated code): static void sn65dsi83_atomic_pre_enable() { regulator_enable(ctx->vcc); if (PLL failed locking) { regulator_disable(ctx->vcc); <---- added by patch being reverted return; } } static void sn65dsi83_atomic_disable() { regulator_disable(ctx->vcc); } The use case for introducing the additional regulator_disable() was removing the module for debugging (see link below for the discussion). If the module is removed after a .atomic_pre_enable, i.e. with an active pipeline from the DRM point of view, .atomic_disable is not called and thus the regulator would not be disabled. According to the discussion however there is no actual use case for removing the module with an active pipeline, except for debugging/development. On the other hand, the occurrence of a PLL lock failure is possible due to any physical reason (e.g. a temporary hardware failure for electrical reasons) so handling it gracefully should be supported. As there is no way for .atomic[_pre]_enable to report an error to the core, the only clean way to support it is calling regulator_disabled() only in .atomic_disable, unconditionally, as it was before. Link: https://lore.kernel.org/all/15244220.uLZWGnKmhe@steina-w/ Fixes: 8a91b29f1f50 ("drm/bridge: ti-sn65dsi83: Fix enable error path") Reviewed-by: Alexander Stein Signed-off-by: Luca Ceresoli --- Many thanks to Alexander for the discussion. --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 4814b7b6d1fd..57a7ed13f996 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -478,7 +478,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret); /* On failure, disable PLL again and exit. */ regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00); - regulator_disable(ctx->vcc); return; }