Re: [PATCH 2/3] ASoC: Ux500: Add machine-driver

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

[-- Attachment #1.1: Type: text/plain, Size: 2398 bytes --]

On Thu, Jun 07, 2012 at 02:00:37PM +0200, Ola Lilja wrote:

> +	mop500_ab8500_remove(mop500_card);
> +
> +	snd_soc_unregister_card(mop500_card);

These seem the wrong way round - we're starting to tear down the card
before we've destroyed the core and hence userspace so something might
still be using it.

> +	platform_set_drvdata(pdev, NULL);

No need to do this, it's a waste of time.

> +static inline const char *get_mclk_str(enum mclk mclk_sel)
> +{
> +	switch (mclk_sel) {
> +	case MCLK_SYSCLK:
> +		return "MCLK_SYSCLK";
> +	case MCLK_ULPCLK:
> +		return "MCLK_ULPCLK";

Why not just drop the MCLK_ from the strings?  I'd expect that'll be
implied by the context...

> +static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct mop500_ab8500_drvdata *drvdata =
> +				snd_soc_card_get_drvdata(codec->card);
> +	unsigned int val;
> +
> +	val = (ucontrol->value.enumerated.item[0] != 0);

Given that this is an enumeration not a bool we should be rejecting out
of bounds values rather than silently fixing them up, this would also
avoid issues if we do add new options for the clock later on for some
reason.

> +	/* Digital interface - Clocks */
> +	SOC_SINGLE("Digital Interface Master Generator Switch",
> +		AB8500_DIGIFCONF1, AB8500_DIGIFCONF1_ENMASTGEN,
> +		1, 0),

This enables and disables a 256fs (or similar ratio) clock output from
the device?

> +	SOC_SINGLE("Digital Interface 0 Bit-clock Switch",
> +		AB8500_DIGIFCONF1, AB8500_DIGIFCONF1_ENFSBITCLK0,
> +		1, 0),
> +	SOC_SINGLE("Digital Interface 1 Bit-clock Switch",
> +		AB8500_DIGIFCONF1, AB8500_DIGIFCONF1_ENFSBITCLK1,
> +		1, 0),

I suspect that these are doing what DAIFMT_CONT is for...

> +int mop500_ab8500_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +
> +	/* Set audio-clock source */
> +	return mop500_ab8500_set_mclk(rtd->card->dev,
> +				snd_soc_card_get_drvdata(rtd->card));

Should we be blocking MCLK changes while the interface is active?
> +	/* Setup codec depending on driver-mode */
> +	driver_mode = (channels == 8) ?
> +		DRIVERMODE_CODEC_ONLY : DRIVERMODE_NORMAL;

Ugh, just use an if or switch statement.

> +	sw_codec = (driver_mode == DRIVERMODE_NORMAL) ? sw_cpu : 20;

Again, if statement please.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]