| Message ID | 1439880714-40931-1-git-send-email-libin.yang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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, 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? Regards, Libin > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
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, 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. -Daniel
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, 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
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); } *ops; };