Re: [PATCH v14 1/1] clk: npcm8xx: add clock controller

[Tomer Maimon]

Hi Stephen,

Very sorry for the late reply.

On Fri, 16 Dec 2022 at 20:44, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
>
> 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[] = { 0, 1, 2, 3 };
> > +static const struct clk_parent_data pll_mux_parents[] = {
> > +       { .fw_name = NPCM8XX_CLK_S_PLL0, .name = 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
What do you mean by associated DT binding? does the.fw_name, for
example, NPCM8XX_CLK_S_PLL0 need to represent in the device tree?
> 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 does the
yes the PLL clocks are internal.
> 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.
but still, I have used devm_clk_hw_register_mux_parent_data_table
function to register the clock mux,
should I use  devm_clk_hw_register_mux_parent_hws function instead?
Does this modification need to be done in all the mux parent struct?
could you point me to some example in the Linux kernel how do you
think that I should represent the mux clock in the NPCM8XX clock
driver?
>
> > +       { .fw_name = NPCM8XX_CLK_S_PLL1, .name = NPCM8XX_CLK_S_PLL1 },
> > +       { .fw_name = NPCM8XX_CLK_S_REFCLK, .name = NPCM8XX_CLK_S_REFCLK },
>
> Maybe this is external? If so, it would be great to have this in the
> binding as a `clocks` property.
O.K.

Thanks,

Tomer