[PATCH v2 0/3] clk: spacemit: refactor common ccu driver

[PATCH v2 0/3] clk: spacemit: refactor common ccu driver

[Yixun Lan]

The goal here is to refactor the clock driver to extract common part, and
reuse it, so in the future, we can maximize the source code usage once
adding new clock driver (specifically, for the SpacemiT K3 SoC here)

Since reset driver register via auxiliary bus which requires a adjustment
of the auxiliary device id accordingly.

This patch will depend on Kconfig fix for building modules [1], and the idea
come from K3 clock's review[2]

Please note, Patch 1 is needed for both clock and reset driver, so extract it
out as single independent patch. Then it can be pulled into both subsystem.

Link: https://lore.kernel.org/all/20251219012819.440972-1-inochiama@gmail.com/ [1]
Link: https://lore.kernel.org/all/aTo8sCPpVM1o9PKX@pie [2]
Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Changes in v2:
- extract common header file which shared by clock and reset
- add SoC namespace to differentiate reset for auxiliary bus
- Link to v1: https://lore.kernel.org/r/20251220-06-k1-clk-common-v1-0-df28a0a91621@gentoo.org

---
Yixun Lan (3):
      clk: spacemit: prepare common ccu header
      clk: spacemit: extract common ccu functions
      reset: spacemit: fix auxiliary device id

 drivers/clk/spacemit/ccu-k1.c     | 188 ++------------------------------------
 drivers/clk/spacemit/ccu_common.c | 171 ++++++++++++++++++++++++++++++++++
 drivers/clk/spacemit/ccu_common.h |  10 ++
 drivers/reset/reset-spacemit.c    |   2 +-
 include/soc/spacemit/ccu.h        |  21 +++++
 include/soc/spacemit/k1-syscon.h  |  13 +--
 6 files changed, 213 insertions(+), 192 deletions(-)
---
base-commit: 99735a742f7e9a3e7f4cb6c58edf1b38101e7657
change-id: 20251217-06-k1-clk-common-8d1c57995047
prerequisite-message-id: 20251219012819.440972-1-inochiama@gmail.com
prerequisite-patch-id: df430730ed961011cee5c5d47b7ace84b3c5ebb7
prerequisite-patch-id: 64003618c33be925602e46b7543f2c13d3f36474

Best regards,
-- 
Yixun Lan

[PATCH v2 1/3] clk: spacemit: prepare common ccu header

[Yixun Lan]

In order to prepare adding clock driver for new SoC, extract common
ccu header file, so it can be shared by all drivers.

Also introduce a reset name macro, so it can be both used in clock
and reset subsystem, explicitly to make them match each other.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 include/soc/spacemit/ccu.h       | 21 +++++++++++++++++++++
 include/soc/spacemit/k1-syscon.h | 13 +++----------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/include/soc/spacemit/ccu.h b/include/soc/spacemit/ccu.h
new file mode 100644
index 000000000000..84dcdecccc05
--- /dev/null
+++ b/include/soc/spacemit/ccu.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __SOC_SPACEMIT_CCU_H__
+#define __SOC_SPACEMIT_CCU_H__
+
+#include <linux/auxiliary_bus.h>
+#include <linux/regmap.h>
+
+/* Auxiliary device used to represent a CCU reset controller */
+struct spacemit_ccu_adev {
+	struct auxiliary_device adev;
+	struct regmap *regmap;
+};
+
+static inline struct spacemit_ccu_adev *
+to_spacemit_ccu_adev(struct auxiliary_device *adev)
+{
+	return container_of(adev, struct spacemit_ccu_adev, adev);
+}
+
+#endif /* __SOC_SPACEMIT_CCU_H__ */
diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
index 354751562c55..13efa7a30853 100644
--- a/include/soc/spacemit/k1-syscon.h
+++ b/include/soc/spacemit/k1-syscon.h
@@ -5,17 +5,10 @@
 #ifndef __SOC_K1_SYSCON_H__
 #define __SOC_K1_SYSCON_H__
 
-/* Auxiliary device used to represent a CCU reset controller */
-struct spacemit_ccu_adev {
-	struct auxiliary_device adev;
-	struct regmap *regmap;
-};
+#include "ccu.h"
 
-static inline struct spacemit_ccu_adev *
-to_spacemit_ccu_adev(struct auxiliary_device *adev)
-{
-	return container_of(adev, struct spacemit_ccu_adev, adev);
-}
+/* Reset name macro, should match in clock and reset */
+#define _K_RST(_unit)			"k1-" #_unit "-reset"
 
 /* APBS register offset */
 #define APBS_PLL1_SWCR1			0x100

-- 
2.52.0

Re: [PATCH v2 1/3] clk: spacemit: prepare common ccu header

[Alex Elder]

On 12/26/25 12:55 AM, Yixun Lan wrote:
> In order to prepare adding clock driver for new SoC, extract common
> ccu header file, so it can be shared by all drivers.

You are moving the definition of the SpacemiT CCU auxiliary
device structure, plus the to_spacemit_ccu_adev() function,
into a new header file.  The reason you're doing this is
because these two things are generic, but they're defined
in the K1 SoC-specific header file "k1-syscon.h".  So you
are creating a new header file for this purpose.

These are things you should explain here, to help orient
reviewers and will inform anyone in the future looking at
commit history.

> Also introduce a reset name macro, so it can be both used in clock
> and reset subsystem, explicitly to make them match each other.

This should go in a separate patch, and should change the
code to use the macro so it builds and continues to function
with the new change place.

However I don't understand why you think it's necessary to
introduce the reset name macro.  Is it because you want to
incorporate an SoC identifier in the name?

Even if this is your reason, I still don't think you need
the macro.  I'll try to explain what I mean in the
next patch.

One more comment, below.

> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>   include/soc/spacemit/ccu.h       | 21 +++++++++++++++++++++
>   include/soc/spacemit/k1-syscon.h | 13 +++----------
>   2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/include/soc/spacemit/ccu.h b/include/soc/spacemit/ccu.h
> new file mode 100644
> index 000000000000..84dcdecccc05
> --- /dev/null
> +++ b/include/soc/spacemit/ccu.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __SOC_SPACEMIT_CCU_H__
> +#define __SOC_SPACEMIT_CCU_H__
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/regmap.h>
> +
> +/* Auxiliary device used to represent a CCU reset controller */
> +struct spacemit_ccu_adev {
> +	struct auxiliary_device adev;
> +	struct regmap *regmap;
> +};
> +
> +static inline struct spacemit_ccu_adev *
> +to_spacemit_ccu_adev(struct auxiliary_device *adev)
> +{
> +	return container_of(adev, struct spacemit_ccu_adev, adev);
> +}
> +
> +#endif /* __SOC_SPACEMIT_CCU_H__ */
> diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
> index 354751562c55..13efa7a30853 100644
> --- a/include/soc/spacemit/k1-syscon.h
> +++ b/include/soc/spacemit/k1-syscon.h
> @@ -5,17 +5,10 @@
>   #ifndef __SOC_K1_SYSCON_H__
>   #define __SOC_K1_SYSCON_H__
>   
> -/* Auxiliary device used to represent a CCU reset controller */
> -struct spacemit_ccu_adev {
> -	struct auxiliary_device adev;
> -	struct regmap *regmap;
> -};
> +#include "ccu.h"
>   
> -static inline struct spacemit_ccu_adev *
> -to_spacemit_ccu_adev(struct auxiliary_device *adev)
> -{
> -	return container_of(adev, struct spacemit_ccu_adev, adev);
> -}
> +/* Reset name macro, should match in clock and reset */
> +#define _K_RST(_unit)			"k1-" #_unit "-reset"

The generic-sounding _K_RST() encodes "k1" in the name,
and it shouldn't.  Also, why do you use the underscore
prefix?

Anyway, I'll keep reading.

					-Alex

>   
>   /* APBS register offset */
>   #define APBS_PLL1_SWCR1			0x100
> 

Re: [PATCH v2 1/3] clk: spacemit: prepare common ccu header

[Yixun Lan]

Hi Alex,

On 18:50 Mon 29 Dec     , Alex Elder wrote:
> On 12/26/25 12:55 AM, Yixun Lan wrote:
> > In order to prepare adding clock driver for new SoC, extract common
> > ccu header file, so it can be shared by all drivers.
> 
> You are moving the definition of the SpacemiT CCU auxiliary
> device structure, plus the to_spacemit_ccu_adev() function,
> into a new header file.  
yes, and this is explaining the code which I consider not necessary,
it's more obvious to read the code..

> The reason you're doing this is
> because these two things are generic, but they're defined
> in the K1 SoC-specific header file "k1-syscon.h".  So you
> are creating a new header file for this purpose.
> 
right
> These are things you should explain here, to help orient
> reviewers and will inform anyone in the future looking at
> commit history.
I thought I've explained the goal/motivation already with above
commit message, maybe I can improve it, so how about:

In order to prepare adding clock driver for new K3 SoC, extract generic
code to a separate common ccu header file, so they are not defined
in K1 SoC-specific file, and then can be shared by all drivers.

> 
> > Also introduce a reset name macro, so it can be both used in clock
> > and reset subsystem, explicitly to make them match each other.
> 
> This should go in a separate patch, and should change the
> code to use the macro so it builds and continues to function
> with the new change place.
>
yes, I could do this in a separate patch

> However I don't understand why you think it's necessary to
> introduce the reset name macro.  Is it because you want to
> incorporate an SoC identifier in the name?
> 
I've explained here:
https://lore.kernel.org/r/20251231020951-GYA2019108@gentoo.org

It's necessary to incorporate the SoC identifier which will help
to differentiate K1 and K3 reset driver, otherwise there will be
driver name collision, lead to reset driver probe failure while
adding K3 SoC ..

> Even if this is your reason, I still don't think you need
> the macro.  I'll try to explain what I mean in the
> next patch.
> 
If you still have concerns, and we can't reach certain agreement,
then I could drop this macro in next version, leave this optimization
to future patches, I don't want main clock driver delayed by it.

I personally tend to keep the macro, but probably the naming need some
improvement..

> One more comment, below.
> 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >   include/soc/spacemit/ccu.h       | 21 +++++++++++++++++++++
> >   include/soc/spacemit/k1-syscon.h | 13 +++----------
> >   2 files changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/soc/spacemit/ccu.h b/include/soc/spacemit/ccu.h
> > new file mode 100644
> > index 000000000000..84dcdecccc05
> > --- /dev/null
> > +++ b/include/soc/spacemit/ccu.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#ifndef __SOC_SPACEMIT_CCU_H__
> > +#define __SOC_SPACEMIT_CCU_H__
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/regmap.h>
> > +
> > +/* Auxiliary device used to represent a CCU reset controller */
> > +struct spacemit_ccu_adev {
> > +	struct auxiliary_device adev;
> > +	struct regmap *regmap;
> > +};
> > +
> > +static inline struct spacemit_ccu_adev *
> > +to_spacemit_ccu_adev(struct auxiliary_device *adev)
> > +{
> > +	return container_of(adev, struct spacemit_ccu_adev, adev);
> > +}
> > +
> > +#endif /* __SOC_SPACEMIT_CCU_H__ */
> > diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
> > index 354751562c55..13efa7a30853 100644
> > --- a/include/soc/spacemit/k1-syscon.h
> > +++ b/include/soc/spacemit/k1-syscon.h
> > @@ -5,17 +5,10 @@
> >   #ifndef __SOC_K1_SYSCON_H__
> >   #define __SOC_K1_SYSCON_H__
> >   
> > -/* Auxiliary device used to represent a CCU reset controller */
> > -struct spacemit_ccu_adev {
> > -	struct auxiliary_device adev;
> > -	struct regmap *regmap;
> > -};
> > +#include "ccu.h"
> >   
> > -static inline struct spacemit_ccu_adev *
> > -to_spacemit_ccu_adev(struct auxiliary_device *adev)
> > -{
> > -	return container_of(adev, struct spacemit_ccu_adev, adev);
> > -}
> > +/* Reset name macro, should match in clock and reset */
> > +#define _K_RST(_unit)			"k1-" #_unit "-reset"
> 
> The generic-sounding _K_RST() encodes "k1" in the name,
> and it shouldn't.  Also, why do you use the underscore
> prefix?
> 
want to make it slightly generic/short but still keep it local for K1 driver,
and also avoid potential collision with other drivers in kernel code..

or do you have any sugestion for better naming?
> Anyway, I'll keep reading.
> 
> 					-Alex
> 
> >   
> >   /* APBS register offset */
> >   #define APBS_PLL1_SWCR1			0x100
> > 
> 
> 

-- 
Yixun Lan (dlan)

Re: [PATCH v2 1/3] clk: spacemit: prepare common ccu header

[Alex Elder]

On 1/1/26 8:38 AM, Yixun Lan wrote:
> Hi Alex,
> 
> On 18:50 Mon 29 Dec     , Alex Elder wrote:
>> On 12/26/25 12:55 AM, Yixun Lan wrote:
>>> In order to prepare adding clock driver for new SoC, extract common
>>> ccu header file, so it can be shared by all drivers.
>>
>> You are moving the definition of the SpacemiT CCU auxiliary
>> device structure, plus the to_spacemit_ccu_adev() function,
>> into a new header file.
> yes, and this is explaining the code which I consider not necessary,
> it's more obvious to read the code..
> 
>> The reason you're doing this is
>> because these two things are generic, but they're defined
>> in the K1 SoC-specific header file "k1-syscon.h".  So you
>> are creating a new header file for this purpose.
>>
> right
>> These are things you should explain here, to help orient
>> reviewers and will inform anyone in the future looking at
>> commit history.
> I thought I've explained the goal/motivation already with above
> commit message, maybe I can improve it, so how about:
> 
> In order to prepare adding clock driver for new K3 SoC, extract generic
> code to a separate common ccu header file, so they are not defined
> in K1 SoC-specific file, and then can be shared by all drivers.

This would be much better.  You don't need to explain every detail
of the code, but providing the motivation this way and explaining
it at a high level helps the reader a lot.

>>> Also introduce a reset name macro, so it can be both used in clock
>>> and reset subsystem, explicitly to make them match each other.
>>
>> This should go in a separate patch, and should change the
>> code to use the macro so it builds and continues to function
>> with the new change place.
>>
> yes, I could do this in a separate patch
> 
>> However I don't understand why you think it's necessary to
>> introduce the reset name macro.  Is it because you want to
>> incorporate an SoC identifier in the name?
>>
> I've explained here:
> https://lore.kernel.org/r/20251231020951-GYA2019108@gentoo.org
> 
> It's necessary to incorporate the SoC identifier which will help
> to differentiate K1 and K3 reset driver, otherwise there will be
> driver name collision, lead to reset driver probe failure while
> adding K3 SoC ..

I just had a talk with Guodong and he helped clear up a
misunderstanding I had about this.  I was thinking about
what happens at probe time, and that only the K1 or the
K3 CCU will get registered.

But he explained that the issue is that two *drivers* claim
to support the same "compatible" auxiliary device name, and
even if only the K1 CCU got registered, both reset drivers
are available in the kernel and you still need to specify
which reset driver you want use.

You are implementing both the K1 and K3 reset code in the
same module, which I think is why this is necessary.

>> Even if this is your reason, I still don't think you need
>> the macro.  I'll try to explain what I mean in the
>> next patch.
>>
> If you still have concerns, and we can't reach certain agreement,
> then I could drop this macro in next version, leave this optimization
> to future patches, I don't want main clock driver delayed by it.

No I no longer have concerns and I accept that you need to
encode the platform/SoC in the reset auxiliary device name.

> I personally tend to keep the macro, but probably the naming need some
> improvement..

What I'd prefer is to just name the resets directly, to encode the
platform ("k1" or "k3") where defined.  I.e.,

  static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
-	.reset_name	= "mpmu-reset",
+	.reset_name	= "k1-mpmu-reset",
  	.hws		= k1_ccu_mpmu_hws,
  	.num		= ARRAY_SIZE(k1_ccu_mpmu_hws),
  };

Does this lead to a problem somewhere else?  What does hiding
this convention behind the _K_RST() macro do that's better
than this?  Is it because you want the separate clock and
reset drivers to use the same convention?

I think it's a little more difficult to talk about this because
we're talking about changes that are implemented by two separate
patch series.

>> One more comment, below.
>>
>>> Signed-off-by: Yixun Lan <dlan@gentoo.org>
>>> ---
>>>    include/soc/spacemit/ccu.h       | 21 +++++++++++++++++++++
>>>    include/soc/spacemit/k1-syscon.h | 13 +++----------
>>>    2 files changed, 24 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/soc/spacemit/ccu.h b/include/soc/spacemit/ccu.h
>>> new file mode 100644
>>> index 000000000000..84dcdecccc05
>>> --- /dev/null
>>> +++ b/include/soc/spacemit/ccu.h
>>> @@ -0,0 +1,21 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#ifndef __SOC_SPACEMIT_CCU_H__
>>> +#define __SOC_SPACEMIT_CCU_H__
>>> +
>>> +#include <linux/auxiliary_bus.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +/* Auxiliary device used to represent a CCU reset controller */
>>> +struct spacemit_ccu_adev {
>>> +	struct auxiliary_device adev;
>>> +	struct regmap *regmap;
>>> +};
>>> +
>>> +static inline struct spacemit_ccu_adev *
>>> +to_spacemit_ccu_adev(struct auxiliary_device *adev)
>>> +{
>>> +	return container_of(adev, struct spacemit_ccu_adev, adev);
>>> +}
>>> +
>>> +#endif /* __SOC_SPACEMIT_CCU_H__ */
>>> diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
>>> index 354751562c55..13efa7a30853 100644
>>> --- a/include/soc/spacemit/k1-syscon.h
>>> +++ b/include/soc/spacemit/k1-syscon.h
>>> @@ -5,17 +5,10 @@
>>>    #ifndef __SOC_K1_SYSCON_H__
>>>    #define __SOC_K1_SYSCON_H__
>>>    
>>> -/* Auxiliary device used to represent a CCU reset controller */
>>> -struct spacemit_ccu_adev {
>>> -	struct auxiliary_device adev;
>>> -	struct regmap *regmap;
>>> -};
>>> +#include "ccu.h"
>>>    
>>> -static inline struct spacemit_ccu_adev *
>>> -to_spacemit_ccu_adev(struct auxiliary_device *adev)
>>> -{
>>> -	return container_of(adev, struct spacemit_ccu_adev, adev);
>>> -}
>>> +/* Reset name macro, should match in clock and reset */
>>> +#define _K_RST(_unit)			"k1-" #_unit "-reset"
>>
>> The generic-sounding _K_RST() encodes "k1" in the name,
>> and it shouldn't.  Also, why do you use the underscore
>> prefix?
>>
> want to make it slightly generic/short but still keep it local for K1 driver,
> and also avoid potential collision with other drivers in kernel code..
> 
> or do you have any sugestion for better naming?

First, I suggest you avoid even using such a macro.  But I
could be wrong about that too...

I would name it RESET_NAME(_unit) or something similar.  It's
only used by code and DTS files that are related to SpacemiT
platforms.

					-Alex

>> Anyway, I'll keep reading.
>>
>> 					-Alex
>>
>>>    
>>>    /* APBS register offset */
>>>    #define APBS_PLL1_SWCR1			0x100
>>>
>>
>>
> 

Re: [PATCH v2 1/3] clk: spacemit: prepare common ccu header

[Yixun Lan]

Hi Alex,

On 09:42 Fri 02 Jan     , Alex Elder wrote:
> On 1/1/26 8:38 AM, Yixun Lan wrote:
> > Hi Alex,
> > 
> > On 18:50 Mon 29 Dec     , Alex Elder wrote:
> >> On 12/26/25 12:55 AM, Yixun Lan wrote:
> >>> In order to prepare adding clock driver for new SoC, extract common
> >>> ccu header file, so it can be shared by all drivers.
> >>
> >> You are moving the definition of the SpacemiT CCU auxiliary
> >> device structure, plus the to_spacemit_ccu_adev() function,
> >> into a new header file.
> > yes, and this is explaining the code which I consider not necessary,
> > it's more obvious to read the code..
> > 
> >> The reason you're doing this is
> >> because these two things are generic, but they're defined
> >> in the K1 SoC-specific header file "k1-syscon.h".  So you
> >> are creating a new header file for this purpose.
> >>
> > right
> >> These are things you should explain here, to help orient
> >> reviewers and will inform anyone in the future looking at
> >> commit history.
> > I thought I've explained the goal/motivation already with above
> > commit message, maybe I can improve it, so how about:
> > 
> > In order to prepare adding clock driver for new K3 SoC, extract generic
> > code to a separate common ccu header file, so they are not defined
> > in K1 SoC-specific file, and then can be shared by all drivers.
> 
> This would be much better.  You don't need to explain every detail
> of the code, but providing the motivation this way and explaining
> it at a high level helps the reader a lot.
> 
> >>> Also introduce a reset name macro, so it can be both used in clock
> >>> and reset subsystem, explicitly to make them match each other.
> >>
> >> This should go in a separate patch, and should change the
> >> code to use the macro so it builds and continues to function
> >> with the new change place.
> >>
> > yes, I could do this in a separate patch
> > 
> >> However I don't understand why you think it's necessary to
> >> introduce the reset name macro.  Is it because you want to
> >> incorporate an SoC identifier in the name?
> >>
> > I've explained here:
> > https://lore.kernel.org/r/20251231020951-GYA2019108@gentoo.org
> > 
> > It's necessary to incorporate the SoC identifier which will help
> > to differentiate K1 and K3 reset driver, otherwise there will be
> > driver name collision, lead to reset driver probe failure while
> > adding K3 SoC ..
> 
> I just had a talk with Guodong and he helped clear up a
> misunderstanding I had about this.  I was thinking about
> what happens at probe time, and that only the K1 or the
> K3 CCU will get registered.
> 
> But he explained that the issue is that two *drivers* claim
> to support the same "compatible" auxiliary device name, and
> even if only the K1 CCU got registered, both reset drivers
> are available in the kernel and you still need to specify
> which reset driver you want use.
> 
> You are implementing both the K1 and K3 reset code in the
> same module, which I think is why this is necessary.
> 
> >> Even if this is your reason, I still don't think you need
> >> the macro.  I'll try to explain what I mean in the
> >> next patch.
> >>
> > If you still have concerns, and we can't reach certain agreement,
> > then I could drop this macro in next version, leave this optimization
> > to future patches, I don't want main clock driver delayed by it.
> 
> No I no longer have concerns and I accept that you need to
> encode the platform/SoC in the reset auxiliary device name.
> 
> > I personally tend to keep the macro, but probably the naming need some
> > improvement..
> 
> What I'd prefer is to just name the resets directly, to encode the
> platform ("k1" or "k3") where defined.  I.e.,
> 
>   static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
> -	.reset_name	= "mpmu-reset",
> +	.reset_name	= "k1-mpmu-reset",
ok, I will go this way
>   	.hws		= k1_ccu_mpmu_hws,
>   	.num		= ARRAY_SIZE(k1_ccu_mpmu_hws),
>   };
> 
> Does this lead to a problem somewhere else?  What does hiding
> this convention behind the _K_RST() macro do that's better
> than this?  Is it because you want the separate clock and
> reset drivers to use the same convention?
yes, I was planing to use same convention for clock and reset

> 
> I think it's a little more difficult to talk about this because
> we're talking about changes that are implemented by two separate
> patch series.
that's true

> 
> >> One more comment, below.
> >>
> >>> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> >>> ---
> >>>    include/soc/spacemit/ccu.h       | 21 +++++++++++++++++++++
> >>>    include/soc/spacemit/k1-syscon.h | 13 +++----------
> >>>    2 files changed, 24 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/include/soc/spacemit/ccu.h b/include/soc/spacemit/ccu.h
> >>> new file mode 100644
> >>> index 000000000000..84dcdecccc05
> >>> --- /dev/null
> >>> +++ b/include/soc/spacemit/ccu.h
> >>> @@ -0,0 +1,21 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-only */
> >>> +
> >>> +#ifndef __SOC_SPACEMIT_CCU_H__
> >>> +#define __SOC_SPACEMIT_CCU_H__
> >>> +
> >>> +#include <linux/auxiliary_bus.h>
> >>> +#include <linux/regmap.h>
> >>> +
> >>> +/* Auxiliary device used to represent a CCU reset controller */
> >>> +struct spacemit_ccu_adev {
> >>> +	struct auxiliary_device adev;
> >>> +	struct regmap *regmap;
> >>> +};
> >>> +
> >>> +static inline struct spacemit_ccu_adev *
> >>> +to_spacemit_ccu_adev(struct auxiliary_device *adev)
> >>> +{
> >>> +	return container_of(adev, struct spacemit_ccu_adev, adev);
> >>> +}
> >>> +
> >>> +#endif /* __SOC_SPACEMIT_CCU_H__ */
> >>> diff --git a/include/soc/spacemit/k1-syscon.h b/include/soc/spacemit/k1-syscon.h
> >>> index 354751562c55..13efa7a30853 100644
> >>> --- a/include/soc/spacemit/k1-syscon.h
> >>> +++ b/include/soc/spacemit/k1-syscon.h
> >>> @@ -5,17 +5,10 @@
> >>>    #ifndef __SOC_K1_SYSCON_H__
> >>>    #define __SOC_K1_SYSCON_H__
> >>>    
> >>> -/* Auxiliary device used to represent a CCU reset controller */
> >>> -struct spacemit_ccu_adev {
> >>> -	struct auxiliary_device adev;
> >>> -	struct regmap *regmap;
> >>> -};
> >>> +#include "ccu.h"
> >>>    
> >>> -static inline struct spacemit_ccu_adev *
> >>> -to_spacemit_ccu_adev(struct auxiliary_device *adev)
> >>> -{
> >>> -	return container_of(adev, struct spacemit_ccu_adev, adev);
> >>> -}
> >>> +/* Reset name macro, should match in clock and reset */
> >>> +#define _K_RST(_unit)			"k1-" #_unit "-reset"
> >>
> >> The generic-sounding _K_RST() encodes "k1" in the name,
> >> and it shouldn't.  Also, why do you use the underscore
> >> prefix?
> >>
> > want to make it slightly generic/short but still keep it local for K1 driver,
> > and also avoid potential collision with other drivers in kernel code..
> > 
> > or do you have any sugestion for better naming?
> 
> First, I suggest you avoid even using such a macro.  But I
> could be wrong about that too...
> 
ok, see previous comment
> I would name it RESET_NAME(_unit) or something similar.  It's
> only used by code and DTS files that are related to SpacemiT
> platforms.
> 
will drop the macro

thanks!
> 					-Alex
> 
> >> Anyway, I'll keep reading.
> >>
> >> 					-Alex
> >>
> >>>    
> >>>    /* APBS register offset */
> >>>    #define APBS_PLL1_SWCR1			0x100
> >>>
> >>
> >>
> > 
> 

-- 
Yixun Lan (dlan)

[PATCH v2 2/3] clk: spacemit: extract common ccu functions

[Yixun Lan]

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 is come from review of K3 clock
driver, please refer this disucssion [1].

Link: https://lore.kernel.org/all/aTo8sCPpVM1o9PKX@pie/ [1]
Suggested-by: Yao Zi <me@ziyao.cc>
Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/clk/spacemit/ccu-k1.c     | 188 ++------------------------------------
 drivers/clk/spacemit/ccu_common.c | 171 ++++++++++++++++++++++++++++++++++
 drivers/clk/spacemit/ccu_common.h |  10 ++
 3 files changed, 188 insertions(+), 181 deletions(-)

diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
index 01d9485b615d..f97d6f0f0c39 100644
--- a/drivers/clk/spacemit/ccu-k1.c
+++ b/drivers/clk/spacemit/ccu-k1.c
@@ -5,15 +5,10 @@
  */
 
 #include <linux/array_size.h>
-#include <linux/auxiliary_bus.h>
 #include <linux/clk-provider.h>
-#include <linux/delay.h>
-#include <linux/idr.h>
-#include <linux/mfd/syscon.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/slab.h>
 #include <soc/spacemit/k1-syscon.h>
 
 #include "ccu_common.h"
@@ -23,14 +18,6 @@
 
 #include <dt-bindings/clock/spacemit,k1-syscon.h>
 
-struct spacemit_ccu_data {
-	const char *reset_name;
-	struct clk_hw **hws;
-	size_t num;
-};
-
-static DEFINE_IDA(auxiliary_ids);
-
 /* APBS clocks start, APBS region contains and only contains all PLL clocks */
 
 /*
@@ -802,7 +789,7 @@ static struct clk_hw *k1_ccu_mpmu_hws[] = {
 };
 
 static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
-	.reset_name	= "mpmu-reset",
+	.reset_name	= _K_RST(mpmu),
 	.hws		= k1_ccu_mpmu_hws,
 	.num		= ARRAY_SIZE(k1_ccu_mpmu_hws),
 };
@@ -913,7 +900,7 @@ static struct clk_hw *k1_ccu_apbc_hws[] = {
 };
 
 static const struct spacemit_ccu_data k1_ccu_apbc_data = {
-	.reset_name	= "apbc-reset",
+	.reset_name	= _K_RST(apbc),
 	.hws		= k1_ccu_apbc_hws,
 	.num		= ARRAY_SIZE(k1_ccu_apbc_hws),
 };
@@ -984,184 +971,23 @@ static struct clk_hw *k1_ccu_apmu_hws[] = {
 };
 
 static const struct spacemit_ccu_data k1_ccu_apmu_data = {
-	.reset_name	= "apmu-reset",
+	.reset_name	= _K_RST(apmu),
 	.hws		= k1_ccu_apmu_hws,
 	.num		= ARRAY_SIZE(k1_ccu_apmu_hws),
 };
 
 static const struct spacemit_ccu_data k1_ccu_rcpu_data = {
-	.reset_name	= "rcpu-reset",
+	.reset_name	= _K_RST(rcpu),
 };
 
 static const struct spacemit_ccu_data k1_ccu_rcpu2_data = {
-	.reset_name	= "rcpu2-reset",
+	.reset_name	= _K_RST(rcpu2),
 };
 
 static const struct spacemit_ccu_data k1_ccu_apbc2_data = {
-	.reset_name	= "apbc2-reset",
+	.reset_name	= _K_RST(apbc2),
 };
 
-static int spacemit_ccu_register(struct device *dev,
-				 struct regmap *regmap,
-				 struct regmap *lock_regmap,
-				 const struct spacemit_ccu_data *data)
-{
-	struct clk_hw_onecell_data *clk_data;
-	int i, ret;
-
-	/* Nothing to do if the CCU does not implement any clocks */
-	if (!data->hws)
-		return 0;
-
-	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
-				GFP_KERNEL);
-	if (!clk_data)
-		return -ENOMEM;
-
-	clk_data->num = data->num;
-
-	for (i = 0; i < data->num; i++) {
-		struct clk_hw *hw = data->hws[i];
-		struct ccu_common *common;
-		const char *name;
-
-		if (!hw) {
-			clk_data->hws[i] = ERR_PTR(-ENOENT);
-			continue;
-		}
-
-		name = hw->init->name;
-
-		common = hw_to_ccu_common(hw);
-		common->regmap		= regmap;
-		common->lock_regmap	= lock_regmap;
-
-		ret = devm_clk_hw_register(dev, hw);
-		if (ret) {
-			dev_err(dev, "Cannot register clock %d - %s\n",
-				i, name);
-			return ret;
-		}
-
-		clk_data->hws[i] = hw;
-	}
-
-	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
-	if (ret)
-		dev_err(dev, "failed to add clock hardware provider (%d)\n", ret);
-
-	return ret;
-}
-
-static void spacemit_cadev_release(struct device *dev)
-{
-	struct auxiliary_device *adev = to_auxiliary_dev(dev);
-
-	ida_free(&auxiliary_ids, adev->id);
-	kfree(to_spacemit_ccu_adev(adev));
-}
-
-static void spacemit_adev_unregister(void *data)
-{
-	struct auxiliary_device *adev = data;
-
-	auxiliary_device_delete(adev);
-	auxiliary_device_uninit(adev);
-}
-
-static int spacemit_ccu_reset_register(struct device *dev,
-				       struct regmap *regmap,
-				       const char *reset_name)
-{
-	struct spacemit_ccu_adev *cadev;
-	struct auxiliary_device *adev;
-	int ret;
-
-	/* Nothing to do if the CCU does not implement a reset controller */
-	if (!reset_name)
-		return 0;
-
-	cadev = kzalloc(sizeof(*cadev), GFP_KERNEL);
-	if (!cadev)
-		return -ENOMEM;
-
-	cadev->regmap = regmap;
-
-	adev = &cadev->adev;
-	adev->name = reset_name;
-	adev->dev.parent = dev;
-	adev->dev.release = spacemit_cadev_release;
-	adev->dev.of_node = dev->of_node;
-	ret = ida_alloc(&auxiliary_ids, GFP_KERNEL);
-	if (ret < 0)
-		goto err_free_cadev;
-	adev->id = ret;
-
-	ret = auxiliary_device_init(adev);
-	if (ret)
-		goto err_free_aux_id;
-
-	ret = auxiliary_device_add(adev);
-	if (ret) {
-		auxiliary_device_uninit(adev);
-		return ret;
-	}
-
-	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
-
-err_free_aux_id:
-	ida_free(&auxiliary_ids, adev->id);
-err_free_cadev:
-	kfree(cadev);
-
-	return ret;
-}
-
-static int k1_ccu_probe(struct platform_device *pdev)
-{
-	struct regmap *base_regmap, *lock_regmap = NULL;
-	const struct spacemit_ccu_data *data;
-	struct device *dev = &pdev->dev;
-	int ret;
-
-	base_regmap = device_node_to_regmap(dev->of_node);
-	if (IS_ERR(base_regmap))
-		return dev_err_probe(dev, PTR_ERR(base_regmap),
-				     "failed to get regmap\n");
-
-	/*
-	 * The lock status of PLLs locate in MPMU region, while PLLs themselves
-	 * are in APBS region. Reference to MPMU syscon is required to check PLL
-	 * status.
-	 */
-	if (of_device_is_compatible(dev->of_node, "spacemit,k1-pll")) {
-		struct device_node *mpmu = of_parse_phandle(dev->of_node,
-							    "spacemit,mpmu", 0);
-		if (!mpmu)
-			return dev_err_probe(dev, -ENODEV,
-					     "Cannot parse MPMU region\n");
-
-		lock_regmap = device_node_to_regmap(mpmu);
-		of_node_put(mpmu);
-
-		if (IS_ERR(lock_regmap))
-			return dev_err_probe(dev, PTR_ERR(lock_regmap),
-					     "failed to get lock regmap\n");
-	}
-
-	data = of_device_get_match_data(dev);
-
-	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, data);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to register clocks\n");
-
-	ret = spacemit_ccu_reset_register(dev, base_regmap, data->reset_name);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to register resets\n");
-
-	return 0;
-}
-
 static const struct of_device_id of_k1_ccu_match[] = {
 	{
 		.compatible	= "spacemit,k1-pll",
@@ -1200,7 +1026,7 @@ static struct platform_driver k1_ccu_driver = {
 		.name		= "spacemit,k1-ccu",
 		.of_match_table = of_k1_ccu_match,
 	},
-	.probe	= k1_ccu_probe,
+	.probe	= spacemit_ccu_probe,
 };
 module_platform_driver(k1_ccu_driver);
 
diff --git a/drivers/clk/spacemit/ccu_common.c b/drivers/clk/spacemit/ccu_common.c
index 4412c4104dab..f1a837aafb46 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);
+static int spacemit_ccu_register(struct device *dev,
+				 struct regmap *regmap,
+				 struct regmap *lock_regmap,
+				 const struct spacemit_ccu_data *data)
+{
+	struct clk_hw_onecell_data *clk_data;
+	int i, ret;
+
+	/* Nothing to do if the CCU does not implement any clocks */
+	if (!data->hws)
+		return 0;
+
+	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
+				GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
+	clk_data->num = data->num;
+
+	for (i = 0; i < data->num; i++) {
+		struct clk_hw *hw = data->hws[i];
+		struct ccu_common *common;
+		const char *name;
+
+		if (!hw) {
+			clk_data->hws[i] = ERR_PTR(-ENOENT);
+			continue;
+		}
+
+		name = hw->init->name;
+
+		common = hw_to_ccu_common(hw);
+		common->regmap		= regmap;
+		common->lock_regmap	= lock_regmap;
+
+		ret = devm_clk_hw_register(dev, hw);
+		if (ret) {
+			dev_err(dev, "Cannot register clock %d - %s\n",
+				i, name);
+			return ret;
+		}
+
+		clk_data->hws[i] = hw;
+	}
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
+	if (ret)
+		dev_err(dev, "failed to add clock hardware provider (%d)\n", ret);
+
+	return ret;
+}
+
+static void spacemit_cadev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+	ida_free(&auxiliary_ids, adev->id);
+	kfree(to_spacemit_ccu_adev(adev));
+}
+
+static void spacemit_adev_unregister(void *data)
+{
+	struct auxiliary_device *adev = data;
+
+	auxiliary_device_delete(adev);
+	auxiliary_device_uninit(adev);
+}
+
+static int spacemit_ccu_reset_register(struct device *dev,
+				       struct regmap *regmap,
+				       const char *reset_name)
+{
+	struct spacemit_ccu_adev *cadev;
+	struct auxiliary_device *adev;
+	int ret;
+
+	/* Nothing to do if the CCU does not implement a reset controller */
+	if (!reset_name)
+		return 0;
+
+	cadev = kzalloc(sizeof(*cadev), GFP_KERNEL);
+	if (!cadev)
+		return -ENOMEM;
+
+	cadev->regmap = regmap;
+
+	adev = &cadev->adev;
+	adev->name = reset_name;
+	adev->dev.parent = dev;
+	adev->dev.release = spacemit_cadev_release;
+	adev->dev.of_node = dev->of_node;
+	ret = ida_alloc(&auxiliary_ids, GFP_KERNEL);
+	if (ret < 0)
+		goto err_free_cadev;
+	adev->id = ret;
+
+	ret = auxiliary_device_init(adev);
+	if (ret)
+		goto err_free_aux_id;
+
+	ret = auxiliary_device_add(adev);
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		return ret;
+	}
+
+	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
+
+err_free_aux_id:
+	ida_free(&auxiliary_ids, adev->id);
+err_free_cadev:
+	kfree(cadev);
+
+	return ret;
+}
+
+int spacemit_ccu_probe(struct platform_device *pdev)
+{
+	struct regmap *base_regmap, *lock_regmap = NULL;
+	const struct spacemit_ccu_data *data;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	base_regmap = device_node_to_regmap(dev->of_node);
+	if (IS_ERR(base_regmap))
+		return dev_err_probe(dev, PTR_ERR(base_regmap),
+				     "failed to get regmap\n");
+
+	/*
+	 * The lock status of PLLs locate in MPMU region, while PLLs themselves
+	 * are in APBS region. Reference to MPMU syscon is required to check PLL
+	 * status.
+	 */
+	if (of_device_is_compatible(dev->of_node, "spacemit,k1-pll")) {
+		struct device_node *mpmu = of_parse_phandle(dev->of_node,
+							    "spacemit,mpmu", 0);
+		if (!mpmu)
+			return dev_err_probe(dev, -ENODEV,
+					     "Cannot parse MPMU region\n");
+
+		lock_regmap = device_node_to_regmap(mpmu);
+		of_node_put(mpmu);
+
+		if (IS_ERR(lock_regmap))
+			return dev_err_probe(dev, PTR_ERR(lock_regmap),
+					     "failed to get lock regmap\n");
+	}
+
+	data = of_device_get_match_data(dev);
+
+	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register clocks\n");
+
+	ret = spacemit_ccu_reset_register(dev, base_regmap, data->reset_name);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register resets\n");
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(spacemit_ccu_probe, "CLK_SPACEMIT");
 
 MODULE_DESCRIPTION("SpacemiT CCU common clock driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/clk/spacemit/ccu_common.h b/drivers/clk/spacemit/ccu_common.h
index da72f3836e0b..9b4ef24875e5 100644
--- a/drivers/clk/spacemit/ccu_common.h
+++ b/drivers/clk/spacemit/ccu_common.h
@@ -7,6 +7,8 @@
 #ifndef _CCU_COMMON_H_
 #define _CCU_COMMON_H_
 
+#include <linux/clk-provider.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 struct ccu_common {
@@ -36,6 +38,12 @@ static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
 	return container_of(hw, struct ccu_common, hw);
 }
 
+struct spacemit_ccu_data {
+	const char *reset_name;
+	struct clk_hw **hws;
+	size_t num;
+};
+
 #define ccu_read(c, reg)						\
 	({								\
 		u32 tmp;						\
@@ -45,4 +53,6 @@ static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
 #define ccu_update(c, reg, mask, val) \
 	regmap_update_bits((c)->regmap, (c)->reg_##reg, mask, val)
 
+int spacemit_ccu_probe(struct platform_device *pdev);
+
 #endif /* _CCU_COMMON_H_ */

-- 
2.52.0

Re: [PATCH v2 2/3] clk: spacemit: extract common ccu functions

[Alex Elder]

On 12/26/25 12:55 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 is come from review of K3 clock
> driver, please refer this disucssion [1].

I understand the point here, and it's just like the first patch:
you're extracting generic code out of the K1-specific file so a
new K3-specific source file can use it too.  This is really good.

However the end result should incorporate *only* generic code
in the generic file, and have the SoC-specific source files
contain everything else.

But as you have it now, the (new) generic probe function
contains special handling for "spacemit,k1-pll", and that's
not generic.

So I suggest you still implement k1_ccu_probe() (and k3_ccu_probe())
separately, allowing each of them to do platform-specific things
before (and/or after) calling the generic probe function.

I had this comment at the end but I'll put it here instead:
You could move the spacemit_ccu_data structure into the common
header file in a separate patch (possibly the one where you
introduced that file in the first place).


OK now I'll talk a bit about why I think you don't need to change
the names of the resets auxiliary devices.  (And even if you do,
I don't think you need the macro.)

Each CCU is defined by its "data" in of_k1_ccu_match[].  The
compatible strings that match those CCUs will be specified in
the DTS files.  For a given platform (K1 or K3), only those
CCUs that make sense for that platform will be defined (i.e.
"spacemit,k1-syscon-mpmu" defines k1_ccu_mpmu_data, which is
appropriate for a K1 platform, not K3).

So even if two resets have the same name, it shouldn't
matter if they're defined for use by separate platforms.
(I think the name is mainly used in defining the auxiliary
device name.)

I might be mistaken, and I don't think adding an SoC identifier
to the reset name hurts anything.  If you do it, it should be
done inside the platform-specific file.

And... I don't know why you don't just add "k1-" or "k3-" to
the name assigned rather than using the macro.  You're only
using it within a platform-specific structure.

static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
	.reset_name	= "k1-mpmu-reset",

rather than
	.reset_name	= _K_RST(mpmu),

					-Alex

> Link: https://lore.kernel.org/all/aTo8sCPpVM1o9PKX@pie/ [1]
> Suggested-by: Yao Zi <me@ziyao.cc>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>   drivers/clk/spacemit/ccu-k1.c     | 188 ++------------------------------------
>   drivers/clk/spacemit/ccu_common.c | 171 ++++++++++++++++++++++++++++++++++
>   drivers/clk/spacemit/ccu_common.h |  10 ++
>   3 files changed, 188 insertions(+), 181 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 01d9485b615d..f97d6f0f0c39 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -5,15 +5,10 @@
>    */
>   
>   #include <linux/array_size.h>
> -#include <linux/auxiliary_bus.h>
>   #include <linux/clk-provider.h>
> -#include <linux/delay.h>
> -#include <linux/idr.h>
> -#include <linux/mfd/syscon.h>
>   #include <linux/minmax.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
> -#include <linux/slab.h>
>   #include <soc/spacemit/k1-syscon.h>
>   
>   #include "ccu_common.h"
> @@ -23,14 +18,6 @@
>   
>   #include <dt-bindings/clock/spacemit,k1-syscon.h>
>   
> -struct spacemit_ccu_data {
> -	const char *reset_name;
> -	struct clk_hw **hws;
> -	size_t num;
> -};
> -
> -static DEFINE_IDA(auxiliary_ids);
> -
>   /* APBS clocks start, APBS region contains and only contains all PLL clocks */
>   
>   /*
> @@ -802,7 +789,7 @@ static struct clk_hw *k1_ccu_mpmu_hws[] = {
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_mpmu_data = {
> -	.reset_name	= "mpmu-reset",
> +	.reset_name	= _K_RST(mpmu),
>   	.hws		= k1_ccu_mpmu_hws,
>   	.num		= ARRAY_SIZE(k1_ccu_mpmu_hws),
>   };
> @@ -913,7 +900,7 @@ static struct clk_hw *k1_ccu_apbc_hws[] = {
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_apbc_data = {
> -	.reset_name	= "apbc-reset",
> +	.reset_name	= _K_RST(apbc),
>   	.hws		= k1_ccu_apbc_hws,
>   	.num		= ARRAY_SIZE(k1_ccu_apbc_hws),
>   };
> @@ -984,184 +971,23 @@ static struct clk_hw *k1_ccu_apmu_hws[] = {
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_apmu_data = {
> -	.reset_name	= "apmu-reset",
> +	.reset_name	= _K_RST(apmu),
>   	.hws		= k1_ccu_apmu_hws,
>   	.num		= ARRAY_SIZE(k1_ccu_apmu_hws),
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_rcpu_data = {
> -	.reset_name	= "rcpu-reset",
> +	.reset_name	= _K_RST(rcpu),
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_rcpu2_data = {
> -	.reset_name	= "rcpu2-reset",
> +	.reset_name	= _K_RST(rcpu2),
>   };
>   
>   static const struct spacemit_ccu_data k1_ccu_apbc2_data = {
> -	.reset_name	= "apbc2-reset",
> +	.reset_name	= _K_RST(apbc2),
>   };
>   
> -static int spacemit_ccu_register(struct device *dev,
> -				 struct regmap *regmap,
> -				 struct regmap *lock_regmap,
> -				 const struct spacemit_ccu_data *data)
> -{
> -	struct clk_hw_onecell_data *clk_data;
> -	int i, ret;
> -
> -	/* Nothing to do if the CCU does not implement any clocks */
> -	if (!data->hws)
> -		return 0;
> -
> -	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
> -				GFP_KERNEL);
> -	if (!clk_data)
> -		return -ENOMEM;
> -
> -	clk_data->num = data->num;
> -
> -	for (i = 0; i < data->num; i++) {
> -		struct clk_hw *hw = data->hws[i];
> -		struct ccu_common *common;
> -		const char *name;
> -
> -		if (!hw) {
> -			clk_data->hws[i] = ERR_PTR(-ENOENT);
> -			continue;
> -		}
> -
> -		name = hw->init->name;
> -
> -		common = hw_to_ccu_common(hw);
> -		common->regmap		= regmap;
> -		common->lock_regmap	= lock_regmap;
> -
> -		ret = devm_clk_hw_register(dev, hw);
> -		if (ret) {
> -			dev_err(dev, "Cannot register clock %d - %s\n",
> -				i, name);
> -			return ret;
> -		}
> -
> -		clk_data->hws[i] = hw;
> -	}
> -
> -	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> -	if (ret)
> -		dev_err(dev, "failed to add clock hardware provider (%d)\n", ret);
> -
> -	return ret;
> -}
> -
> -static void spacemit_cadev_release(struct device *dev)
> -{
> -	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> -
> -	ida_free(&auxiliary_ids, adev->id);
> -	kfree(to_spacemit_ccu_adev(adev));
> -}
> -
> -static void spacemit_adev_unregister(void *data)
> -{
> -	struct auxiliary_device *adev = data;
> -
> -	auxiliary_device_delete(adev);
> -	auxiliary_device_uninit(adev);
> -}
> -
> -static int spacemit_ccu_reset_register(struct device *dev,
> -				       struct regmap *regmap,
> -				       const char *reset_name)
> -{
> -	struct spacemit_ccu_adev *cadev;
> -	struct auxiliary_device *adev;
> -	int ret;
> -
> -	/* Nothing to do if the CCU does not implement a reset controller */
> -	if (!reset_name)
> -		return 0;
> -
> -	cadev = kzalloc(sizeof(*cadev), GFP_KERNEL);
> -	if (!cadev)
> -		return -ENOMEM;
> -
> -	cadev->regmap = regmap;
> -
> -	adev = &cadev->adev;
> -	adev->name = reset_name;
> -	adev->dev.parent = dev;
> -	adev->dev.release = spacemit_cadev_release;
> -	adev->dev.of_node = dev->of_node;
> -	ret = ida_alloc(&auxiliary_ids, GFP_KERNEL);
> -	if (ret < 0)
> -		goto err_free_cadev;
> -	adev->id = ret;
> -
> -	ret = auxiliary_device_init(adev);
> -	if (ret)
> -		goto err_free_aux_id;
> -
> -	ret = auxiliary_device_add(adev);
> -	if (ret) {
> -		auxiliary_device_uninit(adev);
> -		return ret;
> -	}
> -
> -	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
> -
> -err_free_aux_id:
> -	ida_free(&auxiliary_ids, adev->id);
> -err_free_cadev:
> -	kfree(cadev);
> -
> -	return ret;
> -}
> -
> -static int k1_ccu_probe(struct platform_device *pdev)
> -{
> -	struct regmap *base_regmap, *lock_regmap = NULL;
> -	const struct spacemit_ccu_data *data;
> -	struct device *dev = &pdev->dev;
> -	int ret;
> -
> -	base_regmap = device_node_to_regmap(dev->of_node);
> -	if (IS_ERR(base_regmap))
> -		return dev_err_probe(dev, PTR_ERR(base_regmap),
> -				     "failed to get regmap\n");
> -
> -	/*
> -	 * The lock status of PLLs locate in MPMU region, while PLLs themselves
> -	 * are in APBS region. Reference to MPMU syscon is required to check PLL
> -	 * status.
> -	 */
> -	if (of_device_is_compatible(dev->of_node, "spacemit,k1-pll")) {
> -		struct device_node *mpmu = of_parse_phandle(dev->of_node,
> -							    "spacemit,mpmu", 0);
> -		if (!mpmu)
> -			return dev_err_probe(dev, -ENODEV,
> -					     "Cannot parse MPMU region\n");
> -
> -		lock_regmap = device_node_to_regmap(mpmu);
> -		of_node_put(mpmu);
> -
> -		if (IS_ERR(lock_regmap))
> -			return dev_err_probe(dev, PTR_ERR(lock_regmap),
> -					     "failed to get lock regmap\n");
> -	}
> -
> -	data = of_device_get_match_data(dev);
> -
> -	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, data);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to register clocks\n");
> -
> -	ret = spacemit_ccu_reset_register(dev, base_regmap, data->reset_name);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to register resets\n");
> -
> -	return 0;
> -}
> -
>   static const struct of_device_id of_k1_ccu_match[] = {
>   	{
>   		.compatible	= "spacemit,k1-pll",
> @@ -1200,7 +1026,7 @@ static struct platform_driver k1_ccu_driver = {
>   		.name		= "spacemit,k1-ccu",
>   		.of_match_table = of_k1_ccu_match,
>   	},
> -	.probe	= k1_ccu_probe,
> +	.probe	= spacemit_ccu_probe,
>   };
>   module_platform_driver(k1_ccu_driver);
>   
> diff --git a/drivers/clk/spacemit/ccu_common.c b/drivers/clk/spacemit/ccu_common.c
> index 4412c4104dab..f1a837aafb46 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);
> +static int spacemit_ccu_register(struct device *dev,
> +				 struct regmap *regmap,
> +				 struct regmap *lock_regmap,
> +				 const struct spacemit_ccu_data *data)
> +{
> +	struct clk_hw_onecell_data *clk_data;
> +	int i, ret;
> +
> +	/* Nothing to do if the CCU does not implement any clocks */
> +	if (!data->hws)
> +		return 0;
> +
> +	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
> +				GFP_KERNEL);
> +	if (!clk_data)
> +		return -ENOMEM;
> +
> +	clk_data->num = data->num;
> +
> +	for (i = 0; i < data->num; i++) {
> +		struct clk_hw *hw = data->hws[i];
> +		struct ccu_common *common;
> +		const char *name;
> +
> +		if (!hw) {
> +			clk_data->hws[i] = ERR_PTR(-ENOENT);
> +			continue;
> +		}
> +
> +		name = hw->init->name;
> +
> +		common = hw_to_ccu_common(hw);
> +		common->regmap		= regmap;
> +		common->lock_regmap	= lock_regmap;
> +
> +		ret = devm_clk_hw_register(dev, hw);
> +		if (ret) {
> +			dev_err(dev, "Cannot register clock %d - %s\n",
> +				i, name);
> +			return ret;
> +		}
> +
> +		clk_data->hws[i] = hw;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> +	if (ret)
> +		dev_err(dev, "failed to add clock hardware provider (%d)\n", ret);
> +
> +	return ret;
> +}
> +
> +static void spacemit_cadev_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> +	ida_free(&auxiliary_ids, adev->id);
> +	kfree(to_spacemit_ccu_adev(adev));
> +}
> +
> +static void spacemit_adev_unregister(void *data)
> +{
> +	struct auxiliary_device *adev = data;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}
> +
> +static int spacemit_ccu_reset_register(struct device *dev,
> +				       struct regmap *regmap,
> +				       const char *reset_name)
> +{
> +	struct spacemit_ccu_adev *cadev;
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	/* Nothing to do if the CCU does not implement a reset controller */
> +	if (!reset_name)
> +		return 0;
> +
> +	cadev = kzalloc(sizeof(*cadev), GFP_KERNEL);
> +	if (!cadev)
> +		return -ENOMEM;
> +
> +	cadev->regmap = regmap;
> +
> +	adev = &cadev->adev;
> +	adev->name = reset_name;
> +	adev->dev.parent = dev;
> +	adev->dev.release = spacemit_cadev_release;
> +	adev->dev.of_node = dev->of_node;
> +	ret = ida_alloc(&auxiliary_ids, GFP_KERNEL);
> +	if (ret < 0)
> +		goto err_free_cadev;
> +	adev->id = ret;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret)
> +		goto err_free_aux_id;
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	return devm_add_action_or_reset(dev, spacemit_adev_unregister, adev);
> +
> +err_free_aux_id:
> +	ida_free(&auxiliary_ids, adev->id);
> +err_free_cadev:
> +	kfree(cadev);
> +
> +	return ret;
> +}
> +
> +int spacemit_ccu_probe(struct platform_device *pdev)
> +{
> +	struct regmap *base_regmap, *lock_regmap = NULL;
> +	const struct spacemit_ccu_data *data;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	base_regmap = device_node_to_regmap(dev->of_node);
> +	if (IS_ERR(base_regmap))
> +		return dev_err_probe(dev, PTR_ERR(base_regmap),
> +				     "failed to get regmap\n");
> +
> +	/*
> +	 * The lock status of PLLs locate in MPMU region, while PLLs themselves
> +	 * are in APBS region. Reference to MPMU syscon is required to check PLL
> +	 * status.
> +	 */
> +	if (of_device_is_compatible(dev->of_node, "spacemit,k1-pll")) {
> +		struct device_node *mpmu = of_parse_phandle(dev->of_node,
> +							    "spacemit,mpmu", 0);
> +		if (!mpmu)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "Cannot parse MPMU region\n");
> +
> +		lock_regmap = device_node_to_regmap(mpmu);
> +		of_node_put(mpmu);
> +
> +		if (IS_ERR(lock_regmap))
> +			return dev_err_probe(dev, PTR_ERR(lock_regmap),
> +					     "failed to get lock regmap\n");
> +	}
> +
> +	data = of_device_get_match_data(dev);
> +
> +	ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register clocks\n");
> +
> +	ret = spacemit_ccu_reset_register(dev, base_regmap, data->reset_name);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register resets\n");
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(spacemit_ccu_probe, "CLK_SPACEMIT");
>   
>   MODULE_DESCRIPTION("SpacemiT CCU common clock driver");
>   MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/spacemit/ccu_common.h b/drivers/clk/spacemit/ccu_common.h
> index da72f3836e0b..9b4ef24875e5 100644
> --- a/drivers/clk/spacemit/ccu_common.h
> +++ b/drivers/clk/spacemit/ccu_common.h
> @@ -7,6 +7,8 @@
>   #ifndef _CCU_COMMON_H_
>   #define _CCU_COMMON_H_
>   
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
>   #include <linux/regmap.h>
>   
>   struct ccu_common {
> @@ -36,6 +38,12 @@ static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>   	return container_of(hw, struct ccu_common, hw);
>   }
>   
> +struct spacemit_ccu_data {
> +	const char *reset_name;
> +	struct clk_hw **hws;
> +	size_t num;
> +};
> +
>   #define ccu_read(c, reg)						\
>   	({								\
>   		u32 tmp;						\
> @@ -45,4 +53,6 @@ static inline struct ccu_common *hw_to_ccu_common(struct clk_hw *hw)
>   #define ccu_update(c, reg, mask, val) \
>   	regmap_update_bits((c)->regmap, (c)->reg_##reg, mask, val)
>   
> +int spacemit_ccu_probe(struct platform_device *pdev);
> +
>   #endif /* _CCU_COMMON_H_ */
> 

Re: [PATCH v2 2/3] clk: spacemit: extract common ccu functions

[Yao Zi]

On Mon, Dec 29, 2025 at 06:50:14PM -0600, Alex Elder wrote:
> On 12/26/25 12:55 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 is come from review of K3 clock
> > driver, please refer this disucssion [1].
> 
> I understand the point here, and it's just like the first patch:
> you're extracting generic code out of the K1-specific file so a
> new K3-specific source file can use it too.  This is really good.
> 
> However the end result should incorporate *only* generic code
> in the generic file, and have the SoC-specific source files
> contain everything else.
> 
> But as you have it now, the (new) generic probe function
> contains special handling for "spacemit,k1-pll", and that's
> not generic.
> 
> So I suggest you still implement k1_ccu_probe() (and k3_ccu_probe())
> separately, allowing each of them to do platform-specific things
> before (and/or after) calling the generic probe function.

I've raised similar concerns in the series for K3 clock tree[1].

Regards,
Yao Zi

[1]: https://lore.kernel.org/all/aU50DIe9qMneb0GT@pie/

Re: [PATCH v2 2/3] clk: spacemit: extract common ccu functions

[Yixun Lan]

Hi Alex, Yao,

On 04:00 Tue 30 Dec     , Yao Zi wrote:
> On Mon, Dec 29, 2025 at 06:50:14PM -0600, Alex Elder wrote:
> > On 12/26/25 12:55 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 is come from review of K3 clock
> > > driver, please refer this disucssion [1].
> > 
> > I understand the point here, and it's just like the first patch:
> > you're extracting generic code out of the K1-specific file so a
> > new K3-specific source file can use it too.  This is really good.
> > 
> > However the end result should incorporate *only* generic code
> > in the generic file, and have the SoC-specific source files
> > contain everything else.
> > 
> > But as you have it now, the (new) generic probe function
> > contains special handling for "spacemit,k1-pll", and that's
> > not generic.
> > 
> > So I suggest you still implement k1_ccu_probe() (and k3_ccu_probe())
> > separately, allowing each of them to do platform-specific things
> > before (and/or after) calling the generic probe function.
> 
> I've raised similar concerns in the series for K3 clock tree[1].
> 
> Regards,
> Yao Zi
> 
> [1]: https://lore.kernel.org/all/aU50DIe9qMneb0GT@pie/
> 

Ok, since both of you raise this, I will do it in next version
thanks

-- 
Yixun Lan (dlan)

[PATCH v2 3/3] reset: spacemit: fix auxiliary device id

[Yixun Lan]

Due to the auxiliary register procedure moved to ccu common module,
the auxiliary device id need to be adjusted, otherwise reset driver
will fail to probe.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 drivers/reset/reset-spacemit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-spacemit.c b/drivers/reset/reset-spacemit.c
index e1272aff28f7..8922e14fa836 100644
--- a/drivers/reset/reset-spacemit.c
+++ b/drivers/reset/reset-spacemit.c
@@ -278,7 +278,7 @@ static int spacemit_reset_probe(struct auxiliary_device *adev,
 
 #define K1_AUX_DEV_ID(_unit) \
 	{ \
-		.name = "spacemit_ccu_k1." #_unit "-reset", \
+		.name = "spacemit_ccu." _K_RST(_unit), \
 		.driver_data = (kernel_ulong_t)&k1_ ## _unit ## _reset_data, \
 	}
 

-- 
2.52.0

Re: [PATCH v2 3/3] reset: spacemit: fix auxiliary device id

[Alex Elder]

On 12/26/25 12:55 AM, Yixun Lan wrote:
> Due to the auxiliary register procedure moved to ccu common module,
> the auxiliary device id need to be adjusted, otherwise reset driver
> will fail to probe.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>   drivers/reset/reset-spacemit.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-spacemit.c b/drivers/reset/reset-spacemit.c
> index e1272aff28f7..8922e14fa836 100644
> --- a/drivers/reset/reset-spacemit.c
> +++ b/drivers/reset/reset-spacemit.c
> @@ -278,7 +278,7 @@ static int spacemit_reset_probe(struct auxiliary_device *adev,
>   
>   #define K1_AUX_DEV_ID(_unit) \
>   	{ \
> -		.name = "spacemit_ccu_k1." #_unit "-reset", \
> +		.name = "spacemit_ccu." _K_RST(_unit), \
>   		.driver_data = (kernel_ulong_t)&k1_ ## _unit ## _reset_data, \
>   	}

The above macro is named K1_AUX_DEV_ID().  Why don't you
define K3_AUX_DEV_ID(), which could use "k3" in its name?

Anyway, if you go this route I suggest you drop "K1_" from the
name of this macro.

					-Alex

>   
> 

Re: [PATCH v2 3/3] reset: spacemit: fix auxiliary device id

[Yixun Lan]

Hi Alex,

On 18:50 Mon 29 Dec     , Alex Elder wrote:
> On 12/26/25 12:55 AM, Yixun Lan wrote:
> > Due to the auxiliary register procedure moved to ccu common module,
> > the auxiliary device id need to be adjusted, otherwise reset driver
> > will fail to probe.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >   drivers/reset/reset-spacemit.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/reset/reset-spacemit.c b/drivers/reset/reset-spacemit.c
> > index e1272aff28f7..8922e14fa836 100644
> > --- a/drivers/reset/reset-spacemit.c
> > +++ b/drivers/reset/reset-spacemit.c
> > @@ -278,7 +278,7 @@ static int spacemit_reset_probe(struct auxiliary_device *adev,
> >   
> >   #define K1_AUX_DEV_ID(_unit) \
> >   	{ \
> > -		.name = "spacemit_ccu_k1." #_unit "-reset", \
> > +		.name = "spacemit_ccu." _K_RST(_unit), \
> >   		.driver_data = (kernel_ulong_t)&k1_ ## _unit ## _reset_data, \
> >   	}
> 
> The above macro is named K1_AUX_DEV_ID().  Why don't you
> define K3_AUX_DEV_ID(), which could use "k3" in its name?
> 
that should also works, the idea of using same macro '_K_RST()' here
is trying to explictly tell users the clock and reset shares same name

> Anyway, if you go this route I suggest you drop "K1_" from the
> name of this macro.
> 
or could further refactor the code, to make K1/K3 drivers share same macro

anyway I don't want to change this patch, my goal here is tring to fix
reset driver after clock common driver refactored plus the modularization 
introduced, it's more proper to leave those refactor work up to Guodong,
since he did a lot adjustment to add reset support for K3 SoC

-- 
Yixun Lan (dlan)

Re: [PATCH v2 3/3] reset: spacemit: fix auxiliary device id

[Alex Elder]

On 12/30/25 8:09 PM, Yixun Lan wrote:
> Hi Alex,
> 
> On 18:50 Mon 29 Dec     , Alex Elder wrote:
>> On 12/26/25 12:55 AM, Yixun Lan wrote:
>>> Due to the auxiliary register procedure moved to ccu common module,
>>> the auxiliary device id need to be adjusted, otherwise reset driver
>>> will fail to probe.
>>>
>>> Signed-off-by: Yixun Lan <dlan@gentoo.org>
>>> ---
>>>    drivers/reset/reset-spacemit.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/reset/reset-spacemit.c b/drivers/reset/reset-spacemit.c
>>> index e1272aff28f7..8922e14fa836 100644
>>> --- a/drivers/reset/reset-spacemit.c
>>> +++ b/drivers/reset/reset-spacemit.c
>>> @@ -278,7 +278,7 @@ static int spacemit_reset_probe(struct auxiliary_device *adev,
>>>    
>>>    #define K1_AUX_DEV_ID(_unit) \
>>>    	{ \
>>> -		.name = "spacemit_ccu_k1." #_unit "-reset", \
>>> +		.name = "spacemit_ccu." _K_RST(_unit), \
>>>    		.driver_data = (kernel_ulong_t)&k1_ ## _unit ## _reset_data, \
>>>    	}
>>
>> The above macro is named K1_AUX_DEV_ID().  Why don't you
>> define K3_AUX_DEV_ID(), which could use "k3" in its name?
>>
> that should also works, the idea of using same macro '_K_RST()' here
> is trying to explictly tell users the clock and reset shares same name

You mean like "spacemit,k1-syscon-mpmu" is used, and you'd rather
name the reset "spacemit.k1-mpmu-reset" because it sort of matches
a little better than "spacemit_ccu_k1.mpmu-reset"?

Anyway, when K1_AUX_DEV_ID() was defined, it was named that way
to suggest a pattern that would mean K3_AUX_DEV_ID() would also
be defined.

I don't really care where the "k1"/"k3" goes, before or after the
period.  But I think the "k1" could be encoded explicitly here
rather than doing it with _K_RST() (i.e., don't even use a macro).

					-Alex
> 
>> Anyway, if you go this route I suggest you drop "K1_" from the
>> name of this macro.
>>
> or could further refactor the code, to make K1/K3 drivers share same macro
> 
> anyway I don't want to change this patch, my goal here is tring to fix
> reset driver after clock common driver refactored plus the modularization
> introduced, it's more proper to leave those refactor work up to Guodong,
> since he did a lot adjustment to add reset support for K3 SoC
>