Re: [PATCH v2 11/11] drm/mediatek: mtk_dp: Clarify XTAL freq and Debounce registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Undocumented functional hardware configuration change fixing a pre-existing double-shift bug
- [Low] Unused legacy macros left in the header file
--

--- Patch [11]: [PATCH v2 11/11] drm/mediatek: mtk_dp: Clarify XTAL freq and Debounce registers ---
commit 6c2235134edb8d6bc77df71eedc7aaac802ae5fa
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Clarify XTAL freq and Debounce registers

This commit updates the construction of crystal frequency and transmitter
interrupt debounce register values to use FIELD_PREP_CONST() with new
mask definitions. The commit message states the only purpose is to increase
human readability.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 868d918a0b161..4403a25784d4e 100644
> --- 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)
>  {
[ ... ]
> -	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]
Is the commit message description accurate regarding the "only purpose of
increasing human readability"?

This patch appears to introduce a functional hardware configuration change.

In the previous code, HPD_INT_THD_DP_TRANS_P0_LOWER_500US and
HPD_INT_THD_DP_TRANS_P0_UPPER_1100US were defined with shifts of 4 and 6,
and then shifted again by 4 in mtk_dp_initialize_hpd_detect_settings().

This resulted in the value being completely masked out by
HPD_INT_THD_DP_TRANS_P0_MASK, writing 0 to the register.

The new FIELD_PREP_CONST implementation correctly constructs the value 0xA0
and writes it to the hardware.

Since this silently fixes a pre-existing double-shift bug and alters the
actual hardware behavior, should this functional change be explicitly
documented or split into a dedicated fix patch?

> +
> +	/* Connection and Disconnection thresholds: 1.5ms + (5 x 0.1) ms = 2ms */
> +	mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3410,
> +			   FIELD_PREP_CONST(HPD_DISC_THD_DP_TRANS_P0_MASK, 5) |
> +			   FIELD_PREP_CONST(HPD_CONN_THD_DP_TRANS_P0_MASK, 5),
>  			   HPD_DISC_THD_DP_TRANS_P0_MASK |
>  			   HPD_CONN_THD_DP_TRANS_P0_MASK);
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_TRANS_P0_3430,
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> index 9caf743f4fe61..ce47b99dddc43 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> @@ -253,6 +253,8 @@
>  #define MTK_DP_TRANS_P0_3410				0x1410
>  #define HPD_DEB_THD_DP_TRANS_P0_MASK				GENMASK(3, 0)
>  #define HPD_INT_THD_DP_TRANS_P0_MASK				GENMASK(7, 4)
> +#define HPD_INT_HIGH_THD_DP_TRANS_P0_MASK			GENMASK(7, 6)
> +#define HPD_INT_LOW_THD_DP_TRANS_P0_MASK			GENMASK(5, 4)
>  #define HPD_INT_THD_DP_TRANS_P0_LOWER_500US			(2 << 4)
>  #define HPD_INT_THD_DP_TRANS_P0_UPPER_1100US			(2 << 6)

[Severity: Low]
Should these old _500US and _1100US macros be removed?

They appear to be unused dead code now that the configuration in
mtk_dp_initialize_hpd_detect_settings() has been updated to use the new
masks with FIELD_PREP_CONST.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709100617.42583-1-angelogioacchino.delregno@collabora.com?part=11