Summary of this mail: There is activity on the Freecom / Conceptronic / Realtek card on: http://linuxtv.org/hg/~mchehab/rtl2831 Please help us improve. Mauro Carvalho Chehab wrote: > On Fri, 14 Mar 2008 22:24:46 +0100 > Jan Hoogenraad <jan-conceptronic@xxxxxxxx> wrote: > >> Dear v4l maintainer: > Please, just call me by my name ;) OK. From the previous mail I could not discern if this was the same ... > >> I have created a first version of the patch for the Freecom stick, based >> on the latest sources I received today from RealTek. > > Due to several issues I've noticed at the driver, I opted, for now, to add it > as a separate tree. This way, we can fix things there, without affecting the > staging tree. I've made it available at: > http://linuxtv.org/hg/~mchehab/rtl2831 Thanks a lot. This way, the people involved have a place to focus on. Now, I need to find a way to synchronise my tree with this directory. I'll do some reading on the mercurial. I have some scripts to convert things I receive from Realtek. I'll add the new directory names and Lindent at least. > > Also, I noticed that nobody, on RealTek signed it. It would be interesting if > someone there could send us a SOB for the first changeset: > http://linuxtv.org/hg/~mchehab/rtl2831/rev/bb7749446173 Please explain the abbrreviation SOB, and if possible send the text. Would you like to have a paper copy, or is e-mail confirmation to you sufficient ? The text I added in the header is vetted by people from Realtek. They are eager to work together, and willing to learn. Unfortunately, I myself am completely new to linux development. > > Ah, by convention, we name directories in lowercase. So, I've did an "sed" > before applying your patch, as I've explained at the changeset comments. thanks > >> I have received several updates per week from them during the fixing >> time, so I expect some updates later on. > > Ok. > >> Mauro: >> >> Thanks for the help. >> I agree that this is probably the best thing to do. > > Ok, this is the Lindent changeset: > http://linuxtv.org/hg/~mchehab/rtl2831/rev/698c1894a3fd > >> Unfortunately, Lindent does not fix errors that >> make checkpatch >> reports like the two below. >> >> tuner_mxl5005s.h: In '// I2C birdge module demod argument setting': >> tuner_mxl5005s.h:531: ERROR: do not use C99 // comments > > I tried to quickfix this with a small perl script, like: > > for i in *.c *.h; do perl -ne \ > 'if (s|//\s*(.*)\n|/* \1 */\n|) { s|/\*\s*\*/||; } print $_' \ > $i >/tmp/tmp; mv -f /tmp/tmp $i; done > > However, this failed, since there are some comments with // inside. It doesn't > seem to be hard to fix this by a close script. So I found as well, using a similar sed code. > >> tuner_mxl5005s.h: In 'void >> mxl5005s_SetI2cBridgeModuleTunerArg(TUNER_MODULE * pTuner);': >> tuner_mxl5005s.h:532: ERROR: "foo * bar" should be "foo *bar" > > True. Yet, this shows another thing that is forbidden on Linux CodingStyle: the > usage of typedef. While this is valid on a few cases, on most cases we prefer > to use things like "struct foo *foo;". > > --- > > Due to the size of the driver, and the nature of it (a port from other OS), it > is natural that we will have a large amount of issues. Before visiting the code > to check everything, maybe the better approach would be to do some general > comments. > > I'll start commenting some things about CodingStyle. The better is if you could > read kernel Documentation/CodingStyle. > > 1) Kernel already defines several types. Please use the already defined typedefs. > For example: > > +typedef unsigned char U8Data; > > use, instead __u8 > > +typedef unsigned int UData_t; /* type must be at least 32 bits */ > > use, instead __u32 > > +typedef int SData_t; /* type must be at least 32 bits */ > > use, instead __s32 > > +typedef void *Handle_t; /* memory pointer type > > Just use "void *" I'll add those specific cases to my import script; I think that script should (due to the nature of the driver) get a central role, as to keep updates automated. > > 2) We don't use "typedef struct foo". Instead, just declare "struct foo" and > replace all "foo *" to "struct foo *": > > +typedef struct { > + UData_t nAS_Algorithm; > + UData_t f_ref; > + UData_t f_in; > + UData_t f_LO1; > + UData_t f_if1_Center; > + UData_t f_if1_Request; > + UData_t f_if1_bw; > + UData_t f_LO2; > + UData_t f_out; > + UData_t f_out_bw; > + UData_t f_LO1_Step; > + UData_t f_LO2_Step; > + UData_t f_LO1_FracN_Avoid; > + UData_t f_LO2_FracN_Avoid; > + UData_t f_zif_bw; > + UData_t f_min_LO_Separation; > + UData_t maxH1; > + UData_t maxH2; > + UData_t bSpurPresent; > + UData_t bSpurAvoided; > + UData_t nSpursFound; > + UData_t nZones; > + struct MT_ExclZone_t *freeZones; > + struct MT_ExclZone_t *usedZones; > + struct MT_ExclZone_t MT_ExclZones[MAX_ZONES]; > } MT_AvoidSpursData_t; again, same thing; should be scriptable. > > 3) Name convention. Names are generally in lower case. Since we try to have all > lines with maxsize=80, the better is trying to have shorter names. > > I don't think that it would be a good idea to replace all names inside the driver, > since this will make your life harder, when receiving patches from Realtek. > Anyway, please consider this if you need to touch on some var name. > > There are other comments I want to do, about the integration with the tree. I > intend to do it later, after having a better understanding on how the driver > works and what can be done to avoid code duplication with dvb core and to allow > the usage of the tuners by other drivers. Some of us have studied this already, and communicated with Realtek on this, For example, they have an improved handling of the mt2060 tuner. Decoupling the front end, setting this temporatily up as a new driver (would the naming there be something like mt2060_for_rtl2831 ?) and then integration have been on our wish list already. > > Also, I'll need help from other developers on this large task ;) I at least have a lot of people interested already for testing. I've cc-ed them. I don't know their programming capabilities yet, however. > > Cheers, > Mauro > > _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
