Hi, Bo-Chen: On Fri, 2022-07-01 at 14:28 +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx> > > This patch adds a embedded 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 > Jitao shi <jitao.shi@xxxxxxxxxxxx> > > Signed-off-by: Markus Schneider-Pargmann <msp@xxxxxxxxxxxx> > Signed-off-by: Guillaume Ranquet <granquet@xxxxxxxxxxxx> > Signed-off-by: Bo-Chen Chen <rex-bc.chen@xxxxxxxxxxxx> > --- [snip] > +/* > + * We need to handle HPD signal in eDP even though eDP is a always > connected > + * device. Besides connected status, there is another feature for > HPD signal - > + * HPD pulse: it presents an IRQ from sink devices to source devices > (Refer to > + * 5.1.4 of DP1.4 spec). > + */ > +static irqreturn_t mtk_dp_hpd_isr_handler(struct mtk_dp *mtk_dp) > +{ > + bool connected; > + bool hpd_connect_sta; > + u32 irq_status = mtk_dp_swirq_get_clear(mtk_dp) | > + mtk_dp_hwirq_get_clear(mtk_dp); > + struct mtk_dp_train_info *train_info = &mtk_dp->train_info; > + > + if (irq_status & MTK_DP_HPD_INTERRUPT) > + train_info->irq_sta.hpd_inerrupt = true; > + if (irq_status & MTK_DP_HPD_CONNECT) > + hpd_connect_sta = true; > + if (irq_status & MTK_DP_HPD_DISCONNECT) > + train_info->irq_sta.hpd_disconnect = true; hpd_disconnect is used only in this function, so let it be local variable. > + > + if (!irq_status) > + return IRQ_HANDLED; Move this to top of this function. > + > + connected = mtk_dp_plug_state(mtk_dp); > + if (connected || !train_info->cable_plugged_in) > + train_info->irq_sta.hpd_disconnect = false; The truth table of (irq_status & MTK_DP_HPD_DISCONNECT, connected, cable_plugged_in, hpd_disconnect) is 0 0 0 0 0 0 1 0 0 1 0 0 0 1 1 0 1 0 0 0 1 0 1 1 1 1 0 0 1 1 1 0 So the only case that hpd_disconnect is true is (irq_status & MTK_DP_HPD_DISCONNECT) && !connected && train_info- >cable_plugged_in) And train_info->cable_plugged_in is the previous status. The previous status is connected. And irq_status and connected is the new status. The new status is disconnected. I have a question. Why we need both irq_status and connected for new status? I think connected is enough for new status, so we could ignore irq_status. Regards, CK > + else if (!connected || train_info->cable_plugged_in) > + hpd_connect_sta = false; > + > + if (!(hpd_connect_sta || train_info->irq_sta.hpd_disconnect)) > + return IRQ_WAKE_THREAD; > + > + if (hpd_connect_sta) { > + hpd_connect_sta = false; > + train_info->cable_plugged_in = true; > + } else { > + train_info->irq_sta.hpd_disconnect = false; > + train_info->cable_plugged_in = false; > + } > + train_info->cable_state_change = true; > + > + return IRQ_WAKE_THREAD; > +} > +