Re: [alsa-devel] [PATCH 11/25] ASoC: Samsung: Add common I2S driver

On Tue, Oct 19, 2010 at 04:07:05PM +0900, Jassi Brar wrote:

This looks good - there's a few comments below but they're pretty small things.

/**

• struct s3c_audio_pdata - common platform data for audio device drivers
• @cfg_gpio: Callback function to setup mux'ed pins in I2S/PCM/AC97 mode

*/ struct s3c_audio_pdata { int (*cfg_gpio)(struct platform_device *);

• union {

• struct samsung_i2s i2s;
  

• } type;

Might be as easy just to embed a struct s3c_audio_pdata within the struct samsung_i2s?

• switch (bfs) {
• case 48:

• mod |= MOD_BCLK_48FS;
  

• break;
  

• case 32:

• mod |= MOD_BCLK_32FS;
  

• break;
  

• case 24:

• mod |= MOD_BCLK_24FS;
  

• break;
  

• default:

• mod |= MOD_BCLK_16FS;
  

• break;
  

Especially on the write side I'd be a bit more comfortable if these functions didn't silently convert an unspecified setting into a default so that we know that users are getting the setting they asked for.

• if (is_secondary(i2s)) {
  

• 	con |= CON_TXSDMA_ACTIVE;
  

• 	con &= ~CON_TXSDMA_PAUSE;
  

• } else {
  

• 	con |= CON_TXDMA_ACTIVE;
  

• 	con &= ~CON_TXDMA_PAUSE;
  

Can we do this stuff with a variable storing the mask to use? It might compress the code a lot but I've not looked at what the bits actually are.

• /* Allow LRCLK-inverted version of the supported formats */
• switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
• case SND_SOC_DAIFMT_NB_NF:

• break;
  

• case SND_SOC_DAIFMT_NB_IF:

• if (tmp & MOD_LR_RLOW)
  

• 	tmp &= ~MOD_LR_RLOW;
  

• else
  

• 	tmp |= MOD_LR_RLOW;
  

• break;
  

This looks a bit odd - it'll flip MOD_LR_FLOW if the frame is inverted, I'd expect it to want to set a specific value?

• /* We don't care about BFS in Slave mode */
• if (is_slave(i2s))

• return 0;
  

You're skipping more than just rfs setting here. Probably just a case of updating the comment.

+static int i2sv2_i2s_set_clkdiv(struct snd_soc_dai *dai,

• int div_id, int div)

+{

• struct i2s_dai *i2s = to_info(dai);
• struct i2s_dai *other = i2s->pri_dai ? : i2s->sec_dai;
• if (div_id == SAMSUNG_I2S_DIV_BCLK) {

switch statement please.

+#define SAMSUNG_I2S_RATES \

• (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 | SNDRV_PCM_RATE_16000 | \
• SNDRV_PCM_RATE_22050 | SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \
• SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000)

SNDRV_PCM_RATE_8000_96000.

+/*

• • Maximum number of I2S blocks that any SoC can have.
• • The secondary interface of a CPU dai(if there exists any),
• • is indexed at [cpu-dai's ID + MAX_I2S]
• */

+#define MAX_I2S 4

Can this be kept internal to the driver? If not it should be namespaced.