drm/msm: Initial add eDP support in msm drm driver (V2)
PATCH 1/2 drm/msm: Initial add eDP support in msm drm driver (V2)
This change adds a new eDP connector in msm drm driver. With this change, eDP panel can work with msm platform under drm framework.
--- drivers/gpu/drm/msm/Makefile | 6 + drivers/gpu/drm/msm/edp/edp.c | 211 ++++ drivers/gpu/drm/msm/edp/edp.h | 86 ++ drivers/gpu/drm/msm/edp/edp_aux.c | 297 +++++ drivers/gpu/drm/msm/edp/edp_bridge.c | 206 ++++ drivers/gpu/drm/msm/edp/edp_connector.c | 159 +++ drivers/gpu/drm/msm/edp/edp_ctrl.c | 1810 +++++++++++++++++++++++++++++++ drivers/gpu/drm/msm/edp/edp_phy.c | 110 ++ drivers/gpu/drm/msm/msm_drv.h | 6 + 9 files changed, 2891 insertions(+) create mode 100644 drivers/gpu/drm/msm/edp/edp.c create mode 100644 drivers/gpu/drm/msm/edp/edp.h create mode 100644 drivers/gpu/drm/msm/edp/edp_aux.c create mode 100644 drivers/gpu/drm/msm/edp/edp_bridge.c create mode 100644 drivers/gpu/drm/msm/edp/edp_connector.c create mode 100644 drivers/gpu/drm/msm/edp/edp_ctrl.c create mode 100644 drivers/gpu/drm/msm/edp/edp_phy.c
drivers/gpu/drm/msm/Makefile +6 -0
@@ -16,6 +16,12 @@ msm-y := \ hdmi/hdmi_phy_8960.o \ hdmi/hdmi_phy_8x60.o \ hdmi/hdmi_phy_8x74.o \ + edp/edp.o \ + edp/edp_aux.o \ + edp/edp_bridge.o \ + edp/edp_connector.o \ + edp/edp_ctrl.o \ + edp/edp_phy.o \ mdp/mdp_format.o \ mdp/mdp_kms.o \ mdp/mdp4/mdp4_crtc.o \
drivers/gpu/drm/msm/edp/edp.c new file +211 -0
211 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp.h new file +86 -0
86 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp_aux.c new file +297 -0
297 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp_bridge.c new file +206 -0
206 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp_connector.c new file +159 -0
159 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +1810 -0
1,810 lines not on this page · raw message
drivers/gpu/drm/msm/edp/edp_phy.c new file +110 -0
110 lines not on this page · raw message
drivers/gpu/drm/msm/msm_drv.h +6 -0
@@ -76,6 +76,12 @@ struct msm_drm_private {*/ struct hdmi *hdmi; +/* eDP is for mdp5 only, but kms has not been created +* when edp_bind() and edp_init() are called. Here is the only +* place to keep the edp instance. +*/ +struct msm_edp *edp; +/* when we have more than one 'msm_gpu' these need to be an array: */ struct msm_gpu *gpu;struct msm_file_private *lastctx;
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
PATCH 2/2 drm/msm: Add the eDP connector in msm drm driver
Modified the hard-coded hdmi connector/encoder implementations in msm drm driver to support both edp and hdmi.
--- drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c | 38 +++++++++++++++++++++-- drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 47 ++++++++++++++++++++--------- drivers/gpu/drm/msm/msm_drv.c | 2 ++ drivers/gpu/drm/msm/msm_drv.h | 7 +++++ 4 files changed, 77 insertions(+), 17 deletions(-)
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +36 -2
@@ -1,4 +1,5 @@/* +* Copyright (c) 2014, The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark <[email protected]> *
@@ -151,11 +152,13 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder, {struct mdp5_encoder *mdp5_encoder =to_mdp5_encoder (encoder);struct mdp5_kms *mdp5_kms =get_kms (encoder); +struct drm_device *dev = encoder->dev; +struct drm_connector *connector;int intf = mdp5_encoder->intf;uint32_t dtv_hsync_skew, vsync_period, vsync_len, ctrl_pol;uint32_t display_v_start, display_v_end;uint32_t hsync_start_x, hsync_end_x; -uint32_t format; +uint32_t format =0x2100 ;unsigned long flags; mode = adjusted_mode;
@@ -177,7 +180,28 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder,/* probably need to get DATA_EN polarity from panel.. */ dtv_hsync_skew =0 ;/* get this from panel? */ - format =0x213f ;/* get this from panel? */ + +/* Get color format from panel, default is 8bpc */ +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { +if (connector->encoder == encoder) { +switch (connector->display_info.bpc) { +case 4 : + format |=0 ; +break ; +case 5 : + format |=0x15 ; +break ; +case 6 : + format |=0x2A ; +break ; +case 8 : +default : + format |=0x3F ; +break ; + } +break ; + } + } hsync_start_x = (mode->htotal - mode->hsync_start); hsync_end_x = mode->htotal - (mode->hsync_start - mode->hdisplay) -1 ;
@@ -187,6 +211,16 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder, display_v_start = (mode->vtotal - mode->vsync_start) * mode->htotal + dtv_hsync_skew; display_v_end = vsync_period - ((mode->vsync_start - mode->vdisplay) * mode->htotal) + dtv_hsync_skew -1 ; +/* +* For edp only: +* DISPLAY_V_START = (VBP * HCYCLE) + HBP +* DISPLAY_V_END = (VBP + VACTIVE) * HCYCLE - 1 - HFP +*/ +if (mdp5_encoder->intf_id == INTF_eDP) { + display_v_start += (mode->htotal - mode->hsync_start); + display_v_end -= (mode->hsync_start - mode->hdisplay); + } +spin_lock_irqsave (&mdp5_encoder->intf_lock, flags); mdp5_write(mdp5_kms, REG_MDP5_INTF_HSYNC_CTL(intf),
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +32 -15
73 lines not on this page · raw message
drivers/gpu/drm/msm/msm_drv.c +2 -0
@@ -1023,6 +1023,7 @@ static struct platform_driver msm_platform_driver = {static int __init msm_drm_register (void ) {DBG ("init" ); +msm_edp_register ();hdmi_register ();adreno_register ();return platform_driver_register (&msm_platform_driver);
@@ -1034,6 +1035,7 @@ static void __exit msm_drm_unregister(void)platform_driver_unregister (&msm_platform_driver);hdmi_unregister ();adreno_unregister (); +msm_edp_unregister (); }module_init (msm_drm_register);
drivers/gpu/drm/msm/msm_drv.h +7 -0
@@ -224,6 +224,13 @@ int hdmi_modeset_init(struct hdmi *hdmi, struct drm_device *dev,void __init hdmi_register (void ); void __exit hdmi_unregister(void); +struct msm_edp ; +void __init msm_edp_register (void ); +void __exit msm_edp_unregister(void); +int msm_edp_modeset_init (struct msm_edp *edp,struct drm_device *dev, +struct drm_encoder *encoder); + +#ifdef CONFIG_DEBUG_FS void msm_gem_describe (struct drm_gem_object *obj,struct seq_file *m);void msm_gem_describe_objects (struct list_head *list,struct seq_file *m);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
On Fri, Dec 05, 2014 at 04:30:00PM -0500, Hai Li wrote:
[...]
Quoted 19 lines
drivers/gpu/drm/msm/edp/edp.c new file +13
-0@@ -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. +*/ +
drivers/gpu/drm/msm/edp/edp.c line 23
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp.c new file +10
-0+#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).
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp.c new file +5
-0+static void edp_destroy (struct platform_device *pdev) +{ +struct msm_edp *edp =platform_get_drvdata (pdev); + +if (!edp)
drivers/gpu/drm/msm/edp/edp.c line 39
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp.c new file +10
-0+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?
drivers/gpu/drm/msm/edp/edp.c line 49
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp.c new file +10
-0+} + +/* 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.
Quoted 25 lines
drivers/gpu/drm/msm/edp/edp.c new file +25
-0+ 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);
drivers/gpu/drm/msm/edp/edp.c line 84
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp.c new file +10
-0+} + +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?
drivers/gpu/drm/msm/edp/edp.c line 93
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp.c new file +9
-0+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.
drivers/gpu/drm/msm/edp/edp.c line 126
Quoted 4 lines
drivers/gpu/drm/msm/edp/edp.c new file +4
-0+static const struct of_device_id dt_match[] = { + { .compatible ="qcom,mdss-edp" }, + {} +};
Don't you want a MODULE_DEVICE_TABLE here?
Quoted 18 lines
drivers/gpu/drm/msm/edp/edp.c new file +18
-0+/* 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; + }
drivers/gpu/drm/msm/edp/edp.c line 176
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp.c new file +10
-0+ + 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()?
drivers/gpu/drm/msm/edp/edp.c line 179
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp.c new file +3
-0+if (edp->irq <0 ) { + ret = edp->irq; +dev_err (dev->dev,"failed to get irq: %d\n" , ret);
s/irq/IRQ/
Quoted 1 line
drivers/gpu/drm/msm/edp/edp.h +0
-0
[...]
drivers/gpu/drm/msm/edp/edp.h line 22
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp.h +9
-0+#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.
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_aux.c +0
-0
[...]
Quoted 1 line
+#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.
Quoted 28 lines
drivers/gpu/drm/msm/edp/edp_aux.c +28
-0+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] */
drivers/gpu/drm/msm/edp/edp_aux.c line 93
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_aux.c +10
-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?
drivers/gpu/drm/msm/edp/edp_aux.c line 104
Quoted 11 lines
drivers/gpu/drm/msm/edp/edp_aux.c +11
-0+ } + + 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.
Quoted 2 lines
+/*
+ * This function does the real job to process an aux transaction.
s/aux/AUX/
drivers/gpu/drm/msm/edp/edp_aux.c line 149
Quoted 12 lines
+ * 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.
+ */drivers/gpu/drm/msm/edp/edp_aux.c +6
-0+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;
...
}drivers/gpu/drm/msm/edp/edp_aux.c line 156
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_aux.c +6
-0+/* 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?
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_bridge.c +0
-0
[...]
drivers/gpu/drm/msm/edp/edp_bridge.c line 175
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_bridge.c +8
-0+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.
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_connector.c +0
-0
[...]
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_connector.c +5
-0+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; +
drivers/gpu/drm/msm/edp/edp_connector.c line 60
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_connector.c +10
-0+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.
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_connector.c +5
-0+ ret =drm_add_edid_modes (connector, drm_edid); + } + +return ret; +}
[...]
drivers/gpu/drm/msm/edp/edp_connector.c line 115
Quoted 12 lines
drivers/gpu/drm/msm/edp/edp_connector.c +12
-0+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?
Quoted 21 lines
drivers/gpu/drm/msm/edp/edp_connector.c +21
-0+ +/* 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)
drivers/gpu/drm/msm/edp/edp_connector.c line 146
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_connector.c +10
-0+ 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.
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_ctrl.c +0
-0
[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 21
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +8
-0+#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.
Quoted 1 line
+#define
RGB_COMPONENTS3
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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 69
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +5
-0+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.
Quoted 11 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +11
-0+struct edp_ctrl { +struct platform_device *pdev; + +void __iomem *base; + +/* regulators */ +struct regulator *vdda_vreg; +struct regulator *lvl_reg; + +/* clocks */ +struct clk *aux_clk;
drivers/gpu/drm/msm/edp/edp_ctrl.c line 91
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 99
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +8
-0+ +/* 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?
Quoted 19 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +19
-0+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 */
drivers/gpu/drm/msm/edp/edp_ctrl.c line 170
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+ {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.
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +5
-0+static void edp_clk_deinit (struct edp_ctrl *ctrl) +{ +struct device *dev = &ctrl->pdev->dev; + +if (ctrl->aux_clk)
drivers/gpu/drm/msm/edp/edp_ctrl.c line 186
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+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?
drivers/gpu/drm/msm/edp/edp_ctrl.c line 232
Quoted 12 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +12
-0+ 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.
Quoted 33 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +33
-0+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; + } +
drivers/gpu/drm/msm/edp/edp_ctrl.c line 387
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+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.
Quoted 3 lines
+/* 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.
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +6
-0+static int edp_sink_power_state (struct edp_ctrl *ctrl,u8 state) +{ +u8 s = state; + +DBG ("%d" , s); +
drivers/gpu/drm/msm/edp/edp_ctrl.c line 686
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 751
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +6
-0+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.
Quoted 4 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +4
-0+static void edp_host_train_set (struct edp_ctrl *ctrl,u32 train) +{ +int cnt; +u32 data;
drivers/gpu/drm/msm/edp/edp_ctrl.c line 802
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+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?
drivers/gpu/drm/msm/edp/edp_ctrl.c line 810
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +8
-0+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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 914
Quoted 7 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +7
-0+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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1180
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +6
-0+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.
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +2
-0+static void edp_ctrl_on_worker (struct work_struct *work) +{
[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1380
Quoted 7 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +7
-0+} + +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.
[...]
Quoted 1 line
+}
Why are these two functions workers?
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +3
-0+ +irqreturn_t msm_edp_ctrl_irq (struct edp_ctrl *ctrl) +{
[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1449
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +2
-0+if (isr1 &EDP_INTERRUPT_REG_1_HPD ) +DBG ("edp_hpd" );
Don't you want to handle this?
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1454
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +5
-0+ +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.
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +8
-0+DBG ("idle_patterns_sent" ); +complete (&ctrl->idle_comp); + } + +msm_edp_aux_irq (ctrl->aux, isr1); + +return IRQ_HANDLED ; +}
[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1633
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +3
-0+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.
[...]
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+ 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 ;
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1683
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-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?
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +5
-0+ +if (!ctrl->power_on) { +if (!ctrl->cont_splash) +edp_ctrl_phy_aux_enable (ctrl,1 ); +edp_ctrl_irq_enable (ctrl,1 );
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1698
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +10
-0+ } + + 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.
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1721
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +2
-0+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?
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +5
-0+{ +u32 hstart_from_sync, vstart_from_sync; +u32 data; +int ret =0 ; +
[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1752
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c +3
-0+ + vstart_from_sync = mode->vtotal - mode->vsync_start; + hstart_from_sync = (mode->htotal - mode->hsync_start);
No need for the parentheses.
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_phy.c +0
-0
[...]
drivers/gpu/drm/msm/edp/edp_phy.c line 32
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_phy.c +10
-0+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?
drivers/gpu/drm/msm/edp/edp_phy.c line 37
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_phy.c +5
-0+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.
Quoted 11 lines
drivers/gpu/drm/msm/edp/edp_phy.c +11
-0+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) {
drivers/gpu/drm/msm/edp/edp_phy.c line 58
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_phy.c +10
-0+/* 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.
Quoted 1 line
+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?
drivers/gpu/drm/msm/edp/edp_phy.c
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_phy.c +2
-0+{ +int i;
This could also be unsigned int.
Thierry
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/msm: Add the eDP connector in msm drm driver
In reply to Hai LiQuoted 2 lines · Hai Li
Modified the hard-coded hdmi connector/encoder implementations in msm drm driver to support both edp and hdmi.
s/hdmi/HDMI/, s/msm/MSM/, s/drm/DRM/, s/edp/eDP/
[...]
Quoted 1 line
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +0
-0
[...]
Quoted 15 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +10
-1@@ -177,7 +180,28 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder,/* probably need to get DATA_EN polarity from panel.. */ dtv_hsync_skew =0 ;/* get this from panel? */ - format =0x213f ;/* get this from panel? */ + +/* Get color format from panel, default is 8bpc */ +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { +if (connector->encoder == encoder) { +switch (connector->display_info.bpc) { +case 4 : + format |=0 ; +break ; +case 5 : + format |=0x15 ;
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c static void mdp5_encoder_mode_set(struct drm_encoder *encoder, · line 202
Quoted 10 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +10
-0+break ; +case 6 : + format |=0x2A ; +break ; +case 8 : +default : + format |=0x3F ; +break ; + } +break ;
I suppose that it might be obvious from the above switch statement, but having symbolic names for the values of format might still be good for readability.
Quoted 7 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +2
-0+ } + } hsync_start_x = (mode->htotal - mode->hsync_start); hsync_end_x = mode->htotal - (mode->hsync_start - mode->hdisplay) -1 ;@@ -187,6 +211,16 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder, display_v_start = (mode->vtotal - mode->vsync_start) * mode->htotal + dtv_hsync_skew;
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c static void mdp5_encoder_mode_set(struct drm_encoder *encoder, · line 221
Quoted 10 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c +8
-0display_v_end = vsync_period - ((mode->vsync_start - mode->vdisplay) * mode->htotal) + dtv_hsync_skew -1 ; +/* +* For edp only: +* DISPLAY_V_START = (VBP * HCYCLE) + HBP +* DISPLAY_V_END = (VBP + VACTIVE) * HCYCLE - 1 - HFP +*/ +if (mdp5_encoder->intf_id == INTF_eDP) { + display_v_start += (mode->htotal - mode->hsync_start); + display_v_end -= (mode->hsync_start - mode->hdisplay);
No need for the parentheses.
Quoted 1 line
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +0
-0
[...]
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c static int modeset_init(struct mdp5_kms *mdp5_kms) · line 160
Quoted 4 lines
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c +1
-1struct drm_device *dev = mdp5_kms->dev;struct msm_drm_private *priv = dev->dev_private; -struct drm_encoder *encoder; +struct drm_encoder *edp_encoder =NULL , *hdmi_encoder =NULL ;
Can't you simply reuse encoder? It's scope is limited to the conditionals, so no need to have two separate variables.
Thierry
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 15 lines · Thierry Reding
On Fri, Dec 05, 2014 at 04:30:00PM -0500, Hai Li wrote:
[...]drivers/gpu/drm/msm/edp/edp.c line 93
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp.c new file +9
-0+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]
Quoted 22 lines
Quoted 22 lines
drivers/gpu/drm/msm/edp/edp.c new file +22
-0+/* 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);
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp.c line 176
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp.c new file +6
-0+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.
Quoted 20 lines
drivers/gpu/drm/msm/edp/edp.c line 179
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp.c new file +3
-0+if (edp->irq <0 ) { + ret = edp->irq; +dev_err (dev->dev,"failed to get irq: %d\n" , ret);s/irq/IRQ/
Quoted 1 line
drivers/gpu/drm/msm/edp/edp.h +0
-0[...]
drivers/gpu/drm/msm/edp/edp.h line 22
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp.h +9
-0+#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.
Quoted 6 lines
Quoted 1 line
drivers/gpu/drm/msm/edp/edp_aux.c +0
-0[...]
Quoted 1 line
+#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]
Quoted 14 lines
Quoted 14 lines
drivers/gpu/drm/msm/edp/edp_aux.c +13
-0+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 ; + }
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_aux.c
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_aux.c +3
-0+ +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]
Quoted 8 lines
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_aux.c +2
-0+irqreturn_t msm_edp_ctrl_irq (struct edp_ctrl *ctrl) +{[...]
drivers/gpu/drm/msm/edp/edp_aux.c
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_aux.c +2
-0+if (isr1 &EDP_INTERRUPT_REG_1_HPD ) +DBG ("edp_hpd" );Don't you want to handle this?
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_aux.c
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_aux.c +5
-0+ +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
Quoted 32 lines
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp_aux.c +8
-0+DBG ("idle_patterns_sent" ); +complete (&ctrl->idle_comp); + } + +msm_edp_aux_irq (ctrl->aux, isr1); + +return IRQ_HANDLED ; +}[...]
drivers/gpu/drm/msm/edp/edp_aux.c
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_aux.c +3
-0+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.[...]
Quoted 14 lines
drivers/gpu/drm/msm/edp/edp_aux.c +14
-0+ 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) {
Quoted 8 lines
drivers/gpu/drm/msm/edp/edp_aux.c
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_aux.c +6
-0+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
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 16 lines
drivers/gpu/drm/msm/edp/edp.c line 84
Quoted 10 lines
+static int
edp_bind(struct device *dev, struct device *master, void *data)drivers/gpu/drm/msm/edp/edp.c new file +7
-0+{ +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->devto 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?
There are some places in edp_init() to use struct platform_device *. Also, this is to make edp code consistent with hdmi.
Quoted 18 lines
Quoted 13 lines
+ * 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_tedp_aux_transfer(struct drm_dp_aux *drm_aux, struct drm_dp_aux_msg *msg)drivers/gpu/drm/msm/edp/edp_aux.c new file +4
-0+{ +struct edp_aux *aux =to_edp_aux (drm_aux); +ssize_t ret; + bool native = msg->request & (DP_AUX_NATIVE_WRITE &
DP_AUX_NATIVE_READ);Review
Quoted 1 line
(quoted diff) +1
-0+ 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:
Quoted 2 lines
native = msg->request & (DP_AUX_NATIVE_WRITE | DP_AUX_NATIVE_READ);
I think the above solution will not work because it will take DP_AUX_I2C_READ as "native".
Quoted 11 lines
Or perhaps even clearer:
switch (msg->request) { case DP_AUX_NATIVE_WRITE: case DP_AUX_NATIVE_READ: native = true; break; ... }
The switch/case code will work only if we remove other unrelated bits from input msg->request.
Quoted 1 line
From my understanding, the idea to define these symbols is to take BIT(7)
as *native* mark and BIT(0) as *read* mark, and the I2C_WRITE/I2C_READ/NATIVE_WRITE/NATIVE_READ are 4 combinations of these 2 bits. Hence i am still thinking the original way is reflecting the way they are defined.
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_aux.c line 156
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_aux.c new file +6
-0+/* 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?
H/W takes care of the i2c details. S/W only needs to tell H/W if the transaction is i2c or native and the address. Please see edp_msg_fifo_tx().
Quoted 12 lines
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +5
-0+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 oneDP");
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.
Quoted 3 lines
Either way, I don't think using a module parameter for this is the right solution.
I will remove the cont_splash support for now and will have a new change to fully support hand-off, including all display subdrivers.
Quoted 17 lines
Quoted 17 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +17
-0+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 */
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c line 91
Quoted 4 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +4
-0+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_enandbkl_enseem wrongly placed. They should be handled in the panel and backlight drivers, not the eDP driver.
I will move bkl_en to pwm_backlight DT and use gpiod_* APIs. We don't have a panel driver and hope the eDP driver can support all the panels.
Quoted 19 lines
Quoted 19 lines
+static const struct
edp_pixel_clk_divclk_divs[2][EDP_PIXEL_CLK_NUM] = {drivers/gpu/drm/msm/edp/edp_ctrl.c new file +17
-0+ {/* 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 */
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c line 170
Quoted 4 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +4
-0+ {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.
The computation are from internal tools. We can add more entries when we need to support new panels.
Quoted 10 lines
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +2
-0+static void edp_ctrl_on_worker (struct work_struct *work) +{[...]
Quoted 6 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +6
-0+} + +static void edp_ctrl_off_worker (struct work_struct *work) +{ +struct edp_ctrl *ctrl =container_of ( + work, struct edp_ctrl, off_work);
Quoted 4 lines
Quoted 1 line
+}
Why are these two functions workers?
msm_edp_ctrl_power() is called by user thread in bridge->enable/disable and during edp on/off, it will send command to panel and block and wait for panel's response. We don't want to block user thread. Also, the bridge->enable/disable have no return value and there is no way to report error to user. During test, we had a issue when a signal is pending, it will interrupt and wake up the user thread waiting process. In this case, user has no way to know it.
Quoted 9 lines
Quoted 3 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +3
-0+ +irqreturn_t msm_edp_ctrl_irq (struct edp_ctrl *ctrl) +{[...]
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1449
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +2
-0+if (isr1 &EDP_INTERRUPT_REG_1_HPD ) +DBG ("edp_hpd" );Don't you want to handle this?
We will have another change to support HPD. Also, this is not a reliable signal for HPD.
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +10
-0+ +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);
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c line 1698
Quoted 5 lines
drivers/gpu/drm/msm/edp/edp_ctrl.c new file +5
-0+ 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.
I will change the function name.
Quoted 34 lines
Quoted 2 lines
drivers/gpu/drm/msm/edp/edp_phy.c +0
-0[...]
drivers/gpu/drm/msm/edp/edp_phy.c line 32
Quoted 10 lines
drivers/gpu/drm/msm/edp/edp_phy.c +10
-0+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?Quoted 17 lines
drivers/gpu/drm/msm/edp/edp_phy.c +17
-0+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 );
Quoted 9 lines
drivers/gpu/drm/msm/edp/edp_phy.c line 58
Quoted 4 lines
drivers/gpu/drm/msm/edp/edp_phy.c +4
-0+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.
Some of the values are magic number for H/W. They are hard to define.
Thanks,
Hai