From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965016AbcHaPkO (ORCPT ); Wed, 31 Aug 2016 11:40:14 -0400 Received: from down.free-electrons.com ([37.187.137.238]:39856 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S964969AbcHaPkM (ORCPT ); Wed, 31 Aug 2016 11:40:12 -0400 Date: Wed, 31 Aug 2016 17:40:02 +0200 From: Maxime Ripard To: Chen-Yu Tsai Cc: David Airlie , dri-devel , linux-arm-kernel , linux-kernel Subject: Re: [PATCH] drm/sun4i: Clear encoder->bridge if a bridge is not found Message-ID: <20160831154002.GC14379@lukather> References: <20160830122223.21377-1-wens@csie.org> <20160830125622.GF18605@lukather> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zjcmjzIkjQU2rmur" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --zjcmjzIkjQU2rmur Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 30, 2016 at 11:51:26PM +0800, Chen-Yu Tsai wrote: > On Tue, Aug 30, 2016 at 8:56 PM, Maxime Ripard > wrote: > > Hi, > > > > On Tue, Aug 30, 2016 at 08:22:23PM +0800, Chen-Yu Tsai wrote: > >> The KMS helpers (drm_atomic_helper_check_modeset/mode_fixup) pass > >> encoder->bridge directly to drm_bridge_mode_fixup, which expects a > >> valid pointer, or NULL (in which case it just returns). > >> > >> Clear encoder->bridge if a bridge is not found, instead of keeping > >> the ERR_PTR value. > >> > >> Since other drm_bridge functions also follow this pattern of checking > >> for a non-NULL pointer, we can drop the ifs around the calls and just > >> pass the pointer directly. > >> > >> Fixes: 894f5a9f4b4a ("drm/sun4i: Add bridge support") > >> Signed-off-by: Chen-Yu Tsai > >> --- > >> drivers/gpu/drm/sun4i/sun4i_rgb.c | 11 ++++++----- > >> 1 file changed, 6 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i= /sun4i_rgb.c > >> index d4e52522ec53..ee0795152a33 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > >> @@ -154,8 +154,7 @@ static void sun4i_rgb_encoder_enable(struct drm_en= coder *encoder) > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_enable(tcon->panel); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_enable(encoder->bridge); > >> + drm_bridge_enable(encoder->bridge); > >> > >> sun4i_tcon_channel_enable(tcon, 0); > >> } > >> @@ -170,8 +169,7 @@ static void sun4i_rgb_encoder_disable(struct drm_e= ncoder *encoder) > >> > >> sun4i_tcon_channel_disable(tcon, 0); > >> > >> - if (!IS_ERR(encoder->bridge)) > >> - drm_bridge_disable(encoder->bridge); > >> + drm_bridge_disable(encoder->bridge); > > > > I'd rather keep those changes, it makes it obvious that it's something > > optionnal, that can be set to NULL. >=20 > OK. >=20 > >> if (!IS_ERR(tcon->panel)) > >> drm_panel_disable(tcon->panel); > >> @@ -230,6 +228,9 @@ int sun4i_rgb_init(struct drm_device *drm) > >> return 0; > >> } > >> > >> + if (IS_ERR(encoder->bridge)) > >> + encoder->bridge =3D NULL; > >> + > > > > And that could be the else condition of the if statement below. >=20 > That would be a bit confusing, changing it after calling drm_encoder_init. > The code says it ok to do though. The magic really happens only after the encoder has been attached to something, so it's really safe. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --zjcmjzIkjQU2rmur Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXxvpSAAoJEBx+YmzsjxAgb40P/0FoV7pzO2O+vg6yByMmKSFO NMQ+QWFGcGXfz69tp51+D5KDRJuhe3UXOw1QIOehNzYLAhJQ8kCAvDvo25MVoPRE 5U4FZIZNDbO0iGVu/+x1yMC/9DJN/iXOSvMI33J4kcBBK821R6ZRBJRRUJPuR57l OkCF7rKPb/XwnG93rz2TvmeJZIp6KfoZydVTyAgwDNwUvYapPHYrW8VXz8tqftQS bFKPSmzCsHIe0u8EyLvqyNhj1bqMonwQZB9trzoAbPsMyqr3cdlclAqKWf+IOlB0 ULmWA4hWoAHccnXmi8F0GPwmlC4EWInjdJjVJo1TCNSRyKBbKZaLPjd8/7nlmYhw a2bAl+ARPNaqUOsN0k1DEDA4JIGFn2pvV1EGEIAB8mKefDi1vjNpQ7nrEZlUv2l6 NO1Hu/ZTDh4sEURFyvpVuMrE2j9/xYy/ujEJJOH8EJBZPJX1UvFM9jKuWmW2fJy4 eM+qYlxrvTvSm/cl0eFrPJ9SxRY4HRWcpcdcxVZP0zs8ERX0kUA6h45JYZHDPY3L oBWuHgP77SQ50MJLuO5XS7S55ujKn2xgPjdkonLKhpTIAiACBXAxQH2zYfQtj9oD e8kJGGEOPUJ0iSh0cIl1ie2xTMu6Kh2yEy5oo7lqocFA5NWzB7/3b37TTI73wl9w Eq8hleebs7f1aRcLdViV =JEUW -----END PGP SIGNATURE----- --zjcmjzIkjQU2rmur--