On Mon, Dec 8, 2014 at 8:28 AM, Thierry Reding thierry.reding@gmail.com wrote:
On Fri, Dec 05, 2014 at 04:30:00PM -0500, Hai Li wrote: [...]
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.
no, I don't.. and aligned params is kinda annoying if function name or return type changes make it get out of alignment ;-)
but either way is fine
[snip]
+/* 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()?
It may have been a quirk of the downstream 3.10 kernel that I also have to deal with for some devices, but I couldn't get platform_get_irq() to work and so used irq_of_parse_and_map() in the hdmi code. I assume eDP would have the identical problem.
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.
#define is at least consistent w/ rest of drm/msm code (and obj_to_xyz() in drm core).. although if misused it probably gives a more confusing compile error compared to static inline fxn.
I wouldn't reject a patch that converted them all to static inline fxns, but I think this is ok as-is
[snip]
+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.
jfyi, I already have a patch that rips this back out and uses core functions, once drm_dp_link_power_down() is merged ;-)
[snip]
+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.
yes, it is generated..
fyi: https://github.com/freedreno/envytools/tree/master/rnndb
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?
fwiw, the existing code looks to be enough for fixed eDP panels, but doesn't really handle unplug (so some of the stale-edid issues wouldn't crop up yet)..
I am ok will adding full blown DP hot(un)plug as a later patch, but there are going to be a couple spots where we need to be careful about stale EDID, etc..
BR, -R