Re: [PATCH 2/4] drm/i915/display: harden shifts in ICL_DPCLKA_CFGCR0_DDI_CLK_SEL macros

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 19 May 2026, Luca Coelho <luciano.coelho@intel.com> wrote:
> ICL_DPCLKA_CFGCR0 has a 2-bit DDI_CLK_SEL field per combo PHY, for
> PHY_A..PHY_D only.  Any other phy value (PHY_NONE, Type-C/SNPS PHYs)
> is not valid here.
>
> This is not a problem with the current implementation, because phy is
> always valid when these macros are called, but it's more robust to
> cast to unsigned so the shift is always well-defined.
>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_regs.h | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_regs.h b/drivers/gpu/drm/i915/display/intel_display_regs.h
> index 4321f8b529da..bc90a21e8b46 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_regs.h
> @@ -2869,9 +2869,16 @@ enum skl_power_gate {
>  #define  ICL_DPCLKA_CFGCR0_TC_CLK_OFF(tc_port)	(1 << ((tc_port) < TC_PORT_4 ? \
>  						       (tc_port) + 12 : \
>  						       (tc_port) - TC_PORT_4 + 21))
> -#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)	((phy) * 2)
> -#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy)	(3 << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> -#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll, phy)	((pll) << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> +/*
> + * ICL_DPCLKA_CFGCR0 has a 2-bit DDI_CLK_SEL field per combo PHY, for
> + * PHY_A..PHY_D only.  Any other phy value (PHY_NONE, TypeC/SNPS PHYs)
> + * is not valid here.  Cast to unsigned so the shift is always
> + * well-defined.
> + */
> +#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)	((unsigned int)(phy) * 2)
> +#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy)	((u32)3 << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> +#define  ICL_DPCLKA_CFGCR0_DDI_CLK_SEL(pll, phy)	((u32)(pll) << ICL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))
> +

Here too, please avoid direct casts; we have the REG_* helpers exactly
for this purpose.

BR,
Jani.

>  #define  RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy)	_PICK(phy, 0, 2, 4, 27)
>  #define  RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(phy) \
>  	(3 << RKL_DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(phy))

-- 
Jani Nikula, Intel