Patch: slight shrink of vt6421_init_addrs()

Patch: slight shrink of vt6421_init_addrs()

[Adam J. Richter]

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

Hi Tejun and Jeff,

	The discussion about the vt6421 problems caused me to glance
at the code and try a very minor clean-up to vt6421_init_addrs().
Please note that I don't have a vt6421, so this patch is UNTESTED.
The patch does two things:

	1. Elminate the bmdma_addr variable, which was only used once,
	   in an assignment to a field also named bmdma_addr.  So having
	   a separate variable served no documentary purpose.  This change
	   causes the value to be computed a little later, so please make
	   sure that that is OK.  The effect is just to make the source
	   code one line smaller.  The binary size is unchanged by this
	   modification, at least on my x86 configuration, which has
	   SMP and lots of debugging options activated.

	2. Add a variable ata_ports, to replace the six times the
	   same value appeared to be computed.  I assume the compiler
	   was smart enough to avoid most of the recomputation, but
	   this change shrinks the .text by 7 bytes in my configuration,
	   makes the routine more readable and reduces opportunities for
	   typos.

	If it looks OK to everyone, please forward it upstream as
appropriate.

Adam Richter

[-- Attachment #2: vt6421.patch --]
[-- Type: text/plain, Size: 1104 bytes --]

--- drivers/ata/sata_via.c.tejun	2007-02-16 19:16:40.000000000 +0800
+++ drivers/ata/sata_via.c	2007-02-16 19:23:45.000000000 +0800
@@ -406,18 +406,18 @@
 static void vt6421_init_addrs(struct ata_probe_ent *probe_ent,
 			      void __iomem * const *iomap, unsigned int port)
 {
 	void __iomem *reg_addr = iomap[port];
-	void __iomem *bmdma_addr = iomap[4] + (port * 8);
+	struct ata_ioports *ata_ports = &probe_ent->port[port];
 
-	probe_ent->port[port].cmd_addr = reg_addr;
-	probe_ent->port[port].altstatus_addr =
-	probe_ent->port[port].ctl_addr = (void __iomem *)
+	ata_ports->cmd_addr = reg_addr;
+	ata_ports->altstatus_addr =
+	ata_ports->ctl_addr = (void __iomem *)
 		((unsigned long)(reg_addr + 8) | ATA_PCI_CTL_OFS);
-	probe_ent->port[port].bmdma_addr = bmdma_addr;
-	probe_ent->port[port].scr_addr = vt6421_scr_addr(iomap[5], port);
+	ata_ports->bmdma_addr = iomap[4] + (port * 8);
+	ata_ports->scr_addr = vt6421_scr_addr(iomap[5], port);
 
-	ata_std_ports(&probe_ent->port[port]);
+	ata_std_ports(ata_ports);
 }
 
 static struct ata_probe_ent *vt6420_init_probe_ent(struct pci_dev *pdev)
 {

Re: Patch: slight shrink of vt6421_init_addrs()

[Jeff Garzik]

Adam J. Richter wrote:
> Hi Tejun and Jeff,
> 
> 	The discussion about the vt6421 problems caused me to glance
> at the code and try a very minor clean-up to vt6421_init_addrs().
> Please note that I don't have a vt6421, so this patch is UNTESTED.
> The patch does two things:
> 
> 	1. Elminate the bmdma_addr variable, which was only used once,
> 	   in an assignment to a field also named bmdma_addr.  So having
> 	   a separate variable served no documentary purpose.  This change
> 	   causes the value to be computed a little later, so please make
> 	   sure that that is OK.  The effect is just to make the source
> 	   code one line smaller.  The binary size is unchanged by this
> 	   modification, at least on my x86 configuration, which has
> 	   SMP and lots of debugging options activated.
> 
> 	2. Add a variable ata_ports, to replace the six times the
> 	   same value appeared to be computed.  I assume the compiler
> 	   was smart enough to avoid most of the recomputation, but
> 	   this change shrinks the .text by 7 bytes in my configuration,
> 	   makes the routine more readable and reduces opportunities for
> 	   typos.
> 
> 	If it looks OK to everyone, please forward it upstream as
> appropriate.
> 
> Adam Richter

Patch seems OK in theory, except for two procedural stumbling blocks:

1) always always always include a signed-off-by line in your kernel 
patches.  see http://linux.yyz.us/patch-format.html or 
Documentation/SubmittingPatches in the kernel tree.

2) this was stirred a bit more by recent changes, so a rediff + resend 
would be appreciated

Re: Patch: slight shrink of vt6421_init_addrs()

[Adam J. Richter]

On Fri, Feb 23, 2007 at 05:30:03AM -0500, Jeff Garzik wrote:
> Adam J. Richter wrote:
> > Hi Tejun and Jeff,
> > 
> > 	The discussion about the vt6421 problems caused me to glance
> > at the code and try a very minor clean-up to vt6421_init_addrs().
> > Please note that I don't have a vt6421, so this patch is UNTESTED.
> > The patch does two things:
> > 
> > 	1. Elminate the bmdma_addr variable, which was only used once,
> > 	   in an assignment to a field also named bmdma_addr.  So having
> > 	   a separate variable served no documentary purpose.  This change
> > 	   causes the value to be computed a little later, so please make
> > 	   sure that that is OK.  The effect is just to make the source
> > 	   code one line smaller.  The binary size is unchanged by this
> > 	   modification, at least on my x86 configuration, which has
> > 	   SMP and lots of debugging options activated.
> > 
> > 	2. Add a variable ata_ports, to replace the six times the
> > 	   same value appeared to be computed.  I assume the compiler
> > 	   was smart enough to avoid most of the recomputation, but
> > 	   this change shrinks the .text by 7 bytes in my configuration,
> > 	   makes the routine more readable and reduces opportunities for
> > 	   typos.
> > 
> > 	If it looks OK to everyone, please forward it upstream as
> > appropriate.
> > 
> > Adam Richter
> 
> Patch seems OK in theory, except for two procedural stumbling blocks:
> 
> 1) always always always include a signed-off-by line in your kernel 
> patches.  see http://linux.yyz.us/patch-format.html or 
> Documentation/SubmittingPatches in the kernel tree.

OK.

> 2) this was stirred a bit more by recent changes, so a rediff + resend 
> would be appreciated

The original patch applies perfectly cleanly to 2.6.21-rc1-git1, and
compiles without complaint, but here is a rediff specifically against
2.6.21-rc1-git1, using "diff -up" and in "a/... vs. b/..." format,
just to be more conformant to the documents you mentioned.  If there
is some other tree you wanted me to diff against, just let me know.

By the way, your remark that there have been recent changes to
sata_via.c is at least a hopeful sign, as I was about to look into
why why the vt6420 fix did not get into Linus's "release candidate"
2.6.21-rc1 or the latest git snapshot, 2.6.21-rc1-git1.  If there is
still a problem vt6421, I patch that only fixes vt6420 I think would
still be a big improvement.  Anyhow, if you're using a different
sata_via.c, I'm guessing you're about to submit a fix along those
lines.

Anyhow, here is my attempt at closer conformance to the two sets of
instructions you mentioned.  For the benefit of anyone tracking this
thread by subject, I have not changed the subject line to the format
specified in http://linux.yyz.us/patch-format.html, but I expect
to use that format in the future.





        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.

        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

---

Signed-off-by: Adam J. Richter <adam@yggdrasil.com>


--- a/drivers/ata/sata_via.c	2007-02-21 18:22:08.000000000 +0800
+++ b/drivers/ata/sata_via.c	2007-02-24 07:44:20.000000000 +0800
@@ -407,16 +407,16 @@ static void vt6421_init_addrs(struct ata
 			      void __iomem * const *iomap, unsigned int port)
 {
 	void __iomem *reg_addr = iomap[port];
-	void __iomem *bmdma_addr = iomap[4] + (port * 8);
+	struct ata_ioports *ata_ports = &probe_ent->port[port];
 
-	probe_ent->port[port].cmd_addr = reg_addr;
-	probe_ent->port[port].altstatus_addr =
-	probe_ent->port[port].ctl_addr = (void __iomem *)
+	ata_ports->cmd_addr = reg_addr;
+	ata_ports->altstatus_addr =
+	ata_ports->ctl_addr = (void __iomem *)
 		((unsigned long)(reg_addr + 8) | ATA_PCI_CTL_OFS);
-	probe_ent->port[port].bmdma_addr = bmdma_addr;
-	probe_ent->port[port].scr_addr = vt6421_scr_addr(iomap[5], port);
+	ata_ports->bmdma_addr = iomap[4] + (port * 8);
+	ata_ports->scr_addr = vt6421_scr_addr(iomap[5], port);
 
-	ata_std_ports(&probe_ent->port[port]);
+	ata_std_ports(ata_ports);
 }
 
 static struct ata_probe_ent *vt6420_init_probe_ent(struct pci_dev *pdev)

Re: Patch: slight shrink of vt6421_init_addrs()

[Adam J. Richter]

I wrote in haste:
>        If it looks OK to everyone, please forward it upstream as
>appropriate.

	I should have written that if it looks OK to everyone _and_
when it has been tested to your satisfaction, then please forward it
upstream.  I didn't mean to suggest that it should reach Linus
untested.

Adam Richter