Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Aug 18, 2015 at 02:51:51PM +0800, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
> 
> Add the sync_audio_rate callback.
> 
> With the callback, audio driver can trigger
> i915 driver to set the proper N/CTS or N/M
> based on different sample rates.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  include/drm/i915_component.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..aabebcb 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -33,6 +33,7 @@ struct i915_audio_component {
>  		void (*put_power)(struct device *);
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
> +		int (*sync_audio_rate)(struct device *, int port, int rate);

We're missing kerneldoc for this entire struct here, which isn't great.
This needs to be fixed. Please
- pull out the ops structure so it's not inlined (kerneldoc can't handle
  nested ops structures).
- please document all the callbacks with kerneldoc. In 4.3 we can have
  kerneldoc in-line in structures right above each member like this

  /**
   * @put_power:
   *
   * Longer text explaining this hook.
   */
  void (*put_power)(struct device *);

  For each hook please explain at least who calls it (gfx or audio) and
  where exactly it's called in the overall flow.
- Extended the overview doc section with references to the component/ops
  structure would be needed too.

And please make sure it all looks pretty with make htmldocs.

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