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

[Daniel Vetter <daniel@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
-- 
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