[Intel-gfx] [PATCH] drm/i915/display: program audio CDCLK-TS for keepalives

[Kai Vehmanen]

Hi,

On Fri, 10 Sep 2021, Jani Nikula wrote:

> Nitpick, switching to i915 variable name instead of dev_priv is
> preferred for new code throughout.

ack, changed.

> On Fri, 10 Sep 2021, Kai Vehmanen <kai.vehmanen at linux.intel.com> wrote:
> > +	if (DISPLAY_VER(dev_priv) >= 13) {
> > +		tmp = intel_de_read(dev_priv, AUD_TS_CDCLK_M);
> > +		tmp &= ~AUD_TS_CDCLK_M_EN;
> > +		intel_de_write(dev_priv, AUD_TS_CDCLK_M, tmp);
> 
> Nitpick, could use intel_de_rmw() to do this on a single line.

True, changed.

> > +static int get_aud_ts_cdclk_m_n(int refclk, int cdclk, struct aud_ts_cdclk_m_n *aud_ts)
> > +{
> > +	if (!aud_ts)
> > +		return -EINVAL;
> > +
> > +	if (refclk == 24000)
> > +		aud_ts->m = 12;
> > +	else
> > +		aud_ts->m = 15;
> > +
> > +	aud_ts->n = cdclk * aud_ts->m / 24000;
> > +
> > +	return 0;
> 
> Is the return code added for future? For now, it just complicates this
> for no apparent reason.

An early version had a table lookup and some refclk values were not 
supported. But, but, this version just calculates with a formula, so
there's no real reason to have the return code anymore. A good point, 
removed.

> > +			drm_dbg(&dev_priv->drm, "aud_ts_cdclk set to M=%u, N=%u\n",
> > +				aud_ts.m, aud_ts.n);
> 
> Usually drm_dbg_kms() for display code, including audio.

Gah, indeed, fixed.

> I'll need to look up the bspec too, can't do that right now... this was
> just the nitpicks. ;D

Keep 'em coming! :)

Br, Kai