| Message ID | 20200728074425.2749-12-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Convert to managed initialization | expand |
On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote: > The ast driver loads firmware for the DP501 display encoder. The > patch replaces the removal code with a managed release function. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Hm a devm_request_firmware which does exactly this would be nice I think. Maybe as a follow-up refactor? -Daniel > --- > drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- > drivers/gpu/drm/ast/ast_drv.h | 1 - > drivers/gpu/drm/ast/ast_main.c | 3 --- > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > index 4b85a504825a..88121c0e0d05 100644 > --- a/drivers/gpu/drm/ast/ast_dp501.c > +++ b/drivers/gpu/drm/ast/ast_dp501.c > @@ -8,11 +8,24 @@ > > MODULE_FIRMWARE("ast_dp501_fw.bin"); > > +static void ast_release_firmware(void *data) > +{ > + struct ast_private *ast = data; > + > + release_firmware(ast->dp501_fw); > + ast->dp501_fw = NULL; > +} > + > static int ast_load_dp501_microcode(struct drm_device *dev) > { > struct ast_private *ast = to_ast_private(dev); > + int ret; > + > + ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); > + if (ret) > + return ret; > > - return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); > + return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); > } > > static void send_ack(struct ast_private *ast) > @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) > } > } > } > - > -void ast_release_firmware(struct drm_device *dev) > -{ > - struct ast_private *ast = to_ast_private(dev); > - > - release_firmware(ast->dp501_fw); > - ast->dp501_fw = NULL; > -} > diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h > index 86c9a7ac712b..02908d005b99 100644 > --- a/drivers/gpu/drm/ast/ast_drv.h > +++ b/drivers/gpu/drm/ast/ast_drv.h > @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); > bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); > u8 ast_get_dp501_max_clk(struct drm_device *dev); > void ast_init_3rdtx(struct drm_device *dev); > -void ast_release_firmware(struct drm_device *dev); > > /* ast_cursor.c */ > int ast_cursor_init(struct ast_private *ast); > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c > index 792fb7f616ec..e3b7748335a3 100644 > --- a/drivers/gpu/drm/ast/ast_main.c > +++ b/drivers/gpu/drm/ast/ast_main.c > @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv, > > void ast_device_destroy(struct ast_private *ast) > { > - struct drm_device *dev = &ast->base; > - > /* enable standard VGA decode */ > ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); > > - ast_release_firmware(dev); > kfree(ast->dp501_fw_addr); > } > -- > 2.27.0 >
Hi Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch: > On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote: >> The ast driver loads firmware for the DP501 display encoder. The >> patch replaces the removal code with a managed release function. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Hm a devm_request_firmware which does exactly this would be nice I think. > Maybe as a follow-up refactor? There are so many ideas for follow-up patches wrt. devres and drmres, we should add a todo item to collect them. Especially, devres is much more over head in terms of reviews and kernel building/testing tha tit makes sense to collect ideas and address them in larger chunks. Best regards Thomas > -Daniel > >> --- >> drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- >> drivers/gpu/drm/ast/ast_drv.h | 1 - >> drivers/gpu/drm/ast/ast_main.c | 3 --- >> 3 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c >> index 4b85a504825a..88121c0e0d05 100644 >> --- a/drivers/gpu/drm/ast/ast_dp501.c >> +++ b/drivers/gpu/drm/ast/ast_dp501.c >> @@ -8,11 +8,24 @@ >> >> MODULE_FIRMWARE("ast_dp501_fw.bin"); >> >> +static void ast_release_firmware(void *data) >> +{ >> + struct ast_private *ast = data; >> + >> + release_firmware(ast->dp501_fw); >> + ast->dp501_fw = NULL; >> +} >> + >> static int ast_load_dp501_microcode(struct drm_device *dev) >> { >> struct ast_private *ast = to_ast_private(dev); >> + int ret; >> + >> + ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); >> + if (ret) >> + return ret; >> >> - return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); >> + return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); >> } >> >> static void send_ack(struct ast_private *ast) >> @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) >> } >> } >> } >> - >> -void ast_release_firmware(struct drm_device *dev) >> -{ >> - struct ast_private *ast = to_ast_private(dev); >> - >> - release_firmware(ast->dp501_fw); >> - ast->dp501_fw = NULL; >> -} >> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h >> index 86c9a7ac712b..02908d005b99 100644 >> --- a/drivers/gpu/drm/ast/ast_drv.h >> +++ b/drivers/gpu/drm/ast/ast_drv.h >> @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); >> bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); >> u8 ast_get_dp501_max_clk(struct drm_device *dev); >> void ast_init_3rdtx(struct drm_device *dev); >> -void ast_release_firmware(struct drm_device *dev); >> >> /* ast_cursor.c */ >> int ast_cursor_init(struct ast_private *ast); >> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c >> index 792fb7f616ec..e3b7748335a3 100644 >> --- a/drivers/gpu/drm/ast/ast_main.c >> +++ b/drivers/gpu/drm/ast/ast_main.c >> @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv, >> >> void ast_device_destroy(struct ast_private *ast) >> { >> - struct drm_device *dev = &ast->base; >> - >> /* enable standard VGA decode */ >> ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); >> >> - ast_release_firmware(dev); >> kfree(ast->dp501_fw_addr); >> } >> -- >> 2.27.0 >> >
On Tue, Jul 28, 2020 at 11:32:04AM +0200, Thomas Zimmermann wrote: > Hi > > Am 28.07.20 um 11:17 schrieb daniel@ffwll.ch: > > On Tue, Jul 28, 2020 at 09:44:23AM +0200, Thomas Zimmermann wrote: > >> The ast driver loads firmware for the DP501 display encoder. The > >> patch replaces the removal code with a managed release function. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > > Hm a devm_request_firmware which does exactly this would be nice I think. > > Maybe as a follow-up refactor? > > There are so many ideas for follow-up patches wrt. devres and drmres, we > should add a todo item to collect them. Especially, devres is much more > over head in terms of reviews and kernel building/testing tha tit makes > sense to collect ideas and address them in larger chunks. Yeah maybe a section with wanted devres functions in todo.rst makes sense. For devres it depends which subsystem you're dealing with I guess, and how much they want to see before it lands. -Daniel > > Best regards > Thomas > > > -Daniel > > > >> --- > >> drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- > >> drivers/gpu/drm/ast/ast_drv.h | 1 - > >> drivers/gpu/drm/ast/ast_main.c | 3 --- > >> 3 files changed, 14 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c > >> index 4b85a504825a..88121c0e0d05 100644 > >> --- a/drivers/gpu/drm/ast/ast_dp501.c > >> +++ b/drivers/gpu/drm/ast/ast_dp501.c > >> @@ -8,11 +8,24 @@ > >> > >> MODULE_FIRMWARE("ast_dp501_fw.bin"); > >> > >> +static void ast_release_firmware(void *data) > >> +{ > >> + struct ast_private *ast = data; > >> + > >> + release_firmware(ast->dp501_fw); > >> + ast->dp501_fw = NULL; > >> +} > >> + > >> static int ast_load_dp501_microcode(struct drm_device *dev) > >> { > >> struct ast_private *ast = to_ast_private(dev); > >> + int ret; > >> + > >> + ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); > >> + if (ret) > >> + return ret; > >> > >> - return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); > >> + return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); > >> } > >> > >> static void send_ack(struct ast_private *ast) > >> @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) > >> } > >> } > >> } > >> - > >> -void ast_release_firmware(struct drm_device *dev) > >> -{ > >> - struct ast_private *ast = to_ast_private(dev); > >> - > >> - release_firmware(ast->dp501_fw); > >> - ast->dp501_fw = NULL; > >> -} > >> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h > >> index 86c9a7ac712b..02908d005b99 100644 > >> --- a/drivers/gpu/drm/ast/ast_drv.h > >> +++ b/drivers/gpu/drm/ast/ast_drv.h > >> @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); > >> bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); > >> u8 ast_get_dp501_max_clk(struct drm_device *dev); > >> void ast_init_3rdtx(struct drm_device *dev); > >> -void ast_release_firmware(struct drm_device *dev); > >> > >> /* ast_cursor.c */ > >> int ast_cursor_init(struct ast_private *ast); > >> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c > >> index 792fb7f616ec..e3b7748335a3 100644 > >> --- a/drivers/gpu/drm/ast/ast_main.c > >> +++ b/drivers/gpu/drm/ast/ast_main.c > >> @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv, > >> > >> void ast_device_destroy(struct ast_private *ast) > >> { > >> - struct drm_device *dev = &ast->base; > >> - > >> /* enable standard VGA decode */ > >> ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); > >> > >> - ast_release_firmware(dev); > >> kfree(ast->dp501_fw_addr); > >> } > >> -- > >> 2.27.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4b85a504825a..88121c0e0d05 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -8,11 +8,24 @@ MODULE_FIRMWARE("ast_dp501_fw.bin"); +static void ast_release_firmware(void *data) +{ + struct ast_private *ast = data; + + release_firmware(ast->dp501_fw); + ast->dp501_fw = NULL; +} + static int ast_load_dp501_microcode(struct drm_device *dev) { struct ast_private *ast = to_ast_private(dev); + int ret; + + ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); + if (ret) + return ret; - return request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); + return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); } static void send_ack(struct ast_private *ast) @@ -435,11 +448,3 @@ void ast_init_3rdtx(struct drm_device *dev) } } } - -void ast_release_firmware(struct drm_device *dev) -{ - struct ast_private *ast = to_ast_private(dev); - - release_firmware(ast->dp501_fw); - ast->dp501_fw = NULL; -} diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 86c9a7ac712b..02908d005b99 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -312,7 +312,6 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size); bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata); u8 ast_get_dp501_max_clk(struct drm_device *dev); void ast_init_3rdtx(struct drm_device *dev); -void ast_release_firmware(struct drm_device *dev); /* ast_cursor.c */ int ast_cursor_init(struct ast_private *ast); diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 792fb7f616ec..e3b7748335a3 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -442,11 +442,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv, void ast_device_destroy(struct ast_private *ast) { - struct drm_device *dev = &ast->base; - /* enable standard VGA decode */ ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04); - ast_release_firmware(dev); kfree(ast->dp501_fw_addr); }
The ast driver loads firmware for the DP501 display encoder. The patch replaces the removal code with a managed release function. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/ast/ast_dp501.c | 23 ++++++++++++++--------- drivers/gpu/drm/ast/ast_drv.h | 1 - drivers/gpu/drm/ast/ast_main.c | 3 --- 3 files changed, 14 insertions(+), 13 deletions(-)