On Mon, Aug 22, 2016 at 07:22:47PM +0200, Sylwester Nawrocki wrote:
On 08/22/2016 11:22 AM, Charles Keepax wrote:
Yeah I am not sure this is quite the correct approach, there are quite a few corner cases that would not be covered well here. For example an internally divided down 32k in which case both the 32k and MCLK would come from the same pin, or using the 32k to feed an FLL in which case we are trying to enable MCLK1 unnecessarily.
I think we could request the 32k clock source from this part of the code, but without implementing clock drivers for the chips internal clocking I think the main MCLK would need to be requested from the machine driver for now.
On that note, I have been working on a patch chain that adds an actual clock driver for the chip unfortunately this has been delayed somewhat due to issues interfacing SPI backed clocks to the clock framework. Krzysztof Kozlowski has sent a series that appears to resolve these issues for me so hopefully once the clock guys have had a look at that I can send my clock driver. My current implementation mostly just implements the 32k clock but we can start building that out to include the SYSCLKs and FLLs etc. fairly quickly. The best thing might be to wait for that and then build additional features onto that.
Let me know if you want to get an early look at that code.
Thanks for the feedback. I would really like to avoid touching this code and messing up anything in code shared by multiple CODECs. You certainly know it much better than than I do.
I got suggestion from Mark not to request the main MCLK clock in the machine driver. But even if gating of that clock was added to the CODEC driver I would need to get hold of it in the machine driver to get rate of MCLK. So I thought about exporting a helper from the MFD for requesting MCLK clock or specifying MCLK clock back in the sound DT node.
IIUC, you are suggesting to leave the 32k parts of the patch and just drop the MCLK part?
I would certainly be ok with that it is very similar to what my patch does but done from the MFD driver rather than moving things out into a clock driver. Although it looks like in this case we need to solve both clocks for it to be useful to you.
If we provided an interface like:
struct clk * arizona_get_mclk(struct arizona *arizona, int id); void arizona_put_mclk(struct clk *clk);
the machine driver would need to get hold of struct arizona*, which is not that straightforward if we are given on CODEC of_node (it can be a child of I2S or SPI bus).
Then how about specifying MCLK in the sound node and using regular devm_clk_get()?
Yeah I think we want to avoid specifying the MCLK in the sound node as it really is a clock the codec consumes so should be defined there in the DT.
Regarding the clock locking patches, I think it needs some more discussion and should rather be merged early in the development cycle to have good exposure in -next as it's quite an invasive change.
I would agree there, and I am not sure how clear it is what the clock guys will make of the approach yet as well. Which might cause issues if we want to merge something now.
I'd be happy to look at your code, if only to have a better overview and to avoid interfering with you work.
I am afraid I haven't managed to get time today but I will fire you an email with my current work in progress stuff, hopefully tomorrow.
Anyway, my main goal is only to get the tm2_wm5110 sound card upstream, with as little casualties as possible;)
Apologies it seems I missed another version of this on the mailing list, please do feel free to add me or the patches@opensource.wolfsonmicro.com list as a direct CC on any patches using our CODECs, I am always more than happy to look over things and feel bad when I miss stuff. I will have a look at the latest version of the patch.
I think we should be able to do something requesting the 32k approximately as your existing patch here does, but then requesting the main MCLK from the set_pll and set_sysclk handlers. Eventually I would like the internal SYSCLK and FLLs represented in the clock framework, so I want to have a quick think about how that would migrate over. Let me see what I can come up with here.
Thanks, Charles