Re: [PATCH v11 4/5] clk: sophgo: Add SG2042 clock driver

[Chen Wang <unicorn_wang@outlook.com>]

Thank you Stephen for your carefully check and comments, I will improve 
the code according to your inputs.

I have some additional explanations for two of these points.

On 2024/3/9 10:11, Stephen Boyd wrote:

[......]

>> +
>> +/*
>> + * Below array is the total combination lists of POSTDIV1 and POSTDIV2
>> + * for example:
>> + * postdiv1_2[0] = {2, 4, 8}
>> + *           ==> div1 = 2, div2 = 4 , div1 * div2 = 8
>> + * And POSTDIV_RESULT_INDEX point to 3rd element in the array
>> + */
>> +#define        POSTDIV_RESULT_INDEX    2
>> +static int postdiv1_2[][3] = {
> const? And move it to the function scope.
>
>> +       {2, 4,  8}, {3, 3,  9}, {2, 5, 10}, {2, 6, 12},
>> +       {2, 7, 14}, {3, 5, 15}, {4, 4, 16}, {3, 6, 18},
>> +       {4, 5, 20}, {3, 7, 21}, {4, 6, 24}, {5, 5, 25},
>> +       {4, 7, 28}, {5, 6, 30}, {5, 7, 35}, {6, 6, 36},
>> +       {6, 7, 42}, {7, 7, 49}
> It may be better to make it a struct with named members because I have
> no idea what each element means.
I plan to add some comments to explain the meaning of this array member. 
This array is only used in this function, and I think it is somewhat 
unnecessary to define a structure specifically for it. Adding some 
comments will help everyone understand better.

[......]

>> +
>> +/*
>> + * Common data of clock-controller
>> + * Note: this structure will be used both by clkgen & sysclk.
>> + * @iobase: base address of clock-controller
>> + * @regmap: base address of clock-controller for pll, just due to PLL uses
>> + *  regmap while others use iomem.
>> + * @lock: clock register access lock
>> + * @onecell_data: used for adding providers.
>> + */
>> +struct sg2042_clk_data {
>> +       void __iomem *iobase;
> Why not use a regmap for the iobase as well?
I plan to unify it as iomem. Anyway, just use one method should be fine.

[......]