[PATCH v3] ASoC: da7210: Add support for PLL and SRM

[PATCH v3] ASoC: da7210: Add support for PLL and SRM

[Ashish Chavan]

Current DA7210 driver does support PLL mode fully. It uses fixed
value of input master clock and PLL mode is enabled and disabled based
on the sampling frequency being used for playback or recording. It also
doesn't support Sample Rate Measurement feature of DA7210 hardware.

This patch adds full support for PLL and SRM. Basically following three
modes of operation are possible for DA7210 hardware,

(1) I2S SLAVE mode with PLL bypassed
(2) I2S SLAVE mode with PLL enabled
(3) I2S Master mode with PLL enabled

This patch adds support for all three modes. Also, in case of SLAVE mode
with PLL, it supports SRM (Sample Rate Measurement) feature of the chip.

This patch has been tested on DA7210 EVB with Samsung SMDK6410 board.

Signed-off-by: Ashish Chavan <ashish.chavan@kpitcummins.com>
Signed-off-by: David Dajun Chen <dchen@diasemi.com>
---
Changes since v2:
- Combined three tables of pll dividers in to one to minimize duplicate
code
- Revamped logic to fetch appropriate pll dividers
- Replaced magic numbers with proper defines

Changes since v1:
- Removed unnecessary function to configure clock dividers
- Changed data layout and code related to PLL dividers. Tried making it
more readable and robust
---
 sound/soc/codecs/da7210.c |  230 ++++++++++++++++++++++++++++++++++++---------
 1 files changed, 187 insertions(+), 43 deletions(-)

diff --git a/sound/soc/codecs/da7210.c b/sound/soc/codecs/da7210.c
index ab38e93..f7dd409 100644
--- a/sound/soc/codecs/da7210.c
+++ b/sound/soc/codecs/da7210.c
@@ -145,6 +145,7 @@
 #define DA7210_DAI_EN			(1 << 7)
 
 /*PLL_DIV3 bit fields */
+#define DA7210_PLL_DIV_L_MASK		(0xF << 0)
 #define DA7210_MCLK_RANGE_10_20_MHZ	(1 << 4)
 #define DA7210_PLL_BYP			(1 << 6)
 
@@ -161,12 +162,16 @@
 #define DA7210_PLL_FS_48000		(0xB << 0)
 #define DA7210_PLL_FS_88200		(0xE << 0)
 #define DA7210_PLL_FS_96000		(0xF << 0)
+#define DA7210_MCLK_DET_EN		(0x1 << 5)
+#define DA7210_MCLK_SRM_EN		(0x1 << 6)
 #define DA7210_PLL_EN			(0x1 << 7)
 
 /* SOFTMUTE bit fields */
 #define DA7210_RAMP_EN			(1 << 6)
 
 /* CONTROL bit fields */
+#define DA7210_REG_EN			(1 << 0)
+#define DA7210_BIAS_EN			(1 << 2)
 #define DA7210_NOISE_SUP_EN		(1 << 3)
 
 /* IN_GAIN bit fields */
@@ -205,6 +210,56 @@
 #define DA7210_OUT2_OUTMIX_L		(1 << 6)
 #define DA7210_OUT2_EN			(1 << 7)
 
+/* Index fields for PLL divider array */
+#define FREF_IDX		(0 << 0)
+#define DIV1_IDX		(1 << 0)
+#define DIV2_IDX		(2 << 0)
+#define DIV3_IDX		(3 << 0)
+
+/* Number of PLL input frequencies supported */
+#define FREF_CNT		(7 << 0)
+/* Number of PLL dividers for each frequency */
+#define DIV_CNT			(3 << 0)
+/* Number of columns in PLL divider table */
+#define COL_CNT			(DIV_CNT + 1)
+
+/* Index of first relevant row in PLL divider table */
+#define MASTER_2822400_DIV_OFFSET	0
+#define MASTER_3072000_DIV_OFFSET	(MASTER_2822400_DIV_OFFSET + FREF_CNT)
+#define SLAVE_SRM_DIV_OFFSET		(MASTER_3072000_DIV_OFFSET + FREF_CNT)
+
+/* PLL dividers table */
+static const u32 da7210_pll_div[][COL_CNT] = {
+	/* for MASTER mode, fs = 44.1Khz */
+	{ 12000000, 0xE8, 0x6C, 0x2, },		/* MCLK=12Mhz */
+	{ 13000000, 0xDF, 0x28, 0xC, },		/* MCLK=13Mhz */
+	{ 13500000, 0xDB, 0x0A, 0xD, },		/* MCLK=13.5Mhz */
+	{ 14400000, 0xD4, 0x5A, 0x2, },		/* MCLK=14.4Mhz */
+	{ 19200000, 0xBB, 0x43, 0x9, },		/* MCLK=19.2Mhz */
+	{ 19680000, 0xB9, 0x6D, 0xA, },		/* MCLK=19.68Mhz */
+	{ 19800000, 0xB8, 0xFB, 0xB, },		/* MCLK=19.8Mhz */
+	/* for MASTER mode, fs = 48Khz */
+	{ 12000000, 0xF3, 0x12, 0x7, },		/* MCLK=12Mhz */
+	{ 13000000, 0xE8, 0xFD, 0x5, },		/* MCLK=13Mhz */
+	{ 13500000, 0xE4, 0x82, 0x3, },		/* MCLK=13.5Mhz */
+	{ 14400000, 0xDD, 0x3A, 0x0, },		/* MCLK=14.4Mhz */
+	{ 19200000, 0xC1, 0xEB, 0x8, },		/* MCLK=19.2Mhz */
+	{ 19680000, 0xBF, 0xEC, 0x0, },		/* MCLK=19.68Mhz */
+	{ 19800000, 0xBF, 0x70, 0x0, },		/* MCLK=19.8Mhz */
+	/* for SLAVE mode with SRM */
+	{ 12000000, 0xED, 0xBF, 0x5, },		/* MCLK=12Mhz */
+	{ 13000000, 0xE4, 0x13, 0x0, },		/* MCLK=13Mhz */
+	{ 13500000, 0xDF, 0xC6, 0x8, },		/* MCLK=13.5Mhz */
+	{ 14400000, 0xD8, 0xCA, 0x1, },		/* MCLK=14.4Mhz */
+	{ 19200000, 0xBE, 0x97, 0x9, },		/* MCLK=19.2Mhz */
+	{ 19680000, 0xBC, 0xAC, 0xD, },		/* MCLK=19.68Mhz */
+	{ 19800000, 0xBC, 0x35, 0xE, },		/* MCLK=19.8Mhz  */
+};
+
+enum clk_src {
+	DA7210_CLKSRC_MCLK
+};
+
 #define DA7210_VERSION "0.0.1"
 
 /*
@@ -627,6 +682,8 @@ static const struct snd_soc_dapm_route da7210_audio_map[] = {
 /* Codec private data */
 struct da7210_priv {
 	enum snd_soc_control_type control_type;
+	unsigned int mclk_rate;
+	int master;
 };
 
 /*
@@ -673,8 +730,9 @@ static int da7210_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct snd_soc_codec *codec = rtd->codec;
+	struct da7210_priv *da7210 = snd_soc_codec_get_drvdata(codec);
 	u32 dai_cfg1;
-	u32 fs, bypass;
+	u32 fs;
 
 	/* set DAI source to Left and Right ADC */
 	snd_soc_write(codec, DA7210_DAI_SRC_SEL,
@@ -707,43 +765,33 @@ static int da7210_hw_params(struct snd_pcm_substream *substream,
 	switch (params_rate(params)) {
 	case 8000:
 		fs		= DA7210_PLL_FS_8000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	case 11025:
 		fs		= DA7210_PLL_FS_11025;
-		bypass		= 0;
 		break;
 	case 12000:
 		fs		= DA7210_PLL_FS_12000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	case 16000:
 		fs		= DA7210_PLL_FS_16000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	case 22050:
 		fs		= DA7210_PLL_FS_22050;
-		bypass		= 0;
 		break;
 	case 32000:
 		fs		= DA7210_PLL_FS_32000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	case 44100:
 		fs		= DA7210_PLL_FS_44100;
-		bypass		= 0;
 		break;
 	case 48000:
 		fs		= DA7210_PLL_FS_48000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	case 88200:
 		fs		= DA7210_PLL_FS_88200;
-		bypass		= 0;
 		break;
 	case 96000:
 		fs		= DA7210_PLL_FS_96000;
-		bypass		= DA7210_PLL_BYP;
 		break;
 	default:
 		return -EINVAL;
@@ -753,8 +801,15 @@ static int da7210_hw_params(struct snd_pcm_substream *substream,
 	snd_soc_update_bits(codec, DA7210_STARTUP1, DA7210_SC_MST_EN, 0);
 
 	snd_soc_update_bits(codec, DA7210_PLL, DA7210_PLL_FS_MASK, fs);
-	snd_soc_update_bits(codec, DA7210_PLL_DIV3, DA7210_PLL_BYP, bypass);
 
+	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);
+	}
 	/* Enable active mode */
 	snd_soc_update_bits(codec, DA7210_STARTUP1,
 			    DA7210_SC_MST_EN, DA7210_SC_MST_EN);
@@ -768,6 +823,7 @@ static int da7210_hw_params(struct snd_pcm_substream *substream,
 static int da7210_set_dai_fmt(struct snd_soc_dai *codec_dai, u32 fmt)
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
+	struct da7210_priv *da7210 = snd_soc_codec_get_drvdata(codec);
 	u32 dai_cfg1;
 	u32 dai_cfg3;
 
@@ -776,9 +832,11 @@ static int da7210_set_dai_fmt(struct snd_soc_dai *codec_dai, u32 fmt)
 
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBM_CFM:
+		da7210->master = 1;
 		dai_cfg1 |= DA7210_DAI_MODE_MASTER;
 		break;
 	case SND_SOC_DAIFMT_CBS_CFS:
+		da7210->master = 0;
 		dai_cfg1 |= DA7210_DAI_MODE_SLAVE;
 		break;
 	default:
@@ -830,10 +888,120 @@ static int da7210_mute(struct snd_soc_dai *dai, int mute)
 #define DA7210_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
+static int da7210_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+				 int clk_id, unsigned int freq, int dir)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct da7210_priv *da7210 = snd_soc_codec_get_drvdata(codec);
+
+	switch (clk_id) {
+	case DA7210_CLKSRC_MCLK:
+		switch (freq) {
+		case 12000000:
+		case 13000000:
+		case 13500000:
+		case 14400000:
+		case 19200000:
+		case 19680000:
+		case 19800000:
+			da7210->mclk_rate = freq;
+			return 0;
+		default:
+			dev_err(codec_dai->dev, "Unsupported MCLK value %d\n",
+				freq);
+			return -EINVAL;
+		}
+		break;
+	default:
+		dev_err(codec_dai->dev, "Unknown clock source %d\n", clk_id);
+		return -EINVAL;
+	}
+}
+
+/**
+ * da7210_set_dai_pll	:Configure the codec PLL
+ * @param codec_dai	: pointer to codec DAI
+ * @param pll_id	: da7210 has only one pll, so pll_id is always zero
+ * @param fref		: MCLK frequency, should be < 20MHz
+ * @param fout		: FsDM value, Refer page 44 & 45 of datasheet
+ * @return int		: Zero for success, negative error code for error
+ *
+ * Note: Supported PLL input frequencies are 12MHz, 13MHz, 13.5MHz, 14.4MHz,
+ *       19.2MHz, 19.6MHz and 19.8MHz
+ */
+static int da7210_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
+			      int source, unsigned int fref, unsigned int fout)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	struct da7210_priv *da7210 = snd_soc_codec_get_drvdata(codec);
+
+	u8 pll_div1, pll_div2, pll_div3, row_idx, cnt;
+
+	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;
+	}
+
+	for (cnt = 0; cnt < FREF_CNT; cnt++) {
+		if (fref == da7210_pll_div[row_idx + cnt][FREF_IDX]) {
+			pll_div1 = da7210_pll_div[row_idx + cnt][DIV1_IDX];
+			pll_div2 = da7210_pll_div[row_idx + cnt][DIV2_IDX];
+			pll_div3 = da7210_pll_div[row_idx + cnt][DIV3_IDX];
+			break;
+		}
+	}
+	if (cnt >= FREF_CNT)
+		goto err;
+
+	/* Disable active mode */
+	snd_soc_update_bits(codec, DA7210_STARTUP1, DA7210_SC_MST_EN, 0);
+	/* Write PLL dividers */
+	snd_soc_write(codec, DA7210_PLL_DIV1, pll_div1);
+	snd_soc_write(codec, DA7210_PLL_DIV2, pll_div2);
+	snd_soc_update_bits(codec, DA7210_PLL_DIV3,
+				   DA7210_PLL_DIV_L_MASK, pll_div3);
+
+	if (da7210->master) {
+		/* In master mode, no need to enable SRM */
+		snd_soc_update_bits(codec, DA7210_PLL, DA7210_PLL_EN,
+						       DA7210_PLL_EN);
+	} else {
+		/* In slave mode, enable SRM and PLL */
+		snd_soc_update_bits(codec, DA7210_PLL,
+				   (DA7210_PLL_EN | DA7210_MCLK_SRM_EN |
+						     DA7210_MCLK_DET_EN),
+				   (DA7210_PLL_EN | DA7210_MCLK_SRM_EN |
+						    DA7210_MCLK_DET_EN));
+	}
+	/* Enable active mode */
+	snd_soc_update_bits(codec, DA7210_STARTUP1, DA7210_SC_MST_EN,
+						    DA7210_SC_MST_EN);
+	return 0;
+err:
+	dev_err(codec_dai->dev, "Unsupported PLL input frequency %d\n", fref);
+	return -EINVAL;
+}
+
 /* DAI operations */
 static const struct snd_soc_dai_ops da7210_dai_ops = {
 	.hw_params	= da7210_hw_params,
 	.set_fmt	= da7210_set_dai_fmt,
+	.set_sysclk	= da7210_set_dai_sysclk,
+	.set_pll	= da7210_set_dai_pll,
 	.digital_mute	= da7210_mute,
 };
 
@@ -872,24 +1040,13 @@ static int da7210_probe(struct snd_soc_codec *codec)
 		return ret;
 	}
 
-	/* FIXME
-	 *
-	 * This driver use fixed value here
-	 * And below settings expects MCLK = 12.288MHz
-	 *
-	 * When you select different MCLK, please check...
-	 *      DA7210_PLL_DIV1 val
-	 *      DA7210_PLL_DIV2 val
-	 *      DA7210_PLL_DIV3 val
-	 *      DA7210_PLL_DIV3 :: DA7210_MCLK_RANGExxx
-	 */
+	da7210->mclk_rate	= 0;	/* This will be set from set_sysclk() */
+	da7210->master		= 0;	/* This will be set from set_fmt() */
+
+	/* Enable internal regulator & bias current */
+	snd_soc_write(codec, DA7210_CONTROL, DA7210_REG_EN | DA7210_BIAS_EN);
 
-	/*
-	 * make sure that DA7210 use bypass mode before start up
-	 */
 	snd_soc_write(codec, DA7210_STARTUP1, 0);
-	snd_soc_write(codec, DA7210_PLL_DIV3,
-		     DA7210_MCLK_RANGE_10_20_MHZ | DA7210_PLL_BYP);
 
 	/*
 	 * ADC settings
@@ -964,24 +1121,11 @@ static int da7210_probe(struct snd_soc_codec *codec)
 	/* Enable Aux2 */
 	snd_soc_write(codec, DA7210_AUX2, DA7210_AUX2_EN);
 
+	/* Set PLL Master clock range 10-20 MHz */
+	snd_soc_write(codec, DA7210_PLL_DIV3, DA7210_MCLK_RANGE_10_20_MHZ);
 	/* Diable PLL and bypass it */
 	snd_soc_write(codec, DA7210_PLL, DA7210_PLL_FS_48000);
 
-	/*
-	 * If 48kHz sound came, it use bypass mode,
-	 * and when it is 44.1kHz, it use PLL.
-	 *
-	 * This time, this driver sets PLL always ON
-	 * and controls bypass/PLL mode by switching
-	 * DA7210_PLL_DIV3 :: DA7210_PLL_BYP bit.
-	 *   see da7210_hw_params
-	 */
-	snd_soc_write(codec, DA7210_PLL_DIV1, 0xE5); /* MCLK = 12.288MHz */
-	snd_soc_write(codec, DA7210_PLL_DIV2, 0x99);
-	snd_soc_write(codec, DA7210_PLL_DIV3, 0x0A |
-		     DA7210_MCLK_RANGE_10_20_MHZ | DA7210_PLL_BYP);
-	snd_soc_update_bits(codec, DA7210_PLL, DA7210_PLL_EN, DA7210_PLL_EN);
-
 	/* As suggested by Dialog */
 	snd_soc_write(codec, DA7210_A_HID_UNLOCK,	0x8B); /* unlock */
 	snd_soc_write(codec, DA7210_A_TEST_UNLOCK,	0xB4);
-- 
1.7.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Mark Brown]

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

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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

[Ashish Chavan]

On Wed, 2012-02-01 at 11:50 +0000, Mark Brown wrote:
> 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...
> 

OK, will do that.

> > +	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.
> 

Actually it worked fine for me because of the way platform driver is
written. It basically uses static configurations for PLL and PLL bypass
modes. It calls set_sysclk() only in case of PLL. So the codec driver
more or less uses mckl_rate as a flag to see if PLL is enabled. I know
it's not clean and may be I can update it to comply with rest of the
stuff.

> 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.

Yes, got the point.

> 
> > +	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.

AFAIK master/slave configuration is static(depending on the board
configuration) and done via set_fmt() from hw_params() of platform
driver. Can you please point me to an example where dynamic/runtime
setting of I2S master/slave mode is supported?

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On Tue, Feb 07, 2012 at 06:55:49PM +0530, Ashish Chavan wrote:
> On Wed, 2012-02-01 at 11:50 +0000, Mark Brown wrote:
> > On Wed, Feb 01, 2012 at 04:55:29PM +0530, Ashish Chavan wrote:

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

> AFAIK master/slave configuration is static(depending on the board
> configuration) and done via set_fmt() from hw_params() of platform
> driver. Can you please point me to an example where dynamic/runtime
> setting of I2S master/slave mode is supported?

The most common case is things like bluetooth SCO connections where if
there's a baseband you want to clock from the baseband but if there's no
baseband (eg, for VoIP calls) you can't do that.  Some devices also need
to change audio interface configuration depening on things like the
number of channels.

Even without any dynamic reconfiguration you also have cases where the
sequence people use to power things up is to set the clocking and then
configure the audio interface format - even if the audio interface
format is always the same you'll still get it being reconfigured the
first time you start up.

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

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

[Mark Brown]

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

On Tue, Feb 07, 2012 at 06:55:49PM +0530, Ashish Chavan wrote:
> On Wed, 2012-02-01 at 11:50 +0000, Mark Brown wrote:
> > On Wed, Feb 01, 2012 at 04:55:29PM +0530, Ashish Chavan wrote:

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

> AFAIK master/slave configuration is static(depending on the board
> configuration) and done via set_fmt() from hw_params() of platform
> driver. Can you please point me to an example where dynamic/runtime
> setting of I2S master/slave mode is supported?

The most common case is things like bluetooth SCO connections where if
there's a baseband you want to clock from the baseband but if there's no
baseband (eg, for VoIP calls) you can't do that.  Some devices also need
to change audio interface configuration depening on things like the
number of channels.

Even without any dynamic reconfiguration you also have cases where the
sequence people use to power things up is to set the clocking and then
configure the audio interface format - even if the audio interface
format is always the same you'll still get it being reconfigured the
first time you start up.

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

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

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

[Ashish Chavan]

> The most common case is things like bluetooth SCO connections where if
> there's a baseband you want to clock from the baseband but if there's no
> baseband (eg, for VoIP calls) you can't do that.  Some devices also need
> to change audio interface configuration depening on things like the
> number of channels.
> 
> Even without any dynamic reconfiguration you also have cases where the
> sequence people use to power things up is to set the clocking and then
> configure the audio interface format - even if the audio interface
> format is always the same you'll still get it being reconfigured the
> first time you start up.

I see.

BTW I am also planning to add SPI support to this driver. Just want to
know if it should be done via reg-map APIs? If yes, then do I need to
convert existing I2C stuff too to use reg-map?

TIA!

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On Thu, Feb 09, 2012 at 06:12:20PM +0530, Ashish Chavan wrote:

> BTW I am also planning to add SPI support to this driver. Just want to
> know if it should be done via reg-map APIs? If yes, then do I need to
> convert existing I2C stuff too to use reg-map?

Yes, please.  The idea is that all of the I2C and SPI drivers will get
converted over.

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

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

[Mark Brown]

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

On Thu, Feb 09, 2012 at 06:12:20PM +0530, Ashish Chavan wrote:

> BTW I am also planning to add SPI support to this driver. Just want to
> know if it should be done via reg-map APIs? If yes, then do I need to
> convert existing I2C stuff too to use reg-map?

Yes, please.  The idea is that all of the I2C and SPI drivers will get
converted over.

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

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