[PATCHv5 0 of 8] FM Transmitter (si4713) and another changes

[PATCHv5 0 of 8] FM Transmitter (si4713) and another changes

[Eduardo Valentin]

Hello all,

  I'm resending the FM transmitter driver and the proposed changes in
v4l2 api files in order to cover the fmtx extended controls class.

  Difference from version #4 is that now I'm sending the correct patches.
The last patch series was messed up with wrong paths names.

  It is basically the same series of version #3. However I rewrote it
to add the following comments:

  * Check kernel version for i2c helper function. Now the board data
is passed not using i2c_board_info. This way all supported kernel
versions can use the api. Besides that, the .s_config callback was
added in core ops.

  * All patches are against v4l-dvb hg repository.

  Again, comments are welcome.

BR,

---
Eduardo Valentin

[PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414605 -10800
# Branch export
# Node ID 4fb354645426f8b187c2c90cd8528b2518461005
# Parent  142fd6020df3b4d543068155e49a2618140efa49
Device drivers of v4l2_subdev devices may want to have
board specific data. This patch adds an helper function
to allow bridge drivers to pass board specific data to
v4l2_subdev drivers.

For those drivers which need to support kernel versions
bellow 2.6.26, a .s_config callback was added. The
idea of this callback is to pass board configuration
as well. In that case, subdev driver should set .s_config
properly, because v4l2_i2c_new_subdev_board will call
the .s_config directly. Of course, if we are >= 2.6.26,
the same data will be passed through i2c board info as well.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/video/v4l2-common.c |   37 +++++++++++++++++++++++++++++++++++--
 include/linux/v4l2-common.h       |    8 ++++++++
 include/linux/v4l2-subdev.h       |    1 +
 3 files changed, 44 insertions(+), 2 deletions(-)

diff -r 142fd6020df3 -r 4fb354645426 linux/drivers/media/video/v4l2-common.c
--- a/linux/drivers/media/video/v4l2-common.c	Mon May 18 02:31:55 2009 +0000
+++ b/linux/drivers/media/video/v4l2-common.c	Wed May 27 11:56:45 2009 +0300
@@ -819,9 +819,10 @@
 
 
 /* Load an i2c sub-device. */
-struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
+static struct v4l2_subdev *__v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter,
-		const char *module_name, const char *client_type, u8 addr)
+		const char *module_name, const char *client_type, u8 addr,
+		int irq, void *platform_data)
 {
 	struct v4l2_subdev *sd = NULL;
 	struct i2c_client *client;
@@ -840,6 +841,8 @@
 	memset(&info, 0, sizeof(info));
 	strlcpy(info.type, client_type, sizeof(info.type));
 	info.addr = addr;
+	info.irq = irq;
+	info.platform_data = platform_data;
 
 	/* Create the i2c client */
 	client = i2c_new_device(adapter, &info);
@@ -877,8 +880,38 @@
 #endif
 	return sd;
 }
+
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter,
+		const char *module_name, const char *client_type, u8 addr)
+{
+	return __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name,
+		client_type, addr, 0, NULL);
+}
 EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
 
+struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter,
+		const char *module_name, const char *client_type, u8 addr,
+		int irq, void *platform_data)
+{
+	struct v4l2_subdev *sd;
+	int err = 0;
+
+	sd = __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, client_type,
+					addr, irq, platform_data);
+
+	/*
+	 * We return errors from v4l2_subdev_call only if we have the callback
+	 * as the .s_config is not mandatory
+	 */
+	if (sd && sd->ops && sd->ops->core && sd->ops->core->s_config)
+		err = sd->ops->core->s_config(sd, irq, platform_data);
+
+	return err < 0 ? NULL : sd;
+}
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
+
 /* Probe and load an i2c sub-device. */
 struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device *v4l2_dev,
 	struct i2c_adapter *adapter,
diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-common.h
--- a/linux/include/media/v4l2-common.h	Mon May 18 02:31:55 2009 +0000
+++ b/linux/include/media/v4l2-common.h	Wed May 27 11:56:45 2009 +0300
@@ -147,6 +147,14 @@
 struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter,
 		const char *module_name, const char *client_type, u8 addr);
+/*
+ * Same as v4l2_i2c_new_subdev, but with the opportunity to configure
+ * subdevice with board specific data (irq and platform_data).
+ */
+struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
+		struct i2c_adapter *adapter,
+		const char *module_name, const char *client_type, u8 addr,
+		int irq, void *platform_data);
 /* Probe and load an i2c module and return an initialized v4l2_subdev struct.
    Only call request_module if module_name != NULL.
    The client_type argument is the name of the chip that's on the adapter. */
diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-subdev.h
--- a/linux/include/media/v4l2-subdev.h	Mon May 18 02:31:55 2009 +0000
+++ b/linux/include/media/v4l2-subdev.h	Wed May 27 11:56:45 2009 +0300
@@ -96,6 +96,7 @@
 struct v4l2_subdev_core_ops {
 	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident *chip);
 	int (*log_status)(struct v4l2_subdev *sd);
+	int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data);
 	int (*init)(struct v4l2_subdev *sd, u32 val);
 	int (*load_fw)(struct v4l2_subdev *sd);
 	int (*reset)(struct v4l2_subdev *sd, u32 val);

[PATCHv5 2 of 8] v4l2: video device: Add V4L2_CTRL_CLASS_FMTX controls

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414606 -10800
# Branch export
# Node ID ebb409d7a258df2bc7a6dcd72113584b4c0e7ce2
# Parent  4fb354645426f8b187c2c90cd8528b2518461005
This patch adds a new class of extended controls. This class
is intended to support Radio Modulators properties such as:
rds, audio limiters, audio compression, pilot tone generation,
tuning power levels and region related properties.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 include/linux/videodev2.h |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff -r 4fb354645426 -r ebb409d7a258 linux/include/linux/videodev2.h
--- a/linux/include/linux/videodev2.h	Wed May 27 11:56:45 2009 +0300
+++ b/linux/include/linux/videodev2.h	Wed May 27 11:56:46 2009 +0300
@@ -803,6 +803,7 @@
 #define V4L2_CTRL_CLASS_USER 0x00980000	/* Old-style 'user' controls */
 #define V4L2_CTRL_CLASS_MPEG 0x00990000	/* MPEG-compression controls */
 #define V4L2_CTRL_CLASS_CAMERA 0x009a0000	/* Camera class controls */
+#define V4L2_CTRL_CLASS_FMTX 0x009b0000	/* FM Radio Modulator class controls */
 
 #define V4L2_CTRL_ID_MASK      	  (0x0fffffff)
 #define V4L2_CTRL_ID2CLASS(id)    ((id) & 0x0fff0000UL)
@@ -1141,6 +1142,39 @@
 
 #define V4L2_CID_PRIVACY			(V4L2_CID_CAMERA_CLASS_BASE+16)
 
+/* FM Radio Modulator class control IDs */
+#define V4L2_CID_FMTX_CLASS_BASE		(V4L2_CTRL_CLASS_FMTX | 0x900)
+#define V4L2_CID_FMTX_CLASS			(V4L2_CTRL_CLASS_FMTX | 1)
+
+#define V4L2_CID_RDS_ENABLED			(V4L2_CID_FMTX_CLASS_BASE + 1)
+#define V4L2_CID_RDS_PI				(V4L2_CID_FMTX_CLASS_BASE + 2)
+#define V4L2_CID_RDS_PTY			(V4L2_CID_FMTX_CLASS_BASE + 3)
+#define V4L2_CID_RDS_PS_NAME			(V4L2_CID_FMTX_CLASS_BASE + 4)
+#define V4L2_CID_RDS_RADIO_TEXT			(V4L2_CID_FMTX_CLASS_BASE + 5)
+
+#define V4L2_CID_AUDIO_LIMITER_ENABLED		(V4L2_CID_FMTX_CLASS_BASE + 6)
+#define V4L2_CID_AUDIO_LIMITER_RELEASE_TIME	(V4L2_CID_FMTX_CLASS_BASE + 7)
+#define V4L2_CID_AUDIO_LIMITER_DEVIATION	(V4L2_CID_FMTX_CLASS_BASE + 8)
+
+#define V4L2_CID_AUDIO_COMPRESSION_ENABLED	(V4L2_CID_FMTX_CLASS_BASE + 9)
+#define V4L2_CID_AUDIO_COMPRESSION_GAIN		(V4L2_CID_FMTX_CLASS_BASE + 10)
+#define V4L2_CID_AUDIO_COMPRESSION_THRESHOLD	(V4L2_CID_FMTX_CLASS_BASE + 11)
+#define V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME	(V4L2_CID_FMTX_CLASS_BASE + 12)
+#define V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME	(V4L2_CID_FMTX_CLASS_BASE + 13)
+
+#define V4L2_CID_PILOT_TONE_ENABLED		(V4L2_CID_FMTX_CLASS_BASE + 14)
+#define V4L2_CID_PILOT_TONE_DEVIATION		(V4L2_CID_FMTX_CLASS_BASE + 15)
+#define V4L2_CID_PILOT_TONE_FREQUENCY		(V4L2_CID_FMTX_CLASS_BASE + 16)
+
+#define V4L2_CID_PREEMPHASIS			(V4L2_CID_FMTX_CLASS_BASE + 17)
+enum v4l2_fmtx_preemphasis {
+	V4L2_FMTX_PREEMPHASIS_DISABLED		= 0,
+	V4L2_FMTX_PREEMPHASIS_50_uS		= 1,
+	V4L2_FMTX_PREEMPHASIS_75_uS		= 2,
+};
+#define V4L2_CID_TUNE_POWER_LEVEL		(V4L2_CID_FMTX_CLASS_BASE + 18)
+#define V4L2_CID_TUNE_ANTENNA_CAPACITOR		(V4L2_CID_FMTX_CLASS_BASE + 19)
+
 /*
  *	T U N I N G
  */

[PATCHv5 3 of 8] v4l2: video device: Add FMTX controls default configurations

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414606 -10800
# Branch export
# Node ID 85b64a6cc67c0c0d6a795388f8ca02dad2ff9da7
# Parent  ebb409d7a258df2bc7a6dcd72113584b4c0e7ce2
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/video/v4l2-common.c |   46 +++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff -r ebb409d7a258 -r 85b64a6cc67c linux/drivers/media/video/v4l2-common.c
--- a/linux/drivers/media/video/v4l2-common.c	Wed May 27 11:56:46 2009 +0300
+++ b/linux/drivers/media/video/v4l2-common.c	Wed May 27 11:56:46 2009 +0300
@@ -341,6 +341,12 @@
 		"Sepia",
 		NULL
 	};
+	static const char *fmtx_preemphasis[] = {
+		"No preemphasis",
+		"50 useconds",
+		"75 useconds",
+		NULL,
+	};
 
 	switch (id) {
 		case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -379,6 +385,8 @@
 			return camera_exposure_auto;
 		case V4L2_CID_COLORFX:
 			return colorfx;
+		case V4L2_CID_PREEMPHASIS:
+			return fmtx_preemphasis;
 		default:
 			return NULL;
 	}
@@ -477,6 +485,28 @@
 	case V4L2_CID_ZOOM_CONTINUOUS:		return "Zoom, Continuous";
 	case V4L2_CID_PRIVACY:			return "Privacy";
 
+	/* FM Radio Modulator control */
+	case V4L2_CID_FMTX_CLASS:		return "FM Radio Modulator Controls";
+	case V4L2_CID_RDS_ENABLED:		return "RDS Feature Enabled";
+	case V4L2_CID_RDS_PI:			return "RDS Program ID";
+	case V4L2_CID_RDS_PTY:			return "RDS Program Type";
+	case V4L2_CID_RDS_PS_NAME:		return "RDS PS Name";
+	case V4L2_CID_RDS_RADIO_TEXT:		return "RDS Radio Text";
+	case V4L2_CID_AUDIO_LIMITER_ENABLED:	return "Audio Limiter Feature Enabled";
+	case V4L2_CID_AUDIO_LIMITER_RELEASE_TIME: return "Audio Limiter Release Time";
+	case V4L2_CID_AUDIO_LIMITER_DEVIATION:	return "Audio Limiter Deviation";
+	case V4L2_CID_AUDIO_COMPRESSION_ENABLED: return "Audio Compression Feature Enabled";
+	case V4L2_CID_AUDIO_COMPRESSION_GAIN:	return "Audio Compression Gain";
+	case V4L2_CID_AUDIO_COMPRESSION_THRESHOLD: return "Audio Compression Threshold";
+	case V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME: return "Audio Compression Attack Time";
+	case V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME: return "Audio Compression Release Time";
+	case V4L2_CID_PILOT_TONE_ENABLED:	return "Pilot Tone Feature Enabled";
+	case V4L2_CID_PILOT_TONE_DEVIATION:	return "Pilot Tone Deviation";
+	case V4L2_CID_PILOT_TONE_FREQUENCY:	return "Pilot Tone Frequency";
+	case V4L2_CID_PREEMPHASIS:		return "Pre-emphasis settings";
+	case V4L2_CID_TUNE_POWER_LEVEL:		return "Tune Power Level";
+	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:	return "Tune Antenna Capacitor";
+
 	default:
 		return NULL;
 	}
@@ -509,6 +539,10 @@
 	case V4L2_CID_EXPOSURE_AUTO_PRIORITY:
 	case V4L2_CID_FOCUS_AUTO:
 	case V4L2_CID_PRIVACY:
+	case V4L2_CID_RDS_ENABLED:
+	case V4L2_CID_AUDIO_LIMITER_ENABLED:
+	case V4L2_CID_AUDIO_COMPRESSION_ENABLED:
+	case V4L2_CID_PILOT_TONE_ENABLED:
 		qctrl->type = V4L2_CTRL_TYPE_BOOLEAN;
 		min = 0;
 		max = step = 1;
@@ -537,12 +571,14 @@
 	case V4L2_CID_MPEG_STREAM_VBI_FMT:
 	case V4L2_CID_EXPOSURE_AUTO:
 	case V4L2_CID_COLORFX:
+	case V4L2_CID_PREEMPHASIS:
 		qctrl->type = V4L2_CTRL_TYPE_MENU;
 		step = 1;
 		break;
 	case V4L2_CID_USER_CLASS:
 	case V4L2_CID_CAMERA_CLASS:
 	case V4L2_CID_MPEG_CLASS:
+	case V4L2_CID_FMTX_CLASS:
 		qctrl->type = V4L2_CTRL_TYPE_CTRL_CLASS;
 		qctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		min = max = step = def = 0;
@@ -571,6 +607,16 @@
 	case V4L2_CID_BLUE_BALANCE:
 	case V4L2_CID_GAMMA:
 	case V4L2_CID_SHARPNESS:
+	case V4L2_CID_AUDIO_LIMITER_RELEASE_TIME:
+	case V4L2_CID_AUDIO_LIMITER_DEVIATION:
+	case V4L2_CID_AUDIO_COMPRESSION_GAIN:
+	case V4L2_CID_AUDIO_COMPRESSION_THRESHOLD:
+	case V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME:
+	case V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME:
+	case V4L2_CID_PILOT_TONE_DEVIATION:
+	case V4L2_CID_PILOT_TONE_FREQUENCY:
+	case V4L2_CID_TUNE_POWER_LEVEL:
+	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
 		qctrl->flags |= V4L2_CTRL_FLAG_SLIDER;
 		break;
 	case V4L2_CID_PAN_RELATIVE:

[PATCHv5 4 of 8] Add documentation description for FM Transmitter Extended Control Class

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1242209424 -10800
# Branch export
# Node ID 2dd45d3736ae1952dfa98e65b072eb08157fb19d
# Parent  fadf1cddf504609cdb4889f4aa3305ca8d15323a

From: Eduardo Valentin <eduardo.valentin@nokia.com>

This single patch adds documentation description for FM Transmitter (FMTX)
Extended Control Class and its Control IDs. The text was added under
"Extended Controls" section.

Priority: normal

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>

diff -r fadf1cddf504 -r 2dd45d3736ae v4l2-spec/controls.sgml
--- a/v4l2-spec/controls.sgml	Wed May 27 11:56:47 2009 +0300
+++ b/v4l2-spec/controls.sgml	Wed May 13 13:10:24 2009 +0300
@@ -458,6 +458,12 @@
       <para>Unfortunately, the original control API lacked some
 features needed for these new uses and so it was extended into the
 (not terribly originally named) extended control API.</para>
+
+      <para>Even though the MPEG encoding API was the first effort
+to use the Extended Control API, nowadays there are also other classes
+of Extended Controls, such as Camera Controls and FM Transmitter Controls.
+The Extended Controls API as well as all Extended Controls classes are
+described in the following text.</para>
     </section>
 
     <section>
@@ -1816,6 +1822,200 @@
       </tgroup>
     </table>
   </section>
+
+    <section id="fmtx-controls">
+      <title>FM Transmitter Control Reference</title>
+
+      <para>The FM Transmitter (FMTX) class includes controls for common features of
+FM transmissions capable devices. Currently this class include parameters for audio
+compression, pilot tone generation, audio deviation limiter, RDS transmission and
+tuning power features.</para>
+
+      <table pgwide="1" frame="none" id="fmtx-control-id">
+      <title>FMTX Control IDs</title>
+
+      <tgroup cols="4">
+	<colspec colname="c1" colwidth="1*">
+	<colspec colname="c2" colwidth="6*">
+	<colspec colname="c3" colwidth="2*">
+	<colspec colname="c4" colwidth="6*">
+	<spanspec namest="c1" nameend="c2" spanname="id">
+	<spanspec namest="c2" nameend="c4" spanname="descr">
+	<thead>
+	  <row>
+	    <entry spanname="id" align="left">ID</entry>
+	    <entry align="left">Type</entry>
+	  </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
+	  </row>
+	</thead>
+	<tbody valign="top">
+	  <row><entry></entry></row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_FMTX_CLASS</constant>&nbsp;</entry>
+	    <entry>class</entry>
+	  </row><row><entry spanname="descr">The FMTX class
+descriptor. Calling &VIDIOC-QUERYCTRL; for this control will return a
+description of this control class.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_RDS_ENABLED</constant>&nbsp;</entry>
+	    <entry>boolean</entry>
+	  </row>
+	  <row><entry spanname="descr">Enables or disables the RDS transmission feature.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_RDS_PI</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the RDS Programme Identification field
+for transmission.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_RDS_PTY</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">iSets the RDS Programme Type field for transmission.
+This coding of up to 31 pre-defined programme types.</entry>
+	  </row>
+<!--
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_RDS_PS_NAME</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_RDS_RADIO_TEXT</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">.</entry>
+	  </row>
+-->
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_LIMITER_ENABLED</constant>&nbsp;</entry>
+	    <entry>boolean</entry>
+	  </row>
+	  <row><entry spanname="descr">Enables or disables the audio deviation limiter feature.
+The limiter is useful when trying to maximize the audio volume, minimize receiver-generated
+distortion and prevent overmodulation.
+</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_LIMITER_RELEASE_TIME</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the audio deviation limiter feature release time.
+The unit, step and range are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_LIMITER_DEVIATION</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Configures audio frequency deviation level in Hz.
+The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_COMPRESSION_ENABLED</constant>&nbsp;</entry>
+	    <entry>boolean</entry>
+	  </row>
+	  <row><entry spanname="descr">Enables or disables the audio compression feature.
+This feature amplifies signals below the threshold by a fixed gain and compresses audio
+signals above the threshold by the ratio of Threshold/(Gain + Threshold).</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_COMPRESSION_GAIN</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the gain for audio compression feature. It is
+a dB value. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_COMPRESSION_THRESHOLD</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the threshold level for audio compression freature.
+It is a dB value. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the attack time for audio compression feature.
+It is a useconds value. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the release time for audio compression feature.
+It is a useconds value. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_PILOT_TONE_ENABLED</constant>&nbsp;</entry>
+	    <entry>boolean</entry>
+	  </row>
+	  <row><entry spanname="descr">Enables or disables the pilot tone generation feature.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_PILOT_TONE_DEVIATION</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Configures pilot tone frequency deviation level. Unit is
+in Hz. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_PILOT_TONE_FREQUENCY</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Configures pilot tone frequency value. Unit is
+in Hz. The range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_PREEMPHASIS</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Configures the pre-emphasis value for broadcasting.
+A pre-emphasis filter is applied to the broadcast to accentuate the high audio frequencies.
+Depending on the region, a time constant of either 50 or 75 useconds is used. Possible values
+are:</entry>
+	</row><row>
+	<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_FMTX_PREEMPHASIS_DISABLED</constant>&nbsp;</entry>
+		      <entry>No pre-emphasis is applied.</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_FMTX_PREEMPHASIS_50_uS</constant>&nbsp;</entry>
+		      <entry>A pre-emphasis of 50 uS is used.</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_FMTX_PREEMPHASIS_75_uS</constant>&nbsp;</entry>
+		      <entry>A pre-emphasis of 75 uS is used.</entry>
+		    </row>
+		  </tbody>
+		</entrytbl>
+
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TUNE_POWER_LEVEL</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">Sets the output power level for signal transmission.
+Unit is in dBuV. Range and step are driver-specific.</entry>
+	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TUNE_ANTENNA_CAPACITOR</constant>&nbsp;</entry>
+	    <entry>integer</entry>
+	  </row>
+	  <row><entry spanname="descr">This selects the value of antenna tuning capacitor
+manually or automatically if set to zero. Unit, range and step are driver-specific.</entry>
+	  </row>
+	  <row><entry></entry></row>
+	</tbody>
+      </tgroup>
+      </table>
+    </section>
 </section>
 
   <!--

[PATCHv5 5 of 8] FMTx: si4713: Add files to handle si4713 i2c device

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414606 -10800
# Branch export
# Node ID 94b5043b692a6218a667326536d3d76a0c591307
# Parent  85b64a6cc67c0c0d6a795388f8ca02dad2ff9da7
This patch adds files to control si4713 devices.
Internal functions to control device properties
and initialization procedures are into these files.
Also, a v4l2 subdev interface is also exported.
This way other drivers can use this as v4l2 i2c subdevice.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/radio/si4713-subdev.c | 1008 +++++++++++++++++
 drivers/media/radio/si4713.c        | 2100 +++++++++++++++++++++++++++++++++++
 drivers/media/radio/si4713.h        |  282 +++++
 3 files changed, 3390 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/radio/si4713-subdev.c
 create mode 100644 drivers/media/radio/si4713.c
 create mode 100644 drivers/media/radio/si4713.h

diff -r 85b64a6cc67c -r 94b5043b692a linux/drivers/media/radio/si4713-subdev.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/radio/si4713-subdev.c	Wed May 27 11:56:46 2009 +0300
@@ -0,0 +1,1008 @@
+/*
+ * drivers/media/radio/si4713.c
+ *
+ * Silicon Labs Si4713 FM Radio Transmitter I2C commands.
+ *
+ * Copyright (c) 2009 Nokia Corporation
+ * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-i2c-drv.h>
+
+#include "si4713.h"
+
+#define DEFAULT_RDS_PI			0x00
+#define DEFAULT_RDS_PTY			0x00
+#define DEFAULT_RDS_PS_NAME		"Si4713  "
+#define DEFAULT_RDS_RADIO_TEXT		DEFAULT_RDS_PS_NAME
+#define DEFAULT_RDS_DEVIATION		0x00C8
+#define DEFAULT_RDS_PS_REPEAT_COUNT	0x0003
+#define DEFAULT_LIMITER_RTIME		0x1392
+#define DEFAULT_LIMITER_DEV		0x102CA
+#define DEFAULT_PILOT_FREQUENCY 	0x4A38
+#define DEFAULT_PILOT_DEVIATION		0x1A5E
+#define DEFAULT_ACOMP_ATIME		0x0000
+#define DEFAULT_ACOMP_RTIME		0xF4240L
+#define DEFAULT_ACOMP_GAIN		0x0F
+#define DEFAULT_ACOMP_THRESHOLD 	(-0x28)
+#define DEFAULT_MUTE			0x00
+#define DEFAULT_POWER_LEVEL		88
+#define DEFAULT_TUNE_RSSI		0xFF
+
+#define to_si4713_device(sd)	container_of(sd, struct si4713_device, sd)
+
+/* frequency domain transformation (using times 10 to avoid floats) */
+#define FREQDEV_UNIT	100000
+#define FREQV4L2_MULTI	625
+#define dev_to_v4l2(f)	((f * FREQDEV_UNIT) / FREQV4L2_MULTI)
+#define v4l2_to_dev(f)	((f * FREQV4L2_MULTI) / FREQDEV_UNIT)
+
+/*
+ * Sysfs properties
+ * Read and write functions
+ */
+#define property_write(prop, type, mask, check)				\
+static ssize_t si4713_##prop##_write(struct device *dev,		\
+					struct device_attribute *attr,	\
+					const char *buf,		\
+					size_t count)			\
+{									\
+	struct si4713_device *sdev = dev_get_drvdata(dev);		\
+	type value;							\
+	int rval;							\
+									\
+	if (!sdev)							\
+		return -ENODEV;						\
+									\
+	sscanf(buf, mask, &value);					\
+									\
+	if (check)							\
+		return -EDOM;						\
+									\
+	rval = si4713_set_##prop(sdev, value);				\
+									\
+	return rval < 0 ? rval : count;					\
+}
+
+#define property_read(prop, size, mask)					\
+static ssize_t si4713_##prop##_read(struct device *dev,			\
+					struct device_attribute *attr,	\
+					char *buf)			\
+{									\
+	struct si4713_device *sdev = dev_get_drvdata(dev);		\
+	size value;							\
+									\
+	if (!sdev)							\
+		return -ENODEV;						\
+									\
+	value = si4713_get_##prop(sdev);				\
+									\
+	if (value >= 0)							\
+		value = sprintf(buf, mask "\n", value);			\
+									\
+	return value;							\
+}
+
+#define DEFINE_SYSFS_PROPERTY(prop, signal, size, mask, check)		\
+property_write(prop, signal size, mask, check)				\
+property_read(prop, size, mask)						\
+static DEVICE_ATTR(prop, S_IRUGO | S_IWUSR, si4713_##prop##_read,	\
+					si4713_##prop##_write);
+#define DEFINE_SYSFS_PROPERTY_RO(prop, signal, size, mask)		\
+property_read(prop, size, mask)						\
+static DEVICE_ATTR(prop, S_IRUGO, si4713_##prop##_read, NULL);
+
+
+#define property_str_write(prop, size)					\
+static ssize_t si4713_##prop##_write(struct device *dev,		\
+					struct device_attribute *attr,	\
+					const char *buf,		\
+					size_t count)			\
+{									\
+	struct si4713_device *sdev = dev_get_drvdata(dev);		\
+	int rval;							\
+	u8 *in;								\
+									\
+	if (!sdev)							\
+		return -ENODEV;						\
+									\
+	in = kzalloc(size + 1, GFP_KERNEL);				\
+	if (!in)							\
+		return -ENOMEM;						\
+									\
+	/* We don't want to miss the spaces */				\
+	strncpy(in, buf, size);						\
+	rval = si4713_set_##prop(sdev, in);				\
+									\
+	kfree(in);							\
+									\
+	return rval < 0 ? rval : count;					\
+}
+
+#define property_str_read(prop, size)					\
+static ssize_t si4713_##prop##_read(struct device *dev,			\
+					struct device_attribute *attr,	\
+					char *buf)			\
+{									\
+	struct si4713_device *sdev = dev_get_drvdata(dev);		\
+	int count;							\
+	u8 *out;							\
+									\
+	if (!sdev)							\
+		return -ENODEV;						\
+									\
+	out = kzalloc(size + 1, GFP_KERNEL);				\
+	if (!out)							\
+		return -ENOMEM;						\
+									\
+	si4713_get_##prop(sdev, out);					\
+	count = sprintf(buf, "%s\n", out);				\
+									\
+	kfree(out);							\
+									\
+	return count;							\
+}
+
+#define DEFINE_SYSFS_PROPERTY_STR(prop, size)				\
+property_str_write(prop, size)						\
+property_str_read(prop, size)						\
+static DEVICE_ATTR(prop, S_IRUGO | S_IWUSR, si4713_##prop##_read,	\
+					si4713_##prop##_write);
+
+/*
+ * Power level property
+ */
+/* power_level (rw) 88 - 115 or 0 */
+static ssize_t si4713_power_level_write(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf,
+					size_t count)
+{
+	struct si4713_device *sdev = dev_get_drvdata(dev);
+	unsigned int p;
+	int rval, pl;
+
+	if (!sdev) {
+		rval = -ENODEV;
+		goto exit;
+	}
+
+	sscanf(buf, "%u", &p);
+
+	pl = si4713_get_power_level(sdev);
+	if (pl < 0) {
+		rval = pl;
+		goto exit;
+	}
+
+	rval = si4713_set_power_level(sdev, p);
+
+exit:
+	return rval < 0 ? rval : count;
+}
+property_read(power_level, unsigned int, "%u")
+static DEVICE_ATTR(power_level, S_IRUGO | S_IWUSR, si4713_power_level_read,
+					si4713_power_level_write);
+
+DEFINE_SYSFS_PROPERTY(antenna_capacitor, unsigned, int, "%u",
+			value > SI4713_MAX_ANTCAP)
+/*
+ * RDS properties
+ */
+/* rds_pi (rw) 0 - 0xFFFF */
+DEFINE_SYSFS_PROPERTY(rds_pi, unsigned, int, "%x", 0)
+/* rds_pty (rw) 0 - 0x1F */
+DEFINE_SYSFS_PROPERTY(rds_pty, unsigned, int, "%u", value > MAX_RDS_PTY)
+/* rds_enabled (rw) 0 - 1 */
+DEFINE_SYSFS_PROPERTY(rds_enabled, unsigned, int, "%u", 0)
+/* rds_ps_name (rw) strlen (8 - 96) */
+DEFINE_SYSFS_PROPERTY_STR(rds_ps_name, MAX_RDS_PS_NAME)
+/* rds_radio_text (rw) strlen (0 - 384) */
+DEFINE_SYSFS_PROPERTY_STR(rds_radio_text, MAX_RDS_RADIO_TEXT)
+
+/*
+ * Limiter properties
+ */
+/* limiter_release_time (rw) 0 - 102390 */
+DEFINE_SYSFS_PROPERTY(limiter_release_time, unsigned, long, "%lu",
+			value > MAX_LIMITER_RELEASE_TIME)
+/* limiter_deviation (rw) 0 - 90000 */
+DEFINE_SYSFS_PROPERTY(limiter_deviation, unsigned, long, "%lu",
+			value > MAX_LIMITER_DEVIATION)
+/* limiter_enabled (rw) 0 - 1 */
+DEFINE_SYSFS_PROPERTY(limiter_enabled, unsigned, int, "%u", 0)
+
+/*
+ * Pilot tone properties
+ */
+/* pilot_frequency (rw) 0 - 19000 */
+DEFINE_SYSFS_PROPERTY(pilot_frequency, unsigned, int, "%u",
+			value > MAX_PILOT_FREQUENCY)
+/* pilot_deviation (rw) 0 - 90000 */
+DEFINE_SYSFS_PROPERTY(pilot_deviation, unsigned, long, "%lu",
+			value > MAX_PILOT_DEVIATION)
+/* pilot_enabled (rw) 0 - 1 */
+DEFINE_SYSFS_PROPERTY(pilot_enabled, unsigned, int, "%u", 0)
+
+/*
+ * Stereo properties
+ */
+/* stereo_enabled (rw) 0 - 1 */
+DEFINE_SYSFS_PROPERTY(stereo_enabled, unsigned, int, "%u", 0)
+
+/*
+ * Audio Compression properties
+ */
+/* acomp_release_time (rw) 0 - 1000000 */
+DEFINE_SYSFS_PROPERTY(acomp_release_time, unsigned, long, "%lu",
+			value > MAX_ACOMP_RELEASE_TIME)
+/* acomp_attack_time (rw) 0 - 5000 */
+DEFINE_SYSFS_PROPERTY(acomp_attack_time, unsigned, int, "%u",
+			value > MAX_ACOMP_ATTACK_TIME)
+/* acomp_threshold (rw) -40 - 0 */
+property_write(acomp_threshold, int, "%d",
+		value > MAX_ACOMP_THRESHOLD ||
+		value < MIN_ACOMP_THRESHOLD)
+
+static ssize_t si4713_acomp_threshold_read(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct si4713_device *sdev = dev_get_drvdata(dev);
+	int count;
+	s8 thres;
+
+	if (!sdev)
+		return -ENODEV;
+
+	count = si4713_get_acomp_threshold(sdev, &thres);
+
+	if (count >= 0)
+		count = sprintf(buf, "%d\n", thres);
+
+	return count;
+}
+static DEVICE_ATTR(acomp_threshold, S_IRUGO | S_IWUSR,
+					si4713_acomp_threshold_read,
+					si4713_acomp_threshold_write);
+
+/* acomp_gain (rw) 0 - 20 */
+DEFINE_SYSFS_PROPERTY(acomp_gain, unsigned, int, "%u", value > MAX_ACOMP_GAIN)
+/* acomp_enabled (rw) 0 - 1 */
+DEFINE_SYSFS_PROPERTY(acomp_enabled, unsigned, int, "%u", 0)
+
+/* Tune_measure (rw) */
+DEFINE_SYSFS_PROPERTY(tune_measure, unsigned, int, "%u", 0)
+
+/* Preemphasis property */
+DEFINE_SYSFS_PROPERTY(preemphasis, unsigned, int, "%u",
+					((value != PREEMPHASIS_USA) &&
+					(value != PREEMPHASIS_EU) &&
+					(value != PREEMPHASIS_DISABLED)))
+
+static struct attribute *attrs[] = {
+	&dev_attr_power_level.attr,
+	&dev_attr_antenna_capacitor.attr,
+	&dev_attr_rds_pi.attr,
+	&dev_attr_rds_pty.attr,
+	&dev_attr_rds_ps_name.attr,
+	&dev_attr_rds_radio_text.attr,
+	&dev_attr_rds_enabled.attr,
+	&dev_attr_limiter_release_time.attr,
+	&dev_attr_limiter_deviation.attr,
+	&dev_attr_limiter_enabled.attr,
+	&dev_attr_pilot_frequency.attr,
+	&dev_attr_pilot_deviation.attr,
+	&dev_attr_pilot_enabled.attr,
+	&dev_attr_stereo_enabled.attr,
+	&dev_attr_acomp_release_time.attr,
+	&dev_attr_acomp_attack_time.attr,
+	&dev_attr_acomp_threshold.attr,
+	&dev_attr_acomp_gain.attr,
+	&dev_attr_acomp_enabled.attr,
+	&dev_attr_preemphasis.attr,
+	&dev_attr_tune_measure.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attrs,
+};
+
+static irqreturn_t si4713_handler(int irq, void *dev)
+{
+	struct si4713_device *sdev = dev;
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+
+	dev_dbg(&client->dev, "IRQ called, signaling completion work\n");
+	complete(&sdev->work);
+
+	return IRQ_HANDLED;
+}
+
+static int si4713_write_econtrol(struct si4713_device *sdev,
+					struct v4l2_ext_control *control)
+{
+
+	s32 rval = 0;
+	const int pval[] = {
+		PREEMPHASIS_DISABLED,
+		PREEMPHASIS_EU,
+		PREEMPHASIS_USA,
+	};
+
+	switch (control->id) {
+	/* User class controls */
+	case V4L2_CID_AUDIO_MUTE:
+		rval = si4713_set_mute(sdev, control->value);
+		break;
+	/* FMTX class controls */
+	case V4L2_CID_RDS_ENABLED:
+		rval = si4713_set_rds_enabled(sdev, control->value);
+		break;
+	case V4L2_CID_RDS_PI:
+		rval = si4713_set_rds_pi(sdev, control->value);
+		break;
+	case V4L2_CID_RDS_PTY:
+		rval = si4713_set_rds_pty(sdev, control->value);
+		break;
+	/* TODO: String controls not implemented yet */
+	case V4L2_CID_RDS_PS_NAME:
+	case V4L2_CID_RDS_RADIO_TEXT:
+		break;
+
+	case V4L2_CID_AUDIO_LIMITER_ENABLED:
+		rval = si4713_set_limiter_enabled(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_RELEASE_TIME:
+		rval = si4713_set_limiter_release_time(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_DEVIATION:
+		rval = si4713_set_limiter_deviation(sdev, control->value);
+		break;
+
+	case V4L2_CID_AUDIO_COMPRESSION_ENABLED:
+		rval = si4713_set_acomp_enabled(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_GAIN:
+		rval = si4713_set_acomp_gain(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_THRESHOLD:
+		rval = si4713_set_acomp_threshold(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME:
+		rval = si4713_set_acomp_attack_time(sdev, control->value);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME:
+		rval = si4713_set_acomp_release_time(sdev, control->value);
+		break;
+
+	case V4L2_CID_PILOT_TONE_ENABLED:
+		rval = si4713_set_pilot_enabled(sdev, control->value);
+		break;
+	case V4L2_CID_PILOT_TONE_DEVIATION:
+		rval = si4713_set_pilot_deviation(sdev, control->value);
+		break;
+	case V4L2_CID_PILOT_TONE_FREQUENCY:
+		rval = si4713_set_pilot_frequency(sdev, control->value);
+		break;
+
+	case V4L2_CID_PREEMPHASIS:
+		if (control->value >= ARRAY_SIZE(pval) || control->value < 0)
+			rval = -ERANGE;
+		else
+			rval = si4713_set_preemphasis(sdev,
+							pval[control->value]);
+		break;
+	case V4L2_CID_TUNE_POWER_LEVEL:
+		rval = si4713_set_power_level(sdev, control->value);
+		break;
+	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
+		rval = si4713_set_antenna_capacitor(sdev, control->value);
+		break;
+	default:
+		rval = -EINVAL;
+		break;
+	};
+
+	/* FIXME: There are properties with negative values */
+	if (rval >= 0) {
+		control->value = rval;
+		rval = 0;
+	}
+
+	return rval;
+}
+static int si4713_read_econtrol(struct si4713_device *sdev,
+				struct v4l2_ext_control *control)
+{
+
+	s32 rval = 0;
+	s8 val;
+
+	switch (control->id) {
+	/* User class controls */
+	case V4L2_CID_AUDIO_MUTE:
+		rval = si4713_get_mute(sdev);
+		break;
+	/* FMTX class controls */
+	case V4L2_CID_RDS_ENABLED:
+		rval = si4713_get_rds_enabled(sdev);
+		break;
+	case V4L2_CID_RDS_PI:
+		rval = si4713_get_rds_pi(sdev);
+		break;
+	case V4L2_CID_RDS_PTY:
+		rval = si4713_get_rds_pty(sdev);
+		break;
+	/* TODO: String controls not implemented yet */
+	case V4L2_CID_RDS_PS_NAME:
+	case V4L2_CID_RDS_RADIO_TEXT:
+		break;
+
+	case V4L2_CID_AUDIO_LIMITER_ENABLED:
+		rval = si4713_get_limiter_enabled(sdev);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_RELEASE_TIME:
+		rval = si4713_get_limiter_release_time(sdev);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_DEVIATION:
+		rval = si4713_get_limiter_deviation(sdev);
+		break;
+
+	case V4L2_CID_AUDIO_COMPRESSION_ENABLED:
+		rval = si4713_get_acomp_enabled(sdev);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_GAIN:
+		rval = si4713_get_acomp_gain(sdev);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_THRESHOLD:
+		rval = si4713_get_acomp_threshold(sdev, &val);
+		if (rval == 0)
+			rval = val;
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME:
+		rval = si4713_get_acomp_attack_time(sdev);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME:
+		rval = si4713_get_acomp_release_time(sdev);
+		break;
+
+	case V4L2_CID_PILOT_TONE_ENABLED:
+		rval = si4713_get_pilot_enabled(sdev);
+		break;
+	case V4L2_CID_PILOT_TONE_DEVIATION:
+		rval = si4713_get_pilot_deviation(sdev);
+		break;
+	case V4L2_CID_PILOT_TONE_FREQUENCY:
+		rval = si4713_get_pilot_frequency(sdev);
+		break;
+
+	case V4L2_CID_PREEMPHASIS:
+		rval = si4713_get_preemphasis(sdev);
+		break;
+	case V4L2_CID_TUNE_POWER_LEVEL:
+		rval = si4713_get_power_level(sdev);
+		break;
+	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
+		rval = si4713_get_antenna_capacitor(sdev);
+		break;
+	default:
+		rval = -EINVAL;
+		break;
+	};
+
+	/* FIXME: There are properties with negative values */
+	if (rval >= 0) {
+		control->value = rval;
+		rval = 0;
+	}
+
+	return rval;
+}
+
+/*
+ * Video4Linux Subdev Interface
+ */
+
+/*
+ * si4713_s_ext_ctrls - set extended controls value
+ */
+static int si4713_s_ext_ctrls(struct v4l2_subdev *sd,
+				struct v4l2_ext_controls *ctrls)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int i;
+
+	if (ctrls->ctrl_class != V4L2_CTRL_CLASS_FMTX)
+		return -EINVAL;
+
+	for (i = 0; i < ctrls->count; i++) {
+		int err = si4713_write_econtrol(sdev, ctrls->controls + i);
+
+		if (err < 0) {
+			ctrls->error_idx = i;
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * si4713_g_ext_ctrls - get extended controls value
+ */
+static int si4713_g_ext_ctrls(struct v4l2_subdev *sd,
+				struct v4l2_ext_controls *ctrls)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int i;
+
+	if (ctrls->ctrl_class != V4L2_CTRL_CLASS_FMTX)
+		return -EINVAL;
+
+	for (i = 0; i < ctrls->count; i++) {
+		int err = si4713_read_econtrol(sdev, ctrls->controls + i);
+
+		if (err < 0) {
+			ctrls->error_idx = i;
+			return err;
+		}
+	}
+
+	return 0;
+}
+/*
+ * si4713_queryctrl - enumerate control items
+ */
+static int si4713_queryctrl(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc)
+{
+	int rval = 0;
+
+	switch (qc->id) {
+	/* User class controls */
+	/* These are only for kradio and such apps */
+	case V4L2_CID_AUDIO_VOLUME:
+	case V4L2_CID_AUDIO_BALANCE:
+	case V4L2_CID_AUDIO_BASS:
+	case V4L2_CID_AUDIO_TREBLE:
+	case V4L2_CID_AUDIO_LOUDNESS:
+		rval = v4l2_ctrl_query_fill(qc, 0, 0, 0, 0);
+		qc->flags |= V4L2_CTRL_FLAG_DISABLED;
+		break;
+	case V4L2_CID_AUDIO_MUTE:
+		rval = v4l2_ctrl_query_fill(qc, 0, 1, 1, DEFAULT_MUTE);
+		break;
+	/* FMTX class controls */
+	case V4L2_CID_RDS_ENABLED:
+		rval = v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
+		break;
+	case V4L2_CID_RDS_PI:
+		rval = v4l2_ctrl_query_fill(qc, 0, 0xFFFF, 1, DEFAULT_RDS_PI);
+		break;
+	case V4L2_CID_RDS_PTY:
+		rval = v4l2_ctrl_query_fill(qc, 0, 31, 1, DEFAULT_RDS_PTY);
+		break;
+	/* TODO: String controls not implemented yet */
+	case V4L2_CID_RDS_PS_NAME:
+		rval = v4l2_ctrl_query_fill(qc, 0, 0, 0, 0);
+		break;
+	case V4L2_CID_RDS_RADIO_TEXT:
+		rval = v4l2_ctrl_query_fill(qc, 0, 0, 0, 0);
+		break;
+
+	case V4L2_CID_AUDIO_LIMITER_ENABLED:
+		rval = v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_RELEASE_TIME:
+		rval = v4l2_ctrl_query_fill(qc, 250, MAX_LIMITER_RELEASE_TIME,
+						50, DEFAULT_LIMITER_RTIME);
+		break;
+	case V4L2_CID_AUDIO_LIMITER_DEVIATION:
+		rval = v4l2_ctrl_query_fill(qc, 0, MAX_LIMITER_DEVIATION,
+						10, DEFAULT_LIMITER_DEV);
+		break;
+
+	case V4L2_CID_AUDIO_COMPRESSION_ENABLED:
+		rval = v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_GAIN:
+		rval = v4l2_ctrl_query_fill(qc, 0, MAX_ACOMP_GAIN, 1,
+						DEFAULT_ACOMP_GAIN);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_THRESHOLD:
+		rval = v4l2_ctrl_query_fill(qc, MIN_ACOMP_THRESHOLD,
+						MAX_ACOMP_THRESHOLD, 1,
+						DEFAULT_ACOMP_THRESHOLD);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME:
+		rval = v4l2_ctrl_query_fill(qc, 0, MAX_ACOMP_ATTACK_TIME,
+						500, DEFAULT_ACOMP_ATIME);
+		break;
+	case V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME:
+		rval = v4l2_ctrl_query_fill(qc, 100000, MAX_ACOMP_RELEASE_TIME,
+						100000, DEFAULT_ACOMP_RTIME);
+		break;
+
+	case V4L2_CID_PILOT_TONE_ENABLED:
+		rval = v4l2_ctrl_query_fill(qc, 0, 1, 1, 1);
+		break;
+	case V4L2_CID_PILOT_TONE_DEVIATION:
+		rval = v4l2_ctrl_query_fill(qc, 0, MAX_PILOT_DEVIATION,
+						10, DEFAULT_PILOT_DEVIATION);
+		break;
+	case V4L2_CID_PILOT_TONE_FREQUENCY:
+		rval = v4l2_ctrl_query_fill(qc, 0, MAX_PILOT_FREQUENCY,
+						1, DEFAULT_PILOT_FREQUENCY);
+		break;
+
+	case V4L2_CID_PREEMPHASIS:
+		rval = v4l2_ctrl_query_fill(qc, V4L2_FMTX_PREEMPHASIS_DISABLED,
+						V4L2_FMTX_PREEMPHASIS_75_uS,
+						1, V4L2_FMTX_PREEMPHASIS_50_uS);
+		break;
+	case V4L2_CID_TUNE_POWER_LEVEL:
+		rval = v4l2_ctrl_query_fill(qc, 0, 120, 1, 88);
+		break;
+	case V4L2_CID_TUNE_ANTENNA_CAPACITOR:
+		rval = v4l2_ctrl_query_fill(qc, 0, 191, 1, 0);
+		break;
+	default:
+		rval = -EINVAL;
+		break;
+	};
+
+	return rval;
+}
+
+/*
+ * si4713_g_ctrl - get the value of a control
+ */
+static int si4713_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval = 0;
+
+	if (!sdev)
+		return -ENODEV;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUDIO_MUTE:
+		rval = si4713_get_mute(sdev);
+		if (rval >= 0)
+			ctrl->value = rval;
+		break;
+	}
+
+	return rval;
+}
+
+/*
+ * si4713_s_ctrl - set the value of a control
+ */
+static int si4713_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval = 0;
+
+	if (!sdev)
+		return -ENODEV;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUDIO_MUTE:
+		if (ctrl->value) {
+			rval = si4713_set_mute(sdev, ctrl->value);
+			if (rval < 0)
+				goto exit;
+
+			rval = si4713_set_power_state(sdev, POWER_DOWN);
+		} else {
+			rval = si4713_set_power_state(sdev, POWER_UP);
+			if (rval < 0)
+				goto exit;
+
+			rval = si4713_setup(sdev);
+			if (rval < 0)
+				goto exit;
+
+			rval = si4713_set_mute(sdev, ctrl->value);
+		}
+		break;
+	}
+
+exit:
+	return rval;
+}
+
+static const struct v4l2_subdev_core_ops si4713_subdev_core_ops = {
+	.try_ext_ctrls	= NULL,
+	.g_chip_ident	= NULL,
+	.log_status	= NULL,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register	= NULL,
+	.s_register	= NULL,
+#endif
+	.queryctrl	= si4713_queryctrl,
+	.g_ext_ctrls	= si4713_g_ext_ctrls,
+	.s_ext_ctrls	= si4713_s_ext_ctrls,
+	.g_ctrl		= si4713_g_ctrl,
+	.s_ctrl		= si4713_s_ctrl,
+	.ioctl		= NULL,
+	.querymenu	= NULL,
+	.init		= NULL,
+	.load_fw	= NULL,
+	.reset		= NULL,
+	.s_gpio		= NULL,
+	.s_std		= NULL,
+};
+
+/*
+ * si4713_g_tuner - get tuner attributes
+ */
+static int si4713_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *tuner)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval;
+
+	if (!sdev) {
+		rval = -ENODEV;
+		goto exit;
+	}
+
+	if (tuner->index > 0) {
+		rval = -EINVAL;
+		goto exit;
+	}
+
+	strncpy(tuner->name, "FM Transmitter", 32);
+	tuner->type = V4L2_TUNER_RADIO;
+	tuner->rxsubchans = V4L2_TUNER_SUB_STEREO | V4L2_TUNER_SUB_MONO;
+	tuner->capability = V4L2_TUNER_CAP_STEREO | V4L2_TUNER_CAP_LOW;
+
+	/* Report current frequency range limits */
+	tuner->rangelow = dev_to_v4l2(7600);
+	tuner->rangehigh = dev_to_v4l2(10800);
+
+	/* Report current audio mode: mono or stereo */
+	tuner->audmode = V4L2_TUNER_MODE_MONO;
+	rval = si4713_get_stereo_enabled(sdev);
+	if (rval < 0)
+		goto exit;
+	if (rval)
+		tuner->audmode |= V4L2_TUNER_MODE_STEREO;
+
+	/* Report current signal length */
+	rval = si4713_get_tune_measure(sdev);
+	if (rval < 0)
+		goto exit;
+	tuner->signal = rval;
+
+	/* automatic frequency control: -1: freq to low, 1 freq to high */
+	tuner->afc = 0;
+
+exit:
+	return rval;
+}
+
+/*
+ * si4713_s_tuner - set tuner attributes
+ */
+static int si4713_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner *tuner)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval;
+
+	if (!sdev) {
+		rval = -ENODEV;
+		goto exit;
+	}
+
+	if (tuner->index > 0) {
+		rval = -EINVAL;
+		goto exit;
+	}
+
+	/* Set audio mode: mono or stereo */
+	rval = si4713_set_stereo_enabled(sdev,
+				!!(tuner->audmode & V4L2_TUNER_MODE_STEREO));
+	if (rval < 0)
+		goto exit;
+
+	/* TODO: How to set frequency to measure current signal length */
+
+exit:
+	return rval;
+}
+
+/*
+ * si4713_g_frequency - get tuner or modulator radio frequency
+ */
+static int si4713_g_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval = 0;
+	int freq;
+
+	f->type = V4L2_TUNER_RADIO;
+	freq = si4713_get_frequency(sdev);
+
+	if (freq < 0)
+		rval = freq;
+	else
+		f->frequency = dev_to_v4l2(freq);
+
+	return rval;
+}
+
+/*
+ * si4713_s_frequency - set tuner or modulator radio frequency
+ */
+static int si4713_s_frequency(struct v4l2_subdev *sd, struct v4l2_frequency *f)
+{
+	struct si4713_device *sdev = to_si4713_device(sd);
+	int rval = 0;
+
+	if (f->type != V4L2_TUNER_RADIO) {
+		rval = -EINVAL;
+		goto exit;
+	}
+
+	rval = si4713_set_frequency(sdev, v4l2_to_dev(f->frequency));
+
+exit:
+	return rval;
+}
+
+static const struct v4l2_subdev_tuner_ops si4713_subdev_tuner_ops = {
+	.g_frequency	= si4713_g_frequency,
+	.s_frequency	= si4713_s_frequency,
+	.g_tuner	= si4713_g_tuner,
+	.s_tuner	= si4713_s_tuner,
+	.s_type_addr	= NULL,
+	.s_config	= NULL,
+	.s_standby	= NULL,
+	.s_mode		= NULL,
+	.s_radio	= NULL,
+};
+
+static const struct v4l2_subdev_ops si4713_subdev_ops = {
+	.core		= &si4713_subdev_core_ops,
+	.tuner		= &si4713_subdev_tuner_ops,
+};
+
+/*
+ * I2C driver interface
+ */
+/*
+ * si4713_i2c_driver_probe - probe for the device
+ */
+static int si4713_i2c_driver_probe(struct i2c_client *client,
+					const struct i2c_device_id *id)
+{
+	struct si4713_device *sdev;
+	int rval;
+
+	sdev = kzalloc(sizeof *sdev, GFP_KERNEL);
+	if (!sdev) {
+		dev_dbg(&client->dev, "Failed to alloc video device.\n");
+		rval = -ENOMEM;
+		goto exit;
+	}
+
+	sdev->platform_data = client->dev.platform_data;
+	if (!sdev->platform_data) {
+		dev_err(&client->dev, "No platform data registered.\n");
+		rval = -ENODEV;
+		goto free_sdev;
+	}
+
+	v4l2_i2c_subdev_init(&sdev->sd, client, &si4713_subdev_ops);
+
+	mutex_init(&sdev->mutex);
+	init_completion(&sdev->work);
+
+	if (client->irq) {
+		rval = request_irq(client->irq,
+			si4713_handler, IRQF_TRIGGER_FALLING | IRQF_DISABLED,
+			client->name, sdev);
+		if (rval < 0) {
+			dev_err(&client->dev, "Could not request IRQ\n");
+			goto free_sdev;
+		}
+		dev_dbg(&client->dev, "IRQ requested.\n");
+	} else {
+		dev_info(&client->dev, "IRQ not configure. Using timeouts.\n");
+	}
+
+	rval = sysfs_create_group(&client->dev.kobj, &attr_group);
+	if (rval < 0) {
+		dev_err(&client->dev, "Could not register sysfs interface.\n");
+		goto free_irq;
+	}
+
+	rval = si4713_probe(sdev);
+	if (rval < 0) {
+		dev_err(&client->dev, "Failed to probe device information.\n");
+		goto free_sysfs;
+	}
+
+	return 0;
+
+free_sysfs:
+	sysfs_remove_group(&client->dev.kobj, &attr_group);
+free_irq:
+	if (client->irq)
+		free_irq(client->irq, sdev);
+free_sdev:
+	kfree(sdev);
+exit:
+	return rval;
+}
+
+/*
+ * si4713_i2c_driver_remove - remove the device
+ */
+static int __exit si4713_i2c_driver_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct si4713_device *sdev = to_si4713_device(sd);
+
+	/* our client isn't attached */
+	if (!client->adapter)
+		return -ENODEV;
+
+	if (sdev) {
+		sysfs_remove_group(&client->dev.kobj, &attr_group);
+
+		if (sdev->power_state)
+			si4713_set_power_state(sdev, POWER_DOWN);
+
+		if (client->irq > 0)
+			free_irq(client->irq, sdev);
+
+		v4l2_device_unregister_subdev(sd);
+
+		kfree(sdev);
+	}
+
+	return 0;
+}
+
+/*
+ * si4713_i2c_driver - i2c driver interface
+ */
+static const struct i2c_device_id si4713_id[] = {
+	{ "si4713" , 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, si4713_id);
+
+static struct v4l2_i2c_driver_data v4l2_i2c_data = {
+	.name		= "si4713",
+	.probe		= si4713_i2c_driver_probe,
+	.remove         = __exit_p(si4713_i2c_driver_remove),
+	.id_table       = si4713_id,
+};
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eduardo Valentin <eduardo.valentin@nokia.com>");
+MODULE_DESCRIPTION("I2C driver for Si4713 FM Radio Transmitter");
+MODULE_VERSION("0.0.1");
diff -r 85b64a6cc67c -r 94b5043b692a linux/drivers/media/radio/si4713.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/radio/si4713.c	Wed May 27 11:56:46 2009 +0300
@@ -0,0 +1,2100 @@
+/*
+ * drivers/media/radio/si4713.c
+ *
+ * Silicon Labs Si4713 FM Radio Transmitter I2C commands.
+ *
+ * Copyright (c) 2008 Instituto Nokia de Tecnologia - INdT
+ * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <linux/mutex.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+
+#include "si4713.h"
+
+#define MAX_ARGS 7
+
+#define RDS_BLOCK			8
+#define RDS_BLOCK_CLEAR			0x03
+#define RDS_BLOCK_LOAD			0x04
+#define RDS_RADIOTEXT_2A		0x20
+#define RDS_RADIOTEXT_BLK_SIZE		4
+#define RDS_RADIOTEXT_INDEX_MAX		0x0F
+#define RDS_CARRIAGE_RETURN		0x0D
+
+#define rds_ps_nblocks(len)	((len / RDS_BLOCK) + (len % RDS_BLOCK ? 1 : 0))
+#define enable_rds(p)		(p | (1 << 2))
+#define disable_rds(p)		(p & ~(1 << 2))
+#define get_rds_status(p)	((p >> 2) & 0x01)
+
+#define enable_stereo(p)	(p | (1 << 1))
+#define disable_stereo(p)	(p & ~(1 << 1))
+#define get_stereo_status(p)	((p >> 1) & 0x01)
+
+#define enable_limiter(p)	(p | (1 << 1))
+#define disable_limiter(p)	(p & ~(1 << 1))
+#define get_limiter_status(p)	((p >> 1) & 0x01)
+
+#define enable_pilot(p)		(p | (1 << 0))
+#define disable_pilot(p)	(p & ~(1 << 0))
+#define get_pilot_status(p)	((p >> 0) & 0x01)
+
+#define enable_acomp(p)		(p | (1 << 0))
+#define disable_acomp(p)	(p & ~(1 << 0))
+#define get_acomp_status(p)	((p >> 0) & 0x01)
+#define ATTACK_TIME_UNIT	500
+
+#define DEFAULT_RDS_PI			0x00
+#define DEFAULT_RDS_PTY			0x00
+#define DEFAULT_RDS_PS_NAME		"Si4713  "
+#define DEFAULT_RDS_RADIO_TEXT		DEFAULT_RDS_PS_NAME
+#define DEFAULT_RDS_DEVIATION		0x00C8
+#define DEFAULT_RDS_PS_REPEAT_COUNT	0x0003
+#define DEFAULT_LIMITER_RTIME		0x1392
+#define DEFAULT_LIMITER_DEV		0x102CA
+#define DEFAULT_PILOT_FREQUENCY 	0x4A38
+#define DEFAULT_PILOT_DEVIATION		0x1A5E
+#define DEFAULT_ACOMP_ATIME		0x0000
+#define DEFAULT_ACOMP_RTIME		0xF4240L
+#define DEFAULT_ACOMP_GAIN		0x0F
+#define DEFAULT_ACOMP_THRESHOLD 	(-0x28)
+#define DEFAULT_MUTE			0x00
+#define DEFAULT_POWER_LEVEL		88
+#define DEFAULT_FREQUENCY		8800
+#define DEFAULT_TUNE_RSSI		0xFF
+
+#define POWER_OFF			0x00
+#define POWER_ON			0x01
+
+#define msb(x)                  ((u8)((u16) x >> 8))
+#define lsb(x)                  ((u8)((u16) x &  0x00FF))
+#define compose_u16(msb, lsb)	(((u16)msb << 8) | lsb)
+#define check_command_failed(status)	(!(status & SI4713_CTS) || \
+					(status & SI4713_ERR))
+/* mute definition */
+#define set_mute(p)	((p & 1) | ((p & 1) << 1));
+#define get_mute(p)	(p & 0x01)
+#define set_pty(v, pty)	((v & 0xFC1F) | (pty << 5))
+#define get_pty(v)	((v >> 5) & 0x1F)
+
+
+#ifdef DEBUG
+#define DBG_BUFFER(device, message, buffer, size)			\
+	{								\
+		int i;							\
+		char str[(size)*5];					\
+		for (i = 0; i < size; i++)				\
+			sprintf(str + i * 5, " 0x%02x", buffer[i]);	\
+		dev_dbg(device, "%s:%s\n", message, str);		\
+	}
+#else
+#define DBG_BUFFER(device, message, buffer, size)
+#endif
+
+/*
+ * Values for limiter release time
+ *	device	release
+ *	value	time (us)
+ */
+static unsigned long const limiter_times[] = {
+	2000,	250,
+	1000,	500,
+	510,	1000,
+	255,	2000,
+	170,	3000,
+	127,	4020,
+	102,	5010,
+	85,	6020,
+	73,	7010,
+	64,	7990,
+	57,	8970,
+	51,	10030,
+	25,	20470,
+	17,	30110,
+	13,	39380,
+	10,	51190,
+	8,	63690,
+	7,	73140,
+	6,	85330,
+	5,	102390,
+};
+
+/*
+ * Values for audio compression release time
+ *	device	release
+ *	value	time (us)
+ */
+static unsigned long const acomp_rtimes[] = {
+	0,	100000,
+	1,	200000,
+	2,	350000,
+	3,	525000,
+	4,	1000000,
+};
+
+static int usecs_to_dev(unsigned long usecs, unsigned long const array[],
+			int size)
+{
+	int i;
+	int rval = -EINVAL;
+
+	for (i = 0; i < size / 2; i++)
+		if (array[(i * 2) + 1] >= usecs) {
+			rval = array[i * 2];
+			break;
+		}
+
+	return rval;
+}
+
+static unsigned long dev_to_usecs(int value, unsigned long const array[],
+			int size)
+{
+	int i;
+	int rval = -EINVAL;
+
+	for (i = 0; i < size / 2; i++)
+		if (array[i * 2] == value) {
+			rval = array[(i * 2) + 1];
+			break;
+		}
+
+	return rval;
+}
+
+/*
+ * si4713_send_command - sends a command to si4713 and waits its response
+ * @sdev: si4713_device structure for the device we are communicating
+ * @command: command id
+ * @args: command arguments we are sending (up to 7)
+ * @argn: actual size of @args
+ * @response: buffer to place the expected response from the device (up to 15)
+ * @respn: actual size of @response
+ * @usecs: amount of time to wait before reading the response (in usecs)
+ */
+static int si4713_send_command(struct si4713_device *sdev, const u8 command,
+				const u8 args[], const int argn,
+				u8 response[], const int respn, const int usecs)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	u8 data1[MAX_ARGS + 1];
+	int err;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* First send the command and its arguments */
+	data1[0] = command;
+	memcpy(data1 + 1, args, argn);
+	DBG_BUFFER(&client->dev, "Parameters", data1, argn + 1);
+
+	err = i2c_master_send(client, data1, argn + 1);
+	if (err != argn + 1) {
+		dev_err(&client->dev, "Error while sending command 0x%02x\n",
+			command);
+		return (err > 0) ? -EIO : err;
+	}
+
+	/* Wait response from interrupt */
+	if (!wait_for_completion_timeout(&sdev->work,
+				usecs_to_jiffies(usecs) + 1))
+		dev_dbg(&client->dev, "Device took too much time.\n");
+
+	/* Then get the response */
+	err = i2c_master_recv(client, response, respn);
+	if (err != respn) {
+		dev_err(&client->dev,
+			"Error while reading response for command 0x%02x\n",
+			command);
+		return (err > 0) ? -EIO : err;
+	}
+
+	DBG_BUFFER(&client->dev, "Response", response, respn);
+	if (check_command_failed(response[0]))
+		return -EBUSY;
+
+	return 0;
+}
+
+/*
+ * si4713_read_property - reads a si4713 property
+ * @sdev: si4713_device structure for the device we are communicating
+ * @prop: property identification number
+ */
+static int si4713_read_property(struct si4713_device *sdev, u16 prop)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_GET_PROP_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = 0
+	 * 	.Second byte = property's MSB
+	 * 	.Third byte = property's LSB
+	 */
+	const u8 args[SI4713_GET_PROP_NARGS] = {
+		0x00,
+		msb(prop),
+		lsb(prop),
+	};
+
+	err = si4713_send_command(sdev, SI4713_CMD_GET_PROPERTY,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "Status from read prop: 0x%02x\n", val[0]);
+
+	return compose_u16(val[2], val[3]);
+}
+
+/*
+ * si4713_write_property - modifies a si4713 property
+ * @sdev: si4713_device structure for the device we are communicating
+ * @prop: property identification number
+ * @val: new value for that property
+ */
+static int si4713_write_property(struct si4713_device *sdev, u16 prop, u16 val)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int rval;
+	u8 resp[SI4713_SET_PROP_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = 0
+	 * 	.Second byte = property's MSB
+	 * 	.Third byte = property's LSB
+	 * 	.Fourth byte = value's MSB
+	 * 	.Fifth byte = value's LSB
+	 */
+	const u8 args[SI4713_SET_PROP_NARGS] = {
+		0x00,
+		msb(prop),
+		lsb(prop),
+		msb(val),
+		lsb(val),
+	};
+
+	rval = si4713_send_command(sdev, SI4713_CMD_SET_PROPERTY,
+					args, ARRAY_SIZE(args),
+					resp, ARRAY_SIZE(resp),
+					DEFAULT_TIMEOUT);
+
+	if (rval < 0)
+		return rval;
+
+	dev_dbg(&client->dev, "Status from write prop: 0x%02x\n",
+		resp[0]);
+
+	/*
+	 * As there is no command response for SET_PROPERTY,
+	 * wait Tcomp time to finish before proceed, in order
+	 * to have property properly set.
+	 */
+	msleep(TIMEOUT_SET_PROPERTY);
+
+	return rval;
+}
+
+/*
+ * si4713_powerup - Powers the device up
+ * @sdev: si4713_device structure for the device we are communicating
+ */
+static int si4713_powerup(struct si4713_device *sdev)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 resp[SI4713_PWUP_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = Enabled interrupts and boot function
+	 * 	.Second byte = Input operation mode
+	 */
+	const u8 args[SI4713_PWUP_NARGS] = {
+		SI4713_PWUP_CTSIEN | SI4713_PWUP_GPO2OEN | SI4713_PWUP_FUNC_TX,
+		SI4713_PWUP_OPMOD_ANALOG,
+	};
+
+	if (sdev->power_state)
+		return 0;
+
+	sdev->platform_data->set_power(1);
+	err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
+					args, ARRAY_SIZE(args),
+					resp, ARRAY_SIZE(resp),
+					TIMEOUT_POWER_UP);
+
+	if (!err) {
+		dev_dbg(&client->dev, "Powerup response: 0x%02x\n",
+			resp[0]);
+		dev_dbg(&client->dev, "Device in power up mode\n");
+		sdev->power_state = POWER_ON;
+
+		err = si4713_write_property(sdev, SI4713_GPO_IEN,
+						SI4713_STC_INT | SI4713_CTS);
+	} else {
+		sdev->platform_data->set_power(0);
+	}
+
+	return err;
+}
+
+/*
+ * si4713_powerdown - Powers the device down
+ * @sdev: si4713_device structure for the device we are communicating
+ */
+static int si4713_powerdown(struct si4713_device *sdev)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 resp[SI4713_PWDN_NRESP];
+
+	if (!sdev->power_state)
+		return 0;
+
+	err = si4713_send_command(sdev, SI4713_CMD_POWER_DOWN,
+					NULL, 0,
+					resp, ARRAY_SIZE(resp),
+					DEFAULT_TIMEOUT);
+
+	if (!err) {
+		dev_dbg(&client->dev, "Power down response: 0x%02x\n",
+			resp[0]);
+		dev_dbg(&client->dev, "Device in reset mode\n");
+		sdev->platform_data->set_power(0);
+		sdev->power_state = POWER_OFF;
+	}
+
+	return err;
+}
+
+/*
+ * si4713_checkrev - Checks if we are treating a device with the correct rev.
+ * @sdev: si4713_device structure for the device we are communicating
+ */
+#define pr_revision(devicep, buffer)					\
+	dev_info(devicep, "Detected %s (0x%02x) Firmware: %d.%d"	\
+			  " Patch ID: %02x:%02x Component: %d.%d"	\
+			  " Chip Rev.: %s\n",				\
+			buffer[1] == SI4713_PRODUCT_NUMBER ? "Si4713" : "",\
+			buffer[1],					\
+			buffer[2] & 0xF, buffer[3] & 0xF,		\
+			buffer[4], buffer[5],				\
+			buffer[6] & 0xF, buffer[7] & 0xF,		\
+			buffer[8] == 0x41 ? "revA" : "unknown")
+static int si4713_checkrev(struct si4713_device *sdev)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int rval;
+	u8 resp[SI4713_GETREV_NRESP];
+
+	mutex_lock(&sdev->mutex);
+
+	rval = si4713_send_command(sdev, SI4713_CMD_GET_REV,
+					NULL, 0,
+					resp, ARRAY_SIZE(resp),
+					DEFAULT_TIMEOUT);
+
+	if (rval < 0)
+		goto unlock;
+
+	if (resp[1] == SI4713_PRODUCT_NUMBER) {
+		pr_revision(&client->dev, resp);
+	} else {
+		dev_err(&client->dev, "Invalid product number\n");
+		rval = -EINVAL;
+	}
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+/*
+ * si4713_wait_stc - Waits STC interrupt and clears status bits. Usefull
+ *		     for TX_TUNE_POWER, TX_TUNE_FREQ and TX_TUNE_MEAS
+ * @sdev: si4713_device structure for the device we are communicating
+ * @usecs: timeout to wait for STC interrupt signal
+ */
+static int si4713_wait_stc(struct si4713_device *sdev, const int usecs)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 resp[SI4713_GET_STATUS_NRESP];
+
+	/* Wait response from STC interrupt */
+	if (!wait_for_completion_timeout(&sdev->work,
+			usecs_to_jiffies(TIMEOUT_TX_TUNE) + 1))
+		dev_dbg(&client->dev, "Device took too much time.\n");
+
+	/* Clear status bits */
+	err = si4713_send_command(sdev, SI4713_CMD_GET_INT_STATUS,
+					NULL, 0,
+					resp, ARRAY_SIZE(resp),
+					DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		goto exit;
+
+	dev_dbg(&client->dev, "Status bits: 0x%02x\n", resp[0]);
+
+	if (!(resp[0] & SI4713_STC_INT))
+		err = -EIO;
+
+exit:
+	return err;
+}
+
+/*
+ * si4713_tx_tune_freq - Sets the state of the RF carrier and sets the tuning
+ * 			frequency between 76 and 108 MHz in 10 kHz units and
+ * 			steps of 50 kHz.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @frequency: desired frequency (76 - 108 MHz, unit 10 KHz, step 50 kHz)
+ */
+static int si4713_tx_tune_freq(struct si4713_device *sdev, u16 frequency)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_TXFREQ_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = 0
+	 * 	.Second byte = frequency's MSB
+	 * 	.Third byte = frequency's LSB
+	 */
+	const u8 args[SI4713_TXFREQ_NARGS] = {
+		0x00,
+		msb(frequency),
+		lsb(frequency),
+	};
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_TUNE_FREQ,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "Status from tx tune freq: 0x%02x\n",
+		val[0]);
+
+	err = si4713_wait_stc(sdev, TIMEOUT_TX_TUNE);
+	if (err < 0)
+		return err;
+
+	return compose_u16(args[1], args[2]);
+}
+
+/*
+ * si4713_tx_tune_power - Sets the RF voltage level between 88 and 115 dBuV in
+ * 			1 dB units. A value of 0x00 indicates off. The command
+ * 			also sets the antenna tuning capacitance. A value of 0
+ * 			indicates autotuning, and a value of 1 - 191 indicates
+ * 			a manual override, which results in a tuning
+ * 			capacitance of 0.25 pF x @antcap.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @power: tuning power (88 - 115 dBuV, unit/step 1 dB)
+ * @antcap: value of antenna tuning capacitor (0 - 191)
+ */
+static int si4713_tx_tune_power(struct si4713_device *sdev, u8 power,
+				u8 antcap)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_TXPWR_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = 0
+	 * 	.Second byte = 0
+	 * 	.Third byte = power
+	 * 	.Fourth byte = antcap
+	 */
+	const u8 args[SI4713_TXPWR_NARGS] = {
+		0x00,
+		0x00,
+		power,
+		antcap,
+	};
+
+	if (((power > 0) && (power < SI4713_MIN_POWER)) ||
+		power > SI4713_MAX_POWER || antcap > SI4713_MAX_ANTCAP)
+		return -EDOM;
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_TUNE_POWER,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "Status from tx tune power: 0x%02x\n",
+		val[0]);
+
+	return si4713_wait_stc(sdev, TIMEOUT_TX_TUNE_POWER);
+}
+
+/*
+ * si4713_tx_tune_measure - Enters receive mode and measures the received noise
+ * 			level in units of dBuV on the selected frequency.
+ * 			The Frequency must be between 76 and 108 MHz in 10 kHz
+ * 			units and steps of 50 kHz. The command also sets the
+ * 			antenna	tuning capacitance. A value of 0 means
+ * 			autotuning, and a value of 1 to 191 indicates manual
+ * 			override.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @frequency: desired frequency (76 - 108 MHz, unit 10 KHz, step 50 kHz)
+ * @antcap: value of antenna tuning capacitor (0 - 191)
+ */
+static int si4713_tx_tune_measure(struct si4713_device *sdev, u16 frequency,
+					u8 antcap)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_TXMEA_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = 0
+	 * 	.Second byte = frequency's MSB
+	 * 	.Third byte = frequency's LSB
+	 * 	.Fourth byte = antcap
+	 */
+	const u8 args[SI4713_TXMEA_NARGS] = {
+		0x00,
+		msb(frequency),
+		lsb(frequency),
+		antcap,
+	};
+
+	sdev->tune_rssi = DEFAULT_TUNE_RSSI;
+
+	if (antcap > SI4713_MAX_ANTCAP)
+		return -EDOM;
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_TUNE_MEASURE,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "Status from tx tune measure: 0x%02x\n",
+		val[0]);
+
+	return si4713_wait_stc(sdev, TIMEOUT_TX_TUNE);
+}
+
+/*
+ * si4713_tx_tune_status- Returns the status of the tx_tune_freq, tx_tune_mea or
+ * 			tx_tune_power commands. This command return the current
+ * 			frequency, output voltage in dBuV, the antenna tunning
+ * 			capacitance value and the received noise level. The
+ * 			command also clears the stcint interrupt bit when the
+ * 			first bit of its arguments is high.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @intack: 0x01 to clear the seek/tune complete interrupt status indicator.
+ * @frequency: returned frequency
+ * @power: returned power
+ * @antcap: returned antenna capacitance
+ * @noise: returned noise level
+ */
+static int si4713_tx_tune_status(struct si4713_device *sdev, u8 intack,
+					u16 *frequency,	u8 *power,
+					u8 *antcap, u8 *noise)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_TXSTATUS_NRESP];
+	/*
+	 * REVISIT: From Programming Manual
+	 * 	.First byte = intack bit
+	 */
+	const u8 args[SI4713_TXSTATUS_NARGS] = {
+		intack & SI4713_INTACK_MASK,
+	};
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_TUNE_STATUS,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (!err) {
+		dev_dbg(&client->dev,
+			"Status from tx tune status: 0x%02x\n", val[0]);
+		*frequency = compose_u16(val[2], val[3]);
+		sdev->frequency = *frequency;
+		*power = val[5];
+		*antcap = val[6];
+		*noise = val[7];
+		dev_dbg(&client->dev, "Tune status: %d x 10 kHz "
+				"(power %d, antcap %d, rnl %d)\n",
+				*frequency, *power, *antcap, *noise);
+	}
+
+	return err;
+}
+
+/*
+ * si4713_tx_rds_buff - Loads the RDS group buffer FIFO or circular buffer.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @mode: the buffer operation mode.
+ * @rdsb: RDS Block B
+ * @rdsc: RDS Block C
+ * @rdsd: RDS Block D
+ * @intstatus: returns current interrupt status
+ * @cbavail: returns the number of available circular buffer blocks.
+ * @cbused: returns the number of used circular buffer blocks.
+ * @fifoavail: returns the number of available fifo buffer blocks.
+ * @fifoused: returns the number of used fifo buffer blocks.
+ */
+static int si4713_tx_rds_buff(struct si4713_device *sdev, u8 mode, u16 rdsb,
+				u16 rdsc, u16 rdsd, u8 *intstatus, u8 *cbavail,
+				u8 *cbused, u8 *fifoavail, u8 *fifoused)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_RDSBUFF_NRESP];
+
+	const u8 args[SI4713_RDSBUFF_NARGS] = {
+		mode & SI4713_RDSBUFF_MODE_MASK,
+		msb(rdsb),
+		lsb(rdsb),
+		msb(rdsc),
+		lsb(rdsc),
+		msb(rdsd),
+		lsb(rdsd),
+	};
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_RDS_BUFF,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (!err) {
+		dev_dbg(&client->dev,
+			"Status from tx rds buff: 0x%02x\n", val[0]);
+		*intstatus = val[1];
+		*cbavail = val[2];
+		*cbused = val[3];
+		*fifoavail = val[4];
+		*fifoused = val[5];
+		dev_dbg(&client->dev, "rds buffer status: interrupts"
+				" 0x%02x cb avail: %d cb used %d fifo avail"
+				" %d fifo used %d\n", *intstatus, *cbavail,
+				*cbused, *fifoavail, *fifoused);
+	}
+
+	return err;
+}
+
+/*
+ * si4713_tx_rds_ps - Loads the program service buffer.
+ * @sdev: si4713_device structure for the device we are communicating
+ * @psid: program service id to be loaded.
+ * @pschar: assumed 4 size char array to be loaded into the program service
+ */
+static int si4713_tx_rds_ps(struct si4713_device *sdev, u8 psid,
+				unsigned char *pschar)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
+	int err;
+	u8 val[SI4713_RDSPS_NRESP];
+
+	const u8 args[SI4713_RDSPS_NARGS] = {
+		psid & SI4713_RDSPS_PSID_MASK,
+		pschar[0],
+		pschar[1],
+		pschar[2],
+		pschar[3],
+	};
+
+	err = si4713_send_command(sdev, SI4713_CMD_TX_RDS_PS,
+				  args, ARRAY_SIZE(args), val,
+				  ARRAY_SIZE(val), DEFAULT_TIMEOUT);
+
+	if (err < 0)
+		return err;
+
+	dev_dbg(&client->dev, "Status from tx rds ps: 0x%02x\n",
+		val[0]);
+
+	return err;
+}
+
+/*
+ * si4713_init - Sets the device up with default configuration.
+ * @sdev: si4713_device structure for the device we are communicating
+ */
+int si4713_init(struct si4713_device *sdev)
+{
+	int rval;
+
+	rval = si4713_set_rds_pi(sdev, DEFAULT_RDS_PI);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_pty(sdev, DEFAULT_RDS_PTY);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_ps_name(sdev, DEFAULT_RDS_PS_NAME);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_radio_text(sdev, DEFAULT_RDS_RADIO_TEXT);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_enabled(sdev, 1);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_release_time(sdev, DEFAULT_LIMITER_RTIME);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_deviation(sdev, DEFAULT_LIMITER_DEV);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_enabled(sdev, 1);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_frequency(sdev, DEFAULT_PILOT_FREQUENCY);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_deviation(sdev, DEFAULT_PILOT_DEVIATION);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_enabled(sdev, 1);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_stereo_enabled(sdev, 1);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_attack_time(sdev, DEFAULT_ACOMP_ATIME);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_release_time(sdev, DEFAULT_ACOMP_RTIME);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_gain(sdev, DEFAULT_ACOMP_GAIN);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_threshold(sdev, DEFAULT_ACOMP_THRESHOLD);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_enabled(sdev, 1);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_mute(sdev, DEFAULT_MUTE);
+	if (rval < 0)
+		goto exit;
+
+exit:
+	return rval;
+}
+
+/*
+ * si4713_setup - Sets the device up with current configuration.
+ * @sdev: si4713_device structure for the device we are communicating
+ */
+int si4713_setup(struct si4713_device *sdev)
+{
+	struct si4713_device *tmp;
+	int rval;
+
+	tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	/* Get a local copy to avoid race */
+	mutex_lock(&sdev->mutex);
+	memcpy(tmp, sdev, sizeof(*sdev));
+	mutex_unlock(&sdev->mutex);
+
+	rval = si4713_set_rds_pi(sdev, tmp->rds_info.pi);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_pty(sdev, tmp->rds_info.pty);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_ps_name(sdev, tmp->rds_info.ps_name);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_radio_text(sdev, tmp->rds_info.radio_text);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_rds_enabled(sdev, tmp->rds_info.enabled);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_release_time(sdev,
+				tmp->limiter_info.release_time);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_deviation(sdev, tmp->limiter_info.deviation);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_limiter_enabled(sdev, tmp->limiter_info.enabled);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_frequency(sdev, tmp->pilot_info.frequency);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_deviation(sdev, tmp->pilot_info.deviation);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_pilot_enabled(sdev, tmp->pilot_info.enabled);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_stereo_enabled(sdev, tmp->stereo);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_attack_time(sdev, tmp->acomp_info.attack_time);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_release_time(sdev,
+						tmp->acomp_info.release_time);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_gain(sdev, tmp->acomp_info.gain);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_threshold(sdev, tmp->acomp_info.threshold);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_acomp_enabled(sdev, tmp->acomp_info.enabled);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_mute(sdev, tmp->mute);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_frequency(sdev, tmp->frequency ? tmp->frequency :
+					DEFAULT_FREQUENCY);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_power_level(sdev, tmp->power_level ?
+					tmp->power_level :
+					DEFAULT_POWER_LEVEL);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_antenna_capacitor(sdev, tmp->antenna_capacitor);
+
+exit:
+	kfree(tmp);
+	return rval;
+}
+
+int si4713_set_power_level(struct si4713_device *sdev, u8 power_level)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_power(sdev, power_level,
+						sdev->antenna_capacitor);
+
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->power_level = power_level;
+	rval = 0;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_power_level(struct si4713_device *sdev)
+{
+	int rval;
+	u16 f = 0;
+	u8 p, a, n;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_status(sdev, 0x00, &f, &p, &a, &n);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->power_level = p;
+	}
+
+	rval = sdev->power_level;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_antenna_capacitor(struct si4713_device *sdev, u8 value)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_tx_tune_power(sdev, sdev->power_level, value);
+
+	if (!rval)
+		sdev->antenna_capacitor = value;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_antenna_capacitor(struct si4713_device *sdev)
+{
+	int rval = -EINVAL;
+	u16 f = 0;
+	u8 p, a, n;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_status(sdev, 0x00, &f, &p, &a, &n);
+
+		if (rval < 0)
+			goto unlock;
+
+		rval = a;
+	}
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_power_state(struct si4713_device *sdev, u8 value)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (value)
+		rval = si4713_powerup(sdev);
+	else
+		rval = si4713_powerdown(sdev);
+
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_probe(struct si4713_device *sdev)
+{
+	int rval;
+
+	rval = si4713_set_power_state(sdev, POWER_ON);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_checkrev(sdev);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_init(sdev);
+	if (rval < 0)
+		goto exit;
+
+	rval = si4713_set_power_state(sdev, POWER_OFF);
+
+exit:
+	return rval;
+}
+
+int si4713_set_frequency(struct si4713_device *sdev, u16 frequency)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_freq(sdev, frequency);
+		if (rval < 0)
+			goto unlock;
+		sdev->frequency = rval;
+	} else {
+		rval = -ENODEV;
+	}
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_frequency(struct si4713_device *sdev)
+{
+	int rval;
+	u16 f = 0;
+	u8 p, a, n;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_status(sdev, 0x00, &f, &p, &a, &n);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->frequency = f;
+	}
+
+	rval = sdev->frequency;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_mute(struct si4713_device *sdev, u16 mute)
+{
+	int rval = 0;
+
+	mute = set_mute(mute);
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev,
+				SI4713_TX_LINE_INPUT_MUTE, mute);
+
+	if (rval >= 0)
+		sdev->mute = get_mute(mute);
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_mute(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_LINE_INPUT_MUTE);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->mute = rval;
+	}
+
+	rval = get_mute(sdev->mute);
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_rds_pi(struct si4713_device *sdev, u16 pi)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_RDS_PI, pi);
+
+	if (rval >= 0)
+		sdev->rds_info.pi = pi;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_rds_pi(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_RDS_PI);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->rds_info.pi = rval;
+	}
+
+	rval = sdev->rds_info.pi;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_rds_pty(struct si4713_device *sdev, u8 pty)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_RDS_PS_MISC);
+		if (rval < 0)
+			goto unlock;
+
+		p = set_pty(rval, pty);
+
+		rval = si4713_write_property(sdev, SI4713_TX_RDS_PS_MISC, p);
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->rds_info.pty = pty;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_rds_pty(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_RDS_PS_MISC);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->rds_info.pty = get_pty(rval);
+	}
+
+	rval = sdev->rds_info.pty;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_rds_ps_name(struct si4713_device *sdev, char *ps_name)
+{
+	int rval = 0, i;
+	u8 len = 0;
+	u8 *tmp;
+
+	if (!strlen(ps_name))
+		return -EINVAL;
+
+	tmp = kzalloc(MAX_RDS_PS_NAME + 1, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	strncpy(tmp, ps_name, MAX_RDS_PS_NAME);
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		/* Write the new ps name and clear the padding */
+		for (i = 0; i < MAX_RDS_PS_NAME; i += (RDS_BLOCK / 2)) {
+			rval = si4713_tx_rds_ps(sdev, (i / (RDS_BLOCK / 2)),
+						tmp + i);
+			if (rval < 0)
+				goto unlock;
+		}
+
+		/* Setup the size to be sent */
+		len = strlen(tmp) - 1;
+
+		rval = si4713_write_property(sdev,
+				SI4713_TX_RDS_PS_MESSAGE_COUNT,
+				rds_ps_nblocks(len));
+		if (rval < 0)
+			goto unlock;
+
+		rval = si4713_write_property(sdev,
+				SI4713_TX_RDS_PS_REPEAT_COUNT,
+				DEFAULT_RDS_PS_REPEAT_COUNT * 2);
+		if (rval < 0)
+			goto unlock;
+	}
+
+	strncpy(sdev->rds_info.ps_name, tmp, MAX_RDS_PS_NAME);
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	kfree(tmp);
+	return rval;
+}
+
+int si4713_get_rds_ps_name(struct si4713_device *sdev, char *ps_name)
+{
+	mutex_lock(&sdev->mutex);
+	strncpy(ps_name, sdev->rds_info.ps_name, MAX_RDS_PS_NAME);
+	mutex_unlock(&sdev->mutex);
+
+	return 0;
+}
+
+int si4713_set_rds_radio_text(struct si4713_device *sdev, char *radio_text)
+{
+	int rval = 0, i;
+	u16 t_index = 0;
+	u8 s, a, u, fa, fu, b_index = 0, cr_inserted = 0;
+	u8 *tmp;
+
+	if (!strlen(radio_text))
+		return -EINVAL;
+
+	tmp = kzalloc(MAX_RDS_RADIO_TEXT + 1, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	strncpy(tmp, radio_text, MAX_RDS_RADIO_TEXT);
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_rds_buff(sdev, RDS_BLOCK_CLEAR, 0, 0, 0,
+						&s, &a, &u, &fa, &fu);
+		if (rval < 0)
+			goto unlock;
+		do {
+			/* RDS spec says that if the last block isn't used,
+			 * then apply a carriage return
+			 */
+			if (t_index < (RDS_RADIOTEXT_INDEX_MAX * \
+				RDS_RADIOTEXT_BLK_SIZE)) {
+				for (i = 0; i < RDS_RADIOTEXT_BLK_SIZE; i++) {
+					if (!tmp[t_index + i] ||
+						tmp[t_index + i] == \
+						RDS_CARRIAGE_RETURN) {
+						tmp[t_index + i] =
+							RDS_CARRIAGE_RETURN;
+						cr_inserted = 1;
+						break;
+					}
+				}
+			}
+
+			rval = si4713_tx_rds_buff(sdev, RDS_BLOCK_LOAD,
+					compose_u16(RDS_RADIOTEXT_2A,
+						b_index++),
+					compose_u16(tmp[t_index],
+						tmp[t_index + 1]),
+					compose_u16(tmp[t_index + 2],
+						tmp[t_index + 3]),
+					&s, &a, &u, &fa, &fu);
+			if (rval < 0)
+				goto unlock;
+
+			t_index += RDS_RADIOTEXT_BLK_SIZE;
+
+			if (cr_inserted)
+				break;
+		} while (u < a);
+	}
+
+	strncpy(sdev->rds_info.radio_text, tmp, MAX_RDS_RADIO_TEXT);
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	kfree(tmp);
+	return rval;
+}
+
+int si4713_get_rds_radio_text(struct si4713_device *sdev, char *radio_text)
+{
+	mutex_lock(&sdev->mutex);
+	strncpy(radio_text, sdev->rds_info.radio_text, MAX_RDS_RADIO_TEXT);
+	mutex_unlock(&sdev->mutex);
+
+	return 0;
+}
+
+int si4713_set_rds_enabled(struct si4713_device *sdev, u8 enabled)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		p = rval;
+		if (enabled)
+			p = enable_rds(p);
+		else
+			p = disable_rds(p);
+
+		rval = si4713_write_property(sdev, SI4713_TX_COMPONENT_ENABLE,
+				p);
+		if (rval < 0)
+			goto unlock;
+
+		if (enabled) {
+			rval = si4713_write_property(sdev,
+				SI4713_TX_RDS_DEVIATION,
+				DEFAULT_RDS_DEVIATION);
+			if (rval < 0)
+				goto unlock;
+		}
+	}
+
+	sdev->rds_info.enabled = enabled & 0x01;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_rds_enabled(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		sdev->rds_info.enabled = get_rds_status(rval);
+	}
+
+	rval = sdev->rds_info.enabled;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_preemphasis(struct si4713_device *sdev, u8 preemphasis)
+{
+	int rval = 0;
+
+	switch (preemphasis) {
+	case PREEMPHASIS_USA:
+		preemphasis = FMPE_USA;
+		break;
+	case PREEMPHASIS_EU:
+		preemphasis = FMPE_EU;
+		break;
+	case PREEMPHASIS_DISABLED:
+		preemphasis = FMPE_DISABLED;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_PREEMPHASIS,
+								preemphasis);
+
+	if (rval >= 0)
+		sdev->preemphasis = preemphasis;
+
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_preemphasis(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_PREEMPHASIS);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->preemphasis = rval;
+	}
+
+	switch (sdev->preemphasis) {
+	case FMPE_USA:
+		rval = PREEMPHASIS_USA;
+		break;
+	case FMPE_EU:
+		rval = PREEMPHASIS_EU;
+		break;
+	case FMPE_DISABLED:
+		rval = PREEMPHASIS_DISABLED;
+		break;
+	default:
+		rval = -EINVAL;
+		goto unlock;
+	}
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_limiter_enabled(struct si4713_device *sdev, u8 enabled)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		p = rval;
+		if (enabled)
+			p = enable_limiter(p);
+		else
+			p = disable_limiter(p);
+
+		rval = si4713_write_property(sdev, SI4713_TX_ACOMP_ENABLE,
+				p);
+
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->limiter_info.enabled = enabled & 0x01;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_limiter_enabled(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		sdev->limiter_info.enabled = get_limiter_status(rval);
+	}
+
+	rval = sdev->limiter_info.enabled;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_limiter_deviation(struct si4713_device *sdev,
+					unsigned long deviation)
+{
+	int rval = 0;
+
+	/* Device receives in 10Hz units */
+	deviation /= 10;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_AUDIO_DEVIATION,
+						deviation);
+
+	/* Device returns in 10Hz units */
+	if (rval >= 0)
+		sdev->limiter_info.deviation = deviation * 10;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+long si4713_get_limiter_deviation(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_AUDIO_DEVIATION);
+
+		if (rval < 0)
+			goto unlock;
+
+		/* Device returns in 10Hz units */
+		sdev->limiter_info.deviation = rval * 10;
+	}
+
+	rval = sdev->limiter_info.deviation;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_limiter_release_time(struct si4713_device *sdev,
+					unsigned long rtime)
+{
+	int rval;
+
+	rval = usecs_to_dev(rtime, limiter_times, ARRAY_SIZE(limiter_times));
+	if (rval < 0)
+		goto exit;
+
+	rtime = rval;
+	rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev,
+				SI4713_TX_LIMITER_RELEASE_TIME,	rtime);
+
+	if (rval >= 0)
+		sdev->limiter_info.release_time = dev_to_usecs(rtime,
+						limiter_times,
+						ARRAY_SIZE(limiter_times));
+
+	mutex_unlock(&sdev->mutex);
+
+exit:
+	return rval;
+}
+
+long si4713_get_limiter_release_time(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev,
+				SI4713_TX_LIMITER_RELEASE_TIME);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->limiter_info.release_time = dev_to_usecs(rval,
+						limiter_times,
+						ARRAY_SIZE(limiter_times));
+	}
+
+	rval = sdev->limiter_info.release_time;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_stereo_enabled(struct si4713_device *sdev, u8 enabled)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		p = rval;
+		if (enabled)
+			p = enable_stereo(p);
+		else
+			p = disable_stereo(p);
+
+		rval = si4713_write_property(sdev, SI4713_TX_COMPONENT_ENABLE,
+				p);
+
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->stereo = enabled & 0x01;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_stereo_enabled(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		sdev->stereo = get_stereo_status(rval);
+	}
+
+	rval = sdev->stereo;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_pilot_enabled(struct si4713_device *sdev, u8 enabled)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		p = rval;
+		if (enabled)
+			p = enable_pilot(p);
+		else
+			p = disable_pilot(p);
+
+		rval = si4713_write_property(sdev, SI4713_TX_COMPONENT_ENABLE,
+				p);
+
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->pilot_info.enabled = enabled & 0x01;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_pilot_enabled(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_COMPONENT_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		sdev->pilot_info.enabled = get_pilot_status(rval);
+	}
+
+	rval = sdev->pilot_info.enabled;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_pilot_deviation(struct si4713_device *sdev,
+					unsigned long deviation)
+{
+	int rval = 0;
+
+	/* Device receives in 10Hz units */
+	deviation /= 10;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_PILOT_DEVIATION,
+						deviation);
+
+	/* Device returns in 10Hz units */
+	if (rval >= 0)
+		sdev->pilot_info.deviation = deviation * 10;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+long si4713_get_pilot_deviation(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_PILOT_DEVIATION);
+
+		if (rval < 0)
+			goto unlock;
+
+		/* Device returns in 10Hz units */
+		sdev->pilot_info.deviation = rval * 10;
+	}
+
+	rval = sdev->pilot_info.deviation;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_pilot_frequency(struct si4713_device *sdev, u16 freq)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_PILOT_FREQUENCY,
+						freq);
+
+	if (rval >= 0)
+		sdev->pilot_info.frequency = freq;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_pilot_frequency(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_PILOT_FREQUENCY);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->pilot_info.frequency = rval;
+	}
+
+	rval = sdev->pilot_info.frequency;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_acomp_enabled(struct si4713_device *sdev, u8 enabled)
+{
+	int rval = 0;
+	u16 p;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		p = rval;
+		if (enabled)
+			p = enable_acomp(p);
+		else
+			p = disable_acomp(p);
+
+		rval = si4713_write_property(sdev, SI4713_TX_ACOMP_ENABLE, p);
+
+		if (rval < 0)
+			goto unlock;
+	}
+
+	sdev->acomp_info.enabled = enabled & 0x01;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_get_acomp_enabled(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_ENABLE);
+		if (rval < 0)
+			goto unlock;
+
+		sdev->acomp_info.enabled = get_acomp_status(rval);
+	}
+
+	rval = sdev->acomp_info.enabled;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_acomp_gain(struct si4713_device *sdev, u8 gain)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_ACOMP_GAIN, gain);
+
+	if (rval >= 0)
+		sdev->acomp_info.gain = gain;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_acomp_gain(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_GAIN);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->acomp_info.gain = rval;
+	}
+
+	rval = sdev->acomp_info.gain;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_acomp_threshold(struct si4713_device *sdev, s8 threshold)
+{
+	int rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev, SI4713_TX_ACOMP_THRESHOLD,
+						threshold);
+
+	if (rval >= 0)
+		sdev->acomp_info.threshold = threshold;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_acomp_threshold(struct si4713_device *sdev, s8 *threshold)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev, SI4713_TX_ACOMP_THRESHOLD);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->acomp_info.threshold = rval;
+	}
+
+	*threshold = sdev->acomp_info.threshold;
+	rval = 0;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_acomp_release_time(struct si4713_device *sdev,
+					unsigned long rtime)
+{
+	int rval;
+
+	rval = usecs_to_dev(rtime, acomp_rtimes, ARRAY_SIZE(acomp_rtimes));
+	if (rval < 0)
+		goto exit;
+
+	rtime = rval;
+	rval = 0;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev,
+				SI4713_TX_ACOMP_RELEASE_TIME, rtime);
+
+	if (rval >= 0)
+		sdev->acomp_info.release_time = dev_to_usecs(rtime,
+						acomp_rtimes,
+						ARRAY_SIZE(acomp_rtimes));
+
+	mutex_unlock(&sdev->mutex);
+
+exit:
+	return rval;
+}
+
+long si4713_get_acomp_release_time(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev,
+				SI4713_TX_ACOMP_RELEASE_TIME);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->acomp_info.release_time = dev_to_usecs(rval,
+						acomp_rtimes,
+						ARRAY_SIZE(acomp_rtimes));
+	}
+
+	rval = sdev->acomp_info.release_time;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+
+int si4713_set_acomp_attack_time(struct si4713_device *sdev, u16 atime)
+{
+	int rval = 0;
+
+	/* Device receives in 0.5 ms units */
+	atime /= ATTACK_TIME_UNIT;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state)
+		rval = si4713_write_property(sdev,
+				SI4713_TX_ACOMP_ATTACK_TIME, atime);
+
+	if (rval >= 0)
+		sdev->acomp_info.attack_time = atime * ATTACK_TIME_UNIT;
+
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_acomp_attack_time(struct si4713_device *sdev)
+{
+	int rval;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_read_property(sdev,
+				SI4713_TX_ACOMP_RELEASE_TIME);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->acomp_info.release_time = rval * ATTACK_TIME_UNIT;
+	}
+
+	rval = sdev->acomp_info.attack_time;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
+
+int si4713_set_tune_measure(struct si4713_device *sdev, u32 frequency)
+{
+	int rval = -ENODEV;
+
+	mutex_lock(&sdev->mutex);
+	if (sdev->power_state)
+		rval = si4713_tx_tune_measure(sdev, frequency / 10, 0);
+	mutex_unlock(&sdev->mutex);
+
+	return rval;
+}
+
+int si4713_get_tune_measure(struct si4713_device *sdev)
+{
+	int rval;
+	u16 f = 0;
+	u8 p, a, n;
+
+	mutex_lock(&sdev->mutex);
+
+	if (sdev->power_state) {
+		rval = si4713_tx_tune_status(sdev, 0x00, &f, &p, &a, &n);
+
+		if (rval < 0)
+			goto unlock;
+
+		sdev->tune_rssi = n;
+	}
+
+	rval = sdev->tune_rssi;
+
+unlock:
+	mutex_unlock(&sdev->mutex);
+	return rval;
+}
diff -r 85b64a6cc67c -r 94b5043b692a linux/drivers/media/radio/si4713.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/radio/si4713.h	Wed May 27 11:56:46 2009 +0300
@@ -0,0 +1,282 @@
+/*
+ * drivers/media/radio/si4713.h
+ *
+ * Property and commands definitions for Si4713 radio transmitter chip.
+ *
+ * Copyright (c) 2008 Instituto Nokia de Tecnologia - INdT
+ * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ *
+ */
+
+#ifndef SI4713_H
+#define SI4713_H
+
+#include <media/v4l2-subdev.h>
+
+#include "radio-si4713.h"
+
+#define SI4713_PRODUCT_NUMBER		0x0D
+
+/* Command Timeouts */
+#define DEFAULT_TIMEOUT			500
+#define TIMEOUT_SET_PROPERTY		20
+#define TIMEOUT_TX_TUNE_POWER		30000
+#define TIMEOUT_TX_TUNE			110000
+#define TIMEOUT_POWER_UP		200000
+
+/*
+ * Command and its arguments definitions
+ */
+#define SI4713_PWUP_CTSIEN		(1<<7)
+#define SI4713_PWUP_GPO2OEN		(1<<6)
+#define SI4713_PWUP_PATCH		(1<<5)
+#define SI4713_PWUP_XOSCEN		(1<<4)
+#define SI4713_PWUP_FUNC_TX		0x02
+#define SI4713_PWUP_FUNC_PATCH		0x0F
+#define SI4713_PWUP_OPMOD_ANALOG	0x50
+#define SI4713_PWUP_OPMOD_DIGITAL	0x0F
+#define SI4713_PWUP_NARGS		2
+#define SI4713_PWUP_NRESP		1
+#define SI4713_CMD_POWER_UP		0x01
+
+#define SI4713_GETREV_NRESP		9
+#define SI4713_CMD_GET_REV		0x10
+
+#define SI4713_PWDN_NRESP		1
+#define SI4713_CMD_POWER_DOWN		0x11
+
+#define SI4713_SET_PROP_NARGS		5
+#define SI4713_SET_PROP_NRESP		1
+#define SI4713_CMD_SET_PROPERTY		0x12
+
+#define SI4713_GET_PROP_NARGS		3
+#define SI4713_GET_PROP_NRESP		4
+#define SI4713_CMD_GET_PROPERTY		0x13
+
+#define SI4713_GET_STATUS_NRESP		1
+#define SI4713_CMD_GET_INT_STATUS	0x14
+
+#define SI4713_CMD_PATCH_ARGS		0x15
+#define SI4713_CMD_PATCH_DATA		0x16
+
+#define SI4713_MAX_FREQ			10800
+#define SI4713_MIN_FREQ			7600
+#define SI4713_TXFREQ_NARGS		3
+#define SI4713_TXFREQ_NRESP		1
+#define SI4713_CMD_TX_TUNE_FREQ		0x30
+
+#define SI4713_MAX_POWER		120
+#define SI4713_MIN_POWER		88
+#define SI4713_MAX_ANTCAP		191
+#define SI4713_MIN_ANTCAP		0
+#define SI4713_TXPWR_NARGS		4
+#define SI4713_TXPWR_NRESP		1
+#define SI4713_CMD_TX_TUNE_POWER	0x31
+
+#define SI4713_TXMEA_NARGS		4
+#define SI4713_TXMEA_NRESP		1
+#define SI4713_CMD_TX_TUNE_MEASURE	0x32
+
+#define SI4713_INTACK_MASK		0x01
+#define SI4713_TXSTATUS_NARGS		1
+#define SI4713_TXSTATUS_NRESP		8
+#define SI4713_CMD_TX_TUNE_STATUS	0x33
+
+#define SI4713_OVERMOD_BIT		(1 << 2)
+#define SI4713_IALH_BIT			(1 << 1)
+#define SI4713_IALL_BIT			(1 << 0)
+#define SI4713_ASQSTATUS_NARGS		1
+#define SI4713_ASQSTATUS_NRESP		5
+#define SI4713_CMD_TX_ASQ_STATUS	0x34
+
+#define SI4713_RDSBUFF_MODE_MASK	0x87
+#define SI4713_RDSBUFF_NARGS		7
+#define SI4713_RDSBUFF_NRESP		6
+#define SI4713_CMD_TX_RDS_BUFF		0x35
+
+#define SI4713_RDSPS_PSID_MASK		0x1F
+#define SI4713_RDSPS_NARGS		5
+#define SI4713_RDSPS_NRESP		1
+#define SI4713_CMD_TX_RDS_PS		0x36
+
+#define SI4713_CMD_GPO_CTL		0x80
+#define SI4713_CMD_GPO_SET		0x81
+
+/*
+ * Bits from status response
+ */
+#define SI4713_CTS			(1<<7)
+#define SI4713_ERR			(1<<6)
+#define SI4713_RDS_INT			(1<<2)
+#define SI4713_ASQ_INT			(1<<1)
+#define SI4713_STC_INT			(1<<0)
+
+/*
+ * Property definitions
+ */
+#define SI4713_GPO_IEN			0x0001
+#define SI4713_DIG_INPUT_FORMAT		0x0101
+#define SI4713_DIG_INPUT_SAMPLE_RATE	0x0103
+#define SI4713_REFCLK_FREQ		0x0201
+#define SI4713_REFCLK_PRESCALE		0x0202
+#define SI4713_TX_COMPONENT_ENABLE	0x2100
+#define SI4713_TX_AUDIO_DEVIATION	0x2101
+#define SI4713_TX_PILOT_DEVIATION	0x2102
+#define SI4713_TX_RDS_DEVIATION		0x2103
+#define SI4713_TX_LINE_INPUT_LEVEL	0x2104
+#define SI4713_TX_LINE_INPUT_MUTE	0x2105
+#define SI4713_TX_PREEMPHASIS		0x2106
+#define SI4713_TX_PILOT_FREQUENCY	0x2107
+#define SI4713_TX_ACOMP_ENABLE		0x2200
+#define SI4713_TX_ACOMP_THRESHOLD	0x2201
+#define SI4713_TX_ACOMP_ATTACK_TIME	0x2202
+#define SI4713_TX_ACOMP_RELEASE_TIME	0x2203
+#define SI4713_TX_ACOMP_GAIN		0x2204
+#define SI4713_TX_LIMITER_RELEASE_TIME	0x2205
+#define SI4713_TX_ASQ_INTERRUPT_SOURCE	0x2300
+#define SI4713_TX_ASQ_LEVEL_LOW		0x2301
+#define SI4713_TX_ASQ_DURATION_LOW	0x2302
+#define SI4713_TX_ASQ_LEVEL_HIGH	0x2303
+#define SI4713_TX_ASQ_DURATION_HIGH	0x2304
+#define SI4713_TX_RDS_INTERRUPT_SOURCE	0x2C00
+#define SI4713_TX_RDS_PI		0x2C01
+#define SI4713_TX_RDS_PS_MIX		0x2C02
+#define SI4713_TX_RDS_PS_MISC		0x2C03
+#define SI4713_TX_RDS_PS_REPEAT_COUNT	0x2C04
+#define SI4713_TX_RDS_PS_MESSAGE_COUNT	0x2C05
+#define SI4713_TX_RDS_PS_AF		0x2C06
+#define SI4713_TX_RDS_FIFO_SIZE		0x2C07
+
+#define PREEMPHASIS_USA			75
+#define PREEMPHASIS_EU			50
+#define PREEMPHASIS_DISABLED		0
+#define FMPE_USA			0x00
+#define FMPE_EU				0x01
+#define FMPE_DISABLED			0x02
+
+#define POWER_UP			0x01
+#define POWER_DOWN			0x00
+
+struct rds_info {
+	u16 pi;
+#define MAX_RDS_PTY			31
+	u8 pty;
+#define MAX_RDS_PS_NAME			96
+	u8 ps_name[MAX_RDS_PS_NAME + 1];
+#define MAX_RDS_RADIO_TEXT		384
+	u8 radio_text[MAX_RDS_RADIO_TEXT + 1];
+	u8 enabled;
+};
+
+struct limiter_info {
+#define MAX_LIMITER_RELEASE_TIME	102390
+	unsigned long release_time;
+#define MAX_LIMITER_DEVIATION		90000
+	unsigned long deviation;
+	u8 enabled;
+};
+
+struct pilot_info {
+#define MAX_PILOT_DEVIATION		90000
+	unsigned long deviation;
+#define MAX_PILOT_FREQUENCY		19000
+	u16 frequency;
+	u8 enabled;
+};
+
+struct acomp_info {
+#define MAX_ACOMP_RELEASE_TIME		1000000
+	unsigned long release_time;
+#define MAX_ACOMP_ATTACK_TIME		5000
+	u16 attack_time;
+#define MAX_ACOMP_THRESHOLD		0
+#define MIN_ACOMP_THRESHOLD		(-40)
+	s8 threshold;
+#define MAX_ACOMP_GAIN			20
+	u8 gain;
+	u8 enabled;
+};
+
+/*
+ * si4713_device - private data
+ */
+struct si4713_device {
+	/* v4l2_subdev and i2c reference (v4l2_subdev priv data) */
+	struct v4l2_subdev sd;
+	/* private data structures */
+	struct mutex mutex;
+	struct completion work;
+	struct si4713_platform_data *platform_data;
+	struct rds_info rds_info;
+	struct limiter_info limiter_info;
+	struct pilot_info pilot_info;
+	struct acomp_info acomp_info;
+	u16 frequency;
+	u8 preemphasis;
+	u8 mute;
+	u8 power_level;
+	u8 power_state;
+	u8 antenna_capacitor;
+	u8 stereo;
+	u8 tune_rssi;
+};
+
+int si4713_init(struct si4713_device *sdev);
+int si4713_setup(struct si4713_device *sdev);
+int si4713_probe(struct si4713_device *sdev);
+int si4713_set_power_level(struct si4713_device *sdev, u8 power_level);
+int si4713_get_power_level(struct si4713_device *sdev);
+int si4713_set_antenna_capacitor(struct si4713_device *sdev, u8 value);
+int si4713_get_antenna_capacitor(struct si4713_device *sdev);
+int si4713_set_power_state(struct si4713_device *sdev, u8 value);
+int si4713_set_frequency(struct si4713_device *sdev, u16 frequency);
+int si4713_get_frequency(struct si4713_device *sdev);
+int si4713_set_mute(struct si4713_device *sdev, u16 mute);
+int si4713_get_mute(struct si4713_device *sdev);
+int si4713_set_rds_pi(struct si4713_device *sdev, u16 pi);
+int si4713_get_rds_pi(struct si4713_device *sdev);
+int si4713_set_rds_pty(struct si4713_device *sdev, u8 pty);
+int si4713_get_rds_pty(struct si4713_device *sdev);
+int si4713_set_rds_ps_name(struct si4713_device *sdev, char *ps_name);
+int si4713_get_rds_ps_name(struct si4713_device *sdev, char *ps_name);
+int si4713_set_rds_radio_text(struct si4713_device *sdev, char *radio_text);
+int si4713_get_rds_radio_text(struct si4713_device *sdev, char *radio_text);
+int si4713_set_rds_enabled(struct si4713_device *sdev, u8 enabled);
+int si4713_get_rds_enabled(struct si4713_device *sdev);
+int si4713_set_limiter_release_time(struct si4713_device *sdev,
+					unsigned long rtime);
+long si4713_get_limiter_release_time(struct si4713_device *sdev);
+int si4713_set_limiter_deviation(struct si4713_device *sdev,
+					unsigned long deviation);
+long si4713_get_limiter_deviation(struct si4713_device *sdev);
+int si4713_set_limiter_enabled(struct si4713_device *sdev, u8 enabled);
+int si4713_get_limiter_enabled(struct si4713_device *sdev);
+int si4713_set_pilot_frequency(struct si4713_device *sdev, u16 freq);
+int si4713_get_pilot_frequency(struct si4713_device *sdev);
+int si4713_set_pilot_deviation(struct si4713_device *sdev,
+					unsigned long deviation);
+long si4713_get_pilot_deviation(struct si4713_device *sdev);
+int si4713_set_pilot_enabled(struct si4713_device *sdev, u8 enabled);
+int si4713_get_pilot_enabled(struct si4713_device *sdev);
+int si4713_set_stereo_enabled(struct si4713_device *sdev, u8 enabled);
+int si4713_get_stereo_enabled(struct si4713_device *sdev);
+int si4713_set_acomp_enabled(struct si4713_device *sdev, u8 enabled);
+int si4713_get_acomp_enabled(struct si4713_device *sdev);
+int si4713_set_acomp_threshold(struct si4713_device *sdev, s8 threshold);
+int si4713_get_acomp_threshold(struct si4713_device *sdev, s8 *threshold);
+int si4713_set_acomp_gain(struct si4713_device *sdev, u8 gain);
+int si4713_get_acomp_gain(struct si4713_device *sdev);
+int si4713_set_acomp_release_time(struct si4713_device *sdev,
+					unsigned long rtime);
+long si4713_get_acomp_release_time(struct si4713_device *sdev);
+int si4713_set_acomp_attack_time(struct si4713_device *sdev, u16 atime);
+int si4713_get_acomp_attack_time(struct si4713_device *sdev);
+int si4713_set_preemphasis(struct si4713_device *sdev, u8 preemphasis);
+int si4713_get_preemphasis(struct si4713_device *sdev);
+int si4713_set_tune_measure(struct si4713_device *sdev, u32 frequency);
+int si4713_get_tune_measure(struct si4713_device *sdev);
+#endif /* ifndef SI4713_H */

[PATCHv5 6 of 8] FMTx: si4713: Add files to add radio interface for si4713

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414606 -10800
# Branch export
# Node ID 1abb96fdce05e1449faac2223e93056bacf389bd
# Parent  94b5043b692a6218a667326536d3d76a0c591307
This patch adds files which creates the radio interface
for si4713 FM transmitter devices.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/radio/radio-si4713.c |  333 ++++++++++++++++++++++++++++++++++++
 drivers/media/radio/radio-si4713.h |   49 ++++++
 2 files changed, 382 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/radio/radio-si4713.c
 create mode 100644 drivers/media/radio/radio-si4713.h

diff -r 94b5043b692a -r 1abb96fdce05 linux/drivers/media/radio/radio-si4713.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/radio/radio-si4713.c	Wed May 27 11:56:46 2009 +0300
@@ -0,0 +1,333 @@
+/*
+ * drivers/media/radio/radio-si4713.c
+ *
+ * Platform Driver for Silicon Labs Si4713 FM Radio Transmitter:
+ *
+ * Copyright (c) 2008 Instituto Nokia de Tecnologia - INdT
+ * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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 <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/version.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-ioctl.h>
+
+#include "radio-si4713.h"
+#include "si4713.h"
+
+/* module parameters */
+static int radio_nr = -1;	/* radio device minor (-1 ==> auto assign) */
+
+/* radio_si4713_fops - file operations interface */
+static const struct v4l2_file_operations radio_si4713_fops = {
+	.owner		= THIS_MODULE,
+	.ioctl		= video_ioctl2,
+};
+
+/* Video4Linux Interface */
+static int radio_si4713_vidioc_g_audio(struct file *file, void *priv,
+					struct v4l2_audio *audio)
+{
+	if (audio->index > 1)
+		return -EINVAL;
+
+	strncpy(audio->name, "Radio", 32);
+	audio->capability = V4L2_AUDCAP_STEREO;
+
+	return 0;
+}
+
+static int radio_si4713_vidioc_s_audio(struct file *file, void *priv,
+					struct v4l2_audio *audio)
+{
+	if (audio->index != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int radio_si4713_vidioc_g_input(struct file *file, void *priv,
+					unsigned int *i)
+{
+	*i = 0;
+
+	return 0;
+}
+
+static int radio_si4713_vidioc_s_input(struct file *file, void *priv,
+					unsigned int i)
+{
+	if (i != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+/* radio_si4713_vidioc_querycap - query device capabilities */
+static int radio_si4713_vidioc_querycap(struct file *file, void *priv,
+					struct v4l2_capability *capability)
+{
+	struct radio_si4713_device *rsdev;
+
+	rsdev = video_get_drvdata(video_devdata(file));
+
+	strlcpy(capability->driver, "radio-si4713", sizeof(capability->driver));
+	strlcpy(capability->card, "Silicon Labs Si4713 FM Radio Transmitter",
+				sizeof(capability->card));
+	capability->capabilities = V4L2_CAP_TUNER;
+
+	return 0;
+}
+
+/* radio_si4713_vidioc_queryctrl - enumerate control items */
+static int radio_si4713_vidioc_queryctrl(struct file *file, void *priv,
+						struct v4l2_queryctrl *qc)
+{
+
+	/* Must be sorted from low to high control ID! */
+	static const u32 user_ctrls[] = {
+		V4L2_CID_USER_CLASS,
+		V4L2_CID_AUDIO_VOLUME,
+		V4L2_CID_AUDIO_BALANCE,
+		V4L2_CID_AUDIO_BASS,
+		V4L2_CID_AUDIO_TREBLE,
+		V4L2_CID_AUDIO_LOUDNESS,
+		V4L2_CID_AUDIO_MUTE,
+		0
+	};
+
+	/* Must be sorted from low to high control ID! */
+	static const u32 fmtx_ctrls[] = {
+		V4L2_CID_FMTX_CLASS,
+		V4L2_CID_RDS_ENABLED,
+		V4L2_CID_RDS_PI,
+		V4L2_CID_RDS_PTY,
+		V4L2_CID_RDS_PS_NAME,
+		V4L2_CID_RDS_RADIO_TEXT,
+		V4L2_CID_AUDIO_LIMITER_ENABLED,
+		V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
+		V4L2_CID_AUDIO_LIMITER_DEVIATION,
+		V4L2_CID_AUDIO_COMPRESSION_ENABLED,
+		V4L2_CID_AUDIO_COMPRESSION_GAIN,
+		V4L2_CID_AUDIO_COMPRESSION_THRESHOLD,
+		V4L2_CID_AUDIO_COMPRESSION_ATTACK_TIME,
+		V4L2_CID_AUDIO_COMPRESSION_RELEASE_TIME,
+		V4L2_CID_PILOT_TONE_ENABLED,
+		V4L2_CID_PILOT_TONE_DEVIATION,
+		V4L2_CID_PILOT_TONE_FREQUENCY,
+		V4L2_CID_PREEMPHASIS,
+		V4L2_CID_TUNE_POWER_LEVEL,
+		V4L2_CID_TUNE_ANTENNA_CAPACITOR,
+		0
+	};
+	static const u32 *ctrl_classes[] = {
+		user_ctrls,
+		fmtx_ctrls,
+		NULL
+	};
+	struct radio_si4713_device *rsdev;
+
+	rsdev = video_get_drvdata(video_devdata(file));
+
+	qc->id = v4l2_ctrl_next(ctrl_classes, qc->id);
+	if (qc->id == 0)
+		return -EINVAL;
+
+	if (qc->id == V4L2_CID_USER_CLASS || qc->id == V4L2_CID_FMTX_CLASS)
+		return v4l2_ctrl_query_fill(qc, 0, 0, 0, 0);
+
+	return v4l2_device_call_until_err(&rsdev->v4l2_dev, 0, core,
+						queryctrl, qc);
+}
+
+
+/*
+ * radio_si4713_vidioc_template - Produce a v4l2 vidioc call back.
+ * Can be used because we are just a wrapper for v4l2_sub_devs.
+ */
+#define radio_si4713_vidioc_template(type, callback, arg_type)		\
+static int radio_si4713_vidioc_##callback(struct file *file, void *p,	\
+						arg_type a)		\
+{									\
+	struct radio_si4713_device *rsdev;				\
+									\
+	rsdev = video_get_drvdata(video_devdata(file));			\
+									\
+	return v4l2_device_call_until_err(&rsdev->v4l2_dev, 0, type,	\
+							callback, a);	\
+}
+
+radio_si4713_vidioc_template(core, g_ext_ctrls, struct v4l2_ext_controls *)
+radio_si4713_vidioc_template(core, s_ext_ctrls, struct v4l2_ext_controls *)
+radio_si4713_vidioc_template(core, g_ctrl, struct v4l2_control *)
+radio_si4713_vidioc_template(core, s_ctrl, struct v4l2_control *)
+radio_si4713_vidioc_template(tuner, g_tuner, struct v4l2_tuner *)
+radio_si4713_vidioc_template(tuner, s_tuner, struct v4l2_tuner *)
+radio_si4713_vidioc_template(tuner, g_frequency, struct v4l2_frequency *)
+radio_si4713_vidioc_template(tuner, s_frequency, struct v4l2_frequency *)
+
+static struct v4l2_ioctl_ops radio_si4713_ioctl_ops = {
+	.vidioc_g_input		= radio_si4713_vidioc_g_input,
+	.vidioc_s_input		= radio_si4713_vidioc_s_input,
+	.vidioc_g_audio		= radio_si4713_vidioc_g_audio,
+	.vidioc_s_audio		= radio_si4713_vidioc_s_audio,
+	.vidioc_querycap	= radio_si4713_vidioc_querycap,
+	.vidioc_queryctrl	= radio_si4713_vidioc_queryctrl,
+	.vidioc_g_ext_ctrls	= radio_si4713_vidioc_g_ext_ctrls,
+	.vidioc_s_ext_ctrls	= radio_si4713_vidioc_s_ext_ctrls,
+	.vidioc_g_ctrl		= radio_si4713_vidioc_g_ctrl,
+	.vidioc_s_ctrl		= radio_si4713_vidioc_s_ctrl,
+	.vidioc_g_tuner		= radio_si4713_vidioc_g_tuner,
+	.vidioc_s_tuner		= radio_si4713_vidioc_s_tuner,
+	.vidioc_g_frequency	= radio_si4713_vidioc_g_frequency,
+	.vidioc_s_frequency	= radio_si4713_vidioc_s_frequency,
+};
+
+/* radio_si4713_vdev_template - video device interface */
+static struct video_device radio_si4713_vdev_template = {
+	.fops			= &radio_si4713_fops,
+	.name			= "radio-si4713",
+	.release		= video_device_release,
+	.ioctl_ops		= &radio_si4713_ioctl_ops,
+};
+
+/* Platform driver interface */
+/* radio_si4713_pdriver_probe - probe for the device */
+static int radio_si4713_pdriver_probe(struct platform_device *pdev)
+{
+	struct radio_si4713_platform_data *pdata = pdev->platform_data;
+	struct radio_si4713_device *rsdev;
+	struct i2c_adapter *adapter;
+	struct v4l2_subdev *sd;
+	int rval = 0;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "Can not proceed without platform data.\n");
+		rval = -EINVAL;
+		goto exit;
+	}
+
+	rsdev = kzalloc(sizeof *rsdev, GFP_KERNEL);
+	if (!rsdev) {
+		dev_err(&pdev->dev, "Failed to alloc video device.\n");
+		rval = -ENOMEM;
+		goto exit;
+	}
+
+	rval = v4l2_device_register(&pdev->dev, &rsdev->v4l2_dev);
+	if (rval) {
+		dev_err(&pdev->dev, "Failed to register v4l2 device.\n");
+		goto free_rsdev;
+	}
+
+	adapter = i2c_get_adapter(pdata->i2c_bus);
+	if (!adapter) {
+		dev_err(&pdev->dev, "Can not get i2c adapter %d\n",
+							pdata->i2c_bus);
+		rval = -ENODEV;
+		goto unregister_v4l2_dev;
+	}
+
+	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter, "si4713_i2c",
+				"si4713", SI4713_I2C_ADDR_BUSEN_HIGH,
+				pdata->irq, pdata->subdev_pdata);
+	if (!sd) {
+		dev_err(&pdev->dev, "Can not get v4l2 subdevice\n");
+		rval = -ENODEV;
+		goto unregister_v4l2_dev;
+	}
+
+	rsdev->radio_dev = video_device_alloc();
+	if (!rsdev->radio_dev) {
+		dev_err(&pdev->dev, "Failed to alloc video device.\n");
+		rval = -ENOMEM;
+		goto unregister_v4l2_dev;
+	}
+
+	memcpy(rsdev->radio_dev, &radio_si4713_vdev_template,
+			sizeof(radio_si4713_vdev_template));
+	video_set_drvdata(rsdev->radio_dev, rsdev);
+	if (video_register_device(rsdev->radio_dev, VFL_TYPE_RADIO, radio_nr)) {
+		dev_err(&pdev->dev, "Could not register video device.\n");
+		rval = -EIO;
+		goto free_vdev;
+	}
+	dev_info(&pdev->dev, "New device successfully probed\n");
+
+	goto exit;
+
+free_vdev:
+	video_device_release(rsdev->radio_dev);
+unregister_v4l2_dev:
+	v4l2_device_unregister(&rsdev->v4l2_dev);
+free_rsdev:
+	kfree(rsdev);
+exit:
+	return rval;
+}
+
+/* radio_si4713_pdriver_remove - remove the device */
+static int __exit radio_si4713_pdriver_remove(struct platform_device *pdev)
+{
+	struct v4l2_device *v4l2_dev = platform_get_drvdata(pdev);
+	struct radio_si4713_device *rsdev = container_of(v4l2_dev,
+						struct radio_si4713_device,
+						v4l2_dev);
+
+	video_unregister_device(rsdev->radio_dev);
+	v4l2_device_unregister(&rsdev->v4l2_dev);
+	kfree(rsdev);
+
+	return 0;
+}
+
+static struct platform_driver radio_si4713_pdriver = {
+	.driver		= {
+		.name	= "radio-si4713",
+	},
+	.probe		= radio_si4713_pdriver_probe,
+	.remove         = __exit_p(radio_si4713_pdriver_remove),
+};
+
+/* Module Interface */
+static int __init radio_si4713_module_init(void)
+{
+	return platform_driver_register(&radio_si4713_pdriver);
+}
+
+static void __exit radio_si4713_module_exit(void)
+{
+	platform_driver_unregister(&radio_si4713_pdriver);
+}
+
+module_init(radio_si4713_module_init);
+module_exit(radio_si4713_module_exit);
+
+module_param(radio_nr, int, 0);
+MODULE_PARM_DESC(radio_nr,
+		 "Minor number for radio device (-1 ==> auto assign)");
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eduardo Valentin <eduardo.valentin@nokia.com>");
+MODULE_DESCRIPTION("Platform driver for Si4713 FM Radio Transmitter");
+MODULE_VERSION("0.0.1");
diff -r 94b5043b692a -r 1abb96fdce05 linux/drivers/media/radio/radio-si4713.h
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/radio/radio-si4713.h	Wed May 27 11:56:46 2009 +0300
@@ -0,0 +1,49 @@
+/*
+ * drivers/media/radio/radio-si4713.h
+ *
+ * Property and commands definitions for Si4713 radio transmitter chip.
+ *
+ * Copyright (c) 2008 Instituto Nokia de Tecnologia - INdT
+ * Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ *
+ */
+
+#ifndef RADIO_SI4713_H
+#define RADIO_SI4713_H
+
+#include <linux/i2c.h>
+#include <media/v4l2-device.h>
+
+#define SI4713_NAME "radio-si4713"
+
+/* The SI4713 I2C sensor chip has a fixed slave address of 0xc6. */
+#define SI4713_I2C_ADDR_BUSEN_HIGH	0x63
+#define SI4713_I2C_ADDR_BUSEN_LOW	0x11
+
+/*
+ * Platform dependent definition
+ */
+struct si4713_platform_data {
+	/* Set power state, zero is off, non-zero is on. */
+	int (*set_power)(int power);
+};
+
+/*
+ * Platform driver device state struct
+ */
+struct radio_si4713_device {
+	struct v4l2_device		v4l2_dev;
+	struct video_device		*radio_dev;
+};
+
+struct radio_si4713_platform_data {
+	int i2c_bus;
+	int irq;
+	void *subdev_pdata;
+};
+
+#endif /* ifndef RADIO_SI4713_H*/

[PATCHv5 7 of 8] FMTx: si4713: Add Kconfig and Makefile entries

[Eduardo Valentin]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414607 -10800
# Branch export
# Node ID b1d98e675a3c4e9e6d247701c9ac18239e3dcc1c
# Parent  1abb96fdce05e1449faac2223e93056bacf389bd
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 drivers/media/radio/Kconfig  |   22 ++++++++++++++++++++++
 drivers/media/radio/Makefile |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff -r 1abb96fdce05 -r b1d98e675a3c linux/drivers/media/radio/Kconfig
--- a/linux/drivers/media/radio/Kconfig	Wed May 27 11:56:46 2009 +0300
+++ b/linux/drivers/media/radio/Kconfig	Wed May 27 11:56:47 2009 +0300
@@ -339,6 +339,28 @@
 	help
 	  Enter the I/O port of your Zoltrix radio card.
 
+config I2C_SI4713
+	tristate "I2C driver for Silicon Labs Si4713 device"
+	depends on I2C && VIDEO_V4L2
+	---help---
+	  Say Y here if you want support to Si4713 I2C device.
+	  This device driver supports only i2c bus.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called si4713.
+
+config RADIO_SI4713
+	tristate "Silicon Labs Si4713 FM Radio Transmitter support"
+	depends on I2C && VIDEO_V4L2
+	---help---
+	  Say Y here if you want support to Si4713 FM Radio Transmitter.
+	  This device can transmit audio through FM. It can transmit
+	  EDS and EBDS signals as well. This module is the v4l2 radio
+	  interface for the i2c driver of this device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called radio-si4713.
+
 config USB_DSBR
 	tristate "D-Link/GemTek USB FM radio support"
 	depends on USB && VIDEO_V4L2
diff -r 1abb96fdce05 -r b1d98e675a3c linux/drivers/media/radio/Makefile
--- a/linux/drivers/media/radio/Makefile	Wed May 27 11:56:46 2009 +0300
+++ b/linux/drivers/media/radio/Makefile	Wed May 27 11:56:47 2009 +0300
@@ -15,6 +15,9 @@
 obj-$(CONFIG_RADIO_GEMTEK) += radio-gemtek.o
 obj-$(CONFIG_RADIO_GEMTEK_PCI) += radio-gemtek-pci.o
 obj-$(CONFIG_RADIO_TRUST) += radio-trust.o
+obj-$(CONFIG_I2C_SI4713) += si4713-i2c.o
+si4713-i2c-objs := si4713.o si4713-subdev.o
+obj-$(CONFIG_RADIO_SI4713) += radio-si4713.o
 obj-$(CONFIG_RADIO_MAESTRO) += radio-maestro.o
 obj-$(CONFIG_USB_DSBR) += dsbr100.o
 obj-$(CONFIG_USB_SI470X) += radio-si470x.o

[PATCHv5 8 of 8] FMTx: si4713: Add document file

[Eduardo Valentin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6146 bytes --]

# HG changeset patch
# User Eduardo Valentin <eduardo.valentin@nokia.com>
# Date 1243414607 -10800
# Branch export
# Node ID fadf1cddf504609cdb4889f4aa3305ca8d15323a
# Parent  b1d98e675a3c4e9e6d247701c9ac18239e3dcc1c
Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 Documentation/video4linux/si4713.txt |  133 ++++++++++++++++++++++++++++++++++
 1 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/video4linux/si4713.txt

diff -r b1d98e675a3c -r fadf1cddf504 linux/Documentation/video4linux/si4713.txt
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/Documentation/video4linux/si4713.txt	Wed May 27 11:56:47 2009 +0300
@@ -0,0 +1,133 @@
+Driver for I2C radios for the Silicon Labs Si4713 FM Radio Transmitters
+
+Copyright (c) 2009 Nokia Corporation
+Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+
+
+Information about the Device
+============================
+This chip is a Silicon Labs product. It is a I2C device, currently on 0×63 address.
+Basically, it has transmission and signal noise level measurement features.
+
+The Si4713 integrates transmit functions for FM broadcast stereo transmission.
+The chip also allows integrated receive power scanning to identify low signal
+power FM channels.
+
+The chip is programmed using commands and responses. There are also several
+properties which can change the behavior of this chip.
+
+Users must comply with local regulations on radio frequency (RF) transmission.
+
+Device driver description
+=========================
+There are two modules to handle this device. One is a I2C device driver
+and the other is a platform driver.
+
+The I2C device driver exports a v4l2-subdev interface to the kernel. Also
+it exports several device properties through sysfs interface to the user land.
+All properties can also be accessed by v4l2 extended controls interface, by
+using the v4l2-subdev calls (g_ext_ctrls, s_ext_ctrls).
+
+The platform device driver exports a v4l2 radio device interface to user land.
+So, it uses the I2C device driver as a sub device in order to send the user
+commands to the actual device. Basically it is a wrapper to the I2C device driver.
+
+So, in summary, the device driver has two interfaces to the user space.
+
+Applications can use v4l2 radio API to specify frequency of operation, mute state,
+etc. But mostly of its properties will be present in the extended controls.
+However, the device properties can also be accessed through its sysfs directory.
+
+When the v4l2 mute property is set to 1 (true), the driver will turn the chip off.
+
+Properties description
+======================
+
+The properties can be accessed in sysfs device directory. Using v4l2 extended
+controls as well.
+
+# ls
+acomp_attack_time        modalias                 rds_radio_text
+acomp_enabled            name                     region
+acomp_gain               pilot_deviation          region_bottom_frequency
+acomp_release_time       pilot_enabled            region_channel_spacing
+acomp_threshold          pilot_frequency          region_preemphasis
+antenna_capacitor        power                    region_top_frequency
+bus                      power_level              stereo_enabled
+driver                   rds_enabled              subsystem
+limiter_deviation        rds_pi                   tune_measure
+limiter_enabled          rds_ps_name              uevent
+limiter_release_time     rds_pty
+
+Here is a summary of them:
+
+* Pilot is an audible tone sent by the device.
+
+pilot_frequency - Configures the frequency of the stereo pilot tone.
+pilot_deviation - Configures pilot tone frequency deviation level.
+pilot_enabled - Enables or disables the pilot tone feature.
+
+* The si4713 device is capable of applying audio compression to the transmitted signal.
+
+acomp_enabled - Enables or disables the audio dynamic range control feature.
+acomp_gain - Sets the gain for audio dynamic range control.
+acomp_threshold - Sets the threshold level for audio dynamic range control.
+acomp_attack_time - Sets the attack time for audio dynamic range control.
+acomp_release_time - Sets the release time for audio dynamic range control.
+
+* Limiter setups audio deviation limiter feature. Once a over deviation occurs,
+it is possible to adjust the front-end gain of the audio input and always
+prevent over deviation.
+
+limiter_enabled - Enables or disables the limiter feature.
+limiter_deviation - Configures audio frequency deviation level.
+limiter_release_time - Sets the limiter release time.
+
+* Tuning power
+
+power_level - Sets the output power level for signal transmission.
+antenna_capacitor - This selects the value of antenna tuning capacitor manually
+or automatically if set to zero.
+tune_measure - With this you can get the value of signal length of a specific frequency.
+
+* RDS related
+
+rds_enabled - Enables or disables the RDS feature.
+rds_ps_name - Sets the RDS ps name field for transmission.
+rds_radio_text - Sets the RDS radio text for transmission.
+rds_pi - Sets the RDS PI field for transmission.
+rds_pty - Sets the RDS PTY field for transmission.
+
+* Region related
+
+Setting region will affect other region properties.
+
+region_bottom_frequency
+region_channel_spacing
+region_preemphasis
+region_top_frequency
+region - Selects which country specific setting should be assumed.
+
+* stereo_enabled - Enables or disables stereo mode.
+
+Testing
+=======
+Testing is usually done with fmtools utility for managing FM tuner cards.
+The tool can be found under Debian/Testing packages.
+
+The basic command list is:
+
+$ fm on     # Sets mute = false
+$ fm off    # Sets mute = true
+$ fm <freq> # Tunes to the frequency <freq>
+
+Of course, you should have the audio working and play something through alsa
+API to get something different of mute transmitted.
+
+To play with the above described properties, you can just use 'echo' and
+'cat' commands. For example, changing the rds_ps_name property, you just do:
+
+echo "Dummy FM Station" > /sys/bus/i2c/devices/X-0063/rds_ps_name
+
+where "X" is the i2c bus id which the device is connected.
+

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> # HG changeset patch
> # User Eduardo Valentin <eduardo.valentin@nokia.com>
> # Date 1243414605 -10800
> # Branch export
> # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> # Parent  142fd6020df3b4d543068155e49a2618140efa49
> Device drivers of v4l2_subdev devices may want to have
> board specific data. This patch adds an helper function
> to allow bridge drivers to pass board specific data to
> v4l2_subdev drivers.
>
> For those drivers which need to support kernel versions
> bellow 2.6.26, a .s_config callback was added. The
> idea of this callback is to pass board configuration
> as well. In that case, subdev driver should set .s_config
> properly, because v4l2_i2c_new_subdev_board will call
> the .s_config directly. Of course, if we are >= 2.6.26,
> the same data will be passed through i2c board info as well.

Hi Eduardo,

I finally had some time to look at this. After some thought I realized that 
the main problem is really that the API is becoming quite messy. Basically 
there are 9 different ways of loading and initializing a subdev:

First there are three basic initialization calls: no initialization, passing 
irq and platform_data, and passing the i2c_board_info struct directly 
(preferred for drivers that don't need pre-2.6.26 compatibility).

And for each flavor you would like to see three different versions as well: 
one with a fixed known i2c address, one where you probe for a list of 
addresses, and one where you can probe for a single i2c address.

I propose to change the API as follows:

#define V4L2_I2C_ADDRS(addr, addrs...) \
	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })

struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter,
                const char *module_name, const char *client_type,
		u8 addr, const unsigned short *addrs);

struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter,
                const char *module_name, const char *client_type,
                int irq, void *platform_data,
                u8 addr, const unsigned short *addrs);

/* Only available for kernels >= 2.6.26 */
struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter, const char *module_name,
                struct i2c_board_info *info, const unsigned short *addrs);

If you use a fixed address, then only set addr (or info.addr) and set addrs 
to NULL. If you want to probe for a list of addrs, then set addrs to the 
list of addrs. If you want to probe for a single addr, then use 
V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array with 
just two entries. Actually, this macro can also create arrays with more 
entries.

Note that v4l2_i2c_new_subdev will be an inline that calls 
v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.

And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline calling 
v4l2_i2c_new_subdev_board.

This approach reduces the number of functions to just one (not counting the 
inlines) and simplifies things all around. It does mean that all sources 
need to be changed, but if we go this route, then now is the time before 
the 2.6.31 window is closed. And I would also like to remove the '_new' 
from these function names. I never thought it added anything useful.

Comments? If we decide to go this way, then I need to know soon so that I 
can make the changes before the 2.6.31 window closes.

BTW, if the new s_config subdev call is present, then it should always be 
called. That way the subdev driver can safely do all of its initialization 
in s_config, no matter how it was initialized.

Sorry about the long delay in replying to this: it's been very hectic lately 
at the expense of my v4l-dvb work.

Regards,

	Hans

>
> Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
> ---
>  drivers/media/video/v4l2-common.c |   37
> +++++++++++++++++++++++++++++++++++-- include/linux/v4l2-common.h       |
>    8 ++++++++
>  include/linux/v4l2-subdev.h       |    1 +
>  3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff -r 142fd6020df3 -r 4fb354645426
> linux/drivers/media/video/v4l2-common.c ---
> a/linux/drivers/media/video/v4l2-common.c	Mon May 18 02:31:55 2009 +0000
> +++ b/linux/drivers/media/video/v4l2-common.c	Wed May 27 11:56:45 2009
> +0300 @@ -819,9 +819,10 @@
>
>
>  /* Load an i2c sub-device. */
> -struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
> +static struct v4l2_subdev *__v4l2_i2c_new_subdev(struct v4l2_device
> *v4l2_dev, struct i2c_adapter *adapter,
> -		const char *module_name, const char *client_type, u8 addr)
> +		const char *module_name, const char *client_type, u8 addr,
> +		int irq, void *platform_data)
>  {
>  	struct v4l2_subdev *sd = NULL;
>  	struct i2c_client *client;
> @@ -840,6 +841,8 @@
>  	memset(&info, 0, sizeof(info));
>  	strlcpy(info.type, client_type, sizeof(info.type));
>  	info.addr = addr;
> +	info.irq = irq;
> +	info.platform_data = platform_data;
>
>  	/* Create the i2c client */
>  	client = i2c_new_device(adapter, &info);
> @@ -877,8 +880,38 @@
>  #endif
>  	return sd;
>  }
> +
> +struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
> +		struct i2c_adapter *adapter,
> +		const char *module_name, const char *client_type, u8 addr)
> +{
> +	return __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name,
> +		client_type, addr, 0, NULL);
> +}
>  EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
>
> +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> *v4l2_dev, +		struct i2c_adapter *adapter,
> +		const char *module_name, const char *client_type, u8 addr,
> +		int irq, void *platform_data)
> +{
> +	struct v4l2_subdev *sd;
> +	int err = 0;
> +
> +	sd = __v4l2_i2c_new_subdev(v4l2_dev, adapter, module_name, client_type,
> +					addr, irq, platform_data);
> +
> +	/*
> +	 * We return errors from v4l2_subdev_call only if we have the callback
> +	 * as the .s_config is not mandatory
> +	 */
> +	if (sd && sd->ops && sd->ops->core && sd->ops->core->s_config)
> +		err = sd->ops->core->s_config(sd, irq, platform_data);
> +
> +	return err < 0 ? NULL : sd;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
> +
>  /* Probe and load an i2c sub-device. */
>  struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct v4l2_device
> *v4l2_dev, struct i2c_adapter *adapter,
> diff -r 142fd6020df3 -r 4fb354645426 linux/include/media/v4l2-common.h
> --- a/linux/include/media/v4l2-common.h	Mon May 18 02:31:55 2009 +0000
> +++ b/linux/include/media/v4l2-common.h	Wed May 27 11:56:45 2009 +0300
> @@ -147,6 +147,14 @@
>  struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
>  		struct i2c_adapter *adapter,
>  		const char *module_name, const char *client_type, u8 addr);
> +/*
> + * Same as v4l2_i2c_new_subdev, but with the opportunity to configure
> + * subdevice with board specific data (irq and platform_data).
> + */
> +struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> *v4l2_dev, +		struct i2c_adapter *adapter,
> +		const char *module_name, const char *client_type, u8 addr,
> +		int irq, void *platform_data);
>  /* Probe and load an i2c module and return an initialized v4l2_subdev
> struct. Only call request_module if module_name != NULL.
>     The client_type argument is the name of the chip that's on the
> adapter. */ diff -r 142fd6020df3 -r 4fb354645426
> linux/include/media/v4l2-subdev.h ---
> a/linux/include/media/v4l2-subdev.h	Mon May 18 02:31:55 2009 +0000 +++
> b/linux/include/media/v4l2-subdev.h	Wed May 27 11:56:45 2009 +0300 @@
> -96,6 +96,7 @@
>  struct v4l2_subdev_core_ops {
>  	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_dbg_chip_ident
> *chip); int (*log_status)(struct v4l2_subdev *sd);
> +	int (*s_config)(struct v4l2_subdev *sd, int irq, void *platform_data);
>  	int (*init)(struct v4l2_subdev *sd, u32 val);
>  	int (*load_fw)(struct v4l2_subdev *sd);
>  	int (*reset)(struct v4l2_subdev *sd, u32 val);



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > # HG changeset patch
> > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > # Date 1243414605 -10800
> > # Branch export
> > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > Device drivers of v4l2_subdev devices may want to have
> > board specific data. This patch adds an helper function
> > to allow bridge drivers to pass board specific data to
> > v4l2_subdev drivers.
> >
> > For those drivers which need to support kernel versions
> > bellow 2.6.26, a .s_config callback was added. The
> > idea of this callback is to pass board configuration
> > as well. In that case, subdev driver should set .s_config
> > properly, because v4l2_i2c_new_subdev_board will call
> > the .s_config directly. Of course, if we are >= 2.6.26,
> > the same data will be passed through i2c board info as well.
>
> Hi Eduardo,
>
> I finally had some time to look at this. After some thought I realized
> that the main problem is really that the API is becoming quite messy.
> Basically there are 9 different ways of loading and initializing a
> subdev:
>
> First there are three basic initialization calls: no initialization,
> passing irq and platform_data, and passing the i2c_board_info struct
> directly (preferred for drivers that don't need pre-2.6.26
> compatibility).
>
> And for each flavor you would like to see three different versions as
> well: one with a fixed known i2c address, one where you probe for a list
> of addresses, and one where you can probe for a single i2c address.
>
> I propose to change the API as follows:
>
> #define V4L2_I2C_ADDRS(addr, addrs...) \
> 	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
>
> struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
>                 struct i2c_adapter *adapter,
>                 const char *module_name, const char *client_type,
> 		u8 addr, const unsigned short *addrs);
>
> struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
>                 struct i2c_adapter *adapter,
>                 const char *module_name, const char *client_type,
>                 int irq, void *platform_data,
>                 u8 addr, const unsigned short *addrs);
>
> /* Only available for kernels >= 2.6.26 */
> struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> i2c_board_info *info, const unsigned short *addrs);
>
> If you use a fixed address, then only set addr (or info.addr) and set
> addrs to NULL. If you want to probe for a list of addrs, then set addrs
> to the list of addrs. If you want to probe for a single addr, then use
> V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array with
> just two entries. Actually, this macro can also create arrays with more
> entries.
>
> Note that v4l2_i2c_new_subdev will be an inline that calls
> v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.
>
> And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline
> calling v4l2_i2c_new_subdev_board.
>
> This approach reduces the number of functions to just one (not counting
> the inlines) and simplifies things all around. It does mean that all
> sources need to be changed, but if we go this route, then now is the time
> before the 2.6.31 window is closed. And I would also like to remove the
> '_new' from these function names. I never thought it added anything
> useful.
>
> Comments? If we decide to go this way, then I need to know soon so that I
> can make the changes before the 2.6.31 window closes.
>
> BTW, if the new s_config subdev call is present, then it should always be
> called. That way the subdev driver can safely do all of its
> initialization in s_config, no matter how it was initialized.
>
> Sorry about the long delay in replying to this: it's been very hectic
> lately at the expense of my v4l-dvb work.

I've done the initial conversion to the new API (no _cfg or _board version 
yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies things and 
if nobody objects then I'd like to get this in before 2.6.31.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Andy Walls]

On Sat, 2009-06-06 at 14:49 +0200, Hans Verkuil wrote:

> > I propose to change the API as follows:
> >
> > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > 	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })

> > Comments? If we decide to go this way, then I need to know soon so that I
> > can make the changes before the 2.6.31 window closes.


> I've done the initial conversion to the new API (no _cfg or _board version 
> yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies things and 
> if nobody objects then I'd like to get this in before 2.6.31.


+Alternatively, you can create the unsigned short array dynamically: 
+ 
+struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter, 
+	       "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12)); 

Strictly speaking, that's not "dynamically" in the sense of the
generated machine code - everything is going to come from the local
stack and the initialized data space.  The compiler will probably be
smart enough to generate an unnamed array in the initialized data space
anyway, avoiding the use of local stack for the array. :)

Anyway, the macro looks fine to me.

But...


@@ -100,16 +100,16 @@ int cx18_i2c_register(struct cx18 *cx, u 

	if (hw == CX18_HW_TUNER) { 
		/* special tuner group handling */ 
-		sd = v4l2_i2c_new_probed_subdev(&cx->v4l2_dev, 
-				adap, mod, type, cx->card_i2c->radio); 
+		sd = v4l2_i2c_subdev(&cx->v4l2_dev, 
+				adap, mod, type, 0, cx->card_i2c->radio); 


Something happened with readability for maintenance purposes.  We're in
cx18_i2c_register(), we're probing, we're allocating new objects, and
we're registering with two subsystems (i2c and v4l).  However, all we
see on the surface is

    "foo = v4l2_i2c_subdev(blah, blah, blah, ... );"

The ALSA subsystem at least uses "_create" for object constructor type
functions.  The v4l2 subdev framework has sophisticated constructors for
convenience.  I know "new" wasn't strcitly correct, as the function does
probe, create, & register an object.  However, the proposed name does
not make it obvious that it's a constructor, IMO.

Regards,
Andy

> Regards,
> 
> 	Hans

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Saturday 06 June 2009 17:19:08 Andy Walls wrote:
> On Sat, 2009-06-06 at 14:49 +0200, Hans Verkuil wrote:
> > > I propose to change the API as follows:
> > >
> > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > 	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> > >
> > > Comments? If we decide to go this way, then I need to know soon so
> > > that I can make the changes before the 2.6.31 window closes.
> >
> > I've done the initial conversion to the new API (no _cfg or _board
> > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies
> > things and if nobody objects then I'd like to get this in before
> > 2.6.31.
>
> +Alternatively, you can create the unsigned short array dynamically:
> +
> +struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter,
> +	       "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12));
>
> Strictly speaking, that's not "dynamically" in the sense of the
> generated machine code - everything is going to come from the local
> stack and the initialized data space.  The compiler will probably be
> smart enough to generate an unnamed array in the initialized data space
> anyway, avoiding the use of local stack for the array. :)

'on the fly' is perhaps a better term...

> Anyway, the macro looks fine to me.
>
> But...
>
>
> @@ -100,16 +100,16 @@ int cx18_i2c_register(struct cx18 *cx, u
>
> 	if (hw == CX18_HW_TUNER) {
> 		/* special tuner group handling */
> -		sd = v4l2_i2c_new_probed_subdev(&cx->v4l2_dev,
> -				adap, mod, type, cx->card_i2c->radio);
> +		sd = v4l2_i2c_subdev(&cx->v4l2_dev,
> +				adap, mod, type, 0, cx->card_i2c->radio);
>
>
> Something happened with readability for maintenance purposes.  We're in
> cx18_i2c_register(), we're probing, we're allocating new objects, and
> we're registering with two subsystems (i2c and v4l).  However, all we
> see on the surface is
>
>     "foo = v4l2_i2c_subdev(blah, blah, blah, ... );"
>
> The ALSA subsystem at least uses "_create" for object constructor type
> functions.  The v4l2 subdev framework has sophisticated constructors for
> convenience.  I know "new" wasn't strcitly correct, as the function does
> probe, create, & register an object.  However, the proposed name does
> not make it obvious that it's a constructor, IMO.

Hmm, I should probably just leave this as v4l2_i2c_new_subdev since that 
corresponds to the i2c core's i2c_new_device call.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > # HG changeset patch
> > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > # Date 1243414605 -10800
> > > # Branch export
> > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > Device drivers of v4l2_subdev devices may want to have
> > > board specific data. This patch adds an helper function
> > > to allow bridge drivers to pass board specific data to
> > > v4l2_subdev drivers.
> > >
> > > For those drivers which need to support kernel versions
> > > bellow 2.6.26, a .s_config callback was added. The
> > > idea of this callback is to pass board configuration
> > > as well. In that case, subdev driver should set .s_config
> > > properly, because v4l2_i2c_new_subdev_board will call
> > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > the same data will be passed through i2c board info as well.
> >
> > Hi Eduardo,
> >
> > I finally had some time to look at this. After some thought I realized
> > that the main problem is really that the API is becoming quite messy.
> > Basically there are 9 different ways of loading and initializing a
> > subdev:
> >
> > First there are three basic initialization calls: no initialization,
> > passing irq and platform_data, and passing the i2c_board_info struct
> > directly (preferred for drivers that don't need pre-2.6.26
> > compatibility).
> >
> > And for each flavor you would like to see three different versions as
> > well: one with a fixed known i2c address, one where you probe for a
> > list of addresses, and one where you can probe for a single i2c
> > address.
> >
> > I propose to change the API as follows:
> >
> > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > 	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> >
> > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
> >                 struct i2c_adapter *adapter,
> >                 const char *module_name, const char *client_type,
> > 		u8 addr, const unsigned short *addrs);
> >
> > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > *v4l2_dev, struct i2c_adapter *adapter,
> >                 const char *module_name, const char *client_type,
> >                 int irq, void *platform_data,
> >                 u8 addr, const unsigned short *addrs);
> >
> > /* Only available for kernels >= 2.6.26 */
> > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> > i2c_board_info *info, const unsigned short *addrs);
> >
> > If you use a fixed address, then only set addr (or info.addr) and set
> > addrs to NULL. If you want to probe for a list of addrs, then set addrs
> > to the list of addrs. If you want to probe for a single addr, then use
> > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array
> > with just two entries. Actually, this macro can also create arrays with
> > more entries.
> >
> > Note that v4l2_i2c_new_subdev will be an inline that calls
> > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.
> >
> > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline
> > calling v4l2_i2c_new_subdev_board.
> >
> > This approach reduces the number of functions to just one (not counting
> > the inlines) and simplifies things all around. It does mean that all
> > sources need to be changed, but if we go this route, then now is the
> > time before the 2.6.31 window is closed. And I would also like to
> > remove the '_new' from these function names. I never thought it added
> > anything useful.
> >
> > Comments? If we decide to go this way, then I need to know soon so that
> > I can make the changes before the 2.6.31 window closes.
> >
> > BTW, if the new s_config subdev call is present, then it should always
> > be called. That way the subdev driver can safely do all of its
> > initialization in s_config, no matter how it was initialized.
> >
> > Sorry about the long delay in replying to this: it's been very hectic
> > lately at the expense of my v4l-dvb work.
>
> I've done the initial conversion to the new API (no _cfg or _board
> version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies
> things and if nobody objects then I'd like to get this in before 2.6.31.

I've added the new _cfg and _board fucntions as well in this tree. It needs 
a bit of a cleanup before I can do a pull request (the last two patches 
should be merged to one), but otherwise this is the code as I think it 
should be:

/* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the fly 
*/
#define V4L2_I2C_ADDRS(addr, addrs...) \
        ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })

/* Load an i2c module and return an initialized v4l2_subdev struct.
   Only call request_module if module_name != NULL.
   The client_type argument is the name of the chip that's on the adapter. 
*/
struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter,
                const char *module_name, const char *client_type,
                int irq, void *platform_data,
                u8 addr, const unsigned short *addrs);

static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
                struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter,
                const char *module_name, const char *client_type,
                u8 addr, const unsigned short *addrs)
{
        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name,
                        client_type, 0, NULL, addr, addrs);
}

#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
struct i2c_board_info;

struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
                struct i2c_adapter *adapter, const char *module_name,
                struct i2c_board_info *info, const unsigned short *addrs);
#endif

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Trent Piepho]

On Sat, 6 Jun 2009, Hans Verkuil wrote:
> On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > I propose to change the API as follows:
> >
> > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > 	((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> >
> > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
> >                 struct i2c_adapter *adapter,
> >                 const char *module_name, const char *client_type,
> > 		u8 addr, const unsigned short *addrs);
> >
> > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
> >                 struct i2c_adapter *adapter,
> >                 const char *module_name, const char *client_type,
> >                 int irq, void *platform_data,
> >                 u8 addr, const unsigned short *addrs);
> >
> > /* Only available for kernels >= 2.6.26 */
> > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> > i2c_board_info *info, const unsigned short *addrs);

Maybe "addrs" could be changed to something like "probed_addrs" so it's
easier to tell that the two address fields are used differently?

Is v4l2_i2c_new_subdev() in effect just a wrapper around
v4l2_i2c_new_subdev_cfg() that calls it with NO_IRQ and NULL for irq and
platform_data?

And could v4l2_i2c_new_subdev_cfg() be done like this?

struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
                 struct i2c_adapter *adapter, const char *module_name,
		 const char *client_type, int irq, void *platform_data,
                 u8 addr, const unsigned short *addrs)
{
	struct i2c_board_info info = {
		.addr = addr, .platform_data = platform_data, .irq = irq, };

	strlcpy(info.type, client_type, sizeof(info.type));
	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, module_name,
			                 &info, addrs);
}

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Trent Piepho]

On Sat, 6 Jun 2009, Andy Walls wrote:
> +Alternatively, you can create the unsigned short array dynamically:
> +
> +struct v4l2_subdev *sd = v4l2_i2c_subdev(v4l2_dev, adapter,
> +	       "module_foo", "chipid", 0, V4L2_I2C_ADDRS(0x10, 0x12));
>
> Strictly speaking, that's not "dynamically" in the sense of the
> generated machine code - everything is going to come from the local
> stack and the initialized data space.  The compiler will probably be
> smart enough to generate an unnamed array in the initialized data space
> anyway, avoiding the use of local stack for the array. :)

No such luck, gcc will create an array on the stack and then initialize it
with a series of move word instructions.  It isn't even smart enough to
turn:

        movw    $1, (%esp)
        movw    $2, 2(%esp)
        movw    $3, 4(%esp)
        movw    $-1, 6(%esp)

into:
	movl	$0x00020001, (%esp)
	movl	$0xffff0003, 4(%esp)

Now, if you use a different syntax, and change this:

#define V4L2_I2C_ADDRS(addr, addrs...) \
        ((const unsigned short []){ addr, ## addrs, -1 })
#define bar(addrs...)   _bar(V4L2_I2C_ADDRS(addrs))

into this:

#define bar(addr, addrs...) \
        ({ const unsigned short _a[] = {addr, ## addrs, -1}; _bar(_a); })

If all the values are constants, then for the latter method only gcc will
will create an array in the initialized data segment and use that.

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Eduardo Valentin]

Hi Hans,

On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > > # HG changeset patch
> > > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > > # Date 1243414605 -10800
> > > > # Branch export
> > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > > Device drivers of v4l2_subdev devices may want to have
> > > > board specific data. This patch adds an helper function
> > > > to allow bridge drivers to pass board specific data to
> > > > v4l2_subdev drivers.
> > > >
> > > > For those drivers which need to support kernel versions
> > > > bellow 2.6.26, a .s_config callback was added. The
> > > > idea of this callback is to pass board configuration
> > > > as well. In that case, subdev driver should set .s_config
> > > > properly, because v4l2_i2c_new_subdev_board will call
> > > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > > the same data will be passed through i2c board info as well.
> > >
> > > Hi Eduardo,
> > >
> > > I finally had some time to look at this. After some thought I realized
> > > that the main problem is really that the API is becoming quite messy.
> > > Basically there are 9 different ways of loading and initializing a
> > > subdev:
> > >
> > > First there are three basic initialization calls: no initialization,
> > > passing irq and platform_data, and passing the i2c_board_info struct
> > > directly (preferred for drivers that don't need pre-2.6.26
> > > compatibility).
> > >
> > > And for each flavor you would like to see three different versions as
> > > well: one with a fixed known i2c address, one where you probe for a
> > > list of addresses, and one where you can probe for a single i2c
> > > address.
> > >
> > > I propose to change the API as follows:
> > >
> > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> > >
> > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
> > >                 struct i2c_adapter *adapter,
> > >                 const char *module_name, const char *client_type,
> > >             u8 addr, const unsigned short *addrs);
> > >
> > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > *v4l2_dev, struct i2c_adapter *adapter,
> > >                 const char *module_name, const char *client_type,
> > >                 int irq, void *platform_data,
> > >                 u8 addr, const unsigned short *addrs);
> > >
> > > /* Only available for kernels >= 2.6.26 */
> > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> > > i2c_board_info *info, const unsigned short *addrs);
> > >
> > > If you use a fixed address, then only set addr (or info.addr) and set
> > > addrs to NULL. If you want to probe for a list of addrs, then set addrs
> > > to the list of addrs. If you want to probe for a single addr, then use
> > > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs an array
> > > with just two entries. Actually, this macro can also create arrays with
> > > more entries.
> > >
> > > Note that v4l2_i2c_new_subdev will be an inline that calls
> > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.
> > >
> > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline
> > > calling v4l2_i2c_new_subdev_board.
> > >
> > > This approach reduces the number of functions to just one (not counting
> > > the inlines) and simplifies things all around. It does mean that all
> > > sources need to be changed, but if we go this route, then now is the
> > > time before the 2.6.31 window is closed. And I would also like to
> > > remove the '_new' from these function names. I never thought it added
> > > anything useful.
> > >
> > > Comments? If we decide to go this way, then I need to know soon so that
> > > I can make the changes before the 2.6.31 window closes.
> > >
> > > BTW, if the new s_config subdev call is present, then it should always
> > > be called. That way the subdev driver can safely do all of its
> > > initialization in s_config, no matter how it was initialized.
> > >
> > > Sorry about the long delay in replying to this: it's been very hectic
> > > lately at the expense of my v4l-dvb work.
> >
> > I've done the initial conversion to the new API (no _cfg or _board
> > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really simplifies
> > things and if nobody objects then I'd like to get this in before 2.6.31.
>
> I've added the new _cfg and _board fucntions as well in this tree. It needs
> a bit of a cleanup before I can do a pull request (the last two patches
> should be merged to one), but otherwise this is the code as I think it
> should be:
>
> /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the fly
> */
> #define V4L2_I2C_ADDRS(addr, addrs...) \
>        ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
>
> /* Load an i2c module and return an initialized v4l2_subdev struct.
>   Only call request_module if module_name != NULL.
>   The client_type argument is the name of the chip that's on the adapter.
> */
> struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
>                struct i2c_adapter *adapter,
>                const char *module_name, const char *client_type,
>                int irq, void *platform_data,
>                u8 addr, const unsigned short *addrs);
>
> static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
>                struct v4l2_device *v4l2_dev,
>                struct i2c_adapter *adapter,
>                const char *module_name, const char *client_type,
>                u8 addr, const unsigned short *addrs)
> {
>        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name,
>                        client_type, 0, NULL, addr, addrs);
> }
>
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
> struct i2c_board_info;
>
> struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
>                struct i2c_adapter *adapter, const char *module_name,
>                struct i2c_board_info *info, const unsigned short *addrs);
> #endif
>
> Regards,
>
>        Hans

I've cloned your tree and took a look at your code. Well, looks like
the proper way to do this change.
I didn't take this approach because it touchs other drivers. However,
concentrating the code  in only one
function is better. I also saw that you have fixed the kernel version
check in the v4l2_device_unregister
function. Great!

I will resend my series without this patch. I will rebase it on top of
your subdev tree so the new api
can be used straight. Is that ok?

>
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Eduardo Bezerra Valentin

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> Hi Hans,
>
> On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > > > # HG changeset patch
> > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > > > # Date 1243414605 -10800
> > > > > # Branch export
> > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > > > Device drivers of v4l2_subdev devices may want to have
> > > > > board specific data. This patch adds an helper function
> > > > > to allow bridge drivers to pass board specific data to
> > > > > v4l2_subdev drivers.
> > > > >
> > > > > For those drivers which need to support kernel versions
> > > > > bellow 2.6.26, a .s_config callback was added. The
> > > > > idea of this callback is to pass board configuration
> > > > > as well. In that case, subdev driver should set .s_config
> > > > > properly, because v4l2_i2c_new_subdev_board will call
> > > > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > > > the same data will be passed through i2c board info as well.
> > > >
> > > > Hi Eduardo,
> > > >
> > > > I finally had some time to look at this. After some thought I
> > > > realized that the main problem is really that the API is becoming
> > > > quite messy. Basically there are 9 different ways of loading and
> > > > initializing a subdev:
> > > >
> > > > First there are three basic initialization calls: no
> > > > initialization, passing irq and platform_data, and passing the
> > > > i2c_board_info struct directly (preferred for drivers that don't
> > > > need pre-2.6.26 compatibility).
> > > >
> > > > And for each flavor you would like to see three different versions
> > > > as well: one with a fixed known i2c address, one where you probe
> > > > for a list of addresses, and one where you can probe for a single
> > > > i2c address.
> > > >
> > > > I propose to change the API as follows:
> > > >
> > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> > > >
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > >                 const char *module_name, const char *client_type,
> > > >             u8 addr, const unsigned short *addrs);
> > > >
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > >                 const char *module_name, const char *client_type,
> > > >                 int irq, void *platform_data,
> > > >                 u8 addr, const unsigned short *addrs);
> > > >
> > > > /* Only available for kernels >= 2.6.26 */
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name,
> > > > struct i2c_board_info *info, const unsigned short *addrs);
> > > >
> > > > If you use a fixed address, then only set addr (or info.addr) and
> > > > set addrs to NULL. If you want to probe for a list of addrs, then
> > > > set addrs to the list of addrs. If you want to probe for a single
> > > > addr, then use V4L2_I2C_ADDRS(addr) as the addrs argument. This
> > > > constructs an array with just two entries. Actually, this macro can
> > > > also create arrays with more entries.
> > > >
> > > > Note that v4l2_i2c_new_subdev will be an inline that calls
> > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.
> > > >
> > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline
> > > > calling v4l2_i2c_new_subdev_board.
> > > >
> > > > This approach reduces the number of functions to just one (not
> > > > counting the inlines) and simplifies things all around. It does
> > > > mean that all sources need to be changed, but if we go this route,
> > > > then now is the time before the 2.6.31 window is closed. And I
> > > > would also like to remove the '_new' from these function names. I
> > > > never thought it added anything useful.
> > > >
> > > > Comments? If we decide to go this way, then I need to know soon so
> > > > that I can make the changes before the 2.6.31 window closes.
> > > >
> > > > BTW, if the new s_config subdev call is present, then it should
> > > > always be called. That way the subdev driver can safely do all of
> > > > its initialization in s_config, no matter how it was initialized.
> > > >
> > > > Sorry about the long delay in replying to this: it's been very
> > > > hectic lately at the expense of my v4l-dvb work.
> > >
> > > I've done the initial conversion to the new API (no _cfg or _board
> > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really
> > > simplifies things and if nobody objects then I'd like to get this in
> > > before 2.6.31.
> >
> > I've added the new _cfg and _board fucntions as well in this tree. It
> > needs a bit of a cleanup before I can do a pull request (the last two
> > patches should be merged to one), but otherwise this is the code as I
> > think it should be:
> >
> > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the
> > fly */
> > #define V4L2_I2C_ADDRS(addr, addrs...) \
> >        ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> >
> > /* Load an i2c module and return an initialized v4l2_subdev struct.
> >   Only call request_module if module_name != NULL.
> >   The client_type argument is the name of the chip that's on the
> > adapter. */
> > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > *v4l2_dev, struct i2c_adapter *adapter,
> >                const char *module_name, const char *client_type,
> >                int irq, void *platform_data,
> >                u8 addr, const unsigned short *addrs);
> >
> > static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
> >                struct v4l2_device *v4l2_dev,
> >                struct i2c_adapter *adapter,
> >                const char *module_name, const char *client_type,
> >                u8 addr, const unsigned short *addrs)
> > {
> >        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name,
> >                        client_type, 0, NULL, addr, addrs);
> > }
> >
> > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
> > struct i2c_board_info;
> >
> > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> > i2c_board_info *info, const unsigned short *addrs); #endif
> >
> > Regards,
> >
> >        Hans
>
> I've cloned your tree and took a look at your code. Well, looks like
> the proper way to do this change.
> I didn't take this approach because it touchs other drivers. However,
> concentrating the code  in only one
> function is better. I also saw that you have fixed the kernel version
> check in the v4l2_device_unregister
> function. Great!
>
> I will resend my series without this patch. I will rebase it on top of
> your subdev tree so the new api
> can be used straight. Is that ok?

Yes, sure. Just be aware that there may be some small changes to my patch 
based on feedback I get. But it is a good test anyway of this API to see if 
it works well for you.

Regards,

	Hans

>
> > --
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media"
> > in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> Eduardo Bezerra Valentin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Mauro Carvalho Chehab]

Em Sun, 7 Jun 2009 08:40:08 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > Hi Hans,
> >
> > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > > > > # HG changeset patch
> > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > > > > # Date 1243414605 -10800
> > > > > > # Branch export
> > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > > > > Device drivers of v4l2_subdev devices may want to have
> > > > > > board specific data. This patch adds an helper function
> > > > > > to allow bridge drivers to pass board specific data to
> > > > > > v4l2_subdev drivers.
> > > > > >
> > > > > > For those drivers which need to support kernel versions
> > > > > > bellow 2.6.26, a .s_config callback was added. The
> > > > > > idea of this callback is to pass board configuration
> > > > > > as well. In that case, subdev driver should set .s_config
> > > > > > properly, because v4l2_i2c_new_subdev_board will call
> > > > > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > > > > the same data will be passed through i2c board info as well.
> > > > >
> > > > > Hi Eduardo,
> > > > >
> > > > > I finally had some time to look at this. After some thought I
> > > > > realized that the main problem is really that the API is becoming
> > > > > quite messy. Basically there are 9 different ways of loading and
> > > > > initializing a subdev:
> > > > >
> > > > > First there are three basic initialization calls: no
> > > > > initialization, passing irq and platform_data, and passing the
> > > > > i2c_board_info struct directly (preferred for drivers that don't
> > > > > need pre-2.6.26 compatibility).
> > > > >
> > > > > And for each flavor you would like to see three different versions
> > > > > as well: one with a fixed known i2c address, one where you probe
> > > > > for a list of addresses, and one where you can probe for a single
> > > > > i2c address.
> > > > >
> > > > > I propose to change the API as follows:
> > > > >
> > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> > > > >
> > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device
> > > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > > >                 const char *module_name, const char *client_type,
> > > > >             u8 addr, const unsigned short *addrs);
> > > > >
> > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > > >                 const char *module_name, const char *client_type,
> > > > >                 int irq, void *platform_data,
> > > > >                 u8 addr, const unsigned short *addrs);
> > > > >
> > > > > /* Only available for kernels >= 2.6.26 */
> > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name,
> > > > > struct i2c_board_info *info, const unsigned short *addrs);
> > > > >
> > > > > If you use a fixed address, then only set addr (or info.addr) and
> > > > > set addrs to NULL. If you want to probe for a list of addrs, then
> > > > > set addrs to the list of addrs. If you want to probe for a single
> > > > > addr, then use V4L2_I2C_ADDRS(addr) as the addrs argument. This
> > > > > constructs an array with just two entries. Actually, this macro can
> > > > > also create arrays with more entries.
> > > > >
> > > > > Note that v4l2_i2c_new_subdev will be an inline that calls
> > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and platform_data.
> > > > >
> > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an inline
> > > > > calling v4l2_i2c_new_subdev_board.
> > > > >
> > > > > This approach reduces the number of functions to just one (not
> > > > > counting the inlines) and simplifies things all around. It does
> > > > > mean that all sources need to be changed, but if we go this route,
> > > > > then now is the time before the 2.6.31 window is closed. And I
> > > > > would also like to remove the '_new' from these function names. I
> > > > > never thought it added anything useful.
> > > > >
> > > > > Comments? If we decide to go this way, then I need to know soon so
> > > > > that I can make the changes before the 2.6.31 window closes.
> > > > >
> > > > > BTW, if the new s_config subdev call is present, then it should
> > > > > always be called. That way the subdev driver can safely do all of
> > > > > its initialization in s_config, no matter how it was initialized.
> > > > >
> > > > > Sorry about the long delay in replying to this: it's been very
> > > > > hectic lately at the expense of my v4l-dvb work.
> > > >
> > > > I've done the initial conversion to the new API (no _cfg or _board
> > > > version yet) in my ~hverkuil/v4l-dvb-subdev tree. It really
> > > > simplifies things and if nobody objects then I'd like to get this in
> > > > before 2.6.31.

No please. We did already lots of change due to the i2c changes, and there are
still some occasional complaints at ML about regressions that might be due to
i2c changes.

Let's keep 2.6.31 clean, as previously agreed, without new KABI changes.
We should focus 2.6.31 on fixing any core issues that may still have. Only with
2.6.30 we'll start to have feedbacks from normal users.

> > >
> > > I've added the new _cfg and _board fucntions as well in this tree. It
> > > needs a bit of a cleanup before I can do a pull request (the last two
> > > patches should be merged to one), but otherwise this is the code as I
> > > think it should be:
> > >
> > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on the
> > > fly */
> > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > >        ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END })
> > >
> > > /* Load an i2c module and return an initialized v4l2_subdev struct.
> > >   Only call request_module if module_name != NULL.
> > >   The client_type argument is the name of the chip that's on the
> > > adapter. */
> > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > *v4l2_dev, struct i2c_adapter *adapter,
> > >                const char *module_name, const char *client_type,
> > >                int irq, void *platform_data,
> > >                u8 addr, const unsigned short *addrs);
> > >
> > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
> > >                struct v4l2_device *v4l2_dev,
> > >                struct i2c_adapter *adapter,
> > >                const char *module_name, const char *client_type,
> > >                u8 addr, const unsigned short *addrs)
> > > {
> > >        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name,
> > >                        client_type, 0, NULL, addr, addrs);
> > > }
> > >
> > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
> > > struct i2c_board_info;
> > >
> > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name, struct
> > > i2c_board_info *info, const unsigned short *addrs); #endif
> > >
> > > Regards,
> > >
> > >        Hans
> >
> > I've cloned your tree and took a look at your code. Well, looks like
> > the proper way to do this change.
> > I didn't take this approach because it touchs other drivers. However,
> > concentrating the code  in only one
> > function is better. I also saw that you have fixed the kernel version
> > check in the v4l2_device_unregister
> > function. Great!
> >
> > I will resend my series without this patch. I will rebase it on top of
> > your subdev tree so the new api
> > can be used straight. Is that ok?
> 
> Yes, sure. Just be aware that there may be some small changes to my patch 
> based on feedback I get. But it is a good test anyway of this API to see if 
> it works well for you.

Eduardo,

Let's analyze and merge your changes using the current development tree. If you
think that Hans approach is better (I haven't analyzed it yet), then it can
later be converted to the new approach



Cheers,
Mauro

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Douglas Schilling Landgraf]

Hi,

On Sun, 7 Jun 2009 22:29:14 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:

> Em Sun, 7 Jun 2009 08:40:08 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > > Hi Hans,
> > >
> > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl>
> > > wrote:
> > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > > > > > # HG changeset patch
> > > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > > > > > # Date 1243414605 -10800
> > > > > > > # Branch export
> > > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > > > > > Device drivers of v4l2_subdev devices may want to have
> > > > > > > board specific data. This patch adds an helper function
> > > > > > > to allow bridge drivers to pass board specific data to
> > > > > > > v4l2_subdev drivers.
> > > > > > >
> > > > > > > For those drivers which need to support kernel versions
> > > > > > > bellow 2.6.26, a .s_config callback was added. The
> > > > > > > idea of this callback is to pass board configuration
> > > > > > > as well. In that case, subdev driver should set .s_config
> > > > > > > properly, because v4l2_i2c_new_subdev_board will call
> > > > > > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > > > > > the same data will be passed through i2c board info as
> > > > > > > well.
> > > > > >
> > > > > > Hi Eduardo,
> > > > > >
> > > > > > I finally had some time to look at this. After some thought
> > > > > > I realized that the main problem is really that the API is
> > > > > > becoming quite messy. Basically there are 9 different ways
> > > > > > of loading and initializing a subdev:
> > > > > >
> > > > > > First there are three basic initialization calls: no
> > > > > > initialization, passing irq and platform_data, and passing
> > > > > > the i2c_board_info struct directly (preferred for drivers
> > > > > > that don't need pre-2.6.26 compatibility).
> > > > > >
> > > > > > And for each flavor you would like to see three different
> > > > > > versions as well: one with a fixed known i2c address, one
> > > > > > where you probe for a list of addresses, and one where you
> > > > > > can probe for a single i2c address.
> > > > > >
> > > > > > I propose to change the API as follows:
> > > > > >
> > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > > > >     ((const unsigned short []){ addr, ## addrs,
> > > > > > I2C_CLIENT_END })
> > > > > >
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device
> > > > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > > > >                 const char *module_name, const char
> > > > > > *client_type, u8 addr, const unsigned short *addrs);
> > > > > >
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct
> > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter,
> > > > > >                 const char *module_name, const char
> > > > > > *client_type, int irq, void *platform_data,
> > > > > >                 u8 addr, const unsigned short *addrs);
> > > > > >
> > > > > > /* Only available for kernels >= 2.6.26 */
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const
> > > > > > char *module_name, struct i2c_board_info *info, const
> > > > > > unsigned short *addrs);
> > > > > >
> > > > > > If you use a fixed address, then only set addr (or
> > > > > > info.addr) and set addrs to NULL. If you want to probe for
> > > > > > a list of addrs, then set addrs to the list of addrs. If
> > > > > > you want to probe for a single addr, then use
> > > > > > V4L2_I2C_ADDRS(addr) as the addrs argument. This constructs
> > > > > > an array with just two entries. Actually, this macro can
> > > > > > also create arrays with more entries.
> > > > > >
> > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls
> > > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and
> > > > > > platform_data.
> > > > > >
> > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an
> > > > > > inline calling v4l2_i2c_new_subdev_board.
> > > > > >
> > > > > > This approach reduces the number of functions to just one
> > > > > > (not counting the inlines) and simplifies things all
> > > > > > around. It does mean that all sources need to be changed,
> > > > > > but if we go this route, then now is the time before the
> > > > > > 2.6.31 window is closed. And I would also like to remove
> > > > > > the '_new' from these function names. I never thought it
> > > > > > added anything useful.
> > > > > >
> > > > > > Comments? If we decide to go this way, then I need to know
> > > > > > soon so that I can make the changes before the 2.6.31
> > > > > > window closes.
> > > > > >
> > > > > > BTW, if the new s_config subdev call is present, then it
> > > > > > should always be called. That way the subdev driver can
> > > > > > safely do all of its initialization in s_config, no matter
> > > > > > how it was initialized.
> > > > > >
> > > > > > Sorry about the long delay in replying to this: it's been
> > > > > > very hectic lately at the expense of my v4l-dvb work.
> > > > >
> > > > > I've done the initial conversion to the new API (no _cfg or
> > > > > _board version yet) in my ~hverkuil/v4l-dvb-subdev tree. It
> > > > > really simplifies things and if nobody objects then I'd like
> > > > > to get this in before 2.6.31.
> 
> No please. We did already lots of change due to the i2c changes, and
> there are still some occasional complaints at ML about regressions
> that might be due to i2c changes.
> 
> Let's keep 2.6.31 clean, as previously agreed, without new KABI
> changes. We should focus 2.6.31 on fixing any core issues that may
> still have. Only with 2.6.30 we'll start to have feedbacks from
> normal users.
> 
> > > >
> > > > I've added the new _cfg and _board fucntions as well in this
> > > > tree. It needs a bit of a cleanup before I can do a pull
> > > > request (the last two patches should be merged to one), but
> > > > otherwise this is the code as I think it should be:
> > > >
> > > > /* Construct an I2C_CLIENT_END-terminated array of i2c
> > > > addresses on the fly */
> > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > >        ((const unsigned short []){ addr, ## addrs,
> > > > I2C_CLIENT_END })
> > > >
> > > > /* Load an i2c module and return an initialized v4l2_subdev
> > > > struct. Only call request_module if module_name != NULL.
> > > >   The client_type argument is the name of the chip that's on the
> > > > adapter. */
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > >                const char *module_name, const char *client_type,
> > > >                int irq, void *platform_data,
> > > >                u8 addr, const unsigned short *addrs);
> > > >
> > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
> > > >                struct v4l2_device *v4l2_dev,
> > > >                struct i2c_adapter *adapter,
> > > >                const char *module_name, const char *client_type,
> > > >                u8 addr, const unsigned short *addrs)
> > > > {
> > > >        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter,
> > > > module_name, client_type, 0, NULL, addr, addrs);
> > > > }
> > > >
> > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
> > > > struct i2c_board_info;
> > > >
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter, const char
> > > > *module_name, struct i2c_board_info *info, const unsigned short
> > > > *addrs); #endif
> > > >
> > > > Regards,
> > > >
> > > >        Hans
> > >
> > > I've cloned your tree and took a look at your code. Well, looks
> > > like the proper way to do this change.
> > > I didn't take this approach because it touchs other drivers.
> > > However, concentrating the code  in only one
> > > function is better. I also saw that you have fixed the kernel
> > > version check in the v4l2_device_unregister
> > > function. Great!
> > >
> > > I will resend my series without this patch. I will rebase it on
> > > top of your subdev tree so the new api
> > > can be used straight. Is that ok?
> > 
> > Yes, sure. Just be aware that there may be some small changes to my
> > patch based on feedback I get. But it is a good test anyway of this
> > API to see if it works well for you.
> 
> Eduardo,
> 
> Let's analyze and merge your changes using the current development
> tree. If you think that Hans approach is better (I haven't analyzed
> it yet), then it can later be converted to the new approach
> 

I have talked with Eduardo during last week and if there is no
objections, I am ready to request a pull from the current/last
patches series.

Cheers,
Douglas

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Eduardo Valentin]

Hi guys,

On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf wrote:
> Hi,
> 
> On Sun, 7 Jun 2009 22:29:14 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> 
> > Em Sun, 7 Jun 2009 08:40:08 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > > > Hi Hans,
> > > >
> > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl>
> > > > wrote:
> > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:

<snip>

> > 
> > No please. We did already lots of change due to the i2c changes, and
> > there are still some occasional complaints at ML about regressions
> > that might be due to i2c changes.
> > 
> > Let's keep 2.6.31 clean, as previously agreed, without new KABI
> > changes. We should focus 2.6.31 on fixing any core issues that may
> > still have. Only with 2.6.30 we'll start to have feedbacks from
> > normal users.

<snip>

> > > >
> > > > I've cloned your tree and took a look at your code. Well, looks
> > > > like the proper way to do this change.
> > > > I didn't take this approach because it touchs other drivers.
> > > > However, concentrating the code  in only one
> > > > function is better. I also saw that you have fixed the kernel
> > > > version check in the v4l2_device_unregister
> > > > function. Great!
> > > >
> > > > I will resend my series without this patch. I will rebase it on
> > > > top of your subdev tree so the new api
> > > > can be used straight. Is that ok?
> > > 
> > > Yes, sure. Just be aware that there may be some small changes to my
> > > patch based on feedback I get. But it is a good test anyway of this
> > > API to see if it works well for you.
> > 
> > Eduardo,
> > 
> > Let's analyze and merge your changes using the current development
> > tree. If you think that Hans approach is better (I haven't analyzed
> > it yet), then it can later be converted to the new approach
> > 
> 
> I have talked with Eduardo during last week and if there is no
> objections, I am ready to request a pull from the current/last
> patches series.

Yes, my series is already in one of Douglas' trees and we have tested it.
However, in that series there is one patch which does partially what Hans is
proposing. Which is: add a way to pass platform info to i2c drivers, using
v4l2 i2c helper functions. They way it is done in this patch it does not affect
any other driver. Hans did also some re-factoring in existing i2c helper function,
besides adding new way to pass platform data.

If you agree we can use it for now and in next window we
change things to have them using the way Hans did (which is more complete).

What do you think?

> 
> Cheers,
> Douglas


Cheers,

-- 
Eduardo Valentin

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Monday 08 June 2009 03:29:14 Mauro Carvalho Chehab wrote:
> Em Sun, 7 Jun 2009 08:40:08 +0200
>
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > > Hi Hans,
> > >
> > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl> 
wrote:
> > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> > > > > > On Friday 29 May 2009 09:33:21 Eduardo Valentin wrote:
> > > > > > > # HG changeset patch
> > > > > > > # User Eduardo Valentin <eduardo.valentin@nokia.com>
> > > > > > > # Date 1243414605 -10800
> > > > > > > # Branch export
> > > > > > > # Node ID 4fb354645426f8b187c2c90cd8528b2518461005
> > > > > > > # Parent  142fd6020df3b4d543068155e49a2618140efa49
> > > > > > > Device drivers of v4l2_subdev devices may want to have
> > > > > > > board specific data. This patch adds an helper function
> > > > > > > to allow bridge drivers to pass board specific data to
> > > > > > > v4l2_subdev drivers.
> > > > > > >
> > > > > > > For those drivers which need to support kernel versions
> > > > > > > bellow 2.6.26, a .s_config callback was added. The
> > > > > > > idea of this callback is to pass board configuration
> > > > > > > as well. In that case, subdev driver should set .s_config
> > > > > > > properly, because v4l2_i2c_new_subdev_board will call
> > > > > > > the .s_config directly. Of course, if we are >= 2.6.26,
> > > > > > > the same data will be passed through i2c board info as well.
> > > > > >
> > > > > > Hi Eduardo,
> > > > > >
> > > > > > I finally had some time to look at this. After some thought I
> > > > > > realized that the main problem is really that the API is
> > > > > > becoming quite messy. Basically there are 9 different ways of
> > > > > > loading and initializing a subdev:
> > > > > >
> > > > > > First there are three basic initialization calls: no
> > > > > > initialization, passing irq and platform_data, and passing the
> > > > > > i2c_board_info struct directly (preferred for drivers that
> > > > > > don't need pre-2.6.26 compatibility).
> > > > > >
> > > > > > And for each flavor you would like to see three different
> > > > > > versions as well: one with a fixed known i2c address, one where
> > > > > > you probe for a list of addresses, and one where you can probe
> > > > > > for a single i2c address.
> > > > > >
> > > > > > I propose to change the API as follows:
> > > > > >
> > > > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > > > >     ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END
> > > > > > })
> > > > > >
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device
> > > > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > > > >                 const char *module_name, const char
> > > > > > *client_type, u8 addr, const unsigned short *addrs);
> > > > > >
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > > > >                 const char *module_name, const char
> > > > > > *client_type, int irq, void *platform_data,
> > > > > >                 u8 addr, const unsigned short *addrs);
> > > > > >
> > > > > > /* Only available for kernels >= 2.6.26 */
> > > > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> > > > > > v4l2_device *v4l2_dev, struct i2c_adapter *adapter, const char
> > > > > > *module_name, struct i2c_board_info *info, const unsigned short
> > > > > > *addrs);
> > > > > >
> > > > > > If you use a fixed address, then only set addr (or info.addr)
> > > > > > and set addrs to NULL. If you want to probe for a list of
> > > > > > addrs, then set addrs to the list of addrs. If you want to
> > > > > > probe for a single addr, then use V4L2_I2C_ADDRS(addr) as the
> > > > > > addrs argument. This constructs an array with just two entries.
> > > > > > Actually, this macro can also create arrays with more entries.
> > > > > >
> > > > > > Note that v4l2_i2c_new_subdev will be an inline that calls
> > > > > > v4l2_i2c_new_subdev_cfg with 0, NULL for the irq and
> > > > > > platform_data.
> > > > > >
> > > > > > And for kernels >= 2.6.26 v4l2_i2c_new_subdev_cfg can be an
> > > > > > inline calling v4l2_i2c_new_subdev_board.
> > > > > >
> > > > > > This approach reduces the number of functions to just one (not
> > > > > > counting the inlines) and simplifies things all around. It does
> > > > > > mean that all sources need to be changed, but if we go this
> > > > > > route, then now is the time before the 2.6.31 window is closed.
> > > > > > And I would also like to remove the '_new' from these function
> > > > > > names. I never thought it added anything useful.
> > > > > >
> > > > > > Comments? If we decide to go this way, then I need to know soon
> > > > > > so that I can make the changes before the 2.6.31 window closes.
> > > > > >
> > > > > > BTW, if the new s_config subdev call is present, then it should
> > > > > > always be called. That way the subdev driver can safely do all
> > > > > > of its initialization in s_config, no matter how it was
> > > > > > initialized.
> > > > > >
> > > > > > Sorry about the long delay in replying to this: it's been very
> > > > > > hectic lately at the expense of my v4l-dvb work.
> > > > >
> > > > > I've done the initial conversion to the new API (no _cfg or
> > > > > _board version yet) in my ~hverkuil/v4l-dvb-subdev tree. It
> > > > > really simplifies things and if nobody objects then I'd like to
> > > > > get this in before 2.6.31.
>
> No please. We did already lots of change due to the i2c changes, and
> there are still some occasional complaints at ML about regressions that
> might be due to i2c changes.

Please point them out to me. I can't remember seeing those. Admittedly, I've 
been swamped with work in the past few weeks, so I might have missed them.

> Let's keep 2.6.31 clean, as previously agreed, without new KABI changes.
> We should focus 2.6.31 on fixing any core issues that may still have.
> Only with 2.6.30 we'll start to have feedbacks from normal users.

It's a new API, so it is normal that we find things that need improvement. 
This is the case here. Eduardo needs to pass extra initialization info to 
the i2c drivers, so he is adding new functions to the API. But those make 
an overly complex (as I've come to realize) API even more complex. Isn't is 
better to do it right?

And I don't see the advantage of waiting with this. If there are any 
problems with 2.6.30 then we will get them whether we change this internal 
API or not. In addition, I think this is pretty much the only change in the 
API to be queued for 2.6.31.

And how long do you intend to wait? It will take a while before all the 
distros take up 2.6.30. So wait until 2.6.32? 2.6.33?

> > > > I've added the new _cfg and _board fucntions as well in this tree.
> > > > It needs a bit of a cleanup before I can do a pull request (the
> > > > last two patches should be merged to one), but otherwise this is
> > > > the code as I think it should be:
> > > >
> > > > /* Construct an I2C_CLIENT_END-terminated array of i2c addresses on
> > > > the fly */
> > > > #define V4L2_I2C_ADDRS(addr, addrs...) \
> > > >        ((const unsigned short []){ addr, ## addrs, I2C_CLIENT_END
> > > > })
> > > >
> > > > /* Load an i2c module and return an initialized v4l2_subdev struct.
> > > >   Only call request_module if module_name != NULL.
> > > >   The client_type argument is the name of the chip that's on the
> > > > adapter. */
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter,
> > > >                const char *module_name, const char *client_type,
> > > >                int irq, void *platform_data,
> > > >                u8 addr, const unsigned short *addrs);
> > > >
> > > > static inline struct v4l2_subdev *v4l2_i2c_new_subdev(
> > > >                struct v4l2_device *v4l2_dev,
> > > >                struct i2c_adapter *adapter,
> > > >                const char *module_name, const char *client_type,
> > > >                u8 addr, const unsigned short *addrs)
> > > > {
> > > >        return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter,
> > > > module_name, client_type, 0, NULL, addr, addrs); }
> > > >
> > > > #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
> > > > struct i2c_board_info;
> > > >
> > > > struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device
> > > > *v4l2_dev, struct i2c_adapter *adapter, const char *module_name,
> > > > struct i2c_board_info *info, const unsigned short *addrs); #endif
> > > >
> > > > Regards,
> > > >
> > > >        Hans
> > >
> > > I've cloned your tree and took a look at your code. Well, looks like
> > > the proper way to do this change.
> > > I didn't take this approach because it touchs other drivers. However,
> > > concentrating the code  in only one
> > > function is better. I also saw that you have fixed the kernel version
> > > check in the v4l2_device_unregister
> > > function. Great!
> > >
> > > I will resend my series without this patch. I will rebase it on top
> > > of your subdev tree so the new api
> > > can be used straight. Is that ok?
> >
> > Yes, sure. Just be aware that there may be some small changes to my
> > patch based on feedback I get. But it is a good test anyway of this API
> > to see if it works well for you.
>
> Eduardo,
>
> Let's analyze and merge your changes using the current development tree.
> If you think that Hans approach is better (I haven't analyzed it yet),
> then it can later be converted to the new approach

If you really are opposed to my changes, then I need to look at these 
patches again since they need to be modified a fair amount. They will have 
to touch the core subdev API anyway, so let's do it right rather than 
hacking in this new functionality and having to change it again in 2.6.32.

If something is wrong, then let's just fix it instead of trying to hack it 
in. That only increases the chances of more errors. After implementing my 
changes I came to realize that it is a much cleaner approach.

One alternative that I would be OK with is to wait with this until the 
2.6.31 window closes. But then Eduardo's driver won't make 2.6.31 either.

BTW, Eduardo isn't the only one who needs these changes. It crops up 
whenever you deal with embedded devices. So please let us just fix this 
part of the API.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Hans Verkuil]

On Monday 08 June 2009 08:11:32 Eduardo Valentin wrote:
> Hi guys,
>
> On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf 
wrote:
> > Hi,
> >
> > On Sun, 7 Jun 2009 22:29:14 -0300
> >
> > Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> > > Em Sun, 7 Jun 2009 08:40:08 +0200
> > >
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > > > > Hi Hans,
> > > > >
> > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl>
> > > > >
> > > > > wrote:
> > > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
>
> <snip>
>
> > > No please. We did already lots of change due to the i2c changes, and
> > > there are still some occasional complaints at ML about regressions
> > > that might be due to i2c changes.
> > >
> > > Let's keep 2.6.31 clean, as previously agreed, without new KABI
> > > changes. We should focus 2.6.31 on fixing any core issues that may
> > > still have. Only with 2.6.30 we'll start to have feedbacks from
> > > normal users.
>
> <snip>
>
> > > > > I've cloned your tree and took a look at your code. Well, looks
> > > > > like the proper way to do this change.
> > > > > I didn't take this approach because it touchs other drivers.
> > > > > However, concentrating the code  in only one
> > > > > function is better. I also saw that you have fixed the kernel
> > > > > version check in the v4l2_device_unregister
> > > > > function. Great!
> > > > >
> > > > > I will resend my series without this patch. I will rebase it on
> > > > > top of your subdev tree so the new api
> > > > > can be used straight. Is that ok?
> > > >
> > > > Yes, sure. Just be aware that there may be some small changes to my
> > > > patch based on feedback I get. But it is a good test anyway of this
> > > > API to see if it works well for you.
> > >
> > > Eduardo,
> > >
> > > Let's analyze and merge your changes using the current development
> > > tree. If you think that Hans approach is better (I haven't analyzed
> > > it yet), then it can later be converted to the new approach
> >
> > I have talked with Eduardo during last week and if there is no
> > objections, I am ready to request a pull from the current/last
> > patches series.
>
> Yes, my series is already in one of Douglas' trees and we have tested it.
> However, in that series there is one patch which does partially what Hans
> is proposing. Which is: add a way to pass platform info to i2c drivers,
> using v4l2 i2c helper functions. They way it is done in this patch it
> does not affect any other driver. Hans did also some re-factoring in
> existing i2c helper function, besides adding new way to pass platform
> data.

No, I don't agree with that. Your patch has some issues: no cleanup after 
s_config returns an error, and if we introduce s_config then it should be 
called by *all* v4l2_new_subdev* functions. That way i2c drivers that 
implement this can use it reliably for their initialization.

I see no point in doing the same work twice. We have one clean solution into 
which I put quite a bit of time, and one that hacks new functionality into 
an already flawed API.

This was also the reason why I didn't just sign off on Eduardo's patch. I 
strongly suspected I needed to do some proper refactoring first and when I 
finally had the time to look into this last Saturday I discovered it did 
indeed needed refactoring.

>
> If you agree we can use it for now and in next window we
> change things to have them using the way Hans did (which is more
> complete).

Going with a suboptimal solution when a proper clean one is available is a 
really bad idea IMHO.

Regards,

	Hans

>
> What do you think?
>
> > Cheers,
> > Douglas
>
> Cheers,



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCHv5 1 of 8] v4l2_subdev i2c: Add v4l2_i2c_new_subdev_board i2c helper function

[Eduardo Valentin]

On Mon, Jun 08, 2009 at 08:38:32AM +0200, ext Hans Verkuil wrote:
> On Monday 08 June 2009 08:11:32 Eduardo Valentin wrote:
> > Hi guys,
> >
> > On Mon, Jun 08, 2009 at 05:19:22AM +0200, ext Douglas Schilling Landgraf 
> wrote:
> > > Hi,
> > >
> > > On Sun, 7 Jun 2009 22:29:14 -0300
> > >
> > > Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> > > > Em Sun, 7 Jun 2009 08:40:08 +0200
> > > >
> > > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > > > On Saturday 06 June 2009 22:40:21 Eduardo Valentin wrote:
> > > > > > Hi Hans,
> > > > > >
> > > > > > On Sat, Jun 6, 2009 at 8:09 PM, Hans Verkuil <hverkuil@xs4all.nl>
> > > > > >
> > > > > > wrote:
> > > > > > > On Saturday 06 June 2009 14:49:46 Hans Verkuil wrote:
> > > > > > > > On Saturday 06 June 2009 13:59:19 Hans Verkuil wrote:
> >
> > <snip>
> >
> > > > No please. We did already lots of change due to the i2c changes, and
> > > > there are still some occasional complaints at ML about regressions
> > > > that might be due to i2c changes.
> > > >
> > > > Let's keep 2.6.31 clean, as previously agreed, without new KABI
> > > > changes. We should focus 2.6.31 on fixing any core issues that may
> > > > still have. Only with 2.6.30 we'll start to have feedbacks from
> > > > normal users.
> >
> > <snip>
> >
> > > > > > I've cloned your tree and took a look at your code. Well, looks
> > > > > > like the proper way to do this change.
> > > > > > I didn't take this approach because it touchs other drivers.
> > > > > > However, concentrating the code  in only one
> > > > > > function is better. I also saw that you have fixed the kernel
> > > > > > version check in the v4l2_device_unregister
> > > > > > function. Great!
> > > > > >
> > > > > > I will resend my series without this patch. I will rebase it on
> > > > > > top of your subdev tree so the new api
> > > > > > can be used straight. Is that ok?
> > > > >
> > > > > Yes, sure. Just be aware that there may be some small changes to my
> > > > > patch based on feedback I get. But it is a good test anyway of this
> > > > > API to see if it works well for you.
> > > >
> > > > Eduardo,
> > > >
> > > > Let's analyze and merge your changes using the current development
> > > > tree. If you think that Hans approach is better (I haven't analyzed
> > > > it yet), then it can later be converted to the new approach
> > >
> > > I have talked with Eduardo during last week and if there is no
> > > objections, I am ready to request a pull from the current/last
> > > patches series.
> >
> > Yes, my series is already in one of Douglas' trees and we have tested it.
> > However, in that series there is one patch which does partially what Hans
> > is proposing. Which is: add a way to pass platform info to i2c drivers,
> > using v4l2 i2c helper functions. They way it is done in this patch it
> > does not affect any other driver. Hans did also some re-factoring in
> > existing i2c helper function, besides adding new way to pass platform
> > data.
> 
> No, I don't agree with that. Your patch has some issues: no cleanup after 
> s_config returns an error, and if we introduce s_config then it should be 
> called by *all* v4l2_new_subdev* functions. That way i2c drivers that 
> implement this can use it reliably for their initialization.
> 
> I see no point in doing the same work twice. We have one clean solution into 
> which I put quite a bit of time, and one that hacks new functionality into 
> an already flawed API.

Agreed. No point in doing the same work.

> 
> This was also the reason why I didn't just sign off on Eduardo's patch. I 
> strongly suspected I needed to do some proper refactoring first and when I 
> finally had the time to look into this last Saturday I discovered it did 
> indeed needed refactoring.
> 
> >
> > If you agree we can use it for now and in next window we
> > change things to have them using the way Hans did (which is more
> > complete).
> 
> Going with a suboptimal solution when a proper clean one is available is a 
> really bad idea IMHO.

As I already said, I really liked your approach because it re-factors the
API. No problem for me to rebase the patches on top of that.

> 
> Regards,
> 
> 	Hans
> 
> >
> > What do you think?
> >
> > > Cheers,
> > > Douglas
> >
> > Cheers,
> 
> 
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

-- 
Eduardo Valentin