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

2015-08-26 Thread Yang, Libin 
Hi Daniel,

 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
 Daniel Vetter
 Sent: Wednesday, August 26, 2015 4:18 PM
 To: Yang, Libin
 Cc: alsa-de...@alsa-project.org; ti...@suse.de; intel-
 g...@lists.freedesktop.org; daniel.vet...@ffwll.ch;
 jani.nik...@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.y...@intel.com
 wrote:
  From: Libin Yang libin.y...@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.y...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

2015-08-26 Thread Daniel Vetter 
On Tue, Aug 18, 2015 at 02:51:51PM +0800, libin.y...@intel.com wrote:
 From: Libin Yang libin.y...@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.y...@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
-- 
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

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

2015-08-26 Thread Daniel Vetter 
On Wed, Aug 26, 2015 at 08:29:09AM +, Yang, Libin wrote:
 Hi Daniel,
 
  -Original Message-
  From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
  Daniel Vetter
  Sent: Wednesday, August 26, 2015 4:18 PM
  To: Yang, Libin
  Cc: alsa-de...@alsa-project.org; ti...@suse.de; intel-
  g...@lists.freedesktop.org; daniel.vet...@ffwll.ch;
  jani.nik...@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.y...@intel.com
  wrote:
   From: Libin Yang libin.y...@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.y...@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

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

2015-08-26 Thread Yang, Libin 
Hi Daniel,

 -Original Message-
 From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
 Daniel Vetter
 Sent: Wednesday, August 26, 2015 5:08 PM
 To: Yang, Libin
 Cc: Daniel Vetter; alsa-de...@alsa-project.org; ti...@suse.de; intel-
 g...@lists.freedesktop.org; daniel.vet...@ffwll.ch;
 jani.nik...@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 +, Yang, Libin wrote:
  Hi Daniel,
 
   -Original Message-
   From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of
   Daniel Vetter
   Sent: Wednesday, August 26, 2015 4:18 PM
   To: Yang, Libin
   Cc: alsa-de...@alsa-project.org; ti...@suse.de; intel-
   g...@lists.freedesktop.org; daniel.vet...@ffwll.ch;
   jani.nik...@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.y...@intel.com
   wrote:
From: Libin Yang libin.y...@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.y...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback

2015-08-18 Thread libin . yang 
From: Libin Yang libin.y...@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.y...@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);
} *ops;
 };
 
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx