| Message ID | 20190930101841.19630-8-ribalda@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement UNIT_CELL_SIZE control | expand |
On 9/30/19 12:18 PM, Ricardo Ribalda Delgado wrote: > This helper function simplifies the code by not needing a union > v4l2_ctrl_ptr and an assignment every time we need to use > a ctrl_ptr. > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > --- > include/media/v4l2-ctrls.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index c42f164e2c9e..d69cfdffd41d 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -73,6 +73,17 @@ union v4l2_ctrl_ptr { > void *p; > }; > > +/** > + * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a > + * void pointer > + * @ptr: The void pointer > + */ > +static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr) > +{ > + BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *)); > + return (union v4l2_ctrl_ptr) ptr; Huh? Why not just do: union v4l2_ctrl_ptr p = { .p = ptr; }; return p; Or even shorter (not tested): return (union v4l2_ctrl_ptr) { .p = ptr; }; No need for BUILD_BUG_ON that way, which is rather ugly. Regards, Hans > +} > + > /** > * struct v4l2_ctrl_ops - The control operations that the driver has to provide. > * >
On 9/30/19 12:18 PM, Ricardo Ribalda Delgado wrote: > This helper function simplifies the code by not needing a union > v4l2_ctrl_ptr and an assignment every time we need to use > a ctrl_ptr. > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > --- > include/media/v4l2-ctrls.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > index c42f164e2c9e..d69cfdffd41d 100644 > --- a/include/media/v4l2-ctrls.h > +++ b/include/media/v4l2-ctrls.h > @@ -73,6 +73,17 @@ union v4l2_ctrl_ptr { > void *p; > }; > > +/** > + * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a > + * void pointer > + * @ptr: The void pointer > + */ > +static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr) Mismatch between function name and the comment above. But _from_void is not a good name, since it's from a void pointer. How about: v4l2_ctrl_ptr_create(void *ptr) since you create a v4l2_ctrl_ptr. Regards, Hans > +{ > + BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *)); > + return (union v4l2_ctrl_ptr) ptr; > +} > + > /** > * struct v4l2_ctrl_ops - The control operations that the driver has to provide. > * >
Hi Hans Thanks for the review On Mon, Sep 30, 2019 at 3:07 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 9/30/19 12:18 PM, Ricardo Ribalda Delgado wrote: > > This helper function simplifies the code by not needing a union > > v4l2_ctrl_ptr and an assignment every time we need to use > > a ctrl_ptr. > > > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> > > --- > > include/media/v4l2-ctrls.h | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h > > index c42f164e2c9e..d69cfdffd41d 100644 > > --- a/include/media/v4l2-ctrls.h > > +++ b/include/media/v4l2-ctrls.h > > @@ -73,6 +73,17 @@ union v4l2_ctrl_ptr { > > void *p; > > }; > > > > +/** > > + * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a > > + * void pointer > > + * @ptr: The void pointer > > + */ > > +static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr) > > Mismatch between function name and the comment above. > > But _from_void is not a good name, since it's from a void pointer. > > How about: v4l2_ctrl_ptr_create(void *ptr) > > since you create a v4l2_ctrl_ptr. > > Regards, > > Hans After talking on IRC: Will fix both changes and resend v9 after v5.4-rc1 is merged back into the media tree. Thanks again! > > > +{ > > + BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *)); > > + return (union v4l2_ctrl_ptr) ptr; > > +} > > + > > /** > > * struct v4l2_ctrl_ops - The control operations that the driver has to provide. > > * > > >
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h index c42f164e2c9e..d69cfdffd41d 100644 --- a/include/media/v4l2-ctrls.h +++ b/include/media/v4l2-ctrls.h @@ -73,6 +73,17 @@ union v4l2_ctrl_ptr { void *p; }; +/** + * v4l2_ctrl_ptr() - Helper function to return a v4l2_ctrl_ptr from a + * void pointer + * @ptr: The void pointer + */ +static inline union v4l2_ctrl_ptr v4l2_ctrl_ptr_from_void(void *ptr) +{ + BUILD_BUG_ON(sizeof(union v4l2_ctrl_ptr) != sizeof(void *)); + return (union v4l2_ctrl_ptr) ptr; +} + /** * struct v4l2_ctrl_ops - The control operations that the driver has to provide. *
This helper function simplifies the code by not needing a union v4l2_ctrl_ptr and an assignment every time we need to use a ctrl_ptr. Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org> --- include/media/v4l2-ctrls.h | 11 +++++++++++ 1 file changed, 11 insertions(+)