RFC: First draft of guidelines for submitting patches to linux-media

RFC: First draft of guidelines for submitting patches to linux-media

[Hans Verkuil]

Hi all,

As discussed in Barcelona I would write a text describing requirements for new
drivers and what to expect when submitting patches to linux-media.

This is a first rough draft and nothing is fixed yet.

I have a few open questions:

1) Where to put it? One thing I would propose that we improve is to move the
dvb and video4linux directories in Documentation/ to Documentation/media to
correctly reflect the drivers/media structure. If we do that, then we can put
this document in Documentation/media/SubmittingMediaPatches.

Alternatively, this is something we can document in our wiki.

2) Are there DVB requirements as well for new drivers? We discussed a list of
V4L2 requirements in Barcelona, but I wonder if there is a similar list that
can be made for DVB drivers. Input on that will be welcome.

3) This document describes the situation we will have when the submaintainers
take their place early next year. So please check if I got that part right.

4) In Barcelona we discussed 'tags' for patches to help organize them. I've
made a proposal for those in this document. Feedback is very welcome.

5) As discussed in Barcelona there will be git tree maintainers for specific
platforms, but we didn't really go into detail who would be responsible for
which platform. If you want to maintain a particular platform, then please
let me know.

6) The patchwork section is very short at the moment. It should be extended
when patchwork gets support to recognize the various tags.

7) Anything else that should be discussed here?

Again, remember that this is a rough draft only, so be gentle with me :-)

Regards,

	Hans

--------------------------- cut here -------------------------------

General Information
===================

For general information on how to submit patches see:

http://linuxtv.org/wiki/index.php/Developer_Section

In particular the section 'Submitting Your Work'.

This document goes into more detail regarding media specific requirements when
submitting patches and what the patch flow looks like in this subsystem.


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.

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.

V4L2 specific requirements:

- Use struct v4l2_device for bridge drivers, use struct v4l2_subdev for
  sub-device drivers.
- 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.

DVB specific requirements:

TBD


How to deal with checkpatch.pl?
===============================

First of all, the requirement to comply to the kernel coding style is there for
a reason. Sometimes people feel that it is a pointless exercise: after all,
code is code, right? Why would just changing some spacing improve it?

But the coding style is not there to help you (at least, not directly), it is
there to help those who have to review and/or maintain your code as it takes a
lot of time to review code or try to figure out how someone else's code works.
By at least ensuring that the coding style is consistent with other code we can
concentrate on what humans to best: pattern matching. Ever read a book or
article that did not use the correct spelling, grammar and/or punctuation
rules? Did you notice how your brain 'stumbles' whenever it encounters such
mistakes? It makes the text harder to understand and slower to read. The same
happens with code that does not comply to the conventions of the project and it
is the reason why most large projects, both open source and proprietary, have a
coding style.

However, when interpreting the checkpatch output it is good to remember that it
is just an automated tool and there are cases where what checkpatch recommends
does not actually results in the best readable code. This is particularly true
for the line length warnings. A warning that a line is 82 characters long can
probably be ignored, since breaking up such a line will usually make the code
harder to understand. A warning that a line is 101 characters long definitely
needs attention, since that's an indication that the line is really too long.

The guideline here is to check such warnings, but use common sense whether or
not to fix them.

Please do run checkpatch before posting any code to the mailinglist. Code that
clearly violates the kernel coding style will be rejected and you will be asked
to repost after fixing the style. We are not going to waste time trying to
review code that uses a non-standard coding style, our time is too limited for
that.

The sole exception are staging drivers as the only rule there is that it
compiles.


Timeline for code submissions
=============================

After a new kernel is released the merge window will be open for about two
weeks. During that time Linus will merge any pending work for the next kernel.
Once that merge window is closed only regression fixes and serious bug fixes
will be accepted, everything else will be postponed until the next merge
window.

In addition, before anything can be merged (regardless of whether this is
during the merge window or not) the new code should have been in the linux-next
tree for about a week at minimum to ensure there are no conflicts with work
being done in other kernel subsystems.

Furthermore, before code can be added to linux-next it has to be reviewed
first.  This will take time as well. Adding everything up this means that if
you want your code to be merged for the next kernel you should have it posted
to the linux-media mailinglist no later than rc5 of the current kernel, or it
may be too late. In fact, the earlier the better since reviews will take time,
and if corrections need to be made you may have to do several review/submit
cycles.

Remember that the core media developers have a job as well, and so won't always
have the time to review immediately. A general rule of thumb is to post a
reminder if a full week has passed without receiving any feedback. There is a
fair amount of traffic on the mailinglist and it wouldn't be the first time
that a patch was missed by reviewers.

One consequence of this is that as submitter you can get into the situation
that you post something, two weeks later you get a review, you post the
corrected version, you get more reviews 10 days later, etc. So it can be a
drawn-out process. This can be frustrating, but please stick with it. We have
seen cases where people seem to give up, but that is not our intention. We
welcome new code, but since none of the core developers work full time on this
we are constrained by the time we have available. Just be aware of this, plan
accordingly and don't give up.


Contacting developers
=====================

The linux-media mailinglist is the central place to get into contact with
developers. However, there are also two irc channels: #linuxtv (mostly DVB
related) and #v4l (mostly V4L related). Most developers are based in the US or
in Europe, so take those timezones into account.

Finally, you can often find developers during the three main Linux conferences
relevant to us: the Linux Plumbers Conference, the Embedded Linux Conference
and the Embedded Linux Conference Europe.


Patch tags
==========

When posting patches it is recommended to tag them to help us sort through them
quickly and efficiently.

The tags are:

[RFC PATCH x/y]: use this for preliminary patches for which you want to get
some early feedback.

[REVIEW PATCH x/y]: use this for patches that you consider OK for merging, but
that need to be reviewed.

Once your patches have been reviewed/acked you can post either a pull request
("[GIT PULL]") or use the "[FINAL PATCH x/y]" tag if you don't have a public
git tree.

If you post a new version of a patch series, then add 'v1', 'v2', etc. to the
RFC or REVIEW word, e.g.: "[RFCv2 PATCH x/y]".

If your patch is for the current rc kernel (so it is a regression or serious
bug fix), then add " FOR v3.x" after the PATCH or PULL keyword. For example:
"[REVIEW PATCH FOR v3.7 x/y]", or "[GIT PULL FOR v3.7]".

You can use the option --subject-prefix="REVIEW PATCHv1" with the 'git
send-email' to specify the prefix.

Patches without the appropriate tags will be processed manually, which will
take more time and may actually cause them to be dropped altogether.


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.
- Guennadi Liakhovetski: soc-camera drivers
- Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
  for Media Controller core patches.
- 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.

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

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).

Whenever you patch changes state you'll get an email informing you about that.

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Frank Schäfer]

Am 10.12.2012 14:07, schrieb Hans Verkuil:

<snip>
> 3) This document describes the situation we will have when the submaintainers
> take their place early next year. So please check if I got that part right.
...

> 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.
> - Guennadi Liakhovetski: soc-camera drivers
> - Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
>   for Media Controller core patches.
> - 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.
>
> 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
>
> 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.

I've read this "when the submaintainers take their place early next
year, everything will be fine" several times now.
But can anyone please explain me what's going to change ?
AFAICS, the above exactly describes the _current_ situation.
We already have sub-maintainers, sub-trees etc, right !? And the people
listed above are already doing the same job at the moment.

Looking at patchwork, it seems we are behind at least 1 complete kernel
release cycle.
And the reason seems to be, that (at least some) maintainers don't have
the resources to review them in time (no reproaches !).

But to me this seems to be no structural problem.
If a maintainer (permanently) doesn't have the time to review patches,
he should leave maintainership to someone else.

So the actual problem seems to be, that we don't have enough
maintainers/reviewers, right ?


<snip>

> 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).
>
> Whenever you patch changes state you'll get an email informing you about that.

What if people send a V2 of a patch (series). Should they mark V1 as
superseeded themselves ?
And what about maintainers not using patchwork ? Are they nevertheless
supposed to take care of the status of their patches ?

Regards,
Frank

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Hans Verkuil]

On Mon December 10 2012 16:56:16 Frank Schäfer wrote:
> Am 10.12.2012 14:07, schrieb Hans Verkuil:
> 
> <snip>
> > 3) This document describes the situation we will have when the submaintainers
> > take their place early next year. So please check if I got that part right.
> ...
> 
> > 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.
> > - Guennadi Liakhovetski: soc-camera drivers
> > - Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
> >   for Media Controller core patches.
> > - 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.
> >
> > 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
> >
> > 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.
> 
> I've read this "when the submaintainers take their place early next
> year, everything will be fine" several times now.

I doubt everything will be fine, but I sure hope it will be better at least.
In other words, don't expect miracles :-)

> But can anyone please explain me what's going to change ?
> AFAICS, the above exactly describes the _current_ situation.
> We already have sub-maintainers, sub-trees etc, right !? And the people
> listed above are already doing the same job at the moment.
> 
> Looking at patchwork, it seems we are behind at least 1 complete kernel
> release cycle.
> And the reason seems to be, that (at least some) maintainers don't have
> the resources to review them in time (no reproaches !).
> 
> But to me this seems to be no structural problem.
> If a maintainer (permanently) doesn't have the time to review patches,
> he should leave maintainership to someone else.
> 
> So the actual problem seems to be, that we don't have enough
> maintainers/reviewers, right ?

The main problem is that all the work is done by Mauro. Sure, people help
out with reviews but a lot of the final administrative and merge effort is
done by one person only. In particular the patch flow is something he can't
keep up with anymore. So by assigning official submaintainers who get access
to patchwork and can process patches quickly we hope that his job will become
a lot easier.

So the core two changes are 1) giving clear responsibilities to submaintainers
and 2) giving submaintainers access to patchwork to keep track of the patches.

So patch submitters no longer have to wait until Mauro gets around to cleaning
out patchwork. Instead the submaintainers can do that themselves and collect
accepted patches in their git tree and post regular pull requests for Mauro.

It should simplify things substantially for Mauro, we hope.

I think we have enough people doing reviews etc. (although more are always
welcome), it's the patchflow itself that is the problem.

Regards,

	Hans

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Mon, 10 Dec 2012 17:27:29 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Mon December 10 2012 16:56:16 Frank Schäfer wrote:
> > Am 10.12.2012 14:07, schrieb Hans Verkuil:
> > 
> > <snip>
> > > 3) This document describes the situation we will have when the submaintainers
> > > take their place early next year. So please check if I got that part right.
> > ...
> > 
> > > 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.
> > > - Guennadi Liakhovetski: soc-camera drivers
> > > - Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
> > >   for Media Controller core patches.
> > > - 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.
> > >
> > > 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
> > >
> > > 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.
> > 
> > I've read this "when the submaintainers take their place early next
> > year, everything will be fine" several times now.
> 
> I doubt everything will be fine, but I sure hope it will be better at least.
> In other words, don't expect miracles :-)
> 
> > But can anyone please explain me what's going to change ?
> > AFAICS, the above exactly describes the _current_ situation.
> > We already have sub-maintainers, sub-trees etc, right !? And the people
> > listed above are already doing the same job at the moment.
> > 
> > Looking at patchwork, it seems we are behind at least 1 complete kernel
> > release cycle.

No, this is not true; git pull requests are typically handled faster, as
the material there is submitted either by a driver maintainer or by a
sub-maintainer. The delay there for -git is currently 2 weeks, as we closed our
merge window at the beginning of -rc7 (expecting that there won't be a -rc8).

The very large of individual patches have a longer delays, due to the lack of
driver maintainers reviews.

> > And the reason seems to be, that (at least some) maintainers don't have
> > the resources to review them in time (no reproaches !).
> > 
> > But to me this seems to be no structural problem.
> > If a maintainer (permanently) doesn't have the time to review patches,
> > he should leave maintainership to someone else.

Agreed.

> > 
> > So the actual problem seems to be, that we don't have enough
> > maintainers/reviewers, right ?
> 
> The main problem is that all the work is done by Mauro. Sure, people help
> out with reviews but a lot of the final administrative and merge effort is
> done by one person only. In particular the patch flow is something he can't
> keep up with anymore. So by assigning official submaintainers who get access
> to patchwork and can process patches quickly we hope that his job will become
> a lot easier.
> 
> So the core two changes are 1) giving clear responsibilities to submaintainers
> and 2) giving submaintainers access to patchwork to keep track of the patches.
> 
> So patch submitters no longer have to wait until Mauro gets around to cleaning
> out patchwork. Instead the submaintainers can do that themselves and collect
> accepted patches in their git tree and post regular pull requests for Mauro.
> 
> It should simplify things substantially for Mauro, we hope.
> 
> I think we have enough people doing reviews etc. (although more are always
> welcome), it's the patchflow itself that is the problem.

Yeah, the issue is that both reviewed, non-reviewed and rejected/commented
patches go into the very same queue, forcing me to revisit each patch again, 
even the rejected/commented ones, and the previous versions of newer patches.

By giving rights and responsibilities to the sub-maintainers to manage their
stuff directly at patchwork, those patches that tend to stay at patchwork for
a long time will likely disappear, and the queue will be cleaner.

Regards,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Antti Palosaari]

On 12/10/2012 07:38 PM, Mauro Carvalho Chehab wrote:
> Yeah, the issue is that both reviewed, non-reviewed and rejected/commented
> patches go into the very same queue, forcing me to revisit each patch again,
> even the rejected/commented ones, and the previous versions of newer patches.
>
> By giving rights and responsibilities to the sub-maintainers to manage their
> stuff directly at patchwork, those patches that tend to stay at patchwork for
> a long time will likely disappear, and the queue will be cleaner.

Is there any change module maintainer responsibility of patch could do 
what ever he likes to given patch in patchwork?

I have looked it already many times but I can drop only my own patches. 
If someone sends patch to my driver X and I pick it up my GIT tree I 
would like to mark it superseded for patchwork (which is not possible 
currently).

regards
Antti

-- 
http://palosaari.fi/

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Mon, 10 Dec 2012 19:45:40 +0200
Antti Palosaari <crope@iki.fi> escreveu:

> On 12/10/2012 07:38 PM, Mauro Carvalho Chehab wrote:
> > Yeah, the issue is that both reviewed, non-reviewed and rejected/commented
> > patches go into the very same queue, forcing me to revisit each patch again,
> > even the rejected/commented ones, and the previous versions of newer patches.
> >
> > By giving rights and responsibilities to the sub-maintainers to manage their
> > stuff directly at patchwork, those patches that tend to stay at patchwork for
> > a long time will likely disappear, and the queue will be cleaner.
> 
> Is there any change module maintainer responsibility of patch could do 
> what ever he likes to given patch in patchwork?
> 
> I have looked it already many times but I can drop only my own patches. 
> If someone sends patch to my driver X and I pick it up my GIT tree I 
> would like to mark it superseded for patchwork (which is not possible 
> currently).

Patchwork's ACL is very limited. It has 3 types there:
	- People (every email it detects);
	- User (the ones that created a password);
	- Project maintainers;

A "people" can't do anything special, except be promoted to "users", by
setting a password for him.

An "user" can only set his emails, enable/disable opt-out/opt-in, set his
primary project and the number of patches per page.

The Project maintainers can do everything in the project.

It would be great to have a feature there allowing the user to change the
status of their own patches, and to let the project maintainers to delegate
a patch to an user[1].

[1] well, I think it can delegate it right now, but only a project
maintainer can change the patch status, so, delegation doesn't work
if the "delegated user" is not a project owner.

Regards,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Frank Schäfer]

Am 10.12.2012 18:38, schrieb Mauro Carvalho Chehab:
> Em Mon, 10 Dec 2012 17:27:29 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On Mon December 10 2012 16:56:16 Frank Schäfer wrote:
>>> Am 10.12.2012 14:07, schrieb Hans Verkuil:
>>>
>>> <snip>
>>>> 3) This document describes the situation we will have when the submaintainers
>>>> take their place early next year. So please check if I got that part right.
>>> ...
>>>
>>>> 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.
>>>> - Guennadi Liakhovetski: soc-camera drivers
>>>> - Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
>>>>   for Media Controller core patches.
>>>> - 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.
>>>>
>>>> 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
>>>>
>>>> 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.
>>> I've read this "when the submaintainers take their place early next
>>> year, everything will be fine" several times now.
>> I doubt everything will be fine, but I sure hope it will be better at least.
>> In other words, don't expect miracles :-)
>>
>>> But can anyone please explain me what's going to change ?
>>> AFAICS, the above exactly describes the _current_ situation.
>>> We already have sub-maintainers, sub-trees etc, right !? And the people
>>> listed above are already doing the same job at the moment.
>>>
>>> Looking at patchwork, it seems we are behind at least 1 complete kernel
>>> release cycle.
> No, this is not true; git pull requests are typically handled faster, as
> the material there is submitted either by a driver maintainer or by a
> sub-maintainer. The delay there for -git is currently 2 weeks, as we closed our
> merge window at the beginning of -rc7 (expecting that there won't be a -rc8).

But it's not "git pull request" vs. "patches sent to the list directly",
it's "reviewed" vs. "not reviewed", right ?

> The very large of individual patches have a longer delays, due to the lack of
> driver maintainers reviews.

That's what I said, the problem is that we lack maintainers/reviewers.
And people beeing subsystem maintainers AND driver maintainers have to
find a balance between processing pull requests and reviewing patches.
I'm not sure if I have understood yet how this balance should look
like... Can you elaborate a bit on this ?
At the moment it's ~12 weeks / ~2 weeks. What's the target value ? ;)

>
>>> And the reason seems to be, that (at least some) maintainers don't have
>>> the resources to review them in time (no reproaches !).
>>>
>>> But to me this seems to be no structural problem.
>>> If a maintainer (permanently) doesn't have the time to review patches,
>>> he should leave maintainership to someone else.
> Agreed.
>
>>> So the actual problem seems to be, that we don't have enough
>>> maintainers/reviewers, right ?
>> The main problem is that all the work is done by Mauro. Sure, people help
>> out with reviews but a lot of the final administrative and merge effort is
>> done by one person only. In particular the patch flow is something he can't
>> keep up with anymore. So by assigning official submaintainers who get access
>> to patchwork and can process patches quickly we hope that his job will become
>> a lot easier.
>>
>> So the core two changes are 1) giving clear responsibilities to submaintainers
>> and 2) giving submaintainers access to patchwork to keep track of the patches.
>>
>> So patch submitters no longer have to wait until Mauro gets around to cleaning
>> out patchwork. Instead the submaintainers can do that themselves and collect
>> accepted patches in their git tree and post regular pull requests for Mauro.
>>
>> It should simplify things substantially for Mauro, we hope.
>>
>> I think we have enough people doing reviews etc. (although more are always
>> welcome), it's the patchflow itself that is the problem.
> Yeah, the issue is that both reviewed, non-reviewed and rejected/commented
> patches go into the very same queue, forcing me to revisit each patch again, 
> even the rejected/commented ones, and the previous versions of newer patches.
>
> By giving rights and responsibilities to the sub-maintainers to manage their
> stuff directly at patchwork, those patches that tend to stay at patchwork for
> a long time will likely disappear, and the queue will be cleaner.

So who can get an account / is supposed to access patchwork ?
- subsystem maintainers ?
- driver maintainers ?
- patch creators ?

Regards,
Frank

> Regards,
> Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Mon, 10 Dec 2012 20:17:23 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 10.12.2012 18:38, schrieb Mauro Carvalho Chehab:
> > Em Mon, 10 Dec 2012 17:27:29 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> On Mon December 10 2012 16:56:16 Frank Schäfer wrote:
> >>> Am 10.12.2012 14:07, schrieb Hans Verkuil:
> >>>
> >>> <snip>
> >>>> 3) This document describes the situation we will have when the submaintainers
> >>>> take their place early next year. So please check if I got that part right.
> >>> ...
> >>>
> >>>> 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.
> >>>> - Guennadi Liakhovetski: soc-camera drivers
> >>>> - Laurent Pinchart: sensor subdev drivers.  In addition he'll be the reviewer
> >>>>   for Media Controller core patches.
> >>>> - 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.
> >>>>
> >>>> 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
> >>>>
> >>>> 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.
> >>> I've read this "when the submaintainers take their place early next
> >>> year, everything will be fine" several times now.
> >> I doubt everything will be fine, but I sure hope it will be better at least.
> >> In other words, don't expect miracles :-)
> >>
> >>> But can anyone please explain me what's going to change ?
> >>> AFAICS, the above exactly describes the _current_ situation.
> >>> We already have sub-maintainers, sub-trees etc, right !? And the people
> >>> listed above are already doing the same job at the moment.
> >>>
> >>> Looking at patchwork, it seems we are behind at least 1 complete kernel
> >>> release cycle.
> > No, this is not true; git pull requests are typically handled faster, as
> > the material there is submitted either by a driver maintainer or by a
> > sub-maintainer. The delay there for -git is currently 2 weeks, as we closed our
> > merge window at the beginning of -rc7 (expecting that there won't be a -rc8).
> 
> But it's not "git pull request" vs. "patches sent to the list directly",
> it's "reviewed" vs. "not reviewed", right ?

A patch reviewed by a sub-maintainer/driver maintainers typically goes to me 
via a git pull request.

> > The very large of individual patches have a longer delays, due to the lack of
> > driver maintainers reviews.
> 
> That's what I said, the problem is that we lack maintainers/reviewers.

We (used to) lack to have sub-maintainers formally (except for gspca and
soc_camera). Even driver's maintainership is currently shady, as most 
drivers lack a MAINTAINERS entry. That is in the process of being fixed.

> And people beeing subsystem maintainers AND driver maintainers have to
> find a balance between processing pull requests and reviewing patches.
> I'm not sure if I have understood yet how this balance should look
> like... Can you elaborate a bit on this ?
> At the moment it's ~12 weeks / ~2 weeks. What's the target value ? ;)

Please wait for it to be implemented before complaining it ;) The 
sub-maintainers new schema will start to work likely by Feb/Mar 2013.

Also, the reviewing process is not equal to all patches: trivial patches
can be merged quicker; core API changes should take a longer time, as
it is expected to have more review before merging them.

> >>> And the reason seems to be, that (at least some) maintainers don't have
> >>> the resources to review them in time (no reproaches !).
> >>>
> >>> But to me this seems to be no structural problem.
> >>> If a maintainer (permanently) doesn't have the time to review patches,
> >>> he should leave maintainership to someone else.
> > Agreed.
> >
> >>> So the actual problem seems to be, that we don't have enough
> >>> maintainers/reviewers, right ?
> >> The main problem is that all the work is done by Mauro. Sure, people help
> >> out with reviews but a lot of the final administrative and merge effort is
> >> done by one person only. In particular the patch flow is something he can't
> >> keep up with anymore. So by assigning official submaintainers who get access
> >> to patchwork and can process patches quickly we hope that his job will become
> >> a lot easier.
> >>
> >> So the core two changes are 1) giving clear responsibilities to submaintainers
> >> and 2) giving submaintainers access to patchwork to keep track of the patches.
> >>
> >> So patch submitters no longer have to wait until Mauro gets around to cleaning
> >> out patchwork. Instead the submaintainers can do that themselves and collect
> >> accepted patches in their git tree and post regular pull requests for Mauro.
> >>
> >> It should simplify things substantially for Mauro, we hope.
> >>
> >> I think we have enough people doing reviews etc. (although more are always
> >> welcome), it's the patchflow itself that is the problem.
> > Yeah, the issue is that both reviewed, non-reviewed and rejected/commented
> > patches go into the very same queue, forcing me to revisit each patch again, 
> > even the rejected/commented ones, and the previous versions of newer patches.
> >
> > By giving rights and responsibilities to the sub-maintainers to manage their
> > stuff directly at patchwork, those patches that tend to stay at patchwork for
> > a long time will likely disappear, and the queue will be cleaner.
> 
> So who can get an account / is supposed to access patchwork ?
> - subsystem maintainers ?
> - driver maintainers ?
> - patch creators ?

Subsystem maintainers only, except if someone can fix patchwork, adding
proper ACL's there to allow patch creators to manage their own patches
and sub-system maintainers to delegate work to driver maintainers, without
giving them full rights, and being notified about status changes on
those driver's patches.

The current way patchwork implements it requires a very high degree of trust
between the ones handling patches there, as anyone with access to patchwork
can do bad things there affecting someone's else workflow. 

Regards,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Frank Schäfer]

Am 10.12.2012 20:40, schrieb Mauro Carvalho Chehab:

[snip]

>> And people beeing subsystem maintainers AND driver maintainers have to
>> find a balance between processing pull requests and reviewing patches.
>> I'm not sure if I have understood yet how this balance should look
>> like... Can you elaborate a bit on this ?
>> At the moment it's ~12 weeks / ~2 weeks. What's the target value ? ;)
> Please wait for it to be implemented before complaining it ;) The 
> sub-maintainers new schema will start to work likely by Feb/Mar 2013.

I don't want to complain (yet ;) ). I'm just trying to understand what
is supposed to reduce the review times...
Haven't succeeded yet, because the same amount work seems to be
redivided among the same amount of maintainer/reviewer resources (=people).

Anyway, I will be patient and hope that things will evolve as planed.
I will also try to test and/or review patches from other if possible.

[snip]

>> So who can get an account / is supposed to access patchwork ?
>> - subsystem maintainers ?
>> - driver maintainers ?
>> - patch creators ?
> Subsystem maintainers only, except if someone can fix patchwork, adding
> proper ACL's there to allow patch creators to manage their own patches
> and sub-system maintainers to delegate work to driver maintainers, without
> giving them full rights, and being notified about status changes on
> those driver's patches.

Ok, thanks, I think this should be mentioned in the document.

Regards,
Frank

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Mon, 10 Dec 2012 14:07:09 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi all,
> 
> As discussed in Barcelona I would write a text describing requirements for new
> drivers and what to expect when submitting patches to linux-media.
> 
> This is a first rough draft and nothing is fixed yet.
> 
> I have a few open questions:
> 
> 1) Where to put it?

Maybe at media-build.git. I'm thinking on putting there, under devel_contrib,
the main scripts I use here to handle patches.

/me needs some time to sanitize them and add there.

> One thing I would propose that we improve is to move the
> dvb and video4linux directories in Documentation/ to Documentation/media to
> correctly reflect the drivers/media structure. If we do that, then we can put
> this document in Documentation/media/SubmittingMediaPatches.

Hmm... I don't see any other subsystems having their own document for that.
We may need to discuss it upstream before doing that, and be prepared
to answer why we thing sub-systems would need their own rules there.

In any case, I think that the better is to store it at media-build.git tree,
and later open such discussions upstream, if we think it is valuable enough.

> Alternatively, this is something we can document in our wiki.
> 
> 2) Are there DVB requirements as well for new drivers? We discussed a list of
> V4L2 requirements in Barcelona, but I wonder if there is a similar list that
> can be made for DVB drivers. Input on that will be welcome.

See below.

> 3) This document describes the situation we will have when the submaintainers
> take their place early next year. So please check if I got that part right.
> 
> 4) In Barcelona we discussed 'tags' for patches to help organize them. I've
> made a proposal for those in this document. Feedback is very welcome.
> 
> 5) As discussed in Barcelona there will be git tree maintainers for specific
> platforms, but we didn't really go into detail who would be responsible for
> which platform. If you want to maintain a particular platform, then please
> let me know.
> 
> 6) The patchwork section is very short at the moment. It should be extended
> when patchwork gets support to recognize the various tags.
> 
> 7) Anything else that should be discussed here?
> 
> Again, remember that this is a rough draft only, so be gentle with me :-)
> 
> Regards,
> 
> 	Hans
> 
> --------------------------- cut here -------------------------------
> 
> General Information
> ===================
> 
> For general information on how to submit patches see:
> 
> http://linuxtv.org/wiki/index.php/Developer_Section
> 
> In particular the section 'Submitting Your Work'.
> 
> This document goes into more detail regarding media specific requirements when
> submitting patches and what the patch flow looks like in this subsystem.

I think we should add a paragraph here saying that rules may have exceptions,
when there's a clear reason why a certain submission should need a different
criteria.

Also, IMHO, we should add a notice that this list is not exhaustive, and may
be changed, keeping it for at least one or two Kernel cycles, while it doesn't
get proofed/matured, as I'm sure we'll forget things.

> 
> 
> 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."

> 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;
  - Errors should be reported as negative numbers, using the Kernel error
    codes;
  - typedefs should't be used;

> 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;

> 
> How to deal with checkpatch.pl?
> ===============================
> 
> First of all, the requirement to comply to the kernel coding style is there for
> a reason. Sometimes people feel that it is a pointless exercise: after all,
> code is code, right? Why would just changing some spacing improve it?
> 
> But the coding style is not there to help you (at least, not directly), it is
> there to help those who have to review and/or maintain your code as it takes a
> lot of time to review code or try to figure out how someone else's code works.
> By at least ensuring that the coding style is consistent with other code we can
> concentrate on what humans to best: pattern matching. Ever read a book or
> article that did not use the correct spelling, grammar and/or punctuation
> rules? Did you notice how your brain 'stumbles' whenever it encounters such
> mistakes? It makes the text harder to understand and slower to read. The same
> happens with code that does not comply to the conventions of the project and it
> is the reason why most large projects, both open source and proprietary, have a
> coding style.
> 
> However, when interpreting the checkpatch output it is good to remember that it
> is just an automated tool and there are cases where what checkpatch recommends
> does not actually results in the best readable code. This is particularly true
> for the line length warnings. A warning that a line is 82 characters long can
> probably be ignored, since breaking up such a line will usually make the code
> harder to understand. A warning that a line is 101 characters long definitely
> needs attention, since that's an indication that the line is really too long.

I wouldn't say that, as people will likely read that an 82 chars long line
should be ignored ;)

IMHO, the better is to give some examples for this specific warning (for example:
function calls and function declarations with more than 80 chars should likely be
broken, even if they have 81 columns; lines with an string at the end should likely
not be broken, even if it has more than 80 columns, etc).

> The guideline here is to check such warnings, but use common sense whether or
> not to fix them.
> 
> Please do run checkpatch before posting any code to the mailinglist. Code that
> clearly violates the kernel coding style will be rejected and you will be asked
> to repost after fixing the style. We are not going to waste time trying to
> review code that uses a non-standard coding style, our time is too limited for
> that.
> 
> The sole exception are staging drivers as the only rule there is that it
> compiles.
> 
> 
> Timeline for code submissions
> =============================
> 
> After a new kernel is released the merge window will be open for about two
> weeks

Please add:
	"for the maintainers to send him the patches they already received
during the last development cycle, and that went into the linux-next tree
in time for the other maintainers and reviewers to double-check the entire
set of changes for the next Linux version."

(yeah, I know you're talking more about it later, but I think this makes it
a little clearer that no submissions will typically be accepted so late at
the development cycle).

> During that time Linus will merge any pending work for the next kernel.
> Once that merge window is closed only regression fixes and serious bug fixes
> will be accepted, everything else will be postponed

please add:
	"upstream"

> until the next merge window.
> 
> In addition, before anything can be merged (regardless of whether this is
> during the merge window or not) the new code should have been in the linux-next
> tree for about a week at minimum to ensure there are no conflicts with work
> being done in other kernel subsystems.
> 
> Furthermore, before code can be added to linux-next it has to be reviewed
> first.  This will take time as well. Adding everything up this means that if
> you want your code to be merged for the next kernel you should have it posted
> to the linux-media mailinglist no later than rc5 of the current kernel, or it
> may be too late. In fact, the earlier the better since reviews will take time,
> and if corrections need to be made you may have to do several review/submit
> cycles.
> 
> Remember that the core media developers have a job as well, and so won't always
> have the time to review immediately. A general rule of thumb is to post a
> reminder if a full week has passed without receiving any feedback. There is a
> fair amount of traffic on the mailinglist and it wouldn't be the first time
> that a patch was missed by reviewers.
> 
> One consequence of this is that as submitter you can get into the situation
> that you post something, two weeks later you get a review, you post the
> corrected version, you get more reviews 10 days later, etc. So it can be a
> drawn-out process. This can be frustrating, but please stick with it.

Please add:
	"The reason for these measures is to warrant, in our best, that no
regressions will be added into the Kernel, keeping it with a high quality, and,
yet, allowing to release a new Kernel on every 7-9 weeks."

> We have
> seen cases where people seem to give up, but that is not our intention. We
> welcome new code, but since none of the core developers work full time on this
> we are constrained by the time we have available. Just be aware of this, plan
> accordingly and don't give up.
> 
> 
> Contacting developers
> =====================
> 
> The linux-media mailinglist is the central place to get into contact with
> developers. However, there are also two irc channels: #linuxtv (mostly DVB
> related) and #v4l (mostly V4L related). Most developers are based in the US or
> in Europe, so take those timezones into account.

I would add there:

	"If you ask something at the IRC channel, please wait for your
answer, as it may take some time for a developer to be able to find a
timeslot to answer you".
> 
> Finally, you can often find developers during the three main Linux conferences
> relevant to us: the Linux Plumbers Conference, the Embedded Linux Conference
> and the Embedded Linux Conference Europe.

I would say (maybe instead) to the Media mini-summits that happen typically
together with one major Linux Foundation conference (typically either in
Europe or US), like the Linux Plumbers Conference, the Embedded Linux Conference
and/or the Embedded Linux Conference Europe.

> 
> 
> Patch tags
> ==========
> 
> When posting patches it is recommended to tag them to help us sort through them
> quickly and efficiently.
> 
> The tags are:
> 
> [RFC PATCH x/y]: use this for preliminary patches for which you want to get
> some early feedback.
> 
> [REVIEW PATCH x/y]: use this for patches that you consider OK for merging, but
> that need to be reviewed.
> 
> Once your patches have been reviewed/acked you can post either a pull request
> ("[GIT PULL]") or use the "[FINAL PATCH x/y]" tag if you don't have a public
> git tree.
> 
> If you post a new version of a patch series, then add 'v1', 'v2', etc. to the
> RFC or REVIEW word, e.g.: "[RFCv2 PATCH x/y]".
> 
> If your patch is for the current rc kernel (so it is a regression or serious
> bug fix), then add " FOR v3.x" after the PATCH or PULL keyword. For example:
> "[REVIEW PATCH FOR v3.7 x/y]", or "[GIT PULL FOR v3.7]".
> 
> You can use the option --subject-prefix="REVIEW PATCHv1" with the 'git
> send-email' to specify the prefix.
> 
> Patches without the appropriate tags will be processed manually, which will
> take more time and may actually cause them to be dropped altogether.
> 
> 
> 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.

> - 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.

> - 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.

> 
> 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?

> 
> 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?)

> 
> Whenever you patch changes state you'll get an email informing you about that.


Patchwork has an opt-out way to disable those notifications. While I expect
that nobody would opt-out, I think we should mention it, as patchwork is
not a spammer: it only sends email only to track the status of a patch, and 
only after its submission. Also, it offers a way to opt-out of such
notifications.

Regards,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Laurent Pinchart]

Hi,

On Monday 10 December 2012 16:33:13 Mauro Carvalho Chehab wrote:
> Em Mon, 10 Dec 2012 14:07:09 +0100 Hans Verkuil escreveu:
> > Hi all,
> > 
> > As discussed in Barcelona I would write a text describing requirements for
> > new drivers and what to expect when submitting patches to linux-media.
> > 
> > This is a first rough draft and nothing is fixed yet.
> > 
> > I have a few open questions:
> > 
> > 1) Where to put it?
> 
> Maybe at media-build.git.

Not everybody uses (or is even aware of) media-build. The goal here is to make 
sure that new driver developers will run into the guidelines before then spend 
months writing code that can't be upstreamed. Documentation/ thus looks like a 
good place to me.

It might be a good idea to add a reference to the guidelines in the API 
DocBook documentation, regardless of where the guidelines end up being stored. 
If a developer reads a single document only it will probably be the API 
reference.

> I'm thinking on putting there, under devel_contrib, the main scripts I use
> here to handle patches.
> 
> /me needs some time to sanitize them and add there.
> 
> > One thing I would propose that we improve is to move the dvb and
> > video4linux directories in Documentation/ to Documentation/media to
> > correctly reflect the drivers/media structure. If we do that, then we can
> > put this document in Documentation/media/SubmittingMediaPatches.
> 
> Hmm... I don't see any other subsystems having their own document for that.
> We may need to discuss it upstream before doing that, and be prepared
> to answer why we thing sub-systems would need their own rules there.

Things like requiring the use of the control framework is obviously V4L-
specific, we can't add that to Documentation/SubmittingPatches :-)

> In any case, I think that the better is to store it at media-build.git tree,
> and later open such discussions upstream, if we think it is valuable
> enough.
>
> > Alternatively, this is something we can document in our wiki.
> > 
> > 2) Are there DVB requirements as well for new drivers? We discussed a list
> > of V4L2 requirements in Barcelona, but I wonder if there is a similar
> > list that can be made for DVB drivers. Input on that will be welcome.
> 
> See below.
> 
> > 3) This document describes the situation we will have when the
> > submaintainers take their place early next year. So please check if I got
> > that part right.
> > 
> > 4) In Barcelona we discussed 'tags' for patches to help organize them.
> > I've made a proposal for those in this document. Feedback is very welcome.
> > 
> > 5) As discussed in Barcelona there will be git tree maintainers for
> > specific platforms, but we didn't really go into detail who would be
> > responsible for which platform. If you want to maintain a particular
> > platform, then please let me know.
> > 
> > 6) The patchwork section is very short at the moment. It should be
> > extended
> > when patchwork gets support to recognize the various tags.
> > 
> > 7) Anything else that should be discussed here?
> > 
> > Again, remember that this is a rough draft only, so be gentle with me :-)
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > --------------------------- cut here -------------------------------
> > 
> > General Information
> > ===================
> > 
> > For general information on how to submit patches see:
> > 
> > http://linuxtv.org/wiki/index.php/Developer_Section
> > 
> > In particular the section 'Submitting Your Work'.
> > 
> > This document goes into more detail regarding media specific requirements
> > when submitting patches and what the patch flow looks like in this
> > subsystem.
>
> I think we should add a paragraph here saying that rules may have
> exceptions, when there's a clear reason why a certain submission should need
> a different criteria.

I agree. For instance the uvcvideo driver doesn't use the control framework, 
and has good reasons not to. This must be the exception rather than the rule, 
but we might have more than one exception.

> Also, IMHO, we should add a notice that this list is not exhaustive, and may
> be changed, keeping it for at least one or two Kernel cycles, while it
> doesn't get proofed/matured, as I'm sure we'll forget things.
> 
> > 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. And I'm pretty sure we'll always send a notice.

> > 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* ? :-)

>   - Errors should be reported as negative numbers, using the Kernel error
>     codes;

CodingStyle, chapter 16 (although not as clearly stated).

>   - typedefs should't be used;

CodingStyle, chapter 5.

> > 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;
> 
> > How to deal with checkpatch.pl?
> > ===============================
> > 
> > First of all, the requirement to comply to the kernel coding style is
> > there for a reason. Sometimes people feel that it is a pointless
> > exercise: after all, code is code, right? Why would just changing some
> > spacing improve it?
> > 
> > But the coding style is not there to help you (at least, not directly), it
> > is there to help those who have to review and/or maintain your code as it
> > takes a lot of time to review code or try to figure out how someone
> > else's code works. By at least ensuring that the coding style is
> > consistent with other code we can concentrate on what humans to best:
> > pattern matching. Ever read a book or article that did not use the
> > correct spelling, grammar and/or punctuation rules? Did you notice how
> > your brain 'stumbles' whenever it encounters such mistakes? It makes the
> > text harder to understand and slower to read. The same happens with code
> > that does not comply to the conventions of the project and it is the
> > reason why most large projects, both open source and proprietary, have a
> > coding style.
> > 
> > However, when interpreting the checkpatch output it is good to remember
> > that it is just an automated tool and there are cases where what
> > checkpatch recommends does not actually results in the best readable
> > code. This is particularly true for the line length warnings. A warning
> > that a line is 82 characters long can probably be ignored, since breaking
> > up such a line will usually make the code harder to understand. A warning
> > that a line is 101 characters long definitely needs attention, since
> > that's an indication that the line is really too long.
> I wouldn't say that, as people will likely read that an 82 chars long line
> should be ignored ;)
> 
> IMHO, the better is to give some examples for this specific warning (for
> example: function calls and function declarations with more than 80 chars
> should likely be broken, even if they have 81 columns; lines with an string
> at the end should likely not be broken, even if it has more than 80
> columns, etc).

Agreed.

> > The guideline here is to check such warnings, but use common sense whether
> > or not to fix them.
> > 
> > Please do run checkpatch before posting any code to the mailinglist. Code
> > that clearly violates the kernel coding style will be rejected and you
> > will be asked to repost after fixing the style. We are not going to waste
> > time trying to review code that uses a non-standard coding style, our
> > time is too limited for that.
> > 
> > The sole exception are staging drivers as the only rule there is that it
> > compiles.
> > 
> > 
> > Timeline for code submissions
> > =============================
> > 
> > After a new kernel is released the merge window will be open for about two
> > weeks
> 
> Please add:
> 	"for the maintainers to send him the patches they already received

"him" ? I suppose you mean Linus :-)

> during the last development cycle, and that went into the linux-next tree
> in time for the other maintainers and reviewers to double-check the entire
> set of changes for the next Linux version."
> 
> (yeah, I know you're talking more about it later, but I think this makes it
> a little clearer that no submissions will typically be accepted so late at
> the development cycle).
> 
> > During that time Linus will merge any pending work for the next kernel.
> > Once that merge window is closed only regression fixes and serious bug
> > fixes will be accepted, everything else will be postponed
> 
> please add:
> 	"upstream"

postponed upstream ? What do you mean ?

> > until the next merge window.
> > 
> > In addition, before anything can be merged (regardless of whether this is
> > during the merge window or not) the new code should have been in the
> > linux-next tree for about a week at minimum to ensure there are no
> > conflicts with work being done in other kernel subsystems.
> > 
> > Furthermore, before code can be added to linux-next it has to be reviewed
> > first.  This will take time as well. Adding everything up this means that
> > if you want your code to be merged for the next kernel you should have it
> > posted to the linux-media mailinglist no later than rc5 of the current
> > kernel, or it may be too late. In fact, the earlier the better since
> > reviews will take time, and if corrections need to be made you may have
> > to do several review/submit cycles.
> > 
> > Remember that the core media developers have a job as well, and so won't
> > always have the time to review immediately. A general rule of thumb is to
> > post a reminder if a full week has passed without receiving any feedback.
> > There is a fair amount of traffic on the mailinglist and it wouldn't be
> > the first time that a patch was missed by reviewers.
> > 
> > One consequence of this is that as submitter you can get into the
> > situation that you post something, two weeks later you get a review, you
> > post the corrected version, you get more reviews 10 days later, etc. So it
> > can be a drawn-out process. This can be frustrating, but please stick with
> > it.
> 
> Please add:
> 	"The reason for these measures is to warrant, in our best, that no
> regressions will be added into the Kernel, keeping it with a high quality,
> and, yet, allowing to release a new Kernel on every 7-9 weeks."
> 
> > We have seen cases where people seem to give up, but that is not our
> > intention. We welcome new code, but since none of the core developers work
> > full time on this we are constrained by the time we have available. Just
> > be aware of this, plan accordingly and don't give up.
> > 
> > 
> > Contacting developers
> > =====================
> > 
> > The linux-media mailinglist is the central place to get into contact with
> > developers. However, there are also two irc channels: #linuxtv (mostly DVB
> > related) and #v4l (mostly V4L related). Most developers are based in the
> > US or in Europe, so take those timezones into account.
> 
> I would add there:
> 
> 	"If you ask something at the IRC channel, please wait for your
> answer, as it may take some time for a developer to be able to find a
> timeslot to answer you".
> 
> > Finally, you can often find developers during the three main Linux
> > conferences relevant to us: the Linux Plumbers Conference, the Embedded
> > Linux Conference and the Embedded Linux Conference Europe.
> 
> I would say (maybe instead) to the Media mini-summits that happen typically
> together with one major Linux Foundation conference (typically either in
> Europe or US), like the Linux Plumbers Conference, the Embedded Linux
> Conference and/or the Embedded Linux Conference Europe.
> 
> > Patch tags
> > ==========
> > 
> > When posting patches it is recommended to tag them to help us sort through
> > them quickly and efficiently.
> > 
> > The tags are:
> > 
> > [RFC PATCH x/y]: use this for preliminary patches for which you want to
> > get some early feedback.
> > 
> > [REVIEW PATCH x/y]: use this for patches that you consider OK for merging,
> > but that need to be reviewed.
> > 
> > Once your patches have been reviewed/acked you can post either a pull
> > request ("[GIT PULL]") or use the "[FINAL PATCH x/y]" tag if you don't
> > have a public git tree.
> > 
> > If you post a new version of a patch series, then add 'v1', 'v2', etc. to
> > the RFC or REVIEW word, e.g.: "[RFCv2 PATCH x/y]".
> > 
> > If your patch is for the current rc kernel (so it is a regression or
> > serious bug fix), then add " FOR v3.x" after the PATCH or PULL keyword.
> > For example: "[REVIEW PATCH FOR v3.7 x/y]", or "[GIT PULL FOR v3.7]".
> > 
> > You can use the option --subject-prefix="REVIEW PATCHv1" with the 'git
> > send-email' to specify the prefix.
> > 
> > Patches without the appropriate tags will be processed manually, which
> > will take more time and may actually cause them to be dropped altogether.
> > 
> > 
> > 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.

> > - 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.

> > 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.

> > Whenever you patch changes state you'll get an email informing you about
> > that.
>
> Patchwork has an opt-out way to disable those notifications. While I expect
> that nobody would opt-out, I think we should mention it, as patchwork is
> not a spammer: it only sends email only to track the status of a patch, and
> only after its submission. Also, it offers a way to opt-out of such
> notifications.

-- 
Regards,

Laurent Pinchart

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Tue, 11 Dec 2012 00:52:39 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi,
> 
> On Monday 10 December 2012 16:33:13 Mauro Carvalho Chehab wrote:
> > Em Mon, 10 Dec 2012 14:07:09 +0100 Hans Verkuil escreveu:
> > > Hi all,
> > > 
> > > As discussed in Barcelona I would write a text describing requirements for
> > > new drivers and what to expect when submitting patches to linux-media.
> > > 
> > > This is a first rough draft and nothing is fixed yet.
> > > 
> > > I have a few open questions:
> > > 
> > > 1) Where to put it?
> > 
> > Maybe at media-build.git.
> 
> Not everybody uses (or is even aware of) media-build. The goal here is to make 
> sure that new driver developers will run into the guidelines before then spend 
> months writing code that can't be upstreamed. Documentation/ thus looks like a 
> good place to me.
> 
> It might be a good idea to add a reference to the guidelines in the API 
> DocBook documentation, regardless of where the guidelines end up being stored. 
> If a developer reads a single document only it will probably be the API 
> reference.

Agreed.
> 
> > I'm thinking on putting there, under devel_contrib, the main scripts I use
> > here to handle patches.
> > 
> > /me needs some time to sanitize them and add there.
> > 
> > > One thing I would propose that we improve is to move the dvb and
> > > video4linux directories in Documentation/ to Documentation/media to
> > > correctly reflect the drivers/media structure. If we do that, then we can
> > > put this document in Documentation/media/SubmittingMediaPatches.
> > 
> > Hmm... I don't see any other subsystems having their own document for that.
> > We may need to discuss it upstream before doing that, and be prepared
> > to answer why we thing sub-systems would need their own rules there.
> 
> Things like requiring the use of the control framework is obviously V4L-
> specific, we can't add that to Documentation/SubmittingPatches :-)
> 
> > In any case, I think that the better is to store it at media-build.git tree,
> > and later open such discussions upstream, if we think it is valuable
> > enough.
> >
> > > Alternatively, this is something we can document in our wiki.
> > > 
> > > 2) Are there DVB requirements as well for new drivers? We discussed a list
> > > of V4L2 requirements in Barcelona, but I wonder if there is a similar
> > > list that can be made for DVB drivers. Input on that will be welcome.
> > 
> > See below.
> > 
> > > 3) This document describes the situation we will have when the
> > > submaintainers take their place early next year. So please check if I got
> > > that part right.
> > > 
> > > 4) In Barcelona we discussed 'tags' for patches to help organize them.
> > > I've made a proposal for those in this document. Feedback is very welcome.
> > > 
> > > 5) As discussed in Barcelona there will be git tree maintainers for
> > > specific platforms, but we didn't really go into detail who would be
> > > responsible for which platform. If you want to maintain a particular
> > > platform, then please let me know.
> > > 
> > > 6) The patchwork section is very short at the moment. It should be
> > > extended
> > > when patchwork gets support to recognize the various tags.
> > > 
> > > 7) Anything else that should be discussed here?
> > > 
> > > Again, remember that this is a rough draft only, so be gentle with me :-)
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > --------------------------- cut here -------------------------------
> > > 
> > > General Information
> > > ===================
> > > 
> > > For general information on how to submit patches see:
> > > 
> > > http://linuxtv.org/wiki/index.php/Developer_Section
> > > 
> > > In particular the section 'Submitting Your Work'.
> > > 
> > > This document goes into more detail regarding media specific requirements
> > > when submitting patches and what the patch flow looks like in this
> > > subsystem.
> >
> > I think we should add a paragraph here saying that rules may have
> > exceptions, when there's a clear reason why a certain submission should need
> > a different criteria.
> 
> I agree. For instance the uvcvideo driver doesn't use the control framework, 
> and has good reasons not to. This must be the exception rather than the rule, 
> but we might have more than one exception.

Yes.

> 
> > Also, IMHO, we should add a notice that this list is not exhaustive, and may
> > be changed, keeping it for at least one or two Kernel cycles, while it
> > doesn't get proofed/matured, as I'm sure we'll forget things.
> > 
> > > 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.

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;
> > 
> > > How to deal with checkpatch.pl?
> > > ===============================
> > > 
> > > First of all, the requirement to comply to the kernel coding style is
> > > there for a reason. Sometimes people feel that it is a pointless
> > > exercise: after all, code is code, right? Why would just changing some
> > > spacing improve it?
> > > 
> > > But the coding style is not there to help you (at least, not directly), it
> > > is there to help those who have to review and/or maintain your code as it
> > > takes a lot of time to review code or try to figure out how someone
> > > else's code works. By at least ensuring that the coding style is
> > > consistent with other code we can concentrate on what humans to best:
> > > pattern matching. Ever read a book or article that did not use the
> > > correct spelling, grammar and/or punctuation rules? Did you notice how
> > > your brain 'stumbles' whenever it encounters such mistakes? It makes the
> > > text harder to understand and slower to read. The same happens with code
> > > that does not comply to the conventions of the project and it is the
> > > reason why most large projects, both open source and proprietary, have a
> > > coding style.
> > > 
> > > However, when interpreting the checkpatch output it is good to remember
> > > that it is just an automated tool and there are cases where what
> > > checkpatch recommends does not actually results in the best readable
> > > code. This is particularly true for the line length warnings. A warning
> > > that a line is 82 characters long can probably be ignored, since breaking
> > > up such a line will usually make the code harder to understand. A warning
> > > that a line is 101 characters long definitely needs attention, since
> > > that's an indication that the line is really too long.
> > I wouldn't say that, as people will likely read that an 82 chars long line
> > should be ignored ;)
> > 
> > IMHO, the better is to give some examples for this specific warning (for
> > example: function calls and function declarations with more than 80 chars
> > should likely be broken, even if they have 81 columns; lines with an string
> > at the end should likely not be broken, even if it has more than 80
> > columns, etc).
> 
> Agreed.
> 
> > > The guideline here is to check such warnings, but use common sense whether
> > > or not to fix them.
> > > 
> > > Please do run checkpatch before posting any code to the mailinglist. Code
> > > that clearly violates the kernel coding style will be rejected and you
> > > will be asked to repost after fixing the style. We are not going to waste
> > > time trying to review code that uses a non-standard coding style, our
> > > time is too limited for that.
> > > 
> > > The sole exception are staging drivers as the only rule there is that it
> > > compiles.
> > > 
> > > 
> > > Timeline for code submissions
> > > =============================
> > > 
> > > After a new kernel is released the merge window will be open for about two
> > > weeks
> > 
> > Please add:
> > 	"for the maintainers to send him the patches they already received
> 
> "him" ? I suppose you mean Linus :-)

Yes. Maybe it can be changed from "send him" to "send upstream".
> 
> > during the last development cycle, and that went into the linux-next tree
> > in time for the other maintainers and reviewers to double-check the entire
> > set of changes for the next Linux version."
> > 
> > (yeah, I know you're talking more about it later, but I think this makes it
> > a little clearer that no submissions will typically be accepted so late at
> > the development cycle).
> > 
> > > During that time Linus will merge any pending work for the next kernel.
> > > Once that merge window is closed only regression fixes and serious bug
> > > fixes will be accepted, everything else will be postponed
> > 
> > please add:
> > 	"upstream"
> 
> postponed upstream ? What do you mean ?

This is just a small correction, but maybe it can be a little more verbose.

What I meant to say is that, after the merge window, the patches will be 
"postponed for upstream merge until the next merge window".

The original text could give the impression that even at the subsystem
submaintainers/maintainers tree, the patches will be merged only during
a merge window.

> 
> > > until the next merge window.
> > > 
> > > In addition, before anything can be merged (regardless of whether this is
> > > during the merge window or not) the new code should have been in the
> > > linux-next tree for about a week at minimum to ensure there are no
> > > conflicts with work being done in other kernel subsystems.
> > > 
> > > Furthermore, before code can be added to linux-next it has to be reviewed
> > > first.  This will take time as well. Adding everything up this means that
> > > if you want your code to be merged for the next kernel you should have it
> > > posted to the linux-media mailinglist no later than rc5 of the current
> > > kernel, or it may be too late. In fact, the earlier the better since
> > > reviews will take time, and if corrections need to be made you may have
> > > to do several review/submit cycles.
> > > 
> > > Remember that the core media developers have a job as well, and so won't
> > > always have the time to review immediately. A general rule of thumb is to
> > > post a reminder if a full week has passed without receiving any feedback.
> > > There is a fair amount of traffic on the mailinglist and it wouldn't be
> > > the first time that a patch was missed by reviewers.
> > > 
> > > One consequence of this is that as submitter you can get into the
> > > situation that you post something, two weeks later you get a review, you
> > > post the corrected version, you get more reviews 10 days later, etc. So it
> > > can be a drawn-out process. This can be frustrating, but please stick with
> > > it.
> > 
> > Please add:
> > 	"The reason for these measures is to warrant, in our best, that no
> > regressions will be added into the Kernel, keeping it with a high quality,
> > and, yet, allowing to release a new Kernel on every 7-9 weeks."
> > 
> > > We have seen cases where people seem to give up, but that is not our
> > > intention. We welcome new code, but since none of the core developers work
> > > full time on this we are constrained by the time we have available. Just
> > > be aware of this, plan accordingly and don't give up.
> > > 
> > > 
> > > Contacting developers
> > > =====================
> > > 
> > > The linux-media mailinglist is the central place to get into contact with
> > > developers. However, there are also two irc channels: #linuxtv (mostly DVB
> > > related) and #v4l (mostly V4L related). Most developers are based in the
> > > US or in Europe, so take those timezones into account.
> > 
> > I would add there:
> > 
> > 	"If you ask something at the IRC channel, please wait for your
> > answer, as it may take some time for a developer to be able to find a
> > timeslot to answer you".
> > 
> > > Finally, you can often find developers during the three main Linux
> > > conferences relevant to us: the Linux Plumbers Conference, the Embedded
> > > Linux Conference and the Embedded Linux Conference Europe.
> > 
> > I would say (maybe instead) to the Media mini-summits that happen typically
> > together with one major Linux Foundation conference (typically either in
> > Europe or US), like the Linux Plumbers Conference, the Embedded Linux
> > Conference and/or the Embedded Linux Conference Europe.
> > 
> > > Patch tags
> > > ==========
> > > 
> > > When posting patches it is recommended to tag them to help us sort through
> > > them quickly and efficiently.
> > > 
> > > The tags are:
> > > 
> > > [RFC PATCH x/y]: use this for preliminary patches for which you want to
> > > get some early feedback.
> > > 
> > > [REVIEW PATCH x/y]: use this for patches that you consider OK for merging,
> > > but that need to be reviewed.
> > > 
> > > Once your patches have been reviewed/acked you can post either a pull
> > > request ("[GIT PULL]") or use the "[FINAL PATCH x/y]" tag if you don't
> > > have a public git tree.
> > > 
> > > If you post a new version of a patch series, then add 'v1', 'v2', etc. to
> > > the RFC or REVIEW word, e.g.: "[RFCv2 PATCH x/y]".
> > > 
> > > If your patch is for the current rc kernel (so it is a regression or
> > > serious bug fix), then add " FOR v3.x" after the PATCH or PULL keyword.
> > > For example: "[REVIEW PATCH FOR v3.7 x/y]", or "[GIT PULL FOR v3.7]".
> > > 
> > > You can use the option --subject-prefix="REVIEW PATCHv1" with the 'git
> > > send-email' to specify the prefix.
> > > 
> > > Patches without the appropriate tags will be processed manually, which
> > > will take more time and may actually cause them to be dropped altogether.
> > > 
> > > 
> > > 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
> 
> > > - 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, 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.

> > > Whenever you patch changes state you'll get an email informing you about
> > > that.
> >
> > Patchwork has an opt-out way to disable those notifications. While I expect
> > that nobody would opt-out, I think we should mention it, as patchwork is
> > not a spammer: it only sends email only to track the status of a patch, and
> > only after its submission. Also, it offers a way to opt-out of such
> > notifications.
> 


-- 

Cheers,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Laurent Pinchart]

Hi Mauro,

On Tuesday 11 December 2012 08: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.

That's my point, exactly. The text above just sounds a bit too harsh for my 
taste, it might even scare people :-) I of course share your point of view 
that we want developers to understand that they need to work on staging 
drivers and not let them rot,

> > 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.

What about

"If no real effort to get a driver out of staging is noticed, the driver can 
be dropped from the kernel altogether. This policy can be applied over a 
single kernel release period and without any notice, although we will try our 
best to communicate with the driver developers and not to enforce the policy 
too harshly."

> > > > 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.

Maybe we could just add "Follow the CodingStyle guidelines".

> > >   - typedefs should't be used;
> > 
> > CodingStyle, chapter 5.
> 
> 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.

[snip]

> > > > Timeline for code submissions
> > > > =============================
> > > > 
> > > > After a new kernel is released the merge window will be open for about
> > > > two weeks
> > > 
> > > Please add:
> > > 	"for the maintainers to send him the patches they already received
> > 
> > "him" ? I suppose you mean Linus :-)
> 
> Yes. Maybe it can be changed from "send him" to "send upstream".
> 
> > > during the last development cycle, and that went into the linux-next
> > > tree in time for the other maintainers and reviewers to double-check the
> > > entire set of changes for the next Linux version."
> > > 
> > > (yeah, I know you're talking more about it later, but I think this makes
> > > it a little clearer that no submissions will typically be accepted so
> > > late at the development cycle).
> > > 
> > > > During that time Linus will merge any pending work for the next
> > > > kernel. Once that merge window is closed only regression fixes and
> > > > serious bug fixes will be accepted, everything else will be postponed
> > > 
> > > please add:
> > > 	"upstream"
> > 
> > postponed upstream ? What do you mean ?
> 
> This is just a small correction, but maybe it can be a little more verbose.
> 
> What I meant to say is that, after the merge window, the patches will be
> "postponed for upstream merge until the next merge window".
> 
> The original text could give the impression that even at the subsystem
> submaintainers/maintainers tree, the patches will be merged only during
> a merge window.

I got your point and I agree that this needs to be clarified.

> > > > until the next merge window.
> > > > 
> > > > In addition, before anything can be merged (regardless of whether this
> > > > is during the merge window or not) the new code should have been in
> > > > the linux-next tree for about a week at minimum to ensure there are no
> > > > conflicts with work being done in other kernel subsystems.
> > > > 
> > > > Furthermore, before code can be added to linux-next it has to be
> > > > reviewed first. This will take time as well. Adding everything up this
> > > > means that if you want your code to be merged for the next kernel you
> > > > should have it posted to the linux-media mailinglist no later than rc5
> > > > of the current kernel, or it may be too late. In fact, the earlier the
> > > > better since reviews will take time, and if corrections need to be
> > > > made you may have to do several review/submit cycles.
> > > > 
> > > > Remember that the core media developers have a job as well, and so
> > > > won't always have the time to review immediately. A general rule of
> > > > thumb is to post a reminder if a full week has passed without
> > > > receiving any feedback. There is a fair amount of traffic on the
> > > > mailinglist and it wouldn't be the first time that a patch was missed
> > > > by reviewers.
> > > > 
> > > > One consequence of this is that as submitter you can get into the
> > > > situation that you post something, two weeks later you get a review,
> > > > you post the corrected version, you get more reviews 10 days later,
> > > > etc. So it can be a drawn-out process. This can be frustrating, but
> > > > please stick with it.
> > > 
> > > Please add:
> > > 	"The reason for these measures is to warrant, in our best, that no
> > > 
> > > regressions will be added into the Kernel, keeping it with a high
> > > quality, and, yet, allowing to release a new Kernel on every 7-9 weeks."
> > > 
> > > > We have seen cases where people seem to give up, but that is not our
> > > > intention. We welcome new code, but since none of the core developers
> > > > work full time on this we are constrained by the time we have
> > > > available. Just be aware of this, plan accordingly and don't give up.

[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

One of the question that this section tries to answer is who the main contact 
person should be for a given part of the code. I don't want to discourage 
others from reviewing the code of course.

> > > > - 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, 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.

For DaVinci, sure, but not for OMAP2 and OMAP3. Those are separate.

> > > > 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;

I wonder whether this should be an opt-in feature. Otherwise people will by 
default get a mail for every patch they send (assuming patchwork picks up the 
patch correctly, which hopefully is what usually happens :-)).

> 	- allow patch submitters to change the status of their own patches,
> 	  in order to allow them to mark obsoleted/superseeded patches as such;

Should that be web-based, e-mail-based or both ?

> 	- 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.

How do you envision delegation to the sub-maintainers ? Will they have open 
access to patchwork based on a high trust level ?

> > > > Whenever you patch changes state you'll get an email informing you
> > > > about that.
> > > 
> > > Patchwork has an opt-out way to disable those notifications. While I
> > > expect that nobody would opt-out, I think we should mention it, as
> > > patchwork is not a spammer: it only sends email only to track the status
> > > of a patch, and only after its submission. Also, it offers a way to opt-
> > > out of such notifications.

-- 
Regards,

Laurent Pinchart

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Tue, 11 Dec 2012 12:39:06 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 11 December 2012 08: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.
> 
> That's my point, exactly. The text above just sounds a bit too harsh for my 
> taste, it might even scare people :-) I of course share your point of view 
> that we want developers to understand that they need to work on staging 
> drivers and not let them rot,
> 
> > > 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.
> 
> What about
> 
> "If no real effort to get a driver out of staging is noticed, the driver can 
> be dropped from the kernel altogether. This policy can be applied over a 
> single kernel release period and without any notice, although we will try our 
> best to communicate with the driver developers and not to enforce the policy 
> too harshly."

OK.

> > > > > 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.
> 
> Maybe we could just add "Follow the CodingStyle guidelines".

I prefer to keep those two (errno/typedefs) explicitly, as they're the
most common cases. Maybe something like:

- Follow Documentation/CodingStyle guidelines. Two common mistakes that should
be fixed are to declare typedefs or to return positive, driver-specific error
codes on functions, instead of using the standard Linux negative error codes.

> 
> > > >   - typedefs should't be used;
> > > 
> > > CodingStyle, chapter 5.
> > 
> > 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.

[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
> 
> One of the question that this section tries to answer is who the main contact 
> person should be for a given part of the code. I don't want to discourage 
> others from reviewing the code of course.

Yes, but there's no "main contact person" for core changes. For example, VB2
is part of V4L2 core, but Sylwester and Pawel are the main contact people for
it. They won't be that familiar, however, with VB1. Also, in practice, the main
contact point for a given piece of the core can easily be obtained by using
get_maintainer.pl:

$ ./scripts/get_maintainer.pl -f drivers/media/v4l2-core/videobuf2-core.c
Pawel Osciak <pawel@osciak.com> (maintainer:VIDEOBUF2 FRAMEWORK)
Marek Szyprowski <m.szyprowski@samsung.com> (maintainer:VIDEOBUF2 FRAMEWORK)
Kyungmin Park <kyungmin.park@samsung.com> (maintainer:VIDEOBUF2 FRAMEWORK)
Mauro Carvalho Chehab <mchehab@redhat.com> (maintainer:MEDIA INPUT INFRA...)
linux-media@vger.kernel.org (open list:VIDEOBUF2 FRAMEWORK)
linux-kernel@vger.kernel.org (open list)

$ ./scripts/get_maintainer.pl -f drivers/media/v4l2-core/videobuf-dma-sg.c 
Mauro Carvalho Chehab <mchehab@redhat.com> (maintainer:MEDIA INPUT INFRA...,commit_signer:1/2=50%)
Andrew Morton <akpm@linux-foundation.org> (commit_signer:1/2=50%)
Konstantin Khlebnikov <khlebnikov@openvz.org> (commit_signer:1/2=50%)
linux-media@vger.kernel.org (open list:MEDIA INPUT INFRA...)
linux-kernel@vger.kernel.org (open list)

Btw, it makes sense to recommend, at the generic section, the usage of
/scripts/get_maintainer.pl when submitting a patch, in order to get the
proper reviewers.

The main reviewers will naturally pop-up there, as they do their job of
reviewing most of the patches.

> > > > > - 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, 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.
> 
> For DaVinci, sure, but not for OMAP2 and OMAP3. Those are separate.

Well, whatever it is, its better to have it properly documented. The
better is to document those at MAINTAINERS file.

> > > > > 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;
> 
> I wonder whether this should be an opt-in feature. Otherwise people will by 
> default get a mail for every patch they send (assuming patchwork picks up the 
> patch correctly, which hopefully is what usually happens :-)).

Patchwork uses a cronjob to submit the emails. So, if we use the same
cronjob for new patches, if one send a big patch series, he will likely
receive just one confirmation email for the entire series (or two, in the worse
case).

I don't think a separate opt-in would be needed for it. IMHO, if one cared
to work on a patch series, he wants to be sure that his patch won't be missed
in the process. That's basically why patchwork has notification support. Well,
we've seen several times patches (and even git pull requests) that got lost
because they don't match patchwork acceptance criteria. The soonest a patch
loss is noticed, the better.

> 
> > 	- allow patch submitters to change the status of their own patches,
> > 	  in order to allow them to mark obsoleted/superseeded patches as such;
> 
> Should that be web-based, e-mail-based or both ?

Good question. Most people will likely expect a web-based way of doing it.

An e-mail-based one can also be very interesting, as a new patch may have a
meta-tag to mark a previous patch as obsoleted/superseded.
> 
> > 	- 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.
> 
> How do you envision delegation to the sub-maintainers ? Will they have open 
> access to patchwork based on a high trust level ?

While I have high trust level at the sub-maintainers, it concerns me 
a little bit is that everyone with "root" access to the project can 
mangle the others' work. One thing that may help would be to have a
"patch owner" concept there, based on the patch series lsdiff, e. g. if the
driver only touches at, for example, a radio device, patchwork could
tag the patch as owned by the radio sub-maintainer. So, he gets notified
about the changes there, even if the patch is delegated to someone's else
(like the driver's maintainer).

We may find harder to handle patch series that non-trivially touches at both
the core and at some driver.

Also, patchwork currently doesn't bundle a patch series together. It doesn't
make much sense to have two sub-maintainers handling the same patch series.
So, if a patch series touches on two areas, either one of the maintainers
(or me) should handle the entire patchset.

Hmm... there is actually an exception to the above rule... Kernel janitors
tend to group independent patches altogether. So, if we add some intelligence
for patchwork to automatically delegate (or give sub-maintainer's ownership
to a patch series), the sub-maintainer has to be capable of reassigning it.

> 
> > > > > Whenever you patch changes state you'll get an email informing you
> > > > > about that.
> > > > 
> > > > Patchwork has an opt-out way to disable those notifications. While I
> > > > expect that nobody would opt-out, I think we should mention it, as
> > > > patchwork is not a spammer: it only sends email only to track the status
> > > > of a patch, and only after its submission. Also, it offers a way to opt-
> > > > out of such notifications.
> 


-- 

Cheers,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Hans Verkuil]

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 <laurent.pinchart@ideasonboard.com> escreveu:
> 
> > Hi,
> > 
> > On Monday 10 December 2012 16:33:13 Mauro Carvalho Chehab wrote:
> > > Em Mon, 10 Dec 2012 14:07:09 +0100 Hans Verkuil escreveu:
> > > > Hi all,
> > > > 
> > > > As discussed in Barcelona I would write a text describing requirements for
> > > > new drivers and what to expect when submitting patches to linux-media.
> > > > 
> > > > This is a first rough draft and nothing is fixed yet.
> > > > 
> > > > I have a few open questions:
> > > > 
> > > > 1) Where to put it?
> > > 
> > > Maybe at media-build.git.
> > 
> > Not everybody uses (or is even aware of) media-build. The goal here is to make 
> > sure that new driver developers will run into the guidelines before then spend 
> > months writing code that can't be upstreamed. Documentation/ thus looks like a 
> > good place to me.

I agree with Laurent here. The media_build repo isn't widely used, certainly not
in the embedded world. I don't think we should stick documentation there.

> > It might be a good idea to add a reference to the guidelines in the API 
> > DocBook documentation, regardless of where the guidelines end up being stored. 
> > If a developer reads a single document only it will probably be the API 
> > reference.
> 
> Agreed.
> > 
> > > I'm thinking on putting there, under devel_contrib, the main scripts I use
> > > here to handle patches.
> > > 
> > > /me needs some time to sanitize them and add there.
> > > 
> > > > One thing I would propose that we improve is to move the dvb and
> > > > video4linux directories in Documentation/ to Documentation/media to
> > > > correctly reflect the drivers/media structure. If we do that, then we can
> > > > put this document in Documentation/media/SubmittingMediaPatches.
> > > 
> > > Hmm... I don't see any other subsystems having their own document for that.
> > > We may need to discuss it upstream before doing that, and be prepared
> > > to answer why we thing sub-systems would need their own rules there.
> > 
> > Things like requiring the use of the control framework is obviously V4L-
> > specific, we can't add that to Documentation/SubmittingPatches :-)
> > 
> > > In any case, I think that the better is to store it at media-build.git tree,
> > > and later open such discussions upstream, if we think it is valuable
> > > enough.
> > >
> > > > Alternatively, this is something we can document in our wiki.
> > > > 
> > > > 2) Are there DVB requirements as well for new drivers? We discussed a list
> > > > of V4L2 requirements in Barcelona, but I wonder if there is a similar
> > > > list that can be made for DVB drivers. Input on that will be welcome.
> > > 
> > > See below.
> > > 
> > > > 3) This document describes the situation we will have when the
> > > > submaintainers take their place early next year. So please check if I got
> > > > that part right.
> > > > 
> > > > 4) In Barcelona we discussed 'tags' for patches to help organize them.
> > > > I've made a proposal for those in this document. Feedback is very welcome.
> > > > 
> > > > 5) As discussed in Barcelona there will be git tree maintainers for
> > > > specific platforms, but we didn't really go into detail who would be
> > > > responsible for which platform. If you want to maintain a particular
> > > > platform, then please let me know.
> > > > 
> > > > 6) The patchwork section is very short at the moment. It should be
> > > > extended
> > > > when patchwork gets support to recognize the various tags.
> > > > 
> > > > 7) Anything else that should be discussed here?
> > > > 
> > > > Again, remember that this is a rough draft only, so be gentle with me :-)
> > > > 
> > > > Regards,
> > > > 
> > > > 	Hans
> > > > 
> > > > --------------------------- cut here -------------------------------
> > > > 
> > > > General Information
> > > > ===================
> > > > 
> > > > For general information on how to submit patches see:
> > > > 
> > > > http://linuxtv.org/wiki/index.php/Developer_Section
> > > > 
> > > > In particular the section 'Submitting Your Work'.
> > > > 
> > > > This document goes into more detail regarding media specific requirements
> > > > when submitting patches and what the patch flow looks like in this
> > > > subsystem.
> > >
> > > I think we should add a paragraph here saying that rules may have
> > > exceptions, when there's a clear reason why a certain submission should need
> > > a different criteria.
> > 
> > I agree. For instance the uvcvideo driver doesn't use the control framework, 
> > and has good reasons not to. This must be the exception rather than the rule, 
> > but we might have more than one exception.
> 
> Yes.
> 
> > 
> > > Also, IMHO, we should add a notice that this list is not exhaustive, and may
> > > be changed, keeping it for at least one or two Kernel cycles, while it
> > > doesn't get proofed/matured, as I'm sure we'll forget things.
> > > 
> > > > 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.

> 
> 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?

> > > 
> > > > How to deal with checkpatch.pl?
> > > > ===============================
> > > > 
> > > > First of all, the requirement to comply to the kernel coding style is
> > > > there for a reason. Sometimes people feel that it is a pointless
> > > > exercise: after all, code is code, right? Why would just changing some
> > > > spacing improve it?
> > > > 
> > > > But the coding style is not there to help you (at least, not directly), it
> > > > is there to help those who have to review and/or maintain your code as it
> > > > takes a lot of time to review code or try to figure out how someone
> > > > else's code works. By at least ensuring that the coding style is
> > > > consistent with other code we can concentrate on what humans to best:
> > > > pattern matching. Ever read a book or article that did not use the
> > > > correct spelling, grammar and/or punctuation rules? Did you notice how
> > > > your brain 'stumbles' whenever it encounters such mistakes? It makes the
> > > > text harder to understand and slower to read. The same happens with code
> > > > that does not comply to the conventions of the project and it is the
> > > > reason why most large projects, both open source and proprietary, have a
> > > > coding style.
> > > > 
> > > > However, when interpreting the checkpatch output it is good to remember
> > > > that it is just an automated tool and there are cases where what
> > > > checkpatch recommends does not actually results in the best readable
> > > > code. This is particularly true for the line length warnings. A warning
> > > > that a line is 82 characters long can probably be ignored, since breaking
> > > > up such a line will usually make the code harder to understand. A warning
> > > > that a line is 101 characters long definitely needs attention, since
> > > > that's an indication that the line is really too long.
> > > I wouldn't say that, as people will likely read that an 82 chars long line
> > > should be ignored ;)
> > > 
> > > IMHO, the better is to give some examples for this specific warning (for
> > > example: function calls and function declarations with more than 80 chars
> > > should likely be broken, even if they have 81 columns; lines with an string
> > > at the end should likely not be broken, even if it has more than 80
> > > columns, etc).
> > 
> > Agreed.
> > 
> > > > The guideline here is to check such warnings, but use common sense whether
> > > > or not to fix them.
> > > > 
> > > > Please do run checkpatch before posting any code to the mailinglist. Code
> > > > that clearly violates the kernel coding style will be rejected and you
> > > > will be asked to repost after fixing the style. We are not going to waste
> > > > time trying to review code that uses a non-standard coding style, our
> > > > time is too limited for that.
> > > > 
> > > > The sole exception are staging drivers as the only rule there is that it
> > > > compiles.
> > > > 
> > > > 
> > > > Timeline for code submissions
> > > > =============================
> > > > 
> > > > After a new kernel is released the merge window will be open for about two
> > > > weeks
> > > 
> > > Please add:
> > > 	"for the maintainers to send him the patches they already received
> > 
> > "him" ? I suppose you mean Linus :-)
> 
> Yes. Maybe it can be changed from "send him" to "send upstream".
> > 
> > > during the last development cycle, and that went into the linux-next tree
> > > in time for the other maintainers and reviewers to double-check the entire
> > > set of changes for the next Linux version."
> > > 
> > > (yeah, I know you're talking more about it later, but I think this makes it
> > > a little clearer that no submissions will typically be accepted so late at
> > > the development cycle).
> > > 
> > > > During that time Linus will merge any pending work for the next kernel.
> > > > Once that merge window is closed only regression fixes and serious bug
> > > > fixes will be accepted, everything else will be postponed
> > > 
> > > please add:
> > > 	"upstream"
> > 
> > postponed upstream ? What do you mean ?
> 
> This is just a small correction, but maybe it can be a little more verbose.
> 
> What I meant to say is that, after the merge window, the patches will be 
> "postponed for upstream merge until the next merge window".
> 
> The original text could give the impression that even at the subsystem
> submaintainers/maintainers tree, the patches will be merged only during
> a merge window.
> 
> > 
> > > > until the next merge window.
> > > > 
> > > > In addition, before anything can be merged (regardless of whether this is
> > > > during the merge window or not) the new code should have been in the
> > > > linux-next tree for about a week at minimum to ensure there are no
> > > > conflicts with work being done in other kernel subsystems.
> > > > 
> > > > Furthermore, before code can be added to linux-next it has to be reviewed
> > > > first.  This will take time as well. Adding everything up this means that
> > > > if you want your code to be merged for the next kernel you should have it
> > > > posted to the linux-media mailinglist no later than rc5 of the current
> > > > kernel, or it may be too late. In fact, the earlier the better since
> > > > reviews will take time, and if corrections need to be made you may have
> > > > to do several review/submit cycles.
> > > > 
> > > > Remember that the core media developers have a job as well, and so won't
> > > > always have the time to review immediately. A general rule of thumb is to
> > > > post a reminder if a full week has passed without receiving any feedback.
> > > > There is a fair amount of traffic on the mailinglist and it wouldn't be
> > > > the first time that a patch was missed by reviewers.
> > > > 
> > > > One consequence of this is that as submitter you can get into the
> > > > situation that you post something, two weeks later you get a review, you
> > > > post the corrected version, you get more reviews 10 days later, etc. So it
> > > > can be a drawn-out process. This can be frustrating, but please stick with
> > > > it.
> > > 
> > > Please add:
> > > 	"The reason for these measures is to warrant, in our best, that no
> > > regressions will be added into the Kernel, keeping it with a high quality,
> > > and, yet, allowing to release a new Kernel on every 7-9 weeks."
> > > 
> > > > We have seen cases where people seem to give up, but that is not our
> > > > intention. We welcome new code, but since none of the core developers work
> > > > full time on this we are constrained by the time we have available. Just
> > > > be aware of this, plan accordingly and don't give up.
> > > > 
> > > > 
> > > > Contacting developers
> > > > =====================
> > > > 
> > > > The linux-media mailinglist is the central place to get into contact with
> > > > developers. However, there are also two irc channels: #linuxtv (mostly DVB
> > > > related) and #v4l (mostly V4L related). Most developers are based in the
> > > > US or in Europe, so take those timezones into account.
> > > 
> > > I would add there:
> > > 
> > > 	"If you ask something at the IRC channel, please wait for your
> > > answer, as it may take some time for a developer to be able to find a
> > > timeslot to answer you".
> > > 
> > > > Finally, you can often find developers during the three main Linux
> > > > conferences relevant to us: the Linux Plumbers Conference, the Embedded
> > > > Linux Conference and the Embedded Linux Conference Europe.
> > > 
> > > I would say (maybe instead) to the Media mini-summits that happen typically
> > > together with one major Linux Foundation conference (typically either in
> > > Europe or US), like the Linux Plumbers Conference, the Embedded Linux
> > > Conference and/or the Embedded Linux Conference Europe.
> > > 
> > > > Patch tags
> > > > ==========
> > > > 
> > > > When posting patches it is recommended to tag them to help us sort through
> > > > them quickly and efficiently.
> > > > 
> > > > The tags are:
> > > > 
> > > > [RFC PATCH x/y]: use this for preliminary patches for which you want to
> > > > get some early feedback.
> > > > 
> > > > [REVIEW PATCH x/y]: use this for patches that you consider OK for merging,
> > > > but that need to be reviewed.
> > > > 
> > > > Once your patches have been reviewed/acked you can post either a pull
> > > > request ("[GIT PULL]") or use the "[FINAL PATCH x/y]" tag if you don't
> > > > have a public git tree.
> > > > 
> > > > If you post a new version of a patch series, then add 'v1', 'v2', etc. to
> > > > the RFC or REVIEW word, e.g.: "[RFCv2 PATCH x/y]".
> > > > 
> > > > If your patch is for the current rc kernel (so it is a regression or
> > > > serious bug fix), then add " FOR v3.x" after the PATCH or PULL keyword.
> > > > For example: "[REVIEW PATCH FOR v3.7 x/y]", or "[GIT PULL FOR v3.7]".
> > > > 
> > > > You can use the option --subject-prefix="REVIEW PATCHv1" with the 'git
> > > > send-email' to specify the prefix.
> > > > 
> > > > Patches without the appropriate tags will be processed manually, which
> > > > will take more time and may actually cause them to be dropped altogether.
> > > > 
> > > > 
> > > > 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.

> > 
> > > > - 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.

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?

> , 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.

Regards,

	Hans

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Laurent Pinchart]

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

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Tue, 11 Dec 2012 13:20:32 +0100
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

[snip]
> > - 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.

This can be easily detected: if the patch author didn't opt-out, he, 
and the others who signed the v1 patch will receive a notification when it
gets any status change.

Also, when the patch gets merged on my tree, the author will receive 
another email.

So, a fake submission can easily be detected.

Cheers,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Tue, 11 Dec 2012 12:50:21 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> > > 
> > > >   - 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.

It seems you detected a potential patch for CodingStyle ;)

Anyway, this one of the exceptions that would be accepted. Very unlikely
to have it at driver level anyway, as function pointers is more used at
the core handlers.

> > > > > 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?

The DVB internal API already enforces it. There's only one thing
that could be miss-filled due to DVBv3. So, please add:

- dvb_frontend_ops should specify the delivery system, instead of
  specifying the frontend type via dvb_frontend_ops info.type field.

You can also add:

- DVB frontends should not implement dummy function handlers; if the
  function is not implemented, the DVB core should be handled it
  properly.

...

> > 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 think you'll likely be surprised if you play a little bit with
get_maintainer.pl under the DVB tree. For example:

$ ./scripts/get_maintainer.pl -f --git-blame drivers/media/dvb-core/dvb_ca_en50221.c
Mauro Carvalho Chehab <mchehab@redhat.com> (maintainer:MEDIA INPUT INFRA...,commit_signer:2/2=100%,commits:18/24=75%)
Andrew de Quincey <adq_dvb@lidskialf.net> (authored lines:41/172=24%,commits:3/24=12%)
Matthias Dahl <devel@mortal-soul.de> (authored lines:21/172=12%)
Johannes Stezenbach <js@linuxtv.org> (authored lines:19/172=11%,commits:3/24=12%)
Harvey Harrison <harvey.harrison@gmail.com> (authored lines:18/172=10%)
Robert Schedel <r.schedel@yahoo.de> (authored lines:16/172=9%)
Andrew Morton <akpm@osdl.org> (commits:7/24=29%)
Oliver Endriss <o.endriss@gmx.de> (commits:6/24=25%)
linux-media@vger.kernel.org (open list:MEDIA INPUT INFRA...)
linux-kernel@vger.kernel.org (open list)

So, your ack for a patch at dvb_ca_en50221.c is as good as Mike's one,
as none of you ever touched there, according with git blame.

> > > > > - 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.
> 
> I guess we have just two 'SoC-family' maintainers: Prabhakar for davinci, and
> Sylwester for s5p/exynos.

Ok.

> 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?

For drivers, I agree. For core changes through[1], I think it should be
submitted separated.

[1] Of course, very trivial core changes, like just adding a new FOURCC 
proprietary format could just follow through the driver's tree.
> 
> > , 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.

IMO, It should not be that hard to do it automatically, when the reference-ID 
of the original patch is kept. If this is not the case, it is better to not
deprecate the v1, and let someone manually check it.

Regards,
Mauro

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Hans Verkuil]

On Mon 10 December 2012 14:07:09 Hans Verkuil wrote:
> Hi all,
> 
> As discussed in Barcelona I would write a text describing requirements for new
> drivers and what to expect when submitting patches to linux-media.
> 
> This is a first rough draft and nothing is fixed yet.
> 
> I have a few open questions:
> 
> 1) Where to put it? One thing I would propose that we improve is to move the
> dvb and video4linux directories in Documentation/ to Documentation/media to
> correctly reflect the drivers/media structure. If we do that, then we can put
> this document in Documentation/media/SubmittingMediaPatches.
> 
> Alternatively, this is something we can document in our wiki.
> 
> 2) Are there DVB requirements as well for new drivers? We discussed a list of
> V4L2 requirements in Barcelona, but I wonder if there is a similar list that
> can be made for DVB drivers. Input on that will be welcome.
> 
> 3) This document describes the situation we will have when the submaintainers
> take their place early next year. So please check if I got that part right.
> 
> 4) In Barcelona we discussed 'tags' for patches to help organize them. I've
> made a proposal for those in this document. Feedback is very welcome.
> 
> 5) As discussed in Barcelona there will be git tree maintainers for specific
> platforms, but we didn't really go into detail who would be responsible for
> which platform. If you want to maintain a particular platform, then please
> let me know.
> 
> 6) The patchwork section is very short at the moment. It should be extended
> when patchwork gets support to recognize the various tags.
> 
> 7) Anything else that should be discussed here?

How to submit patches for a stable kernel.

I can never remember, and I'm sure I'm not the only one :-)

Regards,

	Hans

Re: RFC: First draft of guidelines for submitting patches to linux-media

[Mauro Carvalho Chehab]

Em Tue, 11 Dec 2012 14:15:31 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Mon 10 December 2012 14:07:09 Hans Verkuil wrote:
> > Hi all,
> > 
> > As discussed in Barcelona I would write a text describing requirements for new
> > drivers and what to expect when submitting patches to linux-media.
> > 
> > This is a first rough draft and nothing is fixed yet.
> > 
> > I have a few open questions:
> > 
> > 1) Where to put it? One thing I would propose that we improve is to move the
> > dvb and video4linux directories in Documentation/ to Documentation/media to
> > correctly reflect the drivers/media structure. If we do that, then we can put
> > this document in Documentation/media/SubmittingMediaPatches.
> > 
> > Alternatively, this is something we can document in our wiki.
> > 
> > 2) Are there DVB requirements as well for new drivers? We discussed a list of
> > V4L2 requirements in Barcelona, but I wonder if there is a similar list that
> > can be made for DVB drivers. Input on that will be welcome.
> > 
> > 3) This document describes the situation we will have when the submaintainers
> > take their place early next year. So please check if I got that part right.
> > 
> > 4) In Barcelona we discussed 'tags' for patches to help organize them. I've
> > made a proposal for those in this document. Feedback is very welcome.
> > 
> > 5) As discussed in Barcelona there will be git tree maintainers for specific
> > platforms, but we didn't really go into detail who would be responsible for
> > which platform. If you want to maintain a particular platform, then please
> > let me know.
> > 
> > 6) The patchwork section is very short at the moment. It should be extended
> > when patchwork gets support to recognize the various tags.
> > 
> > 7) Anything else that should be discussed here?
> 
> How to submit patches for a stable kernel.
> 
> I can never remember, and I'm sure I'm not the only one :-)

The standard way is to add this tag:

	Cc: stable@vger.kernel.org

eventually with a comment saying to what versions it should be applied, like:

	Cc: stable@vger.kernel.org	# for v3.5 and upper

If it is noticed later the need for it on stable, or a backport is needed,
the patch author should send the patch to stable@vger.kernel.org, c/c the ML,
preferably pointing to the upstream commit ID. The patch has to be merged
upstream before being merged at stable.

-- 

Cheers,
Mauro