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

[Yixun Lan <dlan@gentoo.org>]

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

> 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..)

-- 
Yixun Lan (dlan)

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