Patchwork pata_cmd640: don't read CFR pointlessly

login
register
mail settings
Submitter Sergei Shtylyov
Date May 8, 2010, 6:27 p.m.
Message ID <201005082227.19096.sshtylyov@ru.mvista.com>
Download mbox | patch
Permalink /patch/51973/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Sergei Shtylyov - May 8, 2010, 6:27 p.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - May 8, 2010, 9:27 p.m.
On 05/08/2010 02:27 PM, Sergei Shtylyov 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.  Given 
that the chipset docs are not public AFAIK, 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.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - May 8, 2010, 9:38 p.m.
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

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - May 8, 2010, 9:44 p.m.
On 05/08/2010 05:38 PM, Sergei Shtylyov wrote:
> 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).

Based on this, I retract my comment.  Will apply patch as-is.

cmd64x-specs.tar was missing from my local doc archive, and I naively 
assumed everything in my public doc archive (gkernel.sf.net/specs/) was 
also stored locally.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - May 14, 2010, 9:32 p.m.
On 05/08/2010 02:27 PM, Sergei Shtylyov 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(-)

applied


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

Patch

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 */
 	pci_write_config_byte(pdev, CMDTIM, 0);
 	/* 512 byte bursts (sector) */