Message ID | 20200311192823.16213-2-peter@bikeshed.quignogs.org.uk |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | Convert text to ReST list and remove doc build warnings | expand |
On Wed, Mar 11, 2020 at 07:28:23PM +0000, peter@bikeshed.quignogs.org.uk wrote: > From: Peter Lister <peter@bikeshed.quignogs.org.uk> > > Added line breaks and blank lines to separate list items and escaped end-of-line > colons. > > This removes these warnings from doc build... > > ./drivers/net/phy/sfp-bus.c:579: WARNING: Unexpected indentation. > ./drivers/net/phy/sfp-bus.c:619: WARNING: Unexpected indentation. > > Signed-off-by: Peter Lister <peter@bikeshed.quignogs.org.uk> > --- > drivers/net/phy/sfp-bus.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c > index d949ea7b4f8c..df1c66df830f 100644 > --- a/drivers/net/phy/sfp-bus.c > +++ b/drivers/net/phy/sfp-bus.c > @@ -572,12 +572,18 @@ static void sfp_upstream_clear(struct sfp_bus *bus) > * the sfp_bus structure, incrementing its reference count. This must > * be put via sfp_bus_put() when done. > * > - * Returns: on success, a pointer to the sfp_bus structure, > + * Returns\: > + * > + * on success, a pointer to the sfp_bus structure, > * %NULL if no SFP is specified, > + * > * on failure, an error pointer value: > + * > * corresponding to the errors detailed for > * fwnode_property_get_reference_args(). > + * > * %-ENOMEM if we failed to allocate the bus. > + * > * an error from the upstream's connect_phy() method. Is this really necessary? This seems to be rather OTT, and makes the comment way too big IMHO.
Hello Russell, > Is this really necessary? This seems to be rather OTT, and makes the > comment way too big IMHO. The existing form definitely gets the formatted output wrong (I'll send you a screen grab if you like) and causes doc build warnings. So, yes, it needs fixing. ReST makes free with blank lines round blocks and list entries, and I agree this makes for inelegant source annotation. I tried to retain the wording unchanged and present the description as just "whitespace" changes to make a list in the formatted output - as close as I could to what the author appears to intend. If you're OK with a mild rewrite of the return value description, e.g. as two sentences (On success: p; q. On failure: x; y; z.), then we can fix the doc build and have terser source comments and a happier kerneldoc. All the best, Peter Lister
On Wed, Mar 11, 2020 at 10:21:41PM +0000, Peter Lister wrote: > Hello Russell, > > > Is this really necessary? This seems to be rather OTT, and makes the > > comment way too big IMHO. > > The existing form definitely gets the formatted output wrong (I'll send you > a screen grab if you like) and causes doc build warnings. So, yes, it needs > fixing. > > ReST makes free with blank lines round blocks and list entries, and I agree > this makes for inelegant source annotation. I tried to retain the wording > unchanged and present the description as just "whitespace" changes to make a > list in the formatted output - as close as I could to what the author > appears to intend. > > If you're OK with a mild rewrite of the return value description, e.g. as > two sentences (On success: p; q. On failure: x; y; z.), then we can fix the > doc build and have terser source comments and a happier kerneldoc. I think it's more important that the documentation interferes to a minimal degree with the code in the file, so please rewrite if it improves it. (btw, I'm the author.) Thanks.
On Wed, Mar 11, 2020 at 07:28:23PM +0000, peter@bikeshed.quignogs.org.uk wrote: > Added line breaks and blank lines to separate list items and escaped end-of-line > colons. > > This removes these warnings from doc build... > > ./drivers/net/phy/sfp-bus.c:579: WARNING: Unexpected indentation. > ./drivers/net/phy/sfp-bus.c:619: WARNING: Unexpected indentation. I'm all in favour of removing warnings, but I think you've fixed this the wrong way. > @@ -572,12 +572,18 @@ static void sfp_upstream_clear(struct sfp_bus *bus) > * the sfp_bus structure, incrementing its reference count. This must > * be put via sfp_bus_put() when done. > * > - * Returns: on success, a pointer to the sfp_bus structure, > + * Returns\: This should be Return: (not Returns:) and marks a section header, not the beginning of the list. See the "Return values" section in Documentation/doc-guide/kernel-doc.rst > + * > + * on success, a pointer to the sfp_bus structure, > * %NULL if no SFP is specified, > + * > * on failure, an error pointer value: > + * > * corresponding to the errors detailed for > * fwnode_property_get_reference_args(). > + * > * %-ENOMEM if we failed to allocate the bus. > + * > * an error from the upstream's connect_phy() method. Seems to me this should use the " * * VALUE - Description" format described in the document I mentioned above.
diff --git a/drivers/net/phy/sfp-bus.c b/drivers/net/phy/sfp-bus.c index d949ea7b4f8c..df1c66df830f 100644 --- a/drivers/net/phy/sfp-bus.c +++ b/drivers/net/phy/sfp-bus.c @@ -572,12 +572,18 @@ static void sfp_upstream_clear(struct sfp_bus *bus) * the sfp_bus structure, incrementing its reference count. This must * be put via sfp_bus_put() when done. * - * Returns: on success, a pointer to the sfp_bus structure, + * Returns\: + * + * on success, a pointer to the sfp_bus structure, * %NULL if no SFP is specified, + * * on failure, an error pointer value: + * * corresponding to the errors detailed for * fwnode_property_get_reference_args(). + * * %-ENOMEM if we failed to allocate the bus. + * * an error from the upstream's connect_phy() method. */ struct sfp_bus *sfp_bus_find_fwnode(struct fwnode_handle *fwnode) @@ -612,12 +618,18 @@ EXPORT_SYMBOL_GPL(sfp_bus_find_fwnode); * the SFP bus using sfp_register_upstream(). This takes a reference on the * bus, so it is safe to put the bus after this call. * - * Returns: on success, a pointer to the sfp_bus structure, + * Returns\: + * + * on success, a pointer to the sfp_bus structure, * %NULL if no SFP is specified, + * * on failure, an error pointer value: + * * corresponding to the errors detailed for * fwnode_property_get_reference_args(). + * * %-ENOMEM if we failed to allocate the bus. + * * an error from the upstream's connect_phy() method. */ int sfp_bus_add_upstream(struct sfp_bus *bus, void *upstream,