Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20221211204324.169991-2-tmaimon77@gmail.com>
References: <20221211204324.169991-1-tmaimon77@gmail.com> <20221211204324.169991-2-tmaimon77@gmail.com>
Subject: Re: [PATCH v14 1/1] clk: npcm8xx: add clock controller
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     openbmc@lists.ozlabs.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, Tomer Maimon <tmaimon77@gmail.com>
To:     Tomer Maimon <tmaimon77@gmail.com>, avifishman70@gmail.com,
        benjaminfair@google.com, joel@jms.id.au, mturquette@baylibre.com,
        tali.perry1@gmail.com, venture@google.com, yuenn@google.com
Date:   Fri, 16 Dec 2022 10:44:00 -0800
User-Agent: alot/0.10
Message-Id: <20221216184402.8A426C433D2@smtp.kernel.org>
Precedence: bulk

Quoting Tomer Maimon (2022-12-11 12:43:24)
> diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> new file mode 100644
> index 000000000000..08ee7bea6f3a
> --- /dev/null
> +++ b/drivers/clk/clk-npcm8xx.c
> @@ -0,0 +1,650 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
[...]
> +#define NPCM8XX_CLK_S_RCP        "rcp"
> +
> +static const u32 pll_mux_table[] =3D { 0, 1, 2, 3 };
> +static const struct clk_parent_data pll_mux_parents[] =3D {
> +       { .fw_name =3D NPCM8XX_CLK_S_PLL0, .name =3D NPCM8XX_CLK_S_PLL0 },

As this is a new driver either you should only have .fw_name here. The
.name field is a backup to migrate code over to a new binding. When
.fw_name is used there should be an associated DT binding update. I
doubt the usage of .fw_name is correct though, because aren't these clks
internal to the controller? The .fw_name field is about describing
parents that are an input to the clk controller node in DT (because the
controller is a consumer of these clks that are external to the device).

So can you use the .hw field for these internal clks? Check out
CLK_HW_INIT_HWS() macro and friends for a possible way to initialize
this.

> +       { .fw_name =3D NPCM8XX_CLK_S_PLL1, .name =3D NPCM8XX_CLK_S_PLL1 },
> +       { .fw_name =3D NPCM8XX_CLK_S_REFCLK, .name =3D NPCM8XX_CLK_S_REFC=
LK },

Maybe this is external? If so, it would be great to have this in the
binding as a `clocks` property.