Re: [PATCH v3] drm/amd/display: Deduplicate DCN DDC register assignment

[Guilherme Ivo Bozi]

Hi,

Just a gentle ping on this.
Any feedback on v3 would be appreciated.

Thanks.


On Mon, Mar 30, 2026 at 10:52 PM Guilherme Ivo Bozi
<guilherme.bozi@xxxxxx> wrote:
>
> Several DCN generations implement identical define_ddc_registers()
> functions to assign DDC register, shift and mask pointers based on
> GPIO ID.
>
> Introduce a shared inline helper,
> dcn_define_ddc_registers_common(), and convert all DCN
> implementations to use it.
>
> This reduces duplication and improves maintainability without
> changing behavior.
>
> No functional changes intended.
>
> Signed-off-by: Guilherme Ivo Bozi <guilherme.bozi@xxxxxx>
> ---
>
> v2:
>         - Corrected type mismatch (ddc_shift - ddc_sh_mask)
>
> v3:
>         - Fix threading (previous version was not sent as reply)
>         - Squashed fix to avoid build breakage
>
>  .../display/dc/gpio/dcn20/hw_factory_dcn20.c  | 27 ++++---------
>  .../display/dc/gpio/dcn21/hw_factory_dcn21.c  | 27 ++++---------
>  .../display/dc/gpio/dcn30/hw_factory_dcn30.c  | 27 ++++---------
>  .../dc/gpio/dcn315/hw_factory_dcn315.c        | 27 ++++---------
>  .../display/dc/gpio/dcn32/hw_factory_dcn32.c  | 27 ++++---------
>  .../dc/gpio/dcn401/hw_factory_dcn401.c        | 26 ++++---------
>  .../amd/display/dc/gpio/hw_factory_dcn_ddc.h  | 39 +++++++++++++++++++
>  7 files changed, 86 insertions(+), 114 deletions(-)
>  create mode 100644 drivers/gpu/drm/amd/display/dc/gpio/hw_factory_dcn_ddc.h
>
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn20/hw_factory_dcn20.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn20/hw_factory_dcn20.c
> index e0bd0c722e00..905d14079b91 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn20/hw_factory_dcn20.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn20/hw_factory_dcn20.c
> @@ -32,6 +32,8 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
> +
>  #include "hw_factory_dcn20.h"
>
>
> @@ -182,25 +184,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn21/hw_factory_dcn21.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn21/hw_factory_dcn21.c
> index 2f57ee6deabc..f347b8c7e2b6 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn21/hw_factory_dcn21.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn21/hw_factory_dcn21.c
> @@ -32,6 +32,8 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
> +
>  #include "hw_factory_dcn21.h"
>
>  #include "dcn/dcn_2_1_0_offset.h"
> @@ -170,25 +172,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn30/hw_factory_dcn30.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn30/hw_factory_dcn30.c
> index 36a5736c58c9..25eef1ee10fe 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn30/hw_factory_dcn30.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn30/hw_factory_dcn30.c
> @@ -32,6 +32,8 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
> +
>  #include "hw_factory_dcn30.h"
>
>
> @@ -199,25 +201,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn315/hw_factory_dcn315.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn315/hw_factory_dcn315.c
> index 5feebb3b95ca..571a6f1b0cf9 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn315/hw_factory_dcn315.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn315/hw_factory_dcn315.c
> @@ -32,6 +32,8 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
> +
>  #include "hw_factory_dcn315.h"
>
>  #include "dcn/dcn_3_1_5_offset.h"
> @@ -191,25 +193,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn32/hw_factory_dcn32.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn32/hw_factory_dcn32.c
> index 985f10b39750..d6e97b246bae 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn32/hw_factory_dcn32.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn32/hw_factory_dcn32.c
> @@ -32,6 +32,8 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
> +
>  #include "hw_factory_dcn32.h"
>
>  #include "dcn/dcn_3_2_0_offset.h"
> @@ -203,25 +205,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/dcn401/hw_factory_dcn401.c b/drivers/gpu/drm/amd/display/dc/gpio/dcn401/hw_factory_dcn401.c
> index 928abca18a18..06a4d7a8a1ac 100644
> --- a/drivers/gpu/drm/amd/display/dc/gpio/dcn401/hw_factory_dcn401.c
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/dcn401/hw_factory_dcn401.c
> @@ -12,6 +12,7 @@
>  #include "../hw_hpd.h"
>  #include "../hw_generic.h"
>
> +#include "../hw_factory_dcn_ddc.h"
>
>  #include "dcn/dcn_4_1_0_offset.h"
>  #include "dcn/dcn_4_1_0_sh_mask.h"
> @@ -195,25 +196,12 @@ static void define_ddc_registers(
>                 struct hw_gpio_pin *pin,
>                 uint32_t en)
>  {
> -       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> -
> -       switch (pin->id) {
> -       case GPIO_ID_DDC_DATA:
> -               ddc->regs = &ddc_data_regs_dcn[en];
> -               ddc->base.regs = &ddc_data_regs_dcn[en].gpio;
> -               break;
> -       case GPIO_ID_DDC_CLOCK:
> -               ddc->regs = &ddc_clk_regs_dcn[en];
> -               ddc->base.regs = &ddc_clk_regs_dcn[en].gpio;
> -               break;
> -       default:
> -               ASSERT_CRITICAL(false);
> -               return;
> -       }
> -
> -       ddc->shifts = &ddc_shift[en];
> -       ddc->masks = &ddc_mask[en];
> -
> +       dcn_define_ddc_registers_common(
> +               pin, en,
> +               ddc_data_regs_dcn,
> +               ddc_clk_regs_dcn,
> +               ddc_shift,
> +               ddc_mask);
>  }
>
>  static void define_hpd_registers(struct hw_gpio_pin *pin, uint32_t en)
> diff --git a/drivers/gpu/drm/amd/display/dc/gpio/hw_factory_dcn_ddc.h b/drivers/gpu/drm/amd/display/dc/gpio/hw_factory_dcn_ddc.h
> new file mode 100644
> index 000000000000..863177cf67e8
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/dc/gpio/hw_factory_dcn_ddc.h
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright 2024 Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef __DAL_HW_FACTORY_DCN_DDC_H__
> +#define __DAL_HW_FACTORY_DCN_DDC_H__
> +
> +static inline void dcn_define_ddc_registers_common(
> +               struct hw_gpio_pin *pin,
> +               uint32_t en,
> +               const struct ddc_registers *data_regs,
> +               const struct ddc_registers *clk_regs,
> +               const struct ddc_sh_mask *shift,
> +               const struct ddc_sh_mask *mask)
> +{
> +       struct hw_ddc *ddc = HW_DDC_FROM_BASE(pin);
> +
> +       switch (pin->id) {
> +       case GPIO_ID_DDC_DATA:
> +               ddc->regs = &data_regs[en];
> +               ddc->base.regs = &data_regs[en].gpio;
> +               break;
> +
> +       case GPIO_ID_DDC_CLOCK:
> +               ddc->regs = &clk_regs[en];
> +               ddc->base.regs = &clk_regs[en].gpio;
> +               break;
> +
> +       default:
> +               ASSERT_CRITICAL(false);
> +               return;
> +       }
> +
> +       ddc->shifts = &shift[en];
> +       ddc->masks = &mask[en];
> +}
> +
> +#endif /* __DAL_HW_FACTORY_DCN_DDC_H__ */
> --
> 2.47.3
>