[06/10] drm/ast: dp501: Avoid upcasting to struct ast_device

Commit Message

Thomas Zimmermann Sept. 11, 2024, 11:51 a.m. UTC

Several functions receive an instance of struct drm_device only to
upcast it to struct ast_device. Improve type safety by passing the
AST device directly.

Signed-off-by: Thomas Zimmermann <[e-mail]>
---
 drivers/gpu/drm/ast/ast_dp501.c | 53 ++++++++++++++-------------------
 drivers/gpu/drm/ast/ast_drv.h   |  4 +--
 drivers/gpu/drm/ast/ast_main.c  |  2 +-
 drivers/gpu/drm/ast/ast_post.c  |  2 +-
 4 files changed, 27 insertions(+), 34 deletions(-)

Comments

Jocelyn Falempe Sept. 12, 2024, 1:48 p.m. UTC | #1

On 11/09/2024 13:51, Thomas Zimmermann wrote:
> Several functions receive an instance of struct drm_device only to
> upcast it to struct ast_device. Improve type safety by passing the
> AST device directly.
> 
Thanks, it looks good to me. One small comment below

Reviewed-by: Jocelyn Falempe <[e-mail]>

> Signed-off-by: Thomas Zimmermann <[e-mail]>
> ---
>   drivers/gpu/drm/ast/ast_dp501.c | 53 ++++++++++++++-------------------
>   drivers/gpu/drm/ast/ast_drv.h   |  4 +--
>   drivers/gpu/drm/ast/ast_main.c  |  2 +-
>   drivers/gpu/drm/ast/ast_post.c  |  2 +-
>   4 files changed, 27 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index abb03d14c338..e5553334bfde 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -21,9 +21,9 @@ static void ast_release_firmware(void *data)
>   	ast->dp501_fw = NULL;
>   }
>   
> -static int ast_load_dp501_microcode(struct drm_device *dev)
> +static int ast_load_dp501_microcode(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
> +	struct drm_device *dev = &ast->base;
>   	int ret;
>   
>   	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
> @@ -109,10 +109,10 @@ static bool wait_fw_ready(struct ast_device *ast)
>   }
>   #endif
>   
> -static bool ast_write_cmd(struct drm_device *dev, u8 data)
> +static bool ast_write_cmd(struct ast_device *ast, u8 data)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	int retry = 0;
> +
>   	if (wait_nack(ast)) {
>   		send_nack(ast);
>   		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0x9a, 0x00, data);
> @@ -131,10 +131,8 @@ static bool ast_write_cmd(struct drm_device *dev, u8 data)
>   	return false;
>   }
>   
> -static bool ast_write_data(struct drm_device *dev, u8 data)
> +static bool ast_write_data(struct ast_device *ast, u8 data)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
> -
>   	if (wait_nack(ast)) {
>   		send_nack(ast);
>   		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0x9a, 0x00, data);
> @@ -175,10 +173,10 @@ static void clear_cmd(struct ast_device *ast)
>   }
>   #endif
>   
> -static void ast_set_dp501_video_output(struct drm_device *dev, u8 mode)
> +static void ast_set_dp501_video_output(struct ast_device *ast, u8 mode)
>   {
> -	ast_write_cmd(dev, 0x40);
> -	ast_write_data(dev, mode);
> +	ast_write_cmd(ast, 0x40);
> +	ast_write_data(ast, mode);
>   
>   	msleep(10);
>   }
> @@ -188,9 +186,8 @@ static u32 get_fw_base(struct ast_device *ast)
>   	return ast_mindwm(ast, 0x1e6e2104) & 0x7fffffff;
>   }
>   
> -bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size)
> +bool ast_backup_fw(struct ast_device *ast, u8 *addr, u32 size)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	u32 i, data;
>   	u32 boot_address;
>   
> @@ -207,9 +204,8 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size)
>   	return false;
>   }
>   
> -static bool ast_launch_m68k(struct drm_device *dev)
> +static bool ast_launch_m68k(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	u32 i, data, len = 0;
>   	u32 boot_address;
>   	u8 *fw_addr = NULL;
> @@ -226,7 +222,7 @@ static bool ast_launch_m68k(struct drm_device *dev)
>   			len = 32*1024;
>   		} else {
>   			if (!ast->dp501_fw &&
> -			    ast_load_dp501_microcode(dev) < 0)
> +			    ast_load_dp501_microcode(ast) < 0)
>   				return false;
>   
>   			fw_addr = (u8 *)ast->dp501_fw->data;
> @@ -348,9 +344,8 @@ static int ast_dp512_read_edid_block(void *data, u8 *buf, unsigned int block, si
>   	return true;
>   }
>   
> -static bool ast_init_dvo(struct drm_device *dev)
> +static bool ast_init_dvo(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	u8 jreg;
>   	u32 data;
>   	ast_write32(ast, 0xf004, 0x1e6e0000);
> @@ -421,9 +416,8 @@ static bool ast_init_dvo(struct drm_device *dev)
>   }
>   
>   
> -static void ast_init_analog(struct drm_device *dev)
> +static void ast_init_analog(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	u32 data;
>   
>   	/*
> @@ -448,28 +442,27 @@ static void ast_init_analog(struct drm_device *dev)
>   	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xa3, 0xcf, 0x00);
>   }
>   
> -void ast_init_3rdtx(struct drm_device *dev)
> +void ast_init_3rdtx(struct ast_device *ast)
>   {
> -	struct ast_device *ast = to_ast_device(dev);
>   	u8 jreg;
>   
>   	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
>   		jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
>   		switch (jreg & 0x0e) {
>   		case 0x04:
> -			ast_init_dvo(dev);
> +			ast_init_dvo(ast);
>   			break;
>   		case 0x08:
> -			ast_launch_m68k(dev);
> +			ast_launch_m68k(ast);
>   			break;
>   		case 0x0c:
> -			ast_init_dvo(dev);
> +			ast_init_dvo(ast);
>   			break;
>   		default:
>   			if (ast->tx_chip == AST_TX_SIL164)
> -				ast_init_dvo(dev);
> +				ast_init_dvo(ast);
>   			else
> -				ast_init_analog(dev);
> +				ast_init_analog(ast);
>   		}
>   	}
>   }
> @@ -485,17 +478,17 @@ static const struct drm_encoder_funcs ast_dp501_encoder_funcs = {
>   static void ast_dp501_encoder_helper_atomic_enable(struct drm_encoder *encoder,
>   						   struct drm_atomic_state *state)
>   {
> -	struct drm_device *dev = encoder->dev;
> +	struct ast_device *ast = to_ast_device(encoder->dev);
>   
> -	ast_set_dp501_video_output(dev, 1);
> +	ast_set_dp501_video_output(ast, 1);
>   }
>   
>   static void ast_dp501_encoder_helper_atomic_disable(struct drm_encoder *encoder,
>   						    struct drm_atomic_state *state)
>   {
> -	struct drm_device *dev = encoder->dev;
> +	struct ast_device *ast = to_ast_device(encoder->dev);
>   
> -	ast_set_dp501_video_output(dev, 0);
> +	ast_set_dp501_video_output(ast, 0);
>   }
>   
>   static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = {
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index cafc4234e839..e29db59bb7d8 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -455,8 +455,8 @@ int ast_vga_output_init(struct ast_device *ast);
>   int ast_sil164_output_init(struct ast_device *ast);
>   
>   /* ast dp501 */
> -bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
> -void ast_init_3rdtx(struct drm_device *dev);
> +bool ast_backup_fw(struct ast_device *ast, u8 *addr, u32 size);
> +void ast_init_3rdtx(struct ast_device *ast);
>   int ast_dp501_output_init(struct ast_device *ast);
>   
>   /* aspeed DP */
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index d7d503e78e25..3aeb0f4b19d5 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -110,7 +110,7 @@ static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
>   			ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
>   			if (ast->dp501_fw_addr) {
>   				/* backup firmware */
> -				if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
> +				if (ast_backup_fw(ast, ast->dp501_fw_addr, 32*1024)) {

Maybe we can remove the fw_addr parameter of ast_backup_fw(), it's the 
only call to this function.

>   					drmm_kfree(dev, ast->dp501_fw_addr);
>   					ast->dp501_fw_addr = NULL;
>   				}
> diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
> index 902bf8114b6e..324778c72d23 100644
> --- a/drivers/gpu/drm/ast/ast_post.c
> +++ b/drivers/gpu/drm/ast/ast_post.c
> @@ -360,7 +360,7 @@ void ast_post_gpu(struct drm_device *dev)
>   		else
>   			ast_init_dram_reg(dev);
>   
> -		ast_init_3rdtx(dev);
> +		ast_init_3rdtx(ast);
>   	} else {
>   		if (ast->tx_chip == AST_TX_SIL164)
>   			ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xa3, 0xcf, 0x80);	/* Enable DVO */

Thomas Zimmermann Sept. 13, 2024, 8:47 a.m. UTC | #2

Hi

Am 12.09.24 um 15:48 schrieb Jocelyn Falempe:
[...]
>> --- a/drivers/gpu/drm/ast/ast_main.c
>> +++ b/drivers/gpu/drm/ast/ast_main.c
>> @@ -110,7 +110,7 @@ static void ast_detect_tx_chip(struct ast_device 
>> *ast, bool need_post)
>>               ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, 
>> GFP_KERNEL);
>>               if (ast->dp501_fw_addr) {
>>                   /* backup firmware */
>> -                if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
>> +                if (ast_backup_fw(ast, ast->dp501_fw_addr, 32*1024)) {
>
> Maybe we can remove the fw_addr parameter of ast_backup_fw(), it's the 
> only call to this function.

I don't mind, but it belongs in a separate patchset. After doing 
something about the odd TX chips that we talked about, I intent to 
rework the TX- and GPU-POST code.

Best regards
Thomas

>
>>                       drmm_kfree(dev, ast->dp501_fw_addr);
>>                       ast->dp501_fw_addr = NULL;
>>                   }
>> diff --git a/drivers/gpu/drm/ast/ast_post.c 
>> b/drivers/gpu/drm/ast/ast_post.c
>> index 902bf8114b6e..324778c72d23 100644
>> --- a/drivers/gpu/drm/ast/ast_post.c
>> +++ b/drivers/gpu/drm/ast/ast_post.c
>> @@ -360,7 +360,7 @@ void ast_post_gpu(struct drm_device *dev)
>>           else
>>               ast_init_dram_reg(dev);
>>   -        ast_init_3rdtx(dev);
>> +        ast_init_3rdtx(ast);
>>       } else {
>>           if (ast->tx_chip == AST_TX_SIL164)
>>               ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xa3, 0xcf, 
>> 0x80);    /* Enable DVO */
>

Patch

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index abb03d14c338..e5553334bfde 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -21,9 +21,9 @@  static void ast_release_firmware(void *data)
 	ast->dp501_fw = NULL;
 }
 
-static int ast_load_dp501_microcode(struct drm_device *dev)
+static int ast_load_dp501_microcode(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
+	struct drm_device *dev = &ast->base;
 	int ret;
 
 	ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev);
@@ -109,10 +109,10 @@  static bool wait_fw_ready(struct ast_device *ast)
 }
 #endif
 
-static bool ast_write_cmd(struct drm_device *dev, u8 data)
+static bool ast_write_cmd(struct ast_device *ast, u8 data)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	int retry = 0;
+
 	if (wait_nack(ast)) {
 		send_nack(ast);
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0x9a, 0x00, data);
@@ -131,10 +131,8 @@  static bool ast_write_cmd(struct drm_device *dev, u8 data)
 	return false;
 }
 
-static bool ast_write_data(struct drm_device *dev, u8 data)
+static bool ast_write_data(struct ast_device *ast, u8 data)
 {
-	struct ast_device *ast = to_ast_device(dev);
-
 	if (wait_nack(ast)) {
 		send_nack(ast);
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0x9a, 0x00, data);
@@ -175,10 +173,10 @@  static void clear_cmd(struct ast_device *ast)
 }
 #endif
 
-static void ast_set_dp501_video_output(struct drm_device *dev, u8 mode)
+static void ast_set_dp501_video_output(struct ast_device *ast, u8 mode)
 {
-	ast_write_cmd(dev, 0x40);
-	ast_write_data(dev, mode);
+	ast_write_cmd(ast, 0x40);
+	ast_write_data(ast, mode);
 
 	msleep(10);
 }
@@ -188,9 +186,8 @@  static u32 get_fw_base(struct ast_device *ast)
 	return ast_mindwm(ast, 0x1e6e2104) & 0x7fffffff;
 }
 
-bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size)
+bool ast_backup_fw(struct ast_device *ast, u8 *addr, u32 size)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	u32 i, data;
 	u32 boot_address;
 
@@ -207,9 +204,8 @@  bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size)
 	return false;
 }
 
-static bool ast_launch_m68k(struct drm_device *dev)
+static bool ast_launch_m68k(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	u32 i, data, len = 0;
 	u32 boot_address;
 	u8 *fw_addr = NULL;
@@ -226,7 +222,7 @@  static bool ast_launch_m68k(struct drm_device *dev)
 			len = 32*1024;
 		} else {
 			if (!ast->dp501_fw &&
-			    ast_load_dp501_microcode(dev) < 0)
+			    ast_load_dp501_microcode(ast) < 0)
 				return false;
 
 			fw_addr = (u8 *)ast->dp501_fw->data;
@@ -348,9 +344,8 @@  static int ast_dp512_read_edid_block(void *data, u8 *buf, unsigned int block, si
 	return true;
 }
 
-static bool ast_init_dvo(struct drm_device *dev)
+static bool ast_init_dvo(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	u8 jreg;
 	u32 data;
 	ast_write32(ast, 0xf004, 0x1e6e0000);
@@ -421,9 +416,8 @@  static bool ast_init_dvo(struct drm_device *dev)
 }
 
 
-static void ast_init_analog(struct drm_device *dev)
+static void ast_init_analog(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	u32 data;
 
 	/*
@@ -448,28 +442,27 @@  static void ast_init_analog(struct drm_device *dev)
 	ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xa3, 0xcf, 0x00);
 }
 
-void ast_init_3rdtx(struct drm_device *dev)
+void ast_init_3rdtx(struct ast_device *ast)
 {
-	struct ast_device *ast = to_ast_device(dev);
 	u8 jreg;
 
 	if (IS_AST_GEN4(ast) || IS_AST_GEN5(ast)) {
 		jreg = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xd1, 0xff);
 		switch (jreg & 0x0e) {
 		case 0x04:
-			ast_init_dvo(dev);
+			ast_init_dvo(ast);
 			break;
 		case 0x08:
-			ast_launch_m68k(dev);
+			ast_launch_m68k(ast);
 			break;
 		case 0x0c:
-			ast_init_dvo(dev);
+			ast_init_dvo(ast);
 			break;
 		default:
 			if (ast->tx_chip == AST_TX_SIL164)
-				ast_init_dvo(dev);
+				ast_init_dvo(ast);
 			else
-				ast_init_analog(dev);
+				ast_init_analog(ast);
 		}
 	}
 }
@@ -485,17 +478,17 @@  static const struct drm_encoder_funcs ast_dp501_encoder_funcs = {
 static void ast_dp501_encoder_helper_atomic_enable(struct drm_encoder *encoder,
 						   struct drm_atomic_state *state)
 {
-	struct drm_device *dev = encoder->dev;
+	struct ast_device *ast = to_ast_device(encoder->dev);
 
-	ast_set_dp501_video_output(dev, 1);
+	ast_set_dp501_video_output(ast, 1);
 }
 
 static void ast_dp501_encoder_helper_atomic_disable(struct drm_encoder *encoder,
 						    struct drm_atomic_state *state)
 {
-	struct drm_device *dev = encoder->dev;
+	struct ast_device *ast = to_ast_device(encoder->dev);
 
-	ast_set_dp501_video_output(dev, 0);
+	ast_set_dp501_video_output(ast, 0);
 }
 
 static const struct drm_encoder_helper_funcs ast_dp501_encoder_helper_funcs = {
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index cafc4234e839..e29db59bb7d8 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -455,8 +455,8 @@  int ast_vga_output_init(struct ast_device *ast);
 int ast_sil164_output_init(struct ast_device *ast);
 
 /* ast dp501 */
-bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size);
-void ast_init_3rdtx(struct drm_device *dev);
+bool ast_backup_fw(struct ast_device *ast, u8 *addr, u32 size);
+void ast_init_3rdtx(struct ast_device *ast);
 int ast_dp501_output_init(struct ast_device *ast);
 
 /* aspeed DP */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index d7d503e78e25..3aeb0f4b19d5 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -110,7 +110,7 @@  static void ast_detect_tx_chip(struct ast_device *ast, bool need_post)
 			ast->dp501_fw_addr = drmm_kzalloc(dev, 32*1024, GFP_KERNEL);
 			if (ast->dp501_fw_addr) {
 				/* backup firmware */
-				if (ast_backup_fw(dev, ast->dp501_fw_addr, 32*1024)) {
+				if (ast_backup_fw(ast, ast->dp501_fw_addr, 32*1024)) {
 					drmm_kfree(dev, ast->dp501_fw_addr);
 					ast->dp501_fw_addr = NULL;
 				}
diff --git a/drivers/gpu/drm/ast/ast_post.c b/drivers/gpu/drm/ast/ast_post.c
index 902bf8114b6e..324778c72d23 100644
--- a/drivers/gpu/drm/ast/ast_post.c
+++ b/drivers/gpu/drm/ast/ast_post.c
@@ -360,7 +360,7 @@  void ast_post_gpu(struct drm_device *dev)
 		else
 			ast_init_dram_reg(dev);
 
-		ast_init_3rdtx(dev);
+		ast_init_3rdtx(ast);
 	} else {
 		if (ast->tx_chip == AST_TX_SIL164)
 			ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xa3, 0xcf, 0x80);	/* Enable DVO */