Re: [PATCH 1/1] v4l2: use __u32 rather than enums in ioctl()structs

[Sakari Ailus]

Hi Hans,

On Sat, May 05, 2012 at 12:55:01PM +0200, Hans Verkuil wrote:
> On Sat May 5 2012 12:25:45 Sakari Ailus wrote:
> > From: Rémi Denis-Courmont <remi@xxxxxxxxxx>
> > 
> > V4L2 uses the enum type in IOCTL arguments in IOCTLs that were defined until
> > the use of enum was considered less than ideal. Recently Rémi Denis-Courmont
> > brought up the issue by proposing a patch to convert the enums to unsigned:
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg46167.html>
> > 
> > This sparked a long discussion where another solution to the issue was
> > proposed: two sets of IOCTL structures, one with __u32 and the other with
> > enums, and conversion code between the two:
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg47168.html>
> > 
> > Both approaches implement a complete solution that resolves the problem. The
> > first one is simple but requires assuming enums and __u32 are the same in
> > size (so we won't break the ABI) while the second one is more complex and
> > less clean but does not require making that assumption.
> > 
> > The issue boils down to whether enums are fundamentally different from __u32
> > or not, and can the former be substituted by the latter. During the
> > discussion it was concluded that the __u32 has the same size as enums on all
> > archs Linux is supported: it has not been shown that replacing those enums
> > in IOCTL arguments would break neither source or binary compatibility. If no
> > such reason is found, just replacing the enums with __u32s is the way to go.
> > 
> > This is what this patch does. This patch is slightly different from Remi's
> > first RFC (link above): it uses __u32 instead of unsigned and also changes
> > the arguments of VIDIOC_G_PRIORITY and VIDIOC_S_PRIORITY.
> > 
> > Signed-off-by: Rémi Denis-Courmont <remi@xxxxxxxxxx>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
> > ---
> >  include/linux/videodev2.h |   46 ++++++++++++++++++++++----------------------
> >  1 files changed, 23 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 5a09ac3..585e4b4 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -292,10 +292,10 @@ struct v4l2_pix_format {
> >  	__u32         		width;
> >  	__u32			height;
> >  	__u32			pixelformat;
> > -	enum v4l2_field  	field;
> > +	__u32			field;
> 
> One suggestion: add a comment like this:
> 
> 	__u32			field; /* refer to enum v4l2_field */
> 
> This keeps the link between the u32 and the enum values.
> 
> Note that the DocBook documentation also has to be updated.
> 
> Looks good to me otherwise.

Thanks for the comments. I'll make the above changes.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@xxxxxx	jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/