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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
