Hi Tomasz,
On Wed, Jul 16, 2014 at 5:25 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> Hi Abhilash,
>
> Please see my comments inline.
>
> On 15.07.2014 20:36, Abhilash Kesavan wrote:
>> From: "Arjun.K.V" <arjun.kv@xxxxxxxxxxx>
>>
>> Exynos5420 bus device devfreq driver monitors PPMU counters and
>> adjusts INT domain operating frequencies and voltages. Support
>> for 5420 has been added to the extant 5250 support.
>>
>> Signed-off-by: Arjun.K.V <arjun.kv@xxxxxxxxxxx>
>> Signed-off-by: Andrew Bresticker <abrestic@xxxxxxxxxxxx>
>> Signed-off-by: Doug Anderson <dianders@xxxxxxxxxxxx>
>> Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
>> ---
>> drivers/devfreq/Kconfig | 10 +-
>> drivers/devfreq/exynos/exynos5_bus.c | 598 ++++++++++++++++++++++++++++------
>> 2 files changed, 508 insertions(+), 100 deletions(-)
>
> [snip]
>
>>
>> -#define MAX_SAFEVOLT 1100000 /* 1.10V */
>> -/* Assume that the bus is saturated if the utilization is 25% */
>> -#define INT_BUS_SATURATION_RATIO 25
>> +/* Assume that we need to bump up the level if the utilization is 10% */
>> +#define INT_BUS_SATURATION_RATIO 10
>
> There is nothing about this change in commit message and changing the
> ratio from 25% to 10% seems to be rather significant. Please give the
> rationale for this change.
Will do.
>
>> +#define INT_VOLT_STEP_UV 12500
>> +
>> +struct exynos5_busfreq_drv_data {
>> + int busf_type;
>> +};
>> +
>> +enum exynos5_busf_type {
>> + TYPE_BUSF_EXYNOS5250,
>> + TYPE_BUSF_EXYNOS5420,
>> +};
>
> Using this kind of enums is discouraged. Rather than that, just create a
> structure that describes the differences between variants and use this
> as type.
OK.
>
>>
>> enum int_level_idx {
>> LV_0,
>> @@ -41,12 +46,31 @@ enum int_level_idx {
>> _LV_END
>> };
>
> [snip]
>
>> +
>> +static struct int_bus_opp_table *exynos5_int_opp_table;
>> +static struct int_bus_opp_table exynos5250_int_opp_table[] = {
>> {LV_0, 266000, 1025000},
>> {LV_1, 200000, 1025000},
>> {LV_2, 160000, 1025000},
>
> [snip]
>
>> +static struct int_clk_info exynos5420_aclk_300_jpeg[] = {
>> + /* Level, Freq, Parent_Pll */
>> + {LV_0, 300000, D_PLL},
>> + {LV_1, 300000, D_PLL},
>> + {LV_2, 200000, D_PLL},
>> + {LV_3, 150000, D_PLL},
>> + {LV_4, 75000, D_PLL},
>> +};
>
> These should be parsed from DT.
OK.
>
>> +
>> +#define EXYNOS5420_INT_PM_CLK(NAME, CLK, PCLK, CLK_INFO) \
>> +static struct int_comp_clks int_pm_clks_##NAME = { \
>> + .mux_clk_name = CLK, \
>> + .div_clk_name = PCLK, \
>> + .clk_info = CLK_INFO, \
>> +}
>> +
>> +EXYNOS5420_INT_PM_CLK(aclk_200_fsys, "aclk200_fsys",
>> + "aclk200_fsys_d", exynos5420_aclk_200_fsys);
>> +EXYNOS5420_INT_PM_CLK(pclk_200_fsys, "pclk200_fsys",
>> + "pclk200_fsys_d", exynos5420_pclk_200_fsys);
>> +EXYNOS5420_INT_PM_CLK(aclk_100_noc, "aclk100_noc",
>> + "aclk100_noc_d", exynos5420_aclk_100_noc);
>> +EXYNOS5420_INT_PM_CLK(aclk_400_wcore, "aclk400_wcore",
>> + "aclk400_wcore_d", exynos5420_aclk_400_wcore);
>> +EXYNOS5420_INT_PM_CLK(aclk_200_fsys2, "aclk200_fsys2",
>> + "aclk200_fsys2_d", exynos5420_aclk_200_fsys2);
>> +EXYNOS5420_INT_PM_CLK(aclk_400_mscl, "aclk400_mscl",
>> + "aclk400_mscl_d", exynos5420_aclk_400_mscl);
>> +EXYNOS5420_INT_PM_CLK(aclk_166, "aclk166",
>> + "aclk166_d", exynos5420_aclk_166);
>> +EXYNOS5420_INT_PM_CLK(aclk_266, "aclk266",
>> + "aclk266_d", exynos5420_aclk_266);
>> +EXYNOS5420_INT_PM_CLK(aclk_66, "aclk66",
>> + "aclk66_d", exynos5420_aclk_66);
>> +EXYNOS5420_INT_PM_CLK(aclk_300_disp1, "aclk300_disp1",
>> + "aclk300_disp1_d", exynos5420_aclk_300_disp1);
>> +EXYNOS5420_INT_PM_CLK(aclk_300_jpeg, "aclk300_jpeg",
>> + "aclk300_jpeg_d", exynos5420_aclk_300_jpeg);
>> +EXYNOS5420_INT_PM_CLK(aclk_400_disp1, "aclk400_disp1",
>> + "aclk400_disp1_d", exynos5420_aclk_400_disp1);
>
> List of the clocks should be parsed from DT as well, without hardcoding
> data for every SoC in the driver.
>
>> +
>> +static struct int_comp_clks *exynos5420_int_pm_clks[] = {
>> + &int_pm_clks_aclk_200_fsys,
>> + &int_pm_clks_pclk_200_fsys,
>> + &int_pm_clks_aclk_100_noc,
>> + &int_pm_clks_aclk_400_wcore,
>> + &int_pm_clks_aclk_200_fsys2,
>> + &int_pm_clks_aclk_400_mscl,
>> + &int_pm_clks_aclk_166,
>> + &int_pm_clks_aclk_266,
>> + &int_pm_clks_aclk_66,
>> + &int_pm_clks_aclk_300_disp1,
>> + &int_pm_clks_aclk_300_jpeg,
>> + &int_pm_clks_aclk_400_disp1,
>> + NULL,
>> +};
>> +
>> +static int exynos5250_int_set_rate(struct busfreq_data_int *data,
>> unsigned long rate)
>> {
>> int index, i;
>>
>> - for (index = 0; index < ARRAY_SIZE(exynos5_int_opp_table); index++) {
>> - if (exynos5_int_opp_table[index].clk == rate)
>> + for (index = 0; index < ARRAY_SIZE(exynos5250_int_opp_table); index++) {
>> + if (exynos5250_int_opp_table[index].clk == rate)
>> break;
>> }
>>
>> @@ -161,8 +370,8 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
>> return -EINVAL;
>>
>> /* Change the system clock divider values */
>> - for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
>> - struct int_clk *clk_info = &exynos5_int_clks[i];
>> + for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
>> + struct int_simple_clk *clk_info = &exynos5250_int_clks[i];
>> int ret;
>>
>> ret = clk_set_rate(clk_info->clk,
>> @@ -177,10 +386,111 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
>> return 0;
>> }
>>
>> +static struct clk *exynos5420_find_pll(struct busfreq_data_int *data,
>> + enum int_bus_pll target_pll)
>> +{
>> + struct clk *target_src_clk = NULL;
>> +
>> + switch (target_pll) {
>> + case C_PLL:
>> + target_src_clk = data->mout_cpll;
>> + break;
>> + case M_PLL:
>> + target_src_clk = data->mout_mpll;
>> + break;
>> + case D_PLL:
>> + target_src_clk = data->mout_dpll;
>> + break;
>> + case I_PLL:
>> + target_src_clk = data->mout_ipll;
>> + break;
>> + default:
>> + break;
>> + }
>
> What about storing the clocks in an array? Then all you would need to do
> could be as simple as accessing data->plls[target_pll].
OK.
>
>> +
>> + return target_src_clk;
>> +}
>> +
>> +static int exynos5420_int_set_rate(struct busfreq_data_int *data,
>> + unsigned long target_freq, unsigned long pre_freq)
>> +{
>> + unsigned int i;
>> + unsigned long tar_rate;
>> + int target_idx = -EINVAL;
>> + int pre_idx = -EINVAL;
>> + struct int_comp_clks *int_clk;
>> + struct clk *new_src_pll;
>> + struct clk *old_src_pll;
>> + unsigned long old_src_rate, new_src_rate;
>> + unsigned long rate1, rate2, rate3, rate4;
>> +
>> + /* Find the levels for target and previous frequencies */
>> + for (i = 0; i < _LV_END; i++) {
>> + if (exynos5420_int_opp_table[i].clk == target_freq)
>> + target_idx = exynos5420_int_opp_table[i].idx;
>> + if (exynos5420_int_opp_table[i].clk == pre_freq)
>> + pre_idx = exynos5420_int_opp_table[i].idx;
>> + }
>> +
>> + list_for_each_entry(int_clk, &data->list, node) {
>> + tar_rate = int_clk->clk_info[target_idx].target_freq * 1000;
>> +
>> + old_src_pll = clk_get_parent(int_clk->mux_clk);
>> + new_src_pll = exynos5420_find_pll(data,
>> + int_clk->clk_info[target_idx].src_pll);
>> +
>> + if (old_src_pll == new_src_pll) {
>> + /* No need to change pll */
>> + clk_set_rate(int_clk->div_clk, tar_rate);
>> + pr_debug("%s: %s now %lu (%lu)\n", __func__,
>> + int_clk->mux_clk_name,
>> + clk_get_rate(int_clk->div_clk), tar_rate);
>> + continue;
>> + }
>> +
>> + old_src_rate = clk_get_rate(old_src_pll);
>> + new_src_rate = clk_get_rate(new_src_pll);
>> + rate1 = clk_get_rate(int_clk->div_clk);
>> +
>> + /*
>> + * If we're switching to a faster PLL we should bump up the
>> + * divider before switching.
>> + */
>> + if (new_src_rate > old_src_rate) {
>> + int new_div;
>> + unsigned long tmp_rate;
>> +
>> + new_div = DIV_ROUND_UP(new_src_rate, tar_rate);
>> + tmp_rate = DIV_ROUND_UP(old_src_rate, new_div);
>> + clk_set_rate(int_clk->div_clk, tmp_rate);
>> + }
>> + rate2 = clk_get_rate(int_clk->div_clk);
>> +
>> + /* We can safely change the mux now */
>> + clk_set_parent(int_clk->mux_clk, new_src_pll);
>> + rate3 = clk_get_rate(int_clk->div_clk);
>> +
>> + /*
>> + * Give us a proper divider; technically not needed in the case
>> + * where we pre-calculated the divider above (the new_src_rate >
>> + * old_src_rate case), but let's be formal about it.
>> + */
>> + clk_set_rate(int_clk->div_clk, tar_rate);
>> + rate4 = clk_get_rate(int_clk->div_clk);
>> +
>> + pr_debug("%s: %s => PLL %d; %lu=>%lu=>%lu=>%lu (%lu)\n",
>> + __func__, int_clk->mux_clk_name,
>> + int_clk->clk_info[target_idx].src_pll,
>> + rate1, rate2, rate3, rate4, tar_rate);
>> + }
>> + return 0;
>> +}
>
> The above function looks like it could be made much more generic and
> reused for Exynos5250 as well.
I will look at making it common for the two,
>
>> +
>> static int exynos5_int_setvolt(struct busfreq_data_int *data,
>> unsigned long volt)
>> {
>> - return regulator_set_voltage(data->vdd_int, volt, MAX_SAFEVOLT);
>> + return regulator_set_voltage(data->vdd_int, volt,
>> + volt + INT_VOLT_STEP_UV);
>> }
>>
>> static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,
>> @@ -218,18 +528,15 @@ static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,
>> if (data->disabled)
>> goto out;
>>
>> - if (freq > exynos5_int_opp_table[0].clk)
>> - pm_qos_update_request(&data->int_req, freq * 16 / 1000);
>> - else
>> - pm_qos_update_request(&data->int_req, -1);
>> -
>> if (old_freq < freq)
>> err = exynos5_int_setvolt(data, volt);
>> if (err)
>> goto out;
>>
>> - err = exynos5_int_set_rate(data, freq);
>> -
>> + if (data->type == TYPE_BUSF_EXYNOS5250)
>> + err = exynos5250_int_set_rate(data, freq);
>> + else
>> + err = exynos5420_int_set_rate(data, freq, old_freq);
>> if (err)
>> goto out;
>>
>> @@ -267,16 +574,20 @@ static int exynos5_int_get_dev_status(struct device *dev,
>> }
>>
>> static struct devfreq_dev_profile exynos5_devfreq_int_profile = {
>> - .initial_freq = 160000,
>> - .polling_ms = 100,
>> + .polling_ms = 10,
>
> Another change not mentioned in commit message.
>
>> .target = exynos5_busfreq_int_target,
>> .get_dev_status = exynos5_int_get_dev_status,
>> };
>>
>> -static int exynos5250_init_int_tables(struct busfreq_data_int *data)
>> +static int exynos5_init_int_tables(struct busfreq_data_int *data)
>> {
>> int i, err = 0;
>>
>> + if (data->type == TYPE_BUSF_EXYNOS5250)
>> + exynos5_int_opp_table = exynos5250_int_opp_table;
>> + else
>> + exynos5_int_opp_table = exynos5420_int_opp_table;
>> +
>> for (i = LV_0; i < _LV_END; i++) {
>> err = dev_pm_opp_add(data->dev, exynos5_int_opp_table[i].clk,
>> exynos5_int_opp_table[i].volt);
>> @@ -297,6 +608,7 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
>> struct dev_pm_opp *opp;
>> unsigned long maxfreq = ULONG_MAX;
>> unsigned long freq;
>> + unsigned long old_freq;
>> unsigned long volt;
>> int err = 0;
>>
>> @@ -322,8 +634,14 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
>> if (err)
>> goto unlock;
>>
>> - err = exynos5_int_set_rate(data, freq);
>> + old_freq = data->curr_freq;
>>
>> + if (data->type == TYPE_BUSF_EXYNOS5250)
>> + err = exynos5250_int_set_rate(data, freq);
>> + else if (data->type == TYPE_BUSF_EXYNOS5420)
>> + err = exynos5420_int_set_rate(data, freq, old_freq);
>> + else
>> + err = -EINVAL;
>> if (err)
>> goto unlock;
>>
>> @@ -345,16 +663,38 @@ unlock:
>> return NOTIFY_DONE;
>> }
>>
>> +static const struct of_device_id exynos5_busfreq_dt_match[];
>> +
>> +static inline int exynos5_busfreq_get_driver_data(struct platform_device *pdev)
>> +{
>> +#ifdef CONFIG_OF
>
> Exynos is DT-only, so there is no need to handle non-DT cases.
OK.
>
>> + struct exynos5_busfreq_drv_data *data;
>> +
>> + if (pdev->dev.of_node) {
>> + const struct of_device_id *match;
>> +
>> + match = of_match_node(exynos5_busfreq_dt_match,
>> + pdev->dev.of_node);
>> + data = (struct exynos5_busfreq_drv_data *) match->data;
>> + return data->busf_type;
>> + }
>> +#endif
>> + return platform_get_device_id(pdev)->driver_data;
>> +}
>> +
>> static int exynos5_busfreq_int_probe(struct platform_device *pdev)
>> {
>> struct busfreq_data_int *data;
>> struct busfreq_ppmu_data *ppmu_data;
>> + struct device_node *np = pdev->dev.of_node;
>> struct dev_pm_opp *opp;
>> struct device *dev = &pdev->dev;
>> - struct device_node *np;
>> unsigned long initial_freq;
>> unsigned long initial_volt;
>> + struct clk *mux_clk, *div_clk;
>> + struct int_comp_clks *int_pm_clk;
>> int err = 0;
>> + int nr_clk;
>> int i;
>>
>> data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data_int),
>> @@ -364,22 +704,27 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
>> return -ENOMEM;
>> }
>>
>> + INIT_LIST_HEAD(&data->list);
>> + data->type = exynos5_busfreq_get_driver_data(pdev);
>> +
>> ppmu_data = &data->ppmu_data;
>> - ppmu_data->ppmu_end = PPMU_END;
>> + if (data->type == TYPE_BUSF_EXYNOS5250) {
>> + ppmu_data->ppmu_end = PPMU_END_5250;
>> + } else if (data->type == TYPE_BUSF_EXYNOS5420) {
>> + ppmu_data->ppmu_end = PPMU_END_5420;
>> + } else {
>> + dev_err(dev, "Cannot determine the device id %d\n", data->type);
>> + return -EINVAL;
>> + }
>> +
>> ppmu_data->ppmu = devm_kzalloc(dev,
>> - sizeof(struct exynos_ppmu) * PPMU_END,
>> - GFP_KERNEL);
>> + sizeof(struct exynos_ppmu) * (ppmu_data->ppmu_end),
>> + GFP_KERNEL);
>> if (!ppmu_data->ppmu) {
>> dev_err(dev, "Failed to allocate memory for exynos_ppmu\n");
>> return -ENOMEM;
>> }
>>
>> - np = of_find_compatible_node(NULL, NULL, "samsung,exynos5250-ppmu");
>> - if (np == NULL) {
>> - pr_err("Unable to find PPMU node\n");
>> - return -ENOENT;
>> - }
>
> This was actually closer to the right solution than what this series
> does. Actually there was similar attempt already, but for Exynos4 and I
> even suggested how this could be handled properly. Please see [1] for
> the whole discussion.
>
> [1]
> https://www.mail-archive.com/linux-samsung-soc@xxxxxxxxxxxxxxx/msg27491.html
Chanwoo are you working on this for exynos4 ? As far as I can see
there has been no further effort in adding the power plane and ppmu
bindings. So, before I start working on this I wanted to confirm that
you don't have something in the pipeline.
Tomasz, I have read through the thread and as per your suggestions
this is how the dt bindings (for my INT use case) would look like:
ppmu_dmc0_0: ppmu@10D00000 {
/* Resources of PPMU */
}
ppmu_dmc0_1: ppmu@10D00000 {
/* Resources of PPMU */
}
ppmu_dmc_1_0: ppmu@10D00000 {
/* Resources of PPMU */
}
power-plane-int {
compatible = "exynos5420-int, power-plane";
samsung,ppmus = <&ppmu_dmc_0_0>, &ppmu_dmc_0_1>, &ppmu_dmc_1_0>;
samsung,devices = ??
clock-names = "aclk200_fsys", "pclk200_fsys", "aclk100_noc"...;
clocks = <&clock CLK_ACLK200_FSYS2>,...;
vdd_int-supply = <&buck3_reg>; /* VDD_INT */
operating-points = <
266000, 1300000
200000, 1250000
160000, 1225000
133000, 1200000
>;
};
Here are my doubts regarding this:
1) The INT bus consists of several IPs like MFC, DISP, HDMI, ISP etc.
The 3 PPMUs listed above monitor the load on the TOP block which these
IPs are a part of. What should be the "samsung, devices" associated
with it ?
2) I have currently listed the virtual INT bus frequencies in
"operating points" property. So, when there is a high load on the INT
bus, there would be a request for say a virtual frequency of 266MHz
which in turn would bump up several clocks (like MFC, DISP etc) to a
desired level. Is it OK to have the virtual INT bus frequencies listed
in DT ?
3) There are over 10 clocks in 5420 that need to be controlled as part
of the INT domain. Should all these clocks, parent clocks, hardware
recommended source PLLs and the recommended clock rates (at each of
the virtual INT bus frequencies) also be part of the power plane node
? So that I parse all these clocks from dt and associate them with
each of the virtual bus rates. Is that the correct approach ?
>
>> -
>> for (i = 0; i < ppmu_data->ppmu_end; i++) {
>> /* map PPMU memory region */
>> ppmu_data->ppmu[i].hw_base = of_iomap(np, i);
>> @@ -388,13 +733,17 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
>> return -ENOMEM;
>> }
>> }
>> +
>> data->pm_notifier.notifier_call = exynos5_busfreq_int_pm_notifier_event;
>> data->dev = dev;
>> mutex_init(&data->lock);
>>
>> - err = exynos5250_init_int_tables(data);
>> - if (err)
>> + err = exynos5_init_int_tables(data);
>> + if (err) {
>> + dev_err(dev, "Cannot initialize busfreq table %d\n",
>> + data->type);
>> return err;
>> + }
>>
>> data->vdd_int = devm_regulator_get(dev, "vdd_int");
>> if (IS_ERR(data->vdd_int)) {
>> @@ -402,18 +751,70 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
>> return PTR_ERR(data->vdd_int);
>> }
>>
>> - for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
>> - struct int_clk *clk_info = &exynos5_int_clks[i];
>> + if (data->type == TYPE_BUSF_EXYNOS5250) {
>> + for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
>> + struct int_simple_clk *clk_info =
>> + &exynos5250_int_clks[i];
>> +
>> + clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
>> + if (IS_ERR(clk_info->clk)) {
>> + dev_err(dev, "Failed to get clock %s\n",
>> + clk_info->clk_name);
>> + return PTR_ERR(clk_info->clk);
>> + }
>> + }
>> + } else {
>> + data->mout_ipll = devm_clk_get(dev, "mout_ipll");
>> + if (IS_ERR(data->mout_ipll)) {
>> + dev_err(dev, "Cannot get clock \"mout_ipll\"\n");
>> + return PTR_ERR(data->mout_ipll);
>> + }
>>
>> - clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
>> - if (IS_ERR(clk_info->clk)) {
>> - dev_err(dev, "Failed to get clock %s\n",
>> - clk_info->clk_name);
>> - return PTR_ERR(clk_info->clk);
>> + data->mout_mpll = devm_clk_get(dev, "mout_mpll");
>> + if (IS_ERR(data->mout_mpll)) {
>> + dev_err(dev, "Cannot get clock \"mout_mpll\"\n");
>> + return PTR_ERR(data->mout_mpll);
>> + }
>> +
>> + data->mout_dpll = devm_clk_get(dev, "mout_dpll");
>> + if (IS_ERR(data->mout_dpll)) {
>> + dev_err(dev, "Cannot get clock \"mout_dpll\"\n");
>> + return PTR_ERR(data->mout_dpll);
>> + }
>> +
>> + data->mout_cpll = devm_clk_get(dev, "mout_cpll");
>> + if (IS_ERR(data->mout_cpll)) {
>> + dev_err(dev, "Cannot get clock \"mout_cpll\"\n");
>> + return PTR_ERR(data->mout_cpll);
>> + }
>> +
>> + for (nr_clk = 0; exynos5420_int_pm_clks[nr_clk] != NULL;
>> + nr_clk++) {
>> + int_pm_clk = exynos5420_int_pm_clks[nr_clk];
>> + mux_clk = devm_clk_get(dev, int_pm_clk->mux_clk_name);
>> + if (IS_ERR(mux_clk)) {
>> + dev_err(dev, "Cannot get mux clock: %s\n",
>> + int_pm_clk->mux_clk_name);
>> + return PTR_ERR(mux_clk);
>> + }
>> + div_clk = devm_clk_get(dev, int_pm_clk->div_clk_name);
>> + if (IS_ERR(div_clk)) {
>> + dev_err(dev, "Cannot get div clock: %s\n",
>> + int_pm_clk->div_clk_name);
>> + return PTR_ERR(div_clk);
>> + }
>> + int_pm_clk->mux_clk = mux_clk;
>> + int_pm_clk->div_clk = div_clk;
>> + list_add_tail(&int_pm_clk->node, &data->list);
>
> All those could be probably handled with an array and a for loop.
>
> In general, this patch apparently contains many separate changes and not
> only is hard to review but also hard to debug potential problems - git
> bisect has commit granularity.
>
> Please refactor the driver step by step first and then add support for
> new SoC after it has all the needed features.
You are right. I will first post patches adding dt support for only
5250. If that looks good then will add for 5420.
Thanks for the comprehensive review.
Regards,
Abhilash
>
> 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
