Hi Daniel, > -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > Daniel Vetter > Sent: Wednesday, August 26, 2015 5:08 PM > To: Yang, Libin > Cc: Daniel Vetter; alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > jani.nikula@xxxxxxxxxxxxxxx > 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@xxxxxxxx] On Behalf Of > > > Daniel Vetter > > > Sent: Wednesday, August 26, 2015 4:18 PM > > > To: Yang, Libin > > > Cc: alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > > > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx; > > > jani.nikula@xxxxxxxxxxxxxxx > > > 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@xxxxxxxxx > > > wrote: > > > > From: Libin Yang <libin.yang@xxxxxxxxx> > > > > > > > > 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@xxxxxxxxx> > > > > --- > > > > 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. > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
