From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sascha Hauer Subject: Re: [PATCH 14/20] drm: add convenience function to create an enum property Date: Sat, 4 Feb 2012 00:40:13 +0100 Message-ID: <20120203234013.GG1990@pengutronix.de> References: <989b3fff-c66a-410f-9c3f-fdbc911b2e73@zmail16.collab.prod.int.phx2.redhat.com> <20120201130538.GS1990@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: quoted-printable Return-path: Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [92.198.50.35]) by gabe.freedesktop.org (Postfix) with ESMTP id D8B2C9E768 for ; Fri, 3 Feb 2012 15:40:18 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Dave Airlie Cc: David Airlie , kernel@pengutronix.de, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Feb 03, 2012 at 10:08:12AM +0000, Dave Airlie wrote: > On Wed, Feb 1, 2012 at 1:05 PM, Sascha Hauer wro= te: > > On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote: > >> > >> > >> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> > > index 8d593ad..cdbbb40 100644 > >> > > --- a/include/drm/drm_crtc.h > >> > > +++ b/include/drm/drm_crtc.h > >> > > @@ -394,7 +394,7 @@ struct drm_crtc { > >> > > =A0 s64 framedur_ns, linedur_ns, pixeldur_ns; > >> > > > >> > > =A0 /* if you are using the helper */ > >> > > - void *helper_private; > >> > > + struct drm_crtc_helper_funcs *helper_private; > >> > > =A0}; > >> > > > >> > > > >> > > @@ -481,7 +481,7 @@ struct drm_encoder { > >> > > > >> > > =A0 struct drm_crtc *crtc; > >> > > =A0 const struct drm_encoder_funcs *funcs; > >> > > - void *helper_private; > >> > > + struct drm_encoder_helper_funcs *helper_private; > >> > > =A0}; > >> > > > >> > > =A0enum drm_connector_force { > >> > > @@ -573,7 +573,7 @@ struct drm_connector { > >> > > =A0 /* requested DPMS state */ > >> > > =A0 int dpms; > >> > > > >> > > - void *helper_private; > >> > > + struct drm_connector_helper_funcs *helper_private; > >> > > > >> > > =A0 /* forced on connector */ > >> > > =A0 enum drm_connector_force force; > >> > > >> > This is a separate chunk. > >> > >> And totally wrong, using the helper should remain optional, and the > >> helper includes should not be included into the main headers. > > > > The intention was to prevent people from thinking that they > > should invent their own set of helper functions. As these > > are only pointers we could also add a > > > > struct drm_crtc_helper_funcs; > > struct drm_connector_helper_funcs; > > struct drm_encoder_helper_funcs; > > > > to drm_crtc.h without having to include the helper includes. > > > = > This might be better, though driver can invent their own helpers if > they need it, its the whole point of using helpers and not forcing > drivers to do stuff one single way. I hope to get the drivers more uniform. When people want to invent their own helpers I think the question we have to ask is what is wrong with the helpers and what is missing and try to fix/implement this before adding a new set of helpers. If someone really has a good reason to implement new helpers we can easily revert this change, but we shouldn't motivate him to do so. Sascha -- = Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |