On 29/01/15 11:38, Octavian Purdila wrote:
> On Thu, Jan 29, 2015 at 1:46 AM, Hartmut Knaack <knaack.h@xxxxxx> wrote:
>>
>> Octavian Purdila schrieb am 21.12.2014 um 01:42:
>>> Some devices have hardware buffers that can store a number of samples
>>> for later consumption. Hardware usually provides interrupts to notify
>>> the processor when the fifo is full or when it has reached a certain
>>> threshold. This helps with reducing the number of interrupts to the
>>> host processor and thus it helps decreasing the power consumption.
>>>
>>> This patch adds support for hardware fifo to IIO by allowing the
>>> drivers to register operations for flushing the hadware fifo and
>>> setting the watermark level.
>>>
>>> A driver implementing hardware fifo support must also provide a
>>> watermark trigger which must contain "watermark" in its name.
>>>
>>
>> A few comments inline, also addressed to the other IIO maintainers.
>>
>
> Thanks for the review Hartmut. What do you think about the watermark
> trigger approach?
>
> Also, after the discussion with Jonathan I am thinking of exposing a
> dedicated watermark level for the hardware FIFO. That way userspace
> can set a large value for the software buffer watermark and it also
> give control to userspace over the hardware watermark. I think that
> control is very important to achieve a good power/latency balance.
It would be odd to have a high watermark for the software buffer and a
low one for the hardware, but I guess there might be some 'on demand'
applications where this makes sense. Ones where they will do a lower
latency capture if they have time or demand, but otherwise do a high
latency pass. This feels niche enough to me that maybe we don't
want to support it in the first instance, but keep it in mind for
future fun and games!
>
>>> Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
>>> ---
>>> Documentation/ABI/testing/sysfs-bus-iio | 22 +++++++++++++++++
>>> drivers/iio/industrialio-buffer.c | 44 ++++++++++++++++++++++++++++-----
>>> include/linux/iio/iio.h | 17 +++++++++++++
>>> 3 files changed, 77 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 7260f1f..6bb67ac 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -1152,3 +1152,25 @@ Description:
>>> Poll will block until the watermark is reached.
>>> Blocking read will wait until the minimum between the requested
>>> read amount or the low water mark is available.
>>> + If the device has a hardware fifo this value is going to be used
>>> + for the hardware fifo watermark as well.
>>> +
>>> +What: /sys/bus/iio/devices/iio:deviceX/buffer/hwfifo-length
>>> +KernelVersion: 3.20
>>> +Contact: linux-iio@xxxxxxxxxxxxxxx
>>> +Description:
>>> + A single positive integer specifying the maximum number of
>>> + samples that the hardware fifo has. If the device does not
>>> + support hardware fifo this is zero.
>>> + When a device supports hardware fifo it will expose a trigger
>>> + with the name that contains "watermark"
>>> + (e.g. i2c-BMC150A:00-watermark-dev0).
>>> + To use the hardware fifo the user must set an appropriate value
>>> + in the buffer/length and buffer/low_watermark entries and select
>>> + the watermark trigger. At that poin the hardware fifo will be
>>> + enabled and the samples will be collected in a hardware buffer.
>>> + When the number of samples in the hardware fifo reaches the
>>> + watermark level the watermark trigger is issued and data is
>>> + flushed to the devices buffer.
>>> + A hardware buffer flush will also be triggered when reading from
>>> + the device buffer and there is not enough data available.
>>> \ No newline at end of file
>>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>>> index 7f74c7f..3da6d07 100644
>>> --- a/drivers/iio/industrialio-buffer.c
>>> +++ b/drivers/iio/industrialio-buffer.c
>>> @@ -37,9 +37,17 @@ static bool iio_buffer_is_active(struct iio_buffer *buf)
>>> return !list_empty(&buf->buffer_list);
>>> }
>>>
>>> -static size_t iio_buffer_data_available(struct iio_buffer *buf)
>>> +static bool iio_buffer_data_available(struct iio_dev *indio_dev,
>>> + struct iio_buffer *buf, size_t required)
>>> {
>>> - return buf->access->data_available(buf);
>>> + size_t avail = buf->access->data_available(buf);
>>> +
>>> + if (avail < required && indio_dev->hwfifo) {
>>> + indio_dev->hwfifo->flush(indio_dev);
>>> + avail = buf->access->data_available(buf);
>>> + }
>>> +
>>> + return avail >= required;
>>> }
>>>
>>> /**
>>> @@ -66,13 +74,13 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
>>>
>>> do {
>>> if (filp->f_flags & O_NONBLOCK) {
>>> - if (!iio_buffer_data_available(rb)) {
>>> + if (!iio_buffer_data_available(indio_dev, rb, 1)) {
>>> ret = -EAGAIN;
>>> break;
>>> }
>>> } else {
>>> ret = wait_event_interruptible(rb->pollq,
>>> - iio_buffer_data_available(rb) >= to_read ||
>>> + iio_buffer_data_available(indio_dev, rb, to_read) ||
>>> indio_dev->info == NULL);
>>> if (ret)
>>> return ret;
>>> @@ -112,7 +120,7 @@ unsigned int iio_buffer_poll(struct file *filp,
>>> return -ENODEV;
>>>
>>> poll_wait(filp, &rb->pollq, wait);
>>> - if (iio_buffer_data_available(rb) >= rb->low_watermark)
>>> + if (iio_buffer_data_available(indio_dev, rb, rb->low_watermark))
>>> return POLLIN | POLLRDNORM;
>>> return 0;
>>> }
>>> @@ -440,8 +448,14 @@ static ssize_t iio_buffer_write_length(struct device *dev,
>>> if (buffer->length)
>>> val = buffer->length;
>>>
>>> - if (val < buffer->low_watermark)
>>> + if (val < buffer->low_watermark) {
>>> + if (indio_dev->hwfifo) {
>>> + ret = indio_dev->hwfifo->set_watermark(indio_dev, val);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> buffer->low_watermark = val;
>>> + }
>>>
>>> return len;
>>> }
>>> @@ -811,6 +825,12 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>>> goto out;
>>> }
>>>
>>> + if (indio_dev->hwfifo) {
>>> + ret = indio_dev->hwfifo->set_watermark(indio_dev, val);
>>> + if (ret)
>>> + goto out;
>>> + }
>>> +
>>> buffer->low_watermark = val;
>>> ret = 0;
>>> out:
>>> @@ -818,6 +838,16 @@ out:
>>> return ret ? ret : len;
>>> }
>>>
>>> +ssize_t iio_buffer_hwfifo_read_length(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + const struct iio_hwfifo *hwfifo = indio_dev->hwfifo;
>>> +
>>> + return sprintf(buf, "%u\n", hwfifo ? hwfifo->length : 0);
>>> +}
>>> +
>>> static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
>>> iio_buffer_write_length);
>>> static struct device_attribute dev_attr_length_ro = __ATTR(length,
>>> @@ -826,11 +856,13 @@ static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
>>> iio_buffer_show_enable, iio_buffer_store_enable);
>>> static DEVICE_ATTR(low_watermark, S_IRUGO | S_IWUSR,
>>> iio_buffer_show_watermark, iio_buffer_store_watermark);
>>> +static DEVICE_ATTR(hwfifo_length, S_IRUGO, iio_buffer_hwfifo_read_length, NULL);
>>>
>>> static struct attribute *iio_buffer_attrs[] = {
>>> &dev_attr_length.attr,
>>> &dev_attr_enable.attr,
>>> &dev_attr_low_watermark.attr,
>>> + &dev_attr_hwfifo_length.attr,
>>> };
>>>
>>> int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 878d861..f64a05f 100644
>>> --- a/include/linux/iio/iio.h
>>> +++ b/include/linux/iio/iio.h
>>> @@ -417,6 +417,22 @@ struct iio_buffer_setup_ops {
>>> };
>>>
>>> /**
>>> + * struct iio_buffer_hwfifo_ops - hardware fifo operations
>>> + *
>>> + * @length: [DRIVER] the hardware fifo length
>>> + * @set_watermark: [DRIVER] setups the watermark level
>> Maybe use "sets" instead of "setups".
>>
>
> Ok, sensible suggestion, I'll do that in the next version.
>
>> Also, and this is more a general question, as it was quite annoying when getting into
>> the topic: I would prefer to place some information about the expected return values of
>> these functions into these descriptions. What do you guys think about that?
Absolutely - Not sure how one does that whilst keeping within kernel-doc, but
would definitely be a good thing (even if kernel-doc doesn't have a nice format for it!)
>>
>>> + * @flush: [DRIVER] copies data from the hardware buffer to the
>>> + * device buffer
>>> + * @watermark_trig: [DRIVER] an allocated and registered trigger containing
>>> + * "watermark" in its name
>>> + */
>>> +struct iio_hwfifo {
>>> + int length;
>>> + int (*set_watermark)(struct iio_dev *, unsigned int);
>>> + int (*flush)(struct iio_dev *);
>>> +};
>>> +
>>> +/**
>>> * struct iio_dev - industrial I/O device
>>> * @id: [INTERN] used to identify device internally
>>> * @modes: [DRIVER] operating modes supported by device
>>> @@ -491,6 +507,7 @@ struct iio_dev {
>>> int groupcounter;
>>>
>>> unsigned long flags;
>>> + const struct iio_hwfifo *hwfifo;
>> This new entry should be added to the description of the iio_dev struct, as well.
>
> Sure.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
