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