Hi Markus,
On Mon, 2021-09-06 at 21:35 +0200, Markus Schneider-Pargmann wrote:
This patch adds a DisplayPort driver for the Mediatek mt8195 SoC.
It supports both functional units on the mt8195, the embedded DisplayPort as well as the external DisplayPort units. It offers hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up to 4 lanes.
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
[...]
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c new file mode 100644 index 000000000000..1bd07c8d2f69 --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_dp.c @@ -0,0 +1,2881 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2019 MediaTek Inc.
- Copyright (c) 2021 BayLibre
- */
[...]
+#include <linux/component.h>
[...]
+#include <linux/extcon.h> +#include <linux/extcon-provider.h>
[...]
+#include <linux/kthread.h> +#include <linux/mfd/syscon.h>
[...]
+#include <linux/of_gpio.h> +#include <linux/of_graph.h>
[...]
+#include <linux/phy/phy.h>
The list of included headers could be pruned a bit, from a quick look it seems like neither of these are actually used.
[...]
+static void mtk_dp_ssc_enable(struct mtk_dp *mtk_dp, bool enable) +{
- mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE, DP_PWR_STATE_BANDGAP,
DP_PWR_STATE_MASK);- mtk_dp_update_bits(mtk_dp, DP_PHY_DIG_PLL_CTL_1,
enable ? TPLL_SSC_EN : 0, TPLL_SSC_EN);- mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
DP_PWR_STATE_BANDGAP_TPLL_LANE, DP_PWR_STATE_MASK);- udelay(50);
Can usleep_range() be used here? Same for the other delays.
[...]
+static void mtk_dp_hpd_sink_event(struct mtk_dp *mtk_dp) +{
[...]
- if (DP_GET_SINK_COUNT(sink_count) &&
(link_status[2] & DP_DOWNSTREAM_PORT_STATUS_CHANGED)) {mtk_dp->train_info.check_cap_count = 0;kfree(mtk_dp->edid);mtk_dp->edid = NULL;
Should this be protect by a lock? This looks like it could race with the access in mtk_dp_edid_parse_audio_capabilities() or mtk_dp_get_edid()
[...]
+static int mtk_dp_train_handler(struct mtk_dp *mtk_dp) +{
- int ret = 0;
- ret = mtk_dp_train_hpd_handle(mtk_dp);
- if (!mtk_dp->train_info.cable_plugged_in)
return -ENODEV;- if (mtk_dp->train_state == MTK_DP_TRAIN_STATE_NORMAL)
return ret;- switch (mtk_dp->train_state) {
[...]
- case MTK_DP_TRAIN_STATE_TRAINING:
ret = mtk_dp_train_start(mtk_dp);if (!ret) {mtk_dp_video_mute(mtk_dp, true);mtk_dp_audio_mute(mtk_dp, true);mtk_dp->train_state = MTK_DP_TRAIN_STATE_CHECKTIMING;mtk_dp_fec_enable(mtk_dp, mtk_dp->has_fec);} else if (ret != -EAGAIN)mtk_dp->train_state = MTK_DP_TRAIN_STATE_DPIDLE;
A small whitespace issue and missing braces.
Consider running this through checkpatch.pl --strict once for style issues.
[...]
+static irqreturn_t mtk_dp_hpd_event(int hpd, void *dev) +{
- struct mtk_dp *mtk_dp = dev;
- uint32_t irq_status;
- irq_status = mtk_dp_read(mtk_dp, MTK_DP_TOP_IRQ_STATUS);
- if (!irq_status)
return IRQ_HANDLED;
This check seems superfluous given that only the RGS_IRQ_STATUS_TRANSMITTER bit is checked right below:
- if (irq_status & RGS_IRQ_STATUS_TRANSMITTER)
return mtk_dp_hpd_isr_handler(mtk_dp);- return IRQ_HANDLED;
+}
[...]
+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 pre_enabled = mtk_dp->pre_enabled;
- if (mtk_dp->edid)
kfree(mtk_dp->edid);
Unnecessary check, kfree() accepts NULL.
regards Philipp