From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([95.142.166.194]:35190 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756017Ab2DQKGF (ORCPT ); Tue, 17 Apr 2012 06:06:05 -0400 From: Laurent Pinchart To: davinci-linux-open-source@linux.davincidsp.com Cc: Manjunath Hadli , LMML Subject: Re: [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr Date: Tue, 17 Apr 2012 12:06:16 +0200 Message-ID: <3282000.92FtfC8Du0@avalon> In-Reply-To: <1334652791-15833-2-git-send-email-manjunath.hadli@ti.com> References: <1334652791-15833-1-git-send-email-manjunath.hadli@ti.com> <1334652791-15833-2-git-send-email-manjunath.hadli@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Manjunath, Thanks for the patch. On Tuesday 17 April 2012 14:22:59 Manjunath Hadli wrote: > As the same interrupt is shared between capture and display devices, > sometimes we get isr calls where the interrupt might not genuinely belong > to capture or display. Hence, add a condition in the isr to check for > interrupt ownership and channel number to make sure we do not > service wrong interrupts. > > Signed-off-by: Manjunath Hadli > --- > drivers/media/video/davinci/vpif_capture.c | 5 +++++ > drivers/media/video/davinci/vpif_display.c | 5 +++++ > include/media/davinci/vpif_types.h | 2 ++ > 3 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/davinci/vpif_capture.c > b/drivers/media/video/davinci/vpif_capture.c index 6504e40..33d865d 100644 > --- a/drivers/media/video/davinci/vpif_capture.c > +++ b/drivers/media/video/davinci/vpif_capture.c > @@ -333,6 +333,7 @@ static void vpif_schedule_next_buffer(struct common_obj > *common) */ > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) > { > + struct vpif_capture_config *config = vpif_dev->platform_data; > struct vpif_device *dev = &vpif_obj; > struct common_obj *common; > struct channel_obj *ch; > @@ -341,6 +342,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > *dev_id) int fid = -1, i; > > channel_id = *(int *)(dev_id); > + if (!config->intr_status || > + !config->intr_status(vpif_base, channel_id)) > + return IRQ_NONE; > + I don't think this is the right way to handle the situation. You should instead read the interrupt source register for the VPIF capture device, and return IRQ_NONE if you find that no interrupt source has been flagged (and similarly for the display device below). > ch = dev->dev[channel_id]; > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > diff --git a/drivers/media/video/davinci/vpif_display.c > b/drivers/media/video/davinci/vpif_display.c index 7fa34b4..9b59e0b 100644 > --- a/drivers/media/video/davinci/vpif_display.c > +++ b/drivers/media/video/davinci/vpif_display.c > @@ -299,6 +299,7 @@ static void process_interlaced_mode(int fid, struct > common_obj *common) */ > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) > { > + struct vpif_display_config *config = vpif_dev->platform_data; > struct vpif_device *dev = &vpif_obj; > struct channel_obj *ch; > struct common_obj *common; > @@ -307,6 +308,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > *dev_id) int channel_id = 0; > > channel_id = *(int *)(dev_id); > + if (!config->intr_status || > + !config->intr_status(vpif_base, channel_id + 2)) > + return IRQ_NONE; > + > ch = dev->dev[channel_id]; > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > for (i = 0; i < VPIF_NUMOBJECTS; i++) { > diff --git a/include/media/davinci/vpif_types.h > b/include/media/davinci/vpif_types.h index bd8217c..2845bda 100644 > --- a/include/media/davinci/vpif_types.h > +++ b/include/media/davinci/vpif_types.h > @@ -45,6 +45,7 @@ struct vpif_subdev_info { > > struct vpif_display_config { > int (*set_clock)(int, int); > + int (*intr_status)(void __iomem *vpif_base, int); > struct vpif_subdev_info *subdevinfo; > int subdev_count; > const char **output; > @@ -65,6 +66,7 @@ struct vpif_capture_chan_config { > struct vpif_capture_config { > int (*setup_input_channel_mode)(int); > int (*setup_input_path)(int, const char *); > + int (*intr_status)(void __iomem *vpif_base, int); > struct vpif_capture_chan_config chan_config[VPIF_CAPTURE_MAX_CHANNELS]; > struct vpif_subdev_info *subdev_info; > int subdev_count; -- Regards, Laurent Pinchart