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:
signature.asc
Description: Digital signature
