Skip to contentSkip to conversation

staging: cedrus: Fix checkpatch issues

PATCH Superseded Maxime Ripard 4 participants 4 messages Sep 13, 2018

Subsystems (4)
Media (V4L/DVB) Staging Arm/Allwinner sunXi SoC support Allwinner VPU driver
lore
Share & tools
lore

Maxime Ripard

author
#0 lore raw

PATCH v2 1/1 staging: cedrus: Fix checkpatch issues

Checkpatch, when used with --strict, reports a number of issues on the cedrus driver.

Fix those warnings, except for a few, minor, lines too long warnings.

Signed-off-by Maxime Ripard <[email protected]>

---

Changes from v1:
- Removed the find_control wrapping
- Added the bit length to the required variable
- Added __ to the macro arguments

 drivers/staging/media/sunxi/cedrus/cedrus.c   | 10 +++++-----
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 +++++---
 .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 ++++++----
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 19 ++++++++++++++-----
 .../staging/media/sunxi/cedrus/cedrus_regs.h  | 18 ++++++++++--------
 .../staging/media/sunxi/cedrus/cedrus_video.c |  6 ++----
 6 files changed, 42 insertions(+), 29 deletions(-)
drivers/staging/media/sunxi/cedrus/cedrus.c +5 -5
index 1f5f20a1f849..82558455384a 100644
@@ -48,7 +48,7 @@ void *cedrus_find_control_data(struct cedrus_ctx *ctx, u32 id)
 {
 	unsigned int i;
 
-	for (i = 0; ctx->ctrls[i] != NULL; i++)
+	for (i = 0; ctx->ctrls[i]; i++)
 		if (ctx->ctrls[i]->id == id)
 			return ctx->ctrls[i]->p_cur.p;
 

@@ -147,10 +147,10 @@ static int cedrus_request_validate(struct media_request *req)
 			continue;
 
 		ctrl_test = v4l2_ctrl_request_hdl_ctrl_find(hdl,
-			cedrus_controls[i].id);
+							    cedrus_controls[i].id);
 		if (!ctrl_test) {
 			v4l2_info(&ctx->dev->v4l2_dev,
-				 "Missing required codec control\n");
+				  "Missing required codec control\n");
 			return -ENOENT;
 		}
 	}

@@ -310,8 +310,8 @@ static int cedrus_probe(struct platform_device *pdev)
 	dev->mdev.ops = &cedrus_m2m_media_ops;
 	dev->v4l2_dev.mdev = &dev->mdev;
 
-	ret = v4l2_m2m_register_media_controller(dev->m2m_dev,
-			vfd, MEDIA_ENT_F_PROC_VIDEO_DECODER);
+	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
+						 MEDIA_ENT_F_PROC_VIDEO_DECODER);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev,
 			 "Failed to initialize V4L2 M2M media controller\n");
drivers/staging/media/sunxi/cedrus/cedrus.h +5 -3
index 3262341e8c9a..3f61248c57ac 100644
@@ -44,7 +44,7 @@ struct cedrus_control {
 	u32			id;
 	u32			elem_size;
 	enum cedrus_codec	codec;
-	bool			required;
+	unsigned char		required:1;
 };
 
 struct cedrus_mpeg2_run {

@@ -150,12 +150,14 @@ static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
 	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
 }
 
-static inline struct cedrus_buffer *vb2_v4l2_to_cedrus_buffer(const struct vb2_v4l2_buffer *p)
+static inline struct cedrus_buffer *
+vb2_v4l2_to_cedrus_buffer(const struct vb2_v4l2_buffer *p)
 {
 	return container_of(p, struct cedrus_buffer, m2m_buf.vb);
 }
 
-static inline struct cedrus_buffer *vb2_to_cedrus_buffer(const struct vb2_buffer *p)
+static inline struct cedrus_buffer *
+vb2_to_cedrus_buffer(const struct vb2_buffer *p)
 {
 	return vb2_v4l2_to_cedrus_buffer(to_vb2_v4l2_buffer(p));
 }
drivers/staging/media/sunxi/cedrus/cedrus_dec.c +6 -4
index e40180a33951..788811a1414e 100644
@@ -43,10 +43,12 @@ void cedrus_device_run(void *priv)
 
 	switch (ctx->src_fmt.pixelformat) {
 	case V4L2_PIX_FMT_MPEG2_SLICE:
-		run.mpeg2.slice_params = cedrus_find_control_data(ctx,
-			V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
-		run.mpeg2.quantization = cedrus_find_control_data(ctx,
-			V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION);
+		run.mpeg2.slice_params =
+			cedrus_find_control_data(ctx,
+						 V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS);
+		run.mpeg2.quantization =
+			cedrus_find_control_data(ctx,
+						 V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION);
 		break;
 
 	default:
drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c +14 -5
index 029eb1626bf4..9abd39cae38c 100644
@@ -157,14 +157,22 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Forward and backward prediction reference buffers. */
 
-	fwd_luma_addr = cedrus_dst_buf_addr(ctx, slice_params->forward_ref_index, 0);
-	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, slice_params->forward_ref_index, 1);
+	fwd_luma_addr = cedrus_dst_buf_addr(ctx,
+					    slice_params->forward_ref_index,
+					    0);
+	fwd_chroma_addr = cedrus_dst_buf_addr(ctx,
+					      slice_params->forward_ref_index,
+					      1);
 
 	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
 	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
 
-	bwd_luma_addr = cedrus_dst_buf_addr(ctx, slice_params->backward_ref_index, 0);
-	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, slice_params->backward_ref_index, 1);
+	bwd_luma_addr = cedrus_dst_buf_addr(ctx,
+					    slice_params->backward_ref_index,
+					    0);
+	bwd_chroma_addr = cedrus_dst_buf_addr(ctx,
+					      slice_params->backward_ref_index,
+					      1);
 
 	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
 	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);

@@ -179,7 +187,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 
 	/* Source offset and length in bits. */
 
-	cedrus_write(dev, VE_DEC_MPEG_VLD_OFFSET, slice_params->data_bit_offset);
+	cedrus_write(dev, VE_DEC_MPEG_VLD_OFFSET,
+		     slice_params->data_bit_offset);
 
 	reg = slice_params->bit_size - slice_params->data_bit_offset;
 	cedrus_write(dev, VE_DEC_MPEG_VLD_LEN, reg);
drivers/staging/media/sunxi/cedrus/cedrus_regs.h +10 -8
index 9b14d1fb94a0..de2d6b6f64bf 100644
@@ -71,12 +71,9 @@
 
 #define VE_DEC_MPEG_MP12HDR_SLICE_TYPE(t)	(((t) << 28) & GENMASK(30, 28))
 #define VE_DEC_MPEG_MP12HDR_F_CODE_SHIFT(x, y)	(24 - 4 * (y) - 8 * (x))
-#define VE_DEC_MPEG_MP12HDR_F_CODE_MASK(x, y) \
-	GENMASK(VE_DEC_MPEG_MP12HDR_F_CODE_SHIFT(x, y) + 3, \
-		VE_DEC_MPEG_MP12HDR_F_CODE_SHIFT(x, y))
-#define VE_DEC_MPEG_MP12HDR_F_CODE(x, y, v) \
-	(((v) << VE_DEC_MPEG_MP12HDR_F_CODE_SHIFT(x, y)) & \
-	 VE_DEC_MPEG_MP12HDR_F_CODE_MASK(x, y))
+#define VE_DEC_MPEG_MP12HDR_F_CODE(__x, __y, __v) \
+	(((__v) & GENMASK(3, 0)) << VE_DEC_MPEG_MP12HDR_F_CODE_SHIFT(__x, __y))
+
 #define VE_DEC_MPEG_MP12HDR_INTRA_DC_PRECISION(p) \
 	(((p) << 10) & GENMASK(11, 10))
 #define VE_DEC_MPEG_MP12HDR_INTRA_PICTURE_STRUCTURE(s) \

@@ -204,8 +201,13 @@
 #define VE_DEC_MPEG_VLD_ADDR_FIRST_PIC_DATA	BIT(30)
 #define VE_DEC_MPEG_VLD_ADDR_LAST_PIC_DATA	BIT(29)
 #define VE_DEC_MPEG_VLD_ADDR_VALID_PIC_DATA	BIT(28)
-#define VE_DEC_MPEG_VLD_ADDR_BASE(a) \
-	(((a) & GENMASK(27, 4)) | (((a) >> 28) & GENMASK(3, 0)))
+#define VE_DEC_MPEG_VLD_ADDR_BASE(a)					\
+	({								\
+		u32 _tmp = (a);						\
+		u32 _lo = _tmp & GENMASK(27, 4);			\
+		u32 _hi = (_tmp >> 28) & GENMASK(3, 0);			\
+		(_lo | _hi);						\
+	})
 
 #define VE_DEC_MPEG_VLD_OFFSET			(VE_ENGINE_DEC_MPEG + 0x2c)
 #define VE_DEC_MPEG_VLD_LEN			(VE_ENGINE_DEC_MPEG + 0x30)
drivers/staging/media/sunxi/cedrus/cedrus_video.c +2 -4
index d3887f8dea6a..5c5fce678b93 100644
@@ -82,10 +82,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 static bool cedrus_check_format(u32 pixelformat, u32 directions,
 				unsigned int capabilities)
 {
-	struct cedrus_format *fmt = cedrus_find_format(pixelformat, directions,
-						       capabilities);
-
-	return fmt != NULL;
+	return cedrus_find_format(pixelformat, directions, capabilities);
 }
 
 static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt)

@@ -494,6 +491,7 @@ static void cedrus_buf_request_complete(struct vb2_buffer *vb)
 
 	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->hdl);
 }
+
 static struct vb2_ops cedrus_qops = {
 	.queue_setup		= cedrus_queue_setup,
 	.buf_prepare		= cedrus_buf_prepare,
Signature · 1 line
2.17.1
#1 lore raw

Em Thu, 13 Sep 2018 16:40:47 +0200
Maxime Ripard <[email protected]> escreveu:

drivers/staging/media/sunxi/cedrus/cedrus_video.c static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, · line 86

Quoted 12 lines
drivers/staging/media/sunxi/cedrus/cedrus_video.c +1 -4
@@ -82,10 +82,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 static bool cedrus_check_format(u32 pixelformat, u32 directions,
 				unsigned int capabilities)
 {
-	struct cedrus_format *fmt = cedrus_find_format(pixelformat, directions,
-						       capabilities);
-
-	return fmt != NULL;
+	return cedrus_find_format(pixelformat, directions, capabilities);
 }

Hmm... just occurred to me... Why do you need this? I mean, you could simply do something like:

$ git filter-branch -f --tree-filter 'for i in $(git grep -l cedrus_check_format); do \
	sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g -i $i; done ' origin/master..

(or just do a sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g as a separate patch)

and get rid of cedrus_check_format() for good.

Thanks,
Mauro

#2 lore raw
Quoted 20 lines · Mauro Carvalho Chehab

Em Thu, 13 Sep 2018 16:40:47 +0200
Maxime Ripard <[email protected]> escreveu:

drivers/staging/media/sunxi/cedrus/cedrus_video.c static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, · line 86

Quoted 12 lines
drivers/staging/media/sunxi/cedrus/cedrus_video.c +1 -4
@@ -82,10 +82,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 static bool cedrus_check_format(u32 pixelformat, u32 directions,
 				unsigned int capabilities)
 {
-	struct cedrus_format *fmt = cedrus_find_format(pixelformat, directions,
-						       capabilities);
-
-	return fmt != NULL;
+	return cedrus_find_format(pixelformat, directions, capabilities);
 }

Hmm... just occurred to me... Why do you need this? I mean, you could simply do something like:

Quoted 7 lines
$ git filter-branch -f --tree-filter 'for i in $(git grep -l cedrus_check_format); do \
	sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g -i $i; done ' origin/master..

(or just do a sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g as a separate patch)

and get rid of cedrus_check_format() for good.

That looks like a nice follow-up patch. It's a staging driver, it doesn't have to be perfect.

I'll prepare a pull request tomorrow.

Regards,

Hans

#3 lore raw
In reply to Mauro Carvalho Chehab

Hi,

Quoted 20 lines · Mauro Carvalho Chehab

Em Thu, 13 Sep 2018 16:40:47 +0200
Maxime Ripard <[email protected]> escreveu:

drivers/staging/media/sunxi/cedrus/cedrus_video.c static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, · line 86

Quoted 12 lines
drivers/staging/media/sunxi/cedrus/cedrus_video.c +1 -4
@@ -82,10 +82,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions,
 static bool cedrus_check_format(u32 pixelformat, u32 directions,
 				unsigned int capabilities)
 {
-	struct cedrus_format *fmt = cedrus_find_format(pixelformat, directions,
-						       capabilities);
-
-	return fmt != NULL;
+	return cedrus_find_format(pixelformat, directions, capabilities);
 }

Hmm... just occurred to me... Why do you need this? I mean, you could simply do something like:

Quoted 7 lines
$ git filter-branch -f --tree-filter 'for i in $(git grep -l cedrus_check_format); do \
	sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g -i $i; done ' origin/master..

(or just do a sed -E s,\\bcedrus_check_format\\b,cedrus_find_format,g as a separate patch)

and get rid of cedrus_check_format() for good.

Agreed, the name is probably explicit enough anyway. I probably should have done that in the first place anyway.

Cheers,

Paul

Signature · 5 lines
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/