Am 2021-03-11 15:29, schrieb Sameer Pujar:
On 3/11/2021 4:46 PM, Michael Walle wrote:
Am 2021-03-11 12:05, schrieb Sameer Pujar:
It would work and initially I had similar patch, see [0] and related series. Suggestion is to always use "clocks" property with devices only.
I see. But again, I don't think it is correct to change the clock of the codec by default. What happens if this is for example a compatible = "fixed-clock"?
The codec rate won't be changed unless a corresponding "*mclk-fs" is provided.
As you pointed out in the referred thread [0]. simple-audio-card has that clock and judging from the code it is exactly for this reason: to either change/enable it or not.
With this patch you'll switch that to "always change it". Therefore, shouldn't there be a dt flag to indicate wheter simple-audio-card/graph should be in charge of the codecs clock input?
As mentioned above, it does not change always. Requires "*mclk-fs" to do so.
As mentioned earlier, this is changing the sysclk, too. And I'd guess most codecs need a fixed factor for the sysclk, thus if the codec supports generating its own sysclk by a PLL it will need this factor, too.
Which is also defined in the binding:
system-clock-frequency: description: | If a clock is specified and a multiplication factor is given with mclk-fs, the clock will be set to the calculated mclk frequency when the stream starts.
May be below could be a possible alternative?
- Re-order if-else of clock parsing.
if (!of_property_read_u32(node, "system-clock-frequency", &val)) { // Since you are fixing rate already via "assigned-clocks" this may be a duplication. OR
exactly. and also need a device tree change (also for older kernels) on my side.
This could be a last resort, yes. But I'd rather see a flag which indicates whether the simple-audio-card should control the (first) clock of the codec or not. Because until now, this wasn't the case. And I don't know if this was an oversight or on purpose. Kuninori would need to comment on that. And with the "we change mclk by default", we break codecs with automatic sysclk generation.
// "assigned-clocks" can be parsed to understand if a fixed rate is expected.
Sounds like a hack to me. Esp. its doing the same as its already doing right now. That is a "sysfreq = clk_get_rate(codec)".
simple_dai->sysclk = val; } else { // fetch MCLK clock from device and setup sysclk // a. If "*mclk-fs" is given and "clocks" is found, the rate would be updated. // b. If "*mclk-fs" is not mentioned and "clocks" is found, then simple-card utils won't touch rate. It will just do clock enable/disable.
mclk-fs is also a factor for the sysclk, thus it is also needed if there is no mclk (or a fixed mclk).
I don't think you can deduce whether you can change the codecs first clock with the current information :(
}
And its fetching just the first clock, doesn't it? What happens if a codec has two clock inputs?
Yes, it would have been more descriptive if it were specifically looking for clock "mclk". I think the original assumption was codec takes one input clock (MCLK) and uses it for sysclk.
Yeah, I've just noticed that the clk_get_rate() also only works for the first clock of the codec.
-michael