Hi Chanwoo, On 13.03.2014 09:17, Chanwoo Choi wrote:
There are not the clock controller of ppmudmc0/1. This patch control the clock of ppmudmc0/1 which is used for monitoring memory bus utilization. Also, this patch code clean about regulator control and free resource when calling exit/remove function. For example, busfreq@106A0000 { compatible = "samsung,exynos4x12-busfreq"; /* Clock for PPMUDMC0/1 */ clocks = <&clock CLK_PPMUDMC0>, <&clock CLK_PPMUDMC1>; clock-names = "ppmudmc0", "ppmudmc1"; /* Regulator for MIF/INT block */ vdd_mif-supply = <&buck1_reg>; vdd_int-supply = <&buck3_reg>; }; Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx> --- drivers/devfreq/exynos/exynos4_bus.c | 114 ++++++++++++++++++++++++++++++----- 1 file changed, 100 insertions(+), 14 deletions(-) diff --git a/drivers/devfreq/exynos/exynos4_bus.c b/drivers/devfreq/exynos/exynos4_bus.c index 1a0effa..a2a3a47 100644 --- a/drivers/devfreq/exynos/exynos4_bus.c +++ b/drivers/devfreq/exynos/exynos4_bus.c @@ -62,6 +62,11 @@ enum exynos_ppmu_idx { PPMU_END, }; +static const char *exynos_ppmu_clk_name[] = { + [PPMU_DMC0] = "ppmudmc0", + [PPMU_DMC1] = "ppmudmc1", +}; + #define EX4210_LV_MAX LV_2 #define EX4x12_LV_MAX LV_4 #define EX4210_LV_NUM (LV_2 + 1) @@ -86,6 +91,7 @@ struct busfreq_data { struct regulator *vdd_mif; /* Exynos4412/4212 only */ struct busfreq_opp_info curr_oppinfo; struct exynos_ppmu ppmu[PPMU_END]; + struct clk *clk_ppmu[PPMU_END]; struct notifier_block pm_notifier; struct mutex lock; @@ -722,8 +728,26 @@ static int exynos4_bus_get_dev_status(struct device *dev, static void exynos4_bus_exit(struct device *dev) { struct busfreq_data *data = dev_get_drvdata(dev); + int i; + + /* + * Un-map memory map and disable regulator/clocks + * to prevent power leakage. + */ + regulator_disable(data->vdd_int); + if (data->type == TYPE_BUSF_EXYNOS4x12) + regulator_disable(data->vdd_mif); + + for (i = 0; i < PPMU_END; i++) { + if (data->clk_ppmu[i])
This check is invalid. Clock pointers must be checked for validity using the IS_ERR() macro, because NULL is a valid clock pointer value indicating a dummy clock.
+ clk_disable_unprepare(data->clk_ppmu[i]); + } - devfreq_unregister_opp_notifier(dev, data->devfreq); + for (i = 0; i < PPMU_END; i++) { + if (data->ppmu[i].hw_base)
Can this even happen? Is there a PPMU without registers?
+ iounmap(data->ppmu[i].hw_base); + + } } static struct devfreq_dev_profile exynos4_devfreq_profile = { @@ -987,6 +1011,7 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) { struct device *dev = data->dev; struct device_node *np = dev->of_node; + const char **clk_name = exynos_ppmu_clk_name; int i, ret; if (!np) { @@ -1005,8 +1030,70 @@ static int exynos4_busfreq_parse_dt(struct busfreq_data *data) } } + /* + * Get PPMU's clocks to control them. But, if PPMU's clocks + * is default 'pass' state, this driver don't need control + * PPMU's clock. + */ + for (i = 0; i < PPMU_END; i++) { + data->clk_ppmu[i] = devm_clk_get(dev, clk_name[i]); + if (IS_ERR_OR_NULL(data->clk_ppmu[i])) {
Again, this check is invalid. Only IS_ERR() is the correct way to check whether returned clock pointer is valid.
+ dev_warn(dev, "Cannot get %s clock\n", clk_name[i]); + data->clk_ppmu[i] = NULL;
This assignment is wrong. To allow further checking whether the clock was found the value returned from devm_clk_get() must be retained and then IS_ERR() used in further code.
However, I believe it should be an error if a clock is not provided. The driver must make sure that PPMU clocks are ungated before trying to access them, otherwise the system might hang.
+ } + + ret = clk_prepare_enable(data->clk_ppmu[i]);
The code above allows the clock to be skipped, but this line doesn't check whether it is valid. Still, I think the clock should be always required.
+ if (ret < 0) { + dev_warn(dev, "Cannot enable %s clock\n", clk_name[i]); + data->clk_ppmu[i] = NULL; + goto err_clocks; + } + } + + /* Get regulator to control voltage of int block */ + data->vdd_int = devm_regulator_get(dev, "vdd_int"); + if (IS_ERR(data->vdd_int)) { + dev_err(dev, "Failed to get the regulator of vdd_int\n"); + ret = PTR_ERR(data->vdd_int); + goto err_clocks; + } + ret = regulator_enable(data->vdd_int); + if (ret < 0) { + dev_err(dev, "Failed to enable regulator of vdd_int\n"); + goto err_clocks; + } + + switch (data->type) { + case TYPE_BUSF_EXYNOS4210: + break; + case TYPE_BUSF_EXYNOS4x12: + /* Get regulator to control voltage of mif blk if Exynos4x12 */ + data->vdd_mif = devm_regulator_get(dev, "vdd_mif"); + if (IS_ERR(data->vdd_mif)) { + dev_err(dev, "Failed to get the regulator vdd_mif\n"); + ret = PTR_ERR(data->vdd_mif); + goto err_regulator; + } + ret = regulator_enable(data->vdd_mif); + if (ret < 0) { + dev_err(dev, "Failed to enable regulator of vdd_mif\n"); + goto err_regulator; + } + break; + default: + dev_err(dev, "Unknown device type : %d\n", data->type); + return -EINVAL; + }; + return 0; +err_regulator: + regulator_disable(data->vdd_int); +err_clocks: + for (i = 0; i < PPMU_END; i++) { + if (data->clk_ppmu[i])
Invalid check. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html