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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 15, 2017 at 12:45:14AM +0300, Laurent Pinchart wrote:
> Hi Noralf,
> 
> On Wednesday, 13 September 2017 16:41:49 EEST Noralf Trønnes wrote:
> > Den 13.09.2017 04.44, skrev Laurent Pinchart:
> > > 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@ideasonboard.com>
> > >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > >> Reviewed-by: Daniel Vetter <daniel.vetter@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
> 
> [snip]
> 
> > >>  /**
> > >>   * 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.
> 
> That doesn't surprise me too much, as documentation in DRM is pretty recent, 
> and we never agreed to a documentation style.
> 
> > 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.
> 
> Or, as the first person to address this problem, you could set your own rules 
> that everybody else should then follow :-)

Better is to actually switch all cases over. Which yes is a lot more work,
but otherwise it won't converge. Since at least me personally I tend to
just copy the patterns from the same file ...

> > 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'?
> 
> I would only mention DRM when the description could otherwise be 
> misinterpreted, as in DRM device.
> 
> > 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.
> 
> Writing kerneldoc just for the sake of it is mostly pointless. We should write 
> documentation to make it as useful and easy to consume as possible. Keeping 
> that in mind,
> 
>  * @plane: Plane
> 
> isn't very useful. It's clear from the function argument name (and type) that 
> it is a pointer to a plane. I would thus advocate for more detailed parameter 
> descriptions.

There's unfortunately a certain amount of boilerplate in some of the
argument docs. It happens, and as long as the main text explains it all,
I think it's ok to have terse doc like this. If we put nothing, kernel-doc
will complain (and those warnings are kinda nice to spot outdated docs).

Thanks, Daniel

> > I always get comments on my documentation, so it's clearly something I
> > need to find a way to improve.
> 
> I don't think it's specific to your documentation. You're doing a great job, 
> for which I'm personally grateful. The fact that we haven't defined a 
> documentation style in a similar way as we have defined a coding style is an 
> issue of the DRM/KMS community.
> 
> > >>   * @handle: handle created
> > >>   *
> > >>   * Drivers can use this as their &drm_framebuffer_funcs->create_handle
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel