From: halli manjunatha <manjunatha_halli@ti.com>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: mchehab@infradead.org, linux-kernel@vger.kernel.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH v4 3/6] drivers:staging: ti-st: fmdrv_common sources
Date: Fri, 10 Dec 2010 11:48:45 +0530 [thread overview]
Message-ID: <AANLkTik08q3Fz4m8jZRR4+wEihyYXr+6uQecZ36EV-AD@mail.gmail.com> (raw)
In-Reply-To: <201011180919.23001.hverkuil@xs4all.nl>
Hans,
On Thu, Nov 18, 2010 at 1:49 PM, Hans Verkuil <hverkuil@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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2010-12-10 6:19 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-11-16 13:18 [PATCH v4 0/6] FM V4L2 drivers for WL128x manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 1/6] drivers:staging: ti-st: fmdrv common header file manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 2/6] drivers:staging: ti-st: fmdrv_v4l2 sources manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 3/6] drivers:staging: ti-st: fmdrv_common sources manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 4/6] drivers:staging: ti-st: fmdrv_rx sources manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 5/6] drivers:staging: ti-st: fmdrv_tx sources manjunatha_halli
2010-11-16 13:18 ` [PATCH v4 6/6] drivers:staging: ti-st: Kconfig & Makefile change manjunatha_halli
2010-11-18 8:34 ` [PATCH v4 4/6] drivers:staging: ti-st: fmdrv_rx sources Hans Verkuil
2010-11-18 8:19 ` [PATCH v4 3/6] drivers:staging: ti-st: fmdrv_common sources Hans Verkuil
2010-12-10 6:18 ` halli manjunatha [this message]
2010-11-17 1:24 ` [PATCH v4 2/6] drivers:staging: ti-st: fmdrv_v4l2 sources Figo.zhang
2010-11-18 7:49 ` Hans Verkuil
2010-11-18 7:44 ` [PATCH v4 1/6] drivers:staging: ti-st: fmdrv common header file Hans Verkuil
2010-11-17 13:02 ` [PATCH v4 0/6] FM V4L2 drivers for WL128x Ohad Ben-Cohen
2010-11-18 5:54 ` Pavan Savoy
2010-11-18 7:17 ` Hans Verkuil
2010-11-18 8:35 ` Ohad Ben-Cohen
2010-11-18 10:26 ` Savoy, Pavan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AANLkTik08q3Fz4m8jZRR4+wEihyYXr+6uQecZ36EV-AD@mail.gmail.com \
--to=manjunatha_halli@ti.com \
--cc=hverkuil@xs4all.nl \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).