Quoting Ryan Chen (2024-10-31 00:24:39)
> > Subject: Re: [PATCH v7 3/3] clk: aspeed: add AST2700 clock driver.
> >
> > Quoting Ryan Chen (2024-10-27 22:30:18)
> > > diff --git a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new
> > > file mode 100644 index 000000000000..db9ee5031b7c
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-ast2700.c
> > > @@ -0,0 +1,1513 @@
> > > +// SPDX-License-Identifier: GPL-2.0
[...]
> > > +struct ast2700_clk_info {
> > > + const char *name;
> > > + const char * const *parent_names;
> >
> > Please don't use strings for parent names.
> Sorry, do you mean use clk_parent_data struct for parent?
> +const struct clk_parent_data parent; /* For gate */
> +const struct clk_parent_data *parents; /* For mux */
Yes.
>
> >
> > > + const struct clk_div_table *div_table;
> > > + unsigned long fixed_rate;
> > > + unsigned int mult;
> > > + unsigned int div;
> > > + u32 reg;
> > > + u32 flags;
> > > + u32 type;
> > > + u8 clk_idx;
> > > + u8 bit_shift;
> > > + u8 bit_width;
> > > + u8 num_parents;
> > > +};
> > > +
> > [...]
> > > +
> > > +static const struct clk_div_table ast2700_clk_div_table2[] = {
> > > + { 0x0, 2 },
> > > + { 0x1, 4 },
> > > + { 0x2, 6 },
> > > + { 0x3, 8 },
> > > + { 0x4, 10 },
> > > + { 0x5, 12 },
> > > + { 0x6, 14 },
> > > + { 0x7, 16 },
> >
> > Isn't this the default divider setting for struct clk_divider?
> Sorry, I don't catch your point.
> the SoC do have default divider setting. But it can be modified.
> And also have different divider table setting.
I mean that this is the way that struct clk_divider works already. So
you don't need to make the clk_div_table array for what is supported in
code.
> >
> > > + { 0 }
> > > +};
> > > +
> > > +static const struct clk_div_table ast2700_clk_uart_div_table[] = {
> > > + { 0x0, 1 },
> > > + { 0x1, 13 },
> > > + { 0 }
> > [...]
> > > + .bit_shift = 23,
> > > + .bit_width = 3,
> > > + .div_table = ast2700_clk_div_table2,
> > > + },
> > > + [SCU0_CLK_GATE_MCLK] = {
> > > + .type = CLK_GATE_ASPEED,
> > > + .name = "mclk-gate",
> > > + .parent_names = (const char *[]){ "soc0-mpll", },
> > > + .reg = SCU0_CLK_STOP,
> > > + .clk_idx = 0,
> > > + .flags = CLK_IS_CRITICAL,
> > > + },
> > > + [SCU0_CLK_GATE_ECLK] = {
> > > + .type = CLK_GATE_ASPEED,
> > > + .name = "eclk-gate",
> > > + .parent_names = (const char *[]){ },
> > > + .reg = SCU0_CLK_STOP,
> > > + .clk_idx = 1,
> > > + },
> > > + [SCU0_CLK_GATE_2DCLK] = {
> > > + .type = CLK_GATE_ASPEED,
> > > + .name = "gclk-gate",
> > > + .parent_names = (const char *[]){ },
> >
> > This has no parent? Why is parent_names set to an empty array?
> Due to I use clk->parent_names[0] for clk_hw_register_gate, const char *name parameter input.
> If null, that will cause panic for NULL point.
But the parent is NULL? How many parents does this clk have?
> >
> > > + if (!clk_data)
> > > + return devm_of_platform_populate(dev);
> >
> > What is being populated? Isn't there always clk_data?
> Yes, it is always clk_data, I will modify to be following, is it ok?
> If(!clk_data)
> Return -ENODEV;
>
Sure.
> >
> > Please don't use strings for parent_names. Use clk_hw pointers or DT indices.
> Use clk_pareent_data is it ok ?
Yes.
