Message-ID: <20141208132827.GA13942@ulmo.nvidia.com>
Date:	Mon, 8 Dec 2014 14:28:29 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Hai Li <hali@...eaurora.org>
Cc:	dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org, robdclark@...il.com
Subject: 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@...z CVT */
> +		{130250, 32,  199}, /* UXGA 1600x1200@...z CVT */
> +		{148500, 11,  60},  /* FHD 1920x1080@...z */
> +		{154000, 50,  263}, /* WUXGA 1920x1200@...z CVT */
> +		{209250, 31,  120}, /* QXGA 2048x1536@...z CVT */
> +		{268500, 119, 359}, /* WQXGA 2560x1600@...z 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@...z CVT */
> +		{130250, 11,  57},  /* UXGA 1600x1200@...z CVT */
> +		{148500, 11,  50},  /* FHD 1920x1080@...z */
> +		{154000, 47,  206}, /* WUXGA 1920x1200@...z CVT */
> +		{209250, 31,  100}, /* QXGA 2048x1536@...z CVT */
> +		{268500, 107, 269}, /* WQXGA 2560x1600@...z 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

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists