On 03/11/2014 08:42 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 17 Feb 2014 10:57:20 +0100
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Add a new struct and ioctl to extend the amount of information you can
>> get for a control.
>>
>> It gives back a unit string, the range is now a s64 type, and the matrix
>> and element size can be reported through cols/rows/elem_size.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> include/uapi/linux/videodev2.h | 31 +++++++++++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 4d7782a..858a6f3 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1272,6 +1272,35 @@ struct v4l2_queryctrl {
>> __u32 reserved[2];
>> };
>>
>> +/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
>> +struct v4l2_query_ext_ctrl {
>> + __u32 id;
>> + __u32 type;
>> + char name[32];
>> + char unit[32];
>> + union {
>> + __s64 val;
>> + __u32 reserved[4];
>
> Why to reserve 16 bytes here? for anything bigger than 64
> bits, we could use a pointer.
>
> Same applies to the other unions.
The idea was to allow space for min/max/step/def values for compound types
if applicable. But that may have been overengineering.
>
>> + } min;
>> + union {
>> + __s64 val;
>> + __u32 reserved[4];
>> + } max;
>> + union {
>> + __u64 val;
>> + __u32 reserved[4];
>> + } step;
>> + union {
>> + __s64 val;
>> + __u32 reserved[4];
>> + } def;
>
> Please call it default. It is ok to simplify names inside a driver,
> but better to not do it at the API.
default_value, then. 'default' is a keyword. I should probably rename min and max
to minimum and maximum to stay in sync with v4l2_queryctrl.
>
>> + __u32 flags;
>
>> + __u32 cols;
>> + __u32 rows;
>> + __u32 elem_size;
>
> The three above seem to be too specific for an array.
>
> I would put those on a separate struct and add here an union,
> like:
>
> union {
> struct v4l2_array arr;
> __u32 reserved[8];
> }
I have to sleep on this. I'm not sure this helps in any way.
>
>> + __u32 reserved[17];
>
> This also seems too much. Why 17?
It aligned the struct up to some nice number. Also, experience tells me that
whenever I limit the number of reserved fields it bites me later.
>
>> +};
>
>> +
>> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
>> struct v4l2_querymenu {
>> __u32 id;
>> @@ -1965,6 +1994,8 @@ struct v4l2_create_buffers {
>> Never use these in applications! */
>> #define VIDIOC_DBG_G_CHIP_INFO _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>
>> +#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +
>> /* Reminder: when adding new ioctls please add support for them to
>> drivers/media/video/v4l2-compat-ioctl32.c as well! */
>>
>
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
