Date:	Fri, 31 Jul 2015 16:48:28 +0300
From:	Daniel Baluta <daniel.baluta@...el.com>
To:	Crt Mori <cmo@...exis.com>
Cc:	Daniel Baluta <daniel.baluta@...el.com>,
	Johnathan Iain Cameron <jic23@...nel.org>,
	Jonathan Corbet <corbet@....net>,
	Randy Dunlap <rdunlap@...radead.org>,
	Peter Meerwald <pmeerw@...erw.net>,
	Hartmut Knaack <knaack.h@....de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	Herbert Xu <herbert@...dor.apana.org.au>, smueller@...onox.de,
	mmarek@...e.cz, linux-doc@...r.kernel.org,
	Cristina Georgiana Opriceana <cristina.opriceana@...il.com>
Subject: Re: [PATCH v3] DocBook: Add initial documentation for IIO

Hi,

Thanks a lot for review.

<snip>

>> +      Hwmon is very much directed at low sample rate sensors used in
>> +      applications such as fan speed control and temperature measurement. Input
>> +      is, as its name suggests, focused on human interaction input devices
>> +      (keyboard, mouse, touchscreen). In some cases there is considerable
>> +      overlap between these and IIO.
>
> I would make this first sentence in last paragraph more in sense:
> Hwmon is directed at low sample rate sensors used to monitor and control the
> system itself, like fan speed control and temperature measurement.

Agree.

<snip>

>> +    Usually these sensors are connected via SPI or I2C. It is also a common
>> +    use case to have combo functionality (e.g. light plus proximity sensor).
>
> I would spell it out: A common use case of the IIO devices is to
> provide combined
> functionality (e.g. illuminance plus proximity sensor)

Agree.

<snip>

>> +    Here <emphasis> scan_index </emphasis> defines the relative order in which
>> +    the enabled channels are placed inside the buffer, a channel with a lower
> I would change this line to:
> +   the enabled channels are placed inside the buffer. Channel with a lower
>> +    scan_index will be placed before a channel with a higher index. Each
>> +    channels needs to have a unique scan_index.

Agree.

>> +    </para>
>> +    <para>
>> +    It is important to realize that the scan_index does not define the
>> +    absolution position in the buffer. E.g. a channel with the scan_index = 3
> absolute position in buffer.

Correct :).

<snip>

>> +    <sect2 id="iiotrigsysfs"> <title> IIO trigger sysfs interface </title>
>> +      There are two locations in sysfs related to triggers:
>> +      <itemizedlist>
>> +        <listitem><filename>/sys/bus/iio/devices/triggerX</filename>,
> I am missing in here the information that X corresponds to deviceX

Triggers can be independent of devices, but indeed I changed it to
triggerY in order
to avoid confusion.

<snip>

>> +    <listitem><function> iio_buffer_setup_ops</function>, the buffer setup
>> +    functions to be called at predefined points in buffer configuration
> functions should be called at predefined points in buffer configuration
>> +    sequence (e.g. before enable, after disable). If not specified, the
>> +    IIO core uses the default <type>iio_triggered_buffer_setup_ops</type>.
>> +    </listitem>

Well, I'm not sure if I understand. iio_buffer_setup_ops is a
structure with function pointers
to be called at certain points in the trigger setup phase (before
enable, after disable
etc).  So, I think the phrasing is correct.


>> +    <listitem><function>sensor_iio_pollfunc</function>, the function that
>> +    will be used as top half of poll function. It usually does little
> ... It should do as little processing as possible, because it runs in
> interrupt context.

Agree.

I will send v4 asap.

thanks,
Daniel.
--
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