Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4956 bytes --]

On Mon, Oct 27, 2014 at 10:07:05PM +0300, Max Filippov wrote:

> +config SND_SOC_XTENSA_XTFPGA
> +	tristate "SoC Audio for xtensa xtfpga"
> +	depends on XTENSA_PLATFORM_XTFPGA
> +	select SND_SOC_XTFPGA_I2S
> +	select SND_SOC_TLV320AIC23_SPI
> +	select SND_SIMPLE_CARD

I've only got this far in the file and have to leave now so I'll look
properly at the actual code later but the above shouldn't have the CODEC
or card driver selected, if the I2S driver is well written it should be
usable independently of either.

> +++ b/sound/soc/xtensa/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SND_SOC_XTFPGA_I2S) += xtfpga-i2s.o

Please follow the style for all other ASoC drivers and name the module
snd-soc-foo.

> +static void xtfpga_i2s_push_tx(struct xtfpga_i2s *i2s)
> +{
> +	struct snd_pcm_substream *tx_substream;
> +	struct snd_pcm_runtime *runtime;
> +
> +	rcu_read_lock();
> +	tx_substream = rcu_dereference(i2s->tx_substream);
> +	runtime = tx_substream->runtime;
> +#define xtfpga_i2s_tx_loop(i2s, runtime, channels, sample_bits) \
> +	do { \
> +		const u##sample_bits (*p)[channels] = \
> +			(void *)runtime->dma_area; \
> +		for (; i2s->tx_fifo_sz < i2s->tx_fifo_high; \
> +		     i2s->tx_fifo_sz += 2) { \
> +			iowrite32(p[i2s->tx_ptr][0], \
> +				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
> +			iowrite32(p[i2s->tx_ptr][channels - 1], \
> +				  i2s->regs + XTFPGA_I2S_CHAN0_DATA); \
> +			if (++i2s->tx_ptr >= runtime->buffer_size) \
> +				i2s->tx_ptr = 0; \
> +		} \
> +	} while (0)
> +
> +	switch (runtime->sample_bits | (runtime->channels << 8)) {
> +	case 0x110:
> +		xtfpga_i2s_tx_loop(i2s, runtime, 1, 16);
> +		break;

This really needs some rework to be legible.  I'd suggest making your
macro an inline function and either replacing these magic numbers in the
switch statement with defines or just writing the code clearly and
letting the compiler figure it out.

Some documentation explaining what your RCU usage is all about would
also be good...  I'm not clear why it's being used, none of the FIQ
drivers do this and I can't entirely figure out what we're defending
against other than the tasklet which we should be stopping anyway and
why what we're doing is safe.

I would also expect to see the data transfer and I2S bits more split
out, presumably this IP can actually be used in designs with a DMA
controller and one would expect that for practical systems this would be
the common case?

> +	if (int_status & XTFPGA_I2S_INT_UNDERRUN) {
> +		i2s->tx_fifo_sz = 0;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE |
> +				   XTFPGA_I2S_CONFIG_TX_ENABLE, 0);
> +	} else {
> +		i2s->tx_fifo_sz = i2s->tx_fifo_low;
> +		regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +				   XTFPGA_I2S_CONFIG_INT_ENABLE, 0);
> +	}

We're trying to implement a NAPI style thing here?

> +	if (tx_active) {
> +		if (i2s->tx_fifo_high < 256)
> +			xtfpga_i2s_refill_fifo(i2s);
> +		else
> +			tasklet_hi_schedule(&i2s->refill_fifo);

How does the interrupt handler avoid getting into a fight with the
tasklet if the interrupt line is shared?

> +static int xtfpga_i2s_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params,
> +				   struct snd_soc_dai *dai)
> +{
> +	struct xtfpga_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +	unsigned srate = params_rate(params);
> +	unsigned channels = params_channels(params);
> +	unsigned period_size = params_period_size(params);
> +	unsigned sample_size = snd_pcm_format_width(params_format(params));
> +	unsigned freq, ratio, level;
> +	int err;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		break;

This looks buggy, we're accepting two different formats but not storing
anything or telling the hardware - especially concerning given that this
is a master only driver.

> +	regmap_update_bits(i2s->regmap, XTFPGA_I2S_CONFIG,
> +			   XTFPGA_I2S_CONFIG_RES_MASK,
> +			   sample_size << XTFPGA_I2S_CONFIG_RES_BASE);
> +
> +	if (i2s->clk_enabled) {
> +		clk_disable_unprepare(i2s->clk);
> +		i2s->clk_enabled = false;
> +	}
> +	freq = 256 * srate;
> +	err = clk_set_rate(i2s->clk, freq);
> +	if (err < 0)
> +		return err;
> +
> +	err = clk_prepare_enable(i2s->clk);
> +	if (err < 0)
> +		return err;
> +	i2s->clk_enabled = true;

This stuff with the clock seems complicated, why not just leave it
disabled when not in use and take advantage of the reference counting
the core already has?

> +	ratio = (freq - srate * sample_size * 8) /
> +		(srate * sample_size * 4);

What ratio exactly?  This needs more brackets in the first line and a
comment explaining what it's calculating.

> +	i2s->tx_fifo_low = XTFPGA_I2S_FIFO_SIZE / 2;
> +	for (level = 1;
> +	     /* period_size * 2: FIFO always gets 2 samples per frame */
> +	     i2s->tx_fifo_low / 2 >= period_size * 2;
> +	     ++level)
> +		i2s->tx_fifo_low /= 2;

This can't come out with a bad value?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]