Re: [PATCH v2] ASoC: rt5514: add rt5514 SPI driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3093 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]