[alsa-devel] Need help fixing pop/click artifacts in an ASOC driver

[Pierre-Louis Bossart]

On 12/17/18 1:14 PM, Pierre-Louis Bossart wrote:
>
> On 12/17/18 1:02 PM, Mark Brown wrote:
>> On Mon, Dec 17, 2018 at 12:08:36PM -0600, Pierre-Louis Bossart wrote:
>>> On 12/17/18 11:39 AM, Mark Brown wrote:
>>>> That looks a lot like the CODEC should be exporting a GPIO driver 
>>>> so the
>>>> machine driver doesn't actually need the regmap?  The only register
>>>> touched is _GPIO_CONTROL_1.
>>> I am not sure what you meant by 'exporting a GPIO driver' (mostly 
>>> because I
>>> am not familiar with any GPIO framework) but indeed the local 
>>> oscillator
>>> choice is controlled by a single register accessible through regmap 
>>> - and
>>> changes to that register should only happen when the device is a 
>>> specific
>>> state to prevent click/pops.
>> The GPIO framework provides a fairly simple view of GPIOs - from a user
>> point of view it's just getting or setting the value of a line. It
>> looks like the register you're controlling isn't actually controlling a
>> chip feature directly but rather is setting a GPIO on the chip which
>> controls an external clock generator.  I could be misparsing things,
>> though.  I did glance at the pcm512x datasheet and didn't see pins or
>> anything that looked like an oscillator but I could've missed something.
> You are correct, there are two variants of the Hifiberry DAC+, the 
> 'PRO' version with two oscillators on board (SoC configured as 
> bitclock and fsync slave) and the regular without (all clocks provided 
> by the SoC in that case). It's indeed a GPIO control of an external 
> component, not an on-chip capability.
>>
>>> The machine driver should use clk_set_rate() and not directly handle 
>>> regmap
>>> or codec stuff. If it does, or if the clock framework isn't relevant 
>>> here
>>> then we can simplify all this as suggested in
>>> https://patchwork.kernel.org/patch/10444387/. What I was trying to 
>>> do with
>>> the github update is to keep the clock framework, tie it closer with 
>>> the
>>> codec parts with a state variable that prevents wild changes without 
>>> going
>>> back to a 'safe' idle state (similar idea as PulseAudio clock 
>>> changes, which
>>> can only happen when the PCM is not opened and used).
>> Right, I think bringing in the clock framework more is good -
>> effectively all I'm suggesting is changing the control interface used to
>> set the clock to add an indirection through gpiolib so you don't need to
>> pass the raw register map around.
> this looks elegant indeed, will look into it. I'll have to learn more 
> on gpios :-)

I was able to expose the 6 PCM512x GPIOs, toggle them from userspace as 
well as from the clock driver, it's indeed simple enough and could be 
useful for other usages, but there are still two issues with the concept:

1. After toggling a GPIO to enable the 44.1 or 48kHz oscillators, the 
clock driver cannot verify that the codec is locked, for this it'd need 
access to the regmap to check the PCM512x_RATE_DET_4 register. This is 
really needed to detect if the local oscillators are populated or not 
and hence detect the DAC+ PRO vs. the plain vanilla DAC+. An alternative 
would be to let the clock driver program GPIOs, and check in the codec 
driver is the clock is valid or not after the call to 
clk_prepare_enable() and fall back to slave mode in the latter case. I 
believe it'd be more elegant to do all these checks in the clock driver 
itself, and only register a clock if those local oscillators actually 
exist. I also can't figure out how to pass a regmap as platform data, 
the size argument can't be just a pointer and sizeof(*regmap) won't work 
either.

2. there is a mutual dependency between codec and clock driver. The 
codec exposes the gpios needed by the clock driver which in turn 
registers a clock needed by the codec driver. is it acceptable to create 
the clock platform device from the codec driver to remove any sort of 
race conditions or need for deferred probes?

Thanks

-Pierre