[PATCH] Add CLKOUT support to wm8731 codec driver

[PATCH] Add CLKOUT support to wm8731 codec driver

[Bill Gatliff]

The WM8731 Codec driver lacks support for utilizing the CLKOUT signal
as the I2S master clock.  The following patch adds that support, along
with a few minor whitespace cleanups.

Bill Gatliff (1):
  Add support for CLKOUT to wm8731 codec driver

 sound/soc/codecs/wm8731.c |  155 +++++++++++++++++++++++++++------------------
 sound/soc/codecs/wm8731.h |   20 ++++++-
 2 files changed, 113 insertions(+), 62 deletions(-)

[PATCH] Add support for CLKOUT to wm8731 codec driver

[Bill Gatliff]

Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
---
 sound/soc/codecs/wm8731.c |  165 +++++++++++++++++++++++++-------------------
 sound/soc/codecs/wm8731.h |   20 +++++-
 2 files changed, 113 insertions(+), 72 deletions(-)

diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 801b626..0e20956 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -29,6 +29,8 @@
 
 #include "wm8731.h"
 
+#define PREFIX "wm8731: "
+
 static struct snd_soc_codec *wm8731_codec;
 struct snd_soc_codec_device soc_codec_dev_wm8731;
 
@@ -50,16 +52,17 @@ static int wm8731_spi_write(struct spi_device *spi, const char *data, int len);
  * There is no point in caching the reset register
  */
 static const u16 wm8731_reg[WM8731_CACHEREGNUM] = {
-    0x0097, 0x0097, 0x0079, 0x0079,
-    0x000a, 0x0008, 0x009f, 0x000a,
-    0x0000, 0x0000
+	0x0097, 0x0097, 0x0079, 0x0079,
+	0x000a, 0x0008, 0x009f, 0x000a,
+	0x0000, 0x0000
 };
 
 /*
  * read wm8731 register cache
  */
-static inline unsigned int wm8731_read_reg_cache(struct snd_soc_codec *codec,
-	unsigned int reg)
+static inline unsigned int
+wm8731_read_reg_cache(struct snd_soc_codec *codec,
+		      unsigned int reg)
 {
 	u16 *cache = codec->reg_cache;
 	if (reg == WM8731_RESET)
@@ -72,8 +75,9 @@ static inline unsigned int wm8731_read_reg_cache(struct snd_soc_codec *codec,
 /*
  * write wm8731 register cache
  */
-static inline void wm8731_write_reg_cache(struct snd_soc_codec *codec,
-	u16 reg, unsigned int value)
+static inline void
+wm8731_write_reg_cache(struct snd_soc_codec *codec,
+		       u16 reg, unsigned int value)
 {
 	u16 *cache = codec->reg_cache;
 	if (reg >= WM8731_CACHEREGNUM)
@@ -84,11 +88,13 @@ static inline void wm8731_write_reg_cache(struct snd_soc_codec *codec,
 /*
  * write to the WM8731 register space
  */
-static int wm8731_write(struct snd_soc_codec *codec, unsigned int reg,
-	unsigned int value)
+static inline int
+wm8731_write(struct snd_soc_codec *codec,
+	     unsigned int reg, unsigned int value)
 {
 	u8 data[2];
 
+
 	/* data is
 	 *   D15..D9 WM8731 register offset
 	 *   D8...D0 register data
@@ -115,52 +121,52 @@ static const struct soc_enum wm8731_enum[] = {
 
 static const struct snd_kcontrol_new wm8731_snd_controls[] = {
 
-SOC_DOUBLE_R("Master Playback Volume", WM8731_LOUT1V, WM8731_ROUT1V,
-	0, 127, 0),
-SOC_DOUBLE_R("Master Playback ZC Switch", WM8731_LOUT1V, WM8731_ROUT1V,
-	7, 1, 0),
+	SOC_DOUBLE_R("Master Playback Volume", WM8731_LOUT1V, WM8731_ROUT1V,
+		     0, 127, 0),
+	SOC_DOUBLE_R("Master Playback ZC Switch", WM8731_LOUT1V, WM8731_ROUT1V,
+		     7, 1, 0),
 
-SOC_DOUBLE_R("Capture Volume", WM8731_LINVOL, WM8731_RINVOL, 0, 31, 0),
-SOC_DOUBLE_R("Line Capture Switch", WM8731_LINVOL, WM8731_RINVOL, 7, 1, 1),
+	SOC_DOUBLE_R("Capture Volume", WM8731_LINVOL, WM8731_RINVOL, 0, 31, 0),
+	SOC_DOUBLE_R("Line Capture Switch", WM8731_LINVOL, WM8731_RINVOL, 7, 1, 1),
 
-SOC_SINGLE("Mic Boost (+20dB)", WM8731_APANA, 0, 1, 0),
-SOC_SINGLE("Capture Mic Switch", WM8731_APANA, 1, 1, 1),
+	SOC_SINGLE("Mic Boost (+20dB)", WM8731_APANA, 0, 1, 0),
+	SOC_SINGLE("Capture Mic Switch", WM8731_APANA, 1, 1, 1),
 
-SOC_SINGLE("Sidetone Playback Volume", WM8731_APANA, 6, 3, 1),
+	SOC_SINGLE("Sidetone Playback Volume", WM8731_APANA, 6, 3, 1),
 
-SOC_SINGLE("ADC High Pass Filter Switch", WM8731_APDIGI, 0, 1, 1),
-SOC_SINGLE("Store DC Offset Switch", WM8731_APDIGI, 4, 1, 0),
+	SOC_SINGLE("ADC High Pass Filter Switch", WM8731_APDIGI, 0, 1, 1),
+	SOC_SINGLE("Store DC Offset Switch", WM8731_APDIGI, 4, 1, 0),
 
-SOC_ENUM("Playback De-emphasis", wm8731_enum[1]),
+	SOC_ENUM("Playback De-emphasis", wm8731_enum[1]),
 };
 
 /* Output Mixer */
 static const struct snd_kcontrol_new wm8731_output_mixer_controls[] = {
-SOC_DAPM_SINGLE("Line Bypass Switch", WM8731_APANA, 3, 1, 0),
-SOC_DAPM_SINGLE("Mic Sidetone Switch", WM8731_APANA, 5, 1, 0),
-SOC_DAPM_SINGLE("HiFi Playback Switch", WM8731_APANA, 4, 1, 0),
+	SOC_DAPM_SINGLE("Line Bypass Switch", WM8731_APANA, WM8731_APANA_BYPASS, 1, 0),
+	SOC_DAPM_SINGLE("Mic Sidetone Switch", WM8731_APANA, WM8731_APANA_SIDETONE, 1, 0),
+	SOC_DAPM_SINGLE("HiFi Playback Switch", WM8731_APANA, WM8731_APANA_DACSEL, 1, 0),
 };
 
 /* Input mux */
 static const struct snd_kcontrol_new wm8731_input_mux_controls =
-SOC_DAPM_ENUM("Input Select", wm8731_enum[0]);
+	SOC_DAPM_ENUM("Input Select", wm8731_enum[0]);
 
 static const struct snd_soc_dapm_widget wm8731_dapm_widgets[] = {
-SND_SOC_DAPM_MIXER("Output Mixer", WM8731_PWR, 4, 1,
-	&wm8731_output_mixer_controls[0],
-	ARRAY_SIZE(wm8731_output_mixer_controls)),
-SND_SOC_DAPM_DAC("DAC", "HiFi Playback", WM8731_PWR, 3, 1),
-SND_SOC_DAPM_OUTPUT("LOUT"),
-SND_SOC_DAPM_OUTPUT("LHPOUT"),
-SND_SOC_DAPM_OUTPUT("ROUT"),
-SND_SOC_DAPM_OUTPUT("RHPOUT"),
-SND_SOC_DAPM_ADC("ADC", "HiFi Capture", WM8731_PWR, 2, 1),
-SND_SOC_DAPM_MUX("Input Mux", SND_SOC_NOPM, 0, 0, &wm8731_input_mux_controls),
-SND_SOC_DAPM_PGA("Line Input", WM8731_PWR, 0, 1, NULL, 0),
-SND_SOC_DAPM_MICBIAS("Mic Bias", WM8731_PWR, 1, 1),
-SND_SOC_DAPM_INPUT("MICIN"),
-SND_SOC_DAPM_INPUT("RLINEIN"),
-SND_SOC_DAPM_INPUT("LLINEIN"),
+	SND_SOC_DAPM_MIXER("Output Mixer", WM8731_PWR, 4, 1,
+			   &wm8731_output_mixer_controls[0],
+			   ARRAY_SIZE(wm8731_output_mixer_controls)),
+	SND_SOC_DAPM_DAC("DAC", "HiFi Playback", WM8731_PWR, 3, 1),
+	SND_SOC_DAPM_OUTPUT("LOUT"),
+	SND_SOC_DAPM_OUTPUT("LHPOUT"),
+	SND_SOC_DAPM_OUTPUT("ROUT"),
+	SND_SOC_DAPM_OUTPUT("RHPOUT"),
+	SND_SOC_DAPM_ADC("ADC", "HiFi Capture", WM8731_PWR, 2, 1),
+	SND_SOC_DAPM_MUX("Input Mux", SND_SOC_NOPM, 0, 0, &wm8731_input_mux_controls),
+	SND_SOC_DAPM_PGA("Line Input", WM8731_PWR, 0, 1, NULL, 0),
+	SND_SOC_DAPM_MICBIAS("Mic Bias", WM8731_PWR, 1, 1),
+	SND_SOC_DAPM_INPUT("MICIN"),
+	SND_SOC_DAPM_INPUT("RLINEIN"),
+	SND_SOC_DAPM_INPUT("LLINEIN"),
 };
 
 static const struct snd_soc_dapm_route intercon[] = {
@@ -265,8 +271,6 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
 	u16 srate = (coeff_div[i].sr << 2) |
 		(coeff_div[i].bosr << 1) | coeff_div[i].usb;
 
-	wm8731_write(codec, WM8731_SRATE, srate);
-
 	/* bit size */
 	switch (params_format(params)) {
 	case SNDRV_PCM_FORMAT_S16_LE:
@@ -277,8 +281,13 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S24_LE:
 		iface |= 0x0008;
 		break;
+	default:
+		pr_err("%s: unrecognized format %x\n",
+		       __func__, params_format(params));
+		return -EINVAL;
 	}
 
+	wm8731_write(codec, WM8731_SRATE, srate);
 	wm8731_write(codec, WM8731_IFACE, iface);
 	return 0;
 }
@@ -323,10 +332,13 @@ static int wm8731_mute(struct snd_soc_dai *dai, int mute)
 }
 
 static int wm8731_set_dai_sysclk(struct snd_soc_dai *codec_dai,
-		int clk_id, unsigned int freq, int dir)
+				 int clk_id, unsigned int freq, int dir)
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
 	struct wm8731_priv *wm8731 = codec->private_data;
+	u16 reg;
+
+	pr_debug(PREFIX "%s\n", __func__);
 
 	switch (freq) {
 	case 11289600:
@@ -335,18 +347,35 @@ static int wm8731_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	case 16934400:
 	case 18432000:
 		wm8731->sysclk = freq;
-		return 0;
+		break;
+	default:
+		return -EINVAL;
 	}
-	return -EINVAL;
+
+	if (dir == SND_SOC_CLOCK_OUT) {
+		dev_dbg(codec->dev, PREFIX "%s turning on CLKOUT\n", __func__);
+		reg = wm8731_read_reg_cache(codec, WM8731_PWR);
+		reg &= ~(1 << WM8731_PWR_CLKOUTPD);
+		wm8731_write(codec, WM8731_PWR, reg);
+	}
+	else {
+		dev_dbg(codec->dev, PREFIX "%s turning off CLKOUT\n", __func__);
+		reg = wm8731_read_reg_cache(codec, WM8731_PWR);
+		reg |= (1 << WM8731_PWR_CLKOUTPD);
+		wm8731_write(codec, WM8731_PWR, reg);
+	}
+	return 0;
 }
 
 
 static int wm8731_set_dai_fmt(struct snd_soc_dai *codec_dai,
-		unsigned int fmt)
+			      unsigned int fmt)
 {
 	struct snd_soc_codec *codec = codec_dai->codec;
 	u16 iface = 0;
 
+	pr_debug(PREFIX "%s\n", __func__);
+
 	/* set master/slave audio interface */
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
 	case SND_SOC_DAIFMT_CBM_CFM:
@@ -405,25 +434,20 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
 {
 	u16 reg;
 
+	pr_debug(PREFIX "%s\n", __func__);
+
 	switch (level) {
 	case SND_SOC_BIAS_ON:
-#warning "TODO: figure out how to turn on/off CLKOUT properly"
-		wm8731_write(codec, WM8731_PWR, 0);
-		break;
 	case SND_SOC_BIAS_PREPARE:
-#if 1
-#warning "TODO: figure out how to turn on/off CLKOUT properly"
-	  /* probably has to do with SND_SOC_CLOCK_OUT
-	     and whether we're master or slave ... */
-		wm8731_write(codec, WM8731_PWR, 0);
-#endif
-		break;
 	case SND_SOC_BIAS_STANDBY:
-#if 1
-		/* Clear PWROFF, gate CLKOUT, everything else as-is */
+		/* power up the part, but leave other power settings alone */
 		reg = wm8731_read_reg_cache(codec, WM8731_PWR) & 0xff7f;
-		wm8731_write(codec, WM8731_PWR, reg | 0x0040);
-#endif
+
+		/* turn off CLKOUT in STANDBY */
+		if (level == SND_SOC_BIAS_STANDBY)
+			reg |= (1 << WM8731_PWR_CLKOUTPD);
+
+		wm8731_write(codec, WM8731_PWR, reg);
 		break;
 	case SND_SOC_BIAS_OFF:
 		wm8731_write(codec, WM8731_ACTIVE, 0x0);
@@ -434,14 +458,14 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
 	return 0;
 }
 
-#define WM8731_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)
+#define WM8731_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)
 
-#define WM8731_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
-	SNDRV_PCM_FMTBIT_S24_LE)
+#define WM8731_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE | \
+			SNDRV_PCM_FMTBIT_S24_LE)
 
 static struct snd_soc_dai_ops wm8731_dai_ops = {
 	.prepare	= wm8731_pcm_prepare,
@@ -608,8 +632,7 @@ static int wm8731_register(struct wm8731_priv *wm8731)
 
 	/* Disable bypass path by default */
 	reg = wm8731_read_reg_cache(codec, WM8731_APANA);
-	wm8731_write(codec, WM8731_APANA, reg & ~0x4);
-
+	wm8731_write(codec, WM8731_APANA, reg & ~(1 << WM8731_APANA_BYPASS));
 	wm8731_codec = codec;
 
 	ret = snd_soc_register_codec(codec);
@@ -753,14 +776,14 @@ static int __init wm8731_modinit(void)
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 	ret = i2c_add_driver(&wm8731_i2c_driver);
 	if (ret != 0) {
-		printk(KERN_ERR "Failed to register WM8731 I2C driver: %d\n",
+		pr_err("Failed to register WM8731 I2C driver: %d\n",
 		       ret);
 	}
 #endif
 #if defined(CONFIG_SPI_MASTER)
 	ret = spi_register_driver(&wm8731_spi_driver);
 	if (ret != 0) {
-		printk(KERN_ERR "Failed to register WM8731 SPI driver: %d\n",
+		pr_err("Failed to register WM8731 SPI driver: %d\n",
 		       ret);
 	}
 #endif
diff --git a/sound/soc/codecs/wm8731.h b/sound/soc/codecs/wm8731.h
index cd7b806..2e77393 100644
--- a/sound/soc/codecs/wm8731.h
+++ b/sound/soc/codecs/wm8731.h
@@ -31,9 +31,27 @@
 
 #define WM8731_CACHEREGNUM 	10
 
-#define WM8731_SYSCLK	0
+#define WM8731_SYSCLK		0
 #define WM8731_DAI		0
 
+#define WM8731_APANA_MICBOOST	0
+#define WM8731_APANA_MUTEMIC	1
+#define WM8731_APANA_INSEL	2
+#define WM8731_APANA_BYPASS	3
+#define WM8731_APANA_DACSEL	4
+#define WM8731_APANA_SIDETONE	5
+
+#define WM8731_PWR_LINEINPD	0
+#define WM8731_PWR_MICPD	1
+#define WM8731_PWR_ADCPD	2
+#define WM8731_PWR_DACPD	3
+#define WM8731_PWR_OUTPD	4
+#define WM8731_PWR_OSCPD	5
+#define WM8731_PWR_CLKOUTPD	6
+#define WM8731_PWR_POWEROFF	7
+
+
+
 extern struct snd_soc_dai wm8731_dai;
 extern struct snd_soc_codec_device soc_codec_dev_wm8731;
 
-- 
1.5.6.5

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Mark Brown]

On Thu, Oct 29, 2009 at 10:13:22AM -0500, Bill Gatliff wrote:
> 
> Signed-off-by: Bill Gatliff <bgat@billgatliff.com>

Something seems to have got mixed up with the submission here - the
changelog and patch appear to have been detached from each other and
there are quite a few other issues below.  I suspect finger trouble
somewhere along the line.  Please also remember to CC maintainers on
patches to help avoid them getting lost in the mailing list.

> +#define PREFIX "wm8731: "
> +

Use dev_printk() functions, don't introduce things like this.

>   * There is no point in caching the reset register
>   */
>  static const u16 wm8731_reg[WM8731_CACHEREGNUM] = {
> -    0x0097, 0x0097, 0x0079, 0x0079,
> -    0x000a, 0x0008, 0x009f, 0x000a,
> -    0x0000, 0x0000
> +	0x0097, 0x0097, 0x0079, 0x0079,
> +	0x000a, 0x0008, 0x009f, 0x000a,
> +	0x0000, 0x0000

Almost all of the patch actually appears to consist of a large number of
unrelated formatting and stylistic changes like this which aren't
mentioned in your changelog and making it hard for me to review.  Please
rework this as a series of patches, each making a single change.

>  static const struct snd_soc_dapm_route intercon[] = {
> @@ -265,8 +271,6 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
>  	u16 srate = (coeff_div[i].sr << 2) |
>  		(coeff_div[i].bosr << 1) | coeff_div[i].usb;
>  
> -	wm8731_write(codec, WM8731_SRATE, srate);
> -
>  	/* bit size */
>  	switch (params_format(params)) {
>  	case SNDRV_PCM_FORMAT_S16_LE:

This is a separate functional change - it needs to be in a patch by
itself.

> +	if (dir == SND_SOC_CLOCK_OUT) {
> +		dev_dbg(codec->dev, PREFIX "%s turning on CLKOUT\n", __func__);
> +		reg = wm8731_read_reg_cache(codec, WM8731_PWR);
> +		reg &= ~(1 << WM8731_PWR_CLKOUTPD);
> +		wm8731_write(codec, WM8731_PWR, reg);
> +	}
> +	else {

} else { please - I'm surprised checkpatch dosn't warn.

> +		dev_dbg(codec->dev, PREFIX "%s turning off CLKOUT\n", __func__);
> +		reg = wm8731_read_reg_cache(codec, WM8731_PWR);
> +		reg |= (1 << WM8731_PWR_CLKOUTPD);
> +		wm8731_write(codec, WM8731_PWR, reg);
> +	}

It'd be better define a new clock for the CLKOUT pin rather than munging
it in with the master clock.  MCLK is always an input on the WM8731, the
CLKOUT output is a separate pin and so including it in MCLK is likely to
make things confusing and could well introduce errors.

> -#warning "TODO: figure out how to turn on/off CLKOUT properly"
> -		wm8731_write(codec, WM8731_PWR, 0);
> -		break;
>  	case SND_SOC_BIAS_PREPARE:
> -#if 1
> -#warning "TODO: figure out how to turn on/off CLKOUT properly"
> -	  /* probably has to do with SND_SOC_CLOCK_OUT
> -	     and whether we're master or slave ... */
> -		wm8731_write(codec, WM8731_PWR, 0);
> -#endif
> -		break;

This patch isn't against mainline...

> +		/* turn off CLKOUT in STANDBY */
> +		if (level == SND_SOC_BIAS_STANDBY)
> +			reg |= (1 << WM8731_PWR_CLKOUTPD);
> +
> +		wm8731_write(codec, WM8731_PWR, reg);

If you're putting CLKOUT control in the hands of machine drivers give
them complete control over it, mixing management between the machine
drivers and the CODEC driver is likely to cause confusion.

> -#define WM8731_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)
> +#define WM8731_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)

SDNDRV_PCM_RATE_8000_96000.

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Bill Gatliff]

Mark Brown wrote:
> On Thu, Oct 29, 2009 at 10:13:22AM -0500, Bill Gatliff wrote:
>   
>> Signed-off-by: Bill Gatliff <bgat@billgatliff.com>
>>     
>
> Something seems to have got mixed up with the submission here - the
> changelog and patch appear to have been detached from each other and
> there are quite a few other issues below.  I suspect finger trouble
> somewhere along the line.  Please also remember to CC maintainers on
> patches to help avoid them getting lost in the mailing list.
>   

Tell you what, just disregard the whole patch series.  I have no idea
why git chose to send it in the state that it did.  Definitely not what
I would want to submit.

Sorry for the noise, I'll send a real patch series as soon as I can.


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Bill Gatliff]

Mark Brown wrote:
>
>> +		dev_dbg(codec->dev, PREFIX "%s turning off CLKOUT\n", __func__);
>> +		reg = wm8731_read_reg_cache(codec, WM8731_PWR);
>> +		reg |= (1 << WM8731_PWR_CLKOUTPD);
>> +		wm8731_write(codec, WM8731_PWR, reg);
>> +	}
>>     
>
> It'd be better define a new clock for the CLKOUT pin rather than munging
> it in with the master clock.  MCLK is always an input on the WM8731, the
> CLKOUT output is a separate pin and so including it in MCLK is likely to
> make things confusing and could well introduce errors.
>   

Could you explain more about what you meant by this?  I'm trying to
rework these patches now, and I don't understand how to implement what
you are suggesting.


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Mark Brown]

On Fri, Oct 30, 2009 at 11:50:20AM -0500, Bill Gatliff wrote:
> Mark Brown wrote:

> > It'd be better define a new clock for the CLKOUT pin rather than munging
> > it in with the master clock.  MCLK is always an input on the WM8731, the
> > CLKOUT output is a separate pin and so including it in MCLK is likely to
> > make things confusing and could well introduce errors.

> Could you explain more about what you meant by this?  I'm trying to
> rework these patches now, and I don't understand how to implement what
> you are suggesting.

Define two clock IDs and then use a switch statement in the function to
determine which to act on, allowing the master clock to be controlled
separately to the output clock.

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Bill Gatliff]

Mark Brown wrote:
> On Fri, Oct 30, 2009 at 11:50:20AM -0500, Bill Gatliff wrote:
>   
>> Mark Brown wrote:
>>     
>
>   
>>> It'd be better define a new clock for the CLKOUT pin rather than munging
>>> it in with the master clock.  MCLK is always an input on the WM8731, the
>>> CLKOUT output is a separate pin and so including it in MCLK is likely to
>>> make things confusing and could well introduce errors.
>>>       
>
>   
>> Could you explain more about what you meant by this?  I'm trying to
>> rework these patches now, and I don't understand how to implement what
>> you are suggesting.
>>     
>
> Define two clock IDs and then use a switch statement in the function to
> determine which to act on, allowing the master clock to be controlled
> separately to the output clock.
>   

Can you refer me to an existing example?


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Mark Brown]

On 30 Oct 2009, at 18:22, Bill Gatliff <bgat@billgatliff.com> wrote:

> Mark Brown wrote:
>> On Fri, Oct 30, 2009 at 11:50:20AM -0500, Bill Gatliff wrote:
>>
>>> Mark Brown wrote:
>>>
>>
>>
>>>> It'd be better define a new clock for the CLKOUT pin rather than  
>>>> munging
>>>> it in with the master clock.  MCLK is always an input on the  
>>>> WM8731, the
>>>> CLKOUT output is a separate pin and so including it in MCLK is  
>>>> likely to
>>>> make things confusing and could well introduce errors.
>>>>
>>
>>
>>> Could you explain more about what you meant by this?  I'm trying to
>>> rework these patches now, and I don't understand how to implement  
>>> what
>>> you are suggesting.
>>>
>>
>> Define two clock IDs and then use a switch statement in the  
>> function to
>> determine which to act on, allowing the master clock to be controlled
>> separately to the output clock.
>>
>
> Can you refer me to an existing example?

wm8993 supports multiple internal options for sysclk usig the id  
parameter. Could you be more explicit about what you can't follow here  
please? I might be able to help more if I could see what's confusing  
you.

>
>
> b.g.
>
> -- 
> Bill Gatliff
> bgat@billgatliff.com
>

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Bill Gatliff]

Mark Brown wrote:
> wm8993 supports multiple internal options for sysclk usig the id
> parameter. Could you be more explicit about what you can't follow here
> please? I might be able to help more if I could see what's confusing you.

I guess what it comes down to is, I have no idea what I'm doing as far
as the Alsa SoC APIs are concerned.  I understand fully how the hardware
works, but not any of the SND_SOC_* macros.  I'm lost!  :)

I see in the wm8993 code the following:

static const struct snd_soc_dapm_widget wm8993_dapm_widgets[] = {
SND_SOC_DAPM_SUPPLY("CLK_SYS", WM8993_BUS_CONTROL_1, 1, 0, clk_sys_event,
                    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
...

And I also have looked at the clk_sys_event function in the same file. 
And then farther down:

static const struct snd_soc_dapm_route routes[] = {
...
        { "DACL", NULL, "CLK_SYS" },


I guess that the updates to the wm8731 would somehow look like the
above?  I don't mind looking at code and macros at all, I'm just not yet
seeing how all this stuff fits together.


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [PATCH] Add support for CLKOUT to wm8731 codec driver

[Mark Brown]

On Fri, Oct 30, 2009 at 02:04:25PM -0500, Bill Gatliff wrote:
> Mark Brown wrote:

> > wm8993 supports multiple internal options for sysclk usig the id
> > parameter. Could you be more explicit about what you can't follow here
> > please? I might be able to help more if I could see what's confusing you.

> I guess what it comes down to is, I have no idea what I'm doing as far
> as the Alsa SoC APIs are concerned.  I understand fully how the hardware
> works, but not any of the SND_SOC_* macros.  I'm lost!  :)

All I said to do was to take your code to set this via the set_sysclk()
function and use the id that gets passed in to split up the changes to
the master clock and the output clock so the two are adjusted
independently.

> I see in the wm8993 code the following:

> static const struct snd_soc_dapm_widget wm8993_dapm_widgets[] = {
> SND_SOC_DAPM_SUPPLY("CLK_SYS", WM8993_BUS_CONTROL_1, 1, 0, clk_sys_event,
>                     SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> ...

This is used to switch on and off the internal system clock in the
CODEC which supplies the clock to all the digital functionaliy in the
device, it's not something that's visible outside the device.  The
wm8731 is a much older device and doesn't have this sort of detailed
control.