Re: [PATCH v4 3/6] drivers:staging: ti-st: fmdrv_common sources

2010-12-09 Thread halli manjunatha 
Hans,

On Thu, Nov 18, 2010 at 1:49 PM, Hans Verkuil hverk...@xs4all.nl wrote:

 
  These are the sources for the common interfaces required by the FM
  V4L2 driver for TI WL127x and WL128x chips.
 
snip...

 OK, I think the way interrupts are handled should be revamped. It is way too
 complex IMHO. All these action/response handlers have a similar structure,
 particularly for the response handlers as this part of the code is the same
 for all as far as I can tell:

        del_timer(fmdev-irq_info.int_timeout_timer);

        ret = __check_cmdresp_status(fmdev, skb);
        if (ret  0) {
                pr_err((fmdrv): Initiating irq recovery process\n);
                mod_timer(fmdev-irq_info.int_timeout_timer, jiffies +
                          FM_DRV_TX_TIMEOUT);
                return;
        }

 What I think should happen is that rather than having each handler chain the
 other there is one core handler that is taking care of that. So the 
 boilerplate
 code is in that core handler and it is calling the other handlers. So e.g. the
 hw_malfunction handler below could become something like this:

 static int fm_irq_handle_hw_malfunction(struct fmdrv_ops *fmdev, unsigned 
 events)
 {
        if (!(events  FM_MAL_EVENT))
                pr_err((fmdrv): irq: HW MAL int received - do nothing\n);
        return FM_RDS_START_INDEX;
 }

 And unsigned events is set to fmdev-irq_info.flag  fmdev-irq_info.mask.

 This would refactor out a lot of code.

I tried out similar ways of handling interrupts and also tried out
having a single unified
interrupt handler/core handler and branching out onto various handlers
based on the
events received.
However, the way chip tends to send interrupts is slightly more complex here.

We have cases where 2 or more events are combined together which would
cause the FM interrupt, So when I do a FLAG_GET to request the cause
of interrupt, I will have to always check for each of the individual
bits (13 bits...)
More over when a bit is set say, RDS, and I am in the middle of doing
a GET_RDS, I have more interrupts coming in, which requires me to do a
FLAG_GET (generally a lot of low rssi, stereo/mono events..)

So at the moment all we can do is push the following into a function
and thereby reduce a bit of code size - But not complexity.

del_timer(fmdev-irq_info.int_timeout_timer);

ret = __check_cmdresp_status(fmdev, skb);
if (ret  0) {
pr_err((fmdrv): Initiating irq recovery process\n);
mod_timer(fmdev-irq_info.int_timeout_timer, jiffies +
  FM_DRV_TX_TIMEOUT);
return;
}

So, Please suggest on how best such situation where multiple events
have caused 1 interrupt
can be handled ?

example:
After checking HW_MAL function, I begin to check for RDS - this is
where the handler branches out,

check hw malfunction
|
check rds-
|  |
  true   false--- check for tune_op_ended  check
for Tx power enabled
|
send rds get command
|
rds data response
|
rds finish  check for tune_op_ended.--- check for Tx
power enabled...

regards,
Manjunatha,

  +static void fm_irq_handle_hw_malfunction(void *arg)
  +{
  +     struct fmdrv_ops *fmdev;
  +
  +     fmdev = arg;
  +     if (fmdev-irq_info.flag  FM_MAL_EVENT  fmdev-irq_info.mask)
  +             pr_err((fmdrv): irq: HW MAL int received - do nothing\n);
  +
  +     /* Continue next function in interrupt handler table */
  +     fmdev-irq_info.stage_index = FM_RDS_START_INDEX;
  +     fmdev-irq_info.fm_int_handlers[fmdev-irq_info.stage_index](fmdev);
  +}

 Regards,

        Hans

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

Re: [PATCH v4 3/6] drivers:staging: ti-st: fmdrv_common sources

2010-11-18 Thread Hans Verkuil 
On Tuesday, November 16, 2010 14:18:11 manjunatha_ha...@ti.com wrote:
 From: Manjunatha Halli manjunatha_ha...@ti.com
 
 These are the sources for the common interfaces required by the FM
 V4L2 driver for TI WL127x and WL128x chips.
 
 These implement the FM channel-8 protocol communication with
 the chip. This makes use of the Shared Transport as its transport.
 
 Signed-off-by: Manjunatha Halli manjunatha_ha...@ti.com
 ---
  drivers/staging/ti-st/fmdrv_common.c | 2141 
 ++
  drivers/staging/ti-st/fmdrv_common.h |  458 
  2 files changed, 2599 insertions(+), 0 deletions(-)
  create mode 100644 drivers/staging/ti-st/fmdrv_common.c
  create mode 100644 drivers/staging/ti-st/fmdrv_common.h
 
 diff --git a/drivers/staging/ti-st/fmdrv_common.c 
 b/drivers/staging/ti-st/fmdrv_common.c
 new file mode 100644
 index 000..7b8f2da
 --- /dev/null
 +++ b/drivers/staging/ti-st/fmdrv_common.c
 @@ -0,0 +1,2141 @@
 +/*
 + *  FM Driver for Connectivity chip of Texas Instruments.
 + *
 + *  This sub-module of FM driver is common for FM RX and TX
 + *  functionality. This module is responsible for:
 + *  1) Forming group of Channel-8 commands to perform particular
 + * functionality (eg., frequency set require more than
 + * one Channel-8 command to be sent to the chip).
 + *  2) Sending each Channel-8 command to the chip and reading
 + * response back over Shared Transport.
 + *  3) Managing TX and RX Queues and Tasklets.
 + *  4) Handling FM Interrupt packet and taking appropriate action.
 + *  5) Loading FM firmware to the chip (common, FM TX, and FM RX
 + * firmware files based on mode selection)
 + *
 + *  Copyright (C) 2010 Texas Instruments
 + *  Author: Raja Mani raja_m...@ti.com
 + *
 + *  This program is free software; you can redistribute it and/or modify
 + *  it under the terms of the GNU General Public License version 2 as
 + *  published by the Free Software Foundation.
 + *
 + *  This program is distributed in the hope that it will be useful,
 + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + *  GNU General Public License for more details.
 + *
 + *  You should have received a copy of the GNU General Public License
 + *  along with this program; if not, write to the Free Software
 + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 + *
 + */
 +
 +#include linux/module.h
 +#include linux/firmware.h
 +#include linux/delay.h
 +#include fmdrv.h
 +#include fmdrv_v4l2.h
 +#include fmdrv_common.h
 +#include linux/ti_wilink_st.h
 +#include fmdrv_rx.h
 +#include fmdrv_tx.h
 +
 +/* FM chip register table */
 +static struct fm_reg_table fm_reg_info[] = {
 + /* - FM RX registers ---*/
 + /* opcode, type(rd/wr), reg name */
 + {0x00, REG_RD, STEREO_GET},
 + {0x01, REG_RD, RSSI_LVL_GET},
 + {0x02, REG_RD, IF_COUNT_GET},
 + {0x03, REG_RD, FLAG_GET},
 + {0x04, REG_RD, RDS_SYNC_GET},
 + {0x05, REG_RD, RDS_DATA_GET},
 + {0x0a, REG_WR, FREQ_SET},
 + {0x0a, REG_RD, FREQ_GET},
 + {0x0b, REG_WR, AF_FREQ_SET},
 + {0x0b, REG_RD, AF_FREQ_GET},
 + {0x0c, REG_WR, MOST_MODE_SET},
 + {0x0c, REG_RD, MOST_MODE_GET},
 + {0x0d, REG_WR, MOST_BLEND_SET},
 + {0x0d, REG_RD, MOST_BLEND_GET},
 + {0x0e, REG_WR, DEMPH_MODE_SET},
 + {0x0e, REG_RD, DEMPH_MODE_GET},
 + {0x0f, REG_WR, SEARCH_LVL_SET},
 + {0x0f, REG_RD, SEARCH_LVL_GET},
 + {0x10, REG_WR, RX_BAND_SET},
 + {0x10, REG_RD, RX_BAND_GET},
 + {0x11, REG_WR, MUTE_STATUS_SET},
 + {0x11, REG_RD, MUTE_STATUS_GET},
 + {0x12, REG_WR, RDS_PAUSE_LVL_SET},
 + {0x12, REG_RD, RDS_PAUSE_LVL_GET},
 + {0x13, REG_WR, RDS_PAUSE_DUR_SET},
 + {0x13, REG_RD, RDS_PAUSE_DUR_GET},
 + {0x14, REG_WR, RDS_MEM_SET},
 + {0x14, REG_RD, RDS_MEM_GET},
 + {0x15, REG_WR, RDS_BLK_B_SET},
 + {0x15, REG_RD, RDS_BLK_B_GET},
 + {0x16, REG_WR, RDS_MSK_B_SET},
 + {0x16, REG_RD, RDS_MSK_B_GET},
 + {0x17, REG_WR, RDS_PI_MASK_SET},
 + {0x17, REG_RD, RDS_PI_MASK_GET},
 + {0x18, REG_WR, RDS_PI_SET},
 + {0x18, REG_RD, RDS_PI_GET},
 + {0x19, REG_WR, RDS_SYSTEM_SET},
 + {0x19, REG_RD, RDS_SYSTEM_GET},
 + {0x1a, REG_WR, INT_MASK_SET},
 + {0x1a, REG_RD, INT_MASK_GET},
 + {0x1b, REG_WR, SRCH_DIR_SET},
 + {0x1b, REG_RD, SRCH_DIR_GET},
 + {0x1c, REG_WR, VOLUME_SET},
 + {0x1c, REG_RD, VOLUME_GET},
 + {0x1d, REG_WR, AUDIO_ENABLE(SET)},
 + {0x1d, REG_RD, AUDIO_ENABLE(GET)},
 + {0x1e, REG_WR, PCM_MODE_SET},
 + {0x1e, REG_RD, PCM_MODE_SET},
 + {0x1f, REG_WR, I2S_MD_CFG_SET},
 + {0x1f, REG_RD, I2S_MD_CFG_GET},
 + {0x20, REG_WR, POWER_SET},
 + {0x20, REG_RD, POWER_GET},
 + {0x21, REG_WR, INTx_CONFIG_SET},
 + {0x21, REG_RD, INTx_CONFIG_GET},
 + {0x22, REG_WR, PULL_EN_SET},
 + {0x22, REG_RD, PULL_EN_GET},
 + {0x23, REG_WR, HILO_SET},
 +