[alsa-devel] [PATCH 1/3] ASoC: support sample sizes properly in the WM8776 codec driver
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 */
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,
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
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.
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?
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.
- 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.
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.
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.
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?
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.
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. */
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
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.
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.
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;
- case SNDRV_PCM_FORMAT_S20_3LE:
iface |= 0x10;
- case 24:
iface = 0x20;
- 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 */
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);
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.
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.
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.
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.
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.
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.
participants (4)
-
Liam Girdwood -
Mark Brown -
Tabi Timur-B04825 -
Timur Tabi