RE : [PATCH 3/3] Add support to M54xx DMA FEC Driver

[Stany MARCEL]

Hi,

I made a lot of cleanup in my patches, I hope to have some time next week to run my ethernet tests on my board test bench.

If no regressions are identified I'll resubmit my patch.

Regards,

Stany


-------- Message d'origine--------
De: Philippe De Muyter [mailto:phdm@xxxxxxxxx]
Date: mer. 05/09/2012 11:14
À: Greg Ungerer
Cc: Stany MARCEL; linux-m68k@xxxxxxxxxxxxxxxxxxxx; geert@xxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Objet : Re: [PATCH 3/3] Add support to M54xx DMA FEC Driver
 
Hi Stany & Greg

Seeing that I was not the only one wanting to have the m54xx fec dma
driver merged in, and hoping to compare Stany's version to mine,
I have rebased (step by step) my patch from v2.38 to v3.6rc2.
The driver still works and perhaps even better due to some fixes
in other m68k area.

Unfortunately I have not being able to compare it yet fully with Stany's
version because Stany's patch 2/2 did not apply (using `git am') to v3.5
or v3.6rc2.

I have checked my patch using a recent version of checkpatch.pl (not the
v3.5 version, because v3.5 version of checkpatch.pl fails with :
Nested quantifiers in regex; marked by <-- HERE in m/(\((?:[^\(\)]++ <-- HERE |(
?-1))*\))/ at scripts/checkpatch.pl line 340.))

and I am now at :
    464 WARNING: line over 80 characters
     90 WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

Many "volatile" warnings are about such definitions :

#define FEC_FECFRST(x)                 (*(volatile unsigned int *)(x + 0x1C4))
which are afterwards used with

+       FEC_FECFRST(base_addr) |= FEC_SW_RST;
+       FEC_FECFRST(base_addr) &= ~FEC_SW_RST;
+               FEC_FECFRST(base_addr) |= FEC_SW_RST;
+               FEC_FECFRST(base_addr) &= ~FEC_SW_RST;
+       FEC_FECFRST(base_addr) |= FEC_SW_RST | FEC_RST_CTL;
+       FEC_FECFRST(base_addr) &= ~FEC_SW_RST;

Any advice about those ones ?

while many "80 characters" ones are about :
#4014: FILE: arch/m68k/platform/coldfire/MCD_tasks.c:2406:
+       0x6000000b, /* 0098(:1560):      DRD2A: EU0=0 EU1=0 EU2=0 EU3=11 EXT ini
t=0 WS=0 RS=0 */

I would like to keep those lines intact because the comment seems to actually
be the assembler source of the hex value at left, which seems to be a
microcode, and it makes sense to me to keep that on one line.  What do
you think about that ?

I did not include the current status of the patch because of its size
(I did not separate the dma part of the ethernet driver part because
the dma part is useless without the ethernet driver, and linking the
ethernet driver cannot succeed without the dma part), but if you ask,
I'll send it privately.

Best regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/