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

Reply via email to