From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is Date: Tue, 8 Nov 2016 12:30:19 +0100 Message-ID: <20161108113019.GC2085@ulmo.ba.sec> References: <20161106110903.14594-1-icenowy@aosc.xyz> <20161107131736.GD12559@ulmo.ba.sec> <1478528816.2461.25.camel@pengutronix.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0911549151==" Return-path: Received: from mail-pf0-x242.google.com (mail-pf0-x242.google.com [IPv6:2607:f8b0:400e:c00::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 76C1B6E4D5 for ; Tue, 8 Nov 2016 11:30:25 +0000 (UTC) Received: by mail-pf0-x242.google.com with SMTP id 144so10780444pfv.0 for ; Tue, 08 Nov 2016 03:30:25 -0800 (PST) In-Reply-To: <1478528816.2461.25.camel@pengutronix.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Philipp Zabel Cc: Chen-Yu Tsai , Icenowy Zheng , dri-devel , linux-kernel List-Id: dri-devel@lists.freedesktop.org --===============0911549151== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XWOWbaMNXpFDWE00" Content-Disposition: inline --XWOWbaMNXpFDWE00 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 07, 2016 at 03:26:56PM +0100, Philipp Zabel wrote: > Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng wrot= e: > > > > The enable gpio of simple-panel may be used by a simplefb or other > > > > driver on the panel's display before the KMS driver get load. > > > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > > can work. > > > > > > > > Signed-off-by: Icenowy Zheng > > > > --- > > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm= /panel/panel-simple.c > > > > index 113db3c..ccee4c1 100644 > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *de= v, const struct panel_desc *desc) > > > > return PTR_ERR(panel->supply); > > > > > > > > panel->enable_gpio =3D devm_gpiod_get_optional(dev, "enable= ", > > > > - GPIOD_OUT_LOW); > > > > + GPIOD_ASIS); > > >=20 > > > The GPIO requested as-is might be in input mode. You should change the > > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > > allows you to give an initial value. Not sure if it checks for cansle= ep > > > like the set_value calls though. > >=20 > > I'd prefer not to add gpiod_direction_output() calls outside of > > ->probe(). Instead, could we make this patch be smart about taking over > > from an earlier user? Could it read the current direction and value of > > the GPIO and not disable it if it had previously been enabled? >=20 > Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c > already does a similar thing. >=20 > > And even if we go this extra mile there's a possibility that the GPIO > > was just left dangling by earlier software (or hardware) and leaving it > > on would actually be worse than turning the panel off. >=20 > Is this something the encoder driver should communicate to the panel? > That one will know whether its atomic_reset state is enabled or > disabled. At that point it's already too late. Configuration of the GPIO here is within the panel's ->probe() implementation, which is before it can ever even get attached to an encoder/connector. I can't think of a proper solution for this, but perhaps the best heuristic would be to request with GPIOD_ASIS and then query the direction. If it's configured as output we could assume that somebody's configured it explicitly and it has the correct value. That could still break existing use-cases, but it would at least allow basic hand-over. Also, if leaving the GPIO configured as-is causes glitches or other issues, then these are observable with the current code as well, until the panel driver takes over and disables everything. Thierry --XWOWbaMNXpFDWE00 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAABCAAGBQJYIbdAAAoJEN0jrNd/PrOhhhwQAIptazgDF+RqCWyEks4IYbyo aualqnnePlCY9mX9ZimBoHoKpRch3M4tKldAnAyXC44TL32V1bA6+5l8LbUYKTof dVlJBD8jAeEWIvkavVq5KUQm/2DUHhLOOiUY/KqL4iYexN4PclsSvRCpNIudW1kw HZBG1E8YluyEsQG+ibAwXTBaZlZXrcMxqYIHs4YijUWO/HQdqHKnXYVHklyY4y/M 5QKZLV+ykVQklEFEZo3B2LkcjmPGpCw0W72yU4Y0jGmbC9AENQQAJTD5KGENiceu BlfFrrZZD8QlEkNLDcBqmu/VIIFM2DA/IGSiBnraMMocC2c7sr0b7zHU92xCc7IJ GqkVVFyaguVZitPfIDBUf0OIcDfvxcw37588nSlBzjqfu0b78NSvkHew6c7oZXQM 6IJmYQsnjCZejruVMqP9aUmG6eQ0KS7d40RiBnnegLNKkQTTlnDAj/Ol5yN7cIit M+QFqZaGmOktKpE6hA4vC5/C0v5ZhUkw7q/3pgzL2e5Yhhu0lg8wLq55Cpwzor3q R6dg2WMPl1JpZiyXu6Wqr+yfgXWuHM+GaGFbUWV/3/1Irw/sGufyKwigiCZPDNou KPZ+OTcneUY2rDS2K3J9SBMGE3P9wY6kZSNHdI8iG6wbMuK7tC0g+SdqTAwnDiNK AHbDbJmxrIw4TdsGbSlu =S9SX -----END PGP SIGNATURE----- --XWOWbaMNXpFDWE00-- --===============0911549151== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0911549151==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933288AbcKHLa1 (ORCPT ); Tue, 8 Nov 2016 06:30:27 -0500 Received: from mail-pf0-f195.google.com ([209.85.192.195]:34761 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752196AbcKHLaZ (ORCPT ); Tue, 8 Nov 2016 06:30:25 -0500 Date: Tue, 8 Nov 2016 12:30:19 +0100 From: Thierry Reding To: Philipp Zabel Cc: Chen-Yu Tsai , Icenowy Zheng , dri-devel , linux-kernel Subject: Re: [PATCH] drm: panel: simple-panel: get the enable gpio as-is Message-ID: <20161108113019.GC2085@ulmo.ba.sec> References: <20161106110903.14594-1-icenowy@aosc.xyz> <20161107131736.GD12559@ulmo.ba.sec> <1478528816.2461.25.camel@pengutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XWOWbaMNXpFDWE00" Content-Disposition: inline In-Reply-To: <1478528816.2461.25.camel@pengutronix.de> User-Agent: Mutt/1.7.1 (2016-10-04) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --XWOWbaMNXpFDWE00 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 07, 2016 at 03:26:56PM +0100, Philipp Zabel wrote: > Am Montag, den 07.11.2016, 14:17 +0100 schrieb Thierry Reding: > > On Mon, Nov 07, 2016 at 06:12:43PM +0800, Chen-Yu Tsai wrote: > > > On Sun, Nov 6, 2016 at 7:09 PM, Icenowy Zheng wrot= e: > > > > The enable gpio of simple-panel may be used by a simplefb or other > > > > driver on the panel's display before the KMS driver get load. > > > > > > > > Get the GPIO as-is, so the panel won't be disabled, and the simplefb > > > > can work. > > > > > > > > Signed-off-by: Icenowy Zheng > > > > --- > > > > drivers/gpu/drm/panel/panel-simple.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm= /panel/panel-simple.c > > > > index 113db3c..ccee4c1 100644 > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > @@ -312,7 +312,7 @@ static int panel_simple_probe(struct device *de= v, const struct panel_desc *desc) > > > > return PTR_ERR(panel->supply); > > > > > > > > panel->enable_gpio =3D devm_gpiod_get_optional(dev, "enable= ", > > > > - GPIOD_OUT_LOW); > > > > + GPIOD_ASIS); > > >=20 > > > The GPIO requested as-is might be in input mode. You should change the > > > gpiod_set_value calls to gpiod_direction_output calls. The later also > > > allows you to give an initial value. Not sure if it checks for cansle= ep > > > like the set_value calls though. > >=20 > > I'd prefer not to add gpiod_direction_output() calls outside of > > ->probe(). Instead, could we make this patch be smart about taking over > > from an earlier user? Could it read the current direction and value of > > the GPIO and not disable it if it had previously been enabled? >=20 > Seconded, the PWM backlight driver in drivers/video/backlight/pwm_bl.c > already does a similar thing. >=20 > > And even if we go this extra mile there's a possibility that the GPIO > > was just left dangling by earlier software (or hardware) and leaving it > > on would actually be worse than turning the panel off. >=20 > Is this something the encoder driver should communicate to the panel? > That one will know whether its atomic_reset state is enabled or > disabled. At that point it's already too late. Configuration of the GPIO here is within the panel's ->probe() implementation, which is before it can ever even get attached to an encoder/connector. I can't think of a proper solution for this, but perhaps the best heuristic would be to request with GPIOD_ASIS and then query the direction. If it's configured as output we could assume that somebody's configured it explicitly and it has the correct value. That could still break existing use-cases, but it would at least allow basic hand-over. Also, if leaving the GPIO configured as-is causes glitches or other issues, then these are observable with the current code as well, until the panel driver takes over and disables everything. Thierry --XWOWbaMNXpFDWE00 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAABCAAGBQJYIbdAAAoJEN0jrNd/PrOhhhwQAIptazgDF+RqCWyEks4IYbyo aualqnnePlCY9mX9ZimBoHoKpRch3M4tKldAnAyXC44TL32V1bA6+5l8LbUYKTof dVlJBD8jAeEWIvkavVq5KUQm/2DUHhLOOiUY/KqL4iYexN4PclsSvRCpNIudW1kw HZBG1E8YluyEsQG+ibAwXTBaZlZXrcMxqYIHs4YijUWO/HQdqHKnXYVHklyY4y/M 5QKZLV+ykVQklEFEZo3B2LkcjmPGpCw0W72yU4Y0jGmbC9AENQQAJTD5KGENiceu BlfFrrZZD8QlEkNLDcBqmu/VIIFM2DA/IGSiBnraMMocC2c7sr0b7zHU92xCc7IJ GqkVVFyaguVZitPfIDBUf0OIcDfvxcw37588nSlBzjqfu0b78NSvkHew6c7oZXQM 6IJmYQsnjCZejruVMqP9aUmG6eQ0KS7d40RiBnnegLNKkQTTlnDAj/Ol5yN7cIit M+QFqZaGmOktKpE6hA4vC5/C0v5ZhUkw7q/3pgzL2e5Yhhu0lg8wLq55Cpwzor3q R6dg2WMPl1JpZiyXu6Wqr+yfgXWuHM+GaGFbUWV/3/1Irw/sGufyKwigiCZPDNou KPZ+OTcneUY2rDS2K3J9SBMGE3P9wY6kZSNHdI8iG6wbMuK7tC0g+SdqTAwnDiNK AHbDbJmxrIw4TdsGbSlu =S9SX -----END PGP SIGNATURE----- --XWOWbaMNXpFDWE00--