| Message ID | 20200403094033.8288-11-xia.jiang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for mt2701 JPEG ENC support | expand |
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. Best regards, Tomasz
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
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)); 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 + */ enum mtk_jpeg_ctx_state { MTK_JPEG_INIT = 0, MTK_JPEG_RUNNING, @@ -109,9 +112,7 @@ struct mtk_jpeg_q_data { * @out_q: source (output) queue information * @cap_q: destination (capture) queue queue information * @fh: V4L2 file handle - * @dec_param parameters for HW decoding * @state: state of the context - * @header_valid: set if header has been parsed and valid * @colorspace: enum v4l2_colorspace; supplemental to pixelformat * @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding * @quantization: enum v4l2_quantization, colorspace quantization
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(-)