Christoph Hellwig wrote:
> On Sat, Jun 19, 2004 at 06:50:12PM +0200, Carl-Daniel Hailfinger wrote:
> 
>>The code duplication will be addressed soon. Right now I'd like to change
>>the union "u" of v1 and v2 structs to an anonymous union for better
>>readability.
> 
> GCC 2.95 doesn't support anonymous unions, so we can't use it for the kernel.

OK, I avoided that. I will probably use some #define magic or similar to
streamline current code duplication.

So far, I have eliminated three duplicated functions. The rest is going to
be a bit harder because the meaning of the bitfields changed between
different descriptor versions, but it is doable.

The attached patch is against latest 2.6 and should compile cleanly.

The following issues remain:
- Use explicit shifts instead of bitfields
- Check if the changed locking is correct
- Look again at the driver for general bugs
- Streamline the code a bit more

Oh, and the following code was buggy, the patch changed it, but the bug
remained:
@@ -1507,9 +1933,16 @@
        writel(0, base + NvRegWakeUpFlags);
        np->wolenabled = 0;

-       np->tx_flags =
cpu_to_le16(NV_TX_LASTPACKET|NV_TX_LASTPACKET1|NV_TX_VALID);
-       if (id->driver_data & DEV_NEED_LASTPACKET1)
-               np->tx_flags |= cpu_to_le16(NV_TX_LASTPACKET1);
+       if (np->desc_ver == DESC_VER_1) {
+               np->tx_flags =
cpu_to_le16(NV_TX_LASTPACKET|NV_TX_LASTPACKET1|NV_TX_VALID);
+               if (id->driver_data & DEV_NEED_LASTPACKET1)
+                       np->tx_flags |= cpu_to_le16(NV_TX_LASTPACKET1);
+       }
+       else {
+               np->tx_flags =
cpu_to_le32(NV_TX2_LASTPACKET|NV_TX2_LASTPACKET1|NV_TX2_VALID);
+               if (id->driver_data & DEV_NEED_LASTPACKET1)
+                       np->tx_flags |= cpu_to_le32(NV_TX2_LASTPACKET1);
+       }
        if (id->driver_data & DEV_IRQMASK_1)
                np->irqmask = NVREG_IRQMASK_WANTED_1;
        if (id->driver_data & DEV_IRQMASK_2)


np->tx_flags = cpu_to_le16(NV_TX_LASTPACKET|NV_TX_LASTPACKET1|NV_TX_VALID);
if (id->driver_data & DEV_NEED_LASTPACKET1)
        np->tx_flags |= cpu_to_le16(NV_TX_LASTPACKET1);

As one can easily see the second bitwise OR is a NOP because the bit has
been set before. I will fix that in the next release.

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/