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

[Mark Brown <broonie@kernel.org>]

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]