Re: [PATCH 01/28] drm: Polish fbdev helper struct docs

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 01/28] drm: Polish fbdev helper struct docs
Date: Mon, 7 Dec 2015 12:53:48 +0100	[thread overview]
Message-ID: <20151207115348.GQ13177@ulmo> (raw)
In-Reply-To: <20151207115024.GD10243@phenom.ffwll.local>

[-- Attachment #1.1: Type: text/plain, Size: 3814 bytes --]

On Mon, Dec 07, 2015 at 12:50:24PM +0100, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 11:45:22AM +0100, Thierry Reding wrote:
> > On Fri, Dec 04, 2015 at 09:45:42AM +0100, Daniel Vetter wrote:
> > > Mostly this is just adding extensive docs for the callbacks, but also
> > > a few other additions.
> > > 
> > > v2: Use FIXME comments to annotate helper hooks that should be
> > > replaced.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  include/drm/drm_fb_helper.h | 92 ++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 79 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> > > index 87b090c4b730..5ce033e36039 100644
> > > --- a/include/drm/drm_fb_helper.h
> > > +++ b/include/drm/drm_fb_helper.h
> > > @@ -74,25 +74,76 @@ struct drm_fb_helper_surface_size {
> > >  
> > >  /**
> > >   * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation library
> > > - * @gamma_set: Set the given gamma lut register on the given crtc.
> > > - * @gamma_get: Read the given gamma lut register on the given crtc, used to
> > > - *             save the current lut when force-restoring the fbdev for e.g.
> > > - *             kdbg.
> > > - * @fb_probe: Driver callback to allocate and initialize the fbdev info
> > > - *            structure. Furthermore it also needs to allocate the drm
> > > - *            framebuffer used to back the fbdev.
> > > - * @initial_config: Setup an initial fbdev display configuration
> > >   *
> > >   * Driver callbacks used by the fbdev emulation helper library.
> > >   */
> > >  struct drm_fb_helper_funcs {
> > > +	/**
> > > +	 * @gamma_set:
> > > +	 *
> > > +	 * Set the given gamma lut register on the given crtc.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 *
> > > +	 * FIXME:
> > > +	 *
> > > +	 * This callback is functionally redundant with the core gamma table
> > > +	 * support and simply exists because the fbdev hasn't yet been
> > > +	 * refactored to use the core gamma table interfaces.
> > > +	 */
> > >  	void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
> > >  			  u16 blue, int regno);
> > 
> > Pardon my ignorance, but is there a way to document parameters with this
> > new syntax?
> 
> Unfortunately not. Doing that would be quite a bit more rework of the
> entire kerneldoc toolchain I think.

Yes, that was my suspicion as well.

> > > +	/**
> > > +	 * @gamma_get:
> > > +	 *
> > > +	 * Read the given gamma lut register on the given crtc, used to save the
> > > +	 * current lut when force-restoring the fbdev for e.g. kdbg.
> > > +	 *
> > > +	 * This callback is optional.
> > > +	 *
> > > +	 * FIXME:
> > > +	 *
> > > +	 * This callback is functionally redundant with the core gamma table
> > > +	 * support and simply exists because the fbdev hasn't yet been
> > > +	 * refactored to use the core gamma table interfaces.
> > > +	 */
> > >  	void (*gamma_get)(struct drm_crtc *crtc, u16 *red, u16 *green,
> > >  			  u16 *blue, int regno);
> > 
> > Nit: While at it, perhaps (here and in the @gamma_set documentation):
> > s/lut/LUT/ and s/crtc/CRTC/?
> 
> Yeah I thought about our inconsistency wrt upper-case of abbrevations in
> the docs. I think we should do this as a trivial patch thing for newbies.

Fair enough.

Thierry

> > >   * @fb:  Scanout framebuffer object
> > >   * @dev:  DRM device
> > 
> > There seems to be an extra space between the : and the description. That
> > was already there, but maybe worth a follow-up.
> 
> I think fix that up while applying, same for the others.

Okay, either way, this is a good improvement, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx