[PATCH v2 2/4] clk: sophgo: Add CV1800 series clock controller driver

[Emil Renner Berthing]

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 at outlook.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