Re: [PATCH] pata_cmd640: don't read CFR pointlessly

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

Jeff Garzik wrote:

>> cmd640_hardware_init() reads CFR but doesn't use the value read...
>>
>> Signed-off-by: Sergei Shtylyov<sshtylyov@ru.mvista.com>
>>
>> ---
>> The patch is against the recent Linus' tree.
>>
>>   drivers/ata/pata_cmd640.c |    3 ---
>>   1 file changed, 3 deletions(-)
>>
>> Index: linux-2.6/drivers/ata/pata_cmd640.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/ata/pata_cmd640.c
>> +++ linux-2.6/drivers/ata/pata_cmd640.c
>> @@ -181,13 +181,10 @@ static struct ata_port_operations cmd640
>>
>>   static void cmd640_hardware_init(struct pci_dev *pdev)
>>   {
>> -    u8 r;
>>       u8 ctrl;
>>
>>       /* CMD640 detected, commiserations */
>>       pci_write_config_byte(pdev, 0x5B, 0x00);
>> -    /* Get version info */
>> -    pci_read_config_byte(pdev, CFR,&r);
>>       /* PIO0 command cycles */
>
> Agreed with this patch, but we lose some information:  all that 
> remains is an uncommented 'CFR' definition.  Without the code comment, 
> which is deleted in this patch, we have no idea what that register does.

   It does several different things.

> Given that the chipset docs are not public AFAIK,

   They were public once. And I think I've given you that spec along 
with other PCI-64x specs for publishing on 
gkernel.sourceforge.net/specs/ already (can't open 
sii/cmd64x-specs.tar.bz2 now to check).

> this is a net loss of information about this chipset, IMO.
>
> Maybe revise this patch, or add a second patch, which adds a comment 
> where 'CFR' is defined?  Something as simple as "/* version info */" 
> would be sufficient.

   That wouldn't be a good description. The "official" name of the 
register is configuration register.

>     Jeff

WBR, Sergei