| Message ID | 20230510233117.1.I7047714f92ef7569bd21f118ae6aee20b3175a92@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: vcodec: Convert mtk_vcodec_dec_hw platform remove callback | expand |
[expanding the audience a bit for more expertise] Hello, On Wed, May 10, 2023 at 11:31:35PM +0800, Fei Shao wrote: > This aligns with [1] and converts the platform remove callback to > .remove_new(), which returns void. > > [1]: commit a3afc5b10661 ("media: mtk_vcodec_dec_drv: Convert to > platform remove callback returning void") > > Signed-off-by: Fei Shao <fshao@chromium.org> > > --- > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > index b753bf54ebd9..bd5743723da6 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > @@ -193,16 +193,14 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev) > return ret; > } > > -static int mtk_vdec_hw_remove(struct platform_device *pdev) > +static void mtk_vdec_hw_remove(struct platform_device *pdev) > { > pm_runtime_disable(&pdev->dev); > - > - return 0; > } > > static struct platform_driver mtk_vdec_driver = { > .probe = mtk_vdec_hw_probe, > - .remove = mtk_vdec_hw_remove, > + .remove_new = mtk_vdec_hw_remove, > .driver = { > .name = "mtk-vdec-comp", > .of_match_table = mtk_vdec_hw_match, While the patch looks fine, I wonder if having a remove callback just to do pm_runtime_disable() is worth keeping it. Doesn't the core care for things like that? I grepped a bit around, device_unbind_cleanup() is called after device_remove() which calls pm_runtime_reinit(). Does that mean calling pm_runtime_disable in .remove() is useless? In that case, you could just drop the .remove() callback. Best regards Uwe
On Thu, May 11, 2023 at 12:43 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > [expanding the audience a bit for more expertise] > > Hello, > > On Wed, May 10, 2023 at 11:31:35PM +0800, Fei Shao wrote: > > This aligns with [1] and converts the platform remove callback to > > .remove_new(), which returns void. > > > > [1]: commit a3afc5b10661 ("media: mtk_vcodec_dec_drv: Convert to > > platform remove callback returning void") > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > --- > > > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > index b753bf54ebd9..bd5743723da6 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > @@ -193,16 +193,14 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev) > > return ret; > > } > > > > -static int mtk_vdec_hw_remove(struct platform_device *pdev) > > +static void mtk_vdec_hw_remove(struct platform_device *pdev) > > { > > pm_runtime_disable(&pdev->dev); > > - > > - return 0; > > } > > > > static struct platform_driver mtk_vdec_driver = { > > .probe = mtk_vdec_hw_probe, > > - .remove = mtk_vdec_hw_remove, > > + .remove_new = mtk_vdec_hw_remove, > > .driver = { > > .name = "mtk-vdec-comp", > > .of_match_table = mtk_vdec_hw_match, > > While the patch looks fine, I wonder if having a remove callback just to > do pm_runtime_disable() is worth keeping it. Doesn't the core care for > things like that? I grepped a bit around, device_unbind_cleanup() is > called after device_remove() which calls pm_runtime_reinit(). Does that > mean calling pm_runtime_disable in .remove() is useless? In that case, > you could just drop the .remove() callback. Maybe just switch to devm_pm_runtime_enable() on the enable side? ChenYu
Hi, On Thu, May 11, 2023 at 1:50 PM Chen-Yu Tsai <wenst@chromium.org> wrote: > > On Thu, May 11, 2023 at 12:43 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > [expanding the audience a bit for more expertise] > > > > Hello, > > > > On Wed, May 10, 2023 at 11:31:35PM +0800, Fei Shao wrote: > > > This aligns with [1] and converts the platform remove callback to > > > .remove_new(), which returns void. > > > > > > [1]: commit a3afc5b10661 ("media: mtk_vcodec_dec_drv: Convert to > > > platform remove callback returning void") > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org> > > > > > > --- > > > > > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > > index b753bf54ebd9..bd5743723da6 100644 > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c > > > @@ -193,16 +193,14 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev) > > > return ret; > > > } > > > > > > -static int mtk_vdec_hw_remove(struct platform_device *pdev) > > > +static void mtk_vdec_hw_remove(struct platform_device *pdev) > > > { > > > pm_runtime_disable(&pdev->dev); > > > - > > > - return 0; > > > } > > > > > > static struct platform_driver mtk_vdec_driver = { > > > .probe = mtk_vdec_hw_probe, > > > - .remove = mtk_vdec_hw_remove, > > > + .remove_new = mtk_vdec_hw_remove, > > > .driver = { > > > .name = "mtk-vdec-comp", > > > .of_match_table = mtk_vdec_hw_match, > > > > While the patch looks fine, I wonder if having a remove callback just to > > do pm_runtime_disable() is worth keeping it. Doesn't the core care for > > things like that? I grepped a bit around, device_unbind_cleanup() is > > called after device_remove() which calls pm_runtime_reinit(). Does that > > mean calling pm_runtime_disable in .remove() is useless? In that case, > > you could just drop the .remove() callback. Thanks for the feedback. I wonder if the core would handle that for drivers... if I understand it correctly, pm_runtime_reinit() does not disable runtime PM for a device, otherwise pm_runtime_remove() wouldn't need to bother calling __pm_runtime_disable() before pm_runtime_reinit(). In fact, from the runtime_pm documentation [1] I read the following: "Drivers in ->remove() callback should undo the runtime PM changes done in ->probe(). Usually this means calling pm_runtime_disable(), pm_runtime_dont_use_autosuspend() etc." Based on the above I assume it's still necessary given pm_runtime_enable() was called in ->probe(). [1]: https://www.kernel.org/doc/Documentation/power/runtime_pm.txt > > Maybe just switch to devm_pm_runtime_enable() on the enable side? That sounds like a good alternative, I'll revise and send a new patch, thanks. Regards, Fei > > ChenYu
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c index b753bf54ebd9..bd5743723da6 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c @@ -193,16 +193,14 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev) return ret; } -static int mtk_vdec_hw_remove(struct platform_device *pdev) +static void mtk_vdec_hw_remove(struct platform_device *pdev) { pm_runtime_disable(&pdev->dev); - - return 0; } static struct platform_driver mtk_vdec_driver = { .probe = mtk_vdec_hw_probe, - .remove = mtk_vdec_hw_remove, + .remove_new = mtk_vdec_hw_remove, .driver = { .name = "mtk-vdec-comp", .of_match_table = mtk_vdec_hw_match,
This aligns with [1] and converts the platform remove callback to .remove_new(), which returns void. [1]: commit a3afc5b10661 ("media: mtk_vcodec_dec_drv: Convert to platform remove callback returning void") Signed-off-by: Fei Shao <fshao@chromium.org> --- drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_hw.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)