* [PATCH 0/3] [media] wl128x: Fixes and few new features
@ 2012-02-24 20:14 manjunatha_halli
2012-02-24 20:14 ` [PATCH 1/3] wl128x: Fix build errors when GPIOLIB is not enabled manjunatha_halli
0 siblings, 1 reply; 17+ messages in thread
From: manjunatha_halli @ 2012-02-24 20:14 UTC (permalink / raw)
To: linux-media; +Cc: shahed, linux-kernel, Manjunatha Halli
From: Manjunatha Halli <x0130808@ti.com>
Mauro and the list,
This patch set Fix build errors when GPIOLIB is not enabled.
Also this patch adds few new features to TI's FM driver fetures
are listed below,
1) FM TX RDS Support (RT, PS, AF, PI, PTY)
2) FM RX Russian band support
3) FM RX AF set/get
4) FM RX De-emphasis mode set/get
Along with new features this patch also fixes few issues in the driver
like default rssi level for seek, unnecessory logs etc.
Manjunatha Halli (2):
wl128x: Add support for FM TX RDS
wl128x: Add sysfs based support for FM features
Randy Dunlap (1):
wl128x: Fix build errors when GPIOLIB is not enabled.
drivers/media/radio/wl128x/Kconfig | 4 +-
drivers/media/radio/wl128x/fmdrv.h | 1 +
drivers/media/radio/wl128x/fmdrv_common.c | 28 +++-
drivers/media/radio/wl128x/fmdrv_common.h | 41 +++--
drivers/media/radio/wl128x/fmdrv_rx.c | 15 ++-
drivers/media/radio/wl128x/fmdrv_tx.c | 47 +++----
drivers/media/radio/wl128x/fmdrv_tx.h | 3 +-
drivers/media/radio/wl128x/fmdrv_v4l2.c | 235 ++++++++++++++++++++++++++++-
8 files changed, 316 insertions(+), 58 deletions(-)
--
1.7.4.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] wl128x: Fix build errors when GPIOLIB is not enabled.
2012-02-24 20:14 [PATCH 0/3] [media] wl128x: Fixes and few new features manjunatha_halli
@ 2012-02-24 20:14 ` manjunatha_halli
2012-02-24 20:14 ` [PATCH 2/3] wl128x: Add support for FM TX RDS manjunatha_halli
0 siblings, 1 reply; 17+ messages in thread
From: manjunatha_halli @ 2012-02-24 20:14 UTC (permalink / raw)
To: linux-media; +Cc: shahed, linux-kernel, Randy Dunlap, Manjunatha Halli
From: Randy Dunlap <rdunlap@xenotime.net>
Fix wl128x Kconfig to depend on GPIOLIB since TI_ST also
depends on GPIOLIB.
(.text+0xe6d60): undefined reference to `st_register'
(.text+0xe7016): undefined reference to `st_unregister'
(.text+0xe70ce): undefined reference to `st_unregister'
Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Signed-off-by: Manjunatha Halli <manjunatha_halli@ti.com>
---
drivers/media/radio/wl128x/Kconfig | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/radio/wl128x/Kconfig b/drivers/media/radio/wl128x/Kconfig
index 86b2857..ea1e654 100644
--- a/drivers/media/radio/wl128x/Kconfig
+++ b/drivers/media/radio/wl128x/Kconfig
@@ -4,8 +4,8 @@
menu "Texas Instruments WL128x FM driver (ST based)"
config RADIO_WL128X
tristate "Texas Instruments WL128x FM Radio"
- depends on VIDEO_V4L2 && RFKILL
- select TI_ST if NET && GPIOLIB
+ depends on VIDEO_V4L2 && RFKILL && GPIOLIB
+ select TI_ST if NET
help
Choose Y here if you have this FM radio chip.
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/3] wl128x: Add support for FM TX RDS
2012-02-24 20:14 ` [PATCH 1/3] wl128x: Fix build errors when GPIOLIB is not enabled manjunatha_halli
@ 2012-02-24 20:14 ` manjunatha_halli
2012-02-24 20:14 ` [PATCH 3/3] wl128x: Add sysfs based support for FM features manjunatha_halli
0 siblings, 1 reply; 17+ messages in thread
From: manjunatha_halli @ 2012-02-24 20:14 UTC (permalink / raw)
To: linux-media; +Cc: shahed, linux-kernel, Manjunatha Halli
From: Manjunatha Halli <x0130808@ti.com>
This patch adds support for following FM TX RDS features,
1. Radio Text
2. PS Name
3. PI Code
4. PTY Code.
Along with above this patch fixes few other minor issues(like
fm tx get frequency, unnecessary error messages etc).
Signed-off-by: Manjunatha Halli <x0130808@ti.com>
---
drivers/media/radio/wl128x/fmdrv_common.c | 17 +++++++---
drivers/media/radio/wl128x/fmdrv_common.h | 17 +++++++----
drivers/media/radio/wl128x/fmdrv_rx.c | 2 +-
drivers/media/radio/wl128x/fmdrv_tx.c | 41 ++++++++++---------------
drivers/media/radio/wl128x/fmdrv_tx.h | 3 +-
drivers/media/radio/wl128x/fmdrv_v4l2.c | 47 +++++++++++++++++++++++++++++
6 files changed, 90 insertions(+), 37 deletions(-)
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
index bf867a6..fcce61a 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.c
+++ b/drivers/media/radio/wl128x/fmdrv_common.c
@@ -354,7 +354,7 @@ static void send_tasklet(unsigned long arg)
/* Check, is there any timeout happened to last transmitted packet */
if ((jiffies - fmdev->last_tx_jiffies) > FM_DRV_TX_TIMEOUT) {
- fmerr("TX timeout occurred\n");
+ fmdbg("TX timeout occurred\n");
atomic_set(&fmdev->tx_cnt, 1);
}
@@ -615,7 +615,11 @@ static void fm_irq_handle_rds_start(struct fmdev *fmdev)
{
if (fmdev->irq_info.flag & FM_RDS_EVENT & fmdev->irq_info.mask) {
fmdbg("irq: rds threshold reached\n");
- fmdev->irq_info.stage = FM_RDS_SEND_RDS_GETCMD_IDX;
+ /* If RSSI reached below threshold then dont get RDS data */
+ if (fmdev->irq_info.flag & FM_LEV_EVENT)
+ fmdev->irq_info.stage = FM_HW_TUNE_OP_ENDED_IDX;
+ else
+ fmdev->irq_info.stage = FM_RDS_SEND_RDS_GETCMD_IDX;
} else {
/* Continue next function in interrupt handler table */
fmdev->irq_info.stage = FM_HW_TUNE_OP_ENDED_IDX;
@@ -1129,8 +1133,9 @@ int fmc_set_freq(struct fmdev *fmdev, u32 freq_to_set)
int fmc_get_freq(struct fmdev *fmdev, u32 *cur_tuned_frq)
{
- if (fmdev->rx.freq == FM_UNDEFINED_FREQ) {
- fmerr("RX frequency is not set\n");
+ if (fmdev->rx.freq == FM_UNDEFINED_FREQ &&
+ fmdev->tx_data.tx_frq == FM_UNDEFINED_FREQ) {
+ fmerr("RX/TX frequency is not set\n");
return -EPERM;
}
if (cur_tuned_frq == NULL) {
@@ -1144,7 +1149,7 @@ int fmc_get_freq(struct fmdev *fmdev, u32 *cur_tuned_frq)
return 0;
case FM_MODE_TX:
- *cur_tuned_frq = 0; /* TODO : Change this later */
+ *cur_tuned_frq = fmdev->tx_data.tx_frq;
return 0;
default:
@@ -1574,6 +1579,8 @@ int fmc_prepare(struct fmdev *fmdev)
fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
fmdev->irq_info.retry = 0;
+ fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
+
fm_rx_reset_rds_cache(fmdev);
init_waitqueue_head(&fmdev->rx.rds.read_queue);
diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
index d9b9c6c..196ff7d 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.h
+++ b/drivers/media/radio/wl128x/fmdrv_common.h
@@ -48,8 +48,8 @@ struct fm_reg_table {
#define SEARCH_LVL_SET 15
#define BAND_SET 16
#define MUTE_STATUS_SET 17
-#define RDS_PAUSE_LVL_SET 18
-#define RDS_PAUSE_DUR_SET 19
+#define AUD_PAUSE_LVL_SET 18
+#define AUD_PAUSE_DUR_SET 19
#define RDS_MEM_SET 20
#define RDS_BLK_B_SET 21
#define RDS_MSK_B_SET 22
@@ -84,11 +84,12 @@ struct fm_reg_table {
#define FM_POWER_MODE 254
#define FM_INTERRUPT 255
+#define STATION_VALID 123
/* Transmitter API */
#define CHANL_SET 55
-#define CHANL_BW_SET 56
+#define SCAN_SPACING_SET 56
#define REF_SET 57
#define POWER_ENB_SET 90
#define POWER_ATT_SET 58
@@ -103,7 +104,8 @@ struct fm_reg_table {
#define MONO_SET 66
#define MUTE 92
#define MPX_LMT_ENABLE 67
-#define PI_SET 93
+#define REF_ERR_SET 93
+#define PI_SET 68
#define ECC_SET 69
#define PTY 70
#define AF 71
@@ -120,6 +122,10 @@ struct fm_reg_table {
#define TX_AUDIO_LEVEL_TEST 96
#define TX_AUDIO_LEVEL_TEST_THRESHOLD 73
#define TX_AUDIO_INPUT_LEVEL_RANGE_SET 54
+#define TX_AUDIO_LEVEL_GET 7
+#define READ_FMANT_TUNE_VALUE 104
+
+/* New FM APIs (Rx and Tx) */
#define RX_ANTENNA_SELECT 87
#define I2C_DEV_ADDR_SET 86
#define REF_ERR_CALIB_PARAM_SET 88
@@ -131,7 +137,6 @@ struct fm_reg_table {
#define RSSI_BLOCK_SCAN_FREQ_SET 95
#define RSSI_BLOCK_SCAN_START 97
#define RSSI_BLOCK_SCAN_DATA_GET 5
-#define READ_FMANT_TUNE_VALUE 104
/* SKB helpers */
struct fm_skb_cb {
@@ -348,7 +353,7 @@ struct fm_event_msg_hdr {
* with this default values after loading RX firmware.
*/
#define FM_DEFAULT_RX_VOLUME 10
-#define FM_DEFAULT_RSSI_THRESHOLD 3
+#define FM_DEFAULT_RSSI_THRESHOLD 20
/* Range for TX power level in units for dB/uV */
#define FM_PWR_LVL_LOW 91
diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
index 43fb722..a806bda 100644
--- a/drivers/media/radio/wl128x/fmdrv_rx.c
+++ b/drivers/media/radio/wl128x/fmdrv_rx.c
@@ -156,7 +156,7 @@ static int fm_rx_set_channel_spacing(struct fmdev *fmdev, u32 spacing)
/* set channel spacing */
payload = spacing;
- ret = fmc_send_cmd(fmdev, CHANL_BW_SET, REG_WR, &payload,
+ ret = fmc_send_cmd(fmdev, SCAN_SPACING_SET, REG_WR, &payload,
sizeof(payload), NULL, NULL);
if (ret < 0)
return ret;
diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
index 6ea33e0..6d879fb 100644
--- a/drivers/media/radio/wl128x/fmdrv_tx.c
+++ b/drivers/media/radio/wl128x/fmdrv_tx.c
@@ -51,6 +51,7 @@ static int set_rds_text(struct fmdev *fmdev, u8 *rds_text)
u16 payload;
int ret;
+ *(u16 *)rds_text = cpu_to_be16(*(u16 *)rds_text);
ret = fmc_send_cmd(fmdev, RDS_DATA_SET, REG_WR, rds_text,
strlen(rds_text), NULL, NULL);
if (ret < 0)
@@ -66,26 +67,31 @@ static int set_rds_text(struct fmdev *fmdev, u8 *rds_text)
return 0;
}
-static int set_rds_data_mode(struct fmdev *fmdev, u8 mode)
+int set_rds_picode(struct fmdev *fmdev, u16 pi_val)
{
u16 payload;
int ret;
- /* Setting unique PI TODO: how unique? */
- payload = (u16)0xcafe;
+ payload = pi_val;
ret = fmc_send_cmd(fmdev, PI_SET, REG_WR, &payload,
sizeof(payload), NULL, NULL);
if (ret < 0)
return ret;
- /* Set decoder id */
- payload = (u16)0xa;
- ret = fmc_send_cmd(fmdev, DI_SET, REG_WR, &payload,
+ return 0;
+}
+
+int set_rds_pty(struct fmdev *fmdev, u16 pty)
+{
+ u16 payload;
+ u32 ret;
+
+ payload = pty;
+ ret = fmc_send_cmd(fmdev, PTY, REG_WR, &payload,
sizeof(payload), NULL, NULL);
if (ret < 0)
return ret;
- /* TODO: RDS_MODE_GET? */
return 0;
}
@@ -101,7 +107,6 @@ static int set_rds_len(struct fmdev *fmdev, u8 type, u16 len)
if (ret < 0)
return ret;
- /* TODO: LENGTH_GET? */
return 0;
}
@@ -109,20 +114,17 @@ int fm_tx_set_rds_mode(struct fmdev *fmdev, u8 rds_en_dis)
{
u16 payload;
int ret;
- u8 rds_text[] = "Zoom2\n";
+ u8 rds_text[] = "WL12XX Radio\n";
fmdbg("rds_en_dis:%d(E:%d, D:%d)\n", rds_en_dis,
FM_RDS_ENABLE, FM_RDS_DISABLE);
if (rds_en_dis == FM_RDS_ENABLE) {
/* Set RDS length */
- set_rds_len(fmdev, 0, strlen(rds_text));
+ set_rds_len(fmdev, 2, strlen(rds_text));
/* Set RDS text */
set_rds_text(fmdev, rds_text);
-
- /* Set RDS mode */
- set_rds_data_mode(fmdev, 0x0);
}
/* Send command to enable RDS */
@@ -136,13 +138,6 @@ int fm_tx_set_rds_mode(struct fmdev *fmdev, u8 rds_en_dis)
if (ret < 0)
return ret;
- if (rds_en_dis == FM_RDS_ENABLE) {
- /* Set RDS length */
- set_rds_len(fmdev, 0, strlen(rds_text));
-
- /* Set RDS text */
- set_rds_text(fmdev, rds_text);
- }
fmdev->tx_data.rds.flag = rds_en_dis;
return 0;
@@ -156,7 +151,6 @@ int fm_tx_set_radio_text(struct fmdev *fmdev, u8 *rds_text, u8 rds_type)
if (fmdev->curr_fmmode != FM_MODE_TX)
return -EPERM;
- fm_tx_set_rds_mode(fmdev, 0);
/* Set RDS length */
set_rds_len(fmdev, rds_type, strlen(rds_text));
@@ -164,9 +158,6 @@ int fm_tx_set_radio_text(struct fmdev *fmdev, u8 *rds_text, u8 rds_type)
/* Set RDS text */
set_rds_text(fmdev, rds_text);
- /* Set RDS mode */
- set_rds_data_mode(fmdev, 0x0);
-
payload = 1;
ret = fmc_send_cmd(fmdev, RDS_DATA_ENB, REG_WR, &payload,
sizeof(payload), NULL, NULL);
@@ -421,6 +412,8 @@ int fm_tx_set_freq(struct fmdev *fmdev, u32 freq_to_set)
tx->aud_mode = FM_STEREO_MODE;
tx->rds.flag = FM_RDS_DISABLE;
+ tx->tx_frq = freq_to_set * 1000; /* in KHz */
+
return 0;
}
diff --git a/drivers/media/radio/wl128x/fmdrv_tx.h b/drivers/media/radio/wl128x/fmdrv_tx.h
index 11ae2e4..8ed71bd 100644
--- a/drivers/media/radio/wl128x/fmdrv_tx.h
+++ b/drivers/media/radio/wl128x/fmdrv_tx.h
@@ -32,6 +32,7 @@ int fm_tx_set_radio_text(struct fmdev *, u8 *, u8);
int fm_tx_set_af(struct fmdev *, u32);
int fm_tx_set_preemph_filter(struct fmdev *, u32);
int fm_tx_get_tune_cap_val(struct fmdev *);
-
+int set_rds_picode(struct fmdev *, u16);
+int set_rds_pty(struct fmdev *, u16);
#endif
diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
index 077d369..b9da1ae 100644
--- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -210,6 +210,8 @@ static int fm_v4l2_s_ctrl(struct v4l2_ctrl *ctrl)
struct fmdev *fmdev = container_of(ctrl->handler,
struct fmdev, ctrl_handler);
+ int ret;
+
switch (ctrl->id) {
case V4L2_CID_AUDIO_VOLUME: /* set volume */
return fm_rx_set_volume(fmdev, (u16)ctrl->val);
@@ -224,6 +226,38 @@ static int fm_v4l2_s_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_TUNE_PREEMPHASIS:
return fm_tx_set_preemph_filter(fmdev, (u8) ctrl->val);
+ case V4L2_CID_RDS_TX_PI:
+ ret = set_rds_picode(fmdev, ctrl->val);
+ if (ret < 0) {
+ fmerr("Failed to set RDS Radio PS Name\n");
+ return ret;
+ }
+ return 0;
+
+ case V4L2_CID_RDS_TX_PTY:
+ ret = set_rds_pty(fmdev, ctrl->val);
+ if (ret < 0) {
+ fmerr("Failed to set RDS Radio PS Name\n");
+ return ret;
+ }
+ return 0;
+
+ case V4L2_CID_RDS_TX_PS_NAME:
+ ret = fm_tx_set_radio_text(fmdev, ctrl->string, 1);
+ if (ret < 0) {
+ fmerr("Failed to set RDS Radio PS Name\n");
+ return ret;
+ }
+ return 0;
+
+ case V4L2_CID_RDS_TX_RADIO_TEXT:
+ ret = fm_tx_set_radio_text(fmdev, ctrl->string, 2);
+ if (ret < 0) {
+ fmerr("Failed to set RDS Radio Text\n");
+ return ret;
+ }
+ return 0;
+
default:
return -EINVAL;
}
@@ -455,6 +489,7 @@ static int fm_v4l2_vidioc_s_modulator(struct file *file, void *priv,
fmerr("Failed to set mono/stereo mode for TX\n");
return ret;
}
+
ret = fm_tx_set_rds_mode(fmdev, rds_mode);
if (ret < 0)
fmerr("Failed to set rds mode for TX\n");
@@ -549,6 +584,18 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
+ v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
+ V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
+
+ v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
+ V4L2_CID_RDS_TX_PTY, 0, 32, 1, 0);
+
+ v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
+ V4L2_CID_RDS_TX_PS_NAME, 0, 0xffff, 1, 0);
+
+ v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
+ V4L2_CID_RDS_TX_RADIO_TEXT, 0, 0xffff, 1, 0);
+
v4l2_ctrl_new_std_menu(&fmdev->ctrl_handler, &fm_ctrl_ops,
V4L2_CID_TUNE_PREEMPHASIS, V4L2_PREEMPHASIS_75_uS,
0, V4L2_PREEMPHASIS_75_uS);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-24 20:14 ` [PATCH 2/3] wl128x: Add support for FM TX RDS manjunatha_halli
@ 2012-02-24 20:14 ` manjunatha_halli
2012-02-25 8:16 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: manjunatha_halli @ 2012-02-24 20:14 UTC (permalink / raw)
To: linux-media; +Cc: shahed, linux-kernel, Manjunatha Halli
From: Manjunatha Halli <x0130808@ti.com>
This patch adds support for below features via sysfs file system
1) FM RX Band selection (US/Europe, Japan and Russian bands)
2) FM RX RDS AF turn ON/OFF
3) FM RX RSSI level set/get
4) FM TX Alternate Frequency set/get
5) FM RX De-Emphasis mode set/get
Also this patch fixes below issues
1) FM audio volume gain setting
2) Default rssi level is set to 8 instead of 20
3) Issue related to audio mute/unmute
4)Enable FM RX AF support in driver
5)In wrap_around seek mode try for once
Signed-off-by: Manjunatha Halli <x0130808@ti.com>
---
drivers/media/radio/wl128x/fmdrv.h | 1 +
drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
6 files changed, 225 insertions(+), 20 deletions(-)
diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
index d84ad9d..8fe773d 100644
--- a/drivers/media/radio/wl128x/fmdrv.h
+++ b/drivers/media/radio/wl128x/fmdrv.h
@@ -196,6 +196,7 @@ struct fmtx_data {
u16 aud_mode;
u32 preemph;
u32 tx_frq;
+ u32 af_frq;
struct tx_rds rds;
};
diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
index fcce61a..1a580a7 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.c
+++ b/drivers/media/radio/wl128x/fmdrv_common.c
@@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
.top_freq = 90000, /* 90 MHz */
.fm_band = 1,
},
+ /* Russian (OIRT) band */
+ {
+ .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
+ .bot_freq = 65800, /* 65.8 MHz */
+ .top_freq = 74000, /* 74 MHz */
+ .fm_band = 2,
+ },
};
/* Band selection */
@@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
return;
- freq = fmdev->rx.region.bot_freq + (af * 100);
+ freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
if (freq == fmdev->rx.freq) {
fmdbg("Current freq(%d) is matching with received AF(%d)\n",
fmdev->rx.freq, freq);
@@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
fmdev->rx.rds.flag = FM_RDS_DISABLE;
fmdev->rx.freq = FM_UNDEFINED_FREQ;
fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
- fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
+ fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
fmdev->irq_info.retry = 0;
fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
index 196ff7d..aaa54bc 100644
--- a/drivers/media/radio/wl128x/fmdrv_common.h
+++ b/drivers/media/radio/wl128x/fmdrv_common.h
@@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
/* Band types */
#define FM_BAND_EUROPE_US 0
#define FM_BAND_JAPAN 1
+#define FM_BAND_RUSSIAN 2
/* Seek directions */
#define FM_SEARCH_DIRECTION_DOWN 0
@@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
/* Min and Max volume */
#define FM_RX_VOLUME_MIN 0
-#define FM_RX_VOLUME_MAX 70
-
-/* Volume gain step */
-#define FM_RX_VOLUME_GAIN_STEP 0x370
+#define FM_RX_VOLUME_MAX 0xffff
/* Mute modes */
-#define FM_MUTE_ON 0
-#define FM_MUTE_OFF 1
+#define FM_MUTE_OFF 0
+#define FM_MUTE_ON 1
#define FM_MUTE_ATTENUATE 2
#define FM_RX_UNMUTE_MODE 0x00
@@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
#define FM_RX_RF_DEPENDENT_MUTE_OFF 0
/* RSSI threshold min and max */
-#define FM_RX_RSSI_THRESHOLD_MIN -128
-#define FM_RX_RSSI_THRESHOLD_MAX 127
+#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
+#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
/* Stereo/Mono mode */
#define FM_STEREO_MODE 0
@@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
* Default RX mode configuration. Chip will be configured
* with this default values after loading RX firmware.
*/
-#define FM_DEFAULT_RX_VOLUME 10
-#define FM_DEFAULT_RSSI_THRESHOLD 20
+#define FM_DEFAULT_RX_VOLUME 10000
+#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
/* Range for TX power level in units for dB/uV */
#define FM_PWR_LVL_LOW 91
@@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
#define FM_CHANNEL_SPACING_200KHZ 4
#define FM_FREQ_MUL 50
+#define FM_US_BAND_LOW 87500
+#define FM_US_BAND_HIGH 180000
+#define FM_JAPAN_BAND_LOW 76000
+#define FM_JAPAN_BAND_HIGH 90000
+
+#define FM_KHZ 100
+
+
#endif
diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
index a806bda..0965270 100644
--- a/drivers/media/radio/wl128x/fmdrv_rx.c
+++ b/drivers/media/radio/wl128x/fmdrv_rx.c
@@ -276,6 +276,10 @@ again:
/* Calculate frequency index to write */
next_frq = (fmdev->rx.freq -
fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
+
+ /* If no valid chanel then report default frequency */
+ wrap_around = 0;
+
goto again;
}
} else {
@@ -290,6 +294,7 @@ again:
((u32)curr_frq * FM_FREQ_MUL));
}
+
/* Reset RDS cache and current station pointers */
fm_rx_reset_rds_cache(fmdev);
fm_rx_reset_station_info(fmdev);
@@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
return -EINVAL;
}
- vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
payload = vol_to_set;
ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
@@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
return -ENOMEM;
}
- *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
+ *curr_vol = fmdev->rx.volume;
return 0;
}
@@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
int ret;
if (region_to_set != FM_BAND_EUROPE_US &&
- region_to_set != FM_BAND_JAPAN) {
+ region_to_set != FM_BAND_JAPAN &&
+ region_to_set != FM_BAND_RUSSIAN) {
fmerr("Invalid band\n");
return -EINVAL;
}
@@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
fmerr("Invalid RSSI threshold level\n");
return -EINVAL;
}
- payload = (u16)rssi_lvl_toset;
+ payload = (u16) rssi_lvl_toset;
ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
sizeof(payload), NULL, NULL);
if (ret < 0)
diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
index 6d879fb..52e5120 100644
--- a/drivers/media/radio/wl128x/fmdrv_tx.c
+++ b/drivers/media/radio/wl128x/fmdrv_tx.c
@@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
fmdbg("AF: %d\n", af);
- af = (af - 87500) / 100;
+ fmdev->tx_data.af_frq = af;
+ af = (af - FM_US_BAND_LOW) / FM_KHZ;
payload = (u16)af;
- ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
- sizeof(payload), NULL, NULL);
+ ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
if (ret < 0)
return ret;
diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
index b9da1ae..4bd7bf1 100644
--- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
+++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
@@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
return 0;
}
+/**********************************************************************/
+/* functions called from sysfs subsystem */
+
+static ssize_t show_fmtx_af(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
+}
+
+static ssize_t store_fmtx_af(struct device *dev,
+ struct device_attribute *attr, char *buf, size_t size)
+{
+ int ret;
+ unsigned long af_freq;
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 0, &af_freq))
+ return -EINVAL;
+
+ ret = fm_tx_set_af(fmdev, af_freq);
+ if (ret < 0) {
+ fmerr("Failed to set FM TX AF Frequency\n");
+ return ret;
+ }
+ return size;
+}
+
+static ssize_t show_fmrx_deemphasis(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
+ FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
+}
+
+static ssize_t store_fmrx_deemphasis(struct device *dev,
+ struct device_attribute *attr, char *buf, size_t size)
+{
+ int ret;
+ unsigned long deemph_mode;
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 0, &deemph_mode))
+ return -EINVAL;
+
+ if (deemph_mode != 50 && deemph_mode != 75)
+ return -EINVAL;
+
+ if (deemph_mode == 50)
+ deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
+ else
+ deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
+
+ ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
+ if (ret < 0) {
+ fmerr("Failed to set De-emphasis Mode\n");
+ return ret;
+ }
+
+ return size;
+}
+
+static ssize_t show_fmrx_af(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fmdev->rx.af_mode);
+}
+
+static ssize_t store_fmrx_af(struct device *dev,
+ struct device_attribute *attr, char *buf, size_t size)
+{
+ int ret;
+ unsigned long af_mode;
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 0, &af_mode))
+ return -EINVAL;
+
+ if (af_mode < 0 || af_mode > 1)
+ return -EINVAL;
+
+ ret = fm_rx_set_af_switch(fmdev, af_mode);
+ if (ret < 0) {
+ fmerr("Failed to set AF Switch\n");
+ return ret;
+ }
+
+ return size;
+}
+
+static ssize_t show_fmrx_band(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
+}
+
+static ssize_t store_fmrx_band(struct device *dev,
+ struct device_attribute *attr, char *buf, size_t size)
+{
+ int ret;
+ unsigned long fm_band;
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 0, &fm_band))
+ return -EINVAL;
+
+ if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
+ return -EINVAL;
+
+ ret = fm_rx_set_region(fmdev, fm_band);
+ if (ret < 0) {
+ fmerr("Failed to set FM Band\n");
+ return ret;
+ }
+
+ return size;
+}
+
+static ssize_t show_fmrx_rssi_lvl(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
+}
+static ssize_t store_fmrx_rssi_lvl(struct device *dev,
+ struct device_attribute *attr, char *buf, size_t size)
+{
+ int ret;
+ unsigned long rssi_lvl;
+ struct fmdev *fmdev = dev_get_drvdata(dev);
+
+ if (kstrtoul(buf, 0, &rssi_lvl))
+ return -EINVAL;
+
+ ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
+ if (ret < 0) {
+ fmerr("Failed to set RSSI level\n");
+ return ret;
+ }
+
+ return size;
+}
+
+/* structures specific for sysfs entries */
+static struct kobj_attribute v4l2_fmtx_rds_af =
+__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
+
+static struct kobj_attribute v4l2_fm_deemph_mode =
+__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
+
+static struct kobj_attribute v4l2_fm_rds_af =
+__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
+
+static struct kobj_attribute v4l2_fm_band =
+__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
+
+static struct kobj_attribute v4l2_fm_rssi_lvl =
+__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
+
+static struct attribute *v4l2_fm_attrs[] = {
+ &v4l2_fmtx_rds_af.attr,
+ &v4l2_fm_deemph_mode.attr,
+ &v4l2_fm_rds_af.attr,
+ &v4l2_fm_band.attr,
+ &v4l2_fm_rssi_lvl.attr,
+ NULL,
+};
+static struct attribute_group v4l2_fm_attr_grp = {
+ .attrs = v4l2_fm_attrs,
+};
/*
* Handle open request for "/dev/radioX" device.
* Start with FM RX mode as default.
@@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
}
radio_disconnected = 1;
+ /* Register sysfs entries */
+ ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
+ if (ret) {
+ pr_err("failed to create sysfs entries");
+ return ret;
+ }
return ret;
}
@@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
return ret;
}
+ sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
+
ret = fmc_release(fmdev);
if (ret < 0) {
fmerr("FM CORE release failed\n");
@@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
- V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
+ V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-24 20:14 ` [PATCH 3/3] wl128x: Add sysfs based support for FM features manjunatha_halli
@ 2012-02-25 8:16 ` Hans Verkuil
2012-02-27 16:29 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-02-25 8:16 UTC (permalink / raw)
To: manjunatha_halli; +Cc: linux-media, shahed, linux-kernel, Manjunatha Halli
Hi Manjunatha!
On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
> From: Manjunatha Halli <x0130808@ti.com>
>
> This patch adds support for below features via sysfs file system
>
> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> 2) FM RX RDS AF turn ON/OFF
> 3) FM RX RSSI level set/get
> 4) FM TX Alternate Frequency set/get
> 5) FM RX De-Emphasis mode set/get
Why sysfs? Wouldn't it make more sense to create controls for this?
That's the V4L way...
Regards,
Hans
>
> Also this patch fixes below issues
>
> 1) FM audio volume gain setting
> 2) Default rssi level is set to 8 instead of 20
> 3) Issue related to audio mute/unmute
> 4)Enable FM RX AF support in driver
> 5)In wrap_around seek mode try for once
>
> Signed-off-by: Manjunatha Halli <x0130808@ti.com>
> ---
> drivers/media/radio/wl128x/fmdrv.h | 1 +
> drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
> drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
> drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
> drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
> drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
> 6 files changed, 225 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
> index d84ad9d..8fe773d 100644
> --- a/drivers/media/radio/wl128x/fmdrv.h
> +++ b/drivers/media/radio/wl128x/fmdrv.h
> @@ -196,6 +196,7 @@ struct fmtx_data {
> u16 aud_mode;
> u32 preemph;
> u32 tx_frq;
> + u32 af_frq;
> struct tx_rds rds;
> };
>
> diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
> index fcce61a..1a580a7 100644
> --- a/drivers/media/radio/wl128x/fmdrv_common.c
> +++ b/drivers/media/radio/wl128x/fmdrv_common.c
> @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
> .top_freq = 90000, /* 90 MHz */
> .fm_band = 1,
> },
> + /* Russian (OIRT) band */
> + {
> + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
> + .bot_freq = 65800, /* 65.8 MHz */
> + .top_freq = 74000, /* 74 MHz */
> + .fm_band = 2,
> + },
> };
>
> /* Band selection */
> @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
> if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
> return;
>
> - freq = fmdev->rx.region.bot_freq + (af * 100);
> + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
> if (freq == fmdev->rx.freq) {
> fmdbg("Current freq(%d) is matching with received AF(%d)\n",
> fmdev->rx.freq, freq);
> @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
> fmdev->rx.rds.flag = FM_RDS_DISABLE;
> fmdev->rx.freq = FM_UNDEFINED_FREQ;
> fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
> - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
> + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
> fmdev->irq_info.retry = 0;
>
> fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
> diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
> index 196ff7d..aaa54bc 100644
> --- a/drivers/media/radio/wl128x/fmdrv_common.h
> +++ b/drivers/media/radio/wl128x/fmdrv_common.h
> @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
> /* Band types */
> #define FM_BAND_EUROPE_US 0
> #define FM_BAND_JAPAN 1
> +#define FM_BAND_RUSSIAN 2
>
> /* Seek directions */
> #define FM_SEARCH_DIRECTION_DOWN 0
> @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
>
> /* Min and Max volume */
> #define FM_RX_VOLUME_MIN 0
> -#define FM_RX_VOLUME_MAX 70
> -
> -/* Volume gain step */
> -#define FM_RX_VOLUME_GAIN_STEP 0x370
> +#define FM_RX_VOLUME_MAX 0xffff
>
> /* Mute modes */
> -#define FM_MUTE_ON 0
> -#define FM_MUTE_OFF 1
> +#define FM_MUTE_OFF 0
> +#define FM_MUTE_ON 1
> #define FM_MUTE_ATTENUATE 2
>
> #define FM_RX_UNMUTE_MODE 0x00
> @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
> #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
>
> /* RSSI threshold min and max */
> -#define FM_RX_RSSI_THRESHOLD_MIN -128
> -#define FM_RX_RSSI_THRESHOLD_MAX 127
> +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
> +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
>
> /* Stereo/Mono mode */
> #define FM_STEREO_MODE 0
> @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
> * Default RX mode configuration. Chip will be configured
> * with this default values after loading RX firmware.
> */
> -#define FM_DEFAULT_RX_VOLUME 10
> -#define FM_DEFAULT_RSSI_THRESHOLD 20
> +#define FM_DEFAULT_RX_VOLUME 10000
> +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
>
> /* Range for TX power level in units for dB/uV */
> #define FM_PWR_LVL_LOW 91
> @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
> #define FM_CHANNEL_SPACING_200KHZ 4
> #define FM_FREQ_MUL 50
>
> +#define FM_US_BAND_LOW 87500
> +#define FM_US_BAND_HIGH 180000
> +#define FM_JAPAN_BAND_LOW 76000
> +#define FM_JAPAN_BAND_HIGH 90000
> +
> +#define FM_KHZ 100
> +
> +
> #endif
>
> diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
> index a806bda..0965270 100644
> --- a/drivers/media/radio/wl128x/fmdrv_rx.c
> +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
> @@ -276,6 +276,10 @@ again:
> /* Calculate frequency index to write */
> next_frq = (fmdev->rx.freq -
> fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
> +
> + /* If no valid chanel then report default frequency */
> + wrap_around = 0;
> +
> goto again;
> }
> } else {
> @@ -290,6 +294,7 @@ again:
> ((u32)curr_frq * FM_FREQ_MUL));
>
> }
> +
> /* Reset RDS cache and current station pointers */
> fm_rx_reset_rds_cache(fmdev);
> fm_rx_reset_station_info(fmdev);
> @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
> FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
> return -EINVAL;
> }
> - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
>
> payload = vol_to_set;
> ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
> @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
> return -ENOMEM;
> }
>
> - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
> + *curr_vol = fmdev->rx.volume;
>
> return 0;
> }
> @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
> int ret;
>
> if (region_to_set != FM_BAND_EUROPE_US &&
> - region_to_set != FM_BAND_JAPAN) {
> + region_to_set != FM_BAND_JAPAN &&
> + region_to_set != FM_BAND_RUSSIAN) {
> fmerr("Invalid band\n");
> return -EINVAL;
> }
> @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
> fmerr("Invalid RSSI threshold level\n");
> return -EINVAL;
> }
> - payload = (u16)rssi_lvl_toset;
> + payload = (u16) rssi_lvl_toset;
> ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
> sizeof(payload), NULL, NULL);
> if (ret < 0)
> diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
> index 6d879fb..52e5120 100644
> --- a/drivers/media/radio/wl128x/fmdrv_tx.c
> +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
> @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
>
> fmdbg("AF: %d\n", af);
>
> - af = (af - 87500) / 100;
> + fmdev->tx_data.af_frq = af;
> + af = (af - FM_US_BAND_LOW) / FM_KHZ;
> payload = (u16)af;
> - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
> - sizeof(payload), NULL, NULL);
> + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> index b9da1ae..4bd7bf1 100644
> --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
> +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
> return 0;
> }
>
> +/**********************************************************************/
> +/* functions called from sysfs subsystem */
> +
> +static ssize_t show_fmtx_af(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
> +}
> +
> +static ssize_t store_fmtx_af(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t size)
> +{
> + int ret;
> + unsigned long af_freq;
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 0, &af_freq))
> + return -EINVAL;
> +
> + ret = fm_tx_set_af(fmdev, af_freq);
> + if (ret < 0) {
> + fmerr("Failed to set FM TX AF Frequency\n");
> + return ret;
> + }
> + return size;
> +}
> +
> +static ssize_t show_fmrx_deemphasis(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
> + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
> +}
> +
> +static ssize_t store_fmrx_deemphasis(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t size)
> +{
> + int ret;
> + unsigned long deemph_mode;
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 0, &deemph_mode))
> + return -EINVAL;
> +
> + if (deemph_mode != 50 && deemph_mode != 75)
> + return -EINVAL;
> +
> + if (deemph_mode == 50)
> + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
> + else
> + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
> +
> + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
> + if (ret < 0) {
> + fmerr("Failed to set De-emphasis Mode\n");
> + return ret;
> + }
> +
> + return size;
> +}
> +
> +static ssize_t show_fmrx_af(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
> +}
> +
> +static ssize_t store_fmrx_af(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t size)
> +{
> + int ret;
> + unsigned long af_mode;
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 0, &af_mode))
> + return -EINVAL;
> +
> + if (af_mode < 0 || af_mode > 1)
> + return -EINVAL;
> +
> + ret = fm_rx_set_af_switch(fmdev, af_mode);
> + if (ret < 0) {
> + fmerr("Failed to set AF Switch\n");
> + return ret;
> + }
> +
> + return size;
> +}
> +
> +static ssize_t show_fmrx_band(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
> +}
> +
> +static ssize_t store_fmrx_band(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t size)
> +{
> + int ret;
> + unsigned long fm_band;
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 0, &fm_band))
> + return -EINVAL;
> +
> + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
> + return -EINVAL;
> +
> + ret = fm_rx_set_region(fmdev, fm_band);
> + if (ret < 0) {
> + fmerr("Failed to set FM Band\n");
> + return ret;
> + }
> +
> + return size;
> +}
> +
> +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
> +}
> +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
> + struct device_attribute *attr, char *buf, size_t size)
> +{
> + int ret;
> + unsigned long rssi_lvl;
> + struct fmdev *fmdev = dev_get_drvdata(dev);
> +
> + if (kstrtoul(buf, 0, &rssi_lvl))
> + return -EINVAL;
> +
> + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
> + if (ret < 0) {
> + fmerr("Failed to set RSSI level\n");
> + return ret;
> + }
> +
> + return size;
> +}
> +
> +/* structures specific for sysfs entries */
> +static struct kobj_attribute v4l2_fmtx_rds_af =
> +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
> +
> +static struct kobj_attribute v4l2_fm_deemph_mode =
> +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
> +
> +static struct kobj_attribute v4l2_fm_rds_af =
> +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
> +
> +static struct kobj_attribute v4l2_fm_band =
> +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
> +
> +static struct kobj_attribute v4l2_fm_rssi_lvl =
> +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
> +
> +static struct attribute *v4l2_fm_attrs[] = {
> + &v4l2_fmtx_rds_af.attr,
> + &v4l2_fm_deemph_mode.attr,
> + &v4l2_fm_rds_af.attr,
> + &v4l2_fm_band.attr,
> + &v4l2_fm_rssi_lvl.attr,
> + NULL,
> +};
> +static struct attribute_group v4l2_fm_attr_grp = {
> + .attrs = v4l2_fm_attrs,
> +};
> /*
> * Handle open request for "/dev/radioX" device.
> * Start with FM RX mode as default.
> @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
> }
> radio_disconnected = 1;
>
> + /* Register sysfs entries */
> + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> + if (ret) {
> + pr_err("failed to create sysfs entries");
> + return ret;
> + }
> return ret;
> }
>
> @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
> return ret;
> }
>
> + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> +
> ret = fmc_release(fmdev);
> if (ret < 0) {
> fmerr("FM CORE release failed\n");
> @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
> FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
>
> v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
>
> v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-25 8:16 ` Hans Verkuil
@ 2012-02-27 16:29 ` halli manjunatha
2012-02-28 10:05 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-02-27 16:29 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
Hi Hans,
Agreed I don't mind to create new controls for below things
1) FM RX Band selection (US/Europe, Japan and Russian bands)
2) FM RX RDS AF turn ON/OFF
3) FM RX RSSI level set/get
4) FM TX Alternate Frequency set/get
5) FM RX De-Emphasis mode set/get
However, previously you have suggested me to hide few controls (like
band selection) from the user but few of our application wanted
controls like above and that is why I have created the sysfs entries.
Please suggest me how can I move forward with new controls or with sysfs?
Regards
Manju
On Sat, Feb 25, 2012 at 2:16 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Manjunatha!
>
> On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
> > From: Manjunatha Halli <x0130808@ti.com>
> >
> > This patch adds support for below features via sysfs file system
> >
> > 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> > 2) FM RX RDS AF turn ON/OFF
> > 3) FM RX RSSI level set/get
> > 4) FM TX Alternate Frequency set/get
> > 5) FM RX De-Emphasis mode set/get
>
> Why sysfs? Wouldn't it make more sense to create controls for this?
> That's the V4L way...
>
> Regards,
>
> Hans
>
> >
> > Also this patch fixes below issues
> >
> > 1) FM audio volume gain setting
> > 2) Default rssi level is set to 8 instead of 20
> > 3) Issue related to audio mute/unmute
> > 4)Enable FM RX AF support in driver
> > 5)In wrap_around seek mode try for once
> >
> > Signed-off-by: Manjunatha Halli <x0130808@ti.com>
> > ---
> > drivers/media/radio/wl128x/fmdrv.h | 1 +
> > drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
> > drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
> > drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
> > drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
> > drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
> > 6 files changed, 225 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
> > index d84ad9d..8fe773d 100644
> > --- a/drivers/media/radio/wl128x/fmdrv.h
> > +++ b/drivers/media/radio/wl128x/fmdrv.h
> > @@ -196,6 +196,7 @@ struct fmtx_data {
> > u16 aud_mode;
> > u32 preemph;
> > u32 tx_frq;
> > + u32 af_frq;
> > struct tx_rds rds;
> > };
> >
> > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
> > index fcce61a..1a580a7 100644
> > --- a/drivers/media/radio/wl128x/fmdrv_common.c
> > +++ b/drivers/media/radio/wl128x/fmdrv_common.c
> > @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
> > .top_freq = 90000, /* 90 MHz */
> > .fm_band = 1,
> > },
> > + /* Russian (OIRT) band */
> > + {
> > + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
> > + .bot_freq = 65800, /* 65.8 MHz */
> > + .top_freq = 74000, /* 74 MHz */
> > + .fm_band = 2,
> > + },
> > };
> >
> > /* Band selection */
> > @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
> > if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
> > return;
> >
> > - freq = fmdev->rx.region.bot_freq + (af * 100);
> > + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
> > if (freq == fmdev->rx.freq) {
> > fmdbg("Current freq(%d) is matching with received AF(%d)\n",
> > fmdev->rx.freq, freq);
> > @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
> > fmdev->rx.rds.flag = FM_RDS_DISABLE;
> > fmdev->rx.freq = FM_UNDEFINED_FREQ;
> > fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
> > - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
> > + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
> > fmdev->irq_info.retry = 0;
> >
> > fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
> > diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
> > index 196ff7d..aaa54bc 100644
> > --- a/drivers/media/radio/wl128x/fmdrv_common.h
> > +++ b/drivers/media/radio/wl128x/fmdrv_common.h
> > @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
> > /* Band types */
> > #define FM_BAND_EUROPE_US 0
> > #define FM_BAND_JAPAN 1
> > +#define FM_BAND_RUSSIAN 2
> >
> > /* Seek directions */
> > #define FM_SEARCH_DIRECTION_DOWN 0
> > @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
> >
> > /* Min and Max volume */
> > #define FM_RX_VOLUME_MIN 0
> > -#define FM_RX_VOLUME_MAX 70
> > -
> > -/* Volume gain step */
> > -#define FM_RX_VOLUME_GAIN_STEP 0x370
> > +#define FM_RX_VOLUME_MAX 0xffff
> >
> > /* Mute modes */
> > -#define FM_MUTE_ON 0
> > -#define FM_MUTE_OFF 1
> > +#define FM_MUTE_OFF 0
> > +#define FM_MUTE_ON 1
> > #define FM_MUTE_ATTENUATE 2
> >
> > #define FM_RX_UNMUTE_MODE 0x00
> > @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
> > #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
> >
> > /* RSSI threshold min and max */
> > -#define FM_RX_RSSI_THRESHOLD_MIN -128
> > -#define FM_RX_RSSI_THRESHOLD_MAX 127
> > +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
> > +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
> >
> > /* Stereo/Mono mode */
> > #define FM_STEREO_MODE 0
> > @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
> > * Default RX mode configuration. Chip will be configured
> > * with this default values after loading RX firmware.
> > */
> > -#define FM_DEFAULT_RX_VOLUME 10
> > -#define FM_DEFAULT_RSSI_THRESHOLD 20
> > +#define FM_DEFAULT_RX_VOLUME 10000
> > +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
> >
> > /* Range for TX power level in units for dB/uV */
> > #define FM_PWR_LVL_LOW 91
> > @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
> > #define FM_CHANNEL_SPACING_200KHZ 4
> > #define FM_FREQ_MUL 50
> >
> > +#define FM_US_BAND_LOW 87500
> > +#define FM_US_BAND_HIGH 180000
> > +#define FM_JAPAN_BAND_LOW 76000
> > +#define FM_JAPAN_BAND_HIGH 90000
> > +
> > +#define FM_KHZ 100
> > +
> > +
> > #endif
> >
> > diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
> > index a806bda..0965270 100644
> > --- a/drivers/media/radio/wl128x/fmdrv_rx.c
> > +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
> > @@ -276,6 +276,10 @@ again:
> > /* Calculate frequency index to write */
> > next_frq = (fmdev->rx.freq -
> > fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
> > +
> > + /* If no valid chanel then report default frequency */
> > + wrap_around = 0;
> > +
> > goto again;
> > }
> > } else {
> > @@ -290,6 +294,7 @@ again:
> > ((u32)curr_frq * FM_FREQ_MUL));
> >
> > }
> > +
> > /* Reset RDS cache and current station pointers */
> > fm_rx_reset_rds_cache(fmdev);
> > fm_rx_reset_station_info(fmdev);
> > @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
> > FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
> > return -EINVAL;
> > }
> > - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
> >
> > payload = vol_to_set;
> > ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
> > @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
> > return -ENOMEM;
> > }
> >
> > - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
> > + *curr_vol = fmdev->rx.volume;
> >
> > return 0;
> > }
> > @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
> > int ret;
> >
> > if (region_to_set != FM_BAND_EUROPE_US &&
> > - region_to_set != FM_BAND_JAPAN) {
> > + region_to_set != FM_BAND_JAPAN &&
> > + region_to_set != FM_BAND_RUSSIAN) {
> > fmerr("Invalid band\n");
> > return -EINVAL;
> > }
> > @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
> > fmerr("Invalid RSSI threshold level\n");
> > return -EINVAL;
> > }
> > - payload = (u16)rssi_lvl_toset;
> > + payload = (u16) rssi_lvl_toset;
> > ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
> > sizeof(payload), NULL, NULL);
> > if (ret < 0)
> > diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
> > index 6d879fb..52e5120 100644
> > --- a/drivers/media/radio/wl128x/fmdrv_tx.c
> > +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
> > @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
> >
> > fmdbg("AF: %d\n", af);
> >
> > - af = (af - 87500) / 100;
> > + fmdev->tx_data.af_frq = af;
> > + af = (af - FM_US_BAND_LOW) / FM_KHZ;
> > payload = (u16)af;
> > - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
> > - sizeof(payload), NULL, NULL);
> > + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
> > if (ret < 0)
> > return ret;
> >
> > diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > index b9da1ae..4bd7bf1 100644
> > --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
> > return 0;
> > }
> >
> > +/**********************************************************************/
> > +/* functions called from sysfs subsystem */
> > +
> > +static ssize_t show_fmtx_af(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
> > +}
> > +
> > +static ssize_t store_fmtx_af(struct device *dev,
> > + struct device_attribute *attr, char *buf, size_t size)
> > +{
> > + int ret;
> > + unsigned long af_freq;
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + if (kstrtoul(buf, 0, &af_freq))
> > + return -EINVAL;
> > +
> > + ret = fm_tx_set_af(fmdev, af_freq);
> > + if (ret < 0) {
> > + fmerr("Failed to set FM TX AF Frequency\n");
> > + return ret;
> > + }
> > + return size;
> > +}
> > +
> > +static ssize_t show_fmrx_deemphasis(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
> > + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
> > +}
> > +
> > +static ssize_t store_fmrx_deemphasis(struct device *dev,
> > + struct device_attribute *attr, char *buf, size_t size)
> > +{
> > + int ret;
> > + unsigned long deemph_mode;
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + if (kstrtoul(buf, 0, &deemph_mode))
> > + return -EINVAL;
> > +
> > + if (deemph_mode != 50 && deemph_mode != 75)
> > + return -EINVAL;
> > +
> > + if (deemph_mode == 50)
> > + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
> > + else
> > + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
> > +
> > + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
> > + if (ret < 0) {
> > + fmerr("Failed to set De-emphasis Mode\n");
> > + return ret;
> > + }
> > +
> > + return size;
> > +}
> > +
> > +static ssize_t show_fmrx_af(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
> > +}
> > +
> > +static ssize_t store_fmrx_af(struct device *dev,
> > + struct device_attribute *attr, char *buf, size_t size)
> > +{
> > + int ret;
> > + unsigned long af_mode;
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + if (kstrtoul(buf, 0, &af_mode))
> > + return -EINVAL;
> > +
> > + if (af_mode < 0 || af_mode > 1)
> > + return -EINVAL;
> > +
> > + ret = fm_rx_set_af_switch(fmdev, af_mode);
> > + if (ret < 0) {
> > + fmerr("Failed to set AF Switch\n");
> > + return ret;
> > + }
> > +
> > + return size;
> > +}
> > +
> > +static ssize_t show_fmrx_band(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
> > +}
> > +
> > +static ssize_t store_fmrx_band(struct device *dev,
> > + struct device_attribute *attr, char *buf, size_t size)
> > +{
> > + int ret;
> > + unsigned long fm_band;
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + if (kstrtoul(buf, 0, &fm_band))
> > + return -EINVAL;
> > +
> > + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
> > + return -EINVAL;
> > +
> > + ret = fm_rx_set_region(fmdev, fm_band);
> > + if (ret < 0) {
> > + fmerr("Failed to set FM Band\n");
> > + return ret;
> > + }
> > +
> > + return size;
> > +}
> > +
> > +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
> > +}
> > +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
> > + struct device_attribute *attr, char *buf, size_t size)
> > +{
> > + int ret;
> > + unsigned long rssi_lvl;
> > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > +
> > + if (kstrtoul(buf, 0, &rssi_lvl))
> > + return -EINVAL;
> > +
> > + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
> > + if (ret < 0) {
> > + fmerr("Failed to set RSSI level\n");
> > + return ret;
> > + }
> > +
> > + return size;
> > +}
> > +
> > +/* structures specific for sysfs entries */
> > +static struct kobj_attribute v4l2_fmtx_rds_af =
> > +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
> > +
> > +static struct kobj_attribute v4l2_fm_deemph_mode =
> > +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
> > +
> > +static struct kobj_attribute v4l2_fm_rds_af =
> > +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
> > +
> > +static struct kobj_attribute v4l2_fm_band =
> > +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
> > +
> > +static struct kobj_attribute v4l2_fm_rssi_lvl =
> > +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
> > +
> > +static struct attribute *v4l2_fm_attrs[] = {
> > + &v4l2_fmtx_rds_af.attr,
> > + &v4l2_fm_deemph_mode.attr,
> > + &v4l2_fm_rds_af.attr,
> > + &v4l2_fm_band.attr,
> > + &v4l2_fm_rssi_lvl.attr,
> > + NULL,
> > +};
> > +static struct attribute_group v4l2_fm_attr_grp = {
> > + .attrs = v4l2_fm_attrs,
> > +};
> > /*
> > * Handle open request for "/dev/radioX" device.
> > * Start with FM RX mode as default.
> > @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
> > }
> > radio_disconnected = 1;
> >
> > + /* Register sysfs entries */
> > + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> > + if (ret) {
> > + pr_err("failed to create sysfs entries");
> > + return ret;
> > + }
> > return ret;
> > }
> >
> > @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
> > return ret;
> > }
> >
> > + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> > +
> > ret = fmc_release(fmdev);
> > if (ret < 0) {
> > fmerr("FM CORE release failed\n");
> > @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
> > FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
> >
> > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> > - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> > + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
> >
> > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> > V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
> >
> --
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-27 16:29 ` halli manjunatha
@ 2012-02-28 10:05 ` Hans Verkuil
2012-02-28 22:52 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-02-28 10:05 UTC (permalink / raw)
To: halli manjunatha; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
> Hi Hans,
>
> Agreed I don't mind to create new controls for below things
>
> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> 2) FM RX RDS AF turn ON/OFF
> 3) FM RX RSSI level set/get
> 4) FM TX Alternate Frequency set/get
> 5) FM RX De-Emphasis mode set/get
>
> However, previously you have suggested me to hide few controls (like
> band selection) from the user but few of our application wanted
> controls like above and that is why I have created the sysfs entries.
>
> Please suggest me how can I move forward with new controls or with sysfs?
The first question is which of these controls are general to FM receivers or
transmitters, and which are specific to this chipset. The chipset specific
controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
videodev2.h how those are defined). The others should be defined as new
controls of the FM_TX class or the FM_RX class. The FM_RX class should be
defined as a new class as it doesn't exist at the moment. Don't forget to
document these controls clearly.
With regards to the band selection: I remember that there was a discussion,
but not the details. Do you have a link to that discussion? I can't find it
(at least not quickly :-) ).
There is one other thing that would be nice if that could be changed in the
driver: currently there is only one radio device that can be used for both
the receiver and the transmitter. During a V4L meeting last year we had a
discussion about that and the conclusion was that receivers and transmitters
should have their own radio device.
The reason for that decision was that the VIDIOC_G/S_FREQUENCY ioctls cannot
tell the difference between a transmitter and a receiver. If you have separate
receiver and transmitter radio nodes this confusion goes away. In addition,
this same restriction already applies to video and vbi nodes (i.e. a video or
vbi node can be used for capture or output, but not both, unless it is a
mem2mem device).
I don't know how much work it is to implement this, but I would appreciate it
if you could think about this a bit.
Regards,
Hans
>
> Regards
> Manju
>
>
> On Sat, Feb 25, 2012 at 2:16 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > Hi Manjunatha!
> >
> > On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
> > > From: Manjunatha Halli <x0130808@ti.com>
> > >
> > > This patch adds support for below features via sysfs file system
> > >
> > > 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> > > 2) FM RX RDS AF turn ON/OFF
> > > 3) FM RX RSSI level set/get
> > > 4) FM TX Alternate Frequency set/get
> > > 5) FM RX De-Emphasis mode set/get
> >
> > Why sysfs? Wouldn't it make more sense to create controls for this?
> > That's the V4L way...
> >
> > Regards,
> >
> > Hans
> >
> > >
> > > Also this patch fixes below issues
> > >
> > > 1) FM audio volume gain setting
> > > 2) Default rssi level is set to 8 instead of 20
> > > 3) Issue related to audio mute/unmute
> > > 4)Enable FM RX AF support in driver
> > > 5)In wrap_around seek mode try for once
> > >
> > > Signed-off-by: Manjunatha Halli <x0130808@ti.com>
> > > ---
> > > drivers/media/radio/wl128x/fmdrv.h | 1 +
> > > drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
> > > drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
> > > drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
> > > drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
> > > drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
> > > 6 files changed, 225 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
> > > index d84ad9d..8fe773d 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv.h
> > > +++ b/drivers/media/radio/wl128x/fmdrv.h
> > > @@ -196,6 +196,7 @@ struct fmtx_data {
> > > u16 aud_mode;
> > > u32 preemph;
> > > u32 tx_frq;
> > > + u32 af_frq;
> > > struct tx_rds rds;
> > > };
> > >
> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
> > > index fcce61a..1a580a7 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv_common.c
> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.c
> > > @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
> > > .top_freq = 90000, /* 90 MHz */
> > > .fm_band = 1,
> > > },
> > > + /* Russian (OIRT) band */
> > > + {
> > > + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
> > > + .bot_freq = 65800, /* 65.8 MHz */
> > > + .top_freq = 74000, /* 74 MHz */
> > > + .fm_band = 2,
> > > + },
> > > };
> > >
> > > /* Band selection */
> > > @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
> > > if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
> > > return;
> > >
> > > - freq = fmdev->rx.region.bot_freq + (af * 100);
> > > + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
> > > if (freq == fmdev->rx.freq) {
> > > fmdbg("Current freq(%d) is matching with received AF(%d)\n",
> > > fmdev->rx.freq, freq);
> > > @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
> > > fmdev->rx.rds.flag = FM_RDS_DISABLE;
> > > fmdev->rx.freq = FM_UNDEFINED_FREQ;
> > > fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
> > > - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
> > > + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
> > > fmdev->irq_info.retry = 0;
> > >
> > > fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
> > > index 196ff7d..aaa54bc 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv_common.h
> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.h
> > > @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
> > > /* Band types */
> > > #define FM_BAND_EUROPE_US 0
> > > #define FM_BAND_JAPAN 1
> > > +#define FM_BAND_RUSSIAN 2
> > >
> > > /* Seek directions */
> > > #define FM_SEARCH_DIRECTION_DOWN 0
> > > @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
> > >
> > > /* Min and Max volume */
> > > #define FM_RX_VOLUME_MIN 0
> > > -#define FM_RX_VOLUME_MAX 70
> > > -
> > > -/* Volume gain step */
> > > -#define FM_RX_VOLUME_GAIN_STEP 0x370
> > > +#define FM_RX_VOLUME_MAX 0xffff
> > >
> > > /* Mute modes */
> > > -#define FM_MUTE_ON 0
> > > -#define FM_MUTE_OFF 1
> > > +#define FM_MUTE_OFF 0
> > > +#define FM_MUTE_ON 1
> > > #define FM_MUTE_ATTENUATE 2
> > >
> > > #define FM_RX_UNMUTE_MODE 0x00
> > > @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
> > > #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
> > >
> > > /* RSSI threshold min and max */
> > > -#define FM_RX_RSSI_THRESHOLD_MIN -128
> > > -#define FM_RX_RSSI_THRESHOLD_MAX 127
> > > +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
> > > +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
> > >
> > > /* Stereo/Mono mode */
> > > #define FM_STEREO_MODE 0
> > > @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
> > > * Default RX mode configuration. Chip will be configured
> > > * with this default values after loading RX firmware.
> > > */
> > > -#define FM_DEFAULT_RX_VOLUME 10
> > > -#define FM_DEFAULT_RSSI_THRESHOLD 20
> > > +#define FM_DEFAULT_RX_VOLUME 10000
> > > +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
> > >
> > > /* Range for TX power level in units for dB/uV */
> > > #define FM_PWR_LVL_LOW 91
> > > @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
> > > #define FM_CHANNEL_SPACING_200KHZ 4
> > > #define FM_FREQ_MUL 50
> > >
> > > +#define FM_US_BAND_LOW 87500
> > > +#define FM_US_BAND_HIGH 180000
> > > +#define FM_JAPAN_BAND_LOW 76000
> > > +#define FM_JAPAN_BAND_HIGH 90000
> > > +
> > > +#define FM_KHZ 100
> > > +
> > > +
> > > #endif
> > >
> > > diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
> > > index a806bda..0965270 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv_rx.c
> > > +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
> > > @@ -276,6 +276,10 @@ again:
> > > /* Calculate frequency index to write */
> > > next_frq = (fmdev->rx.freq -
> > > fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
> > > +
> > > + /* If no valid chanel then report default frequency */
> > > + wrap_around = 0;
> > > +
> > > goto again;
> > > }
> > > } else {
> > > @@ -290,6 +294,7 @@ again:
> > > ((u32)curr_frq * FM_FREQ_MUL));
> > >
> > > }
> > > +
> > > /* Reset RDS cache and current station pointers */
> > > fm_rx_reset_rds_cache(fmdev);
> > > fm_rx_reset_station_info(fmdev);
> > > @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
> > > FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
> > > return -EINVAL;
> > > }
> > > - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
> > >
> > > payload = vol_to_set;
> > > ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
> > > @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
> > > return -ENOMEM;
> > > }
> > >
> > > - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
> > > + *curr_vol = fmdev->rx.volume;
> > >
> > > return 0;
> > > }
> > > @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
> > > int ret;
> > >
> > > if (region_to_set != FM_BAND_EUROPE_US &&
> > > - region_to_set != FM_BAND_JAPAN) {
> > > + region_to_set != FM_BAND_JAPAN &&
> > > + region_to_set != FM_BAND_RUSSIAN) {
> > > fmerr("Invalid band\n");
> > > return -EINVAL;
> > > }
> > > @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
> > > fmerr("Invalid RSSI threshold level\n");
> > > return -EINVAL;
> > > }
> > > - payload = (u16)rssi_lvl_toset;
> > > + payload = (u16) rssi_lvl_toset;
> > > ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
> > > sizeof(payload), NULL, NULL);
> > > if (ret < 0)
> > > diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
> > > index 6d879fb..52e5120 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv_tx.c
> > > +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
> > > @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
> > >
> > > fmdbg("AF: %d\n", af);
> > >
> > > - af = (af - 87500) / 100;
> > > + fmdev->tx_data.af_frq = af;
> > > + af = (af - FM_US_BAND_LOW) / FM_KHZ;
> > > payload = (u16)af;
> > > - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
> > > - sizeof(payload), NULL, NULL);
> > > + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
> > > if (ret < 0)
> > > return ret;
> > >
> > > diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > > index b9da1ae..4bd7bf1 100644
> > > --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > > +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> > > @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
> > > return 0;
> > > }
> > >
> > > +/**********************************************************************/
> > > +/* functions called from sysfs subsystem */
> > > +
> > > +static ssize_t show_fmtx_af(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
> > > +}
> > > +
> > > +static ssize_t store_fmtx_af(struct device *dev,
> > > + struct device_attribute *attr, char *buf, size_t size)
> > > +{
> > > + int ret;
> > > + unsigned long af_freq;
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + if (kstrtoul(buf, 0, &af_freq))
> > > + return -EINVAL;
> > > +
> > > + ret = fm_tx_set_af(fmdev, af_freq);
> > > + if (ret < 0) {
> > > + fmerr("Failed to set FM TX AF Frequency\n");
> > > + return ret;
> > > + }
> > > + return size;
> > > +}
> > > +
> > > +static ssize_t show_fmrx_deemphasis(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
> > > + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
> > > +}
> > > +
> > > +static ssize_t store_fmrx_deemphasis(struct device *dev,
> > > + struct device_attribute *attr, char *buf, size_t size)
> > > +{
> > > + int ret;
> > > + unsigned long deemph_mode;
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + if (kstrtoul(buf, 0, &deemph_mode))
> > > + return -EINVAL;
> > > +
> > > + if (deemph_mode != 50 && deemph_mode != 75)
> > > + return -EINVAL;
> > > +
> > > + if (deemph_mode == 50)
> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
> > > + else
> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
> > > +
> > > + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
> > > + if (ret < 0) {
> > > + fmerr("Failed to set De-emphasis Mode\n");
> > > + return ret;
> > > + }
> > > +
> > > + return size;
> > > +}
> > > +
> > > +static ssize_t show_fmrx_af(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
> > > +}
> > > +
> > > +static ssize_t store_fmrx_af(struct device *dev,
> > > + struct device_attribute *attr, char *buf, size_t size)
> > > +{
> > > + int ret;
> > > + unsigned long af_mode;
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + if (kstrtoul(buf, 0, &af_mode))
> > > + return -EINVAL;
> > > +
> > > + if (af_mode < 0 || af_mode > 1)
> > > + return -EINVAL;
> > > +
> > > + ret = fm_rx_set_af_switch(fmdev, af_mode);
> > > + if (ret < 0) {
> > > + fmerr("Failed to set AF Switch\n");
> > > + return ret;
> > > + }
> > > +
> > > + return size;
> > > +}
> > > +
> > > +static ssize_t show_fmrx_band(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
> > > +}
> > > +
> > > +static ssize_t store_fmrx_band(struct device *dev,
> > > + struct device_attribute *attr, char *buf, size_t size)
> > > +{
> > > + int ret;
> > > + unsigned long fm_band;
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + if (kstrtoul(buf, 0, &fm_band))
> > > + return -EINVAL;
> > > +
> > > + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
> > > + return -EINVAL;
> > > +
> > > + ret = fm_rx_set_region(fmdev, fm_band);
> > > + if (ret < 0) {
> > > + fmerr("Failed to set FM Band\n");
> > > + return ret;
> > > + }
> > > +
> > > + return size;
> > > +}
> > > +
> > > +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
> > > + struct device_attribute *attr, char *buf)
> > > +{
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
> > > +}
> > > +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
> > > + struct device_attribute *attr, char *buf, size_t size)
> > > +{
> > > + int ret;
> > > + unsigned long rssi_lvl;
> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> > > +
> > > + if (kstrtoul(buf, 0, &rssi_lvl))
> > > + return -EINVAL;
> > > +
> > > + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
> > > + if (ret < 0) {
> > > + fmerr("Failed to set RSSI level\n");
> > > + return ret;
> > > + }
> > > +
> > > + return size;
> > > +}
> > > +
> > > +/* structures specific for sysfs entries */
> > > +static struct kobj_attribute v4l2_fmtx_rds_af =
> > > +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
> > > +
> > > +static struct kobj_attribute v4l2_fm_deemph_mode =
> > > +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
> > > +
> > > +static struct kobj_attribute v4l2_fm_rds_af =
> > > +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
> > > +
> > > +static struct kobj_attribute v4l2_fm_band =
> > > +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
> > > +
> > > +static struct kobj_attribute v4l2_fm_rssi_lvl =
> > > +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
> > > +
> > > +static struct attribute *v4l2_fm_attrs[] = {
> > > + &v4l2_fmtx_rds_af.attr,
> > > + &v4l2_fm_deemph_mode.attr,
> > > + &v4l2_fm_rds_af.attr,
> > > + &v4l2_fm_band.attr,
> > > + &v4l2_fm_rssi_lvl.attr,
> > > + NULL,
> > > +};
> > > +static struct attribute_group v4l2_fm_attr_grp = {
> > > + .attrs = v4l2_fm_attrs,
> > > +};
> > > /*
> > > * Handle open request for "/dev/radioX" device.
> > > * Start with FM RX mode as default.
> > > @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
> > > }
> > > radio_disconnected = 1;
> > >
> > > + /* Register sysfs entries */
> > > + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> > > + if (ret) {
> > > + pr_err("failed to create sysfs entries");
> > > + return ret;
> > > + }
> > > return ret;
> > > }
> > >
> > > @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
> > > return ret;
> > > }
> > >
> > > + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> > > +
> > > ret = fmc_release(fmdev);
> > > if (ret < 0) {
> > > fmerr("FM CORE release failed\n");
> > > @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
> > > FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
> > >
> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> > > - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> > > + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
> > >
> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> > > V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
> > >
> > --
> > 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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-28 10:05 ` Hans Verkuil
@ 2012-02-28 22:52 ` halli manjunatha
2012-02-29 11:25 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-02-28 22:52 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>> Hi Hans,
>>
>> Agreed I don't mind to create new controls for below things
>>
>> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> 2) FM RX RDS AF turn ON/OFF
>> 3) FM RX RSSI level set/get
>> 4) FM TX Alternate Frequency set/get
>> 5) FM RX De-Emphasis mode set/get
>>
>> However, previously you have suggested me to hide few controls (like
>> band selection) from the user but few of our application wanted
>> controls like above and that is why I have created the sysfs entries.
>>
>> Please suggest me how can I move forward with new controls or with sysfs?
>
> The first question is which of these controls are general to FM receivers or
> transmitters, and which are specific to this chipset. The chipset specific
> controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
> videodev2.h how those are defined). The others should be defined as new
> controls of the FM_TX class or the FM_RX class. The FM_RX class should be
> defined as a new class as it doesn't exist at the moment. Don't forget to
> document these controls clearly.
>
> With regards to the band selection: I remember that there was a discussion,
> but not the details. Do you have a link to that discussion? I can't find it
> (at least not quickly :-) ).
Below features are generic to all FM receivers so we can add new CID's
for below features
1) FM RX RDS AF turn ON/OFF
2) FM TX Alternate Frequency set/get
About other 3 features its different issue,
1) FM RX Band selection (US/Europe, Japan and Russian bands)
2) FM RX RSSI level set/get
3) FM RX De-Emphasis mode set/get
All these 3 features are generic to any FM receiver, only question is
does all FM receivers wants to expose these controls to application
writer?
Example Band selection, every FM receiver at the minimum supports both
Europe and Japan band, now the question is should we add a CID to
switch between these two bands?
Below is the link to our discussion on the same topic a few time back.
https://lkml.org/lkml/2010/11/18/43
>
> There is one other thing that would be nice if that could be changed in the
> driver: currently there is only one radio device that can be used for both
> the receiver and the transmitter. During a V4L meeting last year we had a
> discussion about that and the conclusion was that receivers and transmitters
> should have their own radio device.
>
> The reason for that decision was that the VIDIOC_G/S_FREQUENCY ioctls cannot
> tell the difference between a transmitter and a receiver. If you have separate
> receiver and transmitter radio nodes this confusion goes away. In addition,
> this same restriction already applies to video and vbi nodes (i.e. a video or
> vbi node can be used for capture or output, but not both, unless it is a
> mem2mem device).
>
> I don't know how much work it is to implement this, but I would appreciate it
> if you could think about this a bit.
I will look in to this and submit a new patch to create 2 separate
device nodes for FM RX and FM TX once after this patch set.
Regards
Manju
>
> Regards,
>
> Hans
>
>>
>> Regards
>> Manju
>>
>>
>> On Sat, Feb 25, 2012 at 2:16 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >
>> > Hi Manjunatha!
>> >
>> > On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
>> > > From: Manjunatha Halli <x0130808@ti.com>
>> > >
>> > > This patch adds support for below features via sysfs file system
>> > >
>> > > 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> > > 2) FM RX RDS AF turn ON/OFF
>> > > 3) FM RX RSSI level set/get
>> > > 4) FM TX Alternate Frequency set/get
>> > > 5) FM RX De-Emphasis mode set/get
>> >
>> > Why sysfs? Wouldn't it make more sense to create controls for this?
>> > That's the V4L way...
>> >
>> > Regards,
>> >
>> > Hans
>> >
>> > >
>> > > Also this patch fixes below issues
>> > >
>> > > 1) FM audio volume gain setting
>> > > 2) Default rssi level is set to 8 instead of 20
>> > > 3) Issue related to audio mute/unmute
>> > > 4)Enable FM RX AF support in driver
>> > > 5)In wrap_around seek mode try for once
>> > >
>> > > Signed-off-by: Manjunatha Halli <x0130808@ti.com>
>> > > ---
>> > > drivers/media/radio/wl128x/fmdrv.h | 1 +
>> > > drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
>> > > drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
>> > > drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
>> > > drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
>> > > drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
>> > > 6 files changed, 225 insertions(+), 20 deletions(-)
>> > >
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
>> > > index d84ad9d..8fe773d 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv.h
>> > > +++ b/drivers/media/radio/wl128x/fmdrv.h
>> > > @@ -196,6 +196,7 @@ struct fmtx_data {
>> > > u16 aud_mode;
>> > > u32 preemph;
>> > > u32 tx_frq;
>> > > + u32 af_frq;
>> > > struct tx_rds rds;
>> > > };
>> > >
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
>> > > index fcce61a..1a580a7 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv_common.c
>> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.c
>> > > @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
>> > > .top_freq = 90000, /* 90 MHz */
>> > > .fm_band = 1,
>> > > },
>> > > + /* Russian (OIRT) band */
>> > > + {
>> > > + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
>> > > + .bot_freq = 65800, /* 65.8 MHz */
>> > > + .top_freq = 74000, /* 74 MHz */
>> > > + .fm_band = 2,
>> > > + },
>> > > };
>> > >
>> > > /* Band selection */
>> > > @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
>> > > if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
>> > > return;
>> > >
>> > > - freq = fmdev->rx.region.bot_freq + (af * 100);
>> > > + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
>> > > if (freq == fmdev->rx.freq) {
>> > > fmdbg("Current freq(%d) is matching with received AF(%d)\n",
>> > > fmdev->rx.freq, freq);
>> > > @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
>> > > fmdev->rx.rds.flag = FM_RDS_DISABLE;
>> > > fmdev->rx.freq = FM_UNDEFINED_FREQ;
>> > > fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
>> > > - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
>> > > + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
>> > > fmdev->irq_info.retry = 0;
>> > >
>> > > fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
>> > > index 196ff7d..aaa54bc 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv_common.h
>> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.h
>> > > @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
>> > > /* Band types */
>> > > #define FM_BAND_EUROPE_US 0
>> > > #define FM_BAND_JAPAN 1
>> > > +#define FM_BAND_RUSSIAN 2
>> > >
>> > > /* Seek directions */
>> > > #define FM_SEARCH_DIRECTION_DOWN 0
>> > > @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
>> > >
>> > > /* Min and Max volume */
>> > > #define FM_RX_VOLUME_MIN 0
>> > > -#define FM_RX_VOLUME_MAX 70
>> > > -
>> > > -/* Volume gain step */
>> > > -#define FM_RX_VOLUME_GAIN_STEP 0x370
>> > > +#define FM_RX_VOLUME_MAX 0xffff
>> > >
>> > > /* Mute modes */
>> > > -#define FM_MUTE_ON 0
>> > > -#define FM_MUTE_OFF 1
>> > > +#define FM_MUTE_OFF 0
>> > > +#define FM_MUTE_ON 1
>> > > #define FM_MUTE_ATTENUATE 2
>> > >
>> > > #define FM_RX_UNMUTE_MODE 0x00
>> > > @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
>> > > #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
>> > >
>> > > /* RSSI threshold min and max */
>> > > -#define FM_RX_RSSI_THRESHOLD_MIN -128
>> > > -#define FM_RX_RSSI_THRESHOLD_MAX 127
>> > > +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
>> > > +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
>> > >
>> > > /* Stereo/Mono mode */
>> > > #define FM_STEREO_MODE 0
>> > > @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
>> > > * Default RX mode configuration. Chip will be configured
>> > > * with this default values after loading RX firmware.
>> > > */
>> > > -#define FM_DEFAULT_RX_VOLUME 10
>> > > -#define FM_DEFAULT_RSSI_THRESHOLD 20
>> > > +#define FM_DEFAULT_RX_VOLUME 10000
>> > > +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
>> > >
>> > > /* Range for TX power level in units for dB/uV */
>> > > #define FM_PWR_LVL_LOW 91
>> > > @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
>> > > #define FM_CHANNEL_SPACING_200KHZ 4
>> > > #define FM_FREQ_MUL 50
>> > >
>> > > +#define FM_US_BAND_LOW 87500
>> > > +#define FM_US_BAND_HIGH 180000
>> > > +#define FM_JAPAN_BAND_LOW 76000
>> > > +#define FM_JAPAN_BAND_HIGH 90000
>> > > +
>> > > +#define FM_KHZ 100
>> > > +
>> > > +
>> > > #endif
>> > >
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
>> > > index a806bda..0965270 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv_rx.c
>> > > +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
>> > > @@ -276,6 +276,10 @@ again:
>> > > /* Calculate frequency index to write */
>> > > next_frq = (fmdev->rx.freq -
>> > > fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
>> > > +
>> > > + /* If no valid chanel then report default frequency */
>> > > + wrap_around = 0;
>> > > +
>> > > goto again;
>> > > }
>> > > } else {
>> > > @@ -290,6 +294,7 @@ again:
>> > > ((u32)curr_frq * FM_FREQ_MUL));
>> > >
>> > > }
>> > > +
>> > > /* Reset RDS cache and current station pointers */
>> > > fm_rx_reset_rds_cache(fmdev);
>> > > fm_rx_reset_station_info(fmdev);
>> > > @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
>> > > FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
>> > > return -EINVAL;
>> > > }
>> > > - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
>> > >
>> > > payload = vol_to_set;
>> > > ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
>> > > @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
>> > > return -ENOMEM;
>> > > }
>> > >
>> > > - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
>> > > + *curr_vol = fmdev->rx.volume;
>> > >
>> > > return 0;
>> > > }
>> > > @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
>> > > int ret;
>> > >
>> > > if (region_to_set != FM_BAND_EUROPE_US &&
>> > > - region_to_set != FM_BAND_JAPAN) {
>> > > + region_to_set != FM_BAND_JAPAN &&
>> > > + region_to_set != FM_BAND_RUSSIAN) {
>> > > fmerr("Invalid band\n");
>> > > return -EINVAL;
>> > > }
>> > > @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
>> > > fmerr("Invalid RSSI threshold level\n");
>> > > return -EINVAL;
>> > > }
>> > > - payload = (u16)rssi_lvl_toset;
>> > > + payload = (u16) rssi_lvl_toset;
>> > > ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
>> > > sizeof(payload), NULL, NULL);
>> > > if (ret < 0)
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
>> > > index 6d879fb..52e5120 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv_tx.c
>> > > +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
>> > > @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
>> > >
>> > > fmdbg("AF: %d\n", af);
>> > >
>> > > - af = (af - 87500) / 100;
>> > > + fmdev->tx_data.af_frq = af;
>> > > + af = (af - FM_US_BAND_LOW) / FM_KHZ;
>> > > payload = (u16)af;
>> > > - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
>> > > - sizeof(payload), NULL, NULL);
>> > > + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
>> > > if (ret < 0)
>> > > return ret;
>> > >
>> > > diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> > > index b9da1ae..4bd7bf1 100644
>> > > --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> > > +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> > > @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
>> > > return 0;
>> > > }
>> > >
>> > > +/**********************************************************************/
>> > > +/* functions called from sysfs subsystem */
>> > > +
>> > > +static ssize_t show_fmtx_af(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
>> > > +}
>> > > +
>> > > +static ssize_t store_fmtx_af(struct device *dev,
>> > > + struct device_attribute *attr, char *buf, size_t size)
>> > > +{
>> > > + int ret;
>> > > + unsigned long af_freq;
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + if (kstrtoul(buf, 0, &af_freq))
>> > > + return -EINVAL;
>> > > +
>> > > + ret = fm_tx_set_af(fmdev, af_freq);
>> > > + if (ret < 0) {
>> > > + fmerr("Failed to set FM TX AF Frequency\n");
>> > > + return ret;
>> > > + }
>> > > + return size;
>> > > +}
>> > > +
>> > > +static ssize_t show_fmrx_deemphasis(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
>> > > + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
>> > > +}
>> > > +
>> > > +static ssize_t store_fmrx_deemphasis(struct device *dev,
>> > > + struct device_attribute *attr, char *buf, size_t size)
>> > > +{
>> > > + int ret;
>> > > + unsigned long deemph_mode;
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + if (kstrtoul(buf, 0, &deemph_mode))
>> > > + return -EINVAL;
>> > > +
>> > > + if (deemph_mode != 50 && deemph_mode != 75)
>> > > + return -EINVAL;
>> > > +
>> > > + if (deemph_mode == 50)
>> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
>> > > + else
>> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
>> > > +
>> > > + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
>> > > + if (ret < 0) {
>> > > + fmerr("Failed to set De-emphasis Mode\n");
>> > > + return ret;
>> > > + }
>> > > +
>> > > + return size;
>> > > +}
>> > > +
>> > > +static ssize_t show_fmrx_af(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
>> > > +}
>> > > +
>> > > +static ssize_t store_fmrx_af(struct device *dev,
>> > > + struct device_attribute *attr, char *buf, size_t size)
>> > > +{
>> > > + int ret;
>> > > + unsigned long af_mode;
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + if (kstrtoul(buf, 0, &af_mode))
>> > > + return -EINVAL;
>> > > +
>> > > + if (af_mode < 0 || af_mode > 1)
>> > > + return -EINVAL;
>> > > +
>> > > + ret = fm_rx_set_af_switch(fmdev, af_mode);
>> > > + if (ret < 0) {
>> > > + fmerr("Failed to set AF Switch\n");
>> > > + return ret;
>> > > + }
>> > > +
>> > > + return size;
>> > > +}
>> > > +
>> > > +static ssize_t show_fmrx_band(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
>> > > +}
>> > > +
>> > > +static ssize_t store_fmrx_band(struct device *dev,
>> > > + struct device_attribute *attr, char *buf, size_t size)
>> > > +{
>> > > + int ret;
>> > > + unsigned long fm_band;
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + if (kstrtoul(buf, 0, &fm_band))
>> > > + return -EINVAL;
>> > > +
>> > > + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
>> > > + return -EINVAL;
>> > > +
>> > > + ret = fm_rx_set_region(fmdev, fm_band);
>> > > + if (ret < 0) {
>> > > + fmerr("Failed to set FM Band\n");
>> > > + return ret;
>> > > + }
>> > > +
>> > > + return size;
>> > > +}
>> > > +
>> > > +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
>> > > + struct device_attribute *attr, char *buf)
>> > > +{
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
>> > > +}
>> > > +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
>> > > + struct device_attribute *attr, char *buf, size_t size)
>> > > +{
>> > > + int ret;
>> > > + unsigned long rssi_lvl;
>> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> > > +
>> > > + if (kstrtoul(buf, 0, &rssi_lvl))
>> > > + return -EINVAL;
>> > > +
>> > > + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
>> > > + if (ret < 0) {
>> > > + fmerr("Failed to set RSSI level\n");
>> > > + return ret;
>> > > + }
>> > > +
>> > > + return size;
>> > > +}
>> > > +
>> > > +/* structures specific for sysfs entries */
>> > > +static struct kobj_attribute v4l2_fmtx_rds_af =
>> > > +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
>> > > +
>> > > +static struct kobj_attribute v4l2_fm_deemph_mode =
>> > > +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
>> > > +
>> > > +static struct kobj_attribute v4l2_fm_rds_af =
>> > > +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
>> > > +
>> > > +static struct kobj_attribute v4l2_fm_band =
>> > > +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
>> > > +
>> > > +static struct kobj_attribute v4l2_fm_rssi_lvl =
>> > > +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
>> > > +
>> > > +static struct attribute *v4l2_fm_attrs[] = {
>> > > + &v4l2_fmtx_rds_af.attr,
>> > > + &v4l2_fm_deemph_mode.attr,
>> > > + &v4l2_fm_rds_af.attr,
>> > > + &v4l2_fm_band.attr,
>> > > + &v4l2_fm_rssi_lvl.attr,
>> > > + NULL,
>> > > +};
>> > > +static struct attribute_group v4l2_fm_attr_grp = {
>> > > + .attrs = v4l2_fm_attrs,
>> > > +};
>> > > /*
>> > > * Handle open request for "/dev/radioX" device.
>> > > * Start with FM RX mode as default.
>> > > @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
>> > > }
>> > > radio_disconnected = 1;
>> > >
>> > > + /* Register sysfs entries */
>> > > + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
>> > > + if (ret) {
>> > > + pr_err("failed to create sysfs entries");
>> > > + return ret;
>> > > + }
>> > > return ret;
>> > > }
>> > >
>> > > @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
>> > > return ret;
>> > > }
>> > >
>> > > + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
>> > > +
>> > > ret = fmc_release(fmdev);
>> > > if (ret < 0) {
>> > > fmerr("FM CORE release failed\n");
>> > > @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
>> > > FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
>> > >
>> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
>> > > - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
>> > > + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
>> > >
>> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
>> > > V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
>> > >
>> > --
>> > 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
>>
> --
> 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
--
Regards
Halli
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-28 22:52 ` halli manjunatha
@ 2012-02-29 11:25 ` Hans Verkuil
2012-02-29 17:19 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-02-29 11:25 UTC (permalink / raw)
To: halli manjunatha; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
> >> Hi Hans,
> >>
> >> Agreed I don't mind to create new controls for below things
> >>
> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> 2) FM RX RDS AF turn ON/OFF
> >> 3) FM RX RSSI level set/get
> >> 4) FM TX Alternate Frequency set/get
> >> 5) FM RX De-Emphasis mode set/get
> >>
> >> However, previously you have suggested me to hide few controls (like
> >> band selection) from the user but few of our application wanted
> >> controls like above and that is why I have created the sysfs entries.
> >>
> >> Please suggest me how can I move forward with new controls or with sysfs?
> >
> > The first question is which of these controls are general to FM receivers or
> > transmitters, and which are specific to this chipset. The chipset specific
> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
> > videodev2.h how those are defined). The others should be defined as new
> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
> > defined as a new class as it doesn't exist at the moment. Don't forget to
> > document these controls clearly.
> >
> > With regards to the band selection: I remember that there was a discussion,
> > but not the details. Do you have a link to that discussion? I can't find it
> > (at least not quickly :-) ).
>
> Below features are generic to all FM receivers so we can add new CID's
> for below features
> 1) FM RX RDS AF turn ON/OFF
> 2) FM TX Alternate Frequency set/get
>
> About other 3 features its different issue,
> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> 2) FM RX RSSI level set/get
> 3) FM RX De-Emphasis mode set/get
>
> All these 3 features are generic to any FM receiver, only question is
> does all FM receivers wants to expose these controls to application
> writer?
Good question, and there is no good answer at the moment. See e.g. this
IRC discussion:
http://www.spinics.net/lists/linux-media/msg44023.html
In the absence of a good solution to this problem I am inclined to make
these controls driver specific but marked experimental. The experimental
tag allows us to eventually make it a generic control without too much
hassle.
> Example Band selection, every FM receiver at the minimum supports both
> Europe and Japan band, now the question is should we add a CID to
> switch between these two bands?
If we decide to add a band selection control, then that would be a menu
control (since there are up to three bands) and it would only be implemented
by drivers that need it.
What I am still not clear on is *why* you would want this control. What
is the reason your customers want this? What does it allow you to do that
can't be done today?
Regards,
Hans
>
> Below is the link to our discussion on the same topic a few time back.
>
> https://lkml.org/lkml/2010/11/18/43
>
> >
> > There is one other thing that would be nice if that could be changed in the
> > driver: currently there is only one radio device that can be used for both
> > the receiver and the transmitter. During a V4L meeting last year we had a
> > discussion about that and the conclusion was that receivers and transmitters
> > should have their own radio device.
> >
> > The reason for that decision was that the VIDIOC_G/S_FREQUENCY ioctls cannot
> > tell the difference between a transmitter and a receiver. If you have separate
> > receiver and transmitter radio nodes this confusion goes away. In addition,
> > this same restriction already applies to video and vbi nodes (i.e. a video or
> > vbi node can be used for capture or output, but not both, unless it is a
> > mem2mem device).
> >
> > I don't know how much work it is to implement this, but I would appreciate it
> > if you could think about this a bit.
>
> I will look in to this and submit a new patch to create 2 separate
> device nodes for FM RX and FM TX once after this patch set.
>
> Regards
> Manju
> >
> > Regards,
> >
> > Hans
> >
> >>
> >> Regards
> >> Manju
> >>
> >>
> >> On Sat, Feb 25, 2012 at 2:16 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> >
> >> > Hi Manjunatha!
> >> >
> >> > On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
> >> > > From: Manjunatha Halli <x0130808@ti.com>
> >> > >
> >> > > This patch adds support for below features via sysfs file system
> >> > >
> >> > > 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> > > 2) FM RX RDS AF turn ON/OFF
> >> > > 3) FM RX RSSI level set/get
> >> > > 4) FM TX Alternate Frequency set/get
> >> > > 5) FM RX De-Emphasis mode set/get
> >> >
> >> > Why sysfs? Wouldn't it make more sense to create controls for this?
> >> > That's the V4L way...
> >> >
> >> > Regards,
> >> >
> >> > Hans
> >> >
> >> > >
> >> > > Also this patch fixes below issues
> >> > >
> >> > > 1) FM audio volume gain setting
> >> > > 2) Default rssi level is set to 8 instead of 20
> >> > > 3) Issue related to audio mute/unmute
> >> > > 4)Enable FM RX AF support in driver
> >> > > 5)In wrap_around seek mode try for once
> >> > >
> >> > > Signed-off-by: Manjunatha Halli <x0130808@ti.com>
> >> > > ---
> >> > > drivers/media/radio/wl128x/fmdrv.h | 1 +
> >> > > drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
> >> > > drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
> >> > > drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
> >> > > drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
> >> > > drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
> >> > > 6 files changed, 225 insertions(+), 20 deletions(-)
> >> > >
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
> >> > > index d84ad9d..8fe773d 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv.h
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv.h
> >> > > @@ -196,6 +196,7 @@ struct fmtx_data {
> >> > > u16 aud_mode;
> >> > > u32 preemph;
> >> > > u32 tx_frq;
> >> > > + u32 af_frq;
> >> > > struct tx_rds rds;
> >> > > };
> >> > >
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
> >> > > index fcce61a..1a580a7 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv_common.c
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.c
> >> > > @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
> >> > > .top_freq = 90000, /* 90 MHz */
> >> > > .fm_band = 1,
> >> > > },
> >> > > + /* Russian (OIRT) band */
> >> > > + {
> >> > > + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
> >> > > + .bot_freq = 65800, /* 65.8 MHz */
> >> > > + .top_freq = 74000, /* 74 MHz */
> >> > > + .fm_band = 2,
> >> > > + },
> >> > > };
> >> > >
> >> > > /* Band selection */
> >> > > @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
> >> > > if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
> >> > > return;
> >> > >
> >> > > - freq = fmdev->rx.region.bot_freq + (af * 100);
> >> > > + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
> >> > > if (freq == fmdev->rx.freq) {
> >> > > fmdbg("Current freq(%d) is matching with received AF(%d)\n",
> >> > > fmdev->rx.freq, freq);
> >> > > @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
> >> > > fmdev->rx.rds.flag = FM_RDS_DISABLE;
> >> > > fmdev->rx.freq = FM_UNDEFINED_FREQ;
> >> > > fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
> >> > > - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
> >> > > + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
> >> > > fmdev->irq_info.retry = 0;
> >> > >
> >> > > fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
> >> > > index 196ff7d..aaa54bc 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv_common.h
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.h
> >> > > @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
> >> > > /* Band types */
> >> > > #define FM_BAND_EUROPE_US 0
> >> > > #define FM_BAND_JAPAN 1
> >> > > +#define FM_BAND_RUSSIAN 2
> >> > >
> >> > > /* Seek directions */
> >> > > #define FM_SEARCH_DIRECTION_DOWN 0
> >> > > @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
> >> > >
> >> > > /* Min and Max volume */
> >> > > #define FM_RX_VOLUME_MIN 0
> >> > > -#define FM_RX_VOLUME_MAX 70
> >> > > -
> >> > > -/* Volume gain step */
> >> > > -#define FM_RX_VOLUME_GAIN_STEP 0x370
> >> > > +#define FM_RX_VOLUME_MAX 0xffff
> >> > >
> >> > > /* Mute modes */
> >> > > -#define FM_MUTE_ON 0
> >> > > -#define FM_MUTE_OFF 1
> >> > > +#define FM_MUTE_OFF 0
> >> > > +#define FM_MUTE_ON 1
> >> > > #define FM_MUTE_ATTENUATE 2
> >> > >
> >> > > #define FM_RX_UNMUTE_MODE 0x00
> >> > > @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
> >> > > #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
> >> > >
> >> > > /* RSSI threshold min and max */
> >> > > -#define FM_RX_RSSI_THRESHOLD_MIN -128
> >> > > -#define FM_RX_RSSI_THRESHOLD_MAX 127
> >> > > +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
> >> > > +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
> >> > >
> >> > > /* Stereo/Mono mode */
> >> > > #define FM_STEREO_MODE 0
> >> > > @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
> >> > > * Default RX mode configuration. Chip will be configured
> >> > > * with this default values after loading RX firmware.
> >> > > */
> >> > > -#define FM_DEFAULT_RX_VOLUME 10
> >> > > -#define FM_DEFAULT_RSSI_THRESHOLD 20
> >> > > +#define FM_DEFAULT_RX_VOLUME 10000
> >> > > +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
> >> > >
> >> > > /* Range for TX power level in units for dB/uV */
> >> > > #define FM_PWR_LVL_LOW 91
> >> > > @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
> >> > > #define FM_CHANNEL_SPACING_200KHZ 4
> >> > > #define FM_FREQ_MUL 50
> >> > >
> >> > > +#define FM_US_BAND_LOW 87500
> >> > > +#define FM_US_BAND_HIGH 180000
> >> > > +#define FM_JAPAN_BAND_LOW 76000
> >> > > +#define FM_JAPAN_BAND_HIGH 90000
> >> > > +
> >> > > +#define FM_KHZ 100
> >> > > +
> >> > > +
> >> > > #endif
> >> > >
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
> >> > > index a806bda..0965270 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv_rx.c
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
> >> > > @@ -276,6 +276,10 @@ again:
> >> > > /* Calculate frequency index to write */
> >> > > next_frq = (fmdev->rx.freq -
> >> > > fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
> >> > > +
> >> > > + /* If no valid chanel then report default frequency */
> >> > > + wrap_around = 0;
> >> > > +
> >> > > goto again;
> >> > > }
> >> > > } else {
> >> > > @@ -290,6 +294,7 @@ again:
> >> > > ((u32)curr_frq * FM_FREQ_MUL));
> >> > >
> >> > > }
> >> > > +
> >> > > /* Reset RDS cache and current station pointers */
> >> > > fm_rx_reset_rds_cache(fmdev);
> >> > > fm_rx_reset_station_info(fmdev);
> >> > > @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
> >> > > FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
> >> > > return -EINVAL;
> >> > > }
> >> > > - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
> >> > >
> >> > > payload = vol_to_set;
> >> > > ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
> >> > > @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
> >> > > return -ENOMEM;
> >> > > }
> >> > >
> >> > > - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
> >> > > + *curr_vol = fmdev->rx.volume;
> >> > >
> >> > > return 0;
> >> > > }
> >> > > @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
> >> > > int ret;
> >> > >
> >> > > if (region_to_set != FM_BAND_EUROPE_US &&
> >> > > - region_to_set != FM_BAND_JAPAN) {
> >> > > + region_to_set != FM_BAND_JAPAN &&
> >> > > + region_to_set != FM_BAND_RUSSIAN) {
> >> > > fmerr("Invalid band\n");
> >> > > return -EINVAL;
> >> > > }
> >> > > @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
> >> > > fmerr("Invalid RSSI threshold level\n");
> >> > > return -EINVAL;
> >> > > }
> >> > > - payload = (u16)rssi_lvl_toset;
> >> > > + payload = (u16) rssi_lvl_toset;
> >> > > ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
> >> > > sizeof(payload), NULL, NULL);
> >> > > if (ret < 0)
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
> >> > > index 6d879fb..52e5120 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv_tx.c
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
> >> > > @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
> >> > >
> >> > > fmdbg("AF: %d\n", af);
> >> > >
> >> > > - af = (af - 87500) / 100;
> >> > > + fmdev->tx_data.af_frq = af;
> >> > > + af = (af - FM_US_BAND_LOW) / FM_KHZ;
> >> > > payload = (u16)af;
> >> > > - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
> >> > > - sizeof(payload), NULL, NULL);
> >> > > + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
> >> > > if (ret < 0)
> >> > > return ret;
> >> > >
> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> >> > > index b9da1ae..4bd7bf1 100644
> >> > > --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
> >> > > @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
> >> > > return 0;
> >> > > }
> >> > >
> >> > > +/**********************************************************************/
> >> > > +/* functions called from sysfs subsystem */
> >> > > +
> >> > > +static ssize_t show_fmtx_af(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf)
> >> > > +{
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
> >> > > +}
> >> > > +
> >> > > +static ssize_t store_fmtx_af(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf, size_t size)
> >> > > +{
> >> > > + int ret;
> >> > > + unsigned long af_freq;
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + if (kstrtoul(buf, 0, &af_freq))
> >> > > + return -EINVAL;
> >> > > +
> >> > > + ret = fm_tx_set_af(fmdev, af_freq);
> >> > > + if (ret < 0) {
> >> > > + fmerr("Failed to set FM TX AF Frequency\n");
> >> > > + return ret;
> >> > > + }
> >> > > + return size;
> >> > > +}
> >> > > +
> >> > > +static ssize_t show_fmrx_deemphasis(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf)
> >> > > +{
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
> >> > > + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
> >> > > +}
> >> > > +
> >> > > +static ssize_t store_fmrx_deemphasis(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf, size_t size)
> >> > > +{
> >> > > + int ret;
> >> > > + unsigned long deemph_mode;
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + if (kstrtoul(buf, 0, &deemph_mode))
> >> > > + return -EINVAL;
> >> > > +
> >> > > + if (deemph_mode != 50 && deemph_mode != 75)
> >> > > + return -EINVAL;
> >> > > +
> >> > > + if (deemph_mode == 50)
> >> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
> >> > > + else
> >> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
> >> > > +
> >> > > + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
> >> > > + if (ret < 0) {
> >> > > + fmerr("Failed to set De-emphasis Mode\n");
> >> > > + return ret;
> >> > > + }
> >> > > +
> >> > > + return size;
> >> > > +}
> >> > > +
> >> > > +static ssize_t show_fmrx_af(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf)
> >> > > +{
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
> >> > > +}
> >> > > +
> >> > > +static ssize_t store_fmrx_af(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf, size_t size)
> >> > > +{
> >> > > + int ret;
> >> > > + unsigned long af_mode;
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + if (kstrtoul(buf, 0, &af_mode))
> >> > > + return -EINVAL;
> >> > > +
> >> > > + if (af_mode < 0 || af_mode > 1)
> >> > > + return -EINVAL;
> >> > > +
> >> > > + ret = fm_rx_set_af_switch(fmdev, af_mode);
> >> > > + if (ret < 0) {
> >> > > + fmerr("Failed to set AF Switch\n");
> >> > > + return ret;
> >> > > + }
> >> > > +
> >> > > + return size;
> >> > > +}
> >> > > +
> >> > > +static ssize_t show_fmrx_band(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf)
> >> > > +{
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
> >> > > +}
> >> > > +
> >> > > +static ssize_t store_fmrx_band(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf, size_t size)
> >> > > +{
> >> > > + int ret;
> >> > > + unsigned long fm_band;
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + if (kstrtoul(buf, 0, &fm_band))
> >> > > + return -EINVAL;
> >> > > +
> >> > > + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
> >> > > + return -EINVAL;
> >> > > +
> >> > > + ret = fm_rx_set_region(fmdev, fm_band);
> >> > > + if (ret < 0) {
> >> > > + fmerr("Failed to set FM Band\n");
> >> > > + return ret;
> >> > > + }
> >> > > +
> >> > > + return size;
> >> > > +}
> >> > > +
> >> > > +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf)
> >> > > +{
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
> >> > > +}
> >> > > +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
> >> > > + struct device_attribute *attr, char *buf, size_t size)
> >> > > +{
> >> > > + int ret;
> >> > > + unsigned long rssi_lvl;
> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
> >> > > +
> >> > > + if (kstrtoul(buf, 0, &rssi_lvl))
> >> > > + return -EINVAL;
> >> > > +
> >> > > + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
> >> > > + if (ret < 0) {
> >> > > + fmerr("Failed to set RSSI level\n");
> >> > > + return ret;
> >> > > + }
> >> > > +
> >> > > + return size;
> >> > > +}
> >> > > +
> >> > > +/* structures specific for sysfs entries */
> >> > > +static struct kobj_attribute v4l2_fmtx_rds_af =
> >> > > +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
> >> > > +
> >> > > +static struct kobj_attribute v4l2_fm_deemph_mode =
> >> > > +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
> >> > > +
> >> > > +static struct kobj_attribute v4l2_fm_rds_af =
> >> > > +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
> >> > > +
> >> > > +static struct kobj_attribute v4l2_fm_band =
> >> > > +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
> >> > > +
> >> > > +static struct kobj_attribute v4l2_fm_rssi_lvl =
> >> > > +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
> >> > > +
> >> > > +static struct attribute *v4l2_fm_attrs[] = {
> >> > > + &v4l2_fmtx_rds_af.attr,
> >> > > + &v4l2_fm_deemph_mode.attr,
> >> > > + &v4l2_fm_rds_af.attr,
> >> > > + &v4l2_fm_band.attr,
> >> > > + &v4l2_fm_rssi_lvl.attr,
> >> > > + NULL,
> >> > > +};
> >> > > +static struct attribute_group v4l2_fm_attr_grp = {
> >> > > + .attrs = v4l2_fm_attrs,
> >> > > +};
> >> > > /*
> >> > > * Handle open request for "/dev/radioX" device.
> >> > > * Start with FM RX mode as default.
> >> > > @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
> >> > > }
> >> > > radio_disconnected = 1;
> >> > >
> >> > > + /* Register sysfs entries */
> >> > > + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> >> > > + if (ret) {
> >> > > + pr_err("failed to create sysfs entries");
> >> > > + return ret;
> >> > > + }
> >> > > return ret;
> >> > > }
> >> > >
> >> > > @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
> >> > > return ret;
> >> > > }
> >> > >
> >> > > + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
> >> > > +
> >> > > ret = fmc_release(fmdev);
> >> > > if (ret < 0) {
> >> > > fmerr("FM CORE release failed\n");
> >> > > @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
> >> > > FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
> >> > >
> >> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> >> > > - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
> >> > > + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
> >> > >
> >> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
> >> > > V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
> >> > >
> >> > --
> >> > 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
> >>
> > --
> > 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
>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-29 11:25 ` Hans Verkuil
@ 2012-02-29 17:19 ` halli manjunatha
2012-03-02 9:22 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-02-29 17:19 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
>> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>> >> Hi Hans,
>> >>
>> >> Agreed I don't mind to create new controls for below things
>> >>
>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> 2) FM RX RDS AF turn ON/OFF
>> >> 3) FM RX RSSI level set/get
>> >> 4) FM TX Alternate Frequency set/get
>> >> 5) FM RX De-Emphasis mode set/get
>> >>
>> >> However, previously you have suggested me to hide few controls (like
>> >> band selection) from the user but few of our application wanted
>> >> controls like above and that is why I have created the sysfs entries.
>> >>
>> >> Please suggest me how can I move forward with new controls or with sysfs?
>> >
>> > The first question is which of these controls are general to FM receivers or
>> > transmitters, and which are specific to this chipset. The chipset specific
>> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
>> > videodev2.h how those are defined). The others should be defined as new
>> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
>> > defined as a new class as it doesn't exist at the moment. Don't forget to
>> > document these controls clearly.
>> >
>> > With regards to the band selection: I remember that there was a discussion,
>> > but not the details. Do you have a link to that discussion? I can't find it
>> > (at least not quickly :-) ).
>>
>> Below features are generic to all FM receivers so we can add new CID's
>> for below features
>> 1) FM RX RDS AF turn ON/OFF
>> 2) FM TX Alternate Frequency set/get
>>
>> About other 3 features its different issue,
>> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> 2) FM RX RSSI level set/get
>> 3) FM RX De-Emphasis mode set/get
>>
>> All these 3 features are generic to any FM receiver, only question is
>> does all FM receivers wants to expose these controls to application
>> writer?
>
> Good question, and there is no good answer at the moment. See e.g. this
> IRC discussion:
>
> http://www.spinics.net/lists/linux-media/msg44023.html
>
> In the absence of a good solution to this problem I am inclined to make
> these controls driver specific but marked experimental. The experimental
> tag allows us to eventually make it a generic control without too much
> hassle.
Agreed, I will make them driver specific and mark them as experimental.
>
>> Example Band selection, every FM receiver at the minimum supports both
>> Europe and Japan band, now the question is should we add a CID to
>> switch between these two bands?
>
> If we decide to add a band selection control, then that would be a menu
> control (since there are up to three bands) and it would only be implemented
> by drivers that need it.
>
> What I am still not clear on is *why* you would want this control. What
> is the reason your customers want this? What does it allow you to do that
> can't be done today?
There are 2 reasons for this,
First, our chip supports weather band, unlike other bands (Europe,
Japan and Russian) user may wants to
switch to weather band and wants to listen to weather report and again
switches back to normal band.
Second, for FM TX, our chip supports band selection for FM
transmitter, so if the same phone is used in different
regions of world then user can switch to the actual band and start
transmitting by choosing a blank frequency in that band.
Regards
Manju
>
> Regards,
>
> Hans
>
>>
>> Below is the link to our discussion on the same topic a few time back.
>>
>> https://lkml.org/lkml/2010/11/18/43
>>
>> >
>> > There is one other thing that would be nice if that could be changed in the
>> > driver: currently there is only one radio device that can be used for both
>> > the receiver and the transmitter. During a V4L meeting last year we had a
>> > discussion about that and the conclusion was that receivers and transmitters
>> > should have their own radio device.
>> >
>> > The reason for that decision was that the VIDIOC_G/S_FREQUENCY ioctls cannot
>> > tell the difference between a transmitter and a receiver. If you have separate
>> > receiver and transmitter radio nodes this confusion goes away. In addition,
>> > this same restriction already applies to video and vbi nodes (i.e. a video or
>> > vbi node can be used for capture or output, but not both, unless it is a
>> > mem2mem device).
>> >
>> > I don't know how much work it is to implement this, but I would appreciate it
>> > if you could think about this a bit.
>>
>> I will look in to this and submit a new patch to create 2 separate
>> device nodes for FM RX and FM TX once after this patch set.
>>
>> Regards
>> Manju
>> >
>> > Regards,
>> >
>> > Hans
>> >
>> >>
>> >> Regards
>> >> Manju
>> >>
>> >>
>> >> On Sat, Feb 25, 2012 at 2:16 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> >
>> >> > Hi Manjunatha!
>> >> >
>> >> > On Friday, February 24, 2012 21:14:31 manjunatha_halli@ti.com wrote:
>> >> > > From: Manjunatha Halli <x0130808@ti.com>
>> >> > >
>> >> > > This patch adds support for below features via sysfs file system
>> >> > >
>> >> > > 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> > > 2) FM RX RDS AF turn ON/OFF
>> >> > > 3) FM RX RSSI level set/get
>> >> > > 4) FM TX Alternate Frequency set/get
>> >> > > 5) FM RX De-Emphasis mode set/get
>> >> >
>> >> > Why sysfs? Wouldn't it make more sense to create controls for this?
>> >> > That's the V4L way...
>> >> >
>> >> > Regards,
>> >> >
>> >> > Hans
>> >> >
>> >> > >
>> >> > > Also this patch fixes below issues
>> >> > >
>> >> > > 1) FM audio volume gain setting
>> >> > > 2) Default rssi level is set to 8 instead of 20
>> >> > > 3) Issue related to audio mute/unmute
>> >> > > 4)Enable FM RX AF support in driver
>> >> > > 5)In wrap_around seek mode try for once
>> >> > >
>> >> > > Signed-off-by: Manjunatha Halli <x0130808@ti.com>
>> >> > > ---
>> >> > > drivers/media/radio/wl128x/fmdrv.h | 1 +
>> >> > > drivers/media/radio/wl128x/fmdrv_common.c | 11 ++-
>> >> > > drivers/media/radio/wl128x/fmdrv_common.h | 26 +++--
>> >> > > drivers/media/radio/wl128x/fmdrv_rx.c | 13 ++-
>> >> > > drivers/media/radio/wl128x/fmdrv_tx.c | 6 +-
>> >> > > drivers/media/radio/wl128x/fmdrv_v4l2.c | 188 ++++++++++++++++++++++++++++-
>> >> > > 6 files changed, 225 insertions(+), 20 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv.h b/drivers/media/radio/wl128x/fmdrv.h
>> >> > > index d84ad9d..8fe773d 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv.h
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv.h
>> >> > > @@ -196,6 +196,7 @@ struct fmtx_data {
>> >> > > u16 aud_mode;
>> >> > > u32 preemph;
>> >> > > u32 tx_frq;
>> >> > > + u32 af_frq;
>> >> > > struct tx_rds rds;
>> >> > > };
>> >> > >
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.c b/drivers/media/radio/wl128x/fmdrv_common.c
>> >> > > index fcce61a..1a580a7 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv_common.c
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.c
>> >> > > @@ -58,6 +58,13 @@ static struct region_info region_configs[] = {
>> >> > > .top_freq = 90000, /* 90 MHz */
>> >> > > .fm_band = 1,
>> >> > > },
>> >> > > + /* Russian (OIRT) band */
>> >> > > + {
>> >> > > + .chanl_space = FM_CHANNEL_SPACING_200KHZ * FM_FREQ_MUL,
>> >> > > + .bot_freq = 65800, /* 65.8 MHz */
>> >> > > + .top_freq = 74000, /* 74 MHz */
>> >> > > + .fm_band = 2,
>> >> > > + },
>> >> > > };
>> >> > >
>> >> > > /* Band selection */
>> >> > > @@ -659,7 +666,7 @@ static void fm_rx_update_af_cache(struct fmdev *fmdev, u8 af)
>> >> > > if (reg_idx == FM_BAND_JAPAN && af > FM_RDS_MAX_AF_JAPAN)
>> >> > > return;
>> >> > >
>> >> > > - freq = fmdev->rx.region.bot_freq + (af * 100);
>> >> > > + freq = fmdev->rx.region.bot_freq + (af * FM_KHZ);
>> >> > > if (freq == fmdev->rx.freq) {
>> >> > > fmdbg("Current freq(%d) is matching with received AF(%d)\n",
>> >> > > fmdev->rx.freq, freq);
>> >> > > @@ -1576,7 +1583,7 @@ int fmc_prepare(struct fmdev *fmdev)
>> >> > > fmdev->rx.rds.flag = FM_RDS_DISABLE;
>> >> > > fmdev->rx.freq = FM_UNDEFINED_FREQ;
>> >> > > fmdev->rx.rds_mode = FM_RDS_SYSTEM_RDS;
>> >> > > - fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_OFF;
>> >> > > + fmdev->rx.af_mode = FM_RX_RDS_AF_SWITCH_MODE_ON;
>> >> > > fmdev->irq_info.retry = 0;
>> >> > >
>> >> > > fmdev->tx_data.tx_frq = FM_UNDEFINED_FREQ;
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_common.h b/drivers/media/radio/wl128x/fmdrv_common.h
>> >> > > index 196ff7d..aaa54bc 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv_common.h
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_common.h
>> >> > > @@ -203,6 +203,7 @@ struct fm_event_msg_hdr {
>> >> > > /* Band types */
>> >> > > #define FM_BAND_EUROPE_US 0
>> >> > > #define FM_BAND_JAPAN 1
>> >> > > +#define FM_BAND_RUSSIAN 2
>> >> > >
>> >> > > /* Seek directions */
>> >> > > #define FM_SEARCH_DIRECTION_DOWN 0
>> >> > > @@ -216,14 +217,11 @@ struct fm_event_msg_hdr {
>> >> > >
>> >> > > /* Min and Max volume */
>> >> > > #define FM_RX_VOLUME_MIN 0
>> >> > > -#define FM_RX_VOLUME_MAX 70
>> >> > > -
>> >> > > -/* Volume gain step */
>> >> > > -#define FM_RX_VOLUME_GAIN_STEP 0x370
>> >> > > +#define FM_RX_VOLUME_MAX 0xffff
>> >> > >
>> >> > > /* Mute modes */
>> >> > > -#define FM_MUTE_ON 0
>> >> > > -#define FM_MUTE_OFF 1
>> >> > > +#define FM_MUTE_OFF 0
>> >> > > +#define FM_MUTE_ON 1
>> >> > > #define FM_MUTE_ATTENUATE 2
>> >> > >
>> >> > > #define FM_RX_UNMUTE_MODE 0x00
>> >> > > @@ -238,8 +236,8 @@ struct fm_event_msg_hdr {
>> >> > > #define FM_RX_RF_DEPENDENT_MUTE_OFF 0
>> >> > >
>> >> > > /* RSSI threshold min and max */
>> >> > > -#define FM_RX_RSSI_THRESHOLD_MIN -128
>> >> > > -#define FM_RX_RSSI_THRESHOLD_MAX 127
>> >> > > +#define FM_RX_RSSI_THRESHOLD_MIN 0 /* 0 dBuV */
>> >> > > +#define FM_RX_RSSI_THRESHOLD_MAX 127 /* 191.1477 dBuV */
>> >> > >
>> >> > > /* Stereo/Mono mode */
>> >> > > #define FM_STEREO_MODE 0
>> >> > > @@ -352,8 +350,8 @@ struct fm_event_msg_hdr {
>> >> > > * Default RX mode configuration. Chip will be configured
>> >> > > * with this default values after loading RX firmware.
>> >> > > */
>> >> > > -#define FM_DEFAULT_RX_VOLUME 10
>> >> > > -#define FM_DEFAULT_RSSI_THRESHOLD 20
>> >> > > +#define FM_DEFAULT_RX_VOLUME 10000
>> >> > > +#define FM_DEFAULT_RSSI_THRESHOLD 8 /* 12.0408 dBuV */
>> >> > >
>> >> > > /* Range for TX power level in units for dB/uV */
>> >> > > #define FM_PWR_LVL_LOW 91
>> >> > > @@ -403,5 +401,13 @@ int fmc_get_mode(struct fmdev *, u8 *);
>> >> > > #define FM_CHANNEL_SPACING_200KHZ 4
>> >> > > #define FM_FREQ_MUL 50
>> >> > >
>> >> > > +#define FM_US_BAND_LOW 87500
>> >> > > +#define FM_US_BAND_HIGH 180000
>> >> > > +#define FM_JAPAN_BAND_LOW 76000
>> >> > > +#define FM_JAPAN_BAND_HIGH 90000
>> >> > > +
>> >> > > +#define FM_KHZ 100
>> >> > > +
>> >> > > +
>> >> > > #endif
>> >> > >
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_rx.c b/drivers/media/radio/wl128x/fmdrv_rx.c
>> >> > > index a806bda..0965270 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv_rx.c
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_rx.c
>> >> > > @@ -276,6 +276,10 @@ again:
>> >> > > /* Calculate frequency index to write */
>> >> > > next_frq = (fmdev->rx.freq -
>> >> > > fmdev->rx.region.bot_freq) / FM_FREQ_MUL;
>> >> > > +
>> >> > > + /* If no valid chanel then report default frequency */
>> >> > > + wrap_around = 0;
>> >> > > +
>> >> > > goto again;
>> >> > > }
>> >> > > } else {
>> >> > > @@ -290,6 +294,7 @@ again:
>> >> > > ((u32)curr_frq * FM_FREQ_MUL));
>> >> > >
>> >> > > }
>> >> > > +
>> >> > > /* Reset RDS cache and current station pointers */
>> >> > > fm_rx_reset_rds_cache(fmdev);
>> >> > > fm_rx_reset_station_info(fmdev);
>> >> > > @@ -310,7 +315,6 @@ int fm_rx_set_volume(struct fmdev *fmdev, u16 vol_to_set)
>> >> > > FM_RX_VOLUME_MIN, FM_RX_VOLUME_MAX);
>> >> > > return -EINVAL;
>> >> > > }
>> >> > > - vol_to_set *= FM_RX_VOLUME_GAIN_STEP;
>> >> > >
>> >> > > payload = vol_to_set;
>> >> > > ret = fmc_send_cmd(fmdev, VOLUME_SET, REG_WR, &payload,
>> >> > > @@ -333,7 +337,7 @@ int fm_rx_get_volume(struct fmdev *fmdev, u16 *curr_vol)
>> >> > > return -ENOMEM;
>> >> > > }
>> >> > >
>> >> > > - *curr_vol = fmdev->rx.volume / FM_RX_VOLUME_GAIN_STEP;
>> >> > > + *curr_vol = fmdev->rx.volume;
>> >> > >
>> >> > > return 0;
>> >> > > }
>> >> > > @@ -364,7 +368,8 @@ int fm_rx_set_region(struct fmdev *fmdev, u8 region_to_set)
>> >> > > int ret;
>> >> > >
>> >> > > if (region_to_set != FM_BAND_EUROPE_US &&
>> >> > > - region_to_set != FM_BAND_JAPAN) {
>> >> > > + region_to_set != FM_BAND_JAPAN &&
>> >> > > + region_to_set != FM_BAND_RUSSIAN) {
>> >> > > fmerr("Invalid band\n");
>> >> > > return -EINVAL;
>> >> > > }
>> >> > > @@ -550,7 +555,7 @@ int fm_rx_set_rssi_threshold(struct fmdev *fmdev, short rssi_lvl_toset)
>> >> > > fmerr("Invalid RSSI threshold level\n");
>> >> > > return -EINVAL;
>> >> > > }
>> >> > > - payload = (u16)rssi_lvl_toset;
>> >> > > + payload = (u16) rssi_lvl_toset;
>> >> > > ret = fmc_send_cmd(fmdev, SEARCH_LVL_SET, REG_WR, &payload,
>> >> > > sizeof(payload), NULL, NULL);
>> >> > > if (ret < 0)
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_tx.c b/drivers/media/radio/wl128x/fmdrv_tx.c
>> >> > > index 6d879fb..52e5120 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv_tx.c
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_tx.c
>> >> > > @@ -177,10 +177,10 @@ int fm_tx_set_af(struct fmdev *fmdev, u32 af)
>> >> > >
>> >> > > fmdbg("AF: %d\n", af);
>> >> > >
>> >> > > - af = (af - 87500) / 100;
>> >> > > + fmdev->tx_data.af_frq = af;
>> >> > > + af = (af - FM_US_BAND_LOW) / FM_KHZ;
>> >> > > payload = (u16)af;
>> >> > > - ret = fmc_send_cmd(fmdev, TA_SET, REG_WR, &payload,
>> >> > > - sizeof(payload), NULL, NULL);
>> >> > > + ret = fmc_send_cmd(fmdev, AF, REG_WR, &payload, sizeof(payload), NULL, NULL);
>> >> > > if (ret < 0)
>> >> > > return ret;
>> >> > >
>> >> > > diff --git a/drivers/media/radio/wl128x/fmdrv_v4l2.c b/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> >> > > index b9da1ae..4bd7bf1 100644
>> >> > > --- a/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> >> > > +++ b/drivers/media/radio/wl128x/fmdrv_v4l2.c
>> >> > > @@ -110,6 +110,184 @@ static u32 fm_v4l2_fops_poll(struct file *file, struct poll_table_struct *pts)
>> >> > > return 0;
>> >> > > }
>> >> > >
>> >> > > +/**********************************************************************/
>> >> > > +/* functions called from sysfs subsystem */
>> >> > > +
>> >> > > +static ssize_t show_fmtx_af(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf)
>> >> > > +{
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + return sprintf(buf, "%d\n", fmdev->tx_data.af_frq);
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t store_fmtx_af(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf, size_t size)
>> >> > > +{
>> >> > > + int ret;
>> >> > > + unsigned long af_freq;
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + if (kstrtoul(buf, 0, &af_freq))
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + ret = fm_tx_set_af(fmdev, af_freq);
>> >> > > + if (ret < 0) {
>> >> > > + fmerr("Failed to set FM TX AF Frequency\n");
>> >> > > + return ret;
>> >> > > + }
>> >> > > + return size;
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t show_fmrx_deemphasis(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf)
>> >> > > +{
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + return sprintf(buf, "%d\n", (fmdev->rx.deemphasis_mode ==
>> >> > > + FM_RX_EMPHASIS_FILTER_50_USEC) ? 50 : 75);
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t store_fmrx_deemphasis(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf, size_t size)
>> >> > > +{
>> >> > > + int ret;
>> >> > > + unsigned long deemph_mode;
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + if (kstrtoul(buf, 0, &deemph_mode))
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + if (deemph_mode != 50 && deemph_mode != 75)
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + if (deemph_mode == 50)
>> >> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_50_USEC;
>> >> > > + else
>> >> > > + deemph_mode = FM_RX_EMPHASIS_FILTER_75_USEC;
>> >> > > +
>> >> > > + ret = fm_rx_set_deemphasis_mode(fmdev, deemph_mode);
>> >> > > + if (ret < 0) {
>> >> > > + fmerr("Failed to set De-emphasis Mode\n");
>> >> > > + return ret;
>> >> > > + }
>> >> > > +
>> >> > > + return size;
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t show_fmrx_af(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf)
>> >> > > +{
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + return sprintf(buf, "%d\n", fmdev->rx.af_mode);
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t store_fmrx_af(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf, size_t size)
>> >> > > +{
>> >> > > + int ret;
>> >> > > + unsigned long af_mode;
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + if (kstrtoul(buf, 0, &af_mode))
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + if (af_mode < 0 || af_mode > 1)
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + ret = fm_rx_set_af_switch(fmdev, af_mode);
>> >> > > + if (ret < 0) {
>> >> > > + fmerr("Failed to set AF Switch\n");
>> >> > > + return ret;
>> >> > > + }
>> >> > > +
>> >> > > + return size;
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t show_fmrx_band(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf)
>> >> > > +{
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + return sprintf(buf, "%d\n", fmdev->rx.region.fm_band);
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t store_fmrx_band(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf, size_t size)
>> >> > > +{
>> >> > > + int ret;
>> >> > > + unsigned long fm_band;
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + if (kstrtoul(buf, 0, &fm_band))
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + if (fm_band < FM_BAND_EUROPE_US || fm_band > FM_BAND_RUSSIAN)
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + ret = fm_rx_set_region(fmdev, fm_band);
>> >> > > + if (ret < 0) {
>> >> > > + fmerr("Failed to set FM Band\n");
>> >> > > + return ret;
>> >> > > + }
>> >> > > +
>> >> > > + return size;
>> >> > > +}
>> >> > > +
>> >> > > +static ssize_t show_fmrx_rssi_lvl(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf)
>> >> > > +{
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + return sprintf(buf, "%d\n", fmdev->rx.rssi_threshold);
>> >> > > +}
>> >> > > +static ssize_t store_fmrx_rssi_lvl(struct device *dev,
>> >> > > + struct device_attribute *attr, char *buf, size_t size)
>> >> > > +{
>> >> > > + int ret;
>> >> > > + unsigned long rssi_lvl;
>> >> > > + struct fmdev *fmdev = dev_get_drvdata(dev);
>> >> > > +
>> >> > > + if (kstrtoul(buf, 0, &rssi_lvl))
>> >> > > + return -EINVAL;
>> >> > > +
>> >> > > + ret = fm_rx_set_rssi_threshold(fmdev, rssi_lvl);
>> >> > > + if (ret < 0) {
>> >> > > + fmerr("Failed to set RSSI level\n");
>> >> > > + return ret;
>> >> > > + }
>> >> > > +
>> >> > > + return size;
>> >> > > +}
>> >> > > +
>> >> > > +/* structures specific for sysfs entries */
>> >> > > +static struct kobj_attribute v4l2_fmtx_rds_af =
>> >> > > +__ATTR(fmtx_rds_af, 0666, (void *)show_fmtx_af, (void *)store_fmtx_af);
>> >> > > +
>> >> > > +static struct kobj_attribute v4l2_fm_deemph_mode =
>> >> > > +__ATTR(fmrx_deemph_mode, 0666, (void *)show_fmrx_deemphasis, (void *)store_fmrx_deemphasis);
>> >> > > +
>> >> > > +static struct kobj_attribute v4l2_fm_rds_af =
>> >> > > +__ATTR(fmrx_rds_af, 0666, (void *)show_fmrx_af, (void *)store_fmrx_af);
>> >> > > +
>> >> > > +static struct kobj_attribute v4l2_fm_band =
>> >> > > +__ATTR(fmrx_band, 0666, (void *)show_fmrx_band, (void *)store_fmrx_band);
>> >> > > +
>> >> > > +static struct kobj_attribute v4l2_fm_rssi_lvl =
>> >> > > +__ATTR(fmrx_rssi_lvl, 0666, (void *) show_fmrx_rssi_lvl, (void *)store_fmrx_rssi_lvl);
>> >> > > +
>> >> > > +static struct attribute *v4l2_fm_attrs[] = {
>> >> > > + &v4l2_fmtx_rds_af.attr,
>> >> > > + &v4l2_fm_deemph_mode.attr,
>> >> > > + &v4l2_fm_rds_af.attr,
>> >> > > + &v4l2_fm_band.attr,
>> >> > > + &v4l2_fm_rssi_lvl.attr,
>> >> > > + NULL,
>> >> > > +};
>> >> > > +static struct attribute_group v4l2_fm_attr_grp = {
>> >> > > + .attrs = v4l2_fm_attrs,
>> >> > > +};
>> >> > > /*
>> >> > > * Handle open request for "/dev/radioX" device.
>> >> > > * Start with FM RX mode as default.
>> >> > > @@ -142,6 +320,12 @@ static int fm_v4l2_fops_open(struct file *file)
>> >> > > }
>> >> > > radio_disconnected = 1;
>> >> > >
>> >> > > + /* Register sysfs entries */
>> >> > > + ret = sysfs_create_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
>> >> > > + if (ret) {
>> >> > > + pr_err("failed to create sysfs entries");
>> >> > > + return ret;
>> >> > > + }
>> >> > > return ret;
>> >> > > }
>> >> > >
>> >> > > @@ -162,6 +346,8 @@ static int fm_v4l2_fops_release(struct file *file)
>> >> > > return ret;
>> >> > > }
>> >> > >
>> >> > > + sysfs_remove_group(&fmdev->radio_dev->dev.kobj, &v4l2_fm_attr_grp);
>> >> > > +
>> >> > > ret = fmc_release(fmdev);
>> >> > > if (ret < 0) {
>> >> > > fmerr("FM CORE release failed\n");
>> >> > > @@ -582,7 +768,7 @@ int fm_v4l2_init_video_device(struct fmdev *fmdev, int radio_nr)
>> >> > > FM_RX_VOLUME_MAX, 1, FM_RX_VOLUME_MAX);
>> >> > >
>> >> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
>> >> > > - V4L2_CID_AUDIO_MUTE, 0, 1, 1, 1);
>> >> > > + V4L2_CID_AUDIO_MUTE, 0, 1, 1, 0);
>> >> > >
>> >> > > v4l2_ctrl_new_std(&fmdev->ctrl_handler, &fm_ctrl_ops,
>> >> > > V4L2_CID_RDS_TX_PI, 0x0, 0xffff, 1, 0x0);
>> >> > >
>> >> > --
>> >> > 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
>> >>
>> > --
>> > 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
>>
>>
>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-02-29 17:19 ` halli manjunatha
@ 2012-03-02 9:22 ` Hans Verkuil
2012-03-05 16:24 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-03-02 9:22 UTC (permalink / raw)
To: halli manjunatha; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
> >> >> Hi Hans,
> >> >>
> >> >> Agreed I don't mind to create new controls for below things
> >> >>
> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> >> 2) FM RX RDS AF turn ON/OFF
> >> >> 3) FM RX RSSI level set/get
> >> >> 4) FM TX Alternate Frequency set/get
> >> >> 5) FM RX De-Emphasis mode set/get
> >> >>
> >> >> However, previously you have suggested me to hide few controls (like
> >> >> band selection) from the user but few of our application wanted
> >> >> controls like above and that is why I have created the sysfs entries.
> >> >>
> >> >> Please suggest me how can I move forward with new controls or with sysfs?
> >> >
> >> > The first question is which of these controls are general to FM receivers or
> >> > transmitters, and which are specific to this chipset. The chipset specific
> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
> >> > videodev2.h how those are defined). The others should be defined as new
> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
> >> > document these controls clearly.
> >> >
> >> > With regards to the band selection: I remember that there was a discussion,
> >> > but not the details. Do you have a link to that discussion? I can't find it
> >> > (at least not quickly :-) ).
> >>
> >> Below features are generic to all FM receivers so we can add new CID's
> >> for below features
> >> 1) FM RX RDS AF turn ON/OFF
> >> 2) FM TX Alternate Frequency set/get
> >>
> >> About other 3 features its different issue,
> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> 2) FM RX RSSI level set/get
> >> 3) FM RX De-Emphasis mode set/get
> >>
> >> All these 3 features are generic to any FM receiver, only question is
> >> does all FM receivers wants to expose these controls to application
> >> writer?
> >
> > Good question, and there is no good answer at the moment. See e.g. this
> > IRC discussion:
> >
> > http://www.spinics.net/lists/linux-media/msg44023.html
> >
> > In the absence of a good solution to this problem I am inclined to make
> > these controls driver specific but marked experimental. The experimental
> > tag allows us to eventually make it a generic control without too much
> > hassle.
>
> Agreed, I will make them driver specific and mark them as experimental.
>
> >
> >> Example Band selection, every FM receiver at the minimum supports both
> >> Europe and Japan band, now the question is should we add a CID to
> >> switch between these two bands?
> >
> > If we decide to add a band selection control, then that would be a menu
> > control (since there are up to three bands) and it would only be implemented
> > by drivers that need it.
> >
> > What I am still not clear on is *why* you would want this control. What
> > is the reason your customers want this? What does it allow you to do that
> > can't be done today?
>
> There are 2 reasons for this,
>
> First, our chip supports weather band, unlike other bands (Europe,
> Japan and Russian) user may wants to
> switch to weather band and wants to listen to weather report and again
> switches back to normal band.
OK, that makes sense. Are the RX and TX independent with regards to
band selection?
Make sure that when the band is changed the rangelow and rangehigh values
are also changed. If the current frequency is out of that range, then the
frequency should be clamped to the closest value frequency. Although an
alternative strategy might be to remember the last used frequency for each
band. That might make more sense in the case of switching between a normal
band and the weather band. We need to define and document which strategy
should be used here.
BTW, is the receiver for the weather band implemented as a separate receiver?
I read that some devices can listen to the normal band and interrupt that
when a weather report is broadcast on the weather band. That implies two
receivers and it would require a rethink.
Also, is this feature really implemented as separate frequency ranges in
hardware? Or is the receiver able to receive on the whole range of frequencies
from 65.8 (OIRT) to approx. 165 (weather band range)?
Is the datasheet of this device available somewhere?
> Second, for FM TX, our chip supports band selection for FM
> transmitter, so if the same phone is used in different
> regions of world then user can switch to the actual band and start
> transmitting by choosing a blank frequency in that band.
Isn't this something that can be equally easily done in userspace?
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-02 9:22 ` Hans Verkuil
@ 2012-03-05 16:24 ` halli manjunatha
2012-03-07 21:42 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-03-05 16:24 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>> >> >> Hi Hans,
>> >> >>
>> >> >> Agreed I don't mind to create new controls for below things
>> >> >>
>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> >> 2) FM RX RDS AF turn ON/OFF
>> >> >> 3) FM RX RSSI level set/get
>> >> >> 4) FM TX Alternate Frequency set/get
>> >> >> 5) FM RX De-Emphasis mode set/get
>> >> >>
>> >> >> However, previously you have suggested me to hide few controls (like
>> >> >> band selection) from the user but few of our application wanted
>> >> >> controls like above and that is why I have created the sysfs entries.
>> >> >>
>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
>> >> >
>> >> > The first question is which of these controls are general to FM receivers or
>> >> > transmitters, and which are specific to this chipset. The chipset specific
>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
>> >> > videodev2.h how those are defined). The others should be defined as new
>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
>> >> > document these controls clearly.
>> >> >
>> >> > With regards to the band selection: I remember that there was a discussion,
>> >> > but not the details. Do you have a link to that discussion? I can't find it
>> >> > (at least not quickly :-) ).
>> >>
>> >> Below features are generic to all FM receivers so we can add new CID's
>> >> for below features
>> >> 1) FM RX RDS AF turn ON/OFF
>> >> 2) FM TX Alternate Frequency set/get
>> >>
>> >> About other 3 features its different issue,
>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> 2) FM RX RSSI level set/get
>> >> 3) FM RX De-Emphasis mode set/get
>> >>
>> >> All these 3 features are generic to any FM receiver, only question is
>> >> does all FM receivers wants to expose these controls to application
>> >> writer?
>> >
>> > Good question, and there is no good answer at the moment. See e.g. this
>> > IRC discussion:
>> >
>> > http://www.spinics.net/lists/linux-media/msg44023.html
>> >
>> > In the absence of a good solution to this problem I am inclined to make
>> > these controls driver specific but marked experimental. The experimental
>> > tag allows us to eventually make it a generic control without too much
>> > hassle.
>>
>> Agreed, I will make them driver specific and mark them as experimental.
>>
>> >
>> >> Example Band selection, every FM receiver at the minimum supports both
>> >> Europe and Japan band, now the question is should we add a CID to
>> >> switch between these two bands?
>> >
>> > If we decide to add a band selection control, then that would be a menu
>> > control (since there are up to three bands) and it would only be implemented
>> > by drivers that need it.
>> >
>> > What I am still not clear on is *why* you would want this control. What
>> > is the reason your customers want this? What does it allow you to do that
>> > can't be done today?
>>
>> There are 2 reasons for this,
>>
>> First, our chip supports weather band, unlike other bands (Europe,
>> Japan and Russian) user may wants to
>> switch to weather band and wants to listen to weather report and again
>> switches back to normal band.
>
> OK, that makes sense. Are the RX and TX independent with regards to
> band selection?
Yes - RX and TX are independent of band selection
> Make sure that when the band is changed the rangelow and rangehigh values
> are also changed. If the current frequency is out of that range, then the
> frequency should be clamped to the closest value frequency. Although an
> alternative strategy might be to remember the last used frequency for each
> band. That might make more sense in the case of switching between a normal
> band and the weather band. We need to define and document which strategy
> should be used here.
As of now when I switch to new band I just set the frequency to lowest
of the new band.
In this way user can seek and tune to what ever channel he wants.
> BTW, is the receiver for the weather band implemented as a separate receiver?
> I read that some devices can listen to the normal band and interrupt that
> when a weather report is broadcast on the weather band. That implies two
> receivers and it would require a rethink.
>
> Also, is this feature really implemented as separate frequency ranges in
> hardware? Or is the receiver able to receive on the whole range of frequencies
> from 65.8 (OIRT) to approx. 165 (weather band range)?
Our chip wont have 2 receivers, it has only 1 receiver which can
receive on whole frequency range from 65 MHz to 165 MHz.
> Is the datasheet of this device available somewhere?
Sorry our newest chipset supports this feature so yet now we don't
have any datasheet available on net.
>
>> Second, for FM TX, our chip supports band selection for FM
>> transmitter, so if the same phone is used in different
>> regions of world then user can switch to the actual band and start
>> transmitting by choosing a blank frequency in that band.
>
> Isn't this something that can be equally easily done in userspace?
you wants me to do this from driver itself without hinting the
application about the band ?
>
> Regards,
>
> Hans
> --
> 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
--
Regards
Halli
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-05 16:24 ` halli manjunatha
@ 2012-03-07 21:42 ` halli manjunatha
2012-03-09 8:29 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-03-07 21:42 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Mon, Mar 5, 2012 at 10:24 AM, halli manjunatha <hallimanju@gmail.com> wrote:
> On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
>>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
>>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>>> >> >> Hi Hans,
>>> >> >>
>>> >> >> Agreed I don't mind to create new controls for below things
>>> >> >>
>>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>>> >> >> 2) FM RX RDS AF turn ON/OFF
>>> >> >> 3) FM RX RSSI level set/get
>>> >> >> 4) FM TX Alternate Frequency set/get
>>> >> >> 5) FM RX De-Emphasis mode set/get
>>> >> >>
>>> >> >> However, previously you have suggested me to hide few controls (like
>>> >> >> band selection) from the user but few of our application wanted
>>> >> >> controls like above and that is why I have created the sysfs entries.
>>> >> >>
>>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
>>> >> >
>>> >> > The first question is which of these controls are general to FM receivers or
>>> >> > transmitters, and which are specific to this chipset. The chipset specific
>>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
>>> >> > videodev2.h how those are defined). The others should be defined as new
>>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
>>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
>>> >> > document these controls clearly.
>>> >> >
>>> >> > With regards to the band selection: I remember that there was a discussion,
>>> >> > but not the details. Do you have a link to that discussion? I can't find it
>>> >> > (at least not quickly :-) ).
>>> >>
>>> >> Below features are generic to all FM receivers so we can add new CID's
>>> >> for below features
>>> >> 1) FM RX RDS AF turn ON/OFF
>>> >> 2) FM TX Alternate Frequency set/get
>>> >>
>>> >> About other 3 features its different issue,
>>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>>> >> 2) FM RX RSSI level set/get
>>> >> 3) FM RX De-Emphasis mode set/get
>>> >>
>>> >> All these 3 features are generic to any FM receiver, only question is
>>> >> does all FM receivers wants to expose these controls to application
>>> >> writer?
>>> >
>>> > Good question, and there is no good answer at the moment. See e.g. this
>>> > IRC discussion:
>>> >
>>> > http://www.spinics.net/lists/linux-media/msg44023.html
>>> >
>>> > In the absence of a good solution to this problem I am inclined to make
>>> > these controls driver specific but marked experimental. The experimental
>>> > tag allows us to eventually make it a generic control without too much
>>> > hassle.
>>>
>>> Agreed, I will make them driver specific and mark them as experimental.
>>>
>>> >
>>> >> Example Band selection, every FM receiver at the minimum supports both
>>> >> Europe and Japan band, now the question is should we add a CID to
>>> >> switch between these two bands?
>>> >
>>> > If we decide to add a band selection control, then that would be a menu
>>> > control (since there are up to three bands) and it would only be implemented
>>> > by drivers that need it.
>>> >
>>> > What I am still not clear on is *why* you would want this control. What
>>> > is the reason your customers want this? What does it allow you to do that
>>> > can't be done today?
>>>
>>> There are 2 reasons for this,
>>>
>>> First, our chip supports weather band, unlike other bands (Europe,
>>> Japan and Russian) user may wants to
>>> switch to weather band and wants to listen to weather report and again
>>> switches back to normal band.
>>
>> OK, that makes sense. Are the RX and TX independent with regards to
>> band selection?
>
> Yes - RX and TX are independent of band selection
>
>> Make sure that when the band is changed the rangelow and rangehigh values
>> are also changed. If the current frequency is out of that range, then the
>> frequency should be clamped to the closest value frequency. Although an
>> alternative strategy might be to remember the last used frequency for each
>> band. That might make more sense in the case of switching between a normal
>> band and the weather band. We need to define and document which strategy
>> should be used here.
>
> As of now when I switch to new band I just set the frequency to lowest
> of the new band.
> In this way user can seek and tune to what ever channel he wants.
>
Hans,
Which implementation you wants? start with the lowest of the new band
or closer to the frequency of old band? do we need to remember the
present frequency of the band before switching to new band?
Please let me know your views.
Since this feature is required by all FM receivers shall I make this
as a generic CID?
>> BTW, is the receiver for the weather band implemented as a separate receiver?
>> I read that some devices can listen to the normal band and interrupt that
>> when a weather report is broadcast on the weather band. That implies two
>> receivers and it would require a rethink.
>>
>> Also, is this feature really implemented as separate frequency ranges in
>> hardware? Or is the receiver able to receive on the whole range of frequencies
>> from 65.8 (OIRT) to approx. 165 (weather band range)?
>
> Our chip wont have 2 receivers, it has only 1 receiver which can
> receive on whole frequency range from 65 MHz to 165 MHz.
>
>> Is the datasheet of this device available somewhere?
>
> Sorry our newest chipset supports this feature so yet now we don't
> have any datasheet available on net.
>
>>
>>> Second, for FM TX, our chip supports band selection for FM
>>> transmitter, so if the same phone is used in different
>>> regions of world then user can switch to the actual band and start
>>> transmitting by choosing a blank frequency in that band.
>>
>> Isn't this something that can be equally easily done in userspace?
>
> you wants me to do this from driver itself without hinting the
> application about the band ?
>
>>
>> Regards,
>>
>> Hans
>> --
>> 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
>
>
>
> --
> Regards
> Halli
--
Regards
Halli
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-07 21:42 ` halli manjunatha
@ 2012-03-09 8:29 ` Hans Verkuil
2012-03-09 20:44 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-03-09 8:29 UTC (permalink / raw)
To: halli manjunatha; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Wednesday, March 07, 2012 22:42:05 halli manjunatha wrote:
> On Mon, Mar 5, 2012 at 10:24 AM, halli manjunatha <hallimanju@gmail.com> wrote:
> > On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
> >>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
> >>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
> >>> >> >> Hi Hans,
> >>> >> >>
> >>> >> >> Agreed I don't mind to create new controls for below things
> >>> >> >>
> >>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >>> >> >> 2) FM RX RDS AF turn ON/OFF
> >>> >> >> 3) FM RX RSSI level set/get
> >>> >> >> 4) FM TX Alternate Frequency set/get
> >>> >> >> 5) FM RX De-Emphasis mode set/get
> >>> >> >>
> >>> >> >> However, previously you have suggested me to hide few controls (like
> >>> >> >> band selection) from the user but few of our application wanted
> >>> >> >> controls like above and that is why I have created the sysfs entries.
> >>> >> >>
> >>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
> >>> >> >
> >>> >> > The first question is which of these controls are general to FM receivers or
> >>> >> > transmitters, and which are specific to this chipset. The chipset specific
> >>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
> >>> >> > videodev2.h how those are defined). The others should be defined as new
> >>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
> >>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
> >>> >> > document these controls clearly.
> >>> >> >
> >>> >> > With regards to the band selection: I remember that there was a discussion,
> >>> >> > but not the details. Do you have a link to that discussion? I can't find it
> >>> >> > (at least not quickly :-) ).
> >>> >>
> >>> >> Below features are generic to all FM receivers so we can add new CID's
> >>> >> for below features
> >>> >> 1) FM RX RDS AF turn ON/OFF
> >>> >> 2) FM TX Alternate Frequency set/get
> >>> >>
> >>> >> About other 3 features its different issue,
> >>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >>> >> 2) FM RX RSSI level set/get
> >>> >> 3) FM RX De-Emphasis mode set/get
> >>> >>
> >>> >> All these 3 features are generic to any FM receiver, only question is
> >>> >> does all FM receivers wants to expose these controls to application
> >>> >> writer?
> >>> >
> >>> > Good question, and there is no good answer at the moment. See e.g. this
> >>> > IRC discussion:
> >>> >
> >>> > http://www.spinics.net/lists/linux-media/msg44023.html
> >>> >
> >>> > In the absence of a good solution to this problem I am inclined to make
> >>> > these controls driver specific but marked experimental. The experimental
> >>> > tag allows us to eventually make it a generic control without too much
> >>> > hassle.
> >>>
> >>> Agreed, I will make them driver specific and mark them as experimental.
> >>>
> >>> >
> >>> >> Example Band selection, every FM receiver at the minimum supports both
> >>> >> Europe and Japan band, now the question is should we add a CID to
> >>> >> switch between these two bands?
> >>> >
> >>> > If we decide to add a band selection control, then that would be a menu
> >>> > control (since there are up to three bands) and it would only be implemented
> >>> > by drivers that need it.
> >>> >
> >>> > What I am still not clear on is *why* you would want this control. What
> >>> > is the reason your customers want this? What does it allow you to do that
> >>> > can't be done today?
> >>>
> >>> There are 2 reasons for this,
> >>>
> >>> First, our chip supports weather band, unlike other bands (Europe,
> >>> Japan and Russian) user may wants to
> >>> switch to weather band and wants to listen to weather report and again
> >>> switches back to normal band.
> >>
> >> OK, that makes sense. Are the RX and TX independent with regards to
> >> band selection?
> >
> > Yes - RX and TX are independent of band selection
> >
> >> Make sure that when the band is changed the rangelow and rangehigh values
> >> are also changed. If the current frequency is out of that range, then the
> >> frequency should be clamped to the closest value frequency. Although an
> >> alternative strategy might be to remember the last used frequency for each
> >> band. That might make more sense in the case of switching between a normal
> >> band and the weather band. We need to define and document which strategy
> >> should be used here.
> >
> > As of now when I switch to new band I just set the frequency to lowest
> > of the new band.
> > In this way user can seek and tune to what ever channel he wants.
> >
> Hans,
>
> Which implementation you wants? start with the lowest of the new band
> or closer to the frequency of old band? do we need to remember the
> present frequency of the band before switching to new band?
>
> Please let me know your views.
The initial frequency of each band is driver dependent. That's how drivers
act at the moment, so we keep that.
When switching bands it is best to keep the frequency closest to the old band.
This makes sense when switching from e.g. US/Europe and Japan band where there
is overlap in frequencies, so I think that's the best approach.
> Since this feature is required by all FM receivers shall I make this
> as a generic CID?
Yes, but see my next question below.
>
> >> BTW, is the receiver for the weather band implemented as a separate receiver?
> >> I read that some devices can listen to the normal band and interrupt that
> >> when a weather report is broadcast on the weather band. That implies two
> >> receivers and it would require a rethink.
> >>
> >> Also, is this feature really implemented as separate frequency ranges in
> >> hardware? Or is the receiver able to receive on the whole range of frequencies
> >> from 65.8 (OIRT) to approx. 165 (weather band range)?
> >
> > Our chip wont have 2 receivers, it has only 1 receiver which can
> > receive on whole frequency range from 65 MHz to 165 MHz.
If that's the case, then what does the band selector control actually have to do
hardware-wise? Does it just clamp the frequency to the correct frequency range?
> >
> >> Is the datasheet of this device available somewhere?
> >
> > Sorry our newest chipset supports this feature so yet now we don't
> > have any datasheet available on net.
> >
> >>
> >>> Second, for FM TX, our chip supports band selection for FM
> >>> transmitter, so if the same phone is used in different
> >>> regions of world then user can switch to the actual band and start
> >>> transmitting by choosing a blank frequency in that band.
> >>
> >> Isn't this something that can be equally easily done in userspace?
> >
> > you wants me to do this from driver itself without hinting the
> > application about the band ?
I think my question is really the same as above: what does the band selector
control actually have to do for a TX hardware-wise?
None of the existing radio RX and TX drivers need a band selector, because
they just cover the whole frequency range. If apps want to create these bands,
then they can do so themselves. So I am trying to understand why this device
needs a band selector. It's why I was interested in the datasheet...
Note that my replies to you will be sporadic for probably the next few weeks.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-09 8:29 ` Hans Verkuil
@ 2012-03-09 20:44 ` halli manjunatha
2012-03-10 9:56 ` Hans Verkuil
0 siblings, 1 reply; 17+ messages in thread
From: halli manjunatha @ 2012-03-09 20:44 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Fri, Mar 9, 2012 at 2:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Wednesday, March 07, 2012 22:42:05 halli manjunatha wrote:
>> On Mon, Mar 5, 2012 at 10:24 AM, halli manjunatha <hallimanju@gmail.com> wrote:
>> > On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
>> >>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
>> >>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>> >>> >> >> Hi Hans,
>> >>> >> >>
>> >>> >> >> Agreed I don't mind to create new controls for below things
>> >>> >> >>
>> >>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >>> >> >> 2) FM RX RDS AF turn ON/OFF
>> >>> >> >> 3) FM RX RSSI level set/get
>> >>> >> >> 4) FM TX Alternate Frequency set/get
>> >>> >> >> 5) FM RX De-Emphasis mode set/get
>> >>> >> >>
>> >>> >> >> However, previously you have suggested me to hide few controls (like
>> >>> >> >> band selection) from the user but few of our application wanted
>> >>> >> >> controls like above and that is why I have created the sysfs entries.
>> >>> >> >>
>> >>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
>> >>> >> >
>> >>> >> > The first question is which of these controls are general to FM receivers or
>> >>> >> > transmitters, and which are specific to this chipset. The chipset specific
>> >>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
>> >>> >> > videodev2.h how those are defined). The others should be defined as new
>> >>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
>> >>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
>> >>> >> > document these controls clearly.
>> >>> >> >
>> >>> >> > With regards to the band selection: I remember that there was a discussion,
>> >>> >> > but not the details. Do you have a link to that discussion? I can't find it
>> >>> >> > (at least not quickly :-) ).
>> >>> >>
>> >>> >> Below features are generic to all FM receivers so we can add new CID's
>> >>> >> for below features
>> >>> >> 1) FM RX RDS AF turn ON/OFF
>> >>> >> 2) FM TX Alternate Frequency set/get
>> >>> >>
>> >>> >> About other 3 features its different issue,
>> >>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >>> >> 2) FM RX RSSI level set/get
>> >>> >> 3) FM RX De-Emphasis mode set/get
>> >>> >>
>> >>> >> All these 3 features are generic to any FM receiver, only question is
>> >>> >> does all FM receivers wants to expose these controls to application
>> >>> >> writer?
>> >>> >
>> >>> > Good question, and there is no good answer at the moment. See e.g. this
>> >>> > IRC discussion:
>> >>> >
>> >>> > http://www.spinics.net/lists/linux-media/msg44023.html
>> >>> >
>> >>> > In the absence of a good solution to this problem I am inclined to make
>> >>> > these controls driver specific but marked experimental. The experimental
>> >>> > tag allows us to eventually make it a generic control without too much
>> >>> > hassle.
>> >>>
>> >>> Agreed, I will make them driver specific and mark them as experimental.
>> >>>
>> >>> >
>> >>> >> Example Band selection, every FM receiver at the minimum supports both
>> >>> >> Europe and Japan band, now the question is should we add a CID to
>> >>> >> switch between these two bands?
>> >>> >
>> >>> > If we decide to add a band selection control, then that would be a menu
>> >>> > control (since there are up to three bands) and it would only be implemented
>> >>> > by drivers that need it.
>> >>> >
>> >>> > What I am still not clear on is *why* you would want this control. What
>> >>> > is the reason your customers want this? What does it allow you to do that
>> >>> > can't be done today?
>> >>>
>> >>> There are 2 reasons for this,
>> >>>
>> >>> First, our chip supports weather band, unlike other bands (Europe,
>> >>> Japan and Russian) user may wants to
>> >>> switch to weather band and wants to listen to weather report and again
>> >>> switches back to normal band.
>> >>
>> >> OK, that makes sense. Are the RX and TX independent with regards to
>> >> band selection?
>> >
>> > Yes - RX and TX are independent of band selection
>> >
>> >> Make sure that when the band is changed the rangelow and rangehigh values
>> >> are also changed. If the current frequency is out of that range, then the
>> >> frequency should be clamped to the closest value frequency. Although an
>> >> alternative strategy might be to remember the last used frequency for each
>> >> band. That might make more sense in the case of switching between a normal
>> >> band and the weather band. We need to define and document which strategy
>> >> should be used here.
>> >
>> > As of now when I switch to new band I just set the frequency to lowest
>> > of the new band.
>> > In this way user can seek and tune to what ever channel he wants.
>> >
>> Hans,
>>
>> Which implementation you wants? start with the lowest of the new band
>> or closer to the frequency of old band? do we need to remember the
>> present frequency of the band before switching to new band?
>>
>> Please let me know your views.
>
> The initial frequency of each band is driver dependent. That's how drivers
> act at the moment, so we keep that.
>
> When switching bands it is best to keep the frequency closest to the old band.
> This makes sense when switching from e.g. US/Europe and Japan band where there
> is overlap in frequencies, so I think that's the best approach.
>
>> Since this feature is required by all FM receivers shall I make this
>> as a generic CID?
>
> Yes, but see my next question below.
>
>>
>> >> BTW, is the receiver for the weather band implemented as a separate receiver?
>> >> I read that some devices can listen to the normal band and interrupt that
>> >> when a weather report is broadcast on the weather band. That implies two
>> >> receivers and it would require a rethink.
>> >>
>> >> Also, is this feature really implemented as separate frequency ranges in
>> >> hardware? Or is the receiver able to receive on the whole range of frequencies
>> >> from 65.8 (OIRT) to approx. 165 (weather band range)?
>> >
>> > Our chip wont have 2 receivers, it has only 1 receiver which can
>> > receive on whole frequency range from 65 MHz to 165 MHz.
>
> If that's the case, then what does the band selector control actually have to do
> hardware-wise? Does it just clamp the frequency to the correct frequency range?
The Wilink chip can't able to receive on all frequencies at a time,
instead first we need to set the band(need to set chip registers to do
so) on which receiver has to receive, after this the chip can receive
on frequency range of this band only.
Eg: When set to Europe band chip can only receive from 87.5 MHz to 108
MHz, in this case receiver will give band limit reached interrupt if
tried to set frequency other than within this band.
Band selector controller will first switch the band of reception and
then sets the frequency which is within that band.
>
>> >
>> >> Is the datasheet of this device available somewhere?
>> >
>> > Sorry our newest chipset supports this feature so yet now we don't
>> > have any datasheet available on net.
>> >
>> >>
>> >>> Second, for FM TX, our chip supports band selection for FM
>> >>> transmitter, so if the same phone is used in different
>> >>> regions of world then user can switch to the actual band and start
>> >>> transmitting by choosing a blank frequency in that band.
>> >>
>> >> Isn't this something that can be equally easily done in userspace?
>> >
>> > you wants me to do this from driver itself without hinting the
>> > application about the band ?
>
> I think my question is really the same as above: what does the band selector
> control actually have to do for a TX hardware-wise?
Yes TX case I accept we may not need the band switch controller,
instead when user wants to set a frequency, driver will see in which
band this frequency comes and then first switches the FM chip to
transmit in that band and then sets the frequency.
So I don't add any private or public CID for this.
>
> None of the existing radio RX and TX drivers need a band selector, because
> they just cover the whole frequency range. If apps want to create these bands,
> then they can do so themselves. So I am trying to understand why this device
> needs a band selector. It's why I was interested in the datasheet...
In RX case the the above provided solution wont work because
1) If user do hardware seek then seek will be performed within the
present band not throughout from 65MHz to 162 MHz
2) If user wants to switch band he should know the frequency range of
the band to which he is switching and then set the frequency within
that band.
Hardware seek functionality is one of the main reason why we need band
switch, most of the time user may not have knowledge about the
frequency ranges of bands so in that case use will switch to a band
and do hardware seek, if stations found then he will continue on the
same band if not he can check the next available band and do seek
again.
>
> Note that my replies to you will be sporadic for probably the next few weeks.
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-09 20:44 ` halli manjunatha
@ 2012-03-10 9:56 ` Hans Verkuil
2012-03-13 15:20 ` halli manjunatha
0 siblings, 1 reply; 17+ messages in thread
From: Hans Verkuil @ 2012-03-10 9:56 UTC (permalink / raw)
To: halli manjunatha; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Friday, March 09, 2012 21:44:10 halli manjunatha wrote:
> On Fri, Mar 9, 2012 at 2:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Wednesday, March 07, 2012 22:42:05 halli manjunatha wrote:
> >> On Mon, Mar 5, 2012 at 10:24 AM, halli manjunatha <hallimanju@gmail.com> wrote:
> >> > On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> >> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
> >> >>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> >>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
> >> >>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> >>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
> >> >>> >> >> Hi Hans,
> >> >>> >> >>
> >> >>> >> >> Agreed I don't mind to create new controls for below things
> >> >>> >> >>
> >> >>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> >>> >> >> 2) FM RX RDS AF turn ON/OFF
> >> >>> >> >> 3) FM RX RSSI level set/get
> >> >>> >> >> 4) FM TX Alternate Frequency set/get
> >> >>> >> >> 5) FM RX De-Emphasis mode set/get
> >> >>> >> >>
> >> >>> >> >> However, previously you have suggested me to hide few controls (like
> >> >>> >> >> band selection) from the user but few of our application wanted
> >> >>> >> >> controls like above and that is why I have created the sysfs entries.
> >> >>> >> >>
> >> >>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
> >> >>> >> >
> >> >>> >> > The first question is which of these controls are general to FM receivers or
> >> >>> >> > transmitters, and which are specific to this chipset. The chipset specific
> >> >>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
> >> >>> >> > videodev2.h how those are defined). The others should be defined as new
> >> >>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
> >> >>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
> >> >>> >> > document these controls clearly.
> >> >>> >> >
> >> >>> >> > With regards to the band selection: I remember that there was a discussion,
> >> >>> >> > but not the details. Do you have a link to that discussion? I can't find it
> >> >>> >> > (at least not quickly :-) ).
> >> >>> >>
> >> >>> >> Below features are generic to all FM receivers so we can add new CID's
> >> >>> >> for below features
> >> >>> >> 1) FM RX RDS AF turn ON/OFF
> >> >>> >> 2) FM TX Alternate Frequency set/get
> >> >>> >>
> >> >>> >> About other 3 features its different issue,
> >> >>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
> >> >>> >> 2) FM RX RSSI level set/get
> >> >>> >> 3) FM RX De-Emphasis mode set/get
> >> >>> >>
> >> >>> >> All these 3 features are generic to any FM receiver, only question is
> >> >>> >> does all FM receivers wants to expose these controls to application
> >> >>> >> writer?
> >> >>> >
> >> >>> > Good question, and there is no good answer at the moment. See e.g. this
> >> >>> > IRC discussion:
> >> >>> >
> >> >>> > http://www.spinics.net/lists/linux-media/msg44023.html
> >> >>> >
> >> >>> > In the absence of a good solution to this problem I am inclined to make
> >> >>> > these controls driver specific but marked experimental. The experimental
> >> >>> > tag allows us to eventually make it a generic control without too much
> >> >>> > hassle.
> >> >>>
> >> >>> Agreed, I will make them driver specific and mark them as experimental.
> >> >>>
> >> >>> >
> >> >>> >> Example Band selection, every FM receiver at the minimum supports both
> >> >>> >> Europe and Japan band, now the question is should we add a CID to
> >> >>> >> switch between these two bands?
> >> >>> >
> >> >>> > If we decide to add a band selection control, then that would be a menu
> >> >>> > control (since there are up to three bands) and it would only be implemented
> >> >>> > by drivers that need it.
> >> >>> >
> >> >>> > What I am still not clear on is *why* you would want this control. What
> >> >>> > is the reason your customers want this? What does it allow you to do that
> >> >>> > can't be done today?
> >> >>>
> >> >>> There are 2 reasons for this,
> >> >>>
> >> >>> First, our chip supports weather band, unlike other bands (Europe,
> >> >>> Japan and Russian) user may wants to
> >> >>> switch to weather band and wants to listen to weather report and again
> >> >>> switches back to normal band.
> >> >>
> >> >> OK, that makes sense. Are the RX and TX independent with regards to
> >> >> band selection?
> >> >
> >> > Yes - RX and TX are independent of band selection
> >> >
> >> >> Make sure that when the band is changed the rangelow and rangehigh values
> >> >> are also changed. If the current frequency is out of that range, then the
> >> >> frequency should be clamped to the closest value frequency. Although an
> >> >> alternative strategy might be to remember the last used frequency for each
> >> >> band. That might make more sense in the case of switching between a normal
> >> >> band and the weather band. We need to define and document which strategy
> >> >> should be used here.
> >> >
> >> > As of now when I switch to new band I just set the frequency to lowest
> >> > of the new band.
> >> > In this way user can seek and tune to what ever channel he wants.
> >> >
> >> Hans,
> >>
> >> Which implementation you wants? start with the lowest of the new band
> >> or closer to the frequency of old band? do we need to remember the
> >> present frequency of the band before switching to new band?
> >>
> >> Please let me know your views.
> >
> > The initial frequency of each band is driver dependent. That's how drivers
> > act at the moment, so we keep that.
> >
> > When switching bands it is best to keep the frequency closest to the old band.
> > This makes sense when switching from e.g. US/Europe and Japan band where there
> > is overlap in frequencies, so I think that's the best approach.
> >
> >> Since this feature is required by all FM receivers shall I make this
> >> as a generic CID?
> >
> > Yes, but see my next question below.
> >
> >>
> >> >> BTW, is the receiver for the weather band implemented as a separate receiver?
> >> >> I read that some devices can listen to the normal band and interrupt that
> >> >> when a weather report is broadcast on the weather band. That implies two
> >> >> receivers and it would require a rethink.
> >> >>
> >> >> Also, is this feature really implemented as separate frequency ranges in
> >> >> hardware? Or is the receiver able to receive on the whole range of frequencies
> >> >> from 65.8 (OIRT) to approx. 165 (weather band range)?
> >> >
> >> > Our chip wont have 2 receivers, it has only 1 receiver which can
> >> > receive on whole frequency range from 65 MHz to 165 MHz.
> >
> > If that's the case, then what does the band selector control actually have to do
> > hardware-wise? Does it just clamp the frequency to the correct frequency range?
>
> The Wilink chip can't able to receive on all frequencies at a time,
> instead first we need to set the band(need to set chip registers to do
> so) on which receiver has to receive, after this the chip can receive
> on frequency range of this band only.
>
> Eg: When set to Europe band chip can only receive from 87.5 MHz to 108
> MHz, in this case receiver will give band limit reached interrupt if
> tried to set frequency other than within this band.
>
> Band selector controller will first switch the band of reception and
> then sets the frequency which is within that band.
>
> >
> >> >
> >> >> Is the datasheet of this device available somewhere?
> >> >
> >> > Sorry our newest chipset supports this feature so yet now we don't
> >> > have any datasheet available on net.
> >> >
> >> >>
> >> >>> Second, for FM TX, our chip supports band selection for FM
> >> >>> transmitter, so if the same phone is used in different
> >> >>> regions of world then user can switch to the actual band and start
> >> >>> transmitting by choosing a blank frequency in that band.
> >> >>
> >> >> Isn't this something that can be equally easily done in userspace?
> >> >
> >> > you wants me to do this from driver itself without hinting the
> >> > application about the band ?
> >
> > I think my question is really the same as above: what does the band selector
> > control actually have to do for a TX hardware-wise?
>
> Yes TX case I accept we may not need the band switch controller,
> instead when user wants to set a frequency, driver will see in which
> band this frequency comes and then first switches the FM chip to
> transmit in that band and then sets the frequency.
>
> So I don't add any private or public CID for this.
Good.
> >
> > None of the existing radio RX and TX drivers need a band selector, because
> > they just cover the whole frequency range. If apps want to create these bands,
> > then they can do so themselves. So I am trying to understand why this device
> > needs a band selector. It's why I was interested in the datasheet...
>
> In RX case the the above provided solution wont work because
> 1) If user do hardware seek then seek will be performed within the
> present band not throughout from 65MHz to 162 MHz
> 2) If user wants to switch band he should know the frequency range of
> the band to which he is switching and then set the frequency within
> that band.
>
> Hardware seek functionality is one of the main reason why we need band
> switch, most of the time user may not have knowledge about the
> frequency ranges of bands so in that case use will switch to a band
> and do hardware seek, if stations found then he will continue on the
> same band if not he can check the next available band and do seek
> again.
How about this: rather than adding a band selection control, why not add a
new band field to struct v4l2_hw_freq_seek? The default, 0, means just use the
full frequency range. Then define the other bands (these are pretty much
standardized). If a band is not supported by the hardware, -ERANGE is returned.
Otherwise the current frequency is clamped to the specified band frequency range
and you start searching.
Band selection doesn't really make sense outside of hardware seek.
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] wl128x: Add sysfs based support for FM features
2012-03-10 9:56 ` Hans Verkuil
@ 2012-03-13 15:20 ` halli manjunatha
0 siblings, 0 replies; 17+ messages in thread
From: halli manjunatha @ 2012-03-13 15:20 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-kernel, Manjunatha Halli, shahed
On Sat, Mar 10, 2012 at 3:56 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Friday, March 09, 2012 21:44:10 halli manjunatha wrote:
>> On Fri, Mar 9, 2012 at 2:29 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Wednesday, March 07, 2012 22:42:05 halli manjunatha wrote:
>> >> On Mon, Mar 5, 2012 at 10:24 AM, halli manjunatha <hallimanju@gmail.com> wrote:
>> >> > On Fri, Mar 2, 2012 at 3:22 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> >> On Wednesday, February 29, 2012 18:19:27 halli manjunatha wrote:
>> >> >>> On Wed, Feb 29, 2012 at 5:25 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> >>> > On Tuesday, February 28, 2012 23:52:21 halli manjunatha wrote:
>> >> >>> >> On Tue, Feb 28, 2012 at 4:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> >>> >> > On Monday, February 27, 2012 17:29:18 halli manjunatha wrote:
>> >> >>> >> >> Hi Hans,
>> >> >>> >> >>
>> >> >>> >> >> Agreed I don't mind to create new controls for below things
>> >> >>> >> >>
>> >> >>> >> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> >>> >> >> 2) FM RX RDS AF turn ON/OFF
>> >> >>> >> >> 3) FM RX RSSI level set/get
>> >> >>> >> >> 4) FM TX Alternate Frequency set/get
>> >> >>> >> >> 5) FM RX De-Emphasis mode set/get
>> >> >>> >> >>
>> >> >>> >> >> However, previously you have suggested me to hide few controls (like
>> >> >>> >> >> band selection) from the user but few of our application wanted
>> >> >>> >> >> controls like above and that is why I have created the sysfs entries.
>> >> >>> >> >>
>> >> >>> >> >> Please suggest me how can I move forward with new controls or with sysfs?
>> >> >>> >> >
>> >> >>> >> > The first question is which of these controls are general to FM receivers or
>> >> >>> >> > transmitters, and which are specific to this chipset. The chipset specific
>> >> >>> >> > controls should be private controls (look at V4L2_CID_MPEG_CX2341X_BASE in
>> >> >>> >> > videodev2.h how those are defined). The others should be defined as new
>> >> >>> >> > controls of the FM_TX class or the FM_RX class. The FM_RX class should be
>> >> >>> >> > defined as a new class as it doesn't exist at the moment. Don't forget to
>> >> >>> >> > document these controls clearly.
>> >> >>> >> >
>> >> >>> >> > With regards to the band selection: I remember that there was a discussion,
>> >> >>> >> > but not the details. Do you have a link to that discussion? I can't find it
>> >> >>> >> > (at least not quickly :-) ).
>> >> >>> >>
>> >> >>> >> Below features are generic to all FM receivers so we can add new CID's
>> >> >>> >> for below features
>> >> >>> >> 1) FM RX RDS AF turn ON/OFF
>> >> >>> >> 2) FM TX Alternate Frequency set/get
>> >> >>> >>
>> >> >>> >> About other 3 features its different issue,
>> >> >>> >> 1) FM RX Band selection (US/Europe, Japan and Russian bands)
>> >> >>> >> 2) FM RX RSSI level set/get
>> >> >>> >> 3) FM RX De-Emphasis mode set/get
>> >> >>> >>
>> >> >>> >> All these 3 features are generic to any FM receiver, only question is
>> >> >>> >> does all FM receivers wants to expose these controls to application
>> >> >>> >> writer?
>> >> >>> >
>> >> >>> > Good question, and there is no good answer at the moment. See e.g. this
>> >> >>> > IRC discussion:
>> >> >>> >
>> >> >>> > http://www.spinics.net/lists/linux-media/msg44023.html
>> >> >>> >
>> >> >>> > In the absence of a good solution to this problem I am inclined to make
>> >> >>> > these controls driver specific but marked experimental. The experimental
>> >> >>> > tag allows us to eventually make it a generic control without too much
>> >> >>> > hassle.
>> >> >>>
>> >> >>> Agreed, I will make them driver specific and mark them as experimental.
>> >> >>>
>> >> >>> >
>> >> >>> >> Example Band selection, every FM receiver at the minimum supports both
>> >> >>> >> Europe and Japan band, now the question is should we add a CID to
>> >> >>> >> switch between these two bands?
>> >> >>> >
>> >> >>> > If we decide to add a band selection control, then that would be a menu
>> >> >>> > control (since there are up to three bands) and it would only be implemented
>> >> >>> > by drivers that need it.
>> >> >>> >
>> >> >>> > What I am still not clear on is *why* you would want this control. What
>> >> >>> > is the reason your customers want this? What does it allow you to do that
>> >> >>> > can't be done today?
>> >> >>>
>> >> >>> There are 2 reasons for this,
>> >> >>>
>> >> >>> First, our chip supports weather band, unlike other bands (Europe,
>> >> >>> Japan and Russian) user may wants to
>> >> >>> switch to weather band and wants to listen to weather report and again
>> >> >>> switches back to normal band.
>> >> >>
>> >> >> OK, that makes sense. Are the RX and TX independent with regards to
>> >> >> band selection?
>> >> >
>> >> > Yes - RX and TX are independent of band selection
>> >> >
>> >> >> Make sure that when the band is changed the rangelow and rangehigh values
>> >> >> are also changed. If the current frequency is out of that range, then the
>> >> >> frequency should be clamped to the closest value frequency. Although an
>> >> >> alternative strategy might be to remember the last used frequency for each
>> >> >> band. That might make more sense in the case of switching between a normal
>> >> >> band and the weather band. We need to define and document which strategy
>> >> >> should be used here.
>> >> >
>> >> > As of now when I switch to new band I just set the frequency to lowest
>> >> > of the new band.
>> >> > In this way user can seek and tune to what ever channel he wants.
>> >> >
>> >> Hans,
>> >>
>> >> Which implementation you wants? start with the lowest of the new band
>> >> or closer to the frequency of old band? do we need to remember the
>> >> present frequency of the band before switching to new band?
>> >>
>> >> Please let me know your views.
>> >
>> > The initial frequency of each band is driver dependent. That's how drivers
>> > act at the moment, so we keep that.
>> >
>> > When switching bands it is best to keep the frequency closest to the old band.
>> > This makes sense when switching from e.g. US/Europe and Japan band where there
>> > is overlap in frequencies, so I think that's the best approach.
>> >
>> >> Since this feature is required by all FM receivers shall I make this
>> >> as a generic CID?
>> >
>> > Yes, but see my next question below.
>> >
>> >>
>> >> >> BTW, is the receiver for the weather band implemented as a separate receiver?
>> >> >> I read that some devices can listen to the normal band and interrupt that
>> >> >> when a weather report is broadcast on the weather band. That implies two
>> >> >> receivers and it would require a rethink.
>> >> >>
>> >> >> Also, is this feature really implemented as separate frequency ranges in
>> >> >> hardware? Or is the receiver able to receive on the whole range of frequencies
>> >> >> from 65.8 (OIRT) to approx. 165 (weather band range)?
>> >> >
>> >> > Our chip wont have 2 receivers, it has only 1 receiver which can
>> >> > receive on whole frequency range from 65 MHz to 165 MHz.
>> >
>> > If that's the case, then what does the band selector control actually have to do
>> > hardware-wise? Does it just clamp the frequency to the correct frequency range?
>>
>> The Wilink chip can't able to receive on all frequencies at a time,
>> instead first we need to set the band(need to set chip registers to do
>> so) on which receiver has to receive, after this the chip can receive
>> on frequency range of this band only.
>>
>> Eg: When set to Europe band chip can only receive from 87.5 MHz to 108
>> MHz, in this case receiver will give band limit reached interrupt if
>> tried to set frequency other than within this band.
>>
>> Band selector controller will first switch the band of reception and
>> then sets the frequency which is within that band.
>>
>> >
>> >> >
>> >> >> Is the datasheet of this device available somewhere?
>> >> >
>> >> > Sorry our newest chipset supports this feature so yet now we don't
>> >> > have any datasheet available on net.
>> >> >
>> >> >>
>> >> >>> Second, for FM TX, our chip supports band selection for FM
>> >> >>> transmitter, so if the same phone is used in different
>> >> >>> regions of world then user can switch to the actual band and start
>> >> >>> transmitting by choosing a blank frequency in that band.
>> >> >>
>> >> >> Isn't this something that can be equally easily done in userspace?
>> >> >
>> >> > you wants me to do this from driver itself without hinting the
>> >> > application about the band ?
>> >
>> > I think my question is really the same as above: what does the band selector
>> > control actually have to do for a TX hardware-wise?
>>
>> Yes TX case I accept we may not need the band switch controller,
>> instead when user wants to set a frequency, driver will see in which
>> band this frequency comes and then first switches the FM chip to
>> transmit in that band and then sets the frequency.
>>
>> So I don't add any private or public CID for this.
>
> Good.
>
>> >
>> > None of the existing radio RX and TX drivers need a band selector, because
>> > they just cover the whole frequency range. If apps want to create these bands,
>> > then they can do so themselves. So I am trying to understand why this device
>> > needs a band selector. It's why I was interested in the datasheet...
>>
>> In RX case the the above provided solution wont work because
>> 1) If user do hardware seek then seek will be performed within the
>> present band not throughout from 65MHz to 162 MHz
>> 2) If user wants to switch band he should know the frequency range of
>> the band to which he is switching and then set the frequency within
>> that band.
>>
>> Hardware seek functionality is one of the main reason why we need band
>> switch, most of the time user may not have knowledge about the
>> frequency ranges of bands so in that case use will switch to a band
>> and do hardware seek, if stations found then he will continue on the
>> same band if not he can check the next available band and do seek
>> again.
>
> How about this: rather than adding a band selection control, why not add a
> new band field to struct v4l2_hw_freq_seek? The default, 0, means just use the
> full frequency range. Then define the other bands (these are pretty much
> standardized). If a band is not supported by the hardware, -ERANGE is returned.
> Otherwise the current frequency is clamped to the specified band frequency range
> and you start searching.
>
> Band selection doesn't really make sense outside of hardware seek.
Looks fine for me... I will continue with this approach and submit the
next patch set :)
>
> Regards,
>
> Hans
> --
> 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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-03-13 15:20 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-24 20:14 [PATCH 0/3] [media] wl128x: Fixes and few new features manjunatha_halli
2012-02-24 20:14 ` [PATCH 1/3] wl128x: Fix build errors when GPIOLIB is not enabled manjunatha_halli
2012-02-24 20:14 ` [PATCH 2/3] wl128x: Add support for FM TX RDS manjunatha_halli
2012-02-24 20:14 ` [PATCH 3/3] wl128x: Add sysfs based support for FM features manjunatha_halli
2012-02-25 8:16 ` Hans Verkuil
2012-02-27 16:29 ` halli manjunatha
2012-02-28 10:05 ` Hans Verkuil
2012-02-28 22:52 ` halli manjunatha
2012-02-29 11:25 ` Hans Verkuil
2012-02-29 17:19 ` halli manjunatha
2012-03-02 9:22 ` Hans Verkuil
2012-03-05 16:24 ` halli manjunatha
2012-03-07 21:42 ` halli manjunatha
2012-03-09 8:29 ` Hans Verkuil
2012-03-09 20:44 ` halli manjunatha
2012-03-10 9:56 ` Hans Verkuil
2012-03-13 15:20 ` halli manjunatha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).