Message-ID: <52875467.7000604@metafoo.de>
Date:	Sat, 16 Nov 2013 12:17:59 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Florian Meier <florian.meier@...lo.de>
CC:	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Stephen Warren <swarren@...dotorg.org>,
	devicetree <devicetree@...r.kernel.org>,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	linux-rpi-kernel <linux-rpi-kernel@...ts.infradead.org>,
	dmaengine <dmaengine@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [alsa-devel] [RFCv2] ASoC: Add support for BCM2835

On 11/12/2013 07:41 PM, Florian Meier wrote:
> This driver adds support for digital audio (I2S)
> for the BCM2835 SoC that is used by the
> Raspberry Pi. External audio codecs can be
> connected to the Raspberry Pi via P5 header.
> 
> It relies on cyclic DMA engine support for BCM2835.
> 
> Signed-off-by: Florian Meier <florian.meier@...lo.de>

Looks mostly good in my opinion. A few comments on minor bits and pieces.

> diff --git a/sound/soc/bcm/Kconfig b/sound/soc/bcm/Kconfig
> new file mode 100644
> index 0000000..93619ec
> --- /dev/null
> +++ b/sound/soc/bcm/Kconfig
> @@ -0,0 +1,15 @@
> +config SND_BCM2835_SOC_I2S
> +	tristate
> +
> +config SND_BCM2835_SOC
> +	tristate "SoC Audio support for the Broadcom BCM2835 I2S module"
> +	depends on ARCH_BCM2835

I think there is no compile time dependency on ARCH_BCM2835. Changing this
to 'depends on ARCH_BCM2835 || COMPILE_TEST' allows to archive better build
test coverage for the driver.

> +	select SND_SOC_DMAENGINE_PCM
> +	select DMADEVICES
> +	select DMA_BCM2835

I don't think its a good idea to select DMADEVICES or DMA_BCM2835 here,
since those are user selectable symbols from another subsystem. Either
'depends on DMA_BCM2835' or nothing. Will I think nothing is better since
there is no compile time dependency.

> +	select SND_SOC_GENERIC_DMAENGINE_PCM
> +	select REGMAP_MMIO
> +	help
> +	  Say Y or M if you want to add support for codecs attached to
> +	  the BCM2835 I2S interface. You will also need
> +	  to select the audio interfaces to support below.
[...]

> +static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
> +				    bool tx, bool rx)
> +{
> +	int timeout = 1000;
> +	uint32_t syncval;
> +	uint32_t csreg;
> +	uint32_t i2s_active_state;
> +	uint32_t clkreg;
> +	uint32_t clk_active_state;
> +	uint32_t off;
> +	uint32_t clr;
> +
> +	off =  tx ? BCM2835_I2S_TXON : 0;
> +	off |= rx ? BCM2835_I2S_RXON : 0;
> +
> +	clr =  tx ? BCM2835_I2S_TXCLR : 0;
> +	clr |= rx ? BCM2835_I2S_RXCLR : 0;
> +
> +	/* Backup the current state */
> +	regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> +	i2s_active_state = csreg & (BCM2835_I2S_RXON | BCM2835_I2S_TXON);
> +
> +	regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg);
> +	clk_active_state = clkreg & BCM2835_CLK_ENAB;
> +
> +	/* Start clock if not running */
> +	if (!clk_active_state) {
> +		regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
> +			BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB,
> +			BCM2835_CLK_PASSWD | BCM2835_CLK_ENAB);
> +	}
> +
> +	/* Stop I2S module */
> +	regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, off, 0);
> +
> +	/*
> +	 * Clear the FIFOs
> +	 * Requires at least 2 PCM clock cycles to take effect
> +	 */
> +	regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, clr, -1);

I think the -1 can be confusing. Better use either clr or 0xffffffff. Same
applies to a few other places in the driver.

> +
> +	/* Wait for 2 PCM clock cycles */
> +
> +	/*
> +	 * Toggle the SYNC flag - after 2 PCM clock cycles it can be read back
> +	 * FIXME: This does not seem to work for slave mode!
> +	 */
> +	regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &syncval);
> +	syncval &= BCM2835_I2S_SYNC;
> +
> +	regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +			BCM2835_I2S_SYNC, ~syncval);
> +
> +	/* Wait for the SYNC flag changing it's state */
> +	while (timeout > 0) {
> +		timeout--;
> +
> +		regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> +		if ((csreg & BCM2835_I2S_SYNC) != syncval)
> +			break;
> +	}
> +
> +	if (timeout <= 0)
> +		dev_err(dev->dev, "I2S SYNC error!\n");
> +
> +	/* Stop clock if it was not running before */
> +	if (!clk_active_state)
> +		bcm2835_i2s_stop_clock(dev);
> +
> +	/* Restore I2S state */
> +	regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +			BCM2835_I2S_RXON | BCM2835_I2S_TXON, i2s_active_state);
> +}
> +
[...]
> +static int bcm2835_i2s_dai_probe(struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	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;

I'd move the initialization of dma_data to the driver probe() function.

> +
> +	dai->playback_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK];
> +	dai->capture_dma_data = &dev->dma_data[SNDRV_PCM_STREAM_CAPTURE];

snd_soc_dai_init_dma_data()

> +
> +	return 0;
> +}
> +
[...]
> +
> +static int bcm2835_i2s_probe(struct platform_device *pdev)
> +{
> +	struct bcm2835_i2s_dev *dev;
> +	int i;
> +	int ret;
> +	struct regmap *regmap[2];
> +	struct resource *mem[2];
> +
> +	/* request both ioareas */
> +	for (i = 0; i <= 1; i++) {
> +		void __iomem *base;
> +
> +		mem[i] = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		if (!mem[i]) {
> +			dev_err(&pdev->dev, "I2S probe: Memory resource could not be found.\n");
> +			return -ENODEV;
> +		}
> +
> +		base = devm_ioremap_resource(&pdev->dev, mem[i]);
> +		if (!base) {
> +			dev_err(&pdev->dev, "I2S probe: ioremap failed.\n");
> +			return -ENOMEM;
> +		}
> +
> +		regmap[i] = devm_regmap_init_mmio(&pdev->dev, base,
> +					    &bcm2835_regmap_config[i]);
> +		if (IS_ERR(regmap[i])) {
> +			dev_err(&pdev->dev, "I2S probe: regmap init failed\n");
> +			return PTR_ERR(regmap[i]);
> +		}
> +	}
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(struct bcm2835_i2s_dev),

sizeof(*dev)

> +			   GFP_KERNEL);
> +	if (!dev) {
> +		dev_err(&pdev->dev, "I2S probe: kzalloc failed.\n");
> +		return -ENOMEM;
> +	}
> +
> +	dev->i2s_regmap = regmap[0];
> +	dev->clk_regmap = regmap[1];
> +
> +	/* Set the DMA address */
> +	dev->dma_data[SNDRV_PCM_STREAM_PLAYBACK].addr =
> +		(dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG
> +					  + BCM2835_VCMMU_SHIFT;
> +
> +	dev->dma_data[SNDRV_PCM_STREAM_CAPTURE].addr =
> +		(dma_addr_t)mem[0]->start + BCM2835_I2S_FIFO_A_REG
> +					  + BCM2835_VCMMU_SHIFT;
> +
> +	/* Store the pdev */
> +	dev->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, dev);
> +
> +	ret = snd_soc_register_component(&pdev->dev,
> +			&bcm2835_i2s_component, &bcm2835_i2s_dai, 1);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "Could not register DAI: %d\n", ret);
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	ret = bcm2835_pcm_platform_register(&pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
> +		snd_soc_unregister_component(&pdev->dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id bcm2835_i2s_of_match[] = {
> +	{ .compatible = "brcm,bcm2835-i2s", },

Bindings documentation is missing.

> +	{},
> +};
[...]
> +
> +int bcm2835_pcm_platform_register(struct device *dev)
> +{
> +	return snd_dmaengine_pcm_register(dev, NULL, 0);

Now that these functions are just simple wrappers for
snd_dmaengine_pcm_{register,unregister}() there is really no need to keep
them around. Just remove bcm2835-pcm.h and .c and call them directly from
the i2s driver.

> +}
> +EXPORT_SYMBOL_GPL(bcm2835_pcm_platform_register);
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists