Hi Hans, On Tue, Oct 10, 2023 at 4:20 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > Hi Yunke, > > Just one remaining issue: > > On 10/10/2023 04:21, Yunke Cao wrote: > > <snip> > > > @@ -723,17 +741,23 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, > > * @ops: The control ops. > > * @id: The control ID. > > * @p_def: The control's default value. > > + * @p_min: The control's minimum value. > > + * @p_max: The control's maximum value. > > * > > - * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks > > - * to the @p_def field. Use v4l2_ctrl_ptr_create() to create @p_def from a > > - * pointer. Use v4l2_ctrl_ptr_create(NULL) if the default value of the > > - * compound control should be all zeroes. > > + * Same as v4l2_ctrl_new_std(), but with support to compound controls, thanks > > + * to the @p_def/min/max fields. Use v4l2_ctrl_ptr_create() to create > > + * @p_def/min/max from a pointer. Use v4l2_ctrl_ptr_create(NULL) if the > > + * default, minimum and maximum value of the compound control should be all > > + * zeroes. Use v4l2_ctrl_ptr_create(NULL) if the min/max value of the compound > > + * control is not defined, -ENODATA will be returned in this case. > > This still refers to ENODATA, but the text should be improved anyway, even > the original wasn't very good. How about: > > * Same as v4l2_ctrl_new_std(), but with support for compound controls. > * To fill in the @p_def, @p_min and @p_max fields, use v4l2_ctrl_ptr_create() > * to convert a pointer to a const union v4l2_ctrl_ptr. > * Use v4l2_ctrl_ptr_create(NULL) if you want the default, minimum or maximum > * value of the compound control to be all zeroes. > * If the compound control does not set the ``V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX`` > * flag, then it does not has minimum and maximum values. In that case just use > * v4l2_ctrl_ptr_create(NULL) for the @p_min and @p_max arguments. > Thanks for the suggestion! This sounds good to me. Do you mind also reviewing patch 07/11 (the VIVID test control) and https://patchwork.linuxtv.org/project/linux-media/list/?series=11069 (v4l-utils)? So that I can address the comments together in the next version. Best, Yunke > > * > > */ > > struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl, > > const struct v4l2_ctrl_ops *ops, > > u32 id, > > - const union v4l2_ctrl_ptr p_def); > > + const union v4l2_ctrl_ptr p_def, > > + const union v4l2_ctrl_ptr p_min, > > + const union v4l2_ctrl_ptr p_max); > > > > /** > > * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control. > > Other than this the controls part of this series looks good to me. > > Regards, > > Hans >
