[PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Timur Tabi]

Use snd_pcm_format_width() to determine the sample size, instead of
checking specify sample formats and assuming that those are the only
valid format.

This change adds support for big-endian architectures (which use the _BE
formats) and the packed 24-bit format (SNDRV_PCM_FORMAT_S24_3xE).

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/codecs/wm8776.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
index 8e7953b..ad6f0fa 100644
--- a/sound/soc/codecs/wm8776.c
+++ b/sound/soc/codecs/wm8776.c
@@ -215,8 +215,6 @@ static int wm8776_hw_params(struct snd_pcm_substream *substream,
 	int ratio_shift, master;
 	int i;
 
-	iface = 0;
-
 	switch (dai->driver->id) {
 	case WM8776_DAI_DAC:
 		iface_reg = WM8776_DACIFCTRL;
@@ -232,20 +230,23 @@ static int wm8776_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-
 	/* Set word length */
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
+	i = snd_pcm_format_width(params_format(params));
+	switch (i) {
+	case 16:
+		iface = 0;
+	case 20:
+		iface = 0x10;
 		break;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-		iface |= 0x10;
+	case 24:
+		iface = 0x20;
 		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		iface |= 0x20;
-		break;
-	case SNDRV_PCM_FORMAT_S32_LE:
-		iface |= 0x30;
+	case 32:
+		iface = 0x30;
 		break;
+	default:
+		dev_err(codec->dev, "Unsupported sample size: %i\n", i);
+		return -EINVAL;
 	}
 
 	/* Only need to set MCLK/LRCLK ratio if we're master */
-- 
1.7.3.4

[PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

ASoC codec drivers can use the .set_sysclk function to dynamically specify
the list of support sample rates, because that list is often based on
the input clock frequency.  Although the WM8776 includes a .set_sysclk
function, it was also hard-coding the supported sample rates to a list
that depends on a specific input clock frequency.

So change the hard-coded list to a range within the capabilities of the
WM8776 itself, and let wm8776_set_sysclk() do its job.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/codecs/wm8776.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
index ad6f0fa..ca8a593 100644
--- a/sound/soc/codecs/wm8776.c
+++ b/sound/soc/codecs/wm8776.c
@@ -321,11 +321,6 @@ static int wm8776_set_bias_level(struct snd_soc_codec *codec,
 	return 0;
 }
 
-#define WM8776_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
-		      SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
-		      SNDRV_PCM_RATE_96000)
-
-
 #define WM8776_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE |\
 			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
 
@@ -350,7 +345,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
 			.stream_name = "Playback",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = WM8776_RATES,
+			.rates = SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min = 32000,
+			.rate_max = 192000,
 			.formats = WM8776_FORMATS,
 		},
 		.ops = &wm8776_dac_ops,
@@ -362,7 +359,9 @@ static struct snd_soc_dai_driver wm8776_dai[] = {
 			.stream_name = "Capture",
 			.channels_min = 2,
 			.channels_max = 2,
-			.rates = WM8776_RATES,
+			.rates = SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min = 32000,
+			.rate_max = 96000,
 			.formats = WM8776_FORMATS,
 		},
 		.ops = &wm8776_adc_ops,
-- 
1.7.3.4

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Liam Girdwood]

On Tue, 2011-09-13 at 12:59 -0500, Timur Tabi wrote:
> ASoC codec drivers can use the .set_sysclk function to dynamically specify
> the list of support sample rates, because that list is often based on
> the input clock frequency.  Although the WM8776 includes a .set_sysclk
> function, it was also hard-coding the supported sample rates to a list
> that depends on a specific input clock frequency.
> 
> So change the hard-coded list to a range within the capabilities of the
> WM8776 itself, and let wm8776_set_sysclk() do its job.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Tue, Sep 13, 2011 at 12:59:36PM -0500, Timur Tabi wrote:

> ASoC codec drivers can use the .set_sysclk function to dynamically specify
> the list of support sample rates, because that list is often based on
> the input clock frequency.  Although the WM8776 includes a .set_sysclk
> function, it was also hard-coding the supported sample rates to a list
> that depends on a specific input clock frequency.

> So change the hard-coded list to a range within the capabilities of the
> WM8776 itself, and let wm8776_set_sysclk() do its job.

This changelog doesn't correspond to reality.  The set_sysclk() function
in the driver makes no effort to constrain the sample rates based on
sysclk, as is normal for CODEC drivers as the system clock is frequently
configured based on the current sample rate (at the minute the
configured clock is used to set up the clock dividers within the CODEC
based on sample rate).  Trying to implement constraints based on the
system clock is problematic and will normally decrease the usability of
the driver in systems where the clock rates vary.

What's actually going on here is that the driver is being cautious about
supporting non-audio clock rates (mostly because the digital performance
is mainly specified for audio rates) and 192kHz was omitted from the DAC
rates.  The change itself is OK but please resubmit with a more accurate
changelog.

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:

> This changelog doesn't correspond to reality.  The set_sysclk() function
> in the driver makes no effort to constrain the sample rates based on
> sysclk, as is normal for CODEC drivers as the system clock is frequently
> configured based on the current sample rate (at the minute the
> configured clock is used to set up the clock dividers within the CODEC
> based on sample rate). 

My understanding of the .set_sysclk() function is that one of its primary
purpose is to tell the codec what master frequency to use for its sample rate
clock divider.  Although that doesn't technically happen in the .set_sysclk
function itself, that function is typically required.  Without that function,
codecs must hard-code their mclk frequency, which therefore also locks the list
of supported sample rates.

So although I understand that the patch description may not be 100% accurate, I
think saying that it "doesn't correspond to reality" is an exaggeration.  I can
correct the description by changing a few words around.

> Trying to implement constraints based on the
> system clock is problematic and will normally decrease the usability of
> the driver in systems where the clock rates vary.

I don't think I understand that.  set_sysclk() is suppose to increase the
usability of the driver where clock rates vary because it is *the* mechanism for
telling the driver what the clock rate is.  This is how it goes:

1. The machine driver queries the platform to determine the mclk for the codec
2. The machine driver uses set_sysclk() to tell the codec driver what that
frequency is.
3. The codec driver later uses that frequency to determine which sample rates
are actually supported.

How is this problematic?  On the P1022DS, I can dynamically switch between two
mclk frequencies on the codec just by touching a gpio.

> What's actually going on here is that the driver is being cautious about
> supporting non-audio clock rates (mostly because the digital performance
> is mainly specified for audio rates) and 192kHz was omitted from the DAC
> rates.  The change itself is OK but please resubmit with a more accurate
> changelog.

The only change that I understand that you want is to clarify that the sample
rate calculation doesn't happen directly in set_sysclk().  Is there anything
else you're expecting?


-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Thu, Sep 15, 2011 at 10:08:29AM -0500, Timur Tabi wrote:
> Mark Brown wrote:

> > This changelog doesn't correspond to reality.  The set_sysclk() function
> > in the driver makes no effort to constrain the sample rates based on
> > sysclk, as is normal for CODEC drivers as the system clock is frequently
> > configured based on the current sample rate (at the minute the
> > configured clock is used to set up the clock dividers within the CODEC
> > based on sample rate). 

> My understanding of the .set_sysclk() function is that one of its primary
> purpose is to tell the codec what master frequency to use for its sample rate
> clock divider.  Although that doesn't technically happen in the .set_sysclk
> function itself, that function is typically required.  Without that function,
> codecs must hard-code their mclk frequency, which therefore also locks the list
> of supported sample rates.

You're missing the point here.  Your changelog says:

| ASoC codec drivers can use the .set_sysclk function to dynamically specify
| the list of support sample rates, because that list is often based on
| the input clock frequency.  Although the WM8776 includes a .set_sysclk

This isn't what happens at all.  The constraints set in the DAIs
generally just list all the sample rates the device can possibly
support, there's no dynamic information injected into the subsystem
about what's supported.  This is because in many systems the various
clock rates are dynamically controlled and so the clocks are adjusted to
reflect the sample rates the application layer wants.  As a result we
never actually bother specifying the supported rates for the current
clock at all, we just try to make the best of what we're given when it
comes to configuring which is a rather different thing.

There are actually a few drivers that do try to specify the rates they
support based on the sysclk but these are generally more difficult to
use due to the system integration issues that result when the sysclk is
varied.

| function, it was also hard-coding the supported sample rates to a list
| that depends on a specific input clock frequency.

This isn't the case; for example the current sample rate list includes
both 44.1kHz and 48kHz (and other 8kHz based rates) which can't be
generated from the same system clock.

> 3. The codec driver later uses that frequency to determine which sample rates
> are actually supported.

This is the bit that doesn't actually happen - what the drivers actually
do is take a stab at using the clocks they have to configure themselves
when they need to do so.  Any errors that are generated are a byproduct
and they happen very late on in the process and depending on the idiom
we may end up making a best effort rather than actually doing what we're
being asked to do rather than rejecting invalid configurations.

> How is this problematic?  On the P1022DS, I can dynamically switch between two
> mclk frequencies on the codec just by touching a gpio.

It's not a problem to do this, the problem is purely with the changelog.
All you're actually doing is increasing the range of supported rates in
the driver, the discussion of both the current code and the interaction
with set_sysclk() is based on a misunderstanding of how this works.

> > What's actually going on here is that the driver is being cautious about
> > supporting non-audio clock rates (mostly because the digital performance
> > is mainly specified for audio rates) and 192kHz was omitted from the DAC
> > rates.  The change itself is OK but please resubmit with a more accurate
> > changelog.

> The only change that I understand that you want is to clarify that the sample
> rate calculation doesn't happen directly in set_sysclk().  Is there anything
> else you're expecting?

Hopefully the above makes things clearer - all you're doing here is
expanding the range of supported sample rates, the diagnosis of the
problem is substantially off base.  Given that the changelogs are the
main documentation I'd rather not see stuff like this going in there.

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Tabi Timur-B04825]

Mark Brown wrote:

> This isn't what happens at all.  The constraints set in the DAIs
> generally just list all the sample rates the device can possibly
> support, there's no dynamic information injected into the subsystem
> about what's supported.  This is because in many systems the various
> clock rates are dynamically controlled and so the clocks are adjusted to
> reflect the sample rates the application layer wants.

IMHO, these two sentences contradict each other.

> As a result we
> never actually bother specifying the supported rates for the current
> clock at all, we just try to make the best of what we're given when it
> comes to configuring which is a rather different thing.

But why would you do that?  That just creates an artificial limitation on 
the list of supported sample rates.  If you include a set_sysclk() 
function in the codec driver, then you should always specify 
SNDRV_PCM_RATE_CONTINUOUS in snd_soc_dai_driver.rates.  To me, the two go 
hand-in-hand.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Thu, Sep 15, 2011 at 11:06:53PM +0000, Tabi Timur-B04825 wrote:
> Mark Brown wrote:

> > This isn't what happens at all.  The constraints set in the DAIs
> > generally just list all the sample rates the device can possibly
> > support, there's no dynamic information injected into the subsystem
> > about what's supported.  This is because in many systems the various
> > clock rates are dynamically controlled and so the clocks are adjusted to
> > reflect the sample rates the application layer wants.

> IMHO, these two sentences contradict each other.

The first one says that the rates announced by the driver are the
superset of all rates the device can ever support, the second says that
this is done because the system can change the clock rate after we're
done announcing what sample rates we support.

> > As a result we
> > never actually bother specifying the supported rates for the current
> > clock at all, we just try to make the best of what we're given when it
> > comes to configuring which is a rather different thing.

> But why would you do that?  That just creates an artificial limitation on 
> the list of supported sample rates.  If you include a set_sysclk() 
> function in the codec driver, then you should always specify 
> SNDRV_PCM_RATE_CONTINUOUS in snd_soc_dai_driver.rates.  To me, the two go 
> hand-in-hand.

Devices, especially ones with lots of digital in them, often support a
series of specific sample rates rather than a continuous range of sample
rates.  While you can of course configure them with other setups this
will run them out of spec which may cause issues from poor performance
up to serious audio issues or misdriving of outputs (eg, class D speaker
drivers or things using charge pumps).  Most of the time things will be
OK but in general it seems better to try to stay within spec.

Fairly simple devices like the WM8776 which don't contain much digital
are commonly supported over a continuous range of sample rates so
_CONTINUOUS is suitable for them.

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Timur Tabi]

Mark Brown wrote:
> The first one says that the rates announced by the driver are the
> superset of all rates the device can ever support, the second says that
> this is done because the system can change the clock rate after we're
> done announcing what sample rates we support.

Ok, I understand what you're saying.  In fact, it seems this change, while it
allows the codec driver to automatically support all of the sample rates that
the hardware actually supports at the moment, it has the drawback in that it
doesn't tell ASoC what that list is.  All .hw_params() does is accept or reject
any particular sample rate.

Is there a way for set_syclk() to tell ASoC the full list of supported rates?
That is, is there any way it can update snd_soc_dai_driver.playback.rates?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/3] ASoC: support all possible sample rates in the WM8776 driver

[Mark Brown]

On Fri, Sep 16, 2011 at 08:48:46AM -0500, Timur Tabi wrote:

> Ok, I understand what you're saying.  In fact, it seems this change, while it
> allows the codec driver to automatically support all of the sample rates that
> the hardware actually supports at the moment, it has the drawback in that it
> doesn't tell ASoC what that list is.  All .hw_params() does is accept or reject
> any particular sample rate.

Yes.

> Is there a way for set_syclk() to tell ASoC the full list of supported rates?
> That is, is there any way it can update snd_soc_dai_driver.playback.rates?

No, you can't modify the DAI dynamically.  You can set constraints in
the startup() callback as the WM8988 driver does.

[PATCH 3/3] ASoC: improve asynchronous mode support in the fsl_ssi driver

[Timur Tabi]

The Freescale SSI audio controller supports "synchronous" and "asynchronous"
modes.  In synchronous mode, playback and capture use the same input clock,
so sample rates must be the same during simultaneous playback and capture.
Unfortunately, the code which supports asynchronous mode is just broken in
various ways.  In particular, it was constraining sample sizes as well as
the sample rate.

The fix also allows us to simplify the code by eliminating the 'asynchronous',
'playback', and 'capture' variables that were used to keep track of playback
and capture streams.

Unfortunately, it turns out that simulataneous playback and record does not
actually work on the only platform that supports asynchronous mode: the
Freescale P1022DS reference board.  If a second stream is started, the SSI
grinds to halt for both streams.  This is true even if the P1022 is configured
for synchronous mode, so it's likely a hardware problem that needs to be
worked around.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/fsl/fsl_ssi.c |  145 ++++++++++++++++++++++-------------------------
 1 files changed, 68 insertions(+), 77 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 06ac2b9..0268cf9 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -78,7 +78,6 @@
  * @second_stream: pointer to second stream
  * @playback: the number of playback streams opened
  * @capture: the number of capture streams opened
- * @asynchronous: 0=synchronous mode, 1=asynchronous mode
  * @cpu_dai: the CPU DAI for this device
  * @dev_attr: the sysfs device attribute structure
  * @stats: SSI statistics
@@ -90,9 +89,6 @@ struct fsl_ssi_private {
 	unsigned int irq;
 	struct snd_pcm_substream *first_stream;
 	struct snd_pcm_substream *second_stream;
-	unsigned int playback;
-	unsigned int capture;
-	int asynchronous;
 	unsigned int fifo_depth;
 	struct snd_soc_dai_driver cpu_dai_drv;
 	struct device_attribute dev_attr;
@@ -281,15 +277,19 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 			   struct snd_soc_dai *dai)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai);
+	struct fsl_ssi_private *ssi_private =
+		snd_soc_dai_get_drvdata(rtd->cpu_dai);
+	int synchronous = ssi_private->cpu_dai_drv.symmetric_rates;
 
 	/*
 	 * If this is the first stream opened, then request the IRQ
 	 * and initialize the SSI registers.
 	 */
-	if (!ssi_private->playback && !ssi_private->capture) {
+	if (!ssi_private->first_stream) {
 		struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
 
+		ssi_private->first_stream = substream;
+
 		/*
 		 * Section 16.5 of the MPC8610 reference manual says that the
 		 * SSI needs to be disabled before updating the registers we set
@@ -306,7 +306,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 		clrsetbits_be32(&ssi->scr,
 			CCSR_SSI_SCR_I2S_MODE_MASK | CCSR_SSI_SCR_SYN,
 			CCSR_SSI_SCR_TFR_CLK_DIS | CCSR_SSI_SCR_I2S_MODE_SLAVE
-			| (ssi_private->asynchronous ? 0 : CCSR_SSI_SCR_SYN));
+			| (synchronous ? CCSR_SSI_SCR_SYN : 0));
 
 		out_be32(&ssi->stcr,
 			 CCSR_SSI_STCR_TXBIT0 | CCSR_SSI_STCR_TFEN0 |
@@ -323,7 +323,7 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 		 * master.
 		 */
 
-		/* 4. Enable the interrupts and DMA requests */
+		/* Enable the interrupts and DMA requests */
 		out_be32(&ssi->sier, SIER_FLAGS);
 
 		/*
@@ -352,58 +352,47 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
 		 * this is bad is because at this point, the PCM driver has not
 		 * finished initializing the DMA controller.
 		 */
-	}
-
-	if (!ssi_private->first_stream)
-		ssi_private->first_stream = substream;
-	else {
-		/* This is the second stream open, so we need to impose sample
-		 * rate and maybe sample size constraints.  Note that this can
-		 * cause a race condition if the second stream is opened before
-		 * the first stream is fully initialized.
-		 *
-		 * We provide some protection by checking to make sure the first
-		 * stream is initialized, but it's not perfect.  ALSA sometimes
-		 * re-initializes the driver with a different sample rate or
-		 * size.  If the second stream is opened before the first stream
-		 * has received its final parameters, then the second stream may
-		 * be constrained to the wrong sample rate or size.
-		 *
-		 * FIXME: This code does not handle opening and closing streams
-		 * repeatedly.  If you open two streams and then close the first
-		 * one, you may not be able to open another stream until you
-		 * close the second one as well.
-		 */
-		struct snd_pcm_runtime *first_runtime =
-			ssi_private->first_stream->runtime;
-
-		if (!first_runtime->sample_bits) {
-			dev_err(substream->pcm->card->dev,
-				"set sample size in %s stream first\n",
-				substream->stream == SNDRV_PCM_STREAM_PLAYBACK
-				? "capture" : "playback");
-			return -EAGAIN;
-		}
+	} else {
+		if (synchronous) {
+			struct snd_pcm_runtime *first_runtime =
+				ssi_private->first_stream->runtime;
+			/*
+			 * This is the second stream open, and we're in
+			 * synchronous mode, so we need to impose sample
+			 * sample size constraints. This is because STCCR is
+			 * used for playback and capture in synchronous mode,
+			 * so there's no way to specify different word
+			 * lengths.
+			 *
+			 * Note that this can cause a race condition if the
+			 * second stream is opened before the first stream is
+			 * fully initialized.  We provide some protection by
+			 * checking to make sure the first stream is
+			 * initialized, but it's not perfect.  ALSA sometimes
+			 * re-initializes the driver with a different sample
+			 * rate or size.  If the second stream is opened
+			 * before the first stream has received its final
+			 * parameters, then the second stream may be
+			 * constrained to the wrong sample rate or size.
+			 */
+			if (!first_runtime->sample_bits) {
+				dev_err(substream->pcm->card->dev,
+					"set sample size in %s stream first\n",
+					substream->stream ==
+					SNDRV_PCM_STREAM_PLAYBACK
+					? "capture" : "playback");
+				return -EAGAIN;
+			}
 
-		/* If we're in synchronous mode, then we need to constrain
-		 * the sample size as well.  We don't support independent sample
-		 * rates in asynchronous mode.
-		 */
-		if (!ssi_private->asynchronous)
 			snd_pcm_hw_constraint_minmax(substream->runtime,
 				SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
 				first_runtime->sample_bits,
 				first_runtime->sample_bits);
+		}
 
 		ssi_private->second_stream = substream;
 	}
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		ssi_private->playback++;
-
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-		ssi_private->capture++;
-
 	return 0;
 }
 
@@ -424,24 +413,35 @@ static int fsl_ssi_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *cpu_dai)
 {
 	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(cpu_dai);
+	struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
+	unsigned int sample_size =
+		snd_pcm_format_width(params_format(hw_params));
+	u32 wl = CCSR_SSI_SxCCR_WL(sample_size);
+	int enabled = in_be32(&ssi->scr) & CCSR_SSI_SCR_SSIEN;
 
-	if (substream == ssi_private->first_stream) {
-		struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
-		unsigned int sample_size =
-			snd_pcm_format_width(params_format(hw_params));
-		u32 wl = CCSR_SSI_SxCCR_WL(sample_size);
+	/*
+	 * If we're in synchronous mode, and the SSI is already enabled,
+	 * then STCCR is already set properly.
+	 */
+	if (enabled && ssi_private->cpu_dai_drv.symmetric_rates)
+		return 0;
 
-		/* The SSI should always be disabled at this points (SSIEN=0) */
+	/*
+	 * FIXME: The documentation says that SxCCR[WL] should not be
+	 * modified while the SSI is enabled.  The only time this can
+	 * happen is if we're trying to do simultaneous playback and
+	 * capture in asynchronous mode.  Unfortunately, I have been enable
+	 * to get that to work at all on the P1022DS.  Therefore, we don't
+	 * bother to disable/enable the SSI when setting SxCCR[WL], because
+	 * the SSI will stop anyway.  Maybe one day, this will get fixed.
+	 */
 
-		/* In synchronous mode, the SSI uses STCCR for capture */
-		if ((substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ||
-		    !ssi_private->asynchronous)
-			clrsetbits_be32(&ssi->stccr,
-					CCSR_SSI_SxCCR_WL_MASK, wl);
-		else
-			clrsetbits_be32(&ssi->srccr,
-					CCSR_SSI_SxCCR_WL_MASK, wl);
-	}
+	/* In synchronous mode, the SSI uses STCCR for capture */
+	if ((substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ||
+	    ssi_private->cpu_dai_drv.symmetric_rates)
+		clrsetbits_be32(&ssi->stccr, CCSR_SSI_SxCCR_WL_MASK, wl);
+	else
+		clrsetbits_be32(&ssi->srccr, CCSR_SSI_SxCCR_WL_MASK, wl);
 
 	return 0;
 }
@@ -464,7 +464,6 @@ static int fsl_ssi_trigger(struct snd_pcm_substream *substream, int cmd,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			setbits32(&ssi->scr,
@@ -500,12 +499,6 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
 	struct fsl_ssi_private *ssi_private = snd_soc_dai_get_drvdata(rtd->cpu_dai);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		ssi_private->playback--;
-
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
-		ssi_private->capture--;
-
 	if (ssi_private->first_stream == substream)
 		ssi_private->first_stream = ssi_private->second_stream;
 
@@ -514,7 +507,7 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
 	/*
 	 * If this is the last active substream, disable the SSI.
 	 */
-	if (!ssi_private->playback && !ssi_private->capture) {
+	if (!ssi_private->first_stream) {
 		struct ccsr_ssi __iomem *ssi = ssi_private->ssi;
 
 		clrbits32(&ssi->scr, CCSR_SSI_SCR_SSIEN);
@@ -688,9 +681,7 @@ static int __devinit fsl_ssi_probe(struct platform_device *pdev)
 	}
 
 	/* Are the RX and the TX clocks locked? */
-	if (of_find_property(np, "fsl,ssi-asynchronous", NULL))
-		ssi_private->asynchronous = 1;
-	else
+	if (!of_find_property(np, "fsl,ssi-asynchronous", NULL))
 		ssi_private->cpu_dai_drv.symmetric_rates = 1;
 
 	/* Determine the FIFO depth. */
-- 
1.7.3.4

Re: [PATCH 3/3] ASoC: improve asynchronous mode support in the fsl_ssi driver

[Liam Girdwood]

On Tue, 2011-09-13 at 12:59 -0500, Timur Tabi wrote:
> The Freescale SSI audio controller supports "synchronous" and "asynchronous"
> modes.  In synchronous mode, playback and capture use the same input clock,
> so sample rates must be the same during simultaneous playback and capture.
> Unfortunately, the code which supports asynchronous mode is just broken in
> various ways.  In particular, it was constraining sample sizes as well as
> the sample rate.
> 
> The fix also allows us to simplify the code by eliminating the 'asynchronous',
> 'playback', and 'capture' variables that were used to keep track of playback
> and capture streams.
> 
> Unfortunately, it turns out that simulataneous playback and record does not
> actually work on the only platform that supports asynchronous mode: the
> Freescale P1022DS reference board.  If a second stream is started, the SSI
> grinds to halt for both streams.  This is true even if the P1022 is configured
> for synchronous mode, so it's likely a hardware problem that needs to be
> worked around.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH 3/3] ASoC: improve asynchronous mode support in the fsl_ssi driver

[Mark Brown]

On Tue, Sep 13, 2011 at 12:59:37PM -0500, Timur Tabi wrote:
> The Freescale SSI audio controller supports "synchronous" and "asynchronous"
> modes.  In synchronous mode, playback and capture use the same input clock,
> so sample rates must be the same during simultaneous playback and capture.
> Unfortunately, the code which supports asynchronous mode is just broken in
> various ways.  In particular, it was constraining sample sizes as well as
> the sample rate.

Applied, thanks.

Re: [PATCH 3/3] ASoC: improve asynchronous mode support in the fsl_ssi driver

[Mark Brown]

On Tue, Sep 13, 2011 at 12:59:37PM -0500, Timur Tabi wrote:
> The Freescale SSI audio controller supports "synchronous" and "asynchronous"
> modes.  In synchronous mode, playback and capture use the same input clock,
> so sample rates must be the same during simultaneous playback and capture.
> Unfortunately, the code which supports asynchronous mode is just broken in
> various ways.  In particular, it was constraining sample sizes as well as
> the sample rate.

Applied, thanks.

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Liam Girdwood]

On Tue, 2011-09-13 at 12:59 -0500, Timur Tabi wrote:
> Use snd_pcm_format_width() to determine the sample size, instead of
> checking specify sample formats and assuming that those are the only
> valid format.
> 
> This change adds support for big-endian architectures (which use the _BE
> formats) and the packed 24-bit format (SNDRV_PCM_FORMAT_S24_3xE).
> 

Sounds like we also need this for the other codec drivers too at some
point.

> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  sound/soc/codecs/wm8776.c |   25 +++++++++++++------------
>  1 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
> index 8e7953b..ad6f0fa 100644
> --- a/sound/soc/codecs/wm8776.c
> +++ b/sound/soc/codecs/wm8776.c
> @@ -215,8 +215,6 @@ static int wm8776_hw_params(struct snd_pcm_substream *substream,
>  	int ratio_shift, master;
>  	int i;
>  
> -	iface = 0;
> -
>  	switch (dai->driver->id) {
>  	case WM8776_DAI_DAC:
>  		iface_reg = WM8776_DACIFCTRL;
> @@ -232,20 +230,23 @@ static int wm8776_hw_params(struct snd_pcm_substream *substream,
>  		return -EINVAL;
>  	}
>  
> -
>  	/* Set word length */
> -	switch (params_format(params)) {
> -	case SNDRV_PCM_FORMAT_S16_LE:
> +	i = snd_pcm_format_width(params_format(params));
> +	switch (i) {

Just a minor nitpick. I would not have used the intermediate 'i' here
and would have just embedded the snd_pcm call within the switch(). I
guess it may not fit the line width though.

Acked-by: Liam Girdwood <lrg@ti.com>

> +	case 16:
> +		iface = 0;
> +	case 20:
> +		iface = 0x10;
>  		break;
> -	case SNDRV_PCM_FORMAT_S20_3LE:
> -		iface |= 0x10;
> +	case 24:
> +		iface = 0x20;
>  		break;
> -	case SNDRV_PCM_FORMAT_S24_LE:
> -		iface |= 0x20;
> -		break;
> -	case SNDRV_PCM_FORMAT_S32_LE:
> -		iface |= 0x30;
> +	case 32:
> +		iface = 0x30;
>  		break;
> +	default:
> +		dev_err(codec->dev, "Unsupported sample size: %i\n", i);
> +		return -EINVAL;
>  	}
>  
>  	/* Only need to set MCLK/LRCLK ratio if we're master */

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Timur Tabi]

Liam Girdwood wrote:

> Sounds like we also need this for the other codec drivers too at some
> point.

Yes, but I can only test *this* codec on a big-endian system. :-)

>>  	/* Set word length */
>> -	switch (params_format(params)) {
>> -	case SNDRV_PCM_FORMAT_S16_LE:
>> +	i = snd_pcm_format_width(params_format(params));
>> +	switch (i) {
> 
> Just a minor nitpick. I would not have used the intermediate 'i' here
> and would have just embedded the snd_pcm call within the switch(). I
> guess it may not fit the line width though.

'i' is used twice:

	switch (i) {

and

	dev_err(codec->dev, "Unsupported sample size: %i\n", i);

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Mark Brown]

On Wed, Sep 14, 2011 at 04:51:24PM -0500, Timur Tabi wrote:

> >> +	i = snd_pcm_format_width(params_format(params));
> >> +	switch (i) {

> > Just a minor nitpick. I would not have used the intermediate 'i' here
> > and would have just embedded the snd_pcm call within the switch(). I
> > guess it may not fit the line width though.

> 'i' is used twice:

> 	switch (i) {

> and

> 	dev_err(codec->dev, "Unsupported sample size: %i\n", i);

There's no meaningful cost in calling the function twice and it'd be
much more legible to do so.

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Tabi Timur-B04825]

Mark Brown wrote:
>> >  	dev_err(codec->dev, "Unsupported sample size: %i\n", i);

> There's no meaningful cost in calling the function twice

Perhaps.

 > and it'd be
> much more legible to do so.

Well, I just don't agree with that.  It's pretty obvious what 'i' is.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Mark Brown]

On Tue, Sep 13, 2011 at 12:59:35PM -0500, Timur Tabi wrote:

> -	iface = 0;
> -

Random stylistic changes like this make review harder, especially when
they're not mentioned in the changelog.  In this case it's not clear
from the diff alone that there are no other places where iface gets
written to which are going to get trashed by the change and since it
wasn't mentioned in the changelog it's not clear if the change was
intentional or committed by mistake.

An important aspect of review is checking that the change being made
actually do what they're supposed to do, unexpected additional stuff is
a red flag - you have to work out if it's something the sender meant to
include, what it's supposed to do and if it's a good idea.

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Timur Tabi]

Mark Brown wrote:
> On Tue, Sep 13, 2011 at 12:59:35PM -0500, Timur Tabi wrote:
> 
>> -	iface = 0;
>> -
> 
> Random stylistic changes like this make review harder, especially when
> they're not mentioned in the changelog. 

This is not a random stylistic change.  My patch changes the way iface is
calculated.  Therefore, removing an unnecessary pre-initialization of iface is
both obvious and expected.

The "iface = 0" line is used in the original code because the driver did not
have a "default" case for the "switch" statement.  Since I added a default case,
there was no need to pre-initialize iface anymore.

If I had left that line in the driver, someone would have complained that I'm
pre-initializing iface unnecessarily.

> In this case it's not clear
> from the diff alone that there are no other places where iface gets
> written to

Ok, so that means that you need to look at the original code in order to
understand the change completely.  What's wrong with that?  I just don't see how
this is a valid criticism for any patch.

> An important aspect of review is checking that the change being made
> actually do what they're supposed to do, unexpected additional stuff is
> a red flag - you have to work out if it's something the sender meant to
> include, what it's supposed to do and if it's a good idea.

Again, I just don't understand this complaint of my patch.  I've carefully read
your email, and I re-examined my patch and the original code, and I still don't
think that there's anything wrong with the patch.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Mark Brown]

On Thu, Sep 15, 2011 at 10:16:20AM -0500, Timur Tabi wrote:

> This is not a random stylistic change.  My patch changes the way iface is
> calculated.  Therefore, removing an unnecessary pre-initialization of iface is
> both obvious and expected.

No, really it is.

> The "iface = 0" line is used in the original code because the driver did not
> have a "default" case for the "switch" statement.  Since I added a default case,
> there was no need to pre-initialize iface anymore.

It's not that, there's a common idiom in older ASoC drivers where we
initialize the registers we're updating in things like hw_params() to
zero then run through oring in all the various bitfields they update
before a final write at the end.  The code is using that, it just
happens that we end up with only one bitfield to update.  An even better
change would be to use snd_soc_update_bits() to make sure only the
relevant bitfield is touched which is the current state of the art in
idioms for these things.

> If I had left that line in the driver, someone would have complained that I'm
> pre-initializing iface unnecessarily.

That's also the case in the existing code, it's just that the assignment
is masking a bug with the default case (and if we didn't consider that a
bug then of course there's a change to the default behaviour which also
ought to be noted).

In any case, the issue here isn't that the new code is wrong - I'm just
pointing out that you're making the patch harder to review because
you're making changes beyond a straight substitution in of
snd_pcm_format_width() which increases the review difficulty of what
should be a trivial review.

Based on the changelog the first thing I'd look for when reviewing is
that we've got a series of mechanical 1:1 substitutions of references to
SNDRV_PCM_FORMAT and friends with references to snd_pcm_format_width()
and friends.  If you'd just added a note about the style change it'd
have been fine as I'd have been expecting to see changes more like what
you've got.

> > In this case it's not clear
> > from the diff alone that there are no other places where iface gets
> > written to

> Ok, so that means that you need to look at the original code in order to
> understand the change completely.  What's wrong with that?  I just don't see how
> this is a valid criticism for any patch.

Review cost is an issue, not just in terms of it being less effort but
also in terms of increasing the quality of the review - the more that
the reviewer has to infer what's going on the greater the chances that
they'll infer something that isn't there or miss things they should have
seen.  Extra changes that sneak in are a classic source of bugs, things
being missing from the changelog often means that either they were debug
code that went in by accident or they're changes that haven't been
thought through properly.

Had the style change come out all in one diff hunk it'd probably have
been OK but what actually happened was that the first thing seen was a
removal of an assignment to iface which stopped me short.  The fact that
you generally write fairly verbose and clear changlogs makes this
particularly surprising.

> > An important aspect of review is checking that the change being made
> > actually do what they're supposed to do, unexpected additional stuff is
> > a red flag - you have to work out if it's something the sender meant to
> > include, what it's supposed to do and if it's a good idea.

> Again, I just don't understand this complaint of my patch.  I've carefully read
> your email, and I re-examined my patch and the original code, and I still don't
> think that there's anything wrong with the patch.

There's nothing wrong with the code itself, the issue was about how the
process can flow more smoothly.

Re: [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver

[Mark Brown]

On Tue, Sep 13, 2011 at 12:59:35PM -0500, Timur Tabi wrote:
> Use snd_pcm_format_width() to determine the sample size, instead of
> checking specify sample formats and assuming that those are the only
> valid format.

Applied with the legibility problem with i fixed.