diff mbox

v4l: vsp1: Fix function declaration/definition mismatch

Message ID 20170820124006.4256-1-rosca.eugeniu@gmail.com (mailing list archive)
State Not Applicable
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Eugeniu Rosca Aug. 20, 2017, 12:40 p.m. UTC 
From: Eugeniu Rosca <erosca@de.adit-jv.com>

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 <erosca@de.adit-jv.com>
---
 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(-)

Comments

Kieran Bingham Nov. 24, 2017, 6:40 p.m. UTC | #1 
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.

On 20/08/17 13:40, Eugeniu Rosca wrote:
> From: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> 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 <erosca@de.adit-jv.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  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(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
> index c169a060b6d2..1087d7e5c126 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.h
> +++ b/drivers/media/platform/vsp1/vsp1_entity.h
> @@ -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.

>  
>  #endif /* __VSP1_ENTITY_H__ */
> diff --git a/drivers/media/platform/vsp1/vsp1_hgo.h b/drivers/media/platform/vsp1/vsp1_hgo.h
> index c6c0b7a80e0c..76a9cf97b9d3 100644
> --- a/drivers/media/platform/vsp1/vsp1_hgo.h
> +++ b/drivers/media/platform/vsp1/vsp1_hgo.h
> @@ -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.



>  
>  #endif /* __VSP1_HGO_H__ */
> diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h b/drivers/media/platform/vsp1/vsp1_hgt.h
> index 83f2e130942a..37139d54b6c8 100644
> --- a/drivers/media/platform/vsp1/vsp1_hgt.h
> +++ b/drivers/media/platform/vsp1/vsp1_hgt.h
> @@ -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__ */
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.h b/drivers/media/platform/vsp1/vsp1_uds.h
> index 7bf3cdcffc65..9c7f8497b5da 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.h
> +++ b/drivers/media/platform/vsp1/vsp1_uds.h
> @@ -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 Dec. 4, 2017, 10:52 a.m. UTC | #2 
Hello,

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.
> 
> On 20/08/17 13:40, Eugeniu Rosca wrote:
> > From: Eugeniu Rosca <erosca@de.adit-jv.com>
> > 
> > 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 <erosca@de.adit-jv.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  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(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_entity.h
> > b/drivers/media/platform/vsp1/vsp1_entity.h index
> > c169a060b6d2..1087d7e5c126 100644
> > --- a/drivers/media/platform/vsp1/vsp1_entity.h
> > +++ b/drivers/media/platform/vsp1/vsp1_entity.h
> > @@ -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.
> 
> >  #endif /* __VSP1_ENTITY_H__ */
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_hgo.h
> > b/drivers/media/platform/vsp1/vsp1_hgo.h index c6c0b7a80e0c..76a9cf97b9d3
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_hgo.h
> > +++ b/drivers/media/platform/vsp1/vsp1_hgo.h
> > @@ -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.

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.

> So this looks reasonable to me, and the same logic applies to the other
> instances.
> >  #endif /* __VSP1_HGO_H__ */
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h
> > b/drivers/media/platform/vsp1/vsp1_hgt.h index 83f2e130942a..37139d54b6c8
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_hgt.h
> > +++ b/drivers/media/platform/vsp1/vsp1_hgt.h
> > @@ -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__ */
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_uds.h
> > b/drivers/media/platform/vsp1/vsp1_uds.h index 7bf3cdcffc65..9c7f8497b5da
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_uds.h
> > +++ b/drivers/media/platform/vsp1/vsp1_uds.h
> > @@ -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__ */
Eugeniu Rosca Dec. 4, 2017, 6:10 p.m. UTC | #3 
Hello Laurent, Kieran,

On Mon, Dec 04, 2017 at 12:52:17PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> 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.
> > 
> > <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.
> 
> 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: <dhiebert@users.sourceforge.net>,
  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 Dec. 4, 2017, 7:03 p.m. UTC | #4 
Hi Eugeniu,

On Monday, 4 December 2017 20:10:34 EET Eugeniu Rosca wrote:
> On Mon, Dec 04, 2017 at 12:52:17PM +0200, Laurent Pinchart wrote:
> > 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.
> >> 
> >> <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.
> > 
> > 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: <dhiebert@users.sourceforge.net>,
>   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?

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.
diff mbox

Patch

diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
index c169a060b6d2..1087d7e5c126 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.h
+++ b/drivers/media/platform/vsp1/vsp1_entity.h
@@ -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__ */
diff --git a/drivers/media/platform/vsp1/vsp1_hgo.h b/drivers/media/platform/vsp1/vsp1_hgo.h
index c6c0b7a80e0c..76a9cf97b9d3 100644
--- a/drivers/media/platform/vsp1/vsp1_hgo.h
+++ b/drivers/media/platform/vsp1/vsp1_hgo.h
@@ -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__ */
diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h b/drivers/media/platform/vsp1/vsp1_hgt.h
index 83f2e130942a..37139d54b6c8 100644
--- a/drivers/media/platform/vsp1/vsp1_hgt.h
+++ b/drivers/media/platform/vsp1/vsp1_hgt.h
@@ -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__ */
diff --git a/drivers/media/platform/vsp1/vsp1_uds.h b/drivers/media/platform/vsp1/vsp1_uds.h
index 7bf3cdcffc65..9c7f8497b5da 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.h
+++ b/drivers/media/platform/vsp1/vsp1_uds.h
@@ -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__ */