Re: [PATCH 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 1 potential issue(s) to consider:
- [Medium] Incorrect mask definitions for HPD interrupt thresholds causing them to evaluate to 0 and conceptually overlap with debounce thresholds.
--

commit 7214d6f29b496392f70fbd0b4f1b03755f68eecb
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

This commit clarifies the meaning of the sub-fields in the crystal frequency
and transmitter interrupt debounce registers. The goal is to increase human
readability.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 605ead7fa9bf6..b861a5df93167 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c

[ ... ]

> @@ -1248,28 +1254,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]
Will this result in setting the interrupt debounce thresholds to 0?

Because HPD_INT_LOW_THD_DP_TRANS_P0_MASK and HPD_INT_HIGH_THD_DP_TRANS_P0_MASK
are defined as GENMASK(1, 0) and GENMASK(3, 2) instead of their absolute
positions within HPD_INT_THD_DP_TRANS_P0_MASK (GENMASK(7, 4)), the values
produced by FIELD_PREP_CONST fall outside the update mask. 

The combined value evaluates to 0xA (bits 3:0), which means the 
regmap_update_bits() call will silently clear bits 4 through 7 instead of 
setting them.

[ ... ]

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp_reg.h b/drivers/gpu/drm/mediatek/mtk_dp_reg.h
> index 9caf743f4fe61..418a2318c656e 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(3, 2)
> +#define HPD_INT_LOW_THD_DP_TRANS_P0_MASK			GENMASK(1, 0)

[Severity: Medium]
Are these masks defined at the wrong bit positions?

They appear to overlap with HPD_DEB_THD_DP_TRANS_P0_MASK (bits 3:0). If they
are meant to be sub-fields of HPD_INT_THD_DP_TRANS_P0_MASK (bits 7:4), should
they be defined as GENMASK(7, 6) and GENMASK(5, 4) instead so they work
correctly with FIELD_PREP_CONST()?

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