From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DC967C433EF for ; Thu, 14 Oct 2021 13:44:49 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 9FFA360FDC for ; Thu, 14 Oct 2021 13:44:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9FFA360FDC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C7B5F6E878; Thu, 14 Oct 2021 13:44:48 +0000 (UTC) Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4D4456E878 for ; Thu, 14 Oct 2021 13:44:47 +0000 (UTC) Received: from [IPv6:2a02:810a:880:f54:d539:cc83:9c4a:6738] (unknown [IPv6:2a02:810a:880:f54:d539:cc83:9c4a:6738]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dafna) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 424E11F44C6D; Thu, 14 Oct 2021 14:44:45 +0100 (BST) Subject: Re: [PATCH v7, 03/15] media: mtk-vcodec: Refactor vcodec pm interface To: Yunfei Dong , Alexandre Courbot , Hans Verkuil , Tzung-Bi Shih , Tiffany Lin , Andrew-CT Chen , Mauro Carvalho Chehab , Rob Herring , Matthias Brugger , Tomasz Figa Cc: Hsin-Yi Wang , Fritz Koenig , Benjamin Gaignard , Daniel Vetter , dri-devel , Irui Wang , linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, srv_heupstream@mediatek.com, linux-mediatek@lists.infradead.org, Project_Global_Chrome_Upstream_Group@mediatek.com, Tzung-Bi Shih References: <20211011070247.792-1-yunfei.dong@mediatek.com> <20211011070247.792-4-yunfei.dong@mediatek.com> From: Dafna Hirschfeld Message-ID: <6f3e94fe-8e02-e79d-858d-620a057b87f2@collabora.com> Date: Thu, 14 Oct 2021 15:44:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20211011070247.792-4-yunfei.dong@mediatek.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 11.10.21 09:02, Yunfei Dong wrote: > Using the needed param for pm init/release function and remove unused > param mtkdev in 'struct mtk_vcodec_pm'. > I see that there is a lot of code duplication between mtk_vcodec_release_dec_pm.c and mtk_vcodec_release_enc_pm.c I think if you bother to factor the decoder you should do the same factor to the encoder, but actually the much better thing to do is to unify all code duplication between these two files, just for example of identical functions: mtk_vcodec_enc/dec_clock_on/off mtk_vcodec_release_enc_pm mtk_vcodec_init_dec_pm In addition, the function mtk_vcodec_dec_pw_on can be remove since it only calls pm_runtime_resume_and_get. It would be much better to call pm_runtime_resume_and_get directly and not hide it in a different function Thanks, Dafna > Reviewed-by: Tzung-Bi Shih > Signed-off-by: Yunfei Dong > --- > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 ++--- > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 22 ++++++++----------- > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 5 +++-- > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 1 - > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 1 - > 5 files changed, 15 insertions(+), 20 deletions(-) > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > index 8db9cdc66043..dd749d41c75a 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > @@ -249,7 +249,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > if (IS_ERR(dev->fw_handler)) > return PTR_ERR(dev->fw_handler); > > - ret = mtk_vcodec_init_dec_pm(dev); > + ret = mtk_vcodec_init_dec_pm(dev->plat_dev, &dev->pm); > if (ret < 0) { > dev_err(&pdev->dev, "Failed to get mt vcodec clock source"); > goto err_dec_pm; > @@ -379,7 +379,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev) > err_dec_alloc: > v4l2_device_unregister(&dev->v4l2_dev); > err_res: > - mtk_vcodec_release_dec_pm(dev); > + mtk_vcodec_release_dec_pm(&dev->pm); > err_dec_pm: > mtk_vcodec_fw_release(dev->fw_handler); > return ret; > @@ -422,7 +422,7 @@ static int mtk_vcodec_dec_remove(struct platform_device *pdev) > video_unregister_device(dev->vfd_dec); > > v4l2_device_unregister(&dev->v4l2_dev); > - mtk_vcodec_release_dec_pm(dev); > + mtk_vcodec_release_dec_pm(&dev->pm); > mtk_vcodec_fw_release(dev->fw_handler); > return 0; > } > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c > index 6038db96f71c..20bd157a855c 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c > @@ -13,18 +13,15 @@ > #include "mtk_vcodec_dec_pm.h" > #include "mtk_vcodec_util.h" > > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev) > +int mtk_vcodec_init_dec_pm(struct platform_device *pdev, > + struct mtk_vcodec_pm *pm) > { > struct device_node *node; > - struct platform_device *pdev; > - struct mtk_vcodec_pm *pm; > + struct platform_device *larb_pdev; > struct mtk_vcodec_clk *dec_clk; > struct mtk_vcodec_clk_info *clk_info; > int i = 0, ret = 0; > > - pdev = mtkdev->plat_dev; > - pm = &mtkdev->pm; > - pm->mtkdev = mtkdev; > dec_clk = &pm->vdec_clk; > node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0); > if (!node) { > @@ -32,13 +29,12 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev) > return -1; > } > > - pdev = of_find_device_by_node(node); > + larb_pdev = of_find_device_by_node(node); > of_node_put(node); > - if (WARN_ON(!pdev)) { > + if (WARN_ON(!larb_pdev)) { > return -1; > } > - pm->larbvdec = &pdev->dev; > - pdev = mtkdev->plat_dev; > + pm->larbvdec = &larb_pdev->dev; > pm->dev = &pdev->dev; > > dec_clk->clk_num = > @@ -82,10 +78,10 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev) > return ret; > } > > -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev) > +void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm) > { > - pm_runtime_disable(dev->pm.dev); > - put_device(dev->pm.larbvdec); > + pm_runtime_disable(pm->dev); > + put_device(pm->larbvdec); > } > > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm) > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h > index 280aeaefdb65..a3df6aef6cb9 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h > @@ -9,8 +9,9 @@ > > #include "mtk_vcodec_drv.h" > > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev); > -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev); > +int mtk_vcodec_init_dec_pm(struct platform_device *pdev, > + struct mtk_vcodec_pm *pm); > +void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm); > > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm); > void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm); > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > index 3b1e5e3a450e..973b0b3649c6 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h > @@ -195,7 +195,6 @@ struct mtk_vcodec_pm { > struct mtk_vcodec_clk venc_clk; > struct device *larbvenc; > struct device *dev; > - struct mtk_vcodec_dev *mtkdev; > }; > > /** > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > index 1b2e4930ed27..0c8c8f86788c 100644 > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c > @@ -26,7 +26,6 @@ int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev) > pdev = mtkdev->plat_dev; > pm = &mtkdev->pm; > memset(pm, 0, sizeof(struct mtk_vcodec_pm)); > - pm->mtkdev = mtkdev; > pm->dev = &pdev->dev; > dev = &pdev->dev; > enc_clk = &pm->venc_clk; >