Re: [PATCH 1/9] cmd64x: implement clear_irq() method

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Friday 12 June 2009 20:39:51 Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> > Hi Sergei,
> >
> > This is a very nice series 
> 
>    Oh, I'm flattened. :-)
> 
> > and I would like to apply it immediately
> > but unfortunately there is quite a few issues to be addressed first..
> >   
> 
>    Sigh, it's a pity -- I'm now on vacation and won't be available since 
> this Sunday... I should have posted it earlier of course. Will have to 
> try to address your comments tomorrow then.
> 
> >> Convert the driver's two dma_end() methods into clear_irq() methods -- the
> >> driver will now use the standard dma_end() method implementation, ide_dma_end().
> >>
> >> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> >>
> >> ---
> >> The patch is atop of ide-2.6.git 'for-next' branch.
> >>
> >>  drivers/ide/cmd64x.c |   31 +++++++++++++++++--------------
> >>  1 files changed, 17 insertions(+), 14 deletions(-)
> >>     
> >
> > [...]
> >   
> >> @@ -226,11 +226,10 @@ static void cmd64x_set_dma_mode(ide_driv
> >>  		(void) pci_write_config_byte(dev, pciU, regU);
> >>  }
> >>  
> >> -static int cmd648_dma_end(ide_drive_t *drive)
> >> +static void cmd648_clear_irq(ide_drive_t *drive)
> >>  {
> >>  	ide_hwif_t *hwif	= drive->hwif;
> >>  	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
> >>     
> >
> > Don't we need to check whether hwif->dma_base is valid now?
> >   
> 
>    You're right, I have managed to overlook this. I'll change this to 
> pci_resource_start() call instead...

Currently this driver should operate fine without BAR4 set so even if
this is pci_resource_start(), the return value still needs to be checked
against 0 -- it is the reliability/maintainability issue.

> > Also shouldn't this patch be done after fixing core code first?
> >   
> 
>    Fixing what exactly, ide_timer_expiry()?

Yes.  Currently IRQ is cleared in ide_timer_expiry()'s code-path:

 -> hwif->handler [ ide_dma_intr() ]
     -> dma_ops->dma_end [ cmd64{x,8}_dma_end() ]

After this patch it won't be.