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.
> 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".
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?
> + * @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.
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
>
--
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
