Re: [alsa-devel] [PATCH v3] ASoC: da7210: Add support for PLL and SRM

[Mark Brown]

On Wed, Feb 01, 2012 at 04:55:29PM +0530, Ashish Chavan wrote:

> +static const u32 da7210_pll_div[][COL_CNT] = {
> +	/* for MASTER mode, fs = 44.1Khz */
> +	{ 12000000, 0xE8, 0x6C, 0x2, },		/* MCLK=12Mhz */

Even better than defines to index into the array would be an array of
structs with named members...

> +	if (da7210->mclk_rate) {
> +		/* PLL mode, disable PLL bypass */
> +		snd_soc_update_bits(codec, DA7210_PLL_DIV3, DA7210_PLL_BYP, 0);
> +	} else {
> +		/* PLL bypass mode, enable PLL bypass */
> +		snd_soc_update_bits(codec, DA7210_PLL_DIV3, DA7210_PLL_BYP,
> +							    DA7210_PLL_BYP);
> +	}

This is really weird - if anything it looks the wrong way round.
mclk_rate is what's set by set_sysclk() so if the user has configured
MCLK we will never use it even if it's at a suitable rate but if the
user hasn't configured one we rely on it directly.  If anything I'd
expect this code to enable the PLL only if the MCLK is not a suitable
rate.

The normal pattern is that set_sysclk() specifies the clock the bulk of
the device will be using, when used with the PLL that'd be the PLL
output not the PLL input.  Alternatively just specify the MCLK always
and let the driver figure out when to use the PLL.

> +	if (da7210->master) {
> +		/* In PLL master mode, use master mode PLL dividers */
> +		switch (fout) {
> +		case 2822400:
> +			row_idx = MASTER_2822400_DIV_OFFSET;
> +			break;
> +		case 3072000:
> +			row_idx = MASTER_3072000_DIV_OFFSET;
> +			break;
> +		default:
> +			dev_err(codec_dai->dev,
> +				"Unsupported PLL output frequency %d\n", fout);
> +			return -EINVAL;
> +		}
> +	} else {
> +		/* In PLL slave mode, use SRM mode PLL dividers */
> +		row_idx = SLAVE_SRM_DIV_OFFSET;
> +	}

You need checks elsewhere to make sure that the user doesn't try to
reconfigure master/slave while the PLL is active.

Attachment: signature.asc
Description: Digital signature