Messages in this thread Patch in this message | | | Date | Mon, 21 Feb 2022 15:31:57 +0100 | Subject | Re: [PATCH v8 15/19] drm/mediatek: Add mt8195 Embedded DisplayPort driver | From | AngeloGioacchino Del Regno <> |
| |
Il 18/02/22 15:54, Guillaume Ranquet ha scritto: > From: Markus Schneider-Pargmann <msp@baylibre.com> > > 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@mediatek.com>. > > Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > 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@mediatek.com>"); > +MODULE_AUTHOR("Markus Schneider-Pargmann <msp@baylibre.com>"); > +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
| |