Message-ID: <20160823113016.2d131a02@vento.lan>
Date:   Tue, 23 Aug 2016 11:30:16 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Jonathan Corbet <corbet@....net>
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jani Nikula <jani.nikula@...el.com>
Subject: Re: [PATCH 2/3] docs: split up the driver book

Em Mon, 22 Aug 2016 14:57:42 -0600
Jonathan Corbet <corbet@....net> escreveu:

> We don't need to keep it as a single large file anymore; split it up so
> that it is easier to manage and the individual sections can be read
> directly as plain files.
> 
> Cc: Jani Nikula <jani.nikula@...el.com>
> Cc: Mauro Carvalho Chehab <mchehab@...pensource.com>
> Signed-off-by: Jonathan Corbet <corbet@....net>
> ---
>  Documentation/driver-api/basics.rst            | 120 +++++

Hi Jon,

I noticed several issues on the converted document. Just commenting
a few of them, as they all follow a pattern: kernel-doc markups
needs review during the conversion to RST, because, unfortunately,
the conversion is not transparent, as we would want to.

So, I'm pointing a few issues I noticed below.

> Documentation/driver-api/basics.rst            | 120 +++++

At kernel/kthread.c, on the documentation for:

	struct task_struct * kthread_create_on_node(int (*threadfn) (void *data, void * data, int node, const char namefmt[], ...)

The original description is:

	 * If thread is going to be bound on a particular cpu, give its node
	 * in @node, to get NUMA affinity for kthread stack, or else give NUMA_NO_NODE.
	 * When woken, the thread will run @threadfn() with @data as its
	 * argument. @threadfn() can either call do_exit() directly if it is a
	 * standalone thread for which no one will call kthread_stop(), or
	 * return when 'kthread_should_stop()' is true (which means
	 * kthread_stop() has been called).  The return value should be zero
	 * or a negative error number; it will be passed to kthread_stop().

On the output text, you'll see two places with "@:c:func:threadfn()".

The problem here is that threadfn() is a function argument. While this
used to work with DocBooks, now with Sphinx this is not handled well.

I got some other similar cases on media. There, I opted to just remove
the () on some places, or to replace it by \(\) to avoid kernel-doc
to do the wrong thing. See changeset 564aaf69208d ("[media] doc-rst: 
add some needed escape codes").

We could, instead, teach kernel-doc to be smarter, but I'm afraid
that there will always be border cases that it won't cover.

The same problem happened with:
	void kmsg_dump_rewind(struct kmsg_dumper * dumper)

Also, you should notice that it added several references to
kthread_create(), with is actually a define:

	include/linux/kthread.h:#define kthread_create(threadfn, data, namefmt, arg...) \

It probably makes sense to add some markup for kernel-doc to parse it.

>  Documentation/driver-api/drivers.rst           | 654 -------------------------

At struct device description:

	bool:1 offline
	    Set after successful invocation of bus type’s .:c:func:offline().

At request_firmware_nowait() description, you'all also see:

	 ->:c:func:probe() 

(won't mention about other occurrences of c: - you got the idea)

>  Documentation/driver-api/message-based.rst     |  30 ++

You should probably change NOTES by:

	.. note::

Like on this changeset:
	f58cad224796 [media] media-entry.h: Fix a note markup


The description for:

	 int KickStart(MPT_ADAPTER * ioc, int force, int sleepFlag)

Looked weird on my eyes. The original kernel-nano tag is:

/**
 *	KickStart - Perform hard reset of MPT adapter.
 *	@ioc: Pointer to MPT_ADAPTER structure
 *	@force: Force hard reset
 *	@sleepFlag: Specifies whether the process can sleep
 *
 *	This routine places MPT adapter in diagnostic mode via the
 *	WriteSequence register, and then performs a hard reset of adapter
 *	via the Diagnostic register.
 *
 *	Inputs:   sleepflag - CAN_SLEEP (non-interrupt thread)
 *			or NO_SLEEP (interrupt thread, use mdelay)
 *		  force - 1 if doorbell active, board fault state
 *				board operational, IOC_RECOVERY or
 *				IOC_BRINGUP and there is an alt_ioc.
 *			  0 else
 *
 *	Returns:
 *		 1 - hard reset, READY
 *		 0 - no reset due to History bit, READY
 *		-1 - no reset due to History bit but not READY
 *		     OR reset but failed to come READY
 *		-2 - no reset, could not enter DIAG mode
 *		-3 - reset but bad FW bit
 */
static int
KickStart(MPT_ADAPTER *ioc, int force, int sleepFlag)

The first input like:

 *	Inputs:   sleepflag - CAN_SLEEP (non-interrupt thread)

Is internally converted to a bold output like:
	*Inputs sleepflag - CAN_SLEEP (non-interrupt thread)*

And the remaining arguments will be mangled.

At returns arguments, for example, it should be changed to something
like (untested):

 * Returns:
 *
 * - 1 - hard reset, READY
 *
 * - 0 - no reset due to History bit, READY
 *
 * - 1 - no reset due to History bit but not READY
 *       OR reset but failed to come READY
 *
 * - -2 - no reset, could not enter DIAG mode
 *
 * - -3 - reset but bad FW bit

So, basically, all kernel-doc tags should be reviewed for continuation
lines.

...

> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> new file mode 100644
> index 000000000000..b50c41011e47
> --- /dev/null
> +++ b/Documentation/driver-api/index.rst
> @@ -0,0 +1,24 @@
> +========================================
> +The Linux driver implementer's API guide
> +========================================
> +
> +The kernel offers a wide variety of interfaces to support the development
> +of device drivers.  This document is an only somewhat organized collection
> +of some of those interfaces — it will hopefully get better over time!  The
> +available subsections can be seen below.
> +
> +.. class:: toc-title
> +
> +	   Table of contents
> +
> +.. toctree::
> +   :maxdepth: 2

I would add here a
	:numbered:

This way, the entire section will be numbered, and you can remove the
manual numbers from the sections from 
Documentation/driver-api/serial-interfaces.rst (patch 3/3).


> +
> +   basics
> +   infrastructure
> +   message-based
> +   sound
> +   frame-buffer
> +   input
> +   serial-interfaces
> +   miscellaneous


Regards,
Mauro

Powered by blists - more mailing lists