[alsa-devel] [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback

[Yang, Libin]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 26, 2015 5:08 PM
> To: Yang, Libin
> Cc: Daniel Vetter; alsa-devel at alsa-project.org; tiwai at suse.de; intel-
> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch;
> jani.nikula at 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 at ffwll.ch] On Behalf Of
> > > Daniel Vetter
> > > Sent: Wednesday, August 26, 2015 4:18 PM
> > > To: Yang, Libin
> > > Cc: alsa-devel at alsa-project.org; tiwai at suse.de; intel-
> > > gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch;
> > > jani.nikula at 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 at intel.com
> > > wrote:
> > > > From: Libin Yang <libin.yang at 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 at 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