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

[Pierre-Louis Bossart]

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 :-)