Re: [RFC PATCH 0/7] clk: sunxi-ng: Add a RTC CCU driver

[Samuel Holland]

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