diff mbox

ALSA: hda/hdmi - apply Valleyview fix-ups to Cherryview display codec

Message ID 1408351447-132394-1-git-send-email-libin.yang@intel.com (mailing list archive)
State Accepted
Delegated to: Takashi Iwai
Headers show

Commit Message

Yang, Libin Aug. 18, 2014, 8:44 a.m. UTC 
From: Libin Yang <libin.yang@intel.com>

Valleyview and Cherryview have the same behavior on display audio. So this patch
defines is_valleyview_plus() to include codecs for both Valleyview and its successor
Cherryview, and apply Valleyview fix-ups to Cherryview.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 sound/pci/hda/patch_hdmi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Takashi Iwai Aug. 18, 2014, 9:04 a.m. UTC | #1 
At Mon, 18 Aug 2014 16:44:07 +0800,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> Valleyview and Cherryview have the same behavior on display audio. So this patch
> defines is_valleyview_plus() to include codecs for both Valleyview and its successor
> Cherryview, and apply Valleyview fix-ups to Cherryview.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  sound/pci/hda/patch_hdmi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 36badba..99d7d7f 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -50,6 +50,8 @@ MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
>  #define is_haswell_plus(codec) (is_haswell(codec) || is_broadwell(codec))
>  
>  #define is_valleyview(codec) ((codec)->vendor_id == 0x80862882)
> +#define is_cherryview(codec) ((codec)->vendor_id == 0x80862883)
> +#define is_valleyview_plus(codec) (is_valleyview(codec) || is_cherryview(codec))
>  
>  struct hdmi_spec_per_cvt {
>  	hda_nid_t cvt_nid;
> @@ -1459,7 +1461,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>  			    mux_idx);
>  
>  	/* configure unused pins to choose other converters */
> -	if (is_haswell_plus(codec) || is_valleyview(codec))
> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
>  		intel_not_share_assigned_cvt(codec, per_pin->pin_nid, mux_idx);
>  
>  	snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid);
> @@ -1598,7 +1600,8 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>  		 *   and this can make HW reset converter selection on a pin.
>  		 */
>  		if (eld->eld_valid && !old_eld_valid && per_pin->setup) {
> -			if (is_haswell_plus(codec) || is_valleyview(codec)) {
> +			if (is_haswell_plus(codec) ||
> +				is_valleyview_plus(codec)) {
>  				intel_verify_pin_cvt_connect(codec, per_pin);
>  				intel_not_share_assigned_cvt(codec, pin_nid,
>  							per_pin->mux_idx);
> @@ -1779,7 +1782,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  	bool non_pcm;
>  	int pinctl;
>  
> -	if (is_haswell_plus(codec) || is_valleyview(codec)) {
> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
>  		/* Verify pin:cvt selections to avoid silent audio after S3.
>  		 * After S3, the audio driver restores pin:cvt selections
>  		 * but this can happen before gfx is ready and such selection
> @@ -2330,9 +2333,8 @@ static int patch_generic_hdmi(struct hda_codec *codec)
>  		intel_haswell_fixup_enable_dp12(codec);
>  	}
>  
> -	if (is_haswell(codec) || is_valleyview(codec)) {
> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))

This hunk changes is_haswell() to is_haswell_plus() without
explanation.  If it's really needed, split the patch, one to fix
is_haswell() call here, and another to replace is_valleyview() with
is_valleyview_plus().

Fixing some bug is good, but silently fixing something else isn't :)


thanks,

Takashi


>  		codec->depop_delay = 0;
> -	}
>  
>  	if (hdmi_parse_codec(codec) < 0) {
>  		codec->spec = NULL;
> -- 
> 1.9.1
>
Yang, Libin Aug. 19, 2014, 12:17 a.m. UTC | #2 
Hi Takashi,

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, August 18, 2014 5:04 PM
> To: Yang, Libin
> Cc: alsa-devel@alsa-project.org; Lin, Mengdong
> Subject: Re: [PATCH] ALSA: hda/hdmi - apply Valleyview fix-ups to
> Cherryview display codec
> 
> At Mon, 18 Aug 2014 16:44:07 +0800,
> libin.yang@intel.com wrote:
> >
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Valleyview and Cherryview have the same behavior on display audio. So
> > this patch defines is_valleyview_plus() to include codecs for both
> > Valleyview and its successor Cherryview, and apply Valleyview fix-ups to
> Cherryview.
> >
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  sound/pci/hda/patch_hdmi.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index 36badba..99d7d7f 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -50,6 +50,8 @@ MODULE_PARM_DESC(static_hdmi_pcm, "Don't
> restrict
> > PCM parameters per ELD info");  #define is_haswell_plus(codec)
> > (is_haswell(codec) || is_broadwell(codec))
> >
> >  #define is_valleyview(codec) ((codec)->vendor_id == 0x80862882)
> > +#define is_cherryview(codec) ((codec)->vendor_id == 0x80862883)
> > +#define is_valleyview_plus(codec) (is_valleyview(codec) ||
> > +is_cherryview(codec))
> >
> >  struct hdmi_spec_per_cvt {
> >  	hda_nid_t cvt_nid;
> > @@ -1459,7 +1461,7 @@ static int hdmi_pcm_open(struct
> hda_pcm_stream *hinfo,
> >  			    mux_idx);
> >
> >  	/* configure unused pins to choose other converters */
> > -	if (is_haswell_plus(codec) || is_valleyview(codec))
> > +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> >  		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
> mux_idx);
> >
> >  	snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); @@
> > -1598,7 +1600,8 @@ static bool hdmi_present_sense(struct
> hdmi_spec_per_pin *per_pin, int repoll)
> >  		 *   and this can make HW reset converter selection on a pin.
> >  		 */
> >  		if (eld->eld_valid && !old_eld_valid && per_pin->setup) {
> > -			if (is_haswell_plus(codec) || is_valleyview(codec)) {
> > +			if (is_haswell_plus(codec) ||
> > +				is_valleyview_plus(codec)) {
> >  				intel_verify_pin_cvt_connect(codec,
> per_pin);
> >  				intel_not_share_assigned_cvt(codec,
> pin_nid,
> >  							per_pin->mux_idx);
> > @@ -1779,7 +1782,7 @@ static int
> generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >  	bool non_pcm;
> >  	int pinctl;
> >
> > -	if (is_haswell_plus(codec) || is_valleyview(codec)) {
> > +	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
> >  		/* Verify pin:cvt selections to avoid silent audio after S3.
> >  		 * After S3, the audio driver restores pin:cvt selections
> >  		 * but this can happen before gfx is ready and such selection
> @@
> > -2330,9 +2333,8 @@ static int patch_generic_hdmi(struct hda_codec
> *codec)
> >  		intel_haswell_fixup_enable_dp12(codec);
> >  	}
> >
> > -	if (is_haswell(codec) || is_valleyview(codec)) {
> > +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> 
> This hunk changes is_haswell() to is_haswell_plus() without explanation.  If
> it's really needed, split the patch, one to fix
> is_haswell() call here, and another to replace is_valleyview() with
> is_valleyview_plus().
> 
> Fixing some bug is good, but silently fixing something else isn't :)

OK, I will split it into 2 patches. :)

> 
> 
> thanks,
> 
> Takashi
> 
> 
> >  		codec->depop_delay = 0;
> > -	}
> >
> >  	if (hdmi_parse_codec(codec) < 0) {
> >  		codec->spec = NULL;
> > --
> > 1.9.1
> >
diff mbox

Patch

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 36badba..99d7d7f 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -50,6 +50,8 @@  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
 #define is_haswell_plus(codec) (is_haswell(codec) || is_broadwell(codec))
 
 #define is_valleyview(codec) ((codec)->vendor_id == 0x80862882)
+#define is_cherryview(codec) ((codec)->vendor_id == 0x80862883)
+#define is_valleyview_plus(codec) (is_valleyview(codec) || is_cherryview(codec))
 
 struct hdmi_spec_per_cvt {
 	hda_nid_t cvt_nid;
@@ -1459,7 +1461,7 @@  static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 			    mux_idx);
 
 	/* configure unused pins to choose other converters */
-	if (is_haswell_plus(codec) || is_valleyview(codec))
+	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 		intel_not_share_assigned_cvt(codec, per_pin->pin_nid, mux_idx);
 
 	snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid);
@@ -1598,7 +1600,8 @@  static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
 		 *   and this can make HW reset converter selection on a pin.
 		 */
 		if (eld->eld_valid && !old_eld_valid && per_pin->setup) {
-			if (is_haswell_plus(codec) || is_valleyview(codec)) {
+			if (is_haswell_plus(codec) ||
+				is_valleyview_plus(codec)) {
 				intel_verify_pin_cvt_connect(codec, per_pin);
 				intel_not_share_assigned_cvt(codec, pin_nid,
 							per_pin->mux_idx);
@@ -1779,7 +1782,7 @@  static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 	bool non_pcm;
 	int pinctl;
 
-	if (is_haswell_plus(codec) || is_valleyview(codec)) {
+	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
 		/* Verify pin:cvt selections to avoid silent audio after S3.
 		 * After S3, the audio driver restores pin:cvt selections
 		 * but this can happen before gfx is ready and such selection
@@ -2330,9 +2333,8 @@  static int patch_generic_hdmi(struct hda_codec *codec)
 		intel_haswell_fixup_enable_dp12(codec);
 	}
 
-	if (is_haswell(codec) || is_valleyview(codec)) {
+	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
 		codec->depop_delay = 0;
-	}
 
 	if (hdmi_parse_codec(codec) < 0) {
 		codec->spec = NULL;