On Saturday 29 April 2006 16:58, Francois Romieu wrote: > 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 ?
Part of the coding style I use. Bad habit, I know.
I'll change them.
> [...]
> + 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.
Unless I am mistaken, 2 tabs are used at a max.
Perhaps in some cases it is more, because the variable beneath
is a long structure name. I'll go over them again, and check where
they could be minimized.
> Not sure if 'unsigned int' would be welcome instead of 'u8' for
> counter.
Not sure about that either. I usually choose the type of the counter
depending on the max size of that counter.
> [...]
> + for (counter = 0; counter < REGISTER_BUSY_COUNT; counter++) {
> + rt2x00_register_read(rt2x00pci, PHY_CSR3, ®);
>
> "i" is more idiomatic C-kernel than "counter".
Perhaps, but I prefer the usage of the name "counter".
I am not sure if there is a coding style about this? If so I could rename
"counter" to "i".
> [...]
> + 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.
I'll create a bugreport in the rt2x00 project bugzilla
with some of the coding style change requests.
It could take a while before a patch will be ready since I put higher
priority to get the drivers working correctly. ;)
> [...]
> +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, ®);
> + if (rt2x00_get_field32(reg, MCU_CNTL_CSR_READY))
> + break;
> + msleep(1);
> + }
> +
> + if (counter == 1000) {
> ^ -> typo
Good call. Thanks. :)
> [...]
> 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;
Sounds good to me.
I'll make this fix part of next patch series.
pgptzJlUExWaM.pgp
Description: PGP signature
