> Em Sun, 2 Aug 2009 12:10:04 +0200
> Hans Verkuil <[email protected]> escreveu:
>
>> Hi Mauro,
>>
>> Please pull from http://www.linuxtv.org/hg/~hverkuil/v4l-dvb for the
>> following:
>>
>> - v4l: introduce string control support.
>
> Hans,
>
> This looks very weird:
>
> +/* The following two functions really belong in v4l2-common, but that
> causes
> + a circular dependency between modules. We need to think about this,
> but
> + for now this will do. */
> +
> +/* Return non-zero if this control is of type VALUE64.
> + *
> + * This information is used inside v4l2_compat_ioctl32. */
> +static int ctrl_is_value64(u32 id)
> +{
> + return 0;
> +}
> +
> +/* Return non-zero if this control is a pointer type. Currently only
> + * type STRING is a pointer type.
> + *
> + * This information is used inside v4l2_compat_ioctl32. */
> +static int ctrl_is_pointer(u32 id)
> +{
> + return 0;
> +}
> +
> ...
> + } else if (ctrl_is_value64(kctrl->id)) {
> + if (copy_in_user(&kctrl->value64, &uctrl->value64,
> + sizeof(uctrl->value64)))
> + return -EFAULT;
> ...
> + if (ctrl_is_value64(kctrl->id)) {
> + if (copy_in_user(&uctrl->value64, &kctrl->value64,
> + sizeof(uctrl->value64)))
> + return -EFAULT;
> + } else if (!ctrl_is_pointer(kctrl->id) &&
> + copy_in_user(&uctrl->value, &kctrl->value,
> + sizeof(uctrl->value))) {
>
> Why do you need two routines that will always return zero? Why to create a
> code
> that will never be used? v4l2-compat-ioctl32.c is already complex enough
> without adding any bogus code.
When the RDS encoder driver from Eduardo is added, then he will add the
string controls to ctrl_is_pointer() since his driver is the first to
actually implement string controls.
There is indeed no value64 control yet, but I think it is wise to at least
have the code in place to correctly handle such controls. I can remove
that if you want.
>
> I noticed that you're also renaming the names of some fields there:
>
> - struct v4l2_ext_control __user *ucontrols;
> - struct v4l2_ext_control __user *kcontrols = kp->controls;
> + struct v4l2_ext_control __user *uctrl;
> + struct v4l2_ext_control __user *kctrl = kp->controls;
>
> I dunno why you decided to do it, but having this together with the
> changes at
> v4l2-compat-ioctl32.c makes the code even harder to understand. Could you
> please move the v4l2-compat-ioctl32 "cosmetic" changes like above into a
> separate patch?
OK. It was done to avoid creating lots of 'longer than 80 characters'
warnings.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
