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

Hi, Rex:

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

quoted

On Tue, 2022-06-07 at 14:44 +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 int mtk_dp_train_handler(struct mtk_dp *mtk_dp)
+{
+	bool training_done = false;
+	short max_retry = 50;
+	int ret = 0;
+
+	do {
+		switch (mtk_dp->train_state) {
+		case MTK_DP_TRAIN_STATE_STARTUP:

mtk_dp->train_state is initialized as MTK_DP_TRAIN_STATE_STARTUP
even
though HPD ISR does not exist. Does this mean HPD ISR is
redundant?
If
HPD ISR is not redundant, create a new state
MTK_DP_TRAIN_STATE_NONE
for init state.

Hello CK,

I think we don't need MTK_DP_TRAIN_STATE_NONE.
Because it's "DP_TRAIN_STATE" not "DP_STATE", I think it's ok if we
start this state machine with "MTK_DP_TRAIN_STATE_STARTUP".

The initial state is MTK_DP_TRAIN_STATE_STARTUP, and HPD thread would
change state from MTK_DP_TRAIN_STATE_STARTUP to
MTK_DP_TRAIN_STATE_STARTUP, this is redundant. So drop the state
change
in HPD thread.

ok, in edp it's redundant in mtk_dp_hpd_event_thread(), but I will add
it back in dp patch.

quoted

quoted

quoted

+			mtk_dp_state_handler(mtk_dp);
+			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-

quoted

train_info.check_cap_count = 0;

+				mtk_dp->train_state =
MTK_DP_TRAIN_STATE_CHECKEDID;
+			} else {
+				mtk_dp-

quoted

train_info.check_cap_count++;

+
+				if (mtk_dp-

quoted

train_info.check_cap_count

+				    MTK_DP_CHECK_SINK_CAP_TIMEO
UT_COUNT
) {
+					mtk_dp-

quoted

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;

MTK_DP_TRAIN_STATE_CHECKEDID is a redundant state, drop it.

quoted

+			break;
+
+		case MTK_DP_TRAIN_STATE_TRAINING_PRE:
+			mtk_dp_state_handler(mtk_dp);
+			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 == 0) {
+				mtk_dp_video_mute(mtk_dp,
true);
+				mtk_dp->train_state =
MTK_DP_TRAIN_STATE_NORMAL;
+				mtk_dp_fec_enable(mtk_dp,
mtk_dp-

quoted

has_fec);

+			} else if (ret != -EAGAIN) {
+				mtk_dp->train_state =
MTK_DP_TRAIN_STATE_DPIDLE;
+			}
+			break;
+		case MTK_DP_TRAIN_STATE_NORMAL:
+			mtk_dp_state_handler(mtk_dp);
+			training_done = true;
+			break;
+		case MTK_DP_TRAIN_STATE_DPIDLE:

When would this case happen?

Regards,
CK

Yes, if it's disconnected if we are still training for dp.
or failed to training min spec RBR.

I mean, every time state change to MTK_DP_TRAIN_STATE_DPIDLE, it
would
jump out of this loop and would not get into this loop, so this case
would never get in. This is redundant, so remove this.

Regards,
CK

ok, I will remove it

BRs,
Bo-Chen

quoted

BRs,
Rex

quoted

quoted

+			break;
+		default:
+			break;
+		}
+
+		if (ret) {
+			if (ret == -EAGAIN)
+				continue;
+			/*
+			 * If we get any other error number, it
doesn't
+			 * make any sense to keep iterating.
+			 */
+			break;
+		}
+	} while (!training_done || --max_retry);
+
+	return ret;
+}