Re: [PATCH 06/19] em28xx: move video_device structs from struct em28xx to struct v4l2

2014-05-12 Thread Hans Verkuil 
On 05/11/2014 10:50 PM, Frank Schäfer wrote:
 
 Am 09.05.2014 11:19, schrieb Hans Verkuil:
 On 03/24/2014 08:33 PM, Frank Schäfer wrote:
 Signed-off-by: Frank Schäfer fschaefer@googlemail.com
 ---
  drivers/media/usb/em28xx/em28xx-video.c | 120 
 ++--
  drivers/media/usb/em28xx/em28xx.h   |   7 +-
  2 files changed, 56 insertions(+), 71 deletions(-)

 diff --git a/drivers/media/usb/em28xx/em28xx.h 
 b/drivers/media/usb/em28xx/em28xx.h
 index a4d26bf..88d0589 100644
 --- a/drivers/media/usb/em28xx/em28xx.h
 +++ b/drivers/media/usb/em28xx/em28xx.h
 @@ -504,6 +504,10 @@ struct em28xx_v4l2 {
 struct v4l2_device v4l2_dev;
 struct v4l2_ctrl_handler ctrl_handler;
 struct v4l2_clk *clk;
 +
 +   struct video_device *vdev;
 +   struct video_device *vbi_dev;
 +   struct video_device *radio_dev;
 Think about embedding these structs. That way you don't have to allocate 
 them which
 removes the complexity of checking for ENOMEM errors.
 
 Yes, but consider that only em286x and em288x devices provide VBI
 support and we have even less devices with radio support (~ 3 of 100).
 So with most devices, we would waste memory.

The problem with v4l drivers is always complexity, never performance or memory.
Anything that reduces complexity is always a good thing. The extra memory used
is negligible. Since kmalloc rounds up the requested memory to the next power
of two you might even end up with allocating more memory instead of less, but
you'd have to calculate that to see if it is true.

Simplification is always key to media drivers.

Regards,

Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/19] em28xx: move video_device structs from struct em28xx to struct v4l2

2014-05-11 Thread Frank Schäfer 

Am 09.05.2014 11:19, schrieb Hans Verkuil:
 On 03/24/2014 08:33 PM, Frank Schäfer wrote:
 Signed-off-by: Frank Schäfer fschaefer@googlemail.com
 ---
  drivers/media/usb/em28xx/em28xx-video.c | 120 
 ++--
  drivers/media/usb/em28xx/em28xx.h   |   7 +-
  2 files changed, 56 insertions(+), 71 deletions(-)

 diff --git a/drivers/media/usb/em28xx/em28xx-video.c 
 b/drivers/media/usb/em28xx/em28xx-video.c
 index 4fb0053..7252eef 100644
 --- a/drivers/media/usb/em28xx/em28xx-video.c
 +++ b/drivers/media/usb/em28xx/em28xx-video.c
 @@ -1447,7 +1447,7 @@ static int vidioc_enum_input(struct file *file, void 
 *priv,
  (EM28XX_VMUX_CABLE == INPUT(n)-type))
  i-type = V4L2_INPUT_TYPE_TUNER;
  
 -i-std = dev-vdev-tvnorms;
 +i-std = dev-v4l2-vdev-tvnorms;
  /* webcams do not have the STD API */
  if (dev-board.is_webcam)
  i-capabilities = 0;
 @@ -1691,9 +1691,10 @@ static int vidioc_s_register(struct file *file, void 
 *priv,
  static int vidioc_querycap(struct file *file, void  *priv,
  struct v4l2_capability *cap)
  {
 -struct video_device *vdev = video_devdata(file);
 -struct em28xx_fh  *fh  = priv;
 -struct em28xx *dev = fh-dev;
 +struct video_device   *vdev = video_devdata(file);
 +struct em28xx_fh  *fh   = priv;
 +struct em28xx *dev  = fh-dev;
 +struct em28xx_v4l2*v4l2 = dev-v4l2;
  
  strlcpy(cap-driver, em28xx, sizeof(cap-driver));
  strlcpy(cap-card, em28xx_boards[dev-model].name, sizeof(cap-card));
 @@ -1715,9 +1716,9 @@ static int vidioc_querycap(struct file *file, void  
 *priv,
  
  cap-capabilities = cap-device_caps | V4L2_CAP_DEVICE_CAPS |
  V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE | 
 V4L2_CAP_STREAMING;
 -if (dev-vbi_dev)
 +if (v4l2-vbi_dev)
  cap-capabilities |= V4L2_CAP_VBI_CAPTURE;
 -if (dev-radio_dev)
 +if (v4l2-radio_dev)
  cap-capabilities |= V4L2_CAP_RADIO;
  return 0;
  }
 @@ -1955,20 +1956,20 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
  
  em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
  
 -if (dev-radio_dev) {
 +if (v4l2-radio_dev) {
  em28xx_info(V4L2 device %s deregistered\n,
 -video_device_node_name(dev-radio_dev));
 -video_unregister_device(dev-radio_dev);
 +video_device_node_name(v4l2-radio_dev));
 +video_unregister_device(v4l2-radio_dev);
  }
 -if (dev-vbi_dev) {
 +if (v4l2-vbi_dev) {
  em28xx_info(V4L2 device %s deregistered\n,
 -video_device_node_name(dev-vbi_dev));
 -video_unregister_device(dev-vbi_dev);
 +video_device_node_name(v4l2-vbi_dev));
 +video_unregister_device(v4l2-vbi_dev);
  }
 -if (dev-vdev) {
 +if (v4l2-vdev) {
  em28xx_info(V4L2 device %s deregistered\n,
 -video_device_node_name(dev-vdev));
 -video_unregister_device(dev-vdev);
 +video_device_node_name(v4l2-vdev));
 +video_unregister_device(v4l2-vdev);
  }
  
  v4l2_ctrl_handler_free(v4l2-ctrl_handler);
 @@ -2061,23 +2062,6 @@ exit:
  return 0;
  }
  
 -/*
 - * em28xx_videodevice_release()
 - * called when the last user of the video device exits and frees the memeory
 - */
 -static void em28xx_videodevice_release(struct video_device *vdev)
 -{
 -struct em28xx *dev = video_get_drvdata(vdev);
 -
 -video_device_release(vdev);
 -if (vdev == dev-vdev)
 -dev-vdev = NULL;
 -else if (vdev == dev-vbi_dev)
 -dev-vbi_dev = NULL;
 -else if (vdev == dev-radio_dev)
 -dev-radio_dev = NULL;
 -}
 -
  static const struct v4l2_file_operations em28xx_v4l_fops = {
  .owner = THIS_MODULE,
  .open  = em28xx_v4l2_open,
 @@ -2134,7 +2118,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
  static const struct video_device em28xx_video_template = {
  .fops   = em28xx_v4l_fops,
  .ioctl_ops  = video_ioctl_ops,
 -.release= em28xx_videodevice_release,
 +.release= video_device_release,
  .tvnorms= V4L2_STD_ALL,
  };
  
 @@ -2163,7 +2147,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
  static struct video_device em28xx_radio_template = {
  .fops   = radio_fops,
  .ioctl_ops  = radio_ioctl_ops,
 -.release= em28xx_videodevice_release,
 +.release= video_device_release,
  };
  
  /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
 @@ -2493,36 +2477,36 @@ static int em28xx_v4l2_init(struct em28xx *dev)
  goto unregister_dev;
  
  /* allocate and fill video video_device struct */
 -dev-vdev = em28xx_vdev_init(dev, em28xx_video_template, video);
 -if (!dev-vdev) {
 +

Re: [PATCH 06/19] em28xx: move video_device structs from struct em28xx to struct v4l2

2014-05-09 Thread Hans Verkuil 
On 03/24/2014 08:33 PM, Frank Schäfer wrote:
 Signed-off-by: Frank Schäfer fschaefer@googlemail.com
 ---
  drivers/media/usb/em28xx/em28xx-video.c | 120 
 ++--
  drivers/media/usb/em28xx/em28xx.h   |   7 +-
  2 files changed, 56 insertions(+), 71 deletions(-)
 
 diff --git a/drivers/media/usb/em28xx/em28xx-video.c 
 b/drivers/media/usb/em28xx/em28xx-video.c
 index 4fb0053..7252eef 100644
 --- a/drivers/media/usb/em28xx/em28xx-video.c
 +++ b/drivers/media/usb/em28xx/em28xx-video.c
 @@ -1447,7 +1447,7 @@ static int vidioc_enum_input(struct file *file, void 
 *priv,
   (EM28XX_VMUX_CABLE == INPUT(n)-type))
   i-type = V4L2_INPUT_TYPE_TUNER;
  
 - i-std = dev-vdev-tvnorms;
 + i-std = dev-v4l2-vdev-tvnorms;
   /* webcams do not have the STD API */
   if (dev-board.is_webcam)
   i-capabilities = 0;
 @@ -1691,9 +1691,10 @@ static int vidioc_s_register(struct file *file, void 
 *priv,
  static int vidioc_querycap(struct file *file, void  *priv,
   struct v4l2_capability *cap)
  {
 - struct video_device *vdev = video_devdata(file);
 - struct em28xx_fh  *fh  = priv;
 - struct em28xx *dev = fh-dev;
 + struct video_device   *vdev = video_devdata(file);
 + struct em28xx_fh  *fh   = priv;
 + struct em28xx *dev  = fh-dev;
 + struct em28xx_v4l2*v4l2 = dev-v4l2;
  
   strlcpy(cap-driver, em28xx, sizeof(cap-driver));
   strlcpy(cap-card, em28xx_boards[dev-model].name, sizeof(cap-card));
 @@ -1715,9 +1716,9 @@ static int vidioc_querycap(struct file *file, void  
 *priv,
  
   cap-capabilities = cap-device_caps | V4L2_CAP_DEVICE_CAPS |
   V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE | 
 V4L2_CAP_STREAMING;
 - if (dev-vbi_dev)
 + if (v4l2-vbi_dev)
   cap-capabilities |= V4L2_CAP_VBI_CAPTURE;
 - if (dev-radio_dev)
 + if (v4l2-radio_dev)
   cap-capabilities |= V4L2_CAP_RADIO;
   return 0;
  }
 @@ -1955,20 +1956,20 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
  
   em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
  
 - if (dev-radio_dev) {
 + if (v4l2-radio_dev) {
   em28xx_info(V4L2 device %s deregistered\n,
 - video_device_node_name(dev-radio_dev));
 - video_unregister_device(dev-radio_dev);
 + video_device_node_name(v4l2-radio_dev));
 + video_unregister_device(v4l2-radio_dev);
   }
 - if (dev-vbi_dev) {
 + if (v4l2-vbi_dev) {
   em28xx_info(V4L2 device %s deregistered\n,
 - video_device_node_name(dev-vbi_dev));
 - video_unregister_device(dev-vbi_dev);
 + video_device_node_name(v4l2-vbi_dev));
 + video_unregister_device(v4l2-vbi_dev);
   }
 - if (dev-vdev) {
 + if (v4l2-vdev) {
   em28xx_info(V4L2 device %s deregistered\n,
 - video_device_node_name(dev-vdev));
 - video_unregister_device(dev-vdev);
 + video_device_node_name(v4l2-vdev));
 + video_unregister_device(v4l2-vdev);
   }
  
   v4l2_ctrl_handler_free(v4l2-ctrl_handler);
 @@ -2061,23 +2062,6 @@ exit:
   return 0;
  }
  
 -/*
 - * em28xx_videodevice_release()
 - * called when the last user of the video device exits and frees the memeory
 - */
 -static void em28xx_videodevice_release(struct video_device *vdev)
 -{
 - struct em28xx *dev = video_get_drvdata(vdev);
 -
 - video_device_release(vdev);
 - if (vdev == dev-vdev)
 - dev-vdev = NULL;
 - else if (vdev == dev-vbi_dev)
 - dev-vbi_dev = NULL;
 - else if (vdev == dev-radio_dev)
 - dev-radio_dev = NULL;
 -}
 -
  static const struct v4l2_file_operations em28xx_v4l_fops = {
   .owner = THIS_MODULE,
   .open  = em28xx_v4l2_open,
 @@ -2134,7 +2118,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
  static const struct video_device em28xx_video_template = {
   .fops   = em28xx_v4l_fops,
   .ioctl_ops  = video_ioctl_ops,
 - .release= em28xx_videodevice_release,
 + .release= video_device_release,
   .tvnorms= V4L2_STD_ALL,
  };
  
 @@ -2163,7 +2147,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
  static struct video_device em28xx_radio_template = {
   .fops   = radio_fops,
   .ioctl_ops  = radio_ioctl_ops,
 - .release= em28xx_videodevice_release,
 + .release= video_device_release,
  };
  
  /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
 @@ -2493,36 +2477,36 @@ static int em28xx_v4l2_init(struct em28xx *dev)
   goto unregister_dev;
  
   /* allocate and fill video video_device struct */
 - dev-vdev = em28xx_vdev_init(dev, em28xx_video_template,

[PATCH 06/19] em28xx: move video_device structs from struct em28xx to struct v4l2

2014-03-24 Thread Frank Schäfer 
Signed-off-by: Frank Schäfer fschaefer@googlemail.com
---
 drivers/media/usb/em28xx/em28xx-video.c | 120 ++--
 drivers/media/usb/em28xx/em28xx.h   |   7 +-
 2 files changed, 56 insertions(+), 71 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c 
b/drivers/media/usb/em28xx/em28xx-video.c
index 4fb0053..7252eef 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -1447,7 +1447,7 @@ static int vidioc_enum_input(struct file *file, void 
*priv,
(EM28XX_VMUX_CABLE == INPUT(n)-type))
i-type = V4L2_INPUT_TYPE_TUNER;
 
-   i-std = dev-vdev-tvnorms;
+   i-std = dev-v4l2-vdev-tvnorms;
/* webcams do not have the STD API */
if (dev-board.is_webcam)
i-capabilities = 0;
@@ -1691,9 +1691,10 @@ static int vidioc_s_register(struct file *file, void 
*priv,
 static int vidioc_querycap(struct file *file, void  *priv,
struct v4l2_capability *cap)
 {
-   struct video_device *vdev = video_devdata(file);
-   struct em28xx_fh  *fh  = priv;
-   struct em28xx *dev = fh-dev;
+   struct video_device   *vdev = video_devdata(file);
+   struct em28xx_fh  *fh   = priv;
+   struct em28xx *dev  = fh-dev;
+   struct em28xx_v4l2*v4l2 = dev-v4l2;
 
strlcpy(cap-driver, em28xx, sizeof(cap-driver));
strlcpy(cap-card, em28xx_boards[dev-model].name, sizeof(cap-card));
@@ -1715,9 +1716,9 @@ static int vidioc_querycap(struct file *file, void  *priv,
 
cap-capabilities = cap-device_caps | V4L2_CAP_DEVICE_CAPS |
V4L2_CAP_READWRITE | V4L2_CAP_VIDEO_CAPTURE | 
V4L2_CAP_STREAMING;
-   if (dev-vbi_dev)
+   if (v4l2-vbi_dev)
cap-capabilities |= V4L2_CAP_VBI_CAPTURE;
-   if (dev-radio_dev)
+   if (v4l2-radio_dev)
cap-capabilities |= V4L2_CAP_RADIO;
return 0;
 }
@@ -1955,20 +1956,20 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
 
em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE);
 
-   if (dev-radio_dev) {
+   if (v4l2-radio_dev) {
em28xx_info(V4L2 device %s deregistered\n,
-   video_device_node_name(dev-radio_dev));
-   video_unregister_device(dev-radio_dev);
+   video_device_node_name(v4l2-radio_dev));
+   video_unregister_device(v4l2-radio_dev);
}
-   if (dev-vbi_dev) {
+   if (v4l2-vbi_dev) {
em28xx_info(V4L2 device %s deregistered\n,
-   video_device_node_name(dev-vbi_dev));
-   video_unregister_device(dev-vbi_dev);
+   video_device_node_name(v4l2-vbi_dev));
+   video_unregister_device(v4l2-vbi_dev);
}
-   if (dev-vdev) {
+   if (v4l2-vdev) {
em28xx_info(V4L2 device %s deregistered\n,
-   video_device_node_name(dev-vdev));
-   video_unregister_device(dev-vdev);
+   video_device_node_name(v4l2-vdev));
+   video_unregister_device(v4l2-vdev);
}
 
v4l2_ctrl_handler_free(v4l2-ctrl_handler);
@@ -2061,23 +2062,6 @@ exit:
return 0;
 }
 
-/*
- * em28xx_videodevice_release()
- * called when the last user of the video device exits and frees the memeory
- */
-static void em28xx_videodevice_release(struct video_device *vdev)
-{
-   struct em28xx *dev = video_get_drvdata(vdev);
-
-   video_device_release(vdev);
-   if (vdev == dev-vdev)
-   dev-vdev = NULL;
-   else if (vdev == dev-vbi_dev)
-   dev-vbi_dev = NULL;
-   else if (vdev == dev-radio_dev)
-   dev-radio_dev = NULL;
-}
-
 static const struct v4l2_file_operations em28xx_v4l_fops = {
.owner = THIS_MODULE,
.open  = em28xx_v4l2_open,
@@ -2134,7 +2118,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 static const struct video_device em28xx_video_template = {
.fops   = em28xx_v4l_fops,
.ioctl_ops  = video_ioctl_ops,
-   .release= em28xx_videodevice_release,
+   .release= video_device_release,
.tvnorms= V4L2_STD_ALL,
 };
 
@@ -2163,7 +2147,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
 static struct video_device em28xx_radio_template = {
.fops   = radio_fops,
.ioctl_ops  = radio_ioctl_ops,
-   .release= em28xx_videodevice_release,
+   .release= video_device_release,
 };
 
 /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
@@ -2493,36 +2477,36 @@ static int em28xx_v4l2_init(struct em28xx *dev)
goto unregister_dev;
 
/* allocate and fill video video_device struct */
-   dev-vdev = em28xx_vdev_init(dev, em28xx_video_template, video);
-   if (!dev-vdev)