On 12/18/18 5:32 AM, Dimitris Papavasiliou wrote:
On 12/17/18 8:08 PM, Pierre-Louis Bossart wrote:
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).
I have documented, in a previous message, various approaches I tried to prevent the pop, which were mostly based on the assumption that the clock source was changed too "abruptly", or wasn't allowed to settle. In the end, the only state, where the clocks can be switched without a pop seemed to be when the chip is suspended.
It would of course be great if this could be achieved in a natural manner, by waiting for the card to be suspended and only switching clocks in such a state, but I don't see how this can be achieved robustly. The power management behavior is outside of the control of the machine driver, so it can only refuse to switch sample rate when it's not suspended. Perhaps this will work in practice, but I fear it would break applications depending on less restrained control of the hardware.
My point was that the machine driver can track DAPM events and put the device in a 'safe' state on SND_SOC_DAPM_EVENT_OFF so that the rate can be changed when playback restarts. I don't see what prevents us from using the same config as during suspend? It's pretty common to play with clocks with these events, and calling clk_disable_unprepare() could lead to whatever configuration sequence is needed for pop-free restart on startup.