v4l: vsp1: Fix function declaration/definition mismatch

PATCH In review Eugeniu Rosca 4 participants 5 messages Aug 20, 2017 – Dec 4, 2017 Media (V4L/DVB)

lore

Share & tools

lore

Eugeniu Rosca

author

2017-08-20 12:40 UTC #0 lore raw

PATCH 1/1 v4l: vsp1: Fix function declaration/definition mismatch

From: Eugeniu Rosca <[email protected]>

Cppcheck v1.81 complains that the parameter names of certain vsp1 functions don't match between declaration and definition. Fix this. No functional change is confirmed by the empty delta between the disassembled object files before and after the change.

Signed-off-by Eugeniu Rosca <[email protected]>

---
 drivers/media/platform/vsp1/vsp1_entity.h | 5 +++--
 drivers/media/platform/vsp1/vsp1_hgo.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_hgt.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_uds.h    | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

drivers/media/platform/vsp1/vsp1_entity.h +3 -2

index c169a060b6d2..1087d7e5c126 100644
@@ -161,7 +161,8 @@ int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev,
 int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
 				struct v4l2_subdev_pad_config *cfg,
 				struct v4l2_subdev_frame_size_enum *fse,
-				unsigned int min_w, unsigned int min_h,
-				unsigned int max_w, unsigned int max_h);
+				unsigned int min_width, unsigned int min_height,
+				unsigned int max_width,
+				unsigned int max_height);
 
 #endif /* __VSP1_ENTITY_H__ */

drivers/media/platform/vsp1/vsp1_hgo.h +1 -1

index c6c0b7a80e0c..76a9cf97b9d3 100644
@@ -40,6 +40,6 @@ static inline struct vsp1_hgo *to_hgo(struct v4l2_subdev *subdev)
 }
 
 struct vsp1_hgo *vsp1_hgo_create(struct vsp1_device *vsp1);
-void vsp1_hgo_frame_end(struct vsp1_entity *hgo);
+void vsp1_hgo_frame_end(struct vsp1_entity *entity);
 
 #endif /* __VSP1_HGO_H__ */

drivers/media/platform/vsp1/vsp1_hgt.h +1 -1

index 83f2e130942a..37139d54b6c8 100644
@@ -37,6 +37,6 @@ static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev)
 }
 
 struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1);
-void vsp1_hgt_frame_end(struct vsp1_entity *hgt);
+void vsp1_hgt_frame_end(struct vsp1_entity *entity);
 
 #endif /* __VSP1_HGT_H__ */

drivers/media/platform/vsp1/vsp1_uds.h +1 -1

index 7bf3cdcffc65..9c7f8497b5da 100644
@@ -35,7 +35,7 @@ static inline struct vsp1_uds *to_uds(struct v4l2_subdev *subdev)
 
 struct vsp1_uds *vsp1_uds_create(struct vsp1_device *vsp1, unsigned int index);
 
-void vsp1_uds_set_alpha(struct vsp1_entity *uds, struct vsp1_dl_list *dl,
+void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_list *dl,
 			unsigned int alpha);
 
 #endif /* __VSP1_UDS_H__ */

Signature · 1 line

2.14.1

Kieran Bingham

Reviewed-by on v1 1/1

2017-11-24 18:41 UTC #1 lore raw

Hi Eugeniu,

Thankyou for the patch,

Laurent - Any comments on this? Otherwise I'll bundle this in with my suspend/resume patch for a pull request.

Quoted 8 lines · Eugeniu Rosca

From: Eugeniu Rosca <[email protected]>
Cppcheck v1.81 complains that the parameter names of certain vsp1 functions don't match between declaration and definition. Fix this. No functional change is confirmed by the empty delta between the disassembled object files before and after the change.
Signed-off-by Eugeniu Rosca <[email protected]>

Reviewed-by Kieran Bingham <[email protected]>

Quoted 12 lines

---
 drivers/media/platform/vsp1/vsp1_entity.h | 5 +++--
 drivers/media/platform/vsp1/vsp1_hgo.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_hgt.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_uds.h    | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

drivers/media/platform/vsp1/vsp1_entity.h +0 -0

index c169a060b6d2..1087d7e5c126 100644

drivers/media/platform/vsp1/vsp1_entity.h int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev, · line 166

Quoted 8 lines

drivers/media/platform/vsp1/vsp1_entity.h +3 -2

@@ -161,7 +161,8 @@ int vsp1_subdev_enum_mbus_code(struct v4l2_subdev *subdev,
 int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
 				struct v4l2_subdev_pad_config *cfg,
 				struct v4l2_subdev_frame_size_enum *fse,
-				unsigned int min_w, unsigned int min_h,
-				unsigned int max_w, unsigned int max_h);
+				unsigned int min_width, unsigned int min_height,
+				unsigned int max_width,
+				unsigned int max_height);

This is fine.

drivers/media/platform/vsp1/vsp1_hgo.h static inline struct vsp1_hgo *to_hgo(struct v4l2_subdev *subdev) · line 43

Quoted 12 lines

drivers/media/platform/vsp1/vsp1_entity.h +0 -0

 #endif /* __VSP1_ENTITY_H__ */

drivers/media/platform/vsp1/vsp1_hgo.h +1 -1

index c6c0b7a80e0c..76a9cf97b9d3 100644
@@ -40,6 +40,6 @@ static inline struct vsp1_hgo *to_hgo(struct v4l2_subdev *subdev)
 }
 
 struct vsp1_hgo *vsp1_hgo_create(struct vsp1_device *vsp1);
-void vsp1_hgo_frame_end(struct vsp1_entity *hgo);
+void vsp1_hgo_frame_end(struct vsp1_entity *entity);

I was going to say: We know the object is an entity by it's type. Isn't hgo more descriptive for it's name ?

However to answer my own question - The implementation function goes on to define a struct vsp1_hgo *hgo, so no ... the Entity object shouldn't be hgo.

So this looks reasonable to me, and the same logic applies to the other instances.

Quoted 28 lines

drivers/media/platform/vsp1/vsp1_hgo.h +0 -0

 #endif /* __VSP1_HGO_H__ */

drivers/media/platform/vsp1/vsp1_hgt.h +1 -1

index 83f2e130942a..37139d54b6c8 100644
@@ -37,6 +37,6 @@ static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev)
 }
 
 struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1);
-void vsp1_hgt_frame_end(struct vsp1_entity *hgt);
+void vsp1_hgt_frame_end(struct vsp1_entity *entity);
 
 #endif /* __VSP1_HGT_H__ */

drivers/media/platform/vsp1/vsp1_uds.h +1 -1

index 7bf3cdcffc65..9c7f8497b5da 100644
@@ -35,7 +35,7 @@ static inline struct vsp1_uds *to_uds(struct v4l2_subdev *subdev)
 
 struct vsp1_uds *vsp1_uds_create(struct vsp1_device *vsp1, unsigned int index);
 
-void vsp1_uds_set_alpha(struct vsp1_entity *uds, struct vsp1_dl_list *dl,
+void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_list *dl,
 			unsigned int alpha);
 
 #endif /* __VSP1_UDS_H__ */

Laurent Pinchart

2017-12-04 10:52 UTC #2 lore raw

Hello,

Quoted 58 lines · On Friday, 24 November 2017 20:40:57 EET Kieran Bingham wrote:

Hi Eugeniu,
Thankyou for the patch,
Laurent - Any comments on this? Otherwise I'll bundle this in with my suspend/resume patch for a pull request.
Quoted 8 lines · Eugeniu Rosca
From: Eugeniu Rosca <[email protected]>
Cppcheck v1.81 complains that the parameter names of certain vsp1 functions don't match between declaration and definition. Fix this. No functional change is confirmed by the empty delta between the disassembled object files before and after the change.
Signed-off-by Eugeniu Rosca <[email protected]>
Reviewed-by Kieran Bingham <[email protected]>
Quoted 14 lines
---
 drivers/media/platform/vsp1/vsp1_entity.h | 5 +++--
 drivers/media/platform/vsp1/vsp1_hgo.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_hgt.h    | 2 +-
 drivers/media/platform/vsp1/vsp1_uds.h    | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)
(quoted diff) +0 -0
b/drivers/media/platform/vsp1/vsp1_entity.h index
c169a060b6d2..1087d7e5c126 100644
drivers/media/platform/vsp1/vsp1_entity.h int vsp1_subdev_enum_mbus_code(struct v4l2_subdev · line 167
Quoted 9 lines
drivers/media/platform/vsp1/vsp1_entity.h +3 -2
@@ -161,7 +161,8 @@ int vsp1_subdev_enum_mbus_code(struct v4l2_subdev
 *subdev,
 int vsp1_subdev_enum_frame_size(struct v4l2_subdev *subdev,
 				struct v4l2_subdev_pad_config *cfg,
 				struct v4l2_subdev_frame_size_enum *fse,
-				unsigned int min_w, unsigned int min_h,
-				unsigned int max_w, unsigned int max_h);
+				unsigned int min_width, unsigned int min_height,
+				unsigned int max_width,
+				unsigned int max_height);
This is fine.
Quoted 13 lines
drivers/media/platform/vsp1/vsp1_entity.h +0 -0
 #endif /* __VSP1_ENTITY_H__ */
(quoted diff) +0 -0
b/drivers/media/platform/vsp1/vsp1_hgo.h index c6c0b7a80e0c..76a9cf97b9d3 100644
drivers/media/platform/vsp1/vsp1_hgo.h +0 -0
@@ -40,6 +40,6 @@ static inline struct vsp1_hgo *to_hgo(struct v4l2_subdev
 *subdev)> 
 }
 
 struct vsp1_hgo *vsp1_hgo_create(struct vsp1_device *vsp1);
 

Quoted 8 lines

drivers/media/platform/vsp1/vsp1_hgo.h static inline struct vsp1_hgo *to_hgo(struct v4l2_subdev *subdev) · line 43
Quoted 2 lines
drivers/media/platform/vsp1/vsp1_hgo.h +1 -1
-void vsp1_hgo_frame_end(struct vsp1_entity *hgo);
+void vsp1_hgo_frame_end(struct vsp1_entity *entity);
I was going to say: We know the object is an entity by it's type. Isn't hgo more descriptive for it's name ?
However to answer my own question - The implementation function goes on to define a struct vsp1_hgo *hgo, so no ... the Entity object shouldn't be hgo.

And that's exactly why there's a difference between the declaration and implementation :-) Naming the parameter hgo in the declaration makes it clear to the reader what entity is expected. The parameter is then named entity in the function definition to allow for the vsp1_hgo *hgo local variable.

Is the mismatch a real issue ? I don't think the kernel coding style mandates parameter names to be identical between function declaration and definition.

Quoted 35 lines

So this looks reasonable to me, and the same logic applies to the other instances.
Quoted 33 lines
#endif /* __VSP1_HGO_H__ */
(quoted diff) +0 -0
b/drivers/media/platform/vsp1/vsp1_hgt.h index 83f2e130942a..37139d54b6c8 100644
drivers/media/platform/vsp1/vsp1_hgt.h +1 -1
@@ -37,6 +37,6 @@ static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev
 *subdev)> 
 }
 
 struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1);
 
-void vsp1_hgt_frame_end(struct vsp1_entity *hgt);
+void vsp1_hgt_frame_end(struct vsp1_entity *entity);
#endif /* __VSP1_HGT_H__ */
(quoted diff) +0 -0
b/drivers/media/platform/vsp1/vsp1_uds.h index 7bf3cdcffc65..9c7f8497b5da 100644
drivers/media/platform/vsp1/vsp1_uds.h +1 -1
@@ -35,7 +35,7 @@ static inline struct vsp1_uds *to_uds(struct v4l2_subdev
 *subdev)
 struct vsp1_uds *vsp1_uds_create(struct vsp1_device *vsp1, unsigned int
 index);
-void vsp1_uds_set_alpha(struct vsp1_entity *uds, struct vsp1_dl_list *dl,
+void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_list
 *dl,
 			unsigned int alpha);
 
#endif /* __VSP1_UDS_H__ */

Signature · 3 lines

Regards,

Laurent Pinchart

Eugeniu Rosca

2017-12-04 18:10 UTC #3 lore raw

Hello Laurent, Kieran,

Quoted 18 lines · Laurent Pinchart

Hello,
Quoted 7 lines · On Friday, 24 November 2017 20:40:57 EET Kieran Bingham wrote:
Hi Eugeniu,
Thankyou for the patch,
Laurent - Any comments on this? Otherwise I'll bundle this in with my suspend/resume patch for a pull request.
Quoted 7 lines
<CUT>
I was going to say: We know the object is an entity by it's type. Isn't hgo more descriptive for it's name ?
However to answer my own question - The implementation function goes on to define a struct vsp1_hgo *hgo, so no ... the Entity object shouldn't be hgo.

Quoted 7 lines

And that's exactly why there's a difference between the declaration and implementation :-) Naming the parameter hgo in the declaration makes it clear to the reader what entity is expected. The parameter is then named entity in the function definition to allow for the vsp1_hgo *hgo local variable.
Is the mismatch a real issue ? I don't think the kernel coding style mandates parameter names to be identical between function declaration and definition.

You are the DRM/VSP1 and kernel experts, so feel free to drop the patch. Still IMO what makes kernel coding style sweet is its simplicity [1]. Here is some statistics computed with exuberant ctags and cpccheck.

$ git describe HEAD
v4.15-rc2

$ ctags --version
Exuberant Ctags 5.9~svn20110310, Copyright (C) 1996-2009 Darren Hiebert
  Addresses: <[email protected]>,
  http://ctags.sourceforge.net
    Optional compiled features: +wildcards, +regex

# Number of function definitions in drivers/media:

$ find drivers/media -type d | \
  xargs -I {} sh -c "ctags -x --c-types=f {}/*" | wc -l
24913

# Number of func declaration/definition mismatches found by cppcheck:

$ cppcheck --force --enable=all --inconclusive  drivers/media/ 2>&1 | \
  grep declaration | wc -l
168

It looks like only (168/24913) * 100% = 0,67% of all drivers/media functions have a mismatch between function declaration and function definition. Why making this number worse?

BR,
Eugeniu.

[1] ./Documentation/process/coding-style.rst: Kernel coding style is super simple.

Laurent Pinchart

2017-12-04 19:03 UTC #4 lore raw

Hi Eugeniu,

Quoted 46 lines · On Monday, 4 December 2017 20:10:34 EET Eugeniu Rosca wrote:

Quoted 23 lines · Laurent Pinchart
Quoted 7 lines · On Friday, 24 November 2017 20:40:57 EET Kieran Bingham wrote:
Hi Eugeniu,
Thankyou for the patch,
Laurent - Any comments on this? Otherwise I'll bundle this in with my suspend/resume patch for a pull request.
Quoted 8 lines
<CUT>
I was going to say: We know the object is an entity by it's type. Isn't hgo more descriptive for it's name ?
However to answer my own question - The implementation function goes on to define a struct vsp1_hgo *hgo, so no ... the Entity object shouldn't be hgo.
And that's exactly why there's a difference between the declaration and implementation :-) Naming the parameter hgo in the declaration makes it clear to the reader what entity is expected. The parameter is then named entity in the function definition to allow for the vsp1_hgo *hgo local variable.
Quoted 3 lines
Is the mismatch a real issue ? I don't think the kernel coding style mandates parameter names to be identical between function declaration and definition.
You are the DRM/VSP1 and kernel experts, so feel free to drop the patch. Still IMO what makes kernel coding style sweet is its simplicity [1]. Here is some statistics computed with exuberant ctags and cpccheck.
$ git describe HEAD
v4.15-rc2
$ ctags --version
Exuberant Ctags 5.9~svn20110310, Copyright (C) 1996-2009 Darren Hiebert
  Addresses: <[email protected]>,
  http://ctags.sourceforge.net
    Optional compiled features: +wildcards, +regex
# Number of function definitions in drivers/media:
$ find drivers/media -type d | \
  xargs -I {} sh -c "ctags -x --c-types=f {}/*" | wc -l
24913

Quoted 8 lines

# Number of func declaration/definition mismatches found by cppcheck:
$ cppcheck --force --enable=all --inconclusive  drivers/media/ 2>&1 | \
  grep declaration | wc -l
168
It looks like only (168/24913) * 100% = 0,67% of all drivers/media functions have a mismatch between function declaration and function definition. Why making this number worse?

Of course the goal is not to introduce a mismatch for the sake of it. It makes sense for the declaration and definition to match in most cases. My point is that in this particular case I believe the mismatch makes the code more readable.

Signature · 3 lines

Regards,

Laurent Pinchart