Hi Ian,
Personally I'm very appreciate your patches, they'll will
help submitting HP iPaqs SOCs/MFDs, you know... ;-)
Thus, much thanks in advance.
Few comments...
On Wed, Nov 21, 2007 at 03:54:15AM +0000, ian wrote:
> On Wed, 2007-11-21 at 10:23 +0800, eric miao wrote:
> > Roughly went through the patch, looks good, here comes the remind, though :-)
> >
> > 1. is it possible to use some name other than "soc_core", maybe
> > "tmio_core" so that other multifunction chips sharing a core base
> > will live easier.
>
> It's (soc-core) not tmio MFD specific - its already used by other MFD
> chips (although obviously not ones in mainline (yet!)
>
> it might be better named 'mfd-core' though, as thats its intended use...
>
> > 2. those C++ style comments "//" are not so pleasant...
>
> Should I clean them up and resubmit?
I'd resubmit cleaned up version. I think four or even more
resubmissions is inevitable for such patch-set (new general code +
a lot of drivers).
About patches their self... I think soc_add_devices could be
split into two small functions, thus you'll get rid of high
indentation level + code will be more reader friendly.
Ideally, checkpatch.pl should be happy. If it will, then there will
be less nitpicks somebody can pull. ;-)
Here it is:
- - - -
~/linux-2.6$ scripts/checkpatch.pl ~/0001-Reuseable-SOC-core-code-suitable-for-multifunction-c.patch
WARNING: line over 80 characters
#57: FILE: drivers/mfd/soc-core.c:37:
+#define SIGNED_SHIFT(val, shift) ((shift) >= 0 ? ((val) << (shift)) : ((val) >> -(shift)))
WARNING: line over 80 characters
#60: FILE: drivers/mfd/soc-core.c:40:
+ struct soc_device_data *soc, int nr_devs,
WARNING: line over 80 characters
#84: FILE: drivers/mfd/soc-core.c:64:
+ res = kzalloc(blk->num_resources * sizeof (struct resource), GFP_KERNEL);
WARNING: no space between function name and open parenthesis '('
#84: FILE: drivers/mfd/soc-core.c:64:
+ res = kzalloc(blk->num_resources * sizeof (struct resource), GFP_KERNEL);
ERROR: do not use C99 // comments
#89: FILE: drivers/mfd/soc-core.c:69:
+ res[r].name = blk->res[r].name; // Fixme - should copy
WARNING: braces {} are not necessary for single statement blocks
#93: FILE: drivers/mfd/soc-core.c:73:
+ if (blk->res[r].flags & IORESOURCE_MEM) {
+ base = mem->start;
+ } else if ((blk->res[r].flags & IORESOURCE_IRQ) &&
WARNING: braces {} are not necessary for single statement blocks
#95: FILE: drivers/mfd/soc-core.c:75:
+ } else if ((blk->res[r].flags & IORESOURCE_IRQ) &&
+ (blk->res[r].flags & IORESOURCE_IRQ_SOC_SUBDEVICE)) {
+ base = irq_base;
+ }
WARNING: line over 80 characters
#96: FILE: drivers/mfd/soc-core.c:76:
+ (blk->res[r].flags & IORESOURCE_IRQ_SOC_SUBDEVICE)) {
WARNING: line over 80 characters
#103: FILE: drivers/mfd/soc-core.c:83:
+ res[r].start = base + SIGNED_SHIFT(blk->res[r].start, relative_addr_shift);
WARNING: line over 80 characters
#104: FILE: drivers/mfd/soc-core.c:84:
+ res[r].end = base + SIGNED_SHIFT(blk->res[r].end, relative_addr_shift);
ERROR: Missing Signed-off-by: line(s)
total: 2 errors, 9 warnings, 145 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
- - - -
There is false positive though:
if (...) {
single_stmt;
} else {
one;
two;
}
^^^ is perfectly OK and preferred, IIRC. checkpatch isn't ideal,
but it's mostly good.
> More to the point, who should I be submitting them to? the files under
> arm/ are obviously for RMK to peruse, but I couldnt find an entry for
> drivers/mfd in MAINTAINERS...
Well, don't know about drivers/mfd/*. Probably there simply isn't
any [official] maintainer, thus lkml is the right place.
There is one not so obvious thing though: you should not submit patches
with To/Cc'ing lkml (open list) and linux-arm-kernel (subscribers-only).
Russell King will probably point to linux-arm-kernel etiquette article
(http://www.arm.linux.org.uk/mailinglists/etiquette.php
"Cross-posting between linux-arm* lists and other lists.")
So, either place linux-arm-kernel into Bcc:, or duplicate stuff for
lkml and linux-arm-kernel separately, thus they'll not see each
others' To/Cc.
Looking forward to your patches!
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
