Re: [PATCH 1/2] ASoC: max98090: Add master clock handling

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]