Em Wed, 18 Sep 2019 15:36:20 +0300 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> escreveu: > Hi Mauro, > > On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote: > > Document the basic policies of the media subsystem profile. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx> > > --- > > > > That's basically a modified version of: > > https://patchwork.linuxtv.org/patch/52999/ > > > > Applied to the new template > > > > Documentation/media/index.rst | 1 + > > .../media/maintainer-entry-profile.rst | 140 ++++++++++++++++++ > > MAINTAINERS | 1 + > > 3 files changed, 142 insertions(+) > > create mode 100644 Documentation/media/maintainer-entry-profile.rst > > > > diff --git a/Documentation/media/index.rst b/Documentation/media/index.rst > > index 0301c25ff887..ac94685b822a 100644 > > --- a/Documentation/media/index.rst > > +++ b/Documentation/media/index.rst > > @@ -12,6 +12,7 @@ Linux Media Subsystem Documentation > > .. toctree:: > > :maxdepth: 2 > > > > + maintainer-entry-profile > > media_uapi > > media_kapi > > dvb-drivers/index > > diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst > > new file mode 100644 > > index 000000000000..81d3b0f9c36a > > --- /dev/null > > +++ b/Documentation/media/maintainer-entry-profile.rst > > @@ -0,0 +1,140 @@ > > +Media Subsystem Profile > > +======================= > > + > > +Overview > > +-------- > > + > > +The media subsystem cover support for a variety of devices: stream > > s/cover/covers/ > > > +capture, analog and digital TV, cameras, remote controllers, HDMI CEC > > +and media pipeline control. > > + > > +It covers, mainly, the contents of those directories: > > + > > + - drivers/media > > + - drivers/staging/media > > + - Documentation/media > > + - include/media > > + > > +Both media userspace and Kernel APIs are documented and should be kept in > > +sync with the API changes. It means that all patches that add new > > +features to the subsystem should also bring changes to the corresponding > > +API files. > > + > > +Also, patches for device drivers that changes the Open Firmware/Device > > +Tree bindings should be reviewed by the Device Tree maintainers. > > You may want to make it clear that this covers bindings only, not driver > code that parses the DT. I would just remove "for device drivers", as > the rule should also apply to DT bindings documentation that is not > driver-specific. > > > +Due to the size and wide scope of the media subsystem, media's > > +maintainership model is to have sub-maintainers that have a broad > > +knowledge of an specific aspect of the subsystem. It is a > > +sub-maintainers task to review the patches, providing feedback to users > > s/sub-maintainers/sub-maintainer's/ > > > +if the patches are following the subsystem rules and are properly using > > +the media internal and external APIs. > > + > > +Patches for the media subsystem should be sent to the media mailing list > > +at linux-media@xxxxxxxxxxxxxxx as plain text only e-mail. Emails with > > +HTML will be automatically rejected by the mail server. There's no need > > +to copy the maintainer or sub-maintainer(s). > > There's too much traffic on mailing lists for me to follow everything, I > much prefer being CC'ed on patches. Well, by using patchwork, the best is to take a look on it at least for the patches that you're interested. You could script something using pwclient in order to make it easier. Anyway, not sure if the other sub-maintainers see the same way. From my side, I prefer not to be c/c, as this is just more noise, as I just rely on patchwork for media patches. What about changing this to: Patches for the media subsystem should be sent to the media mailing list at linux-media@xxxxxxxxxxxxxxx as plain text only e-mail. Emails with HTML will be automatically rejected by the mail server. It could be wise to also copy the sub-maintainer(s). > > > +Media's workflow is heavily based on Patchwork, meaning that, once a patch > > +is submitted, it should appear at: > > + > > + - https://patchwork.linuxtv.org/project/linux-media/list/ > > + > > +If it doesn't automatically appear there after a few minutes, then > > +probably something got wrong on your submission. Please check if the > > +email is in plain text only and if your emailer is not mangling with > > +whitespaces before complaining or submit it again. > > + > > +Sub-maintainers > > ++++++++++++++++ > > + > > +At the media subsystem, we have a group of senior developers that are > > How about "experienced" instead of "senior" ? Quality doesn't always > come with age, neither for people nor wine :-) > > > +responsible for doing the code reviews at the drivers (called > > +sub-maintainers), and another senior developer responsible for the > > +subsystem as a hole. For core changes, whenever possible, multiple > > +media (sub-)maintainers do the review. > > + > > +The sub-maintainers work on specific areas of the subsystem, as > > +described below: > > + > > +Digital TV: > > + Sean Young <sean@xxxxxxxx> > > + > > +HDMI CEC: > > + Hans Verkuil <hverkuil@xxxxxxxxx> > > + > > +Media controller drivers: > > + Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > + > > +Remote Controllers: > > + Sean Young <sean@xxxxxxxx> > > + > > +Sensor drivers: > > + Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > + > > +V4L2 drivers: > > + Hans Verkuil <hverkuil@xxxxxxxxx> > > + > > +Submit Checklist Addendum > > +------------------------- > > + > > +There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/ > > +that should be used in order to check if the drivers are properly > > +implementing the media APIs. > > + > > +Those tests need to be passed before the patches go upstream. > > s/need to be passed/need to pass/ > > > + > > +Also, please notice that we build the Kernel with:: > > + > > + make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script > > + > > +Where the check script is:: > > + > > + #!/bin/bash > > + /devel/smatch/smatch -p=kernel $@ >&2 > > + /devel/sparse/sparse $@ >&2 > > + > > +Be sure to not introduce new warnings on your patches. > > While static analysers deliver value, I fear this is a high barrier to > entry for new contributors. Will change this to: Be sure to not introduce new warnings on your patches without a very good reason. Especially for new contributors, I really expect patches to not introduce new warnings, as it is usually a lot more painful to fix them later than to ask devs to do things right at the first place. > > > + > > +Key Cycle Dates > > +--------------- > > + > > +New submissions can be sent at any time, but if they intend to hit the > > +next merge window they should be sent before -rc5, and ideally stabilized > > +in the linux-media branch by -rc6. > > + > > +Coding Style Addendum > > +--------------------- > > + > > +Media development uses checkpatch on strict mode to verify the code style, e. g.:: > > + > > + $ ./script/checkpatch.pl --strict > > But we still accept some warnings. I think this should be documented. True, but not sure if we should enter into too specific details here. What about adding something like: Please notice that the goal here is to improve code readability. On a few cases, checkpatch may actually point to something that would look worse. So, you should use good send sense here, being prepared to justify any coding style decision. Please also notice that, on some cases, when you fix one issue, you may receive warnings about lines longer than 80 columns. It is fine to have longer lines if this means that other warnings will be fixed by that. Yet, if you're having more than 80 columns on a line, please consider simplifying the code - if too idented - or to use shorter names for variables. > > > + > > +Review Cadence > > +-------------- > > + > > +Provided that your patch is at https://patchwork.linuxtv.org, it should > > +be sooner or later handled, so you don't need to re-submit a patch. > > + > > +Except for bug fixes, we don't usually add new patches to the development > > +tree between -rc6 and the next -rc1. > > + > > +Please notice that the media subsystem is a high traffic one, so it > > +could take a while for us to be able to review your patches. Feel free > > +to ping if you don't get a feedback on a couple of weeks or to ask > > s/on a/in a/ > > > +other developers to publicly add Reviewed-by and, more importantly, > > +Tested-by tags. > > + > > +Please notice that we expect a detailed description for Tested-by, > > s/notice/note/ > > > +identifying what boards were used at the test and what it was tested. > > + > > +Style Cleanup Patches > > +--------------------- > > + > > +Style-cleanups are welcome when they come together with other changes > > +at the files where the style changes will affect. > > + > > +We may accept pure standalone style-cleanups, but they should be grouped > > +per directory. So, for example, if you're doing a cleanup at drivers > > +under drivers/media, please send a single patch for all drivers under > > +drivers/media/pci, another one for drivers/media/usb and so on. > > How about also stating that if the cleanup is low volume, a single patch > for the whole subsystem is preferred ? I think that should actually be > the rule, with a split to ease review only when the patch would be too > big. > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 7c62b45201d7..e7f44ec05a42 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -10077,6 +10077,7 @@ W: https://linuxtv.org > > Q: http://patchwork.kernel.org/project/linux-media/list/ > > T: git git://linuxtv.org/media_tree.git > > S: Maintained > > +P: Documentation/media/maintainer-entry-profile.rst > > F: Documentation/devicetree/bindings/media/ > > F: Documentation/media/ > > F: drivers/media/ > I'm enclosing the diff against the past version. I'll send a final version once the profiles patchset arrives upstream. Thanks, Mauro diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst index 81d3b0f9c36a..68d642abe2c1 100644 --- a/Documentation/media/maintainer-entry-profile.rst +++ b/Documentation/media/maintainer-entry-profile.rst @@ -4,7 +4,7 @@ Media Subsystem Profile Overview -------- -The media subsystem cover support for a variety of devices: stream +The media subsystem covers support for a variety of devices: stream capture, analog and digital TV, cameras, remote controllers, HDMI CEC and media pipeline control. @@ -20,20 +20,20 @@ sync with the API changes. It means that all patches that add new features to the subsystem should also bring changes to the corresponding API files. -Also, patches for device drivers that changes the Open Firmware/Device -Tree bindings should be reviewed by the Device Tree maintainers. +Also, patches that changes the Open Firmware/Device Tree bindings should +also be reviewed by the Device Tree maintainers. Due to the size and wide scope of the media subsystem, media's maintainership model is to have sub-maintainers that have a broad knowledge of an specific aspect of the subsystem. It is a -sub-maintainers task to review the patches, providing feedback to users +sub-maintainer's task to review the patches, providing feedback to users if the patches are following the subsystem rules and are properly using the media internal and external APIs. Patches for the media subsystem should be sent to the media mailing list at linux-media@xxxxxxxxxxxxxxx as plain text only e-mail. Emails with -HTML will be automatically rejected by the mail server. There's no need -to copy the maintainer or sub-maintainer(s). +HTML will be automatically rejected by the mail server. It could be wise +to also copy the sub-maintainer(s). Media's workflow is heavily based on Patchwork, meaning that, once a patch is submitted, it should appear at: @@ -48,8 +48,8 @@ whitespaces before complaining or submit it again. Sub-maintainers +++++++++++++++ -At the media subsystem, we have a group of senior developers that are -responsible for doing the code reviews at the drivers (called +At the media subsystem, we have a group of experienced developers that +are responsible for doing the code reviews at the drivers (called sub-maintainers), and another senior developer responsible for the subsystem as a hole. For core changes, whenever possible, multiple media (sub-)maintainers do the review. @@ -82,7 +82,7 @@ There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/ that should be used in order to check if the drivers are properly implementing the media APIs. -Those tests need to be passed before the patches go upstream. +Those tests need to pass before the patches go upstream. Also, please notice that we build the Kernel with:: @@ -94,7 +94,8 @@ Where the check script is:: /devel/smatch/smatch -p=kernel $@ >&2 /devel/sparse/sparse $@ >&2 -Be sure to not introduce new warnings on your patches. +Be sure to not introduce new warnings on your patches without a +very good reason. Key Cycle Dates --------------- @@ -110,6 +111,20 @@ Media development uses checkpatch on strict mode to verify the code style, e. g. $ ./script/checkpatch.pl --strict +Please notice that the goal here is to improve code readability. On a few +cases, checkpatch may actually point to something that would look worse. + +So, you should use good send sense here, being prepared to justify any +coding style decision. + +Please also notice that, on some cases, when you fix one issue, you may +receive warnings about lines longer than 80 columns. It is fine to have +longer lines if this means that other warnings will be fixed by that. + +Yet, if you're having more than 80 columns on a line, please consider +simplifying the code - if too idented - or to use shorter names for +variables. + Review Cadence -------------- @@ -121,11 +136,11 @@ tree between -rc6 and the next -rc1. Please notice that the media subsystem is a high traffic one, so it could take a while for us to be able to review your patches. Feel free -to ping if you don't get a feedback on a couple of weeks or to ask +to ping if you don't get a feedback in a couple of weeks or to ask other developers to publicly add Reviewed-by and, more importantly, Tested-by tags. -Please notice that we expect a detailed description for Tested-by, +Please note that we expect a detailed description for Tested-by, identifying what boards were used at the test and what it was tested. Style Cleanup Patches @@ -134,7 +149,9 @@ Style Cleanup Patches Style-cleanups are welcome when they come together with other changes at the files where the style changes will affect. -We may accept pure standalone style-cleanups, but they should be grouped -per directory. So, for example, if you're doing a cleanup at drivers -under drivers/media, please send a single patch for all drivers under -drivers/media/pci, another one for drivers/media/usb and so on. +We may accept pure standalone style-cleanups, but they should ideally +be one patch for the hole subsystem (if the cleanup is low volume), +or at least be grouped per directory. So, for example, if you're doing +big cleanup changes at drivers under drivers/media, please send a single +patch for all drivers under drivers/media/pci, another one for +drivers/media/usb and so on.
