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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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.

After a second thought, set_ncts is not very good, we should
consider DP's naming and handling DP mode together.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux