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

[Ivo van Doorn <ivdoorn@gmail.com>]

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