Re: [PATCH v3 2/4] clk: spacemit: extract common ccu functions

[Alex Elder <elder@riscstar.com>]

On 1/6/26 4:27 PM, Yixun Lan wrote:
> Hi Alex,
> 
> On 08:43 Tue 06 Jan     , Alex Elder wrote:
>> On 1/3/26 1:26 AM, Yixun Lan wrote:
>>> Refactor the probe function of SpacemiT's clock, and extract a common ccu
>>> file, so new clock driver added in the future can share the same code,
>>> which would lower the burden of maintenance. Since this commit changes the
>>> module name where the auxiliary device registered, the auxiliary device id
>>> need to be adjusted. Idea of the patch comes from the review of K3 clock
>>> driver, please refer to this disucssion[1] for more detail.
>>
>> Are all of the hunks of moved code moved without change (I
>> think so)?  If so I think it's worth mentioning that.  If
>> not, you should explain whatever differs, and why.  (I would
> yes, no literal changes with this patch except probe() refactored,
> and the real effective change is the module name changed which
> I mentioned already

Thanks for confirming.  No need to change your description,
just consider this for future patches.

>> expect the only thing that would have to change is making
>> spacemit_ccu_probe() public.)
> to make spacemit_ccu_probe() public, we move SoC specific code
> out of this function which should have no functionality change..
> 
> (I think the above commit message is ok, and would not plan to send
> out another version if no serious comment incoming, unless you insist)
> 
>>
>> I made one minor comment below.  I didn't verify, but I
>> assume this is all just moving the code around, and based
>> on that:
>>
>> Reviewed-by: Alex Elder <elder@riscstar.com>
>>
> [snip]...
>>> diff --git a/drivers/clk/spacemit/ccu_common.c b/drivers/clk/spacemit/ccu_common.c
>>> index 4412c4104dab..5f05b17f8452 100644
>>> --- a/drivers/clk/spacemit/ccu_common.c
>>> +++ b/drivers/clk/spacemit/ccu_common.c
>>> @@ -1,6 +1,177 @@
>>>    // SPDX-License-Identifier: GPL-2.0-only
>>>    
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/device/devres.h>
>>> +#include <linux/mfd/syscon.h>
>>>    #include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/slab.h>
>>> +#include <soc/spacemit/ccu.h>
>>> +
>>> +#include "ccu_common.h"
>>> +
>>> +static DEFINE_IDA(auxiliary_ids);
>>
>> I'd insert a space here to make the definition above stand out a
>> bit more.
>>
> do you mean a blank line?
> (I could do this while applying this patch since it's quite trivial..)

Yes that's what I mean, and yes, fine to add that when applying.

					-Alex

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv