Re: [PATCH] drm: Add kernel-doc for plane functions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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älä <ville.syrjala@linux.intel.com>
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  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 *encoder)
> >  }
> >  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 core. 
> Note that the function does *not* free the plane structure itself, this is the 
> responsibility of the caller.

Again just copy-pasted from somewhere.

> 
> > + */
> >  void drm_plane_cleanup(struct drm_plane *plane)
> >  {
> >  	struct drm_device *dev = 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 renamed 
> to drm_plane_disable(), and the documentation updated to mention that the 
> function just disables the plane and disassociate with from its frame buffer.

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älä
Intel OTC