diff mbox series

[v8,09/12] media: mtk-vcodec: Get rid of mtk_smi_larb_get/put

Message ID 20210929013719.25120-10-yong.wu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Clean up "mediatek,larb" | expand

Commit Message

Yong Wu (吴勇) Sept. 29, 2021, 1:37 a.m. UTC 
MediaTek IOMMU has already added the device_link between the consumer
and smi-larb device. If the vcodec device call the pm_runtime_get_sync,
the smi-larb's pm_runtime_get_sync also be called automatically.

CC: Tiffany Lin <tiffany.lin@mediatek.com>
CC: Irui Wang <irui.wang@mediatek.com>
Signed-off-by: Yong Wu <yong.wu@mediatek.com>
Reviewed-by: Evan Green <evgreen@chromium.org>
Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 +++-------------
 .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
 .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
 .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 44 +++----------------
 4 files changed, 10 insertions(+), 75 deletions(-)

Comments

Dafna Hirschfeld Sept. 29, 2021, 12:13 p.m. UTC | #1 
On 29.09.21 03:37, Yong Wu wrote:
> MediaTek IOMMU has already added the device_link between the consumer
> and smi-larb device. If the vcodec device call the pm_runtime_get_sync,
> the smi-larb's pm_runtime_get_sync also be called automatically.
> 
> CC: Tiffany Lin <tiffany.lin@mediatek.com>
> CC: Irui Wang <irui.wang@mediatek.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> Reviewed-by: Evan Green <evgreen@chromium.org>
> Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>   .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 +++-------------
>   .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
>   .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
>   .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 44 +++----------------
>   4 files changed, 10 insertions(+), 75 deletions(-)
> 
> 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..d0bf9aa3b29d 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> @@ -8,14 +8,12 @@
>   #include <linux/of_address.h>
>   #include <linux/of_platform.h>
>   #include <linux/pm_runtime.h>
> -#include <soc/mediatek/smi.h>
>   
>   #include "mtk_vcodec_dec_pm.h"
>   #include "mtk_vcodec_util.h"
>   
>   int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
>   {
> -	struct device_node *node;
>   	struct platform_device *pdev;
>   	struct mtk_vcodec_pm *pm;
>   	struct mtk_vcodec_clk *dec_clk;
> @@ -26,18 +24,7 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
>   	pm = &mtkdev->pm;
>   	pm->mtkdev = mtkdev;
>   	dec_clk = &pm->vdec_clk;
> -	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> -	if (!node) {
> -		mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
> -		return -1;
> -	}
>   
> -	pdev = of_find_device_by_node(node);
> -	of_node_put(node);
> -	if (WARN_ON(!pdev)) {
> -		return -1;
> -	}
> -	pm->larbvdec = &pdev->dev;
>   	pdev = mtkdev->plat_dev;
>   	pm->dev = &pdev->dev;
>   
> @@ -47,14 +34,11 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
>   		dec_clk->clk_info = devm_kcalloc(&pdev->dev,
>   			dec_clk->clk_num, sizeof(*clk_info),
>   			GFP_KERNEL);
> -		if (!dec_clk->clk_info) {
> -			ret = -ENOMEM;
> -			goto put_device;
> -		}
> +		if (!dec_clk->clk_info)
> +			return -ENOMEM;
>   	} else {
>   		mtk_v4l2_err("Failed to get vdec clock count");
> -		ret = -EINVAL;
> -		goto put_device;
> +		return -EINVAL;
>   	}
>   
>   	for (i = 0; i < dec_clk->clk_num; i++) {
> @@ -63,29 +47,24 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
>   			"clock-names", i, &clk_info->clk_name);
>   		if (ret) {
>   			mtk_v4l2_err("Failed to get clock name id = %d", i);
> -			goto put_device;
> +			return ret;
>   		}
>   		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
>   			clk_info->clk_name);
>   		if (IS_ERR(clk_info->vcodec_clk)) {
>   			mtk_v4l2_err("devm_clk_get (%d)%s fail", i,
>   				clk_info->clk_name);
> -			ret = PTR_ERR(clk_info->vcodec_clk);
> -			goto put_device;
> +			return PTR_ERR(clk_info->vcodec_clk);
>   		}
>   	}
>   
>   	pm_runtime_enable(&pdev->dev);
>   	return 0;
> -put_device:
> -	put_device(pm->larbvdec);
> -	return ret;
>   }
>   
>   void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
>   {
>   	pm_runtime_disable(dev->pm.dev);
> -	put_device(dev->pm.larbvdec);
>   }

Now that functions only do  'pm_runtime_disable(dev->pm.dev);' so it will be more
readable to remove the function mtk_vcodec_release_dec_pm
and replace with pm_runtime_disable(dev->pm.dev);
Same for the 'enc' equivalent.

Thanks,
Dafna

>   
>   int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> @@ -122,11 +101,6 @@ void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
>   		}
>   	}
>   
> -	ret = mtk_smi_larb_get(pm->larbvdec);
> -	if (ret) {
> -		mtk_v4l2_err("mtk_smi_larb_get larbvdec fail %d", ret);
> -		goto error;
> -	}>   	return;
>   
>   error:
> @@ -139,7 +113,6 @@ void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
>   	struct mtk_vcodec_clk *dec_clk = &pm->vdec_clk;
>   	int i = 0;
>   
> -	mtk_smi_larb_put(pm->larbvdec);
>   	for (i = dec_clk->clk_num - 1; i >= 0; i--)
>   		clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
>   }
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> index c6c7672fecfb..64b73dd880ce 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> @@ -189,10 +189,7 @@ struct mtk_vcodec_clk {
>    */
>   struct mtk_vcodec_pm {
>   	struct mtk_vcodec_clk	vdec_clk;
> -	struct device	*larbvdec;
> -
>   	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.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> index 416f356af363..9a1515cf862d 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> @@ -8,7 +8,6 @@
>   #include <media/v4l2-event.h>
>   #include <media/v4l2-mem2mem.h>
>   #include <media/videobuf2-dma-contig.h>
> -#include <soc/mediatek/smi.h>
>   #include <linux/pm_runtime.h>
>   
>   #include "mtk_vcodec_drv.h"
> 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..dffb190267ed 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> @@ -8,58 +8,36 @@
>   #include <linux/of_address.h>
>   #include <linux/of_platform.h>
>   #include <linux/pm_runtime.h>
> -#include <soc/mediatek/smi.h>
>   
>   #include "mtk_vcodec_enc_pm.h"
>   #include "mtk_vcodec_util.h"
>   
>   int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
>   {
> -	struct device_node *node;
>   	struct platform_device *pdev;
>   	struct mtk_vcodec_pm *pm;
>   	struct mtk_vcodec_clk *enc_clk;
>   	struct mtk_vcodec_clk_info *clk_info;
>   	int ret = 0, i = 0;
> -	struct device *dev;
>   
>   	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;
>   
> -	node = of_parse_phandle(dev->of_node, "mediatek,larb", 0);
> -	if (!node) {
> -		mtk_v4l2_err("no mediatek,larb found");
> -		return -ENODEV;
> -	}
> -	pdev = of_find_device_by_node(node);
> -	of_node_put(node);
> -	if (!pdev) {
> -		mtk_v4l2_err("no mediatek,larb device found");
> -		return -ENODEV;
> -	}
> -	pm->larbvenc = &pdev->dev;
> -	pdev = mtkdev->plat_dev;
> -	pm->dev = &pdev->dev;
> -
>   	enc_clk->clk_num = of_property_count_strings(pdev->dev.of_node,
>   		"clock-names");
>   	if (enc_clk->clk_num > 0) {
>   		enc_clk->clk_info = devm_kcalloc(&pdev->dev,
>   			enc_clk->clk_num, sizeof(*clk_info),
>   			GFP_KERNEL);
> -		if (!enc_clk->clk_info) {
> -			ret = -ENOMEM;
> -			goto put_larbvenc;
> -		}
> +		if (!enc_clk->clk_info)
> +			return -ENOMEM;
>   	} else {
>   		mtk_v4l2_err("Failed to get venc clock count");
> -		ret = -EINVAL;
> -		goto put_larbvenc;
> +		return -EINVAL;
>   	}
>   
>   	for (i = 0; i < enc_clk->clk_num; i++) {
> @@ -68,29 +46,23 @@ int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
>   			"clock-names", i, &clk_info->clk_name);
>   		if (ret) {
>   			mtk_v4l2_err("venc failed to get clk name %d", i);
> -			goto put_larbvenc;
> +			return ret;
>   		}
>   		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
>   			clk_info->clk_name);
>   		if (IS_ERR(clk_info->vcodec_clk)) {
>   			mtk_v4l2_err("venc devm_clk_get (%d)%s fail", i,
>   				clk_info->clk_name);
> -			ret = PTR_ERR(clk_info->vcodec_clk);
> -			goto put_larbvenc;
> +			return PTR_ERR(clk_info->vcodec_clk);
>   		}
>   	}
>   
>   	return 0;
> -
> -put_larbvenc:
> -	put_device(pm->larbvenc);
> -	return ret;
>   }
>   
>   void mtk_vcodec_release_enc_pm(struct mtk_vcodec_dev *mtkdev)
>   {
>   	pm_runtime_disable(mtkdev->pm.dev);
> -	put_device(mtkdev->pm.larbvenc);
>   }
>   
>   
> @@ -108,11 +80,6 @@ void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
>   		}
>   	}
>   
> -	ret = mtk_smi_larb_get(pm->larbvenc);
> -	if (ret) {
> -		mtk_v4l2_err("mtk_smi_larb_get larb3 fail %d", ret);
> -		goto clkerr;
> -	}
>   	return;
>   
>   clkerr:
> @@ -125,7 +92,6 @@ void mtk_vcodec_enc_clock_off(struct mtk_vcodec_pm *pm)
>   	struct mtk_vcodec_clk *enc_clk = &pm->venc_clk;
>   	int i = 0;
>   
> -	mtk_smi_larb_put(pm->larbvenc);
>   	for (i = enc_clk->clk_num - 1; i >= 0; i--)
>   		clk_disable_unprepare(enc_clk->clk_info[i].vcodec_clk);
>   }
>
Yong Wu (吴勇) Sept. 30, 2021, 3:28 a.m. UTC | #2 
Hi Dafna,

Thanks very much for the review.

On Wed, 2021-09-29 at 14:13 +0200, Dafna Hirschfeld wrote:
> 
> On 29.09.21 03:37, Yong Wu wrote:
> > MediaTek IOMMU has already added the device_link between the
> > consumer
> > and smi-larb device. If the vcodec device call the
> > pm_runtime_get_sync,
> > the smi-larb's pm_runtime_get_sync also be called automatically.
> > 
> > CC: Tiffany Lin <tiffany.lin@mediatek.com>
> > CC: Irui Wang <irui.wang@mediatek.com>
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > Reviewed-by: Evan Green <evgreen@chromium.org>
> > Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >   .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 +++-----------
> > --
> >   .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
> >   .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
> >   .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 44 +++-----------
> > -----
> >   4 files changed, 10 insertions(+), 75 deletions(-)
> > 
> > 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..d0bf9aa3b29d 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -8,14 +8,12 @@
> >   #include <linux/of_address.h>
> >   #include <linux/of_platform.h>
> >   #include <linux/pm_runtime.h>
> > -#include <soc/mediatek/smi.h>
> >   
> >   #include "mtk_vcodec_dec_pm.h"
> >   #include "mtk_vcodec_util.h"
> >   
> >   int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
> >   {
> > -	struct device_node *node;
> >   	struct platform_device *pdev;
> >   	struct mtk_vcodec_pm *pm;
> >   	struct mtk_vcodec_clk *dec_clk;
> > @@ -26,18 +24,7 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev
> > *mtkdev)
> >   	pm = &mtkdev->pm;
> >   	pm->mtkdev = mtkdev;
> >   	dec_clk = &pm->vdec_clk;
> > -	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> > -	if (!node) {
> > -		mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
> > -		return -1;
> > -	}
> >   
> > -	pdev = of_find_device_by_node(node);
> > -	of_node_put(node);
> > -	if (WARN_ON(!pdev)) {
> > -		return -1;
> > -	}
> > -	pm->larbvdec = &pdev->dev;
> >   	pdev = mtkdev->plat_dev;
> >   	pm->dev = &pdev->dev;
> >   
> > @@ -47,14 +34,11 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> >   		dec_clk->clk_info = devm_kcalloc(&pdev->dev,
> >   			dec_clk->clk_num, sizeof(*clk_info),
> >   			GFP_KERNEL);
> > -		if (!dec_clk->clk_info) {
> > -			ret = -ENOMEM;
> > -			goto put_device;
> > -		}
> > +		if (!dec_clk->clk_info)
> > +			return -ENOMEM;
> >   	} else {
> >   		mtk_v4l2_err("Failed to get vdec clock count");
> > -		ret = -EINVAL;
> > -		goto put_device;
> > +		return -EINVAL;
> >   	}
> >   
> >   	for (i = 0; i < dec_clk->clk_num; i++) {
> > @@ -63,29 +47,24 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> >   			"clock-names", i, &clk_info->clk_name);
> >   		if (ret) {
> >   			mtk_v4l2_err("Failed to get clock name id =
> > %d", i);
> > -			goto put_device;
> > +			return ret;
> >   		}
> >   		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
> >   			clk_info->clk_name);
> >   		if (IS_ERR(clk_info->vcodec_clk)) {
> >   			mtk_v4l2_err("devm_clk_get (%d)%s fail", i,
> >   				clk_info->clk_name);
> > -			ret = PTR_ERR(clk_info->vcodec_clk);
> > -			goto put_device;
> > +			return PTR_ERR(clk_info->vcodec_clk);
> >   		}
> >   	}
> >   
> >   	pm_runtime_enable(&pdev->dev);
> >   	return 0;
> > -put_device:
> > -	put_device(pm->larbvdec);
> > -	return ret;
> >   }
> >   
> >   void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
> >   {
> >   	pm_runtime_disable(dev->pm.dev);
> > -	put_device(dev->pm.larbvdec);
> >   }
> 
> Now that functions only do  'pm_runtime_disable(dev->pm.dev);' so it
> will be more
> readable to remove the function mtk_vcodec_release_dec_pm
> and replace with pm_runtime_disable(dev->pm.dev);
> Same for the 'enc' equivalent.

Make sense. But It may be not proper if using pm_runtime_disable
as the symmetry with mtk_vcodec_init_dec_pm in the mtk_vcodec_probe.

Maybe we should move pm_runtime_enable out from mtk_vcodec_init_dec_pm
into mtk_vcodec_probe. I could do a new patch for this. Is this ok for
you?

> 
> Thanks,
> Dafna

[snip]
Dafna Hirschfeld Sept. 30, 2021, 10:57 a.m. UTC | #3 
On 30.09.21 05:28, Yong Wu wrote:
> Hi Dafna,
> 
> Thanks very much for the review.
> 
> On Wed, 2021-09-29 at 14:13 +0200, Dafna Hirschfeld wrote:
>>
>> On 29.09.21 03:37, Yong Wu wrote:
>>> MediaTek IOMMU has already added the device_link between the
>>> consumer
>>> and smi-larb device. If the vcodec device call the
>>> pm_runtime_get_sync,
>>> the smi-larb's pm_runtime_get_sync also be called automatically.
>>>
>>> CC: Tiffany Lin <tiffany.lin@mediatek.com>
>>> CC: Irui Wang <irui.wang@mediatek.com>
>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
>>> Reviewed-by: Evan Green <evgreen@chromium.org>
>>> Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
>>> Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>    .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 +++-----------
>>> --
>>>    .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
>>>    .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
>>>    .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 44 +++-----------
>>> -----
>>>    4 files changed, 10 insertions(+), 75 deletions(-)
>>>
>>> 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..d0bf9aa3b29d 100644
>>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
>>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
>>> @@ -8,14 +8,12 @@
>>>    #include <linux/of_address.h>
>>>    #include <linux/of_platform.h>
>>>    #include <linux/pm_runtime.h>
>>> -#include <soc/mediatek/smi.h>
>>>    
>>>    #include "mtk_vcodec_dec_pm.h"
>>>    #include "mtk_vcodec_util.h"
>>>    
>>>    int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
>>>    {
>>> -	struct device_node *node;
>>>    	struct platform_device *pdev;
>>>    	struct mtk_vcodec_pm *pm;
>>>    	struct mtk_vcodec_clk *dec_clk;
>>> @@ -26,18 +24,7 @@ int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev
>>> *mtkdev)
>>>    	pm = &mtkdev->pm;
>>>    	pm->mtkdev = mtkdev;
>>>    	dec_clk = &pm->vdec_clk;
>>> -	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
>>> -	if (!node) {
>>> -		mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
>>> -		return -1;
>>> -	}
>>>    
>>> -	pdev = of_find_device_by_node(node);
>>> -	of_node_put(node);
>>> -	if (WARN_ON(!pdev)) {
>>> -		return -1;
>>> -	}
>>> -	pm->larbvdec = &pdev->dev;
>>>    	pdev = mtkdev->plat_dev;
>>>    	pm->dev = &pdev->dev;
>>>    
>>> @@ -47,14 +34,11 @@ int mtk_vcodec_init_dec_pm(struct
>>> mtk_vcodec_dev *mtkdev)
>>>    		dec_clk->clk_info = devm_kcalloc(&pdev->dev,
>>>    			dec_clk->clk_num, sizeof(*clk_info),
>>>    			GFP_KERNEL);
>>> -		if (!dec_clk->clk_info) {
>>> -			ret = -ENOMEM;
>>> -			goto put_device;
>>> -		}
>>> +		if (!dec_clk->clk_info)
>>> +			return -ENOMEM;
>>>    	} else {
>>>    		mtk_v4l2_err("Failed to get vdec clock count");
>>> -		ret = -EINVAL;
>>> -		goto put_device;
>>> +		return -EINVAL;
>>>    	}
>>>    
>>>    	for (i = 0; i < dec_clk->clk_num; i++) {
>>> @@ -63,29 +47,24 @@ int mtk_vcodec_init_dec_pm(struct
>>> mtk_vcodec_dev *mtkdev)
>>>    			"clock-names", i, &clk_info->clk_name);
>>>    		if (ret) {
>>>    			mtk_v4l2_err("Failed to get clock name id =
>>> %d", i);
>>> -			goto put_device;
>>> +			return ret;
>>>    		}
>>>    		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
>>>    			clk_info->clk_name);
>>>    		if (IS_ERR(clk_info->vcodec_clk)) {
>>>    			mtk_v4l2_err("devm_clk_get (%d)%s fail", i,
>>>    				clk_info->clk_name);
>>> -			ret = PTR_ERR(clk_info->vcodec_clk);
>>> -			goto put_device;
>>> +			return PTR_ERR(clk_info->vcodec_clk);
>>>    		}
>>>    	}
>>>    
>>>    	pm_runtime_enable(&pdev->dev);
>>>    	return 0;
>>> -put_device:
>>> -	put_device(pm->larbvdec);
>>> -	return ret;
>>>    }
>>>    
>>>    void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
>>>    {
>>>    	pm_runtime_disable(dev->pm.dev);
>>> -	put_device(dev->pm.larbvdec);
>>>    }
>>
>> Now that functions only do  'pm_runtime_disable(dev->pm.dev);' so it
>> will be more
>> readable to remove the function mtk_vcodec_release_dec_pm
>> and replace with pm_runtime_disable(dev->pm.dev);
>> Same for the 'enc' equivalent.
> 
> Make sense. But It may be not proper if using pm_runtime_disable
> as the symmetry with mtk_vcodec_init_dec_pm in the mtk_vcodec_probe.
> 
> Maybe we should move pm_runtime_enable out from mtk_vcodec_init_dec_pm
> into mtk_vcodec_probe. I could do a new patch for this. Is this ok for
> you?

yes, there is also asymettry when calling pm_runtime* in general,
I see in the decoder it is called from mtk_vcodec_dec_pm.c
but in the encoder it is called from mtk_vcodec_enc.c,

I think all calls to pm_runtime* should be out of the *_pm.c files
since for example 'mtk_vcodec_dec_pw_on' also do just one call to
pm_runtime_resume_and_get so this function can also be removed.

thanks,
Dafna

> 
>>
>> Thanks,
>> Dafna
> 
> [snip]
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
Yong Wu (吴勇) Oct. 7, 2021, 2:57 a.m. UTC | #4 
On Thu, 2021-09-30 at 12:57 +0200, Dafna Hirschfeld wrote:
> 
> On 30.09.21 05:28, Yong Wu wrote:
> > Hi Dafna,
> > 
> > Thanks very much for the review.
> > 
> > On Wed, 2021-09-29 at 14:13 +0200, Dafna Hirschfeld wrote:
> > > 
> > > On 29.09.21 03:37, Yong Wu wrote:
> > > > MediaTek IOMMU has already added the device_link between the
> > > > consumer
> > > > and smi-larb device. If the vcodec device call the
> > > > pm_runtime_get_sync,
> > > > the smi-larb's pm_runtime_get_sync also be called
> > > > automatically.
> > > > 
> > > > CC: Tiffany Lin <tiffany.lin@mediatek.com>
> > > > CC: Irui Wang <irui.wang@mediatek.com>
> > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > > > Reviewed-by: Evan Green <evgreen@chromium.org>
> > > > Acked-by: Tiffany Lin <tiffany.lin@mediatek.com>
> > > > Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > > ---
> > > >    .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   | 37 +++----
> > > > -------
> > > > --
> > > >    .../platform/mtk-vcodec/mtk_vcodec_drv.h      |  3 --
> > > >    .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 -
> > > >    .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c   | 44 +++----
> > > > -------

[snip]

> > > >    void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
> > > >    {
> > > >    	pm_runtime_disable(dev->pm.dev);
> > > > -	put_device(dev->pm.larbvdec);
> > > >    }
> > > 
> > > Now that functions only do  'pm_runtime_disable(dev->pm.dev);' so
> > > it
> > > will be more
> > > readable to remove the function mtk_vcodec_release_dec_pm
> > > and replace with pm_runtime_disable(dev->pm.dev);
> > > Same for the 'enc' equivalent.
> > 
> > Make sense. But It may be not proper if using pm_runtime_disable
> > as the symmetry with mtk_vcodec_init_dec_pm in the
> > mtk_vcodec_probe.
> > 
> > Maybe we should move pm_runtime_enable out from
> > mtk_vcodec_init_dec_pm
> > into mtk_vcodec_probe. I could do a new patch for this. Is this ok
> > for
> > you?
> 
> yes, there is also asymettry when calling pm_runtime* in general,
> I see in the decoder it is called from mtk_vcodec_dec_pm.c
> but in the encoder it is called from mtk_vcodec_enc.c,
> 
> I think all calls to pm_runtime* should be out of the *_pm.c files

OK. I will try this.

> since for example 'mtk_vcodec_dec_pw_on' also do just one call to
> pm_runtime_resume_and_get so this function can also be removed.

I guess this one should be reserved to vcodec guys. I see this function
is changed at [1]. Let's keep this patchset clean.

[1] 
https://patchwork.kernel.org/project/linux-mediatek/patch/20210901083215.25984-10-yunfei.dong@mediatek.com/

> 
> thanks,
> Dafna
> 
> > 
> > > 
> > > Thanks,
> > > Dafna
> > 
> > [snip]
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> >
diff mbox series

Patch

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..d0bf9aa3b29d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -8,14 +8,12 @@ 
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
-#include <soc/mediatek/smi.h>
 
 #include "mtk_vcodec_dec_pm.h"
 #include "mtk_vcodec_util.h"
 
 int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
 {
-	struct device_node *node;
 	struct platform_device *pdev;
 	struct mtk_vcodec_pm *pm;
 	struct mtk_vcodec_clk *dec_clk;
@@ -26,18 +24,7 @@  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
 	pm = &mtkdev->pm;
 	pm->mtkdev = mtkdev;
 	dec_clk = &pm->vdec_clk;
-	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
-	if (!node) {
-		mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
-		return -1;
-	}
 
-	pdev = of_find_device_by_node(node);
-	of_node_put(node);
-	if (WARN_ON(!pdev)) {
-		return -1;
-	}
-	pm->larbvdec = &pdev->dev;
 	pdev = mtkdev->plat_dev;
 	pm->dev = &pdev->dev;
 
@@ -47,14 +34,11 @@  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
 		dec_clk->clk_info = devm_kcalloc(&pdev->dev,
 			dec_clk->clk_num, sizeof(*clk_info),
 			GFP_KERNEL);
-		if (!dec_clk->clk_info) {
-			ret = -ENOMEM;
-			goto put_device;
-		}
+		if (!dec_clk->clk_info)
+			return -ENOMEM;
 	} else {
 		mtk_v4l2_err("Failed to get vdec clock count");
-		ret = -EINVAL;
-		goto put_device;
+		return -EINVAL;
 	}
 
 	for (i = 0; i < dec_clk->clk_num; i++) {
@@ -63,29 +47,24 @@  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
 			"clock-names", i, &clk_info->clk_name);
 		if (ret) {
 			mtk_v4l2_err("Failed to get clock name id = %d", i);
-			goto put_device;
+			return ret;
 		}
 		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
 			clk_info->clk_name);
 		if (IS_ERR(clk_info->vcodec_clk)) {
 			mtk_v4l2_err("devm_clk_get (%d)%s fail", i,
 				clk_info->clk_name);
-			ret = PTR_ERR(clk_info->vcodec_clk);
-			goto put_device;
+			return PTR_ERR(clk_info->vcodec_clk);
 		}
 	}
 
 	pm_runtime_enable(&pdev->dev);
 	return 0;
-put_device:
-	put_device(pm->larbvdec);
-	return ret;
 }
 
 void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
 {
 	pm_runtime_disable(dev->pm.dev);
-	put_device(dev->pm.larbvdec);
 }
 
 int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
@@ -122,11 +101,6 @@  void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
 		}
 	}
 
-	ret = mtk_smi_larb_get(pm->larbvdec);
-	if (ret) {
-		mtk_v4l2_err("mtk_smi_larb_get larbvdec fail %d", ret);
-		goto error;
-	}
 	return;
 
 error:
@@ -139,7 +113,6 @@  void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
 	struct mtk_vcodec_clk *dec_clk = &pm->vdec_clk;
 	int i = 0;
 
-	mtk_smi_larb_put(pm->larbvdec);
 	for (i = dec_clk->clk_num - 1; i >= 0; i--)
 		clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
 }
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
index c6c7672fecfb..64b73dd880ce 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
@@ -189,10 +189,7 @@  struct mtk_vcodec_clk {
  */
 struct mtk_vcodec_pm {
 	struct mtk_vcodec_clk	vdec_clk;
-	struct device	*larbvdec;
-
 	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.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
index 416f356af363..9a1515cf862d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
@@ -8,7 +8,6 @@ 
 #include <media/v4l2-event.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/videobuf2-dma-contig.h>
-#include <soc/mediatek/smi.h>
 #include <linux/pm_runtime.h>
 
 #include "mtk_vcodec_drv.h"
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..dffb190267ed 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
@@ -8,58 +8,36 @@ 
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pm_runtime.h>
-#include <soc/mediatek/smi.h>
 
 #include "mtk_vcodec_enc_pm.h"
 #include "mtk_vcodec_util.h"
 
 int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
 {
-	struct device_node *node;
 	struct platform_device *pdev;
 	struct mtk_vcodec_pm *pm;
 	struct mtk_vcodec_clk *enc_clk;
 	struct mtk_vcodec_clk_info *clk_info;
 	int ret = 0, i = 0;
-	struct device *dev;
 
 	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;
 
-	node = of_parse_phandle(dev->of_node, "mediatek,larb", 0);
-	if (!node) {
-		mtk_v4l2_err("no mediatek,larb found");
-		return -ENODEV;
-	}
-	pdev = of_find_device_by_node(node);
-	of_node_put(node);
-	if (!pdev) {
-		mtk_v4l2_err("no mediatek,larb device found");
-		return -ENODEV;
-	}
-	pm->larbvenc = &pdev->dev;
-	pdev = mtkdev->plat_dev;
-	pm->dev = &pdev->dev;
-
 	enc_clk->clk_num = of_property_count_strings(pdev->dev.of_node,
 		"clock-names");
 	if (enc_clk->clk_num > 0) {
 		enc_clk->clk_info = devm_kcalloc(&pdev->dev,
 			enc_clk->clk_num, sizeof(*clk_info),
 			GFP_KERNEL);
-		if (!enc_clk->clk_info) {
-			ret = -ENOMEM;
-			goto put_larbvenc;
-		}
+		if (!enc_clk->clk_info)
+			return -ENOMEM;
 	} else {
 		mtk_v4l2_err("Failed to get venc clock count");
-		ret = -EINVAL;
-		goto put_larbvenc;
+		return -EINVAL;
 	}
 
 	for (i = 0; i < enc_clk->clk_num; i++) {
@@ -68,29 +46,23 @@  int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
 			"clock-names", i, &clk_info->clk_name);
 		if (ret) {
 			mtk_v4l2_err("venc failed to get clk name %d", i);
-			goto put_larbvenc;
+			return ret;
 		}
 		clk_info->vcodec_clk = devm_clk_get(&pdev->dev,
 			clk_info->clk_name);
 		if (IS_ERR(clk_info->vcodec_clk)) {
 			mtk_v4l2_err("venc devm_clk_get (%d)%s fail", i,
 				clk_info->clk_name);
-			ret = PTR_ERR(clk_info->vcodec_clk);
-			goto put_larbvenc;
+			return PTR_ERR(clk_info->vcodec_clk);
 		}
 	}
 
 	return 0;
-
-put_larbvenc:
-	put_device(pm->larbvenc);
-	return ret;
 }
 
 void mtk_vcodec_release_enc_pm(struct mtk_vcodec_dev *mtkdev)
 {
 	pm_runtime_disable(mtkdev->pm.dev);
-	put_device(mtkdev->pm.larbvenc);
 }
 
 
@@ -108,11 +80,6 @@  void mtk_vcodec_enc_clock_on(struct mtk_vcodec_pm *pm)
 		}
 	}
 
-	ret = mtk_smi_larb_get(pm->larbvenc);
-	if (ret) {
-		mtk_v4l2_err("mtk_smi_larb_get larb3 fail %d", ret);
-		goto clkerr;
-	}
 	return;
 
 clkerr:
@@ -125,7 +92,6 @@  void mtk_vcodec_enc_clock_off(struct mtk_vcodec_pm *pm)
 	struct mtk_vcodec_clk *enc_clk = &pm->venc_clk;
 	int i = 0;
 
-	mtk_smi_larb_put(pm->larbvenc);
 	for (i = enc_clk->clk_num - 1; i >= 0; i--)
 		clk_disable_unprepare(enc_clk->clk_info[i].vcodec_clk);
 }