[PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

Some boards, such as the HiFiBerry DAC+ Pro, use a pair of external
oscillators, to generate 44.1 or 48kHz multiples and are forced to
resort to hacks [1] in order to support 24-bit data without ending up
with fractional dividers.  This patch allows the machine driver to use
32-bit frames for 24-bit data to avoid such issues.

Although the datasheet (p. 15) seems to suggest that only a handful
of ratios are supported, it's not very explicit about it, so we allow
the full range of values supported by the underlying register in the
callback, to avoid needlessly rejecting potentially usable
configurations.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143442.html

Signed-off-by: Dimitris Papavasiliou <dpapavas@gmail.com>
---

Notes:
    This is essentially the same patch submitted previously as an RFC[1].
    It has been split in two, in order to separate the set_bclk_ratio
    implementation from the clocking calculation fix and some conservative
    checking has been added to the argument of the callback.
    
    [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144213.html

 sound/soc/codecs/pcm512x.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 6cb1653be804..b63d9392bae3 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -55,6 +55,7 @@ struct pcm512x_priv {
 	unsigned long overclock_dsp;
 	int mute;
 	struct mutex mutex;
+	unsigned int bclk_ratio;
 };
 
 /*
@@ -915,10 +916,15 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai,
 	int fssp;
 	int gpio;
 
-	lrclk_div = snd_soc_params_to_frame_size(params);
-	if (lrclk_div == 0) {
-		dev_err(dev, "No LRCLK?\n");
-		return -EINVAL;
+	if (pcm512x->bclk_ratio > 0) {
+		lrclk_div = pcm512x->bclk_ratio;
+	} else {
+		lrclk_div = snd_soc_params_to_frame_size(params);
+
+		if (lrclk_div == 0) {
+			dev_err(dev, "No LRCLK?\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!pcm512x->pll_out) {
@@ -1383,6 +1389,19 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
+static int pcm512x_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+
+	if (ratio > 256)
+		return -EINVAL;
+
+	pcm512x->bclk_ratio = ratio;
+
+	return 0;
+}
+
 static int pcm512x_digital_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
@@ -1438,6 +1457,7 @@ static const struct snd_soc_dai_ops pcm512x_dai_ops = {
 	.hw_params = pcm512x_hw_params,
 	.set_fmt = pcm512x_set_fmt,
 	.digital_mute = pcm512x_digital_mute,
+	.set_bclk_ratio = pcm512x_set_bclk_ratio,
 };
 
 static struct snd_soc_dai_driver pcm512x_dai = {
-- 
2.11.0

Re: [PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Peter Rosin]

On 2019-01-26 14:17, Dimitris Papavasiliou wrote:
> Some boards, such as the HiFiBerry DAC+ Pro, use a pair of external
> oscillators, to generate 44.1 or 48kHz multiples and are forced to
> resort to hacks [1] in order to support 24-bit data without ending up
> with fractional dividers.  This patch allows the machine driver to use
> 32-bit frames for 24-bit data to avoid such issues.
> 
> Although the datasheet (p. 15) seems to suggest that only a handful
> of ratios are supported, it's not very explicit about it, so we allow
> the full range of values supported by the underlying register in the
> callback, to avoid needlessly rejecting potentially usable
> configurations.
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143442.html
> 
> Signed-off-by: Dimitris Papavasiliou <dpapavas@gmail.com>
> ---
> 
> Notes:
>     This is essentially the same patch submitted previously as an RFC[1].
>     It has been split in two, in order to separate the set_bclk_ratio
>     implementation from the clocking calculation fix and some conservative
>     checking has been added to the argument of the callback.
>     
>     [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144213.html
> 
>  sound/soc/codecs/pcm512x.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
> index 6cb1653be804..b63d9392bae3 100644
> --- a/sound/soc/codecs/pcm512x.c
> +++ b/sound/soc/codecs/pcm512x.c
> @@ -55,6 +55,7 @@ struct pcm512x_priv {
>  	unsigned long overclock_dsp;
>  	int mute;
>  	struct mutex mutex;
> +	unsigned int bclk_ratio;
>  };
>  
>  /*
> @@ -915,10 +916,15 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai,
>  	int fssp;
>  	int gpio;
>  
> -	lrclk_div = snd_soc_params_to_frame_size(params);
> -	if (lrclk_div == 0) {
> -		dev_err(dev, "No LRCLK?\n");
> -		return -EINVAL;
> +	if (pcm512x->bclk_ratio > 0) {
> +		lrclk_div = pcm512x->bclk_ratio;
> +	} else {
> +		lrclk_div = snd_soc_params_to_frame_size(params);
> +
> +		if (lrclk_div == 0) {
> +			dev_err(dev, "No LRCLK?\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	if (!pcm512x->pll_out) {
> @@ -1383,6 +1389,19 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  	return 0;
>  }
>  
> +static int pcm512x_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
> +
> +	if (ratio > 256)
> +		return -EINVAL;

ratio == 0 should also generate -EINVAL.

Cheers,
Peter

> +
> +	pcm512x->bclk_ratio = ratio;
> +
> +	return 0;
> +}
> +
>  static int pcm512x_digital_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_component *component = dai->component;
> @@ -1438,6 +1457,7 @@ static const struct snd_soc_dai_ops pcm512x_dai_ops = {
>  	.hw_params = pcm512x_hw_params,
>  	.set_fmt = pcm512x_set_fmt,
>  	.digital_mute = pcm512x_digital_mute,
> +	.set_bclk_ratio = pcm512x_set_bclk_ratio,
>  };
>  
>  static struct snd_soc_dai_driver pcm512x_dai = {
> 

Re: [PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

On 1/26/19 8:48 PM, Peter Rosin wrote:
> On 2019-01-26 14:17, Dimitris Papavasiliou wrote:
>> +static int pcm512x_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
>> +{
>> +	struct snd_soc_component *component = dai->component;
>> +	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
>> +
>> +	if (ratio > 256)
>> +		return -EINVAL;
> 
> ratio == 0 should also generate -EINVAL.

Sorry, I forgot to mention in the notes, that I allowed setting a
zero ratio on purpose, in order to allow "unsetting" the bclk
ratio (since it only takes effect if it is positive).  I'm not
sure if it will ever be necessary to do that in practice, but I
thought I'd allow it, since it has a valid effect.

Re: [PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 516 bytes --]

On Sat, Jan 26, 2019 at 09:00:09PM +0200, Dimitris Papavasiliou wrote:
> On 1/26/19 8:48 PM, Peter Rosin wrote:

> > ratio == 0 should also generate -EINVAL.

> Sorry, I forgot to mention in the notes, that I allowed setting a
> zero ratio on purpose, in order to allow "unsetting" the bclk
> ratio (since it only takes effect if it is positive).  I'm not
> sure if it will ever be necessary to do that in practice, but I
> thought I'd allow it, since it has a valid effect.

This is fairly idiomatic for ASoC FWIW.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Applied "ASoC: pcm512x: Implement the set_bclk_ratio interface" to the asoc tree

[Mark Brown]

The patch

   ASoC: pcm512x: Implement the set_bclk_ratio interface

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

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

>From ccc8d6c7b6d2f521a4b10c7f6d027f46c7a686bf Mon Sep 17 00:00:00 2001
From: Dimitris Papavasiliou <dpapavas@gmail.com>
Date: Sat, 26 Jan 2019 15:17:01 +0200
Subject: [PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

Some boards, such as the HiFiBerry DAC+ Pro, use a pair of external
oscillators, to generate 44.1 or 48kHz multiples and are forced to
resort to hacks [1] in order to support 24-bit data without ending up
with fractional dividers.  This patch allows the machine driver to use
32-bit frames for 24-bit data to avoid such issues.

Although the datasheet (p. 15) seems to suggest that only a handful
of ratios are supported, it's not very explicit about it, so we allow
the full range of values supported by the underlying register in the
callback, to avoid needlessly rejecting potentially usable
configurations.

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143442.html

Signed-off-by: Dimitris Papavasiliou <dpapavas@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/pcm512x.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4cc24a5d5c31..ce8c5dbd2164 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -55,6 +55,7 @@ struct pcm512x_priv {
 	unsigned long overclock_dsp;
 	int mute;
 	struct mutex mutex;
+	unsigned int bclk_ratio;
 };
 
 /*
@@ -915,10 +916,15 @@ static int pcm512x_set_dividers(struct snd_soc_dai *dai,
 	int fssp;
 	int gpio;
 
-	lrclk_div = snd_soc_params_to_frame_size(params);
-	if (lrclk_div == 0) {
-		dev_err(dev, "No LRCLK?\n");
-		return -EINVAL;
+	if (pcm512x->bclk_ratio > 0) {
+		lrclk_div = pcm512x->bclk_ratio;
+	} else {
+		lrclk_div = snd_soc_params_to_frame_size(params);
+
+		if (lrclk_div == 0) {
+			dev_err(dev, "No LRCLK?\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!pcm512x->pll_out) {
@@ -1383,6 +1389,19 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
+static int pcm512x_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
+{
+	struct snd_soc_component *component = dai->component;
+	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+
+	if (ratio > 256)
+		return -EINVAL;
+
+	pcm512x->bclk_ratio = ratio;
+
+	return 0;
+}
+
 static int pcm512x_digital_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct snd_soc_component *component = dai->component;
@@ -1435,6 +1454,7 @@ static const struct snd_soc_dai_ops pcm512x_dai_ops = {
 	.hw_params = pcm512x_hw_params,
 	.set_fmt = pcm512x_set_fmt,
 	.digital_mute = pcm512x_digital_mute,
+	.set_bclk_ratio = pcm512x_set_bclk_ratio,
 };
 
 static struct snd_soc_dai_driver pcm512x_dai = {
-- 
2.20.1

Re: [PATCH 1/2] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

Many thanks for reviewing (and applying) this Mark and equally
many thanks to everyone who took the time to provide feedback!

On 1/28/19 2:10 PM, Mark Brown wrote:
> On Sat, Jan 26, 2019 at 09:00:09PM +0200, Dimitris Papavasiliou wrote:
>> On 1/26/19 8:48 PM, Peter Rosin wrote:
> 
>>> ratio == 0 should also generate -EINVAL.
> 
>> Sorry, I forgot to mention in the notes, that I allowed setting a
>> zero ratio on purpose, in order to allow "unsetting" the bclk
>> ratio (since it only takes effect if it is positive).  I'm not
>> sure if it will ever be necessary to do that in practice, but I
>> thought I'd allow it, since it has a valid effect.
> 
> This is fairly idiomatic for ASoC FWIW.
>