Re: [PATCH 2/3] rt2x00 drivers: rt61pci

[Francois Romieu]

Ivo van Doorn <[EMAIL PROTECTED]> :
> From: Ivo van Doorn <[EMAIL PROTECTED]>
> 
> This adds the rt61pci driver to the tree 
> 
> Signed-off-by: Ivo van Doorn <[EMAIL PROTECTED]>
> 
> Available on server:
> http://mendiosus.nl/rt2x00/rt61pci.diff

It is nice that you are doing this work but I still don't feel
the same while reading your patches or tg3.c/sky2.c (for instance).

+static inline void
+rt2x00_register_read(

grep accepts regular expression and ctags works quite well.
Why do you cut an expression which would fit on a 80 columns line ?

[...]
+       u32             reg;
+       u8              counter;

It's (mostly) fine with me if you want to align the declaration
but why do you use more than the minimum amount of required tab ?
Elsewhere the variable is completely shifted right: beyond a point,
it does not make the code more readable.

Not sure if 'unsigned int' would be welcome instead of 'u8' for
counter.

[...]
+       for (counter = 0; counter < REGISTER_BUSY_COUNT; counter++) {
+               rt2x00_register_read(rt2x00pci, PHY_CSR3, &reg);

"i" is more idiomatic C-kernel than "counter".

[...]
+       if (rt2x00_rf(&rt2x00pci->chip, RF5225)
+       || rt2x00_rf(&rt2x00pci->chip, RF2527))

If you put the || at the end of the previous line, you can indent
the second line with four withspaces, thus aligning the content.

[...]
+static void
+rt61pci_init_firmware_cont(const struct firmware *fw, void *context)
[...]
+       for (counter = 0; counter < 100; counter++) {
+               rt2x00_register_read(rt2x00pci, MCU_CNTL_CSR, &reg);
+               if (rt2x00_get_field32(reg, MCU_CNTL_CSR_READY))
+                       break;
+               msleep(1);
+       }
+
+       if (counter == 1000) {
                          ^ -> typo


[...]
static int
+rt61pci_init_firmware(struct rt2x00_pci *rt2x00pci)
+{
+       /*
+        * Read correct firmware from harddisk.
+        */
+       if (rt2x00_rt(&rt2x00pci->chip, RT2561))
+               return request_firmware_nowait(THIS_MODULE, 1,
+                       "rt2561.bin", &rt2x00pci->pci_dev->dev, rt2x00pci,
+                       rt61pci_init_firmware_cont);
+       else if (rt2x00_rt(&rt2x00pci->chip, RT2561s))
+               return request_firmware_nowait(THIS_MODULE, 1,
+                       "rt2561s.bin", &rt2x00pci->pci_dev->dev, rt2x00pci,
+                       rt61pci_init_firmware_cont);
+       else if (rt2x00_rt(&rt2x00pci->chip, RT2661))
+               return request_firmware_nowait(THIS_MODULE, 1,
+                       "rt2661.bin", &rt2x00pci->pci_dev->dev, rt2x00pci,
+                       rt61pci_init_firmware_cont);

        struct {
                char *name;
                unsigned int chip;
        } firmware[] = {
                { "rt2561.bin",         RT2561 },
                { "rt2561s.bin",        RT2561s },
                { "rt2561.bin",         RT2661 }
        };
        int rc = -EINVAL;
        unsigned int i;

        for (i = 0; i < ARRAY_SIZE(firmware); i++) {
                if (!rt2x00_rt(&rt2x00pci->chip, firmware[i].chip)) 
                        continue;
                rc = request_firmware_nowait(THIS_MODULE, 1,
                        firmware[i].name, &rt2x00pci->pci_dev->dev,
                        rt2x00pci, rt61pci_init_firmware_cont);
                break;
        }
        return rc;

-- 
Ueimor
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html