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

[Tiffany Lin]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> escreveu:
> > >   
> > > > Add v4l2 layer decoder driver for MT8173
> > > > 
> > > > Signed-off-by: Tiffany Lin <tiffany.lin@xxxxxxxxxxxx>  
> > >   
> > > > +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