Hi Hans, On Monday 14 May 2012 15:51:27 Hans Verkuil wrote: > On Mon May 14 2012 15:00:05 Laurent Pinchart wrote: > > On Thursday 10 May 2012 09:05:11 Hans Verkuil wrote: > > > From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > > > > Rather than testing whether an ioctl is implemented in the driver or not > > > every time the ioctl is called, do it upfront when the device is > > > registered. > > > > > > This also allows a driver to disable certain ioctls based on the > > > capabilities of the detected board, something you can't do today without > > > creating separate v4l2_ioctl_ops structs for each new variation. > > > > > > For the most part it is pretty straightforward, but for control ioctls a > > > flag is needed since it is possible that you have per-filehandle > > > controls, and that can't be determined upfront of course. > > > > > > Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > --- > > > > > > drivers/media/video/v4l2-dev.c | 171 +++++++++++++++++ > > > drivers/media/video/v4l2-ioctl.c | 391 > > > ++++++++++------------------------- > > > include/media/v4l2-dev.h | 11 ++ > > > 3 files changed, 297 insertions(+), 276 deletions(-) > > > > > > diff --git a/drivers/media/video/v4l2-dev.c > > > b/drivers/media/video/v4l2-dev.c index a51a061..4d98ee1 100644 > > > --- a/drivers/media/video/v4l2-dev.c > > > +++ b/drivers/media/video/v4l2-dev.c > > > @@ -516,6 +516,175 @@ static int get_index(struct video_device *vdev) > > > > > > return find_first_zero_bit(used, VIDEO_NUM_DEVICES); > > > > > > } > > > > > > +#define SET_VALID_IOCTL(ops, cmd, op) \ > > > + if (ops->op) \ > > > + set_bit(_IOC_NR(cmd), valid_ioctls) > > > + > > > +/* This determines which ioctls are actually implemented in the driver. > > > + It's a one-time thing which simplifies video_ioctl2 as it can just > > > do > > > + a bit test. > > > + > > > + Note that drivers can override this by setting bits to 1 in > > > + vdev->valid_ioctls. If an ioctl is marked as 1 when this function is > > > + called, then that ioctl will actually be marked as unimplemented. > > > + > > > + It does that by first setting up the local valid_ioctls bitmap, and > > > + at the end do a: > > > + > > > + vdev->valid_ioctls = valid_ioctls & ~(vdev->valid_ioctls) > > > > Wouldn't it be more logical to initialize valid_ioctls to all 1s and clear > > bits in v4l2_dont_use_cmd() ? Otherwise the meaning of the field changes > > depending on whether the device is registered or not. > > The reason is that drivers initialize struct video_device to 0. There is > also no initialization function that can set it to all 1. So then you would > have to modify all drivers, and that's way overkill. > > If you have a better suggestion, then I'm all ears! Right, that's a problem. One possible solution would be to rename valid_ioctls to invalid_ioctls (and obviously update the code accordingly). > > Another bikeshedding comment, what about renaming v4l2_dont_use_cmd() with > > something that includes ioctl in the name ? > > > > - v4l2_dont_use_ioctl > > - v4l2_dont_use_ioctl_cmd > > - v4l2_ioctl_cmd_not_used > > - v4l2_ioctl_dont_use > > - v4l2_ioctl_dont_use_cmd > > - v4l2_disable_ioctl > > - v4l2_disable_ioctl_cmd > > How about: > > v4l2_is_known_ioctl -> v4l2_is_known_ioctl_cmd > v4l2_dont_use_lock -> v4l2_disable_ioctl_cmd_locking > v4l2_dont_use_cmd -> v4l2_disable_ioctl_cmd > > or: > > v4l2_ioctl_cmd_is_known > v4l2_ioctl_cmd_disable_locking > v4l2_ioctl_cmd_disable > > The last set looks better but sounds worse :-) I have no strong preference, the format sounds a little bit better to me. I would probably have dropped cmd, but that's up to you. -- Regards, Laurent Pinchart -- 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
