Re: [PATCH] fix pata-rb532-cf

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

Hello, I wrote:

>> After applying the following changes I could verify functionality by
>> mounting a filesystem on the cfdisk and reading/writing files in it.
>>
>> The symbols rb532_gpio_set_ilevel and rb532_gpio_set_istat are not yet
>> available in a vanilla kernel, an appropriate patch has already been
>> sent to the linux-mips mailinglist.
>>
>> Also change rb532_pata_data_xfer() so it reads and writes 4-byte blocks,
>> like the original driver did. Rename the offset definition of the
>> buffered data register for clearness.
>>   
>
>   Looks ike I'll have to NAK this part...

   I'd advise to break up the patch since you're fixing 2 unrelated 
things now...

>> index f8b3ffc..bdf413e 100644
>> --- a/drivers/ata/pata_rb532_cf.c
>> +++ b/drivers/ata/pata_rb532_cf.c
>>   
> diff --git a/drivers/ata/pata_rb532_cf.c b/drivers/ata/pata_rb532_cf.c
> [...]
>> @@ -39,9 +40,11 @@
>>  #define RB500_CF_MAXPORTS    1
>>  #define RB500_CF_IO_DELAY    400
>>  
>> -#define RB500_CF_REG_CMD    0x0800
>> +#define RB500_CF_REG_BASE    0x0800
>>  #define RB500_CF_REG_CTRL    0x080E
>> -#define RB500_CF_REG_DATA    0x0C00
>> +/* 32bit buffered data register offset */
>> +#define RB500_CF_REG_DBUF32    0x0C00
>> +#define RB500_CF_REG_ERR    0x080D
>>   
>
>   Wouldn't hurt to have the macros in the ascending address order...
>
>> @@ -72,21 +75,26 @@ static void rb532_pata_exec_command(struct 
>> ata_port *ap,
>>      rb532_pata_finish_io(ap);
>>  }
>>  
>> -static void rb532_pata_data_xfer(struct ata_device *adev, unsigned 
>> char *buf,
>> +static unsigned int rb532_pata_data_xfer(struct ata_device *adev, 
>> unsigned char *buf,
>>                  unsigned int buflen, int write_data)
>>  {
>> +    int i;

  I'd have made this the last variable in the declartion block...

>>      struct ata_port *ap = adev->link->ap;
>>      void __iomem *ioaddr = ap->ioaddr.data_addr;
>>  
>> +    BUG_ON(buflen % sizeof(u32));

   Well, if there's no chance for an ATAPI device to be connected...

>> +
>>      if (write_data) {
>> -        for (; buflen > 0; buflen--, buf++)
>> -            writeb(*buf, ioaddr);
>> +        for(i = 0; i < buflen / sizeof(u32); i++)
>> +            writel(((u32 *)buf)[i], ioaddr);
>>      } else {
>> -        for (; buflen > 0; buflen--, buf++)
>> -            *buf = readb(ioaddr);
>> +        for(i = 0; i < buflen / sizeof(u32); i++)
>> +            ((u32 *)buf)[i] = readl(ioaddr);
>>      }
>
>   So, I didn't get what was wrong with using readsl() and writesl()?
>   Besides, using readl() and witel() this way would be wrong on BE 
> mode since the data is expected to be stored to memory in the LE order.
>

   Hm, I'seeing that RB532 only supports LE it seems -- though I wodner 
why it also selects CONFIG_SWAP_IO_SPACE which shouldn't affect anything 
in LE mode...
Anyway, using readl()/writel() this way is wrong...

MBR, Sergei