[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 at 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
>