[alsa-devel] [PATCH] cs4270: add support for slave mode configurations
Added support for scenarios where the Cirrus CS4270 audio codec is slave to the bitclk and lrclk. Mixed setups are unsupported.
Signed-off-by: Daniel Mack daniel@caiaq.de --- sound/soc/codecs/cs4270.c | 24 +++++++++++++++++++++++- 1 files changed, 23 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index cd4a9ee..339e0f6 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -109,6 +109,7 @@ struct cs4270_private { u8 reg_cache[CS4270_NUMREGS]; unsigned int mclk; /* Input frequency of the MCLK pin */ unsigned int mode; /* The mode (I2S or left-justified) */ + unsigned int slave_mode; };
/** @@ -247,6 +248,7 @@ static int cs4270_set_dai_fmt(struct snd_soc_dai *codec_dai, struct cs4270_private *cs4270 = codec->private_data; int ret = 0;
+ /* set DAI format */ switch (format & SND_SOC_DAIFMT_FORMAT_MASK) { case SND_SOC_DAIFMT_I2S: case SND_SOC_DAIFMT_LEFT_J: @@ -257,6 +259,21 @@ static int cs4270_set_dai_fmt(struct snd_soc_dai *codec_dai, ret = -EINVAL; }
+ /* set master/slave audio interface */ + switch (format & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBS_CFS: + cs4270->slave_mode = 1; + break; + case SND_SOC_DAIFMT_CBM_CFM: + cs4270->slave_mode = 0; + break; + case SND_SOC_DAIFMT_CBM_CFS: + /* unsupported - cs4270 can eigther be slave or master to + * both the bitclk and the lrclk. */ + default: + ret = -EINVAL; + } + return ret; }
@@ -399,7 +416,12 @@ static int cs4270_hw_params(struct snd_pcm_substream *substream,
reg = snd_soc_read(codec, CS4270_MODE); reg &= ~(CS4270_MODE_SPEED_MASK | CS4270_MODE_DIV_MASK); - reg |= cs4270_mode_ratios[i].speed_mode | cs4270_mode_ratios[i].mclk; + reg |= cs4270_mode_ratios[i].mclk; + + if (cs4270->slave_mode) + reg |= CS4270_MODE_SLAVE; + else + reg |= cs4270_mode_ratios[i].speed_mode;
ret = snd_soc_write(codec, CS4270_MODE, reg); if (ret < 0) {
On Wed, Feb 25, 2009 at 02:37:21PM +0100, Daniel Mack wrote:
reg = snd_soc_read(codec, CS4270_MODE); reg &= ~(CS4270_MODE_SPEED_MASK | CS4270_MODE_DIV_MASK);
- reg |= cs4270_mode_ratios[i].speed_mode | cs4270_mode_ratios[i].mclk;
- reg |= cs4270_mode_ratios[i].mclk;
- if (cs4270->slave_mode)
reg |= CS4270_MODE_SLAVE;- else
reg |= cs4270_mode_ratios[i].speed_mode;
Shouldn't this be clearing MODE_SLAVE if it's in master mode? Since we're doing a read/modify/write here it'd probably just be as easy to set or clear the bit when setting the DAI format rather than storing the data and setting it here.
On Wed, Feb 25, 2009 at 03:34:45PM +0000, Mark Brown wrote:
On Wed, Feb 25, 2009 at 02:37:21PM +0100, Daniel Mack wrote:
reg = snd_soc_read(codec, CS4270_MODE); reg &= ~(CS4270_MODE_SPEED_MASK | CS4270_MODE_DIV_MASK);
- reg |= cs4270_mode_ratios[i].speed_mode | cs4270_mode_ratios[i].mclk;
- reg |= cs4270_mode_ratios[i].mclk;
- if (cs4270->slave_mode)
reg |= CS4270_MODE_SLAVE;- else
reg |= cs4270_mode_ratios[i].speed_mode;Shouldn't this be clearing MODE_SLAVE if it's in master mode?
MODE_SLAVE is unset by the CS4270_MODE_SPEED_MASK mask:
#define CS4270_MODE_SPEED_MASK 0x30 #define CS4270_MODE_SLAVE 0x30
Since we're doing a read/modify/write here it'd probably just be as easy to set or clear the bit when setting the DAI format rather than storing the data and setting it here.
I though so too, but the problem is that the bits that select the speed mode are the same that select the slave mode. Hence, if I write to the register in set_dai_fmt(), I'd have to add a special case in the other part again. Doing it this looked much cleaner to me. Does that make sense?
Daniel
On Wed, Feb 25, 2009 at 7:37 AM, Daniel Mack daniel@caiaq.de wrote:
Added support for scenarios where the Cirrus CS4270 audio codec is slave to the bitclk and lrclk. Mixed setups are unsupported.
Signed-off-by: Daniel Mack daniel@caiaq.de
FYI, it's customary to CC: the author of a driver when posting patches for it.
/* set master/slave audio interface */switch (format & SND_SOC_DAIFMT_MASTER_MASK) {case SND_SOC_DAIFMT_CBS_CFS:cs4270->slave_mode = 1;break;case SND_SOC_DAIFMT_CBM_CFM:cs4270->slave_mode = 0;break;case SND_SOC_DAIFMT_CBM_CFS:/* unsupported - cs4270 can eigther be slave or master to
Typo. However, I suggest you get rid of the "case SND_SOC_DAIFMT_CBM_CFS" and in the "default:", just have this:
default:
/* all other modes are unsupported by the hardware */
ret = -EINVAL;}
reg = snd_soc_read(codec, CS4270_MODE); reg &= ~(CS4270_MODE_SPEED_MASK | CS4270_MODE_DIV_MASK);
reg |= cs4270_mode_ratios[i].speed_mode | cs4270_mode_ratios[i].mclk;
reg |= cs4270_mode_ratios[i].mclk;if (cs4270->slave_mode)reg |= CS4270_MODE_SLAVE;elsereg |= cs4270_mode_ratios[i].speed_mode;
Are you sure that the mclk bits are still correct in slave mode? I'm looking at table 5 in the CS4270 manual, and it lists settings for 1x,2x,4x speed even though the register is set to slave mode instead.
On Fri, Feb 27, 2009 at 03:51:58PM -0600, Timur Tabi wrote:
/* set master/slave audio interface */switch (format & SND_SOC_DAIFMT_MASTER_MASK) {case SND_SOC_DAIFMT_CBS_CFS:cs4270->slave_mode = 1;break;case SND_SOC_DAIFMT_CBM_CFM:cs4270->slave_mode = 0;break;case SND_SOC_DAIFMT_CBM_CFS:/* unsupported - cs4270 can eigther be slave or master toTypo. However, I suggest you get rid of the "case SND_SOC_DAIFMT_CBM_CFS" and in the "default:", just have this:
default:/* all other modes are unsupported by the hardware */
ret = -EINVAL;}
Ok, I'll post a patch for this.
reg = snd_soc_read(codec, CS4270_MODE); reg &= ~(CS4270_MODE_SPEED_MASK | CS4270_MODE_DIV_MASK);
reg |= cs4270_mode_ratios[i].speed_mode | cs4270_mode_ratios[i].mclk;
reg |= cs4270_mode_ratios[i].mclk;if (cs4270->slave_mode)reg |= CS4270_MODE_SLAVE;elsereg |= cs4270_mode_ratios[i].speed_mode;Are you sure that the mclk bits are still correct in slave mode? I'm looking at table 5 in the CS4270 manual, and it lists settings for 1x,2x,4x speed even though the register is set to slave mode instead.
Hmm - mclk bits are set with 'reg |= cs4270_mode_ratios[i].mclk;' unconditionally, and both speed flags are used when in slave mode. Don't see where you suspect a flaw here?
Daniel
On Sat, Feb 28, 2009 at 6:13 AM, Daniel Mack daniel@caiaq.de wrote:
Hmm - mclk bits are set with 'reg |= cs4270_mode_ratios[i].mclk;' unconditionally, and both speed flags are used when in slave mode. Don't see where you suspect a flaw here?
I think the code is fine, my question is more about the behavior of the hardware. We were never able to get our board to run reliably with the cs4270 in slave mode, so I never gave it much thought. I was hoping you could explain why you still need to set the mclk bits in the mode register when you're in slave mode.
Hi Timur,
On Sat, Feb 28, 2009 at 10:32:30AM -0600, Timur Tabi wrote:
On Sat, Feb 28, 2009 at 6:13 AM, Daniel Mack daniel@caiaq.de wrote:
Hmm - mclk bits are set with 'reg |= cs4270_mode_ratios[i].mclk;' unconditionally, and both speed flags are used when in slave mode. Don't see where you suspect a flaw here?
I think the code is fine, my question is more about the behavior of the hardware. We were never able to get our board to run reliably with the cs4270 in slave mode, so I never gave it much thought. I was hoping you could explain why you still need to set the mclk bits in the mode register when you're in slave mode.
We use the CS4270 in many projects (external soundcards) and in some of them, we even link up to four of them together and put them all to slave mode which works perfectly fine. It's just the first time I use it on a Linux based board, though.
Setting the MCLKs for slave mode is probably not necessary (the datesheet says so, too), but it doesn't harm either. We're doing that for our other applications, so I copied the behaviour. In general, I can says that this codec is a very smoothly working piece of hardware, we never had any hazzle with it and use it for years now - what are the effects that you see?
Daniel
On Sat, Feb 28, 2009 at 11:47 AM, Daniel Mack daniel@caiaq.de wrote:
Setting the MCLKs for slave mode is probably not necessary (the datesheet says so, too), but it doesn't harm either. We're doing that for our other applications, so I copied the behaviour.
My only concern with that is that the code now implies that the mclk bits need to be set even in slave mode.
In general, I can says that this codec is a very smoothly working piece of hardware, we never had any hazzle with it and use it for years now - what are the effects that you see?
The codec itself seems to work fine. I don't know what the problem with our board was that required us to use the cs4270 in master mode. It was probably not worth the effort trying to make it work in slave mode.
This removes a misspelled comment and got rid of superfluous switch case.
Cc: Timur Tabi timur@freescale.com Signed-off-by: Daniel Mack daniel@caiaq.de --- sound/soc/codecs/cs4270.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 339e0f6..f86f33c 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -267,10 +267,8 @@ static int cs4270_set_dai_fmt(struct snd_soc_dai *codec_dai, case SND_SOC_DAIFMT_CBM_CFM: cs4270->slave_mode = 0; break; - case SND_SOC_DAIFMT_CBM_CFS: - /* unsupported - cs4270 can eigther be slave or master to - * both the bitclk and the lrclk. */ default: + /* all other modes are unsupported by the hardware */ ret = -EINVAL; }
On Sat, Feb 28, 2009 at 6:21 AM, Daniel Mack daniel@caiaq.de wrote:
This removes a misspelled comment and got rid of superfluous switch case.
Cc: Timur Tabi timur@freescale.com Signed-off-by: Daniel Mack daniel@caiaq.de
Acked-by: Timur Tabi timur@freescale.com
Mark, please add "cs4270:" to the summary when you commit this patch.
participants (4)
-
Daniel Mack -
Mark Brown -
Mark Brown -
Timur Tabi