Re: [PATCH v2 05/13] clk: renesas: Pass sub struct of cpg_mssr_priv to cpg_clk_register

[Paul Barker <paul.barker.ct@bp.renesas.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3364 bytes --]

Hi Thierry,

On 17/02/2025 10:52, Thierry Bultel wrote:
> In a coming evolution, the registration callback will need more parameters
> from cpg_mssr_priv (like another base address with clock controllers
> with double register block).
> Instead of adding more parameters, mode the needed parameters to a public
> sub-struct.

This is missing the Signed-off-by line.

I also think you should mention that rmw_lock is moved to the new struct
as it will be needed in the subsequent patch, since none of the
functions modified by this patch to take a `struct cpg_mssr_pub`
argument use that lock.

[snip]

> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> index 027100e84ee4..aafeb139fdb4 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -345,9 +345,11 @@ static const struct soc_device_attribute cpg_quirks_match[] __initconst = {
>  
>  struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>  	const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> -	struct clk **clks, void __iomem *base,
> -	struct raw_notifier_head *notifiers)
> +	struct cpg_mssr_pub *pub)
>  {
> +	struct raw_notifier_head *notifiers = &pub->notifiers;
> +	void __iomem *base = pub->base0;
> +	struct clk **clks = pub->clks;
>  	const struct clk *parent;
>  	unsigned int mult = 1;
>  	unsigned int div = 1;
> @@ -431,7 +433,7 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>  			}
>  
>  			writel(value, csn->reg);
> -			cpg_simple_notifier_register(notifiers, csn);
> +			cpg_simple_notifier_register(&pub->notifiers, csn);

This change isn't needed as you've added a notifiers variable at the
start of the function.

>  			break;
>  		}
>  

[snip]

> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 79e7a90c3b1b..7d5fba3aef19 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -127,7 +127,6 @@ static const u16 srstclr_for_gen4[] = {
>   *
>   * @rcdev: Optional reset controller entity
>   * @dev: CPG/MSSR device
> - * @base: CPG/MSSR register block base address
>   * @reg_layout: CPG/MSSR register layout
>   * @rmw_lock: protects RMW register accesses
>   * @np: Device node in DT for this CPG/MSSR module
> @@ -143,6 +142,7 @@ static const u16 srstclr_for_gen4[] = {
>   *                 [].val: Saved values of SMSTPCR[]
>   * @reserved_ids: Temporary used, reserved id list
>   * @num_reserved_ids: Temporary used, number of reserved id list
> + * @pub: Data passed to clock registration callback
>   * @clks: Array containing all Core and Module Clocks
>   */
>  struct cpg_mssr_priv {
> @@ -150,16 +150,13 @@ struct cpg_mssr_priv {
>  	struct reset_controller_dev rcdev;
>  #endif
>  	struct device *dev;
> -	void __iomem *base;
>  	enum clk_reg_layout reg_layout;
> -	spinlock_t rmw_lock;

You can remove the documentation line for @base in the comment above.

>  	struct device_node *np;
>  
>  	unsigned int num_core_clks;
>  	unsigned int num_mod_clks;
>  	unsigned int last_dt_core_clk;
>  
> -	struct raw_notifier_head notifiers;

As above, you can remove the @notifiers entry from the comment above.

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]