Hi Hans
On Wed, Oct 16, 2019 at 2:43 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> On 10/16/19 2:39 PM, Ricardo Ribalda Delgado wrote:
> > Hi Hans:
> >
> > On Wed, Oct 16, 2019 at 2:32 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>
> >> On 10/16/19 2:20 PM, Ricardo Ribalda Delgado wrote:
> >>> Hi Hans
> >>>
> >>> Not that awkward, the user has to use the brand new
> >>> v4l2_ctrl_ptr_create() ;). But if you prefer void * I can make the
> >>> change.
> >>
> >> Well, a struct v4l2_ctrl_config is typically a static const, so you can't use
> >> v4l2_ctrl_ptr_create().
> >>
> >> Hmm, perhaps it is as easy as:
> >>
> >> static const struct v4l2_area def_area = {
> >> ...
> >> };
> >>
> >> static const struct v4l2_ctrl_config ctrl = {
> >> ...
> >>
> >> .p_def.p_area = &def_area,
> >> ...
> >> };
> >>
> >> Can you do a quick compile check that I am not overlooking anything?
> >>
> >> If this works, then I'll take this patch.
> >
> > Testing with gcc 9.2.1
> >
> > This works fine, no warning/error:
> >
> > static struct v4l2_area unit_size = {
> > .width = UNIT_SIZE,
> > .height = UNIT_SIZE,
> > };
> > static struct v4l2_ctrl_config area_ctrl = {
> > .type = V4L2_CTRL_TYPE_AREA,
> > .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > .p_def.p_area = &unit_size,
> > };
> >
> > but if unit_size is set as CONST:
> > static const struct v4l2_area
> >
> > Then:
> > drivers/qtec/qtec_sony.c: In function ‘qtec_sony_probe’:
> > drivers/qtec/qtec_sony.c:3151:19: warning: initialization discards
> > ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> > 3151 | .p_def.p_area = &unit_size,
> > |
>
> Hmm. So we need a const void *p_def instead.
>
If we make p_def in ctrl_config const then this will fail:
drivers/qtec/qtec_sony.c:3273:18: error: assignment of read-only member ‘p_def’
3273 | area_ctrl.p_def = v4l2_ctrl_ptr_create((void *)&unit_size);
I think we need to leave it as in the proposed patch.
> Regards,
>
> Hans
>
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>> Regards
> >>>
> >>> On Wed, Oct 16, 2019 at 2:17 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
> >>>>
> >>>> On 10/14/19 4:14 PM, Ricardo Ribalda Delgado wrote:
> >>>>> This allows setting the default value on compound controls created via
> >>>>> v4l2_ctrl_new_custom.
> >>>>>
> >>>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx>
> >>>>> ---
> >>>>> drivers/media/v4l2-core/v4l2-ctrls.c | 2 +-
> >>>>> include/media/v4l2-ctrls.h | 2 ++
> >>>>> 2 files changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> index bf50d37ef6c1..12cf38f73f7b 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>> @@ -2583,7 +2583,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> >>>>> type, min, max,
> >>>>> is_menu ? cfg->menu_skip_mask : step, def,
> >>>>> cfg->dims, cfg->elem_size,
> >>>>> - flags, qmenu, qmenu_int, ptr_null, priv);
> >>>>> + flags, qmenu, qmenu_int, cfg->p_def, priv);
> >>>>> if (ctrl)
> >>>>> ctrl->is_private = cfg->is_private;
> >>>>> return ctrl;
> >>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>> index 26205ba3a0a0..2fca5b823961 100644
> >>>>> --- a/include/media/v4l2-ctrls.h
> >>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>> @@ -375,6 +375,7 @@ struct v4l2_ctrl_handler {
> >>>>> * @max: The control's maximum value.
> >>>>> * @step: The control's step value for non-menu controls.
> >>>>> * @def: The control's default value.
> >>>>> + * @p_def: The control's default value for compound controls.
> >>>>> * @dims: The size of each dimension.
> >>>>> * @elem_size: The size in bytes of the control.
> >>>>> * @flags: The control's flags.
> >>>>> @@ -403,6 +404,7 @@ struct v4l2_ctrl_config {
> >>>>> s64 max;
> >>>>> u64 step;
> >>>>> s64 def;
> >>>>> + union v4l2_ctrl_ptr p_def;
> >>>>> u32 dims[V4L2_CTRL_MAX_DIMS];
> >>>>> u32 elem_size;
> >>>>> u32 flags;
> >>>>>
> >>>>
> >>>> I'm not sure about this. It might be a bit awkward to initialize p_def given that it is a union.
> >>>>
> >>>> Perhaps a simple void pointer would be easier?
> >>>>
> >>>> Regards,
> >>>>
> >>>> Hans
> >>
>
