On 9/3/21 9:50 AM, Maxime Ripard wrote: > Hi, > > On Wed, Sep 01, 2021 at 12:39:44AM -0500, Samuel Holland wrote: >> This patch series adds a CCU driver for the RTC in the H616 and R329. >> The extra patches at the end of this series show how it would be >> explanded to additional hardware variants. >> >> The driver is intended to support the existing binding used for the H6, >> but also an updated binding which includes all RTC input clocks. I do >> not know how to best represent that binding -- that is a major reason >> why this series is an RFC. >> >> A future patch series could add functionality to the driver to manage >> IOSC calibration at boot and during suspend/resume. >> >> It may be possible to support all of these hardware variants in the >> existing RTC clock driver and avoid some duplicate code, but I'm >> concerned about the complexity there, without any of the CCU >> abstraction. >> >> This series is currently based on top of the other series I just sent >> (clk: sunxi-ng: Lifetime fixes and module support), but I can rebase it >> elsewhere. > > I'm generally ok with this, it makes sense to move it to sunxi-ng, > especially with that other series of yours. > > My main concern about this is the split driver approach. We used to have > that before in the RTC too, but it was mostly due to the early clock > requirements. With your previous work, that requirement is not there > anymore and we can just register it as a device just like the other > clock providers. That's a good point. Originally, I had this RTC CCU providing osc24M, so it did need to be an early provider. But with the current version, we could have the RTC platform driver call devm_sunxi_ccu_probe. That does seem cleaner. (Since it wasn't immediately obvious to me why this works: the only early provider remaining is the sun5i CCU, and it doesn't use the sun6i RTC driver.) > And since we can register all those clocks at device probe time, we > don't really need to split the driver in two (and especially in two > different places). The only obstacle to this after your previous series > is that we don't have of_sunxi_ccu_probe / devm_sunxi_ccu_probe > functions public, but that can easily be fixed by moving their > definition to include/linux/clk/sunxi-ng.h Where are you thinking the clock definitions would go? We don't export any of those structures (ccu_mux, ccu_common) or macros (SUNXI_CCU_GATE_DATA) in a public header either. Would you want to export those? That seems like a lot of churn. Or would we put the CCU descriptions in drivers/clk/sunxi-ng and export a function that the RTC driver can call? (Or some other idea?) Regards, Samuel
