Hi Hans, On Tuesday 11 December 2012 12:50:21 Hans Verkuil wrote: > I've added a few quick remarks below. I'll collect all the very useful > feedback on Friday and post a new version. After that I'm on vacation > for three weeks. > > On Tue 11 December 2012 11:29:30 Mauro Carvalho Chehab wrote: > > Em Tue, 11 Dec 2012 00:52:39 +0100 Laurent Pinchart escreveu: > > > On Monday 10 December 2012 16:33:13 Mauro Carvalho Chehab wrote: > > > > Em Mon, 10 Dec 2012 14:07:09 +0100 Hans Verkuil escreveu:
[snip] > > > > > Submitting New Media Drivers > > > > > ============================ > > > > > > > > > > When submitting new media drivers for inclusion in > > > > > drivers/staging/media all that is required is that the driver > > > > > compiles with the latest kernel and that an entry is added to the > > > > > MAINTAINERS file > > > > > > > > Please add: > > > > "and what is missing there for it to be promoted to be a main > > > > driver > > > > is documented at the TODO file. > > > > > > > > It should be noticed, however, that it is expected that the > > > > driver > > > > > > > > will be fixed to fulfill the requirements for upstream addition. If a > > > > driver at staging lacks relevant patches fixing it for more than a > > > > kernel cycle, it can be dropped without further notice." > > > > > > Maybe a single kernel cycle is a bit too strict. The unexpected can > > > happen, so let's not be too harsh. > > > > The above is not saying that it should be fixed on one kernel cycle. It > > says, instead, that drivers without relevant changes during a cycle can > > be dropped. We'll likely not enforce it too hard, except if we notice > > some sort of bad faith from the driver's maintainer. > > > > > And I'm pretty sure we'll always send a notice. > > > > The "notice" will likely be just a patch to the ML c/c the driver's > > maintainer and the mailing list. As the driver's maintainer email's > > address might have changed, and/or he might not be subscribed at the ML, > > it may happen that such email will never reach him. > > > > So, the "it can be dropped without further notice" wording is meant to > > avoid later complains that from driver's maintainer that he was not > > warned. It also passes the idea that no ack from the driver's maintainer > > is needed/expected on such patch. > > > > If you think it is badly written, feel free to change it, but keeping this > > idea. > > > > > > > For inclusion as a non-staging driver the requirements are more > > > > > strict: > > > > > > > > > > General requirements: > > > > > > > > > > - It must pass checkpatch.pl, but see the note regarding > > > > > interpreting the output from checkpatch below. > > > > > > > > > > - An entry for the driver is added to the MAINTAINERS file. > > > > > > > > Please add: > > > > - Properly use the kernel internal APIs; > > > > - Should re-invent the wheel, by adding new defines, math logic, etc > > > > that already exists in the Kernel; > > > > > > Should *not* ? :-) > > > > Gah... Yeah, it should read, instead: "shouldn't". > > > > > > - Errors should be reported as negative numbers, using the Kernel > > > > error codes; > > > > > > CodingStyle, chapter 16 (although not as clearly stated). > > > > Yes, I know. Yet, this is one of the most frequent bad things we notice on > > code from new developers. IMHO, it doesn't hurt to explicitly say it here, > > likely pointing to the CodingStyle corresponding chapter. > > > > > > - typedefs should't be used; > > > > > > CodingStyle, chapter 5. > > Surprisingly this chapter doesn't mention typedefs for function pointers. > Those are very hard to manage without a typedef. Agreed. > > Same as above: that's the second most frequent bad thing. It seems that > > windows-way is to create typedefs for each struct and return positive, > > driver-specific return codes. At least I saw the very same pattern on all > > windows drivers I looked. > > > > > > > V4L2 specific requirements: > > > > > > > > > > - Use struct v4l2_device for bridge drivers, use struct v4l2_subdev > > > > > for sub-device drivers. > > > > > > > > Please add: > > > > - each I2C chip should be mapped as a separate sub-device driver; > > > > > > > > > - Use the control framework for control handling. > > > > > - Use struct v4l2_fh if the driver supports events (implied by the > > > > > use of controls) or priority handling. > > > > > > > > > > - Use videobuf2 for buffer handling. Mike Krufky will look into > > > > > extending vb2 to support DVB buffers. Note: using vb2 for VBI > > > > > devices has not been tested yet, but it should work. Please > > > > > contact the mailinglist in case of problems with that. > > > > > > > > > > - Must pass the v4l2-compliance tests. > > > > > > > > Please add: > > > > - hybrid tuners should be shared with DVB; > > > > > > > > > DVB specific requirements: > > > > - Use the DVB core, for both internal and external APIs; > > > > - Each I2C-based chip should have its own driver; > > > > - Tuners and frontends should be mapped as different drivers; > > > > - hybrid tuners should be shared with V4L; > > Should something be mentioned with regards to DVBv5 and using > GET/SET_PROPERTY? [snip] > > > > > Reviewed-by/Acked-by > > > > > ==================== > > > > > > > > > > Within the media subsystem there are three levels of maintainership: > > > > > Mauro Carvalho Chehab is the maintainer of the whole subsystem and > > > > > the DVB/V4L/IR/Media Controller core code in particular, then there > > > > > are a number of submaintainers for specific areas of the subsystem: > > > > > > > > > > - Kamil Debski: codec (aka memory-to-memory) drivers > > > > > - Hans de Goede: non-UVC USB webcam drivers > > > > > - Mike Krufky: frontends/tuners/demodulators In addition he'll be > > > > > the reviewer for DVB core patches. > > > > > > > > I'll change it to "a reviewer", as perhaps he won't be able to review > > > > everything, and because we're welcoming others to also review it. > > > > > > Maybe "the core reviewer" or "the main reviewer" ? Everybody is of > > > course welcome to review patches, the point here is to state who a good > > > contact person is when a patch doesn't get reviewed. > > > > Well, having the name there as a reviewer is enough to say that the person > > is a good contact when a patch doesn't get reviewed. > > > > When we point that responsibility to a single person, I'm afraid that > > the message passed is that he is the sole/main responsible for reviewing > > core changes, and this is not the case, as it is everybody's > > responsibility to review v4l/dvb/media controller core changes > > True, but I think it is a good idea to have a main reviewer assigned whose > Acked-by you definitely need. Sure, I can ack a DVB core patch, but that > carries a lot less weight than if Mike acks it. I'm a bit unsure here. For instance I of course welcome your Acked-by on my patches, but as you're listed as the reviewer for V4L2 drivers, would that be required for an OMAP3 ISP patch that fixes a device-related issue ? I don't expect you to become an expert on device-specific parts of all V4L2 drivers :-) > > > > > - Guennadi Liakhovetski: soc-camera drivers > > > > > - Laurent Pinchart: sensor subdev drivers. In addition he'll be the > > > > > reviewer for Media Controller core patches. > > > > > > > > I'll change it to "a reviewer", as perhaps he won't be able to review > > > > everything, and because we're welcoming others to also review it. > > > > > > Ditto. > > > > > > > > - Hans Verkuil: V4L2 drivers and video A/D and D/A subdev drivers > > > > > (aka video receivers and transmitters). In addition he'll be the > > > > > reviewer for V4L2 core patches. > > > > > > > > I'll change it to "a reviewer", as perhaps he won't be able to review > > > > everything, and because we're welcoming others to also review it. > > > > > > Ditto. > > > > > > > > Finally there are maintainers for specific drivers. This is > > > > > documented in the MAINTAINERS file. > > > > > > > > > > When modifying existing code you need to get the > > > > > Reviewed-by/Acked-by of the maintainer of that code. So CC that > > > > > maintainer when posting patches. If said maintainer is unavailable > > > > > then the submaintainer or even Mauro can accept it as well, but that > > > > > should be the exception, not the rule. > > > > > > > > > > Once patches are accepted they will flow through the git tree of the > > > > > submaintainer to the git tree of the maintainer (Mauro) who will do > > > > > a final review. > > > > > > > > > > There are a few exceptions: code for certain platforms goes through > > > > > git trees specific to that platform. The submaintainer will still > > > > > review it and add a acked-by or reviewed-by line, but it will not go > > > > > through the submaintainer's git tree. > > > > > > > > > > The platform maintainers are: > > > > > > > > > > TDB > > > > > > > > - s5p/exynos? > > > > - DaVinci? > > > > - Omap3? > > > > - Omap2? > > > > - dvb-usb-v2? > > > > > > Some of those (OMAP2 and OMAP3 at least) are really single drivers. I'm > > > not sure whether we need to list them as platforms. > > > > We're currently handling all those Nokia/TI drivers as one "platform set" > > of drivers and waiting for Prabhakar to merge them on his tree and submit > > via git pull request > > That's only for the davinci code. Prabhakar doesn't handle omap3, that's a > single driver at the moment. Ideally there would be an omap directory where > TI would maintain the omap family, but we all know that's not the case. Indeed. OMAP4/5 won't be supported unless someone finds time to work on the driver, and if I'm not mistaken there will be no OMAP6+. > I guess we have just two 'SoC-family' maintainers: Prabhakar for davinci, > and Sylwester for s5p/exynos. > > One thing that we might want to add here is that submaintainers can submit > patches for the drivers that they maintain through their own git tree. > > I.e., Laurent maintains omap3, but strictly speaking that would have to go > through my git tree. But I think that's silly, all that is needed is my > Acked-by. > > What do you think? I agree, driver maintainers with a git tree should send pull requests directly to Mauro. There's not much point in adding an intermediate git tree there, we don't have enough driver maintainers who send pull requests. > > , just like all s5p/exynos stuff, where Sylwester is acting > > as a sub-maintainer. > > > > So, either someone has to explicitly say otherwise, or we should document > > it here. > > > > > > > In case patches touch on areas that are the responsibility of > > > > > multiple submaintainers, then they will decide among one another who > > > > > will merge the patches. > > > > > > > > > > > > > > > Patchwork > > > > > ========= > > > > > > > > > > Patchwork is an automated system that takes care of all posted > > > > > patches. It can be found here: > > > > > http://patchwork.linuxtv.org/project/linux-media/list/ > > > > > > > > > > If your patch does not appear in patchwork after [TBD], then check > > > > > if you used the right patch tags and if your patch is formatted > > > > > correctly (no HTML, no mangled lines). > > > > > > > > s/[TBD]/a couple minutes/ > > > > > > > > Please add: > > > > Unfortunately, patchwork currently doesn't send you any email > > > > when a > > > > patch successfully arrives there. > > > > > > > > (perhaps Laurent could take a look on this for us?) > > > > > > Sure. Do you have a list of features you would like to see implemented > > > in patchwork ? I can't look at that before January though. > > > > We can work on it together on such lists. The ones I remember right now > > are: > > - confirmation email when a patch is successfully added; > > - allow patch submitters to change the status of their own patches, > > in order to allow them to mark obsoleted/superseeded patches as such; > > > > - create some levels of ACL on patchwork, in order to make delegations > > work, e. g. let the maintainer/sub-maintainers to send a patch to > > a driver maintainer, while keeping control about what's happening > > with a delegated patch. > > - detect the tags described in this document and set the patchwork state > accordingly. > - not sure if this is possible: if a v2 patch series is posted, then > automatically remove v1. This would require sanity checks: same subject, > same author. There's a security issue here as it's easy to spoof a sender e-mail address, but I don't think that we need to care about it. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
