[PATCH 03/12] drm/msm/dsi: Add DSI PLL for 28nm 8960 PHY

[Stephen Boyd]

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?

> +#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.

> +{
> +	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 set

s/aready/already/

> +	 * 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?

> +}
> +
> +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?

> +	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?

> +	bytediv_init.num_parents = 1;
> +
> +	/* DIV2 */

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project