On 5/30/22 9:16 PM, AngeloGioacchino Del Regno wrote: > Il 27/05/22 13:00, Johnson Wang ha scritto: >> We introduce a devfreq driver for the MediaTek Cache Coherent Interconnect >> (CCI) used by some MediaTek SoCs. >> >> In this driver, we use the passive devfreq driver to get target frequencies >> and adjust voltages accordingly. In MT8183 and MT8186, the MediaTek CCI >> is supplied by the same regulators with the little core CPUs. >> >> Signed-off-by: Jia-Wei Chang <jia-wei.chang@xxxxxxxxxxxx> >> Signed-off-by: Johnson Wang <johnson.wang@xxxxxxxxxxxx> >> Acked-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx> >> --- >> This patch depends on "devfreq-testing"[1]. >> [1]https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/log/?h=devfreq-testing >> --- >> drivers/devfreq/Kconfig | 10 + >> drivers/devfreq/Makefile | 1 + >> drivers/devfreq/mtk-cci-devfreq.c | 441 ++++++++++++++++++++++++++++++ >> 3 files changed, 452 insertions(+) >> create mode 100644 drivers/devfreq/mtk-cci-devfreq.c >> >> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig >> index 87eb2b837e68..9754d8b31621 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 >> + select DEVFREQ_GOV_PASSIVE >> + help >> + This adds a devfreq driver for MediaTek Cache Coherent Interconnect >> + which is shared the same regulators with the cpu cluster. It can track >> + buck voltages and update a proper CCI frequency. Use the notification >> + to get the regulator status. >> + >> config ARM_RK3399_DMC_DEVFREQ >> tristate "ARM RK3399 DMC DEVFREQ Driver" >> depends on (ARCH_ROCKCHIP && HAVE_ARM_SMCCC) || \ >> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile >> index 0b6be92a25d9..bf40d04928d0 100644 >> --- a/drivers/devfreq/Makefile >> +++ b/drivers/devfreq/Makefile >> @@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o >> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o >> obj-$(CONFIG_ARM_IMX_BUS_DEVFREQ) += imx-bus.o >> obj-$(CONFIG_ARM_IMX8M_DDRC_DEVFREQ) += imx8m-ddrc.o >> +obj-$(CONFIG_ARM_MEDIATEK_CCI_DEVFREQ) += mtk-cci-devfreq.o >> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o >> obj-$(CONFIG_ARM_SUN8I_A33_MBUS_DEVFREQ) += sun8i-a33-mbus.o >> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o >> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c >> new file mode 100644 >> index 000000000000..df42da35b312 >> --- /dev/null >> +++ b/drivers/devfreq/mtk-cci-devfreq.c >> @@ -0,0 +1,441 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2022 MediaTek Inc. >> + */ >> + > > ..snip.. > >> +}; >> + >> +static int mtk_ccifreq_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct mtk_ccifreq_drv *drv; >> + struct devfreq_passive_data *passive_data; >> + struct dev_pm_opp *opp; >> + unsigned long rate, opp_volt; >> + int ret; >> + >> + drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL); >> + if (!drv) >> + return -ENOMEM; >> + >> + drv->dev = dev; >> + drv->soc_data = (const struct mtk_ccifreq_platform_data *) >> + of_device_get_match_data(&pdev->dev); >> + mutex_init(&drv->reg_lock); >> + platform_set_drvdata(pdev, drv); >> + >> + drv->cci_clk = devm_clk_get(dev, "cci"); >> + if (IS_ERR(drv->cci_clk)) { >> + ret = PTR_ERR(drv->cci_clk); >> + return dev_err_probe(dev, ret, >> + "failed to get cci clk: %d\n", ret); >> + } >> + >> + drv->inter_clk = devm_clk_get(dev, "intermediate"); >> + if (IS_ERR(drv->inter_clk)) { >> + ret = PTR_ERR(drv->inter_clk); >> + return dev_err_probe(dev, ret, >> + "failed to get intermediate clk: %d\n", ret); >> + } >> + >> + drv->proc_reg = devm_regulator_get_optional(dev, "proc"); > > In the devicetree binding for this driver, the "proc" regulator is *not* optional, > but here you're using devm_regulator_get_optional. > > If this is not optional, you should use devm_regulator_get() instead. > >> + if (IS_ERR(drv->proc_reg)) { >> + ret = PTR_ERR(drv->proc_reg); >> + return dev_err_probe(dev, ret, >> + "failed to get proc regulator: %d\n", ret); > > There's no need to print ret... dev_err_probe() takes care of that for you already: > in this case, you're printing the value of ret twice. > I agree. better to remove the additional 'ret' printing from dev_err_probe. >> + } >> + >> + ret = regulator_enable(drv->proc_reg); > > If you move this call after the devm_regulator_get_optional() call for the sram > vreg, you will be able to use dev_err_probe for the latter as well. > >> + if (ret) { >> + dev_err(dev, "failed to enable proc regulator\n"); > > Why aren't you using dev_err_probe here, like you've done for the other instances? dev_err is enough because it means that get to regulator instance via regulator_get_optional. If error from regulator_enable happen, it depends on the internal logic of pmic driver. > >> + return ret; >> + } >> + >> + drv->sram_reg = devm_regulator_get_optional(dev, "sram"); >> + if (IS_ERR(drv->sram_reg)) >> + drv->sram_reg = NULL; > > When you use regulator_get_optional() (including the devm_ variant of it), you > shall return an error, if there's any... that's what the _optional() is for. > > if (IS_ERR(drv->sram_reg)) > return dev_err_probe(dev, PTR_ERR(drv->proc_reg), > "failed to get sram regulator"); > >> + else { >> + ret = regulator_enable(drv->sram_reg); >> + if (ret) { >> + dev_err(dev, "failed to enable sram regulator\n"); >> + goto out_free_resources; >> + } >> + } >> + > > Regards, > Angelo > Hi Johnson, Thanks for your work. Before merge, better to remove the 'ret' printing from dev_err_probe above. I'll merge it. -- Best Regards, Chanwoo Choi Samsung Electronics
