Hi, Mauro Carvalho Chehab wrote: > Oliver Endriss wrote: > > Mauro Carvalho Chehab wrote: > >> Oliver Endriss wrote: > >>> Mauro, > >>> > >>> Please pull from http://endriss@xxxxxxxxxxx/hg/~endriss/ngene > >> It were not an easy to work on this changeset, since it hit a bug at gentree.pl script. > >> Basically, this drivers use #elseif directive instead of #elif, making gentree.pl to > >> die, and generating broken converted patches. It took some time to discover and fix > >> the bug. > >> > >> Anyway, I've applied the converted patches on a temporary git tree: > >> http://git.linuxtv.org/mchehab/ngene.git > >> > >> Please double check if everything is ok there. > > > > I noticed that all '#if 0' code has been stripped. > > > > While this is ok (and desired) for the kernel, this stuff should be > > preserved at the development trees. This code might be useful, whenever > > addional features or cards are to be added. > > As Douglas will be pulling from -hg, the #if's will be there. Ok, fine. > >> I had to add a hack at one of the patches, due to the frontend incompatibilities > >> at stv090x, starting on this changeset: > >> > >> Author: Matthias Benesch <twoof7@xxxxxxxxxx> > >> Date: Sat Dec 19 12:48:22 2009 -0300 > >> > >> V4L/DVB: ngene: Added Media-Pointer MP-S2/CineS2 DVB-S2 Twin Tuner > >> > >> The hack were removed on this one: > >> > >> Author: Oliver Endriss <o.endriss@xxxxxx> > >> Date: Mon Feb 1 22:01:31 2010 -0300 > >> > >> V4L/DVB: ngene: Adapt to current frontend drivers > > > > There was no problem with HG, because > > - the first changeset was applied before the incompatibility occurred > > - the second was applied as a fix for the incompatibility > > > > I don't know how this could be avoided. Except for dropping the HG tree > > and creating a new one. > > A good idea is to split the Kconfig/Makefile changes into a separate patch. > At the final submission, all that is needed is to move it to the end of the > patch series. Ok. > Anyway, the hack I did wouldn't cause any harm, and will prevent breaking the useful > bisect tool, when tracking for some bugs on kernel. > > >> After applying all the patches, I'm getting this error: > >> > >> drivers/media/dvb/ngene/ngene-core.c: In function ‘ngene_i2c_init’: > >> drivers/media/dvb/ngene/ngene-core.c:860: warning: passing argument 1 of ‘__mutex_init’ from incompatible pointer type > >> > >> On this line: > >> mutex_init(&adap->bus_lock); > > > > Strange, I do not get any warnings with kernel 2.6.32.3 and gcc 4.1.2. > > > > The line causing the problems is > > > > mutex_init(&adap->bus_lock); > > > > mutex.h: > > # define mutex_init(mutex) \ > > ... > > __mutex_init((mutex), #mutex, &__key); \ > > ... > > > > extern void __mutex_init(struct mutex *lock, const char *name, > > struct lock_class_key *key); > > > > adap is a pointer to 'struct i2c_adapter': > > > > i2c.h: > > struct i2c_adapter { > > ... > > struct mutex bus_lock; > > ... > > > > I do not see any problem here. > > Ah, the realtime patches: > > drivers/i2c/i2c-core.c: rt_mutex_init(&adap->bus_lock); > > Several mutexes are currently changing, to improve kernel responsiveness. > > > > > Something must have been changed in recent git. > > > > Could you try if > > #include <linux/mutex.h> > > helps? > > It doesn't. It is really a bad idea to initialize an i2c mutex inside the driver. Ack. See below. > Instead, the driver should be calling i2c_register_adapter(). No, i2c_register_adapter() is a static function. i2c_add_adapter is the right way. As it calls i2c_register_adapter(), mutex_init() can be removed, and everything is fine. > There are other initializations there that i2c core needs to do, in order > to work properly. > > >> The final diff also hit a few troubles at the driver, as pointed by checkpatch.pl: > >> > >> WARNING: Use #include <linux/io.h> instead of <asm/io.h> > >> #113: FILE: drivers/media/dvb/ngene/ngene-core.c:35: > >> +#include <asm/io.h> > >> > >> WARNING: line over 80 characters > >> #348: FILE: drivers/media/dvb/ngene/ngene-core.c:270: > >> + HOST_TO_NGENE, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], buf[6], buf[7]); > >> > >> WARNING: line over 80 characters > >> #352: FILE: drivers/media/dvb/ngene/ngene-core.c:274: > >> + NGENE_TO_HOST, buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], buf[6], buf[7]); > >> > >> ERROR: that open brace { should be on the previous line > >> #1693: FILE: drivers/media/dvb/ngene/ngene-core.c:1615: > >> + u8 tsin4_config[6] = > >> + {3072 / 64, 3072 / 64, 0, 3072 / 64, 3072 / 64, 0}; > >> > >> ERROR: that open brace { should be on the previous line > >> #1695: FILE: drivers/media/dvb/ngene/ngene-core.c:1617: > >> + u8 default_config[6] = > >> + {4096 / 64, 4096 / 64, 0, 2048 / 64, 2048 / 64, 0}; > >> > >> WARNING: Use #include <linux/scatterlist.h> instead of <asm/scatterlist.h> > >> #2142: FILE: drivers/media/dvb/ngene/ngene.h:32: > >> +#include <asm/scatterlist.h> > >> > >> WARNING: do not add new typedefs > >> #2313: FILE: drivers/media/dvb/ngene/ngene.h:203: > >> +typedef struct EVENT_BUFFER *PEVENT_BUFFER; > >> > >> WARNING: do not add new typedefs > >> #2601: FILE: drivers/media/dvb/ngene/ngene.h:491: > >> +typedef struct HW_SCATTER_GATHER_ELEMENT *PHW_SCATTER_GATHER_ELEMENT; > >> > >> WARNING: do not add new typedefs > >> #2602: FILE: drivers/media/dvb/ngene/ngene.h:492: > >> +typedef struct FWRB *PFWRB; > >> > >> WARNING: do not add new typedefs > >> #2667: FILE: drivers/media/dvb/ngene/ngene.h:557: > >> +typedef struct { > >> > >> total: 2 errors, 8 warnings, 2935 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. > > > > Now I remember, why I never wanted to submit anything to the kernel > > again. As always it was a pleasure to work for this stupid tool. :-( > > It is pointing some real problems: the usage of asm-specific includes, > likely breaking compatibility with some architectures and the definition > of new types for structs. Only very few cases, typedef is allowed on > kernel, since it hides the type of the vars. I suspect that this also > make source code analyzers like sparse to generate false positives. I consider the '#include' messages useful (2 of 10), the rest is bogus. Well, I don't want to waste more of my time with lengthy discussions. It is enough that I spent another evening fixing checkpatch stuff. Oliver -- ---------------------------------------------------------------- VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/ 4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/ Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/ ---------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
