Hi Krzysztof,
On Fri, 2022-04-08 at 10:21 +0200, Krzysztof Kozlowski wrote:
> 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.
Thank you for your review.
I will add COMPILE_TEST and see if this driver is compiled with it.
>
> (...)
>
> > +
> > +static int mtk_ccifreq_remove(struct platform_device *pdev)
> > +{
> > + struct device *cci_dev = &pdev->dev;
>
> s/cci_dev/dev/
> Everywhere.
>
Sure, I will modify it in the next version.
> > + 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.
>
No, this driver must be built with OF due to our CPU arch.
Should I add some dependencies in Kconfig to ensure OF is enabled?
> > + },
> > +};
> > +
> > +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()?
I will try to use module_platform_driver() and update it in the next
version.
>
> > +
> > +MODULE_DESCRIPTION("MediaTek CCI devfreq driver");
> > +MODULE_AUTHOR("Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
>
>
> Best regards,
> Krzysztof
BRs,
Johnson Wang
