diff mbox

[v5,3/9] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver

Message ID 20161021110104.5733240e@vento.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Mauro Carvalho Chehab Oct. 21, 2016, 1:01 p.m. UTC 
Em Fri, 2 Sep 2016 20:19:54 +0800
Tiffany Lin <tiffany.lin@mediatek.com> escreveu:

> Add v4l2 layer decoder driver for MT8173
> 
> Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>

> +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> +{
> +	int ret = 0;
> +
> +	switch (fourcc) {
> +	case V4L2_PIX_FMT_H264:
> +	case V4L2_PIX_FMT_VP8:
> +	default:
> +		return -EINVAL;
> +	}

Did you ever test this driver? The above code will *always* return
-EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!

I suspect that what you wanted to do, instead, is:

	switch (fourcc) {
	case V4L2_PIX_FMT_H264:
	case V4L2_PIX_FMT_VP8:
		break;
	default:
		return -EINVAL;

Btw, this patch series has also several issues that were pointed by
checkpatch. Please *always* run checkpatch when submitting your work.

You should take a look at the Kernel documentation about how to
submit patches, at:
	https://mchehab.fedorapeople.org/kernel_docs/process/index.html

PS.: this time, I fixed the checkpatch issues for you. So, let me know
if the patch below is OK, and I'll merge it at media upstream,
assuming that the other patches in this series are ok.

Comments

Mauro Carvalho Chehab Oct. 21, 2016, 1:08 p.m. UTC | #1 
Em Fri, 21 Oct 2016 11:01:04 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Fri, 2 Sep 2016 20:19:54 +0800
> Tiffany Lin <tiffany.lin@mediatek.com> escreveu:
> 
> > Add v4l2 layer decoder driver for MT8173
> > 
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>  
> 
> > +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> > +{
> > +	int ret = 0;
> > +
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_H264:
> > +	case V4L2_PIX_FMT_VP8:
> > +	default:
> > +		return -EINVAL;
> > +	}  
> 
> Did you ever test this driver? The above code will *always* return
> -EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!
> 
> I suspect that what you wanted to do, instead, is:
> 
> 	switch (fourcc) {
> 	case V4L2_PIX_FMT_H264:
> 	case V4L2_PIX_FMT_VP8:
> 		break;
> 	default:
> 		return -EINVAL;

Yeah, a latter patch in this series added a break there.

Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
tiffany.lin Oct. 24, 2016, 3:22 a.m. UTC | #2 
Hi Mauro,

On Fri, 2016-10-21 at 11:01 -0200, Mauro Carvalho Chehab wrote:
> Em Fri, 2 Sep 2016 20:19:54 +0800
> Tiffany Lin <tiffany.lin@mediatek.com> escreveu:
> 
> > Add v4l2 layer decoder driver for MT8173
> > 
> > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>
> 
> > +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> > +{
> > +	int ret = 0;
> > +
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_H264:
> > +	case V4L2_PIX_FMT_VP8:
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> Did you ever test this driver? The above code will *always* return
> -EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!
> 
> I suspect that what you wanted to do, instead, is:
> 
> 	switch (fourcc) {
> 	case V4L2_PIX_FMT_H264:
> 	case V4L2_PIX_FMT_VP8:
> 		break;
> 	default:
> 		return -EINVAL;
> 

The original idea here is that vp8 and h264 are added in later patches.
If get this patch without later patches, it should return -EINVAL.


> Btw, this patch series has also several issues that were pointed by
> checkpatch. Please *always* run checkpatch when submitting your work.
> 
> You should take a look at the Kernel documentation about how to
> submit patches, at:
> 	https://mchehab.fedorapeople.org/kernel_docs/process/index.html
> 
> PS.: this time, I fixed the checkpatch issues for you. So, let me know
> if the patch below is OK, and I'll merge it at media upstream,
> assuming that the other patches in this series are ok.
> 

I did run checkpatch, but I don't know why these issues missed.
probably I run checkpatch for all files not for patches.
I will take a look at the documentation and keep this in mind for future
upstream.
Appreciated for your help.


best regards,
Tiffany

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Oct. 24, 2016, 9:05 a.m. UTC | #3 
Em Mon, 24 Oct 2016 11:22:08 +0800
Tiffany Lin <tiffany.lin@mediatek.com> escreveu:

> Hi Mauro,
> 
> On Fri, 2016-10-21 at 11:01 -0200, Mauro Carvalho Chehab wrote:
> > Em Fri, 2 Sep 2016 20:19:54 +0800
> > Tiffany Lin <tiffany.lin@mediatek.com> escreveu:
> >   
> > > Add v4l2 layer decoder driver for MT8173
> > > 
> > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>  
> >   
> > > +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	switch (fourcc) {
> > > +	case V4L2_PIX_FMT_H264:
> > > +	case V4L2_PIX_FMT_VP8:
> > > +	default:
> > > +		return -EINVAL;
> > > +	}  
> > 
> > Did you ever test this driver? The above code will *always* return
> > -EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!
> > 
> > I suspect that what you wanted to do, instead, is:
> > 
> > 	switch (fourcc) {
> > 	case V4L2_PIX_FMT_H264:
> > 	case V4L2_PIX_FMT_VP8:
> > 		break;
> > 	default:
> > 		return -EINVAL;
> >   
> 
> The original idea here is that vp8 and h264 are added in later patches.
> If get this patch without later patches, it should return -EINVAL.

I noticed your idea, but next time, don't add dead code like that.
Reviewers check patch by patch at the order they're present at the
patch series.

So, don't add something broken by purpose, assuming that it would
be fixed later.

> 
> 
> > Btw, this patch series has also several issues that were pointed by
> > checkpatch. Please *always* run checkpatch when submitting your work.
> > 
> > You should take a look at the Kernel documentation about how to
> > submit patches, at:
> > 	https://mchehab.fedorapeople.org/kernel_docs/process/index.html
> > 
> > PS.: this time, I fixed the checkpatch issues for you. So, let me know
> > if the patch below is OK, and I'll merge it at media upstream,
> > assuming that the other patches in this series are ok.
> >   
> 
> I did run checkpatch, but I don't know why these issues missed.
> probably I run checkpatch for all files not for patches.
> I will take a look at the documentation and keep this in mind for future
> upstream.
> Appreciated for your help.

Checkpatch should be run patch by patch, as we expect that all patches
will follow the coding style and will compile fine, without introducing
warnings.

I do compile the Kernel for every single patch I merge.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
tiffany.lin Oct. 24, 2016, 10:49 a.m. UTC | #4 
Hi Mauro,

On Mon, 2016-10-24 at 07:05 -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 24 Oct 2016 11:22:08 +0800
> Tiffany Lin <tiffany.lin@mediatek.com> escreveu:
> 
> > Hi Mauro,
> > 
> > On Fri, 2016-10-21 at 11:01 -0200, Mauro Carvalho Chehab wrote:
> > > Em Fri, 2 Sep 2016 20:19:54 +0800
> > > Tiffany Lin <tiffany.lin@mediatek.com> escreveu:
> > >   
> > > > Add v4l2 layer decoder driver for MT8173
> > > > 
> > > > Signed-off-by: Tiffany Lin <tiffany.lin@mediatek.com>  
> > >   
> > > > +int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
> > > > +{
> > > > +	int ret = 0;
> > > > +
> > > > +	switch (fourcc) {
> > > > +	case V4L2_PIX_FMT_H264:
> > > > +	case V4L2_PIX_FMT_VP8:
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}  
> > > 
> > > Did you ever test this driver? The above code will *always* return
> > > -EINVAL, with will cause vidioc_vdec_s_fmt() to always fail!
> > > 
> > > I suspect that what you wanted to do, instead, is:
> > > 
> > > 	switch (fourcc) {
> > > 	case V4L2_PIX_FMT_H264:
> > > 	case V4L2_PIX_FMT_VP8:
> > > 		break;
> > > 	default:
> > > 		return -EINVAL;
> > >   
> > 
> > The original idea here is that vp8 and h264 are added in later patches.
> > If get this patch without later patches, it should return -EINVAL.
> 
> I noticed your idea, but next time, don't add dead code like that.
> Reviewers check patch by patch at the order they're present at the
> patch series.
> 
> So, don't add something broken by purpose, assuming that it would
> be fixed later.
> 
Got it.
> > 
> > 
> > > Btw, this patch series has also several issues that were pointed by
> > > checkpatch. Please *always* run checkpatch when submitting your work.
> > > 
> > > You should take a look at the Kernel documentation about how to
> > > submit patches, at:
> > > 	https://mchehab.fedorapeople.org/kernel_docs/process/index.html
> > > 
> > > PS.: this time, I fixed the checkpatch issues for you. So, let me know
> > > if the patch below is OK, and I'll merge it at media upstream,
> > > assuming that the other patches in this series are ok.
> > >   
> > 
> > I did run checkpatch, but I don't know why these issues missed.
> > probably I run checkpatch for all files not for patches.
> > I will take a look at the documentation and keep this in mind for future
> > upstream.
> > Appreciated for your help.
> 
> Checkpatch should be run patch by patch, as we expect that all patches
> will follow the coding style and will compile fine, without introducing
> warnings.
> 
> I do compile the Kernel for every single patch I merge.
> 
Got it. I will follow this.

best regards,
Tiffany

> Regards,
> Mauro


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

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 18182f5676d8..79ca03ac449c 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -26,14 +26,12 @@  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
 {
 	struct device_node *node;
 	struct platform_device *pdev;
-	struct device *dev;
 	struct mtk_vcodec_pm *pm;
 	int ret = 0;
 
 	pdev = mtkdev->plat_dev;
 	pm = &mtkdev->pm;
 	pm->mtkdev = mtkdev;
-	dev = &pdev->dev;
 	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
 	if (!node) {
 		mtk_v4l2_err("of_parse_phandle mediatek,larb fail!");
diff --git a/drivers/media/platform/mtk-vcodec/vdec_drv_if.c b/drivers/media/platform/mtk-vcodec/vdec_drv_if.c
index 3cb04ef45144..9813b2ffd5fa 100644
--- a/drivers/media/platform/mtk-vcodec/vdec_drv_if.c
+++ b/drivers/media/platform/mtk-vcodec/vdec_drv_if.c
@@ -31,6 +31,7 @@  int vdec_if_init(struct mtk_vcodec_ctx *ctx, unsigned int fourcc)
 	switch (fourcc) {
 	case V4L2_PIX_FMT_H264:
 	case V4L2_PIX_FMT_VP8:
+		break;
 	default:
 		return -EINVAL;
 	}