Patchwork [82/86] pata_via: clear UDMA transfer mode bit for PIO and MWDMA

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date Nov. 25, 2009, 5:58 p.m.
Message ID <200911251858.38491.bzolnier@gmail.com>
Download mbox | patch
Permalink /patch/39414/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - Nov. 25, 2009, 5:58 p.m.
On Wednesday 25 November 2009 06:52:57 pm Alan Cox wrote:
> >  	/* Set UDMA unless device is not UDMA capable */
> > -	if (udma_type && t.udma) {
> > -		u8 cable80_status;
> > +	if (udma_type) {
> > +		u8 udma_etc;
> 
> This seems odd the original test is that we are a udma controller and
> have a udma timing specified. Otherwise there is no point doing any work.

The patch is based on the data sheet and is needed.

We may not hit the problem with the current chipset tuning model but things
are not set in the stone and one day we may pay a price for embedding such
undocumented assumptions into our drivers.

> The rest seems to be gratuitious register renaming and makes the patches
> hard to review. Please split this sort of stuff up.

The name now matches documentation.

Do you really find such tidy patch hard to review?

From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] pata_via: clear UDMA transfer mode bit for PIO and MWDMA

Fix register naming while at it.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/pata_via.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)



--
Bartlomiej Zolnierkiewicz
--
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
Alan Cox - Nov. 25, 2009, 6:06 p.m.
> Do you really find such tidy patch hard to review?

When doing it at speed - yes.

>  	/* Set UDMA unless device is not UDMA capable */
> -	if (udma_type && t.udma) {
> -		u8 cable80_status;
> +	if (udma_type) {
> +		u8 udma_etc;

Ok that makes sense - we in fact could hit this case on a failure
changedown from UDMA to PIO.
--
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
Bartlomiej Zolnierkiewicz - Nov. 25, 2009, 6:10 p.m.
On Wednesday 25 November 2009 07:06:48 pm Alan Cox wrote:
> > Do you really find such tidy patch hard to review?
> 
> When doing it at speed - yes.

Do you review everybody else patches 'at speed' or only mine?

--
Bartlomiej Zolnierkiewicz
--
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
Alan Cox - Nov. 25, 2009, 7:34 p.m.
On Wed, 25 Nov 2009 19:10:06 +0100
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> On Wednesday 25 November 2009 07:06:48 pm Alan Cox wrote:
> > > Do you really find such tidy patch hard to review?
> > 
> > When doing it at speed - yes.
> 
> Do you review everybody else patches 'at speed' or only mine?

All of them when I've got a lot of stuff to do. Owing to a lack of time
machine and 500 hour days it's neccessary.

Also btw take it is a compliment - there are some people's patches I
always review in detail and it isn't the patches you expect to be *right*
that you spend the time on...

--
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
Bartlomiej Zolnierkiewicz - Nov. 25, 2009, 7:53 p.m.
On Wednesday 25 November 2009 08:34:19 pm Alan Cox wrote:
> On Wed, 25 Nov 2009 19:10:06 +0100
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > On Wednesday 25 November 2009 07:06:48 pm Alan Cox wrote:
> > > > Do you really find such tidy patch hard to review?
> > > 
> > > When doing it at speed - yes.
> > 
> > Do you review everybody else patches 'at speed' or only mine?
> 
> All of them when I've got a lot of stuff to do. Owing to a lack of time
> machine and 500 hour days it's neccessary.
> 
> Also btw take it is a compliment - there are some people's patches I
> always review in detail and it isn't the patches you expect to be *right*
> that you spend the time on...

Well, you don't really have to review every single patch touching areas
that  you're just interested in (and if you're maintaining those drivers
please document it in MAINTAINERS file, thanks).

The problem is that in case you're not entirely correct, which is much
more likely when doing things 'at speed', you're will be wasting time
for everybody (not that I really care that much about  others' time but
I do care about mine).

--
Bartlomiej Zolnierkiewicz
--
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: b/drivers/ata/pata_via.c
===================================================================
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -296,14 +296,21 @@  static void via_do_set_mode(struct ata_p
 	}
 
 	/* Set UDMA unless device is not UDMA capable */
-	if (udma_type && t.udma) {
-		u8 cable80_status;
+	if (udma_type) {
+		u8 udma_etc;
 
-		/* Get 80-wire cable detection bit */
-		pci_read_config_byte(pdev, 0x50 + offset, &cable80_status);
-		cable80_status &= 0x10;
+		pci_read_config_byte(pdev, 0x50 + offset, &udma_etc);
 
-		pci_write_config_byte(pdev, 0x50 + offset, ut | cable80_status);
+		/* clear transfer mode bit */
+		udma_etc &= ~0x20;
+
+		if (t.udma) {
+			/* preserve 80-wire cable detection bit */
+			udma_etc &= 0x10;
+			udma_etc |= ut;
+		}
+
+		pci_write_config_byte(pdev, 0x50 + offset, udma_etc);
 	}
 }