Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 5:08 PM To: Yang, Libin Cc: Daniel Vetter; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
On Wed, Aug 26, 2015 at 08:29:09AM +0000, Yang, Libin wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 4:18 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
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, intrate);
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.
Could we start another patch session for this task because, as you know, this is a little independent on these patches? What do you think?
Nowadays I want proper docs for new features, and for places where we missed them thus far they need to be backfilled. Also there's some good confusion in the review about how this all works together, so better docs seem called for no matter what to get this in. Instead of just adding all the explanations to commit messages only I figured it's better to do them as kerneldoc.
Yes, I agree and I will add it for the sync_audio_rate function in the next version.
Regards, Libin
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch