Em Thu, 6 Aug 2009 22:04:13 +0200 Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > Hi Mauro, > > Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the following: > > - v4l: introduce string control support. > - v4l2-spec: document the new string control type. > - v4l2-ctl: modulator bug fixes > - v4l2-ctl: add support for string controls > > It's the same as the previous version, except for reverting the variable > renaming and for improving the comments above the ctrl_is_value64 and > ctrl_is_pointer functions. I've decided to keep the ctrl_is_value64() > function because I want to keep the correct value64 conversion code in > compat32 rather than having to do that work again at some point in the > future. I agree with Trent, this really sucks: static inline int ctrl_is_value64(u32 id) { - return 0; + switch (id) { + case V4L2_CID_RDS_TX_PS_NAME: + case V4L2_CID_RDS_TX_RADIO_TEXT: + return 1; + default: + return 0; + } } Yet, by looking on how the struct is defined: struct v4l2_ext_control { __u32 id; __u32 reserved2[2]; union { __s32 value; __s64 value64; void *reserved; }; } __attribute__ ((packed)); Except on the strings case, I don't see why we need a compat stuff for this routine at the first place, copying field by field. Correct me if I'm wrong, but, on both 32 and 64 bit cases, the fields will be at the same position on both 32 and 64 bits. So, the code could be just: copy_in_user(&kcontrols, &ucontrols, sizeof(ucontrols)); if (ctrl_is_pointer(id)) { /* get_user/compat_ptr magic */ } I also agree with Trent that, instead of having to maintain some sort of list or code to identify string controls, the better would be to reserve one bit to indicate pointer controls (like bit 31). People tend to forget about v4l2-compat32 layer, and I bet that, if we do some sort of list, people will forget to update it when adding a new control, and compat32 layer will break. So, ctrl_is_pointer() can be just: #define ctrl_is_pointer(id) (id & (1 << 31)) > Also note that it is very likely that these two functions will disappear > again in a few months: once the v4l framework has proper control support > these functions should no longer be needed since this information can > then be obtained directly from the framework. This don't change the fact that you'll need a sort of list or code that associates a control id with a type, called by the compat layer. Testing for a bit costs less than any table seek method. Cheers, Mauro -- 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
