From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm: Add kernel-doc for plane functions Date: Wed, 5 Jun 2013 14:52:29 +0300 Message-ID: <20130605115229.GG5004@intel.com> References: <2290036.G3tXmZusqD@avalon> <1370332715-1131-1-git-send-email-ville.syrjala@linux.intel.com> <2041669.WXR2M19hJY@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <2041669.WXR2M19hJY@avalon> 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: Laurent Pinchart Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, Jun 05, 2013 at 04:13:01AM +0200, Laurent Pinchart wrote: > Hi Ville, > = > Thank you for the patch. > = > On Tuesday 04 June 2013 10:58:35 ville.syrjala@linux.intel.com wrote: > > From: Ville Syrj=E4l=E4 > > = > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/drm_crtc.c | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > = > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index f00ba75..f1f11e1 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -795,6 +795,21 @@ void drm_encoder_cleanup(struct drm_encoder *encod= er) > > } > > EXPORT_SYMBOL(drm_encoder_cleanup); > > = > > +/** > > + * drm_plane_init - Initialise a new plane object > > + * @dev: DRM device > > + * @plane: plane object to init > > + * @possible_crtcs: bitmask of possible CRTCs > > + * @funcs: callbacks for the new plane > > + * @formats: array of supported formats (%DRM_FORMAT_*) > > + * @format_count: number of elements in @formats > > + * @priv: plane is private (hidden from userspace)? > > + * > > + * Inits a new object created as base part of an driver plane object. > = > s/an driver/a driver/ You can blame the guy who wrote the drm_crtc_init() docs :) > = > > + * > > + * RETURNS: > > + * Zero on success, error code on failure. > > + */ > > int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, > > unsigned long possible_crtcs, > > const struct drm_plane_funcs *funcs, > > @@ -843,6 +858,13 @@ int drm_plane_init(struct drm_device *dev, struct > > drm_plane *plane, } > > EXPORT_SYMBOL(drm_plane_init); > > = > > +/** > > + * drm_plane_cleanup - Cleans up the core plane usage. > = > Nitpicking, you could remove the full stop at the end of the line to be = > consistent with the other two kerneldoc blocks. > = > And s/Cleans/Clean/ Same deal here. I'll fix up the originals as well... > = > > + * @plane: plane to cleanup > > + * > > + * Cleanup @plane. Removes from drm modesetting space > > + * does NOT free object, caller does that. > = > As this is documentation, I'd use a more verbose style. > = > This function clean up @plane and removes it from the DRM mode setting co= re. = > Note that the function does *not* free the plane structure itself, this i= s the = > responsibility of the caller. Again just copy-pasted from somewhere. > = > > + */ > > void drm_plane_cleanup(struct drm_plane *plane) > > { > > struct drm_device *dev =3D plane->dev; > > @@ -859,6 +881,15 @@ void drm_plane_cleanup(struct drm_plane *plane) > > } > > EXPORT_SYMBOL(drm_plane_cleanup); > > = > > +/** > > + * drm_plane_force_disable - Forcibly disable a plane > > + * @plane: plane to disable > > + * > > + * Forces the plane to be disabled. > = > This feels a bit unclear to me. In particular, how is "force_disable" = > different from just disabling the plane ? Maybe the function should be re= named = > to drm_plane_disable(), and the documentation updated to mention that the = > function just disables the plane and disassociate with from its frame buf= fer. Normal disable would happen in response to the setplane ioctl w/ NULL fb, whereas this guy is meant more for unsolicited disable. I'm afraid if I call it drm_plane_disable() someone will send a patch to call it from setplane, or people start to call it from drivers' disable_plane hook. -- = Ville Syrj=E4l=E4 Intel OTC