On Tue, 2022-06-07 at 14:21 +0800, CK Hu wrote:

Hi, Rex:

On Mon, 2022-05-23 at 12:47 +0200, Guillaume Ranquet wrote:

quoted

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
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 [off-list ref].

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
Signed-off-by: Guillaume Ranquet <redacted>
---

[snip]

quoted

+
+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_disc
onnected;
+
+	if (event < 0)

event is always > 0, isn't it?

Hello CK,

ok, I will move this to dp patch.

quoted

+		return IRQ_HANDLED;
+
+	if (mtk_dp->drm_dev) {
+		dev_info(mtk_dp->dev, "drm_helper_hpd_irq_event\n");
+		drm_helper_hpd_irq_event(mtk_dp->bridge.dev);

I think this ISR would come once. If bridge has not attached, the drm
core would lost this event. Maybe you should enable eDP hardware
after
bridge attached or send this event when attached.

for edp patch, I will move it to (mtk_dp_bridge_attach).
for dp patch, I will add it back.

quoted

+	}
+
+	if (mtk_dp->train_info.cable_state_change) {

Executing this thread imply cable_state_change = true, so drop
cable_state_change.

In mtk_dp_hpd_isr_handler(), there is another irq
"MTK_DP_HPD_INTERRUPT" which means the sink devices give a interrupt to
source device. it's not about connected status, so I think we still
need this.

quoted

+		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)) {

I do not like two variable to present one thing. If

mtk_dp->train_info.cable_plugged_in = false
and
mtk_dp_plug_state(mtk_dp) = ture

What does this mean? I think this mean 'now' is connected because
cable_plugged_in is old information and mtk_dp_plug_state() is
current
information.

But I would like to keep cable_plugged_in and drop
mtk_dp_plug_state()
because cable_plugged_in would be changed in isr and it would be the
same as mtk_dp_plug_state().

Regards,
CK

ok, I will drop this.

BRs,
Rex

quoted

+			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_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_LA
NE,
+					   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_linkrate,
+				      buf[mtk_dp->max_linkrate]);
+			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_dbg(mtk_dp->dev, "MTK_DP_HPD_INTERRUPT\n");
+		mtk_dp->train_info.irq_status &= ~MTK_DP_HPD_INTERRUPT;
+		mtk_dp_hpd_sink_event(mtk_dp);
+	}
+
+	return IRQ_HANDLED;
+}
+