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.ji...@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
