[alsa-devel] [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback

[Yang, Libin]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:05 PM
> To: Jani Nikula
> Cc: Yang, Libin; alsa-devel at alsa-project.org; tiwai at suse.de; intel-
> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
> > On Mon, 10 Aug 2015, libin.yang at intel.com wrote:
> > > +	n_low = n & 0xfff;
> > > +	n_up = (n >> 12) & 0xff;
> > > +
> > > +	/* 4. set the N/CTS */
> > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > > +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > > +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > > +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > > +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> >
> > You could squash the ors and ands together.
> 
> Also read-modify-write considered evil. If you need to save a few bits
> please be explicit about it like
> 
> 	tmp &= BITS_WE_WANT_TO_KEEP;

OK, I will try to use this format.

> 
> if there's no bits to keep just start out with 0. If we have too much rwm
> to hw registers then probably something with the overall design is
> botched, e.g. with the pipe config precomputation we managed to
> remove
> almost all the rwm code for modesets.

Yes, if we can set the register by starting out with 0,
it will be better. But in this case, there are some reserved
bits. We should reserve its bits for compatibility, I think.

> -Daniel
> 
> >
> > > +	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct i915_audio_component_ops
> i915_audio_component_ops = {
> > >  	.owner		= THIS_MODULE,
> > >  	.get_power	= i915_audio_component_get_power,
> > >  	.put_power	= i915_audio_component_put_power,
> > >  	.codec_wake_override =
> i915_audio_component_codec_wake_override,
> > >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > > +	.set_ncts = i915_audio_component_set_ncts,
> > >  };
> > >
> > >  static int i915_audio_component_bind(struct device *i915_dev,
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch