Re: [PATCH] S3C64XX I2S: Added machine driver for WM8580

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

On Wed, Sep 16, 2009 at 07:02:42PM +0900, Jassi wrote:

> New machine driver for WM8580 I2S i/f on SMDK64XX.
> By default SoC-Slave is set and WM8580 is configured to use it's
> PLLA to generate clocks from a 12MHz crystal attached to WM8580.

This looks sane as a starting point and most of the driver looks OK.

> I2S controller-0 is set as default, set S3C64XX_I2S_CNTRLR to 2
> inorder to use I2S_v4 controller of S3C6410.

I'm not entirely sure what's going on in this configuration - the
primary AIFs of the CODEC appear to be hard wired to the IISv4 interface
in the schematics I have, IIS port 0 is only wired to the secondary AIF
of the WM8580 so will need support for that implementing.  The end
result should be an extra set of DAI links for the secondary AIF.  Could
you clarify, please?

> +/* SMDK64XX has a 12MHZ crystal attached to WM8580 */
> +#define SMDK64XX_WM8580_FREQ (12000000)

No need for the () here.

> +	switch (params_rate(params)) {
> +	case 8000:
> +		pll_out = 8192000 / 4; break;

These are all a bit odd - you're taking two magic numbers and performing
an operation on them, giving another magic number.  For the most part
the resulting magic number is always 256fs (I suspect the cases where
they aren't should be 256fs).  Probably replacing all of this with

	pll_out = params_rate(params) * 256

would work.

> +	/* We block CODCLK from MCLK of WM8580 */
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, S3C64XX_CODCLKSRC_EXT,
> +					0, SND_SOC_CLOCK_IN);
> +	if (ret < 0)
> +		return ret;

The code is OK (modulo the issues with the previous patch) but the
comment is exceptionally difficult to parse.

> +	/* Explicitly set WM8580-ADC/DAC to source from MCLK */
> +	ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_ADC_CLKSEL,
> +					WM8580_CLKSRC_MCLK);

Obviously this will need updating for the actual patch for the WM8580
driver.

> +	if (ret < 0)
> +		return ret;
> +	ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_DAC_CLKSEL,

Missing blank line here.

> +	/* Don't output to the disconnected pin */
> +	ret = snd_soc_dai_set_clkdiv(codec_dai, WM8580_CLKOUTSRC,
> +					WM8580_CLKSRC_NONE);
> +	if (ret < 0)
> +		return ret;

If you're going to do this do it on init, it's nothing to do with
starting the audio stream.

> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_U8:
> +	case SNDRV_PCM_FORMAT_S8:
> +		bfs = 16;
> +		rfs = 256;
> +		break;
> +	case SNDRV_PCM_FORMAT_U16_LE:
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		bfs = 32;
> +		rfs = 256;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

This ought to get factored out into the CPU DAI driver, at least as a
library function - there's nothing board specific going on here.

> +	/* LineIn enabled by default */
> +	snd_soc_dapm_disable_pin(codec, "MicIn");
> +	snd_soc_dapm_enable_pin(codec, "LineIn");

Please add a comment explaining that if this is changed it needs to be
done with a resistor fit mod - this begs the question "How do I change
away from the default?", especially given the many switches for things
like this on the board.

For completeness if you're going to handle the mapping to the external
jacks there's also the possiblity that the signals will be switched out
to the PMIC card or WM9713.  It's probably better to just not bother,
it's not really adding anything since there's no jack detect or anything
on the board.  All that'll happen is that people are forced to rebuild
for DIP switch changes.

The other option would be to add controls mirroring the DIP switch
settings, but that'd not really cover the resistor fit stuff.

> +	/* Stereo enabled by default */
> +	snd_soc_dapm_enable_pin(codec, "Front-L/R");
> +	snd_soc_dapm_disable_pin(codec, "Center/Sub");
> +	snd_soc_dapm_disable_pin(codec, "Rear-L/R");

Similar issues here - putting a fixed configuration from this in the
driver doesn't buy anything.