Hans Verkuil wrote:
On Wednesday, June 15, 2011 11:30:07 Sakari Ailus wrote:
Hi Hans,
Many thanks for the patch. I'm very happy to see this!
I have just one comment below.
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 45e9c1e..042b893 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -43,17 +43,6 @@ struct v4l2_subscribed_event {
u32 id;
};
-struct v4l2_events {
- wait_queue_head_t wait;
- struct list_head subscribed; /* Subscribed events */
- struct list_head free; /* Events ready for use */
- struct list_head available; /* Dequeueable event */
- unsigned int navailable;
- unsigned int nallocated; /* Number of allocated events */
- u32 sequence;
-};
-
-int v4l2_event_init(struct v4l2_fh *fh);
int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
void v4l2_event_free(struct v4l2_fh *fh);
int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index d247111..bfc0457 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -29,15 +29,22 @@
#include<linux/list.h>
struct video_device;
-struct v4l2_events;
struct v4l2_ctrl_handler;
struct v4l2_fh {
struct list_head list;
struct video_device *vdev;
- struct v4l2_events *events; /* events, pending and subscribed */
struct v4l2_ctrl_handler *ctrl_handler;
enum v4l2_priority prio;
+
+ /* Events */
+ wait_queue_head_t wait;
+ struct list_head subscribed; /* Subscribed events */
+ struct list_head free; /* Events ready for use */
+ struct list_head available; /* Dequeueable event */
+ unsigned int navailable;
+ unsigned int nallocated; /* Number of allocated events */
+ u32 sequence;
A question: why to move the fields from v4l2_events to v4l2_fh? Events may
be more important part of V4L2 than before but they're still not file
handles. :-) The event related field names have no hing they'd be related to
events --- "free", for example.
The only reason that the v4l2_events struct existed was that there were so few
drivers that needed events. So why allocate memory that you don't need? That
all changes with the control event: almost all drivers will need that since
almost all drivers have events.
Merging it makes the code easier and v4l2_fh_init can become a void function
(so no more error checking needed). And since these fields are always there, I
no longer need to check whether fh->events is NULL or not.
I can add a patch renaming some of the event fields if you prefer, but I don't
think they are that bad. Note that 'free' and 'nallocated' are removed
completely in a later patch.
Thanks for the explanation. What I had in mind that what other fields
possibly would be added to v4l2_fh in the future? If there will be many,
in that case keeping event related fields in a separate structure might
make sense. I have none in mind right now, though, so perhaps this could
be given a second thought if we're adding more things to the v4l2_fh
structure?
I think this patchset is a significant improvement to the old behaviour.
Regards,
--
Sakari Ailus
sakari.ailus@xxxxxx
--
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
