Re: [RFC PATCH 4/5] drm: i915: add DisplayPort amp unmute for LPE audio mode

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org, broonie@kernel.org
Subject: Re: [RFC PATCH 4/5] drm: i915: add DisplayPort amp unmute for LPE audio mode
Date: Fri, 27 Jan 2017 15:55:51 +0200	[thread overview]
Message-ID: <20170127135551.GU31595@intel.com> (raw)
In-Reply-To: <8760l04nmh.fsf@intel.com>

On Fri, Jan 27, 2017 at 03:51:34PM +0200, Jani Nikula wrote:
> On Fri, 27 Jan 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote:
> >> On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
> >> > Enable chicken bit on LPE mode setup and unmute amp on
> >> > notification
> >> >
> >> > FIXME: should these two phases done somewhere else?
> >> >
> >> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_reg.h        | 12 ++++++++++++
> >> >  drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++
> >> >  2 files changed, 39 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> > index a9ffc8d..ee90f64 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells {
> >> >  #define I915_HDMI_LPE_AUDIO_BASE	(VLV_DISPLAY_BASE + 0x65000)
> >> >  #define I915_HDMI_LPE_AUDIO_SIZE	0x1000
> >> >  
> >> > +/* DisplayPort Audio w/ LPE */
> >> > +#define CHICKEN_BIT_DBG_ENABLE		(1 << 0)
> >> > +#define AMP_UNMUTE		        (1 << 1)
> >
> > That should be called AMP_MUTE I think,
> >
> >> 
> >> The convention is to define registers first and the contents/bits for
> >> each register immedialy below. For groups of registers (like
> >> PORT_EN_B/C/D below) define all registers first and bits immediately
> >> below. (But note that the chicken register is not part of the group.)
> >> 
> >> > +#define AUD_CHICKEN_BIT_REG		0x62F38
> >
> > Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the
> > letter.
> >
> >> > +#define AUD_PORT_EN_B_DBG		0x62F20
> >> > +#define AUD_PORT_EN_C_DBG		0x62F28
> >> > +#define AUD_PORT_EN_D_DBG		0x62F2C
> >
> > These match the spec. But to match the standard i915 convention they
> > should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit
> > register.
> >
> >> 
> >> Please don't define these separately without the display base, use
> >> (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of
> >> VLV_DISPLAY_BASE in the file follow the same convention.
> >> 
> >> > +#define VLV_AUD_CHICKEN_BIT_REG		_MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG)
> >> > +#define VLV_AUD_PORT_EN_B_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG)
> >> > +#define VLV_AUD_PORT_EN_C_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG)
> >> > +#define VLV_AUD_PORT_EN_D_DBG		_MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG)
> >> > +
> >> 
> >> Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those
> >> reg offsets don't make it easy...
> >
> > _MMIO_PORT3().
> 
> Works for ports A, B, C, but here we have ports B, C, D.

(port)-PORT_B is easy enough to stick in there ;)

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx