[PATCH 2/3] rt2x00 drivers: rt61pci

[PATCH 2/3] rt2x00 drivers: rt61pci

[Ivo van Doorn]

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

From: Ivo van Doorn <IvDoorn@gmail.com>

This adds the rt61pci driver to the tree 

Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>

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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Francois Romieu]

Ivo van Doorn <ivdoorn@gmail.com> :
> From: Ivo van Doorn <IvDoorn@gmail.com>
> 
> This adds the rt61pci driver to the tree 
> 
> Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
> 
> 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

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

[Ivo van Doorn]

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

On Saturday 29 April 2006 16:58, Francois Romieu wrote:
> Ivo van Doorn <ivdoorn@gmail.com> :
> > From: Ivo van Doorn <IvDoorn@gmail.com>
> > 
> > This adds the rt61pci driver to the tree 
> > 
> > Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>
> > 
> > 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, &reg);
> 
> "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, &reg);
> +		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.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Francois Romieu]

Ivo van Doorn <ivdoorn@gmail.com> :
[...]
> Not sure about that either. I usually choose the type of the counter
> depending on the max size of that counter.

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

[...]
> 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".

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'll create a bugreport in the rt2x00 project bugzilla
> with some of the coding style change requests.

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

/me hides...

-- 
Ueimor

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

[Ivo van Doorn]

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

On Saturday 29 April 2006 18:10, Francois Romieu wrote:
> Ivo van Doorn <ivdoorn@gmail.com> :
> [...]
> > Not sure about that either. I usually choose the type of the counter
> > depending on the max size of that counter.
> 
> It is not arch-neutral. powerpc favors unsigned int over int but I am
> too lazy to check if the size matters.

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

> [...]
> > 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".
> 
> 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'll create a bugreport in the rt2x00 project bugzilla
> > with some of the coding style change requests.
> 
> I will not complain if you open a bugreport to provide a git repo
> as well.
> 
> /me hides...

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]