[PATCH v2 1/2] drm/gem-fb-helper: Cleanup docs

[Noralf Trønnes]

Hi Laurent,


Den 13.09.2017 04.44, skrev Laurent Pinchart:
> Hi Noralf,
>
> Thank you for the patch.
>
> On Monday, 11 September 2017 19:37:44 EEST Noralf Trønnes wrote:
>> Make the docs read a little better.
>>
>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> ---
>>
>> Changes:
>> Addressed Daniel's comments.
>>
>>   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 25 ++++++++++++++-----------
>> 1 file changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index d54a083..e2ca002
>> 100644
>> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> @@ -27,18 +27,21 @@
>>    * DOC: overview
>>    *
>>    * This library provides helpers for drivers that don't subclass
>> - * &drm_framebuffer and and use &drm_gem_object for their backing storage.
>> + * &drm_framebuffer and use &drm_gem_object for their backing storage.
>>    *
>>    * Drivers without additional needs to validate framebuffers can simply use
>> - * drm_gem_fb_create() and everything is wired up automatically. But all -
>> * parts can be used individually.
>> + * drm_gem_fb_create() and everything is wired up automatically. Other
>> drivers + * can use all parts independently.
>>    */
>>
>>   /**
>>    * drm_gem_fb_get_obj() - Get GEM object for framebuffer
>> - * @fb: The framebuffer
>> + * @fb: framebuffer
>>    * @plane: Which plane
> Nitpicking, you should capitalize all entries or none.
>
>>    *
>> + * No additional reference is taken beyond the one that the &drm_frambuffer
>> + * already holds.
>> + *
>>    * Returns the GEM object for given framebuffer.
>>    */
>>   struct drm_gem_object *drm_gem_fb_get_obj(struct drm_framebuffer *fb,
>> @@ -82,7 +85,7 @@ drm_gem_fb_alloc(struct drm_device *dev,
>>
>>   /**
>>    * drm_gem_fb_destroy - Free GEM backed framebuffer
>> - * @fb: DRM framebuffer
>> + * @fb: framebuffer
>>    *
>>    * Frees a GEM backed framebuffer with its backing buffer(s) and the
>> structure
>> * itself. Drivers can use this as their &drm_framebuffer_funcs->destroy
>> @@ -102,8 +105,8 @@
>> EXPORT_SYMBOL(drm_gem_fb_destroy);
>>
>>   /**
>>    * drm_gem_fb_create_handle - Create handle for GEM backed framebuffer
>> - * @fb: DRM framebuffer
>> - * @file: drm file
>> + * @fb: framebuffer
>> + * @file: DRM file
> I wonder why framebuffer doesn't need a DRM while file does :-)

Yes this is haphazard.
I think my problem is that I haven't been able to pick up a consistent
"signal" from the DRM subsystem when it comes to how documentation
should be written. Code patterns are fairly consistent and looks much
the same including variable names, but documentation is more or less
all over the place.

So I guess I need to come to grips with this, since this isn't the last
time I have to write documentation. I have to make myself some rules
that I can look at next time when all of this is forgotten.

Should description entries be Capitalized?
My gut feeling is that most DRM docs don't do that, but when humans
write for humans we do capatalize the start of sentences. So I guess
that's the natural thing to do.

Should DRM objects start with DRM in the argument doc entries?
'DRM device' is a given since the kernel has many types of devices, but
should we write 'DRM framebuffer' or 'Framebuffer'?

How descriptive should the descriptions be?
Let's take this example:

/**
  * drm_gem_fb_prepare_fb() - Prepare gem framebuffer
  * @plane: Which plane
  * @state: Plane state the fence will be attached to
  *
  * This can be used as the &drm_plane_helper_funcs.prepare_fb hook.
  *
  * This function checks if the plane FB has an dma-buf attached, extracts
  * the exclusive fence and attaches it to plane state for the atomic helper
  * to wait on.

Both the @state description and the body says that the fence will be
attached to the plane state. Would it be better to just say:
  * @state: Plane state

Another way to ask this is:
Should the documentation be terse or should it be repeating itself?

Then we have (copied from the cma library):
  * @plane: Which plane

Which is probably short for: The plane which we are operating/acting on.

More possibilities:
  * @plane: DRM plane
  * @plane: Plane
  * @plane: The plane for which a framebuffer is prepared for scanout

The text for the last one is also available when clicking on the
&drm_plane_helper_funcs.prepare_fb link, so it's repeating something
that is one click away.

I always get comments on my documentation, so it's clearly something I
need to find a way to improve.

Noralf.

>>    * @handle: handle created
>>    *
>>    * Drivers can use this as their &drm_framebuffer_funcs->create_handle
>> @@ -124,7 +127,7 @@ EXPORT_SYMBOL(drm_gem_fb_create_handle);
>>    *                                  &drm_mode_config_funcs.fb_create
>>    *                                  callback
>>    * @dev: DRM device
>> - * @file: drm file for the ioctl call
>> + * @file: DRM file for the ioctl call
>>    * @mode_cmd: metadata from the userspace fb creation request
>>    * @funcs: vtable to be used for the new framebuffer object
>>    *
>> @@ -194,7 +197,7 @@ static const struct drm_framebuffer_funcs
>> drm_gem_fb_funcs = { /**
>>    * drm_gem_fb_create() - &drm_mode_config_funcs.fb_create callback function
>> * @dev: DRM device
>> - * @file: drm file for the ioctl call
>> + * @file: DRM file for the ioctl call
>>    * @mode_cmd: metadata from the userspace fb creation request
>>    *
>>    * If your hardware has special alignment or pitch requirements these
>> should be @@ -214,11 +217,11 @@ EXPORT_SYMBOL_GPL(drm_gem_fb_create);
>>   /**
>>    * drm_gem_fb_prepare_fb() - Prepare gem framebuffer
>>    * @plane: Which plane
>> - * @state: Plane state attach fence to
>> + * @state: Plane state the fence will be attached to
>>    *
>>    * This can be used as the &drm_plane_helper_funcs.prepare_fb hook.
>>    *
>> - * This function checks if the plane FB has an dma-buf attached, extracts
>> + * This function checks if the plane FB has a dma-buf attached, extracts
>>    * the exclusive fence and attaches it to plane state for the atomic helper
>> * to wait on.
>>    *
>> --
>> 2.7.4
>