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

[Inochi Amaoto]

>
>
>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@xxxxxxxxxxx>
>>>> 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.