Messages in this thread | | | Date | Wed, 22 May 2013 11:39:42 -0500 | From | Mark Brown <> | Subject | Re: [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. [unhandled content-type:application/pgp-signature] | |