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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
