Skip to content

MI: Add kernel-doc documentation for MI API#399

Merged
igaw merged 4 commits intolinux-nvme:masterfrom
CodeConstruct:mi-kdoc
Jun 23, 2022
Merged

MI: Add kernel-doc documentation for MI API#399
igaw merged 4 commits intolinux-nvme:masterfrom
CodeConstruct:mi-kdoc

Conversation

@jk-ozlabs
Copy link
Copy Markdown
Collaborator

This change adds kernel-doc documentation for the new definitions in mi.h, and a file-level header for general API information.

We also fix a misnamed argument found while writing the docs, added as a small prerequisite patch.

Let me know if I've missed any conventions from the core libnvme docs.

This is a starting controller ID, not a starting port ID, fix the name
to suit.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 23, 2022

Codecov Report

Merging #399 (9ff9e34) into master (2b122ad) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #399   +/-   ##
======================================
  Coverage    7.61%   7.61%           
======================================
  Files          29      29           
  Lines        4414    4414           
  Branches      828     828           
======================================
  Hits          336     336           
  Misses       3937    3937           
  Partials      141     141           
Impacted Files Coverage Δ
src/nvme/mi.c 33.72% <0.00%> (ø)
src/nvme/mi.h 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b122ad...9ff9e34. Read the comment docs.

This change adds proper kdoc-style documentation to the mi.h
definitions, and includes mi.h in the docs builds.

No functional change, just comments.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Just pushed an update, with a new mi doc base, as doc/mi.rst.in. However, I'm not 10% clear whether I need both the .rst.in and the .rst, so I've only inuded the former for now, as they just seem to be duplicates. Let me know if you need both though.

@jk-ozlabs jk-ozlabs changed the title MI: Add kernel-doc documentaiton for MI API MI: Add kernel-doc documentation for MI API Jun 23, 2022
Add a base `mi.rst.in` document, for some background information on the
NVMe-MI interface, alongside the new mi API doc.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Just a bunch of amendments for typos; sorry for the noise, we should be good now!

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Wow! Thanks a lot for this.

Just pushed an update, with a new mi doc base, as doc/mi.rst.in. However, I'm not 10% clear whether I need both the .rst.in and the .rst, so I've only inuded the former for now, as they just seem to be duplicates. Let me know if you need both though.

The .rst.in source file is for .rst. We have a configure_file step in the meson rules so that we are able to update any variables in the text, e.g. VERSION. Not completely sure if we actually still need the .rst file. Have to look into it but that is for later.

substs = configuration_data()
substs.set('NAME',    meson.project_name())
substs.set('VERSION', meson.project_version())
substs.set('LICENSE', meson.project_license()[0])

foreach file : sphinx_sources
  static_sources += configure_file(input: file + '.in',
                                   output: file,
                                   configuration: substs)

Is this PR independent or is it ontop of our PR #396 ?

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Not completely sure if we actually still need the .rst file

I've only added the mi.rst.in here, and everything seems to work OK. However, I have added the references to both index.rst and index.rst.in because I'm being cautious about what might be required :)

Is this PR independent or is it ontop of our PR #396 ?

This is independent at the moment, so is based directly on the master branch. I have an update to #396 which will add the scan documentation to the new mi.rst.in (added by this change) as you've requested in that PR.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Ok, let's merge this first in this case.

@igaw igaw merged commit b7fe911 into linux-nvme:master Jun 23, 2022
@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

It looks like these changes didn't make to readthedocs site:

https://libnvme.readthedocs.io/en/latest/index.html

although it says the latest build was against this commit. Ah, this might be the reason
why we have the .rst file also in the tree.

https://readthedocs.org/api/v2/build/17255626.txt

@jk-ozlabs
Copy link
Copy Markdown
Collaborator Author

Yep, that would seem to be the issue:

doc/index.rst:9: WARNING: toctree contains reference to nonexisting document 'mi'

I'll send an update.

@igaw
Copy link
Copy Markdown
Collaborator

igaw commented Jun 23, 2022

Maybe it is possible to trick readthedocs to run the .rst.in to .rst step. Then we could drop these files from the source tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants