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

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 05/22/2014 11:34 AM, Mark Brown wrote:
> 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.

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
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 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.

> 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.

Ah yes, if we don't add the clock to DT, this code won't do anything, so
nothing will break.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html