| Message ID | 1758f7525f6c8c602f36eef5e07a97ddfb1b548f.1669381013.git.vkh@melexis.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: mlx7502x ToF camera support | expand |
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(-)