Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Philippe CORNU <philippe.cornu@st.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	Alexandre TORGUE <alexandre.torgue@st.com>,
	David Airlie <airlied@linux.ie>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Yannick FERTRE <yannick.fertre@st.com>,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH v2 2/2] drm/bridge: sii902x: add optional power supplies
Date: Tue, 15 May 2018 10:36:30 +0300	[thread overview]
Message-ID: <2814752.ZM2IG9T3fV@avalon> (raw)
In-Reply-To: <8ed2cfcd-df26-fa9c-c45d-7bf4020f0faa@st.com>

Hi Philippe,

On Monday, 14 May 2018 21:58:48 EEST Philippe CORNU wrote:
> On 05/14/2018 12:33 PM, Andrzej Hajda wrote:
> > On 14.05.2018 11:38, Philippe CORNU wrote:
> >> On 04/25/2018 09:53 AM, Philippe Cornu wrote:
> >>> Add the optional power supplies using the description found in
> >>> "SiI9022A/SiI9024A HDMI Transmitter Data Sheet (August 2016)".
> >>>
> >>> The sii902x input IOs are not "io safe" so it is important to
> >>> enable/disable voltage regulators during probe/remove phases to
> >>> avoid damages.
> > 
> > What exactly does it mean? Ie I understand that the chip has some
> > limitations, but why enabling/disabling regulators in probe/remove
> > should solve it?
> 
> thank you for your comment.
> 
> And sorry for the "bad" explanation in the 2nd paragraph about the fact 
> that inputs are not "io safe". I added this 2nd paragraph in v2 
> following a good comment from Laurent on adding the management of the 
> regulators outside the probe/remove for a better power consumption 
> management (enable/disable regulators only when the ic is used for 
> displaying something for instance...). But after a deeper analysis, I 
> realized that the only way to improve the power consumption is to 
> implement & test the sii902x various sleep modes, that is out-of-scope 
> of this small patch and also out-of-scope of my test board I use on 
> which the sii902x bridge ic power consumption is very low compare to the 
> rest of the system...
> 
> I will remove this "explanation" in v3 as it creates confusion.

I'd rather keep it and expand it explain why enabling/disabling regulators at 
probe/remove solves the problem. Your patch otherwise looks OK (although if 
you submit a v3 anyway you could also rename err_disable_regulator to 
err_disable_regulators).

> >>> Signed-off-by: Philippe Cornu <philippe.cornu@st.com>
> >>> ---
> >>> 
> >>> drivers/gpu/drm/bridge/sii902x.c | 38 ++++++++++++++++++++++++++++++----
> >>> 1 file changed, 34 insertions(+), 4 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel