On 10/15/2015 02:05 AM, Stephen Boyd wrote:
On 10/14, Archit Taneja wrote:
diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c new file mode 100644 index 0000000..e71b4ee --- /dev/null +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c @@ -0,0 +1,529 @@ +/*
- Copyright (c) 2012-2015, The Linux Foundation. All rights reserved.
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License version 2 and
- only version 2 as published by the Free Software Foundation.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- */
+#include <linux/clk.h>
Is this include used?
It isn't. I'll remove it.
+#include <linux/clk-provider.h>
+#include "dsi_pll.h" +#include "dsi.xml.h"
+/*
[..]
+#define to_pll_28nm(x) container_of(x, struct dsi_pll_28nm, base)
+static bool pll_28nm_poll_for_ready(struct dsi_pll_28nm *pll_28nm,
u32 nb_tries, u32 timeout_us)Why not use unsigned types for these counts? I don't imagine we care about being precisely 32 bits.
Yeah. We don't even need an unsigned type. I'll replace it with integer type.
+{
- bool pll_locked = false;
- u32 val;
[..]
- DBG("id=%d", pll_28nm->id);
- /*
* before enabling the PLL, configure the bit clock divider since we* don't expose it as a clock to the outside world* 1: read back the byte clock divider that should aready be sets/aready/already/
Thanks, I'll fix this.
* 2: divide by 8 to get bit clock divider* 3: write it to POSTDIV1*/- val = pll_read(base + REG_DSI_28nm_8960_PHY_PLL_CTRL_9);
- byte_div = val + 1;
- bit_div = byte_div / 8;
- val = pll_read(base + REG_DSI_28nm_8960_PHY_PLL_CTRL_8);
[..]
+static void dsi_pll_28nm_destroy(struct msm_dsi_pll *pll) +{
- struct dsi_pll_28nm *pll_28nm = to_pll_28nm(pll);
- int i;
- msm_dsi_pll_helper_unregister_clks(pll_28nm->pdev,
pll_28nm->clks, pll_28nm->num_clks);- for (i = 0; i < NUM_PROVIDED_CLKS; i++)
pll_28nm->provided_clks[i] = NULL;- pll_28nm->num_clks = 0;
- pll_28nm->clk_data.clks = NULL;
- pll_28nm->clk_data.clk_num = 0;
Is all this really necessary?
It isn't. I copy pasted from dsi_pll_28nm.c and it had this. Will make a patch to remove it from that file too.
+}
+static int pll_28nm_register(struct dsi_pll_28nm *pll_28nm) +{
- char clk_name[32], parent[32], vco_name[32];
- struct clk_init_data vco_init = {
.parent_names = (const char *[]){ "pxo" },.num_parents = 1,.name = vco_name,.ops = &clk_ops_dsi_pll_28nm_vco,- };
- struct device *dev = &pll_28nm->pdev->dev;
- struct clk **clks = pll_28nm->clks;
- struct clk **provided_clks = pll_28nm->provided_clks;
- struct clk_bytediv *bytediv;
- struct clk_init_data bytediv_init;
struct clk_init_data bytediv_init = { };
Just in case we add some new field there?
Will do.
- int ret, num = 0;
- DBG("%d", pll_28nm->id);
- bytediv = devm_kzalloc(dev, sizeof(*bytediv), GFP_KERNEL);
- if (!bytediv)
return -ENOMEM;- pll_28nm->bytediv = bytediv;
- snprintf(vco_name, 32, "dsi%dvco_clk", pll_28nm->id);
- pll_28nm->base.clk_hw.init = &vco_init;
- clks[num++] = clk_register(dev, &pll_28nm->base.clk_hw);
- /* prepare and register bytediv */
- bytediv->hw.init = &bytediv_init;
- bytediv->reg = pll_28nm->mmio + REG_DSI_28nm_8960_PHY_PLL_CTRL_9;
- snprintf(parent, 32, "dsi%dvco_clk", pll_28nm->id);
- snprintf(clk_name, 32, "dsi%dpllbyte", pll_28nm->id);
- bytediv_init.name = clk_name;
- bytediv_init.ops = &clk_bytediv_ops;
- bytediv_init.flags = CLK_SET_RATE_PARENT;
- bytediv_init.parent_names = (const char *[]) { parent };
Can't we just do &parent instead of this anonymous array?
&parent doesn't make sense here. parent in this function is an array of characters, not a pointer to a character.
I can think of only this way. We do something similar when we call clk_register_mux() in dsi_pll_28nm.c.
Archit
- bytediv_init.num_parents = 1;
- /* DIV2 */