On Tue, Oct 19, 2010 at 6:35 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
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?
struct s3c_audio_pdata is shared among I2S, AC97, SPDIF and PCM. Here the idea is that controller drivers add specific structures to the union as and when they need it.
- 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.
Ok. I'll add a case for 16 and print error in default.
- 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.
sorry, I am unable to understand what you suggest
- /* 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?
The frame polarity is specified by the Format(I2S, LSB, MSB), so we set/clear MOD_LR_RLOW acc to the Format requested. The Inversion request works relative to the Format -- if Frame inversion is requested, we simply flip it from the value set during Format setting.
- /* 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.
Yes, I need to update 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.
Switch didn't look very pretty with just one case, so I made it if-else Anyways, I'll change it to switch.
+#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.
of course !
+/*
- 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.
ASoC machine drivers need to index the secondary dai that is automatically created and registered by the cpu driver. So, it needs to be available outside. I'll make it SAMSUNG_I2S_SECOFF -- secondary offset of I2S(?)
Thanks.