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: <CAJM55Z9CCH3n5g4_cRid-inr0SU_Kk39AhbrCavGJKhOoz=asw@mail.gmail.com>
Date:   Wed, 6 Dec 2023 04:24:46 -0600
From:   Emil Renner Berthing <emil.renner.berthing@...onical.com>
To:     Inochi Amaoto <inochiama@...look.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:     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


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