Hi, > -----Original Message----- > From: Yang, Libin > Sent: Tuesday, August 11, 2015 10:41 AM > To: Jani Nikula; alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx > Subject: RE: [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@xxxxxxxxxxxxxxx] > > Sent: Monday, August 10, 2015 7:46 PM > > To: Yang, Libin; alsa-devel@xxxxxxxxxxxxxxxx; tiwai@xxxxxxx; intel- > > gfx@xxxxxxxxxxxxxxxxxxxxx; daniel.vetter@xxxxxxxx > > Subject: Re: [PATCH v2 1/4] drm/i915: Add audio set_ncts > > callback > > > > On Mon, 10 Aug 2015, libin.yang@xxxxxxxxx wrote: > > > From: Libin Yang <libin.yang@xxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > --- > > > 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()? > > 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@xxxxxxxxxxxxxxxxxxxxx > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
