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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 19 Mar 2024 15:40:46 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Stephen Boyd <sboyd@...nel.org>, Chen Wang <unicornxw@...il.com>,
 aou@...s.berkeley.edu, chao.wei@...hgo.com, conor@...nel.org,
 devicetree@...r.kernel.org, guoren@...nel.org, haijiao.liu@...hgo.com,
 inochiama@...look.com, jszhang@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
 mturquette@...libre.com, palmer@...belt.com, paul.walmsley@...ive.com,
 richardcochran@...il.com, robh+dt@...nel.org, samuel.holland@...ive.com,
 xiaoguang.xing@...hgo.com
Subject: Re: [PATCH v11 4/5] clk: sophgo: Add SG2042 clock driver

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.

[......]