Skip to contentSkip to conversation

gspca: replaced static allocation by video_device_alloc/video_device_release

PATCH In review Ezequiel 3 participants 4 messages Nov 19, 2011 – Nov 21, 2011 Media (V4L/DVB)

lore
Share & tools
lore

Ezequiel

author
#0 lore raw

PATCH v2 1/1 gspca: replaced static allocation by video_device_alloc/video_device_release

Pushed video_device initialization into a separate function. Replace static allocation of struct video_device by video_device_alloc/video_device_release usage.

---
Previous version was sent stupidly incomplete by mistake.
---

drivers/media/video/gspca/gspca.c +39 -18
index 881e04c..1f27f05 100644
@@ -1292,10 +1292,12 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 
 static void gspca_release(struct video_device *vfd)
 {
-	struct gspca_dev *gspca_dev = container_of(vfd, struct gspca_dev, vdev);
+	struct gspca_dev *gspca_dev = video_get_drvdata(vfd);
 
 	PDEBUG(D_PROBE, "%s released",
-		video_device_node_name(&gspca_dev->vdev));
+		video_device_node_name(gspca_dev->vdev));
+
+	video_device_release(vfd);
 
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
@@ -1304,9 +1306,11 @@ static void gspca_release(struct video_device *vfd)
 static int dev_open(struct file *file)
 {
 	struct gspca_dev *gspca_dev;
+	struct video_device *vdev;
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
+	vdev = video_devdata(file);
+	gspca_dev = video_get_drvdata(vdev);
 	if (!gspca_dev->present)
 		return -ENODEV;
 
@@ -1318,10 +1322,10 @@ static int dev_open(struct file *file)
 #ifdef GSPCA_DEBUG
 	/* activate the v4l2 debug */
 	if (gspca_debug & D_V4L2)
-		gspca_dev->vdev.debug |= V4L2_DEBUG_IOCTL
+		gspca_dev->vdev->debug |= V4L2_DEBUG_IOCTL
 					| V4L2_DEBUG_IOCTL_ARG;
 	else
-		gspca_dev->vdev.debug &= ~(V4L2_DEBUG_IOCTL
+		gspca_dev->vdev->debug &= ~(V4L2_DEBUG_IOCTL
 					| V4L2_DEBUG_IOCTL_ARG);
 #endif
 	return 0;
@@ -2242,13 +2246,6 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
 	.vidioc_g_chip_ident	= vidioc_g_chip_ident,
 };
 
-static const struct video_device gspca_template = {
-	.name = "gspca main driver",
-	.fops = &dev_fops,
-	.ioctl_ops = &dev_ioctl_ops,
-	.release = gspca_release,
-};
-
 /* initialize the controls */
 static void ctrls_init(struct gspca_dev *gspca_dev)
 {
@@ -2265,6 +2262,26 @@ static void ctrls_init(struct gspca_dev *gspca_dev)
 	}
 }
 
+/* initialize a video_device struct */
+static int vdev_init(struct gspca_dev *dev, struct device *parent)
+{
+	struct video_device *vdev = video_device_alloc();
+	if (vdev == NULL)
+		return -ENOMEM;
+
+	/* fill struct video_device */
+	strlcpy(vdev->name, "gspca main driver", sizeof(vdev->name));
+	vdev->fops = &dev_fops,
+	vdev->ioctl_ops = &dev_ioctl_ops,
+	vdev->release = gspca_release,
+	vdev->parent = parent;
+
+	dev->vdev = vdev;
+	video_set_drvdata(vdev, dev);
+
+	return 0;
+}
+
 /*
  * probe and create a new gspca device
  *
@@ -2343,11 +2360,15 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	init_waitqueue_head(&gspca_dev->wq);
 
 	/* init video stuff */
-	memcpy(&gspca_dev->vdev, &gspca_template, sizeof gspca_template);
-	gspca_dev->vdev.parent = &intf->dev;
+	ret = vdev_init(gspca_dev, &intf->dev);
+	if (ret < 0) {
+		pr_err("initialize video device err %d\n", ret);
+		goto out;
+	}
+
 	gspca_dev->module = module;
 	gspca_dev->present = 1;
-	ret = video_register_device(&gspca_dev->vdev,
+	ret = video_register_device(gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
 				  -1);
 	if (ret < 0) {
@@ -2356,7 +2377,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	}
 
 	usb_set_intfdata(intf, gspca_dev);
-	PDEBUG(D_PROBE, "%s created", video_device_node_name(&gspca_dev->vdev));
+	PDEBUG(D_PROBE, "%s created", video_device_node_name(gspca_dev->vdev));
 
 	gspca_input_create_urb(gspca_dev);
 
@@ -2411,7 +2432,7 @@ void gspca_disconnect(struct usb_interface *intf)
 #endif
 
 	PDEBUG(D_PROBE, "%s disconnect",
-		video_device_node_name(&gspca_dev->vdev));
+		video_device_node_name(gspca_dev->vdev));
 	mutex_lock(&gspca_dev->usb_lock);
 
 	gspca_dev->present = 0;
@@ -2436,7 +2457,7 @@ void gspca_disconnect(struct usb_interface *intf)
 
 	/* release the device */
 	/* (this will call gspca_release() immediately or on last close) */
-	video_unregister_device(&gspca_dev->vdev);
+	video_unregister_device(gspca_dev->vdev);
 
 /*	PDEBUG(D_PROBE, "disconnect complete"); */
 }
drivers/media/video/gspca/gspca.h +1 -1
index e444f16..f89b8fb 100644
@@ -154,9 +154,9 @@ struct gspca_frame {
 };
 
 struct gspca_dev {
-	struct video_device vdev;	/* !! must be the first item */
 	struct module *module;		/* subdriver handling the device */
 	struct usb_device *dev;
+	struct video_device *vdev;	/* not necesarily the first item ;) */
 	struct file *capt_file;		/* file doing video capture */
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
 	struct input_dev *input_dev;
#1 lore raw
Quoted 6 lines · On Sat, 19 Nov 2011 18:46:21 -0300 Ezequiel <[email protected]> wrote:

Pushed video_device initialization into a separate function. Replace static allocation of struct video_device by video_device_alloc/video_device_release usage.

---

Hi Ezequiel,

just a general comment on commit messages: when doing some invasive changes it is especially important to explain the WHY next to the WHAT and the high-level HOW.

[...]

drivers/media/video/gspca/gspca.c static int vidioc_enum_frameintervals(struct file *filp, void *priv, · line 1292

Quoted 6 lines
drivers/media/video/gspca/gspca.c +0 -0
index 881e04c..1f27f05 100644
@@ -1292,10 +1292,12 @@ static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 

[...]

Thanks,
Antonio

Signature · 8 lines
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
#2 lore raw
In reply to Ezequiel

Hi,

Quoted 4 lines · Ezequiel

Pushed video_device initialization into a separate function. Replace static allocation of struct video_device by video_device_alloc/video_device_release usage.

NACK again! There is no reason to do this, it just makes the code more complicated without gaining anything. As already commented by Antonio Ospite your commit message lacks the why of this patch / the reason to do such a patch. The diffstat clearly shows it is adding code not removing / simplifying it and it so doing so without any good reasons!

Regards,

Hans

author
#3 lore raw
Quoted 7 lines · Hans de Goede

NACK again! There is no reason to do this, it just makes the code more complicated without gaining anything. As already commented by Antonio Ospite your commit message lacks the why of this patch / the reason to do such a patch. The diffstat clearly shows it is adding code not removing / simplifying it and it so doing so without any good reasons!

Yes, it's true: I omit the the reason in the commit message. The point of the patch was improving readability of the code.

But it was altogether wrong, as Jean-Francois patiently explained to me in another thread.

Thanks to you too for the patience,
Ezequiel.