linux-kernel - Re: [PATCH v2 2/4] clk: sophgo: Add CV1800 series clock controller 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <IA1PR20MB4953D20414ECD451EC2F388CBB84A@IA1PR20MB4953.namprd20.prod.outlook.com>
Date:   Wed,  6 Dec 2023 19:01:34 +0800
From:   Inochi Amaoto <inochiama@...look.com>
To:     Emil Renner Berthing <emil.renner.berthing@...onical.com>,
        Chen Wang <unicorn_wang@...look.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Chao Wei <chao.wei@...hgo.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>
Cc:     Inochi Amaoto <inochiama@...look.com>,
        Jisheng Zhang <jszhang@...nel.org>, qiujingbao.dlmu@...il.com,
        dlan@...too.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 2/4] clk: sophgo: Add CV1800 series clock controller driver

>
>
>Inochi Amaoto wrote:
>>>
>>> On 2023/12/5 19:55, Inochi Amaoto wrote:
>>>> Add driver for CV1800 series clock controller.
>>> Add more clarification on your changes. Seems you add several files with different names for different products, what's your design idea, please add some brief introduction.
>>
>> In fact, it just adds the driver for the whole CV18XX series. I do not
>> think its clock controller has something different for different product.
>> The CV181X does have more clock, but it shares the same driver code of
>> CV180X. All the things just follow the manual and are for the hardware
>> design. Anyway, I will have a try.
>>
>>>>
>>>> Signed-off-by: Inochi Amaoto <inochiama@...look.com>
>>>> Link: https://github.com/milkv-duo/duo-files/blob/main/hardware/CV1800B/CV180X-Clock-v1.xlsx
>>>> Link: https://github.com/milkv-duo/duo-files/blob/main/hardware/CV1800B/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf
>>>> ---
>>>> ......
>>>> diff --git a/drivers/clk/sophgo/Kconfig b/drivers/clk/sophgo/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..243d58a30117
>>>> --- /dev/null
>>>> +++ b/drivers/clk/sophgo/Kconfig
>>>> @@ -0,0 +1,7 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# common clock support for SOPHGO SoC family.
>>> Drop this comment line, moving forward, this Kconfig file will be re-used for more different sophgo products.
>>
>> Why? I do not think this have some impact on reuse.
>>
>>>> +
>>>> +config CLK_SOPHGO_CV1800
>>>> +    tristate "Support for the Sophgo CV1800 series SoCs clock controller"
>>>> +    default y
>>>> +    depends on ARCH_SOPHGO || COMPILE_TEST
>>> Suggest to add some help words for this config item.
>>>
>>
>> There is no extra information other than this title.
>> In fact, I think the description title is enough to describe this. Add
>> a duplicate help is useless.
>
>I'd also like to see some more information here. Eg. what are examples of SoC's
>in the CV1800 series. checkpatch also complains:
>
>WARNING: please write a help paragraph that fully describes the config symbol
>#337: FILE: drivers/clk/sophgo/Kconfig:4:
>+config CLK_SOPHGO_CV1800
>+	tristate "Support for the Sophgo CV1800 series SoCs clock controller"
>+	default y
>+	depends on ARCH_SOPHGO || COMPILE_TEST
>

OK, thanks for your advice, I will add this help.

>
>Also the driver says "tristate" here, but defaults to built-in. If it works as
>a module it should be default m to not waste memory on systems not needing this.
>If it can't work properly as a module then tristate should be bool and the
>driver should be updated for that.
>
>/Emil
>

I will have a try and determine the right value. Thanks.