[alsa-devel] [PATCHv3] ASoC: Add support for BCM2835

[Shevchenko, Andriy]

On Mon, 2013-11-18 at 12:28 +0100, 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.

Few comments below.

[]

> +static void bcm2835_i2s_stop_clock(struct bcm2835_i2s_dev *dev)
> +{
> +	uint32_t clkreg;
> +	int timeout = 1000;
> +
> +	/* Stop clock */
> +	regmap_update_bits(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG,
> +			BCM2835_CLK_PASSWD_MASK | BCM2835_CLK_ENAB,
> +			BCM2835_CLK_PASSWD);
> +
> +	/* Wait for the BUSY flag going down */
> +	while (timeout > 0) {
> +		timeout--;

while (--timeout) {

> +
> +		regmap_read(dev->clk_regmap, BCM2835_CLK_PCMCTL_REG, &clkreg);
> +		if (!(clkreg & BCM2835_CLK_BUSY))
> +			break;
> +	}
> +
> +	if (timeout <= 0) {

if (!timeout) {

[]

> +static void bcm2835_i2s_clear_fifos(struct bcm2835_i2s_dev *dev,
> +				    bool tx, bool rx)
> +{

[]

> +	/* Wait for the SYNC flag changing it's state */
> +	while (timeout > 0) {
> +		timeout--;

while (--timeout) {

> +		regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> +		if ((csreg & BCM2835_I2S_SYNC) != syncval)
> +			break;
> +	}
> +
> +	if (timeout <= 0)

if (!timeout)

[]

> +static int bcm2835_i2s_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	unsigned int sampling_rate = params_rate(params);
> +	unsigned int data_length, data_delay, bclk_ratio;
> +	unsigned int ch1pos, ch2pos, mode, format;
> +	unsigned int mash = BCM2835_CLK_MASH_1;
> +	unsigned int divi, divf, target_frequency;
> +	int clk_src = -1;
> +	unsigned int master = dev->fmt & SND_SOC_DAIFMT_MASTER_MASK;
> +	uint32_t bit_master =	(master == SND_SOC_DAIFMT_CBS_CFS
> +					|| master == SND_SOC_DAIFMT_CBS_CFM);

bool ?

> +
> +	uint32_t frame_master =	(master == SND_SOC_DAIFMT_CBS_CFS

So, if master == SND_SOC_DAIFMT_CBS_CFS both bit_master and frame_master
will be true. Is it correct?

> +					|| master == SND_SOC_DAIFMT_CBM_CFS);

Ditto.

> +	uint32_t csreg;
> +
> +	/*
> +	 * If a stream is already enabled,
> +	 * the registers are already set properly.
> +	 */
> +	regmap_read(dev->i2s_regmap, BCM2835_I2S_CS_A_REG, &csreg);
> +
> +	if (csreg & (BCM2835_I2S_TXON | BCM2835_I2S_RXON))
> +		return 0;
> +

> +	if (bcm2835_clk_freq[clk_src] % target_frequency == 0
> +			&& bit_master && frame_master) {
> +		divi = bcm2835_clk_freq[clk_src]/target_frequency;
> +		divf = 0;
> +	} else {
> +		uint64_t dividend;
> +
> +		if (!dev->bclk_ratio) {
> +			/*
> +			 * Overwrite bclk_ratio, because the
> +			 * above trick is not needed or can
> +			 * not be used.
> +			 */
> +			bclk_ratio = 2*data_length;

= 2 * data_length;

> +		}
> +
> +		target_frequency = sampling_rate*bclk_ratio;
> +
> +		clk_src = BCM2835_CLK_SRC_PLLD;
> +		mash = BCM2835_CLK_MASH_1;
> +
> +		dividend = bcm2835_clk_freq[clk_src];
> +		dividend *= 1024;
> +		do_div(dividend, target_frequency);
> +		divi = dividend / 1024;
> +		divf = dividend % 1024;

1024 is a magic number?
Could you use shifts and bitwise ANDs here?

> +	ch1pos = data_delay;
> +	ch2pos = bclk_ratio/2+data_delay;

' / 2 + '

[]

> +	mode |= BCM2835_I2S_FLEN(bclk_ratio-1);
> +	mode |= BCM2835_I2S_FSLEN(bclk_ratio/2);

Ditto. Spaces are missed.

[]

> +	case SND_SOC_DAIFMT_IB_IF:
> +		/* Both - therefore, none for BCM*/

'M */'

> +		break;
> +	case SND_SOC_DAIFMT_NB_IF:
> +		/*
> +		 * Invert only frame sync - therefore,
> +		 * Invert only bit clock for BCM

Something wrong with casing in the commentary.

> +		 */
> +		mode |= BCM2835_I2S_CLKI;
> +		break;
> +	case SND_SOC_DAIFMT_IB_NF:
> +		/*
> +		 * Invert only bit clock - therefore,
> +		 * Invert only frame sync for BCM

Ditto.

> +		 */
> +		mode |= BCM2835_I2S_FSI;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	regmap_write(dev->i2s_regmap, BCM2835_I2S_MODE_A_REG, mode);
> +
> +

Redundant empty line.

[]

> +
> +	return 0;
> +}
> +
> +

Ditto.

[]

> +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 (!dai->active) {
> +		/* 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, BCM2835_I2S_EN);
> +
> +		/*
> +		 * Disable STBY

Dot is missing?

> +		 * Requires at least 4 PCM clock cycles to take effect
> +		 */
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_STBY, BCM2835_I2S_STBY);
> +	}
> +
> +	return 0;
> +}



> +static void bcm2835_i2s_shutdown(struct snd_pcm_substream *substream,
> +		struct snd_soc_dai *dai)
> +{
> +	struct bcm2835_i2s_dev *dev = snd_soc_dai_get_drvdata(dai);
> +
> +	bcm2835_i2s_stop(dev, substream, dai);
> +
> +	/* If both streams are stopped, disable module and clock */
> +	if (!dai->active) {

if (dai->active)
 return;

> +		/* Disable the module */
> +		regmap_update_bits(dev->i2s_regmap, BCM2835_I2S_CS_A_REG,
> +				BCM2835_I2S_EN, 0);
> +
> +		/*
> +		 * Stopping clock is necessary, because stop does
> +		 * not stop the clock when SND_SOC_DAIFMT_CONT
> +		 */
> +		bcm2835_i2s_stop_clock(dev);
> +	}
> +}
> +

[]

> +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;
> +		}

Unneeded messaging and check.

> +
> +		base = devm_ioremap_resource(&pdev->dev, mem[i]);
> +		if (!base) {

It returns ERR_PTR.
Thus,

if (IS_ERR(base)
return PTR_ERR(base)


> +			dev_err(&pdev->dev, "I2S probe: ioremap failed.\n");

Unneeded messaging.

> +			return -ENOMEM;

Wrong error code. See above.


-- 
Andy Shevchenko <andriy.shevchenko at intel.com>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.