Re: [PATCH v7 2/7] drivers:media:radio: wl128x: fmdrv_v4l2 sources

2010-12-22 Thread Hans Verkuil 
On Wednesday, December 22, 2010 07:19:54 halli manjunatha wrote:
 10 at 5:08 PM, Hans Verkuil hverk...@xs4all.nl wrote:
  On Friday, December 17, 2010 12:06:31 manjunatha_ha...@ti.com wrote:
  From: Manjunatha Halli manjunatha_ha...@ti.com
 
  This module interfaces V4L2 subsystem and FM common module.
  It registers itself with V4L2 as Radio module.
 
  Signed-off-by: Manjunatha Halli manjunatha_ha...@ti.com
  ---
   drivers/media/radio/wl128x/fmdrv_v4l2.c |  588 
  +++
   drivers/media/radio/wl128x/fmdrv_v4l2.h |   33 ++
   2 files changed, 621 insertions(+), 0 deletions(-)
   create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.c
   create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.h
 
  diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c 
  b/drivers/media/radio/wl128x/fmdrv_v4l2.c
  new file mode 100644
  index 000..623102f
  --- /dev/null
  +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
 
  snip
 
  +static const struct v4l2_file_operations fm_drv_fops = {
  + .owner = THIS_MODULE,
  + .read = fm_v4l2_fops_read,
  + .write = fm_v4l2_fops_write,
  + .poll = fm_v4l2_fops_poll,
  + .ioctl = video_ioctl2,
 
  Please use unlocked_ioctl. The .ioctl call is deprecated since it relied on 
  the
  Big Kernel Lock which is in the process of being removed from the kernel. 
  The
  BKL serialized all ioctl calls, unlocked_ioctl relies on the driver to 
  serialize
  where necessary.
 
  There are two ways of doing the conversion: one is to do all the locking 
  within
  the driver, the other is to use core-assisted locking. How to do the 
  core-assisted
  locking is described in Documentation/video4linux/v4l2-framework.txt, but 
  I'll
  repeat the relevant part here:
 
  v4l2_file_operations and locking
  
 
  You can set a pointer to a mutex_lock in struct video_device. Usually this
  will be either a top-level mutex or a mutex per device node. If you want
  finer-grained locking then you have to set it to NULL and do you own 
  locking.
 
  If a lock is specified then all file operations will be serialized on that
  lock. If you use videobuf then you must pass the same lock to the videobuf
  queue initialize function: if videobuf has to wait for a frame to arrive, 
  then
  it will temporarily unlock the lock and relock it afterwards. If your driver
  also waits in the code, then you should do the same to allow other processes
  to access the device node while the first process is waiting for something.
 
  The implementation of a hotplug disconnect should also take the lock before
  calling v4l2_device_disconnect.
 
  + .open = fm_v4l2_fops_open,
  + .release = fm_v4l2_fops_release,
  +};
 
 Hans,
 
 I did this in my driver,
 --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
 +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
 @@ -477,7 +477,7 @@ static const struct v4l2_file_operations fm_drv_fops = {
 .read = fm_v4l2_fops_read,
 .write = fm_v4l2_fops_write,
 .poll = fm_v4l2_fops_poll,
 -   .ioctl = video_ioctl2,
 +   .unlocked_ioctl = video_ioctl2,
 .open = fm_v4l2_fops_open,
 .release = fm_v4l2_fops_release,
 
 and apparently didn't face any issues during regression.. (related to
 concurrency...)
 and more-over all of my ioctls from the application being called in
 process context are all synchronous, i.e
 application calls and waits for it to finish before calling another ioctl.
 
 So is this approach alright?
 Is there a reason to opt for the .ioctl ? The locked approach?
 Or have mutex locks inside the unlocked handlers?
 
 If not I will go ahead and submit v8 using the unlocked_ioctl

The old use of .ioctl would serialize all ioctl calls. So if multiple processes
opened the video device node and made ioctl calls, then the Big Kernel Lock 
would
serialize those ioctls. Switching to .unlocked_ioctl will no longer serialize
those calls and so you will have to do something to prevent race conditions.

Either identify the critical sections and lock them manually, or use the core
assisted locking scheme that V4L2 provides to serialize all file operations.

I recommend the latter since it is very simple to implement and it does the
job. Since there is no locking whatsoever in the driver I think that manually
adding locking is too much work (and too hard to verify correctness).

Regards,

Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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 v7 2/7] drivers:media:radio: wl128x: fmdrv_v4l2 sources

2010-12-21 Thread halli manjunatha 
10 at 5:08 PM, Hans Verkuil hverk...@xs4all.nl wrote:
 On Friday, December 17, 2010 12:06:31 manjunatha_ha...@ti.com wrote:
 From: Manjunatha Halli manjunatha_ha...@ti.com

 This module interfaces V4L2 subsystem and FM common module.
 It registers itself with V4L2 as Radio module.

 Signed-off-by: Manjunatha Halli manjunatha_ha...@ti.com
 ---
  drivers/media/radio/wl128x/fmdrv_v4l2.c |  588 
 +++
  drivers/media/radio/wl128x/fmdrv_v4l2.h |   33 ++
  2 files changed, 621 insertions(+), 0 deletions(-)
  create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.c
  create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.h

 diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c 
 b/drivers/media/radio/wl128x/fmdrv_v4l2.c
 new file mode 100644
 index 000..623102f
 --- /dev/null
 +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c

 snip

 +static const struct v4l2_file_operations fm_drv_fops = {
 +     .owner = THIS_MODULE,
 +     .read = fm_v4l2_fops_read,
 +     .write = fm_v4l2_fops_write,
 +     .poll = fm_v4l2_fops_poll,
 +     .ioctl = video_ioctl2,

 Please use unlocked_ioctl. The .ioctl call is deprecated since it relied on 
 the
 Big Kernel Lock which is in the process of being removed from the kernel. The
 BKL serialized all ioctl calls, unlocked_ioctl relies on the driver to 
 serialize
 where necessary.

 There are two ways of doing the conversion: one is to do all the locking 
 within
 the driver, the other is to use core-assisted locking. How to do the 
 core-assisted
 locking is described in Documentation/video4linux/v4l2-framework.txt, but I'll
 repeat the relevant part here:

 v4l2_file_operations and locking
 

 You can set a pointer to a mutex_lock in struct video_device. Usually this
 will be either a top-level mutex or a mutex per device node. If you want
 finer-grained locking then you have to set it to NULL and do you own locking.

 If a lock is specified then all file operations will be serialized on that
 lock. If you use videobuf then you must pass the same lock to the videobuf
 queue initialize function: if videobuf has to wait for a frame to arrive, then
 it will temporarily unlock the lock and relock it afterwards. If your driver
 also waits in the code, then you should do the same to allow other processes
 to access the device node while the first process is waiting for something.

 The implementation of a hotplug disconnect should also take the lock before
 calling v4l2_device_disconnect.

 +     .open = fm_v4l2_fops_open,
 +     .release = fm_v4l2_fops_release,
 +};

Hans,

I did this in my driver,
--- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -477,7 +477,7 @@ static const struct v4l2_file_operations fm_drv_fops = {
.read = fm_v4l2_fops_read,
.write = fm_v4l2_fops_write,
.poll = fm_v4l2_fops_poll,
-   .ioctl = video_ioctl2,
+   .unlocked_ioctl = video_ioctl2,
.open = fm_v4l2_fops_open,
.release = fm_v4l2_fops_release,

and apparently didn't face any issues during regression.. (related to
concurrency...)
and more-over all of my ioctls from the application being called in
process context are all synchronous, i.e
application calls and waits for it to finish before calling another ioctl.

So is this approach alright?
Is there a reason to opt for the .ioctl ? The locked approach?
Or have mutex locks inside the unlocked handlers?

If not I will go ahead and submit v8 using the unlocked_ioctl


 Regards,

        Hans

 --
 Hans Verkuil - video4linux developer - sponsored by Cisco
 --
 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




-- 
Regards
Halli
--
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 v7 2/7] drivers:media:radio: wl128x: fmdrv_v4l2 sources

2010-12-18 Thread Hans Verkuil 
On Friday, December 17, 2010 12:06:31 manjunatha_ha...@ti.com wrote:
 From: Manjunatha Halli manjunatha_ha...@ti.com
 
 This module interfaces V4L2 subsystem and FM common module.
 It registers itself with V4L2 as Radio module.
 
 Signed-off-by: Manjunatha Halli manjunatha_ha...@ti.com
 ---
  drivers/media/radio/wl128x/fmdrv_v4l2.c |  588 
 +++
  drivers/media/radio/wl128x/fmdrv_v4l2.h |   33 ++
  2 files changed, 621 insertions(+), 0 deletions(-)
  create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.c
  create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.h
 
 diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c 
 b/drivers/media/radio/wl128x/fmdrv_v4l2.c
 new file mode 100644
 index 000..623102f
 --- /dev/null
 +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c

snip

 +static const struct v4l2_file_operations fm_drv_fops = {
 + .owner = THIS_MODULE,
 + .read = fm_v4l2_fops_read,
 + .write = fm_v4l2_fops_write,
 + .poll = fm_v4l2_fops_poll,
 + .ioctl = video_ioctl2,

Please use unlocked_ioctl. The .ioctl call is deprecated since it relied on the
Big Kernel Lock which is in the process of being removed from the kernel. The
BKL serialized all ioctl calls, unlocked_ioctl relies on the driver to serialize
where necessary.

There are two ways of doing the conversion: one is to do all the locking within
the driver, the other is to use core-assisted locking. How to do the 
core-assisted
locking is described in Documentation/video4linux/v4l2-framework.txt, but I'll
repeat the relevant part here:

v4l2_file_operations and locking


You can set a pointer to a mutex_lock in struct video_device. Usually this
will be either a top-level mutex or a mutex per device node. If you want
finer-grained locking then you have to set it to NULL and do you own locking.

If a lock is specified then all file operations will be serialized on that
lock. If you use videobuf then you must pass the same lock to the videobuf
queue initialize function: if videobuf has to wait for a frame to arrive, then
it will temporarily unlock the lock and relock it afterwards. If your driver
also waits in the code, then you should do the same to allow other processes
to access the device node while the first process is waiting for something.

The implementation of a hotplug disconnect should also take the lock before
calling v4l2_device_disconnect.

 + .open = fm_v4l2_fops_open,
 + .release = fm_v4l2_fops_release,
 +};

Regards,

Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco
--
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

[PATCH v7 2/7] drivers:media:radio: wl128x: fmdrv_v4l2 sources

2010-12-17 Thread manjunatha_halli 
From: Manjunatha Halli manjunatha_ha...@ti.com

This module interfaces V4L2 subsystem and FM common module.
It registers itself with V4L2 as Radio module.

Signed-off-by: Manjunatha Halli manjunatha_ha...@ti.com
---
 drivers/media/radio/wl128x/fmdrv_v4l2.c |  588 +++
 drivers/media/radio/wl128x/fmdrv_v4l2.h |   33 ++
 2 files changed, 621 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.c
 create mode 100644 drivers/media/radio/wl128x/fmdrv_v4l2.h

diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c 
b/drivers/media/radio/wl128x/fmdrv_v4l2.c
new file mode 100644
index 000..623102f
--- /dev/null
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -0,0 +1,588 @@
+/*
+ *  FM Driver for Connectivity chip of Texas Instruments.
+ *  This file provides interfaces to V4L2 subsystem.
+ *
+ *  This module registers with V4L2 subsystem as Radio
+ *  data system interface (/dev/radio). During the registration,
+ *  it will expose two set of function pointers.
+ *
+ *1) File operation related API (open, close, read, write, poll...etc).
+ *2) Set of V4L2 IOCTL complaint API.
+ *
+ *  Copyright (C) 2010 Texas Instruments
+ *  Author: Raja Mani raja_m...@ti.com
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include fmdrv.h
+#include fmdrv_v4l2.h
+#include fmdrv_common.h
+#include fmdrv_rx.h
+#include fmdrv_tx.h
+
+static struct video_device *gradio_dev;
+static unsigned char radio_disconnected;
+
+/* -- V4L2 RADIO (/dev/radioX) device file operation interfaces --- */
+
+/* Read RX RDS data */
+static ssize_t fm_v4l2_fops_read(struct file *file, char __user * buf,
+   size_t count, loff_t *ppos)
+{
+   unsigned char rds_mode;
+   int ret;
+   struct fmdrv_ops *fmdev;
+
+   fmdev = video_drvdata(file);
+
+   if (!radio_disconnected) {
+   pr_err((fmdrv): FM device is already disconnected\n);
+   return -EIO;
+   }
+
+   /* Turn on RDS mode , if it is disabled */
+   ret = fm_rx_get_rds_mode(fmdev, rds_mode);
+   if (ret  0) {
+   pr_err((fmdrv): Unable to read current rds mode\n);
+   return ret;
+   }
+
+   if (rds_mode == FM_RDS_DISABLE) {
+   ret = fmc_set_rds_mode(fmdev, FM_RDS_ENABLE);
+   if (ret  0) {
+   pr_err((fmdrv): Failed to enable rds mode\n);
+   return ret;
+   }
+   }
+
+   /* Copy RDS data from internal buffer to user buffer */
+   ret = fmc_transfer_rds_from_internal_buff(fmdev, file, buf, count);
+
+   return ret;
+}
+
+/* Write TX RDS data */
+static ssize_t fm_v4l2_fops_write(struct file *file, const char __user * buf,
+   size_t count, loff_t *ppos)
+{
+   struct tx_rds rds;
+   int ret;
+   struct fmdrv_ops *fmdev;
+
+   ret = copy_from_user(rds, buf, sizeof(rds));
+   pr_debug((fmdrv): (%d)type: %d, text %s, af %d\n,
+  ret, rds.text_type, rds.text, rds.af_freq);
+
+   fmdev = video_drvdata(file);
+   fm_tx_set_radio_text(fmdev, rds.text, rds.text_type);
+   fm_tx_set_af(fmdev, rds.af_freq);
+
+   return 0;
+}
+
+static unsigned int fm_v4l2_fops_poll(struct file *file,
+ struct poll_table_struct *pts)
+{
+   int ret;
+   struct fmdrv_ops *fmdev;
+
+   fmdev = video_drvdata(file);
+   ret = fmc_is_rds_data_available(fmdev, file, pts);
+   if (ret  0)
+   return POLLIN | POLLRDNORM;
+
+   return 0;
+}
+
+/*
+ * Handle open request for /dev/radioX device.
+ * Start with FM RX mode as default.
+ */
+static int fm_v4l2_fops_open(struct file *file)
+{
+   int ret;
+   struct fmdrv_ops *fmdev = NULL;
+
+   /* Don't allow multiple open */
+   if (radio_disconnected) {
+   pr_err((fmdrv): FM device is already opened\n);
+   return -EBUSY;
+   }
+
+   fmdev = video_drvdata(file);
+
+   ret = fmc_prepare(fmdev);
+   if (ret  0) {
+   pr_err((fmdrv): Unable to prepare FM CORE\n);
+   return ret;
+   }
+
+   pr_debug((fmdrv): Load FM RX firmware..\n);
+
+   ret = fmc_set_mode(fmdev, FM_MODE_RX);
+   if (ret  0) {
+