[v5,1/4] drm/i915: Add audio sync_audio_rate callback
diff mbox

Message ID 1439880714-40931-1-git-send-email-libin.yang@intel.com
State New
Headers show

Commit Message

Yang, Libin Aug. 18, 2015, 6:51 a.m. UTC
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(+)

Comments

Daniel Vetter Aug. 26, 2015, 8:17 a.m. UTC | #1
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
Yang, Libin Aug. 26, 2015, 8:29 a.m. UTC | #2
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
Daniel Vetter Aug. 26, 2015, 9:08 a.m. UTC | #3
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
Yang, Libin Aug. 26, 2015, 11:08 a.m. UTC | #4
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

Patch
diff mbox

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;
 };