Re: [PATCH] clk: renesas: rcar-gen3: Use FIELD_GET()

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Geert,

Thanks for your work.

On 2026-09-03 14:05:25 +0200, Geert Uytterhoeven wrote:
> Improve readability by using the FIELD_GET() helper instead of
> open-coding the same operation, and by adding field definitions to get
> rid of hardcoded values.
> 
> While at it, move register definitions that are only used inside the
> rcar-gen3-cpg.c source file out of the rcar-gen3-cpg.h header file.
> Add comments where appropriate.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> To be queued in renesas-clk for v7.4.
> 
>  drivers/clk/renesas/rcar-gen3-cpg.c | 15 +++++++++------
>  drivers/clk/renesas/rcar-gen3-cpg.h |  3 +--
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
> index e5b503332217dcdc..bbb76e4fa333553d 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -36,7 +36,11 @@
>  
>  #define CPG_PLLnCR_STC_MASK	GENMASK(30, 24)	/* PLL Circuit Mult. Ratio */
>  
> -#define CPG_RCKCR_CKSEL	BIT(15)	/* RCLK Clock Source Select */
> +#define CPG_RPCCKCR		0x238	/* RPC-IF Clock Frequency Control Reg */
> +#define CPG_RPCCKCR_DIV_RPCSRC	GENMASK(4, 3)	/* RPCSRC divider */
> +
> +#define CPG_RCKCR_CKSEL		BIT(15)		/* RCLK Clock Source Select */
> +#define CPG_RCKCR_DIV		GENMASK(5, 0)	/* RCLK Division Ratio */
>  
>  /* PLL Clocks */
>  struct cpg_pll_clk {
> @@ -395,7 +399,7 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>  		 * the multiplier value.
>  		 */
>  		value = readl(base + CPG_PLL4CR);
> -		mult = (((value >> 24) & 0x7f) + 1) * 2;
> +		mult = (FIELD_GET(CPG_PLLnCR_STC_MASK, value) + 1) * 2;
>  		break;
>  
>  	case CLK_TYPE_GEN3_SDH:
> @@ -420,7 +424,7 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>  			 * RINT is default.
>  			 * Only if EXTALR is populated, we switch to it.
>  			 */
> -			value = readl(csn->reg) & 0x3f;
> +			value = readl(csn->reg) & CPG_RCKCR_DIV;
>  
>  			if (clk_get_rate(clks[cpg_clk_extalr])) {
>  				parent = clks[cpg_clk_extalr];
> @@ -496,9 +500,8 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct device *dev,
>  		 * MD[4:1] pins and CPG_RPCCKCR[4:3] register value for
>  		 * which has been set prior to booting the kernel.
>  		 */
> -		value = (readl(base + CPG_RPCCKCR) & GENMASK(4, 3)) >> 3;
> -
> -		switch (value) {
> +		value = readl(base + CPG_RPCCKCR);
> +		switch (FIELD_GET(CPG_RPCCKCR_DIV_RPCSRC, value)) {
>  		case 0:
>  			div = 5;
>  			break;
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.h b/drivers/clk/renesas/rcar-gen3-cpg.h
> index d15a5d1df71c788b..fa25911b993edfb9 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.h
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.h
> @@ -76,8 +76,7 @@ struct rcar_gen3_cpg_pll_config {
>  	u8 osc_prediv;
>  };
>  
> -#define CPG_RPCCKCR	0x238
> -#define CPG_RCKCR	0x240
> +#define CPG_RCKCR		0x240	/* RCLK Frequency Control Register */
>  
>  struct clk *rcar_gen3_cpg_clk_register(struct device *dev,
>  	const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> -- 
> 2.43.0
> 
> 

-- 
Kind Regards,
Niklas Söderlund