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@xxxxxxxxxxx>
> > ---
> > 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)
>
--
--
BR,
Volodymyr Kharuk
