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

Thierry Reding
2015-12-07T11:53:48Z

[-- 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 --]