On Tue, Jan 26, 2010 at 03:08:44PM +0100, Guennadi Liakhovetski wrote:
here f_MCLK is what is fed into the codec, f_OPCLK is the master clock, that the codec is generating for the outside world, PLL_mul, MCLK_div, and OPCLK_div are internal coefficients, that have to be configured. So, there are 3 equations with 4 unknowns: f_PLLOUT, PLL_mul, MCLK_div, and OPCLK_div. Since there are additional constraints on all of them, there are usually not too many possible solutions. Still, this system is not uniquely resolvable and you have to use some heuristics to select a specific solution.
Right, the CODEC driver needs something to tell it what frequency to generate from both the PLL and OPCLK. Given the PLL configuration it can work out the CODEC-side bits of things but it has no idea what's going on with OPCLK or how that is related to the PLL.
Now, Mark, you are suggesting, that the board code should call
snd_soc_dai_set_pll(dai, 0, 0, f_in, f_out);
with f_out = f_PLLOUT. This means, the board has to use its own heuristics to configure the codec, which I find a bad idea. Especially since there is not a single simple recipe like "f_PLLOUT = f_OPCLK, OPCLK_div = 1," you really have to vary all those variables to get to a suitable result. Now,
The CODEC only needs to know OPCLKDIV and the PLL output frequency. It doesn't really care that these are related. There's no way it can know what the requirements are for OPCLK, when it's safe to vary it or in what way it's safe to vary it - reconfiguring the PLL requires that the PLL be stopped, which may be undesirable. The CODEC driver can't assume that OPCLK is only needed when the CODEC is handling audio, and
There's also the fact that set_pll() normally does exactly that - it configures the PLL. If you start making it do other magic
You remember those superfluous "was-comments," that we have removed. See, the board driver chooses an OPCLK_div, and, thereby, f_PLLOUT. And this is
I'm not sure how those comments are directly related to this issue?
exactly what I disliked about that code and was quite happy to remove. If you insist, I will put it back and switch to using f_PLLOUT for .set_pll(), but I emphasise, that I really dislike this approach, and I really believe, that the board really should keep its fingers away from devices' internals - ever. The only thing that the board knows is input clock, required output clock and sampling rate. The rest should be solely a matter of specific device drivers.
The machine driver already needs to know about the clocking scheme for the system - at the end of the day it's entirely specific to the combination of CODEC, CPU, whatever other components are there, the way they've been wired up and anything system level that software has to be able to achieve. Solving this sort of problem in a generic fashion is non-trivial, there's a reason why we don't have an algorithm for it hidden away in a generic clock API.
If you want to try to do this automatically I'd say that at a bare minimum you need to remove set_pll() from the driver API and make configuring OPCLK a bolt on which is not part of the standard flow for using the device with the PLL. Use set_sysclk() to specify the input frequency for the PLL, and use something like the set_clkdiv() API to specify the OPCLK output rate.
Your previous patches have all treated the PLL output frequency and OPCLK frequency too interchangibly (or documented themselves as doing so) - if the normal flow is worrying about what OPCLK is or if the system clock is being stored as OPCLK something is wrong.