Re: [alsa-devel] [RFC] ASoC: Add support for BCM2835

[Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>]

On 11/08/2013 09:56 PM, Florian Meier wrote:

Is the I2S clock part of a generic clocking module? If yes it might be
better to implement this as a clock chip driver.

[...]
> +static void bcm2835_i2s_stop(struct bcm2835_i2s_dev *dev,
> +		struct snd_pcm_substream *substream)
> +{
> +	unsigned int still_running;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_RXON, 0);
> +	} else {
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_TXON, 0);
> +	}
> +
> +	regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &still_running);
> +	still_running &= BCM2835_I2S_TXON | BCM2835_I2S_RXON;
> +

still_running can be replaced with dai->active, I think

> +	/* Stop also the clock when not SND_SOC_DAIFMT_CONT */
> +	if (!still_running && !(dev->fmt & SND_SOC_DAIFMT_CONT))
> +		bcm2835_i2s_stop_clock(dev);
> +
> +}
> +
> +static int bcm2835_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		bcm2835_i2s_start_clock(dev);
> +
> +		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +			regmap_update_bits(dev->i2s_regmap,
> +					BCM2835_I2S_CS_A_REG,
> +					BCM2835_I2S_RXON, BCM2835_I2S_RXON);
> +		} else {
> +			regmap_update_bits(dev->i2s_regmap,
> +					BCM2835_I2S_CS_A_REG,
> +					BCM2835_I2S_TXON, BCM2835_I2S_TXON);
> +		}

Doing something like

		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
			mask = BCM2835_I2S_RXON;
		else
			mask = BCM2835_I2S_TXON;

		regmap_update_bits(dev->i2s_regmap,
			BCM2835_I2S_CS_A_REG, mask, mask);

makes the code a bit shorter. Same for bcm2835_i2s_stop().
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		bcm2835_i2s_stop(dev, substream);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int bcm2835_i2s_startup(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	if (!dev->first_stream) {
> +		dev->first_stream = substream;
> +

Since you use first_stream and second_stream for anything else I think you
can just use dai->active here instead.

> +		/* should this still be running stop it */
> +		bcm2835_i2s_stop_clock(dev);
> +
> +		/* enable PCM block */
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_EN, -1);
> +
> +		/*
> +		 * Disable STBY
> +		 * Requires at least 4 PCM clock cycles to take effect
> +		 */
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_STBY, -1);
> +
> +	} else {
> +		dev->second_stream = substream;
> +	}
> +
> +	return 0;
> +}
> +
[...]
> +static int bcm2835_i2s_dai_probe(struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	/* Set the appropriate DMA parameters */
> +	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr =
> +		(dma_addr_t)BCM2835_I2S_FIFO_PHYSICAL_ADDR;
> +	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr =
> +		(dma_addr_t)BCM2835_I2S_FIFO_PHYSICAL_ADDR;

The dma address for the FIFO should not be hardcoded but rather use an
offset to the resource that get's pass to the driver.

	dma_data->addr = mem->start + BCM2835_I2S_FIFO_A_REG;

I think it should be fine to initialize dma_data already in the driver
probe() function.

> +
> +	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].slave_id =
> +		BCM2835_DMA_DREQ_PCM_TX;
> +	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].slave_id =
> +		BCM2835_DMA_DREQ_PCM_RX;
> +
> +	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr_width =
> +		DMA_SLAVE_BUSWIDTH_4_BYTES;
> +	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr_width =
> +		DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	/* TODO other burst parameters possible? */
> +	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].maxburst = 2;
> +	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].maxburst = 2;
> +
> +	dai->playback_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
> +	dai->capture_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_CAPTURE];
> +
> +	return 0;
> +}
> +
[...]
> +
> +static bool bcm2835_i2s_wr_rd_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case BCM2835_I2S_CS_A_REG:
> +	case BCM2835_I2S_FIFO_A_REG:
> +	case BCM2835_I2S_MODE_A_REG:
> +	case BCM2835_I2S_RXC_A_REG:
> +	case BCM2835_I2S_TXC_A_REG:
> +	case BCM2835_I2S_DREQ_A_REG:
> +	case BCM2835_I2S_INTEN_A_REG:
> +	case BCM2835_I2S_INTSTC_A_REG:
> +	case BCM2835_I2S_GRAY_REG:
> +		return true;
> +	default:
> +		return false;
> +	};
> +}

I think you use the default implementation of the here, which is basically
(reg >= 0 && reg <= config->max_register). Just leave the writeable_reg and
readable_reg fields of your config NULL.

[...]
> +
> +static const struct regmap_config bcm2835_i2s_regmap_config = {
> +};

This one seems to be unused.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html