Re: [PATCH v12 11/17] docs: counter: Document character device interface

From: David Lechner <david@lechnology.com>
To: William Breathitt Gray <vilhelm.gray@gmail.com>, jic23@kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com, kernel@pengutronix.de,
	a.fatoum@pengutronix.de, kamel.bouhara@bootlin.com,
	gwendal@chromium.org, alexandre.belloni@bootlin.com,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, syednwaris@gmail.com,
	patrick.havelange@essensium.com, fabrice.gasnier@st.com,
	mcoquelin.stm32@gmail.com, alexandre.torgue@st.com,
	o.rempel@pengutronix.de, jarkko.nikula@linux.intel.com
Subject: Re: [PATCH v12 11/17] docs: counter: Document character device interface
Date: Sat, 10 Jul 2021 15:15:06 -0500	[thread overview]
Message-ID: <10ae3615-1fe4-0dce-5aa6-e865de2655a7@lechnology.com> (raw)
In-Reply-To: <186e7a1cd7dc822cc9290683b463c3e675959e1a.1625471640.git.vilhelm.gray@gmail.com>

On 7/5/21 3:18 AM, William Breathitt Gray wrote:
> This patch adds high-level documentation about the Counter subsystem
> character device interface.
> 
> Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> ---
>   Documentation/driver-api/generic-counter.rst  | 185 ++++++++++++++----
>   .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>   2 files changed, 145 insertions(+), 41 deletions(-)
> 
> diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst
> index f6397218aa4c..62a702e7f994 100644
> --- a/Documentation/driver-api/generic-counter.rst
> +++ b/Documentation/driver-api/generic-counter.rst

> +
> +Counter Character Device
> +========================
> +
> +Counter character device nodes are created under the ``/dev`` directory
> +as ``counterX``, where ``X`` is the respective counter device id.
> +Defines for the standard Counter data types are exposed via the
> +userspace ``include/uapi/linux/counter.h`` file.
> +
> +Counter events
> +--------------
> +Counter device drivers can support Counter events by utilizing the
> +``counter_push_event`` function::
> +
> +        void counter_push_event(struct counter_device *const counter, const u8 event,
> +                                const u8 channel);
> +
> +The event id is specified by the ``event`` parameter; the event channel
> +id is specified by the ``channel`` parameter. When this function is
> +called, the Counter data associated with the respective event is
> +gathered, and a ``struct counter_event`` is generated for each datum and
> +pushed to userspace.
> +
> +Counter events can be configured by users to report various Counter
> +data of interest. This can be conceptualized as a list of Counter
> +component read calls to perform. For example::

Won't the :: here make this appear as text instead of an HTML table?

(might need to change ~~~ to --- [top line] and === [middle line])

> +
> +        +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
> +        | COUNTER_EVENT_OVERFLOW | COUNTER_EVENT_INDEX    |
> +        +~~~~~~~~~~~~~~~~~~~~~~~~+~~~~~~~~~~~~~~~~~~~~~~~~+
> +        | Channel 0              | Channel 0              |
> +        +------------------------+------------------------+
> +        | * Count 0              | * Signal 0             |
> +        | * Count 1              | * Signal 0 Extension 0 |
> +        | * Signal 3             | * Extension 4          |
> +        | * Count 4 Extension 2  +------------------------+
> +        | * Signal 5 Extension 0 | Channel 1              |
> +        |                        +------------------------+
> +        |                        | * Signal 4             |
> +        |                        | * Signal 4 Extension 0 |
> +        |                        | * Count 7              |
> +        +------------------------+------------------------+
> +
> +When ``counter_push_event(counter, COUNTER_EVENT_INDEX, 1)`` is called
> +for example, it will go down the list for the ``COUNTER_EVENT_INDEX``
> +event channel 1 and execute the read callbacks for Signal 4, Signal 4
> +Extension 0, and Count 4 -- the data returned for each is pushed to a
> +kfifo as a ``struct counter_event``, which userspace can retrieve via a
> +standard read operation on the respective character device node.
> +
> +Userspace
> +---------
> +Userspace applications can configure Counter events via ioctl operations
> +on the Counter character device node. There following ioctl codes are
> +supported and provided by the ``linux/counter.h`` userspace header file:
> +
> +* COUNTER_ADD_WATCH_IOCTL:
> +  Queues a Counter watch for the specified event. The queued watches
> +  will not be applied until ``COUNTER_ENABLE_EVENTS_IOCTL`` is called.
> +
> +* COUNTER_ENABLE_EVENTS_IOCTL:
> +  Enables monitoring the events specified by the Counter watches that
> +  were queued by ``COUNTER_ADD_WATCH_IOCTL``. If events are already
> +  enabled, the new set of watches replaces the old one. Calling this
> +  ioctl also has the effect of clearing the queue of watches added by
> +  ``COUNTER_ADD_WATCH_IOCTL``.
> +
> +* COUNTER_DISABLE_EVENTS_IOCTL:
> +  Stops monitoring the previously enabled events.

I wouldn't mind seeing more of this documentation in the actual header
file and just referenced here with :c:macro:`COUNTER_ADD_WATCH_IOCTL`

> +
> +To configure events to gather Counter data, users first populate a
> +``struct counter_watch`` with the relevant event id, event channel id,
> +and the information for the desired Counter component from which to
> +read, and then pass it via the ``COUNTER_ADD_WATCH_IOCTL`` ioctl
> +command.
> +
> +Note that an event can be watched without gathering Counter data by
> +setting the ``component.type`` member equal to
> +``COUNTER_COMPONENT_NONE``. With this configuration the Counter
> +character device will simply populate the event timestamps for those
> +respective ``struct counter_event`` elements and ignore the component
> +value.

To make sure I am understanding this correctly, scope + parent
determines this part of the path:

	/sys/.../counterX/<scope><parent>/<component>

Or in the case that scope == COUNTER_SCOPE_DEVICE then parent
is not applicable:

	/sys/.../counterX/<component>

I suggested parent_id instead of parent earlier, but maybe
scope_id would be a better name? (Or rename scope to parent_type?)

> +
> +The ``COUNTER_ADD_WATCH_IOCTL`` command will buffer these Counter
> +watches. When ready, the ``COUNTER_ENABLE_EVENTS_IOCTL`` ioctl command
> +may be used to activate these Counter watches.
> +
> +Userspace applications can then execute a ``read`` operation (optionally
> +calling ``poll`` first) on the Counter character device node to retrieve
> +``struct counter_event`` elements with the desired data.