Re: [PATCH v23 3/3] clk: npcm8xx: add clock controller

[Stephen Boyd <sboyd@kernel.org>]

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 allows 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_clk_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 = {};
> +       int ret;
> +
> +       clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = name;
> +       init.ops = npcm8xx_clk_ops;
> +       init.parent_data = parent_data;
> +       init.parent_hws = parent_hw ? &parent_hw : NULL;

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

> +       init.num_parents = num_parents;
> +       init.flags = flags;
> +
> +       clk->clk_regmap = clk_regmap;
> +       clk->hw.init = &init;
> +       clk->offset = offset;
> +       clk->shift = shift;
> +       clk->mask = mask;
> +       clk->width = width;
> +       clk->table = table;
> +       clk->flags = clk_flags;
> +
> +       ret = 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 = to_npcm8xx_clk(hw);
> +       unsigned int val;
> +
> +       regmap_read(div->clk_regmap, div->offset, &val);
> +       val = val >> div->shift;
> +       val &= 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 = {
> +       .recalc_rate = npcm8xx_clk_div_get_parent,
> +};
> +
> +static int npcm8xx_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *parent_np = 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 = &pdev->dev;
> +       struct regmap *clk_regmap;
> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> +                                                        NPCM8XX_NUM_CLOCKS),
> +                                       GFP_KERNEL);
> +       if (!npcm8xx_clk_data)
> +               return -ENOMEM;
> +
> +       clk_regmap = 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 = "nuvoton,npcm845-gcr", "syscon";
                        reg = <0x0 0xf0800000 0x0 0x1000>;
                };