Re: [PATCH v3] drm/amd/display: remove unused cgs register accessor typedefs

[Alex Deucher]

Applied.  Thanks!

On Tue, Oct 20, 2026 at 3:37 AM Omokefe Emmanuel Onanaroghene
<emmaonana18@xxxxxxxxx> wrote:
>
> kernel-doc reports prototype mismatches for the cgs register accessor
> typedefs, e.g.:
>
>   drivers/gpu/drm/amd/include/cgs_common.h:89: expecting prototype
>   for cgs_read_register(). Prototype was for uint32_t() instead
>
> Each of the four typedefs is used exactly once, as the type of a struct
> cgs_ops member, so they are merely an unnecessary layer of indirection.
> Drop the typedefs along with their kernel-doc comments and write the
> function pointer types directly in struct cgs_ops.
>
> While at it, drop the unused cgs_handle_t and cgs_get_firmware_info
> typedefs, and document the members of struct cgs_firmware_info.
>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Link: https://lore.kernel.org/all/981f01e1-0c3b-4854-9d72-8bf22af9fc64@xxxxxxx/
> Signed-off-by: Omokefe Emmanuel Onanaroghene <emmaonana18@xxxxxxxxx>
> Tested-by: Arnaud Bonnet <abo@xxxxxxxxxxx>
> ---
> added kernel-doc comments for struct cgs_firmware_info
>
>  drivers/gpu/drm/amd/include/cgs_common.h | 74 +++++++++---------------
>  1 file changed, 26 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/include/cgs_common.h b/drivers/gpu/drm/amd/include/cgs_common.hZZ
> index f40b6a03fe63..79522e1bec4f 100644
> --- a/drivers/gpu/drm/amd/include/cgs_common.h
> +++ b/drivers/gpu/drm/amd/include/cgs_common.h
> @@ -63,6 +63,14 @@ enum cgs_ucode_id {
>
>  /**
>   * struct cgs_firmware_info - Firmware information
> + * @version: firmware header version
> + * @fw_version: firmware version reported by the IP block
> + * @feature_version: firmware feature version
> + * @image_size: firmware image size in bytes
> + * @mc_addr: GPU address of the firmware image
> + * @ucode_start_address: ucode start address (SMU firmware only)
> + * @kptr: kernel virtual address of the firmware image
> + * @is_kicker: whether this is a "kicker" (k) firmware variant
>   */
>  struct cgs_firmware_info {
>         uint16_t                version;
> @@ -78,45 +86,6 @@ struct cgs_firmware_info {
>         bool                    is_kicker;
>  };
>
> -typedef unsigned long cgs_handle_t;
> -
> -/**
> - * cgs_read_register() - Read an MMIO register
> - * @cgs_device:        opaque device handle
> - * @offset:    register offset
> - *
> - * Return:  register value
> - */
> -typedef uint32_t (*cgs_read_register_t)(struct cgs_device *cgs_device, unsigned offset);
> -
> -/**
> - * cgs_write_register() - Write an MMIO register
> - * @cgs_device:        opaque device handle
> - * @offset:    register offset
> - * @value:     register value
> - */
> -typedef void (*cgs_write_register_t)(struct cgs_device *cgs_device, unsigned offset,
> -                                    uint32_t value);
> -
> -/**
> - * cgs_read_ind_register() - Read an indirect register
> - * @cgs_device:        opaque device handle
> - * @offset:    register offset
> - *
> - * Return:  register value
> - */
> -typedef uint32_t (*cgs_read_ind_register_t)(struct cgs_device *cgs_device, enum cgs_ind_reg space,
> -                                           unsigned index);
> -
> -/**
> - * cgs_write_ind_register() - Write an indirect register
> - * @cgs_device:        opaque device handle
> - * @offset:    register offset
> - * @value:     register value
> - */
> -typedef void (*cgs_write_ind_register_t)(struct cgs_device *cgs_device, enum cgs_ind_reg space,
> -                                        unsigned index, uint32_t value);
> -
>  #define CGS_REG_FIELD_SHIFT(reg, field) reg##__##field##__SHIFT
>  #define CGS_REG_FIELD_MASK(reg, field) reg##__##field##_MASK
>
> @@ -133,18 +102,27 @@ typedef void (*cgs_write_ind_register_t)(struct cgs_device *cgs_device, enum cgs
>  #define CGS_WREG32_FIELD_IND(device, space, reg, field, val)   \
>         cgs_write_ind_register(device, space, ix##reg, (cgs_read_ind_register(device, space, ix##reg) & ~CGS_REG_FIELD_MASK(reg, field)) | (val) << CGS_REG_FIELD_SHIFT(reg, field))
>
> -typedef int (*cgs_get_firmware_info)(struct cgs_device *cgs_device,
> -                                    enum cgs_ucode_id type,
> -                                    struct cgs_firmware_info *info);
> -
> +/**
> + * struct cgs_ops - Common Graphics Services operations
> + * @read_register: read register value
> + * @write_register: write register value
> + * @read_ind_register: read indirect register value
> + * @write_ind_register: write indirect register value
> + * @get_firmware_info: get firmware information
> + */
>  struct cgs_ops {
>         /* MMIO access */
> -       cgs_read_register_t read_register;
> -       cgs_write_register_t write_register;
> -       cgs_read_ind_register_t read_ind_register;
> -       cgs_write_ind_register_t write_ind_register;
> +       uint32_t (*read_register)(struct cgs_device *cgs_device, unsigned int offset);
> +       void (*write_register)(struct cgs_device *cgs_device, unsigned int offset,
> +                              uint32_t value);
> +       uint32_t (*read_ind_register)(struct cgs_device *cgs_device, enum cgs_ind_reg space,
> +                                     unsigned int index);
> +       void (*write_ind_register)(struct cgs_device *cgs_device, enum cgs_ind_reg space,
> +                                  unsigned int index, uint32_t value);
>         /* Firmware Info */
> -       cgs_get_firmware_info get_firmware_info;
> +       int (*get_firmware_info)(struct cgs_device *cgs_device,
> +                                enum cgs_ucode_id type,
> +                                struct cgs_firmware_info *info);
>  };
>
>  struct cgs_os_ops; /* To be define in OS-specific CGS header */
> --
> 2.43.0
>