Re: [PATCH 06/28] drm/bridge: Improve kerneldoc

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

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

On Fri, Dec 04, 2015 at 09:45:47AM +0100, Daniel Vetter wrote:
> Especially document the assumptions and semantics of the callbacks
> carefully. Just a warm-up excercise really.
> 
> v2: Spelling fixes (Eric).
> 
> v3: Consolidate more with existing docs:
> 
> - Remove the overview section explaining the bridge funcs, that's
>   now all in the drm_bridge_funcs kerneldoc in much more detail.
> 
> - Use & to reference structs so that kerneldoc automatically inserts
>   hyperlinks.
> 
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Archit Taneja <architt@codeaurora.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_bridge.c |  69 +++++++++++------------------
>  include/drm/drm_crtc.h       | 102 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 122 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 6b8f7211e543..26dd1cfb6078 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -31,14 +31,14 @@
>  /**
>   * DOC: overview
>   *
> - * drm_bridge represents a device that hangs on to an encoder. These are handy
> - * when a regular drm_encoder entity isn't enough to represent the entire
> + * struct &drm_bridge represents a device that hangs on to an encoder. These are
> + * handy when a regular &drm_encoder entity isn't enough to represent the entire
>   * encoder chain.
>   *
> - * A bridge is always associated to a single drm_encoder at a time, but can be
> + * A bridge is always associated to a single &drm_encoder at a time, but can be

I think I've seen either "associated with" or "attached to", but the
above sounds strange to me. Anyway, it's nothing you've introduced in
this patch, so might as well stay the way it is.

>   * either connected to it directly, or through an intermediate bridge:
>   *
> - * encoder ---> bridge B ---> bridge A
> + *     encoder ---> bridge B ---> bridge A
>   *
>   * Here, the output of the encoder feeds to bridge B, and that furthers feeds to
>   * bridge A.
> @@ -46,11 +46,16 @@
>   * The driver using the bridge is responsible to make the associations between
>   * the encoder and bridges. Once these links are made, the bridges will
>   * participate along with encoder functions to perform mode_set/enable/disable
> - * through the ops provided in drm_bridge_funcs.
> + * through the ops provided in &drm_bridge_funcs.
>   *
>   * drm_bridge, like drm_panel, aren't drm_mode_object entities like planes,
> - * crtcs, encoders or connectors. They just provide additional hooks to get the
> - * desired output at the end of the encoder chain.
> + * crtcs, encoders or connectors and hence are not visible to userspace. They

s/crtcs/CRTCs/

> @@ -122,34 +127,12 @@ EXPORT_SYMBOL(drm_bridge_attach);
>  /**
>   * DOC: bridge callbacks
>   *
> - * The drm_bridge_funcs ops are populated by the bridge driver. The drm
> - * internals(atomic and crtc helpers) use the helpers defined in drm_bridge.c
> - * These helpers call a specific drm_bridge_funcs op for all the bridges
> + * The &drm_bridge_funcs ops are populated by the bridge driver. The drm

s/drm/DRM/

> + * internals (atomic and crtc helpers) use the helpers defined in drm_bridge.c

s/crtc/CRTC/

>  /**
> @@ -159,7 +142,7 @@ EXPORT_SYMBOL(drm_bridge_attach);
>   * @mode: desired mode to be set for the bridge
>   * @adjusted_mode: updated mode that works for this bridge
>   *
> - * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the
> + * Calls 'mode_fixup' &drm_bridge_funcs op for all the bridges in the

"->mode_fixup()"?

> @@ -186,11 +169,11 @@ bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>  
>  /**
> - * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all
> + * drm_bridge_disable - calls 'disable' &drm_bridge_funcs op for all
>   *			bridges in the encoder chain.
>   * @bridge: bridge control structure
>   *
> - * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder
> + * Calls 'disable' &drm_bridge_funcs op for all the bridges in the encoder

"->disable()"? Perhaps not worth it because there's a bunch of these in
this file.

>  struct drm_bridge_funcs {
>  	int (*attach)(struct drm_bridge *bridge);
> +
> +	/**
> +	 * @mode_fixup:
> +	 *
> +	 * This callback is used to validate and adjust a mode. The paramater
> +	 * mode is the display mode that should be fed to the next element in
> +	 * the display chain, either the final &drm_connector or the next
> +	 * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
> +	 * requires. It can be modified by this callback and does not need to
> +	 * match mode.
> +	 *
> +	 * This is the only hook that allows a bridge to reject a modeset. If
> +	 * this function passes all other callbacks must succeed for this
> +	 * configuration.
> +	 *
> +	 * NOTE:
> +	 *
> +	 * This function is called in the check phase of atomic modesets, which
> +	 * can be aborted for any reason (including on userspace's request to
> +	 * just check whether a configuration would be possible). Drivers MUST
> +	 * NOT touch any persistent state (hardware or software) or data
> +	 * structures except the passed in adjusted_mode parameter.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * True if an acceptable configuration is possible, false if the modeset
> +	 * operation should be rejected.
> +	 */
>  	bool (*mode_fixup)(struct drm_bridge *bridge,
>  			   const struct drm_display_mode *mode,
>  			   struct drm_display_mode *adjusted_mode);
> +	/**
> +	 * @disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right before
> +	 * the preceding element in the display pipe is disabled. If the
> +	 * preceding element is a bridge this means it's called before that
> +	 * bridge's ->disaable function. If the preceding element is a

s/->disaable/->disable()/?

> +	 * &drm_encoder it's called right before the encoder's ->disable,
> +	 * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs.

->disable(), ->prepare() or ->dpms()?

> +	 *
> +	 * The bridge can assume that the display pipe (i.e. clocks and timing
> +	 * signals) feeding it is still running when this callback is called.
> +	 */
>  	void (*disable)(struct drm_bridge *bridge);
> +
> +	/**
> +	 * @post_disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right after
> +	 * the preceding element in the display pipe is disabled. If the
> +	 * preceding element is a bridge this means it's called after that
> +	 * bridge's ->post_disaable function. If the preceding element is a
> +	 * &drm_encoder it's called right after the encoder's ->disable,
> +	 * ->prepare or ->dpms hook from struct &drm_encoder_helper_funcs.

Same comments as for ->disable(). Perhaps this should also say what the
difference is to ->disable()? But maybe that's more suitable for a
follow-up patch.

> +	 *
> +	 * The bridge must assume that the display pipe (i.e. clocks and timing
> +	 * singals) feeding it is no longer running when this callback is

"signals". I guess this is the difference. Perhaps mention in the above
paragraph that ->post_disable() is called after ->disable(), though that
much should be obvious.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 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