Re: [PATCH v2 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Johnson Wang <johnson.wang@mediatek.com>,
	cw00.choi@samsung.com, krzk+dt@kernel.org, robh+dt@kernel.org,
	kyungmin.park@samsung.com
Cc: khilman@kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, jia-wei.chang@mediatek.com,
	Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH v2 2/2] PM / devfreq: mediatek: Introduce MediaTek CCI devfreq driver
Date: Fri, 8 Apr 2022 10:21:30 +0200	[thread overview]
Message-ID: <76d58182-2f56-32b2-42e9-2ecbdd09ba3d@linaro.org> (raw)
In-Reply-To: <20220408052150.22536-3-johnson.wang@mediatek.com>

On 08/04/2022 07:21, Johnson Wang wrote:
> We introduce a devfreq driver for the MediaTek Cache Coherent Interconnect
> (CCI) used by some MediaTek SoCs.
> 
(...)

> index 87eb2b837e68..d985597f343f 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -120,6 +120,16 @@ config ARM_TEGRA_DEVFREQ
>  	  It reads ACTMON counters of memory controllers and adjusts the
>  	  operating frequencies and voltages with OPP support.
>  
> +config ARM_MEDIATEK_CCI_DEVFREQ
> +	tristate "MEDIATEK CCI DEVFREQ Driver"
> +	depends on ARM_MEDIATEK_CPUFREQ

|| COMPILE_TEST? And check if it test compiles.

(...)

> +
> +static int mtk_ccifreq_remove(struct platform_device *pdev)
> +{
> +	struct device *cci_dev = &pdev->dev;

s/cci_dev/dev/
Everywhere.

> +	struct mtk_ccifreq_drv *drv;
> +
> +	drv = platform_get_drvdata(pdev);
> +
> +	dev_pm_opp_unregister_notifier(cci_dev, &drv->opp_nb);
> +	dev_pm_opp_of_remove_table(cci_dev);
> +	clk_disable_unprepare(drv->inter_clk);
> +	clk_disable_unprepare(drv->cci_clk);
> +	regulator_disable(drv->proc_reg);
> +	if (!IS_ERR(drv->sram_reg))
> +		regulator_disable(drv->sram_reg);
> +
> +	return 0;
> +}
> +
> +static const struct mtk_ccifreq_platform_data mt8183_platform_data = {
> +	.min_volt_shift = 100000,
> +	.max_volt_shift = 200000,
> +	.proc_max_volt = 1150000,
> +	.sram_min_volt = 0,
> +	.sram_max_volt = 1150000,
> +};
> +
> +static const struct mtk_ccifreq_platform_data mt8186_platform_data = {
> +	.min_volt_shift = 100000,
> +	.max_volt_shift = 250000,
> +	.proc_max_volt = 1118750,
> +	.sram_min_volt = 850000,
> +	.sram_max_volt = 1118750,
> +};
> +
> +static const struct of_device_id mtk_ccifreq_machines[] = {
> +	{ .compatible = "mediatek,mt8183-cci", .data = &mt8183_platform_data },
> +	{ .compatible = "mediatek,mt8186-cci", .data = &mt8186_platform_data },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mtk_ccifreq_machines);
> +
> +static struct platform_driver mtk_ccifreq_platdrv = {
> +	.probe	= mtk_ccifreq_probe,
> +	.remove	= mtk_ccifreq_remove,
> +	.driver = {
> +		.name = "mtk-ccifreq",
> +		.of_match_table = of_match_ptr(mtk_ccifreq_machines),

You use of_match_ptr() so is it possible to build it without OF? If so,
then mtk_ccifreq_machines needs maybe_unused.

> +	},
> +};
> +
> +static int __init mtk_ccifreq_platdrv_init(void)
> +{
> +	return platform_driver_register(&mtk_ccifreq_platdrv);
> +}
> +module_init(mtk_ccifreq_platdrv_init)
> +
> +static void __exit mtk_ccifreq_platdrv_exit(void)
> +{
> +	platform_driver_unregister(&mtk_ccifreq_platdrv);
> +}
> +module_exit(mtk_ccifreq_platdrv_exit)

Why not module_platform_driver()?

> +
> +MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
> +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@mediatek.com>");
> +MODULE_LICENSE("GPL v2");

Best regards,
Krzysztof