Hi Tom, On Wed, 27 Nov 2024 at 15:11, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 27 Nov 2024 at 07:15, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote: > > > > Hi, > > > > > > > > On Tue, 26 Nov 2024 at 16:32, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote: > > > > > > On 11/26/24 23:33, Tom Rini wrote: > > > > > > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote: > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini <tr...@konsulko.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote: > > > > > > > > > > Hi Heinrich, > > > > > > > > > > > > > > > > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt > > > > > > > > > > <xypron.g...@gmx.de> wrote: > > > > > > > > > > > > > > > > > > > > > > On 25.11.24 21:44, Simon Glass wrote: > > > > > > > > > > > > Exported functions should be documented in the header > > > > > > > > > > > > file, not the > > > > > > > > > > > > implementation. We tend to make such updates on a > > > > > > > > > > > > piecemeal basis to > > > > > > > > > > > > avoid a 'flag day'. Move some comments related to > > > > > > > > > > > > memory allocation to > > > > > > > > > > > > follow the convention. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > > > > > > > > > > > > > > > > Please, have a look at this line in doc/ > > > > > > > > > > > > > > > > > > > > > > doc/api/efi.rst:78: > > > > > > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c > > > > > > > > > > > > > > > > > > > > Hmm, we should not add C files as then we end up with all > > > > > > > > > > sorts of > > > > > > > > > > internal functions, like checksum(). The help is a bit of a > > > > > > > > > > mess on > > > > > > > > > > that page IMO and it could use an index at the top or side. > > > > > > > > > > > > > > > > > > Why? Looking at the linux kernel (where we borrow this > > > > > > > > > framework from > > > > > > > > > and lots of developers expect to follow that style) it's > > > > > > > > > extremely > > > > > > > > > common to kernel-doc C files. I don't see why documenting > > > > > > > > > internal > > > > > > > > > functions (which should be clear as being internal) is a > > > > > > > > > problem. > > > > > > > > > > > > > > > > It depends what you mean by docs. U-Boot puts the documentation > > > > > > > > for > > > > > > > > exported functions in header files. > > > > > > > > > > > > > > I'm sorry, that's a circular definition. And no, there is not a > > > > > > > hard and > > > > > > > fast rule that we only put kernel-doc style comments in header > > > > > > > files > > > > > > > (which are consumed by other files and tooling to generate > > > > > > > documentation). > > > > > > > > > > > > > > > Putting docs in C files clutters the docs with internal > > > > > > > > implementations, when most people just want to see the API. The > > > > > > > > way > > > > > > > > U-Boot does things, it is easy to see the internal > > > > > > > > (implementation) > > > > > > > > docs by looking at the C code and the external functions by > > > > > > > > looking at > > > > > > > > the header files. > > > > > > > > > > > > > > Or it makes things harder to understand because the human readable > > > > > > > documentation for a given function is separate from the file the > > > > > > > implements the function. This is possibly why in the linux > > > > > > > kernel, where > > > > > > > we borrow "kernel-doc" from, documentation is frequently in the C > > > > > > > file > > > > > > > which is then included in Documentation files (the type you > > > > > > > objected to > > > > > > > in the start of this patch) which include additional > > > > > > > human-readable > > > > > > > explanation of the intention and usage of the API in question. > > > > > > > > > > > > > > > > > > > For static functions we will continue to have function > > > > > > documentation in > > > > > > the C files and not in the headers. I personally never looked up > > > > > > the API > > > > > > documentation in the HTML docs but always in the source. > > > > > > > > Same here. > > > > > > > > > > > > > > > > I am not opposed against moving the EFI function documentation into > > > > > > the > > > > > > header files. But we should update doc/develop/efi.rst accordingly > > > > > > in > > > > > > the same patch series. This is missing here. > > > > > > > > OK > > > > > > > > > > > > > > My point is that the linux kernel is about 60% C files and 40% header > > > > > files being kernel-doc'd in to documentation files and so "U-Boot > > > > > doesn't put documentation in to C files" seems like the wrong > > > > > approach. > > > > > > > > U-Boot tries to document functions in header files. At the risk of > > > > making a blanket statement with thousands of counter-examples, that is > > > > not so much the case in Linux. > > > > > > > > To a large extent, people developing features in U-Boot rely on the > > > > header files to see what functions they can call. With the > > > > documentation there, they often don't need to look at the source. I > > > > certainly don't, unless the header-file comment is inadequate. > > > > > > I think this is a case of what you do, rather than what everyone else > > > does. > > > > That's not my reading of the source tree. > > So, if we call this point 1 for a moment. > > > The file we are talking about has some comments in the header and some > > in the source file. When you use an lsp client, it doesn't know what > > to do. > > Sounds like a bad client? Because again, we should be following the > kernel here. > > > > We want to be more like the linux kernel here, rather than less > > > like the linux kernel, given the number of kernel people also working > > > here. If Heinrich is fine with re-arranging everything, OK. But I think > > > documentation with the code makes it less likely to get out of date, > > > both in terms of intentional changes and bug fixes. > > > > I am not suggesting separating documentation from code. I agree that > > makes no sense. What I have consistently done (and asked for in > > reviews) is: > > > > - exported functions are documented next to their prototype in the header > > - static functions are documented next to their implementation > > So going back to point 1 now, yes. This is what I'm saying. Your > preference, expressed through reviews and telling people to change when > they do it other ways, is where the current situation comes from. > > > This makes it easy to read the header file and see what the module is > > doing, without having to dive into the implementation, which could be > > 1000 lines or more. It also makes for more useful kerneldoc, IMO, > > since we are showing functions which are exported and therefore more > > useful for development. Adding the entire C code of U-Boot into > > kerneldoc seems like a bad use of space. > > Um, I don't follow you here. If the code doesn't have kerneldoc > comments, it's not included. I just made a silly test with a change to > include/clk.h. It's not included in the output. And "the code might be > so big it needs refactoring" is not a good excuse.
Yes, I see that static functions don't appear by default, so adding C files doesn't necessarily result in functions appearing in the doc output. > > The kernel is inconsistent here. The first example I went to look for of > something that uses both C and header files in a document was > Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and > include/linux/pstore_blk.h are kernel-doc'd in and the header describes > a public function of the C file while the C file describes other > functions too. Yes, pstore docs seems like a nice way to do things, for docs. For people working on the code base, I strongly believe that exported functions and structs are the main way to understand how to use a subsystem, so their docs should be in the header file. Regards, Simon
