Re: [PATCH] media: v4l2-ctrl: Add p_def to v4l2_ctrl_config

[Hans Verkuil]

On 10/16/19 2:43 PM, Hans Verkuil 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.

Ah, v4l2_ctrl_ptr_create() expects a non-const pointer.

What happens if union v4l2_ctrl_ptr p_def; in struct v4l2_ctrl changes
to const union v4l2_ctrl_ptr p_def; ?

You'll need to add const elsewhere as well, but since the default value
is const, this might work.

I'm not entirely sure this is correct code, though, but it's worth trying
it.

Regards,

	Hans

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