* [RFC PATCH 01/14] iio: Move scan mask management to the core
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 02/14] iio: hw_consumer: use new scanmask functions Alexandru Ardelean
` (13 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
From: Lars-Peter Clausen <lars@metafoo.de>
Let the core handle the buffer scan mask management including allocation
and channel selection. Having this handled in a central place rather than
open-coding it all over the place will make it easier to change the
implementation.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/buffer/industrialio-buffer-cb.c | 16 ++++-----
drivers/iio/industrialio-buffer.c | 36 +++++++++++++++------
drivers/iio/inkern.c | 15 +++++++++
include/linux/iio/consumer.h | 10 ++++++
4 files changed, 58 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
index 47c96f7f4976..3db389ea3f4e 100644
--- a/drivers/iio/buffer/industrialio-buffer-cb.c
+++ b/drivers/iio/buffer/industrialio-buffer-cb.c
@@ -34,7 +34,7 @@ static void iio_buffer_cb_release(struct iio_buffer *buffer)
{
struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer);
- bitmap_free(cb_buff->buffer.scan_mask);
+ iio_buffer_free_scanmask(&cb_buff->buffer);
kfree(cb_buff);
}
@@ -72,27 +72,25 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
}
cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
- cb_buff->buffer.scan_mask = bitmap_zalloc(cb_buff->indio_dev->masklength,
- GFP_KERNEL);
- if (cb_buff->buffer.scan_mask == NULL) {
- ret = -ENOMEM;
+
+ ret = iio_buffer_alloc_scanmask(&cb_buff->buffer, cb_buff->indio_dev);
+ if (ret)
goto error_release_channels;
- }
+
chan = &cb_buff->channels[0];
while (chan->indio_dev) {
if (chan->indio_dev != cb_buff->indio_dev) {
ret = -EINVAL;
goto error_free_scan_mask;
}
- set_bit(chan->channel->scan_index,
- cb_buff->buffer.scan_mask);
+ iio_buffer_channel_enable(&cb_buff->buffer, chan);
chan++;
}
return cb_buff;
error_free_scan_mask:
- bitmap_free(cb_buff->buffer.scan_mask);
+ iio_buffer_free_scanmask(&cb_buff->buffer);
error_release_channels:
iio_channel_release_all(cb_buff->channels);
error_free_cb_buff:
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index eae39eaf49af..7eed97c1df14 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -208,6 +208,26 @@ void iio_buffer_init(struct iio_buffer *buffer)
}
EXPORT_SYMBOL(iio_buffer_init);
+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+ struct iio_dev *indio_dev)
+{
+ if (!indio_dev->masklength)
+ return -EINVAL;
+
+ buffer->scan_mask = bitmap_zalloc(indio_dev->masklength, GFP_KERNEL);
+ if (buffer->scan_mask == NULL)
+ return -ENOMEM;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_alloc_scanmask);
+
+void iio_buffer_free_scanmask(struct iio_buffer *buffer)
+{
+ bitmap_free(buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_free_scanmask);
+
/**
* iio_buffer_set_attrs - Set buffer specific attributes
* @buffer: The buffer for which we are setting attributes
@@ -1306,14 +1326,10 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
indio_dev->scan_index_timestamp =
channels[i].scan_index;
}
- if (indio_dev->masklength && buffer->scan_mask == NULL) {
- buffer->scan_mask = bitmap_zalloc(indio_dev->masklength,
- GFP_KERNEL);
- if (buffer->scan_mask == NULL) {
- ret = -ENOMEM;
- goto error_cleanup_dynamic;
- }
- }
+
+ ret = iio_buffer_alloc_scanmask(buffer, indio_dev);
+ if (ret)
+ goto error_cleanup_dynamic;
}
buffer->scan_el_group.name = iio_scan_elements_group_name;
@@ -1334,7 +1350,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
return 0;
error_free_scan_mask:
- bitmap_free(buffer->scan_mask);
+ iio_buffer_free_scanmask(buffer);
error_cleanup_dynamic:
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
kfree(buffer->buffer_group.attrs);
@@ -1349,7 +1365,7 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
if (!buffer)
return;
- bitmap_free(buffer->scan_mask);
+ iio_buffer_free_scanmask(buffer);
kfree(buffer->buffer_group.attrs);
kfree(buffer->scan_el_group.attrs);
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index ede99e0d5371..f35cb9985edc 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -11,6 +11,7 @@
#include <linux/iio/iio.h>
#include "iio_core.h"
+#include <linux/iio/buffer_impl.h>
#include <linux/iio/machine.h>
#include <linux/iio/driver.h>
#include <linux/iio/consumer.h>
@@ -857,6 +858,20 @@ int iio_write_channel_raw(struct iio_channel *chan, int val)
}
EXPORT_SYMBOL_GPL(iio_write_channel_raw);
+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+ const struct iio_channel *chan)
+{
+ set_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_enable);
+
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+ const struct iio_channel *chan)
+{
+ clear_bit(chan->channel->scan_index, buffer->scan_mask);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_channel_disable);
+
unsigned int iio_get_channel_ext_info_count(struct iio_channel *chan)
{
const struct iio_chan_spec_ext_info *ext_info;
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index c4118dcb8e05..dbc87c26250a 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -12,6 +12,7 @@
struct iio_dev;
struct iio_chan_spec;
+struct iio_buffer;
struct device;
/**
@@ -342,6 +343,15 @@ int iio_read_channel_scale(struct iio_channel *chan, int *val,
int iio_convert_raw_to_processed(struct iio_channel *chan, int raw,
int *processed, unsigned int scale);
+void iio_buffer_channel_enable(struct iio_buffer *buffer,
+ const struct iio_channel *chan);
+void iio_buffer_channel_disable(struct iio_buffer *buffer,
+ const struct iio_channel *chan);
+
+int iio_buffer_alloc_scanmask(struct iio_buffer *buffer,
+ struct iio_dev *indio_dev);
+void iio_buffer_free_scanmask(struct iio_buffer *buffer);
+
/**
* iio_get_channel_ext_info_count() - get number of ext_info attributes
* connected to the channel.
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 02/14] iio: hw_consumer: use new scanmask functions
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 01/14] iio: Move scan mask management to the core Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 03/14] iio: buffer: add back-ref from iio_buffer to iio_dev Alexandru Ardelean
` (12 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
From: Lars-Peter Clausen <lars@metafoo.de>
This change moves the handling of the scanmasks to the new wrapper
functions that were added in a previous commit.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/buffer/industrialio-hw-consumer.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-hw-consumer.c b/drivers/iio/buffer/industrialio-hw-consumer.c
index f2d27788f666..f1cc72520685 100644
--- a/drivers/iio/buffer/industrialio-hw-consumer.c
+++ b/drivers/iio/buffer/industrialio-hw-consumer.c
@@ -28,7 +28,6 @@ struct hw_consumer_buffer {
struct list_head head;
struct iio_dev *indio_dev;
struct iio_buffer buffer;
- long scan_mask[];
};
static struct hw_consumer_buffer *iio_buffer_to_hw_consumer_buffer(
@@ -41,6 +40,8 @@ static void iio_hw_buf_release(struct iio_buffer *buffer)
{
struct hw_consumer_buffer *hw_buf =
iio_buffer_to_hw_consumer_buffer(buffer);
+
+ iio_buffer_free_scanmask(buffer);
kfree(hw_buf);
}
@@ -52,26 +53,34 @@ static const struct iio_buffer_access_funcs iio_hw_buf_access = {
static struct hw_consumer_buffer *iio_hw_consumer_get_buffer(
struct iio_hw_consumer *hwc, struct iio_dev *indio_dev)
{
- size_t mask_size = BITS_TO_LONGS(indio_dev->masklength) * sizeof(long);
struct hw_consumer_buffer *buf;
+ int ret;
list_for_each_entry(buf, &hwc->buffers, head) {
if (buf->indio_dev == indio_dev)
return buf;
}
- buf = kzalloc(sizeof(*buf) + mask_size, GFP_KERNEL);
+ buf = kzalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
return NULL;
buf->buffer.access = &iio_hw_buf_access;
buf->indio_dev = indio_dev;
- buf->buffer.scan_mask = buf->scan_mask;
iio_buffer_init(&buf->buffer);
+
+ ret = iio_buffer_alloc_scanmask(&buf->buffer, indio_dev);
+ if (ret)
+ goto err_free_buf;
+
list_add_tail(&buf->head, &hwc->buffers);
return buf;
+
+err_free_buf:
+ kfree(buf);
+ return NULL;
}
/**
@@ -106,7 +115,7 @@ struct iio_hw_consumer *iio_hw_consumer_alloc(struct device *dev)
ret = -ENOMEM;
goto err_put_buffers;
}
- set_bit(chan->channel->scan_index, buf->buffer.scan_mask);
+ iio_buffer_channel_enable(&buf->buffer, chan);
chan++;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 03/14] iio: buffer: add back-ref from iio_buffer to iio_dev
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 01/14] iio: Move scan mask management to the core Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 02/14] iio: hw_consumer: use new scanmask functions Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 04/14] iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() Alexandru Ardelean
` (11 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
An IIO device will have multiple buffers, but it shouldn't be allowed that
an IIO buffer should belong to more than 1 IIO device.
Once things get moved more from IIO device to the IIO buffer, and an IIO
device will be able to have more than 1 buffer attached, there will be a
need for a back-ref to the IIO device [from the IIO buffer].
This change adds that.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 2 ++
include/linux/iio/buffer_impl.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 7eed97c1df14..3b1071deba00 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1493,5 +1493,7 @@ void iio_device_attach_buffer(struct iio_dev *indio_dev,
struct iio_buffer *buffer)
{
indio_dev->buffer = iio_buffer_get(buffer);
+
+ indio_dev->buffer->indio_dev = indio_dev;
}
EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index a63dc07b7350..67d73d465e02 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -69,6 +69,9 @@ struct iio_buffer_access_funcs {
* those writing new buffer implementations.
*/
struct iio_buffer {
+ /** @indio_dev: IIO device to which this buffer belongs to. */
+ struct iio_dev *indio_dev;
+
/** @length: Number of datums in buffer. */
unsigned int length;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 04/14] iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put()
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (2 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 03/14] iio: buffer: add back-ref from iio_buffer to iio_dev Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 05/14] iio: core: register chardev only if needed Alexandru Ardelean
` (10 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
The name (and the wrapper) seems superfluous now, but when more buffers
will be attached to the IIO device this will be a bit more useful.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 4 ++++
drivers/iio/industrialio-buffer.c | 10 ++++++++++
drivers/iio/industrialio-core.c | 2 +-
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fd9a5f1d5e51..39ec0344fb68 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -51,6 +51,8 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
#define iio_buffer_poll_addr (&iio_buffer_poll)
#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
+void iio_device_buffers_put(struct iio_dev *indio_dev);
+
void iio_disable_all_buffers(struct iio_dev *indio_dev);
void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
@@ -66,6 +68,8 @@ static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
static inline void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) {}
+static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
+
static inline void iio_disable_all_buffers(struct iio_dev *indio_dev) {}
static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 3b1071deba00..93f187455461 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1371,6 +1371,16 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
}
+void iio_device_buffers_put(struct iio_dev *indio_dev)
+{
+ struct iio_buffer *buffer = indio_dev->buffer;
+
+ if (!buffer)
+ return;
+
+ iio_buffer_put(buffer);
+}
+
/**
* iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected
* @indio_dev: the iio device
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 462d3e810013..32c489139cd2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1481,7 +1481,7 @@ static void iio_dev_release(struct device *device)
iio_device_unregister_eventset(indio_dev);
iio_device_unregister_sysfs(indio_dev);
- iio_buffer_put(indio_dev->buffer);
+ iio_device_buffers_put(indio_dev);
ida_simple_remove(&iio_ida, indio_dev->id);
kfree(indio_dev);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 05/14] iio: core: register chardev only if needed
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (3 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 04/14] iio: core,buffer: wrap iio_buffer_put() call into iio_buffers_put() Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-24 16:40 ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 06/14] iio: buffer,event: duplicate chardev creation for buffers & events Alexandru Ardelean
` (9 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
The final intent is to localize all buffer ops into the
industrialio-buffer.c file, to be able to add support for multiple buffers
per IIO device.
We only need a chardev if we need to support buffers and/or events.
With this change, a chardev will be created only if an IIO buffer is
attached OR an event_interface is configured.
Otherwise, no chardev will be created, and the IIO device will get
registered with the 'device_add()' call.
Quite a lot of IIO devices don't really need a chardev, so this is a minor
improvement to the IIO core, as the IIO device will take up (slightly)
fewer resources.
What's important to remember, is that removing a chardev also requires that
'indio_dev->dev.devt' to be initialized. If it is, a '/dev/iio:deviceX'
file will be created by the kernel, regardless of whether it has a chardev
attached to it or not.
If that field is not initialized, cdev_device_{add,del}() behave{s} like
device_{add,del}().
Also, since we plan to add more chardevs for IIO buffers, we need to
consider now a separate IDA object just for chardevs.
Currently, the only benefit is that chardev IDs will be continuous.
However, when adding a chardev per IIO buffer, the IDs for the chardev
could outpace the IDs for IIO devices, so these should be decoupled.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-core.c | 58 +++++++++++++++++++++++++++++----
1 file changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 32c489139cd2..d74279efeca4 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -35,6 +35,9 @@
/* IDA to assign each registered device a unique id */
static DEFINE_IDA(iio_ida);
+/* IDA to assign each registered character device a unique id */
+static DEFINE_IDA(iio_chrdev_ida);
+
static dev_t iio_devt;
#define IIO_DEV_MAX 256
@@ -1680,8 +1683,40 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
return 0;
}
+static int iio_device_alloc_chrdev_id(struct device *dev)
+{
+ int id;
+
+ id = ida_simple_get(&iio_chrdev_ida, 0, 0, GFP_KERNEL);
+ if (id < 0) {
+ /* cannot use a dev_err as the name isn't available */
+ dev_err(dev, "failed to get device id\n");
+ return id;
+ }
+
+ dev->devt = MKDEV(MAJOR(iio_devt), id);
+
+ return 0;
+}
+
+static void iio_device_free_chrdev_id(struct device *dev)
+{
+ if (!dev->devt)
+ return;
+ ida_simple_remove(&iio_chrdev_ida, MINOR(dev->devt));
+}
+
static const struct iio_buffer_setup_ops noop_ring_setup_ops;
+static const struct file_operations iio_event_fileops = {
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = iio_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+ .open = iio_chrdev_open,
+ .release = iio_chrdev_release,
+};
+
int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
{
int ret;
@@ -1701,9 +1736,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
if (ret < 0)
return ret;
- /* configure elements for the chrdev */
- indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
-
iio_device_register_debugfs(indio_dev);
ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
@@ -1732,16 +1764,27 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
indio_dev->setup_ops == NULL)
indio_dev->setup_ops = &noop_ring_setup_ops;
- cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+ if (indio_dev->buffer)
+ cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
+ else if (indio_dev->event_interface)
+ cdev_init(&indio_dev->chrdev, &iio_event_fileops);
- indio_dev->chrdev.owner = this_mod;
+ if (indio_dev->buffer || indio_dev->event_interface) {
+ indio_dev->chrdev.owner = this_mod;
+
+ ret = iio_device_alloc_chrdev_id(&indio_dev->dev);
+ if (ret)
+ goto error_unreg_eventset;
+ }
ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
- if (ret < 0)
- goto error_unreg_eventset;
+ if (ret)
+ goto error_free_chrdev_id;
return 0;
+error_free_chrdev_id:
+ iio_device_free_chrdev_id(&indio_dev->dev);
error_unreg_eventset:
iio_device_unregister_eventset(indio_dev);
error_free_sysfs:
@@ -1761,6 +1804,7 @@ EXPORT_SYMBOL(__iio_device_register);
void iio_device_unregister(struct iio_dev *indio_dev)
{
cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+ iio_device_free_chrdev_id(&indio_dev->dev);
mutex_lock(&indio_dev->info_exist_lock);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 05/14] iio: core: register chardev only if needed
2020-05-08 13:53 ` [RFC PATCH 05/14] iio: core: register chardev only if needed Alexandru Ardelean
@ 2020-05-24 16:40 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-24 16:40 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, lars
On Fri, 8 May 2020 16:53:39 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> The final intent is to localize all buffer ops into the
> industrialio-buffer.c file, to be able to add support for multiple buffers
> per IIO device.
>
> We only need a chardev if we need to support buffers and/or events.
>
> With this change, a chardev will be created only if an IIO buffer is
> attached OR an event_interface is configured.
>
> Otherwise, no chardev will be created, and the IIO device will get
> registered with the 'device_add()' call.
>
> Quite a lot of IIO devices don't really need a chardev, so this is a minor
> improvement to the IIO core, as the IIO device will take up (slightly)
> fewer resources.
>
> What's important to remember, is that removing a chardev also requires that
> 'indio_dev->dev.devt' to be initialized. If it is, a '/dev/iio:deviceX'
> file will be created by the kernel, regardless of whether it has a chardev
> attached to it or not.
> If that field is not initialized, cdev_device_{add,del}() behave{s} like
> device_{add,del}().
>
> Also, since we plan to add more chardevs for IIO buffers, we need to
> consider now a separate IDA object just for chardevs.
> Currently, the only benefit is that chardev IDs will be continuous.
> However, when adding a chardev per IIO buffer, the IDs for the chardev
> could outpace the IDs for IIO devices, so these should be decoupled.
Given we are still discussing the need for more chardevs I guess
we can't look at patches after this one 'yet'. Up until here they
were all fine independent of the rest of the series.
I was wondering if I could pick up the first part as it is useful
whatever path we take with the rest of the series!
Jonathan
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/industrialio-core.c | 58 +++++++++++++++++++++++++++++----
> 1 file changed, 51 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 32c489139cd2..d74279efeca4 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -35,6 +35,9 @@
> /* IDA to assign each registered device a unique id */
> static DEFINE_IDA(iio_ida);
>
> +/* IDA to assign each registered character device a unique id */
> +static DEFINE_IDA(iio_chrdev_ida);
> +
> static dev_t iio_devt;
>
> #define IIO_DEV_MAX 256
> @@ -1680,8 +1683,40 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> return 0;
> }
>
> +static int iio_device_alloc_chrdev_id(struct device *dev)
> +{
> + int id;
> +
> + id = ida_simple_get(&iio_chrdev_ida, 0, 0, GFP_KERNEL);
> + if (id < 0) {
> + /* cannot use a dev_err as the name isn't available */
> + dev_err(dev, "failed to get device id\n");
> + return id;
> + }
> +
> + dev->devt = MKDEV(MAJOR(iio_devt), id);
> +
> + return 0;
> +}
> +
> +static void iio_device_free_chrdev_id(struct device *dev)
> +{
> + if (!dev->devt)
> + return;
> + ida_simple_remove(&iio_chrdev_ida, MINOR(dev->devt));
> +}
> +
> static const struct iio_buffer_setup_ops noop_ring_setup_ops;
>
> +static const struct file_operations iio_event_fileops = {
> + .owner = THIS_MODULE,
> + .llseek = noop_llseek,
> + .unlocked_ioctl = iio_ioctl,
> + .compat_ioctl = compat_ptr_ioctl,
> + .open = iio_chrdev_open,
> + .release = iio_chrdev_release,
> +};
> +
> int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> {
> int ret;
> @@ -1701,9 +1736,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> if (ret < 0)
> return ret;
>
> - /* configure elements for the chrdev */
> - indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> -
> iio_device_register_debugfs(indio_dev);
>
> ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> @@ -1732,16 +1764,27 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> indio_dev->setup_ops == NULL)
> indio_dev->setup_ops = &noop_ring_setup_ops;
>
> - cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> + if (indio_dev->buffer)
> + cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> + else if (indio_dev->event_interface)
> + cdev_init(&indio_dev->chrdev, &iio_event_fileops);
>
> - indio_dev->chrdev.owner = this_mod;
> + if (indio_dev->buffer || indio_dev->event_interface) {
> + indio_dev->chrdev.owner = this_mod;
> +
> + ret = iio_device_alloc_chrdev_id(&indio_dev->dev);
> + if (ret)
> + goto error_unreg_eventset;
> + }
>
> ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
> - if (ret < 0)
> - goto error_unreg_eventset;
> + if (ret)
> + goto error_free_chrdev_id;
>
> return 0;
>
> +error_free_chrdev_id:
> + iio_device_free_chrdev_id(&indio_dev->dev);
> error_unreg_eventset:
> iio_device_unregister_eventset(indio_dev);
> error_free_sysfs:
> @@ -1761,6 +1804,7 @@ EXPORT_SYMBOL(__iio_device_register);
> void iio_device_unregister(struct iio_dev *indio_dev)
> {
> cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> + iio_device_free_chrdev_id(&indio_dev->dev);
>
> mutex_lock(&indio_dev->info_exist_lock);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 06/14] iio: buffer,event: duplicate chardev creation for buffers & events
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (4 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 05/14] iio: core: register chardev only if needed Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers Alexandru Ardelean
` (8 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
This patch moves the chardev creation into industrialio-buffer.c &
industrialio-event.c. The move has to be in a single step (which makes the
patch a bit big), in order to pass the reference of the chardev to
'indio_dev->chrdev'.
For structure purposes, industrialio-core.c should be the place where
cdev_device_add()/device_add() gets called, and the deletion function as
well.
What happens after this patch is:
- 'indio_dev->chrdev' is converted to a pointer
- if there is an IIO buffer, iio_device_buffer_attach_chrdev() will attach
it's chardev to 'indio_chrdev'
- if it doesn't, the event interface will attach a reference to it's
chardev (if it is instantiated) via iio_device_event_attach_chrdev()
That way, the control of the 'legacy' chardev is still visible in
'industrialio-core.c'. So, each logic file (for buffers & events) shouldn't
hide things too much away from the core file.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 16 ++---
drivers/iio/industrialio-buffer.c | 90 +++++++++++++++++++++++++---
drivers/iio/industrialio-core.c | 99 +++----------------------------
drivers/iio/industrialio-event.c | 98 +++++++++++++++++++++++++++++-
include/linux/iio/buffer_impl.h | 7 +++
include/linux/iio/iio.h | 6 +-
6 files changed, 204 insertions(+), 112 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 39ec0344fb68..a527a66be9e5 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -40,17 +40,14 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
#ifdef CONFIG_IIO_BUFFER
struct poll_table_struct;
-__poll_t iio_buffer_poll(struct file *filp,
- struct poll_table_struct *wait);
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
- size_t n, loff_t *f_ps);
+long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg);
+
+void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
-#define iio_buffer_poll_addr (&iio_buffer_poll)
-#define iio_buffer_read_outer_addr (&iio_buffer_read_outer)
-
void iio_device_buffers_put(struct iio_dev *indio_dev);
void iio_disable_all_buffers(struct iio_dev *indio_dev);
@@ -58,8 +55,7 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
#else
-#define iio_buffer_poll_addr NULL
-#define iio_buffer_read_outer_addr NULL
+static inline void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev) {}
static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
@@ -77,8 +73,8 @@ static inline void iio_buffer_wakeup_poll(struct iio_dev *indio_dev) {}
int iio_device_register_eventset(struct iio_dev *indio_dev);
void iio_device_unregister_eventset(struct iio_dev *indio_dev);
+void iio_device_event_attach_chrdev(struct iio_dev *indio_dev);
void iio_device_wakeup_eventset(struct iio_dev *indio_dev);
-int iio_event_getfd(struct iio_dev *indio_dev);
struct iio_event_interface;
bool iio_event_enabled(const struct iio_event_interface *ev_int);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 93f187455461..1400688f5e42 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -99,11 +99,11 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
* Return: negative values corresponding to error codes or ret != 0
* for ending the reading activity
**/
-ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
- size_t n, loff_t *f_ps)
+static ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
+ size_t n, loff_t *f_ps)
{
- struct iio_dev *indio_dev = filp->private_data;
- struct iio_buffer *rb = indio_dev->buffer;
+ struct iio_buffer *rb = filp->private_data;
+ struct iio_dev *indio_dev = rb->indio_dev;
DEFINE_WAIT_FUNC(wait, woken_wake_function);
size_t datum_size;
size_t to_wait;
@@ -165,11 +165,11 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
* Return: (EPOLLIN | EPOLLRDNORM) if data is available for reading
* or 0 for other cases
*/
-__poll_t iio_buffer_poll(struct file *filp,
+static __poll_t iio_buffer_poll(struct file *filp,
struct poll_table_struct *wait)
{
- struct iio_dev *indio_dev = filp->private_data;
- struct iio_buffer *rb = indio_dev->buffer;
+ struct iio_buffer *rb = filp->private_data;
+ struct iio_dev *indio_dev = rb->indio_dev;
if (!indio_dev->info || rb == NULL)
return 0;
@@ -180,6 +180,48 @@ __poll_t iio_buffer_poll(struct file *filp,
return 0;
}
+/**
+ * iio_buffer_chrdev_open() - chrdev file open for buffer access
+ * @inode: Inode structure for identifying the device in the file system
+ * @filp: File structure for iio device used to keep and later access
+ * private data
+ *
+ * Return: 0 on success or -EBUSY if the device is already opened
+ **/
+static int iio_buffer_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct iio_buffer *buffer = container_of(inode->i_cdev,
+ struct iio_buffer, chrdev);
+
+ if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags))
+ return -EBUSY;
+
+ iio_buffer_get(buffer);
+
+ filp->private_data = buffer;
+
+ return 0;
+}
+
+/**
+ * iio_buffer_chrdev_release() - chrdev file close for buffer access
+ * @inode: Inode structure pointer for the char device
+ * @filp: File structure pointer for the char device
+ *
+ * Return: 0 for successful release
+ */
+static int iio_buffer_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct iio_buffer *buffer = container_of(inode->i_cdev,
+ struct iio_buffer, chrdev);
+
+ clear_bit(IIO_BUSY_BIT_POS, &buffer->file_ops_flags);
+
+ iio_buffer_put(buffer);
+
+ return 0;
+}
+
/**
* iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
* @indio_dev: The IIO device
@@ -1149,6 +1191,17 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
iio_buffer_deactivate_all(indio_dev);
}
+static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
+ unsigned long arg)
+{
+ struct iio_buffer *buffer = filep->private_data;
+
+ if (!buffer || !buffer->access)
+ return -ENODEV;
+
+ return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
+}
+
static ssize_t iio_buffer_store_enable(struct device *dev,
struct device_attribute *attr,
const char *buf,
@@ -1371,6 +1424,29 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
}
+static const struct file_operations iio_buffer_fileops = {
+ .read = iio_buffer_read_outer,
+ .release = iio_buffer_chrdev_release,
+ .open = iio_buffer_chrdev_open,
+ .poll = iio_buffer_poll,
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = iio_buffer_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
+};
+
+void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev)
+{
+ struct iio_buffer *buffer = indio_dev->buffer;
+
+ if (!buffer)
+ return;
+
+ cdev_init(&buffer->chrdev, &iio_buffer_fileops);
+
+ indio_dev->chrdev = &buffer->chrdev;
+}
+
void iio_device_buffers_put(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index d74279efeca4..32e045c7f0c1 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1587,79 +1587,6 @@ struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
}
EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
-/**
- * iio_chrdev_open() - chrdev file open for buffer access and ioctls
- * @inode: Inode structure for identifying the device in the file system
- * @filp: File structure for iio device used to keep and later access
- * private data
- *
- * Return: 0 on success or -EBUSY if the device is already opened
- **/
-static int iio_chrdev_open(struct inode *inode, struct file *filp)
-{
- struct iio_dev *indio_dev = container_of(inode->i_cdev,
- struct iio_dev, chrdev);
-
- if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
- return -EBUSY;
-
- iio_device_get(indio_dev);
-
- filp->private_data = indio_dev;
-
- return 0;
-}
-
-/**
- * iio_chrdev_release() - chrdev file close buffer access and ioctls
- * @inode: Inode structure pointer for the char device
- * @filp: File structure pointer for the char device
- *
- * Return: 0 for successful release
- */
-static int iio_chrdev_release(struct inode *inode, struct file *filp)
-{
- struct iio_dev *indio_dev = container_of(inode->i_cdev,
- struct iio_dev, chrdev);
- clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
- iio_device_put(indio_dev);
-
- return 0;
-}
-
-/* Somewhat of a cross file organization violation - ioctls here are actually
- * event related */
-static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
-{
- struct iio_dev *indio_dev = filp->private_data;
- int __user *ip = (int __user *)arg;
- int fd;
-
- if (!indio_dev->info)
- return -ENODEV;
-
- if (cmd == IIO_GET_EVENT_FD_IOCTL) {
- fd = iio_event_getfd(indio_dev);
- if (fd < 0)
- return fd;
- if (copy_to_user(ip, &fd, sizeof(fd)))
- return -EFAULT;
- return 0;
- }
- return -EINVAL;
-}
-
-static const struct file_operations iio_buffer_fileops = {
- .read = iio_buffer_read_outer_addr,
- .release = iio_chrdev_release,
- .open = iio_chrdev_open,
- .poll = iio_buffer_poll_addr,
- .owner = THIS_MODULE,
- .llseek = noop_llseek,
- .unlocked_ioctl = iio_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
-};
-
static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
{
int i, j;
@@ -1708,15 +1635,6 @@ static void iio_device_free_chrdev_id(struct device *dev)
static const struct iio_buffer_setup_ops noop_ring_setup_ops;
-static const struct file_operations iio_event_fileops = {
- .owner = THIS_MODULE,
- .llseek = noop_llseek,
- .unlocked_ioctl = iio_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
- .open = iio_chrdev_open,
- .release = iio_chrdev_release,
-};
-
int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
{
int ret;
@@ -1764,20 +1682,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
indio_dev->setup_ops == NULL)
indio_dev->setup_ops = &noop_ring_setup_ops;
- if (indio_dev->buffer)
- cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
- else if (indio_dev->event_interface)
- cdev_init(&indio_dev->chrdev, &iio_event_fileops);
+ iio_device_buffer_attach_chrdev(indio_dev);
+
+ /* No chrdev attached from buffer, we go with event-only chrdev */
+ if (!indio_dev->chrdev)
+ iio_device_event_attach_chrdev(indio_dev);
- if (indio_dev->buffer || indio_dev->event_interface) {
- indio_dev->chrdev.owner = this_mod;
+ if (indio_dev->chrdev) {
+ indio_dev->chrdev->owner = this_mod;
ret = iio_device_alloc_chrdev_id(&indio_dev->dev);
if (ret)
goto error_unreg_eventset;
}
- ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
+ ret = cdev_device_add(indio_dev->chrdev, &indio_dev->dev);
if (ret)
goto error_free_chrdev_id;
@@ -1803,7 +1722,7 @@ EXPORT_SYMBOL(__iio_device_register);
**/
void iio_device_unregister(struct iio_dev *indio_dev)
{
- cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
+ cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
iio_device_free_chrdev_id(&indio_dev->dev);
mutex_lock(&indio_dev->info_exist_lock);
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 5b17c92d3b50..0674b2117c98 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -30,6 +30,8 @@
* @flags: file operations related flags including busy flag.
* @group: event interface sysfs attribute group
* @read_lock: lock to protect kfifo read operations
+ * @chrdev: associated chardev for this event
+ * @indio_dev: IIO device to which this event interface belongs to
*/
struct iio_event_interface {
wait_queue_head_t wait;
@@ -39,6 +41,9 @@ struct iio_event_interface {
unsigned long flags;
struct attribute_group group;
struct mutex read_lock;
+
+ struct cdev chrdev;
+ struct iio_dev *indio_dev;
};
bool iio_event_enabled(const struct iio_event_interface *ev_int)
@@ -182,7 +187,7 @@ static const struct file_operations iio_event_chrdev_fileops = {
.llseek = noop_llseek,
};
-int iio_event_getfd(struct iio_dev *indio_dev)
+static int iio_event_getfd(struct iio_dev *indio_dev)
{
struct iio_event_interface *ev_int = indio_dev->event_interface;
int fd;
@@ -215,6 +220,97 @@ int iio_event_getfd(struct iio_dev *indio_dev)
return fd;
}
+/**
+ * iio_chrdev_open() - chrdev file open for event ioctls
+ * @inode: Inode structure for identifying the device in the file system
+ * @filp: File structure for iio device used to keep and later access
+ * private data
+ *
+ * Return: 0 on success or -EBUSY if the device is already opened
+ **/
+static int iio_chrdev_open(struct inode *inode, struct file *filp)
+{
+ struct iio_event_interface *ev =
+ container_of(inode->i_cdev, struct iio_event_interface, chrdev);
+
+ if (test_and_set_bit(IIO_BUSY_BIT_POS, &ev->flags))
+ return -EBUSY;
+
+ iio_device_get(ev->indio_dev);
+
+ filp->private_data = ev;
+
+ return 0;
+}
+
+/**
+ * iio_chrdev_release() - chrdev file close for event ioctls
+ * @inode: Inode structure pointer for the char device
+ * @filp: File structure pointer for the char device
+ *
+ * Return: 0 for successful release
+ */
+static int iio_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct iio_event_interface *ev =
+ container_of(inode->i_cdev, struct iio_event_interface, chrdev);
+
+ clear_bit(IIO_BUSY_BIT_POS, &ev->flags);
+ iio_device_put(ev->indio_dev);
+
+ return 0;
+}
+
+long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ int __user *ip = (int __user *)arg;
+ int fd;
+
+ if (!indio_dev->info)
+ return -ENODEV;
+
+ if (cmd == IIO_GET_EVENT_FD_IOCTL) {
+ fd = iio_event_getfd(indio_dev);
+ if (fd < 0)
+ return fd;
+ if (copy_to_user(ip, &fd, sizeof(fd)))
+ return -EFAULT;
+ return 0;
+ }
+ return -EINVAL;
+}
+
+static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct iio_event_interface *ev = filp->private_data;
+
+ return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
+}
+
+static const struct file_operations iio_event_fileops = {
+ .release = iio_chrdev_release,
+ .open = iio_chrdev_open,
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = iio_event_ioctl_wrapper,
+ .compat_ioctl = compat_ptr_ioctl,
+};
+
+void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
+{
+ struct iio_event_interface *ev = indio_dev->event_interface;
+
+ if (!ev)
+ return;
+
+ cdev_init(&ev->chrdev, &iio_event_fileops);
+
+ ev->indio_dev = indio_dev;
+ indio_dev->chrdev = &ev->chrdev;
+}
+
static const char * const iio_ev_type_text[] = {
[IIO_EV_TYPE_THRESH] = "thresh",
[IIO_EV_TYPE_MAG] = "mag",
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 67d73d465e02..46fc977deae3 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: GPL-2.0 */
#ifndef _IIO_BUFFER_GENERIC_IMPL_H_
#define _IIO_BUFFER_GENERIC_IMPL_H_
+#include <linux/cdev.h>
#include <linux/sysfs.h>
#include <linux/kref.h>
@@ -72,6 +73,12 @@ struct iio_buffer {
/** @indio_dev: IIO device to which this buffer belongs to. */
struct iio_dev *indio_dev;
+ /** @chrdev: associated character device. */
+ struct cdev chrdev;
+
+ /** @file_ops_flags: file ops related flags including busy flag. */
+ unsigned long file_ops_flags;
+
/** @length: Number of datums in buffer. */
unsigned int length;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 5f9f439a4f01..52992be44e9e 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -516,10 +516,9 @@ struct iio_buffer_setup_ops {
* @info_exist_lock: [INTERN] lock to prevent use during removal
* @setup_ops: [DRIVER] callbacks to call before and after buffer
* enable/disable
- * @chrdev: [INTERN] associated character device
+ * @chrdev: [INTERN] reference to associated character device
* @groups: [INTERN] attribute groups
* @groupcounter: [INTERN] index of next attribute group
- * @flags: [INTERN] file ops related flags including busy flag.
* @debugfs_dentry: [INTERN] device specific debugfs dentry.
* @cached_reg_addr: [INTERN] cached register address for debugfs reads.
*/
@@ -559,12 +558,11 @@ struct iio_dev {
clockid_t clock_id;
struct mutex info_exist_lock;
const struct iio_buffer_setup_ops *setup_ops;
- struct cdev chrdev;
+ struct cdev *chrdev;
#define IIO_MAX_GROUPS 6
const struct attribute_group *groups[IIO_MAX_GROUPS + 1];
int groupcounter;
- unsigned long flags;
#if defined(CONFIG_DEBUG_FS)
struct dentry *debugfs_dentry;
unsigned cached_reg_addr;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (5 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 06/14] iio: buffer,event: duplicate chardev creation for buffers & events Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-24 16:45 ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism Alexandru Ardelean
` (7 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
The aim of this is to reduce the organization violation of ioctl() calls in
IIO core. Currently, since the chardev is split across files, event ioctl()
calls need to be called in buffer ioctl() calls.
The 'industrialio-core.c' file will provide a 'iio_device_ioctl()' which
will iterate over a list of ioctls registered with the IIO device. These
can be event ioctl() or buffer ioctl() calls, or something else.
This is needed, since there is currently one chardev per IIO device and
that is used for both event handling and reading from the buffer.
Each ioctl() will have to return a IIO_IOCTL_UNHANDLED code (which is
positive 1), if the ioctl() did not handle the call in any. This eliminates
any potential ambiguities; if we were to have used error codes it would
have been uncertain whether they were actual errors, or whether
the registered ioctl() doesn't service the command.
If any ioctl() returns 0, it was considered that it was serviced
successfully and the loop will exit.
One assumption for all registered ioctl() handlers is that they are
statically allocated, so the iio_device_unregister() which just remove all
of them from the device's ioctl() handler list.
Also, something that is a bit hard to do [at this point] and may not be
worth the effort of doing, is to check whether registered ioctl()
calls/commands overlap. This should be unlikely to happen, and should get
caught at review time. Though, new ioctl() calls would likely not be added
too often.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 14 ++++++++++++++
drivers/iio/industrialio-core.c | 33 +++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 2 ++
3 files changed, 49 insertions(+)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index a527a66be9e5..34c3e19229d8 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -17,6 +17,20 @@ struct iio_dev;
extern struct device_type iio_device_type;
+#define IIO_IOCTL_UNHANDLED 1
+struct iio_ioctl_handler {
+ struct list_head entry;
+ long (*ioctl)(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg);
+};
+
+long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg);
+
+void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
+ struct iio_ioctl_handler *h);
+void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
+
int __iio_add_chan_devattr(const char *postfix,
struct iio_chan_spec const *chan,
ssize_t (*func)(struct device *dev,
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 32e045c7f0c1..5df3af5e7dcb 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1534,6 +1534,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
}
dev_set_name(&dev->dev, "iio:device%d", dev->id);
INIT_LIST_HEAD(&dev->buffer_list);
+ INIT_LIST_HEAD(&dev->ioctl_handlers);
return dev;
}
@@ -1587,6 +1588,33 @@ struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
}
EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
+void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
+ struct iio_ioctl_handler *h)
+{
+ /* this assumes that all ioctl() handlers are statically allocated */
+ list_add_tail(&h->entry, &indio_dev->ioctl_handlers);
+}
+
+long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ struct iio_ioctl_handler *h;
+ int ret;
+
+ if (!indio_dev->info)
+ return -ENODEV;
+
+ list_for_each_entry(h, &indio_dev->ioctl_handlers, entry) {
+ ret = h->ioctl(indio_dev, filp, cmd, arg);
+ if (ret == 0)
+ return 0;
+ if (ret != IIO_IOCTL_UNHANDLED)
+ return ret;
+ }
+
+ return -EINVAL;
+}
+
static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
{
int i, j;
@@ -1722,6 +1750,8 @@ EXPORT_SYMBOL(__iio_device_register);
**/
void iio_device_unregister(struct iio_dev *indio_dev)
{
+ struct iio_ioctl_handler *h, *t;
+
cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
iio_device_free_chrdev_id(&indio_dev->dev);
@@ -1731,6 +1761,9 @@ void iio_device_unregister(struct iio_dev *indio_dev)
iio_disable_all_buffers(indio_dev);
+ list_for_each_entry_safe(h, t, &indio_dev->ioctl_handlers, entry)
+ list_del(&h->entry);
+
indio_dev->info = NULL;
iio_device_wakeup_eventset(indio_dev);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 52992be44e9e..b6ca8d85629e 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -488,6 +488,7 @@ struct iio_buffer_setup_ops {
* @currentmode: [DRIVER] current operating mode
* @dev: [DRIVER] device structure, should be assigned a parent
* and owner
+ * @ioctl_handlers: [INTERN] list of registered ioctl handlers
* @event_interface: [INTERN] event chrdevs associated with interrupt lines
* @buffer: [DRIVER] any buffer present
* @buffer_list: [INTERN] list of all buffers currently attached
@@ -529,6 +530,7 @@ struct iio_dev {
int modes;
int currentmode;
struct device dev;
+ struct list_head ioctl_handlers;
struct iio_event_interface *event_interface;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers
2020-05-08 13:53 ` [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers Alexandru Ardelean
@ 2020-05-24 16:45 ` Jonathan Cameron
2020-05-25 7:24 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-24 16:45 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, lars
On Fri, 8 May 2020 16:53:41 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> The aim of this is to reduce the organization violation of ioctl() calls in
> IIO core. Currently, since the chardev is split across files, event ioctl()
> calls need to be called in buffer ioctl() calls.
>
> The 'industrialio-core.c' file will provide a 'iio_device_ioctl()' which
> will iterate over a list of ioctls registered with the IIO device. These
> can be event ioctl() or buffer ioctl() calls, or something else.
> This is needed, since there is currently one chardev per IIO device and
> that is used for both event handling and reading from the buffer.
>
> Each ioctl() will have to return a IIO_IOCTL_UNHANDLED code (which is
> positive 1), if the ioctl() did not handle the call in any. This eliminates
> any potential ambiguities; if we were to have used error codes it would
> have been uncertain whether they were actual errors, or whether
> the registered ioctl() doesn't service the command.
>
> If any ioctl() returns 0, it was considered that it was serviced
> successfully and the loop will exit.
>
> One assumption for all registered ioctl() handlers is that they are
> statically allocated, so the iio_device_unregister() which just remove all
> of them from the device's ioctl() handler list.
>
> Also, something that is a bit hard to do [at this point] and may not be
> worth the effort of doing, is to check whether registered ioctl()
> calls/commands overlap. This should be unlikely to happen, and should get
> caught at review time. Though, new ioctl() calls would likely not be added
> too often.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
We seem to have dropped the locking in here. What am I missing that
stops us racing a remove with the ioctl? If there is a reason that
can't race, please add comments there so I don't wonders sometime in
the future.
The check on iio_dev->info means we won't start the ioctl if the
remove has been called, but if we switch immediately after that,
anything can happen before we start calling the ioctls.
J
> ---
> drivers/iio/iio_core.h | 14 ++++++++++++++
> drivers/iio/industrialio-core.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 2 ++
> 3 files changed, 49 insertions(+)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index a527a66be9e5..34c3e19229d8 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -17,6 +17,20 @@ struct iio_dev;
>
> extern struct device_type iio_device_type;
>
> +#define IIO_IOCTL_UNHANDLED 1
> +struct iio_ioctl_handler {
> + struct list_head entry;
> + long (*ioctl)(struct iio_dev *indio_dev, struct file *filp,
> + unsigned int cmd, unsigned long arg);
> +};
> +
> +long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
> + unsigned int cmd, unsigned long arg);
> +
> +void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> + struct iio_ioctl_handler *h);
> +void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
> +
> int __iio_add_chan_devattr(const char *postfix,
> struct iio_chan_spec const *chan,
> ssize_t (*func)(struct device *dev,
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 32e045c7f0c1..5df3af5e7dcb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1534,6 +1534,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> }
> dev_set_name(&dev->dev, "iio:device%d", dev->id);
> INIT_LIST_HEAD(&dev->buffer_list);
> + INIT_LIST_HEAD(&dev->ioctl_handlers);
>
> return dev;
> }
> @@ -1587,6 +1588,33 @@ struct iio_dev *devm_iio_device_alloc(struct device *dev, int sizeof_priv)
> }
> EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
>
> +void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> + struct iio_ioctl_handler *h)
> +{
> + /* this assumes that all ioctl() handlers are statically allocated */
> + list_add_tail(&h->entry, &indio_dev->ioctl_handlers);
> +}
> +
> +long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct iio_ioctl_handler *h;
> + int ret;
> +
> + if (!indio_dev->info)
> + return -ENODEV;
The locking is gone?
> +
> + list_for_each_entry(h, &indio_dev->ioctl_handlers, entry) {
> + ret = h->ioctl(indio_dev, filp, cmd, arg);
> + if (ret == 0)
> + return 0;
> + if (ret != IIO_IOCTL_UNHANDLED)
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> {
> int i, j;
> @@ -1722,6 +1750,8 @@ EXPORT_SYMBOL(__iio_device_register);
> **/
> void iio_device_unregister(struct iio_dev *indio_dev)
> {
> + struct iio_ioctl_handler *h, *t;
> +
> cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
> iio_device_free_chrdev_id(&indio_dev->dev);
>
> @@ -1731,6 +1761,9 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>
> iio_disable_all_buffers(indio_dev);
>
> + list_for_each_entry_safe(h, t, &indio_dev->ioctl_handlers, entry)
> + list_del(&h->entry);
> +
> indio_dev->info = NULL;
>
> iio_device_wakeup_eventset(indio_dev);
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 52992be44e9e..b6ca8d85629e 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -488,6 +488,7 @@ struct iio_buffer_setup_ops {
> * @currentmode: [DRIVER] current operating mode
> * @dev: [DRIVER] device structure, should be assigned a parent
> * and owner
> + * @ioctl_handlers: [INTERN] list of registered ioctl handlers
> * @event_interface: [INTERN] event chrdevs associated with interrupt lines
> * @buffer: [DRIVER] any buffer present
> * @buffer_list: [INTERN] list of all buffers currently attached
> @@ -529,6 +530,7 @@ struct iio_dev {
> int modes;
> int currentmode;
> struct device dev;
> + struct list_head ioctl_handlers;
>
> struct iio_event_interface *event_interface;
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers
2020-05-24 16:45 ` Jonathan Cameron
@ 2020-05-25 7:24 ` Ardelean, Alexandru
0 siblings, 0 replies; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-25 7:24 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, lars
On Sun, 2020-05-24 at 17:45 +0100, Jonathan Cameron wrote:
> [External]
>
> On Fri, 8 May 2020 16:53:41 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > The aim of this is to reduce the organization violation of ioctl() calls in
> > IIO core. Currently, since the chardev is split across files, event ioctl()
> > calls need to be called in buffer ioctl() calls.
> >
> > The 'industrialio-core.c' file will provide a 'iio_device_ioctl()' which
> > will iterate over a list of ioctls registered with the IIO device. These
> > can be event ioctl() or buffer ioctl() calls, or something else.
> > This is needed, since there is currently one chardev per IIO device and
> > that is used for both event handling and reading from the buffer.
> >
> > Each ioctl() will have to return a IIO_IOCTL_UNHANDLED code (which is
> > positive 1), if the ioctl() did not handle the call in any. This eliminates
> > any potential ambiguities; if we were to have used error codes it would
> > have been uncertain whether they were actual errors, or whether
> > the registered ioctl() doesn't service the command.
> >
> > If any ioctl() returns 0, it was considered that it was serviced
> > successfully and the loop will exit.
> >
> > One assumption for all registered ioctl() handlers is that they are
> > statically allocated, so the iio_device_unregister() which just remove all
> > of them from the device's ioctl() handler list.
> >
> > Also, something that is a bit hard to do [at this point] and may not be
> > worth the effort of doing, is to check whether registered ioctl()
> > calls/commands overlap. This should be unlikely to happen, and should get
> > caught at review time. Though, new ioctl() calls would likely not be added
> > too often.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> We seem to have dropped the locking in here. What am I missing that
> stops us racing a remove with the ioctl? If there is a reason that
> can't race, please add comments there so I don't wonders sometime in
> the future.
>
Yeah.
My bad about the locking.
I have too many branches for this stuff.
But for the chardev split, there's a branch that should have the locking in
place.
I'll re-check that, when sending only the chardev split anyway.
> The check on iio_dev->info means we won't start the ioctl if the
> remove has been called, but if we switch immediately after that,
> anything can happen before we start calling the ioctls.
>
> J
>
> > ---
> > drivers/iio/iio_core.h | 14 ++++++++++++++
> > drivers/iio/industrialio-core.c | 33 +++++++++++++++++++++++++++++++++
> > include/linux/iio/iio.h | 2 ++
> > 3 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index a527a66be9e5..34c3e19229d8 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -17,6 +17,20 @@ struct iio_dev;
> >
> > extern struct device_type iio_device_type;
> >
> > +#define IIO_IOCTL_UNHANDLED 1
> > +struct iio_ioctl_handler {
> > + struct list_head entry;
> > + long (*ioctl)(struct iio_dev *indio_dev, struct file *filp,
> > + unsigned int cmd, unsigned long arg);
> > +};
> > +
> > +long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > + unsigned int cmd, unsigned long arg);
> > +
> > +void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> > + struct iio_ioctl_handler *h);
> > +void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
> > +
> > int __iio_add_chan_devattr(const char *postfix,
> > struct iio_chan_spec const *chan,
> > ssize_t (*func)(struct device *dev,
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index 32e045c7f0c1..5df3af5e7dcb 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1534,6 +1534,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
> > }
> > dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > INIT_LIST_HEAD(&dev->buffer_list);
> > + INIT_LIST_HEAD(&dev->ioctl_handlers);
> >
> > return dev;
> > }
> > @@ -1587,6 +1588,33 @@ struct iio_dev *devm_iio_device_alloc(struct device
> > *dev, int sizeof_priv)
> > }
> > EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
> >
> > +void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> > + struct iio_ioctl_handler *h)
> > +{
> > + /* this assumes that all ioctl() handlers are statically allocated */
> > + list_add_tail(&h->entry, &indio_dev->ioctl_handlers);
> > +}
> > +
> > +long iio_device_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > + unsigned int cmd, unsigned long arg)
> > +{
> > + struct iio_ioctl_handler *h;
> > + int ret;
> > +
> > + if (!indio_dev->info)
> > + return -ENODEV;
>
> The locking is gone?
> > +
> > + list_for_each_entry(h, &indio_dev->ioctl_handlers, entry) {
> > + ret = h->ioctl(indio_dev, filp, cmd, arg);
> > + if (ret == 0)
> > + return 0;
> > + if (ret != IIO_IOCTL_UNHANDLED)
> > + return ret;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
> > {
> > int i, j;
> > @@ -1722,6 +1750,8 @@ EXPORT_SYMBOL(__iio_device_register);
> > **/
> > void iio_device_unregister(struct iio_dev *indio_dev)
> > {
> > + struct iio_ioctl_handler *h, *t;
> > +
> > cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
> > iio_device_free_chrdev_id(&indio_dev->dev);
> >
> > @@ -1731,6 +1761,9 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> >
> > iio_disable_all_buffers(indio_dev);
> >
> > + list_for_each_entry_safe(h, t, &indio_dev->ioctl_handlers, entry)
> > + list_del(&h->entry);
> > +
> > indio_dev->info = NULL;
> >
> > iio_device_wakeup_eventset(indio_dev);
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 52992be44e9e..b6ca8d85629e 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -488,6 +488,7 @@ struct iio_buffer_setup_ops {
> > * @currentmode: [DRIVER] current operating mode
> > * @dev: [DRIVER] device structure, should be assigned a parent
> > * and owner
> > + * @ioctl_handlers: [INTERN] list of registered ioctl handlers
> > * @event_interface: [INTERN] event chrdevs associated with interrupt
> > lines
> > * @buffer: [DRIVER] any buffer present
> > * @buffer_list: [INTERN] list of all buffers currently attached
> > @@ -529,6 +530,7 @@ struct iio_dev {
> > int modes;
> > int currentmode;
> > struct device dev;
> > + struct list_head ioctl_handlers;
> >
> > struct iio_event_interface *event_interface;
> >
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (6 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 07/14] iio: core: add simple centralized mechanism for ioctl() handlers Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-24 16:47 ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg Alexandru Ardelean
` (6 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
This change makes use of the new centralized ioctl() mechanism. The event
interface registers it's ioctl() handler to IIO device.
Both the buffer & event interface call 'iio_device_ioctl()', which should
take care of all of indio_dev's ioctl() calls.
Later, we may add per-buffer ioctl() calls, and since each buffer will get
it's own chardev, the buffer ioctl() handler will need a bit of tweaking
for the first/legacy buffer (i.e. indio_dev->buffer).
Also, those per-buffer ioctl() calls will not be registered with this
mechanism.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 3 ---
drivers/iio/industrialio-buffer.c | 2 +-
drivers/iio/industrialio-event.c | 14 ++++++++------
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 34c3e19229d8..f68de4af2738 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -54,9 +54,6 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
#ifdef CONFIG_IIO_BUFFER
struct poll_table_struct;
-long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
- unsigned int cmd, unsigned long arg);
-
void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 1400688f5e42..e7a847e7b103 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1199,7 +1199,7 @@ static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
if (!buffer || !buffer->access)
return -ENODEV;
- return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
+ return iio_device_ioctl(buffer->indio_dev, filep, cmd, arg);
}
static ssize_t iio_buffer_store_enable(struct device *dev,
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 0674b2117c98..1961c1d19370 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -32,6 +32,7 @@
* @read_lock: lock to protect kfifo read operations
* @chrdev: associated chardev for this event
* @indio_dev: IIO device to which this event interface belongs to
+ * @ioctl_handler: handler for event ioctl() calls
*/
struct iio_event_interface {
wait_queue_head_t wait;
@@ -44,6 +45,7 @@ struct iio_event_interface {
struct cdev chrdev;
struct iio_dev *indio_dev;
+ struct iio_ioctl_handler ioctl_handler;
};
bool iio_event_enabled(const struct iio_event_interface *ev_int)
@@ -261,15 +263,12 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
return 0;
}
-long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
+static long iio_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
unsigned int cmd, unsigned long arg)
{
int __user *ip = (int __user *)arg;
int fd;
- if (!indio_dev->info)
- return -ENODEV;
-
if (cmd == IIO_GET_EVENT_FD_IOCTL) {
fd = iio_event_getfd(indio_dev);
if (fd < 0)
@@ -278,7 +277,7 @@ long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
return -EFAULT;
return 0;
}
- return -EINVAL;
+ return IIO_IOCTL_UNHANDLED;
}
static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
@@ -286,7 +285,7 @@ static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
{
struct iio_event_interface *ev = filp->private_data;
- return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
+ return iio_device_ioctl(ev->indio_dev, filp, cmd, arg);
}
static const struct file_operations iio_event_fileops = {
@@ -308,7 +307,10 @@ void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
cdev_init(&ev->chrdev, &iio_event_fileops);
ev->indio_dev = indio_dev;
+ ev->ioctl_handler.ioctl = iio_event_ioctl;
indio_dev->chrdev = &ev->chrdev;
+
+ iio_device_ioctl_handler_register(indio_dev, &ev->ioctl_handler);
}
static const char * const iio_ev_type_text[] = {
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism
2020-05-08 13:53 ` [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism Alexandru Ardelean
@ 2020-05-24 16:47 ` Jonathan Cameron
2020-05-25 7:27 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-24 16:47 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, lars
On Fri, 8 May 2020 16:53:42 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> This change makes use of the new centralized ioctl() mechanism. The event
> interface registers it's ioctl() handler to IIO device.
> Both the buffer & event interface call 'iio_device_ioctl()', which should
> take care of all of indio_dev's ioctl() calls.
>
> Later, we may add per-buffer ioctl() calls, and since each buffer will get
> it's own chardev, the buffer ioctl() handler will need a bit of tweaking
> for the first/legacy buffer (i.e. indio_dev->buffer).
Do we have an ioctls that aren't safe if we just use them on 'any'
buffer? I don't think we do yet, though I guess we may have in the future.
> Also, those per-buffer ioctl() calls will not be registered with this
> mechanism.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/iio_core.h | 3 ---
> drivers/iio/industrialio-buffer.c | 2 +-
> drivers/iio/industrialio-event.c | 14 ++++++++------
> 3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 34c3e19229d8..f68de4af2738 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -54,9 +54,6 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
> #ifdef CONFIG_IIO_BUFFER
> struct poll_table_struct;
>
> -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> - unsigned int cmd, unsigned long arg);
> -
> void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
>
> int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 1400688f5e42..e7a847e7b103 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1199,7 +1199,7 @@ static long iio_buffer_ioctl(struct file *filep, unsigned int cmd,
> if (!buffer || !buffer->access)
> return -ENODEV;
>
> - return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> + return iio_device_ioctl(buffer->indio_dev, filep, cmd, arg);
> }
>
> static ssize_t iio_buffer_store_enable(struct device *dev,
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 0674b2117c98..1961c1d19370 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -32,6 +32,7 @@
> * @read_lock: lock to protect kfifo read operations
> * @chrdev: associated chardev for this event
> * @indio_dev: IIO device to which this event interface belongs to
> + * @ioctl_handler: handler for event ioctl() calls
> */
> struct iio_event_interface {
> wait_queue_head_t wait;
> @@ -44,6 +45,7 @@ struct iio_event_interface {
>
> struct cdev chrdev;
> struct iio_dev *indio_dev;
> + struct iio_ioctl_handler ioctl_handler;
> };
>
> bool iio_event_enabled(const struct iio_event_interface *ev_int)
> @@ -261,15 +263,12 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
> return 0;
> }
>
> -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> +static long iio_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> unsigned int cmd, unsigned long arg)
> {
> int __user *ip = (int __user *)arg;
> int fd;
>
> - if (!indio_dev->info)
> - return -ENODEV;
> -
> if (cmd == IIO_GET_EVENT_FD_IOCTL) {
> fd = iio_event_getfd(indio_dev);
> if (fd < 0)
> @@ -278,7 +277,7 @@ long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> return -EFAULT;
> return 0;
> }
> - return -EINVAL;
> + return IIO_IOCTL_UNHANDLED;
> }
>
> static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> @@ -286,7 +285,7 @@ static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> {
> struct iio_event_interface *ev = filp->private_data;
>
> - return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
> + return iio_device_ioctl(ev->indio_dev, filp, cmd, arg);
> }
>
> static const struct file_operations iio_event_fileops = {
> @@ -308,7 +307,10 @@ void iio_device_event_attach_chrdev(struct iio_dev *indio_dev)
> cdev_init(&ev->chrdev, &iio_event_fileops);
>
> ev->indio_dev = indio_dev;
> + ev->ioctl_handler.ioctl = iio_event_ioctl;
> indio_dev->chrdev = &ev->chrdev;
> +
> + iio_device_ioctl_handler_register(indio_dev, &ev->ioctl_handler);
> }
>
> static const char * const iio_ev_type_text[] = {
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism
2020-05-24 16:47 ` Jonathan Cameron
@ 2020-05-25 7:27 ` Ardelean, Alexandru
2020-05-31 15:20 ` Jonathan Cameron
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-25 7:27 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, lars
On Sun, 2020-05-24 at 17:47 +0100, Jonathan Cameron wrote:
> [External]
>
> On Fri, 8 May 2020 16:53:42 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > This change makes use of the new centralized ioctl() mechanism. The event
> > interface registers it's ioctl() handler to IIO device.
> > Both the buffer & event interface call 'iio_device_ioctl()', which should
> > take care of all of indio_dev's ioctl() calls.
> >
> > Later, we may add per-buffer ioctl() calls, and since each buffer will get
> > it's own chardev, the buffer ioctl() handler will need a bit of tweaking
> > for the first/legacy buffer (i.e. indio_dev->buffer).
>
> Do we have an ioctls that aren't safe if we just use them on 'any'
> buffer? I don't think we do yet, though I guess we may have in the future.
>
This was done in the idea that we may want to keep the /dev/iio:deviceX for
backwards compatibility.
But, it's undetermined yet how this will look.
I am currently working on more rework stuff [as you've seen].
I'd try to do some of the rework now, before adding more stuff [like
iio_dev_opaque].
To avoid adding this work, then having to rework it.
>
> > Also, those per-buffer ioctl() calls will not be registered with this
> > mechanism.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/iio/iio_core.h | 3 ---
> > drivers/iio/industrialio-buffer.c | 2 +-
> > drivers/iio/industrialio-event.c | 14 ++++++++------
> > 3 files changed, 9 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 34c3e19229d8..f68de4af2738 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -54,9 +54,6 @@ ssize_t iio_format_value(char *buf, unsigned int type, int
> > size, int *vals);
> > #ifdef CONFIG_IIO_BUFFER
> > struct poll_table_struct;
> >
> > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > - unsigned int cmd, unsigned long arg);
> > -
> > void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
> >
> > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index 1400688f5e42..e7a847e7b103 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1199,7 +1199,7 @@ static long iio_buffer_ioctl(struct file *filep,
> > unsigned int cmd,
> > if (!buffer || !buffer->access)
> > return -ENODEV;
> >
> > - return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> > + return iio_device_ioctl(buffer->indio_dev, filep, cmd, arg);
> > }
> >
> > static ssize_t iio_buffer_store_enable(struct device *dev,
> > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> > event.c
> > index 0674b2117c98..1961c1d19370 100644
> > --- a/drivers/iio/industrialio-event.c
> > +++ b/drivers/iio/industrialio-event.c
> > @@ -32,6 +32,7 @@
> > * @read_lock: lock to protect kfifo read operations
> > * @chrdev: associated chardev for this event
> > * @indio_dev: IIO device to which this event interface belongs
> > to
> > + * @ioctl_handler: handler for event ioctl() calls
> > */
> > struct iio_event_interface {
> > wait_queue_head_t wait;
> > @@ -44,6 +45,7 @@ struct iio_event_interface {
> >
> > struct cdev chrdev;
> > struct iio_dev *indio_dev;
> > + struct iio_ioctl_handler ioctl_handler;
> > };
> >
> > bool iio_event_enabled(const struct iio_event_interface *ev_int)
> > @@ -261,15 +263,12 @@ static int iio_chrdev_release(struct inode *inode,
> > struct file *filp)
> > return 0;
> > }
> >
> > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > +static long iio_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > unsigned int cmd, unsigned long arg)
> > {
> > int __user *ip = (int __user *)arg;
> > int fd;
> >
> > - if (!indio_dev->info)
> > - return -ENODEV;
> > -
> > if (cmd == IIO_GET_EVENT_FD_IOCTL) {
> > fd = iio_event_getfd(indio_dev);
> > if (fd < 0)
> > @@ -278,7 +277,7 @@ long iio_device_event_ioctl(struct iio_dev *indio_dev,
> > struct file *filp,
> > return -EFAULT;
> > return 0;
> > }
> > - return -EINVAL;
> > + return IIO_IOCTL_UNHANDLED;
> > }
> >
> > static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> > @@ -286,7 +285,7 @@ static long iio_event_ioctl_wrapper(struct file *filp,
> > unsigned int cmd,
> > {
> > struct iio_event_interface *ev = filp->private_data;
> >
> > - return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
> > + return iio_device_ioctl(ev->indio_dev, filp, cmd, arg);
> > }
> >
> > static const struct file_operations iio_event_fileops = {
> > @@ -308,7 +307,10 @@ void iio_device_event_attach_chrdev(struct iio_dev
> > *indio_dev)
> > cdev_init(&ev->chrdev, &iio_event_fileops);
> >
> > ev->indio_dev = indio_dev;
> > + ev->ioctl_handler.ioctl = iio_event_ioctl;
> > indio_dev->chrdev = &ev->chrdev;
> > +
> > + iio_device_ioctl_handler_register(indio_dev, &ev->ioctl_handler);
> > }
> >
> > static const char * const iio_ev_type_text[] = {
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism
2020-05-25 7:27 ` Ardelean, Alexandru
@ 2020-05-31 15:20 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-31 15:20 UTC (permalink / raw)
To: Ardelean, Alexandru; +Cc: linux-kernel, linux-iio, lars
On Mon, 25 May 2020 07:27:43 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> On Sun, 2020-05-24 at 17:47 +0100, Jonathan Cameron wrote:
> > [External]
> >
> > On Fri, 8 May 2020 16:53:42 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > This change makes use of the new centralized ioctl() mechanism. The event
> > > interface registers it's ioctl() handler to IIO device.
> > > Both the buffer & event interface call 'iio_device_ioctl()', which should
> > > take care of all of indio_dev's ioctl() calls.
> > >
> > > Later, we may add per-buffer ioctl() calls, and since each buffer will get
> > > it's own chardev, the buffer ioctl() handler will need a bit of tweaking
> > > for the first/legacy buffer (i.e. indio_dev->buffer).
> >
> > Do we have an ioctls that aren't safe if we just use them on 'any'
> > buffer? I don't think we do yet, though I guess we may have in the future.
> >
>
> This was done in the idea that we may want to keep the /dev/iio:deviceX for
> backwards compatibility.
> But, it's undetermined yet how this will look.
> I am currently working on more rework stuff [as you've seen].
> I'd try to do some of the rework now, before adding more stuff [like
> iio_dev_opaque].
> To avoid adding this work, then having to rework it.
Agreed. You are being so productive at the moment you may end up blocking
yourself. Best perhaps to slow down and clear some of the backlog.
Jonathan
>
> >
> > > Also, those per-buffer ioctl() calls will not be registered with this
> > > mechanism.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > drivers/iio/iio_core.h | 3 ---
> > > drivers/iio/industrialio-buffer.c | 2 +-
> > > drivers/iio/industrialio-event.c | 14 ++++++++------
> > > 3 files changed, 9 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 34c3e19229d8..f68de4af2738 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -54,9 +54,6 @@ ssize_t iio_format_value(char *buf, unsigned int type, int
> > > size, int *vals);
> > > #ifdef CONFIG_IIO_BUFFER
> > > struct poll_table_struct;
> > >
> > > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > - unsigned int cmd, unsigned long arg);
> > > -
> > > void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
> > >
> > > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index 1400688f5e42..e7a847e7b103 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1199,7 +1199,7 @@ static long iio_buffer_ioctl(struct file *filep,
> > > unsigned int cmd,
> > > if (!buffer || !buffer->access)
> > > return -ENODEV;
> > >
> > > - return iio_device_event_ioctl(buffer->indio_dev, filep, cmd, arg);
> > > + return iio_device_ioctl(buffer->indio_dev, filep, cmd, arg);
> > > }
> > >
> > > static ssize_t iio_buffer_store_enable(struct device *dev,
> > > diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-
> > > event.c
> > > index 0674b2117c98..1961c1d19370 100644
> > > --- a/drivers/iio/industrialio-event.c
> > > +++ b/drivers/iio/industrialio-event.c
> > > @@ -32,6 +32,7 @@
> > > * @read_lock: lock to protect kfifo read operations
> > > * @chrdev: associated chardev for this event
> > > * @indio_dev: IIO device to which this event interface belongs
> > > to
> > > + * @ioctl_handler: handler for event ioctl() calls
> > > */
> > > struct iio_event_interface {
> > > wait_queue_head_t wait;
> > > @@ -44,6 +45,7 @@ struct iio_event_interface {
> > >
> > > struct cdev chrdev;
> > > struct iio_dev *indio_dev;
> > > + struct iio_ioctl_handler ioctl_handler;
> > > };
> > >
> > > bool iio_event_enabled(const struct iio_event_interface *ev_int)
> > > @@ -261,15 +263,12 @@ static int iio_chrdev_release(struct inode *inode,
> > > struct file *filp)
> > > return 0;
> > > }
> > >
> > > -long iio_device_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > +static long iio_event_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > > unsigned int cmd, unsigned long arg)
> > > {
> > > int __user *ip = (int __user *)arg;
> > > int fd;
> > >
> > > - if (!indio_dev->info)
> > > - return -ENODEV;
> > > -
> > > if (cmd == IIO_GET_EVENT_FD_IOCTL) {
> > > fd = iio_event_getfd(indio_dev);
> > > if (fd < 0)
> > > @@ -278,7 +277,7 @@ long iio_device_event_ioctl(struct iio_dev *indio_dev,
> > > struct file *filp,
> > > return -EFAULT;
> > > return 0;
> > > }
> > > - return -EINVAL;
> > > + return IIO_IOCTL_UNHANDLED;
> > > }
> > >
> > > static long iio_event_ioctl_wrapper(struct file *filp, unsigned int cmd,
> > > @@ -286,7 +285,7 @@ static long iio_event_ioctl_wrapper(struct file *filp,
> > > unsigned int cmd,
> > > {
> > > struct iio_event_interface *ev = filp->private_data;
> > >
> > > - return iio_device_event_ioctl(ev->indio_dev, filp, cmd, arg);
> > > + return iio_device_ioctl(ev->indio_dev, filp, cmd, arg);
> > > }
> > >
> > > static const struct file_operations iio_event_fileops = {
> > > @@ -308,7 +307,10 @@ void iio_device_event_attach_chrdev(struct iio_dev
> > > *indio_dev)
> > > cdev_init(&ev->chrdev, &iio_event_fileops);
> > >
> > > ev->indio_dev = indio_dev;
> > > + ev->ioctl_handler.ioctl = iio_event_ioctl;
> > > indio_dev->chrdev = &ev->chrdev;
> > > +
> > > + iio_device_ioctl_handler_register(indio_dev, &ev->ioctl_handler);
> > > }
> > >
> > > static const char * const iio_ev_type_text[] = {
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (7 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 08/14] iio: core: use new common ioctl() mechanism Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-24 16:49 ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 10/14] iio: buffer: remove attrcount_orig var from sysfs creation Alexandru Ardelean
` (5 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
primary argument. This change converts to take an IIO buffer as a primary
argument.
That will allow the functions to get called for multiple buffers.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e7a847e7b103..6b1b5d5387bd 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1312,26 +1312,14 @@ static struct attribute *iio_buffer_attrs[] = {
&dev_attr_data_available.attr,
};
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
+static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
{
+ struct iio_dev *indio_dev = buffer->indio_dev;
struct iio_dev_attr *p;
struct attribute **attr;
- struct iio_buffer *buffer = indio_dev->buffer;
int ret, i, attrn, attrcount, attrcount_orig = 0;
const struct iio_chan_spec *channels;
- channels = indio_dev->channels;
- if (channels) {
- int ml = indio_dev->masklength;
-
- for (i = 0; i < indio_dev->num_channels; i++)
- ml = max(ml, channels[i].scan_index + 1);
- indio_dev->masklength = ml;
- }
-
- if (!buffer)
- return 0;
-
attrcount = 0;
if (buffer->attrs) {
while (buffer->attrs[attrcount] != NULL)
@@ -1411,19 +1399,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
return ret;
}
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
+ const struct iio_chan_spec *channels;
+ int i;
+
+ channels = indio_dev->channels;
+ if (channels) {
+ int ml = indio_dev->masklength;
+
+ for (i = 0; i < indio_dev->num_channels; i++)
+ ml = max(ml, channels[i].scan_index + 1);
+ indio_dev->masklength = ml;
+ }
if (!buffer)
- return;
+ return 0;
+
+ return __iio_buffer_alloc_sysfs_and_mask(buffer);
+}
+static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
iio_buffer_free_scanmask(buffer);
kfree(buffer->buffer_group.attrs);
kfree(buffer->scan_el_group.attrs);
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
}
+void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+{
+ struct iio_buffer *buffer = indio_dev->buffer;
+
+ if (!buffer)
+ return;
+
+ __iio_buffer_free_sysfs_and_mask(buffer);
+}
+
static const struct file_operations iio_buffer_fileops = {
.read = iio_buffer_read_outer,
.release = iio_buffer_chrdev_release,
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg
2020-05-08 13:53 ` [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg Alexandru Ardelean
@ 2020-05-24 16:49 ` Jonathan Cameron
2020-05-25 7:28 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-24 16:49 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, lars
On Fri, 8 May 2020 16:53:43 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> primary argument. This change converts to take an IIO buffer as a primary
> argument.
>
> That will allow the functions to get called for multiple buffers.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Looks good to me. We'll need this whatever the interface ends up being as
need the separate control infrastructure.
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index e7a847e7b103..6b1b5d5387bd 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1312,26 +1312,14 @@ static struct attribute *iio_buffer_attrs[] = {
> &dev_attr_data_available.attr,
> };
>
> -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
> {
> + struct iio_dev *indio_dev = buffer->indio_dev;
> struct iio_dev_attr *p;
> struct attribute **attr;
> - struct iio_buffer *buffer = indio_dev->buffer;
> int ret, i, attrn, attrcount, attrcount_orig = 0;
> const struct iio_chan_spec *channels;
>
> - channels = indio_dev->channels;
> - if (channels) {
> - int ml = indio_dev->masklength;
> -
> - for (i = 0; i < indio_dev->num_channels; i++)
> - ml = max(ml, channels[i].scan_index + 1);
> - indio_dev->masklength = ml;
> - }
> -
> - if (!buffer)
> - return 0;
> -
> attrcount = 0;
> if (buffer->attrs) {
> while (buffer->attrs[attrcount] != NULL)
> @@ -1411,19 +1399,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> return ret;
> }
>
> -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> {
> struct iio_buffer *buffer = indio_dev->buffer;
> + const struct iio_chan_spec *channels;
> + int i;
> +
> + channels = indio_dev->channels;
> + if (channels) {
> + int ml = indio_dev->masklength;
> +
> + for (i = 0; i < indio_dev->num_channels; i++)
> + ml = max(ml, channels[i].scan_index + 1);
> + indio_dev->masklength = ml;
> + }
>
> if (!buffer)
> - return;
> + return 0;
> +
> + return __iio_buffer_alloc_sysfs_and_mask(buffer);
> +}
>
> +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> +{
> iio_buffer_free_scanmask(buffer);
> kfree(buffer->buffer_group.attrs);
> kfree(buffer->scan_el_group.attrs);
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> }
>
> +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> +{
> + struct iio_buffer *buffer = indio_dev->buffer;
> +
> + if (!buffer)
> + return;
> +
> + __iio_buffer_free_sysfs_and_mask(buffer);
> +}
> +
> static const struct file_operations iio_buffer_fileops = {
> .read = iio_buffer_read_outer,
> .release = iio_buffer_chrdev_release,
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg
2020-05-24 16:49 ` Jonathan Cameron
@ 2020-05-25 7:28 ` Ardelean, Alexandru
2020-05-31 15:21 ` Jonathan Cameron
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-25 7:28 UTC (permalink / raw)
To: jic23; +Cc: linux-kernel, linux-iio, lars
On Sun, 2020-05-24 at 17:49 +0100, Jonathan Cameron wrote:
> [External]
>
> On Fri, 8 May 2020 16:53:43 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > primary argument. This change converts to take an IIO buffer as a primary
> > argument.
> >
> > That will allow the functions to get called for multiple buffers.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Looks good to me. We'll need this whatever the interface ends up being as
> need the separate control infrastructure.
I was wanting to split this into it's own patch at some point and send it now.
I'll probably do it.
>
> Jonathan
>
> > ---
> > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> > 1 file changed, 30 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index e7a847e7b103..6b1b5d5387bd 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1312,26 +1312,14 @@ static struct attribute *iio_buffer_attrs[] = {
> > &dev_attr_data_available.attr,
> > };
> >
> > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
> > {
> > + struct iio_dev *indio_dev = buffer->indio_dev;
> > struct iio_dev_attr *p;
> > struct attribute **attr;
> > - struct iio_buffer *buffer = indio_dev->buffer;
> > int ret, i, attrn, attrcount, attrcount_orig = 0;
> > const struct iio_chan_spec *channels;
> >
> > - channels = indio_dev->channels;
> > - if (channels) {
> > - int ml = indio_dev->masklength;
> > -
> > - for (i = 0; i < indio_dev->num_channels; i++)
> > - ml = max(ml, channels[i].scan_index + 1);
> > - indio_dev->masklength = ml;
> > - }
> > -
> > - if (!buffer)
> > - return 0;
> > -
> > attrcount = 0;
> > if (buffer->attrs) {
> > while (buffer->attrs[attrcount] != NULL)
> > @@ -1411,19 +1399,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > *indio_dev)
> > return ret;
> > }
> >
> > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > {
> > struct iio_buffer *buffer = indio_dev->buffer;
> > + const struct iio_chan_spec *channels;
> > + int i;
> > +
> > + channels = indio_dev->channels;
> > + if (channels) {
> > + int ml = indio_dev->masklength;
> > +
> > + for (i = 0; i < indio_dev->num_channels; i++)
> > + ml = max(ml, channels[i].scan_index + 1);
> > + indio_dev->masklength = ml;
> > + }
> >
> > if (!buffer)
> > - return;
> > + return 0;
> > +
> > + return __iio_buffer_alloc_sysfs_and_mask(buffer);
> > +}
> >
> > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > +{
> > iio_buffer_free_scanmask(buffer);
> > kfree(buffer->buffer_group.attrs);
> > kfree(buffer->scan_el_group.attrs);
> > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > }
> >
> > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > +{
> > + struct iio_buffer *buffer = indio_dev->buffer;
> > +
> > + if (!buffer)
> > + return;
> > +
> > + __iio_buffer_free_sysfs_and_mask(buffer);
> > +}
> > +
> > static const struct file_operations iio_buffer_fileops = {
> > .read = iio_buffer_read_outer,
> > .release = iio_buffer_chrdev_release,
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg
2020-05-25 7:28 ` Ardelean, Alexandru
@ 2020-05-31 15:21 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-31 15:21 UTC (permalink / raw)
To: Ardelean, Alexandru; +Cc: linux-kernel, linux-iio, lars
On Mon, 25 May 2020 07:28:18 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> On Sun, 2020-05-24 at 17:49 +0100, Jonathan Cameron wrote:
> > [External]
> >
> > On Fri, 8 May 2020 16:53:43 +0300
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as
> > > primary argument. This change converts to take an IIO buffer as a primary
> > > argument.
> > >
> > > That will allow the functions to get called for multiple buffers.
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > Looks good to me. We'll need this whatever the interface ends up being as
> > need the separate control infrastructure.
>
> I was wanting to split this into it's own patch at some point and send it now.
> I'll probably do it.
Great.
>
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++-----------
> > > 1 file changed, 30 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > > buffer.c
> > > index e7a847e7b103..6b1b5d5387bd 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -1312,26 +1312,14 @@ static struct attribute *iio_buffer_attrs[] = {
> > > &dev_attr_data_available.attr,
> > > };
> > >
> > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
> > > {
> > > + struct iio_dev *indio_dev = buffer->indio_dev;
> > > struct iio_dev_attr *p;
> > > struct attribute **attr;
> > > - struct iio_buffer *buffer = indio_dev->buffer;
> > > int ret, i, attrn, attrcount, attrcount_orig = 0;
> > > const struct iio_chan_spec *channels;
> > >
> > > - channels = indio_dev->channels;
> > > - if (channels) {
> > > - int ml = indio_dev->masklength;
> > > -
> > > - for (i = 0; i < indio_dev->num_channels; i++)
> > > - ml = max(ml, channels[i].scan_index + 1);
> > > - indio_dev->masklength = ml;
> > > - }
> > > -
> > > - if (!buffer)
> > > - return 0;
> > > -
> > > attrcount = 0;
> > > if (buffer->attrs) {
> > > while (buffer->attrs[attrcount] != NULL)
> > > @@ -1411,19 +1399,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev
> > > *indio_dev)
> > > return ret;
> > > }
> > >
> > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > {
> > > struct iio_buffer *buffer = indio_dev->buffer;
> > > + const struct iio_chan_spec *channels;
> > > + int i;
> > > +
> > > + channels = indio_dev->channels;
> > > + if (channels) {
> > > + int ml = indio_dev->masklength;
> > > +
> > > + for (i = 0; i < indio_dev->num_channels; i++)
> > > + ml = max(ml, channels[i].scan_index + 1);
> > > + indio_dev->masklength = ml;
> > > + }
> > >
> > > if (!buffer)
> > > - return;
> > > + return 0;
> > > +
> > > + return __iio_buffer_alloc_sysfs_and_mask(buffer);
> > > +}
> > >
> > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > > +{
> > > iio_buffer_free_scanmask(buffer);
> > > kfree(buffer->buffer_group.attrs);
> > > kfree(buffer->scan_el_group.attrs);
> > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > > }
> > >
> > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > +{
> > > + struct iio_buffer *buffer = indio_dev->buffer;
> > > +
> > > + if (!buffer)
> > > + return;
> > > +
> > > + __iio_buffer_free_sysfs_and_mask(buffer);
> > > +}
> > > +
> > > static const struct file_operations iio_buffer_fileops = {
> > > .read = iio_buffer_read_outer,
> > > .release = iio_buffer_chrdev_release,
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 10/14] iio: buffer: remove attrcount_orig var from sysfs creation
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (8 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 09/14] iio: buffer: split buffer sysfs creation to take buffer as primary arg Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 11/14] iio: buffer: add underlying device object and convert buffers to devices Alexandru Ardelean
` (4 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
The variable no longer does anything.
It's around because of some older code that required it to keep track of
certain attribute numbers.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 6b1b5d5387bd..efebf74a05af 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1317,7 +1317,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
struct iio_dev *indio_dev = buffer->indio_dev;
struct iio_dev_attr *p;
struct attribute **attr;
- int ret, i, attrn, attrcount, attrcount_orig = 0;
+ int ret, i, attrn, attrcount;
const struct iio_chan_spec *channels;
attrcount = 0;
@@ -1349,7 +1349,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
- attrcount = attrcount_orig;
+ attrcount = 0;
INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
channels = indio_dev->channels;
if (channels) {
@@ -1382,7 +1382,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
ret = -ENOMEM;
goto error_free_scan_mask;
}
- attrn = attrcount_orig;
+ attrn = 0;
list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 11/14] iio: buffer: add underlying device object and convert buffers to devices
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (9 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 10/14] iio: buffer: remove attrcount_orig var from sysfs creation Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 12/14] iio: buffer: symlink the scan_elements dir back into IIO device's dir Alexandru Ardelean
` (3 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
WIP
Currently, IIO is broken here.
Each buffer is now an object.
The part where this is broken is backwards compatibility.
We need to:
- convert all external buffer attributes to unpack to IIO buffers and not
IIO devices
- symlink the 'scan_elements' folder of the (first) IIO buffer device to
the IIO device
- symlink the chardev of the first IIO buffer device to the IIO device
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 11 ++--
drivers/iio/industrialio-buffer.c | 98 ++++++++++++++++++++++++-------
drivers/iio/industrialio-core.c | 39 +++++++-----
include/linux/iio/buffer_impl.h | 6 ++
include/linux/iio/iio.h | 2 +-
5 files changed, 116 insertions(+), 40 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index f68de4af2738..890577766f9b 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -54,10 +54,13 @@ ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
#ifdef CONFIG_IIO_BUFFER
struct poll_table_struct;
+int iio_device_alloc_chrdev_id(struct device *dev);
+void iio_device_free_chrdev_id(struct device *dev);
+
void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev);
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev);
+int iio_device_buffers_init(struct iio_dev *indio_dev);
+void iio_device_buffers_cleanup(struct iio_dev *indio_dev);
void iio_device_buffers_put(struct iio_dev *indio_dev);
@@ -68,12 +71,12 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
static inline void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev) {}
-static inline int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
+static inline int iio_device_buffers_init(struct iio_dev *indio_dev)
{
return 0;
}
-static inline void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) {}
+static inline void iio_device_buffers_cleanup(struct iio_dev *indio_dev) {}
static inline void iio_device_buffers_put(struct iio_dev *indio_dev) {}
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index efebf74a05af..6c35de7ebd9e 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1235,8 +1235,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
return (ret < 0) ? ret : len;
}
-static const char * const iio_scan_elements_group_name = "scan_elements";
-
static ssize_t iio_buffer_show_watermark(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -1312,7 +1310,7 @@ static struct attribute *iio_buffer_attrs[] = {
&dev_attr_data_available.attr,
};
-static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
+static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
{
struct iio_dev *indio_dev = buffer->indio_dev;
struct iio_dev_attr *p;
@@ -1344,10 +1342,9 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
- buffer->buffer_group.name = "buffer";
buffer->buffer_group.attrs = attr;
- indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
+ buffer->groups[0] = &buffer->buffer_group;
attrcount = 0;
INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
@@ -1373,7 +1370,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
goto error_cleanup_dynamic;
}
- buffer->scan_el_group.name = iio_scan_elements_group_name;
+ buffer->scan_el_group.name = "scan_elements";
buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
sizeof(buffer->scan_el_group.attrs[0]),
@@ -1386,7 +1383,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
- indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
+ buffer->groups[1] = &buffer->scan_el_group;
return 0;
@@ -1399,11 +1396,66 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
return ret;
}
-int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
+static void iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
+ iio_buffer_free_scanmask(buffer);
+ kfree(buffer->buffer_group.attrs);
+ kfree(buffer->scan_el_group.attrs);
+ iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+}
+
+static int iio_device_buffer_init(struct iio_dev *indio_dev,
+ struct iio_buffer *buffer,
+ int index)
+{
+ int ret;
+
+ ret = iio_buffer_alloc_sysfs_and_mask(buffer);
+ if (ret)
+ return ret;
+
+ ret = iio_device_alloc_chrdev_id(&buffer->dev);
+ if (ret)
+ goto error_free_sysfs_and_mask;
+
+ buffer->dev.parent = &indio_dev->dev;
+ buffer->dev.groups = buffer->groups;
+ buffer->dev.bus = &iio_bus_type;
+ device_initialize(&buffer->dev);
+
+ dev_set_name(&buffer->dev, "iio:buffer%d:%d",
+ indio_dev->id, index);
+
+ ret = cdev_device_add(&buffer->chrdev, &buffer->dev);
+ if (ret)
+ goto error_free_chrdev_id;
+
+ return 0;
+
+error_free_chrdev_id:
+ iio_device_free_chrdev_id(&buffer->dev);
+error_free_sysfs_and_mask:
+ iio_buffer_free_sysfs_and_mask(buffer);
+ return ret;
+}
+
+void iio_device_buffer_cleanup(struct iio_buffer *buffer)
+{
+ if (!buffer)
+ return;
+
+ iio_buffer_free_sysfs_and_mask(buffer);
+
+ cdev_device_del(&buffer->chrdev, &buffer->dev);
+
+ iio_device_free_chrdev_id(&buffer->dev);
+}
+
+int iio_device_buffers_init(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
const struct iio_chan_spec *channels;
- int i;
+ int i, ret;
channels = indio_dev->channels;
if (channels) {
@@ -1417,25 +1469,29 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
if (!buffer)
return 0;
- return __iio_buffer_alloc_sysfs_and_mask(buffer);
-}
+ ret = iio_device_buffer_init(indio_dev, buffer, 0);
+ if (ret)
+ return ret;
-static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
-{
- iio_buffer_free_scanmask(buffer);
- kfree(buffer->buffer_group.attrs);
- kfree(buffer->scan_el_group.attrs);
- iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+ ret = sysfs_create_link(&indio_dev->dev.kobj,
+ &buffer->dev.kobj, "buffer");
+ if (ret)
+ goto error_cleanup_buffers;
+
+ return 0;
+
+error_cleanup_buffers:
+ iio_device_buffer_cleanup(buffer);
+ return 0;
}
-void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
+void iio_device_buffers_cleanup(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
- if (!buffer)
- return;
+ sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
- __iio_buffer_free_sysfs_and_mask(buffer);
+ iio_device_buffer_cleanup(buffer);
}
static const struct file_operations iio_buffer_fileops = {
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 5df3af5e7dcb..b27fabf13e9c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1638,7 +1638,7 @@ static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
return 0;
}
-static int iio_device_alloc_chrdev_id(struct device *dev)
+int iio_device_alloc_chrdev_id(struct device *dev)
{
int id;
@@ -1654,7 +1654,7 @@ static int iio_device_alloc_chrdev_id(struct device *dev)
return 0;
}
-static void iio_device_free_chrdev_id(struct device *dev)
+void iio_device_free_chrdev_id(struct device *dev)
{
if (!dev->devt)
return;
@@ -1684,18 +1684,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
iio_device_register_debugfs(indio_dev);
- ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
- if (ret) {
- dev_err(indio_dev->dev.parent,
- "Failed to create buffer sysfs interfaces\n");
- goto error_unreg_debugfs;
- }
-
ret = iio_device_register_sysfs(indio_dev);
if (ret) {
dev_err(indio_dev->dev.parent,
"Failed to register sysfs interfaces\n");
- goto error_buffer_free_sysfs;
+ goto error_unreg_debugfs;
}
ret = iio_device_register_eventset(indio_dev);
if (ret) {
@@ -1712,6 +1705,26 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
iio_device_buffer_attach_chrdev(indio_dev);
+ if (indio_dev->chrdev) {
+ ret = device_add(&indio_dev->dev);
+
+ if (ret) {
+ put_device(&indio_dev->dev);
+ goto error_unreg_eventset;
+ }
+
+ ret = iio_device_buffers_init(indio_dev);
+ if (ret) {
+ device_del(&indio_dev->dev);
+
+ dev_err(indio_dev->dev.parent,
+ "Failed to create buffer sysfs interfaces\n");
+ goto error_unreg_eventset;
+ }
+
+ return 0;
+ }
+
/* No chrdev attached from buffer, we go with event-only chrdev */
if (!indio_dev->chrdev)
iio_device_event_attach_chrdev(indio_dev);
@@ -1736,8 +1749,6 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
iio_device_unregister_eventset(indio_dev);
error_free_sysfs:
iio_device_unregister_sysfs(indio_dev);
-error_buffer_free_sysfs:
- iio_buffer_free_sysfs_and_mask(indio_dev);
error_unreg_debugfs:
iio_device_unregister_debugfs(indio_dev);
return ret;
@@ -1752,6 +1763,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
{
struct iio_ioctl_handler *h, *t;
+ iio_device_buffers_cleanup(indio_dev);
+
cdev_device_del(indio_dev->chrdev, &indio_dev->dev);
iio_device_free_chrdev_id(&indio_dev->dev);
@@ -1770,8 +1783,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
iio_buffer_wakeup_poll(indio_dev);
mutex_unlock(&indio_dev->info_exist_lock);
-
- iio_buffer_free_sysfs_and_mask(indio_dev);
}
EXPORT_SYMBOL(iio_device_unregister);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 46fc977deae3..eca3fe630230 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -104,6 +104,12 @@ struct iio_buffer {
unsigned int watermark;
/* private: */
+ /* @dev: underlying device object. */
+ struct device dev;
+
+#define IIO_BUFFER_MAX_GROUP 2
+ const struct attribute_group *groups[IIO_BUFFER_MAX_GROUP + 1];
+
/* @scan_timestamp: Does the scan mode include a timestamp. */
bool scan_timestamp;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b6ca8d85629e..671f5818fa67 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -561,7 +561,7 @@ struct iio_dev {
struct mutex info_exist_lock;
const struct iio_buffer_setup_ops *setup_ops;
struct cdev *chrdev;
-#define IIO_MAX_GROUPS 6
+#define IIO_MAX_GROUPS 5
const struct attribute_group *groups[IIO_MAX_GROUPS + 1];
int groupcounter;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 12/14] iio: buffer: symlink the scan_elements dir back into IIO device's dir
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (10 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 11/14] iio: buffer: add underlying device object and convert buffers to devices Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-08 13:53 ` [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly Alexandru Ardelean
` (2 subsequent siblings)
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
WIP
Need to explicitly create the scan_elements dir to symlink it.
Admittedly we could try to use some kernfs logic to dig out the kobject of
the 'scan_elements' group, it doesn't seem to be done outside of the fs/
kernel directory.
We need to use the sysfs_() function suite, and that means creating it by
hand after the IIO buffer device was created and added so that we have a
parent kobject to attach this folder to.
After we create it by hand there is a kobject to which to symlink this to
back to the IIO device.
IIO still broken
What's left:
- convert all external buffer attributes to unpack to IIO buffers and not
IIO devices
- symlink the chardev of the first IIO buffer device to the IIO device
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 149 ++++++++++++++++++++++++------
include/linux/iio/buffer_impl.h | 7 +-
2 files changed, 122 insertions(+), 34 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 6c35de7ebd9e..b14281442387 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1310,15 +1310,11 @@ static struct attribute *iio_buffer_attrs[] = {
&dev_attr_data_available.attr,
};
-static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
+static int iio_buffer_alloc_sysfs(struct iio_buffer *buffer)
{
- struct iio_dev *indio_dev = buffer->indio_dev;
- struct iio_dev_attr *p;
struct attribute **attr;
- int ret, i, attrn, attrcount;
- const struct iio_chan_spec *channels;
+ int attrcount = 0;
- attrcount = 0;
if (buffer->attrs) {
while (buffer->attrs[attrcount] != NULL)
attrcount++;
@@ -1346,7 +1342,60 @@ static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
buffer->groups[0] = &buffer->buffer_group;
- attrcount = 0;
+ return 0;
+}
+
+static ssize_t iio_scan_el_dir_show(struct kobject *kobj,
+ struct attribute *attr, char *buf)
+{
+ struct device_attribute *dattr =
+ container_of(attr, struct device_attribute, attr);
+ struct iio_buffer *buffer =
+ container_of(kobj, struct iio_buffer, scan_el_dir);
+
+ if (!dattr->show)
+ return -EIO;
+
+ return dattr->show(&buffer->dev, dattr, buf);
+}
+
+static ssize_t iio_scan_el_dir_store(struct kobject *kobj,
+ struct attribute *attr,
+ const char *buf, size_t len)
+{
+ struct device_attribute *dattr =
+ container_of(attr, struct device_attribute, attr);
+ struct iio_buffer *buffer =
+ container_of(kobj, struct iio_buffer, scan_el_dir);
+
+ if (!dattr->store)
+ return -EIO;
+
+ return dattr->store(&buffer->dev, dattr, buf, len);
+}
+
+static struct sysfs_ops iio_scan_el_dir_ops = {
+ .show = iio_scan_el_dir_show,
+ .store = iio_scan_el_dir_store,
+};
+
+static void iio_buffer_dir_noop_release(struct kobject *kobj)
+{
+ /* nothing to do yet */
+}
+
+static struct kobj_type iio_scan_el_dir_ktype = {
+ .release = iio_buffer_dir_noop_release,
+ .sysfs_ops = &iio_scan_el_dir_ops,
+};
+
+static int iio_buffer_alloc_scan_sysfs(struct iio_buffer *buffer)
+{
+ struct iio_dev *indio_dev = buffer->indio_dev;
+ struct iio_dev_attr *p;
+ int ret, i;
+ const struct iio_chan_spec *channels;
+
INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
channels = indio_dev->channels;
if (channels) {
@@ -1359,7 +1408,6 @@ static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
&channels[i]);
if (ret < 0)
goto error_cleanup_dynamic;
- attrcount += ret;
if (channels[i].type == IIO_TIMESTAMP)
indio_dev->scan_index_timestamp =
channels[i].scan_index;
@@ -1370,37 +1418,52 @@ static int iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer)
goto error_cleanup_dynamic;
}
- buffer->scan_el_group.name = "scan_elements";
-
- buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
- sizeof(buffer->scan_el_group.attrs[0]),
- GFP_KERNEL);
- if (buffer->scan_el_group.attrs == NULL) {
- ret = -ENOMEM;
+ ret = kobject_init_and_add(&buffer->scan_el_dir,
+ &iio_scan_el_dir_ktype, &buffer->dev.kobj,
+ "scan_elements");
+ if (ret)
goto error_free_scan_mask;
- }
- attrn = 0;
- list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
- buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr;
- buffer->groups[1] = &buffer->scan_el_group;
+ i = 0;
+ list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) {
+ ret = sysfs_create_file(&buffer->scan_el_dir,
+ &p->dev_attr.attr);
+ if (ret)
+ goto error_remove_scan_el_dir;
+ i++;
+ }
return 0;
+error_remove_scan_el_dir:
+ list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) {
+ if (i == 0)
+ break;
+ sysfs_remove_file(&buffer->scan_el_dir, &p->dev_attr.attr);
+ i--;
+ }
+ kobject_put(&buffer->scan_el_dir);
error_free_scan_mask:
iio_buffer_free_scanmask(buffer);
error_cleanup_dynamic:
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
- kfree(buffer->buffer_group.attrs);
return ret;
}
-static void iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+static void iio_buffer_free_sysfs(struct iio_buffer *buffer)
{
iio_buffer_free_scanmask(buffer);
kfree(buffer->buffer_group.attrs);
- kfree(buffer->scan_el_group.attrs);
+}
+
+static void iio_buffer_free_scan_sysfs(struct iio_buffer *buffer)
+{
+ struct iio_dev_attr *p;
+
+ list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
+ sysfs_remove_file(&buffer->scan_el_dir, &p->dev_attr.attr);
+ kobject_put(&buffer->scan_el_dir);
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
}
@@ -1410,7 +1473,7 @@ static int iio_device_buffer_init(struct iio_dev *indio_dev,
{
int ret;
- ret = iio_buffer_alloc_sysfs_and_mask(buffer);
+ ret = iio_buffer_alloc_sysfs(buffer);
if (ret)
return ret;
@@ -1430,12 +1493,18 @@ static int iio_device_buffer_init(struct iio_dev *indio_dev,
if (ret)
goto error_free_chrdev_id;
+ ret = iio_buffer_alloc_scan_sysfs(buffer);
+ if (ret)
+ goto error_cdev_device_del;
+
return 0;
+error_cdev_device_del:
+ cdev_device_del(&buffer->chrdev, &buffer->dev);
error_free_chrdev_id:
iio_device_free_chrdev_id(&buffer->dev);
error_free_sysfs_and_mask:
- iio_buffer_free_sysfs_and_mask(buffer);
+ iio_buffer_free_sysfs(buffer);
return ret;
}
@@ -1444,13 +1513,33 @@ void iio_device_buffer_cleanup(struct iio_buffer *buffer)
if (!buffer)
return;
- iio_buffer_free_sysfs_and_mask(buffer);
+ iio_buffer_free_scan_sysfs(buffer);
cdev_device_del(&buffer->chrdev, &buffer->dev);
+ iio_buffer_free_sysfs(buffer);
+
iio_device_free_chrdev_id(&buffer->dev);
}
+static int iio_device_link_legacy_folders(struct iio_dev *indio_dev,
+ struct iio_buffer *buffer)
+{
+ int ret;
+
+ ret = sysfs_create_link(&indio_dev->dev.kobj,
+ &buffer->dev.kobj, "buffer");
+ if (ret)
+ return ret;
+
+ ret = sysfs_create_link(&indio_dev->dev.kobj,
+ &buffer->scan_el_dir, "scan_elements");
+ if (ret)
+ sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+
+ return ret;
+}
+
int iio_device_buffers_init(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
@@ -1473,14 +1562,13 @@ int iio_device_buffers_init(struct iio_dev *indio_dev)
if (ret)
return ret;
- ret = sysfs_create_link(&indio_dev->dev.kobj,
- &buffer->dev.kobj, "buffer");
+ ret = iio_device_link_legacy_folders(indio_dev, buffer);
if (ret)
- goto error_cleanup_buffers;
+ goto error_buffers_cleanup;
return 0;
-error_cleanup_buffers:
+error_buffers_cleanup:
iio_device_buffer_cleanup(buffer);
return 0;
}
@@ -1490,6 +1578,7 @@ void iio_device_buffers_cleanup(struct iio_dev *indio_dev)
struct iio_buffer *buffer = indio_dev->buffer;
sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+ sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
iio_device_buffer_cleanup(buffer);
}
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index eca3fe630230..e8203b6d51a1 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -107,7 +107,7 @@ struct iio_buffer {
/* @dev: underlying device object. */
struct device dev;
-#define IIO_BUFFER_MAX_GROUP 2
+#define IIO_BUFFER_MAX_GROUP 1
const struct attribute_group *groups[IIO_BUFFER_MAX_GROUP + 1];
/* @scan_timestamp: Does the scan mode include a timestamp. */
@@ -120,10 +120,9 @@ struct iio_buffer {
struct attribute_group buffer_group;
/*
- * @scan_el_group: Attribute group for those attributes not
- * created from the iio_chan_info array.
+ * @scan_el_dir: kobject for the 'scan_elements' directory
*/
- struct attribute_group scan_el_group;
+ struct kobject scan_el_dir;
/* @attrs: Standard attributes of the buffer. */
const struct attribute **attrs;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (11 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 12/14] iio: buffer: symlink the scan_elements dir back into IIO device's dir Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-24 17:28 ` Jonathan Cameron
2020-05-08 13:53 ` [RFC PATCH 14/14] iio: buffer: convert single buffer to list of buffers Alexandru Ardelean
2020-05-09 8:52 ` [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Lars-Peter Clausen
14 siblings, 1 reply; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
WIP
This change fixes how IIO buffer attributes get unpacked.
There could be a saner way to unpack these without needed to change the
drivers that attach buffer attributes incorrectly via
iio_buffer_set_attrs()
It seems that the design idea was that there is a single buffer per IIO
device, so all drivers attached buffer attributes for FIFO to the IIO
buffer.
Now with the change to add a device object to the IIO buffer, and shifting
around the device-attributes, a 'device' object unpacks to an IIO buffer
object, which needs some special handling.'
One idea to fix this, is to get rid of iio_buffer_set_attrs() somehow.
Or, to maybe allocate some wrapper device-attributes.
With this change, IIO still needs (to work with the older ABI):
- symlink the chardev of the first IIO buffer device to the IIO device
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/accel/adxl372.c | 6 ++-
drivers/iio/accel/bmc150-accel-core.c | 6 ++-
.../buffer/industrialio-buffer-dmaengine.c | 4 +-
drivers/iio/industrialio-buffer.c | 51 +++++++++++--------
include/linux/iio/buffer.h | 3 ++
5 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 60daf04ce188..528bce008671 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -745,7 +745,8 @@ static ssize_t adxl372_get_fifo_enabled(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
struct adxl372_state *st = iio_priv(indio_dev);
return sprintf(buf, "%d\n", st->fifo_mode);
@@ -755,7 +756,8 @@ static ssize_t adxl372_get_fifo_watermark(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
struct adxl372_state *st = iio_priv(indio_dev);
return sprintf(buf, "%d\n", st->watermark);
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index 121b4e89f038..c02287165980 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -763,7 +763,8 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
struct bmc150_accel_data *data = iio_priv(indio_dev);
int wm;
@@ -778,7 +779,8 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
struct bmc150_accel_data *data = iio_priv(indio_dev);
bool state;
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index 6dedf12b69a4..7728fdadcc3e 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -130,9 +130,9 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
struct dmaengine_buffer *dmaengine_buffer =
- iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
+ iio_buffer_to_dmaengine_buffer(buffer);
return sprintf(buf, "%zu\n", dmaengine_buffer->align);
}
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index b14281442387..aa2f46c0949f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -29,6 +29,19 @@ static const char * const iio_endian_prefix[] = {
[IIO_LE] = "le",
};
+struct iio_buffer *dev_to_iio_buffer(struct device *dev)
+{
+ return container_of(dev, struct iio_buffer, dev);
+}
+EXPORT_SYMBOL_GPL(dev_to_iio_buffer);
+
+struct iio_dev *iio_buffer_get_attached_iio_dev(struct iio_buffer *buffer)
+{
+ return buffer ? NULL : buffer->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_get_attached_iio_dev);
+
+
static bool iio_buffer_is_active(struct iio_buffer *buf)
{
return !list_empty(&buf->buffer_list);
@@ -324,9 +337,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
int ret;
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
/* Ensure ret is 0 or 1. */
ret = !!test_bit(to_iio_dev_attr(attr)->address,
@@ -436,10 +448,10 @@ static ssize_t iio_scan_el_store(struct device *dev,
const char *buf,
size_t len)
{
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = buffer->indio_dev;
int ret;
bool state;
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
ret = strtobool(buf, &state);
@@ -474,8 +486,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
return sprintf(buf, "%d\n", buffer->scan_timestamp);
}
@@ -485,9 +496,9 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
const char *buf,
size_t len)
{
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = buffer->indio_dev;
int ret;
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
bool state;
ret = strtobool(buf, &state);
@@ -563,8 +574,7 @@ static ssize_t iio_buffer_read_length(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
return sprintf(buf, "%d\n", buffer->length);
}
@@ -573,8 +583,8 @@ static ssize_t iio_buffer_write_length(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = buffer->indio_dev;
unsigned int val;
int ret;
@@ -606,8 +616,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
}
@@ -1207,10 +1216,10 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
const char *buf,
size_t len)
{
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = buffer->indio_dev;
int ret;
bool requested_state;
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
bool inlist;
ret = strtobool(buf, &requested_state);
@@ -1239,8 +1248,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
return sprintf(buf, "%u\n", buffer->watermark);
}
@@ -1250,8 +1258,8 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
const char *buf,
size_t len)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
+ struct iio_dev *indio_dev = buffer->indio_dev;
unsigned int val;
int ret;
@@ -1284,8 +1292,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = dev_to_iio_buffer(dev);
return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
}
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index fbba4093f06c..a688e98c694c 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,6 +11,9 @@
struct iio_buffer;
+struct iio_buffer *dev_to_iio_buffer(struct device *dev);
+struct iio_dev *iio_buffer_get_attached_iio_dev(struct iio_buffer *buffer);
+
void iio_buffer_set_attrs(struct iio_buffer *buffer,
const struct attribute **attrs);
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly
2020-05-08 13:53 ` [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly Alexandru Ardelean
@ 2020-05-24 17:28 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-24 17:28 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, lars
On Fri, 8 May 2020 16:53:47 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> WIP
>
> This change fixes how IIO buffer attributes get unpacked.
> There could be a saner way to unpack these without needed to change the
> drivers that attach buffer attributes incorrectly via
> iio_buffer_set_attrs()
>
> It seems that the design idea was that there is a single buffer per IIO
> device, so all drivers attached buffer attributes for FIFO to the IIO
> buffer.
Agreed - though I think what you have here is about the best we can
do and actually seems a sensible change in general.
There is a bit a mess in the abstraction, but I'm not totally sure
how to tidy that up.
These attrs are properties of the pre-demux buffer, whereas we
present them in the same directory as the post-demux buffer
(there of course can be other post-demux buffers such as in kernel
consumers). The scan_elements are a characteristic of this particular
post-demux buffer as it's length etc.
Feels like we should tidy that up, but we still potentially need
one set of these for each pre-demux buffer. Maybe we need
to explicitly call out hw fifos? It'll be a mess as sometimes
we have a single hw fifo, feeding multiple IIO devices (hopefully
just multiple buffers in the future).
Jonathan
>
> Now with the change to add a device object to the IIO buffer, and shifting
> around the device-attributes, a 'device' object unpacks to an IIO buffer
> object, which needs some special handling.'
>
> One idea to fix this, is to get rid of iio_buffer_set_attrs() somehow.
> Or, to maybe allocate some wrapper device-attributes.
>
> With this change, IIO still needs (to work with the older ABI):
> - symlink the chardev of the first IIO buffer device to the IIO device
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/accel/adxl372.c | 6 ++-
> drivers/iio/accel/bmc150-accel-core.c | 6 ++-
> .../buffer/industrialio-buffer-dmaengine.c | 4 +-
> drivers/iio/industrialio-buffer.c | 51 +++++++++++--------
> include/linux/iio/buffer.h | 3 ++
> 5 files changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
> index 60daf04ce188..528bce008671 100644
> --- a/drivers/iio/accel/adxl372.c
> +++ b/drivers/iio/accel/adxl372.c
> @@ -745,7 +745,8 @@ static ssize_t adxl372_get_fifo_enabled(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
> struct adxl372_state *st = iio_priv(indio_dev);
>
> return sprintf(buf, "%d\n", st->fifo_mode);
> @@ -755,7 +756,8 @@ static ssize_t adxl372_get_fifo_watermark(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
> struct adxl372_state *st = iio_priv(indio_dev);
>
> return sprintf(buf, "%d\n", st->watermark);
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 121b4e89f038..c02287165980 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -763,7 +763,8 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> int wm;
>
> @@ -778,7 +779,8 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = iio_buffer_get_attached_iio_dev(buffer);
> struct bmc150_accel_data *data = iio_priv(indio_dev);
> bool state;
>
> diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> index 6dedf12b69a4..7728fdadcc3e 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
> @@ -130,9 +130,9 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
> static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> struct dmaengine_buffer *dmaengine_buffer =
> - iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
> + iio_buffer_to_dmaengine_buffer(buffer);
>
> return sprintf(buf, "%zu\n", dmaengine_buffer->align);
> }
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index b14281442387..aa2f46c0949f 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -29,6 +29,19 @@ static const char * const iio_endian_prefix[] = {
> [IIO_LE] = "le",
> };
>
> +struct iio_buffer *dev_to_iio_buffer(struct device *dev)
> +{
> + return container_of(dev, struct iio_buffer, dev);
> +}
> +EXPORT_SYMBOL_GPL(dev_to_iio_buffer);
> +
> +struct iio_dev *iio_buffer_get_attached_iio_dev(struct iio_buffer *buffer)
> +{
> + return buffer ? NULL : buffer->indio_dev;
> +}
> +EXPORT_SYMBOL_GPL(iio_buffer_get_attached_iio_dev);
> +
> +
> static bool iio_buffer_is_active(struct iio_buffer *buf)
> {
> return !list_empty(&buf->buffer_list);
> @@ -324,9 +337,8 @@ static ssize_t iio_scan_el_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> int ret;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
>
> /* Ensure ret is 0 or 1. */
> ret = !!test_bit(to_iio_dev_attr(attr)->address,
> @@ -436,10 +448,10 @@ static ssize_t iio_scan_el_store(struct device *dev,
> const char *buf,
> size_t len)
> {
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = buffer->indio_dev;
> int ret;
> bool state;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>
> ret = strtobool(buf, &state);
> @@ -474,8 +486,7 @@ static ssize_t iio_scan_el_ts_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
>
> return sprintf(buf, "%d\n", buffer->scan_timestamp);
> }
> @@ -485,9 +496,9 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
> const char *buf,
> size_t len)
> {
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = buffer->indio_dev;
> int ret;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> bool state;
>
> ret = strtobool(buf, &state);
> @@ -563,8 +574,7 @@ static ssize_t iio_buffer_read_length(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
>
> return sprintf(buf, "%d\n", buffer->length);
> }
> @@ -573,8 +583,8 @@ static ssize_t iio_buffer_write_length(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t len)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = buffer->indio_dev;
> unsigned int val;
> int ret;
>
> @@ -606,8 +616,7 @@ static ssize_t iio_buffer_show_enable(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
>
> return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
> }
> @@ -1207,10 +1216,10 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> const char *buf,
> size_t len)
> {
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = buffer->indio_dev;
> int ret;
> bool requested_state;
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> bool inlist;
>
> ret = strtobool(buf, &requested_state);
> @@ -1239,8 +1248,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
>
> return sprintf(buf, "%u\n", buffer->watermark);
> }
> @@ -1250,8 +1258,8 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
> const char *buf,
> size_t len)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
> + struct iio_dev *indio_dev = buffer->indio_dev;
> unsigned int val;
> int ret;
>
> @@ -1284,8 +1292,7 @@ static ssize_t iio_dma_show_data_available(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_buffer *buffer = dev_to_iio_buffer(dev);
>
> return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
> }
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index fbba4093f06c..a688e98c694c 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -11,6 +11,9 @@
>
> struct iio_buffer;
>
> +struct iio_buffer *dev_to_iio_buffer(struct device *dev);
> +struct iio_dev *iio_buffer_get_attached_iio_dev(struct iio_buffer *buffer);
> +
> void iio_buffer_set_attrs(struct iio_buffer *buffer,
> const struct attribute **attrs);
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [RFC PATCH 14/14] iio: buffer: convert single buffer to list of buffers
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (12 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 13/14] iio: unpack all iio buffer attributes correctly Alexandru Ardelean
@ 2020-05-08 13:53 ` Alexandru Ardelean
2020-05-09 8:52 ` [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Lars-Peter Clausen
14 siblings, 0 replies; 39+ messages in thread
From: Alexandru Ardelean @ 2020-05-08 13:53 UTC (permalink / raw)
To: linux-iio, linux-kernel; +Cc: jic23, lars, Alexandru Ardelean
WIP
Time to add support for multiple buffers.
At this point it should be just managing a list.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 74 ++++++++++++++++++++++---------
drivers/iio/industrialio-core.c | 1 +
include/linux/iio/buffer_impl.h | 3 ++
include/linux/iio/iio.h | 2 +
4 files changed, 58 insertions(+), 22 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index aa2f46c0949f..c335484a6651 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -244,12 +244,13 @@ static int iio_buffer_chrdev_release(struct inode *inode, struct file *filp)
*/
void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *b;
- if (!buffer)
+ if (list_empty(&indio_dev->attached_buffers))
return;
- wake_up(&buffer->pollq);
+ list_for_each_entry(b, &indio_dev->attached_buffers, attached_entry)
+ wake_up(&b->pollq);
}
void iio_buffer_init(struct iio_buffer *buffer)
@@ -1547,10 +1548,27 @@ static int iio_device_link_legacy_folders(struct iio_dev *indio_dev,
return ret;
}
+static void __iio_device_buffers_cleanup(struct iio_dev *indio_dev, int idx)
+{
+ struct iio_buffer *b;
+
+ if (list_empty(&indio_dev->attached_buffers))
+ return;
+
+ list_for_each_entry(b, &indio_dev->attached_buffers, attached_entry) {
+ if (idx == 0)
+ break;
+
+ iio_device_buffer_cleanup(b);
+ if (idx > 0)
+ idx--;
+ }
+}
+
int iio_device_buffers_init(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
const struct iio_chan_spec *channels;
+ struct iio_buffer *b;
int i, ret;
channels = indio_dev->channels;
@@ -1562,32 +1580,38 @@ int iio_device_buffers_init(struct iio_dev *indio_dev)
indio_dev->masklength = ml;
}
- if (!buffer)
+ if (list_empty(&indio_dev->attached_buffers))
return 0;
- ret = iio_device_buffer_init(indio_dev, buffer, 0);
- if (ret)
- return ret;
+ i = 0;
+ list_for_each_entry(b, &indio_dev->attached_buffers, attached_entry) {
+ ret = iio_device_buffer_init(indio_dev, b, i);
+ if (ret)
+ goto error_buffers_cleanup;
- ret = iio_device_link_legacy_folders(indio_dev, buffer);
- if (ret)
- goto error_buffers_cleanup;
+ if (i == 0) {
+ ret = iio_device_link_legacy_folders(indio_dev, b);
+ if (ret) {
+ iio_device_buffer_cleanup(b);
+ goto error_buffers_cleanup;
+ }
+ }
+ i++;
+ }
return 0;
error_buffers_cleanup:
- iio_device_buffer_cleanup(buffer);
- return 0;
+ __iio_device_buffers_cleanup(indio_dev, i);
+ return ret;
}
void iio_device_buffers_cleanup(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
-
sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
- iio_device_buffer_cleanup(buffer);
+ __iio_device_buffers_cleanup(indio_dev, -1);
}
static const struct file_operations iio_buffer_fileops = {
@@ -1615,12 +1639,13 @@ void iio_device_buffer_attach_chrdev(struct iio_dev *indio_dev)
void iio_device_buffers_put(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *b;
- if (!buffer)
+ if (list_empty(&indio_dev->attached_buffers))
return;
- iio_buffer_put(buffer);
+ list_for_each_entry(b, &indio_dev->attached_buffers, attached_entry)
+ iio_buffer_put(b);
}
/**
@@ -1733,7 +1758,7 @@ void iio_buffer_put(struct iio_buffer *buffer)
EXPORT_SYMBOL_GPL(iio_buffer_put);
/**
- * iio_device_attach_buffer - Attach a buffer to a IIO device
+ * iio_device_attach_buffer_dir - Attach a buffer to a IIO device direction
* @indio_dev: The device the buffer should be attached to
* @buffer: The buffer to attach to the device
*
@@ -1744,8 +1769,13 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
void iio_device_attach_buffer(struct iio_dev *indio_dev,
struct iio_buffer *buffer)
{
- indio_dev->buffer = iio_buffer_get(buffer);
+ buffer = iio_buffer_get(buffer);
+ buffer->indio_dev = indio_dev;
+
+ /* keep this for legacy */
+ if (!indio_dev->buffer)
+ indio_dev->buffer = buffer;
- indio_dev->buffer->indio_dev = indio_dev;
+ list_add_tail(&buffer->attached_entry, &indio_dev->attached_buffers);
}
EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index b27fabf13e9c..067e5de76b6c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1534,6 +1534,7 @@ struct iio_dev *iio_device_alloc(int sizeof_priv)
}
dev_set_name(&dev->dev, "iio:device%d", dev->id);
INIT_LIST_HEAD(&dev->buffer_list);
+ INIT_LIST_HEAD(&dev->attached_buffers);
INIT_LIST_HEAD(&dev->ioctl_handlers);
return dev;
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index e8203b6d51a1..9ee3a7c0a657 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -130,6 +130,9 @@ struct iio_buffer {
/* @demux_bounce: Buffer for doing gather from incoming scan. */
void *demux_bounce;
+ /* @attached_buffers: Entry in the devices list of attached buffers. */
+ struct list_head attached_entry;
+
/* @buffer_list: Entry in the devices list of current buffers. */
struct list_head buffer_list;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 671f5818fa67..32ba1a303454 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -492,6 +492,7 @@ struct iio_buffer_setup_ops {
* @event_interface: [INTERN] event chrdevs associated with interrupt lines
* @buffer: [DRIVER] any buffer present
* @buffer_list: [INTERN] list of all buffers currently attached
+ * @attached_buffers: [INTERN] list of all attached buffers
* @scan_bytes: [INTERN] num bytes captured to be fed to buffer demux
* @mlock: [INTERN] lock used to prevent simultaneous device state
* changes
@@ -536,6 +537,7 @@ struct iio_dev {
struct iio_buffer *buffer;
struct list_head buffer_list;
+ struct list_head attached_buffers;
int scan_bytes;
struct mutex mlock;
--
2.17.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-08 13:53 [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Alexandru Ardelean
` (13 preceding siblings ...)
2020-05-08 13:53 ` [RFC PATCH 14/14] iio: buffer: convert single buffer to list of buffers Alexandru Ardelean
@ 2020-05-09 8:52 ` Lars-Peter Clausen
2020-05-10 10:09 ` Jonathan Cameron
14 siblings, 1 reply; 39+ messages in thread
From: Lars-Peter Clausen @ 2020-05-09 8:52 UTC (permalink / raw)
To: Alexandru Ardelean, linux-iio, linux-kernel; +Cc: jic23
On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> [...]
> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> This is because the 'buffer->dev.parent = &indio_dev->dev'.
> But I do feel this is correct.
> So, now I don't know whether to leave it like that or symlink to shorter
> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
> mostly to make the names unique. It would have looked weird to do
> '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
>
> So, now I'm thinking of whether all this is acceptable.
> Or what is acceptable?
> Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
> What else should I consider moving forward?
> What means forward?
> Where did I leave my beer?
Looking at how the /dev/ devices are named I think we can provide a name
that is different from the dev_name() of the device. Have a look at
device_get_devnode() in drivers/base/core.c. We should be able to
provide the name for the chardev through the devnode() callback.
While we are at this, do we want to move the new devices into an iio
subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-09 8:52 ` [RFC PATCH 00/14] iio: buffer: add support for multiple buffers Lars-Peter Clausen
@ 2020-05-10 10:09 ` Jonathan Cameron
2020-05-11 10:33 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-10 10:09 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: Alexandru Ardelean, linux-iio, linux-kernel
On Sat, 9 May 2020 10:52:14 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > [...]
> > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > But I do feel this is correct.
> > So, now I don't know whether to leave it like that or symlink to shorter
> > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
> > mostly to make the names unique. It would have looked weird to do
> > '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
> >
> > So, now I'm thinking of whether all this is acceptable.
> > Or what is acceptable?
> > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
> > What else should I consider moving forward?
> > What means forward?
> > Where did I leave my beer?
>
> Looking at how the /dev/ devices are named I think we can provide a name
> that is different from the dev_name() of the device. Have a look at
> device_get_devnode() in drivers/base/core.c. We should be able to
> provide the name for the chardev through the devnode() callback.
>
> While we are at this, do we want to move the new devices into an iio
> subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
Possibly on the folder. I can't for the life of me remember why I decided
not to do that the first time around - I'll leave it at the
mysterious "it may turn out to be harder than you'd think..."
Hopefully not ;)
Do we want to make the naming a bit more self describing, something like
iio/device0:buffer0? Given the legacy interface will be outside
the directory anyway, could even do
iio/device0/buffer0 with link to iio:device0
iio/device0/buffer1 with no legacy link.
Ah, the bikeshedding fun we have ahead of us!
I think this set is going to take too much thinking for a Sunday
so may take me a little while to do a proper review...
+ I have a few other side projects I want to hammer on today :)
Jonathan
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-10 10:09 ` Jonathan Cameron
@ 2020-05-11 10:33 ` Ardelean, Alexandru
2020-05-11 10:37 ` Lars-Peter Clausen
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-11 10:33 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> [External]
>
> On Sat, 9 May 2020 10:52:14 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > [...]
> > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> > > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > > But I do feel this is correct.
> > > So, now I don't know whether to leave it like that or symlink to shorter
> > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
> > > mostly to make the names unique. It would have looked weird to do
> > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
> > >
> > > So, now I'm thinking of whether all this is acceptable.
> > > Or what is acceptable?
> > > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
> > > What else should I consider moving forward?
> > > What means forward?
> > > Where did I leave my beer?
> >
> > Looking at how the /dev/ devices are named I think we can provide a name
> > that is different from the dev_name() of the device. Have a look at
> > device_get_devnode() in drivers/base/core.c. We should be able to
> > provide the name for the chardev through the devnode() callback.
> >
> > While we are at this, do we want to move the new devices into an iio
> > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
>
> Possibly on the folder. I can't for the life of me remember why I decided
> not to do that the first time around - I'll leave it at the
> mysterious "it may turn out to be harder than you'd think..."
> Hopefully not ;)
I was also thinking about the /dev/iio subfolder while doing this.
I can copy that from /dev/input
They seem to do it already.
I don't know how difficult it would be. But it looks like a good precedent.
My concern regarding going to use stuff from core [like device_get_devnode()] is
that it seems to bypass some layers of kernel.
If I do 'git grep device_get_devnode', I get:
drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid,
&gid, &tmp);
drivers/base/core.c: * device_get_devnode - path of device node file
drivers/base/core.c:const char *device_get_devnode(struct device *dev,
drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, &req.mode,
&req.uid, &req.gid, &tmp);
drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, NULL,
NULL, &tmp);
include/linux/device.h:extern const char *device_get_devnode(struct device *dev,
(END)
So, basically, most uses of device_get_devnode() are in core code, and I feel
that this may be sanctioned somewhere by some core people, if I do it.
I could be wrong, but if you disagree, I'll take your word for it.
I tried using devtmpfs_create_node() directly, and it worked, but again, doing
'git grep devtmpfs_create_node'
drivers/base/base.h:int devtmpfs_create_node(struct device *dev);
drivers/base/base.h:static inline int devtmpfs_create_node(struct device *dev) {
return 0; }
drivers/base/core.c: devtmpfs_create_node(dev);
drivers/base/devtmpfs.c:int devtmpfs_create_node(struct device *dev)
drivers/s390/char/hmcdrv_dev.c: * See: devtmpfs.c, function
devtmpfs_create_node()
Most calls are in core, and the one in hmcdrv driver isn't convincing enough to
do it.
In fact, some may consider it dangerous as it allows drivers/frameworks to use
it as precedent to do more name manipulation.
Again, if you guys say that this would be acceptable, I can use
device_get_devnode() and other stuff from core.
>
> Do we want to make the naming a bit more self describing, something like
> iio/device0:buffer0? Given the legacy interface will be outside
> the directory anyway, could even do
>
> iio/device0/buffer0 with link to iio:device0
> iio/device0/buffer1 with no legacy link.
>
> Ah, the bikeshedding fun we have ahead of us!
This may also depend on how much code it takes to implement these many levels of
folder hierarchy.
>
> I think this set is going to take too much thinking for a Sunday
> so may take me a little while to do a proper review...
> + I have a few other side projects I want to hammer on today :)
>
> Jonathan
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 10:33 ` Ardelean, Alexandru
@ 2020-05-11 10:37 ` Lars-Peter Clausen
2020-05-11 13:03 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Lars-Peter Clausen @ 2020-05-11 10:37 UTC (permalink / raw)
To: Ardelean, Alexandru, jic23; +Cc: linux-kernel, linux-iio
On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
>> [External]
>>
>> On Sat, 9 May 2020 10:52:14 +0200
>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
>>>> [...]
>>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
>>>> This is because the 'buffer->dev.parent = &indio_dev->dev'.
>>>> But I do feel this is correct.
>>>> So, now I don't know whether to leave it like that or symlink to shorter
>>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
>>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
>>>> mostly to make the names unique. It would have looked weird to do
>>>> '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
>>>>
>>>> So, now I'm thinking of whether all this is acceptable.
>>>> Or what is acceptable?
>>>> Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
>>>> What else should I consider moving forward?
>>>> What means forward?
>>>> Where did I leave my beer?
>>> Looking at how the /dev/ devices are named I think we can provide a name
>>> that is different from the dev_name() of the device. Have a look at
>>> device_get_devnode() in drivers/base/core.c. We should be able to
>>> provide the name for the chardev through the devnode() callback.
>>>
>>> While we are at this, do we want to move the new devices into an iio
>>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
>> Possibly on the folder. I can't for the life of me remember why I decided
>> not to do that the first time around - I'll leave it at the
>> mysterious "it may turn out to be harder than you'd think..."
>> Hopefully not ;)
> I was also thinking about the /dev/iio subfolder while doing this.
> I can copy that from /dev/input
> They seem to do it already.
> I don't know how difficult it would be. But it looks like a good precedent.
All you have to do is return "iio/..." from the devnode() callback.
>
> My concern regarding going to use stuff from core [like device_get_devnode()] is
> that it seems to bypass some layers of kernel.
> If I do 'git grep device_get_devnode', I get:
>
> drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid,
> &gid, &tmp);
> drivers/base/core.c: * device_get_devnode - path of device node file
> drivers/base/core.c:const char *device_get_devnode(struct device *dev,
> drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, &req.mode,
> &req.uid, &req.gid, &tmp);
> drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL, NULL,
> NULL, &tmp);
> include/linux/device.h:extern const char *device_get_devnode(struct device *dev,
> (END)
>
> So, basically, most uses of device_get_devnode() are in core code, and I feel
> that this may be sanctioned somewhere by some core people, if I do it.
> I could be wrong, but if you disagree, I'll take your word for it.
You are not supposed to use the function itself, you should implement
the devnode() callback for the IIO bus, which is then used by the core
device_get_devnode() function.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 10:37 ` Lars-Peter Clausen
@ 2020-05-11 13:03 ` Ardelean, Alexandru
2020-05-11 13:24 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-11 13:03 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> [External]
>
> On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > [External]
> > >
> > > On Sat, 9 May 2020 10:52:14 +0200
> > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > >
> > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > [...]
> > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > > > > But I do feel this is correct.
> > > > > So, now I don't know whether to leave it like that or symlink to
> > > > > shorter
> > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
> > > > > mostly to make the names unique. It would have looked weird to do
> > > > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
> > > > >
> > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > Or what is acceptable?
> > > > > Should I symlink 'iio:device3/iio:buffer3:0' -> 'iio:device3/buffer0'?
> > > > > What else should I consider moving forward?
> > > > > What means forward?
> > > > > Where did I leave my beer?
> > > > Looking at how the /dev/ devices are named I think we can provide a name
> > > > that is different from the dev_name() of the device. Have a look at
> > > > device_get_devnode() in drivers/base/core.c. We should be able to
> > > > provide the name for the chardev through the devnode() callback.
> > > >
> > > > While we are at this, do we want to move the new devices into an iio
> > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > Possibly on the folder. I can't for the life of me remember why I decided
> > > not to do that the first time around - I'll leave it at the
> > > mysterious "it may turn out to be harder than you'd think..."
> > > Hopefully not ;)
> > I was also thinking about the /dev/iio subfolder while doing this.
> > I can copy that from /dev/input
> > They seem to do it already.
> > I don't know how difficult it would be. But it looks like a good precedent.
>
> All you have to do is return "iio/..." from the devnode() callback.
I admit I did not look closely into drivers/input/input.c before mentioning this
as as good precedent.
But, I looks like /dev/inpput is a class.
While IIO devices are a bus_type devices.
Should we start implementing an IIO class? or?
>
> > My concern regarding going to use stuff from core [like
> > device_get_devnode()] is
> > that it seems to bypass some layers of kernel.
> > If I do 'git grep device_get_devnode', I get:
> >
> > drivers/base/core.c: name = device_get_devnode(dev, &mode, &uid,
> > &gid, &tmp);
> > drivers/base/core.c: * device_get_devnode - path of device node file
> > drivers/base/core.c:const char *device_get_devnode(struct device *dev,
> > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev,
> > &req.mode,
> > &req.uid, &req.gid, &tmp);
> > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL,
> > NULL,
> > NULL, &tmp);
> > include/linux/device.h:extern const char *device_get_devnode(struct device
> > *dev,
> > (END)
> >
> > So, basically, most uses of device_get_devnode() are in core code, and I
> > feel
> > that this may be sanctioned somewhere by some core people, if I do it.
> > I could be wrong, but if you disagree, I'll take your word for it.
> You are not supposed to use the function itself, you should implement
> the devnode() callback for the IIO bus, which is then used by the core
> device_get_devnode() function.
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 13:03 ` Ardelean, Alexandru
@ 2020-05-11 13:24 ` Ardelean, Alexandru
2020-05-11 13:58 ` Lars-Peter Clausen
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-11 13:24 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> [External]
>
> On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > [External]
> >
> > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > [External]
> > > >
> > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > >
> > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > [...]
> > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > > > > > But I do feel this is correct.
> > > > > > So, now I don't know whether to leave it like that or symlink to
> > > > > > shorter
> > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
> > > > > > mostly to make the names unique. It would have looked weird to do
> > > > > > '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
> > > > > >
> > > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > > Or what is acceptable?
> > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > 'iio:device3/buffer0'?
> > > > > > What else should I consider moving forward?
> > > > > > What means forward?
> > > > > > Where did I leave my beer?
> > > > > Looking at how the /dev/ devices are named I think we can provide a
> > > > > name
> > > > > that is different from the dev_name() of the device. Have a look at
> > > > > device_get_devnode() in drivers/base/core.c. We should be able to
> > > > > provide the name for the chardev through the devnode() callback.
> > > > >
> > > > > While we are at this, do we want to move the new devices into an iio
> > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > Possibly on the folder. I can't for the life of me remember why I
> > > > decided
> > > > not to do that the first time around - I'll leave it at the
> > > > mysterious "it may turn out to be harder than you'd think..."
> > > > Hopefully not ;)
> > > I was also thinking about the /dev/iio subfolder while doing this.
> > > I can copy that from /dev/input
> > > They seem to do it already.
> > > I don't know how difficult it would be. But it looks like a good
> > > precedent.
> >
> > All you have to do is return "iio/..." from the devnode() callback.
>
> I admit I did not look closely into drivers/input/input.c before mentioning
> this
> as as good precedent.
>
> But, I looks like /dev/inpput is a class.
> While IIO devices are a bus_type devices.
> Should we start implementing an IIO class? or?
What I should have highlighted [before] with this, is that there is no devnode()
callback for the bus_type [type].
>
>
> > > My concern regarding going to use stuff from core [like
> > > device_get_devnode()] is
> > > that it seems to bypass some layers of kernel.
> > > If I do 'git grep device_get_devnode', I get:
> > >
> > > drivers/base/core.c: name = device_get_devnode(dev, &mode,
> > > &uid,
> > > &gid, &tmp);
> > > drivers/base/core.c: * device_get_devnode - path of device node file
> > > drivers/base/core.c:const char *device_get_devnode(struct device *dev,
> > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev,
> > > &req.mode,
> > > &req.uid, &req.gid, &tmp);
> > > drivers/base/devtmpfs.c: req.name = device_get_devnode(dev, NULL,
> > > NULL,
> > > NULL, &tmp);
> > > include/linux/device.h:extern const char *device_get_devnode(struct device
> > > *dev,
> > > (END)
> > >
> > > So, basically, most uses of device_get_devnode() are in core code, and I
> > > feel
> > > that this may be sanctioned somewhere by some core people, if I do it.
> > > I could be wrong, but if you disagree, I'll take your word for it.
> > You are not supposed to use the function itself, you should implement
> > the devnode() callback for the IIO bus, which is then used by the core
> > device_get_devnode() function.
> >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 13:24 ` Ardelean, Alexandru
@ 2020-05-11 13:58 ` Lars-Peter Clausen
2020-05-11 14:56 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Lars-Peter Clausen @ 2020-05-11 13:58 UTC (permalink / raw)
To: Ardelean, Alexandru, jic23; +Cc: linux-kernel, linux-iio
On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
>> [External]
>>
>> On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
>>> [External]
>>>
>>> On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
>>>> On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
>>>>> [External]
>>>>>
>>>>> On Sat, 9 May 2020 10:52:14 +0200
>>>>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>
>>>>>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
>>>>>>> [...]
>>>>>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
>>>>>>> This is because the 'buffer->dev.parent = &indio_dev->dev'.
>>>>>>> But I do feel this is correct.
>>>>>>> So, now I don't know whether to leave it like that or symlink to
>>>>>>> shorter
>>>>>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
>>>>>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y' is
>>>>>>> mostly to make the names unique. It would have looked weird to do
>>>>>>> '/dev/buffer1' if I would have named the buffer devices 'bufferX'.
>>>>>>>
>>>>>>> So, now I'm thinking of whether all this is acceptable.
>>>>>>> Or what is acceptable?
>>>>>>> Should I symlink 'iio:device3/iio:buffer3:0' ->
>>>>>>> 'iio:device3/buffer0'?
>>>>>>> What else should I consider moving forward?
>>>>>>> What means forward?
>>>>>>> Where did I leave my beer?
>>>>>> Looking at how the /dev/ devices are named I think we can provide a
>>>>>> name
>>>>>> that is different from the dev_name() of the device. Have a look at
>>>>>> device_get_devnode() in drivers/base/core.c. We should be able to
>>>>>> provide the name for the chardev through the devnode() callback.
>>>>>>
>>>>>> While we are at this, do we want to move the new devices into an iio
>>>>>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
>>>>> Possibly on the folder. I can't for the life of me remember why I
>>>>> decided
>>>>> not to do that the first time around - I'll leave it at the
>>>>> mysterious "it may turn out to be harder than you'd think..."
>>>>> Hopefully not ;)
>>>> I was also thinking about the /dev/iio subfolder while doing this.
>>>> I can copy that from /dev/input
>>>> They seem to do it already.
>>>> I don't know how difficult it would be. But it looks like a good
>>>> precedent.
>>> All you have to do is return "iio/..." from the devnode() callback.
>> I admit I did not look closely into drivers/input/input.c before mentioning
>> this
>> as as good precedent.
>>
>> But, I looks like /dev/inpput is a class.
>> While IIO devices are a bus_type devices.
>> Should we start implementing an IIO class? or?
> What I should have highlighted [before] with this, is that there is no devnode()
> callback for the bus_type [type].
But there is one in device_type :)
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 13:58 ` Lars-Peter Clausen
@ 2020-05-11 14:56 ` Ardelean, Alexandru
2020-05-11 19:56 ` Lars-Peter Clausen
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-11 14:56 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> [External]
>
> On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > [External]
> > >
> > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > [External]
> > > >
> > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > [External]
> > > > > >
> > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > >
> > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > [...]
> > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
> > > > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > > > > > > > But I do feel this is correct.
> > > > > > > > So, now I don't know whether to leave it like that or symlink to
> > > > > > > > shorter
> > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > > > > The reason for naming the IIO buffer devices to 'iio:bufferX:Y'
> > > > > > > > is
> > > > > > > > mostly to make the names unique. It would have looked weird to
> > > > > > > > do
> > > > > > > > '/dev/buffer1' if I would have named the buffer devices
> > > > > > > > 'bufferX'.
> > > > > > > >
> > > > > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > > > > Or what is acceptable?
> > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > What else should I consider moving forward?
> > > > > > > > What means forward?
> > > > > > > > Where did I leave my beer?
> > > > > > > Looking at how the /dev/ devices are named I think we can provide
> > > > > > > a
> > > > > > > name
> > > > > > > that is different from the dev_name() of the device. Have a look
> > > > > > > at
> > > > > > > device_get_devnode() in drivers/base/core.c. We should be able to
> > > > > > > provide the name for the chardev through the devnode() callback.
> > > > > > >
> > > > > > > While we are at this, do we want to move the new devices into an
> > > > > > > iio
> > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > Possibly on the folder. I can't for the life of me remember why I
> > > > > > decided
> > > > > > not to do that the first time around - I'll leave it at the
> > > > > > mysterious "it may turn out to be harder than you'd think..."
> > > > > > Hopefully not ;)
> > > > > I was also thinking about the /dev/iio subfolder while doing this.
> > > > > I can copy that from /dev/input
> > > > > They seem to do it already.
> > > > > I don't know how difficult it would be. But it looks like a good
> > > > > precedent.
> > > > All you have to do is return "iio/..." from the devnode() callback.
> > > I admit I did not look closely into drivers/input/input.c before
> > > mentioning
> > > this
> > > as as good precedent.
> > >
> > > But, I looks like /dev/inpput is a class.
> > > While IIO devices are a bus_type devices.
> > > Should we start implementing an IIO class? or?
> > What I should have highlighted [before] with this, is that there is no
> > devnode()
> > callback for the bus_type [type].
> But there is one in device_type :)
Many thanks :)
That worked nicely.
I now have:
root@analog:~# ls /dev/iio/*
/dev/iio/iio:device0 /dev/iio/iio:device1
/dev/iio/device3:
buffer0 buffer1 buffer2 buffer3
/dev/iio/device4:
buffer0
It looks like I can shift these around as needed.
This is just an experiment.
I managed to move the iio devices under /dev/iio, though probably the IIO
devices will still be around as /dev/iio:deviceX for legacy reasons.
Two things remain unresolved.
1. The name of the IIO buffer device.
root@analog:/sys/bus/iio/devices# ls iio\:device3/
buffer in_voltage0_test_mode name
events in_voltage1_test_mode of_node
iio:buffer:3:0 in_voltage_sampling_frequency power
iio:buffer:3:1 in_voltage_scale scan_elements
iio:buffer:3:2 in_voltage_scale_available subsystem
iio:buffer:3:3 in_voltage_test_mode_available uevent
Right now, each buffer device is named 'iio:buffer:X:Y'.
One suggesttion was 'iio:deviceX:bufferY'
I'm suspecting the latter is preferred as when you sort the folders, buffers
come right after the iio:deviceX folders in /sys/bus/iio/devices.
I don't feel it matters much the device name of the IIO buffer if we symlink it
to a shorter form.
I'm guessing, we symlink these devices to short-hand 'bufferY' folders in each
'iio:deviceX'?
So, you'd get something like:
drwxr-xr-x 8 root root 0 May 11 14:40 .
drwxr-xr-x 4 root root 0 May 11 14:35 ..
lrwxrwxrwx 1 root root 0 May 11 14:35 buffer -> iio:buffer:3:0
lrwxrwxrwx 4 root root 0 May 11 14:35 buffer0 -> iio:buffer:3:0
lrwxrwxrwx 4 root root 0 May 11 14:35 buffer1 -> iio:buffer:3:1
lrwxrwxrwx 4 root root 0 May 11 14:35 buffer2 -> iio:buffer:3:2
lrwxrwxrwx 4 root root 0 May 11 14:35 buffer3 -> iio:buffer:3:3
drwxrwxrwx 2 root root 0 May 11 14:35 events
drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:0
drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:1
drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:2
drwxrwxrwx 4 root root 0 May 11 14:35 iio:buffer:3:3
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage0_test_mode
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage1_test_mode
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_sampling_frequency
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_scale
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_scale_available
-rw-rw-rw- 1 root root 4096 May 11 14:35 in_voltage_test_mode_available
-rw-rw-rw- 1 root root 4096 May 11 14:35 name
lrwxrwxrwx 1 root root 0 May 11 14:35 of_node ->
../../../../../firmware/devicetree/base/fpga-axi@0/axi-ad9680-hpc@44a10000
drwxrwxrwx 2 root root 0 May 11 14:35 power
lrwxrwxrwx 1 root root 0 May 11 14:35 scan_elements ->
iio:buffer:3:0/scan_elements
lrwxrwxrwx 1 root root 0 May 11 14:35 subsystem -> ../../../../../bus/iio
-rw-rw-rw- 1 root root 4096 May 11 14:35 uevent
1a. /sys/bus/iio/devices looks like this:
iio:buffer:3:0 (this would become iio:device3:buffer0 )
iio:buffer:3:1
iio:buffer
:3:2
iio:buffer:3:3
iio:buffer:4:0
iio:device0
iio:device1
iio:device2
iio:device3
iio:
device4
One minor issue here is that the buffers get listed in the /sys/bus/iio/devices
folder, because I'm adding them to the iio bus, to be able to get a chardev
[from the pre-allocated chardev region of IIO].
libiio gets a little confused, as it sees these buffers are IIO buffer capable
devices;
iio:buffer:3:0: (buffer capable)
2 channels found:
voltage0: (input, index: 0, format: le:S14/16>>0)
voltage1: (input, index: 1, format: le:S14/16>>0)
5 device-specific attributes found:
attr 0: data_available value: 0
attr 1: enable value: 0
attr 2: length value: 4096
attr 3: length_align_bytes value: 8
attr 4: watermark value: 2048
iio:buffer:3:1: (buffer capable)
Hopefully, this is not a big problem, but let's see.
2. I know this is [still] stupid now; but any suggestions one how to symlink
/dev/iio:device3 -> /dev/iio/device3/buffer0 ?
Regarding this one, I may try a few things, but any suggestion is welcome.
Thanks
Alex
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 14:56 ` Ardelean, Alexandru
@ 2020-05-11 19:56 ` Lars-Peter Clausen
2020-05-12 6:26 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Lars-Peter Clausen @ 2020-05-11 19:56 UTC (permalink / raw)
To: Ardelean, Alexandru, jic23; +Cc: linux-kernel, linux-iio
On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
>> [External]
>>
>> On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
>>> On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
>>>> [External]
>>>>
>>>> On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
>>>>> [External]
>>>>>
>>>>> On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
>>>>>> On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
>>>>>>> [External]
>>>>>>>
>>>>>>> On Sat, 9 May 2020 10:52:14 +0200
>>>>>>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>>>
>>>>>>>> On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
>>>>>>>>> [...]
>>>>>>>>> What I don't like, is that iio:device3 has iio:buffer3:0 (to 3).
>>>>>>>>> This is because the 'buffer->dev.parent = &indio_dev->dev'.
>>>>>>>>> But I do feel this is correct.
>>>>>>>>> So, now I don't know whether to leave it like that or symlink to
>>>>>>>>> shorter
>>>>>>>>> versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
>>>>>>>>> The reason for naming the IIO buffer devices to 'iio:bufferX:Y'
>>>>>>>>> is
>>>>>>>>> mostly to make the names unique. It would have looked weird to
>>>>>>>>> do
>>>>>>>>> '/dev/buffer1' if I would have named the buffer devices
>>>>>>>>> 'bufferX'.
>>>>>>>>>
>>>>>>>>> So, now I'm thinking of whether all this is acceptable.
>>>>>>>>> Or what is acceptable?
>>>>>>>>> Should I symlink 'iio:device3/iio:buffer3:0' ->
>>>>>>>>> 'iio:device3/buffer0'?
>>>>>>>>> What else should I consider moving forward?
>>>>>>>>> What means forward?
>>>>>>>>> Where did I leave my beer?
>>>>>>>> Looking at how the /dev/ devices are named I think we can provide
>>>>>>>> a
>>>>>>>> name
>>>>>>>> that is different from the dev_name() of the device. Have a look
>>>>>>>> at
>>>>>>>> device_get_devnode() in drivers/base/core.c. We should be able to
>>>>>>>> provide the name for the chardev through the devnode() callback.
>>>>>>>>
>>>>>>>> While we are at this, do we want to move the new devices into an
>>>>>>>> iio
>>>>>>>> subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
>>>>>>> Possibly on the folder. I can't for the life of me remember why I
>>>>>>> decided
>>>>>>> not to do that the first time around - I'll leave it at the
>>>>>>> mysterious "it may turn out to be harder than you'd think..."
>>>>>>> Hopefully not ;)
>>>>>> I was also thinking about the /dev/iio subfolder while doing this.
>>>>>> I can copy that from /dev/input
>>>>>> They seem to do it already.
>>>>>> I don't know how difficult it would be. But it looks like a good
>>>>>> precedent.
>>>>> All you have to do is return "iio/..." from the devnode() callback.
>>>> I admit I did not look closely into drivers/input/input.c before
>>>> mentioning
>>>> this
>>>> as as good precedent.
>>>>
>>>> But, I looks like /dev/inpput is a class.
>>>> While IIO devices are a bus_type devices.
>>>> Should we start implementing an IIO class? or?
>>> What I should have highlighted [before] with this, is that there is no
>>> devnode()
>>> callback for the bus_type [type].
>> But there is one in device_type :)
> Many thanks :)
> That worked nicely.
>
> I now have:
>
> root@analog:~# ls /dev/iio/*
> /dev/iio/iio:device0 /dev/iio/iio:device1
>
> /dev/iio/device3:
> buffer0 buffer1 buffer2 buffer3
>
> /dev/iio/device4:
> buffer0
>
>
> It looks like I can shift these around as needed.
> This is just an experiment.
> I managed to move the iio devices under /dev/iio, though probably the IIO
> devices will still be around as /dev/iio:deviceX for legacy reasons.
>
> Two things remain unresolved.
> 1. The name of the IIO buffer device.
>
> root@analog:/sys/bus/iio/devices# ls iio\:device3/
> buffer in_voltage0_test_mode name
> events in_voltage1_test_mode of_node
> iio:buffer:3:0 in_voltage_sampling_frequency power
> iio:buffer:3:1 in_voltage_scale scan_elements
> iio:buffer:3:2 in_voltage_scale_available subsystem
> iio:buffer:3:3 in_voltage_test_mode_available uevent
>
>
> Right now, each buffer device is named 'iio:buffer:X:Y'.
> One suggesttion was 'iio:deviceX:bufferY'
> I'm suspecting the latter is preferred as when you sort the folders, buffers
> come right after the iio:deviceX folders in /sys/bus/iio/devices.
>
> I don't feel it matters much the device name of the IIO buffer if we symlink it
> to a shorter form.
>
> I'm guessing, we symlink these devices to short-hand 'bufferY' folders in each
> 'iio:deviceX'?
I think that would be a bit excessive. Only for the legacy buffer we
need to have a symlink.
> [...]
> 2. I know this is [still] stupid now; but any suggestions one how to symlink
> /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
>
Does not seem to be possible. Userspace will have to take care of it.
This means we need to keep legacy devices in /dev/ and only new buffers
in /dev/iio/.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-11 19:56 ` Lars-Peter Clausen
@ 2020-05-12 6:26 ` Ardelean, Alexandru
2020-05-16 13:08 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-12 6:26 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:
> [External]
>
> On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> > > [External]
> > >
> > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > > > [External]
> > > > >
> > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > > > [External]
> > > > > >
> > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > > > [External]
> > > > > > > >
> > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > >
> > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > > > [...]
> > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0 (to
> > > > > > > > > > 3).
> > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev->dev'.
> > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > symlink to
> > > > > > > > > > shorter
> > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > is
> > > > > > > > > > mostly to make the names unique. It would have looked weird
> > > > > > > > > > to
> > > > > > > > > > do
> > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices
> > > > > > > > > > 'bufferX'.
> > > > > > > > > >
> > > > > > > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > > > > > > Or what is acceptable?
> > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > What means forward?
> > > > > > > > > > Where did I leave my beer?
> > > > > > > > > Looking at how the /dev/ devices are named I think we can
> > > > > > > > > provide
> > > > > > > > > a
> > > > > > > > > name
> > > > > > > > > that is different from the dev_name() of the device. Have a
> > > > > > > > > look
> > > > > > > > > at
> > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be able
> > > > > > > > > to
> > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > callback.
> > > > > > > > >
> > > > > > > > > While we are at this, do we want to move the new devices into
> > > > > > > > > an
> > > > > > > > > iio
> > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > > > Possibly on the folder. I can't for the life of me remember why
> > > > > > > > I
> > > > > > > > decided
> > > > > > > > not to do that the first time around - I'll leave it at the
> > > > > > > > mysterious "it may turn out to be harder than you'd think..."
> > > > > > > > Hopefully not ;)
> > > > > > > I was also thinking about the /dev/iio subfolder while doing this.
> > > > > > > I can copy that from /dev/input
> > > > > > > They seem to do it already.
> > > > > > > I don't know how difficult it would be. But it looks like a good
> > > > > > > precedent.
> > > > > > All you have to do is return "iio/..." from the devnode() callback.
> > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > mentioning
> > > > > this
> > > > > as as good precedent.
> > > > >
> > > > > But, I looks like /dev/inpput is a class.
> > > > > While IIO devices are a bus_type devices.
> > > > > Should we start implementing an IIO class? or?
> > > > What I should have highlighted [before] with this, is that there is no
> > > > devnode()
> > > > callback for the bus_type [type].
> > > But there is one in device_type :)
> > Many thanks :)
> > That worked nicely.
> >
> > I now have:
> >
> > root@analog:~# ls /dev/iio/*
> > /dev/iio/iio:device0 /dev/iio/iio:device1
> >
> > /dev/iio/device3:
> > buffer0 buffer1 buffer2 buffer3
> >
> > /dev/iio/device4:
> > buffer0
> >
> >
> > It looks like I can shift these around as needed.
> > This is just an experiment.
> > I managed to move the iio devices under /dev/iio, though probably the IIO
> > devices will still be around as /dev/iio:deviceX for legacy reasons.
> >
> > Two things remain unresolved.
> > 1. The name of the IIO buffer device.
> >
> > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > buffer in_voltage0_test_mode name
> > events in_voltage1_test_mode of_node
> > iio:buffer:3:0 in_voltage_sampling_frequency power
> > iio:buffer:3:1 in_voltage_scale scan_elements
> > iio:buffer:3:2 in_voltage_scale_available subsystem
> > iio:buffer:3:3 in_voltage_test_mode_available uevent
> >
> >
> > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > One suggesttion was 'iio:deviceX:bufferY'
> > I'm suspecting the latter is preferred as when you sort the folders, buffers
> > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> >
> > I don't feel it matters much the device name of the IIO buffer if we symlink
> > it
> > to a shorter form.
> >
> > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in
> > each
> > 'iio:deviceX'?
>
> I think that would be a bit excessive. Only for the legacy buffer we
> need to have a symlink.
>
> > [...]
> > 2. I know this is [still] stupid now; but any suggestions one how to symlink
> > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> >
> Does not seem to be possible. Userspace will have to take care of it.
> This means we need to keep legacy devices in /dev/ and only new buffers
> in /dev/iio/.
One thought about this, was that we keep the chardev for the IIO device for
this.
i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
This means that for a device with 4 buffers, you get 5 chardevs.
This also seems a bit much/excessive. Maybe also in terms of source-code.
It would at least mean not moving the event-only chardev to 'industrialio-
event.c', OR move it, and have the same chardev in 3 places ['industrialio-
event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
Maybe this sort-of makes sense to have for a few years/kernel-revisions until
things clean-up.
I guess at this point, the maintainer should have the final say about this.
>
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-12 6:26 ` Ardelean, Alexandru
@ 2020-05-16 13:08 ` Ardelean, Alexandru
2020-05-16 16:24 ` Jonathan Cameron
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-16 13:08 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-kernel, linux-iio
On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:
> [External]
>
> On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:
> > [External]
> >
> > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> > > > [External]
> > > >
> > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > > > > [External]
> > > > > >
> > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > > > > [External]
> > > > > > >
> > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > > > > [External]
> > > > > > > > >
> > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > >
> > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > > > > [...]
> > > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0
> > > > > > > > > > > (to
> > > > > > > > > > > 3).
> > > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev-
> > > > > > > > > > > >dev'.
> > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > symlink to
> > > > > > > > > > > shorter
> > > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > is
> > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > weird
> > > > > > > > > > > to
> > > > > > > > > > > do
> > > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices
> > > > > > > > > > > 'bufferX'.
> > > > > > > > > > >
> > > > > > > > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > What means forward?
> > > > > > > > > > > Where did I leave my beer?
> > > > > > > > > > Looking at how the /dev/ devices are named I think we can
> > > > > > > > > > provide
> > > > > > > > > > a
> > > > > > > > > > name
> > > > > > > > > > that is different from the dev_name() of the device. Have a
> > > > > > > > > > look
> > > > > > > > > > at
> > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be
> > > > > > > > > > able
> > > > > > > > > > to
> > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > callback.
> > > > > > > > > >
> > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > into
> > > > > > > > > > an
> > > > > > > > > > iio
> > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > > > > Possibly on the folder. I can't for the life of me remember
> > > > > > > > > why
> > > > > > > > > I
> > > > > > > > > decided
> > > > > > > > > not to do that the first time around - I'll leave it at the
> > > > > > > > > mysterious "it may turn out to be harder than you'd think..."
> > > > > > > > > Hopefully not ;)
> > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > this.
> > > > > > > > I can copy that from /dev/input
> > > > > > > > They seem to do it already.
> > > > > > > > I don't know how difficult it would be. But it looks like a good
> > > > > > > > precedent.
> > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > callback.
> > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > mentioning
> > > > > > this
> > > > > > as as good precedent.
> > > > > >
> > > > > > But, I looks like /dev/inpput is a class.
> > > > > > While IIO devices are a bus_type devices.
> > > > > > Should we start implementing an IIO class? or?
> > > > > What I should have highlighted [before] with this, is that there is no
> > > > > devnode()
> > > > > callback for the bus_type [type].
> > > > But there is one in device_type :)
> > > Many thanks :)
> > > That worked nicely.
> > >
> > > I now have:
> > >
> > > root@analog:~# ls /dev/iio/*
> > > /dev/iio/iio:device0 /dev/iio/iio:device1
> > >
> > > /dev/iio/device3:
> > > buffer0 buffer1 buffer2 buffer3
> > >
> > > /dev/iio/device4:
> > > buffer0
> > >
> > >
> > > It looks like I can shift these around as needed.
> > > This is just an experiment.
> > > I managed to move the iio devices under /dev/iio, though probably the IIO
> > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > >
> > > Two things remain unresolved.
> > > 1. The name of the IIO buffer device.
> > >
> > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > buffer in_voltage0_test_mode name
> > > events in_voltage1_test_mode of_node
> > > iio:buffer:3:0 in_voltage_sampling_frequency power
> > > iio:buffer:3:1 in_voltage_scale scan_elements
> > > iio:buffer:3:2 in_voltage_scale_available subsystem
> > > iio:buffer:3:3 in_voltage_test_mode_available uevent
> > >
> > >
> > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > One suggesttion was 'iio:deviceX:bufferY'
> > > I'm suspecting the latter is preferred as when you sort the folders,
> > > buffers
> > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > >
> > > I don't feel it matters much the device name of the IIO buffer if we
> > > symlink
> > > it
> > > to a shorter form.
> > >
> > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in
> > > each
> > > 'iio:deviceX'?
> >
> > I think that would be a bit excessive. Only for the legacy buffer we
> > need to have a symlink.
> >
> > > [...]
> > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > symlink
> > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > >
> > Does not seem to be possible. Userspace will have to take care of it.
> > This means we need to keep legacy devices in /dev/ and only new buffers
> > in /dev/iio/.
>
> One thought about this, was that we keep the chardev for the IIO device for
> this.
> i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> This means that for a device with 4 buffers, you get 5 chardevs.
> This also seems a bit much/excessive. Maybe also in terms of source-code.
> It would at least mean not moving the event-only chardev to 'industrialio-
> event.c', OR move it, and have the same chardev in 3 places ['industrialio-
> event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
>
> Maybe this sort-of makes sense to have for a few years/kernel-revisions until
> things clean-up.
>
> I guess at this point, the maintainer should have the final say about this.
Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' thing
a feature for new devices, and leave '/dev/iio:deviceX' devices [for buffers] a
thing for current devices.
It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name [for
this yet].
Over time, people can convert existing drivers to the new IIO-buffer format, if
they want to. That also gives them a bit better control over symlinking
'/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse if
they want to].
That may create confusion I guess during a transition period.
And it would [ideally] have a mechanism [preferably at build/compile time] to
notify users to use the new IIO buffer mechanism [vs the old one] when adding
new drivers.
Otherwise, there's the risk of people copying the old IIO buffer mechanism.
This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying.
>
> >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-16 13:08 ` Ardelean, Alexandru
@ 2020-05-16 16:24 ` Jonathan Cameron
2020-05-17 6:26 ` Ardelean, Alexandru
0 siblings, 1 reply; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-16 16:24 UTC (permalink / raw)
To: Ardelean, Alexandru; +Cc: lars, linux-kernel, linux-iio
On Sat, 16 May 2020 13:08:46 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:
> > [External]
> >
> > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:
> > > [External]
> > >
> > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> > > > > [External]
> > > > >
> > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > > > > > [External]
> > > > > > >
> > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > > > > > [External]
> > > > > > > >
> > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > > > > > [External]
> > > > > > > > > >
> > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > > >
> > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > > > > > [...]
> > > > > > > > > > > > What I don't like, is that iio:device3 has iio:buffer3:0
> > > > > > > > > > > > (to
> > > > > > > > > > > > 3).
> > > > > > > > > > > > This is because the 'buffer->dev.parent = &indio_dev-
> > > > > > > > > > > > >dev'.
> > > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > > symlink to
> > > > > > > > > > > > shorter
> > > > > > > > > > > > versions like 'iio:buffer3:Y' -> 'iio:device3/bufferY'.
> > > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > > is
> > > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > > weird
> > > > > > > > > > > > to
> > > > > > > > > > > > do
> > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer devices
> > > > > > > > > > > > 'bufferX'.
> > > > > > > > > > > >
> > > > > > > > > > > > So, now I'm thinking of whether all this is acceptable.
> > > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > > What means forward?
> > > > > > > > > > > > Where did I leave my beer?
> > > > > > > > > > > Looking at how the /dev/ devices are named I think we can
> > > > > > > > > > > provide
> > > > > > > > > > > a
> > > > > > > > > > > name
> > > > > > > > > > > that is different from the dev_name() of the device. Have a
> > > > > > > > > > > look
> > > > > > > > > > > at
> > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should be
> > > > > > > > > > > able
> > > > > > > > > > > to
> > > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > > callback.
> > > > > > > > > > >
> > > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > > into
> > > > > > > > > > > an
> > > > > > > > > > > iio
> > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > > > > > Possibly on the folder. I can't for the life of me remember
> > > > > > > > > > why
> > > > > > > > > > I
> > > > > > > > > > decided
> > > > > > > > > > not to do that the first time around - I'll leave it at the
> > > > > > > > > > mysterious "it may turn out to be harder than you'd think..."
> > > > > > > > > > Hopefully not ;)
> > > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > > this.
> > > > > > > > > I can copy that from /dev/input
> > > > > > > > > They seem to do it already.
> > > > > > > > > I don't know how difficult it would be. But it looks like a good
> > > > > > > > > precedent.
> > > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > > callback.
> > > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > > mentioning
> > > > > > > this
> > > > > > > as as good precedent.
> > > > > > >
> > > > > > > But, I looks like /dev/inpput is a class.
> > > > > > > While IIO devices are a bus_type devices.
> > > > > > > Should we start implementing an IIO class? or?
> > > > > > What I should have highlighted [before] with this, is that there is no
> > > > > > devnode()
> > > > > > callback for the bus_type [type].
> > > > > But there is one in device_type :)
> > > > Many thanks :)
> > > > That worked nicely.
> > > >
> > > > I now have:
> > > >
> > > > root@analog:~# ls /dev/iio/*
> > > > /dev/iio/iio:device0 /dev/iio/iio:device1
> > > >
> > > > /dev/iio/device3:
> > > > buffer0 buffer1 buffer2 buffer3
> > > >
> > > > /dev/iio/device4:
> > > > buffer0
> > > >
> > > >
> > > > It looks like I can shift these around as needed.
> > > > This is just an experiment.
> > > > I managed to move the iio devices under /dev/iio, though probably the IIO
> > > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > > >
> > > > Two things remain unresolved.
> > > > 1. The name of the IIO buffer device.
> > > >
> > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > > buffer in_voltage0_test_mode name
> > > > events in_voltage1_test_mode of_node
> > > > iio:buffer:3:0 in_voltage_sampling_frequency power
> > > > iio:buffer:3:1 in_voltage_scale scan_elements
> > > > iio:buffer:3:2 in_voltage_scale_available subsystem
> > > > iio:buffer:3:3 in_voltage_test_mode_available uevent
> > > >
> > > >
> > > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > > One suggesttion was 'iio:deviceX:bufferY'
> > > > I'm suspecting the latter is preferred as when you sort the folders,
> > > > buffers
> > > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > > >
> > > > I don't feel it matters much the device name of the IIO buffer if we
> > > > symlink
> > > > it
> > > > to a shorter form.
> > > >
> > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders in
> > > > each
> > > > 'iio:deviceX'?
> > >
> > > I think that would be a bit excessive. Only for the legacy buffer we
> > > need to have a symlink.
> > >
> > > > [...]
> > > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > > symlink
> > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > > >
> > > Does not seem to be possible. Userspace will have to take care of it.
> > > This means we need to keep legacy devices in /dev/ and only new buffers
> > > in /dev/iio/.
> >
> > One thought about this, was that we keep the chardev for the IIO device for
> > this.
> > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> > This means that for a device with 4 buffers, you get 5 chardevs.
> > This also seems a bit much/excessive. Maybe also in terms of source-code.
> > It would at least mean not moving the event-only chardev to 'industrialio-
> > event.c', OR move it, and have the same chardev in 3 places ['industrialio-
> > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
> >
> > Maybe this sort-of makes sense to have for a few years/kernel-revisions until
> > things clean-up.
> >
> > I guess at this point, the maintainer should have the final say about this.
>
> Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY' thing
> a feature for new devices, and leave '/dev/iio:deviceX' devices [for buffers] a
> thing for current devices.
> It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name [for
> this yet].
Definitely a no to that. If we make this transition it needs to be
automatic and subsystem wide. At some point we could have a kconfig option
to disable the legacy interface subsystem wise as a precursor to eventually
dropping it.
>
> Over time, people can convert existing drivers to the new IIO-buffer format, if
> they want to. That also gives them a bit better control over symlinking
> '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse if
> they want to].
>
> That may create confusion I guess during a transition period.
> And it would [ideally] have a mechanism [preferably at build/compile time] to
> notify users to use the new IIO buffer mechanism [vs the old one] when adding
> new drivers.
> Otherwise, there's the risk of people copying the old IIO buffer mechanism.
> This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying.
If we can't do this in a transparent fashion we need to rethink.
The existing interface 'has' to remain and do something sensible. Realistically
we need to keep it in place for 3-5 years at least.
I'm not yet convinced the complexity is worthwhile. We 'could' fallback to
the same trick used for events and use an ioctl to access all buffers
other than the first one... Then we retain one chardev per iio device
and still get the flexibility we need to have multiple buffers.
In some ways it is tidier, even if a bit less intuitive...
If we can't build the symlinks we were all kind of assuming we could
we may need to rethink the overall path.
Anyhow, you are doing great work exploring the options!
Thanks,
Jonathan
>
>
> >
> > >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-16 16:24 ` Jonathan Cameron
@ 2020-05-17 6:26 ` Ardelean, Alexandru
2020-05-17 13:40 ` Jonathan Cameron
0 siblings, 1 reply; 39+ messages in thread
From: Ardelean, Alexandru @ 2020-05-17 6:26 UTC (permalink / raw)
To: jic23; +Cc: lars, linux-kernel, linux-iio
On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote:
> [External]
>
> On Sat, 16 May 2020 13:08:46 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
>
> > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:
> > > [External]
> > >
> > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:
> > > > [External]
> > > >
> > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> > > > > > [External]
> > > > > >
> > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > > > > > > [External]
> > > > > > > >
> > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > > > > > > [External]
> > > > > > > > >
> > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > > > > > > [External]
> > > > > > > > > > >
> > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > > > >
> > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > > > > > > [...]
> > > > > > > > > > > > > What I don't like, is that iio:device3 has
> > > > > > > > > > > > > iio:buffer3:0
> > > > > > > > > > > > > (to
> > > > > > > > > > > > > 3).
> > > > > > > > > > > > > This is because the 'buffer->dev.parent =
> > > > > > > > > > > > > &indio_dev-
> > > > > > > > > > > > > > dev'.
> > > > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > > > symlink to
> > > > > > > > > > > > > shorter
> > > > > > > > > > > > > versions like 'iio:buffer3:Y' ->
> > > > > > > > > > > > > 'iio:device3/bufferY'.
> > > > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > > > is
> > > > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > > > weird
> > > > > > > > > > > > > to
> > > > > > > > > > > > > do
> > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer
> > > > > > > > > > > > > devices
> > > > > > > > > > > > > 'bufferX'.
> > > > > > > > > > > > >
> > > > > > > > > > > > > So, now I'm thinking of whether all this is
> > > > > > > > > > > > > acceptable.
> > > > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > > > What means forward?
> > > > > > > > > > > > > Where did I leave my beer?
> > > > > > > > > > > > Looking at how the /dev/ devices are named I think we
> > > > > > > > > > > > can
> > > > > > > > > > > > provide
> > > > > > > > > > > > a
> > > > > > > > > > > > name
> > > > > > > > > > > > that is different from the dev_name() of the device.
> > > > > > > > > > > > Have a
> > > > > > > > > > > > look
> > > > > > > > > > > > at
> > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should
> > > > > > > > > > > > be
> > > > > > > > > > > > able
> > > > > > > > > > > > to
> > > > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > > > callback.
> > > > > > > > > > > >
> > > > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > > > into
> > > > > > > > > > > > an
> > > > > > > > > > > > iio
> > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > > > > > > Possibly on the folder. I can't for the life of me
> > > > > > > > > > > remember
> > > > > > > > > > > why
> > > > > > > > > > > I
> > > > > > > > > > > decided
> > > > > > > > > > > not to do that the first time around - I'll leave it at
> > > > > > > > > > > the
> > > > > > > > > > > mysterious "it may turn out to be harder than you'd
> > > > > > > > > > > think..."
> > > > > > > > > > > Hopefully not ;)
> > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > > > this.
> > > > > > > > > > I can copy that from /dev/input
> > > > > > > > > > They seem to do it already.
> > > > > > > > > > I don't know how difficult it would be. But it looks like a
> > > > > > > > > > good
> > > > > > > > > > precedent.
> > > > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > > > callback.
> > > > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > > > mentioning
> > > > > > > > this
> > > > > > > > as as good precedent.
> > > > > > > >
> > > > > > > > But, I looks like /dev/inpput is a class.
> > > > > > > > While IIO devices are a bus_type devices.
> > > > > > > > Should we start implementing an IIO class? or?
> > > > > > > What I should have highlighted [before] with this, is that there
> > > > > > > is no
> > > > > > > devnode()
> > > > > > > callback for the bus_type [type].
> > > > > > But there is one in device_type :)
> > > > > Many thanks :)
> > > > > That worked nicely.
> > > > >
> > > > > I now have:
> > > > >
> > > > > root@analog:~# ls /dev/iio/*
> > > > > /dev/iio/iio:device0 /dev/iio/iio:device1
> > > > >
> > > > > /dev/iio/device3:
> > > > > buffer0 buffer1 buffer2 buffer3
> > > > >
> > > > > /dev/iio/device4:
> > > > > buffer0
> > > > >
> > > > >
> > > > > It looks like I can shift these around as needed.
> > > > > This is just an experiment.
> > > > > I managed to move the iio devices under /dev/iio, though probably the
> > > > > IIO
> > > > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > > > >
> > > > > Two things remain unresolved.
> > > > > 1. The name of the IIO buffer device.
> > > > >
> > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > > > buffer in_voltage0_test_mode name
> > > > > events in_voltage1_test_mode of_node
> > > > > iio:buffer:3:0 in_voltage_sampling_frequency power
> > > > > iio:buffer:3:1 in_voltage_scale scan_elements
> > > > > iio:buffer:3:2 in_voltage_scale_available subsystem
> > > > > iio:buffer:3:3 in_voltage_test_mode_available uevent
> > > > >
> > > > >
> > > > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > > > One suggesttion was 'iio:deviceX:bufferY'
> > > > > I'm suspecting the latter is preferred as when you sort the folders,
> > > > > buffers
> > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > > > >
> > > > > I don't feel it matters much the device name of the IIO buffer if we
> > > > > symlink
> > > > > it
> > > > > to a shorter form.
> > > > >
> > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders
> > > > > in
> > > > > each
> > > > > 'iio:deviceX'?
> > > >
> > > > I think that would be a bit excessive. Only for the legacy buffer we
> > > > need to have a symlink.
> > > >
> > > > > [...]
> > > > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > > > symlink
> > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > > > >
> > > > Does not seem to be possible. Userspace will have to take care of it.
> > > > This means we need to keep legacy devices in /dev/ and only new buffers
> > > > in /dev/iio/.
> > >
> > > One thought about this, was that we keep the chardev for the IIO device
> > > for
> > > this.
> > > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> > > This means that for a device with 4 buffers, you get 5 chardevs.
> > > This also seems a bit much/excessive. Maybe also in terms of source-code.
> > > It would at least mean not moving the event-only chardev to 'industrialio-
> > > event.c', OR move it, and have the same chardev in 3 places
> > > ['industrialio-
> > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
> > >
> > > Maybe this sort-of makes sense to have for a few years/kernel-revisions
> > > until
> > > things clean-up.
> > >
> > > I guess at this point, the maintainer should have the final say about
> > > this.
> >
> > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY'
> > thing
> > a feature for new devices, and leave '/dev/iio:deviceX' devices [for
> > buffers] a
> > thing for current devices.
> > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name
> > [for
> > this yet].
>
> Definitely a no to that. If we make this transition it needs to be
> automatic and subsystem wide. At some point we could have a kconfig option
> to disable the legacy interface subsystem wise as a precursor to eventually
> dropping it.
>
> > Over time, people can convert existing drivers to the new IIO-buffer format,
> > if
> > they want to. That also gives them a bit better control over symlinking
> > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse
> > if
> > they want to].
> >
> > That may create confusion I guess during a transition period.
> > And it would [ideally] have a mechanism [preferably at build/compile time]
> > to
> > notify users to use the new IIO buffer mechanism [vs the old one] when
> > adding
> > new drivers.
> > Otherwise, there's the risk of people copying the old IIO buffer mechanism.
> > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying.
>
> If we can't do this in a transparent fashion we need to rethink.
> The existing interface 'has' to remain and do something sensible.
> Realistically
> we need to keep it in place for 3-5 years at least.
>
> I'm not yet convinced the complexity is worthwhile. We 'could' fallback to
> the same trick used for events and use an ioctl to access all buffers
> other than the first one... Then we retain one chardev per iio device
> and still get the flexibility we need to have multiple buffers.
> In some ways it is tidier, even if a bit less intuitive...
> If we can't build the symlinks we were all kind of assuming we could
> we may need to rethink the overall path.
>
> Anyhow, you are doing great work exploring the options!
I wonder if you got to read the idea about adding more chardevs.
The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer
object. I'm not sure about any race issues here.
The bad-part [I feel] is that we get more duplication on chardev file_operations
(open, release, ioctl, etc).
We need to re-wrap code-paths so that they open the same buffer.
And the number of chardevs per IIO device increases by 1 (for a device with 4
buffers == 4 chardevs + 1 legacy)
I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated
towards it either.
This also allows some /dev/iio/deviceX/eventY chardevs.
And some other types of chardevs /dev/iio/deviceX/somethingNewY
But, if I think about it, the only benefit of this [over anon inodes for
chardevs] is that it allows us to do direct access via cat/echo to the actual
chardev of the buffer.
Then, there's also the fact that adding more chardevs increases complexity to
userspace, so it won't matter much. People would probably prefer some userspace
IIO library to do the data read/write.
I'm getting the feeling now that the final debathe is 'anon inodes or not'
Thoughts here?
>
> Thanks,
>
> Jonathan
>
>
> >
> > >
> > > >
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [RFC PATCH 00/14] iio: buffer: add support for multiple buffers
2020-05-17 6:26 ` Ardelean, Alexandru
@ 2020-05-17 13:40 ` Jonathan Cameron
0 siblings, 0 replies; 39+ messages in thread
From: Jonathan Cameron @ 2020-05-17 13:40 UTC (permalink / raw)
To: Ardelean, Alexandru; +Cc: lars, linux-kernel, linux-iio
On Sun, 17 May 2020 06:26:17 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> On Sat, 2020-05-16 at 17:24 +0100, Jonathan Cameron wrote:
> > [External]
> >
> > On Sat, 16 May 2020 13:08:46 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >
> > > On Tue, 2020-05-12 at 06:26 +0000, Ardelean, Alexandru wrote:
> > > > [External]
> > > >
> > > > On Mon, 2020-05-11 at 21:56 +0200, Lars-Peter Clausen wrote:
> > > > > [External]
> > > > >
> > > > > On 5/11/20 4:56 PM, Ardelean, Alexandru wrote:
> > > > > > On Mon, 2020-05-11 at 15:58 +0200, Lars-Peter Clausen wrote:
> > > > > > > [External]
> > > > > > >
> > > > > > > On 5/11/20 3:24 PM, Ardelean, Alexandru wrote:
> > > > > > > > On Mon, 2020-05-11 at 13:03 +0000, Ardelean, Alexandru wrote:
> > > > > > > > > [External]
> > > > > > > > >
> > > > > > > > > On Mon, 2020-05-11 at 12:37 +0200, Lars-Peter Clausen wrote:
> > > > > > > > > > [External]
> > > > > > > > > >
> > > > > > > > > > On 5/11/20 12:33 PM, Ardelean, Alexandru wrote:
> > > > > > > > > > > On Sun, 2020-05-10 at 11:09 +0100, Jonathan Cameron wrote:
> > > > > > > > > > > > [External]
> > > > > > > > > > > >
> > > > > > > > > > > > On Sat, 9 May 2020 10:52:14 +0200
> > > > > > > > > > > > Lars-Peter Clausen <lars@metafoo.de> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > On 5/8/20 3:53 PM, Alexandru Ardelean wrote:
> > > > > > > > > > > > > > [...]
> > > > > > > > > > > > > > What I don't like, is that iio:device3 has
> > > > > > > > > > > > > > iio:buffer3:0
> > > > > > > > > > > > > > (to
> > > > > > > > > > > > > > 3).
> > > > > > > > > > > > > > This is because the 'buffer->dev.parent =
> > > > > > > > > > > > > > &indio_dev-
> > > > > > > > > > > > > > > dev'.
> > > > > > > > > > > > > > But I do feel this is correct.
> > > > > > > > > > > > > > So, now I don't know whether to leave it like that or
> > > > > > > > > > > > > > symlink to
> > > > > > > > > > > > > > shorter
> > > > > > > > > > > > > > versions like 'iio:buffer3:Y' ->
> > > > > > > > > > > > > > 'iio:device3/bufferY'.
> > > > > > > > > > > > > > The reason for naming the IIO buffer devices to
> > > > > > > > > > > > > > 'iio:bufferX:Y'
> > > > > > > > > > > > > > is
> > > > > > > > > > > > > > mostly to make the names unique. It would have looked
> > > > > > > > > > > > > > weird
> > > > > > > > > > > > > > to
> > > > > > > > > > > > > > do
> > > > > > > > > > > > > > '/dev/buffer1' if I would have named the buffer
> > > > > > > > > > > > > > devices
> > > > > > > > > > > > > > 'bufferX'.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So, now I'm thinking of whether all this is
> > > > > > > > > > > > > > acceptable.
> > > > > > > > > > > > > > Or what is acceptable?
> > > > > > > > > > > > > > Should I symlink 'iio:device3/iio:buffer3:0' ->
> > > > > > > > > > > > > > 'iio:device3/buffer0'?
> > > > > > > > > > > > > > What else should I consider moving forward?
> > > > > > > > > > > > > > What means forward?
> > > > > > > > > > > > > > Where did I leave my beer?
> > > > > > > > > > > > > Looking at how the /dev/ devices are named I think we
> > > > > > > > > > > > > can
> > > > > > > > > > > > > provide
> > > > > > > > > > > > > a
> > > > > > > > > > > > > name
> > > > > > > > > > > > > that is different from the dev_name() of the device.
> > > > > > > > > > > > > Have a
> > > > > > > > > > > > > look
> > > > > > > > > > > > > at
> > > > > > > > > > > > > device_get_devnode() in drivers/base/core.c. We should
> > > > > > > > > > > > > be
> > > > > > > > > > > > > able
> > > > > > > > > > > > > to
> > > > > > > > > > > > > provide the name for the chardev through the devnode()
> > > > > > > > > > > > > callback.
> > > > > > > > > > > > >
> > > > > > > > > > > > > While we are at this, do we want to move the new devices
> > > > > > > > > > > > > into
> > > > > > > > > > > > > an
> > > > > > > > > > > > > iio
> > > > > > > > > > > > > subfolder? So iio/buffer0:0 instead of iio:buffer0:0?
> > > > > > > > > > > > Possibly on the folder. I can't for the life of me
> > > > > > > > > > > > remember
> > > > > > > > > > > > why
> > > > > > > > > > > > I
> > > > > > > > > > > > decided
> > > > > > > > > > > > not to do that the first time around - I'll leave it at
> > > > > > > > > > > > the
> > > > > > > > > > > > mysterious "it may turn out to be harder than you'd
> > > > > > > > > > > > think..."
> > > > > > > > > > > > Hopefully not ;)
> > > > > > > > > > > I was also thinking about the /dev/iio subfolder while doing
> > > > > > > > > > > this.
> > > > > > > > > > > I can copy that from /dev/input
> > > > > > > > > > > They seem to do it already.
> > > > > > > > > > > I don't know how difficult it would be. But it looks like a
> > > > > > > > > > > good
> > > > > > > > > > > precedent.
> > > > > > > > > > All you have to do is return "iio/..." from the devnode()
> > > > > > > > > > callback.
> > > > > > > > > I admit I did not look closely into drivers/input/input.c before
> > > > > > > > > mentioning
> > > > > > > > > this
> > > > > > > > > as as good precedent.
> > > > > > > > >
> > > > > > > > > But, I looks like /dev/inpput is a class.
> > > > > > > > > While IIO devices are a bus_type devices.
> > > > > > > > > Should we start implementing an IIO class? or?
> > > > > > > > What I should have highlighted [before] with this, is that there
> > > > > > > > is no
> > > > > > > > devnode()
> > > > > > > > callback for the bus_type [type].
> > > > > > > But there is one in device_type :)
> > > > > > Many thanks :)
> > > > > > That worked nicely.
> > > > > >
> > > > > > I now have:
> > > > > >
> > > > > > root@analog:~# ls /dev/iio/*
> > > > > > /dev/iio/iio:device0 /dev/iio/iio:device1
> > > > > >
> > > > > > /dev/iio/device3:
> > > > > > buffer0 buffer1 buffer2 buffer3
> > > > > >
> > > > > > /dev/iio/device4:
> > > > > > buffer0
> > > > > >
> > > > > >
> > > > > > It looks like I can shift these around as needed.
> > > > > > This is just an experiment.
> > > > > > I managed to move the iio devices under /dev/iio, though probably the
> > > > > > IIO
> > > > > > devices will still be around as /dev/iio:deviceX for legacy reasons.
> > > > > >
> > > > > > Two things remain unresolved.
> > > > > > 1. The name of the IIO buffer device.
> > > > > >
> > > > > > root@analog:/sys/bus/iio/devices# ls iio\:device3/
> > > > > > buffer in_voltage0_test_mode name
> > > > > > events in_voltage1_test_mode of_node
> > > > > > iio:buffer:3:0 in_voltage_sampling_frequency power
> > > > > > iio:buffer:3:1 in_voltage_scale scan_elements
> > > > > > iio:buffer:3:2 in_voltage_scale_available subsystem
> > > > > > iio:buffer:3:3 in_voltage_test_mode_available uevent
> > > > > >
> > > > > >
> > > > > > Right now, each buffer device is named 'iio:buffer:X:Y'.
> > > > > > One suggesttion was 'iio:deviceX:bufferY'
> > > > > > I'm suspecting the latter is preferred as when you sort the folders,
> > > > > > buffers
> > > > > > come right after the iio:deviceX folders in /sys/bus/iio/devices.
> > > > > >
> > > > > > I don't feel it matters much the device name of the IIO buffer if we
> > > > > > symlink
> > > > > > it
> > > > > > to a shorter form.
> > > > > >
> > > > > > I'm guessing, we symlink these devices to short-hand 'bufferY' folders
> > > > > > in
> > > > > > each
> > > > > > 'iio:deviceX'?
> > > > >
> > > > > I think that would be a bit excessive. Only for the legacy buffer we
> > > > > need to have a symlink.
> > > > >
> > > > > > [...]
> > > > > > 2. I know this is [still] stupid now; but any suggestions one how to
> > > > > > symlink
> > > > > > /dev/iio:device3 -> /dev/iio/device3/buffer0 ?
> > > > > >
> > > > > Does not seem to be possible. Userspace will have to take care of it.
> > > > > This means we need to keep legacy devices in /dev/ and only new buffers
> > > > > in /dev/iio/.
> > > >
> > > > One thought about this, was that we keep the chardev for the IIO device
> > > > for
> > > > this.
> > > > i.e. /dev/iio:deviceX and /dev/iio/deviceX/buffer0 open the same buffer.
> > > > This means that for a device with 4 buffers, you get 5 chardevs.
> > > > This also seems a bit much/excessive. Maybe also in terms of source-code.
> > > > It would at least mean not moving the event-only chardev to 'industrialio-
> > > > event.c', OR move it, and have the same chardev in 3 places
> > > > ['industrialio-
> > > > event.c', 'industrialio-buffer.c' & 'industrialio-buffer.c'
> > > >
> > > > Maybe this sort-of makes sense to have for a few years/kernel-revisions
> > > > until
> > > > things clean-up.
> > > >
> > > > I guess at this point, the maintainer should have the final say about
> > > > this.
> > >
> > > Another 'compromise' idea, is that we make this '/dev/iio/deviceX/bufferY'
> > > thing
> > > a feature for new devices, and leave '/dev/iio:deviceX' devices [for
> > > buffers] a
> > > thing for current devices.
> > > It would mean adding a 'new' iio_device_attach_buffer(); no idea on a name
> > > [for
> > > this yet].
> >
> > Definitely a no to that. If we make this transition it needs to be
> > automatic and subsystem wide. At some point we could have a kconfig option
> > to disable the legacy interface subsystem wise as a precursor to eventually
> > dropping it.
> >
> > > Over time, people can convert existing drivers to the new IIO-buffer format,
> > > if
> > > they want to. That also gives them a bit better control over symlinking
> > > '/dev/iio:deviceX' -> '/dev/iio/deviceX/bufferY' [or symlinking in reverse
> > > if
> > > they want to].
> > >
> > > That may create confusion I guess during a transition period.
> > > And it would [ideally] have a mechanism [preferably at build/compile time]
> > > to
> > > notify users to use the new IIO buffer mechanism [vs the old one] when
> > > adding
> > > new drivers.
> > > Otherwise, there's the risk of people copying the old IIO buffer mechanism.
> > > This can be brought-up at review, but ¯\_(ツ)_/¯ ; it can be annoying.
> >
> > If we can't do this in a transparent fashion we need to rethink.
> > The existing interface 'has' to remain and do something sensible.
> > Realistically
> > we need to keep it in place for 3-5 years at least.
> >
> > I'm not yet convinced the complexity is worthwhile. We 'could' fallback to
> > the same trick used for events and use an ioctl to access all buffers
> > other than the first one... Then we retain one chardev per iio device
> > and still get the flexibility we need to have multiple buffers.
> > In some ways it is tidier, even if a bit less intuitive...
> > If we can't build the symlinks we were all kind of assuming we could
> > we may need to rethink the overall path.
> >
> > Anyhow, you are doing great work exploring the options!
>
> I wonder if you got to read the idea about adding more chardevs.
>
> The one where /dev/iio:deviceX & /dev/iio/deviceX/buffer0 open the same buffer
> object. I'm not sure about any race issues here.
> The bad-part [I feel] is that we get more duplication on chardev file_operations
> (open, release, ioctl, etc).
> We need to re-wrap code-paths so that they open the same buffer.
> And the number of chardevs per IIO device increases by 1 (for a device with 4
> buffers == 4 chardevs + 1 legacy)
I read that one and it strikes me as a hack. Two interfaces to the same
thing is always a bad idea if we can possibly avoid it. I was happy enough
with a link if that was doable because then they are the same device with
same dev-node etc. Though thinking more on that it will be hard to get
all the relevant links in place.
>
> I kinda like the idea of the /dev/iio/ folder. But I'm not strongly opinionated
> towards it either.
> This also allows some /dev/iio/deviceX/eventY chardevs.
> And some other types of chardevs /dev/iio/deviceX/somethingNewY
That expansion of chrdevs is kind of what worries me. We end up with
more and more of them.
>
> But, if I think about it, the only benefit of this [over anon inodes for
> chardevs] is that it allows us to do direct access via cat/echo to the actual
> chardev of the buffer.
> Then, there's also the fact that adding more chardevs increases complexity to
> userspace, so it won't matter much. People would probably prefer some userspace
> IIO library to do the data read/write.
>
> I'm getting the feeling now that the final debathe is 'anon inodes or not'
Yes, I think that is the big question. Without your work to explore
the options we wouldn't really know what the other options were.
I'm far from sure what the 'right' answer is to this.
So there is another corner case we should think about. What do we
do about the devices that current expose multiple buffers by having multiple
IIO devices? Usually that occurs for devices that can do weird interleaving
in hardware fifos - hence can produce some channels at N times the frequency of
others - or where we have one shared stream with tagged data.
I suspect it's going to be really hard to map those across to the new interface
with any sort of backwards compatibility. They expose complete iio devices
with all the infrastructure that entails. Maybe we just leave them alone?
J
>
> Thoughts here?
>
>
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> > >
> > > >
> > > > >
^ permalink raw reply [flat|nested] 39+ messages in thread