Re: [PATCH 1/2] drm/msm: Initial add eDP support in msm drm driver (V2)

On Fri, Dec 05, 2014 at 04:30:00PM -0500, Hai Li wrote: [...]

diff --git a/drivers/gpu/drm/msm/edp/edp.c b/drivers/gpu/drm/msm/edp/edp.c new file mode 100644 index 0000000..32e21e1 --- /dev/null +++ b/drivers/gpu/drm/msm/edp/edp.c @@ -0,0 +1,211 @@ +/*

• • Copyright (c) 2014, 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/of_irq.h> +#include "edp.h"

+static irqreturn_t edp_irq(int irq, void *dev_id) +{

• struct msm_edp *edp = dev_id;
• /* Process eDP irq */
• return msm_edp_ctrl_irq(edp->ctrl);

+}

I find that the architecture of this makes it really difficult to review. If I want to see what this function does I now have to jump somewhere else in this patch (over 2000 lines ahead).

+static void edp_destroy(struct platform_device *pdev) +{

• struct msm_edp *edp = platform_get_drvdata(pdev);
• if (!edp)

• return;
  

• if (edp->ctrl) {

• msm_edp_ctrl_destroy(edp->ctrl);
  

• edp->ctrl = NULL;
  

• }
• platform_set_drvdata(pdev, NULL);
• devm_kfree(&pdev->dev, edp);

The whole point of the devm_* functions is that you don't have to clean them up manually. Why do you need to call this here?

+}

+/* construct hdmi at bind/probe time, grab all the resources. */ +static struct msm_edp *edp_init(struct platform_device *pdev) +{

• struct msm_edp *edp = NULL;
• int ret;
• if (!pdev) {

• pr_err("no edp device\n");
  

s/edp/eDP/ here and in a few other places that I haven't pointed out explicitly.

• ret = -ENXIO;
  

• goto fail;
  

• }
• edp = devm_kzalloc(&pdev->dev, sizeof(*edp), GFP_KERNEL);
• if (!edp) {

• ret = -ENOMEM;
  

• goto fail;
  

• }
• DBG("edp probed=%p", edp);
• edp->pdev = pdev;
• platform_set_drvdata(pdev, edp);
• ret = msm_edp_ctrl_init(edp);
• if (ret)

• goto fail;
  

• return edp;

+fail:

• if (edp)

• edp_destroy(pdev);
  

• return ERR_PTR(ret);

+}

+static int edp_bind(struct device *dev, struct device *master, void *data) +{

• struct drm_device *drm = dev_get_drvdata(master);
• struct msm_drm_private *priv = drm->dev_private;
• struct msm_edp *edp;
• DBG("");
• edp = edp_init(to_platform_device(dev));

There's a lot of this casting to platform devices and then using pdev->dev to get at the struct device. I don't immediately see a use for the platform device, so why not just stick with struct device * consistently?

• if (IS_ERR(edp))

• return PTR_ERR(edp);
  

• priv->edp = edp;
• return 0;

+}

+static void edp_unbind(struct device *dev, struct device *master,

• void *data)
  

We typically align parameters on subsequent lines with the first parameter on the first line. But perhaps Rob doesn't care so much.

+static const struct of_device_id dt_match[] = {

• { .compatible = "qcom,mdss-edp" },
• {}

+};

Don't you want a MODULE_DEVICE_TABLE here?

+/* Second part of initialization, the drm/kms level modeset_init */ +int msm_edp_modeset_init(struct msm_edp *edp,

• struct drm_device *dev, struct drm_encoder *encoder)
  

+{

• struct platform_device *pdev = edp->pdev;
• struct msm_drm_private *priv = dev->dev_private;
• int ret;
• edp->encoder = encoder;
• edp->dev = dev;
• edp->bridge = msm_edp_bridge_init(edp);
• if (IS_ERR(edp->bridge)) {

• ret = PTR_ERR(edp->bridge);
  

• dev_err(dev->dev, "failed to create eDP bridge: %d\n", ret);
  

• edp->bridge = NULL;
  

• goto fail;
  

• }
• edp->connector = msm_edp_connector_init(edp);
• if (IS_ERR(edp->connector)) {

• ret = PTR_ERR(edp->connector);
  

• dev_err(dev->dev, "failed to create eDP connector: %d\n", ret);
  

• edp->connector = NULL;
  

• goto fail;
  

• }
• edp->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);

Why not use the more idiomatic platform_get_irq()?

• if (edp->irq < 0) {

• ret = edp->irq;
  

• dev_err(dev->dev, "failed to get irq: %d\n", ret);
  

s/irq/IRQ/

diff --git a/drivers/gpu/drm/msm/edp/edp.h b/drivers/gpu/drm/msm/edp/edp.h

[...]

+#ifndef __EDP_CONNECTOR_H__ +#define __EDP_CONNECTOR_H__

+#include <linux/clk.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/of_gpio.h> +#include <linux/pwm.h> +#include <linux/i2c.h>

These should be alphabetically sorted.

diff --git a/drivers/gpu/drm/msm/edp/edp_aux.c b/drivers/gpu/drm/msm/edp/edp_aux.c

[...]

+#define to_edp_aux(x) container_of(x, struct edp_aux, drm_aux)

Perhaps this should be a static inline function for better type safety.

+static int edp_msg_fifo_tx(struct edp_aux *aux, struct drm_dp_aux_msg *msg) +{

• u32 data[4];
• u32 reg, len;
• bool native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
• bool read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
• u8 *msgdata = msg->buffer;
• int i;
• if (read)

• len = 4;
  

• else

• len = msg->size + 4;
  

• /*

• * cmd fifo only has depth of 144 bytes
  

• */
  

• if (len > AUX_CMD_FIFO_LEN)

• return -EINVAL;
  

• /* Pack cmd and write to HW */
• data[0] = (msg->address >> 16) & 0xf; /* addr[19:16] */
• if (read)

• data[0] |=  BIT(4);		/* R/W */
  

• data[1] = (msg->address >> 8) & 0xff; /* addr[15:8] */
• data[2] = msg->address & 0xff; /* addr[7:0] */
• data[3] = (msg->size - 1) & 0xff; /* len[7:0] */
• for (i = 0; i < len; i++) {

• reg = (i < 4) ? data[i] : msgdata[i - 4];
  

• reg = EDP_AUX_DATA_DATA(reg); /* index = 0, write */
  

• if (i == 0)
  

• 	reg |= EDP_AUX_DATA_INDEX_WRITE;
  

• edp_write(aux->base + REG_EDP_AUX_DATA, reg);
  

• /* Write data 1 by 1 into the FIFO */
  

• wmb();
  

I don't understand why you think you need these. You already use the right accessors and they already provide correct barriers. Are you really sure you need them?

• }
• reg = 0; /* Transaction number is always 1 */
• if (!native) /* i2c */

• reg |= EDP_AUX_TRANS_CTRL_I2C;
  

• reg |= EDP_AUX_TRANS_CTRL_GO;
• edp_write(aux->base + REG_EDP_AUX_TRANS_CTRL, reg);
• /* Make sure transaction is triggered */
• wmb();

Same here... and in various other places.

+/*

• • This function does the real job to process an aux transaction.

s/aux/AUX/

• • It will call msm_edp_aux_ctrl() function to reset the aux channel,
• • if the waiting is timeout.
• • The caller who triggers the transaction should avoid the
• • msm_edp_aux_ctrl() running concurrently in other threads, i.e.
• • start transaction only when aux channel is fully enabled.
• */

+ssize_t edp_aux_transfer(struct drm_dp_aux *drm_aux, struct drm_dp_aux_msg *msg) +{

• struct edp_aux *aux = to_edp_aux(drm_aux);
• ssize_t ret;
• bool native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
• bool read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);

These checks are confusing. It seems like they might actually work because of how these symbols are defined, but I'd expect something like:

native = msg->request & (DP_AUX_NATIVE_WRITE | DP_AUX_NATIVE_READ);

Or perhaps even clearer:

switch (msg->request) { case DP_AUX_NATIVE_WRITE: case DP_AUX_NATIVE_READ: native = true; break;

... }

• /* Ignore address only message */
• if ((msg->size == 0) || (msg->buffer == NULL)) {

• msg->reply = native ?
  

• 	DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
  

• return msg->size;
  

• }

How do you support I2C-over-AUX then? How else will the device know which I2C slave to address?

diff --git a/drivers/gpu/drm/msm/edp/edp_bridge.c b/drivers/gpu/drm/msm/edp/edp_bridge.c

[...]

+static const struct drm_bridge_funcs edp_bridge_funcs = {

• .pre_enable = edp_bridge_pre_enable,
  

• .enable = edp_bridge_enable,
  

• .disable = edp_bridge_disable,
  

• .post_disable = edp_bridge_post_disable,
  

• .mode_set = edp_bridge_mode_set,
  

• .destroy = edp_bridge_destroy,
  

• .mode_fixup = edp_bridge_mode_fixup,
  

These should be indented using a single tab.

diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c

[...]

+static int edp_connector_get_modes(struct drm_connector *connector) +{

• struct edp_connector *edp_connector = to_edp_connector(connector);
• struct msm_edp *edp = edp_connector->edp;
• struct edid *drm_edid = NULL;
• int ret = 0;
• DBG("");
• ret = msm_edp_ctrl_get_edid(edp->ctrl, connector, &drm_edid);
• if (ret)

• return ret;
  

• if (drm_edid) {

• drm_mode_connector_update_edid_property(connector, drm_edid);
  

I think you want to call this unconditionally to make sure the EDID property is cleared if you couldn't get a new one. Otherwise you'll end up with a stale EDID in sysfs.

• ret = drm_add_edid_modes(connector, drm_edid);
  

• }
• return ret;

+}

[...]

+static const struct drm_connector_funcs edp_connector_funcs = {

• .dpms = drm_helper_connector_dpms,
• .detect = edp_connector_detect,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = edp_connector_destroy,

+};

+static const struct drm_connector_helper_funcs edp_connector_helper_funcs = {

• .get_modes = edp_connector_get_modes,
• .mode_valid = edp_connector_mode_valid,
• .best_encoder = edp_connector_best_encoder,

+};

This is missing mandatory callbacks for atomic modesetting, isn't this going to simply crash when applied on top of a recent kernel with atomic modesetting support?

+/* initialize connector */ +struct drm_connector *msm_edp_connector_init(struct msm_edp *edp) +{

• struct drm_connector *connector = NULL;
• struct edp_connector *edp_connector;
• int ret;
• edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);
• if (!edp_connector) {

• ret = -ENOMEM;
  

• goto fail;
  

• }
• edp_connector->edp = edp;
• connector = &edp_connector->base;
• ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs,

• 	DRM_MODE_CONNECTOR_eDP);
  

• if (ret)

• goto fail;
  

• drm_connector_helper_add(connector, &edp_connector_helper_funcs);
• /* We don't support HPD, so only poll status until connected. */
• connector->polled = DRM_CONNECTOR_POLL_CONNECT;
• /* Display driver doesn't support interlace now. */
• connector->interlace_allowed = 0;
• connector->doublescan_allowed = 0;

These are boolean, so their value should be false rather than 0.

diff --git a/drivers/gpu/drm/msm/edp/edp_ctrl.c b/drivers/gpu/drm/msm/edp/edp_ctrl.c

[...]

+#include <linux/kernel.h> +#include <linux/gpio.h> +#include <linux/leds.h> +#include "edp.h" +#include "edp.xml.h" +#include "drm_dp_helper.h" +#include "drm_edid.h" +#include "drm_crtc.h"

I think a more natural ordering would be linux/*, drm_*, edp.*, because that's most generic to most specific.

+#define RGB_COMPONENTS 3

In my opinion this is overkill. Just use a literal 3 in the two places where this is actually used. The context is enough to know what this is for.

+static int cont_splash; /* 1 to enable continuous splash screen */ +EXPORT_SYMBOL(cont_splash);

+module_param(cont_splash, int, 0); +MODULE_PARM_DESC(cont_splash, "Enable continuous splash screen on eDP");

Huh? Is this supposed to allow hand-off from firmware to kernel? If so I don't think that's going to work without having proper support for it across the driver. I don't see support for this in the MDP subdriver, so I doubt that it's going to work at all.

Either way, I don't think using a module parameter for this is the right solution.

+struct edp_ctrl {

• struct platform_device *pdev;
• void __iomem *base;
• /* regulators */
• struct regulator *vdda_vreg;
• struct regulator *lvl_reg;
• /* clocks */
• struct clk *aux_clk;
• struct clk *pixel_clk;
• struct clk *ahb_clk;
• struct clk *link_clk;
• struct clk *mdp_core_clk;
• /* gpios */
• int gpio_panel_en;
• int gpio_panel_hpd;
• int gpio_lvl_en;
• int gpio_bkl_en;

These should really be using the new gpiod_*() API. Also, at least panel_en and bkl_en seem wrongly placed. They should be handled in the panel and backlight drivers, not the eDP driver.

Also it seems like gpio_lvl_en and lvl_reg are two different ways of representing the same thing. You should use the regulator only and if it's a simple GPIO use the fixed regulator with a gpio property.

• /* backlight */
• struct pwm_device *bl_pwm;
• u32 pwm_period;
• u32 bl_level;

+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE

• struct backlight_device *backlight_dev;

+#endif

This looks very much like a duplicate of pwm-backlight. Any reason why you can't use it?

+struct edp_pixel_clk_div {

• u32 rate; /* in kHz */
• u32 m;
• u32 n;

+};

+#define EDP_PIXEL_CLK_NUM 8 +static const struct edp_pixel_clk_div clk_divs[2][EDP_PIXEL_CLK_NUM] = {

• { /* Link clock = 162MHz, source clock = 810MHz */

• {119000, 31,  211}, /* WSXGA+ 1680x1050@60Hz CVT */
  

• {130250, 32,  199}, /* UXGA 1600x1200@60Hz CVT */
  

• {148500, 11,  60},  /* FHD 1920x1080@60Hz */
  

• {154000, 50,  263}, /* WUXGA 1920x1200@60Hz CVT */
  

• {209250, 31,  120}, /* QXGA 2048x1536@60Hz CVT */
  

• {268500, 119, 359}, /* WQXGA 2560x1600@60Hz CVT */
  

• {138530, 33,  193}, /* AUO B116HAN03.0 Panel */
  

• {141400, 48,  275}, /* AUO B133HTN01.2 Panel */
  

• },
• { /* Link clock = 270MHz, source clock = 675MHz */

• {119000, 52,  295}, /* WSXGA+ 1680x1050@60Hz CVT */
  

• {130250, 11,  57},  /* UXGA 1600x1200@60Hz CVT */
  

• {148500, 11,  50},  /* FHD 1920x1080@60Hz */
  

• {154000, 47,  206}, /* WUXGA 1920x1200@60Hz CVT */
  

• {209250, 31,  100}, /* QXGA 2048x1536@60Hz CVT */
  

• {268500, 107, 269}, /* WQXGA 2560x1600@60Hz CVT */
  

• {138530, 63,  307}, /* AUO B116HAN03.0 Panel */
  

• {141400, 53,  253}, /* AUO B133HTN01.2 Panel */
  

• },

+};

Can't you compute these programmatically? If you rely on this table you'll need to extend it everytime you want to support a new panel or resolution.

+static void edp_clk_deinit(struct edp_ctrl *ctrl) +{

• struct device *dev = &ctrl->pdev->dev;
• if (ctrl->aux_clk)

• devm_clk_put(dev, ctrl->aux_clk);
  

• if (ctrl->pixel_clk)

• devm_clk_put(dev, ctrl->pixel_clk);
  

• if (ctrl->ahb_clk)

• devm_clk_put(dev, ctrl->ahb_clk);
  

• if (ctrl->link_clk)

• devm_clk_put(dev, ctrl->link_clk);
  

• if (ctrl->mdp_core_clk)

• devm_clk_put(dev, ctrl->mdp_core_clk);
  

+}

What's the point of using devm_* if you do manual cleanup anyway?

• ctrl->mdp_core_clk = devm_clk_get(dev, "mdp_core_clk");
• if (IS_ERR(ctrl->mdp_core_clk)) {

• pr_err("%s: Can't find mdp_core_clk", __func__);
  

• ctrl->mdp_core_clk = NULL;
  

• goto edp_clk_err;
  

• }
• return 0;

+edp_clk_err:

• edp_clk_deinit(ctrl);
• return -EPERM;

You should really propagate a proper error code here.

+static int edp_regulator_init(struct edp_ctrl *ctrl) +{

• struct device *dev = &ctrl->pdev->dev;
• int ret;
• DBG("");
• ctrl->vdda_vreg = devm_regulator_get(dev, "vdda");
• if (IS_ERR(ctrl->vdda_vreg)) {

• pr_err("%s: Could not get vdda reg, ret = %ld\n", __func__,
  

• 		PTR_ERR(ctrl->vdda_vreg));
  

• ctrl->vdda_vreg = NULL;
  

• ret = -ENODEV;
  

• goto f0;
  

• }
• ret = regulator_set_voltage(ctrl->vdda_vreg, VDDA_MIN_UV, VDDA_MAX_UV);
• if (ret) {

• pr_err("%s:vdda_vreg set_voltage failed, %d\n", __func__, ret);
  

• goto f1;
  

• }
• ret = regulator_set_optimum_mode(ctrl->vdda_vreg, VDDA_UA_ON_LOAD);
• if (ret < 0) {

• pr_err("%s: vdda_vreg set regulator mode failed.\n", __func__);
  

• goto f1;
  

• }
• ret = regulator_enable(ctrl->vdda_vreg);
• if (ret) {

• pr_err("%s: Failed to enable vdda_vreg regulator.\n", __func__);
  

• goto f2;
  

• }
• DBG("exit");
• return 0;

+f2:

• regulator_set_optimum_mode(ctrl->vdda_vreg, VDDA_UA_OFF_LOAD);

+f1:

• devm_regulator_put(ctrl->vdda_vreg);
• ctrl->vdda_vreg = NULL;

+f0:

• return ret;

The label names could be improved here.

+/* The power source of the level translation chip is different on different

• • target boards, i.e. a gpio or a regulator.
• */

Like I said above you should simply always make this a regulator and use a fixed GPIO regulator if it's controlled by a GPIO.

+static int edp_sink_power_state(struct edp_ctrl *ctrl, u8 state) +{

• u8 s = state;
• DBG("%d", s);
• if (ctrl->dp_link.revision < 0x11)

• return 0;
  

• if (drm_dp_dpcd_write(ctrl->drm_aux, DP_SET_POWER, &s, 1) < 1) {

• pr_err("%s: Set power state to panel failed\n", __func__);
  

• return -ENOLINK;
  

• }
• return 0;

+}

This is essentially drm_dp_link_power_up()/drm_dp_link_power_down(). Please use common code where available. And if it's not available yet the code is completely generic, please add a core function so that other drivers can reuse it.

+static int edp_train_pattern_set_write(struct edp_ctrl *ctrl, u8 pattern) +{

• u8 p = pattern;
• DBG("pattern=%x", p);
• if (drm_dp_dpcd_write(ctrl->drm_aux, 0x102, &p, 1) < 1) {

0x102 is DP_TRAINING_PATTERN_SET.

+static void edp_host_train_set(struct edp_ctrl *ctrl, u32 train) +{

• int cnt;
• u32 data;
• u32 bit;
• bit = 1;
• bit <<= (train - 1);
• DBG("%s: bit=%d train=%d", __func__, bit, train);
• edp_state_ctrl(ctrl, bit);
• bit = 8;
• bit <<= (train - 1);
• cnt = 10;

Maybe do that as part of the declaration?

• while (cnt--) {

• data = edp_read(ctrl->base + REG_EDP_MAINLINK_READY);
  

• if (data & bit)
  

• 	break;
  

• }
• if (cnt == 0)

• pr_err("%s: set link_train=%d failed\n", __func__, train);
  

I don't think this works as was intended. while (cnt--) will still execute the loop once because the post-fix operator is applied after the variable is evaluated. That is, after the loop terminates, cnt will really be -1, so the error won't be printed. It will only be printed if the loop happens to terminate on the penultimate iteration.

• int tries;
• int ret = 0;
• int rlen;
• DBG("");
• edp_host_train_set(ctrl, 0x02); /* train_2 */

Perhaps use the DP_TRAINING_PATTERN_* symbolic names and avoid the comment.

+static int edp_ctrl_training(struct edp_ctrl *ctrl) +{

• int ret;
• /* Do link training only when power is on */
• if (ctrl->cont_splash || (!ctrl->power_on))

No need for the parentheses around !ctrl->power_on.

+static void edp_ctrl_on_worker(struct work_struct *work) +{

[...]

+}

+static void edp_ctrl_off_worker(struct work_struct *work) +{

• struct edp_ctrl *ctrl = container_of(

• 		work, struct edp_ctrl, off_work);
  

• int ret = 0;

No need to initialize this.

[...]

+}

Why are these two functions workers?

+irqreturn_t msm_edp_ctrl_irq(struct edp_ctrl *ctrl) +{

[...]

• if (isr1 & EDP_INTERRUPT_REG_1_HPD)

• DBG("edp_hpd");
  

Don't you want to handle this?

• if (isr2 & EDP_INTERRUPT_REG_2_READY_FOR_VIDEO)

• DBG("edp_video_ready");
  

• if (isr2 & EDP_INTERRUPT_REG_2_IDLE_PATTERNs_SENT) {

s/PATTERNs/PATTERNS/? I was going to make that comment to the definition of this define, but I can't seem to find it. I suspect that it comes from one of the generated headers, but I can't seem to find either the generated header nor the XML.

• DBG("idle_patterns_sent");
  

• complete(&ctrl->idle_comp);
  

• }
• msm_edp_aux_irq(ctrl->aux, isr1);
• return IRQ_HANDLED;

+}

[...]

+bool msm_edp_ctrl_panel_connected(struct edp_ctrl *ctrl) +{

• bool ret;

This is unnecessary, the only place where this is used is to return the value of ctrl->edp_connected. You can use that directly instead.

[...]

• ret = ctrl->edp_connected;
• mutex_unlock(&ctrl->dev_mutex);
• return ret;

+}

+int msm_edp_ctrl_get_edid(struct edp_ctrl *ctrl,

• struct drm_connector *connector, struct edid **edid)
  

+{

• int ret = 0;
• mutex_lock(&ctrl->dev_mutex);
• if (ctrl->edid) {

• if (edid) {
  

• 	DBG("Just return edid buffer");
  

• 	*edid = ctrl->edid;
  

• }
  

• goto unlock_ret;
  

• }

Is there a way to invalidate an existing EDID?

• if (!ctrl->power_on) {

• if (!ctrl->cont_splash)
  

• 	edp_ctrl_phy_aux_enable(ctrl, 1);
  

• edp_ctrl_irq_enable(ctrl, 1);
  

• }
• ret = drm_dp_link_probe(ctrl->drm_aux, &ctrl->dp_link);
• if (ret) {

• pr_err("%s: read dpcd cap failed, %d\n", __func__, ret);
  

• goto disable_ret;
  

• }
• /* Initialize link rate as panel max link rate */
• ctrl->link_rate = drm_dp_link_rate_to_bw_code(ctrl->dp_link.rate);

There's a lot of code here that should probably be a separate function rather than be called as part of retrieving the EDID.

+int msm_edp_ctrl_timing_cfg(struct edp_ctrl *ctrl,

• struct drm_display_mode *mode, struct drm_display_info *info)

Can mode and info be const?

+{

• u32 hstart_from_sync, vstart_from_sync;
• u32 data;
• int ret = 0;

[...]

• vstart_from_sync = mode->vtotal - mode->vsync_start;
• hstart_from_sync = (mode->htotal - mode->hsync_start);

No need for the parentheses.

diff --git a/drivers/gpu/drm/msm/edp/edp_phy.c b/drivers/gpu/drm/msm/edp/edp_phy.c

[...]

+bool msm_edp_phy_ready(struct edp_phy *phy) +{

• u32 status;
• int cnt;
• cnt = 100;
• while (--cnt) {

• status = edp_read(phy->base +
  

• 		REG_EDP_PHY_GLB_PHY_STATUS);
  

• if (status & 0x01)
  

Can you add a define for 0x01?

• 	break;
  

• usleep_range(500, 1000);
  

• }
• if (cnt <= 0) {

This is a better version than above, except that cnt can never be negative. It will be zero upon timeout.

• pr_err("%s: PHY NOT ready\n", __func__);
  

• return false;
  

• } else {

• return true;
  

• }

+}

+void msm_edp_phy_ctrl(struct edp_phy *phy, int enable) +{

• DBG("enable=%d", enable);
• if (enable) {

• /* Reset */
  

• edp_write(phy->base + REG_EDP_PHY_CTRL, 0x005); /* bit 0, 2 */
  

• wmb();
  

• usleep_range(500, 1000);
  

• edp_write(phy->base + REG_EDP_PHY_CTRL, 0x000);
  

• edp_write(phy->base + REG_EDP_PHY_GLB_PD_CTL, 0x3f);
  

• edp_write(phy->base + REG_EDP_PHY_GLB_CFG, 0x1);
  

• } else {

• edp_write(phy->base + REG_EDP_PHY_GLB_PD_CTL, 0xc0);
  

• }

Please, also add defines for the values here. It's impossible to tell from the code what this does or what might need fixing if there was a bug.

+void msm_edp_phy_lane_power_ctrl(struct edp_phy *phy, int up, int max_lane)

bool for up? And unsigned int for max_lane?

+{

• int i;

This could also be unsigned int.

Thierry