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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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/

> + *
> + * 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/

> + * @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. 

> + */
>  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.

> + *
> + * Used when the plane's current framebuffer is destroyed,
> + * and when restoring fbdev mode.
> + */
>  void drm_plane_force_disable(struct drm_plane *plane)
>  {
>  	int ret;
-- 
Regards,

Laurent Pinchart