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

[Volodymyr Kharuk <vkh@melexis.com>]

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?

-- 
--
BR,
Volodymyr Kharuk