On 23 May 2014 16:44, Mark Brown broonie@kernel.org wrote:

On Fri, May 23, 2014 at 11:05:17AM +0530, Tushar Behera wrote:

...

I tested with moving clk_enable/clk_disable calls to set_bias_level(): SND_SOC_BIAS_PREPARE. That works well for me. Does it look okay?

...

•           if (!IS_ERR(max98090->mclk)) {
  

•                   if (codec->dapm.bias_level == SND_SOC_BIAS_ON)
  

•                           clk_disable(max98090->mclk);
  

•                   else
  

•                           clk_enable(max98090->mclk);
  

•           }
            break;
  

Should be clk_prepare_enable() and similarly for the disable and you should check the error codes but yes, that looks good.

I was planning to keep clk_prepare/clk_unprepare in probe/remove.

We are not doing anything specific with the return codes here, so might be I will add only a error message in case enable/disable fails.

On 23 May 2014 17:11, Mark Brown broonie@kernel.org wrote:

On Fri, May 23, 2014 at 05:06:27PM +0530, Tushar Behera wrote:

...

On 23 May 2014 16:44, Mark Brown broonie@kernel.org wrote:

...

...

Should be clk_prepare_enable() and similarly for the disable and you should check the error codes but yes, that looks good.

...

I was planning to keep clk_prepare/clk_unprepare in probe/remove.

Why - what purpose would it serve to leave the clock prepared but not enabled?

I was getting some kernel slow path warning initially while preparing this code, but not able to reproduce that warning now.

On Thu, May 22, 2014 at 09:51:38AM -0600, Stephen Warren wrote:

On 05/22/2014 03:17 AM, Tushar Behera wrote:

...

• if (!IS_ERR(max98090->mclk)) {

• freq = clk_round_rate(max98090->mclk, freq);
  

• clk_set_rate(max98090->mclk, freq);
  

• }

What are the intended semantics of set_sysclk()? sound/soc/tegra/tegra_wm98090.c assumes that set_sysclk() is a notification to the CODEC driver to tell it what rate the MCLK input is set to (the rate is set before calling set_sysclk), whereas the code above assumes that this function is to tell the CODEC to somehow configure its input clock to be a particular rate. I have a feeling the code above might fail on Tegra.

It's a bit of both. Since we've never had a generic clock API (and still don't really due to everything being a shambles) it's generally just a notification to the CODEC that it's at the specified rate, however if the CODEC knows about its dependencies it seems sensible for it to tell the clock API about what was asked. Ideally we want to remove all the ASoC specific clock control and use the clock API.

The Tegra driver will presumably succeed unless someone does the appropriate clock hookup in DT, at which point clk_set_rate() for the rate the clock is already at really ought to succeed so things should continue to work.

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.

On Thu, May 22, 2014 at 04:24:55PM -0600, Stephen Warren wrote:

My main worry is that this patch opens the door for set_sysclk() to perform some kind of calculation to determine the MCLK rate. Right now, this patch doesn't do that, but there's nothing obvious from the code that no CODEC is allowed to do that. After all, sysclk has a parameter to indicate *which* clock in the CODEC to set. Some CODEC driver author might write something where the machine driver tells the CODEC driver the desired rate of some CODEC-internal PLL, from which set_sysclk() calculates what it needs for MCLK, and then goes off and requests that value from the clock API.

I really think you're reading too much into this - the set_sysclk() API isn't any different to the clock API here really and most of the potential for doing really problematic stuff and defining your clocks to mean funny things exists anyway. Of course people could do tasteless things but that's always going to be a risk and we also want to try to minimise the amount of redundant code people have to write.

It should be fairly easy to spot substantial abuse since the driver would need to call clk_set_rate() with a different rate.

Ignoring that, I'm still not sure that the CODEC driver setting the MCLK rate is appropriate. If we have 1 MCLK output from an SoC, connected to 2 different CODECs, why wouldn't the overall machine driver call clk_set_rate() on that clock, and then notify the two CODEC drivers of that rate. Making each CODEC's set_sysclk() call clk_set_rate() on the same clock with the same value seems redundant, albeit it should work out fine since they both request the same rate.

Right, it should work fine and it's less work for the machine drivers. The alternative is that every machine driver using a device with a programmable input has the code to get that clock from the CODEC device and set it in sync with telling the CODEC about it which is redundant and makes life harder for generic drivers like simple card (which obviously can't know the specifics of every CODEC it works with).

It's certainly a more normal thing from a device model and device tree point of view for the CODEC to be responsible for getting the resource and it seems natural to extend that to such basic management as setting the rate.

If anything I think want to expose some or all of the CODEC clock trees to the clock API, sometimes (rarely but not never) the CODEC PLLs and FLLs are used to supply things outside the audio subsystem if they happen to be going spare, it's difficult to do that at the minute since there's no guaranteed API for being a clock provider. At the minute we're mostly reimplementing a custom clock API which seems like the wrong way to be doing things. That might go towards answering your concerns since set_sysclk() would end up as a clock API call (or at least a thin wrapper around one).

On Thu, May 22, 2014 at 02:47:08PM +0530, Tushar Behera wrote:

If master clock is provided through device tree, then update the master clock frequency during set_sysclk.

Same issues here.