Re: [PATCH v8 10/14] media: platform: Delete redundant code for improving code quality

[Xia Jiang <xia.jiang@mediatek.com>]

On Thu, 2020-05-21 at 15:49 +0000, Tomasz Figa wrote:
> Hi Xia,
> 
> On Fri, Apr 03, 2020 at 05:40:29PM +0800, Xia Jiang wrote:
> > Delete unused member variables annotation.
> > Delete unused variable definition.
> > Delete redundant log print, because V4L2 debug logs already print it.
> > 
> > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com>
> > ---
> > v8: no changes
> > ---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 16 ++--------------
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h |  5 +++--
> >  2 files changed, 5 insertions(+), 16 deletions(-)
> > 
> 
> Thank you for the patch. Please see my comments inline.
> 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index 4e64046a6854..9e59b9a51ef0 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -182,7 +182,6 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> >  				   struct mtk_jpeg_ctx *ctx, int q_type)
> >  {
> >  	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > -	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> >  	int i;
> >  
> >  	memset(pix_mp->reserved, 0, sizeof(pix_mp->reserved));
> > @@ -190,7 +189,7 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> >  
> >  	if (ctx->state != MTK_JPEG_INIT) {
> >  		mtk_jpeg_adjust_fmt_mplane(ctx, f);
> > -		goto end;
> > +		return 0;
> >  	}
> >  
> >  	pix_mp->num_planes = fmt->colplanes;
> > @@ -210,7 +209,7 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> >  		pfmt->sizeimage = round_up(pfmt->sizeimage, 128);
> >  		if (pfmt->sizeimage == 0)
> >  			pfmt->sizeimage = MTK_JPEG_DEFAULT_SIZEIMAGE;
> > -		goto end;
> > +		return 0;
> >  	}
> >  
> >  	/* type is MTK_JPEG_FMT_TYPE_CAPTURE */
> > @@ -224,20 +223,9 @@ static int mtk_jpeg_try_fmt_mplane(struct v4l2_format *f,
> >  		u32 stride = pix_mp->width * fmt->h_sample[i] / 4;
> >  		u32 h = pix_mp->height * fmt->v_sample[i] / 4;
> >  
> > -		memset(pfmt->reserved, 0, sizeof(pfmt->reserved));
> 
> This change is not mentioned in the description. I'd suggest moving it
> to a separate patch, because it's a functional change.
> 
> >  		pfmt->bytesperline = stride;
> >  		pfmt->sizeimage = stride * h;
> >  	}
> > -end:
> > -	v4l2_dbg(2, debug, &jpeg->v4l2_dev, "wxh:%ux%u\n",
> > -		 pix_mp->width, pix_mp->height);
> > -	for (i = 0; i < pix_mp->num_planes; i++) {
> > -		v4l2_dbg(2, debug, &jpeg->v4l2_dev,
> > -			 "plane[%d] bpl=%u, size=%u\n",
> > -			 i,
> > -			 pix_mp->plane_fmt[i].bytesperline,
> > -			 pix_mp->plane_fmt[i].sizeimage);
> > -	}
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 64a731261214..9bbd615b1067 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -30,6 +30,9 @@
> >  
> >  #define MTK_JPEG_DEFAULT_SIZEIMAGE	(1 * 1024 * 1024)
> >  
> > +/**
> > + * enum mtk_jpeg_ctx_state - contex state of jpeg
> 
> typo: s/contex/context/
> 
> But I'd rephrase it to "states of the context state machine".
> 
> > + */
> 
> Not mentioned in the description. Also, the documentation of an enum
> should have descriptions for the values.
Done.
> 
> Best regards,
> Tomasz