Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754584AbeAKNNK (ORCPT + 1 other); Thu, 11 Jan 2018 08:13:10 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:52141 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751923AbeAKNNI (ORCPT ); Thu, 11 Jan 2018 08:13:08 -0500 Date: Thu, 11 Jan 2018 14:12:56 +0100 From: Maxime Ripard To: Laurent Pinchart Cc: Daniel Vetter , Jani Nikula , Sean Paul , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Mark Brown Subject: Re: [PATCH] drm/panel: lvds: Handle the optional regulator case properly Message-ID: <20180111131256.bmr6qbgzcmyfg4mj@flea.lan> References: <20180110155941.16109-1-maxime.ripard@free-electrons.com> <1944741.zEkzsdPTSS@avalon> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="bjcr3hln6amh3ggw" Content-Disposition: inline In-Reply-To: <1944741.zEkzsdPTSS@avalon> User-Agent: NeoMutt/20171215 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: --bjcr3hln6amh3ggw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Thu, Jan 11, 2018 at 03:05:01PM +0200, Laurent Pinchart wrote: > Hi Maxime, >=20 > (CC'ing Mark Brown) >=20 > Thank you for the patch. >=20 > On Wednesday, 10 January 2018 17:59:41 EET Maxime Ripard wrote: > > The devm_regulator_get_optional function, unlike it was assumed in the > > commit a1c55bccf600 ("drm/panel: lvds: Add support for the power-supply > > property"), is actually returning an error pointer with -ENODEV instead= of > > NULL when there's no regulator to find. > >=20 > > Make sure we handle that case properly. > >=20 > > Fixes: a1c55bccf600 ("drm/panel: lvds: Add support for the power-supply > > property") Signed-off-by: Maxime Ripard > > --- > > drivers/gpu/drm/panel/panel-lvds.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/panel/panel-lvds.c > > b/drivers/gpu/drm/panel/panel-lvds.c index 57e38a9e7ab4..9f46e7095c0e > > 100644 > > --- a/drivers/gpu/drm/panel/panel-lvds.c > > +++ b/drivers/gpu/drm/panel/panel-lvds.c > > @@ -215,8 +215,13 @@ static int panel_lvds_probe(struct platform_device > > *pdev) lvds->supply =3D devm_regulator_get_optional(lvds->dev, "power"); > > if (IS_ERR(lvds->supply)) { > > ret =3D PTR_ERR(lvds->supply); > > - dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > > - return ret; > > + > > + if (ret !=3D -ENODEV) { > > + dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > > + return ret; >=20 > I wouldn't print an error message if ret =3D=3D -EPROBE_DEFER. >=20 > > + } else { > > + lvds->supply =3D NULL; > > + } > > } >=20 > How about >=20 > lvds->supply =3D devm_regulator_get_optional(lvds->dev, "power"); > if (IS_ERR(lvds->supply)) { > ret =3D PTR_ERR(lvds->supply); > if (ret !=3D -ENODEV) { > if (ret =3D=3D -EPROBE_DEFER) I guess that would be !=3D -EPROBE_DEFER > dev_err(lvds->dev, "failed to request regulator: %d\n", ret); > return ret; > } >=20 > lvds->supply =3D NULL; > } Otherwise, it works for me. > My preference, however, would be for devm_regulator_get_optional() to ret= urn=20 > NULL when no regulator is present. The current implementation returns -EN= ODEV=20 > in multiple cases, making it impossible to properly discriminate between= =20 > having no regulator and not being able to get the regulator due to an err= or. It would feel more intuitive to me too, but it would also require to fix most of the call sites that would have a similar pattern. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --bjcr3hln6amh3ggw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEE0VqZU19dR2zEVaqr0rTAlCFNr3QFAlpXYtgACgkQ0rTAlCFN r3TIbg/9E5RKXxMIcntRM5+yCq1+wf37hvRPhHUEBwOm0GdqE1SyrDr+60QWgyj4 lb/IIJwp7whOKqWivLuV4J9hgepLbqUqw29+ianePLGBN6encuZlTfRspmQUvSZb rYRjXeyYUHYyUOuyeRRqwqJP/pmibGGOJuK1lG0h3Y6QAxx4Wm3HJiQ2AtyUl4s7 MLlkh5CIQVuKiZ9bjC0expaPCWTW1hJGPQiLAI4azV/RSO0DAI0sk6MNDRGjuKvL uxbcFw6CUoBe0jG+OUmfo8JoDvSCFeBEY1su+6OKJerun6Z08pEJcIv9m9T4Cmao PmcYNqYUd/lJskdiyEYTrbRHyN5gMe1gTC+jxuhxtzr8uetJUMbwnyn2yIFwR0at REKDPqLxmf8/tChIpRn0oMZlINleZc4jrTUJzdNJI/F5qvb2K0zFR68RIIxNZFIl ihRarZjUFUewivvPJo90JpDooRabEbO+IdxzuAKtZO/Ppr0yUsx5Y0yPWX64GIoe 5oKf76EMdJcTevPUwdWgvSf8LFMe06SwAGhd21Nx+jqkuvzOTPloAAURd5ho7K+2 t0Iww6/jCqZvI79X9qrE5LzjLzx1s4xpPoYIG2wuSB6mxi1a/26nuyeErVbfzqLr xk7s/SrKNzB2RVXUs06ZQq9HkJbjbih951vEh7uP4J175PAz6Q8= =3Cvy -----END PGP SIGNATURE----- --bjcr3hln6amh3ggw--