rt2x00 drivers: rt61pci

From: Ivo van Doorn <[email protected]>

This adds the rt61pci driver to the tree

Available on server:
http://mendiosus.nl/rt2x00/rt61pci.diff

Ivo van Doorn <[email protected]> :

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

Part of the coding style I use. Bad habit, I know. I'll change them.

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 about that either. I usually choose the type of the counter depending on the max size of that 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".

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. ;)

Sounds good to me.
I'll make this fix part of next patch series.

Ivo van Doorn <[email protected]> :
[...]

It is not arch-neutral. powerpc favors unsigned int over int but I am too lazy to check if the size matters.

[...]

Chapter 4 of Documentation/CodingStyle goes in this direction. The document is not meant to be a taken too literally though.

The repetitive use of 'counter' + foo[counter].blah tends to be a bit bloaty.

[...]

I will not complain if you open a bugreport to provide a git repo as well.

/me hides...

Ueimor

Ok, I'll make sure the counters will made of the unsigned int type.

The git repository has indeed been discussed between the rt2x00 developers before. And at the moment we see no major benefit from using git. We need to have something like CVS for regular users to use rt2x00 and we don't want them to use git only to use the drivers.
In the CVS tree we have multiple backwards compatibility fixes for both rt2x00 as the dscape stack to make it work on kernel 2.6.13 and above. So making a git repository would mean a lot duplicate work, since we have to keep 2 trees up to date, as well as making sure the wireless-dev tree contains an up to date version.