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@xxxxxxxxxxxxxxx> 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
