Re: [PATCH v3 4/8] media: v4l: ctrls-api: Allow array update in __v4l2_ctrl_modify_range

[Volodymyr Kharuk <vkh@melexis.com>]

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)
> 

-- 
--
BR,
Volodymyr Kharuk