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

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Alan Cox wrote:

>>+#define TX4939IDE_readl(base, reg) \
>>+	__raw_readl((void __iomem *)((base) + TX4939IDE_REG32(reg)))
>>+#define TX4939IDE_readw(base, reg) \
>>+	__raw_readw((void __iomem *)((base) + TX4939IDE_REG16(reg)))
>>+#define TX4939IDE_readb(base, reg) \
>>+	__raw_readb((void __iomem *)((base) + TX4939IDE_REG8(reg)))
>>+#define TX4939IDE_writel(val, base, reg) \
>>+	__raw_writel(val, (void __iomem *)((base) + TX4939IDE_REG32(reg)))
>>+#define TX4939IDE_writew(val, base, reg) \
>>+	__raw_writew(val, (void __iomem *)((base) + TX4939IDE_REG16(reg)))
>>+#define TX4939IDE_writeb(val, base, reg) \
>>+	__raw_writeb(val, (void __iomem *)((base) + TX4939IDE_REG8(reg)))

> It's generally frowned upon to hide all the detail in macros, it is much
> easier to read and understand the code if you don't do this.

>>+#define TX4939IDE_BASE(hwif)	((hwif)->io_ports.data_addr & ~0xfff)

> Why do you have void __iomem casts all over the write methods not in the
> _BASE() method - that would let sparse do its job properly

    I don't get why there's need for & at all -- isn't IDE data register 
address always on 4K boundary?

>>+	for (i = 0; i < MAX_DRIVES; i++) {
>>+		if (drive != &hwif->drives[i] &&

> You don't actually need the first test.

    No, he does need it -- in order not to clamp the new PIO mode based on the 
previosly selected one. Although, one should call ide_get_paired_drive() ISO 
this loop.

> This also appears wrong. In your
> tests MW_DMA_0 is 'faster' than PIO4 but in fact MW_DMA_0 PIO timings are
> *slower* than PIO4 so the mode is not in fact slower.

    I don't think it's about the DMA timings at all. Though indeed, MWDMA0/1 
do (iff it's drive's max) implies slower max PIO mode than PIO4.

>>+	case XFER_MW_DMA_2:
>>+	case XFER_MW_DMA_1:
>>+	case XFER_MW_DMA_0:
>>+	case XFER_PIO_4:
>>+		value |= 0x0400;
>>+		break;

> This looks odd according to the speed tables. Can you clarify what is
> going on ?

    This apparently selects the command PIO timing safest for both drives but 
does this incorrectly -- the current DMA (or even PIO) mode shouldn't be a 
part of the equation.  There are several examples how to do this including 
siimage.c and cs5535.c...

MBR, Sergei