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

[Alex Elder]

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@xxxxxxxxxxxx>
> [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