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

[Chen Wang <unicorn_wang@xxxxxxxxxxx>]

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.

[......]