media: v4l2-ctrl: Initialize _BUTTON and _CTRL_CLASS
PATCH 1/2 media: v4l2-ctrl: Initialize _BUTTON and _CTRL_CLASS
These two control types don't really need a default value, as they are not expected to carry any value.
However, it's slightly clearer to initialize them explicitly instead of falling back to the switch default.
--- drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +2 -0
@@ -1530,6 +1530,8 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_MENU :case V4L2_CTRL_TYPE_BITMASK :case V4L2_CTRL_TYPE_BOOLEAN : +case V4L2_CTRL_TYPE_BUTTON : +case V4L2_CTRL_TYPE_CTRL_CLASS : ptr.p_s32[idx] = ctrl->default_value;break ;case V4L2_CTRL_TYPE_U8 :
Signature · 1 line
2.20.1
PATCH 2/2 media: v4l2-ctrl: Move compound control initialization
Rework std_init adding an explicit initialization for compound controls.
While here, make sure the control is initialized to zero, before providing default values for all its fields.
--- drivers/media/v4l2-core/v4l2-ctrls.c | 55 +++++++++++++++++----------- 1 file changed, 34 insertions(+), 21 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls.c +34 -21
@@ -1506,25 +1506,49 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, } } -static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +static void std_init_compound (const struct v4l2_ctrl *ctrl,u32 idx,union v4l2_ctrl_ptr ptr) {struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params; +memset (ptr.p,0 , ctrl->elem_size); +/* * The cast is needed to get rid of a gcc warning complaining that * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the * v4l2_ctrl_type enum. */ switch ((u32 )ctrl->type) { +case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : + p_mpeg2_slice_params = ptr.p; +/* 4:2:0 */ + p_mpeg2_slice_params->sequence.chroma_format =1 ; +/* 8 bits */ + p_mpeg2_slice_params->picture.intra_dc_precision =0 ; +/* interlaced top field */ + p_mpeg2_slice_params->picture.picture_structure =1 ; + p_mpeg2_slice_params->picture.picture_coding_type = +V4L2_MPEG2_PICTURE_CODING_TYPE_I ; +return ; +default : + idx *= ctrl->elem_size; +memset (ptr.p + idx,0 , ctrl->elem_size); +return ; + } +} + +static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ +switch (ctrl->type) {case V4L2_CTRL_TYPE_STRING : idx *= ctrl->elem_size;memset (ptr.p_char + idx,' ' , ctrl->minimum); ptr.p_char[idx + ctrl->minimum] ='\0' ; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER64 : ptr.p_s64[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER :case V4L2_CTRL_TYPE_INTEGER_MENU :case V4L2_CTRL_TYPE_MENU :
@@ -1533,32 +1557,21 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_BUTTON :case V4L2_CTRL_TYPE_CTRL_CLASS : ptr.p_s32[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U8 : ptr.p_u8[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U16 : ptr.p_u16[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U32 : ptr.p_u32[idx] = ctrl->default_value; -break ; -case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : - p_mpeg2_slice_params = ptr.p; -/* 4:2:0 */ - p_mpeg2_slice_params->sequence.chroma_format =1 ; -/* 8 bits */ - p_mpeg2_slice_params->picture.intra_dc_precision =0 ; -/* interlaced top field */ - p_mpeg2_slice_params->picture.picture_structure =1 ; - p_mpeg2_slice_params->picture.picture_coding_type = -V4L2_MPEG2_PICTURE_CODING_TYPE_I ; -break ; +return ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); -break ; +std_init_compound (ctrl, idx, ptr); +return ; } + }static void std_log (const struct v4l2_ctrl *ctrl)
Signature · 1 line
2.20.1
Re: [PATCH 2/2] media: v4l2-ctrl: Move compound control initialization
Hi Ezequiel,
Quoted 17 lines · On Wed, 29 May 2019 16:28:11 -0300 Ezequiel Garcia <[email protected]>…
Rework
std_initadding an explicit initialization for compound controls.While here, make sure the control is initialized to zero, before providing default values for all its fields.
Signed-off-by Ezequiel Garcia <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls.c | 55 +++++++++++++++++----------- 1 file changed, 34 insertions(+), 21 deletions(-)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0@@ -1506,25 +1506,49 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, }
drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1515
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3
-1} -static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +static void std_init_compound (const struct v4l2_ctrl *ctrl,u32 idx,union v4l2_ctrl_ptr ptr) {struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params; +memset (ptr.p,0 , ctrl->elem_size); +
Hm, shouldn't we have
idx *= ctrl->elem_size; memset(ptr.p + idx, 0, ctrl->elem_size);
instead?
Looks like your solution always resets the first element of an array even if idx > 0.
Quoted 11 lines
drivers/media/v4l2-core/v4l2-ctrls.c +5
-0/* * The cast is needed to get rid of a gcc warning complaining that * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the * v4l2_ctrl_type enum. */ switch ((u32 )ctrl->type) { +case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : + p_mpeg2_slice_params = ptr.p; +/* 4:2:0 */ + p_mpeg2_slice_params->sequence.chroma_format =1 ; +/* 8 bits */
drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1536
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +10
-0+ p_mpeg2_slice_params->picture.intra_dc_precision =0 ; +/* interlaced top field */ + p_mpeg2_slice_params->picture.picture_structure =1 ; + p_mpeg2_slice_params->picture.picture_coding_type = +V4L2_MPEG2_PICTURE_CODING_TYPE_I ; +return ; +default : + idx *= ctrl->elem_size; +memset (ptr.p + idx,0 , ctrl->elem_size); +return ;
If you fix the first memset() you can get rid of this one. Also don't see why you use return instead of break in this switch-case block (there's no reason to bail out early in this function).
Quoted 45 lines
drivers/media/v4l2-core/v4l2-ctrls.c +12
-14+ } +} + +static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ +switch (ctrl->type) {case V4L2_CTRL_TYPE_STRING : idx *= ctrl->elem_size;memset (ptr.p_char + idx,' ' , ctrl->minimum); ptr.p_char[idx + ctrl->minimum] ='\0' ; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER64 : ptr.p_s64[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER :case V4L2_CTRL_TYPE_INTEGER_MENU :case V4L2_CTRL_TYPE_MENU :@@ -1533,32 +1557,21 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_BUTTON :case V4L2_CTRL_TYPE_CTRL_CLASS : ptr.p_s32[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U8 : ptr.p_u8[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U16 : ptr.p_u16[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U32 : ptr.p_u32[idx] = ctrl->default_value; -break ; -case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : - p_mpeg2_slice_params = ptr.p; -/* 4:2:0 */ - p_mpeg2_slice_params->sequence.chroma_format =1 ; -/* 8 bits */ - p_mpeg2_slice_params->picture.intra_dc_precision =0 ; -/* interlaced top field */ - p_mpeg2_slice_params->picture.picture_structure =1 ;
drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1572
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3
-6- p_mpeg2_slice_params->picture.picture_coding_type = -V4L2_MPEG2_PICTURE_CODING_TYPE_I ; -break ; +return ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); -break ; +std_init_compound (ctrl, idx, ptr); +return ;
Same comment here: I see no reasons to have return instead of break.
drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1574
Quoted 2 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-0} +
You can get rid of this blank line.
drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1577
Quoted 3 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0
-0}static void std_log (const struct v4l2_ctrl *ctrl)
Regards,
Boris
Re: [PATCH 2/2] media: v4l2-ctrl: Move compound control initialization
Quoted 35 lines · Boris Brezillon
Hi Ezequiel,
Quoted 17 lines · On Wed, 29 May 2019 16:28:11 -0300 Ezequiel Garcia <[email protected]>…
Rework
std_initadding an explicit initialization for compound controls.While here, make sure the control is initialized to zero, before providing default values for all its fields.
Signed-off-by Ezequiel Garcia <[email protected]>--- drivers/media/v4l2-core/v4l2-ctrls.c | 55 +++++++++++++++++----------- 1 file changed, 34 insertions(+), 21 deletions(-)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0@@ -1506,25 +1506,49 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, }drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1515
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3
-1} -static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +static void std_init_compound (const struct v4l2_ctrl *ctrl,u32 idx,union v4l2_ctrl_ptr ptr) {struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params; +memset (ptr.p,0 , ctrl->elem_size); +Hm, shouldn't we have
Quoted 8 lines
idx *= ctrl->elem_size; memset(ptr.p + idx, 0, ctrl->elem_size);instead?
Looks like your solution always resets the first element of an array even if idx > 0.
Yeah, this patch is poorly thought. This and the other comments make sense.
Thanks for the review,
Ezequiel
Quoted 96 lines
Quoted 11 lines
drivers/media/v4l2-core/v4l2-ctrls.c +5
-0/* * The cast is needed to get rid of a gcc warning complaining that * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS is not part of the * v4l2_ctrl_type enum. */ switch ((u32 )ctrl->type) { +case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : + p_mpeg2_slice_params = ptr.p; +/* 4:2:0 */ + p_mpeg2_slice_params->sequence.chroma_format =1 ; +/* 8 bits */ drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1536
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +10
-0+ p_mpeg2_slice_params->picture.intra_dc_precision =0 ; +/* interlaced top field */ + p_mpeg2_slice_params->picture.picture_structure =1 ; + p_mpeg2_slice_params->picture.picture_coding_type = +V4L2_MPEG2_PICTURE_CODING_TYPE_I ; +return ; +default : + idx *= ctrl->elem_size; +memset (ptr.p + idx,0 , ctrl->elem_size); +return ;If you fix the first
memset()you can get rid of this one. Also don't see why you use return instead of break in this switch-case block (there's no reason to bail out early in this function).Quoted 45 lines
drivers/media/v4l2-core/v4l2-ctrls.c +12
-14+ } +} + +static void std_init (const struct v4l2_ctrl *ctrl,u32 idx, +union v4l2_ctrl_ptr ptr) +{ +switch (ctrl->type) {case V4L2_CTRL_TYPE_STRING : idx *= ctrl->elem_size;memset (ptr.p_char + idx,' ' , ctrl->minimum); ptr.p_char[idx + ctrl->minimum] ='\0' ; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER64 : ptr.p_s64[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_INTEGER :case V4L2_CTRL_TYPE_INTEGER_MENU :case V4L2_CTRL_TYPE_MENU :@@ -1533,32 +1557,21 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,case V4L2_CTRL_TYPE_BUTTON :case V4L2_CTRL_TYPE_CTRL_CLASS : ptr.p_s32[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U8 : ptr.p_u8[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U16 : ptr.p_u16[idx] = ctrl->default_value; -break ; +return ;case V4L2_CTRL_TYPE_U32 : ptr.p_u32[idx] = ctrl->default_value; -break ; -case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS : - p_mpeg2_slice_params = ptr.p; -/* 4:2:0 */ - p_mpeg2_slice_params->sequence.chroma_format =1 ; -/* 8 bits */ - p_mpeg2_slice_params->picture.intra_dc_precision =0 ; -/* interlaced top field */ - p_mpeg2_slice_params->picture.picture_structure =1 ;drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1572
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3
-6- p_mpeg2_slice_params->picture.picture_coding_type = -V4L2_MPEG2_PICTURE_CODING_TYPE_I ; -break ; +return ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); -break ; +std_init_compound (ctrl, idx, ptr); +return ;Same comment here: I see no reasons to have return instead of break.
drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1574
Quoted 2 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-0} +You can get rid of this blank line.
drivers/media/v4l2-core/v4l2-ctrls.c static void std_init(const struct v4l2_ctrl *ctrl, u32 idx, · line 1577
Quoted 3 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0
-0}static void std_log (const struct v4l2_ctrl *ctrl)Regards,
Boris