Re: [PATCH 07/12] drm/irq: kerneldoc polish

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, May 14, 2014 at 08:51:09PM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
[...]
> index dd786d84daab..5ff986bd4de4 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1,6 +1,5 @@
> -/**
> - * \file drm_irq.c
> - * IRQ support
> +/*
> + * drm_irq.c IRQ and vblank support

Perhaps drop the filename here? It's completely redundant.

> +/**
> + * drm_vblank_cleanup - cleanup vblank support
> + * @dev: drm device

Technically DRM is an abbreviation and therefore should be all
uppercase.

> +/**
> + * drm_vblank_init - initialize vblank support
> + * @dev: drm_device

s/drm_device/DRM device/?

> + * @num_crtcs: number of crtcs supported by @dev

CRTC is also an abbreviation, so I think this should be s/crtcs/CRTCs/.

> + * Returns:

According to Documentation/kernel-doc-nano-HOWTO.txt this section should
be called "Return:". I'm not sure it matters all that much, since it
seems to be only used as the title of a new section, but I think it
makes sense to follow the recommendation of the documentation.

>  /**
> - * Install IRQ handler.
> - *
> - * \param dev DRM device.
> + * drm_irq_install - install IRQ handler
> + * @dev: drm device
> + * @irq: irq number to install the handler for

I know you're probably going to hate me for this, but: IRQ is an
abbreviation. =)

> -/**
> +/*
>   * IRQ control ioctl.
>   *
>   * \param inode device inode.

Perhaps it would still make sense to change the comment style to
kernel-doc, even if it isn't exported.

>  /**
> - * drm_calc_vbltimestamp_from_scanoutpos - helper routine for kms
> - * drivers. Implements calculation of exact vblank timestamps from
> - * given drm_display_mode timings and current video scanout position
> - * of a crtc. This can be called from within get_vblank_timestamp()
> - * implementation of a kms driver to implement the actual timestamping.
> + * drm_calc_vbltimestamp_from_scanoutpos - precise vblank timestamp helper
> + * @dev: drm device
> + * @crtc: Which crtc's vblank timestamp to retrieve
> + * @max_error: Desired maximum allowable error in timestamps (nanosecs)
> + *             On return contains true maximum error of timestamp
> + * @vblank_time: Pointer to struct timeval which should receive the timestamp
> + * @flags: Flags to pass to driver:
> + *         0 = Default,
> + *         DRM_CALLED_FROM_VBLIRQ = If function is called from vbl irq handler
> + * @refcrtc: drm_crtc* of crtc which defines scanout timing

I think s/drm_crtc * of// would be okay here.

>  /**
>   * drm_vblank_off - disable vblank events on a CRTC
> - * @dev: DRM device
> + * @dev: drm device
>   * @crtc: CRTC in question
> + *
> + * Drivers can use this function to shut down the vblank interrupt handling when
> + * disabling a crtc. This function ensures that the latest vblank frame count is
> + * stored so that drm_vblank_on can restore it again.

drm_vblank_on() to have it correctly highlighted?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel