media: v4l2-ctrl: Initialize _BUTTON and _CTRL_CLASS
PATCH v2 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 v2 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.
--
Changes from v1:
* Drop the s/break/return replacements
* Drop unneeded default cases
* Fix memset to take account of the index
drivers/media/v4l2-core/v4l2-ctrls.c | 37 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 14 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls.c +23 -14
@@ -1506,17 +1506,38 @@ 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; + idx *= ctrl->elem_size; +memset (ptr.p + idx,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 ; +break ; + } +} + +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);
@@ -1543,20 +1564,8 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,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 ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); +std_init_compound (ctrl, idx, ptr);break ; } }
Signature · 1 line
2.20.1
Re: [PATCH v2 2/2] media: v4l2-ctrl: Move compound control initialization
Quoted 21 lines · On Thu, 30 May 2019 14:19:09 -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]>--
Changes from v1:
* Drop the s/break/return replacements
* Drop unneeded default cases
* Fix memset to take account of the indexdrivers/media/v4l2-core/v4l2-ctrls.c | 37 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 14 deletions(-)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0
drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1510
Quoted 6 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-1@@ -1506,17 +1506,38 @@ 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)
nit: union v4l2_ctrl_ptr is not aligned on the std_init_compound() open parens.
The patch looks good otherwise:
Quoted 55 lines
drivers/media/v4l2-core/v4l2-ctrls.c +22
-13{struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params; + idx *= ctrl->elem_size; +memset (ptr.p + idx,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 ; +break ; + } +} + +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);@@ -1543,20 +1564,8 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,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 ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); +std_init_compound (ctrl, idx, ptr);break ; } }
Re: [PATCH v2 2/2] media: v4l2-ctrl: Move compound control initialization
In reply to Ezequiel GarciaQuoted 21 lines · Ezequiel Garcia
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]>--
Changes from v1:
* Drop the s/break/return replacements
* Drop unneeded default cases
* Fix memset to take account of the indexdrivers/media/v4l2-core/v4l2-ctrls.c | 37 +++++++++++++++++----------- 1 file changed, 23 insertions(+), 14 deletions(-)drivers/media/v4l2-core/v4l2-ctrls.c +0
-0
drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1510
Quoted 6 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1
-1@@ -1506,17 +1506,38 @@ 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)
As Boris mentioned, fix the alignment with the open (.
Quoted 8 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3
-0{struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params; + idx *= ctrl->elem_size; +memset (ptr.p + idx,0 , ctrl->elem_size); +/* * The cast is needed to get rid of a gcc warning complaining that
drivers/media/v4l2-core/v4l2-ctrls.c static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx, · line 1528
Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6
-0* 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 ;
I'd drop this assignment. Everything is already zeroed, so why still do this?
Regards,
Hans
Quoted 38 lines
drivers/media/v4l2-core/v4l2-ctrls.c +13
-13+/* 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 ; + } +} + +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);@@ -1543,20 +1564,8 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,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 ;default : - idx *= ctrl->elem_size; -memset (ptr.p + idx,0 , ctrl->elem_size); +std_init_compound (ctrl, idx, ptr);break ; } }
Quoted 5 lines · On Thu, 30 May 2019 14:19:08 -0300 Ezequiel Garcia <[email protected]>…
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.
If they don't carry any value, why not having a case that does nothing? I find it disturbing to assign a default to something that does not have a value attached to it.
Quoted 19 lines
Signed-off-by Ezequiel Garcia <[email protected]>--- 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 :
Quoted 6 lines · Boris Brezillon
Quoted 3 lines · On Thu, 30 May 2019 14:19:08 -0300 Ezequiel Garcia <[email protected]>…
These two control types don't really need a default value, as they are not expected to carry any value.
Quoted 6 lines
Quoted 2 lines
However, it's slightly clearer to initialize them explicitly instead of falling back to the switch default.
If they don't carry any value, why not having a case that does nothing? I find it disturbing to assign a default to something that does not have a value attached to it.
I agree to a point. It is still good to initialize these, if only to make sure that there is a sane value in the control.
How about this:
case V4L2_CTRL_TYPE_BOOLEAN: ptr.p_s32[idx] = ctrl->default_value; break; case V4L2_CTRL_TYPE_BUTTON: case V4L2_CTRL_TYPE_CTRL_CLASS: ptr.p_s32[idx] = 0; break;
Regards,
Hans
Quoted 21 lines
Quoted 20 lines
Signed-off-by Ezequiel Garcia <[email protected]>--- 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 :