* [PATCH v5 01/17] docs: ioctl-number.rst: reserve IIO subsystem ioctl() space
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 02/17] iio: core: register chardev only if needed Alexandru Ardelean
` (15 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
Currently, only the 'i' 0x90 ioctl() actually exists and is defined in
'include/uapi/linux/iio/events.h'.
It's the IIO_GET_EVENT_FD_IOCTL, which is used to retrieve and FD for
reading events from an IIO device.
We will want to add more ioct() numbers, so with this change the 'i'
0x90-0x9F space is reserved for IIO ioctl() calls.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
Documentation/userspace-api/ioctl/ioctl-number.rst | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index a4c75a28c839..179783aac8dd 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -245,6 +245,7 @@ Code Seq# Include File Comments
'i' 00-3F linux/i2o-dev.h conflict!
'i' 0B-1F linux/ipmi.h conflict!
'i' 80-8F linux/i8k.h
+'i' 90-9F `linux/iio/*.h` IIO
'j' 00-3F linux/joystick.h
'k' 00-0F linux/spi/spidev.h conflict!
'k' 00-05 video/kyro.h conflict!
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 02/17] iio: core: register chardev only if needed
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 01/17] docs: ioctl-number.rst: reserve IIO subsystem ioctl() space Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 03/17] iio: core-trigger: make iio_device_register_trigger_consumer() an int return Alexandru Ardelean
` (14 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
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.
In order to not create a chardev, we mostly just need to not initialize the
indio_dev->dev.devt field. If that is un-initialized, cdev_device_add()
behaves like device_add().
This change has a small chance of breaking some userspace ABI, because it
removes un-needed chardevs. While these chardevs (that are being removed)
have always been unusable, it is likely that some scripts may check their
existence (for whatever logic).
And we also hope that before opening these chardevs, userspace would have
already checked for some pre-conditions to make sure that opening these
chardevs makes sense.
For the most part, there is also the hope that it would be easier to change
userspace code than revert this. But in the case that reverting this is
required, it should be easy enough to do it.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-core.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 7db761afa578..0a6fd299a978 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1761,6 +1761,15 @@ static const struct file_operations iio_buffer_fileops = {
.release = iio_chrdev_release,
};
+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,
+};
+
static int iio_check_unique_scan_index(struct iio_dev *indio_dev)
{
int i, j;
@@ -1788,6 +1797,7 @@ static const struct iio_buffer_setup_ops noop_ring_setup_ops;
int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
int ret;
if (!indio_dev->info)
@@ -1805,9 +1815,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);
@@ -1836,9 +1843,15 @@ 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 (iio_dev_opaque->event_interface)
+ cdev_init(&indio_dev->chrdev, &iio_event_fileops);
- indio_dev->chrdev.owner = this_mod;
+ if (indio_dev->buffer || iio_dev_opaque->event_interface) {
+ indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
+ indio_dev->chrdev.owner = this_mod;
+ }
ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
if (ret < 0)
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 03/17] iio: core-trigger: make iio_device_register_trigger_consumer() an int return
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 01/17] docs: ioctl-number.rst: reserve IIO subsystem ioctl() space Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 02/17] iio: core: register chardev only if needed Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 04/17] iio: core: rework iio device group creation Alexandru Ardelean
` (13 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
Oddly enough the noop function is an int-return. This one seems to be void.
This change converts it to int, because we want to change how groups are
registered. With that change this function could error out with -ENOMEM.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core_trigger.h | 4 +++-
drivers/iio/industrialio-trigger.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/iio_core_trigger.h b/drivers/iio/iio_core_trigger.h
index 374816bc3e73..e1a56824e07f 100644
--- a/drivers/iio/iio_core_trigger.h
+++ b/drivers/iio/iio_core_trigger.h
@@ -9,8 +9,10 @@
/**
* iio_device_register_trigger_consumer() - set up an iio_dev to use triggers
* @indio_dev: iio_dev associated with the device that will consume the trigger
+ *
+ * Return 0 if successful, negative otherwise
**/
-void iio_device_register_trigger_consumer(struct iio_dev *indio_dev);
+int iio_device_register_trigger_consumer(struct iio_dev *indio_dev);
/**
* iio_device_unregister_trigger_consumer() - reverse the registration process
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ea3c9859b258..438d5012e8b8 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -692,10 +692,12 @@ int iio_trigger_validate_own_device(struct iio_trigger *trig,
}
EXPORT_SYMBOL(iio_trigger_validate_own_device);
-void iio_device_register_trigger_consumer(struct iio_dev *indio_dev)
+int iio_device_register_trigger_consumer(struct iio_dev *indio_dev)
{
indio_dev->groups[indio_dev->groupcounter++] =
&iio_trigger_consumer_attr_group;
+
+ return 0;
}
void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 04/17] iio: core: rework iio device group creation
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (2 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 03/17] iio: core-trigger: make iio_device_register_trigger_consumer() an int return Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 05/17] iio: buffer: group attr count and attr alloc Alexandru Ardelean
` (12 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
Up until now, the device groups that an IIO device had were limited to 6.
Two of these groups would account for buffer attributes (the buffer/ and
scan_elements/ directories).
Since we want to add multiple buffers per IIO device, this number may not
be enough, when adding a second buffer. So, this change reallocates the
groups array whenever an IIO device group is added, via a
iio_device_register_sysfs_group() helper.
This also means that the groups array should be assigned to
'indio_dev.dev.groups' really late, right before {cdev_}device_add() is
called to do the entire setup.
And we also must take care to free this array when the sysfs resources are
being cleaned up.
With this change we can also move the 'groups' & 'groupcounter' fields to
the iio_dev_opaque object. Up until now, this didn't make a whole lot of
sense (especially since we weren't sure how multibuffer support would look
like in the end).
But doing it now kills one birds with one stone.
An alternative, would be to add a configurable Kconfig symbol
CONFIG_IIO_MAX_BUFFERS_PER_DEVICE (or something like that) and compute a
static maximum of the groups we can support per IIO device. But that would
probably annoy a few people since that would make the system less
configurable.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 3 +++
drivers/iio/industrialio-buffer.c | 12 ++++++++++--
drivers/iio/industrialio-core.c | 30 +++++++++++++++++++++++++++---
drivers/iio/industrialio-event.c | 5 ++++-
drivers/iio/industrialio-trigger.c | 6 ++----
include/linux/iio/iio-opaque.h | 4 ++++
include/linux/iio/iio.h | 5 -----
7 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fced02cadcc3..7d5b179c1fe7 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -46,6 +46,9 @@ int __iio_add_chan_devattr(const char *postfix,
struct list_head *attr_list);
void iio_free_chan_devattr_list(struct list_head *attr_list);
+int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
+ const struct attribute_group *group);
+
ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals);
/* Event interface flags */
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 2f7426a2f47c..cc846988fdb9 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1287,7 +1287,9 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
buffer->buffer_group.name = "buffer";
buffer->buffer_group.attrs = attr;
- indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
+ ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
+ if (ret)
+ goto error_free_buffer_attrs;
attrcount = 0;
INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
@@ -1330,14 +1332,20 @@ 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;
+
+ ret = iio_device_register_sysfs_group(indio_dev, &buffer->scan_el_group);
+ if (ret)
+ goto error_free_scan_el_attrs;
return 0;
+error_free_scan_el_attrs:
+ kfree(buffer->scan_el_group.attrs);
error_free_scan_mask:
bitmap_free(buffer->scan_mask);
error_cleanup_dynamic:
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+error_free_buffer_attrs:
kfree(buffer->buffer_group.attrs);
return ret;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 0a6fd299a978..2db460ac30b2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1452,6 +1452,25 @@ static ssize_t iio_store_timestamp_clock(struct device *dev,
return len;
}
+int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
+ const struct attribute_group *group)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ const struct attribute_group **new, **old = iio_dev_opaque->groups;
+ unsigned int cnt = iio_dev_opaque->groupcounter;
+
+ new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ new[iio_dev_opaque->groupcounter++] = group;
+ new[iio_dev_opaque->groupcounter] = NULL;
+
+ iio_dev_opaque->groups = new;
+
+ return 0;
+}
+
static DEVICE_ATTR(current_timestamp_clock, S_IRUGO | S_IWUSR,
iio_show_timestamp_clock, iio_store_timestamp_clock);
@@ -1525,8 +1544,10 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
if (clk)
iio_dev_opaque->chan_attr_group.attrs[attrn++] = clk;
- indio_dev->groups[indio_dev->groupcounter++] =
- &iio_dev_opaque->chan_attr_group;
+ ret = iio_device_register_sysfs_group(indio_dev,
+ &iio_dev_opaque->chan_attr_group);
+ if (ret)
+ goto error_clear_attrs;
return 0;
@@ -1543,6 +1564,7 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
kfree(iio_dev_opaque->chan_attr_group.attrs);
iio_dev_opaque->chan_attr_group.attrs = NULL;
+ kfree(iio_dev_opaque->groups);
}
static void iio_dev_release(struct device *device)
@@ -1592,7 +1614,6 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
dev->dev.parent = parent;
- dev->dev.groups = dev->groups;
dev->dev.type = &iio_device_type;
dev->dev.bus = &iio_bus_type;
device_initialize(&dev->dev);
@@ -1853,6 +1874,9 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
indio_dev->chrdev.owner = this_mod;
}
+ /* assign device groups now; they should be all registered now */
+ indio_dev->dev.groups = iio_dev_opaque->groups;
+
ret = cdev_device_add(&indio_dev->chrdev, &indio_dev->dev);
if (ret < 0)
goto error_unreg_eventset;
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 7e532117ac55..ea8947cc21e4 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -544,7 +544,10 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
/* Add all elements from the list. */
list_for_each_entry(p, &ev_int->dev_attr_list, l)
ev_int->group.attrs[attrn++] = &p->dev_attr.attr;
- indio_dev->groups[indio_dev->groupcounter++] = &ev_int->group;
+
+ ret = iio_device_register_sysfs_group(indio_dev, &ev_int->group);
+ if (ret)
+ goto error_free_setup_event_lines;
ev_int->ioctl_handler.ioctl = iio_event_ioctl;
iio_device_ioctl_handler_register(&iio_dev_opaque->indio_dev,
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 438d5012e8b8..a035d5c2a445 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -694,10 +694,8 @@ EXPORT_SYMBOL(iio_trigger_validate_own_device);
int iio_device_register_trigger_consumer(struct iio_dev *indio_dev)
{
- indio_dev->groups[indio_dev->groupcounter++] =
- &iio_trigger_consumer_attr_group;
-
- return 0;
+ return iio_device_register_sysfs_group(indio_dev,
+ &iio_trigger_consumer_attr_group);
}
void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 07c5a8e52ca8..8ba13a5c7af6 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -12,6 +12,8 @@
* attributes
* @chan_attr_group: group for all attrs in base directory
* @ioctl_handlers: ioctl handlers registered with the core handler
+ * @groups: attribute groups
+ * @groupcounter: index of next attribute group
* @debugfs_dentry: device specific debugfs dentry
* @cached_reg_addr: cached register address for debugfs reads
* @read_buf: read buffer to be used for the initial reg read
@@ -24,6 +26,8 @@ struct iio_dev_opaque {
struct list_head channel_attr_list;
struct attribute_group chan_attr_group;
struct list_head ioctl_handlers;
+ const struct attribute_group **groups;
+ int groupcounter;
#if defined(CONFIG_DEBUG_FS)
struct dentry *debugfs_dentry;
unsigned cached_reg_addr;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index e4a9822e6495..f8585d01fc76 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -518,8 +518,6 @@ struct iio_buffer_setup_ops {
* @setup_ops: [DRIVER] callbacks to call before and after buffer
* enable/disable
* @chrdev: [INTERN] associated character device
- * @groups: [INTERN] attribute groups
- * @groupcounter: [INTERN] index of next attribute group
* @flags: [INTERN] file ops related flags including busy flag.
* @priv: [DRIVER] reference to driver's private information
* **MUST** be accessed **ONLY** via iio_priv() helper
@@ -556,9 +554,6 @@ struct iio_dev {
struct mutex info_exist_lock;
const struct iio_buffer_setup_ops *setup_ops;
struct cdev chrdev;
-#define IIO_MAX_GROUPS 6
- const struct attribute_group *groups[IIO_MAX_GROUPS + 1];
- int groupcounter;
unsigned long flags;
void *priv;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 05/17] iio: buffer: group attr count and attr alloc
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (3 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 04/17] iio: core: rework iio device group creation Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-14 12:57 ` Jonathan Cameron
2021-02-11 12:24 ` [PATCH v5 06/17] iio: core: merge buffer/ & scan_elements/ attributes Alexandru Ardelean
` (11 subsequent siblings)
16 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
If we want to merge the attributes of the buffer/ and scan_elements/
directories, we'll need to count all attributes first, then (depending on
the attribute group) either allocate 2 attribute groups, or a single one.
Historically an IIO buffer was described by 2 subdirectories under
/sys/bus/iio/iio:devicesX (i.e. buffer/ and scan_elements/); these subdirs
were actually 2 separate attribute groups on the iio_buffer object.
Moving forward, if we want to allow more than one buffer per IIO device,
keeping 2 subdirectories for each IIO buffer is a bit cumbersome
(especially for userpace ABI). So, we will merge the attributes of these 2
subdirs under a /sys/bus/iio/iio:devicesX/bufferY subdirectory. To do this,
we need to count all attributes first, and then distribute them based on
which buffer this is. For the first buffer, we'll need to also allocate the
legacy 2 attribute groups (for buffer/ and scan_elements/), and also a
/sys/bus/iio/iio:devicesX/buffer0 attribute group.
For buffer1 and above, just a single attribute group will be allocated (the
merged one).
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 71 ++++++++++++++++---------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index cc846988fdb9..23f22be62cc7 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1257,41 +1257,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
{
struct iio_dev_attr *p;
struct attribute **attr;
- int ret, i, attrn, attrcount;
+ int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
const struct iio_chan_spec *channels;
- attrcount = 0;
+ buffer_attrcount = 0;
if (buffer->attrs) {
- while (buffer->attrs[attrcount] != NULL)
- attrcount++;
+ while (buffer->attrs[buffer_attrcount] != NULL)
+ buffer_attrcount++;
}
- attr = kcalloc(attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
- sizeof(struct attribute *), GFP_KERNEL);
- if (!attr)
- return -ENOMEM;
-
- memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
- if (!buffer->access->set_length)
- attr[0] = &dev_attr_length_ro.attr;
-
- if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
- attr[2] = &dev_attr_watermark_ro.attr;
-
- if (buffer->attrs)
- memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
- sizeof(struct attribute *) * attrcount);
-
- attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
-
- buffer->buffer_group.name = "buffer";
- buffer->buffer_group.attrs = attr;
-
- ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
- if (ret)
- goto error_free_buffer_attrs;
-
- attrcount = 0;
+ scan_el_attrcount = 0;
INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
channels = indio_dev->channels;
if (channels) {
@@ -1304,7 +1279,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
&channels[i]);
if (ret < 0)
goto error_cleanup_dynamic;
- attrcount += ret;
+ scan_el_attrcount += ret;
if (channels[i].type == IIO_TIMESTAMP)
indio_dev->scan_index_timestamp =
channels[i].scan_index;
@@ -1319,9 +1294,37 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
}
}
+ attr = kcalloc(buffer_attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
+ sizeof(struct attribute *), GFP_KERNEL);
+ if (!attr) {
+ ret = -ENOMEM;
+ goto error_free_scan_mask;
+ }
+
+ memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
+ if (!buffer->access->set_length)
+ attr[0] = &dev_attr_length_ro.attr;
+
+ if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
+ attr[2] = &dev_attr_watermark_ro.attr;
+
+ if (buffer->attrs)
+ memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
+ sizeof(struct attribute *) * buffer_attrcount);
+
+ buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
+ attr[buffer_attrcount] = NULL;
+
+ buffer->buffer_group.name = "buffer";
+ buffer->buffer_group.attrs = attr;
+
+ ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
+ if (ret)
+ goto error_free_buffer_attrs;
+
buffer->scan_el_group.name = iio_scan_elements_group_name;
- buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
+ buffer->scan_el_group.attrs = kcalloc(scan_el_attrcount + 1,
sizeof(buffer->scan_el_group.attrs[0]),
GFP_KERNEL);
if (buffer->scan_el_group.attrs == NULL) {
@@ -1341,12 +1344,12 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
error_free_scan_el_attrs:
kfree(buffer->scan_el_group.attrs);
+error_free_buffer_attrs:
+ kfree(buffer->buffer_group.attrs);
error_free_scan_mask:
bitmap_free(buffer->scan_mask);
error_cleanup_dynamic:
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-error_free_buffer_attrs:
- kfree(buffer->buffer_group.attrs);
return ret;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 05/17] iio: buffer: group attr count and attr alloc
2021-02-11 12:24 ` [PATCH v5 05/17] iio: buffer: group attr count and attr alloc Alexandru Ardelean
@ 2021-02-14 12:57 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2021-02-14 12:57 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, rafael, gregkh
On Thu, 11 Feb 2021 14:24:40 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> If we want to merge the attributes of the buffer/ and scan_elements/
> directories, we'll need to count all attributes first, then (depending on
> the attribute group) either allocate 2 attribute groups, or a single one.
> Historically an IIO buffer was described by 2 subdirectories under
> /sys/bus/iio/iio:devicesX (i.e. buffer/ and scan_elements/); these subdirs
> were actually 2 separate attribute groups on the iio_buffer object.
>
> Moving forward, if we want to allow more than one buffer per IIO device,
> keeping 2 subdirectories for each IIO buffer is a bit cumbersome
> (especially for userpace ABI). So, we will merge the attributes of these 2
> subdirs under a /sys/bus/iio/iio:devicesX/bufferY subdirectory. To do this,
> we need to count all attributes first, and then distribute them based on
> which buffer this is. For the first buffer, we'll need to also allocate the
> legacy 2 attribute groups (for buffer/ and scan_elements/), and also a
> /sys/bus/iio/iio:devicesX/buffer0 attribute group.
>
> For buffer1 and above, just a single attribute group will be allocated (the
> merged one).
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
One totally trivial thing noticed in here. If we happen to respin
please tidy it up, if I take these as is I 'might' tweak it, or I might
not bother depending on what mood I'm in later today.
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 71 ++++++++++++++++---------------
> 1 file changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index cc846988fdb9..23f22be62cc7 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1257,41 +1257,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> {
> struct iio_dev_attr *p;
> struct attribute **attr;
> - int ret, i, attrn, attrcount;
> + int ret, i, attrn, scan_el_attrcount, buffer_attrcount;
> const struct iio_chan_spec *channels;
>
> - attrcount = 0;
> + buffer_attrcount = 0;
> if (buffer->attrs) {
> - while (buffer->attrs[attrcount] != NULL)
> - attrcount++;
> + while (buffer->attrs[buffer_attrcount] != NULL)
> + buffer_attrcount++;
> }
>
> - attr = kcalloc(attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
> - sizeof(struct attribute *), GFP_KERNEL);
> - if (!attr)
> - return -ENOMEM;
> -
> - memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
> - if (!buffer->access->set_length)
> - attr[0] = &dev_attr_length_ro.attr;
> -
> - if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
> - attr[2] = &dev_attr_watermark_ro.attr;
> -
> - if (buffer->attrs)
> - memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> - sizeof(struct attribute *) * attrcount);
> -
> - attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> -
> - buffer->buffer_group.name = "buffer";
> - buffer->buffer_group.attrs = attr;
> -
> - ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> - if (ret)
> - goto error_free_buffer_attrs;
> -
> - attrcount = 0;
> + scan_el_attrcount = 0;
> INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> channels = indio_dev->channels;
> if (channels) {
> @@ -1304,7 +1279,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> &channels[i]);
> if (ret < 0)
> goto error_cleanup_dynamic;
> - attrcount += ret;
> + scan_el_attrcount += ret;
> if (channels[i].type == IIO_TIMESTAMP)
> indio_dev->scan_index_timestamp =
> channels[i].scan_index;
> @@ -1319,9 +1294,37 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> }
> }
>
> + attr = kcalloc(buffer_attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
> + sizeof(struct attribute *), GFP_KERNEL);
nitpick, I'd have made that sizeof(*attr)
> + if (!attr) {
> + ret = -ENOMEM;
> + goto error_free_scan_mask;
> + }
> +
> + memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
> + if (!buffer->access->set_length)
> + attr[0] = &dev_attr_length_ro.attr;
> +
> + if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
> + attr[2] = &dev_attr_watermark_ro.attr;
> +
> + if (buffer->attrs)
> + memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> + sizeof(struct attribute *) * buffer_attrcount);
> +
> + buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
> + attr[buffer_attrcount] = NULL;
> +
> + buffer->buffer_group.name = "buffer";
> + buffer->buffer_group.attrs = attr;
> +
> + ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> + if (ret)
> + goto error_free_buffer_attrs;
> +
> buffer->scan_el_group.name = iio_scan_elements_group_name;
>
> - buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
> + buffer->scan_el_group.attrs = kcalloc(scan_el_attrcount + 1,
> sizeof(buffer->scan_el_group.attrs[0]),
> GFP_KERNEL);
> if (buffer->scan_el_group.attrs == NULL) {
> @@ -1341,12 +1344,12 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>
> error_free_scan_el_attrs:
> kfree(buffer->scan_el_group.attrs);
> +error_free_buffer_attrs:
> + kfree(buffer->buffer_group.attrs);
> error_free_scan_mask:
> bitmap_free(buffer->scan_mask);
> error_cleanup_dynamic:
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> -error_free_buffer_attrs:
> - kfree(buffer->buffer_group.attrs);
>
> return ret;
> }
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 06/17] iio: core: merge buffer/ & scan_elements/ attributes
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (4 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 05/17] iio: buffer: group attr count and attr alloc Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-14 13:02 ` Jonathan Cameron
2021-02-11 12:24 ` [PATCH v5 07/17] iio: add reference to iio buffer on iio_dev_attr Alexandru Ardelean
` (10 subsequent siblings)
16 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
With this change, we create a new directory for the IIO device called
buffer0, under which both the old buffer/ and scan_elements/ are stored.
This is done to simplify the addition of multiple IIO buffers per IIO
device. Otherwise we would need to add a bufferX/ and scan_elementsX/
directory for each IIO buffer.
With the current way of storing attribute groups, we can't have directories
stored under each other (i.e. scan_elements/ under buffer/), so the best
approach moving forward is to merge their attributes.
The old/legacy buffer/ & scan_elements/ groups are not stored on the opaque
IIO device object. This way the IIO buffer can have just a single
attribute_group object, saving a bit of memory when adding multiple IIO
buffers.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 115 +++++++++++++++++++++++-------
include/linux/iio/buffer_impl.h | 9 +--
include/linux/iio/iio-opaque.h | 4 ++
3 files changed, 95 insertions(+), 33 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 23f22be62cc7..fcbbffb904bf 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1175,8 +1175,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)
@@ -1252,8 +1250,68 @@ static struct attribute *iio_buffer_attrs[] = {
&dev_attr_data_available.attr,
};
+static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev,
+ struct attribute **buffer_attrs,
+ int buffer_attrcount,
+ int scan_el_attrcount)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct attribute_group *group;
+ int ret;
+
+ group = &iio_dev_opaque->legacy_buffer_group;
+
+ group->attrs = kcalloc(buffer_attrcount + 1,
+ sizeof(struct attribute *), GFP_KERNEL);
+ if (!group->attrs)
+ return -ENOMEM;
+
+ memcpy(group->attrs, buffer_attrs,
+ buffer_attrcount * sizeof(struct attribute *));
+ group->name = "buffer";
+
+ ret = iio_device_register_sysfs_group(indio_dev, group);
+ if (ret)
+ goto error_free_buffer_attrs;
+
+ group = &iio_dev_opaque->legacy_scan_el_group;
+
+ group->attrs = kcalloc(scan_el_attrcount + 1,
+ sizeof(struct attribute *), GFP_KERNEL);
+ if (!group->attrs) {
+ ret = -ENOMEM;
+ goto error_free_buffer_attrs;
+ }
+
+ memcpy(group->attrs, &buffer_attrs[buffer_attrcount],
+ scan_el_attrcount * sizeof(struct attribute *));
+ group->name = "scan_elements";
+
+ ret = iio_device_register_sysfs_group(indio_dev, group);
+ if (ret)
+ goto error_free_scan_el_attrs;
+
+ return 0;
+
+error_free_buffer_attrs:
+ kfree(iio_dev_opaque->legacy_buffer_group.attrs);
+error_free_scan_el_attrs:
+ kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
+
+ return ret;
+}
+
+static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+ kfree(iio_dev_opaque->legacy_buffer_group.attrs);
+ kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
+}
+
static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
- struct iio_dev *indio_dev)
+ struct iio_dev *indio_dev,
+ int index)
{
struct iio_dev_attr *p;
struct attribute **attr;
@@ -1294,8 +1352,8 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
}
}
- attr = kcalloc(buffer_attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
- sizeof(struct attribute *), GFP_KERNEL);
+ attrn = buffer_attrcount + scan_el_attrcount + ARRAY_SIZE(iio_buffer_attrs);
+ attr = kcalloc(attrn + 1, sizeof(struct attribute *), GFP_KERNEL);
if (!attr) {
ret = -ENOMEM;
goto error_free_scan_mask;
@@ -1313,37 +1371,38 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
sizeof(struct attribute *) * buffer_attrcount);
buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
- attr[buffer_attrcount] = NULL;
-
- buffer->buffer_group.name = "buffer";
- buffer->buffer_group.attrs = attr;
- ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
- if (ret)
- goto error_free_buffer_attrs;
+ attrn = buffer_attrcount;
- buffer->scan_el_group.name = iio_scan_elements_group_name;
+ list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
+ attr[attrn++] = &p->dev_attr.attr;
- buffer->scan_el_group.attrs = kcalloc(scan_el_attrcount + 1,
- sizeof(buffer->scan_el_group.attrs[0]),
- GFP_KERNEL);
- if (buffer->scan_el_group.attrs == NULL) {
+ buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index);
+ if (!buffer->buffer_group.name) {
ret = -ENOMEM;
- goto error_free_scan_mask;
+ goto error_free_buffer_attrs;
}
- 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->buffer_group.attrs = attr;
- ret = iio_device_register_sysfs_group(indio_dev, &buffer->scan_el_group);
+ ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
if (ret)
- goto error_free_scan_el_attrs;
+ goto error_free_buffer_attr_group_name;
+
+ /* we only need to link the legacy buffer groups for the first buffer */
+ if (index > 0)
+ return 0;
+
+ ret = iio_buffer_register_legacy_sysfs_groups(indio_dev, attr,
+ buffer_attrcount,
+ scan_el_attrcount);
+ if (ret)
+ goto error_free_buffer_attr_group_name;
return 0;
-error_free_scan_el_attrs:
- kfree(buffer->scan_el_group.attrs);
+error_free_buffer_attr_group_name:
+ kfree(buffer->buffer_group.name);
error_free_buffer_attrs:
kfree(buffer->buffer_group.attrs);
error_free_scan_mask:
@@ -1372,14 +1431,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
if (!buffer)
return 0;
- return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
+ return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
}
static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
{
bitmap_free(buffer->scan_mask);
+ kfree(buffer->buffer_group.name);
kfree(buffer->buffer_group.attrs);
- kfree(buffer->scan_el_group.attrs);
iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
}
@@ -1390,6 +1449,8 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
if (!buffer)
return;
+ iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
+
__iio_buffer_free_sysfs_and_mask(buffer);
}
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index a63dc07b7350..3e555e58475b 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -100,14 +100,11 @@ struct iio_buffer {
/* @scan_el_dev_attr_list: List of scan element related attributes. */
struct list_head scan_el_dev_attr_list;
- /* @buffer_group: Attributes of the buffer group. */
- struct attribute_group buffer_group;
-
/*
- * @scan_el_group: Attribute group for those attributes not
- * created from the iio_chan_info array.
+ * @buffer_group: Attributes of the new buffer group.
+ * Includes scan elements attributes.
*/
- struct attribute_group scan_el_group;
+ struct attribute_group buffer_group;
/* @attrs: Standard attributes of the buffer. */
const struct attribute **attrs;
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 8ba13a5c7af6..3e4c3cd248fd 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -14,6 +14,8 @@
* @ioctl_handlers: ioctl handlers registered with the core handler
* @groups: attribute groups
* @groupcounter: index of next attribute group
+ * @legacy_scan_el_group: attribute group for legacy scan elements attribute group
+ * @legacy_buffer_el_group: attribute group for legacy buffer attributes group
* @debugfs_dentry: device specific debugfs dentry
* @cached_reg_addr: cached register address for debugfs reads
* @read_buf: read buffer to be used for the initial reg read
@@ -28,6 +30,8 @@ struct iio_dev_opaque {
struct list_head ioctl_handlers;
const struct attribute_group **groups;
int groupcounter;
+ struct attribute_group legacy_scan_el_group;
+ struct attribute_group legacy_buffer_group;
#if defined(CONFIG_DEBUG_FS)
struct dentry *debugfs_dentry;
unsigned cached_reg_addr;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 06/17] iio: core: merge buffer/ & scan_elements/ attributes
2021-02-11 12:24 ` [PATCH v5 06/17] iio: core: merge buffer/ & scan_elements/ attributes Alexandru Ardelean
@ 2021-02-14 13:02 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2021-02-14 13:02 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, rafael, gregkh
On Thu, 11 Feb 2021 14:24:41 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> With this change, we create a new directory for the IIO device called
> buffer0, under which both the old buffer/ and scan_elements/ are stored.
>
> This is done to simplify the addition of multiple IIO buffers per IIO
> device. Otherwise we would need to add a bufferX/ and scan_elementsX/
> directory for each IIO buffer.
> With the current way of storing attribute groups, we can't have directories
> stored under each other (i.e. scan_elements/ under buffer/), so the best
> approach moving forward is to merge their attributes.
>
> The old/legacy buffer/ & scan_elements/ groups are not stored on the opaque
> IIO device object. This way the IIO buffer can have just a single
> attribute_group object, saving a bit of memory when adding multiple IIO
> buffers.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Two trivial things inline.
Jonathan
> ---
> drivers/iio/industrialio-buffer.c | 115 +++++++++++++++++++++++-------
> include/linux/iio/buffer_impl.h | 9 +--
> include/linux/iio/iio-opaque.h | 4 ++
> 3 files changed, 95 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 23f22be62cc7..fcbbffb904bf 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1175,8 +1175,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)
> @@ -1252,8 +1250,68 @@ static struct attribute *iio_buffer_attrs[] = {
> &dev_attr_data_available.attr,
> };
>
> +static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev,
> + struct attribute **buffer_attrs,
> + int buffer_attrcount,
> + int scan_el_attrcount)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct attribute_group *group;
> + int ret;
> +
> + group = &iio_dev_opaque->legacy_buffer_group;
> +
> + group->attrs = kcalloc(buffer_attrcount + 1,
> + sizeof(struct attribute *), GFP_KERNEL);
> + if (!group->attrs)
> + return -ENOMEM;
> +
> + memcpy(group->attrs, buffer_attrs,
> + buffer_attrcount * sizeof(struct attribute *));
> + group->name = "buffer";
> +
> + ret = iio_device_register_sysfs_group(indio_dev, group);
> + if (ret)
> + goto error_free_buffer_attrs;
> +
> + group = &iio_dev_opaque->legacy_scan_el_group;
> +
> + group->attrs = kcalloc(scan_el_attrcount + 1,
> + sizeof(struct attribute *), GFP_KERNEL);
> + if (!group->attrs) {
> + ret = -ENOMEM;
> + goto error_free_buffer_attrs;
> + }
> +
> + memcpy(group->attrs, &buffer_attrs[buffer_attrcount],
> + scan_el_attrcount * sizeof(struct attribute *));
> + group->name = "scan_elements";
> +
> + ret = iio_device_register_sysfs_group(indio_dev, group);
> + if (ret)
> + goto error_free_scan_el_attrs;
> +
> + return 0;
> +
> +error_free_buffer_attrs:
> + kfree(iio_dev_opaque->legacy_buffer_group.attrs);
> +error_free_scan_el_attrs:
> + kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> +
> + return ret;
> +}
> +
> +static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> + kfree(iio_dev_opaque->legacy_buffer_group.attrs);
> + kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> +}
> +
> static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> - struct iio_dev *indio_dev)
> + struct iio_dev *indio_dev,
> + int index)
> {
> struct iio_dev_attr *p;
> struct attribute **attr;
> @@ -1294,8 +1352,8 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> }
> }
>
> - attr = kcalloc(buffer_attrcount + ARRAY_SIZE(iio_buffer_attrs) + 1,
> - sizeof(struct attribute *), GFP_KERNEL);
> + attrn = buffer_attrcount + scan_el_attrcount + ARRAY_SIZE(iio_buffer_attrs);
> + attr = kcalloc(attrn + 1, sizeof(struct attribute *), GFP_KERNEL);
> if (!attr) {
> ret = -ENOMEM;
> goto error_free_scan_mask;
> @@ -1313,37 +1371,38 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> sizeof(struct attribute *) * buffer_attrcount);
>
> buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
> - attr[buffer_attrcount] = NULL;
> -
> - buffer->buffer_group.name = "buffer";
> - buffer->buffer_group.attrs = attr;
>
> - ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> - if (ret)
> - goto error_free_buffer_attrs;
> + attrn = buffer_attrcount;
>
> - buffer->scan_el_group.name = iio_scan_elements_group_name;
> + list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> + attr[attrn++] = &p->dev_attr.attr;
>
> - buffer->scan_el_group.attrs = kcalloc(scan_el_attrcount + 1,
> - sizeof(buffer->scan_el_group.attrs[0]),
> - GFP_KERNEL);
> - if (buffer->scan_el_group.attrs == NULL) {
> + buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index);
> + if (!buffer->buffer_group.name) {
> ret = -ENOMEM;
> - goto error_free_scan_mask;
> + goto error_free_buffer_attrs;
> }
> - 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->buffer_group.attrs = attr;
>
> - ret = iio_device_register_sysfs_group(indio_dev, &buffer->scan_el_group);
> + ret = iio_device_register_sysfs_group(indio_dev, &buffer->buffer_group);
> if (ret)
> - goto error_free_scan_el_attrs;
> + goto error_free_buffer_attr_group_name;
> +
> + /* we only need to link the legacy buffer groups for the first buffer */
Not linking as such any more so comment could do with an update.
> + if (index > 0)
> + return 0;
> +
> + ret = iio_buffer_register_legacy_sysfs_groups(indio_dev, attr,
> + buffer_attrcount,
> + scan_el_attrcount);
> + if (ret)
> + goto error_free_buffer_attr_group_name;
>
> return 0;
>
> -error_free_scan_el_attrs:
> - kfree(buffer->scan_el_group.attrs);
> +error_free_buffer_attr_group_name:
> + kfree(buffer->buffer_group.name);
> error_free_buffer_attrs:
> kfree(buffer->buffer_group.attrs);
> error_free_scan_mask:
> @@ -1372,14 +1431,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!buffer)
> return 0;
>
> - return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> }
>
> static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> {
> bitmap_free(buffer->scan_mask);
> + kfree(buffer->buffer_group.name);
> kfree(buffer->buffer_group.attrs);
> - kfree(buffer->scan_el_group.attrs);
> iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> }
>
> @@ -1390,6 +1449,8 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!buffer)
> return;
>
> + iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
> +
> __iio_buffer_free_sysfs_and_mask(buffer);
> }
>
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index a63dc07b7350..3e555e58475b 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -100,14 +100,11 @@ struct iio_buffer {
> /* @scan_el_dev_attr_list: List of scan element related attributes. */
> struct list_head scan_el_dev_attr_list;
>
> - /* @buffer_group: Attributes of the buffer group. */
> - struct attribute_group buffer_group;
> -
> /*
> - * @scan_el_group: Attribute group for those attributes not
> - * created from the iio_chan_info array.
> + * @buffer_group: Attributes of the new buffer group.
> + * Includes scan elements attributes.
> */
> - struct attribute_group scan_el_group;
> + struct attribute_group buffer_group;
>
> /* @attrs: Standard attributes of the buffer. */
> const struct attribute **attrs;
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index 8ba13a5c7af6..3e4c3cd248fd 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -14,6 +14,8 @@
> * @ioctl_handlers: ioctl handlers registered with the core handler
> * @groups: attribute groups
> * @groupcounter: index of next attribute group
> + * @legacy_scan_el_group: attribute group for legacy scan elements attribute group
> + * @legacy_buffer_el_group: attribute group for legacy buffer attributes group
Wrong name
> * @debugfs_dentry: device specific debugfs dentry
> * @cached_reg_addr: cached register address for debugfs reads
> * @read_buf: read buffer to be used for the initial reg read
> @@ -28,6 +30,8 @@ struct iio_dev_opaque {
> struct list_head ioctl_handlers;
> const struct attribute_group **groups;
> int groupcounter;
> + struct attribute_group legacy_scan_el_group;
> + struct attribute_group legacy_buffer_group;
> #if defined(CONFIG_DEBUG_FS)
> struct dentry *debugfs_dentry;
> unsigned cached_reg_addr;
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 07/17] iio: add reference to iio buffer on iio_dev_attr
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (5 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 06/17] iio: core: merge buffer/ & scan_elements/ attributes Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 08/17] iio: buffer: wrap all buffer attributes into iio_dev_attr Alexandru Ardelean
` (9 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
This change adds a reference to a 'struct iio_buffer' object on the
iio_dev_attr object. This way, we can use the created iio_dev_attr objects
on per-buffer basis (since they're allocated anyway).
A minor downside of this change is that the number of parameters on
__iio_add_chan_devattr() grows by 1. This looks like it could do with a bit
of a re-think.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 2 ++
drivers/iio/industrialio-buffer.c | 4 ++++
drivers/iio/industrialio-core.c | 6 ++++++
drivers/iio/industrialio-event.c | 1 +
include/linux/iio/sysfs.h | 3 +++
5 files changed, 16 insertions(+)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 7d5b179c1fe7..731f5170d5b9 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -12,6 +12,7 @@
#include <linux/kernel.h>
#include <linux/device.h>
+struct iio_buffer;
struct iio_chan_spec;
struct iio_dev;
@@ -43,6 +44,7 @@ int __iio_add_chan_devattr(const char *postfix,
u64 mask,
enum iio_shared_by shared_by,
struct device *dev,
+ struct iio_buffer *buffer,
struct list_head *attr_list);
void iio_free_chan_devattr_list(struct list_head *attr_list);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index fcbbffb904bf..530b8697f3d8 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -447,6 +447,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
0,
IIO_SEPARATE,
&indio_dev->dev,
+ buffer,
&buffer->scan_el_dev_attr_list);
if (ret)
return ret;
@@ -458,6 +459,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
0,
0,
&indio_dev->dev,
+ buffer,
&buffer->scan_el_dev_attr_list);
if (ret)
return ret;
@@ -470,6 +472,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
chan->scan_index,
0,
&indio_dev->dev,
+ buffer,
&buffer->scan_el_dev_attr_list);
else
ret = __iio_add_chan_devattr("en",
@@ -479,6 +482,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
chan->scan_index,
0,
&indio_dev->dev,
+ buffer,
&buffer->scan_el_dev_attr_list);
if (ret)
return ret;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2db460ac30b2..2e8e656e41dd 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1114,6 +1114,7 @@ int __iio_add_chan_devattr(const char *postfix,
u64 mask,
enum iio_shared_by shared_by,
struct device *dev,
+ struct iio_buffer *buffer,
struct list_head *attr_list)
{
int ret;
@@ -1129,6 +1130,7 @@ int __iio_add_chan_devattr(const char *postfix,
goto error_iio_dev_attr_free;
iio_attr->c = chan;
iio_attr->address = mask;
+ iio_attr->buffer = buffer;
list_for_each_entry(t, attr_list, l)
if (strcmp(t->dev_attr.attr.name,
iio_attr->dev_attr.attr.name) == 0) {
@@ -1165,6 +1167,7 @@ static int iio_device_add_channel_label(struct iio_dev *indio_dev,
0,
IIO_SEPARATE,
&indio_dev->dev,
+ NULL,
&iio_dev_opaque->channel_attr_list);
if (ret < 0)
return ret;
@@ -1190,6 +1193,7 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
i,
shared_by,
&indio_dev->dev,
+ NULL,
&iio_dev_opaque->channel_attr_list);
if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
continue;
@@ -1226,6 +1230,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
i,
shared_by,
&indio_dev->dev,
+ NULL,
&iio_dev_opaque->channel_attr_list);
kfree(avail_postfix);
if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
@@ -1322,6 +1327,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
i,
ext_info->shared,
&indio_dev->dev,
+ NULL,
&iio_dev_opaque->channel_attr_list);
i++;
if (ret == -EBUSY && ext_info->shared)
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index ea8947cc21e4..a30e289fc362 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -385,6 +385,7 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
ret = __iio_add_chan_devattr(postfix, chan, show, store,
(i << 16) | spec_index, shared_by, &indio_dev->dev,
+ NULL,
&iio_dev_opaque->event_interface->dev_attr_list);
kfree(postfix);
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index b532c875bc24..e51fba66de4b 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -9,6 +9,7 @@
#ifndef _INDUSTRIAL_IO_SYSFS_H_
#define _INDUSTRIAL_IO_SYSFS_H_
+struct iio_buffer;
struct iio_chan_spec;
/**
@@ -17,12 +18,14 @@ struct iio_chan_spec;
* @address: associated register address
* @l: list head for maintaining list of dynamically created attrs
* @c: specification for the underlying channel
+ * @buffer: the IIO buffer to which this attribute belongs to (if any)
*/
struct iio_dev_attr {
struct device_attribute dev_attr;
u64 address;
struct list_head l;
struct iio_chan_spec const *c;
+ struct iio_buffer *buffer;
};
#define to_iio_dev_attr(_dev_attr) \
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 08/17] iio: buffer: wrap all buffer attributes into iio_dev_attr
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (6 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 07/17] iio: add reference to iio buffer on iio_dev_attr Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 09/17] iio: buffer: dmaengine: obtain buffer object from attribute Alexandru Ardelean
` (8 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
This change wraps all buffer attributes into iio_dev_attr objects, and
assigns a reference to the IIO buffer they belong to.
With the addition of multiple IIO buffers per one IIO device, we need a way
to know which IIO buffer is being enabled/disabled/controlled.
We know that all buffer attributes are device_attributes. So we can wrap
them with a iio_dev_attr types. In the iio_dev_attr type, we can also hold
a reference to an IIO buffer.
So, we end up being able to allocate wrapped attributes for all buffer
attributes (even the one from other drivers).
The neat part with this mechanism, is that we don't need to add any extra
cleanup, because these attributes are being added to a dynamic list that
will get cleaned up via iio_free_chan_devattr_list().
With this change, the 'buffer->scan_el_dev_attr_list' list is being renamed
to 'buffer->buffer_attr_list', effectively merging (or finalizing the
merge) of the buffer/ & scan_elements/ attributes internally.
Accessing these new buffer attributes can now be done via
'to_iio_dev_attr(attr)->buffer' inside the show/store handlers.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 76 ++++++++++++++++++++-----------
include/linux/iio/buffer_impl.h | 4 +-
2 files changed, 52 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 530b8697f3d8..d79d81485a3f 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -253,8 +253,7 @@ static ssize_t iio_scan_el_show(struct device *dev,
char *buf)
{
int ret;
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
/* Ensure ret is 0 or 1. */
ret = !!test_bit(to_iio_dev_attr(attr)->address,
@@ -367,8 +366,8 @@ static ssize_t iio_scan_el_store(struct device *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);
+ struct iio_buffer *buffer = this_attr->buffer;
ret = strtobool(buf, &state);
if (ret < 0)
@@ -402,8 +401,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 = to_iio_dev_attr(attr)->buffer;
return sprintf(buf, "%d\n", buffer->scan_timestamp);
}
@@ -415,7 +413,7 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
{
int ret;
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
bool state;
ret = strtobool(buf, &state);
@@ -448,7 +446,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
IIO_SEPARATE,
&indio_dev->dev,
buffer,
- &buffer->scan_el_dev_attr_list);
+ &buffer->buffer_attr_list);
if (ret)
return ret;
attrcount++;
@@ -460,7 +458,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
0,
&indio_dev->dev,
buffer,
- &buffer->scan_el_dev_attr_list);
+ &buffer->buffer_attr_list);
if (ret)
return ret;
attrcount++;
@@ -473,7 +471,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
0,
&indio_dev->dev,
buffer,
- &buffer->scan_el_dev_attr_list);
+ &buffer->buffer_attr_list);
else
ret = __iio_add_chan_devattr("en",
chan,
@@ -483,7 +481,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
0,
&indio_dev->dev,
buffer,
- &buffer->scan_el_dev_attr_list);
+ &buffer->buffer_attr_list);
if (ret)
return ret;
attrcount++;
@@ -495,8 +493,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 = to_iio_dev_attr(attr)->buffer;
return sprintf(buf, "%d\n", buffer->length);
}
@@ -506,7 +503,7 @@ static ssize_t iio_buffer_write_length(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 = to_iio_dev_attr(attr)->buffer;
unsigned int val;
int ret;
@@ -538,8 +535,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 = to_iio_dev_attr(attr)->buffer;
return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
}
@@ -1154,7 +1150,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
int ret;
bool requested_state;
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
bool inlist;
ret = strtobool(buf, &requested_state);
@@ -1183,8 +1179,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 = to_iio_dev_attr(attr)->buffer;
return sprintf(buf, "%u\n", buffer->watermark);
}
@@ -1195,7 +1190,7 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
size_t len)
{
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_buffer *buffer = to_iio_dev_attr(attr)->buffer;
unsigned int val;
int ret;
@@ -1228,8 +1223,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 = to_iio_dev_attr(attr)->buffer;
return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
}
@@ -1254,6 +1248,26 @@ static struct attribute *iio_buffer_attrs[] = {
&dev_attr_data_available.attr,
};
+#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+
+static struct attribute *iio_buffer_wrap_attr(struct iio_buffer *buffer,
+ struct attribute *attr)
+{
+ struct device_attribute *dattr = to_dev_attr(attr);
+ struct iio_dev_attr *iio_attr;
+
+ iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
+ if (!iio_attr)
+ return NULL;
+
+ iio_attr->buffer = buffer;
+ memcpy(&iio_attr->dev_attr, dattr, sizeof(iio_attr->dev_attr));
+
+ list_add(&iio_attr->l, &buffer->buffer_attr_list);
+
+ return &iio_attr->dev_attr.attr;
+}
+
static int iio_buffer_register_legacy_sysfs_groups(struct iio_dev *indio_dev,
struct attribute **buffer_attrs,
int buffer_attrcount,
@@ -1329,7 +1343,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
}
scan_el_attrcount = 0;
- INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
+ INIT_LIST_HEAD(&buffer->buffer_attr_list);
channels = indio_dev->channels;
if (channels) {
/* new magic */
@@ -1376,9 +1390,19 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
buffer_attrcount += ARRAY_SIZE(iio_buffer_attrs);
- attrn = buffer_attrcount;
+ for (i = 0; i < buffer_attrcount; i++) {
+ struct attribute *wrapped;
+
+ wrapped = iio_buffer_wrap_attr(buffer, attr[i]);
+ if (!wrapped) {
+ ret = -ENOMEM;
+ goto error_free_scan_mask;
+ }
+ attr[i] = wrapped;
+ }
- list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
+ attrn = 0;
+ list_for_each_entry(p, &buffer->buffer_attr_list, l)
attr[attrn++] = &p->dev_attr.attr;
buffer->buffer_group.name = kasprintf(GFP_KERNEL, "buffer%d", index);
@@ -1412,7 +1436,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
error_free_scan_mask:
bitmap_free(buffer->scan_mask);
error_cleanup_dynamic:
- iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+ iio_free_chan_devattr_list(&buffer->buffer_attr_list);
return ret;
}
@@ -1443,7 +1467,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
bitmap_free(buffer->scan_mask);
kfree(buffer->buffer_group.name);
kfree(buffer->buffer_group.attrs);
- iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+ iio_free_chan_devattr_list(&buffer->buffer_attr_list);
}
void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 3e555e58475b..41044320e581 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -97,8 +97,8 @@ struct iio_buffer {
/* @scan_timestamp: Does the scan mode include a timestamp. */
bool scan_timestamp;
- /* @scan_el_dev_attr_list: List of scan element related attributes. */
- struct list_head scan_el_dev_attr_list;
+ /* @buffer_attr_list: List of buffer attributes. */
+ struct list_head buffer_attr_list;
/*
* @buffer_group: Attributes of the new buffer group.
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 09/17] iio: buffer: dmaengine: obtain buffer object from attribute
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (7 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 08/17] iio: buffer: wrap all buffer attributes into iio_dev_attr Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 10/17] iio: core: wrap iio device & buffer into struct for character devices Alexandru Ardelean
` (7 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
The reference to the IIO buffer object is stored on the attribute object.
So we need to unwind it to obtain it.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/buffer/industrialio-buffer-dmaengine.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b0cb9a35f5cd..8ecdbae83414 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -132,9 +132,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 = to_iio_dev_attr(attr)->buffer;
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);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 10/17] iio: core: wrap iio device & buffer into struct for character devices
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (8 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 09/17] iio: buffer: dmaengine: obtain buffer object from attribute Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 11/17] iio: buffer: move __iio_buffer_free_sysfs_and_mask() before alloc Alexandru Ardelean
` (6 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
In order to keep backwards compatibility with the current chardev
mechanism, and in order to add support for multiple buffers per IIO device,
we need to pass both the IIO device & IIO buffer to the chardev.
This is particularly needed for the iio_buffer_read_outer() function, where
we need to pass another buffer object than 'indio_dev->buffer'.
Since we'll also open some chardevs via anon inodes, we can pass extra
buffers in that function by assigning another object to the
iio_dev_buffer_pair object.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 5 +++++
drivers/iio/industrialio-buffer.c | 10 ++++++----
drivers/iio/industrialio-core.c | 18 ++++++++++++++++--
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 731f5170d5b9..87868fff7d37 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -18,6 +18,11 @@ struct iio_dev;
extern struct device_type iio_device_type;
+struct iio_dev_buffer_pair {
+ struct iio_dev *indio_dev;
+ struct iio_buffer *buffer;
+};
+
#define IIO_IOCTL_UNHANDLED 1
struct iio_ioctl_handler {
struct list_head entry;
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index d79d81485a3f..a6148ed24e41 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -104,8 +104,9 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
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_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+ struct iio_dev *indio_dev = ib->indio_dev;
DEFINE_WAIT_FUNC(wait, woken_wake_function);
size_t datum_size;
size_t to_wait;
@@ -170,8 +171,9 @@ ssize_t iio_buffer_read_outer(struct file *filp, char __user *buf,
__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_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+ struct iio_dev *indio_dev = ib->indio_dev;
if (!indio_dev->info || rb == NULL)
return 0;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2e8e656e41dd..1be94df3e591 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1703,13 +1703,24 @@ 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);
+ struct iio_dev_buffer_pair *ib;
if (test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->flags))
return -EBUSY;
iio_device_get(indio_dev);
- filp->private_data = indio_dev;
+ ib = kmalloc(sizeof(*ib), GFP_KERNEL);
+ if (!ib) {
+ iio_device_put(indio_dev);
+ clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
+ return -ENOMEM;
+ }
+
+ ib->indio_dev = indio_dev;
+ ib->buffer = indio_dev->buffer;
+
+ filp->private_data = ib;
return 0;
}
@@ -1723,10 +1734,12 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
*/
static int iio_chrdev_release(struct inode *inode, struct file *filp)
{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
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);
+ kfree(ib);
return 0;
}
@@ -1746,7 +1759,8 @@ void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h)
static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
- struct iio_dev *indio_dev = filp->private_data;
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ struct iio_dev *indio_dev = ib->indio_dev;
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
struct iio_ioctl_handler *h;
int ret = -ENODEV;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 11/17] iio: buffer: move __iio_buffer_free_sysfs_and_mask() before alloc
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (9 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 10/17] iio: core: wrap iio device & buffer into struct for character devices Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers Alexandru Ardelean
` (5 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
The __iio_buffer_free_sysfs_and_mask() function will be used in
iio_buffer_alloc_sysfs_and_mask() when multiple buffers will be attached to
the IIO device.
This will need to be used to cleanup resources on each buffer, when the
buffers cleanup unwind will occur on the error path.
The move is done in this patch to make the patch that adds multiple buffers
per IIO device a bit cleaner.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-buffer.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a6148ed24e41..209b3a32bdbb 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1443,6 +1443,14 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
return ret;
}
+static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
+{
+ bitmap_free(buffer->scan_mask);
+ kfree(buffer->buffer_group.name);
+ kfree(buffer->buffer_group.attrs);
+ iio_free_chan_devattr_list(&buffer->buffer_attr_list);
+}
+
int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
@@ -1464,14 +1472,6 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
}
-static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
-{
- bitmap_free(buffer->scan_mask);
- kfree(buffer->buffer_group.name);
- kfree(buffer->buffer_group.attrs);
- iio_free_chan_devattr_list(&buffer->buffer_attr_list);
-}
-
void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
{
struct iio_buffer *buffer = indio_dev->buffer;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (10 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 11/17] iio: buffer: move __iio_buffer_free_sysfs_and_mask() before alloc Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-14 13:19 ` Jonathan Cameron
2021-02-11 12:24 ` [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device Alexandru Ardelean
` (4 subsequent siblings)
16 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
With this change, calling iio_device_attach_buffer() will actually attach
more buffers.
Right now this doesn't do any validation of whether a buffer is attached
twice; maybe that can be added later (if needed). Attaching a buffer more
than once should yield noticeably bad results.
The first buffer is the legacy buffer, so a reference is kept to it.
At this point, accessing the data for the extra buffers (that are added
after the first one) isn't possible yet.
The iio_device_attach_buffer() is also changed to return an error code,
which for now is -ENOMEM if the array could not be realloc-ed for more
buffers.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 2 +
drivers/iio/industrialio-buffer.c | 96 +++++++++++++++++++++++++------
drivers/iio/industrialio-core.c | 6 +-
include/linux/iio/buffer.h | 4 +-
include/linux/iio/buffer_impl.h | 3 +
include/linux/iio/iio-opaque.h | 4 ++
6 files changed, 93 insertions(+), 22 deletions(-)
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 87868fff7d37..4690c3240a5d 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -77,6 +77,7 @@ void iio_buffer_free_sysfs_and_mask(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);
+void iio_buffers_put(struct iio_dev *indio_dev);
#else
@@ -92,6 +93,7 @@ static inline void iio_buffer_free_sysfs_and_mask(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) {}
+static inline void iio_buffers_put(struct iio_dev *indio_dev) {}
#endif
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 209b3a32bdbb..1e8e4c2ff00e 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -193,12 +193,14 @@ __poll_t iio_buffer_poll(struct file *filp,
*/
void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
-
- if (!buffer)
- return;
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_buffer *buffer;
+ unsigned int i;
- wake_up(&buffer->pollq);
+ for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
+ buffer = iio_dev_opaque->attached_buffers[i];
+ wake_up(&buffer->pollq);
+ }
}
void iio_buffer_init(struct iio_buffer *buffer)
@@ -212,6 +214,18 @@ void iio_buffer_init(struct iio_buffer *buffer)
}
EXPORT_SYMBOL(iio_buffer_init);
+void iio_buffers_put(struct iio_dev *indio_dev)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_buffer *buffer;
+ unsigned int i;
+
+ for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
+ buffer = iio_dev_opaque->attached_buffers[i];
+ iio_buffer_put(buffer);
+ }
+}
+
static ssize_t iio_show_scan_index(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -1453,9 +1467,11 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
const struct iio_chan_spec *channels;
- int i;
+ struct iio_buffer *buffer;
+ int unwind_idx;
+ int ret, i;
channels = indio_dev->channels;
if (channels) {
@@ -1466,22 +1482,46 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
indio_dev->masklength = ml;
}
- if (!buffer)
+ if (!iio_dev_opaque->attached_buffers_cnt)
return 0;
- return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
+ for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
+ buffer = iio_dev_opaque->attached_buffers[i];
+ ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
+ if (ret) {
+ unwind_idx = i;
+ goto error_unwind_sysfs_and_mask;
+ }
+ }
+
+ return 0;
+
+error_unwind_sysfs_and_mask:
+ for (; unwind_idx >= 0; unwind_idx--) {
+ buffer = iio_dev_opaque->attached_buffers[unwind_idx];
+ __iio_buffer_free_sysfs_and_mask(buffer);
+ }
+ kfree(iio_dev_opaque->attached_buffers);
+ return ret;
}
void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
{
- struct iio_buffer *buffer = indio_dev->buffer;
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_buffer *buffer;
+ int i;
- if (!buffer)
+ if (!iio_dev_opaque->attached_buffers_cnt)
return;
iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
- __iio_buffer_free_sysfs_and_mask(buffer);
+ for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
+ buffer = iio_dev_opaque->attached_buffers[i];
+ __iio_buffer_free_sysfs_and_mask(buffer);
+ }
+
+ kfree(iio_dev_opaque->attached_buffers);
}
/**
@@ -1599,13 +1639,35 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
* @indio_dev: The device the buffer should be attached to
* @buffer: The buffer to attach to the device
*
+ * Return 0 if successful, negative if error.
+ *
* This function attaches a buffer to a IIO device. The buffer stays attached to
- * the device until the device is freed. The function should only be called at
- * most once per device.
+ * the device until the device is freed. For legacy reasons, the first attached
+ * buffer will also be assigned to 'indio_dev->buffer'.
*/
-void iio_device_attach_buffer(struct iio_dev *indio_dev,
- struct iio_buffer *buffer)
+int iio_device_attach_buffer(struct iio_dev *indio_dev,
+ struct iio_buffer *buffer)
{
- indio_dev->buffer = iio_buffer_get(buffer);
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ struct iio_buffer **new, **old = iio_dev_opaque->attached_buffers;
+ unsigned int cnt = iio_dev_opaque->attached_buffers_cnt;
+
+ cnt++;
+
+ new = krealloc(old, sizeof(*new) * cnt, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+ iio_dev_opaque->attached_buffers = new;
+
+ buffer = iio_buffer_get(buffer);
+
+ /* first buffer is legacy; attach it to the IIO device directly */
+ if (!indio_dev->buffer)
+ indio_dev->buffer = buffer;
+
+ iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
+ iio_dev_opaque->attached_buffers_cnt = cnt;
+
+ return 0;
}
EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 1be94df3e591..26b05dddfa71 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1583,7 +1583,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_buffers_put(indio_dev);
ida_simple_remove(&iio_ida, indio_dev->id);
kfree(iio_dev_opaque);
@@ -1884,12 +1884,12 @@ 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)
+ if (iio_dev_opaque->attached_buffers_cnt)
cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
else if (iio_dev_opaque->event_interface)
cdev_init(&indio_dev->chrdev, &iio_event_fileops);
- if (indio_dev->buffer || iio_dev_opaque->event_interface) {
+ if (iio_dev_opaque->attached_buffers_cnt || iio_dev_opaque->event_interface) {
indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
indio_dev->chrdev.owner = this_mod;
}
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 8febc23f5f26..b6928ac5c63d 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -41,7 +41,7 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
const unsigned long *mask);
-void iio_device_attach_buffer(struct iio_dev *indio_dev,
- struct iio_buffer *buffer);
+int iio_device_attach_buffer(struct iio_dev *indio_dev,
+ struct iio_buffer *buffer);
#endif /* _IIO_BUFFER_GENERIC_H_ */
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 41044320e581..768b90c64412 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -112,6 +112,9 @@ struct iio_buffer {
/* @demux_bounce: Buffer for doing gather from incoming scan. */
void *demux_bounce;
+ /* @attached_entry: Entry in the devices list of buffers attached by the driver. */
+ 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-opaque.h b/include/linux/iio/iio-opaque.h
index 3e4c3cd248fd..c909835b6247 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -7,6 +7,8 @@
* struct iio_dev_opaque - industrial I/O device opaque information
* @indio_dev: public industrial I/O device information
* @event_interface: event chrdevs associated with interrupt lines
+ * @attached_buffers: array of buffers statically attached by the driver
+ * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
* @buffer_list: list of all buffers currently attached
* @channel_attr_list: keep track of automatically created channel
* attributes
@@ -24,6 +26,8 @@
struct iio_dev_opaque {
struct iio_dev indio_dev;
struct iio_event_interface *event_interface;
+ struct iio_buffer **attached_buffers;
+ unsigned int attached_buffers_cnt;
struct list_head buffer_list;
struct list_head channel_attr_list;
struct attribute_group chan_attr_group;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers
2021-02-11 12:24 ` [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers Alexandru Ardelean
@ 2021-02-14 13:19 ` Jonathan Cameron
2021-02-14 14:02 ` Alexandru Ardelean
0 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2021-02-14 13:19 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, rafael, gregkh
On Thu, 11 Feb 2021 14:24:47 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> With this change, calling iio_device_attach_buffer() will actually attach
> more buffers.
> Right now this doesn't do any validation of whether a buffer is attached
> twice; maybe that can be added later (if needed). Attaching a buffer more
> than once should yield noticeably bad results.
>
> The first buffer is the legacy buffer, so a reference is kept to it.
>
> At this point, accessing the data for the extra buffers (that are added
> after the first one) isn't possible yet.
>
> The iio_device_attach_buffer() is also changed to return an error code,
> which for now is -ENOMEM if the array could not be realloc-ed for more
> buffers.
This is called from a bunch of places, so we need to add error handling
for those which isn't currently present.
Now arguably it is unlikely to fail, so I'm not that bothered about seeing
them all in place as part of this series, but I would like the handling
to have been posted at least.
Otherwise, a few passing comments in here, but nothing to actually change.
Jonathan
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/iio_core.h | 2 +
> drivers/iio/industrialio-buffer.c | 96 +++++++++++++++++++++++++------
> drivers/iio/industrialio-core.c | 6 +-
> include/linux/iio/buffer.h | 4 +-
> include/linux/iio/buffer_impl.h | 3 +
> include/linux/iio/iio-opaque.h | 4 ++
> 6 files changed, 93 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 87868fff7d37..4690c3240a5d 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -77,6 +77,7 @@ void iio_buffer_free_sysfs_and_mask(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);
> +void iio_buffers_put(struct iio_dev *indio_dev);
>
> #else
>
> @@ -92,6 +93,7 @@ static inline void iio_buffer_free_sysfs_and_mask(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) {}
> +static inline void iio_buffers_put(struct iio_dev *indio_dev) {}
>
> #endif
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 209b3a32bdbb..1e8e4c2ff00e 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -193,12 +193,14 @@ __poll_t iio_buffer_poll(struct file *filp,
> */
> void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> {
> - struct iio_buffer *buffer = indio_dev->buffer;
> -
> - if (!buffer)
> - return;
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_buffer *buffer;
> + unsigned int i;
>
> - wake_up(&buffer->pollq);
> + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> + buffer = iio_dev_opaque->attached_buffers[i];
> + wake_up(&buffer->pollq);
> + }
> }
>
> void iio_buffer_init(struct iio_buffer *buffer)
> @@ -212,6 +214,18 @@ void iio_buffer_init(struct iio_buffer *buffer)
> }
> EXPORT_SYMBOL(iio_buffer_init);
>
> +void iio_buffers_put(struct iio_dev *indio_dev)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_buffer *buffer;
> + unsigned int i;
> +
> + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> + buffer = iio_dev_opaque->attached_buffers[i];
> + iio_buffer_put(buffer);
> + }
> +}
> +
> static ssize_t iio_show_scan_index(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -1453,9 +1467,11 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>
> int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> {
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> const struct iio_chan_spec *channels;
> - int i;
> + struct iio_buffer *buffer;
> + int unwind_idx;
> + int ret, i;
>
> channels = indio_dev->channels;
> if (channels) {
> @@ -1466,22 +1482,46 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> indio_dev->masklength = ml;
> }
>
> - if (!buffer)
> + if (!iio_dev_opaque->attached_buffers_cnt)
> return 0;
>
> - return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> + buffer = iio_dev_opaque->attached_buffers[i];
> + ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
> + if (ret) {
> + unwind_idx = i;
> + goto error_unwind_sysfs_and_mask;
> + }
> + }
> +
> + return 0;
> +
> +error_unwind_sysfs_and_mask:
> + for (; unwind_idx >= 0; unwind_idx--) {
> + buffer = iio_dev_opaque->attached_buffers[unwind_idx];
> + __iio_buffer_free_sysfs_and_mask(buffer);
> + }
> + kfree(iio_dev_opaque->attached_buffers);
> + return ret;
> }
>
> void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> {
Perhaps we should do a follow up adjusting names to slightly better
reflect they are not handling multiple buffers.
> - struct iio_buffer *buffer = indio_dev->buffer;
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_buffer *buffer;
> + int i;
>
> - if (!buffer)
> + if (!iio_dev_opaque->attached_buffers_cnt)
> return;
>
> iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
>
> - __iio_buffer_free_sysfs_and_mask(buffer);
> + for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> + buffer = iio_dev_opaque->attached_buffers[i];
> + __iio_buffer_free_sysfs_and_mask(buffer);
> + }
> +
> + kfree(iio_dev_opaque->attached_buffers);
Hmm. This does feel like it's in slightly the wrong place, but
I'm not sure where it actually should be.
Given this is the last element in the call sequence I guess it doesn't
makes sense to bring in another function just to tidy up this connection.
> }
>
> /**
> @@ -1599,13 +1639,35 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
> * @indio_dev: The device the buffer should be attached to
> * @buffer: The buffer to attach to the device
> *
> + * Return 0 if successful, negative if error.
> + *
> * This function attaches a buffer to a IIO device. The buffer stays attached to
> - * the device until the device is freed. The function should only be called at
> - * most once per device.
> + * the device until the device is freed. For legacy reasons, the first attached
> + * buffer will also be assigned to 'indio_dev->buffer'.
> */
> -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> - struct iio_buffer *buffer)
> +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> + struct iio_buffer *buffer)
> {
> - indio_dev->buffer = iio_buffer_get(buffer);
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + struct iio_buffer **new, **old = iio_dev_opaque->attached_buffers;
> + unsigned int cnt = iio_dev_opaque->attached_buffers_cnt;
> +
> + cnt++;
> +
> + new = krealloc(old, sizeof(*new) * cnt, GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> + iio_dev_opaque->attached_buffers = new;
> +
> + buffer = iio_buffer_get(buffer);
> +
> + /* first buffer is legacy; attach it to the IIO device directly */
> + if (!indio_dev->buffer)
> + indio_dev->buffer = buffer;
> +
> + iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> + iio_dev_opaque->attached_buffers_cnt = cnt;
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 1be94df3e591..26b05dddfa71 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1583,7 +1583,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_buffers_put(indio_dev);
>
> ida_simple_remove(&iio_ida, indio_dev->id);
> kfree(iio_dev_opaque);
> @@ -1884,12 +1884,12 @@ 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)
> + if (iio_dev_opaque->attached_buffers_cnt)
> cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> else if (iio_dev_opaque->event_interface)
> cdev_init(&indio_dev->chrdev, &iio_event_fileops);
>
> - if (indio_dev->buffer || iio_dev_opaque->event_interface) {
> + if (iio_dev_opaque->attached_buffers_cnt || iio_dev_opaque->event_interface) {
> indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> indio_dev->chrdev.owner = this_mod;
> }
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index 8febc23f5f26..b6928ac5c63d 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -41,7 +41,7 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
> bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
> const unsigned long *mask);
>
> -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> - struct iio_buffer *buffer);
> +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> + struct iio_buffer *buffer);
>
> #endif /* _IIO_BUFFER_GENERIC_H_ */
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 41044320e581..768b90c64412 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -112,6 +112,9 @@ struct iio_buffer {
> /* @demux_bounce: Buffer for doing gather from incoming scan. */
> void *demux_bounce;
>
> + /* @attached_entry: Entry in the devices list of buffers attached by the driver. */
> + 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-opaque.h b/include/linux/iio/iio-opaque.h
> index 3e4c3cd248fd..c909835b6247 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -7,6 +7,8 @@
> * struct iio_dev_opaque - industrial I/O device opaque information
> * @indio_dev: public industrial I/O device information
> * @event_interface: event chrdevs associated with interrupt lines
> + * @attached_buffers: array of buffers statically attached by the driver
> + * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
> * @buffer_list: list of all buffers currently attached
> * @channel_attr_list: keep track of automatically created channel
> * attributes
> @@ -24,6 +26,8 @@
> struct iio_dev_opaque {
> struct iio_dev indio_dev;
> struct iio_event_interface *event_interface;
> + struct iio_buffer **attached_buffers;
> + unsigned int attached_buffers_cnt;
> struct list_head buffer_list;
> struct list_head channel_attr_list;
> struct attribute_group chan_attr_group;
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers
2021-02-14 13:19 ` Jonathan Cameron
@ 2021-02-14 14:02 ` Alexandru Ardelean
2021-02-14 14:37 ` Alexandru Ardelean
0 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-14 14:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Alexandru Ardelean, LKML, linux-iio, Lars-Peter Clausen,
Hennerich, Michael, Nuno Sá,
Bogdan, Dragos, Rafael J. Wysocki, Greg Kroah-Hartman
On Sun, Feb 14, 2021 at 3:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 11 Feb 2021 14:24:47 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > With this change, calling iio_device_attach_buffer() will actually attach
> > more buffers.
> > Right now this doesn't do any validation of whether a buffer is attached
> > twice; maybe that can be added later (if needed). Attaching a buffer more
> > than once should yield noticeably bad results.
> >
> > The first buffer is the legacy buffer, so a reference is kept to it.
> >
> > At this point, accessing the data for the extra buffers (that are added
> > after the first one) isn't possible yet.
> >
> > The iio_device_attach_buffer() is also changed to return an error code,
> > which for now is -ENOMEM if the array could not be realloc-ed for more
> > buffers.
>
> This is called from a bunch of places, so we need to add error handling
> for those which isn't currently present.
>
> Now arguably it is unlikely to fail, so I'm not that bothered about seeing
> them all in place as part of this series, but I would like the handling
> to have been posted at least.
I know what you're referring to.
I have a slightly better approach before updating this.
I was going to send it a little later.
I wanted to send it earlier, but I wanted to avoid overlapping myself
with too many patches.
Let me send something now [since it is ready].
>
> Otherwise, a few passing comments in here, but nothing to actually change.
>
>
> Jonathan
>
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> > drivers/iio/iio_core.h | 2 +
> > drivers/iio/industrialio-buffer.c | 96 +++++++++++++++++++++++++------
> > drivers/iio/industrialio-core.c | 6 +-
> > include/linux/iio/buffer.h | 4 +-
> > include/linux/iio/buffer_impl.h | 3 +
> > include/linux/iio/iio-opaque.h | 4 ++
> > 6 files changed, 93 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 87868fff7d37..4690c3240a5d 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -77,6 +77,7 @@ void iio_buffer_free_sysfs_and_mask(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);
> > +void iio_buffers_put(struct iio_dev *indio_dev);
> >
> > #else
> >
> > @@ -92,6 +93,7 @@ static inline void iio_buffer_free_sysfs_and_mask(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) {}
> > +static inline void iio_buffers_put(struct iio_dev *indio_dev) {}
> >
> > #endif
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 209b3a32bdbb..1e8e4c2ff00e 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -193,12 +193,14 @@ __poll_t iio_buffer_poll(struct file *filp,
> > */
> > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> > {
> > - struct iio_buffer *buffer = indio_dev->buffer;
> > -
> > - if (!buffer)
> > - return;
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + struct iio_buffer *buffer;
> > + unsigned int i;
> >
> > - wake_up(&buffer->pollq);
> > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > + buffer = iio_dev_opaque->attached_buffers[i];
> > + wake_up(&buffer->pollq);
> > + }
> > }
> >
> > void iio_buffer_init(struct iio_buffer *buffer)
> > @@ -212,6 +214,18 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > }
> > EXPORT_SYMBOL(iio_buffer_init);
> >
> > +void iio_buffers_put(struct iio_dev *indio_dev)
> > +{
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + struct iio_buffer *buffer;
> > + unsigned int i;
> > +
> > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > + buffer = iio_dev_opaque->attached_buffers[i];
> > + iio_buffer_put(buffer);
> > + }
> > +}
> > +
> > static ssize_t iio_show_scan_index(struct device *dev,
> > struct device_attribute *attr,
> > char *buf)
> > @@ -1453,9 +1467,11 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >
> > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > {
> > - struct iio_buffer *buffer = indio_dev->buffer;
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > const struct iio_chan_spec *channels;
> > - int i;
> > + struct iio_buffer *buffer;
> > + int unwind_idx;
> > + int ret, i;
> >
> > channels = indio_dev->channels;
> > if (channels) {
> > @@ -1466,22 +1482,46 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > indio_dev->masklength = ml;
> > }
> >
> > - if (!buffer)
> > + if (!iio_dev_opaque->attached_buffers_cnt)
> > return 0;
> >
> > - return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > + buffer = iio_dev_opaque->attached_buffers[i];
> > + ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
> > + if (ret) {
> > + unwind_idx = i;
> > + goto error_unwind_sysfs_and_mask;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +error_unwind_sysfs_and_mask:
> > + for (; unwind_idx >= 0; unwind_idx--) {
> > + buffer = iio_dev_opaque->attached_buffers[unwind_idx];
> > + __iio_buffer_free_sysfs_and_mask(buffer);
> > + }
> > + kfree(iio_dev_opaque->attached_buffers);
> > + return ret;
> > }
> >
> > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > {
>
> Perhaps we should do a follow up adjusting names to slightly better
> reflect they are not handling multiple buffers.
>
> > - struct iio_buffer *buffer = indio_dev->buffer;
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + struct iio_buffer *buffer;
> > + int i;
> >
> > - if (!buffer)
> > + if (!iio_dev_opaque->attached_buffers_cnt)
> > return;
> >
> > iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
> >
> > - __iio_buffer_free_sysfs_and_mask(buffer);
> > + for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> > + buffer = iio_dev_opaque->attached_buffers[i];
> > + __iio_buffer_free_sysfs_and_mask(buffer);
> > + }
> > +
> > + kfree(iio_dev_opaque->attached_buffers);
>
> Hmm. This does feel like it's in slightly the wrong place, but
> I'm not sure where it actually should be.
>
> Given this is the last element in the call sequence I guess it doesn't
> makes sense to bring in another function just to tidy up this connection.
>
> > }
> >
> > /**
> > @@ -1599,13 +1639,35 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
> > * @indio_dev: The device the buffer should be attached to
> > * @buffer: The buffer to attach to the device
> > *
> > + * Return 0 if successful, negative if error.
> > + *
> > * This function attaches a buffer to a IIO device. The buffer stays attached to
> > - * the device until the device is freed. The function should only be called at
> > - * most once per device.
> > + * the device until the device is freed. For legacy reasons, the first attached
> > + * buffer will also be assigned to 'indio_dev->buffer'.
> > */
> > -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> > - struct iio_buffer *buffer)
> > +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> > + struct iio_buffer *buffer)
> > {
> > - indio_dev->buffer = iio_buffer_get(buffer);
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + struct iio_buffer **new, **old = iio_dev_opaque->attached_buffers;
> > + unsigned int cnt = iio_dev_opaque->attached_buffers_cnt;
> > +
> > + cnt++;
> > +
> > + new = krealloc(old, sizeof(*new) * cnt, GFP_KERNEL);
> > + if (!new)
> > + return -ENOMEM;
> > + iio_dev_opaque->attached_buffers = new;
> > +
> > + buffer = iio_buffer_get(buffer);
> > +
> > + /* first buffer is legacy; attach it to the IIO device directly */
> > + if (!indio_dev->buffer)
> > + indio_dev->buffer = buffer;
> > +
> > + iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> > + iio_dev_opaque->attached_buffers_cnt = cnt;
> > +
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 1be94df3e591..26b05dddfa71 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1583,7 +1583,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_buffers_put(indio_dev);
> >
> > ida_simple_remove(&iio_ida, indio_dev->id);
> > kfree(iio_dev_opaque);
> > @@ -1884,12 +1884,12 @@ 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)
> > + if (iio_dev_opaque->attached_buffers_cnt)
> > cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > else if (iio_dev_opaque->event_interface)
> > cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> >
> > - if (indio_dev->buffer || iio_dev_opaque->event_interface) {
> > + if (iio_dev_opaque->attached_buffers_cnt || iio_dev_opaque->event_interface) {
> > indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> > indio_dev->chrdev.owner = this_mod;
> > }
> > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> > index 8febc23f5f26..b6928ac5c63d 100644
> > --- a/include/linux/iio/buffer.h
> > +++ b/include/linux/iio/buffer.h
> > @@ -41,7 +41,7 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
> > bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
> > const unsigned long *mask);
> >
> > -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> > - struct iio_buffer *buffer);
> > +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> > + struct iio_buffer *buffer);
> >
> > #endif /* _IIO_BUFFER_GENERIC_H_ */
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index 41044320e581..768b90c64412 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -112,6 +112,9 @@ struct iio_buffer {
> > /* @demux_bounce: Buffer for doing gather from incoming scan. */
> > void *demux_bounce;
> >
> > + /* @attached_entry: Entry in the devices list of buffers attached by the driver. */
> > + 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-opaque.h b/include/linux/iio/iio-opaque.h
> > index 3e4c3cd248fd..c909835b6247 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -7,6 +7,8 @@
> > * struct iio_dev_opaque - industrial I/O device opaque information
> > * @indio_dev: public industrial I/O device information
> > * @event_interface: event chrdevs associated with interrupt lines
> > + * @attached_buffers: array of buffers statically attached by the driver
> > + * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
> > * @buffer_list: list of all buffers currently attached
> > * @channel_attr_list: keep track of automatically created channel
> > * attributes
> > @@ -24,6 +26,8 @@
> > struct iio_dev_opaque {
> > struct iio_dev indio_dev;
> > struct iio_event_interface *event_interface;
> > + struct iio_buffer **attached_buffers;
> > + unsigned int attached_buffers_cnt;
> > struct list_head buffer_list;
> > struct list_head channel_attr_list;
> > struct attribute_group chan_attr_group;
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers
2021-02-14 14:02 ` Alexandru Ardelean
@ 2021-02-14 14:37 ` Alexandru Ardelean
0 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-14 14:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Alexandru Ardelean, LKML, linux-iio, Lars-Peter Clausen,
Hennerich, Michael, Nuno Sá,
Bogdan, Dragos, Rafael J. Wysocki, Greg Kroah-Hartman
On Sun, Feb 14, 2021 at 4:02 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Sun, Feb 14, 2021 at 3:21 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 11 Feb 2021 14:24:47 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >
> > > With this change, calling iio_device_attach_buffer() will actually attach
> > > more buffers.
> > > Right now this doesn't do any validation of whether a buffer is attached
> > > twice; maybe that can be added later (if needed). Attaching a buffer more
> > > than once should yield noticeably bad results.
> > >
> > > The first buffer is the legacy buffer, so a reference is kept to it.
> > >
> > > At this point, accessing the data for the extra buffers (that are added
> > > after the first one) isn't possible yet.
> > >
> > > The iio_device_attach_buffer() is also changed to return an error code,
> > > which for now is -ENOMEM if the array could not be realloc-ed for more
> > > buffers.
> >
> > This is called from a bunch of places, so we need to add error handling
> > for those which isn't currently present.
> >
> > Now arguably it is unlikely to fail, so I'm not that bothered about seeing
> > them all in place as part of this series, but I would like the handling
> > to have been posted at least.
>
> I know what you're referring to.
> I have a slightly better approach before updating this.
> I was going to send it a little later.
> I wanted to send it earlier, but I wanted to avoid overlapping myself
> with too many patches.
> Let me send something now [since it is ready].
So, coming back here.
See series [1]:
https://lore.kernel.org/linux-iio/20210214143313.67202-1-alexandru.ardelean@analog.com/T/#t
I could probably re-spin this series on top of series [1].
That would make things cleaner in terms of handling
iio_device_attach_buffer() in just 2 places I think.
>
> >
> > Otherwise, a few passing comments in here, but nothing to actually change.
> >
> >
> > Jonathan
> >
> > >
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > drivers/iio/iio_core.h | 2 +
> > > drivers/iio/industrialio-buffer.c | 96 +++++++++++++++++++++++++------
> > > drivers/iio/industrialio-core.c | 6 +-
> > > include/linux/iio/buffer.h | 4 +-
> > > include/linux/iio/buffer_impl.h | 3 +
> > > include/linux/iio/iio-opaque.h | 4 ++
> > > 6 files changed, 93 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > > index 87868fff7d37..4690c3240a5d 100644
> > > --- a/drivers/iio/iio_core.h
> > > +++ b/drivers/iio/iio_core.h
> > > @@ -77,6 +77,7 @@ void iio_buffer_free_sysfs_and_mask(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);
> > > +void iio_buffers_put(struct iio_dev *indio_dev);
> > >
> > > #else
> > >
> > > @@ -92,6 +93,7 @@ static inline void iio_buffer_free_sysfs_and_mask(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) {}
> > > +static inline void iio_buffers_put(struct iio_dev *indio_dev) {}
> > >
> > > #endif
> > >
> > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > > index 209b3a32bdbb..1e8e4c2ff00e 100644
> > > --- a/drivers/iio/industrialio-buffer.c
> > > +++ b/drivers/iio/industrialio-buffer.c
> > > @@ -193,12 +193,14 @@ __poll_t iio_buffer_poll(struct file *filp,
> > > */
> > > void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> > > {
> > > - struct iio_buffer *buffer = indio_dev->buffer;
> > > -
> > > - if (!buffer)
> > > - return;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + struct iio_buffer *buffer;
> > > + unsigned int i;
> > >
> > > - wake_up(&buffer->pollq);
> > > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > > + buffer = iio_dev_opaque->attached_buffers[i];
> > > + wake_up(&buffer->pollq);
> > > + }
> > > }
> > >
> > > void iio_buffer_init(struct iio_buffer *buffer)
> > > @@ -212,6 +214,18 @@ void iio_buffer_init(struct iio_buffer *buffer)
> > > }
> > > EXPORT_SYMBOL(iio_buffer_init);
> > >
> > > +void iio_buffers_put(struct iio_dev *indio_dev)
> > > +{
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + struct iio_buffer *buffer;
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > > + buffer = iio_dev_opaque->attached_buffers[i];
> > > + iio_buffer_put(buffer);
> > > + }
> > > +}
> > > +
> > > static ssize_t iio_show_scan_index(struct device *dev,
> > > struct device_attribute *attr,
> > > char *buf)
> > > @@ -1453,9 +1467,11 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> > >
> > > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > {
> > > - struct iio_buffer *buffer = indio_dev->buffer;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > const struct iio_chan_spec *channels;
> > > - int i;
> > > + struct iio_buffer *buffer;
> > > + int unwind_idx;
> > > + int ret, i;
> > >
> > > channels = indio_dev->channels;
> > > if (channels) {
> > > @@ -1466,22 +1482,46 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > > indio_dev->masklength = ml;
> > > }
> > >
> > > - if (!buffer)
> > > + if (!iio_dev_opaque->attached_buffers_cnt)
> > > return 0;
> > >
> > > - return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> > > + for (i = 0; i < iio_dev_opaque->attached_buffers_cnt; i++) {
> > > + buffer = iio_dev_opaque->attached_buffers[i];
> > > + ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
> > > + if (ret) {
> > > + unwind_idx = i;
> > > + goto error_unwind_sysfs_and_mask;
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +error_unwind_sysfs_and_mask:
> > > + for (; unwind_idx >= 0; unwind_idx--) {
> > > + buffer = iio_dev_opaque->attached_buffers[unwind_idx];
> > > + __iio_buffer_free_sysfs_and_mask(buffer);
> > > + }
> > > + kfree(iio_dev_opaque->attached_buffers);
> > > + return ret;
> > > }
> > >
> > > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > > {
> >
> > Perhaps we should do a follow up adjusting names to slightly better
> > reflect they are not handling multiple buffers.
> >
> > > - struct iio_buffer *buffer = indio_dev->buffer;
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + struct iio_buffer *buffer;
> > > + int i;
> > >
> > > - if (!buffer)
> > > + if (!iio_dev_opaque->attached_buffers_cnt)
> > > return;
> > >
> > > iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
> > >
> > > - __iio_buffer_free_sysfs_and_mask(buffer);
> > > + for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> > > + buffer = iio_dev_opaque->attached_buffers[i];
> > > + __iio_buffer_free_sysfs_and_mask(buffer);
> > > + }
> > > +
> > > + kfree(iio_dev_opaque->attached_buffers);
> >
> > Hmm. This does feel like it's in slightly the wrong place, but
> > I'm not sure where it actually should be.
> >
> > Given this is the last element in the call sequence I guess it doesn't
> > makes sense to bring in another function just to tidy up this connection.
> >
> > > }
> > >
> > > /**
> > > @@ -1599,13 +1639,35 @@ EXPORT_SYMBOL_GPL(iio_buffer_put);
> > > * @indio_dev: The device the buffer should be attached to
> > > * @buffer: The buffer to attach to the device
> > > *
> > > + * Return 0 if successful, negative if error.
> > > + *
> > > * This function attaches a buffer to a IIO device. The buffer stays attached to
> > > - * the device until the device is freed. The function should only be called at
> > > - * most once per device.
> > > + * the device until the device is freed. For legacy reasons, the first attached
> > > + * buffer will also be assigned to 'indio_dev->buffer'.
> > > */
> > > -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> > > - struct iio_buffer *buffer)
> > > +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> > > + struct iio_buffer *buffer)
> > > {
> > > - indio_dev->buffer = iio_buffer_get(buffer);
> > > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > > + struct iio_buffer **new, **old = iio_dev_opaque->attached_buffers;
> > > + unsigned int cnt = iio_dev_opaque->attached_buffers_cnt;
> > > +
> > > + cnt++;
> > > +
> > > + new = krealloc(old, sizeof(*new) * cnt, GFP_KERNEL);
> > > + if (!new)
> > > + return -ENOMEM;
> > > + iio_dev_opaque->attached_buffers = new;
> > > +
> > > + buffer = iio_buffer_get(buffer);
> > > +
> > > + /* first buffer is legacy; attach it to the IIO device directly */
> > > + if (!indio_dev->buffer)
> > > + indio_dev->buffer = buffer;
> > > +
> > > + iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> > > + iio_dev_opaque->attached_buffers_cnt = cnt;
> > > +
> > > + return 0;
> > > }
> > > EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > > index 1be94df3e591..26b05dddfa71 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -1583,7 +1583,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_buffers_put(indio_dev);
> > >
> > > ida_simple_remove(&iio_ida, indio_dev->id);
> > > kfree(iio_dev_opaque);
> > > @@ -1884,12 +1884,12 @@ 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)
> > > + if (iio_dev_opaque->attached_buffers_cnt)
> > > cdev_init(&indio_dev->chrdev, &iio_buffer_fileops);
> > > else if (iio_dev_opaque->event_interface)
> > > cdev_init(&indio_dev->chrdev, &iio_event_fileops);
> > >
> > > - if (indio_dev->buffer || iio_dev_opaque->event_interface) {
> > > + if (iio_dev_opaque->attached_buffers_cnt || iio_dev_opaque->event_interface) {
> > > indio_dev->dev.devt = MKDEV(MAJOR(iio_devt), indio_dev->id);
> > > indio_dev->chrdev.owner = this_mod;
> > > }
> > > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> > > index 8febc23f5f26..b6928ac5c63d 100644
> > > --- a/include/linux/iio/buffer.h
> > > +++ b/include/linux/iio/buffer.h
> > > @@ -41,7 +41,7 @@ static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
> > > bool iio_validate_scan_mask_onehot(struct iio_dev *indio_dev,
> > > const unsigned long *mask);
> > >
> > > -void iio_device_attach_buffer(struct iio_dev *indio_dev,
> > > - struct iio_buffer *buffer);
> > > +int iio_device_attach_buffer(struct iio_dev *indio_dev,
> > > + struct iio_buffer *buffer);
> > >
> > > #endif /* _IIO_BUFFER_GENERIC_H_ */
> > > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > > index 41044320e581..768b90c64412 100644
> > > --- a/include/linux/iio/buffer_impl.h
> > > +++ b/include/linux/iio/buffer_impl.h
> > > @@ -112,6 +112,9 @@ struct iio_buffer {
> > > /* @demux_bounce: Buffer for doing gather from incoming scan. */
> > > void *demux_bounce;
> > >
> > > + /* @attached_entry: Entry in the devices list of buffers attached by the driver. */
> > > + 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-opaque.h b/include/linux/iio/iio-opaque.h
> > > index 3e4c3cd248fd..c909835b6247 100644
> > > --- a/include/linux/iio/iio-opaque.h
> > > +++ b/include/linux/iio/iio-opaque.h
> > > @@ -7,6 +7,8 @@
> > > * struct iio_dev_opaque - industrial I/O device opaque information
> > > * @indio_dev: public industrial I/O device information
> > > * @event_interface: event chrdevs associated with interrupt lines
> > > + * @attached_buffers: array of buffers statically attached by the driver
> > > + * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
> > > * @buffer_list: list of all buffers currently attached
> > > * @channel_attr_list: keep track of automatically created channel
> > > * attributes
> > > @@ -24,6 +26,8 @@
> > > struct iio_dev_opaque {
> > > struct iio_dev indio_dev;
> > > struct iio_event_interface *event_interface;
> > > + struct iio_buffer **attached_buffers;
> > > + unsigned int attached_buffers_cnt;
> > > struct list_head buffer_list;
> > > struct list_head channel_attr_list;
> > > struct attribute_group chan_attr_group;
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (11 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 12/17] iio: buffer: introduce support for attaching more IIO buffers Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-14 14:37 ` Jonathan Cameron
2021-02-11 12:24 ` [PATCH v5 14/17] iio: core: rename 'dev' -> 'indio_dev' in iio_device_alloc() Alexandru Ardelean
` (3 subsequent siblings)
16 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
With this change, an ioctl() call is added to open a character device for a
buffer. The ioctl() number is 'i' 0x91, which follows the
IIO_GET_EVENT_FD_IOCTL ioctl.
The ioctl() will return an FD for the requested buffer index. The indexes
are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
variable).
Since there doesn't seem to be a sane way to return the FD for buffer0 to
be the same FD for the /dev/iio:deviceX, this ioctl() will return another
FD for buffer0 (or the first buffer). This duplicate FD will be able to
access the same buffer object (for buffer0) as accessing directly the
/dev/iio:deviceX chardev.
Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
index for each buffer (and the count) can be deduced from the
'/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
bufferY folders).
Used following C code to test this:
-------------------------------------------------------------------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <fcntl.h"
#include <errno.h>
#define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
int main(int argc, char *argv[])
{
int fd;
int fd1;
int ret;
if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
return -1;
}
fprintf(stderr, "Using FD %d\n", fd);
fd1 = atoi(argv[1]);
ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
if (ret < 0) {
fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
close(fd);
return -1;
}
fprintf(stderr, "Got FD %d\n", fd1);
close(fd1);
close(fd);
return 0;
}
-------------------------------------------------------------------
Results are:
-------------------------------------------------------------------
# ./test 0
Using FD 3
Got FD 4
# ./test 1
Using FD 3
Got FD 4
# ./test 2
Using FD 3
Got FD 4
# ./test 3
Using FD 3
Got FD 4
# ls /sys/bus/iio/devices/iio\:device0
buffer buffer0 buffer1 buffer2 buffer3 dev
in_voltage_sampling_frequency in_voltage_scale
in_voltage_scale_available
name of_node power scan_elements subsystem uevent
-------------------------------------------------------------------
iio:device0 has some fake kfifo buffers attached to an IIO device.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/iio_core.h | 12 +--
drivers/iio/industrialio-buffer.c | 138 ++++++++++++++++++++++++++++--
drivers/iio/industrialio-core.c | 1 +
include/linux/iio/buffer_impl.h | 5 ++
include/linux/iio/iio-opaque.h | 2 +
include/uapi/linux/iio/buffer.h | 10 +++
6 files changed, 157 insertions(+), 11 deletions(-)
create mode 100644 include/uapi/linux/iio/buffer.h
diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 4690c3240a5d..88db1feb5857 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -64,16 +64,16 @@ 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);
+__poll_t iio_buffer_poll_wrapper(struct file *filp,
+ struct poll_table_struct *wait);
+ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
+ size_t n, loff_t *f_ps);
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)
+#define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
+#define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
void iio_disable_all_buffers(struct iio_dev *indio_dev);
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 1e8e4c2ff00e..3aa6702a5811 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -9,6 +9,7 @@
* - Better memory allocation techniques?
* - Alternative access techniques?
*/
+#include <linux/anon_inodes.h>
#include <linux/kernel.h>
#include <linux/export.h>
#include <linux/device.h>
@@ -89,7 +90,7 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
}
/**
- * iio_buffer_read_outer() - chrdev read for buffer access
+ * iio_buffer_read() - chrdev read for buffer access
* @filp: File structure pointer for the char device
* @buf: Destination buffer for iio buffer read
* @n: First n bytes to read
@@ -101,8 +102,8 @@ 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(struct file *filp, char __user *buf,
+ size_t n, loff_t *f_ps)
{
struct iio_dev_buffer_pair *ib = filp->private_data;
struct iio_buffer *rb = ib->buffer;
@@ -168,8 +169,8 @@ 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,
- struct poll_table_struct *wait)
+static __poll_t iio_buffer_poll(struct file *filp,
+ struct poll_table_struct *wait)
{
struct iio_dev_buffer_pair *ib = filp->private_data;
struct iio_buffer *rb = ib->buffer;
@@ -184,6 +185,32 @@ __poll_t iio_buffer_poll(struct file *filp,
return 0;
}
+ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
+ size_t n, loff_t *f_ps)
+{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+
+ /* check if buffer was opened through new API */
+ if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
+ return -EBUSY;
+
+ return iio_buffer_read(filp, buf, n, f_ps);
+}
+
+__poll_t iio_buffer_poll_wrapper(struct file *filp,
+ struct poll_table_struct *wait)
+{
+ struct iio_dev_buffer_pair *ib = filp->private_data;
+ struct iio_buffer *rb = ib->buffer;
+
+ /* check if buffer was opened through new API */
+ if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
+ return 0;
+
+ return iio_buffer_poll(filp, wait);
+}
+
/**
* iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
* @indio_dev: The IIO device
@@ -1343,6 +1370,91 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
}
+static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
+{
+ struct iio_dev_buffer_pair *ib = filep->private_data;
+ struct iio_dev *indio_dev = ib->indio_dev;
+ struct iio_buffer *buffer = ib->buffer;
+
+ wake_up(&buffer->pollq);
+ clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+ iio_device_put(indio_dev);
+ kfree(ib);
+
+ return 0;
+}
+
+static const struct file_operations iio_buffer_chrdev_fileops = {
+ .owner = THIS_MODULE,
+ .llseek = noop_llseek,
+ .read = iio_buffer_read,
+ .poll = iio_buffer_poll,
+ .compat_ioctl = compat_ptr_ioctl,
+ .release = iio_buffer_chrdev_release,
+};
+
+static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ int __user *ival = (int __user *)arg;
+ char buf_name[sizeof("iio:buffer:xxx")];
+ struct iio_dev_buffer_pair *ib;
+ struct iio_buffer *buffer;
+ int fd, idx;
+
+ if (copy_from_user(&idx, ival, sizeof(idx)))
+ return -EFAULT;
+
+ if (idx >= iio_dev_opaque->attached_buffers_cnt)
+ return -ENODEV;
+
+ buffer = iio_dev_opaque->attached_buffers[idx];
+
+ if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags))
+ return -EBUSY;
+
+ iio_device_get(indio_dev);
+
+ ib = kzalloc(sizeof(*ib), GFP_KERNEL);
+ if (!ib) {
+ fd = -ENOMEM;
+ goto error_iio_dev_put;
+ }
+
+ ib->indio_dev = indio_dev;
+ ib->buffer = buffer;
+
+ fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
+ ib, O_RDWR | O_CLOEXEC);
+ if (fd < 0)
+ goto error_free_ib;
+
+ if (copy_to_user(ival, &fd, sizeof(fd))) {
+ fd = -EFAULT;
+ goto error_free_ib;
+ }
+
+ return fd;
+
+error_free_ib:
+ kfree(ib);
+error_iio_dev_put:
+ iio_device_put(indio_dev);
+ clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+ return fd;
+}
+
+static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
+ unsigned int cmd, unsigned long arg)
+{
+ switch (cmd) {
+ case IIO_BUFFER_GET_FD_IOCTL:
+ return iio_device_buffer_getfd(indio_dev, arg);
+ default:
+ return IIO_IOCTL_UNHANDLED;
+ }
+}
+
static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
struct iio_dev *indio_dev,
int index)
@@ -1472,6 +1584,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
struct iio_buffer *buffer;
int unwind_idx;
int ret, i;
+ size_t sz;
channels = indio_dev->channels;
if (channels) {
@@ -1493,6 +1606,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
goto error_unwind_sysfs_and_mask;
}
}
+ unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
+
+ sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
+ iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
+ if (!iio_dev_opaque->buffer_ioctl_handler) {
+ ret = -ENOMEM;
+ goto error_unwind_sysfs_and_mask;
+ }
+
+ iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
+ iio_device_ioctl_handler_register(indio_dev,
+ iio_dev_opaque->buffer_ioctl_handler);
return 0;
@@ -1514,6 +1639,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
if (!iio_dev_opaque->attached_buffers_cnt)
return;
+ iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
+ kfree(iio_dev_opaque->buffer_ioctl_handler);
+
iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 26b05dddfa71..febb3a0d91f3 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1737,6 +1737,7 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
struct iio_dev_buffer_pair *ib = filp->private_data;
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);
kfree(ib);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 768b90c64412..245b32918ae1 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -6,6 +6,8 @@
#ifdef CONFIG_IIO_BUFFER
+#include <uapi/linux/iio/buffer.h>
+
struct iio_dev;
struct iio_buffer;
@@ -72,6 +74,9 @@ struct iio_buffer {
/** @length: Number of datums in buffer. */
unsigned int length;
+ /** @flags: File ops flags including busy flag. */
+ unsigned long flags;
+
/** @bytes_per_datum: Size of individual datum including timestamp. */
size_t bytes_per_datum;
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index c909835b6247..2c3374d465da 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -9,6 +9,7 @@
* @event_interface: event chrdevs associated with interrupt lines
* @attached_buffers: array of buffers statically attached by the driver
* @attached_buffers_cnt: number of buffers in the array of statically attached buffers
+ * @buffer_ioctl_handler: ioctl() handler for this IIO device's buffer interface
* @buffer_list: list of all buffers currently attached
* @channel_attr_list: keep track of automatically created channel
* attributes
@@ -28,6 +29,7 @@ struct iio_dev_opaque {
struct iio_event_interface *event_interface;
struct iio_buffer **attached_buffers;
unsigned int attached_buffers_cnt;
+ struct iio_ioctl_handler *buffer_ioctl_handler;
struct list_head buffer_list;
struct list_head channel_attr_list;
struct attribute_group chan_attr_group;
diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
new file mode 100644
index 000000000000..13939032b3f6
--- /dev/null
+++ b/include/uapi/linux/iio/buffer.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* industrial I/O buffer definitions needed both in and out of kernel
+ */
+
+#ifndef _UAPI_IIO_BUFFER_H_
+#define _UAPI_IIO_BUFFER_H_
+
+#define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
+
+#endif /* _UAPI_IIO_BUFFER_H_ */
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device
2021-02-11 12:24 ` [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device Alexandru Ardelean
@ 2021-02-14 14:37 ` Jonathan Cameron
2021-02-15 9:35 ` Alexandru Ardelean
0 siblings, 1 reply; 26+ messages in thread
From: Jonathan Cameron @ 2021-02-14 14:37 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, rafael, gregkh
On Thu, 11 Feb 2021 14:24:48 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> With this change, an ioctl() call is added to open a character device for a
> buffer. The ioctl() number is 'i' 0x91, which follows the
> IIO_GET_EVENT_FD_IOCTL ioctl.
>
> The ioctl() will return an FD for the requested buffer index. The indexes
> are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> variable).
>
> Since there doesn't seem to be a sane way to return the FD for buffer0 to
> be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> FD for buffer0 (or the first buffer). This duplicate FD will be able to
> access the same buffer object (for buffer0) as accessing directly the
> /dev/iio:deviceX chardev.
>
> Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> index for each buffer (and the count) can be deduced from the
> '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> bufferY folders).
>
> Used following C code to test this:
> -------------------------------------------------------------------
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <sys/ioctl.h>
> #include <fcntl.h"
> #include <errno.h>
>
> #define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
>
> int main(int argc, char *argv[])
> {
> int fd;
> int fd1;
> int ret;
>
> if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> return -1;
> }
>
> fprintf(stderr, "Using FD %d\n", fd);
>
> fd1 = atoi(argv[1]);
>
> ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> if (ret < 0) {
> fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> close(fd);
> return -1;
> }
>
> fprintf(stderr, "Got FD %d\n", fd1);
>
> close(fd1);
> close(fd);
>
> return 0;
> }
> -------------------------------------------------------------------
>
> Results are:
> -------------------------------------------------------------------
> # ./test 0
> Using FD 3
> Got FD 4
>
> # ./test 1
> Using FD 3
> Got FD 4
>
> # ./test 2
> Using FD 3
> Got FD 4
>
> # ./test 3
> Using FD 3
> Got FD 4
>
> # ls /sys/bus/iio/devices/iio\:device0
> buffer buffer0 buffer1 buffer2 buffer3 dev
> in_voltage_sampling_frequency in_voltage_scale
> in_voltage_scale_available
> name of_node power scan_elements subsystem uevent
> -------------------------------------------------------------------
>
> iio:device0 has some fake kfifo buffers attached to an IIO device.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
There are a few corners of this that I'm not quite confident enough to just
'fix up' whilst applying.
> ---
> drivers/iio/iio_core.h | 12 +--
> drivers/iio/industrialio-buffer.c | 138 ++++++++++++++++++++++++++++--
> drivers/iio/industrialio-core.c | 1 +
> include/linux/iio/buffer_impl.h | 5 ++
> include/linux/iio/iio-opaque.h | 2 +
> include/uapi/linux/iio/buffer.h | 10 +++
> 6 files changed, 157 insertions(+), 11 deletions(-)
> create mode 100644 include/uapi/linux/iio/buffer.h
>
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 4690c3240a5d..88db1feb5857 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -64,16 +64,16 @@ 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);
> +__poll_t iio_buffer_poll_wrapper(struct file *filp,
> + struct poll_table_struct *wait);
> +ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> + size_t n, loff_t *f_ps);
>
> 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)
> +#define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> +#define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
>
> void iio_disable_all_buffers(struct iio_dev *indio_dev);
> 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 1e8e4c2ff00e..3aa6702a5811 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -9,6 +9,7 @@
> * - Better memory allocation techniques?
> * - Alternative access techniques?
> */
> +#include <linux/anon_inodes.h>
> #include <linux/kernel.h>
> #include <linux/export.h>
> #include <linux/device.h>
> @@ -89,7 +90,7 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
> }
>
> /**
> - * iio_buffer_read_outer() - chrdev read for buffer access
> + * iio_buffer_read() - chrdev read for buffer access
> * @filp: File structure pointer for the char device
> * @buf: Destination buffer for iio buffer read
> * @n: First n bytes to read
> @@ -101,8 +102,8 @@ 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(struct file *filp, char __user *buf,
> + size_t n, loff_t *f_ps)
> {
> struct iio_dev_buffer_pair *ib = filp->private_data;
> struct iio_buffer *rb = ib->buffer;
> @@ -168,8 +169,8 @@ 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,
> - struct poll_table_struct *wait)
> +static __poll_t iio_buffer_poll(struct file *filp,
> + struct poll_table_struct *wait)
> {
> struct iio_dev_buffer_pair *ib = filp->private_data;
> struct iio_buffer *rb = ib->buffer;
> @@ -184,6 +185,32 @@ __poll_t iio_buffer_poll(struct file *filp,
> return 0;
> }
>
> +ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> + size_t n, loff_t *f_ps)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> +
> + /* check if buffer was opened through new API */
> + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> + return -EBUSY;
> +
> + return iio_buffer_read(filp, buf, n, f_ps);
> +}
> +
> +__poll_t iio_buffer_poll_wrapper(struct file *filp,
> + struct poll_table_struct *wait)
> +{
> + struct iio_dev_buffer_pair *ib = filp->private_data;
> + struct iio_buffer *rb = ib->buffer;
> +
> + /* check if buffer was opened through new API */
> + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> + return 0;
> +
> + return iio_buffer_poll(filp, wait);
> +}
> +
> /**
> * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> * @indio_dev: The IIO device
> @@ -1343,6 +1370,91 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> }
>
> +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> +{
> + struct iio_dev_buffer_pair *ib = filep->private_data;
> + struct iio_dev *indio_dev = ib->indio_dev;
> + struct iio_buffer *buffer = ib->buffer;
> +
> + wake_up(&buffer->pollq);
> + clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> + iio_device_put(indio_dev);
Ordering here isn't quite the reverse of that done in _getfd()
below. Good to tidy that up, probably by swapping the test_and_set_bit()
and iio_device_get below (with additional goto needed I think).
> + kfree(ib);
> +
> + return 0;
> +}
> +
> +static const struct file_operations iio_buffer_chrdev_fileops = {
> + .owner = THIS_MODULE,
> + .llseek = noop_llseek,
> + .read = iio_buffer_read,
> + .poll = iio_buffer_poll,
> + .compat_ioctl = compat_ptr_ioctl,
> + .release = iio_buffer_chrdev_release,
> +};
> +
> +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + int __user *ival = (int __user *)arg;
> + char buf_name[sizeof("iio:buffer:xxx")];
I'm missing where this is assigned. Also not immediately sure, but I think
this can just be a string used directly in anon_inode_getfd().
I don't think we need to be unique for the different buffers as it's
a 'class name' rather than needing to be specific.
> + struct iio_dev_buffer_pair *ib;
> + struct iio_buffer *buffer;
> + int fd, idx;
> +
> + if (copy_from_user(&idx, ival, sizeof(idx)))
> + return -EFAULT;
> +
> + if (idx >= iio_dev_opaque->attached_buffers_cnt)
> + return -ENODEV;
> +
> + buffer = iio_dev_opaque->attached_buffers[idx];
> +
> + if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags))
> + return -EBUSY;
> +
> + iio_device_get(indio_dev);
> +
> + ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> + if (!ib) {
> + fd = -ENOMEM;
> + goto error_iio_dev_put;
> + }
> +
> + ib->indio_dev = indio_dev;
> + ib->buffer = buffer;
> +
> + fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
> + ib, O_RDWR | O_CLOEXEC);
> + if (fd < 0)
> + goto error_free_ib;
> +
> + if (copy_to_user(ival, &fd, sizeof(fd))) {
> + fd = -EFAULT;
I suspect the iio_events equivalent of this is wrong, as we should be
tidying up the fd and we aren't.
put_unused_fd() I think, but not 100% sure on what the right handling is here.
> + goto error_free_ib;
> + }
> +
> + return fd;
> +
> +error_free_ib:
> + kfree(ib);
> +error_iio_dev_put:
> + iio_device_put(indio_dev);
> + clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> + return fd;
> +}
> +
> +static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
> + unsigned int cmd, unsigned long arg)
> +{
> + switch (cmd) {
> + case IIO_BUFFER_GET_FD_IOCTL:
> + return iio_device_buffer_getfd(indio_dev, arg);
> + default:
> + return IIO_IOCTL_UNHANDLED;
> + }
> +}
> +
> static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> struct iio_dev *indio_dev,
> int index)
> @@ -1472,6 +1584,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> struct iio_buffer *buffer;
> int unwind_idx;
> int ret, i;
> + size_t sz;
>
> channels = indio_dev->channels;
> if (channels) {
> @@ -1493,6 +1606,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> goto error_unwind_sysfs_and_mask;
> }
> }
> + unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
> +
> + sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
> + iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
> + if (!iio_dev_opaque->buffer_ioctl_handler) {
> + ret = -ENOMEM;
> + goto error_unwind_sysfs_and_mask;
> + }
> +
> + iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
> + iio_device_ioctl_handler_register(indio_dev,
> + iio_dev_opaque->buffer_ioctl_handler);
>
> return 0;
>
> @@ -1514,6 +1639,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> if (!iio_dev_opaque->attached_buffers_cnt)
> return;
>
> + iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> + kfree(iio_dev_opaque->buffer_ioctl_handler);
> +
> iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
>
> for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 26b05dddfa71..febb3a0d91f3 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1737,6 +1737,7 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
> struct iio_dev_buffer_pair *ib = filp->private_data;
> struct iio_dev *indio_dev = container_of(inode->i_cdev,
> struct iio_dev, chrdev);
> +
Stray change that doesn't belong in here.
> clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
> iio_device_put(indio_dev);
> kfree(ib);
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 768b90c64412..245b32918ae1 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -6,6 +6,8 @@
>
> #ifdef CONFIG_IIO_BUFFER
>
> +#include <uapi/linux/iio/buffer.h>
> +
> struct iio_dev;
> struct iio_buffer;
>
> @@ -72,6 +74,9 @@ struct iio_buffer {
> /** @length: Number of datums in buffer. */
> unsigned int length;
>
> + /** @flags: File ops flags including busy flag. */
> + unsigned long flags;
> +
> /** @bytes_per_datum: Size of individual datum including timestamp. */
> size_t bytes_per_datum;
>
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index c909835b6247..2c3374d465da 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -9,6 +9,7 @@
> * @event_interface: event chrdevs associated with interrupt lines
> * @attached_buffers: array of buffers statically attached by the driver
> * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
> + * @buffer_ioctl_handler: ioctl() handler for this IIO device's buffer interface
> * @buffer_list: list of all buffers currently attached
> * @channel_attr_list: keep track of automatically created channel
> * attributes
> @@ -28,6 +29,7 @@ struct iio_dev_opaque {
> struct iio_event_interface *event_interface;
> struct iio_buffer **attached_buffers;
> unsigned int attached_buffers_cnt;
> + struct iio_ioctl_handler *buffer_ioctl_handler;
> struct list_head buffer_list;
> struct list_head channel_attr_list;
> struct attribute_group chan_attr_group;
> diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
> new file mode 100644
> index 000000000000..13939032b3f6
> --- /dev/null
> +++ b/include/uapi/linux/iio/buffer.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* industrial I/O buffer definitions needed both in and out of kernel
> + */
> +
> +#ifndef _UAPI_IIO_BUFFER_H_
> +#define _UAPI_IIO_BUFFER_H_
> +
> +#define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
> +
> +#endif /* _UAPI_IIO_BUFFER_H_ */
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device
2021-02-14 14:37 ` Jonathan Cameron
@ 2021-02-15 9:35 ` Alexandru Ardelean
0 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-15 9:35 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Alexandru Ardelean, LKML, linux-iio, Lars-Peter Clausen,
Hennerich, Michael, Nuno Sá,
Bogdan, Dragos, Rafael J. Wysocki, Greg Kroah-Hartman
On Sun, Feb 14, 2021 at 4:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 11 Feb 2021 14:24:48 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > With this change, an ioctl() call is added to open a character device for a
> > buffer. The ioctl() number is 'i' 0x91, which follows the
> > IIO_GET_EVENT_FD_IOCTL ioctl.
> >
> > The ioctl() will return an FD for the requested buffer index. The indexes
> > are the same from the /sys/iio/devices/iio:deviceX/bufferY (i.e. the Y
> > variable).
> >
> > Since there doesn't seem to be a sane way to return the FD for buffer0 to
> > be the same FD for the /dev/iio:deviceX, this ioctl() will return another
> > FD for buffer0 (or the first buffer). This duplicate FD will be able to
> > access the same buffer object (for buffer0) as accessing directly the
> > /dev/iio:deviceX chardev.
> >
> > Also, there is no IIO_BUFFER_GET_BUFFER_COUNT ioctl() implemented, as the
> > index for each buffer (and the count) can be deduced from the
> > '/sys/bus/iio/devices/iio:deviceX/bufferY' folders (i.e the number of
> > bufferY folders).
> >
> > Used following C code to test this:
> > -------------------------------------------------------------------
> >
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> > #include <sys/ioctl.h>
> > #include <fcntl.h"
> > #include <errno.h>
> >
> > #define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
> >
> > int main(int argc, char *argv[])
> > {
> > int fd;
> > int fd1;
> > int ret;
> >
> > if ((fd = open("/dev/iio:device0", O_RDWR))<0) {
> > fprintf(stderr, "Error open() %d errno %d\n",fd, errno);
> > return -1;
> > }
> >
> > fprintf(stderr, "Using FD %d\n", fd);
> >
> > fd1 = atoi(argv[1]);
> >
> > ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &fd1);
> > if (ret < 0) {
> > fprintf(stderr, "Error for buffer %d ioctl() %d errno %d\n", fd1, ret, errno);
> > close(fd);
> > return -1;
> > }
> >
> > fprintf(stderr, "Got FD %d\n", fd1);
> >
> > close(fd1);
> > close(fd);
> >
> > return 0;
> > }
> > -------------------------------------------------------------------
> >
> > Results are:
> > -------------------------------------------------------------------
> > # ./test 0
> > Using FD 3
> > Got FD 4
> >
> > # ./test 1
> > Using FD 3
> > Got FD 4
> >
> > # ./test 2
> > Using FD 3
> > Got FD 4
> >
> > # ./test 3
> > Using FD 3
> > Got FD 4
> >
> > # ls /sys/bus/iio/devices/iio\:device0
> > buffer buffer0 buffer1 buffer2 buffer3 dev
> > in_voltage_sampling_frequency in_voltage_scale
> > in_voltage_scale_available
> > name of_node power scan_elements subsystem uevent
> > -------------------------------------------------------------------
> >
> > iio:device0 has some fake kfifo buffers attached to an IIO device.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> There are a few corners of this that I'm not quite confident enough to just
> 'fix up' whilst applying.
>
>
> > ---
> > drivers/iio/iio_core.h | 12 +--
> > drivers/iio/industrialio-buffer.c | 138 ++++++++++++++++++++++++++++--
> > drivers/iio/industrialio-core.c | 1 +
> > include/linux/iio/buffer_impl.h | 5 ++
> > include/linux/iio/iio-opaque.h | 2 +
> > include/uapi/linux/iio/buffer.h | 10 +++
> > 6 files changed, 157 insertions(+), 11 deletions(-)
> > create mode 100644 include/uapi/linux/iio/buffer.h
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index 4690c3240a5d..88db1feb5857 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -64,16 +64,16 @@ 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);
> > +__poll_t iio_buffer_poll_wrapper(struct file *filp,
> > + struct poll_table_struct *wait);
> > +ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> > + size_t n, loff_t *f_ps);
> >
> > 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)
> > +#define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
> > +#define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> >
> > void iio_disable_all_buffers(struct iio_dev *indio_dev);
> > 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 1e8e4c2ff00e..3aa6702a5811 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -9,6 +9,7 @@
> > * - Better memory allocation techniques?
> > * - Alternative access techniques?
> > */
> > +#include <linux/anon_inodes.h>
> > #include <linux/kernel.h>
> > #include <linux/export.h>
> > #include <linux/device.h>
> > @@ -89,7 +90,7 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev, struct iio_buffer *buf,
> > }
> >
> > /**
> > - * iio_buffer_read_outer() - chrdev read for buffer access
> > + * iio_buffer_read() - chrdev read for buffer access
> > * @filp: File structure pointer for the char device
> > * @buf: Destination buffer for iio buffer read
> > * @n: First n bytes to read
> > @@ -101,8 +102,8 @@ 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(struct file *filp, char __user *buf,
> > + size_t n, loff_t *f_ps)
> > {
> > struct iio_dev_buffer_pair *ib = filp->private_data;
> > struct iio_buffer *rb = ib->buffer;
> > @@ -168,8 +169,8 @@ 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,
> > - struct poll_table_struct *wait)
> > +static __poll_t iio_buffer_poll(struct file *filp,
> > + struct poll_table_struct *wait)
> > {
> > struct iio_dev_buffer_pair *ib = filp->private_data;
> > struct iio_buffer *rb = ib->buffer;
> > @@ -184,6 +185,32 @@ __poll_t iio_buffer_poll(struct file *filp,
> > return 0;
> > }
> >
> > +ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
> > + size_t n, loff_t *f_ps)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > +
> > + /* check if buffer was opened through new API */
> > + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> > + return -EBUSY;
> > +
> > + return iio_buffer_read(filp, buf, n, f_ps);
> > +}
> > +
> > +__poll_t iio_buffer_poll_wrapper(struct file *filp,
> > + struct poll_table_struct *wait)
> > +{
> > + struct iio_dev_buffer_pair *ib = filp->private_data;
> > + struct iio_buffer *rb = ib->buffer;
> > +
> > + /* check if buffer was opened through new API */
> > + if (test_bit(IIO_BUSY_BIT_POS, &rb->flags))
> > + return 0;
> > +
> > + return iio_buffer_poll(filp, wait);
> > +}
> > +
> > /**
> > * iio_buffer_wakeup_poll - Wakes up the buffer waitqueue
> > * @indio_dev: The IIO device
> > @@ -1343,6 +1370,91 @@ static void iio_buffer_unregister_legacy_sysfs_groups(struct iio_dev *indio_dev)
> > kfree(iio_dev_opaque->legacy_scan_el_group.attrs);
> > }
> >
> > +static int iio_buffer_chrdev_release(struct inode *inode, struct file *filep)
> > +{
> > + struct iio_dev_buffer_pair *ib = filep->private_data;
> > + struct iio_dev *indio_dev = ib->indio_dev;
> > + struct iio_buffer *buffer = ib->buffer;
> > +
> > + wake_up(&buffer->pollq);
> > + clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > + iio_device_put(indio_dev);
>
> Ordering here isn't quite the reverse of that done in _getfd()
> below. Good to tidy that up, probably by swapping the test_and_set_bit()
> and iio_device_get below (with additional goto needed I think).
>
> > + kfree(ib);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct file_operations iio_buffer_chrdev_fileops = {
> > + .owner = THIS_MODULE,
> > + .llseek = noop_llseek,
> > + .read = iio_buffer_read,
> > + .poll = iio_buffer_poll,
> > + .compat_ioctl = compat_ptr_ioctl,
> > + .release = iio_buffer_chrdev_release,
> > +};
> > +
> > +static long iio_device_buffer_getfd(struct iio_dev *indio_dev, unsigned long arg)
> > +{
> > + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> > + int __user *ival = (int __user *)arg;
> > + char buf_name[sizeof("iio:buffer:xxx")];
>
> I'm missing where this is assigned. Also not immediately sure, but I think
> this can just be a string used directly in anon_inode_getfd().
> I don't think we need to be unique for the different buffers as it's
> a 'class name' rather than needing to be specific.
>
> > + struct iio_dev_buffer_pair *ib;
> > + struct iio_buffer *buffer;
> > + int fd, idx;
> > +
> > + if (copy_from_user(&idx, ival, sizeof(idx)))
> > + return -EFAULT;
> > +
> > + if (idx >= iio_dev_opaque->attached_buffers_cnt)
> > + return -ENODEV;
> > +
> > + buffer = iio_dev_opaque->attached_buffers[idx];
> > +
> > + if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags))
> > + return -EBUSY;
> > +
> > + iio_device_get(indio_dev);
> > +
> > + ib = kzalloc(sizeof(*ib), GFP_KERNEL);
> > + if (!ib) {
> > + fd = -ENOMEM;
> > + goto error_iio_dev_put;
> > + }
> > +
> > + ib->indio_dev = indio_dev;
> > + ib->buffer = buffer;
> > +
> > + fd = anon_inode_getfd(buf_name, &iio_buffer_chrdev_fileops,
> > + ib, O_RDWR | O_CLOEXEC);
> > + if (fd < 0)
> > + goto error_free_ib;
> > +
> > + if (copy_to_user(ival, &fd, sizeof(fd))) {
> > + fd = -EFAULT;
>
> I suspect the iio_events equivalent of this is wrong, as we should be
> tidying up the fd and we aren't.
>
> put_unused_fd() I think, but not 100% sure on what the right handling is here.
i'm a bit unsure here as well;
been trying to find examples of this clean-up [on error] in other
places, but i couldn't find any clear-to-the-point examples;
so, put_unused_fd() should be it; looks like that should be the one;
>
>
> > + goto error_free_ib;
> > + }
> > +
> > + return fd;
> > +
> > +error_free_ib:
> > + kfree(ib);
> > +error_iio_dev_put:
> > + iio_device_put(indio_dev);
> > + clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > + return fd;
> > +}
> > +
> > +static long iio_device_buffer_ioctl(struct iio_dev *indio_dev, struct file *filp,
> > + unsigned int cmd, unsigned long arg)
> > +{
> > + switch (cmd) {
> > + case IIO_BUFFER_GET_FD_IOCTL:
> > + return iio_device_buffer_getfd(indio_dev, arg);
> > + default:
> > + return IIO_IOCTL_UNHANDLED;
> > + }
> > +}
> > +
> > static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> > struct iio_dev *indio_dev,
> > int index)
> > @@ -1472,6 +1584,7 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > struct iio_buffer *buffer;
> > int unwind_idx;
> > int ret, i;
> > + size_t sz;
> >
> > channels = indio_dev->channels;
> > if (channels) {
> > @@ -1493,6 +1606,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> > goto error_unwind_sysfs_and_mask;
> > }
> > }
> > + unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
> > +
> > + sz = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
> > + iio_dev_opaque->buffer_ioctl_handler = kzalloc(sz, GFP_KERNEL);
> > + if (!iio_dev_opaque->buffer_ioctl_handler) {
> > + ret = -ENOMEM;
> > + goto error_unwind_sysfs_and_mask;
> > + }
> > +
> > + iio_dev_opaque->buffer_ioctl_handler->ioctl = iio_device_buffer_ioctl;
> > + iio_device_ioctl_handler_register(indio_dev,
> > + iio_dev_opaque->buffer_ioctl_handler);
> >
> > return 0;
> >
> > @@ -1514,6 +1639,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > if (!iio_dev_opaque->attached_buffers_cnt)
> > return;
> >
> > + iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> > + kfree(iio_dev_opaque->buffer_ioctl_handler);
> > +
> > iio_buffer_unregister_legacy_sysfs_groups(indio_dev);
> >
> > for (i = iio_dev_opaque->attached_buffers_cnt - 1; i >= 0; i--) {
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 26b05dddfa71..febb3a0d91f3 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1737,6 +1737,7 @@ static int iio_chrdev_release(struct inode *inode, struct file *filp)
> > struct iio_dev_buffer_pair *ib = filp->private_data;
> > struct iio_dev *indio_dev = container_of(inode->i_cdev,
> > struct iio_dev, chrdev);
> > +
>
> Stray change that doesn't belong in here.
>
>
> > clear_bit(IIO_BUSY_BIT_POS, &indio_dev->flags);
> > iio_device_put(indio_dev);
> > kfree(ib);
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index 768b90c64412..245b32918ae1 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -6,6 +6,8 @@
> >
> > #ifdef CONFIG_IIO_BUFFER
> >
> > +#include <uapi/linux/iio/buffer.h>
> > +
> > struct iio_dev;
> > struct iio_buffer;
> >
> > @@ -72,6 +74,9 @@ struct iio_buffer {
> > /** @length: Number of datums in buffer. */
> > unsigned int length;
> >
> > + /** @flags: File ops flags including busy flag. */
> > + unsigned long flags;
> > +
> > /** @bytes_per_datum: Size of individual datum including timestamp. */
> > size_t bytes_per_datum;
> >
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> > index c909835b6247..2c3374d465da 100644
> > --- a/include/linux/iio/iio-opaque.h
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -9,6 +9,7 @@
> > * @event_interface: event chrdevs associated with interrupt lines
> > * @attached_buffers: array of buffers statically attached by the driver
> > * @attached_buffers_cnt: number of buffers in the array of statically attached buffers
> > + * @buffer_ioctl_handler: ioctl() handler for this IIO device's buffer interface
> > * @buffer_list: list of all buffers currently attached
> > * @channel_attr_list: keep track of automatically created channel
> > * attributes
> > @@ -28,6 +29,7 @@ struct iio_dev_opaque {
> > struct iio_event_interface *event_interface;
> > struct iio_buffer **attached_buffers;
> > unsigned int attached_buffers_cnt;
> > + struct iio_ioctl_handler *buffer_ioctl_handler;
> > struct list_head buffer_list;
> > struct list_head channel_attr_list;
> > struct attribute_group chan_attr_group;
> > diff --git a/include/uapi/linux/iio/buffer.h b/include/uapi/linux/iio/buffer.h
> > new file mode 100644
> > index 000000000000..13939032b3f6
> > --- /dev/null
> > +++ b/include/uapi/linux/iio/buffer.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* industrial I/O buffer definitions needed both in and out of kernel
> > + */
> > +
> > +#ifndef _UAPI_IIO_BUFFER_H_
> > +#define _UAPI_IIO_BUFFER_H_
> > +
> > +#define IIO_BUFFER_GET_FD_IOCTL _IOWR('i', 0x91, int)
> > +
> > +#endif /* _UAPI_IIO_BUFFER_H_ */
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 14/17] iio: core: rename 'dev' -> 'indio_dev' in iio_device_alloc()
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (12 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 13/17] iio: buffer: add ioctl() to support opening extra buffers for IIO device Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 15/17] tools: iio: make iioutils_get_type() private in iio_utils Alexandru Ardelean
` (2 subsequent siblings)
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
The 'dev' variable name usually refers to 'struct device' types. However in
iio_device_alloc() this was used for the 'struct iio_dev' type, which was
sometimes causing minor confusions.
This change renames the variable to 'indio_dev', which is the usual name
used around IIO for 'struct iio_dev' type objects.
It makes grepping a bit easier as well.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
drivers/iio/industrialio-core.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index febb3a0d91f3..86ddc752ea96 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1602,7 +1602,7 @@ struct device_type iio_device_type = {
struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
{
struct iio_dev_opaque *iio_dev_opaque;
- struct iio_dev *dev;
+ struct iio_dev *indio_dev;
size_t alloc_size;
alloc_size = sizeof(struct iio_dev_opaque);
@@ -1615,31 +1615,31 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
if (!iio_dev_opaque)
return NULL;
- dev = &iio_dev_opaque->indio_dev;
- dev->priv = (char *)iio_dev_opaque +
+ indio_dev = &iio_dev_opaque->indio_dev;
+ indio_dev->priv = (char *)iio_dev_opaque +
ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
- dev->dev.parent = parent;
- dev->dev.type = &iio_device_type;
- dev->dev.bus = &iio_bus_type;
- device_initialize(&dev->dev);
- dev_set_drvdata(&dev->dev, (void *)dev);
- mutex_init(&dev->mlock);
- mutex_init(&dev->info_exist_lock);
+ indio_dev->dev.parent = parent;
+ indio_dev->dev.type = &iio_device_type;
+ indio_dev->dev.bus = &iio_bus_type;
+ device_initialize(&indio_dev->dev);
+ dev_set_drvdata(&indio_dev->dev, (void *)indio_dev);
+ mutex_init(&indio_dev->mlock);
+ mutex_init(&indio_dev->info_exist_lock);
INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list);
- dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
- if (dev->id < 0) {
+ indio_dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
+ if (indio_dev->id < 0) {
/* cannot use a dev_err as the name isn't available */
pr_err("failed to get device id\n");
kfree(iio_dev_opaque);
return NULL;
}
- dev_set_name(&dev->dev, "iio:device%d", dev->id);
+ dev_set_name(&indio_dev->dev, "iio:device%d", indio_dev->id);
INIT_LIST_HEAD(&iio_dev_opaque->buffer_list);
INIT_LIST_HEAD(&iio_dev_opaque->ioctl_handlers);
- return dev;
+ return indio_dev;
}
EXPORT_SYMBOL(iio_device_alloc);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 15/17] tools: iio: make iioutils_get_type() private in iio_utils
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (13 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 14/17] iio: core: rename 'dev' -> 'indio_dev' in iio_device_alloc() Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 16/17] tools: iio: privatize globals and functions in iio_generic_buffer.c file Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 17/17] tools: iio: convert iio_generic_buffer to use new IIO buffer API Alexandru Ardelean
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
This is a bit of a tidy-up, but also helps with extending the
iioutils_get_type() function a bit, as we don't need to use it outside of
the iio_utils.c file. So, we'll need to update it only in one place.
With this change, the 'unsigned' types are updated to 'unsigned int' in the
iioutils_get_type() function definition.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
tools/iio/iio_utils.c | 9 +++++----
tools/iio/iio_utils.h | 4 ----
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index 7399eb7f1378..a96002f2c2d5 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -82,10 +82,11 @@ int iioutils_break_up_name(const char *full_name, char **generic_name)
*
* Returns a value >= 0 on success, otherwise a negative error code.
**/
-int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
- unsigned *shift, uint64_t *mask, unsigned *be,
- const char *device_dir, const char *name,
- const char *generic_name)
+static int iioutils_get_type(unsigned int *is_signed, unsigned int *bytes,
+ unsigned int *bits_used, unsigned int *shift,
+ uint64_t *mask, unsigned int *be,
+ const char *device_dir, const char *name,
+ const char *generic_name)
{
FILE *sysfsfp;
int ret;
diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
index 74bde4fde2c8..a5d0aa8a57d3 100644
--- a/tools/iio/iio_utils.h
+++ b/tools/iio/iio_utils.h
@@ -57,10 +57,6 @@ static inline int iioutils_check_suffix(const char *str, const char *suffix)
}
int iioutils_break_up_name(const char *full_name, char **generic_name);
-int iioutils_get_type(unsigned *is_signed, unsigned *bytes, unsigned *bits_used,
- unsigned *shift, uint64_t *mask, unsigned *be,
- const char *device_dir, const char *name,
- const char *generic_name);
int iioutils_get_param_float(float *output, const char *param_name,
const char *device_dir, const char *name,
const char *generic_name);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 16/17] tools: iio: privatize globals and functions in iio_generic_buffer.c file
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (14 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 15/17] tools: iio: make iioutils_get_type() private in iio_utils Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-11 12:24 ` [PATCH v5 17/17] tools: iio: convert iio_generic_buffer to use new IIO buffer API Alexandru Ardelean
16 siblings, 0 replies; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
Mostly a tidy-up.
But also helps to understand the limits of scope of these functions and
globals.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
tools/iio/iio_generic_buffer.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 34d63bcebcd2..7c7240553777 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -49,7 +49,7 @@ enum autochan {
* Has the side effect of filling the channels[i].location values used
* in processing the buffer output.
**/
-int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
+static int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
{
int bytes = 0;
int i = 0;
@@ -68,7 +68,7 @@ int size_from_channelarray(struct iio_channel_info *channels, int num_channels)
return bytes;
}
-void print1byte(uint8_t input, struct iio_channel_info *info)
+static void print1byte(uint8_t input, struct iio_channel_info *info)
{
/*
* Shift before conversion to avoid sign extension
@@ -85,7 +85,7 @@ void print1byte(uint8_t input, struct iio_channel_info *info)
}
}
-void print2byte(uint16_t input, struct iio_channel_info *info)
+static void print2byte(uint16_t input, struct iio_channel_info *info)
{
/* First swap if incorrect endian */
if (info->be)
@@ -108,7 +108,7 @@ void print2byte(uint16_t input, struct iio_channel_info *info)
}
}
-void print4byte(uint32_t input, struct iio_channel_info *info)
+static void print4byte(uint32_t input, struct iio_channel_info *info)
{
/* First swap if incorrect endian */
if (info->be)
@@ -131,7 +131,7 @@ void print4byte(uint32_t input, struct iio_channel_info *info)
}
}
-void print8byte(uint64_t input, struct iio_channel_info *info)
+static void print8byte(uint64_t input, struct iio_channel_info *info)
{
/* First swap if incorrect endian */
if (info->be)
@@ -167,9 +167,8 @@ void print8byte(uint64_t input, struct iio_channel_info *info)
* to fill the location offsets.
* @num_channels: number of channels
**/
-void process_scan(char *data,
- struct iio_channel_info *channels,
- int num_channels)
+static void process_scan(char *data, struct iio_channel_info *channels,
+ int num_channels)
{
int k;
@@ -238,7 +237,7 @@ static int enable_disable_all_channels(char *dev_dir_name, int enable)
return 0;
}
-void print_usage(void)
+static void print_usage(void)
{
fprintf(stderr, "Usage: generic_buffer [options]...\n"
"Capture, convert and output data from IIO device buffer\n"
@@ -257,12 +256,12 @@ void print_usage(void)
" -w <n> Set delay between reads in us (event-less mode)\n");
}
-enum autochan autochannels = AUTOCHANNELS_DISABLED;
-char *dev_dir_name = NULL;
-char *buf_dir_name = NULL;
-bool current_trigger_set = false;
+static enum autochan autochannels = AUTOCHANNELS_DISABLED;
+static char *dev_dir_name = NULL;
+static char *buf_dir_name = NULL;
+static bool current_trigger_set = false;
-void cleanup(void)
+static void cleanup(void)
{
int ret;
@@ -294,14 +293,14 @@ void cleanup(void)
}
}
-void sig_handler(int signum)
+static void sig_handler(int signum)
{
fprintf(stderr, "Caught signal %d\n", signum);
cleanup();
exit(-signum);
}
-void register_cleanup(void)
+static void register_cleanup(void)
{
struct sigaction sa = { .sa_handler = sig_handler };
const int signums[] = { SIGINT, SIGTERM, SIGABRT };
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 17/17] tools: iio: convert iio_generic_buffer to use new IIO buffer API
2021-02-11 12:24 [PATCH v5 00/17] iio: core,buffer: add support for multiple IIO buffers per IIO device Alexandru Ardelean
` (15 preceding siblings ...)
2021-02-11 12:24 ` [PATCH v5 16/17] tools: iio: privatize globals and functions in iio_generic_buffer.c file Alexandru Ardelean
@ 2021-02-11 12:24 ` Alexandru Ardelean
2021-02-14 14:49 ` Jonathan Cameron
16 siblings, 1 reply; 26+ messages in thread
From: Alexandru Ardelean @ 2021-02-11 12:24 UTC (permalink / raw)
To: linux-kernel, linux-iio
Cc: lars, Michael.Hennerich, jic23, nuno.sa, dragos.bogdan, rafael,
gregkh, Alexandru Ardelean
This change makes use of the new IIO buffer API to read data from an IIO
buffer.
It doesn't read the /sys/bus/iio/devices/iio:deviceX/scan_elements dir
anymore, it reads /sys/bus/iio/devices/iio:deviceX/bufferY, where all the
scan_elements have been merged together with the old/classical buffer
attributes.
And it makes use of the new IIO_BUFFER_GET_FD_IOCTL ioctl to get an FD for
the IIO buffer for which to read data from.
It also does a quick sanity check to see that -EBUSY is returned if reading
the chardev after the ioctl() has succeeded.
This was tested with the following cases:
1. Tested buffer0 works with ioctl()
2. Tested that buffer0 can't be opened via /dev/iio:deviceX after ioctl()
3. Moved valid buffer0 to be buffer1, and tested that data comes from it
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
tools/iio/Makefile | 1 +
tools/iio/iio_generic_buffer.c | 120 ++++++++++++++++++++++++++-------
tools/iio/iio_utils.c | 13 ++--
tools/iio/iio_utils.h | 4 +-
4 files changed, 105 insertions(+), 33 deletions(-)
diff --git a/tools/iio/Makefile b/tools/iio/Makefile
index 3de763d9ab70..5d12ac4e7f8f 100644
--- a/tools/iio/Makefile
+++ b/tools/iio/Makefile
@@ -27,6 +27,7 @@ include $(srctree)/tools/build/Makefile.include
#
$(OUTPUT)include/linux/iio: ../../include/uapi/linux/iio
mkdir -p $(OUTPUT)include/linux/iio 2>&1 || true
+ ln -sf $(CURDIR)/../../include/uapi/linux/iio/buffer.h $@
ln -sf $(CURDIR)/../../include/uapi/linux/iio/events.h $@
ln -sf $(CURDIR)/../../include/uapi/linux/iio/types.h $@
diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 7c7240553777..fdd08514d556 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -30,6 +30,8 @@
#include <inttypes.h>
#include <stdbool.h>
#include <signal.h>
+#include <sys/ioctl.h>
+#include <linux/iio/buffer.h>
#include "iio_utils.h"
/**
@@ -197,7 +199,7 @@ static void process_scan(char *data, struct iio_channel_info *channels,
printf("\n");
}
-static int enable_disable_all_channels(char *dev_dir_name, int enable)
+static int enable_disable_all_channels(char *dev_dir_name, int buffer_idx, int enable)
{
const struct dirent *ent;
char scanelemdir[256];
@@ -205,7 +207,7 @@ static int enable_disable_all_channels(char *dev_dir_name, int enable)
int ret;
snprintf(scanelemdir, sizeof(scanelemdir),
- FORMAT_SCAN_ELEMENTS_DIR, dev_dir_name);
+ FORMAT_SCAN_ELEMENTS_DIR, dev_dir_name, buffer_idx);
scanelemdir[sizeof(scanelemdir)-1] = '\0';
dp = opendir(scanelemdir);
@@ -243,6 +245,7 @@ static void print_usage(void)
"Capture, convert and output data from IIO device buffer\n"
" -a Auto-activate all available channels\n"
" -A Force-activate ALL channels\n"
+ " -b <n> The buffer which to open (by index), default 0\n"
" -c <n> Do n conversions, or loop forever if n < 0\n"
" -e Disable wait for event (new data)\n"
" -g Use trigger-less mode\n"
@@ -259,6 +262,7 @@ static void print_usage(void)
static enum autochan autochannels = AUTOCHANNELS_DISABLED;
static char *dev_dir_name = NULL;
static char *buf_dir_name = NULL;
+static int buffer_idx = 0;
static bool current_trigger_set = false;
static void cleanup(void)
@@ -286,7 +290,7 @@ static void cleanup(void)
/* Disable channels if auto-enabled */
if (dev_dir_name && autochannels == AUTOCHANNELS_ACTIVE) {
- ret = enable_disable_all_channels(dev_dir_name, 0);
+ ret = enable_disable_all_channels(dev_dir_name, buffer_idx, 0);
if (ret)
fprintf(stderr, "Failed to disable all channels\n");
autochannels = AUTOCHANNELS_DISABLED;
@@ -333,7 +337,9 @@ int main(int argc, char **argv)
unsigned long long j;
unsigned long toread;
int ret, c;
- int fp = -1;
+ struct stat st;
+ int fd = -1;
+ int buf_fd = -1;
int num_channels = 0;
char *trigger_name = NULL, *device_name = NULL;
@@ -352,7 +358,7 @@ int main(int argc, char **argv)
register_cleanup();
- while ((c = getopt_long(argc, argv, "aAc:egl:n:N:t:T:w:?", longopts,
+ while ((c = getopt_long(argc, argv, "aAb:c:egl:n:N:t:T:w:?", longopts,
NULL)) != -1) {
switch (c) {
case 'a':
@@ -361,7 +367,20 @@ int main(int argc, char **argv)
case 'A':
autochannels = AUTOCHANNELS_ENABLED;
force_autochannels = true;
- break;
+ break;
+ case 'b':
+ errno = 0;
+ buffer_idx = strtoll(optarg, &dummy, 10);
+ if (errno) {
+ ret = -errno;
+ goto error;
+ }
+ if (buffer_idx < 0) {
+ ret = -ERANGE;
+ goto error;
+ }
+
+ break;
case 'c':
errno = 0;
num_loops = strtoll(optarg, &dummy, 10);
@@ -518,7 +537,7 @@ int main(int argc, char **argv)
* Parse the files in scan_elements to identify what channels are
* present
*/
- ret = build_channel_array(dev_dir_name, &channels, &num_channels);
+ ret = build_channel_array(dev_dir_name, buffer_idx, &channels, &num_channels);
if (ret) {
fprintf(stderr, "Problem reading scan element information\n"
"diag %s\n", dev_dir_name);
@@ -535,7 +554,7 @@ int main(int argc, char **argv)
(autochannels == AUTOCHANNELS_ENABLED && force_autochannels)) {
fprintf(stderr, "Enabling all channels\n");
- ret = enable_disable_all_channels(dev_dir_name, 1);
+ ret = enable_disable_all_channels(dev_dir_name, buffer_idx, 1);
if (ret) {
fprintf(stderr, "Failed to enable all channels\n");
goto error;
@@ -544,7 +563,7 @@ int main(int argc, char **argv)
/* This flags that we need to disable the channels again */
autochannels = AUTOCHANNELS_ACTIVE;
- ret = build_channel_array(dev_dir_name, &channels,
+ ret = build_channel_array(dev_dir_name, buffer_idx, &channels,
&num_channels);
if (ret) {
fprintf(stderr, "Problem reading scan element "
@@ -565,7 +584,7 @@ int main(int argc, char **argv)
fprintf(stderr, "Enable channels manually in "
FORMAT_SCAN_ELEMENTS_DIR
"/*_en or pass -a to autoenable channels and "
- "try again.\n", dev_dir_name);
+ "try again.\n", dev_dir_name, buffer_idx);
ret = -ENOENT;
goto error;
}
@@ -576,12 +595,25 @@ int main(int argc, char **argv)
* be built rather than found.
*/
ret = asprintf(&buf_dir_name,
- "%siio:device%d/buffer", iio_dir, dev_num);
+ "%siio:device%d/buffer%d", iio_dir, dev_num, buffer_idx);
if (ret < 0) {
ret = -ENOMEM;
goto error;
}
+ if (stat(buf_dir_name, &st)) {
+ fprintf(stderr, "Could not stat() '%s', got error %d: %s\n",
+ buf_dir_name, errno, strerror(errno));
+ ret = -errno;
+ goto error;
+ }
+
+ if (!S_ISDIR(st.st_mode)) {
+ fprintf(stderr, "File '%s' is not a directory\n", buf_dir_name);
+ ret = -EFAULT;
+ goto error;
+ }
+
if (!notrigger) {
printf("%s %s\n", dev_dir_name, trigger_name);
/*
@@ -598,6 +630,37 @@ int main(int argc, char **argv)
}
}
+ ret = asprintf(&buffer_access, "/dev/iio:device%d", dev_num);
+ if (ret < 0) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ /* Attempt to open non blocking the access dev */
+ fd = open(buffer_access, O_RDONLY | O_NONBLOCK);
+ if (fd == -1) { /* TODO: If it isn't there make the node */
+ ret = -errno;
+ fprintf(stderr, "Failed to open %s\n", buffer_access);
+ goto error;
+ }
+
+ /* specify for which buffer index we want an FD */
+ buf_fd = buffer_idx;
+
+ ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &buf_fd);
+ if (ret == -1 || buf_fd == -1) {
+ ret = -errno;
+ if (ret == -ENODEV || ret == -EINVAL)
+ fprintf(stderr,
+ "This device does not support buffers\n");
+ else
+ fprintf(stderr, "Failed to retrieve buffer fd\n");
+ if (close(fd) == -1)
+ perror("Failed to close character device file");
+
+ goto error;
+ }
+
/* Setup ring buffer parameters */
ret = write_sysfs_int("length", buf_dir_name, buf_len);
if (ret < 0)
@@ -607,7 +670,8 @@ int main(int argc, char **argv)
ret = write_sysfs_int("enable", buf_dir_name, 1);
if (ret < 0) {
fprintf(stderr,
- "Failed to enable buffer: %s\n", strerror(-ret));
+ "Failed to enable buffer '%s': %s\n",
+ buf_dir_name, strerror(-ret));
goto error;
}
@@ -618,24 +682,26 @@ int main(int argc, char **argv)
goto error;
}
- ret = asprintf(&buffer_access, "/dev/iio:device%d", dev_num);
- if (ret < 0) {
- ret = -ENOMEM;
- goto error;
+ /* if this is buffer0, check that we get EBUSY after this point */
+ if (buffer_idx == 0) {
+ errno = 0;
+ read_size = read(fd, data, 1);
+ if (read_size > -1 || errno != EBUSY) {
+ ret = -EFAULT;
+ perror("Reading from '%s' should not be possible after ioctl()");
+ goto error;
+ }
}
- /* Attempt to open non blocking the access dev */
- fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
- if (fp == -1) { /* TODO: If it isn't there make the node */
- ret = -errno;
- fprintf(stderr, "Failed to open %s\n", buffer_access);
- goto error;
- }
+ /* close now the main chardev FD and let the buffer FD work */
+ if (close(fd) == -1)
+ perror("Failed to close character device file");
+ fd = -1;
for (j = 0; j < num_loops || num_loops < 0; j++) {
if (!noevents) {
struct pollfd pfd = {
- .fd = fp,
+ .fd = buf_fd,
.events = POLLIN,
};
@@ -653,7 +719,7 @@ int main(int argc, char **argv)
toread = 64;
}
- read_size = read(fp, data, toread * scan_size);
+ read_size = read(buf_fd, data, toread * scan_size);
if (read_size < 0) {
if (errno == EAGAIN) {
fprintf(stderr, "nothing available\n");
@@ -670,7 +736,9 @@ int main(int argc, char **argv)
error:
cleanup();
- if (fp >= 0 && close(fp) == -1)
+ if (fd >= 0 && close(fd) == -1)
+ perror("Failed to close character device");
+ if (buf_fd >= 0 && close(buf_fd) == -1)
perror("Failed to close buffer");
free(buffer_access);
free(data);
diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
index a96002f2c2d5..aadee6d34c74 100644
--- a/tools/iio/iio_utils.c
+++ b/tools/iio/iio_utils.c
@@ -77,6 +77,7 @@ int iioutils_break_up_name(const char *full_name, char **generic_name)
* @mask: output a bit mask for the raw data
* @be: output if data in big endian
* @device_dir: the IIO device directory
+ * @buffer_idx: the IIO buffer index
* @name: the channel name
* @generic_name: the channel type name
*
@@ -85,8 +86,8 @@ int iioutils_break_up_name(const char *full_name, char **generic_name)
static int iioutils_get_type(unsigned int *is_signed, unsigned int *bytes,
unsigned int *bits_used, unsigned int *shift,
uint64_t *mask, unsigned int *be,
- const char *device_dir, const char *name,
- const char *generic_name)
+ const char *device_dir, int buffer_idx,
+ const char *name, const char *generic_name)
{
FILE *sysfsfp;
int ret;
@@ -96,7 +97,7 @@ static int iioutils_get_type(unsigned int *is_signed, unsigned int *bytes,
unsigned padint;
const struct dirent *ent;
- ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir);
+ ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir, buffer_idx);
if (ret < 0)
return -ENOMEM;
@@ -304,12 +305,13 @@ void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
/**
* build_channel_array() - function to figure out what channels are present
* @device_dir: the IIO device directory in sysfs
+ * @buffer_idx: the IIO buffer for this channel array
* @ci_array: output the resulting array of iio_channel_info
* @counter: output the amount of array elements
*
* Returns 0 on success, otherwise a negative error code.
**/
-int build_channel_array(const char *device_dir,
+int build_channel_array(const char *device_dir, int buffer_idx,
struct iio_channel_info **ci_array, int *counter)
{
DIR *dp;
@@ -322,7 +324,7 @@ int build_channel_array(const char *device_dir,
char *filename;
*counter = 0;
- ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir);
+ ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir, buffer_idx);
if (ret < 0)
return -ENOMEM;
@@ -503,6 +505,7 @@ int build_channel_array(const char *device_dir,
¤t->mask,
¤t->be,
device_dir,
+ buffer_idx,
current->name,
current->generic_name);
if (ret < 0)
diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
index a5d0aa8a57d3..336752cade4f 100644
--- a/tools/iio/iio_utils.h
+++ b/tools/iio/iio_utils.h
@@ -12,7 +12,7 @@
/* Made up value to limit allocation sizes */
#define IIO_MAX_NAME_LENGTH 64
-#define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
+#define FORMAT_SCAN_ELEMENTS_DIR "%s/buffer%d"
#define FORMAT_TYPE_FILE "%s_type"
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
@@ -61,7 +61,7 @@ int iioutils_get_param_float(float *output, const char *param_name,
const char *device_dir, const char *name,
const char *generic_name);
void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
-int build_channel_array(const char *device_dir,
+int build_channel_array(const char *device_dir, int buffer_idx,
struct iio_channel_info **ci_array, int *counter);
int find_type_by_name(const char *name, const char *type);
int write_sysfs_int(const char *filename, const char *basedir, int val);
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 17/17] tools: iio: convert iio_generic_buffer to use new IIO buffer API
2021-02-11 12:24 ` [PATCH v5 17/17] tools: iio: convert iio_generic_buffer to use new IIO buffer API Alexandru Ardelean
@ 2021-02-14 14:49 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2021-02-14 14:49 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: linux-kernel, linux-iio, lars, Michael.Hennerich, nuno.sa,
dragos.bogdan, rafael, gregkh
On Thu, 11 Feb 2021 14:24:52 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> This change makes use of the new IIO buffer API to read data from an IIO
> buffer.
> It doesn't read the /sys/bus/iio/devices/iio:deviceX/scan_elements dir
> anymore, it reads /sys/bus/iio/devices/iio:deviceX/bufferY, where all the
> scan_elements have been merged together with the old/classical buffer
> attributes.
>
> And it makes use of the new IIO_BUFFER_GET_FD_IOCTL ioctl to get an FD for
> the IIO buffer for which to read data from.
> It also does a quick sanity check to see that -EBUSY is returned if reading
> the chardev after the ioctl() has succeeded.
>
> This was tested with the following cases:
> 1. Tested buffer0 works with ioctl()
> 2. Tested that buffer0 can't be opened via /dev/iio:deviceX after ioctl()
As mentioned below, this good to have but needs stronger comment next to it
for anyone using this code as a basis for their own. Normally you just
wouldn't try doing this!
> 3. Moved valid buffer0 to be buffer1, and tested that data comes from it
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
A few other things inline.
Thanks,
Jonathan
> ---
> tools/iio/Makefile | 1 +
> tools/iio/iio_generic_buffer.c | 120 ++++++++++++++++++++++++++-------
> tools/iio/iio_utils.c | 13 ++--
> tools/iio/iio_utils.h | 4 +-
> 4 files changed, 105 insertions(+), 33 deletions(-)
>
> diff --git a/tools/iio/Makefile b/tools/iio/Makefile
> index 3de763d9ab70..5d12ac4e7f8f 100644
> --- a/tools/iio/Makefile
> +++ b/tools/iio/Makefile
> @@ -27,6 +27,7 @@ include $(srctree)/tools/build/Makefile.include
> #
> $(OUTPUT)include/linux/iio: ../../include/uapi/linux/iio
> mkdir -p $(OUTPUT)include/linux/iio 2>&1 || true
> + ln -sf $(CURDIR)/../../include/uapi/linux/iio/buffer.h $@
> ln -sf $(CURDIR)/../../include/uapi/linux/iio/events.h $@
> ln -sf $(CURDIR)/../../include/uapi/linux/iio/types.h $@
>
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 7c7240553777..fdd08514d556 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -30,6 +30,8 @@
> #include <inttypes.h>
> #include <stdbool.h>
> #include <signal.h>
> +#include <sys/ioctl.h>
> +#include <linux/iio/buffer.h>
> #include "iio_utils.h"
>
> /**
> @@ -197,7 +199,7 @@ static void process_scan(char *data, struct iio_channel_info *channels,
> printf("\n");
> }
>
> -static int enable_disable_all_channels(char *dev_dir_name, int enable)
> +static int enable_disable_all_channels(char *dev_dir_name, int buffer_idx, int enable)
> {
> const struct dirent *ent;
> char scanelemdir[256];
> @@ -205,7 +207,7 @@ static int enable_disable_all_channels(char *dev_dir_name, int enable)
> int ret;
>
> snprintf(scanelemdir, sizeof(scanelemdir),
> - FORMAT_SCAN_ELEMENTS_DIR, dev_dir_name);
> + FORMAT_SCAN_ELEMENTS_DIR, dev_dir_name, buffer_idx);
> scanelemdir[sizeof(scanelemdir)-1] = '\0';
>
> dp = opendir(scanelemdir);
> @@ -243,6 +245,7 @@ static void print_usage(void)
> "Capture, convert and output data from IIO device buffer\n"
> " -a Auto-activate all available channels\n"
> " -A Force-activate ALL channels\n"
> + " -b <n> The buffer which to open (by index), default 0\n"
> " -c <n> Do n conversions, or loop forever if n < 0\n"
> " -e Disable wait for event (new data)\n"
> " -g Use trigger-less mode\n"
> @@ -259,6 +262,7 @@ static void print_usage(void)
> static enum autochan autochannels = AUTOCHANNELS_DISABLED;
> static char *dev_dir_name = NULL;
> static char *buf_dir_name = NULL;
> +static int buffer_idx = 0;
> static bool current_trigger_set = false;
>
> static void cleanup(void)
> @@ -286,7 +290,7 @@ static void cleanup(void)
>
> /* Disable channels if auto-enabled */
> if (dev_dir_name && autochannels == AUTOCHANNELS_ACTIVE) {
> - ret = enable_disable_all_channels(dev_dir_name, 0);
> + ret = enable_disable_all_channels(dev_dir_name, buffer_idx, 0);
> if (ret)
> fprintf(stderr, "Failed to disable all channels\n");
> autochannels = AUTOCHANNELS_DISABLED;
> @@ -333,7 +337,9 @@ int main(int argc, char **argv)
> unsigned long long j;
> unsigned long toread;
> int ret, c;
> - int fp = -1;
> + struct stat st;
> + int fd = -1;
> + int buf_fd = -1;
>
> int num_channels = 0;
> char *trigger_name = NULL, *device_name = NULL;
> @@ -352,7 +358,7 @@ int main(int argc, char **argv)
>
> register_cleanup();
>
> - while ((c = getopt_long(argc, argv, "aAc:egl:n:N:t:T:w:?", longopts,
> + while ((c = getopt_long(argc, argv, "aAb:c:egl:n:N:t:T:w:?", longopts,
> NULL)) != -1) {
> switch (c) {
> case 'a':
> @@ -361,7 +367,20 @@ int main(int argc, char **argv)
> case 'A':
> autochannels = AUTOCHANNELS_ENABLED;
> force_autochannels = true;
> - break;
> + break;
> + case 'b':
> + errno = 0;
> + buffer_idx = strtoll(optarg, &dummy, 10);
> + if (errno) {
> + ret = -errno;
> + goto error;
> + }
> + if (buffer_idx < 0) {
> + ret = -ERANGE;
> + goto error;
> + }
> +
> + break;
> case 'c':
> errno = 0;
> num_loops = strtoll(optarg, &dummy, 10);
> @@ -518,7 +537,7 @@ int main(int argc, char **argv)
> * Parse the files in scan_elements to identify what channels are
> * present
> */
> - ret = build_channel_array(dev_dir_name, &channels, &num_channels);
> + ret = build_channel_array(dev_dir_name, buffer_idx, &channels, &num_channels);
> if (ret) {
> fprintf(stderr, "Problem reading scan element information\n"
> "diag %s\n", dev_dir_name);
> @@ -535,7 +554,7 @@ int main(int argc, char **argv)
> (autochannels == AUTOCHANNELS_ENABLED && force_autochannels)) {
> fprintf(stderr, "Enabling all channels\n");
>
> - ret = enable_disable_all_channels(dev_dir_name, 1);
> + ret = enable_disable_all_channels(dev_dir_name, buffer_idx, 1);
> if (ret) {
> fprintf(stderr, "Failed to enable all channels\n");
> goto error;
> @@ -544,7 +563,7 @@ int main(int argc, char **argv)
> /* This flags that we need to disable the channels again */
> autochannels = AUTOCHANNELS_ACTIVE;
>
> - ret = build_channel_array(dev_dir_name, &channels,
> + ret = build_channel_array(dev_dir_name, buffer_idx, &channels,
> &num_channels);
> if (ret) {
> fprintf(stderr, "Problem reading scan element "
> @@ -565,7 +584,7 @@ int main(int argc, char **argv)
> fprintf(stderr, "Enable channels manually in "
> FORMAT_SCAN_ELEMENTS_DIR
> "/*_en or pass -a to autoenable channels and "
> - "try again.\n", dev_dir_name);
> + "try again.\n", dev_dir_name, buffer_idx);
> ret = -ENOENT;
> goto error;
> }
> @@ -576,12 +595,25 @@ int main(int argc, char **argv)
> * be built rather than found.
> */
> ret = asprintf(&buf_dir_name,
> - "%siio:device%d/buffer", iio_dir, dev_num);
> + "%siio:device%d/buffer%d", iio_dir, dev_num, buffer_idx);
> if (ret < 0) {
> ret = -ENOMEM;
> goto error;
> }
>
> + if (stat(buf_dir_name, &st)) {
> + fprintf(stderr, "Could not stat() '%s', got error %d: %s\n",
> + buf_dir_name, errno, strerror(errno));
> + ret = -errno;
> + goto error;
> + }
> +
> + if (!S_ISDIR(st.st_mode)) {
> + fprintf(stderr, "File '%s' is not a directory\n", buf_dir_name);
> + ret = -EFAULT;
> + goto error;
> + }
> +
> if (!notrigger) {
> printf("%s %s\n", dev_dir_name, trigger_name);
> /*
> @@ -598,6 +630,37 @@ int main(int argc, char **argv)
> }
> }
>
> + ret = asprintf(&buffer_access, "/dev/iio:device%d", dev_num);
> + if (ret < 0) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + /* Attempt to open non blocking the access dev */
No point in opening this one non blocking any more as only used
for the ioctl.
> + fd = open(buffer_access, O_RDONLY | O_NONBLOCK);
> + if (fd == -1) { /* TODO: If it isn't there make the node */
> + ret = -errno;
> + fprintf(stderr, "Failed to open %s\n", buffer_access);
> + goto error;
> + }
> +
> + /* specify for which buffer index we want an FD */
> + buf_fd = buffer_idx;
> +
> + ret = ioctl(fd, IIO_BUFFER_GET_FD_IOCTL, &buf_fd);
> + if (ret == -1 || buf_fd == -1) {
> + ret = -errno;
> + if (ret == -ENODEV || ret == -EINVAL)
> + fprintf(stderr,
> + "This device does not support buffers\n");
This many buffers perhaps?
> + else
> + fprintf(stderr, "Failed to retrieve buffer fd\n");
> + if (close(fd) == -1)
> + perror("Failed to close character device file");
Why close this here?
> +
> + goto error;
> + }
> +
> /* Setup ring buffer parameters */
> ret = write_sysfs_int("length", buf_dir_name, buf_len);
> if (ret < 0)
> @@ -607,7 +670,8 @@ int main(int argc, char **argv)
> ret = write_sysfs_int("enable", buf_dir_name, 1);
> if (ret < 0) {
> fprintf(stderr,
> - "Failed to enable buffer: %s\n", strerror(-ret));
> + "Failed to enable buffer '%s': %s\n",
> + buf_dir_name, strerror(-ret));
> goto error;
> }
>
> @@ -618,24 +682,26 @@ int main(int argc, char **argv)
> goto error;
> }
>
> - ret = asprintf(&buffer_access, "/dev/iio:device%d", dev_num);
> - if (ret < 0) {
> - ret = -ENOMEM;
> - goto error;
> + /* if this is buffer0, check that we get EBUSY after this point */
> + if (buffer_idx == 0) {
Whilst this check is good from a sanity check point of view, I'd add
a clear comment that there is no need to do this in a normal program!
> + errno = 0;
> + read_size = read(fd, data, 1);
> + if (read_size > -1 || errno != EBUSY) {
> + ret = -EFAULT;
> + perror("Reading from '%s' should not be possible after ioctl()");
> + goto error;
> + }
> }
>
> - /* Attempt to open non blocking the access dev */
> - fp = open(buffer_access, O_RDONLY | O_NONBLOCK);
> - if (fp == -1) { /* TODO: If it isn't there make the node */
> - ret = -errno;
> - fprintf(stderr, "Failed to open %s\n", buffer_access);
> - goto error;
> - }
> + /* close now the main chardev FD and let the buffer FD work */
> + if (close(fd) == -1)
> + perror("Failed to close character device file");
> + fd = -1;
>
> for (j = 0; j < num_loops || num_loops < 0; j++) {
> if (!noevents) {
> struct pollfd pfd = {
> - .fd = fp,
> + .fd = buf_fd,
> .events = POLLIN,
> };
>
> @@ -653,7 +719,7 @@ int main(int argc, char **argv)
> toread = 64;
> }
>
> - read_size = read(fp, data, toread * scan_size);
> + read_size = read(buf_fd, data, toread * scan_size);
> if (read_size < 0) {
> if (errno == EAGAIN) {
> fprintf(stderr, "nothing available\n");
> @@ -670,7 +736,9 @@ int main(int argc, char **argv)
> error:
> cleanup();
>
> - if (fp >= 0 && close(fp) == -1)
> + if (fd >= 0 && close(fd) == -1)
> + perror("Failed to close character device");
> + if (buf_fd >= 0 && close(buf_fd) == -1)
> perror("Failed to close buffer");
> free(buffer_access);
> free(data);
> diff --git a/tools/iio/iio_utils.c b/tools/iio/iio_utils.c
> index a96002f2c2d5..aadee6d34c74 100644
> --- a/tools/iio/iio_utils.c
> +++ b/tools/iio/iio_utils.c
> @@ -77,6 +77,7 @@ int iioutils_break_up_name(const char *full_name, char **generic_name)
> * @mask: output a bit mask for the raw data
> * @be: output if data in big endian
> * @device_dir: the IIO device directory
> + * @buffer_idx: the IIO buffer index
> * @name: the channel name
> * @generic_name: the channel type name
> *
> @@ -85,8 +86,8 @@ int iioutils_break_up_name(const char *full_name, char **generic_name)
> static int iioutils_get_type(unsigned int *is_signed, unsigned int *bytes,
> unsigned int *bits_used, unsigned int *shift,
> uint64_t *mask, unsigned int *be,
> - const char *device_dir, const char *name,
> - const char *generic_name)
> + const char *device_dir, int buffer_idx,
> + const char *name, const char *generic_name)
> {
> FILE *sysfsfp;
> int ret;
> @@ -96,7 +97,7 @@ static int iioutils_get_type(unsigned int *is_signed, unsigned int *bytes,
> unsigned padint;
> const struct dirent *ent;
>
> - ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir);
> + ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir, buffer_idx);
> if (ret < 0)
> return -ENOMEM;
>
> @@ -304,12 +305,13 @@ void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt)
> /**
> * build_channel_array() - function to figure out what channels are present
> * @device_dir: the IIO device directory in sysfs
> + * @buffer_idx: the IIO buffer for this channel array
> * @ci_array: output the resulting array of iio_channel_info
> * @counter: output the amount of array elements
> *
> * Returns 0 on success, otherwise a negative error code.
> **/
> -int build_channel_array(const char *device_dir,
> +int build_channel_array(const char *device_dir, int buffer_idx,
> struct iio_channel_info **ci_array, int *counter)
> {
> DIR *dp;
> @@ -322,7 +324,7 @@ int build_channel_array(const char *device_dir,
> char *filename;
>
> *counter = 0;
> - ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir);
> + ret = asprintf(&scan_el_dir, FORMAT_SCAN_ELEMENTS_DIR, device_dir, buffer_idx);
> if (ret < 0)
> return -ENOMEM;
>
> @@ -503,6 +505,7 @@ int build_channel_array(const char *device_dir,
> ¤t->mask,
> ¤t->be,
> device_dir,
> + buffer_idx,
> current->name,
> current->generic_name);
> if (ret < 0)
> diff --git a/tools/iio/iio_utils.h b/tools/iio/iio_utils.h
> index a5d0aa8a57d3..336752cade4f 100644
> --- a/tools/iio/iio_utils.h
> +++ b/tools/iio/iio_utils.h
> @@ -12,7 +12,7 @@
> /* Made up value to limit allocation sizes */
> #define IIO_MAX_NAME_LENGTH 64
>
> -#define FORMAT_SCAN_ELEMENTS_DIR "%s/scan_elements"
> +#define FORMAT_SCAN_ELEMENTS_DIR "%s/buffer%d"
> #define FORMAT_TYPE_FILE "%s_type"
>
> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
> @@ -61,7 +61,7 @@ int iioutils_get_param_float(float *output, const char *param_name,
> const char *device_dir, const char *name,
> const char *generic_name);
> void bsort_channel_array_by_index(struct iio_channel_info *ci_array, int cnt);
> -int build_channel_array(const char *device_dir,
> +int build_channel_array(const char *device_dir, int buffer_idx,
> struct iio_channel_info **ci_array, int *counter);
> int find_type_by_name(const char *name, const char *type);
> int write_sysfs_int(const char *filename, const char *basedir, int val);
^ permalink raw reply [flat|nested] 26+ messages in thread