Messages in this thread | | | Subject | Re: [PATCH v14 1/1] clk: npcm8xx: add clock controller | From | Stephen Boyd <> | Date | Wed, 25 Jan 2023 18:41:04 -0800 |
| |
Quoting Tomer Maimon (2023-01-17 09:35:33) > Hi Stephen, > > Very sorry for the late reply. > > On Fri, 16 Dec 2022 at 20:44, Stephen Boyd <sboyd@kernel.org> 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?
Yes it should match a string in the "clock-names" property for this clk provider's device node.
> > 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.
Ok.
> > 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?
Probably, yes.
> 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?
I don't know. If the clk is external to the provider, then it should be in .fw_name or .index and be provided through DT. Otherwise, if the clk is internal to the clk provider use direct pointers.
> > > > > + { .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. >
Is it external? If so, then fw_name would be correct. You can look at the kernel-doc above clk_core_get(), but I really just need to spend a few hours and write a proper kernel-doc for this stuff.
| |