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