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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: Yang, Libin
>> Sent: Tuesday, August 11, 2015 10:41 AM
>> To: Jani Nikula; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
>> Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
>> callback
>> 
>> Hi Jani,
>> 
>> Thanks for reviewing, please see my comments
>> 
>> > -----Original Message-----
>> > From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> > Sent: Monday, August 10, 2015 7:46 PM
>> > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
>> > Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts
>> > callback
>> >
>> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> > > From: Libin Yang <libin.yang@intel.com>
>> > >
>> > > Add the set_ncts callback.
>> > >
>> > > With the callback, audio driver can trigger
>> > > i915 driver to set the proper N/CTS
>> > > based on different sample rates.
>> > >
>> > > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> > > ---
>> > >  include/drm/i915_component.h | 2 ++
>> > >  1 file changed, 2 insertions(+)
>> > >
>> > > diff --git a/include/drm/i915_component.h
>> > b/include/drm/i915_component.h
>> > > index c9a8b64..7305881 100644
>> > > --- a/include/drm/i915_component.h
>> > > +++ b/include/drm/i915_component.h
>> > > @@ -33,6 +33,8 @@ struct i915_audio_component {
>> > >  		void (*put_power)(struct device *);
>> > >  		void (*codec_wake_override)(struct device *, bool
>> > enable);
>> > >  		int (*get_cdclk_freq)(struct device *);
>> > > +		int (*set_ncts)(struct device *, int port, int dev_entry,
>> > > +					int rate);
>> >
>> > I'd rather call this set_audio_rate or similar, and dropping the
>> > references to N and CTS. The caller does not need to know.
>> 
>> But it seems the set_audio_rate will confuse the user. From the
>> literal meaning, it is setting the rate of audio, such as sample rate,
>> which will make audio driver developers confused.
>> And the function is just setting N/CTS which is defined from
>> HDMI SPEC.
>
> What about the name sync_audio_rate()?

I'm fine with that.

BR,
Jani.

>
>> 
>> BTW: your comment reminds me that get_cdclk_freq() seems
>> to need change the name as cdclk is not in the open spec.
>> 
>> >
>> > I'm also not fond of adding a dev_entry parameter and leaving it
>> > unused. I do not think we know specifically how we're going to
>> identify
>> > MST sinks, and the interface may need to be changed anyway. Let's
>> > force
>> > an update in the caller side as well when there's actually sensible
>> > support in our side.
>> 
>> The device entry is a concept in HDA spec. For driver implementation,
>> I think we can use an int variable or a struct device to represent it.
>> A int variable is an easy way. There is some place in audio driver using
>> int variable to represent the deivce entry. And audio driver will not
>> care how gfx driver supports the DP1.2 MST, I mean audio driver will
>> not know the structures inside gfx driver.
>> 
>> I will remove this parameter in this version if you are thinking the
>> MST interface is indeterminate.
>> 
>> BTW: do you know when gfx will normally support MST?
>> 
>> Best Regards,
>> Libin
>> 
>> >
>> > BR,
>> > Jani.
>> >
>> > >  	} *ops;
>> > >  };
>> > >
>> > > --
>> > > 1.9.1
>> > >
>> > > _______________________________________________
>> > > Intel-gfx mailing list
>> > > Intel-gfx@lists.freedesktop.org
>> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> > --
>> > Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center