| Message ID | 1758f7525f6c8c602f36eef5e07a97ddfb1b548f.1669381013.git.vkh@melexis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: mlx7502x ToF camera support | expand |
On 25/11/2022 14:34, Volodymyr Kharuk wrote: > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > from union. It will allow to work with any type. > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > --- > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > index d0a3aa3806fb..09735644a2f1 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > case V4L2_CTRL_TYPE_U8: > case V4L2_CTRL_TYPE_U16: > case V4L2_CTRL_TYPE_U32: > - if (ctrl->is_array) > - return -EINVAL; > ret = check_range(ctrl->type, min, max, step, def); > if (ret) > return ret; > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > ctrl->default_value = def; > } > cur_to_new(ctrl); > - if (validate_new(ctrl, ctrl->p_new)) { > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > - *ctrl->p_new.p_s64 = def; > - else > - *ctrl->p_new.p_s32 = def; > - } > + if (validate_new(ctrl, ctrl->p_new)) > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); Hmm, this sets *all* elements of the array to the default_value, not just the elements whose value is out of range. Is that what you want? Should this perhaps depend on the type of control? I.e. in some cases it might make sense to do this, in other cases it makes more sense to only adjust the elements that are out of range. How does that work for this TINT control? Regards, Hans > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > - value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64; > - else > - value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32; > + value_changed = !ctrl->type_ops->equal(ctrl, ctrl->p_cur, ctrl->p_new); > if (value_changed) > ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE); > else if (range_changed)
Hi Hans, On Fri, Nov 25, 2022 at 03:35:34PM +0100, Hans Verkuil wrote: > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > > from union. It will allow to work with any type. > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > --- > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > > 1 file changed, 3 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > index d0a3aa3806fb..09735644a2f1 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > case V4L2_CTRL_TYPE_U8: > > case V4L2_CTRL_TYPE_U16: > > case V4L2_CTRL_TYPE_U32: > > - if (ctrl->is_array) > > - return -EINVAL; > > ret = check_range(ctrl->type, min, max, step, def); > > if (ret) > > return ret; > > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > ctrl->default_value = def; > > } > > cur_to_new(ctrl); > > - if (validate_new(ctrl, ctrl->p_new)) { > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > - *ctrl->p_new.p_s64 = def; > > - else > > - *ctrl->p_new.p_s32 = def; > > - } > > + if (validate_new(ctrl, ctrl->p_new)) > > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); > > Hmm, this sets *all* elements of the array to the default_value, not > just the elements whose value is out of range. > > Is that what you want? Should this perhaps depend on the type of > control? I.e. in some cases it might make sense to do this, in other > cases it makes more sense to only adjust the elements that are out > of range. > > How does that work for this TINT control? Actually for types like INTEGER/U8/U16/U32/BOOLEAN/BUTTON/BITMASK, the function validate_new will return zero always as well as they fix the range on the fly. So that is ok for mlx7502x sensors. For types like compound/string/menu indeed, it will sets all elements of array to default array. To fix it I propose to fix it by using the functions std_validate_elem to check per element and then set the default manually. But then it means, that __v4l2_ctrl_modify_range will work only for standart v4l2 types, and not if driver use their own implementation. Is it fine for you? What do you think? > > Regards, > > Hans > > > > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > - value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64; > > - else > > - value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32; > > + value_changed = !ctrl->type_ops->equal(ctrl, ctrl->p_cur, ctrl->p_new); > > if (value_changed) > > ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE); > > else if (range_changed) >
On Thu, Dec 01, 2022 at 05:44:52PM +0200, Volodymyr Kharuk wrote: > Hi Hans, > > On Fri, Nov 25, 2022 at 03:35:34PM +0100, Hans Verkuil wrote: > > On 25/11/2022 14:34, Volodymyr Kharuk wrote: > > > For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use > > > __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 > > > from union. It will allow to work with any type. > > > > > > Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> > > > --- > > > drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ > > > 1 file changed, 3 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > index d0a3aa3806fb..09735644a2f1 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c > > > @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > > case V4L2_CTRL_TYPE_U8: > > > case V4L2_CTRL_TYPE_U16: > > > case V4L2_CTRL_TYPE_U32: > > > - if (ctrl->is_array) > > > - return -EINVAL; > > > ret = check_range(ctrl->type, min, max, step, def); > > > if (ret) > > > return ret; > > > @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, > > > ctrl->default_value = def; > > > } > > > cur_to_new(ctrl); > > > - if (validate_new(ctrl, ctrl->p_new)) { > > > - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) > > > - *ctrl->p_new.p_s64 = def; > > > - else > > > - *ctrl->p_new.p_s32 = def; > > > - } > > > + if (validate_new(ctrl, ctrl->p_new)) > > > + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); > > > > Hmm, this sets *all* elements of the array to the default_value, not > > just the elements whose value is out of range. > > > > Is that what you want? Should this perhaps depend on the type of > > control? I.e. in some cases it might make sense to do this, in other > > cases it makes more sense to only adjust the elements that are out > > of range. > > > > How does that work for this TINT control? > Actually for types like INTEGER/U8/U16/U32/BOOLEAN/BUTTON/BITMASK, the function > validate_new will return zero always as well as they fix the range on the fly. > So that is ok for mlx7502x sensors. > > For types like compound/string/menu indeed, it will sets all elements of array > to default array. To fix it I propose to fix it by using the functions > std_validate_elem to check per element and then set the default manually. > But then it means, that __v4l2_ctrl_modify_range will work only for standart > v4l2 types, and not if driver use their own implementation. > > Is it fine for you? What do you think? I've another idea. If validate_new is fixing on the fly and we can't modify range for copmound/string types, so we can forbidden array for MENU types. Then validate_new will do the job for the rest types. As for now there are no needs in arrays for MENU types. What do you think?
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c index d0a3aa3806fb..09735644a2f1 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c @@ -942,8 +942,6 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, case V4L2_CTRL_TYPE_U8: case V4L2_CTRL_TYPE_U16: case V4L2_CTRL_TYPE_U32: - if (ctrl->is_array) - return -EINVAL; ret = check_range(ctrl->type, min, max, step, def); if (ret) return ret; @@ -960,17 +958,10 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl, ctrl->default_value = def; } cur_to_new(ctrl); - if (validate_new(ctrl, ctrl->p_new)) { - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) - *ctrl->p_new.p_s64 = def; - else - *ctrl->p_new.p_s32 = def; - } + if (validate_new(ctrl, ctrl->p_new)) + ctrl->type_ops->init(ctrl, 0, ctrl->p_new); - if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64) - value_changed = *ctrl->p_new.p_s64 != *ctrl->p_cur.p_s64; - else - value_changed = *ctrl->p_new.p_s32 != *ctrl->p_cur.p_s32; + value_changed = !ctrl->type_ops->equal(ctrl, ctrl->p_cur, ctrl->p_new); if (value_changed) ret = set_ctrl(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE); else if (range_changed)
For V4L2_CID_TOF_TINT, which is dynamic array, it is required to use __v4l2_ctrl_modify_range. So the idea is to use type_ops instead of u64 from union. It will allow to work with any type. Signed-off-by: Volodymyr Kharuk <vkh@melexis.com> --- drivers/media/v4l2-core/v4l2-ctrls-api.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)