[alsa-devel] [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

[Lin, Mengdong]

Hi Takashi,

Thanks for your review! The revised patches are submitted: 
[PATCH v2 1/2] drm/i915: provide interface for audio driver to query cdclk
[PATCH v2 2/2] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

And we think these patches may have Cc to stable, since Haswell need them.

Hi Jani,

Your first patch was also submitted to Intel gfx mailing list. Thanks for your help on this issue!

Regards
Mengdong

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Wednesday, July 02, 2014 11:01 PM
> To: Lin, Mengdong
> Cc: alsa-devel at alsa-project.org; Nikula, Jani
> Subject: Re: [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK
> for HSW/BDW display HDA controller
> 
> At Wed,  2 Jul 2014 22:44:07 +0800,
> mengdong.lin at intel.com wrote:
> >
> > From: Mengdong Lin <mengdong.lin at intel.com>
> >
> > For HSW/BDW display HD-A controller, this patch restores BCLK by
> > setting the M/N values as per the core display clock (CDCLK) queried
> > from i915 display driver.
> >
> > And the audio driver will also restore BCLK in azx_first_init() since
> > the display driver can turn off the shared power in boot phase if only
> > eDP is connected and M/N values will be lost and must be
> reprogrammed.
> >
> > Signed-off-by: Mengdong Lin <mengdong.lin at intel.com>
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 25753db..c7dfd1d 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
> >
> >  struct hda_intel {
> >  	struct azx chip;
> > -
> > -	/* HSW/BDW display HDA controller to restore BCLK from CDCLK */
> > -	unsigned int bclk_m;
> > -	unsigned int bclk_n;
> >  };
> >
> >
> > @@ -598,20 +594,41 @@ static int param_set_xint(const char *val,
> const
> > struct kernel_param *kp)  #define azx_del_card_list(chip) /* NOP */
> > #endif /* CONFIG_PM */
> >
> > -static void haswell_save_bclk(struct azx *chip) -{
> > -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> > -
> > -	hda->bclk_m = azx_readw(chip, EM4);
> > -	hda->bclk_n = azx_readw(chip, EM5);
> > -}
> >
> >  static void haswell_restore_bclk(struct azx *chip)  {
> > -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> > +	int cdclk_freq;
> > +	unsigned int bclk_m, bclk_n;
> > +
> > +	cdclk_freq = hda_get_display_clk();
> > +	switch (cdclk_freq) {
> > +	case 337500:
> > +		bclk_m = 16;
> > +		bclk_n = 225;
> > +		break;
> > +
> > +	case 450000: /* default CDCLK 450MHz */
> > +		bclk_m = 4;
> > +		bclk_n = 75;
> > +		break;
> > +
> > +	case 540000:
> > +		bclk_m = 4;
> > +		bclk_n = 90;
> > +		break;
> > +
> > +	case 675000:
> > +		bclk_m = 8;
> > +		bclk_n = 225;
> > +		break;
> >
> > -	azx_writew(chip, EM4, hda->bclk_m);
> > -	azx_writew(chip, EM5, hda->bclk_n);
> > +	default:
> > +		bclk_m = 4;
> > +		bclk_n = 75;
> 
> You can put "default:" around "case 45000:", and move the comment
> about default CDCLK there.
> 
> > +	}
> > +
> > +	azx_writew(chip, EM4, bclk_m);
> > +	azx_writew(chip, EM5, bclk_n);
> >  }
> 
> IMO, the whole function can be moved to hda_i915.c and provided as a
> function such as hda_set_display_clk().  Then you can drop
> hda_get_display_clk() but call get_cdclk() internally.
> 
> Also, all these patches may have Cc to stable, right?
> 
> 
> thanks,
> 
> Takashi