Re: [PATCH v2 2/4] drm/i915: implement set_ncts callback

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
> On Mon, 10 Aug 2015, libin.yang@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;

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.
-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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx