On Thu, May 22, 2014 at 11:50:55AM -0600, Stephen Warren wrote: > I think we should nail down exactly what set_sysclk() means. Since it > takes an explicit MCLK clock rate (rather than e.g. sample rate) right > now, surely it's a notification of what the clock /is/, not a request > for the CODEC to set up its input clock. If we expect the CODEC to go to It really should be that from a device model/clock API point of view and it certainly is with the patch proposed, the fact that it happens not to have been is just a product of the poor clock support in Linux than anything else. > the clock driver and request an MCLK for itself (e.g. based on sample > rate), surely that should happen from some function besides > set_sysclk(), with different semantics e.g. hw_params(). I'm not sure where you see the CODEC going off and deciding the MCLK rate itself here? Essentially all that's happening here is that set_sysclk() is behaving like the clock API does and setting its own rate to what it was explicitly asked to set, including bouncing that request up the chain. The rounding could go if we wanted to be a bit stricter - if the machine driver is doing a good job it should never change anything - but otherwise I just can't see what you're concerned about. > I'm not convinced that the CODEC can trigger its input clock changes in > general. In Tegra, there needs to be centralized clock management, e.g. > to prevent one audio stream using a 44.1KHz-based rate and another using > a 48KHz-based rate. That's because the main audio PLL can't generate > both sets of frequencies at once. This can't be done in individual CODEC > drivers, since they shouldn't know about the Tegra restrictions. Doing > it in the clock driver in response to the CODEC's request for a specific > input clock, might work, but then the CODEC would have to call into the > clk driver from e.g. hw_params() rather than set_sysclk(). If that's how > it's supposed to work, then this patch is adding code in the wrong > place. If set_sysclk() doesn't get removed from the CODEC API, then > doing all this clock setup logic in the machine driver, as the > tegra_wm89090.c machine driver does, seems to make the most sense for now. What you're describing seems to make things worse not better for your problem? I'm sorry, I just can't follow what you're concerned about here at all. The current situation is that the CODEC just gets told what it should run SYSCLK at, all the patch does really is factor out the code to call clk_set_rate() on a programmable parent clock and look that clock up in DT. Both before and after the patch the CODEC driver takes no decisions on the rate. If we started doing things in hw_params() the CODEC driver would have to start taking decisions since it would only get the sample rate and so on provided.
Attachment:
signature.asc
Description: Digital signature
