diff mbox series

[1/7] doc: Migrate CodingStyle wiki page to Sphinx

Message ID 20220711171439.1657280-1-trini@konsulko.com
State Superseded
Delegated to: Heinrich Schuchardt
Headers show
Series [1/7] doc: Migrate CodingStyle wiki page to Sphinx | expand

Commit Message

Tom Rini July 11, 2022, 5:14 p.m. UTC
Move the current CodingStyle wiki page to doc/develop/codingstyle.rst.
The changes here are for formatting or slight rewording so that it reads
well when linking to other Sphinx documents.

Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Tom Rini <trini@konsulko.com>
---
Changes in v2:
- Assorted wiki -> Sphinx style corrections and a few typo fixes, per
  Heinrich
---
 doc/develop/codingstyle.rst | 255 ++++++++++++++++++++++++++++++++++++
 doc/develop/index.rst       |   8 ++
 2 files changed, 263 insertions(+)
 create mode 100644 doc/develop/codingstyle.rst

Comments

Martin Bonner July 11, 2022, 7:02 p.m. UTC | #1
On // comments:
a) I thought I had read recently that they are now accepted.
b) Can we stop calling them "C++ style"?  They have been legal in standard
C for all of this millennium!  There are university graduates who were born
after // comments became legal in standard C.
c) If we don't currently accept them, can we change the style now (see
previous point)?

A response along the lines of "this is just moving the wiki into .rst files
- we can actually change things separately" would seem entirely reasonable.
Martin


On Mon, 11 Jul 2022 at 19:15, Tom Rini <trini@konsulko.com> wrote:

> Move the current CodingStyle wiki page to doc/develop/codingstyle.rst.
> The changes here are for formatting or slight rewording so that it reads
> well when linking to other Sphinx documents.
>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Changes in v2:
> - Assorted wiki -> Sphinx style corrections and a few typo fixes, per
>   Heinrich
> ---
>  doc/develop/codingstyle.rst | 255 ++++++++++++++++++++++++++++++++++++
>  doc/develop/index.rst       |   8 ++
>  2 files changed, 263 insertions(+)
>  create mode 100644 doc/develop/codingstyle.rst
>
> diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
> new file mode 100644
> index 000000000000..bbeca42e656b
> --- /dev/null
> +++ b/doc/develop/codingstyle.rst
> @@ -0,0 +1,255 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +U-Boot Coding Style
> +===================
> +
> +The following Coding Style requirements shall be mandatory for all code
> contributed to
> +the U-Boot project.
> +
> +Exceptions are only allowed if code from other projects is integrated
> with no
> +or only minimal changes.
> +
> +The following rules apply:
> +
> +* All contributions to U-Boot should conform to the `Linux kernel
> +  coding style <
> https://www.kernel.org/doc/html/latest/process/coding-style.html>`_
> +  and the `Lindent script <
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Lindent
> >`_.
> +  * The exception for net files to the `multi-line comment
> +  <
> https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
> >`_
> +  applies only to Linux, not to U-Boot. Only large hunks which are copied
> +  unchanged from Linux may retain that comment format.
> +
> +* Use patman to send your patches (``tools/patman/patman -H`` for full
> +  instructions). With a few tags in your commits this will check your
> patches
> +  and take care of emailing them.
> +
> +* If you don't use patman, make sure to run ``scripts/checkpatch.pl``.
> For
> +  more information, read :doc:`checkpatch`. Note that this should be done
> +  *before* posting on the mailing list!
> +
> +* Source files originating from different projects (for example the MTD
> +  subsystem or the hush shell code from the BusyBox project) may, after
> +  careful consideration, be exempted from these rules. For such files, the
> +  original coding style may be kept to ease subsequent migration to newer
> +  versions of those sources.
> +
> +* Please note that U-Boot is implemented in C (and to some small parts in
> +  Assembler); no C++ is used, so please do not use C++ style comments
> (//) in
> +  your code.
> +
> +  * The sole exception here is for SPDX tags in some files (checkpatch.pl
> will warn you).
> +
> +* Please also stick to the following formatting rules:
> +
> +  * Remove any trailing white space
> +
> +  * Use TAB characters for indentation and vertical alignment, not spaces
> +
> +  * Make sure NOT to use DOS ``\r\n`` line feeds
> +
> +  * Do not add more than 2 consecutive empty lines to source files
> +
> +  * Do not add trailing empty lines to source files
> +
> +  * Using the option ``git config --global color.diff auto`` will help to
> +    visually see whitespace problems in ``diff`` output from ``git``.
> +
> +  * In Emacs one can use ``=M-x whitespace-global-mode=`` to get visual
> +    feedback on the nasty details. ``=M-x whitespace-cleanup=`` does The
> Right
> +    Thing (tm)
> +
> +Submissions of new code or patches that do not conform to these
> requirements
> +shall be rejected with a request to reformat the changes.
> +
> +U-Boot Code Documentation
> +-------------------------
> +
> +U-Boot adopted the kernel-doc annotation style, this is the only
> exception from
> +multi-line comment rule of Coding Style. While not mandatory, adding
> +documentation is strongly advised. The Linux kernel `kernel-doc
> +<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html>`_
> +documentation applies with no changes.
> +
> +Use structures for I/O access
> +-----------------------------
> +
> +U-Boot typically uses a C structure to map out the registers in an I/O
> region,
> +rather than offsets. The reasons for this are:
> +
> +* It dissociates the register location (offset) from the register type,
> which
> +  means the developer has to make sure the type is right for each access,
> +  whereas with the struct method, this is checked by the compiler;
> +
> +* It avoids actually writing all offsets, which is (more) error- prone;
> +
> +* It allows for better compile time sanity-checking of values we write to
> registers.
> +
> +Some reasons why you might not use C structures:
> +
> +* Where the registers appear at different offsets in different hardware
> +  revisions supported by the same driver
> +
> +* Where the driver only uses a small subset of registers and it is not
> worth
> +  defining a struct to cover them all, with large empty regions
> +
> +* Where the offset of a register might be hard to figure out when buried
> a long
> +  way down a structure, possibly with embedded sub-structures
> +
> +* This may need to change to the kernel model if we allow for more
> run-time
> +  detection of what drivers are appropriate for what we're running on.
> +
> +Please use check_member() to verify that your structure is the expected
> size,
> +or that particular members appear at the right offset.
> +
> +Include files
> +-------------
> +
> +You should follow this ordering in U-Boot. The common.h header (which is
> going
> +away at some point) should always be first, followed by other headers in
> order,
> +then headers with directories, then local files:
> +
> +.. code-block:: C
> +
> +   #include <common.h>
> +   #include <bootstage.h>
> +   #include <dm.h>
> +   #include <others.h>
> +   #include <asm/...>
> +   #include <arm/arch/...>
> +   #include <dm/device_compat/.h>
> +   #include <linux/...>
> +   #include "local.h"
> +
> +Within that order, sort your includes.
> +
> +It is important to include common.h first since it provides basic
> features used
> +by most files, e.g. CONFIG options.
> +
> +For files that need to be compiled for the host (e.g. tools), you need to
> use
> +``#ifndef USE_HOSTCC`` to avoid including common.h since it includes a
> lot of
> +internal U-Boot things. See common/image.c for an example.
> +
> +If your file uses driver model, include <dm.h> in the C file. Do not
> include
> +dm.h in a header file. Try to use forward declarations (e.g. ``struct
> +udevice``) instead.
> +
> +Filenames
> +---------
> +
> +For .c and .h files try to use underscore rather than hyphen unless you
> want
> +the file to stand out (e.g. driver-model uclasses should be named
> xxx-uclass.h.
> +Avoid upper case and keep the names fairly short.
> +
> +Function and struct comments
> +----------------------------
> +
> +Non-trivial functions should have a comment which describes what they do.
> If it
> +is an exported function, put the comment in the header file so the API is
> in
> +one place. If it is a static function, put it in the C file.
> +
> +If the function returns errors, mention that and list the different
> errors that
> +are returned. If it is merely passing errors back from a function it
> calls,
> +then you can skip that.
> +
> +See `here
> +<
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> >`_
> +for style.
> +
> +Driver model
> +------------
> +
> +When declaring a device, try to use ``struct udevice *dev``, i.e. ``dev``
> as the name:
> +
> +.. code-block:: C
> +
> +   struct udevice *dev;
> +
> +Use ``ret`` as the return value:
> +
> +.. code-block:: C
> +
> +   struct udevice *dev;
> +   int ret;
> +
> +   ret = uclass_first_device_err(UCLASS_ACPI_PMC, &dev);
> +   if (ret)
> +           return log_msg_ret("pmc", dev);
> +
> +Consider using log_ret() or log_msg_ret() to return a value (see above).
> +
> +Add a ``p`` suffix on return arguments:
> +
> +.. code-block:: C
> +
> +   int dm_pci_find_class(uint find_class, int index, struct udevice
> **devp)
> +   {
> +   ...
> +           *devp = dev;
> +
> +           return 0;
> +   }
> +
> +There are standard variable names that you should use in drivers:
> +
> +* ``struct xxx_priv`` and ``priv`` for dev_get_priv()
> +
> +* ``struct xxx_plat`` and ``plat`` for dev_get_platdata()
> +
> +For example:
> +
> +.. code-block:: C
> +
> +   struct simple_bus_plat {
> +      u32 base;
> +      u32 size;
> +      u32 target;
> +   };
> +
> +   /* Davinci MMC board definitions */
> +   struct davinci_mmc_priv {
> +      struct davinci_mmc_regs *reg_base;   /* Register base address */
> +      uint input_clk;      /* Input clock to MMC controller */
> +      struct gpio_desc cd_gpio;       /* Card Detect GPIO */
> +      struct gpio_desc wp_gpio;       /* Write Protect GPIO */
> +   };
> +
> +      struct rcar_gpio_priv *priv = dev_get_priv(dev);
> +
> +      struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
> +
> +Other
> +-----
> +
> +Some minor things:
> +
> +* Put a blank line before the last ``return`` in a function unless it is
> the only line:
> +
> +.. code-block:: C
> +
> +   struct udevice *pci_get_controller(struct udevice *dev)
> +   {
> +      while (device_is_on_pci_bus(dev))
> +         dev = dev->parent;
> +
> +      return dev;
> +   }
> +
> +Tests
> +-----
> +
> +Please add tests when you add code. Please change or expand tests when
> you change code.
> +
> +Run the tests with::
> +
> +   make check
> +   make qcheck   (skips some tests)
> +
> +Python tests are in test/py/tests - see the docs in test/py for info.
> +
> +Try to write your tests in C if you can. For example, tests to check a
> command
> +will be much faster (10-100x or more) if they can directly call
> run_command()
> +and ut_check_console_line() instead of using Python to send commands over
> a
> +pipe to U-Boot.
> +
> +Tests run all supported CI systems (gitlab, travis, azure) using scripts
> in the
> +root of the U-Boot tree.
> diff --git a/doc/develop/index.rst b/doc/develop/index.rst
> index fe3564a9fbf4..dde47994c71a 100644
> --- a/doc/develop/index.rst
> +++ b/doc/develop/index.rst
> @@ -3,6 +3,14 @@
>  Develop U-Boot
>  ==============
>
> +General
> +-------
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   codingstyle
> +
>  Implementation
>  --------------
>
> --
> 2.25.1
>
>
Tom Rini July 11, 2022, 7:04 p.m. UTC | #2
On Mon, Jul 11, 2022 at 09:02:47PM +0200, Martin Bonner wrote:

> On // comments:
> a) I thought I had read recently that they are now accepted.
> b) Can we stop calling them "C++ style"?  They have been legal in standard
> C for all of this millennium!  There are university graduates who were born
> after // comments became legal in standard C.
> c) If we don't currently accept them, can we change the style now (see
> previous point)?
> 
> A response along the lines of "this is just moving the wiki into .rst files
> - we can actually change things separately" would seem entirely reasonable.
> Martin

Yeah, I didn't cc you on
https://patchwork.ozlabs.org/project/uboot/patch/20220711171439.1657280-3-trini@konsulko.com/,
sorry
Heinrich Schuchardt July 13, 2022, 5:06 p.m. UTC | #3
On 7/11/22 19:14, Tom Rini wrote:
> Move the current CodingStyle wiki page to doc/develop/codingstyle.rst.
> The changes here are for formatting or slight rewording so that it reads
> well when linking to other Sphinx documents.
>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Changes in v2:
> - Assorted wiki -> Sphinx style corrections and a few typo fixes, per
>    Heinrich
> ---
>   doc/develop/codingstyle.rst | 255 ++++++++++++++++++++++++++++++++++++
>   doc/develop/index.rst       |   8 ++
>   2 files changed, 263 insertions(+)
>   create mode 100644 doc/develop/codingstyle.rst
>
> diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
> new file mode 100644
> index 000000000000..bbeca42e656b
> --- /dev/null
> +++ b/doc/develop/codingstyle.rst
> @@ -0,0 +1,255 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +U-Boot Coding Style
> +===================
> +
> +The following Coding Style requirements shall be mandatory for all code contributed to
> +the U-Boot project.
> +
> +Exceptions are only allowed if code from other projects is integrated with no
> +or only minimal changes.
> +
> +The following rules apply:
> +
> +* All contributions to U-Boot should conform to the `Linux kernel
> +  coding style <https://www.kernel.org/doc/html/latest/process/coding-style.html>`_
> +  and the `Lindent script <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Lindent>`_.
> +  * The exception for net files to the `multi-line comment
> +  <https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting>`_
> +  applies only to Linux, not to U-Boot. Only large hunks which are copied
> +  unchanged from Linux may retain that comment format.
> +
> +* Use patman to send your patches (``tools/patman/patman -H`` for full
> +  instructions). With a few tags in your commits this will check your patches
> +  and take care of emailing them.
> +
> +* If you don't use patman, make sure to run ``scripts/checkpatch.pl``. For
> +  more information, read :doc:`checkpatch`. Note that this should be done
> +  *before* posting on the mailing list!
> +
> +* Source files originating from different projects (for example the MTD
> +  subsystem or the hush shell code from the BusyBox project) may, after
> +  careful consideration, be exempted from these rules. For such files, the
> +  original coding style may be kept to ease subsequent migration to newer
> +  versions of those sources.
> +
> +* Please note that U-Boot is implemented in C (and to some small parts in
> +  Assembler); no C++ is used, so please do not use C++ style comments (//) in
> +  your code.
> +
> +  * The sole exception here is for SPDX tags in some files (checkpatch.pl will warn you).
> +
> +* Please also stick to the following formatting rules:
> +
> +  * Remove any trailing white space
> +
> +  * Use TAB characters for indentation and vertical alignment, not spaces

This only holds true for C and assembler code.
For Python we use 4 spaces per indent.

> +
> +  * Make sure NOT to use DOS ``\r\n`` line feeds

%s/DOS/Windows/ - The documentation is not dedicated to veterans.
%s/line feeds/line ends/ - a line feed is always 0x0a.

\r\n are escape codes in C. On a Windows machine these will be rendered
as CR CR LF.

Please, use CR LF instead of \r\n.

> +
> +  * Do not add more than 2 consecutive empty lines to source files
> +
> +  * Do not add trailing empty lines to source files
> +
> +  * Using the option ``git config --global color.diff auto`` will help to
> +    visually see whitespace problems in ``diff`` output from ``git``.
> +
> +  * In Emacs one can use ``=M-x whitespace-global-mode=`` to get visual
> +    feedback on the nasty details. ``=M-x whitespace-cleanup=`` does The Right
> +    Thing (tm)

No clue why 'The Right Thing' should be a trademark here.
Please, remove this bogus.

> +
> +Submissions of new code or patches that do not conform to these requirements
> +shall be rejected with a request to reformat the changes.
> +
> +U-Boot Code Documentation
> +-------------------------
> +
> +U-Boot adopted the kernel-doc annotation style, this is the only exception from
> +multi-line comment rule of Coding Style. While not mandatory, adding

Do you mean the extra asterisk?

> +documentation is strongly advised. The Linux kernel `kernel-doc
> +<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html>`_
> +documentation applies with no changes.
> +
> +Use structures for I/O access
> +-----------------------------
> +
> +U-Boot typically uses a C structure to map out the registers in an I/O region,
> +rather than offsets. The reasons for this are:
> +
> +* It dissociates the register location (offset) from the register type, which
> +  means the developer has to make sure the type is right for each access,
> +  whereas with the struct method, this is checked by the compiler;
> +
> +* It avoids actually writing all offsets, which is (more) error- prone;

%s/error- prone/error-prone/

> +
> +* It allows for better compile time sanity-checking of values we write to registers.
> +
> +Some reasons why you might not use C structures:
> +
> +* Where the registers appear at different offsets in different hardware
> +  revisions supported by the same driver
> +
> +* Where the driver only uses a small subset of registers and it is not worth
> +  defining a struct to cover them all, with large empty regions
> +
> +* Where the offset of a register might be hard to figure out when buried a long
> +  way down a structure, possibly with embedded sub-structures
> +
> +* This may need to change to the kernel model if we allow for more run-time
> +  detection of what drivers are appropriate for what we're running on.
> +
> +Please use check_member() to verify that your structure is the expected size,

%s/check_member()/the check_member() macro/

> +or that particular members appear at the right offset.
> +
> +Include files
> +-------------
> +
> +You should follow this ordering in U-Boot. The common.h header (which is going
> +away at some point) should always be first, followed by other headers in order,
> +then headers with directories, then local files:
> +
> +.. code-block:: C
> +
> +   #include <common.h>
> +   #include <bootstage.h>
> +   #include <dm.h>
> +   #include <others.h>
> +   #include <asm/...>
> +   #include <arm/arch/...>
> +   #include <dm/device_compat/.h>
> +   #include <linux/...>
> +   #include "local.h"
> +
> +Within that order, sort your includes.
> +
> +It is important to include common.h first since it provides basic features used
> +by most files, e.g. CONFIG options.
> +
> +For files that need to be compiled for the host (e.g. tools), you need to use
> +``#ifndef USE_HOSTCC`` to avoid including common.h since it includes a lot of
> +internal U-Boot things. See common/image.c for an example.
> +
> +If your file uses driver model, include <dm.h> in the C file. Do not include

%s/uses/uses the/

Best regards

Heinrich

> +dm.h in a header file. Try to use forward declarations (e.g. ``struct
> +udevice``) instead.
> +
> +Filenames
> +---------
> +
> +For .c and .h files try to use underscore rather than hyphen unless you want
> +the file to stand out (e.g. driver-model uclasses should be named xxx-uclass.h.
> +Avoid upper case and keep the names fairly short.
> +
> +Function and struct comments
> +----------------------------
> +
> +Non-trivial functions should have a comment which describes what they do. If it
> +is an exported function, put the comment in the header file so the API is in
> +one place. If it is a static function, put it in the C file.
> +
> +If the function returns errors, mention that and list the different errors that
> +are returned. If it is merely passing errors back from a function it calls,
> +then you can skip that.
> +
> +See `here
> +<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation>`_
> +for style.
> +
> +Driver model
> +------------
> +
> +When declaring a device, try to use ``struct udevice *dev``, i.e. ``dev`` as the name:
> +
> +.. code-block:: C
> +
> +   struct udevice *dev;
> +
> +Use ``ret`` as the return value:
> +
> +.. code-block:: C
> +
> +   struct udevice *dev;
> +   int ret;
> +
> +   ret = uclass_first_device_err(UCLASS_ACPI_PMC, &dev);
> +   if (ret)
> +           return log_msg_ret("pmc", dev);
> +
> +Consider using log_ret() or log_msg_ret() to return a value (see above).
> +
> +Add a ``p`` suffix on return arguments:
> +
> +.. code-block:: C
> +
> +   int dm_pci_find_class(uint find_class, int index, struct udevice **devp)
> +   {
> +   ...
> +           *devp = dev;
> +
> +           return 0;
> +   }
> +
> +There are standard variable names that you should use in drivers:
> +
> +* ``struct xxx_priv`` and ``priv`` for dev_get_priv()
> +
> +* ``struct xxx_plat`` and ``plat`` for dev_get_platdata()
> +
> +For example:
> +
> +.. code-block:: C
> +
> +   struct simple_bus_plat {
> +      u32 base;
> +      u32 size;
> +      u32 target;
> +   };
> +
> +   /* Davinci MMC board definitions */
> +   struct davinci_mmc_priv {
> +      struct davinci_mmc_regs *reg_base;   /* Register base address */
> +      uint input_clk;      /* Input clock to MMC controller */
> +      struct gpio_desc cd_gpio;       /* Card Detect GPIO */
> +      struct gpio_desc wp_gpio;       /* Write Protect GPIO */
> +   };
> +
> +      struct rcar_gpio_priv *priv = dev_get_priv(dev);
> +
> +      struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
> +
> +Other
> +-----
> +
> +Some minor things:
> +
> +* Put a blank line before the last ``return`` in a function unless it is the only line:
> +
> +.. code-block:: C
> +
> +   struct udevice *pci_get_controller(struct udevice *dev)
> +   {
> +      while (device_is_on_pci_bus(dev))
> +         dev = dev->parent;
> +
> +      return dev;
> +   }
> +
> +Tests
> +-----
> +
> +Please add tests when you add code. Please change or expand tests when you change code.
> +
> +Run the tests with::
> +
> +   make check
> +   make qcheck   (skips some tests)
> +
> +Python tests are in test/py/tests - see the docs in test/py for info.
> +
> +Try to write your tests in C if you can. For example, tests to check a command
> +will be much faster (10-100x or more) if they can directly call run_command()
> +and ut_check_console_line() instead of using Python to send commands over a
> +pipe to U-Boot.
> +
> +Tests run all supported CI systems (gitlab, travis, azure) using scripts in the
> +root of the U-Boot tree.
> diff --git a/doc/develop/index.rst b/doc/develop/index.rst
> index fe3564a9fbf4..dde47994c71a 100644
> --- a/doc/develop/index.rst
> +++ b/doc/develop/index.rst
> @@ -3,6 +3,14 @@
>   Develop U-Boot
>   ==============
>
> +General
> +-------
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   codingstyle
> +
>   Implementation
>   --------------
>
Tom Rini July 13, 2022, 5:30 p.m. UTC | #4
On Wed, Jul 13, 2022 at 07:06:56PM +0200, Heinrich Schuchardt wrote:
> On 7/11/22 19:14, Tom Rini wrote:
> > Move the current CodingStyle wiki page to doc/develop/codingstyle.rst.
> > The changes here are for formatting or slight rewording so that it reads
> > well when linking to other Sphinx documents.
> > 
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Changes in v2:
> > - Assorted wiki -> Sphinx style corrections and a few typo fixes, per
> >    Heinrich
> > ---
> >   doc/develop/codingstyle.rst | 255 ++++++++++++++++++++++++++++++++++++
> >   doc/develop/index.rst       |   8 ++
> >   2 files changed, 263 insertions(+)
> >   create mode 100644 doc/develop/codingstyle.rst
> > 
> > diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
> > new file mode 100644
> > index 000000000000..bbeca42e656b
> > --- /dev/null
> > +++ b/doc/develop/codingstyle.rst
> > @@ -0,0 +1,255 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +U-Boot Coding Style
> > +===================
> > +
> > +The following Coding Style requirements shall be mandatory for all code contributed to
> > +the U-Boot project.
> > +
> > +Exceptions are only allowed if code from other projects is integrated with no
> > +or only minimal changes.
> > +
> > +The following rules apply:
> > +
> > +* All contributions to U-Boot should conform to the `Linux kernel
> > +  coding style <https://www.kernel.org/doc/html/latest/process/coding-style.html>`_
> > +  and the `Lindent script <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Lindent>`_.
> > +  * The exception for net files to the `multi-line comment
> > +  <https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting>`_
> > +  applies only to Linux, not to U-Boot. Only large hunks which are copied
> > +  unchanged from Linux may retain that comment format.
> > +
> > +* Use patman to send your patches (``tools/patman/patman -H`` for full
> > +  instructions). With a few tags in your commits this will check your patches
> > +  and take care of emailing them.
> > +
> > +* If you don't use patman, make sure to run ``scripts/checkpatch.pl``. For
> > +  more information, read :doc:`checkpatch`. Note that this should be done
> > +  *before* posting on the mailing list!
> > +
> > +* Source files originating from different projects (for example the MTD
> > +  subsystem or the hush shell code from the BusyBox project) may, after
> > +  careful consideration, be exempted from these rules. For such files, the
> > +  original coding style may be kept to ease subsequent migration to newer
> > +  versions of those sources.
> > +
> > +* Please note that U-Boot is implemented in C (and to some small parts in
> > +  Assembler); no C++ is used, so please do not use C++ style comments (//) in
> > +  your code.
> > +
> > +  * The sole exception here is for SPDX tags in some files (checkpatch.pl will warn you).
> > +
> > +* Please also stick to the following formatting rules:
> > +
> > +  * Remove any trailing white space
> > +
> > +  * Use TAB characters for indentation and vertical alignment, not spaces
> 
> This only holds true for C and assembler code.
> For Python we use 4 spaces per indent.
> 
> > +
> > +  * Make sure NOT to use DOS ``\r\n`` line feeds
> 
> %s/DOS/Windows/ - The documentation is not dedicated to veterans.
> %s/line feeds/line ends/ - a line feed is always 0x0a.
> 
> \r\n are escape codes in C. On a Windows machine these will be rendered
> as CR CR LF.
> 
> Please, use CR LF instead of \r\n.
> 
> > +
> > +  * Do not add more than 2 consecutive empty lines to source files
> > +
> > +  * Do not add trailing empty lines to source files
> > +
> > +  * Using the option ``git config --global color.diff auto`` will help to
> > +    visually see whitespace problems in ``diff`` output from ``git``.
> > +
> > +  * In Emacs one can use ``=M-x whitespace-global-mode=`` to get visual
> > +    feedback on the nasty details. ``=M-x whitespace-cleanup=`` does The Right
> > +    Thing (tm)
> 
> No clue why 'The Right Thing' should be a trademark here.
> Please, remove this bogus.
> 
> > +
> > +Submissions of new code or patches that do not conform to these requirements
> > +shall be rejected with a request to reformat the changes.
> > +
> > +U-Boot Code Documentation
> > +-------------------------
> > +
> > +U-Boot adopted the kernel-doc annotation style, this is the only exception from
> > +multi-line comment rule of Coding Style. While not mandatory, adding
> 
> Do you mean the extra asterisk?
> 
> > +documentation is strongly advised. The Linux kernel `kernel-doc
> > +<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html>`_
> > +documentation applies with no changes.
> > +
> > +Use structures for I/O access
> > +-----------------------------
> > +
> > +U-Boot typically uses a C structure to map out the registers in an I/O region,
> > +rather than offsets. The reasons for this are:
> > +
> > +* It dissociates the register location (offset) from the register type, which
> > +  means the developer has to make sure the type is right for each access,
> > +  whereas with the struct method, this is checked by the compiler;
> > +
> > +* It avoids actually writing all offsets, which is (more) error- prone;
> 
> %s/error- prone/error-prone/
> 
> > +
> > +* It allows for better compile time sanity-checking of values we write to registers.
> > +
> > +Some reasons why you might not use C structures:
> > +
> > +* Where the registers appear at different offsets in different hardware
> > +  revisions supported by the same driver
> > +
> > +* Where the driver only uses a small subset of registers and it is not worth
> > +  defining a struct to cover them all, with large empty regions
> > +
> > +* Where the offset of a register might be hard to figure out when buried a long
> > +  way down a structure, possibly with embedded sub-structures
> > +
> > +* This may need to change to the kernel model if we allow for more run-time
> > +  detection of what drivers are appropriate for what we're running on.
> > +
> > +Please use check_member() to verify that your structure is the expected size,
> 
> %s/check_member()/the check_member() macro/
> 
> > +or that particular members appear at the right offset.
> > +
> > +Include files
> > +-------------
> > +
> > +You should follow this ordering in U-Boot. The common.h header (which is going
> > +away at some point) should always be first, followed by other headers in order,
> > +then headers with directories, then local files:
> > +
> > +.. code-block:: C
> > +
> > +   #include <common.h>
> > +   #include <bootstage.h>
> > +   #include <dm.h>
> > +   #include <others.h>
> > +   #include <asm/...>
> > +   #include <arm/arch/...>
> > +   #include <dm/device_compat/.h>
> > +   #include <linux/...>
> > +   #include "local.h"
> > +
> > +Within that order, sort your includes.
> > +
> > +It is important to include common.h first since it provides basic features used
> > +by most files, e.g. CONFIG options.
> > +
> > +For files that need to be compiled for the host (e.g. tools), you need to use
> > +``#ifndef USE_HOSTCC`` to avoid including common.h since it includes a lot of
> > +internal U-Boot things. See common/image.c for an example.
> > +
> > +If your file uses driver model, include <dm.h> in the C file. Do not include
> 
> %s/uses/uses the/

I'll do another iteration with some of the minor spelling / formatting
fixes, but I do not want to start updating the document in the import
patch.
Tom Rini July 13, 2022, 5:41 p.m. UTC | #5
On Wed, Jul 13, 2022 at 07:06:56PM +0200, Heinrich Schuchardt wrote:
> On 7/11/22 19:14, Tom Rini wrote:
> > Move the current CodingStyle wiki page to doc/develop/codingstyle.rst.
> > The changes here are for formatting or slight rewording so that it reads
> > well when linking to other Sphinx documents.
> > 
> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Changes in v2:
> > - Assorted wiki -> Sphinx style corrections and a few typo fixes, per
> >    Heinrich
> > ---
> >   doc/develop/codingstyle.rst | 255 ++++++++++++++++++++++++++++++++++++
> >   doc/develop/index.rst       |   8 ++
> >   2 files changed, 263 insertions(+)
> >   create mode 100644 doc/develop/codingstyle.rst
> > 
> > diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
> > new file mode 100644
> > index 000000000000..bbeca42e656b
> > --- /dev/null
> > +++ b/doc/develop/codingstyle.rst
> > @@ -0,0 +1,255 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +U-Boot Coding Style
> > +===================
> > +
> > +The following Coding Style requirements shall be mandatory for all code contributed to
> > +the U-Boot project.
> > +
> > +Exceptions are only allowed if code from other projects is integrated with no
> > +or only minimal changes.
> > +
> > +The following rules apply:
> > +
> > +* All contributions to U-Boot should conform to the `Linux kernel
> > +  coding style <https://www.kernel.org/doc/html/latest/process/coding-style.html>`_
> > +  and the `Lindent script <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Lindent>`_.
> > +  * The exception for net files to the `multi-line comment
> > +  <https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting>`_
> > +  applies only to Linux, not to U-Boot. Only large hunks which are copied
> > +  unchanged from Linux may retain that comment format.
> > +
> > +* Use patman to send your patches (``tools/patman/patman -H`` for full
> > +  instructions). With a few tags in your commits this will check your patches
> > +  and take care of emailing them.
> > +
> > +* If you don't use patman, make sure to run ``scripts/checkpatch.pl``. For
> > +  more information, read :doc:`checkpatch`. Note that this should be done
> > +  *before* posting on the mailing list!
> > +
> > +* Source files originating from different projects (for example the MTD
> > +  subsystem or the hush shell code from the BusyBox project) may, after
> > +  careful consideration, be exempted from these rules. For such files, the
> > +  original coding style may be kept to ease subsequent migration to newer
> > +  versions of those sources.
> > +
> > +* Please note that U-Boot is implemented in C (and to some small parts in
> > +  Assembler); no C++ is used, so please do not use C++ style comments (//) in
> > +  your code.
> > +
> > +  * The sole exception here is for SPDX tags in some files (checkpatch.pl will warn you).
> > +
> > +* Please also stick to the following formatting rules:
> > +
> > +  * Remove any trailing white space
> > +
> > +  * Use TAB characters for indentation and vertical alignment, not spaces
> 
> This only holds true for C and assembler code.
> For Python we use 4 spaces per indent.
> 
> > +
> > +  * Make sure NOT to use DOS ``\r\n`` line feeds
> 
> %s/DOS/Windows/ - The documentation is not dedicated to veterans.
> %s/line feeds/line ends/ - a line feed is always 0x0a.
> 
> \r\n are escape codes in C. On a Windows machine these will be rendered
> as CR CR LF.
> 
> Please, use CR LF instead of \r\n.
> 
> > +
> > +  * Do not add more than 2 consecutive empty lines to source files
> > +
> > +  * Do not add trailing empty lines to source files
> > +
> > +  * Using the option ``git config --global color.diff auto`` will help to
> > +    visually see whitespace problems in ``diff`` output from ``git``.
> > +
> > +  * In Emacs one can use ``=M-x whitespace-global-mode=`` to get visual
> > +    feedback on the nasty details. ``=M-x whitespace-cleanup=`` does The Right
> > +    Thing (tm)
> 
> No clue why 'The Right Thing' should be a trademark here.
> Please, remove this bogus.

The phrase "The Right Thing (tm)" is I guess an old joke, as it implies
it will probably work, but still might mess up from time to time.  I'd
rather keep it.

> > +
> > +Submissions of new code or patches that do not conform to these requirements
> > +shall be rejected with a request to reformat the changes.
> > +
> > +U-Boot Code Documentation
> > +-------------------------
> > +
> > +U-Boot adopted the kernel-doc annotation style, this is the only exception from
> > +multi-line comment rule of Coding Style. While not mandatory, adding
> 
> Do you mean the extra asterisk?

Yes.

> > +documentation is strongly advised. The Linux kernel `kernel-doc
> > +<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html>`_
> > +documentation applies with no changes.
> > +
> > +Use structures for I/O access
> > +-----------------------------
> > +
> > +U-Boot typically uses a C structure to map out the registers in an I/O region,
> > +rather than offsets. The reasons for this are:
> > +
> > +* It dissociates the register location (offset) from the register type, which
> > +  means the developer has to make sure the type is right for each access,
> > +  whereas with the struct method, this is checked by the compiler;
> > +
> > +* It avoids actually writing all offsets, which is (more) error- prone;
> 
> %s/error- prone/error-prone/
> 
> > +
> > +* It allows for better compile time sanity-checking of values we write to registers.
> > +
> > +Some reasons why you might not use C structures:
> > +
> > +* Where the registers appear at different offsets in different hardware
> > +  revisions supported by the same driver
> > +
> > +* Where the driver only uses a small subset of registers and it is not worth
> > +  defining a struct to cover them all, with large empty regions
> > +
> > +* Where the offset of a register might be hard to figure out when buried a long
> > +  way down a structure, possibly with embedded sub-structures
> > +
> > +* This may need to change to the kernel model if we allow for more run-time
> > +  detection of what drivers are appropriate for what we're running on.
> > +
> > +Please use check_member() to verify that your structure is the expected size,
> 
> %s/check_member()/the check_member() macro/

Since I'm in here anyhow, sure.

> > +or that particular members appear at the right offset.
> > +
> > +Include files
> > +-------------
> > +
> > +You should follow this ordering in U-Boot. The common.h header (which is going
> > +away at some point) should always be first, followed by other headers in order,
> > +then headers with directories, then local files:
> > +
> > +.. code-block:: C
> > +
> > +   #include <common.h>
> > +   #include <bootstage.h>
> > +   #include <dm.h>
> > +   #include <others.h>
> > +   #include <asm/...>
> > +   #include <arm/arch/...>
> > +   #include <dm/device_compat/.h>
> > +   #include <linux/...>
> > +   #include "local.h"
> > +
> > +Within that order, sort your includes.
> > +
> > +It is important to include common.h first since it provides basic features used
> > +by most files, e.g. CONFIG options.
> > +
> > +For files that need to be compiled for the host (e.g. tools), you need to use
> > +``#ifndef USE_HOSTCC`` to avoid including common.h since it includes a lot of
> > +internal U-Boot things. See common/image.c for an example.
> > +
> > +If your file uses driver model, include <dm.h> in the C file. Do not include
> 
> %s/uses/uses the/

We more consistently say "uses driver model" (9 times) not "uses the
driver model" (1 time).
diff mbox series

Patch

diff --git a/doc/develop/codingstyle.rst b/doc/develop/codingstyle.rst
new file mode 100644
index 000000000000..bbeca42e656b
--- /dev/null
+++ b/doc/develop/codingstyle.rst
@@ -0,0 +1,255 @@ 
+.. SPDX-License-Identifier: GPL-2.0+:
+
+U-Boot Coding Style
+===================
+
+The following Coding Style requirements shall be mandatory for all code contributed to
+the U-Boot project.
+
+Exceptions are only allowed if code from other projects is integrated with no
+or only minimal changes.
+
+The following rules apply:
+
+* All contributions to U-Boot should conform to the `Linux kernel
+  coding style <https://www.kernel.org/doc/html/latest/process/coding-style.html>`_
+  and the `Lindent script <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Lindent>`_.
+  * The exception for net files to the `multi-line comment
+  <https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting>`_
+  applies only to Linux, not to U-Boot. Only large hunks which are copied
+  unchanged from Linux may retain that comment format.
+
+* Use patman to send your patches (``tools/patman/patman -H`` for full
+  instructions). With a few tags in your commits this will check your patches
+  and take care of emailing them.
+
+* If you don't use patman, make sure to run ``scripts/checkpatch.pl``. For
+  more information, read :doc:`checkpatch`. Note that this should be done
+  *before* posting on the mailing list!
+
+* Source files originating from different projects (for example the MTD
+  subsystem or the hush shell code from the BusyBox project) may, after
+  careful consideration, be exempted from these rules. For such files, the
+  original coding style may be kept to ease subsequent migration to newer
+  versions of those sources.
+
+* Please note that U-Boot is implemented in C (and to some small parts in
+  Assembler); no C++ is used, so please do not use C++ style comments (//) in
+  your code.
+
+  * The sole exception here is for SPDX tags in some files (checkpatch.pl will warn you).
+
+* Please also stick to the following formatting rules:
+
+  * Remove any trailing white space
+
+  * Use TAB characters for indentation and vertical alignment, not spaces
+
+  * Make sure NOT to use DOS ``\r\n`` line feeds
+
+  * Do not add more than 2 consecutive empty lines to source files
+
+  * Do not add trailing empty lines to source files
+
+  * Using the option ``git config --global color.diff auto`` will help to
+    visually see whitespace problems in ``diff`` output from ``git``.
+
+  * In Emacs one can use ``=M-x whitespace-global-mode=`` to get visual
+    feedback on the nasty details. ``=M-x whitespace-cleanup=`` does The Right
+    Thing (tm)
+
+Submissions of new code or patches that do not conform to these requirements
+shall be rejected with a request to reformat the changes.
+
+U-Boot Code Documentation
+-------------------------
+
+U-Boot adopted the kernel-doc annotation style, this is the only exception from
+multi-line comment rule of Coding Style. While not mandatory, adding
+documentation is strongly advised. The Linux kernel `kernel-doc
+<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html>`_
+documentation applies with no changes.
+
+Use structures for I/O access
+-----------------------------
+
+U-Boot typically uses a C structure to map out the registers in an I/O region,
+rather than offsets. The reasons for this are:
+
+* It dissociates the register location (offset) from the register type, which
+  means the developer has to make sure the type is right for each access,
+  whereas with the struct method, this is checked by the compiler;
+
+* It avoids actually writing all offsets, which is (more) error- prone;
+
+* It allows for better compile time sanity-checking of values we write to registers.
+
+Some reasons why you might not use C structures:
+
+* Where the registers appear at different offsets in different hardware
+  revisions supported by the same driver
+
+* Where the driver only uses a small subset of registers and it is not worth
+  defining a struct to cover them all, with large empty regions
+
+* Where the offset of a register might be hard to figure out when buried a long
+  way down a structure, possibly with embedded sub-structures
+
+* This may need to change to the kernel model if we allow for more run-time
+  detection of what drivers are appropriate for what we're running on.
+
+Please use check_member() to verify that your structure is the expected size,
+or that particular members appear at the right offset.
+
+Include files
+-------------
+
+You should follow this ordering in U-Boot. The common.h header (which is going
+away at some point) should always be first, followed by other headers in order,
+then headers with directories, then local files:
+
+.. code-block:: C
+
+   #include <common.h>
+   #include <bootstage.h>
+   #include <dm.h>
+   #include <others.h>
+   #include <asm/...>
+   #include <arm/arch/...>
+   #include <dm/device_compat/.h>
+   #include <linux/...>
+   #include "local.h"
+
+Within that order, sort your includes.
+
+It is important to include common.h first since it provides basic features used
+by most files, e.g. CONFIG options.
+
+For files that need to be compiled for the host (e.g. tools), you need to use
+``#ifndef USE_HOSTCC`` to avoid including common.h since it includes a lot of
+internal U-Boot things. See common/image.c for an example.
+
+If your file uses driver model, include <dm.h> in the C file. Do not include
+dm.h in a header file. Try to use forward declarations (e.g. ``struct
+udevice``) instead.
+
+Filenames
+---------
+
+For .c and .h files try to use underscore rather than hyphen unless you want
+the file to stand out (e.g. driver-model uclasses should be named xxx-uclass.h.
+Avoid upper case and keep the names fairly short.
+
+Function and struct comments
+----------------------------
+
+Non-trivial functions should have a comment which describes what they do. If it
+is an exported function, put the comment in the header file so the API is in
+one place. If it is a static function, put it in the C file.
+
+If the function returns errors, mention that and list the different errors that
+are returned. If it is merely passing errors back from a function it calls,
+then you can skip that.
+
+See `here
+<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation>`_
+for style.
+
+Driver model
+------------
+
+When declaring a device, try to use ``struct udevice *dev``, i.e. ``dev`` as the name:
+
+.. code-block:: C
+
+   struct udevice *dev;
+
+Use ``ret`` as the return value:
+
+.. code-block:: C
+
+   struct udevice *dev;
+   int ret;
+
+   ret = uclass_first_device_err(UCLASS_ACPI_PMC, &dev);
+   if (ret)
+           return log_msg_ret("pmc", dev);
+
+Consider using log_ret() or log_msg_ret() to return a value (see above).
+
+Add a ``p`` suffix on return arguments:
+
+.. code-block:: C
+
+   int dm_pci_find_class(uint find_class, int index, struct udevice **devp)
+   {
+   ...
+           *devp = dev;
+
+           return 0;
+   }
+
+There are standard variable names that you should use in drivers:
+
+* ``struct xxx_priv`` and ``priv`` for dev_get_priv()
+
+* ``struct xxx_plat`` and ``plat`` for dev_get_platdata()
+
+For example:
+
+.. code-block:: C
+
+   struct simple_bus_plat {
+      u32 base;
+      u32 size;
+      u32 target;
+   };
+
+   /* Davinci MMC board definitions */
+   struct davinci_mmc_priv {
+      struct davinci_mmc_regs *reg_base;   /* Register base address */
+      uint input_clk;      /* Input clock to MMC controller */
+      struct gpio_desc cd_gpio;       /* Card Detect GPIO */
+      struct gpio_desc wp_gpio;       /* Write Protect GPIO */
+   };
+
+      struct rcar_gpio_priv *priv = dev_get_priv(dev);
+
+      struct pl01x_serial_platdata *plat = dev_get_platdata(dev);
+
+Other
+-----
+
+Some minor things:
+
+* Put a blank line before the last ``return`` in a function unless it is the only line:
+
+.. code-block:: C
+
+   struct udevice *pci_get_controller(struct udevice *dev)
+   {
+      while (device_is_on_pci_bus(dev))
+         dev = dev->parent;
+
+      return dev;
+   }
+
+Tests
+-----
+
+Please add tests when you add code. Please change or expand tests when you change code.
+
+Run the tests with::
+
+   make check
+   make qcheck   (skips some tests)
+
+Python tests are in test/py/tests - see the docs in test/py for info.
+
+Try to write your tests in C if you can. For example, tests to check a command
+will be much faster (10-100x or more) if they can directly call run_command()
+and ut_check_console_line() instead of using Python to send commands over a
+pipe to U-Boot.
+
+Tests run all supported CI systems (gitlab, travis, azure) using scripts in the
+root of the U-Boot tree.
diff --git a/doc/develop/index.rst b/doc/develop/index.rst
index fe3564a9fbf4..dde47994c71a 100644
--- a/doc/develop/index.rst
+++ b/doc/develop/index.rst
@@ -3,6 +3,14 @@ 
 Develop U-Boot
 ==============
 
+General
+-------
+
+.. toctree::
+   :maxdepth: 1
+
+   codingstyle
+
 Implementation
 --------------