Messages in this thread | | | Date | Thu, 22 Nov 2007 03:49:59 +0300 | From | Anton Vorontsov <> | Subject | Re: [patch] 0/4 Support for Toshiba TMIO multifunction devices |
| |
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: cbou@mail.ru backup email: ya-cbou@yandex.ru irc://irc.freenode.net/bd2 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |