[v1] ASoC: jz4740-i2s: Make I2S divider calculations more robust
Commit Message
When the CPU supplies bit/frame clocks, the system clock (clk_i2s)
is divided to produce the bit clock. This is a simple 1/N divider
with a fairly limited range, so for a given system clock frequency
only a few sample rates can be produced. Usually a wider range of
sample rates is supported by varying the system clock frequency.
The old calculation method was not very robust and could easily
produce the wrong clock rate, especially with non-standard rates.
For example, if the system clock is 1.99x the target bit clock
rate, the divider would be calculated as 1 instead of the more
accurate 2.
Instead, use a more accurate method that considers two adjacent
divider settings and selects the one that produces the least error
versus the requested rate. If the error is 5% or higher then the
rate setting is rejected to prevent garbled audio.
Skip divider calculation when the codec is supplying both the bit
and frame clock; in that case, the divider outputs are unused and
we don't want to constrain the sample rate.
Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
---
sound/soc/jz4740/jz4740-i2s.c | 54 ++++++++++++++++++++++++++++++++---
1 file changed, 50 insertions(+), 4 deletions(-)
Comments
On Tue, 09 May 2023 13:51:34 +0100, Aidan MacDonald wrote:
> When the CPU supplies bit/frame clocks, the system clock (clk_i2s)
> is divided to produce the bit clock. This is a simple 1/N divider
> with a fairly limited range, so for a given system clock frequency
> only a few sample rates can be produced. Usually a wider range of
> sample rates is supported by varying the system clock frequency.
>
> The old calculation method was not very robust and could easily
> produce the wrong clock rate, especially with non-standard rates.
> For example, if the system clock is 1.99x the target bit clock
> rate, the divider would be calculated as 1 instead of the more
> accurate 2.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: jz4740-i2s: Make I2S divider calculations more robust
commit: ad721bc919edfd8b4b06977458a412011e2f0c50
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
@@ -218,18 +218,48 @@ static int jz4740_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
return 0;
}
+static int jz4740_i2s_get_i2sdiv(unsigned long mclk, unsigned long rate,
+ unsigned long i2sdiv_max)
+{
+ unsigned long div, rate1, rate2, err1, err2;
+
+ div = mclk / (64 * rate);
+ if (div == 0)
+ div = 1;
+
+ rate1 = mclk / (64 * div);
+ rate2 = mclk / (64 * (div + 1));
+
+ err1 = abs(rate1 - rate);
+ err2 = abs(rate2 - rate);
+
+ /*
+ * Choose the divider that produces the smallest error in the
+ * output rate and reject dividers with a 5% or higher error.
+ * In the event that both dividers are outside the acceptable
+ * error margin, reject the rate to prevent distorted audio.
+ * (The number 5% is arbitrary.)
+ */
+ if (div <= i2sdiv_max && err1 <= err2 && err1 < rate/20)
+ return div;
+ if (div < i2sdiv_max && err2 < rate/20)
+ return div + 1;
+
+ return -EINVAL;
+}
+
static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
{
struct jz4740_i2s *i2s = snd_soc_dai_get_drvdata(dai);
struct regmap_field *div_field;
+ unsigned long i2sdiv_max;
unsigned int sample_size;
- uint32_t ctrl;
- int div;
+ uint32_t ctrl, conf;
+ int div = 1;
regmap_read(i2s->regmap, JZ_REG_AIC_CTRL, &ctrl);
-
- div = clk_get_rate(i2s->clk_i2s) / (64 * params_rate(params));
+ regmap_read(i2s->regmap, JZ_REG_AIC_CONF, &conf);
switch (params_format(params)) {
case SNDRV_PCM_FORMAT_S8:
@@ -258,11 +288,27 @@ static int jz4740_i2s_hw_params(struct snd_pcm_substream *substream,
ctrl &= ~JZ_AIC_CTRL_MONO_TO_STEREO;
div_field = i2s->field_i2sdiv_playback;
+ i2sdiv_max = GENMASK(i2s->soc_info->field_i2sdiv_playback.msb,
+ i2s->soc_info->field_i2sdiv_playback.lsb);
} else {
ctrl &= ~JZ_AIC_CTRL_INPUT_SAMPLE_SIZE;
ctrl |= FIELD_PREP(JZ_AIC_CTRL_INPUT_SAMPLE_SIZE, sample_size);
div_field = i2s->field_i2sdiv_capture;
+ i2sdiv_max = GENMASK(i2s->soc_info->field_i2sdiv_capture.msb,
+ i2s->soc_info->field_i2sdiv_capture.lsb);
+ }
+
+ /*
+ * Only calculate I2SDIV if we're supplying the bit or frame clock.
+ * If the codec is supplying both clocks then the divider output is
+ * unused, and we don't want it to limit the allowed sample rates.
+ */
+ if (conf & (JZ_AIC_CONF_BIT_CLK_MASTER | JZ_AIC_CONF_SYNC_CLK_MASTER)) {
+ div = jz4740_i2s_get_i2sdiv(clk_get_rate(i2s->clk_i2s),
+ params_rate(params), i2sdiv_max);
+ if (div < 0)
+ return div;
}
regmap_write(i2s->regmap, JZ_REG_AIC_CTRL, ctrl);