Re: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Chris Paterson <Chris.Paterson2@renesas.com>,
	Biju Das <biju.das@bp.renesas.com>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: Re: [PATCH 2/2] drivers: clk: renesas: r9a07g044-cpg: Add SDHI clock and reset entries
Date: Mon, 6 Sep 2021 13:54:21 +0200	[thread overview]
Message-ID: <CAMuHMdXDJyp=eVGRgf6vzKKMyxhPN8_znC9fUxJu+mTwXYyiHA@mail.gmail.com> (raw)
In-Reply-To: <20210804180803.29087-3-biju.das.jz@bp.renesas.com>

Hi Biju,

On Wed, Aug 4, 2021 at 8:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Add SDHI{0,1} mux, clock and reset entries to CPG driver.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a07g044-cpg.c
> +++ b/drivers/clk/renesas/r9a07g044-cpg.c

> @@ -77,6 +85,11 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED(".pll6_2", CLK_PLL6_2, CLK_PLL6, 1, 1),
>
>         DEF_FIXED(".pll2_div2", CLK_PLL2_DIV2, CLK_PLL2, 1, 2),
> +       DEF_FIXED(".clk800fix_c", CLK_800FIX_C, CLK_PLL2, 1, 2),
> +       DEF_FIXED(".clk533fix_c", CLK_533FIX_C, CLK_PLL2, 2, 6),

"2, 6" can be simplified to "1, 3".

> +       DEF_FIXED(".div_pll2_div8", CLK_DIV_PLL2_DIV8, CLK_800FIX_C, 1, 2),
> +       DEF_FIXED(".div_pll2_div12", CLK_DIV_PLL2_DIV12, CLK_533FIX_C, 1, 2),

I just love the confusing clock naming in the User's Manual!
DIV_PLL2_DIV8 runs at PLL2 / 4, and DIV_PLL2_DIV12 runs at PLL2 / 6 :-(

> +
>         DEF_FIXED(".pll2_div16", CLK_PLL2_DIV16, CLK_PLL2, 1, 16),
>         DEF_FIXED(".pll2_div20", CLK_PLL2_DIV20, CLK_PLL2, 1, 20),
>
> @@ -103,6 +116,12 @@ static const struct cpg_core_clk r9a07g044_core_clks[] __initconst = {
>         DEF_FIXED("ZT", R9A07G044_CLK_ZT, CLK_PLL3_DIV2_4_2, 1, 1),
>         DEF_MUX("HP", R9A07G044_CLK_HP, SEL_PLL6_2,
>                 sel_pll6_2, ARRAY_SIZE(sel_pll6_2), 0, CLK_MUX_HIWORD_MASK),
> +       DEF_SD_MUX("SD0", R9A07G044_CLK_SD0, SEL_SDHI0,
> +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),
> +       DEF_SD_MUX("SD1", R9A07G044_CLK_SD1, SEL_SDHI1,
> +                  sel_shdi, ARRAY_SIZE(sel_shdi), 0, 0),

Looks like both .flag and .mux_flags are unneeded?

> +       DEF_FIXED("SD0_DIV4", CLK_SD0_DIV4, R9A07G044_CLK_SD0, 1, 4),
> +       DEF_FIXED("SD1_DIV4", CLK_SD1_DIV4, R9A07G044_CLK_SD1, 1, 4),
>  };
>
>  static struct rzg2l_mod_clk r9a07g044_mod_clks[] = {

The rest looks good to me.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds