[RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

>From 5e6ad135fd063d4b22cd962de43499a161bbc7db Mon Sep 17 00:00:00 2001
From: Dimitris Papavasiliou <dpapavas@gmail.com>
Date: Fri, 11 Jan 2019 22:13:27 +0200
Subject: [RFC 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.

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

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

Notes:
    The patch also includes a related change in the calculation of the
    bclk divider.  The current calculation is not very clear to me, as I
    can't find much concrete information on what this sample rate fraction
    is, how it's calculated and how, therefore, its denominator would fit
    into the calculation.  It also yields slightly wrong dividers in
    certain cases, which the machine driver for my HiFiBerry DAC+ Pro
    board seemingly tries to circumvent, by updating the rate fraction so
    as to suit this calculation.
    
    The updated calculation seems to work fine in my tests and, as far as
    I can see, should correctly yield the smallest bit clock rate that
    would fit the frame.  Please comment if anything looks off.

 sound/soc/codecs/pcm512x.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4cc24a5d5c31..8cd728f9a1eb 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,16 +916,21 @@ 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) {
 		sck_rate = clk_get_rate(pcm512x->sclk);
-		bclk_div = params->rate_den * 64 / lrclk_div;
-		bclk_rate = DIV_ROUND_CLOSEST(sck_rate, bclk_div);
+		bclk_rate = params_rate(params) * lrclk_div;
+		bclk_div = DIV_ROUND_CLOSEST(sck_rate, bclk_rate);
 
 		mck_rate = sck_rate;
 	} else {
@@ -1383,6 +1389,16 @@ 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);
+
+	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 +1451,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: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Peter Rosin]

On 2019-01-13 21:16, Dimitris Papavasiliou wrote:
> From 5e6ad135fd063d4b22cd962de43499a161bbc7db Mon Sep 17 00:00:00 2001
> From: Dimitris Papavasiliou <dpapavas@gmail.com>
> Date: Fri, 11 Jan 2019 22:13:27 +0200
> Subject: [RFC 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

Ouch, is the PLL not fine-grained enough? That seems unlikely. Did
they perhaps simply not know how to wire the chip for the PLL to work?

> 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.
> 
> [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143442.html
> 
> Signed-off-by: Dimitris Papavasiliou <dpapavas@gmail.com>
> ---
> 
> Notes:
>     The patch also includes a related change in the calculation of the
>     bclk divider.  The current calculation is not very clear to me, as I
>     can't find much concrete information on what this sample rate fraction
>     is, how it's calculated and how, therefore, its denominator would fit
>     into the calculation.  It also yields slightly wrong dividers in
>     certain cases, which the machine driver for my HiFiBerry DAC+ Pro
>     board seemingly tries to circumvent, by updating the rate fraction so
>     as to suit this calculation.
>     
>     The updated calculation seems to work fine in my tests and, as far as
>     I can see, should correctly yield the smallest bit clock rate that
>     would fit the frame.  Please comment if anything looks off.

On reading this, I think this should be a separate patch. But maybe that's
just me?

>  sound/soc/codecs/pcm512x.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
> index 4cc24a5d5c31..8cd728f9a1eb 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,16 +916,21 @@ 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) {
>  		sck_rate = clk_get_rate(pcm512x->sclk);
> -		bclk_div = params->rate_den * 64 / lrclk_div;
> -		bclk_rate = DIV_ROUND_CLOSEST(sck_rate, bclk_div);
> +		bclk_rate = params_rate(params) * lrclk_div;
> +		bclk_div = DIV_ROUND_CLOSEST(sck_rate, bclk_rate);

Ok, so I apparently wrote this. Half a decade ago or so. It should be known
that my focus was entirely on the pll-case. I added the non-pll-case mostly
as a courtesy and we are not using it over here. If you don't have an even
divider, you should be using the pll anyway, right? :-) At least if you
knew you had to wire the HW so that the pll is an option... The datasheets
are not very clear, at least they were not way back when for the chip we
focused on (the pcm5142, big relief when I discovered the pcm5242
datasheet).

Anyway, the value of bclk_rate is obviously not what matters, that's just a
helper variable in the pll-case and (previously) not used at all in the
non-pll-case. It is the value of bclk_div that matters. So, whatever code
gets you to the desired bclk_div is what you should aim for and AFAICT your
above change makes sense.

I do wonder why I hard-coded the 64, I have a vague memory of it being
quite deliberate. It's all a haze now though...


>  
>  		mck_rate = sck_rate;
>  	} else {
> @@ -1383,6 +1389,16 @@ 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);
> +
> +	pcm512x->bclk_ratio = ratio;

You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
accepting a divider that can't be programmed? But maybe that's enforced
somewhere else? And perhaps the sanity check should be even stricter?

Cheers,
Peter

> +
> +	return 0;
> +}
> +
>  static int pcm512x_digital_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_component *component = dai->component;
> @@ -1435,6 +1451,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: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

Thank you for your comments Peter.

On 1/14/19 1:13 PM, Peter Rosin wrote:> On 2019-01-13 21:16, 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
>
> Ouch, is the PLL not fine-grained enough? That seems unlikely. Did
> they perhaps simply not know how to wire the chip for the PLL to work?

There's a cheaper version of the board, which operates in slave
mode, without external clocks.  My understanding, based on their
marketing blurb, is that it's done in order to improve audio
quality by minimizing clock jitter.  The datasheet mentions that
using an external clock would be desirable, in order to "keep the
jitter evident in the PLL (in all PLLs) out of the DAC", but it
suggests using the external clock only for the DAC clock and the
clocks derived from it and keep the PLL for the audio processing
block.

Whether this would have been preferable, but they couldn't figure
out how to get it to work, I can't really tell.  What I can say,
is that the current design is causing all sorts of problems, none
of which seem to be evident when the device is configured to run
in slave mode.

>> Notes:
>>      The patch also includes a related change in the calculation of the
>>      bclk divider.  The current calculation is not very clear to me, as I
>>      can't find much concrete information on what this sample rate fraction
>>      is, how it's calculated and how, therefore, its denominator would fit
>>      into the calculation.  It also yields slightly wrong dividers in
>>      certain cases, which the machine driver for my HiFiBerry DAC+ Pro
>>      board seemingly tries to circumvent, by updating the rate fraction so
>>      as to suit this calculation.
>>
>>      The updated calculation seems to work fine in my tests and, as far as
>>      I can see, should correctly yield the smallest bit clock rate that
>>      would fit the frame.  Please comment if anything looks off.
>
> On reading this, I think this should be a separate patch. But maybe that's
> just me?

No, I also considered splitting this into two patches, but
decided to keep the RFC in one piece, to keep the discussion in
one place.  The patches are related after all, in the sense that
it probably wouldn't make much sense to just change the
calculation, if the set_bclk_ratio patch is rejected.  If the
patch proves to be, in principle at least, acceptable, I'll
probably submit it as two separate patches.

>> -    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) {
>>           sck_rate = clk_get_rate(pcm512x->sclk);
>> -        bclk_div = params->rate_den * 64 / lrclk_div;
>> -        bclk_rate = DIV_ROUND_CLOSEST(sck_rate, bclk_div);
>> +        bclk_rate = params_rate(params) * lrclk_div;
>> +        bclk_div = DIV_ROUND_CLOSEST(sck_rate, bclk_rate);
>
> Ok, so I apparently wrote this. Half a decade ago or so. It should be known
> that my focus was entirely on the pll-case. I added the non-pll-case mostly
> as a courtesy and we are not using it over here. If you don't have an even
> divider, you should be using the pll anyway, right? :-) At least if you
> knew you had to wire the HW so that the pll is an option... The datasheets
> are not very clear, at least they were not way back when for the chip we
> focused on (the pcm5142, big relief when I discovered the pcm5242
> datasheet).

It's good to know this code path doesn't affect you.  The TSE-850
seems to be the only driver using the pcm512x CODEC driver in the
mainline kernel and I was worrying that the change might cause
trouble for you.

>> +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);
>> +
>> +    pcm512x->bclk_ratio = ratio;
>
> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
> accepting a divider that can't be programmed? But maybe that's enforced
> somewhere else? And perhaps the sanity check should be even stricter?

Yes, you're probably right.  This RFC was mostly meant as a
proof-of-concept implementation, in order to get some feedback on
whether this approach is appropriate, or at least makes some sense.
If it does, I'll have to look further into what checks are necessary.

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Peter Rosin]

On 2019-01-14 15:53, Dimitris Papavasiliou wrote:
> Thank you for your comments Peter.
> 
> On 1/14/19 1:13 PM, Peter Rosin wrote:> On 2019-01-13 21:16, 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
>>
>> Ouch, is the PLL not fine-grained enough? That seems unlikely. Did
>> they perhaps simply not know how to wire the chip for the PLL to work?
> 
> There's a cheaper version of the board, which operates in slave
> mode, without external clocks.  My understanding, based on their
> marketing blurb, is that it's done in order to improve audio
> quality by minimizing clock jitter.  The datasheet mentions that
> using an external clock would be desirable, in order to "keep the
> jitter evident in the PLL (in all PLLs) out of the DAC", but it
> suggests using the external clock only for the DAC clock and the
> clocks derived from it and keep the PLL for the audio processing
> block.
> 
> Whether this would have been preferable, but they couldn't figure
> out how to get it to work, I can't really tell.  What I can say,
> is that the current design is causing all sorts of problems, none
> of which seem to be evident when the device is configured to run
> in slave mode.

Right, the slave part of the driver is way simpler than all the
nasty clock handling required when mastering the AIF...

>>> Notes:
>>>      The patch also includes a related change in the calculation of the
>>>      bclk divider.  The current calculation is not very clear to me, as I
>>>      can't find much concrete information on what this sample rate fraction
>>>      is, how it's calculated and how, therefore, its denominator would fit
>>>      into the calculation.  It also yields slightly wrong dividers in
>>>      certain cases, which the machine driver for my HiFiBerry DAC+ Pro
>>>      board seemingly tries to circumvent, by updating the rate fraction so
>>>      as to suit this calculation.
>>>
>>>      The updated calculation seems to work fine in my tests and, as far as
>>>      I can see, should correctly yield the smallest bit clock rate that
>>>      would fit the frame.  Please comment if anything looks off.
>>
>> On reading this, I think this should be a separate patch. But maybe that's
>> just me?
> 
> No, I also considered splitting this into two patches, but
> decided to keep the RFC in one piece, to keep the discussion in
> one place.  The patches are related after all, in the sense that
> it probably wouldn't make much sense to just change the
> calculation, if the set_bclk_ratio patch is rejected.  If the
> patch proves to be, in principle at least, acceptable, I'll
> probably submit it as two separate patches.
> 
>>> -    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) {
>>>           sck_rate = clk_get_rate(pcm512x->sclk);
>>> -        bclk_div = params->rate_den * 64 / lrclk_div;
>>> -        bclk_rate = DIV_ROUND_CLOSEST(sck_rate, bclk_div);
>>> +        bclk_rate = params_rate(params) * lrclk_div;
>>> +        bclk_div = DIV_ROUND_CLOSEST(sck_rate, bclk_rate);
>>
>> Ok, so I apparently wrote this. Half a decade ago or so. It should be known
>> that my focus was entirely on the pll-case. I added the non-pll-case mostly
>> as a courtesy and we are not using it over here. If you don't have an even
>> divider, you should be using the pll anyway, right? :-) At least if you
>> knew you had to wire the HW so that the pll is an option... The datasheets
>> are not very clear, at least they were not way back when for the chip we
>> focused on (the pcm5142, big relief when I discovered the pcm5242
>> datasheet).
> 
> It's good to know this code path doesn't affect you.  The TSE-850
> seems to be the only driver using the pcm512x CODEC driver in the
> mainline kernel and I was worrying that the change might cause
> trouble for you.

Yes, we are typically only using a single sample frequency (250kHz)
and have naturally selected an external xtal so that the pll can be
avoided (sck is 16MHz, but since we have the pll option, the branch
you changed is not affecting our case). At least not if
pcm512x->bclk_ratio is left at zero (or set to 64)...

>>> +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);
>>> +
>>> +    pcm512x->bclk_ratio = ratio;
>>
>> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
>> accepting a divider that can't be programmed? But maybe that's enforced
>> somewhere else? And perhaps the sanity check should be even stricter?

...and when double-checking that, this dai op is only ever called from
snd_soc_dai_set_bclk_ratio, and that function is in turn not called from
anywhere AFAICT. What's the point of this dai op anyway? Or, what am I
missing?

Cheers,
Peter

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

On Tue, Jan 15, 2019 at 10:06 AM Peter Rosin <peda@axentia.se> wrote:
> ...and when double-checking that, this dai op is only ever called from
> snd_soc_dai_set_bclk_ratio, and that function is in turn not called from
> anywhere AFAICT. What's the point of this dai op anyway? Or, what am I
> missing?

Well, my use for this, would be to allow the machine driver to call
snd_soc_dai_set_bclk_ratio, in order to explicitly set the frame
width.  This would allow it to use 32-bit frames for 24-bit data and
avoid the fractional dividers that would otherwise result.  Right now,
the machine driver (which lives in the Raspberry Pi kernel tree) uses
a workaround for this, that relies on a patch made to the CODEC driver
to half-support TDM slots, which hasn't be pushed upstream, and
wouldn't be accepted if it had, since it's essentially a hack.  You
can find more details here:

http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143507.html

What the intended use for the snd_soc_dai_set_bclk_ratio is, I'm not
so clear about, hence this RFC.

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Peter Rosin]

On 2019-01-15 12:11, Dimitris Papavasiliou wrote:
> On Tue, Jan 15, 2019 at 10:06 AM Peter Rosin <peda@axentia.se> wrote:
>> ...and when double-checking that, this dai op is only ever called from
>> snd_soc_dai_set_bclk_ratio, and that function is in turn not called from
>> anywhere AFAICT. What's the point of this dai op anyway? Or, what am I
>> missing?
> 
> Well, my use for this, would be to allow the machine driver to call
> snd_soc_dai_set_bclk_ratio, in order to explicitly set the frame
> width.  This would allow it to use 32-bit frames for 24-bit data and
> avoid the fractional dividers that would otherwise result.  Right now,
> the machine driver (which lives in the Raspberry Pi kernel tree) uses
> a workaround for this, that relies on a patch made to the CODEC driver
> to half-support TDM slots, which hasn't be pushed upstream, and
> wouldn't be accepted if it had, since it's essentially a hack.  You
> can find more details here:
> 
> http://mailman.alsa-project.org/pipermail/alsa-devel/2018-December/143507.html
> 
> What the intended use for the snd_soc_dai_set_bclk_ratio is, I'm not
> so clear about, hence this RFC.

I did some digging. The dai op was added in 3.13 (without users), then one
user appeared in 3.19 (sound/soc/intel/boards/bytcr_rt5640.c) and another
(presumably copy-pasted) in 4.5 (sound/soc/intel/boards/bytcr_rt5651.c).
Both these users are suspect since neither the rt5640 nor the rt5651 codec
have ever sported a set_bclk_ratio dai op, and presumably the boards have
the named codecs on them. But what do I know...

The user added in 3.19 was removed for 4.9 and the other for 4.17, so
that's why there are no upstream users at the moment.

Cheers,
Peter

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Pierre-Louis Bossart]

>>   
>>   		mck_rate = sck_rate;
>>   	} else {
>> @@ -1383,6 +1389,16 @@ 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);
>> +
>> +	pcm512x->bclk_ratio = ratio;
> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
> accepting a divider that can't be programmed? But maybe that's enforced
> somewhere else? And perhaps the sanity check should be even stricter?
Yes it should be stricter with a power of two only. I tried really hard 
to make this codec work with ratios of 50 (and a 19.2 MHz mclk) on an Up 
board and it's just not possible, probably not supported by hardware.

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Peter Rosin]

On 2019-01-14 18:36, Pierre-Louis Bossart wrote:
> 
>>>   
>>>   		mck_rate = sck_rate;
>>>   	} else {
>>> @@ -1383,6 +1389,16 @@ 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);
>>> +
>>> +	pcm512x->bclk_ratio = ratio;
>> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
>> accepting a divider that can't be programmed? But maybe that's enforced
>> somewhere else? And perhaps the sanity check should be even stricter?
> Yes it should be stricter with a power of two only. I tried really hard 
> to make this codec work with ratios of 50 (and a 19.2 MHz mclk) on an Up 
> board and it's just not possible, probably not supported by hardware.

Disallowing anything but powers of two just because 50 doesn't work seems
pretty wild. According to docs I think 48 should work just fine.

Section 8.3.2 Audio Data Interface page 16.
http://www.ti.com/lit/ds/symlink/pcm5142.pdf

Or?

Cheers,
Peter

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Dimitris Papavasiliou]

On Tue, Jan 15, 2019 at 10:08 AM Peter Rosin <peda@axentia.se> wrote:
>
> On 2019-01-14 18:36, Pierre-Louis Bossart wrote:
> >
> >> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
> >> accepting a divider that can't be programmed? But maybe that's enforced
> >> somewhere else? And perhaps the sanity check should be even stricter?
> > Yes it should be stricter with a power of two only. I tried really hard
> > to make this codec work with ratios of 50 (and a 19.2 MHz mclk) on an Up
> > board and it's just not possible, probably not supported by hardware.
>
> Disallowing anything but powers of two just because 50 doesn't work seems
> pretty wild. According to docs I think 48 should work just fine.
>
> Section 8.3.2 Audio Data Interface page 16.
> http://www.ti.com/lit/ds/symlink/pcm5142.pdf
>
> Or?

According to the datasheet[1], you can program any divider between 1
and 256, via register 33 (page 80), but, as Peter points out, in the
description of the audio data interface (page 15), only the values 32,
48, 64, 128, 256 are mentioned as permissible.  So one approach would
be to restrict the values accepted by the callback to this set.  Since
the datasheet is not terribly clear on whether only these are allowed,
and since the ratio is set explicitly in the machine driver, it could
be argued that it would be preferable to allow all values that can be
set in the register, to minimize the risk of needlessly rejecting
valid configurations.

[1] http://www.ti.com/lit/ds/symlink/pcm5121.pdf

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Pierre-Louis Bossart]

On 1/15/19 6:19 AM, Dimitris Papavasiliou wrote:
> On Tue, Jan 15, 2019 at 10:08 AM Peter Rosin <peda@axentia.se> wrote:
>> On 2019-01-14 18:36, Pierre-Louis Bossart wrote:
>>>> You should perhaps check if (ratio >= 1 && ratio <= 256) prior to
>>>> accepting a divider that can't be programmed? But maybe that's enforced
>>>> somewhere else? And perhaps the sanity check should be even stricter?
>>> Yes it should be stricter with a power of two only. I tried really hard
>>> to make this codec work with ratios of 50 (and a 19.2 MHz mclk) on an Up
>>> board and it's just not possible, probably not supported by hardware.
>> Disallowing anything but powers of two just because 50 doesn't work seems
>> pretty wild. According to docs I think 48 should work just fine.
>>
>> Section 8.3.2 Audio Data Interface page 16.
>> http://www.ti.com/lit/ds/symlink/pcm5142.pdf
>>
>> Or?
> According to the datasheet[1], you can program any divider between 1
> and 256, via register 33 (page 80), but, as Peter points out, in the
> description of the audio data interface (page 15), only the values 32,
> 48, 64, 128, 256 are mentioned as permissible.  So one approach would
> be to restrict the values accepted by the callback to this set.  Since
> the datasheet is not terribly clear on whether only these are allowed,
> and since the ratio is set explicitly in the machine driver, it could
> be argued that it would be preferable to allow all values that can be
> set in the register, to minimize the risk of needlessly rejecting
> valid configurations.
>
> [1] http://www.ti.com/lit/ds/symlink/pcm5121.pdf
I meant multiple of 16, sorry. multiples of 25/50 commonly used with 
other codecs will not work.

Re: [RFC PATCH] ASoC: pcm512x: Implement the set_bclk_ratio interface

[Mark Brown]

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

On Sun, Jan 13, 2019 at 10:16:21PM +0200, 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.

This seems fine to me.  I think when I accepted the patch for setting
bclk ratio that there was hardware which specified the ratio directly as
part of the configuration but it seems that this was not the case; on
the other hand this seems like a reasonable use for it.  

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

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