From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 1/2] drm: Add few more wrapper functions for drm panel Date: Wed, 2 Mar 2016 16:25:49 +0100 Message-ID: <20160302152549.GA21035@ulmo.nvidia.com> References: <87d1rfu344.fsf@intel.com> <1456923511-6251-1-git-send-email-m.deepak@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0829155808==" Return-path: Received: from mail-pa0-x232.google.com (mail-pa0-x232.google.com [IPv6:2607:f8b0:400e:c03::232]) by gabe.freedesktop.org (Postfix) with ESMTPS id 25A936E8BD for ; Wed, 2 Mar 2016 15:25:54 +0000 (UTC) Received: by mail-pa0-x232.google.com with SMTP id fl4so134563199pad.0 for ; Wed, 02 Mar 2016 07:25:54 -0800 (PST) In-Reply-To: <1456923511-6251-1-git-send-email-m.deepak@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Deepak M Cc: dri-devel@lists.freedesktop.org, Jani Nikula , Daniel Vetter , Gaurav K Singh List-Id: dri-devel@lists.freedesktop.org --===============0829155808== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SLDf9lqlvOQaIe6s" Content-Disposition: inline --SLDf9lqlvOQaIe6s Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 02, 2016 at 06:28:31PM +0530, Deepak M wrote: > Currently there are few pair of functions which > are called during the panel enable/disable sequence. > To improve the granularity, adding few more wrapper > functions so that the functions are more specific > on what they are doing. >=20 > Cc: Thierry Reding > Cc: David Airlie > Cc: Ville Syrj=C3=A4l=C3=A4 > Cc: Daniel Vetter > Cc: Jani Nikula > Signed-off-by: Deepak M > Signed-off-by: Gaurav K Singh > --- > include/drm/drm_panel.h | 92 +++++++++++++++++++++++++++++++++++++++++--= ------ > 1 file changed, 77 insertions(+), 15 deletions(-) Sorry, but no. You're not giving any rationale for why you think the granularity needs to be increased. The documentation already states that panel drivers can use ->enable() and ->disable() to turn on and off the backlight, why do you need the extra callbacks? The same is true for the ->prepare() and ->unprepare() callbacks, which are described to perform what your new ->power_on() and ->power_off() callbacks would do. Increasing the granularity isn't always a good thing. It means that drivers can now call the functions in many more variations. If you think you really need finer granularity, you need to do a better job of explaining why. Also, Cc'ing me on the second patch, which I do not have in any of my inboxes, and which I assume contains a usage example of these new callbacks, might help me understand the need for this. One more comment below... > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h [...] > struct drm_panel_funcs { > int (*disable)(struct drm_panel *panel); > @@ -73,6 +90,10 @@ struct drm_panel_funcs { > int (*get_modes)(struct drm_panel *panel); > int (*get_timings)(struct drm_panel *panel, unsigned int num_timings, > struct display_timing *timings); > + int (*power_on)(struct drm_panel *panel); > + int (*power_off)(struct drm_panel *panel); > + int (*backlight_on)(struct drm_panel *panel); > + int (*backlight_off)(struct drm_panel *panel); > }; [...] > +static inline int drm_panel_get_info(struct drm_panel *panel, > + struct drm_connector *connector) > +{ > + if (connector && panel && panel->funcs && panel->funcs->get_info) > + return panel->funcs->get_info(panel, connector); > + > + return panel ? -ENOSYS : -EINVAL; > +} This callback no longer exists, so this patch is going to break compilation. Thierry --SLDf9lqlvOQaIe6s Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJW1wX6AAoJEN0jrNd/PrOhopMP/At5hFlmJqCiQTkTu8FGoSyz fInwNe4mPMt9EL80vgtLGV2ET0G6HC+vduVSNnD9UEFfloMpbnY9bctEY/YO44o7 OrL9WYON1ktGs7Itj50XZf+aSPu07A4D5PvhmUBnsEKYnwJfMzBnXZcwZXUQz6d/ ngN8Ww0QwL0BEHfx5LLQVMyLErdQDwrGHoScAKD1AtbaqlT43jbC8aFAPlAZMT5B 7QvvkYgmZRO97HOi8wMWiyk3qTFgQ2qsLBrjzt/CQd2tHFqfSg6aPaVG7OQMVBOw E8t1nkXke4HaxaZPrq0ATUWwLHBL7X/lC5MoIzbC/exoodl1suOBxkmuhbuOnXgH kBTtrhve6qNfRgKOsWCMmYsRUirsc75z/Qk8xnUwH5WBFIFMY8GVLEM201qfR9gR bAwqv0ZJ7Qh+JAkUzE8pmkQlub6+JaNKjkyi2Bao4CS1hT5dSFyJ/aum4OdIPCKb 8B2ZnZqtNUSZOe/BLUYa6BVqXEDA2pgu+kt9v7Uj5jz/9y0WQ9MuBRZRm7g147nu B0E5OyIIGnhI7k0gPrIOfPJ/lJepRt+hxgF8C/ErzYGFa6gx+pXQ2bq3FUR1Vvig /7VK5rkfz7A1AWSnslBUK4ErH9A5+ppbVVj+KLHeqim7xmghvvTOTNjCxV3KMkys F28nbWgeWxkLo3IdGFhZ =BvJH -----END PGP SIGNATURE----- --SLDf9lqlvOQaIe6s-- --===============0829155808== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0829155808==--