iio: buffer-dma: Expose data available
diff mbox

Message ID 1512161180-30116-1-git-send-email-mfornero@gmail.com
State New
Headers show

Commit Message

mfornero@gmail.com Dec. 1, 2017, 8:46 p.m. UTC
From: Matt Fornero <matt.fornero@mathworks.com>

Add a sysfs attribute that exposes the buffer data available to
userspace. This attribute can be checked at runtime to determine the
overall buffer fill level (across all allocated DMA buffers).

Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
---
 drivers/iio/buffer/industrialio-buffer-dma.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Jonathan Cameron Dec. 2, 2017, 11:52 a.m. UTC | #1
On Fri,  1 Dec 2017 15:46:20 -0500
mfornero@gmail.com wrote:

> From: Matt Fornero <matt.fornero@mathworks.com>
> 
> Add a sysfs attribute that exposes the buffer data available to
> userspace. This attribute can be checked at runtime to determine the
> overall buffer fill level (across all allocated DMA buffers).
> 
> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>

Seems sensible to me.

Lars?

Thanks,

Jonathan
> ---
>  drivers/iio/buffer/industrialio-buffer-dma.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
> index ff03324..8739a41 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> @@ -16,6 +16,7 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  #include <linux/iio/buffer-dma.h>
> +#include <linux/iio/sysfs.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/sizes.h>
>  
> @@ -599,6 +600,28 @@ int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
>  }
>  EXPORT_SYMBOL_GPL(iio_dma_buffer_set_length);
>  
> +
> +static ssize_t iio_dma_get_data_available(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	size_t bytes;
> +
> +	bytes = iio_dma_buffer_data_available(indio_dev->buffer);
> +
> +	return sprintf(buf, "%llu\n", (unsigned long long)bytes);
> +}
> +
> +static IIO_DEVICE_ATTR(data_available, S_IRUGO,
> +		       iio_dma_get_data_available, NULL, 0);
> +
> +static const struct attribute *iio_dma_buffer_attributes[] = {
> +	&iio_dev_attr_data_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +
>  /**
>   * iio_dma_buffer_init() - Initialize DMA buffer queue
>   * @queue: Buffer to initialize
> @@ -615,6 +638,7 @@ int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
>  	iio_buffer_init(&queue->buffer);
>  	queue->buffer.length = PAGE_SIZE;
>  	queue->buffer.watermark = queue->buffer.length / 2;
> +	queue->buffer.attrs = iio_dma_buffer_attributes;
>  	queue->dev = dev;
>  	queue->ops = ops;
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Dec. 5, 2017, 9:32 a.m. UTC | #2
On 12/02/2017 12:52 PM, Jonathan Cameron wrote:
> On Fri,  1 Dec 2017 15:46:20 -0500
> mfornero@gmail.com wrote:
> 
>> From: Matt Fornero <matt.fornero@mathworks.com>
>>
>> Add a sysfs attribute that exposes the buffer data available to
>> userspace. This attribute can be checked at runtime to determine the
>> overall buffer fill level (across all allocated DMA buffers).
>>
>> Signed-off-by: Matt Fornero <matt.fornero@mathworks.com>
> 
> Seems sensible to me.
> 
> Lars?

I guess one question is whether this should be generic.
iio_dma_buffer_data_available() is a generic function and not specific to
the DMA buffers, so it would work just fine for FIFO based buffers as well.

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Fornero Dec. 5, 2017, 6:05 p.m. UTC | #3
On Tue, Dec 5, 2017 at 1:32 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> I guess one question is whether this should be generic.
> iio_dma_buffer_data_available() is a generic function and not specific to
> the DMA buffers, so it would work just fine for FIFO based buffers as well.

So it looks like we could implement this generically as a member of
iio_buffer_attrs, and have it simply use iio_buffer_data_available()
instead of iio_dma_buffer_data_available().

This would also be a bit cleaner, as the iio_buffer_attrs logic already
has code to append existings attrs, allowing device-specific attrs to be
easily added.

What about the case when the data_available element in
iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
Do we modify iio_buffer_data_available() to return 0, do we make the
sysfs function return -ENOENT or -EINVAL (or simply not expose the sysfs
interface for this case)?
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Dec. 5, 2017, 6:08 p.m. UTC | #4
On 12/05/2017 07:01 PM, Matthew Fornero wrote:
> On Tue, Dec 5, 2017 at 1:32 AM Lars-Peter Clausen <lars@metafoo.de
> <mailto:lars@metafoo.de>> wrote:
>> I guess one question is whether this should be generic.
>> iio_dma_buffer_data_available() is a generic function and not specific to the
>> DMA buffers, so it would work just fine for FIFO based buffers as well.
> 
> So it looks like we could implement this generically as a member of
> iio_buffer_attrs, and have it simply use iio_buffer_data_available()
> instead of iio_dma_buffer_data_available().
> 
> This would also be a bit cleaner, as the iio_buffer_attrs logic already
> has code to append existings attrs, allowing device-specific attrs to be
> easily added.
> 
> What about the case when the data_available element in 
> iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)? 
> Do we modify iio_buffer_data_available() to return 0, do we make the
> sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
> interface for this case?

The callback buffer does not support any of the interfaces required for the
userspace facing side. E.g. there is no read() callback.

For proper support of the userspace interfaces data_available() is required,
otherwise read() would never return. So I think this is fine, we'd never
register the userspacing interface if it didn't have the data_available
callback.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Fornero Dec. 5, 2017, 6:11 p.m. UTC | #5
On Tue, Dec 5, 2017 at 10:08 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> > What about the case when the data_available element in
> > iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
> > Do we modify iio_buffer_data_available() to return 0, do we make the
> > sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
> > interface for this case?
>
> The callback buffer does not support any of the interfaces required for the
> userspace facing side. E.g. there is no read() callback.
>
> For proper support of the userspace interfaces data_available() is required,
> otherwise read() would never return. So I think this is fine, we'd never
> register the userspacing interface if it didn't have the data_available
> callback.

So there should already be provisions in place for *not* registering the
cb buffer with user-space, meaning no additional logic would be required
for exposing / not exposing a "data_available" sysfs attribute, correct?

Assuming the above is correct, I'll come back with a generic v2 of this.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen Dec. 5, 2017, 6:38 p.m. UTC | #6
On 12/05/2017 07:11 PM, Matthew Fornero wrote:
> On Tue, Dec 5, 2017 at 10:08 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> What about the case when the data_available element in
>>> iio_buffer_access_funcs is undefined (e.g. industrialio-buffer-cb)?
>>> Do we modify iio_buffer_data_available() to return 0, do we make the
>>> sysfs function return -ENOENT or -EINVAL, or simply not expose the sysfs
>>> interface for this case?
>>
>> The callback buffer does not support any of the interfaces required for the
>> userspace facing side. E.g. there is no read() callback.
>>
>> For proper support of the userspace interfaces data_available() is required,
>> otherwise read() would never return. So I think this is fine, we'd never
>> register the userspacing interface if it didn't have the data_available
>> callback.
> 
> So there should already be provisions in place for *not* registering the
> cb buffer with user-space, meaning no additional logic would be required
> for exposing / not exposing a "data_available" sysfs attribute, correct?

Yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index ff03324..8739a41 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -16,6 +16,7 @@ 
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 #include <linux/iio/buffer-dma.h>
+#include <linux/iio/sysfs.h>
 #include <linux/dma-mapping.h>
 #include <linux/sizes.h>
 
@@ -599,6 +600,28 @@  int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
 }
 EXPORT_SYMBOL_GPL(iio_dma_buffer_set_length);
 
+
+static ssize_t iio_dma_get_data_available(struct device *dev,
+					       struct device_attribute *attr,
+					       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	size_t bytes;
+
+	bytes = iio_dma_buffer_data_available(indio_dev->buffer);
+
+	return sprintf(buf, "%llu\n", (unsigned long long)bytes);
+}
+
+static IIO_DEVICE_ATTR(data_available, S_IRUGO,
+		       iio_dma_get_data_available, NULL, 0);
+
+static const struct attribute *iio_dma_buffer_attributes[] = {
+	&iio_dev_attr_data_available.dev_attr.attr,
+	NULL,
+};
+
+
 /**
  * iio_dma_buffer_init() - Initialize DMA buffer queue
  * @queue: Buffer to initialize
@@ -615,6 +638,7 @@  int iio_dma_buffer_init(struct iio_dma_buffer_queue *queue,
 	iio_buffer_init(&queue->buffer);
 	queue->buffer.length = PAGE_SIZE;
 	queue->buffer.watermark = queue->buffer.length / 2;
+	queue->buffer.attrs = iio_dma_buffer_attributes;
 	queue->dev = dev;
 	queue->ops = ops;