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

[Arnaud Bonnet]

On 10/17/26 3:00 PM, Omokefe Emmanuel Onanaroghene 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.
> 
> 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>
> ---
>  drivers/gpu/drm/amd/include/cgs_common.h | 60 +++++++-----------------
>  1 file changed, 18 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/include/cgs_common.h b/drivers/gpu/drm/amd/include/cgs_common.h
> index f40b6a03fe63..1750401c7d08 100644
> --- a/drivers/gpu/drm/amd/include/cgs_common.h
> +++ b/drivers/gpu/drm/amd/include/cgs_common.h
> @@ -80,43 +80,6 @@ struct cgs_firmware_info {
>  
>  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
>  
> @@ -137,14 +100,27 @@ typedef int (*cgs_get_firmware_info)(struct cgs_device *cgs_device,
>  				     enum cgs_ucode_id type,
>  				     struct cgs_firmware_info *info);

Hi Omokefe, you probably forgot
-typedef unsigned long cgs_handle_t;
-typedef int (*cgs_get_firmware_info)(struct cgs_device *cgs_device,
-                                    enum cgs_ucode_id type,
-                                    struct cgs_firmware_info *info);

As discussed I have built and ran some IGT on hardware for the versions
with and without these two, so you can still use the tag if you send v3:

Tested-by: Arnaud Bonnet <abo@xxxxxxxxxxx>

>  
> +/**
> + * 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 */