Quoting Ryan Chen (2024-12-01 19:12:01)

quoted

quoted

quoted

Subject: Re: [PATCH v7 3/3] clk: aspeed: add AST2700 clock driver.

Quoting Ryan Chen (2024-10-27 22:30:18)

quoted

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

[...]

quoted

quoted

quoted

+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.

And I find a better way for parent_data. The following is my modification. 
And parent_data will be union data structure. Like following.
Is this good direction? 

Looks OK.

#define DIVIDER_CLK(_id, _name, _parent, _reg, _shift, _width, _div_table) \
        [_id] = { \
                .type = CLK_DIVIDER, \
                .name = _name, \
                .data = { \
                        .div = { \
                                .parent = _parent, \
                                .reg = _reg, \
                                .bit_shift = _shift, \
                                .bit_width = _width, \
                                .div_table = _div_table, \
                        }, \
                }, \
        }
struct ast2700_clk_info {
        const char *name;
        u8 clk_idx;
        u32 reg;
        u32 type;
        union {
                struct ast2700_clk_fixed_factor_data factor;
                struct ast2700_clk_fixed_rate_data rate;
                struct ast2700_clk_gate_data gate;
                struct ast2700_clk_div_data div;
                struct ast2700_clk_pll_data pll;
                struct ast2700_clk_mux_data mux;
        } data;
};

struct ast2700_clk_div_data {
        const struct clk_div_table *div_table;
        const struct clk_parent_data *parent;
        u8 bit_shift;
        u8 bit_width;
        u32 reg;
};

static const struct ast2700_clk_info ast2700_scu0_clk_info[] __initconst = {
...........................
        DIVIDER_CLK(SCU0_CLK_AHB, "soc0-ahb", soc0_ahbmux,

Can you also show what soc0_ahbmux is?

                    SCU0_HWSTRAP1, 5, 2, ast2700_hclk_div_table),
......................

quoted

quoted

quoted

quoted

+       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;
+};
+

[...]

quoted

+
+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.

Sorry, I understand your point. But I trace the code didn't get any clue. "clk_divider work already".
finally function call will be __clk_hw_register_divider
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-divider.c#L589
It still need table point need to address. Can you give me more direction or example?

The 'table' member is optional. When the table is NULL, we treat each
number that fits into the divider field width as a divider. It may be
CLK_DIVIDER_EVEN_INTEGERS that you have here though.