Received-SPF: pass (google.com: domain of linux-kernel+bounces-75962-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <74e003c6d80611ddd826ac21f48b4b3a.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20240131182653.2673554-4-tmaimon77@gmail.com>
References: <20240131182653.2673554-1-tmaimon77@gmail.com> <20240131182653.2673554-4-tmaimon77@gmail.com>
Subject: Re: [PATCH v23 3/3] 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, devicetree@vger.kernel.org, Tomer Maimon <tmaimon77@gmail.com>
To: Tomer Maimon <tmaimon77@gmail.com>, benjaminfair@google.com, joel@jms.id.au, krzysztof.kozlowski+dt@linaro.org, mturquette@baylibre.com, robh+dt@kernel.org, tali.perry1@gmail.com, venture@google.com, yuenn@google.com
Date: Wed, 21 Feb 2024 21:58:01 -0800
User-Agent: alot/0.10

Quoting Tomer Maimon (2024-01-31 10:26:53)
> diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> new file mode 100644
> index 000000000000..eacb579d30af
> --- /dev/null
> +++ b/drivers/clk/clk-npcm8xx.c
> @@ -0,0 +1,509 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NPCM8xx Clock Generator
> + * All the clocks are initialized by the bootloader, so this driver allo=
ws only
[...]
> +
> +/* external clock definition */
> +#define NPCM8XX_CLK_S_REFCLK   "refclk"
> +
> +/* pll definition */
> +#define NPCM8XX_CLK_S_PLL0     "pll0"
> +#define NPCM8XX_CLK_S_PLL1     "pll1"
> +#define NPCM8XX_CLK_S_PLL2     "pll2"
> +#define NPCM8XX_CLK_S_PLL_GFX  "pll_gfx"
> +
> +/* early divider definition */
> +#define NPCM8XX_CLK_S_PLL2_DIV2                "pll2_div2"
> +#define NPCM8XX_CLK_S_PLL_GFX_DIV2     "pll_gfx_div2"
> +#define NPCM8XX_CLK_S_PLL1_DIV2                "pll1_div2"
> +
> +/* mux definition */
> +#define NPCM8XX_CLK_S_CPU_MUX     "cpu_mux"
> +
> +/* div definition */
> +#define NPCM8XX_CLK_S_TH          "th"
> +#define NPCM8XX_CLK_S_AXI         "axi"

Please inline all these string #defines to the place they're used.

> +
> +static struct clk_hw hw_pll1_div2, hw_pll2_div2, hw_gfx_div2, hw_pre_clk;
[..]
> +static struct clk_hw *
> +npcm8xx_clk_register(struct device *dev, const char *name,
> +                    struct regmap *clk_regmap, unsigned int offset,
> +                    unsigned long flags, const struct clk_ops *npcm8xx_c=
lk_ops,
> +                    const struct clk_parent_data *parent_data,
> +                    const struct clk_hw *parent_hw, u8 num_parents,
> +                    u8 shift, u32 mask, unsigned long width,
> +                    const u32 *table, unsigned long clk_flags)
> +{
> +       struct npcm8xx_clk *clk;
> +       struct clk_init_data init =3D {};
> +       int ret;
> +
> +       clk =3D devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name =3D name;
> +       init.ops =3D npcm8xx_clk_ops;
> +       init.parent_data =3D parent_data;
> +       init.parent_hws =3D parent_hw ? &parent_hw : NULL;

Is it necessary to check? Can't it be set unconditionally?

> +       init.num_parents =3D num_parents;
> +       init.flags =3D flags;
> +
> +       clk->clk_regmap =3D clk_regmap;
> +       clk->hw.init =3D &init;
> +       clk->offset =3D offset;
> +       clk->shift =3D shift;
> +       clk->mask =3D mask;
> +       clk->width =3D width;
> +       clk->table =3D table;
> +       clk->flags =3D clk_flags;
> +
> +       ret =3D devm_clk_hw_register(dev, &clk->hw);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return &clk->hw;
[...]
> +
> +static unsigned long npcm8xx_clk_div_get_parent(struct clk_hw *hw,
> +                                               unsigned long parent_rate)
> +{
> +       struct npcm8xx_clk *div =3D to_npcm8xx_clk(hw);
> +       unsigned int val;
> +
> +       regmap_read(div->clk_regmap, div->offset, &val);
> +       val =3D val >> div->shift;
> +       val &=3D clk_div_mask(div->width);
> +
> +       return divider_recalc_rate(hw, parent_rate, val, NULL, div->flags,
> +                                  div->width);
> +}
> +
> +static const struct clk_ops npcm8xx_clk_div_ops =3D {
> +       .recalc_rate =3D npcm8xx_clk_div_get_parent,
> +};
> +
> +static int npcm8xx_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *parent_np =3D of_get_parent(pdev->dev.of_node=
);

The parent of this device is not a syscon.

> +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> +       struct device *dev =3D &pdev->dev;
> +       struct regmap *clk_regmap;
> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       npcm8xx_clk_data =3D devm_kzalloc(dev, struct_size(npcm8xx_clk_da=
ta, hws,
> +                                                        NPCM8XX_NUM_CLOC=
KS),
> +                                       GFP_KERNEL);
> +       if (!npcm8xx_clk_data)
> +               return -ENOMEM;
> +
> +       clk_regmap =3D syscon_node_to_regmap(parent_np);
> +       of_node_put(parent_np);

Is there another binding update that is going to move this node to be a
child of the syscon?

		gcr: system-controller@f0800000 {
                        compatible =3D "nuvoton,npcm845-gcr", "syscon";
                        reg =3D <0x0 0xf0800000 0x0 0x1000>;
                };