Re: [PATCH] ASoC: Add support for BCM2708

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature