From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Garzik Subject: Re: [PATCH] pata_cmd640: don't read CFR pointlessly Date: Sat, 08 May 2010 17:27:24 -0400 Message-ID: <4BE5D73C.30707@pobox.com> References: <201005082227.19096.sshtylyov@ru.mvista.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-vw0-f46.google.com ([209.85.212.46]:39336 "EHLO mail-vw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751905Ab0EHV11 (ORCPT ); Sat, 8 May 2010 17:27:27 -0400 Received: by vws3 with SMTP id 3so619295vws.19 for ; Sat, 08 May 2010 14:27:26 -0700 (PDT) In-Reply-To: <201005082227.19096.sshtylyov@ru.mvista.com> Sender: linux-ide-owner@vger.kernel.org List-Id: linux-ide@vger.kernel.org To: Sergei Shtylyov Cc: linux-ide@vger.kernel.org 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 > > --- > 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