Re: [alsa-devel] [PATCH] ASoC: Add support for BCM2708

On Wed, May 22, 2013 at 04:10:20PM +0200, Florian Meier wrote:

This driver adds support for digital audio (I2S) for the BCM2708 SoC that is used by the Raspberry Pi. External audio codecs can be connected to the Raspberry Pi via P5 header.

Split this up into a patch series, for example one per CPU side driver and one per machine driver. I've given the code a relatively quick run through here, it looks mostly sensible though DT would be nice but there's a few comments.

+static inline void bcm2708_i2s_write_reg(struct bcm2708_i2s_dev *dev,

• 			   int reg, u32 val)
  

+{

• dev_dbg(dev->dev, "I2S write to register %p = %x\n",

• 	dev->clk_base + reg, val);
  

• __raw_writel(val, dev->i2s_base + reg);

+}

This all looks like you want to use regmap-mmio - that'll get you all the logging and so on for free too.

+static void bcm2708_i2s_start_clock(struct bcm2708_i2s_dev *dev) +{

• /*

• * Start the clock if in master mode.
  

• */
  

• unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
• if (master == SND_SOC_DAIFMT_CBS_CFS

• 	|| master == SND_SOC_DAIFMT_CBS_CFM) {
  

This looks like it should be a switch statement.

• unsigned int clkreg = bcm2708_clk_read_reg(dev,
  

• 		BCM2708_CLK_PCMCTL_REG);
  

• bcm2708_clk_write_reg(dev, BCM2708_CLK_PCMCTL_REG,
  

• 		BCM2708_CLK_PASSWD | clkreg | BCM2708_CLK_ENAB);
  

• }

Shouldn't this be using your set bits operation?

+static bool bcm2708_i2s_check_other_stream(struct bcm2708_i2s_dev *dev,

• 			struct snd_pcm_substream *substream,
  

• 			struct snd_pcm_substream *other_stream,
  

• 			struct snd_pcm_hw_params *params)
  

+{

• if (other_stream->runtime->format &&

• (other_stream->runtime->format != params_format(params))) {
  

• dev_err(dev->dev,
  

• 	"Sample formats of streams are different. %i (%s) != %i (%s) Initialization failed!\n",
  

• 	other_stream->runtime->format,
  

• 	(other_stream->stream ==
  

• 	 SNDRV_PCM_STREAM_PLAYBACK ?
  

• 	 "playback" : "capture"),
  

• 	params_format(params),
  

• 	(substream->stream ==
  

• 	 SNDRV_PCM_STREAM_PLAYBACK ?
  

• 	 "playback" : "capture"));
  

This is illegible.

• /* Ensure, that both streams have the same settings */
• struct snd_pcm_substream *other_stream = dev->first_stream;
• if (other_stream == substream)

• other_stream = dev->second_stream;
  

Set constraints for this so that the upper layers will stop mismatched settings being configured.

• /*

• * Clear both FIFOs if the one that should be started
  

• * is not empty at the moment. This should only happen
  

• * after overrun. Otherwise, hw_params would have cleared
  

• * the FIFO.
  

• */
  

Why both?

• case SNDRV_PCM_TRIGGER_START:
• case SNDRV_PCM_TRIGGER_RESUME:
• case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:

• bcm2708_i2s_start(dev, substream);
  

These functions are pretty small, I'd just inline them.

• /*
  

•  * This is the second stream open, so we need to impose
  

•  * sample size and sampling rate constraints.
  

•  * This is because frame length and clock cannot be specified
  

•  * seperately.
  

•  *
  

•  * Note that this can cause a race condition if the
  

•  * second stream is opened before the first stream is
  

•  * fully initialized.  We provide some protection by
  

•  * checking to make sure the first stream is
  

•  * initialized, but it's not perfect.  ALSA sometimes
  

•  * re-initializes the driver with a different sample
  

•  * rate or size.  If the second stream is opened
  

•  * before the first stream has received its final
  

•  * parameters, then the second stream may be
  

•  * constrained to the wrong sample rate or size.
  

•  *
  

•  * We will continue in case of failure and recheck the
  

•  * constraint in hw_params.
  

•  */
  

• if (!first_runtime->format) {
  

• 	dev_err(substream->pcm->card->dev,
  

• 			"Set format in %s stream first! "
  

• 			"Initialization may fail.\n",
  

Don't split strings, but in any case you should just let people set up - hopefully they'll try to set something that might work anyway. Any code for fixing this up should be in the core, like symmetric_rates.

+static void bcm2708_i2s_setup_gpio(void) +{

• /*

• * This is the common way to handle the GPIO pins for
  

• * the Raspberry Pi.
  

• * TODO Better way would be to handle
  

• * this in the device tree!
  

• */
  

Yup.

• /* request both ioareas */
• for (i = 0; i <= 1; i++) {

• struct resource *mem, *ioarea;
  

• mem = platform_get_resource(pdev, IORESOURCE_MEM, i);
  

• if (!mem) {
  

devm_ioremap_resource().

• /* get DMA data (e.g. FIFO address and DREQ) */
• dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
• /*

• * There seems to be no use for bufferless
  

• * transfer with this SoC.
  

• */
  

• if (!dma_data)

• return 0;
  

This is now supported properly by the framework, no need for this.

• dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
• switch (cmd) {
• case SNDRV_PCM_TRIGGER_START:
• case SNDRV_PCM_TRIGGER_RESUME:
• case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:

• break;
  

• case SNDRV_PCM_TRIGGER_STOP:
• case SNDRV_PCM_TRIGGER_SUSPEND:
• case SNDRV_PCM_TRIGGER_PAUSE_PUSH:

• break;
  

• default:

• ret = -EINVAL;
  

• }
• if (ret == 0)

• ret = snd_dmaengine_pcm_trigger(substream, cmd);
  

Juse don't have switch statement.

+static snd_pcm_uframes_t bcm2708_pcm_pointer(

• struct snd_pcm_substream *substream)
  

+{

• return snd_dmaengine_pcm_pointer(substream);

+}

Should just be able to use snd_dmaengine_pcm_pointer() as the ops.

+static int bcm2708_pcm_mmap(struct snd_pcm_substream *substream,

• struct vm_area_struct *vma)

This should be a generic op...

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• return dma_mmap_writecombine(substream->pcm->card->dev, vma,

Should be using the DMA controller device not the card.

• 		     runtime->dma_area,
  

• 		     runtime->dma_addr,
  

• 		     runtime->dma_bytes);
  

+}

+static int bcm2708_pcm_preallocate_dma_buffer(struct snd_pcm *pcm,

• int stream)

+{

• struct snd_pcm_substream *substream = pcm->streams[stream].substream;
• struct snd_dma_buffer *buf = &substream->dma_buffer;
• size_t size = bcm2708_pcm_hardware.buffer_bytes_max;
• buf->dev.type = SNDRV_DMA_TYPE_DEV;
• buf->dev.dev = pcm->card->dev;

DMA controller device.

+static int snd_rpi_mbed_init(struct snd_soc_pcm_runtime *rtd) +{

• return 0;

+}

Empty functions can just be removed.

+static const unsigned int wm8731_rates_12288000[] = {

• 8000, 32000, 48000, 96000,

+};

+static struct snd_pcm_hw_constraint_list wm8731_constraints_12288000 = {

• .list = wm8731_rates_12288000,
• .count = ARRAY_SIZE(wm8731_rates_12288000),

+};

+static int snd_rpi_proto_startup(struct snd_pcm_substream *substream) +{

• /* Setup constraints, because there is a 12.288 MHz XTAL on the board */
• snd_pcm_hw_constraint_list(substream->runtime, 0,

• 		SNDRV_PCM_HW_PARAM_RATE,
  

• 		&wm8731_constraints_12288000);
  

• return 0;

+}

Push this into the CODEC driver, this will be true for the device on any platform with a fixed clock.

+static int snd_rpi_proto_hw_params(struct snd_pcm_substream *substream,

• 		       struct snd_pcm_hw_params *params)
  

+{

• struct snd_soc_pcm_runtime *rtd = substream->private_data;
• struct snd_soc_dai *codec_dai = rtd->codec_dai;
• int sysclk = 12288000; /* This is fixed on this board */
• /* Set proto sysclk */
• int ret = snd_soc_dai_set_sysclk(codec_dai, WM8731_SYSCLK_XTAL,

• sysclk, SND_SOC_CLOCK_IN);
  

• if (ret < 0) {

• dev_err(substream->pcm->dev,
  

• 		"Failed to set WM8731 SYSCLK: %d\n", ret);
  

• return ret;
  

• }

Since this is fixed just do it once on init (eg, in late_probe()) rather than every single time.