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