Re: [PATCH v8 15/19] drm/mediatek: Add mt8195 Embedded DisplayPort driver
From: AngeloGioacchino Del Regno
Date: Mon Feb 21 2022 - 09:33:19 EST
Il 18/02/22 15:54, Guillaume Ranquet ha scritto:
From: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
This patch adds a DisplayPort driver for the Mediatek mt8195 SoC.
It supports the mt8195, the embedded DisplayPort units. It offers
hot-plug-detection and DisplayPort 1.4 with up to 4 lanes.
The driver creates a child device for the phy. The child device will
never exist without the parent being active. As they are sharing a
register range, the parent passes a regmap pointer to the child so that
both can work with the same register range. The phy driver sets device
data that is read by the parent to get the phy device that can be used
to control the phy properties.
This driver is based on an initial version by
Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>.
Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>
Signed-off-by: Guillaume Ranquet <granquet@xxxxxxxxxxxx>
Reported-by: kernel test robot <lkp@xxxxxxxxx>
---
drivers/gpu/drm/mediatek/Kconfig | 7 +
drivers/gpu/drm/mediatek/Makefile | 2 +
drivers/gpu/drm/mediatek/mtk_dp.c | 2358 ++++++++++++++++++++++++
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 ++++++
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 +
6 files changed, 2937 insertions(+)
create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c
create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h
diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediatek/Kconfig
index 2976d21e9a34a..029b94c716131 100644
--- a/drivers/gpu/drm/mediatek/Kconfig
+++ b/drivers/gpu/drm/mediatek/Kconfig
@@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI
select PHY_MTK_HDMI
help
DRM/KMS HDMI driver for Mediatek SoCs
+
+config MTK_DPTX_SUPPORT
+ tristate "DRM DPTX Support for Mediatek SoCs"
+ depends on DRM_MEDIATEK
+ select PHY_MTK_DP
+ help
+ DRM/KMS Display Port driver for Mediatek SoCs.
diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
index 29098d7c8307c..d86a6406055e6 100644
--- a/drivers/gpu/drm/mediatek/Makefile
+++ b/drivers/gpu/drm/mediatek/Makefile
@@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
mtk_hdmi_ddc.o
obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
+
+obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
new file mode 100644
index 0000000000000..5b60293ecc0d3
--- /dev/null
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -0,0 +1,2358 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ * Copyright (c) 2021 BayLibre
+ */
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_dp_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_of.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <linux/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <sound/hdmi-codec.h>
+#include <video/videomode.h>
+
+#include "mtk_dp_reg.h"
+
+#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
+#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
+
+#define MTK_DP_MAX_LANES 4
+#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3
DP_MAX_LANES and DP_MAX_LINK_RATE should be SoC-specific properties and
not definitions, as newer SoCs may support a lower, or higher (which is
right now impossible but future-proofing the driver looks right).
+
+#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08
+
+#define MTK_DP_TRAIN_RETRY_LIMIT 8
+#define MTK_DP_TRAIN_MAX_ITERATIONS 5
+
+#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20
+
+#define MTK_DP_DP_VERSION_11 0x11
+
+enum mtk_dp_state {
+ MTK_DP_STATE_INITIAL,
+ MTK_DP_STATE_IDLE,
+ MTK_DP_STATE_PREPARE,
+ MTK_DP_STATE_NORMAL,
+};
+
+enum mtk_dp_train_state {
+ MTK_DP_TRAIN_STATE_STARTUP = 0,
+ MTK_DP_TRAIN_STATE_CHECKCAP,
+ MTK_DP_TRAIN_STATE_CHECKEDID,
+ MTK_DP_TRAIN_STATE_TRAINING_PRE,
+ MTK_DP_TRAIN_STATE_TRAINING,
+ MTK_DP_TRAIN_STATE_NORMAL,
+ MTK_DP_TRAIN_STATE_DPIDLE,
+};
+
+struct mtk_dp_timings {
+ struct videomode vm;
+
+ u16 htotal;
+ u16 vtotal;
+ u8 frame_rate;
+ u32 pix_rate_khz;
+};
+
+struct mtk_dp_train_info {
+ bool tps3;
+ bool tps4;
+ bool sink_ssc;
+ bool cable_plugged_in;
+ bool cable_state_change;
+ bool cr_done;
+ bool eq_done;
+
+ // link_rate is in multiple of 0.27Gbps
C-style comments please:
/* link_rate is in multiples of 0.27Gbps */
+ int link_rate;
+ int lane_count;
+
+ int irq_status;
Can irq_status ever be < 0?
+ int check_cap_count;
+};
+
C-style comments please. Here and everywhere else.
+// Same values as used by the DP Spec for MISC0 bits 1 and 2
+enum mtk_dp_color_format {
+ MTK_DP_COLOR_FORMAT_RGB_444 = 0,
+ MTK_DP_COLOR_FORMAT_YUV_422 = 1,
+ MTK_DP_COLOR_FORMAT_YUV_444 = 2,
+ MTK_DP_COLOR_FORMAT_YUV_420 = 3,
+ MTK_DP_COLOR_FORMAT_YONLY = 4,
+ MTK_DP_COLOR_FORMAT_RAW = 5,
+ MTK_DP_COLOR_FORMAT_RESERVED = 6,
+ MTK_DP_COLOR_FORMAT_DEFAULT = MTK_DP_COLOR_FORMAT_RGB_444,
+ MTK_DP_COLOR_FORMAT_UNKNOWN = 15,
+};
+
+// Multiple of 0.27Gbps
+enum mtk_dp_linkrate {
+ MTK_DP_LINKRATE_RBR = 0x6,
+ MTK_DP_LINKRATE_HBR = 0xA,
+ MTK_DP_LINKRATE_HBR2 = 0x14,
+ MTK_DP_LINKRATE_HBR25 = 0x19,
+ MTK_DP_LINKRATE_HBR3 = 0x1E,
+};
+
+// Same values as used for DP Spec MISC0 bits 5,6,7
+enum mtk_dp_color_depth {
+ MTK_DP_COLOR_DEPTH_6BIT = 0,
+ MTK_DP_COLOR_DEPTH_8BIT = 1,
+ MTK_DP_COLOR_DEPTH_10BIT = 2,
+ MTK_DP_COLOR_DEPTH_12BIT = 3,
+ MTK_DP_COLOR_DEPTH_16BIT = 4,
+ MTK_DP_COLOR_DEPTH_UNKNOWN = 5,
+};
+
+struct mtk_dp_info {
+ enum mtk_dp_color_depth depth;
+ enum mtk_dp_color_format format;
+ struct mtk_dp_timings timings;
+};
+
+struct dp_cal_data {
+ unsigned int glb_bias_trim;
+ unsigned int clktx_impse;
+
I would propose:
u32 tx_impsel_nmos_ln[MTK_DP_MAX_LANES];
u32 tx_impsel_pmos_ln[MTK_DP_MAX_LANES];
+ unsigned int ln0_tx_impsel_pmos;
+ unsigned int ln0_tx_impsel_nmos;
+ unsigned int ln1_tx_impsel_pmos;
+ unsigned int ln1_tx_impsel_nmos;
+ unsigned int ln2_tx_impsel_pmos;
+ unsigned int ln2_tx_impsel_nmos;
+ unsigned int ln3_tx_impsel_pmos;
+ unsigned int ln3_tx_impsel_nmos;
+};
+
+struct mtk_dp {
+ struct device *dev;
+ struct platform_device *phy_dev;
+ struct phy *phy;
+ struct dp_cal_data cal_data;
+
+ struct drm_device *drm_dev;
+ struct drm_bridge bridge;
+ struct drm_bridge *next_bridge;
+ struct drm_dp_aux aux;
+
+ /* Protects edid as it is used in both bridge ops and IRQ handler */
+ struct mutex edid_lock;
+ struct edid *edid;
+
+ u8 rx_cap[DP_RECEIVER_CAP_SIZE];
+
+ struct mtk_dp_info info;
+ enum mtk_dp_state state;
+
+ struct mtk_dp_train_info train_info;
+ enum mtk_dp_train_state train_state;
+ unsigned int input_fmt;
+
+ struct regmap *regs;
+ struct clk *dp_tx_clk;
+
+ bool enabled;
+
+ bool has_fec;
+ /* Protects the mtk_dp struct */
+ struct mutex dp_lock;
+
+ hdmi_codec_plugged_cb plugged_cb;
+ struct device *codec_dev;
+ u8 connector_eld[MAX_ELD_BYTES];
+ struct drm_connector *conn;
+};
+
+static struct regmap_config mtk_dp_regmap_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = SEC_OFFSET + 0x90,
+ .name = "mtk-dp-registers",
+};
+
+static struct mtk_dp *mtk_dp_from_bridge(struct drm_bridge *b)
+{
+ return container_of(b, struct mtk_dp, bridge);
+}
+
+static u32 mtk_dp_read(struct mtk_dp *mtk_dp, u32 offset)
+{
+ u32 read_val;
+ int ret;
+
+ ret = regmap_read(mtk_dp->regs, offset, &read_val);
+ if (ret) {
+ dev_err(mtk_dp->dev, "Failed to read register 0x%x: %d\n",
+ offset, ret);
+ return 0;
+ }
+
+ return read_val;
+}
+
+static void mtk_dp_write(struct mtk_dp *mtk_dp, u32 offset, u32 val)
+{
+ int ret;
+
+ ret = regmap_write(mtk_dp->regs, offset, val);
+ if (ret)
+ dev_err(mtk_dp->dev,
+ "Failed to write register 0x%x with value 0x%x: %d\n",
+ offset, val, ret);
You should return the error for the caller to take action.
+}
+
+static void mtk_dp_update_bits(struct mtk_dp *mtk_dp, u32 offset, u32 val,
+ u32 mask)
+{
+ int ret;
+
+ ret = regmap_update_bits(mtk_dp->regs, offset, mask, val);
+ if (ret)
+ dev_err(mtk_dp->dev,
+ "Failed to update register 0x%x with value 0x%x, mask 0x%x: %d\n",
+ offset, val, mask, ret);
Same here.
+}
+
+static void mtk_dp_bulk_16bit_write(struct mtk_dp *mtk_dp, u32 offset, u8 *buf,
+ size_t length)
+{
+ int i;
+ int num_regs = (length + 1) / 2;
+
C-style comments please.
+ // 2 bytes per register
+ for (i = 0; i < num_regs; i++) {
+ u32 val = buf[i * 2] |
+ (i * 2 + 1 < length ? buf[i * 2 + 1] << 8 : 0);
+
+ mtk_dp_write(mtk_dp, offset + i * 4, val);
Does it really make sense to keep writing if some write fails?
+ }
+}
+
+static unsigned long mtk_dp_sip_atf_call(unsigned int cmd, unsigned int para)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(MTK_DP_SIP_CONTROL_AARCH32, cmd, para, 0, 0, 0, 0, 0,
+ &res);
+
+ pr_debug("[DPTX]%s cmd 0x%x, p1 0x%x, ret 0x%lx-0x%lx", __func__, cmd,
+ para, res.a0, res.a1);
+ return res.a1;
+}
+
+static void mtk_dp_msa_bypass_disable(struct mtk_dp *mtk_dp)
+{
+ const u16 bits_to_set =
+ BIT(HTOTAL_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(VTOTAL_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(HSTART_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(VSTART_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(HWIDTH_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(VHEIGHT_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(HSP_SEL_DP_ENC0_P0_SHIFT) | BIT(HSW_SEL_DP_ENC0_P0_SHIFT) |
+ BIT(VSP_SEL_DP_ENC0_P0_SHIFT) | BIT(VSW_SEL_DP_ENC0_P0_SHIFT);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3030, bits_to_set,
+ bits_to_set);
+}
+
+static void mtk_dp_set_msa(struct mtk_dp *mtk_dp)
+{
+ struct mtk_dp_timings *timings = &mtk_dp->info.timings;
+
If you really don't expect (I don't) the following writes to error out, then
it makes sense to simply call regmap_update_bits here instead of using that
helper...
...otherwise, you should check if anything went wrong on each call, and
make this function to eventually return the error to the caller.
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3010, timings->htotal,
+ HTOTAL_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3018,
+ timings->vm.hsync_len + timings->vm.hback_porch,
+ HSTART_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3028,
+ timings->vm.hsync_len << HSW_SW_DP_ENC0_P0_SHIFT,
+ HSW_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3028, 0,
+ HSP_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3020, timings->vm.hactive,
+ HWIDTH_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3014, timings->vtotal,
+ VTOTAL_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_301C,
+ timings->vm.vsync_len + timings->vm.vback_porch,
+ VSTART_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_302C,
+ timings->vm.vsync_len << VSW_SW_DP_ENC0_P0_SHIFT,
+ VSW_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_302C, 0,
+ VSP_SW_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3024, timings->vm.vactive,
+ VHEIGHT_SW_DP_ENC0_P0_MASK);
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3064, timings->vm.hactive,
+ HDE_NUM_LAST_DP_ENC0_P0_MASK);
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3154, timings->htotal,
+ PGEN_HTOTAL_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3158,
+ timings->vm.hfront_porch,
+ PGEN_HSYNC_RISING_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_315C, timings->vm.hsync_len,
+ PGEN_HSYNC_PULSE_WIDTH_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3160,
+ timings->vm.hback_porch + timings->vm.hsync_len,
+ PGEN_HFDE_START_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3164, timings->vm.hactive,
+ PGEN_HFDE_ACTIVE_WIDTH_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3168, timings->vtotal,
+ PGEN_VTOTAL_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_316C,
+ timings->vm.vfront_porch,
+ PGEN_VSYNC_RISING_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3170, timings->vm.vsync_len,
+ PGEN_VSYNC_PULSE_WIDTH_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3174,
+ timings->vm.vback_porch + timings->vm.vsync_len,
+ PGEN_VFDE_START_DP_ENC0_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3178, timings->vm.vactive,
+ PGEN_VFDE_ACTIVE_WIDTH_DP_ENC0_P0_MASK);
+}
+
+static void mtk_dp_set_color_format(struct mtk_dp *mtk_dp,
+ enum mtk_dp_color_format color_format)
+{
+ u32 val;
+
+ mtk_dp->info.format = color_format;
+
C-style comments!
Also, check return value, or avoid helper function usage.
+ // Update MISC0
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034,
+ color_format << DP_TEST_COLOR_FORMAT_SHIFT,
+ DP_TEST_COLOR_FORMAT_MASK);
+
+ switch (color_format) {
+ case MTK_DP_COLOR_FORMAT_YUV_422:
+ val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR422;
+ break;
+ case MTK_DP_COLOR_FORMAT_YUV_420:
+ val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_YCBCR420;
+ break;
+ case MTK_DP_COLOR_FORMAT_YONLY:
+ case MTK_DP_COLOR_FORMAT_RAW:
+ case MTK_DP_COLOR_FORMAT_RESERVED:
+ case MTK_DP_COLOR_FORMAT_UNKNOWN:
+ drm_warn(mtk_dp->drm_dev, "Unsupported color format: %d\n",
+ color_format);
+ fallthrough;
+ case MTK_DP_COLOR_FORMAT_RGB_444:
+ case MTK_DP_COLOR_FORMAT_YUV_444:
+ val = PIXEL_ENCODE_FORMAT_DP_ENC0_P0_RGB;
+ break;
+ }
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val,
+ PIXEL_ENCODE_FORMAT_DP_ENC0_P0_MASK);
+}
+
+static void mtk_dp_set_color_depth(struct mtk_dp *mtk_dp,
+ enum mtk_dp_color_depth color_depth)
+{
+ u32 val;
+
+ mtk_dp->info.depth = color_depth;
+
+ // Update MISC0
ditto.
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3034,
+ color_depth << DP_TEST_BIT_DEPTH_SHIFT,
+ DP_TEST_BIT_DEPTH_MASK);
+
+ switch (color_depth) {
+ case MTK_DP_COLOR_DEPTH_6BIT:
+ val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_6BIT;
+ break;
+ case MTK_DP_COLOR_DEPTH_8BIT:
+ val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_8BIT;
+ break;
+ case MTK_DP_COLOR_DEPTH_10BIT:
+ val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_10BIT;
+ break;
+ case MTK_DP_COLOR_DEPTH_12BIT:
+ val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT;
+ break;
+ case MTK_DP_COLOR_DEPTH_16BIT:
+ val = VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT;
+ break;
+ case MTK_DP_COLOR_DEPTH_UNKNOWN:
+ drm_warn(mtk_dp->drm_dev, "Unsupported color depth %d\n",
+ color_depth);
+ return;
+ }
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_303C, val,
+ VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK);
+}
+
+static void mtk_dp_mn_overwrite_disable(struct mtk_dp *mtk_dp)
+{
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0,
+ VIDEO_M_CODE_SEL_DP_ENC0_P0_MASK);
also here........ and everywhere else.
+}
+
..snip..
+static void mtk_dp_aux_set_length(struct mtk_dp *mtk_dp, size_t length)
+{
+ if (length > 0) {
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3650,
+ (length - 1)
+ << MCU_REQ_DATA_NUM_AUX_TX_P0_SHIFT,
+ MCU_REQ_DATA_NUM_AUX_TX_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_362C, 0,
+ AUX_NO_LENGTH_AUX_TX_P0_MASK |
+ AUX_TX_AUXTX_OV_EN_AUX_TX_P0_MASK |
+ AUX_RESERVED_RW_0_AUX_TX_P0_MASK);
+ } else {
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_362C,
+ BIT(AUX_NO_LENGTH_AUX_TX_P0_SHIFT),
+ AUX_NO_LENGTH_AUX_TX_P0_MASK |
+ AUX_TX_AUXTX_OV_EN_AUX_TX_P0_MASK |
+ AUX_RESERVED_RW_0_AUX_TX_P0_MASK);
Something in this function needs an indentation fix :))
+ }
+}
+
+static int mtk_dp_aux_wait_for_completion(struct mtk_dp *mtk_dp, bool is_read)
+{
+ int wait_reply = MTK_DP_AUX_WAIT_REPLY_COUNT;
+
+ while (--wait_reply) {
+ u32 aux_irq_status;
+
+ if (is_read) {
+ u32 fifo_status =
+ mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3618);
Fits in one line, 83 cols is ok.
+
+ if (fifo_status &
+ (AUX_RX_FIFO_WRITE_POINTER_AUX_TX_P0_MASK |
+ AUX_RX_FIFO_FULL_AUX_TX_P0_MASK)) {
+ return 0;
+ }
+ }
+
+ aux_irq_status = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3640);
+ if (aux_irq_status & AUX_RX_RECV_COMPLETE_IRQ_TX_P0_MASK)
+ return 0;
+
+ if (aux_irq_status & AUX_400US_TIMEOUT_IRQ_AUX_TX_P0_MASK)
+ return -ETIMEDOUT;
+
Please describe the reason for this sleep.
+ usleep_range(100, 500);
+ }
+
+ return -ETIMEDOUT;
+}
+
..snip..
+
+static void mtk_dp_initialize_aux_settings(struct mtk_dp *mtk_dp)
+{
More C-style comments to fix, here and everywhere else.
+ // modify timeout threshold = 1595 [12 : 8]
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_360C, 0x1595,
+ AUX_TIMEOUT_THR_AUX_TX_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3658, 0,
+ AUX_TX_OV_EN_AUX_TX_P0_MASK);
+ // 25 for 26M
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3634,
+ 25 << AUX_TX_OVER_SAMPLE_RATE_AUX_TX_P0_SHIFT,
+ AUX_TX_OVER_SAMPLE_RATE_AUX_TX_P0_MASK);
+ // 13 for 26M
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_3614,
+ 13 << AUX_RX_UI_CNT_THR_AUX_TX_P0_SHIFT,
+ AUX_RX_UI_CNT_THR_AUX_TX_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_AUX_P0_37C8,
+ BIT(MTK_ATOP_EN_AUX_TX_P0_SHIFT),
+ MTK_ATOP_EN_AUX_TX_P0_MASK);
+}
+
+static void mtk_dp_initialize_digital_settings(struct mtk_dp *mtk_dp)
+{
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_304C, 0,
+ VBID_VIDEO_MUTE_DP_ENC0_P0_MASK);
+ mtk_dp_set_color_format(mtk_dp, MTK_DP_COLOR_FORMAT_RGB_444);
+ mtk_dp_set_color_depth(mtk_dp, MTK_DP_COLOR_DEPTH_8BIT);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3368,
+ 1 << BS2BS_MODE_DP_ENC1_P0_SHIFT,
+ BS2BS_MODE_DP_ENC1_P0_MASK);
+
+ // dp tx encoder reset all sw
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004,
+ BIT(DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0_SHIFT),
+ DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0_MASK);
Please describe the reason for this sleep.
+ usleep_range(1000, 5000);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_3004, 0,
+ DP_TX_ENCODER_4P_RESET_SW_DP_ENC0_P0_MASK);
+}
+
+static void mtk_dp_digital_sw_reset(struct mtk_dp *mtk_dp)
+{
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_340C,
+ BIT(DP_TX_TRANSMITTER_4P_RESET_SW_DP_TRANS_P0_SHIFT),
+ DP_TX_TRANSMITTER_4P_RESET_SW_DP_TRANS_P0_MASK);
Same here.
+ usleep_range(1000, 5000);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_340C, 0,
+ DP_TX_TRANSMITTER_4P_RESET_SW_DP_TRANS_P0_MASK);
+}
+
..snip..
+
+static int mtk_dp_phy_configure(struct mtk_dp *mtk_dp,
+ enum mtk_dp_linkrate link_rate, int lane_count)
+{
+ int ret;
+ union phy_configure_opts
+ phy_opts = { .dp = {
+ .link_rate =
+ link_rate_to_mb_per_s(mtk_dp, link_rate),
+ .set_rate = 1,
+ .lanes = lane_count,
+ .set_lanes = 1,
+ .ssc = mtk_dp->train_info.sink_ssc,
+ } };
This one looks a bit odd with this kind of style, let's do:
union phy_configure_opts phy_opts = {
.dp = {
.link_rate = link_rate_to_mb_per_s(mtk_dp, link_rate),
.set_rate = 1,
.lanes = lane_count,
.set_lanes = 1,
.ssc = mtk_dp->train_info.sink_ssc,
},
};
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP,
+ DP_PWR_STATE_MASK);
+
+ ret = phy_configure(mtk_dp->phy, &phy_opts);
+
Does it really make sense to mtk_dp_set_cal_data(), etc, if phy_configure() fails?
+ mtk_dp_set_cal_data(mtk_dp);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+ DP_PWR_STATE_BANDGAP_TPLL_LANE, DP_PWR_STATE_MASK);
+ return ret;
+}
+
+static void mtk_dp_set_idle_pattern(struct mtk_dp *mtk_dp, bool enable)
+{
+ const u32 val = POST_MISC_DATA_LANE0_OV_DP_TRANS_P0_MASK |
+ POST_MISC_DATA_LANE1_OV_DP_TRANS_P0_MASK |
+ POST_MISC_DATA_LANE2_OV_DP_TRANS_P0_MASK |
+ POST_MISC_DATA_LANE3_OV_DP_TRANS_P0_MASK;
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3580, enable ? val : 0, val);
+}
+
+static void mtk_dp_train_set_pattern(struct mtk_dp *mtk_dp, int pattern)
+{
+ if (pattern < 0 || pattern > 4) {
+ drm_err(mtk_dp->drm_dev,
+ "Implementation error, no such pattern %d\n", pattern);
+ return;
...I think that the caller should care about that failure, and do something about
it to either execute a recovery sequence, or fix this situation somehow, or just
return this to DRM.
I would say that if the link doesn't correctly execute/finish training, the output
will suffer (random corruption, permanent corruption, or simply not working link).
+ }
+
+ if (pattern == 1) // TPS1
+ mtk_dp_set_idle_pattern(mtk_dp, false);
+
+ mtk_dp_update_bits(mtk_dp,
+ MTK_DP_TRANS_P0_3400,
+ pattern ? BIT(pattern - 1) << PATTERN1_EN_DP_TRANS_P0_SHIFT : 0,
+ PATTERN1_EN_DP_TRANS_P0_MASK | PATTERN2_EN_DP_TRANS_P0_MASK |
+ PATTERN3_EN_DP_TRANS_P0_MASK |
+ PATTERN4_EN_DP_TRANS_P0_MASK);
+}
+
...snip...
+
+static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
+{
+ u32 sram_read_start = MTK_DP_TBC_BUF_READ_START_ADDR;
Define this variable inside of the if branch and, while at it, remove the
redundant assignment. This value is never used, as you're initializing it
right after.
+
+ if (mtk_dp->train_info.lane_count > 0) {
+ sram_read_start = min_t(u32,
+ MTK_DP_TBC_BUF_READ_START_ADDR,
+ mtk_dp->info.timings.vm.hactive /
+ (mtk_dp->train_info.lane_count * 4 * 2 * 2));
+ mtk_dp_set_sram_read_start(mtk_dp, sram_read_start);
+ }
+
+ mtk_dp_setup_encoder(mtk_dp);
+}
+
..snip..
+
+static void mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp)
+{
+ ssize_t ret;
+ u8 sink_count;
+ u8 link_status[DP_LINK_STATUS_SIZE] = {};
+ u32 sink_count_reg;
+ u32 link_status_reg;
Since you're initializing these immediately after - just initialize them in
the declaration instead: it's simply shorter.
If you have instead plans for varying these register offsets on a per-SoC
basis, then... same thing: we'd expect that data to be available for sure
if we reach this function, as the existance of it would be validated during
probe.
+ bool locked;
+
+ sink_count_reg = DP_SINK_COUNT_ESI;
+ link_status_reg = DP_LANE0_1_STATUS;
+
+ ret = drm_dp_dpcd_readb(&mtk_dp->aux, sink_count_reg, &sink_count);
+ if (ret < 0) {
+ drm_info(mtk_dp->drm_dev, "Read sink count failed: %ld\n", ret);
You're returning (and imo, you should return an error to the caller), yet this is
a drm_info message and not a drm_err.
"by nose", this doesn't smell right :-)
+ return;
+ }
+
+ ret = drm_dp_dpcd_readb(&mtk_dp->aux, DP_SINK_COUNT, &sink_count);
This is duplicated (and overwriting sink_count).
+ if (ret < 0) {
+ drm_info(mtk_dp->drm_dev,
+ "Read DP_SINK_COUNT_ESI failed: %ld\n", ret);
+ return;
+ }
+
+ ret = drm_dp_dpcd_read(&mtk_dp->aux, link_status_reg, link_status,
+ sizeof(link_status));
+ if (!ret) {
+ drm_info(mtk_dp->drm_dev, "Read link status failed: %ld\n",
+ ret);
+ return;
+ }
+
+ locked = drm_dp_channel_eq_ok(link_status,
+ mtk_dp->train_info.lane_count);
+ if (!locked && mtk_dp->train_state > MTK_DP_TRAIN_STATE_TRAINING_PRE)
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE;
+
+ if (link_status[1] & DP_REMOTE_CONTROL_COMMAND_PENDING)
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR,
+ DP_REMOTE_CONTROL_COMMAND_PENDING);
+
+ if (DP_GET_SINK_COUNT(sink_count) &&
+ (link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) {
+ mtk_dp_edid_free(mtk_dp);
+ mtk_dp->train_info.check_cap_count = 0;
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID;
For long sleeps (20ms is very long!) it is not just nice, but *mandatory* to
provide an explaination. Please add a comment to explain why.
+ msleep(20);
+ }
+}
+
..snip..
+static int mtk_dp_train_flow(struct mtk_dp *mtk_dp, int target_link_rate,
+ int target_lane_count)
+{
+ u8 link_status[DP_LINK_STATUS_SIZE] = {};
+ u8 lane_adjust[2] = {};
+ bool pass_tps1 = false;
+ bool pass_tps2_3 = false;
+ int train_retries;
+ int status_control;
+ int iteration_count;
+ u8 prev_lane_adjust;
+ u8 val;
+
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LINK_BW_SET, target_link_rate);
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LANE_COUNT_SET,
+ target_lane_count | DP_LANE_COUNT_ENHANCED_FRAME_EN);
+
+ if (mtk_dp->train_info.sink_ssc)
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_DOWNSPREAD_CTRL,
+ DP_SPREAD_AMP_0_5);
+
+ train_retries = 0;
+ status_control = 0;
+ iteration_count = 1;
+ prev_lane_adjust = 0xFF;
+
+ mtk_dp_set_lanes(mtk_dp, target_lane_count / 2);
+ mtk_dp_phy_configure(mtk_dp, target_link_rate, target_lane_count);
+
+ dev_dbg(mtk_dp->dev,
+ "Link train target_link_rate = 0x%x, target_lane_count = 0x%x\n",
+ target_link_rate, target_lane_count);
+
+ do {
+ train_retries++;
+ if (!mtk_dp->train_info.cable_plugged_in ||
+ ((mtk_dp->train_info.irq_status & MTK_DP_HPD_DISCONNECT) !=
+ 0x0)) {
+ return -ENODEV;
+ }
+
+ if (mtk_dp->train_state < MTK_DP_TRAIN_STATE_TRAINING)
+ return -EAGAIN;
+
+ if (!pass_tps1) {
+ mtk_dp_training_set_scramble(mtk_dp, false);
+
+ if (status_control == 0) {
+ status_control = 1;
+ mtk_dp_train_set_pattern(mtk_dp, 1);
+ val = DP_LINK_SCRAMBLING_DISABLE |
+ DP_TRAINING_PATTERN_1;
+ drm_dp_dpcd_writeb(&mtk_dp->aux,
+ DP_TRAINING_PATTERN_SET,
+ DP_LINK_SCRAMBLING_DISABLE |
+ DP_TRAINING_PATTERN_1);
+ drm_dp_dpcd_read(&mtk_dp->aux,
+ DP_ADJUST_REQUEST_LANE0_1,
+ lane_adjust,
+ sizeof(lane_adjust));
+ iteration_count++;
+
+ mtk_dp_train_update_swing_pre(mtk_dp,
+ target_lane_count, lane_adjust);
+ }
+
+ drm_dp_link_train_clock_recovery_delay(&mtk_dp->aux,
+ mtk_dp->rx_cap);
+ mtk_dp_read_link_status(mtk_dp, link_status);
+
+ if (drm_dp_clock_recovery_ok(link_status,
+ target_lane_count)) {
+ mtk_dp->train_info.cr_done = true;
+ pass_tps1 = true;
+ train_retries = 0;
+ iteration_count = 1;
+ dev_dbg(mtk_dp->dev, "Link train CR pass\n");
+ } else if (prev_lane_adjust == link_status[4]) {
+ iteration_count++;
+ if (prev_lane_adjust &
+ DP_ADJUST_VOLTAGE_SWING_LANE0_MASK)
+ break;
+ } else {
+ prev_lane_adjust = link_status[4];
+ }
+ dev_dbg(mtk_dp->dev, "Link train CQ fail\n");
+ } else if (pass_tps1 && !pass_tps2_3) {
+ if (status_control == 1) {
+ status_control = 2;
+ if (mtk_dp->train_info.tps4) {
+ mtk_dp_train_set_pattern(mtk_dp, 4);
+ val = DP_TRAINING_PATTERN_4;
+ } else if (mtk_dp->train_info.tps3) {
+ mtk_dp_train_set_pattern(mtk_dp, 3);
+ val = DP_LINK_SCRAMBLING_DISABLE |
+ DP_TRAINING_PATTERN_3;
+ } else {
+ mtk_dp_train_set_pattern(mtk_dp, 2);
+ val = DP_LINK_SCRAMBLING_DISABLE |
+ DP_TRAINING_PATTERN_2;
+ }
+ drm_dp_dpcd_writeb(&mtk_dp->aux,
+ DP_TRAINING_PATTERN_SET,
+ val);
+
+ drm_dp_dpcd_read(&mtk_dp->aux,
+ DP_ADJUST_REQUEST_LANE0_1,
+ lane_adjust,
+ sizeof(lane_adjust));
+
+ iteration_count++;
+ mtk_dp_train_update_swing_pre(mtk_dp,
+ target_lane_count, lane_adjust);
+ }
+
+ drm_dp_link_train_channel_eq_delay(&mtk_dp->aux,
+ mtk_dp->rx_cap);
+
+ mtk_dp_read_link_status(mtk_dp, link_status);
+
+ if (!drm_dp_clock_recovery_ok(link_status,
+ target_lane_count)) {
+ mtk_dp->train_info.cr_done = false;
+ mtk_dp->train_info.eq_done = false;
+ dev_dbg(mtk_dp->dev, "Link train EQ fail\n");
+ break;
+ }
+
+ if (drm_dp_channel_eq_ok(link_status,
+ target_lane_count)) {
+ mtk_dp->train_info.eq_done = true;
+ pass_tps2_3 = true;
+ dev_dbg(mtk_dp->dev, "Link train EQ pass\n");
+ break;
+ }
+
+ if (prev_lane_adjust == link_status[4])
+ iteration_count++;
+ else
+ prev_lane_adjust = link_status[4];
+ }
+
+ drm_dp_dpcd_read(&mtk_dp->aux, DP_ADJUST_REQUEST_LANE0_1,
+ lane_adjust, sizeof(lane_adjust));
+ mtk_dp_train_update_swing_pre(mtk_dp, target_lane_count,
+ lane_adjust);
+ } while (train_retries < MTK_DP_TRAIN_RETRY_LIMIT &&
+ iteration_count < MTK_DP_TRAIN_MAX_ITERATIONS);
+
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_TRAINING_PATTERN_SET,
+ DP_TRAINING_PATTERN_DISABLE);
+ mtk_dp_train_set_pattern(mtk_dp, 0);
+
I would do instead:
if (!pass_tps2_3)
return -ETIMEDOUT;
mtk_dp->train_info.link_....blah
return 0;
}
+ if (pass_tps2_3) {
+ mtk_dp->train_info.link_rate = target_link_rate;
+ mtk_dp->train_info.lane_count = target_lane_count;
+
+ mtk_dp_training_set_scramble(mtk_dp, true);
+
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_LANE_COUNT_SET,
+ target_lane_count |
+ DP_LANE_COUNT_ENHANCED_FRAME_EN);
+ mtk_dp_set_enhanced_frame_mode(mtk_dp, true);
+
+ return 0;
+ }
+
+ return -ETIMEDOUT;
+}
+
..snip..
+
+static int mtk_dp_train_start(struct mtk_dp *mtk_dp)
+{
+ int ret = 0;
+ int lane_count;
+ int link_rate;
+ int train_limit;
+ int max_link_rate;
+ int plug_wait;
+
+ for (plug_wait = 7; !mtk_dp_plug_state(mtk_dp) && plug_wait > 0;
+ --plug_wait)
+ usleep_range(1000, 5000);
+ if (plug_wait == 0) {
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE;
+ return -ENODEV;
+ }
+
+ link_rate = mtk_dp->rx_cap[1];
+ lane_count = mtk_dp->rx_cap[2] & 0x1F;
+
+ mtk_dp->train_info.link_rate = min(MTK_DP_MAX_LINK_RATE, link_rate);
+ mtk_dp->train_info.lane_count = min(MTK_DP_MAX_LANES, lane_count);
+ link_rate = mtk_dp->train_info.link_rate;
+ lane_count = mtk_dp->train_info.lane_count;
+
+ switch (link_rate) {
+ case MTK_DP_LINKRATE_RBR:
+ case MTK_DP_LINKRATE_HBR:
+ case MTK_DP_LINKRATE_HBR2:
+ case MTK_DP_LINKRATE_HBR25:
+ case MTK_DP_LINKRATE_HBR3:
+ break;
+ default:
+ mtk_dp->train_info.link_rate = MTK_DP_LINKRATE_HBR3;
+ break;
+ };
+
+ max_link_rate = link_rate;
+ for (train_limit = 0; train_limit <= 6; ++train_limit) {
I would say that train_limit should be 6, and you should decrement instead.
for (train_limit = 6; train_limit > 0; train_limit--) {
+ mtk_dp->train_info.cr_done = false;
+ mtk_dp->train_info.eq_done = false;
+
+ mtk_dp_train_change_mode(mtk_dp);
+ ret = mtk_dp_train_flow(mtk_dp, link_rate, lane_count);
+ if (ret == -ENODEV || ret == -EAGAIN)
This function returns 0 for success, negative number for error; so, it is
sufficient here to simply check
if (ret)
+ return ret;
+
+ if (!mtk_dp->train_info.cr_done) {
+ switch (link_rate) {
+ case MTK_DP_LINKRATE_RBR:
+ lane_count = lane_count / 2;
+ link_rate = max_link_rate;
+ if (lane_count == 0x0) {
if (lane_count == 0) {
+ mtk_dp->train_state =
+ MTK_DP_TRAIN_STATE_DPIDLE;
+ return -EIO;
+ }
+ break;
+ case MTK_DP_LINKRATE_HBR:
+ link_rate = MTK_DP_LINKRATE_RBR;
+ break;
+ case MTK_DP_LINKRATE_HBR2:
+ link_rate = MTK_DP_LINKRATE_HBR;
+ break;
+ case MTK_DP_LINKRATE_HBR3:
+ link_rate = MTK_DP_LINKRATE_HBR2;
+ break;
+ default:
+ return -EINVAL;
+ };
+ } else if (!mtk_dp->train_info.eq_done) {
+ lane_count /= 2;
+ if (lane_count == 0)
+ return -EIO;
Please reorder the following as such:
if (lane_count == 0)
return -EIO;
lane_count /= 2;
Obviously, 0 divided by anything is still zero.
+ } else {
I would do:
break;
+ return 0;
+ }
+ }
if (train_limit == 0)
return -ETIMEDOUT;
return 0;
+
+ return -ETIMEDOUT;
+}
+
+static void mtk_dp_train_handler(struct mtk_dp *mtk_dp)
+{
+ int ret = 0;
+ int i = 50;
+
+ do {
This is a for loop, masked as a while loop... so please use a for loop.
+ if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL)
+ continue;
+
+ switch (mtk_dp->train_state) {
+ case MTK_DP_TRAIN_STATE_STARTUP:
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP;
+ break;
+
+ case MTK_DP_TRAIN_STATE_CHECKCAP:
+ if (mtk_dp_parse_capabilities(mtk_dp)) {
+ mtk_dp->train_info.check_cap_count = 0;
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKEDID;
+ } else {
+ mtk_dp->train_info.check_cap_count++;
+
+ if (mtk_dp->train_info.check_cap_count >
+ MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT) {
+ mtk_dp->train_info.check_cap_count = 0;
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE;
+ ret = -ETIMEDOUT;
+ }
+ }
+ break;
+
+ case MTK_DP_TRAIN_STATE_CHECKEDID:
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING_PRE;
+ break;
+
+ case MTK_DP_TRAIN_STATE_TRAINING_PRE:
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_TRAINING;
+ break;
+
+ case MTK_DP_TRAIN_STATE_TRAINING:
+ ret = mtk_dp_train_start(mtk_dp);
+ if (!ret) {
+ mtk_dp_video_mute(mtk_dp, true);
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_NORMAL;
+ mtk_dp_fec_enable(mtk_dp, mtk_dp->has_fec);
+ } else if (ret != -EAGAIN) {
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE;
+ }
+
+ ret = 0;
+ break;
+
+ case MTK_DP_TRAIN_STATE_NORMAL:
+ break;
+ case MTK_DP_TRAIN_STATE_DPIDLE:
+ break;
+ default:
+ break;
+ }
+ } while (ret && i--);
+
+ if (ret)
+ drm_err(mtk_dp->drm_dev, "Train handler failed %d\n", ret);
You should return an error.
+}
+
..snip..
+static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev)
+{
+ struct mtk_dp *mtk_dp = dev;
+ int event;
+ u8 buf[DP_RECEIVER_CAP_SIZE] = {};
+
+ event = mtk_dp_plug_state(mtk_dp) ? connector_status_connected :
+ connector_status_disconnected;
+
+ if (event < 0)
+ return IRQ_HANDLED;
+
+ if (mtk_dp->drm_dev) {
+ dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n");
This is a debugging message: dev_dbg() here.
+ drm_helper_hpd_irq_event(mtk_dp->bridge.dev);
+ }
+
+ if (mtk_dp->train_info.cable_state_change) {
+ mtk_dp->train_info.cable_state_change = false;
+
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
+
+ if (!mtk_dp->train_info.cable_plugged_in ||
+ !mtk_dp_plug_state(mtk_dp)) {
+ mtk_dp_video_mute(mtk_dp, true);
+
+ mtk_dp_initialize_priv_data(mtk_dp);
+ mtk_dp_set_idle_pattern(mtk_dp, true);
+ if (mtk_dp->has_fec)
+ mtk_dp_fec_enable(mtk_dp, false);
+
+ mtk_dp_edid_free(mtk_dp);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+ DP_PWR_STATE_BANDGAP_TPLL,
+ DP_PWR_STATE_MASK);
+ } else {
+ mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
+ DP_PWR_STATE_BANDGAP_TPLL_LANE,
+ DP_PWR_STATE_MASK);
+ drm_dp_read_dpcd_caps(&mtk_dp->aux, buf);
+ mtk_dp->train_info.link_rate =
+ min_t(int, MTK_DP_MAX_LINK_RATE,
+ buf[DP_MAX_LINK_RATE]);
+ mtk_dp->train_info.lane_count =
+ min_t(int, MTK_DP_MAX_LANES,
+ drm_dp_max_lane_count(buf));
+ }
+ }
+
+ if (mtk_dp->train_info.irq_status & MTK_DP_HPD_INTERRUPT) {
+ dev_info(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n");
dev_dbg()
+ mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT;
+ mtk_dp_hpd_sink_event(mtk_dp);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t mtk_dp_hpd_isr_handler(struct mtk_dp *mtk_dp)
+{
+ bool connected;
+ u16 swirq_status = mtk_dp_swirq_get_clear(mtk_dp);
+ u8 hwirq_status = mtk_dp_hwirq_get_clear(mtk_dp);
+ struct mtk_dp_train_info *train_info = &mtk_dp->train_info;
+
+ train_info->irq_status |= hwirq_status | swirq_status;
+
+ if (!train_info->irq_status)
+ return IRQ_HANDLED;
+
+ connected = mtk_dp_plug_state(mtk_dp);
+ if (connected || !train_info->cable_plugged_in)
+ train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT;
+ else if (!connected || train_info->cable_plugged_in)
+ train_info->irq_status &= ~MTK_DP_HPD_CONNECT;
+
+ if (!(train_info->irq_status &
+ (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT)))
+ return IRQ_HANDLED;
+
+ if (train_info->irq_status & MTK_DP_HPD_CONNECT) {
+ train_info->irq_status &= ~MTK_DP_HPD_CONNECT;
+ train_info->cable_plugged_in = true;
+ } else {
+ train_info->irq_status &= ~MTK_DP_HPD_DISCONNECT;
+ train_info->cable_plugged_in = false;
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
+ }
+ train_info->cable_state_change = true;
+
+ return IRQ_WAKE_THREAD;
+}
+
+static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev)
+{
+ struct mtk_dp *mtk_dp = dev;
+ u32 irq_status;
+
+ irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS);
+
+ if (!irq_status)
+ return IRQ_HANDLED;
+
+ if (irq_status & RGS_IRQ_STATUS_TRANSMITTER)
+ return mtk_dp_hpd_isr_handler(mtk_dp);
+
+ return IRQ_HANDLED;
+}
+
+static int mtk_dp_dt_parse(struct mtk_dp *mtk_dp,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ int ret = 0;
+ void __iomem *base;
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ mtk_dp->regs = devm_regmap_init_mmio(dev, base, &mtk_dp_regmap_config);
+ if (IS_ERR(mtk_dp->regs))
+ return PTR_ERR(mtk_dp->regs);
+
+ mtk_dp->dp_tx_clk = devm_clk_get(dev, "faxi");
+ if (IS_ERR(mtk_dp->dp_tx_clk)) {
+ ret = PTR_ERR(mtk_dp->dp_tx_clk);
+ dev_info(dev, "Failed to get dptx clock: %d\n", ret);
Is this an optional clock? If so, devm_clk_get_optional(), otherwise, please
return an error.
In either case, the print would be dev_err().
+ mtk_dp->dp_tx_clk = NULL;
+ }
+
+ return 0;
+}
+
+static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
+{
+ return connector_status_connected;
+}
+
+static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
+ struct drm_connector *connector)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+ bool enabled = mtk_dp->enabled;
+ struct edid *new_edid = NULL;
+
+ if (!enabled)
+ drm_bridge_chain_pre_enable(bridge);
+
+ drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
+ usleep_range(2000, 5000);
+
+ if (mtk_dp_plug_state(mtk_dp))
+ new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
+
+ if (!enabled)
+ drm_bridge_chain_post_disable(bridge);
+
+ mutex_lock(&mtk_dp->edid_lock);
+ kfree(mtk_dp->edid);
+ mtk_dp->edid = NULL;
+
+ if (!new_edid) {
+ mutex_unlock(&mtk_dp->edid_lock);
+ return NULL;
+ }
+
+ mtk_dp->edid = drm_edid_duplicate(new_edid);
+ mutex_unlock(&mtk_dp->edid_lock);
+
+ return new_edid;
+}
+
+static ssize_t mtk_dp_aux_transfer(struct drm_dp_aux *mtk_aux,
+ struct drm_dp_aux_msg *msg)
+{
+ ssize_t err = -EAGAIN;
+ struct mtk_dp *mtk_dp;
+ bool is_read;
+ u8 request;
+ size_t accessed_bytes = 0;
+ int retry = 3, ret = 0;
+
+ mtk_dp = container_of(mtk_aux, struct mtk_dp, aux);
+
+ if (!mtk_dp->train_info.cable_plugged_in ||
+ mtk_dp->train_info.irq_status & MTK_DP_HPD_DISCONNECT) {
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKCAP;
+ err = -EAGAIN;
+ goto err;
+ }
+
+ switch (msg->request) {
+ case DP_AUX_I2C_MOT:
+ case DP_AUX_I2C_WRITE:
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE_STATUS_UPDATE:
+ case DP_AUX_I2C_WRITE_STATUS_UPDATE | DP_AUX_I2C_MOT:
+ request = msg->request & ~DP_AUX_I2C_WRITE_STATUS_UPDATE;
+ is_read = false;
+ break;
+ case DP_AUX_I2C_READ:
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ | DP_AUX_I2C_MOT:
+ request = msg->request;
+ is_read = true;
+ break;
+ default:
+ drm_err(mtk_aux->drm_dev, "invalid aux cmd = %d\n",
+ msg->request);
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (msg->size == 0) {
+ ret = mtk_dp_aux_do_transfer(mtk_dp, is_read, request,
+ msg->address + accessed_bytes,
+ msg->buffer + accessed_bytes, 0);
+ }
Have you tried to build this driver before submitting it?!
I say that this won't compile.
+ } else {
+ while (accessed_bytes < msg->size) {
+ size_t to_access =
+ min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES,
+ msg->size - accessed_bytes);
+ while (retry--) {
+ ret = mtk_dp_aux_do_transfer(mtk_dp,
+ is_read, request,
+ msg->address + accessed_bytes,
+ msg->buffer + accessed_bytes,
+ to_access);
+ if (ret == 0)
+ break;
+ usleep_range(50, 100);
+ }
+ if (!retry && ret) {
This should be:
if (!retry || ret) {
drm_err(.....
+ drm_info(mtk_dp->drm_dev,
+ "Failed to do AUX transfer: %d\n",
+ ret);
+ break;
+ }
+ accessed_bytes += to_access;
+ }
+ }
+err:
+ if (ret) {
+ msg->reply = DP_AUX_NATIVE_REPLY_NACK | DP_AUX_I2C_REPLY_NACK;
+ return err;
+ }
+
+ msg->reply = DP_AUX_NATIVE_REPLY_ACK | DP_AUX_I2C_REPLY_ACK;
+ return msg->size;
+}
+
+static void mtk_dp_aux_init(struct mtk_dp *mtk_dp)
+{
I don't see any advantage in putting these three lines in a helper,
called just once.
+ drm_dp_aux_init(&mtk_dp->aux);
+ mtk_dp->aux.name = "aux_mtk_dp";
+ mtk_dp->aux.transfer = mtk_dp_aux_transfer;
+}
+
+static void mtk_dp_poweroff(struct mtk_dp *mtk_dp)
+{
+ mutex_lock(&mtk_dp->dp_lock);
+
+ mtk_dp_hwirq_enable(mtk_dp, false);
+ mtk_dp_power_disable(mtk_dp);
+ phy_exit(mtk_dp->phy);
+ clk_disable_unprepare(mtk_dp->dp_tx_clk);
+
+ mutex_unlock(&mtk_dp->dp_lock);
+}
+
+static int mtk_dp_poweron(struct mtk_dp *mtk_dp)
+{
+ int ret = 0;
+
+ mutex_lock(&mtk_dp->dp_lock);
+
+ ret = clk_prepare_enable(mtk_dp->dp_tx_clk);
+ if (ret < 0) {
+ dev_err(mtk_dp->dev, "Fail to enable clock: %d\n", ret);
+ goto err;
+ }
+ ret = phy_init(mtk_dp->phy);
+ if (ret) {
+ dev_err(mtk_dp->dev, "Failed to initialize phy: %d\n", ret);
+ goto err_phy_init;
+ }
+ ret = mtk_dp_phy_configure(mtk_dp, MTK_DP_LINKRATE_RBR, 1);
+ if (ret) {
+ dev_err(mtk_dp->dev, "Failed to configure phy: %d\n", ret);
+ goto err_phy_config;
+ }
+
+ mtk_dp_init_port(mtk_dp);
+ mtk_dp_power_enable(mtk_dp);
+ mtk_dp_hwirq_enable(mtk_dp, true);
+
+err_phy_config:
+ phy_exit(mtk_dp->phy);
+err_phy_init:
+ clk_disable_unprepare(mtk_dp->dp_tx_clk);
+err:
+ mutex_unlock(&mtk_dp->dp_lock);
+ return ret;
+}
+
+static int mtk_dp_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+ int ret;
+
+ ret = mtk_dp_poweron(mtk_dp);
+ if (ret)
+ return ret;
+
+ if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
+ dev_err(mtk_dp->dev, "Driver does not provide a connector!");
+ return -EINVAL;
+ }
+
+ if (mtk_dp->next_bridge) {
+ ret = drm_bridge_attach(bridge->encoder, mtk_dp->next_bridge,
+ &mtk_dp->bridge, flags);
+ if (ret) {
+ drm_warn(mtk_dp->drm_dev,
+ "Failed to attach external bridge: %d\n", ret);
+ goto err_bridge_attach;
You are simply returning ret on your "err_bridge_attach" label, so you can as
well remove the label and simply `return ret` here.
+ }
+ }
+
+ mtk_dp->drm_dev = bridge->dev;
+
+ return 0;
+
+err_bridge_attach:
+ return ret;
+}
+
+static void mtk_dp_bridge_detach(struct drm_bridge *bridge)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+
+ mtk_dp->drm_dev = NULL;
+}
+
+static void mtk_dp_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_state)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+
+ mtk_dp_video_mute(mtk_dp, true);
+ mtk_dp->state = MTK_DP_STATE_IDLE;
+ mtk_dp->train_state = MTK_DP_TRAIN_STATE_STARTUP;
+
+ mtk_dp->enabled = false;
Big sleeps mandate a comment to explain why.
+ msleep(100);
+ mtk_dp_poweroff(mtk_dp);
+}
+
..snip..
+
+static enum drm_mode_status
+mtk_dp_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+ u32 rx_linkrate;
+ u32 bpp;
+
+ bpp = info->color_formats & DRM_COLOR_FORMAT_YCRCB422 ? 16 : 24;
rx_linkrate = (u32)mtk_dp->train_info.link_rate;
/* The link rate selector is in steps of 0.27Gbps */
rx_linkrate *= 27000;
/* Finally, multiply by number of lanes to get the current (bw) rate */
rx_linkrate *= mtk_dp->train_info.lane_count;
if ((mode->clock > 600000) ||
(rx_linkrate < (mode->clock * bpp) / 8) ||
((mode->clock * 1000) / (mode->htotal * mode->vtotal) > 62))
return MODE_CLOCK_HIGH;
return MODE_OK;
Besides, what's that 62? Looks like a magic number that comes out of the nowhere.
Please explain that with a comment in the code.
+ rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000;
+ if (rx_linkrate * mtk_dp->train_info.lane_count < mode->clock * bpp / 8)
+ return MODE_CLOCK_HIGH;
+
+ if (mode->clock > 600000)
+ return MODE_CLOCK_HIGH;
+
+ if ((mode->clock * 1000) / (mode->htotal * mode->vtotal) > 62)
+ return MODE_CLOCK_HIGH;
+
+ return MODE_OK;
+}
+
+static u32 *mtk_dp_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ unsigned int *num_output_fmts)
+{
+ u32 *output_fmts;
+
+ *num_output_fmts = 0;
+ output_fmts = kcalloc(1, sizeof(*output_fmts), GFP_KERNEL);
A stack allocation looks more appropriate...
.....and easier, as well, here: if you did that,
you would've also avoided a comment reminding you that you're
initializing the only member of the array right after an allocation that
sets the memory to zero, which would be unnecessary in this case :-P
P.S.: Just stack allocate it... :-)
+ if (!output_fmts)
+ return NULL;
+ *num_output_fmts = 1;
+ output_fmts[0] = MEDIA_BUS_FMT_FIXED;
+ return output_fmts;
+}
+
+static const u32 mt8195_input_fmts[] = {
+ MEDIA_BUS_FMT_RGB888_1X24,
+ MEDIA_BUS_FMT_YUV8_1X24,
+ MEDIA_BUS_FMT_YUYV8_1X16,
+};
+
+static u32 *mtk_dp_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state,
+ u32 output_fmt,
+ unsigned int *num_input_fmts)
+{
+ u32 *input_fmts;
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+ struct drm_display_mode *mode = &crtc_state->adjusted_mode;
+ struct drm_display_info *display_info =
+ &conn_state->connector->display_info;
+ u32 rx_linkrate;
+ u32 bpp;
+
+ bpp = (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422) ? 16 :
+ 24;
Why is this '24' on a new line?
Please fix.
+ rx_linkrate = (u32)mtk_dp->train_info.link_rate * 27000;
+ *num_input_fmts = 0;
+ input_fmts = kcalloc(ARRAY_SIZE(mt8195_input_fmts), sizeof(*input_fmts),
+ GFP_KERNEL);
You're copying the entire mt8195_input_fmts in here, so a kmalloc is fine.
+ if (!input_fmts)
+ return NULL;
+
+ *num_input_fmts = ARRAY_SIZE(mt8195_input_fmts);
+
+ memcpy(input_fmts, mt8195_input_fmts,
+ sizeof(*input_fmts) * ARRAY_SIZE(mt8195_input_fmts));
+
+ if (((rx_linkrate * mtk_dp->train_info.lane_count) <
+ (mode->clock * 24 / 8)) &&
+ ((rx_linkrate * mtk_dp->train_info.lane_count) >
+ (mode->clock * 16 / 8)) &&
+ (display_info->color_formats & DRM_COLOR_FORMAT_YCRCB422)) {
+ kfree(input_fmts);
+ input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL);
+ *num_input_fmts = 1;
+ input_fmts[0] = MEDIA_BUS_FMT_YUYV8_1X16;
+ }
+
+ return input_fmts;
+}
+
+static int mtk_dp_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
+ unsigned int input_bus_format;
+
+ input_bus_format = bridge_state->input_bus_cfg.format;
+
+ dev_info(mtk_dp->dev, "input format 0x%04x, output format 0x%04x\n",
dev_dbg() here, please.
+ bridge_state->input_bus_cfg.format,
+ bridge_state->output_bus_cfg.format);
+
+ mtk_dp->input_fmt = input_bus_format;
+ if (mtk_dp->input_fmt == MEDIA_BUS_FMT_YUYV8_1X16)
+ mtk_dp->info.format = MTK_DP_COLOR_FORMAT_YUV_422;
+ else
+ mtk_dp->info.format = MTK_DP_COLOR_FORMAT_RGB_444;
+
+ return 0;
+}
+
+static const struct drm_bridge_funcs mtk_dp_bridge_funcs = {
+ .atomic_check = mtk_dp_bridge_atomic_check,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_get_output_bus_fmts = mtk_dp_bridge_atomic_get_output_bus_fmts,
+ .atomic_get_input_bus_fmts = mtk_dp_bridge_atomic_get_input_bus_fmts,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .attach = mtk_dp_bridge_attach,
+ .detach = mtk_dp_bridge_detach,
+ .atomic_enable = mtk_dp_bridge_atomic_enable,
+ .atomic_disable = mtk_dp_bridge_atomic_disable,
+ .mode_valid = mtk_dp_bridge_mode_valid,
+ .get_edid = mtk_dp_get_edid,
+ .detect = mtk_dp_bdg_detect,
+};
+
+static int mtk_dp_probe(struct platform_device *pdev)
+{
+ struct mtk_dp *mtk_dp;
+ struct device *dev = &pdev->dev;
+ int ret;
+ int irq_num = 0;
+
+ mtk_dp = devm_kzalloc(dev, sizeof(*mtk_dp), GFP_KERNEL);
+ if (!mtk_dp)
+ return -ENOMEM;
+
+ mtk_dp->dev = dev;
+
+ irq_num = platform_get_irq(pdev, 0);
if (irq_num < 0)
return dev_err_probe(dev, irq_num, "failed to...blah");
+ if (irq_num < 0) {
+ dev_err(dev, "failed to request dp irq resource\n");
+ return irq_num;
+ }
+
+ mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
if (IS_ERR(mtk_dp->next_bridge))
return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
"Failed to get bridge\n");
+ if (IS_ERR(mtk_dp->next_bridge)) {
+ ret = PTR_ERR(mtk_dp->next_bridge);
+ dev_err_probe(dev, ret, "Failed to get bridge\n");
+ return ret;
+ }
+
+ ret = mtk_dp_dt_parse(mtk_dp, pdev);
+ if (ret)
+ return ret;
+
+ mtk_dp_aux_init(mtk_dp);
+
+ ret = devm_request_threaded_irq(dev, irq_num, mtk_dp_hpd_event,
+ mtk_dp_hpd_event_thread,
+ IRQ_TYPE_LEVEL_HIGH, dev_name(dev),
+ mtk_dp);
+ if (ret) {
+ dev_err(dev, "failed to request mediatek dptx irq\n");
dev_err_probe() here and everywhere else.
+ return -EPROBE_DEFER;
+ }
+
+ mutex_init(&mtk_dp->dp_lock);
+ mutex_init(&mtk_dp->edid_lock);
+
+ platform_set_drvdata(pdev, mtk_dp);
+
+ mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy",
+ PLATFORM_DEVID_AUTO,
+ &mtk_dp->regs,
+ sizeof(struct regmap *));
+ if (IS_ERR(mtk_dp->phy_dev)) {
+ dev_err(dev, "Failed to create device mediatek-dp-phy: %ld\n",
+ PTR_ERR(mtk_dp->phy_dev));
+ return PTR_ERR(mtk_dp->phy_dev);
+ }
You won't need to do that if you follow my advice about using device-tree to
register mediatek-dp-phy...
+
+ mtk_dp_get_calibration_data(mtk_dp);
+
+ mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
+ if (IS_ERR(mtk_dp->phy)) {
+ dev_err(dev, "Failed to get phy: %ld\n", PTR_ERR(mtk_dp->phy));
+ platform_device_unregister(mtk_dp->phy_dev);
... and you will be able to use dev_err_probe here as well.
+ return PTR_ERR(mtk_dp->phy);
+ }
+
+ mtk_dp->bridge.funcs = &mtk_dp_bridge_funcs;
+ mtk_dp->bridge.of_node = dev->of_node;
+ mtk_dp->bridge.type = DRM_MODE_CONNECTOR_eDP;
+
+ mtk_dp->bridge.ops =
+ DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
+ drm_bridge_add(&mtk_dp->bridge);
+
+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+
+ return 0;
+}
+
...snip...
+
+static const struct of_device_id mtk_dp_of_match[] = {
+ {
+ .compatible = "mediatek,mt8195-edp-tx",
+ },
This fits on a single line:
{ .compatible = "mediatek,mt8195-edp-tx" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mtk_dp_of_match);
+
+struct platform_driver mtk_dp_driver = {
+ .probe = mtk_dp_probe,
+ .remove = mtk_dp_remove,
+ .driver = {
+ .name = "mediatek-drm-dp",
+ .of_match_table = mtk_dp_of_match,
+ .pm = &mtk_dp_pm_ops,
+ },
+};
+
+MODULE_AUTHOR("Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>");
+MODULE_AUTHOR("Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>");
+MODULE_DESCRIPTION("MediaTek DisplayPort Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
new file mode 100644
index 0000000000000..79952ac30e9e6
--- /dev/null
+++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
@@ -0,0 +1,568 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019 MediaTek Inc.
+ * Copyright (c) 2021 BayLibre
+ */
+#ifndef _MTK_DP_REG_H_
+#define _MTK_DP_REG_H_
+
+#define MTK_DP_SIP_CONTROL_AARCH32 0x82000523
+#define MTK_DP_SIP_ATF_VIDEO_UNMUTE 0x20
+#define MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE 0x21
+
+#define DP_PHY_GLB_BIAS_GEN_00 0x0000
This is just zero, but being a register offset definition, it's fine
if you write it as 0x0.
+#define RG_XTP_GLB_BIAS_INTR_CTRL (0x1f << 16)
This is GENMASK(20, 16)
... and also please use GENMASK() for all of the others.
+
+#define DP_PHY_GLB_DPAUX_TX 0x0008
This is 0x8
+#define RG_CKM_PT0_CKTX_IMPSEL (0xf << 20)
+
..snip..
+#define MTK_DP_ENC0_P0_3000 (ENC0_OFFSET + 0x000)
+#define LANE_NUM_DP_ENC0_P0_MASK 0x3
+#define VIDEO_MUTE_SW_DP_ENC0_P0_MASK 0x4
+#define VIDEO_MUTE_SW_DP_ENC0_P0_SHIFT 2
At least some of these *_SHIFT definitions are, in reality, register bits that
are used to enable/disable something...
In these cases, remove _SHIFT and directly define them as BIT(x).
Also, please, for all the _MASK you should be using GENMASK().
Example for this one:
#define VIDEO_MUTE_SW_DP_ENC0_P0 BIT(2)
+#define VIDEO_MUTE_SEL_DP_ENC0_P0_MASK 0x8
+#define VIDEO_MUTE_SEL_DP_ENC0_P0_SHIFT 3
+#define ENHANCED_FRAME_EN_DP_ENC0_P0_MASK 0x10
+#define ENHANCED_FRAME_EN_DP_ENC0_P0_SHIFT 4
..snip..
+#define MTK_DP_ENC0_P0_303C (ENC0_OFFSET + 0x03C)
+#define SRAM_START_READ_THRD_DP_ENC0_P0_MASK 0x3f
+#define SRAM_START_READ_THRD_DP_ENC0_P0_SHIFT 0
+#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_MASK 0x700
+#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_SHIFT 8
+#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_16BIT \
+ (0 << VIDEO_COLOR_DEPTH_DP_ENC0_P0_SHIFT)
This (and not only) fits in one line... but also, 0 shifted by a million bits is
still zero, so this is just zero.
+#define VIDEO_COLOR_DEPTH_DP_ENC0_P0_12BIT \
+ (1 << VIDEO_COLOR_DEPTH_DP_ENC0_P0_SHIFT)
..snip..
+#define MTK_DP_AUX_P0_3704 (AUX_OFFSET + 0x104)
+#define AUX_TX_FIFO_WRITE_DATA_NEW_MODE_TOGGLE_AUX_TX_P0_MASK 0x2
+#define AUX_TX_FIFO_NEW_MODE_EN_AUX_TX_P0_MASK 0x4
+#define AUX_TX_FIFO_NEW_MODE_EN_AUX_TX_P0_SHIFT 2
+
+#define MTK_DP_AUX_P0_3708 (AUX_OFFSET + 0x108)
+
+#define MTK_DP_AUX_P0_37C8 (AUX_OFFSET + 0x1C8)
Please, lower case hex.
+#define MTK_ATOP_EN_AUX_TX_P0_MASK 0x1
+#define MTK_ATOP_EN_AUX_TX_P0_SHIFT 0
+
Regards,
Angelo