On Fri, Feb 05, 2016 at 12:42:13PM +0800, Oder Chiou wrote: > The patch adds the rt5514 SPI driver for loading the firmware of DSP and > retrieving the voice data after the system is waked up by voice. Please have words with your hardware designers about having devices with multiple control interfaces :/ > config SND_SOC_RT5514 > tristate > + default y if SND_SOC_RT5514_SPI=y > + default m if SND_SOC_RT5514_SPI=m > + > +config SND_SOC_RT5514_SPI > + tristate > + depends on SPI_MASTER This is really confused and probably broken. I'm surprised that having a default for a hidden symbol like SND_SOC_RT5514 would do anything and there's nothing here that ensures that the dependencies are correct with respect to I2C. > +static int rt5514_spi_prepare(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); > + > + rt5514_dsp->dma_offset = 0; > + rt5514_dsp->dsp_offset = 0; > + rt5514_spi_read_addr(RT5514_BUFFER_VOICE_BASE, &rt5514_dsp->buf_base); > + rt5514_spi_read_addr(RT5514_BUFFER_VOICE_LIMIT, &rt5514_dsp->buf_limit); > + rt5514_spi_read_addr(RT5514_BUFFER_VOICE_RP, &rt5514_dsp->buf_rp); > + rt5514_spi_read_addr(RT5514_BUFFER_VOICE_SIZE, &rt5514_dsp->buf_size); > + > + if (rt5514_dsp->buf_base && rt5514_dsp->buf_limit && > + rt5514_dsp->buf_rp && rt5514_dsp->buf_size) > + schedule_delayed_work(&rt5514_dsp->copy_work, 0); Scheduling this work in prepare() seems wrong - it's going to start doing things before we trigger anything. I'd expect this to be done on trigger (which is fine). > + x[0].len = 5; > + x[0].tx_buf = write_buf; > + spi_message_add_tail(&x[0], &message); > + > + x[1].len = 4; > + x[1].tx_buf = write_buf; > + spi_message_add_tail(&x[1], &message); > + > + x[2].len = end; > + x[2].rx_buf = rxbuf + offset; > + spi_message_add_tail(&x[2], &message); This all looks a lot like a regmap... only funny thing is the command byte on the front of everything which we could add support for, AFAICT we could just use burst I/O for everything. > + offset += RT5514_SPI_BUF_LEN; Is this a limitation of the device? > +EXPORT_SYMBOL_GPL(rt5514_spi_burst_read); Why are all these symbols exported? > +static void rt5514_enable_dsp_clock(struct rt5514_priv *rt5514) > +{ > + /* Reset */ > + regmap_write(rt5514->i2c_regmap, 0x18002000, 0x000010ec); > + /* LDO_I_limit */ > + regmap_write(rt5514->i2c_regmap, 0x18002200, 0x00028604); > + /* (for reset DSP) mini-core reset */ > + regmap_write(rt5514->i2c_regmap, 0x18002f00, 0x0005514b); This looks like it's doing rather more than enabling the clock, there looks to be quite a bit of use case specific stuff in here... > + request_firmware(&fw, "0x4ff60000.dat", codec->dev); > + if (fw) { That needs a *much* more sensible filename, at least put the firmware in a directory for the CODEC. > + SOC_SINGLE_EXT("DSP Control", SND_SOC_NOPM, 0, 1, 0, > + rt5514_dsp_mode_get, rt5514_dsp_mode_put), It's not clear what this is intended to mean to userspace... looks like it might be intended to be an on/off switch but that's more a DAPM thing. > + if (snd_soc_codec_get_bias_level(codec) == > + SND_SOC_BIAS_STANDBY) { > + if (rt5514->dsp_mode) { > + rt5514->dsp_mode = 0; So we reset the value for dsp_mode when powering up... that's confusing?
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel