[alsa-devel] [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.