Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The patch silently fixes a pre-existing double-shift bug that was incorrectly writing 0 to the HPD interrupt debounce thresholds. This alters the hardware state, contradicting the commit message's claim that its 'only purpose [is] increasing human readability'. --
--- Patch [12]: [PATCH v4 12/12] drm/mediatek: mtk_dp: Clarify XTAL freq and Debounce registers --- commit 947b13de5fec9b2788067fcdae3f6c00732e0060 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dp: Clarify XTAL freq and Debounce registers This patch clarifies the meaning of the sub-fields in the crystal frequency and transmitter interrupt debounce registers for increased human readability. > Clarify the meaning of the sub-fields in the crystal frequency and > transmitter interrupt debounce registers, with the only purpose of > increasing human readability. [ ... ] > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -1260,28 +1266,29 @@ static void > mtk_dp_initialize_aux_hpd_detect_settings(struct mtk_dp *mtk_dp) > > static void mtk_dp_initialize_hpd_detect_settings(struct mtk_dp *mtk_dp) > { > - u32 val; > - > /* Mask AUX TOP interrupt, as this uses transmitter for HPD */ > mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_IRQ_MASK, > AUX_TOP_IRQ_MSK, AUX_TOP_IRQ_MSK); > > - /* Debounce threshold */ > + /* Set Hotplug debounce threshold time: xtal_period * deb_thd */ > mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410, > 8, HPD_DEB_THD_DP_TRANS_P0_MASK); > > - val = (HPD_INT_THD_DP_TRANS_P0_LOWER_500US | > - HPD_INT_THD_DP_TRANS_P0_UPPER_1100US) << 4; > - mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410, > - val, HPD_INT_THD_DP_TRANS_P0_MASK); > - > /* > - * Connect threshold 1.5ms + 5 x 0.1ms = 2ms > - * Disconnect threshold 1.5ms + 5 x 0.1ms = 2ms > + * Set interrupt debounce threshold time > + * VAL | 0 1 2 3 INT_THD > + * Low | 100 300 500 700 uS > + * High | 700 900 1100 1300 uS > */ > - val = (5 << 8) | (5 << 12); > mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410, > - val, > + FIELD_PREP_CONST(HPD_INT_LOW_THD_DP_TRANS_P0_MASK, > 2) | > + FIELD_PREP_CONST(HPD_INT_HIGH_THD_DP_TRANS_P0_MASK, > 2), > + HPD_INT_THD_DP_TRANS_P0_MASK); [Severity: Medium] Does this patch silently fix a functional bug rather than just improving readability? In the original code, the LOWER_500US and UPPER_1100US macros were already shifted by 4 and 6 respectively. Applying << 4 on top of them resulted in a double shift, making val equal to 0xA00. When that was masked with HPD_INT_THD_DP_TRANS_P0_MASK (which is 0xF0), it evaluated to 0, incorrectly programming the hardware. The new code using FIELD_PREP_CONST calculates 0xA0, correctly applying the intended 500/1100uS thresholds. Since the commit message states this has the only purpose of increasing human readability, could this functional fix be overlooked for stable backports? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
