On Mon, Mar 14, 2016 at 06:58:21PM +0800, Oder Chiou wrote:
This looks mostly good, a few relatively small things here that should be simple to fix:
+static void rt5514_spi_copy_work(struct work_struct *work) +{
- struct rt5514_dsp *rt5514_dsp =
container_of(work, struct rt5514_dsp, copy_work.work);- struct snd_pcm_runtime *runtime = rt5514_dsp->substream->runtime;
- size_t period_bytes, truncated_bytes = 0;
- mutex_lock(&rt5514_dsp->dma_lock);
_copy_work() holds dma_lock for the entire time it runs...
+static int rt5514_spi_hw_free(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct rt5514_dsp *rt5514_dsp =
snd_soc_platform_get_drvdata(rtd->platform);- mutex_lock(&rt5514_dsp->dma_lock);
- cancel_delayed_work_sync(&rt5514_dsp->copy_work);
- rt5514_dsp->substream = NULL;
- mutex_unlock(&rt5514_dsp->dma_lock);
...but _hw_free() tries to cancel the work while holding dma_lock. This means we've got a deadlock, if the work starts running after the mutex is acquired in _hw_free() then it will block trying to get the mutex which will never get released because we're waiting for the work to complete.
Looks like the cancel needs to be after the unlock.
+static struct spi_driver rt5514_spi_driver = {
- .driver = {
.name = "rt5514",.of_match_table = of_match_ptr(rt5514_of_match),
Weird indentation here.
+static void rt5514_enable_dsp_prepare(struct rt5514_priv *rt5514) +{
- /* Reset */
- regmap_write(rt5514->i2c_regmap, 0x18002000, 0x000010ec);
This is a *very* big table of magic numbers with what look like system specific settings in there (there seem to be some things routing to and from the ADC, and some clocking setup...). Are you sure none of this depends on the runtime configuration?
- if (rt5514->dsp_enabled) {
rt5514_enable_dsp_prepare(rt5514);request_firmware(&fw, RT5514_FIRMWARE1, codec->dev);if (fw) {+#if defined(CONFIG_SND_SOC_RT5514_SPI)
rt5514_spi_burst_write(0x4ff60000, fw->data,((fw->size/8)+1)*8);+#endif
This will silently report success if SPI support is disabled, I'd expect either the control would be entirely absent or we'd report an error.
if (snd_soc_codec_get_bias_level(codec) ==SND_SOC_BIAS_STANDBY) {/*** If the DSP is enabled in start of recording, the* DSP should be disabled, and sync back to normal* recording settings to make sure recording* properly.*/if (rt5514->dsp_enabled) {rt5514->dsp_enabled = 0;regcache_mark_dirty(rt5514->i2c_regmap);regcache_mark_dirty(rt5514->regmap);regcache_sync(rt5514->i2c_regmap);regcache_sync(rt5514->regmap);}
This talks specifically about recording but this is in set_bias_level(). I'd expect something to do with the start/stop of recording to be attached to something like a DAPM event for an ADC or DAI widget.