Oliver Endriss wrote: > Hi Mauro, > > Mauro Carvalho Chehab wrote: >> Hi Oliver, >> >> 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. >> 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. 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. Instead, the driver should be calling i2c_register_adapter(). 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. > >> The two 80 col warnings and the two errors don't seem relevant, but please fix the other warnings. >> >> As usual, after receiving your fixes for the above, I'll review the driver >> manually. > > Except for the mutex_init() problem, everything should be fixed with > this changeset: http://linuxtv.org/hg/~endriss/ngene/rev/8ad7907e3020 > Please check. Thanks. I'll add it at the -git. -- Cheers, Mauro -- 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
