Re: [DPU PATCH v2 3/3] drm/msm/dp: add support for DP PLL driver

[Sam Ravnborg <sam@ravnborg.org>]

Hi Chandan

A few comments in the following.
Mostly nitpicks / style stuff, not a throughly review.

	Sam

> +config DRM_MSM_DP_PLL
> +	bool "Enable DP PLL driver in MSM DRM"

So DRM_MSM_DP_PLL cannot be 'm'.

> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -137,4 +137,10 @@ msm-$(CONFIG_DRM_MSM_DSI_14NM_PHY) += dsi/pll/dsi_pll_14nm.o
>  msm-$(CONFIG_DRM_MSM_DSI_10NM_PHY) += dsi/pll/dsi_pll_10nm.o
>  endif
>  
> +ifeq ($(CONFIG_DRM_MSM_DP_PLL),y)
> +msm-y += dp/pll/dp_pll.o
> +msm-y += dp/pll/dp_pll_10nm.o
> +msm-y += dp/pll/dp_pll_10nm_util.o
> +endif

Please write this in the Kbuild caninical style like this:
msm-$(DRM_MSM_DP_PLL) += dp/pll/dp_pll.o
msm-$(DRM_MSM_DP_PLL) += dp/pll/dp_pll_10nm.o
etc.

Or even better - descend into msm/dp/pll to build it - this is normal kernel style.

> +	if (!dp_parser) {
> +		DRM_ERROR("Parser not initialized.\n");
> +		return -EINVAL;
> +	}
> +
> +	pll_node = of_parse_phandle(pdev->dev.of_node, "pll-node", 0);
> +	if (!pll_node) {
> +		DRM_DEV_ERROR(&pdev->dev, "cannot find pll device\n");
> +		return -ENXIO;
> +	}
> +
> +	pll_pdev = of_find_device_by_node(pll_node);
> +	if (pll_pdev)
> +		dp_parser->pll = platform_get_drvdata(pll_pdev);
> +
> +	of_node_put(pll_node);
> +
> +	if (!pll_pdev || !dp_parser->pll) {
> +		DRM_DEV_ERROR(&pdev->dev, "%s: pll driver is not ready\n", __func__);
> +		return -EPROBE_DEFER;
> +	}

The use of DRM_*ERROR is inconsistent.
In one place DRM_ERROR is used, and string ends with '.'
In one place DRM_DEV_ERROR is used with a simple string.
In one place DRM_DEV_ERROR is used where the __func__ is added as parameter.
When reading the code such inconsistencies makes it harder to follow the code.

> +
> +	dp_parser->pll_dev = get_device(&pll_pdev->dev);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t dp_display_irq(int irq, void *dev_id)
>  {
>  	struct dp_display_private *dp = dev_id;
> @@ -114,6 +156,12 @@ static int dp_display_bind(struct device *dev, struct device *master,
>  		goto end;
>  	}
>  
> +	rc = dp_get_pll(dp);
> +	if (rc) {
> +		DRM_ERROR(" DP get PLL instance failed\n");
Any reason why the error is indented with a space?
Also, is the DRM*ERROR in dp_get_pll() not enough?

> diff --git a/drivers/gpu/drm/msm/dp/dp_power.h b/drivers/gpu/drm/msm/dp/dp_power.h
> index 76e2d3b..40d7e73 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.h
> +++ b/drivers/gpu/drm/msm/dp/dp_power.h
> @@ -14,6 +14,7 @@
>   * @init: initializes the regulators/core clocks/GPIOs/pinctrl
>   * @deinit: turns off the regulators/core clocks/GPIOs/pinctrl
>   * @clk_enable: enable/disable the DP clocks
> + * @set_link_clk_parent: set the parent of DP link clock
>   * @set_pixel_clk_parent: set the parent of DP pixel clock
>   */
>  struct dp_power {

This chunk is unrelated - it just added some missing doc.
Do it belong in another patch?

> diff --git a/drivers/gpu/drm/msm/dp/pll/dp_pll.c b/drivers/gpu/drm/msm/dp/pll/dp_pll.c
> new file mode 100644
> index 0000000..28f0e92
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/dp/pll/dp_pll.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
> + */
> +
> +#include "dp_pll.h"
> +
> +int msm_dp_pll_util_parse_dt_clock(struct platform_device *pdev,
> +					struct msm_dp_pll *pll)
> +{
> +	u32 i = 0, rc = 0;
> +	struct dss_module_power *mp = &pll->mp;
> +	const char *clock_name;
> +	u32 clock_rate;
> +
> +	mp->num_clk = of_property_count_strings(pdev->dev.of_node,
> +							"clock-names");
> +	if (mp->num_clk <= 0) {
> +		DRM_ERROR("clocks are not defined\n");
> +		goto clk_err;
> +	}
You have a pdev->dev, so use DRM_DEV_ERROR()

> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
> +			enum msm_dp_pll_type type, int id)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct msm_dp_pll *pll;
> +
> +	switch (type) {
> +	case MSM_DP_PLL_10NM:
> +		pll = msm_dp_pll_10nm_init(pdev, id);
> +		break;
> +	default:
> +		pll = ERR_PTR(-ENXIO);
> +		break;
> +	}
> +
> +	if (IS_ERR(pll)) {
> +		DRM_DEV_ERROR(dev, "%s: failed to init DP PLL\n", __func__);
> +		return pll;
> +	}
> +
> +	pll->type = type;
> +
> +	DBG("DP:%d PLL registered", id);
Avoid rolling your own DEBUG macros.

> +}
> +
> +static const struct of_device_id dp_pll_dt_match[] = {
> +#ifdef CONFIG_DRM_MSM_DP_10NM_PLL
> +	{ .compatible = "qcom,dp-pll-10nm" },
> +#endif
> +	{}
> +};
We only have one entry here.

> +
> +static int dp_pll_driver_probe(struct platform_device *pdev)
> +{
> +	struct msm_dp_pll *pll;
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *match;
> +	enum msm_dp_pll_type type;
> +
> +	match = of_match_node(dp_pll_dt_match, dev->of_node);
> +	if (!match)
> +		return -ENODEV;
> +
> +	if (!strcmp(match->compatible, "qcom,dp-pll-10nm"))
> +		type = MSM_DP_PLL_10NM;
> +	else
> +		type = MSM_DP_PLL_MAX;
So the if will always be true, because we can only match one compatible - no?


> +
> +	pll = msm_dp_pll_init(pdev, type, 0);
> +	if (IS_ERR_OR_NULL(pll)) {
> +		dev_info(dev,
> +			"%s: pll init failed: %ld, need separate pll clk driver\n",
> +			__func__, PTR_ERR(pll));
dev_info => DRM_DEV_ERROR


> +static int dp_pll_driver_remove(struct platform_device *pdev)
> +{
> +	struct msm_dp_pll *pll = platform_get_drvdata(pdev);
> +
> +	if (pll) {
> +		//msm_dsi_pll_destroy(pll);
Debug artifact?


> +#define PLL_REG_W(base, offset, data)	\
> +				writel_relaxed((data), (base) + (offset))
> +#define PLL_REG_R(base, offset)	readl_relaxed((base) + (offset))
I recall you wrote in the commit message that the _relaxed
variants was dropped?

> +
> +struct msm_dp_pll *msm_dp_pll_init(struct platform_device *pdev,
> +			enum msm_dp_pll_type type, int id);
> +
> +int msm_dp_pll_util_parse_dt_clock(struct platform_device *pdev,
> +					struct msm_dp_pll *pll);
Indent of sencond line with additional function arguments
has inconsistent indent.
Follwo same style in all files, preferable the DRM style.

> +
> +#ifdef CONFIG_DRM_MSM_DP_10NM_PLL
> +struct msm_dp_pll *msm_dp_pll_10nm_init(struct platform_device *pdev, int id);
> +#else
> +struct msm_dp_pll *msm_dp_pll_10nm_init(struct platform_device *pdev, int id)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif
I cannot see how this works if CONFIG_DRM_MSM_DP_10NM_PLL is not defined.
It looks like you have both a function in a header (no static inline?)
and a function with the same name in a .c file.
Maybe this driver was not built without the CONFIG_DRM_MSM_DP_10NM_PLL set to y?

> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + *		+------------------------------+
> + *		|         DP_VCO_CLK           |
> + *		|                              |
> + *		|    +-------------------+     |
> + *		|    |   (DP PLL/VCO)    |     |
> + *		|    +---------+---------+     |
> + *		|              v               |
> + *		|   +----------+-----------+   |
> + *		|   | hsclk_divsel_clk_src |   |
> + *		|   +----------+-----------+   |
> + *		+------------------------------+
> + *				|
> + *	 +------------<---------v------------>----------+
> + *	 |                                              |
> + * +-----v------------+                                 |
> + * | dp_link_clk_src  |                                 |
> + * |    divsel_ten    |                                 |
> + * +---------+--------+                                 |
> + *	|                                               |
> + *	|                                               |
> + *	v                                               v
> + * Input to DISPCC block                                |
> + * for link clk, crypto clk                             |
> + * and interface clock                                  |
> + *							|
> + *							|
> + *	+--------<------------+-----------------+---<---+
> + *	|                     |                 |
> + * +-------v------+  +--------v-----+  +--------v------+
> + * | vco_divided  |  | vco_divided  |  | vco_divided   |
> + * |    _clk_src  |  |    _clk_src  |  |    _clk_src   |
> + * |              |  |              |  |               |
> + * |divsel_six    |  |  divsel_two  |  |  divsel_four  |
> + * +-------+------+  +-----+--------+  +--------+------+
> + *         |	           |		        |
> + *	v------->----------v-------------<------v
> + *                         |
> + *		+----------+---------+
> + *		|   vco_divided_clk  |
> + *		|       _src_mux     |
> + *		+---------+----------+
> + *                        |
> + *                        v
> + *              Input to DISPCC block
> + *              for DP pixel clock
Nice drawing!
Try to avoid mixing space and tabs in the above.


> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/clk.h>
Please sort in alphabetic order.

> +/* Op structures */
Op => Ops?

> +}
> +
> +static int clk_mux_determine_rate(struct clk_hw *hw,
> +				     struct clk_rate_request *req)
> +{
> +	int ret = 0;
> +
> +	ret = __clk_mux_determine_rate_closest(hw, req);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the new parent of mux if there is a new valid parent */
> +	if (hw->clk && req->best_parent_hw->clk)
> +		clk_set_parent(hw->clk, req->best_parent_hw->clk);
Should you check error code here?

> +
> +	return 0;
> +}
> +
> +}
> +
> +static int dp_pll_10nm_register(struct dp_pll_10nm *pll_10nm)
> +{
> +	char clk_name[32], parent[32], vco_name[32];
> +	struct clk_init_data vco_init = {
> +		.parent_names = (const char *[]){ "bi_tcxo" },
> +		.num_parents = 1,
> +		.name = vco_name,
> +		.flags = CLK_IGNORE_UNUSED,
> +		.ops = &dp_10nm_vco_clk_ops,
> +	};
> +	struct device *dev = &pll_10nm->pdev->dev;
> +	struct clk_hw **hws = pll_10nm->hws;
> +	struct clk_hw_onecell_data *hw_data;
> +	struct clk_hw *hw;
> +	int num = 0;
> +	int ret;
> +
> +	DBG("DP->id = %d", pll_10nm->id);
Avoid own DBG macros.

> +
> +	DBG("DP PLL%d", id);
Again.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel