Re: [PATCH 02/10] drm/ast: Move Gen7+ POST code to separate source file

[Jocelyn Falempe <jfalempe@redhat.com>]

On 02/07/2025 15:12, Thomas Zimmermann wrote:
> Move POST code for Gen7+ to separate source file and hide it in
> ast_2600_post(). There's not much going on here except for enabling
> the DP transmitter chip.

Thanks, it looks good to me.

Only a cosmetic comment below.

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/ast/Makefile   |  1 +
>   drivers/gpu/drm/ast/ast_2600.c | 46 ++++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/ast/ast_drv.h  |  3 +++
>   drivers/gpu/drm/ast/ast_post.c |  8 +++---
>   4 files changed, 53 insertions(+), 5 deletions(-)
>   create mode 100644 drivers/gpu/drm/ast/ast_2600.c
> 
> diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
> index 8d09ba5d5889..0f09e0fa741b 100644
> --- a/drivers/gpu/drm/ast/Makefile
> +++ b/drivers/gpu/drm/ast/Makefile
> @@ -4,6 +4,7 @@
>   # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>   
>   ast-y := \
> +	ast_2600.o \
>   	ast_cursor.o \
>   	ast_ddc.o \
>   	ast_dp501.o \
> diff --git a/drivers/gpu/drm/ast/ast_2600.c b/drivers/gpu/drm/ast/ast_2600.c
> new file mode 100644
> index 000000000000..556571efa0b2
> --- /dev/null
> +++ b/drivers/gpu/drm/ast/ast_2600.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2012 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + */
> +/*
> + * Authors: Dave Airlie <airlied@redhat.com>
> + */
> +
> +#include "ast_drv.h"
> +
> +/*
> + * POST
> + */
> +
> +int ast_2600_post(struct ast_device *ast)
> +{
> +	int ret;
> +
> +	if (ast->tx_chip == AST_TX_ASTDP) {
> +		ret = ast_dp_launch(ast);
> +		if (ret)
> +			return ret;

That can be simplified, as the ret variable isn't useful.

if (ast->tx_chip == AST_TX_ASTDP)
	return ast_dp_launch(ast0);

> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 2ee402096cd9..570c2fe98b58 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -417,6 +417,9 @@ struct ast_crtc_state {
>   
>   int ast_mm_init(struct ast_device *ast);
>   
> +/* ast_2600.c */
> +int ast_2600_post(struct ast_device *ast);
> +
>   /* ast post */
>   int ast_post_gpu(struct ast_device *ast);
>   u32 ast_mindwm(struct ast_device *ast, u32 r);
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 36542d266f9c..03a7367bdc71 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -348,11 +348,9 @@ int ast_post_gpu(struct ast_device *ast)
>   	ast_set_def_ext_reg(ast);
>   
>   	if (AST_GEN(ast) >= 7) {
> -		if (ast->tx_chip == AST_TX_ASTDP) {
> -			ret = ast_dp_launch(ast);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = ast_2600_post(ast);
> +		if (ret)
> +			return ret;
>   	} else if (AST_GEN(ast) >= 6) {
>   		if (ast->config_mode == ast_use_p2a) {
>   			ast_post_chip_2500(ast);