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

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?