| Message ID | 1512161180-30116-1-git-send-email-mfornero@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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;