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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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.