On Wed, 2022-06-08 at 17:15 +0800, CK Hu wrote:

Hi, Rex:

On Wed, 2022-06-08 at 16:43 +0800, Rex-BC Chen wrote:

quoted

On Wed, 2022-06-08 at 10:23 +0800, CK Hu wrote:

quoted

Hi, Rex:

On Tue, 2022-06-07 at 20:24 +0800, Rex-BC Chen wrote:

quoted

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

quoted

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

event is always > 0, isn't it?

Hello CK,

ok, I will move this to dp patch.

quoted

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.

I find out that mtk_dp_poweron() is in top of
mtk_dp_bridge_attach().
If move mtk_dp_poweron() to bottom of mtk_dp_bridge_attach(),
mtk_dp-

quoted

drm_dev would not be NULL here. So we could drop this checking.

Hello CK,

If we failed to setup phy(ret!=0), we alos need to deattach this
bridge.
I don't think  it's a good idea just for remove this.

OK, move mtk_dp_hwirq_enable() out of mtk_dp_poweron() and to the
bottom of mtk_dp_bridge_attach(). irq is not part of power.

I will do this and drop "if (mtk_dp->drm_dev)"

quoted

quoted

quoted

quoted

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.

In bottom of mtk_dp_hpd_isr_handler(), the code is:

+	train_info->cable_state_change = true;
+
+	return IRQ_WAKE_THREAD;

This thread is called only when return IRQ_WAKE_THREAD, and
before
return IRQ_WAKE_THREAD, train_info->cable_state_change is always
set
to
true. So in this thread, train_info->cable_state_change must be
true.

As mentioned, this irq handler function is not only for connected
status.

this could be return if this irq is interrupt from sink device.
+	if (!(train_info->irq_status &
+	      (MTK_DP_HPD_CONNECT | MTK_DP_HPD_DISCONNECT)))
+		return IRQ_HANDLED;

According to [1], return IRQ_WAKE_THREAD to wake up thread. So return
IRQ_HANDLED would not wake up thread.

[1] 

https://www.kernel.org/doc/htmldocs/kernel-api/API-request-threaded-irq.html

Regards,
CK

yes, you are right. I will return IRQ_WAKE_THREAD for handle sink
interrupt.

quoted

BRs,
Bo-Chen

quoted

Regards,
CK

quoted

quoted

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

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-

quoted

max_linkrate,

+				      buf[mtk_dp-

quoted

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;
+}
+