| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 16 Jul 2015 04:24:17 -0600
From: Jonathan Corbet <corbet@....net>
To: Daniel Baluta <daniel.baluta@...el.com>
Cc: jic23@...nel.org, pmeerw@...erw.net, knaack.h@....de,
lars@...afoo.de, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org
Subject: Re: [PATCH] DocBook: Add initial documentation for IIO
On Wed, 8 Jul 2015 15:04:48 +0300
Daniel Baluta <daniel.baluta@...el.com> wrote:
> This is intended to help developers faster find their way
> inside the Industrial I/O core and reduce time spent on IIO
> drivers development.
Seems like good stuff to have, sorry it's taken me so long to have a look
at it. Any other IIO folks want to send comments or an ack?
A few comments of mine below...
[...]
> diff --git a/Documentation/DocBook/iio.tmpl b/Documentation/DocBook/iio.tmpl
> new file mode 100644
> index 0000000..417bb26
> --- /dev/null
> +++ b/Documentation/DocBook/iio.tmpl
> @@ -0,0 +1,593 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE book PUBLIC "-//OASIS//DTD DocBook XML V4.1.2//EN"
> + "http://www.oasis-open.org/docbook/xml/4.1.2/docbookx.dtd" []>
> +
> +<book id="iioid">
> + <bookinfo>
> + <title>Industrial I/O driver developer's guide </title>
> +
> + <authorgroup>
> + <author>
> + <firstname>Daniel</firstname>
> + <surname>Baluta</surname>
> + <affiliation>
> + <address>
> + <email>daniel.baluta@...el.com</email>
> + </address>
> + </affiliation>
> + </author>
> + </authorgroup>
> +
> + <copyright>
> + <year>2015</year>
> + <holder>Intel Corporation</holder>
> + </copyright>
> +
> + <legalnotice>
> + <para>
> + This documentation is free software; you can redistribute
> + it and/or modify it under the terms of the GNU General Public
> + License version 2.
> + </para>
> +
> + <para>
> + This program is distributed in the hope that it will be
> + useful, but WITHOUT ANY WARRANTY; without even the implied
> + warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + For more details see the file COPYING in the source
> + distribution of Linux.
Do we really need this paragraph? It's not even a "program" by some views,
at least.
> + </para>
> + </legalnotice>
> + </bookinfo>
> +
> + <toc></toc>
> +
> + <chapter id="intro">
> + <title>Introduction</title>
> + <para>
> + The main purpose of the Industrial I/O subsystem (IIO) is to provide
> + support for devices that in some sense are analog to digital converts
s/converts/converters/
[...]
> + <sect2 id="iioattr"> <title> IIO device sysfs interface </title>
> + <para>
> + Attributes are sysfs files used to expose chip info and also allowing
> + applications to set various configuration parameters. Common
> + attributes are:
> + <itemizedlist>
> + <listitem><filename>/sys/bus/iio/iio:deviceX/name</filename>,
> + description of the physical chip for device X.
> + </listitem>
> + <listitem><filename>/sys/bus/iio/iio:deviceX/dev</filename>,
> + shows the major:minor pair associated with
> + /dev/iio:deviceX node.
> + </listitem>
> + <listitem><filename>/sys/bus/iio:deviceX/sampling_frequency_available
> + </filename> available discrete set of sampling frequency values
> + for device X.
> + </listitem>
> + </itemizedlist>
This list might be easier to read by proving the directory name once at the
top, then just putting the attribute names here.
> + Available standard attributes for IIO devices are described in the
> + <filename>Documentation/ABI/testing/sysfs-bus-iio </filename> file
> + in the Linux kernel sources.
Should that move out of testing at some point?
[...]
> + An IIO channel is described by the <type> struct iio_chan_spec
> + </type>. A thermometer driver for the temperature sensor in the
> + example above would have to describe its channel as follows:
> + <programlisting>
> + static const struct iio_chan_spec temp_channel[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> + },
> + };
> + </programlisting>
It seems we're skipping over the contents of .info_mask_separate? IIO
developers will need to know about that, right?
> + When there are multiple channels per device we need to set the <type>
> + .modified </type> field of <type>iio_chan_spec</type> to 1. For example,
> + a light sensor can have two channels one, for infrared light and one for
> + both infrared and visible light.
That seems really opaque. What does ".modified" actually indicate?
Clearly not the number of channels...
> + <programlisting>
> + static const struct iio_chan_spec light_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_LIGHT_IR,
It seems like the contents of this field would be good to document too.
[...]
> + <para> In order to be useful, a buffer needs to have an associated
> + trigger. Future chapters will add details about triggers and how
> + drivers use triggers to start data capture, moving samples from device
> + registers to buffer storage and then upward to user space applications.
I'm gonna hold you to that! :)
> + </para>
> + </sect2>
> + <sect2 id="iiobuffersetup"> <title> IIO buffer setup </title>
> + <para>The important bits for selecting data channels
> + configuration are exposed to userspace applications via the <filename>
s/channels/channel/. Even better would be "data-channel".
> + /sys/bus/iio/iio:deviceX/scan_elements/</filename> directory. This
> + file contains attributes of the following form:
> + <itemizedlist>
> + <listitem><emphasis>enable</emphasis>, used for enabling a channel.
> + If and only if his attribute is non zero, thena triggered capture
s/his/its/; also fix "thena"
> + will contain data sample for this channel.
sample*s*
> + </listitem>
> + <listitem><emphasis>type</emphasis>, description of the scan element
> + data storage within the buffer and hence the form in which it is
> + read from user space. Format is <emphasis>
> + [be|le]:[s|u]bits/storagebits[>>shift] </emphasis>.
OK, I'm slow, but this is not telling me much. What's "scan element"?
> + <itemizedlist>
> + <listitem> <emphasis>be</emphasis> or <emphasis>le</emphasis> specifies
> + big or little endian.
> + </listitem>
> + <listitem>
> + <emphasis>s </emphasis>or <emphasis>u</emphasis> specifies if
> + signed (2's complement) or unsigned.
> + </listitem>
> + <listitem>bits is the number of bits of data
> + </listitem>
> + <listitem>storagebits is the space (after padding) that it occupies in the
> + buffer.
> + </listitem>
> + <listitem>
> + <emphasis>shift</emphasis> if specified, is the shift that needs
> + to be a applied prior to masking out unused bits
> + </listitem>
> + </itemizedlist>
> + </listitem>
> + </itemizedlist>
An example or two would be helpful here.
> + For implementing buffer support a driver should initialize the following
> + fields in <type>iio_chan_spec</type> definition:
> +
> + <programlisting>
> + struct iio_chan_spec {
> + /* other members */
> + int scan_index
> + struct {
> + u8 realbits;
> + u8 storagebits;
> + u8 shift;
> + enum iio_endian endianness;
> + } scan_type;
> + }
> + </programlisting>
> + Here is what a 3-axis, 12 bits accelerometer channels definition
> + looks like with buffer support:
channel. (or maybe "channel's").
> + <programlisting>
> + struct struct iio_chan_spec accel_channels[] = {
> + {
> + .type = IIO_ACCEL,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + /* other stuff here */
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
You didn't mention .sign above. Why 's'?
[...]
> + <listitem><function> iio_buffer_setup_ops</function>, buffer setup
> + functions to be called at predefined points in buffer configuration
> + sequence (e.g. before enable, after disable). If not specified, IIO
> + core uses default <type>iio_triggered_buffer_setup_ops</type>.
*the* IIO core uses *the* default...
> + </listitem>
> + <listitem><function>sensor_iio_pollfunc</function>, function that
*the* function...
> + will be used as top half of poll function. It usually does little
> + processing (as it runs in interrupt context). Most common operation
*The* most common... [starting a new theme here, it seems]
> + is recording of the current timestamp and for this reason one can
> + use the IIO core defined <function>iio_pollfunc_store_time</function>
> + function.
> + </listitem>
> + <listitem><function>sensor_trigger_handler</function>, function that
> + will be used as bottom half of poll function. Here all the
A couple more the's here, please
> + processing takes place. It usually reads data from the device and
> + stores it in the internal buffer together with the timestamp
> + recorded in the top half.
> + </listitem>
> + </itemizedlist>
> + </sect2>
> + </sect1>
> + </chapter>
> + <chapter id='iioresources'>
> + <title> Resources </title>
> + IIO core may change during time so the best documentation to read is the
> + source code. There are several locations where you should look:
*The* IIO core. So you say we're wasting our time with this file? :)
> + <itemizedlist>
> + <listitem>
> + <filename>drivers/iio/</filename>, contains the IIO core plus
> + and directories for each sensor type (e.g. accel, magnetometer,
> + etc.)
> + </listitem>
> + <listitem>
> + <filename>include/linux/iio/</filename>, contains the header
> + files, nice to read for the internal kernel interfaces.
> + </listitem>
> + <listitem>
> + <filename>include/uapi/linux/iio/</filename>, contains file to be
file*s*
> + used by user space applications.
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists