Re: [PATCH] fix pata-rb532-cf

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

Hello, I wrote:

>>>> -#define RB500_CF_REG_CMD    0x0800
>>>> +#define RB500_CF_REG_BASE    0x0800
>>>> #define RB500_CF_REG_CTRL    0x080E
>>>> #define RB500_CF_REG_DATA    0x0C00

   This is actually not the IDE data register itself but some data 
buffer register and supporting 32-bit I/O (and presumably implementing 
read prefetch)...

>>>> -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)
>>>> {
>>>>     struct ata_port *ap = adev->link->ap;
>>>>     void __iomem *ioaddr = ap->ioaddr.data_addr;
>>>> +    unsigned int ret = buflen;
>>>>
>>>>     if (write_data) {
>>>>         for (; buflen > 0; buflen--, buf++)
>>>> @@ -87,6 +94,8 @@ static void rb532_pata_data_xfer(struct 
>>>> ata_device *adev, unsigned char *buf,
>>>>     }
>>>>
>>>>     rb532_pata_finish_io(adev->link->ap);
>>>> +
>>>> +    return ret;
>>>>  
>>>>       
>>>   Totally unneeded variable.
>>>     
>>
>> I wanted to preserve the behaviour of ata_sff_data_xfer(), i.e.
>> returning the number of bytes written. The variable exists, because
>> buflen is being decremented in the loop and therefore zero afterwards.
>>   
>
>   Oops, sorry -- I've managed to miss that decrement... :-<
>
>> Returning a static value also should be no alternative, as it could lead
>> to unexpected behaviour on the caller side. So, do I miss something?
>>   
>
>   No. The alternatives would be to use local variable as a loop 
> counter or, better yet, using readsb()/writesb() instead of the loops...
> Wait! The original driver used 32-bit I/O to this register, not 8-bit 
> -- so it looks like you have artificially slowed it down... :-/

   Sorry again -- looks like it was Florian who was the author of the 
original driver...

MBR, Sergei