[PATCH] cs4270: add support for slave mode configurations

[PATCH] cs4270: add support for slave mode configurations

[Daniel Mack]

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) {
-- 
1.6.1.3

Re: [PATCH] cs4270: add support for slave mode configurations

[Mark Brown]

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.

Re: [PATCH] cs4270: add support for slave mode configurations

[Daniel Mack]

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

Re: [PATCH] cs4270: add support for slave mode configurations

[Mark Brown]

On Wed, Feb 25, 2009 at 04:45:41PM +0100, Daniel Mack wrote:

> MODE_SLAVE is unset by the CS4270_MODE_SPEED_MASK mask:

Oh, fail.  Applied the patch.

Re: [PATCH] cs4270: add support for slave mode configurations

[Timur Tabi]

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;
> +       else
> +               reg |= 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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] cs4270: add support for slave mode configurations

[Daniel Mack]

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 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;
> > +       }
> > +

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;
> > +       else
> > +               reg |= 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

[PATCH] fix typo and removed unneeded switch case

[Daniel Mack]

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;
 	}
 
-- 
1.6.1.3

Re: [PATCH] cs4270: add support for slave mode configurations

[Timur Tabi]

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.


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] cs4270: add support for slave mode configurations

[Daniel Mack]

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

Re: [PATCH] cs4270: add support for slave mode configurations

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fix typo and removed unneeded switch case

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] fix typo and removed unneeded switch case

[Mark Brown]

On Sat, Feb 28, 2009 at 07:11:02PM -0600, Timur Tabi wrote:

> Mark, please add "cs4270:" to the summary when you commit this patch.

Applied (with an ASoC prefix and "for cs4270" suffix).