media: Document coding style requirements

  There are a few additional coding style conventions in place in
the media subsystem. If they do not get documented, it's hard to enforce
them during review as well as it is hard for developers to follow them
without having previously contributed to the subsystem.

Add them to the subsystem profile documentation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---

All points are up for discussion ofc.

But the idea is to get to have more requirement defined, as otherwise
it's very hard to enforce them during review.

Thanks
   j

---
 .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

--
2.33.0
  

Hi Jacopo,

Thanks for the patch.

On Wed, Oct 13, 2021 at 11:20:05AM +0200, Jacopo Mondi wrote:
> There are a few additional coding style conventions in place in
> the media subsystem. If they do not get documented, it's hard to enforce
> them during review as well as it is hard for developers to follow them
> without having previously contributed to the subsystem.
> 
> Add them to the subsystem profile documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 
> All points are up for discussion ofc.
> 
> But the idea is to get to have more requirement defined, as otherwise
> it's very hard to enforce them during review.

Thanks for the patch.

Aren't these all common and/or preferred practices outside the media tree
as well? I suppose not each one of these is universally enforced though.

The coding style guide is lacking documentation on such things though.

> 
> Thanks
>    j
> 
> ---
>  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> index eb1cdfd280ba..9c376f843e1c 100644
> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
>      - when they avoid a line to end with an open parenthesis or an open
>        bracket.
> 
> +There are a few additional requirements which are not enforced by tooling
> +but mostly during the review process:
> +
> +    - C++ style comments are not allowed, if not for SPDX headers;
> +    - hexadecimal values should be spelled using lowercase letters;
> +    - one structure/enum member declaration per line;
> +    - one variable declaration per line;
> +    - prefer variable declaration order in reverse-x-mas-tree over
> +      initialization at variable declare time;
> +
> +      As an example, the following style is preferred::
> +
> +         struct priv_struct *priv = container_of(....)
> +         struct foo_struct *foo = priv->foo;
> +         int b;
> +
> +         b = a_very_long_operation_name(foo, s->bar)
> +
> +      over the following one::
> +
> +         struct priv_struct *priv = container_of(....)
> +         struct foo_struct *foo = priv->foo;
> +         int b = a_very_long_operation_name(foo, s->bar)

I wouldn't say this is required or even preferred if you have a dependency
between the variables.

Rather I'd say the latter is undesirable if a_very_long_operation_name()
can fail. But that's a bit out of scope now.

> +
>  Key Cycle Dates
>  ---------------
  

Hi Sakari,

On Tue, Oct 19, 2021 at 12:24:41PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the patch.
>
> On Wed, Oct 13, 2021 at 11:20:05AM +0200, Jacopo Mondi wrote:
> > There are a few additional coding style conventions in place in
> > the media subsystem. If they do not get documented, it's hard to enforce
> > them during review as well as it is hard for developers to follow them
> > without having previously contributed to the subsystem.
> >
> > Add them to the subsystem profile documentation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >
> > All points are up for discussion ofc.
> >
> > But the idea is to get to have more requirement defined, as otherwise
> > it's very hard to enforce them during review.
>
> Thanks for the patch.
>
> Aren't these all common and/or preferred practices outside the media tree
> as well? I suppose not each one of these is universally enforced though.

Apparently not :)

Particularly, after the lifting of the 80-cols stringent limit, each
subsystem has its own preferences on that regard (I'm not active in
that many subsystem, but I've been recently asked not to break lines
that goes up to 100 cols in a different subsystem, if I knew that from
the beginning, I could have saved a v2 just to un-break lines).

I find this very confusing, as each subsystem you interact with
you're asked to comply with undocumented preferences. Hence, better
document them here so that developers knows what comments they might
expect to receive, and reviewers have ground to justify their
requests.

>
> The coding style guide is lacking documentation on such things though.
>
> >
> > Thanks
> >    j
> >
> > ---
> >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > index eb1cdfd280ba..9c376f843e1c 100644
> > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> >      - when they avoid a line to end with an open parenthesis or an open
> >        bracket.
> >
> > +There are a few additional requirements which are not enforced by tooling
> > +but mostly during the review process:
> > +
> > +    - C++ style comments are not allowed, if not for SPDX headers;
> > +    - hexadecimal values should be spelled using lowercase letters;
> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;
> > +
> > +      As an example, the following style is preferred::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b;
> > +
> > +         b = a_very_long_operation_name(foo, s->bar)
> > +
> > +      over the following one::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b = a_very_long_operation_name(foo, s->bar)
>
> I wouldn't say this is required or even preferred if you have a dependency
> between the variables.
>
> Rather I'd say the latter is undesirable if a_very_long_operation_name()
> can fail. But that's a bit out of scope now.
>

I'm fine to drop it, all the points are there mainly to sparkle
discussions. But I think we should record something or decide that
everything not prohibited is allowed (maybe that's the case already
and I failed to understand it ?)

I've been confronted with a "on what ground are you asking this ?"
question recently when reviewing a driver commented only in C++ style.
I got nothing to offer except the 'no other driver does that' which
has proven not to be enough to convince the developer. Having a set of
rules you can refer to would have helped (and would have saved me
quite a few email exchanges that ended up in nothing).

Please note this serves for the other way around too. If it get
decided that everything that is not prohibited is allowed, I would
have not made those comments in first place. So please do not interpret
this as only directed to developers but also for less experienced
reviewers to know what changes in style they can ask for.

Thanks
   j

> > +
> >  Key Cycle Dates
> >  ---------------
>
> --
> Kind regards,
>
> Sakari Ailus
  

On 13/10/2021 11:20, Jacopo Mondi wrote:
> There are a few additional coding style conventions in place in
> the media subsystem. If they do not get documented, it's hard to enforce
> them during review as well as it is hard for developers to follow them
> without having previously contributed to the subsystem.
> 
> Add them to the subsystem profile documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> 
> All points are up for discussion ofc.
> 
> But the idea is to get to have more requirement defined, as otherwise
> it's very hard to enforce them during review.
> 
> Thanks
>    j
> 
> ---
>  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> index eb1cdfd280ba..9c376f843e1c 100644
> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
>      - when they avoid a line to end with an open parenthesis or an open
>        bracket.
> 
> +There are a few additional requirements which are not enforced by tooling
> +but mostly during the review process:
> +
> +    - C++ style comments are not allowed, if not for SPDX headers;

if not -> except

> +    - hexadecimal values should be spelled using lowercase letters;
> +    - one structure/enum member declaration per line;
> +    - one variable declaration per line;

Hmm, I don't mind something like: int i, j;

But for anything more complex I too prefer one declaration per line.

> +    - prefer variable declaration order in reverse-x-mas-tree over
> +      initialization at variable declare time;

Add something like:

...unless there are dependencies or other readability reasons to
depart from this.

> +
> +      As an example, the following style is preferred::
> +
> +         struct priv_struct *priv = container_of(....)
> +         struct foo_struct *foo = priv->foo;
> +         int b;
> +
> +         b = a_very_long_operation_name(foo, s->bar)
> +
> +      over the following one::
> +
> +         struct priv_struct *priv = container_of(....)
> +         struct foo_struct *foo = priv->foo;
> +         int b = a_very_long_operation_name(foo, s->bar)

I'm not sure if this is what you typically see.

Perhaps this is a better example:

	int i;
	struct foo_struct *foo = priv->foo;
	int result;

should be written as:

	struct foo_struct *foo = priv->foo;
	int result;
	int i;

Regards,

	Hans

> +
>  Key Cycle Dates
>  ---------------
> 
> --
> 2.33.0
>
  

On 21/10/2021 16:00, Hans Verkuil wrote:
> On 13/10/2021 11:20, Jacopo Mondi wrote:
>> There are a few additional coding style conventions in place in
>> the media subsystem. If they do not get documented, it's hard to enforce
>> them during review as well as it is hard for developers to follow them
>> without having previously contributed to the subsystem.
>>
>> Add them to the subsystem profile documentation.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>
>> All points are up for discussion ofc.
>>
>> But the idea is to get to have more requirement defined, as otherwise
>> it's very hard to enforce them during review.
>>
>> Thanks
>>    j
>>
>> ---
>>  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
>> index eb1cdfd280ba..9c376f843e1c 100644
>> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
>> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
>> @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
>>      - when they avoid a line to end with an open parenthesis or an open
>>        bracket.
>>
>> +There are a few additional requirements which are not enforced by tooling
>> +but mostly during the review process:
>> +
>> +    - C++ style comments are not allowed, if not for SPDX headers;
> 
> if not -> except
> 
>> +    - hexadecimal values should be spelled using lowercase letters;
>> +    - one structure/enum member declaration per line;
>> +    - one variable declaration per line;
> 
> Hmm, I don't mind something like: int i, j;
> 
> But for anything more complex I too prefer one declaration per line.
> 
>> +    - prefer variable declaration order in reverse-x-mas-tree over
>> +      initialization at variable declare time;
> 
> Add something like:
> 
> ...unless there are dependencies or other readability reasons to
> depart from this.
> 
>> +
>> +      As an example, the following style is preferred::
>> +
>> +         struct priv_struct *priv = container_of(....)
>> +         struct foo_struct *foo = priv->foo;
>> +         int b;
>> +
>> +         b = a_very_long_operation_name(foo, s->bar)
>> +
>> +      over the following one::
>> +
>> +         struct priv_struct *priv = container_of(....)
>> +         struct foo_struct *foo = priv->foo;
>> +         int b = a_very_long_operation_name(foo, s->bar)
> 
> I'm not sure if this is what you typically see.
> 
> Perhaps this is a better example:
> 
> 	int i;
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 
> should be written as:
> 
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 	int i;

There is one other requirement: the patches must be run through
scripts/checkpatch.pl --strict. Anything that --strict notifies you of and
that is reasonable to fix (not everything can be fixed) should be fixed.

Also (although perhaps out of scope for a coding style) before new V4L2
drivers or substantial enhancements to V4L2 drivers can be accepted, you must
run 'v4l2-compliance -s' for the video device (or even better use -m if the driver
creates a media device) and include the output with the cover letter of
the patch series. Obviously, any failures should be fixed.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>> +
>>  Key Cycle Dates
>>  ---------------
>>
>> --
>> 2.33.0
>>
>
  

Em Thu, 21 Oct 2021 16:00:40 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 13/10/2021 11:20, Jacopo Mondi wrote:
> > There are a few additional coding style conventions in place in
> > the media subsystem. If they do not get documented, it's hard to enforce
> > them during review as well as it is hard for developers to follow them
> > without having previously contributed to the subsystem.
> > 
> > Add them to the subsystem profile documentation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 
> > All points are up for discussion ofc.
> > 
> > But the idea is to get to have more requirement defined, as otherwise
> > it's very hard to enforce them during review.
> > 
> > Thanks
> >    j
> > 
> > ---
> >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > index eb1cdfd280ba..9c376f843e1c 100644
> > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> >      - when they avoid a line to end with an open parenthesis or an open
> >        bracket.
> > 
> > +There are a few additional requirements which are not enforced by tooling
> > +but mostly during the review process:
> > +
> > +    - C++ style comments are not allowed, if not for SPDX headers;  
> 
> if not -> except

While I prefer C99, I'm not really against having C++ comments on single
line comments. 

> 
> > +    - hexadecimal values should be spelled using lowercase letters;

> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;  
> 
> Hmm, I don't mind something like: int i, j;

I don't mind having things like:

	struct *dev, *parent_dev;

or even:

	struct *parent_dev, *dev = pdev->dev;

What it is really ugly is having multiple initialized vars at the
same declaration, like:

	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;

or, even worse:

	struct *dev = pdev->dev, *parent_dev = dev.parent;


> But for anything more complex I too prefer one declaration per line.
> 
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;  
> 
> Add something like:
> 
> ...unless there are dependencies or other readability reasons to
> depart from this.

+1

> 
> > +
> > +      As an example, the following style is preferred::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b;
> > +
> > +         b = a_very_long_operation_name(foo, s->bar)
> > +
> > +      over the following one::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b = a_very_long_operation_name(foo, s->bar)  
> 
> I'm not sure if this is what you typically see.
> 
> Perhaps this is a better example:
> 
> 	int i;
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 
> should be written as:
> 
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 	int i;
> 
> Regards,
> 
> 	Hans
> 
> > +
> >  Key Cycle Dates
> >  ---------------
> > 
> > --
> > 2.33.0
> >   
>
  

Em Thu, 21 Oct 2021 16:10:08 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> There is one other requirement: the patches must be run through
> scripts/checkpatch.pl --strict. Anything that --strict notifies you of and
> that is reasonable to fix (not everything can be fixed) should be fixed.

This is already there:

	Coding Style Addendum
	+++++++++++++++++++++
	
	Media development uses ``checkpatch.pl`` on strict mode to verify the code
	style, e.g.::

		$ ./scripts/checkpatch.pl --strict --max-line-length=80

> 
> Also (although perhaps out of scope for a coding style) before new V4L2
> drivers or substantial enhancements to V4L2 drivers can be accepted, you must
> run 'v4l2-compliance -s' for the video device (or even better use -m if the driver
> creates a media device) and include the output with the cover letter of
> the patch series. Obviously, any failures should be fixed.

This is also there:

	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:
	
	====================	=======================================================
	Type			Tool
	====================	=======================================================
	V4L2 drivers\ [3]_	``v4l2-compliance``
	V4L2 virtual drivers	``contrib/test/test-media``
	CEC drivers		``cec-compliance``
	====================	=======================================================
	
	.. [3] The ``v4l2-compliance`` also covers the media controller usage inside
	       V4L2 drivers.
	
	Other compilance tools are under development to check other parts of the
	subsystem.
	
	Those tests need to pass before the patches go upstream.

Regards,
Mauro
  

On Thu, Oct 21, 2021 at 04:00:40PM +0200, Hans Verkuil wrote:
> On 13/10/2021 11:20, Jacopo Mondi wrote:
> > There are a few additional coding style conventions in place in
> > the media subsystem. If they do not get documented, it's hard to enforce
> > them during review as well as it is hard for developers to follow them
> > without having previously contributed to the subsystem.
> > 
> > Add them to the subsystem profile documentation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 
> > All points are up for discussion ofc.
> > 
> > But the idea is to get to have more requirement defined, as otherwise
> > it's very hard to enforce them during review.
> > 
> > Thanks
> >    j
> > 
> > ---
> >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > index eb1cdfd280ba..9c376f843e1c 100644
> > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> >      - when they avoid a line to end with an open parenthesis or an open
> >        bracket.
> > 
> > +There are a few additional requirements which are not enforced by tooling
> > +but mostly during the review process:
> > +
> > +    - C++ style comments are not allowed, if not for SPDX headers;
> 
> if not -> except
> 
> > +    - hexadecimal values should be spelled using lowercase letters;
> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;
> 
> Hmm, I don't mind something like: int i, j;
> 
> But for anything more complex I too prefer one declaration per line.
> 
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;
> 
> Add something like:
> 
> ...unless there are dependencies or other readability reasons to
> depart from this.

This should probably go as the top-level, it's a valid comment for most
(all ?) rules.

> > +
> > +      As an example, the following style is preferred::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b;
> > +
> > +         b = a_very_long_operation_name(foo, s->bar)
> > +
> > +      over the following one::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b = a_very_long_operation_name(foo, s->bar)
> 
> I'm not sure if this is what you typically see.
> 
> Perhaps this is a better example:
> 
> 	int i;
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 
> should be written as:
> 
> 	struct foo_struct *foo = priv->foo;
> 	int result;
> 	int i;
> 
> > +
> >  Key Cycle Dates
> >  ---------------
> >
  

Hi Mauro,

On Thu, Oct 21, 2021 at 03:55:12PM +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Oct 2021 16:00:40 +0200 Hans Verkuil escreveu:
> > On 13/10/2021 11:20, Jacopo Mondi wrote:
> > > There are a few additional coding style conventions in place in
> > > the media subsystem. If they do not get documented, it's hard to enforce
> > > them during review as well as it is hard for developers to follow them
> > > without having previously contributed to the subsystem.
> > > 
> > > Add them to the subsystem profile documentation.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > 
> > > All points are up for discussion ofc.
> > > 
> > > But the idea is to get to have more requirement defined, as otherwise
> > > it's very hard to enforce them during review.
> > > 
> > > Thanks
> > >    j
> > > 
> > > ---
> > >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > index eb1cdfd280ba..9c376f843e1c 100644
> > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> > >      - when they avoid a line to end with an open parenthesis or an open
> > >        bracket.
> > > 
> > > +There are a few additional requirements which are not enforced by tooling
> > > +but mostly during the review process:
> > > +
> > > +    - C++ style comments are not allowed, if not for SPDX headers;  
> > 
> > if not -> except
> 
> While I prefer C99, I'm not really against having C++ comments on single
> line comments. 
> 
> > > +    - hexadecimal values should be spelled using lowercase letters;
> 
> > > +    - one structure/enum member declaration per line;
> > > +    - one variable declaration per line;  
> > 
> > Hmm, I don't mind something like: int i, j;
> 
> I don't mind having things like:
> 
> 	struct *dev, *parent_dev;
> 
> or even:
> 
> 	struct *parent_dev, *dev = pdev->dev;
> 
> What it is really ugly is having multiple initialized vars at the
> same declaration, like:
> 
> 	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;
> 
> or, even worse:
> 
> 	struct *dev = pdev->dev, *parent_dev = dev.parent;

Cording style is one of the main candidate areas for bikeshedding. The
first question that we should answer, I believe, is whether or not we
want to define a more precise coding style for the subsystem to achieve
higher uniformity, and how much latitude we want to give to developers.
For instance, I don't mind

	unsigned int i, j;

too much, but I would scream in horror at

	char *name = dev_name, c = '\0';

(I'm sad C even allows declaring a char pointer and a char variable on
the same line like this). There are lots of cases between those two
extremes that are more or less good (or bad) depending on who you ask,
so we won't be able to come up with a precise set of rules that draw a
line somewhere in the middle. What we could do is err more on the side
of strictness, for instance with

- One variable declaration per line. As an exception, grouping multiple
  single-letter counter variables on a single line is allowed.

(or even allowing no exception). This is probably stricter than it needs
to be, and in some cases it will result in a few more lines of code, but
if it brings increased readability and maintainability through
uniformity it's something we could consider.

The same reasoning can apply to C++ comments, we can decide to allow
them or not, but the more flexibility there will be in the rules, the
less uniformity we'll have, which I personally believe hinders
readability.

Please don't reply to details of this specific example here, what I'd
like to discuss first is the overall scope and principles.

> > But for anything more complex I too prefer one declaration per line.
> > 
> > > +    - prefer variable declaration order in reverse-x-mas-tree over
> > > +      initialization at variable declare time;  
> > 
> > Add something like:
> > 
> > ...unless there are dependencies or other readability reasons to
> > depart from this.
> 
> +1
> 
> > > +
> > > +      As an example, the following style is preferred::
> > > +
> > > +         struct priv_struct *priv = container_of(....)
> > > +         struct foo_struct *foo = priv->foo;
> > > +         int b;
> > > +
> > > +         b = a_very_long_operation_name(foo, s->bar)
> > > +
> > > +      over the following one::
> > > +
> > > +         struct priv_struct *priv = container_of(....)
> > > +         struct foo_struct *foo = priv->foo;
> > > +         int b = a_very_long_operation_name(foo, s->bar)  
> > 
> > I'm not sure if this is what you typically see.
> > 
> > Perhaps this is a better example:
> > 
> > 	int i;
> > 	struct foo_struct *foo = priv->foo;
> > 	int result;
> > 
> > should be written as:
> > 
> > 	struct foo_struct *foo = priv->foo;
> > 	int result;
> > 	int i;
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > +
> > >  Key Cycle Dates
> > >  ---------------
> > >
  

Hi Sakari,

On Tue, Oct 19, 2021 at 12:24:41PM +0300, Sakari Ailus wrote:
> On Wed, Oct 13, 2021 at 11:20:05AM +0200, Jacopo Mondi wrote:
> > There are a few additional coding style conventions in place in
> > the media subsystem. If they do not get documented, it's hard to enforce
> > them during review as well as it is hard for developers to follow them
> > without having previously contributed to the subsystem.
> > 
> > Add them to the subsystem profile documentation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > 
> > All points are up for discussion ofc.
> > 
> > But the idea is to get to have more requirement defined, as otherwise
> > it's very hard to enforce them during review.
> 
> Thanks for the patch.
> 
> Aren't these all common and/or preferred practices outside the media tree
> as well? I suppose not each one of these is universally enforced though.

They're not I'm afraid :-) Different subsystems have different
preferences, and within the realm of what a subsystem allows, different
parts also use different coding style rules. It's the same for media,
depending on who maintains a set of drivers, the rules will be
different.

> The coding style guide is lacking documentation on such things though.

Trying to fix that with a top-down approach will in my opinion not work.
I'd rather focus on media first to see if we can do something at the
subsystem level, in a bottom-up way (I've even considered writing rules
specific to sensor drivers, but if we can reach an agreement at the
subsystem level, that would be better).

> > ---
> >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > index eb1cdfd280ba..9c376f843e1c 100644
> > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> >      - when they avoid a line to end with an open parenthesis or an open
> >        bracket.
> > 
> > +There are a few additional requirements which are not enforced by tooling
> > +but mostly during the review process:
> > +
> > +    - C++ style comments are not allowed, if not for SPDX headers;
> > +    - hexadecimal values should be spelled using lowercase letters;
> > +    - one structure/enum member declaration per line;
> > +    - one variable declaration per line;
> > +    - prefer variable declaration order in reverse-x-mas-tree over
> > +      initialization at variable declare time;
> > +
> > +      As an example, the following style is preferred::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b;
> > +
> > +         b = a_very_long_operation_name(foo, s->bar)
> > +
> > +      over the following one::
> > +
> > +         struct priv_struct *priv = container_of(....)
> > +         struct foo_struct *foo = priv->foo;
> > +         int b = a_very_long_operation_name(foo, s->bar)
> 
> I wouldn't say this is required or even preferred if you have a dependency
> between the variables.
> 
> Rather I'd say the latter is undesirable if a_very_long_operation_name()
> can fail. But that's a bit out of scope now.
> 
> > +
> >  Key Cycle Dates
> >  ---------------
  

Em Thu, 21 Oct 2021 18:31:15 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 

> > > > +    - one structure/enum member declaration per line;
> > > > +    - one variable declaration per line;    
> > > 
> > > Hmm, I don't mind something like: int i, j;  
> > 
> > I don't mind having things like:
> > 
> > 	struct *dev, *parent_dev;
> > 
> > or even:
> > 
> > 	struct *parent_dev, *dev = pdev->dev;
> > 
> > What it is really ugly is having multiple initialized vars at the
> > same declaration, like:
> > 
> > 	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;
> > 
> > or, even worse:
> > 
> > 	struct *dev = pdev->dev, *parent_dev = dev.parent;  
> 
> Cording style is one of the main candidate areas for bikeshedding. The
> first question that we should answer, I believe, is whether or not we
> want to define a more precise coding style for the subsystem to achieve
> higher uniformity, and how much latitude we want to give to developers.

I would prefer to give more freedom to developers, provided that the
code is easy to read/maintain. Having to request multiple reviews just
due coding style nitpicking seems to be a waste of time for everyone ;-)

> For instance, I don't mind
> 
> 	unsigned int i, j;
> 
> too much, but I would scream in horror at
> 
> 	char *name = dev_name, c = '\0';

Yeah, multiple vars being declared and assigned at the same line is something
that should be avoided. See, even single letter vars with obvious assigns,
like:

	int i = 0, j = 1;

are less readable than:

	int	i = 0;
	int	j = 1;

> (I'm sad C even allows declaring a char pointer and a char variable on
> the same line like this). There are lots of cases between those two
> extremes that are more or less good (or bad) depending on who you ask,
> so we won't be able to come up with a precise set of rules that draw a
> line somewhere in the middle. What we could do is err more on the side
> of strictness, for instance with
> 
> - One variable declaration per line. As an exception, grouping multiple
>   single-letter counter variables on a single line is allowed.
> 
> (or even allowing no exception). This is probably stricter than it needs
> to be, and in some cases it will result in a few more lines of code, but
> if it brings increased readability and maintainability through
> uniformity it's something we could consider.

I don't think that things like:

	int ret, i, j;

are less readable/maintainable than:

	int ret;
	int i;
	int j;

Between the above, I would opt to the shorter format, when there's no 
variable initialization (no matter if the vars have single or multiple
chars).

On the other hand, I won't be nacking/rejecting a patch if it uses
the longer format, as, for me, both are equivalent, in terms of
maintenance and readability.

So, for me, the rule should be just:

- don't declare and initialize multiple variables at the same line.

> 
> The same reasoning can apply to C++ comments, we can decide to allow
> them or not, but the more flexibility there will be in the rules, the
> less uniformity we'll have, which I personally believe hinders
> readability.

Yeah, agreed. 

Regards,
Mauro
  

Hi Mauro,

On Thu, Oct 21, 2021 at 03:55:12PM +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Oct 2021 16:00:40 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
> > On 13/10/2021 11:20, Jacopo Mondi wrote:
> > > There are a few additional coding style conventions in place in
> > > the media subsystem. If they do not get documented, it's hard to enforce
> > > them during review as well as it is hard for developers to follow them
> > > without having previously contributed to the subsystem.
> > >
> > > Add them to the subsystem profile documentation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >
> > > All points are up for discussion ofc.
> > >
> > > But the idea is to get to have more requirement defined, as otherwise
> > > it's very hard to enforce them during review.
> > >
> > > Thanks
> > >    j
> > >
> > > ---
> > >  .../media/maintainer-entry-profile.rst        | 24 +++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > index eb1cdfd280ba..9c376f843e1c 100644
> > > --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> > > @@ -180,6 +180,30 @@ In particular, we accept lines with more than 80 columns:
> > >      - when they avoid a line to end with an open parenthesis or an open
> > >        bracket.
> > >
> > > +There are a few additional requirements which are not enforced by tooling
> > > +but mostly during the review process:
> > > +
> > > +    - C++ style comments are not allowed, if not for SPDX headers;
> >
> > if not -> except
>
> While I prefer C99, I'm not really against having C++ comments on single

Afaict C99 allowed // commenting style. We have to go back to C90 for
only /* */ :)

> line comments.
>

I wouldn't be against either, but I fear that without a slightly more
stringent rule we'll continue to have a lot of push-pull about what is
allowed or not.

I wouldn't be against something like "C++ comments are accepted for
single line comments" but as soon as someone uses them for multiple
lines we'll be back to discuss why 1 lines is fine 2 are not.
Execeptions also requires developers and reviewers to get back to the
subsystem rules for all tiny details. I feel one single rule, when possible,
would be simpler.

> >
> > > +    - hexadecimal values should be spelled using lowercase letters;
>
> > > +    - one structure/enum member declaration per line;
> > > +    - one variable declaration per line;
> >
> > Hmm, I don't mind something like: int i, j;
>
> I don't mind having things like:
>
> 	struct *dev, *parent_dev;
>
> or even:
>
> 	struct *parent_dev, *dev = pdev->dev;
>
> What it is really ugly is having multiple initialized vars at the
> same declaration, like:
>
> 	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;
>
> or, even worse:
>
> 	struct *dev = pdev->dev, *parent_dev = dev.parent;
>
>
> > But for anything more complex I too prefer one declaration per line.
> >
> > > +    - prefer variable declaration order in reverse-x-mas-tree over
> > > +      initialization at variable declare time;
> >
> > Add something like:
> >
> > ...unless there are dependencies or other readability reasons to
> > depart from this.
>
> +1
>
> >
> > > +
> > > +      As an example, the following style is preferred::
> > > +
> > > +         struct priv_struct *priv = container_of(....)
> > > +         struct foo_struct *foo = priv->foo;
> > > +         int b;
> > > +
> > > +         b = a_very_long_operation_name(foo, s->bar)
> > > +
> > > +      over the following one::
> > > +
> > > +         struct priv_struct *priv = container_of(....)
> > > +         struct foo_struct *foo = priv->foo;
> > > +         int b = a_very_long_operation_name(foo, s->bar)
> >
> > I'm not sure if this is what you typically see.
> >
> > Perhaps this is a better example:
> >
> > 	int i;
> > 	struct foo_struct *foo = priv->foo;
> > 	int result;
> >
> > should be written as:
> >
> > 	struct foo_struct *foo = priv->foo;
> > 	int result;
> > 	int i;
> >
> > Regards,
> >
> > 	Hans
> >
> > > +
> > >  Key Cycle Dates
> > >  ---------------
> > >
> > > --
> > > 2.33.0
> > >
> >
  

Hi Mauro, Laurent,

On Thu, Oct 21, 2021 at 05:17:59PM +0100, Mauro Carvalho Chehab wrote:
> Em Thu, 21 Oct 2021 18:31:15 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>
> > Hi Mauro,
> >
>
> > > > > +    - one structure/enum member declaration per line;
> > > > > +    - one variable declaration per line;
> > > >
> > > > Hmm, I don't mind something like: int i, j;
> > >
> > > I don't mind having things like:
> > >
> > > 	struct *dev, *parent_dev;
> > >
> > > or even:
> > >
> > > 	struct *parent_dev, *dev = pdev->dev;
> > >
> > > What it is really ugly is having multiple initialized vars at the
> > > same declaration, like:
> > >
> > > 	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;
> > >
> > > or, even worse:
> > >
> > > 	struct *dev = pdev->dev, *parent_dev = dev.parent;
> >
> > Cording style is one of the main candidate areas for bikeshedding. The
> > first question that we should answer, I believe, is whether or not we
> > want to define a more precise coding style for the subsystem to achieve
> > higher uniformity, and how much latitude we want to give to developers.
>
> I would prefer to give more freedom to developers, provided that the
> code is easy to read/maintain. Having to request multiple reviews just
> due coding style nitpicking seems to be a waste of time for everyone ;-)
>

I agree in principle, but at the same time, a particularly stubborn
confrontation during a review made me realize that most 'rules' are
tribal knowledge, and a particularly stubborn developer might impose
his own preferences arguing that everything that is not prohibited is
allowed. If you add to that in the most common case cargo cult is
the default way to find out what a rule is, if one driver escapes
others will take inspiration from it.

Now, I'm fine if it gets decided that everything not prohibited is
allowed, but then I fear it will be very hard to maintain a consistent
style among the subsystem.


> > For instance, I don't mind
> >
> > 	unsigned int i, j;
> >
> > too much, but I would scream in horror at
> >
> > 	char *name = dev_name, c = '\0';
>
> Yeah, multiple vars being declared and assigned at the same line is something
> that should be avoided. See, even single letter vars with obvious assigns,
> like:
>
> 	int i = 0, j = 1;
>
> are less readable than:
>
> 	int	i = 0;
> 	int	j = 1;
>
> > (I'm sad C even allows declaring a char pointer and a char variable on
> > the same line like this). There are lots of cases between those two
> > extremes that are more or less good (or bad) depending on who you ask,
> > so we won't be able to come up with a precise set of rules that draw a
> > line somewhere in the middle. What we could do is err more on the side
> > of strictness, for instance with
> >
> > - One variable declaration per line. As an exception, grouping multiple
> >   single-letter counter variables on a single line is allowed.
> >
> > (or even allowing no exception). This is probably stricter than it needs
> > to be, and in some cases it will result in a few more lines of code, but
> > if it brings increased readability and maintainability through
> > uniformity it's something we could consider.
>
> I don't think that things like:
>
> 	int ret, i, j;
>
> are less readable/maintainable than:
>
> 	int ret;
> 	int i;
> 	int j;
>
> Between the above, I would opt to the shorter format, when there's no
> variable initialization (no matter if the vars have single or multiple
> chars).
>
> On the other hand, I won't be nacking/rejecting a patch if it uses
> the longer format, as, for me, both are equivalent, in terms of
> maintenance and readability.
>
> So, for me, the rule should be just:
>
> - don't declare and initialize multiple variables at the same line.
>
> >
> > The same reasoning can apply to C++ comments, we can decide to allow
> > them or not, but the more flexibility there will be in the rules, the
> > less uniformity we'll have, which I personally believe hinders
> > readability.
>
> Yeah, agreed.

Thanks, I'll send a new version taking all your comments into account.

Thanks
   j

>
> Regards,
> Mauro
  

Hello,

On Thu, Oct 21, 2021 at 08:20:42PM +0200, Jacopo Mondi wrote:
> On Thu, Oct 21, 2021 at 05:17:59PM +0100, Mauro Carvalho Chehab wrote:
> > Em Thu, 21 Oct 2021 18:31:15 +0300 Laurent Pinchart escreveu:
> >
> > > > > > +    - one structure/enum member declaration per line;
> > > > > > +    - one variable declaration per line;
> > > > >
> > > > > Hmm, I don't mind something like: int i, j;
> > > >
> > > > I don't mind having things like:
> > > >
> > > > 	struct *dev, *parent_dev;
> > > >
> > > > or even:
> > > >
> > > > 	struct *parent_dev, *dev = pdev->dev;
> > > >
> > > > What it is really ugly is having multiple initialized vars at the
> > > > same declaration, like:
> > > >
> > > > 	struct *parent_dev = pdev->dev.parent, *dev = pdev->dev;
> > > >
> > > > or, even worse:
> > > >
> > > > 	struct *dev = pdev->dev, *parent_dev = dev.parent;
> > >
> > > Cording style is one of the main candidate areas for bikeshedding. The
> > > first question that we should answer, I believe, is whether or not we
> > > want to define a more precise coding style for the subsystem to achieve
> > > higher uniformity, and how much latitude we want to give to developers.
> >
> > I would prefer to give more freedom to developers, provided that the
> > code is easy to read/maintain. Having to request multiple reviews just
> > due coding style nitpicking seems to be a waste of time for everyone ;-)
> 
> I agree in principle, but at the same time, a particularly stubborn
> confrontation during a review made me realize that most 'rules' are
> tribal knowledge, and a particularly stubborn developer might impose
> his own preferences arguing that everything that is not prohibited is
> allowed. If you add to that in the most common case cargo cult is
> the default way to find out what a rule is, if one driver escapes
> others will take inspiration from it.
> 
> Now, I'm fine if it gets decided that everything not prohibited is
> allowed, but then I fear it will be very hard to maintain a consistent
> style among the subsystem.

I agree with this. Possibly more problematic than a consistent style, we
will then also have different reviewers asking for different style
changes during review, which will be confusing for developers and will
waste everybody's type. I see a more detailed style guide as a way to
streamline the process and make it more efficient.

> > > For instance, I don't mind
> > >
> > > 	unsigned int i, j;
> > >
> > > too much, but I would scream in horror at
> > >
> > > 	char *name = dev_name, c = '\0';
> >
> > Yeah, multiple vars being declared and assigned at the same line is something
> > that should be avoided. See, even single letter vars with obvious assigns,
> > like:
> >
> > 	int i = 0, j = 1;
> >
> > are less readable than:
> >
> > 	int	i = 0;
> > 	int	j = 1;
> >
> > > (I'm sad C even allows declaring a char pointer and a char variable on
> > > the same line like this). There are lots of cases between those two
> > > extremes that are more or less good (or bad) depending on who you ask,
> > > so we won't be able to come up with a precise set of rules that draw a
> > > line somewhere in the middle. What we could do is err more on the side
> > > of strictness, for instance with
> > >
> > > - One variable declaration per line. As an exception, grouping multiple
> > >   single-letter counter variables on a single line is allowed.
> > >
> > > (or even allowing no exception). This is probably stricter than it needs
> > > to be, and in some cases it will result in a few more lines of code, but
> > > if it brings increased readability and maintainability through
> > > uniformity it's something we could consider.
> >
> > I don't think that things like:
> >
> > 	int ret, i, j;
> >
> > are less readable/maintainable than:
> >
> > 	int ret;
> > 	int i;
> > 	int j;
> >
> > Between the above, I would opt to the shorter format, when there's no
> > variable initialization (no matter if the vars have single or multiple
> > chars).
> >
> > On the other hand, I won't be nacking/rejecting a patch if it uses
> > the longer format, as, for me, both are equivalent, in terms of
> > maintenance and readability.
> >
> > So, for me, the rule should be just:
> >
> > - don't declare and initialize multiple variables at the same line.
> >
> > >
> > > The same reasoning can apply to C++ comments, we can decide to allow
> > > them or not, but the more flexibility there will be in the rules, the
> > > less uniformity we'll have, which I personally believe hinders
> > > readability.
> >
> > Yeah, agreed.
> 
> Thanks, I'll send a new version taking all your comments into account.