Re: [PATCH v1 2/4] clk: qcom: Add support for Display Clock Controller on SM8450

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 23 Jun 06:47 CDT 2022, Dmitry Baryshkov wrote:
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index bc4dcf356d82..f409b891fce4 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -635,6 +635,15 @@ config SM_DISPCC_6350
>  	  Say Y if you want to support display devices and functionality such as
>  	  splash screen.
>  
> +config SM_DISPCC_8450
> +	tristate "SM8450 Display Clock Controller"
> +	depends on SM_GCC_8450
> +	help
> +	  Support for the display clock controller on Qualcomm Technologies, Inc
> +	  SM8250 devices.

s/2/4/

> +	  Say Y if you want to support display devices and functionality such as
> +	  splash screen.
> +
[..]
> diff --git a/drivers/clk/qcom/dispcc-sm8450.c b/drivers/clk/qcom/dispcc-sm8450.c
[..]
> +static struct clk_alpha_pll disp_cc_pll0 = {
> +	.offset = 0x0,
> +	.vco_table = lucid_evo_vco,
> +	.num_vco = ARRAY_SIZE(lucid_evo_vco),
> +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
> +	.clkr = {
> +		.hw.init = &(struct clk_init_data){
> +			.name = "disp_cc_pll0",
> +			.parent_data = &(const struct clk_parent_data){
> +				.fw_name = "bi_tcxo",

Can we please have this transitioned to using .index as in the other new
clock drivers?

Also, you have a bi_tcxo clk_parent_data below, how about using that?

And lastly, could I please have a space inbetween ) and {?

> +			},
> +			.num_parents = 1,
> +			.ops = &clk_alpha_pll_reset_lucid_evo_ops,
> +		},
> +	},
> +};
> +
[..]
> +static int __init disp_cc_sm8450_init(void)
> +{
> +	return platform_driver_register(&disp_cc_sm8450_driver);
> +}
> +subsys_initcall(disp_cc_sm8450_init);
> +
> +static void __exit disp_cc_sm8450_exit(void)
> +{
> +	platform_driver_unregister(&disp_cc_sm8450_driver);
> +}
> +module_exit(disp_cc_sm8450_exit);

You should be able to module_platform_driver() this instead.

> +
> +MODULE_DESCRIPTION("QTI DISP_CC WAIPIO Driver");

While not secret, please update this to use SM8450 and perhaps a more
human readable form?

> +MODULE_LICENSE("GPL v2");

Please change this to MODULE_LICENSE("GPL")

Regards,
Bjorn