Re: [RFC PATCH 1/5] v4l2: add matrix support.

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 07/08/2013 09:15 AM, Hans Verkuil wrote:
> On Sun July 7 2013 23:50:51 Sylwester Nawrocki wrote:
>> On 06/28/2013 02:27 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil<hans.verkuil@cisco.com>
>>>
>>> This patch adds core support for matrices: querying, getting and setting.
>>>
>>> Two initial matrix types are defined for motion detection (defining regions
>>> and thresholds).
>>>
>>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-dev.c   |  3 ++
>>>   drivers/media/v4l2-core/v4l2-ioctl.c | 23 ++++++++++++-
>>>   include/media/v4l2-ioctl.h           |  8 +++++
>>>   include/uapi/linux/videodev2.h       | 64 ++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 97 insertions(+), 1 deletion(-)
>>
[...]
>>> +/* Define to which motion detection region each element belongs.
>>> + * Each element is a __u8. */
>>> +#define V4L2_MATRIX_TYPE_MD_REGION     (1)
>>> +/* Define the motion detection threshold for each element.
>>> + * Each element is a __u16. */
>>> +#define V4L2_MATRIX_TYPE_MD_THRESHOLD  (2)
>>> +
>>> +/**
>>> + * struct v4l2_query_matrix - VIDIOC_QUERY_MATRIX argument
>>> + * @type:	matrix type
>>> + * @ref:	reference to some object (if any) owning the matrix
>>> + * @columns:	number of columns in the matrix
>>> + * @rows:	number of rows in the matrix
>>> + * @elem_min:	minimum matrix element value
>>> + * @elem_max:	maximum matrix element value
>>> + * @elem_size:	size in bytes each matrix element
>>> + * @reserved:	future extensions, applications and drivers must zero this.
>>> + */
>>> +struct v4l2_query_matrix {
>>> +	__u32 type;
>>> +	union {
>>> +		__u32 reserved[4];
>>> +	} ref;
>>> +	__u32 columns;
>>> +	__u32 rows;
>>> +	union {
>>> +		__s64 val;
>>> +		__u64 uval;
>>> +		__u32 reserved[4];
>>> +	} elem_min;
>>> +	union {
>>> +		__s64 val;
>>> +		__u64 uval;
>>> +		__u32 reserved[4];
>>> +	} elem_max;
>>> +	__u32 elem_size;
>>
>> How about reordering it to something like:
>>
>> 	struct {
>> 		union {
>> 			__s64 val;
>> 			__u64 uval;
>> 			__u32 reserved[4];
>> 		} min;
>> 		union {
>> 			__s64 val;
>> 			__u64 uval;
>> 			__u32 reserved[4];
>> 		} max;
>> 		__u32 size;
>> 	} element;
>>
>> ?
> 
> Makes sense, although I prefer 'elem' over the longer 'element'. Would that
> be OK with you?

Yes, I'm fine with that. Just thought using full words where sensible is
a good practice. But the shorter form seems better indeed in this case.

Regards,
Sylwester