Re: [PATCH 1/2] ide: Add tx4939ide driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 12 Sep 2008 02:33:12 +0400, Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx> wrote:
> > +static void tx4939ide_dma_host_set(ide_drive_t *drive, int on)
> > +{
> > +	ide_hwif_t *hwif	= HWIF(drive);
> > +	u8 unit			= drive->dn & 1;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	u8 dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > +	if (on)
> > +		dma_stat |= (1 << (5 + unit));
> > +	else
> > +		dma_stat &= ~(1 << (5 + unit));
> > +
> > +	TX4939IDE_writeb(dma_stat, base, DMA_stat);
> > +}
> >   
> 
>    BTW, you could save on using ide_dma_host_set() in LE mode if you'd 
> set hwif->dma_base properly...

Yes.  I like endian-free approach in general, but there is already
some ifdefs in this driver.  I have no strong opinion here.

> > +static int __tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = drive->hwif;
> > +	struct request *rq = HWGROUP(drive)->rq;
> > +	unsigned int reading;
> > +	u8 dma_stat;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +
> [...]
> > +
> > +	/* read DMA status for INTR & ERROR flags */
> > +	dma_stat = TX4939IDE_readb(base, DMA_stat);
> > +
> > +	/* clear INTR & ERROR flags */
> > +	TX4939IDE_writeb(dma_stat | 6, base, DMA_stat);
> > +	/* recover intmask cleared by writing to bit2 of DMA_stat */
> > +	TX4939IDE_writew(TX4939IDE_IGNORE_INTS << 8, base, int_ctl);
> >   
> 
>    I think it might be worth factoring the BMDMA status clearing code 
> into a separate function...

OK.  Good idea.

> > +static int tx4939ide_dma_setup(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = HWIF(drive);
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	int is_slave = drive->dn & 1;
> > +	unsigned int nframes;
> > +	int rc, i;
> > +	unsigned int sect_size = queue_hardsect_size(drive->queue);
> > +	u16 select_data;
> > +
> > +	select_data = (hwif->select_data >> (is_slave ? 16 : 0)) & 0xffff;
> > +	TX4939IDE_writew(select_data, base, Sys_Ctl);
> > +	if (is_slave)
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_2);
> > +	else
> > +		TX4939IDE_writew(sect_size / 2, base, Xfer_Cnt_1);
> > +
> > +	rc = __tx4939ide_dma_setup(drive);
> 
>    Hm, I think you need to call this earlier and do an early exit if it 
> returns 1.
> 
> > +	if (rc == 0) {
> >   
> 
>    Better check for !=0, return early and avoid unnecessary 
> indentatiuon, isn't it?

It mighet be better.  I'll try it.

> > +static void tx4939ide_dma_start(ide_drive_t *drive)
> > +{
> > +	ide_hwif_t *hwif = drive->hwif;
> > +	unsigned long base = TX4939IDE_BASE(hwif);
> > +	u8 dma_cmd;
> > +
> > +	/* Note that this is done *after* the cmd has
> > +	 * been issued to the drive, as per the BM-IDE spec.
> > +	 */
> > +	dma_cmd = TX4939IDE_readb(base, DMA_Cmd);
> > +	/* start DMA */
> > +	TX4939IDE_writeb(dma_cmd | 1, base, DMA_Cmd);
> > +
> > +	wmb();
> > +}
> >   
> 
>    You could save by using ide_dma_start() in LE mode too...

Ditto.

> > +#ifdef __BIG_ENDIAN
> > +/* custom iops (independent from SWAP_IO_SPACE) */
> > +static u8 mm_inb(unsigned long port)
> > +{
> > +	return (u8)readb((void __iomem *)port);
> > +}
> > +static void mm_outb(u8 value, unsigned long port)
> > +{
> > +	writeb(value, (void __iomem *)port);
> > +}
> >   
> 
>    I'm not sure readb()/writeb() are good substitute for 
> __raw_readb()/__raw_writeb() as __swizzle_addr_b() might be actually 
> changing the address...

__swizzle_addr_b() used for both readb() and __raw_readb().  ioswabb()
is used for readb() and __raw_ioswabb() is used for __raw_readb().
Hm, __raw_readb() might be better for consistency, though I cannot
imagine any custom ioswabb() ;-)

---
Atsushi Nemoto
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux