Re: [PATCH 1/5] ASoC: Intel: Add Intel SST audio DSP low level shim driver.

[Liam Girdwood <liam.r.girdwood@linux.intel.com>]

On Fri, 2014-02-14 at 09:29 +0100, Takashi Iwai wrote:
> At Thu, 13 Feb 2014 19:15:26 +0000,
> Liam Girdwood wrote:

> > +/* Public API */
> > +void sst_dsp_shim_write(struct sst_dsp *sst, u32 offset, u32 value)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&sst->spinlock, flags);
> > +	sst->ops->write(sst->addr.shim, offset, value);
> > +	spin_unlock_irqrestore(&sst->spinlock, flags);
> > +}
> > +EXPORT_SYMBOL(sst_dsp_shim_write);
> 
> Any reason not to use *_GPL() version?
> 

No that should be GPL. Will fix.


> > +int sst_dsp_shim_update_bits64_unlocked(struct sst_dsp *sst, u32 offset,
> > +				u64 mask, u64 value)
> > +{
> > +	bool change;
> > +	u64 old, new;
> > +
> > +	old = sst_dsp_shim_read64_unlocked(sst, offset);
> > +
> > +	new = (old & (~mask)) | (value & mask);
> > +
> > +	change = (old != new);
> > +	if (change)
> > +		sst_dsp_shim_write64_unlocked(sst, offset, new);
> > +
> > +	return change;
> > +}
> > +EXPORT_SYMBOL(sst_dsp_shim_update_bits64_unlocked);
> 
> The locked versions can be simplified by calling the unlocked
> function.
> 

True, will change for V2


> > +
> > +void sst_dsp_ipc_msg_tx(struct sst_dsp *dsp, u32 msg)
> > +{
> > +	sst_dsp_shim_write_unlocked(dsp, SST_IPCX, msg | SST_IPCX_BUSY);
> > +	trace_sst_ipc_msg_tx(msg);
> 
> The trace isn't defined yet in the first patch, so the build fails
> here.  Each commit should be at least compilable.
> 

It is build-able for bisect etc, the Make/Kconfig patch is last in this
series after the trace patch. 

Liam