[RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per IIO device

[RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per IIO device

[Alexandru Ardelean]

Continuing from:
  https://lore.kernel.org/linux-iio/20200517144023.6c5cb169@archlinux/

This is a V2 of the initial attempt in the discussion above.
But it did not occur to me that I should mark it as V2 when I generated
the patches.
I've only tested [so far] that the current IIO buffer mechnism still works.
And decided to show this sketch patchset.

This requires the ioctl() centralization mechanism, for which I sent a
fix earlier.
  https://lore.kernel.org/linux-iio/CA+U=Dsqf3UgyM666Gg9EmehpWiucDx2P0bmsC9JR--JJDT_eWQ@mail.gmail.com/T/#t
  https://lore.kernel.org/linux-iio/20201117095154.7189-1-alexandru.ardelean@analog.com/T/#u

The gist of this is that now, the first IIO buffer should work as
before, but all extra buffers should go through the anon inodes
mechanism.
I'd need to find a device or a way or a chip to test these extra buffers
stuff. But I'm confident that this current form should eventually work
with multiple IIO buffers per 1 IIO device and with anon inodes.

Maybe I'll take some of the patches in this set separately and send them
individually. The problem with patchsets like this that tackle changes
in a framework (like IIO) is that I become unsure after the 5th-7th patch,
that the approach is correct. And I get even more unsure after that.

I'll create some userspace code to test this a bit, but I thought I'd
send an RFC in the meantime.

Alexandru Ardelean (12):
  iio: core: register chardev only if needed
  iio: buffer: add back-ref from iio_buffer to iio_dev
  iio: buffer: rework buffer & scan_elements dir creation
  iio: buffer: add index to the first IIO buffer dir and symlink it back
  iio: core: split __iio_device_attr_init() to init only the attr object
  iio: buffer: re-route scan_elements via it's kobj_type
  iio: buffer: re-route core buffer attributes via it's new kobj_type
  iio: buffer: add helper to get the IIO device to which a buffer
    belongs
  iio: re-route all buffer attributes through new buffer kobj_type
  iio: core: wrap iio device & buffer into struct for character devices
  iio: buffer: introduce support for attaching more IIO buffers
  iio: buffer: add ioctl() to support opening extra buffers for IIO
    device

 drivers/iio/accel/adxl372.c                   |  36 +-
 drivers/iio/accel/bmc150-accel-core.c         |  34 +-
 drivers/iio/adc/at91-sama5d2_adc.c            |  30 +-
 .../buffer/industrialio-buffer-dmaengine.c    |  13 +-
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  30 +-
 .../common/hid-sensors/hid-sensor-trigger.c   |  32 +-
 drivers/iio/iio_core.h                        |  11 +
 drivers/iio/industrialio-buffer.c             | 582 ++++++++++++++----
 drivers/iio/industrialio-core.c               | 117 ++--
 include/linux/iio/buffer.h                    |   2 +
 include/linux/iio/buffer_impl.h               |  25 +-
 include/linux/iio/iio-opaque.h                |   6 +
 include/linux/iio/iio.h                       |   2 +-
 include/linux/iio/sysfs.h                     |  50 ++
 include/uapi/linux/iio/buffer.h               |  16 +
 15 files changed, 735 insertions(+), 251 deletions(-)
 create mode 100644 include/uapi/linux/iio/buffer.h

-- 
2.17.1

[RFC PATCH 01/12] iio: core: register chardev only if needed

[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().

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 419d6f8acc13..ca8b11541477 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = {
 	.compat_ioctl = compat_ptr_ioctl,
 };
 
+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;
@@ -1790,6 +1799,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)
@@ -1807,9 +1817,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);
@@ -1838,9 +1845,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

[RFC PATCH 02/12] iio: buffer: add back-ref from iio_buffer to iio_dev

[Alexandru Ardelean]

An IIO device will have multiple buffers, but it shouldn't be allowed that
an IIO buffer should belong to more than 1 IIO device.

Once things get moved more from IIO device to the IIO buffer, and an IIO
device will be able to have more than 1 buffer attached, there will be a
need for a back-ref to the IIO device [from the IIO buffer].

This change adds that.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 2 ++
 include/linux/iio/buffer_impl.h   | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 9663dec3dcf3..08aa8e0782ce 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1505,5 +1505,7 @@ void iio_device_attach_buffer(struct iio_dev *indio_dev,
 			      struct iio_buffer *buffer)
 {
 	indio_dev->buffer = iio_buffer_get(buffer);
+
+	indio_dev->buffer->indio_dev = indio_dev;
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index a63dc07b7350..67d73d465e02 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -69,6 +69,9 @@ struct iio_buffer_access_funcs {
  * those writing new buffer implementations.
  */
 struct iio_buffer {
+	/** @indio_dev: IIO device to which this buffer belongs to. */
+	struct iio_dev *indio_dev;
+
 	/** @length: Number of datums in buffer. */
 	unsigned int length;
 
-- 
2.17.1

[RFC PATCH 03/12] iio: buffer: rework buffer & scan_elements dir creation

[Alexandru Ardelean]

When adding more than one IIO buffer per IIO device, we will need to create
a buffer & scan_elements directory for each buffer.
We also want to move the 'scan_elements' to be a sub-directory of the
'buffer' folder.

The format we want to reach is, for a iio:device0 folder, for 2 buffers
[for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
it's own 'scan_elements' subfolder.

So, for example:
   iio:device0/buffer0
      scan_elements/

   iio:device0/buffer1
      scan_elements/

The other attributes under 'bufferX' would remain unchanged.

However, we would also need to symlink back to the old 'buffer' &
'scan_elements' folders, to keep backwards compatibility.

Doing all these, require that we maintain the kobjects for each 'bufferX'
and 'scan_elements' so that we can symlink them back. We also need to
implement the sysfs_ops for these folders.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 151 ++++++++++++++++++++++++++----
 drivers/iio/industrialio-core.c   |  24 ++---
 include/linux/iio/buffer_impl.h   |  14 ++-
 include/linux/iio/iio.h           |   2 +-
 4 files changed, 156 insertions(+), 35 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 08aa8e0782ce..8b31faf049a5 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,6 +1250,101 @@ static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_data_available.attr,
 };
 
+#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+
+static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
+					struct attribute *attr,
+					char *buf)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
+	struct device_attribute *dattr;
+
+	dattr = to_dev_attr(attr);
+
+	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+}
+
+static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
+					 struct attribute *attr,
+					 const char *buf,
+					 size_t len)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
+	struct device_attribute *dattr;
+
+	dattr = to_dev_attr(attr);
+
+	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+}
+
+static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
+	.show = iio_buffer_dir_attr_show,
+	.store = iio_buffer_dir_attr_store,
+};
+
+static struct kobj_type iio_buffer_dir_ktype = {
+	.sysfs_ops = &iio_buffer_dir_sysfs_ops,
+};
+
+static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
+					 struct attribute *attr,
+					 char *buf)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
+	struct device_attribute *dattr = to_dev_attr(attr);
+
+	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+}
+
+static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
+					  struct attribute *attr,
+					  const char *buf,
+					  size_t len)
+{
+	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
+	struct device_attribute *dattr = to_dev_attr(attr);
+
+	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+}
+
+static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
+	.show = iio_scan_el_dir_attr_show,
+	.store = iio_scan_el_dir_attr_store,
+};
+
+static struct kobj_type iio_scan_el_dir_ktype = {
+	.sysfs_ops = &iio_scan_el_dir_sysfs_ops,
+};
+
+/*
+ * This iio_sysfs_{add,del}_attrs() are essentially re-implementations of
+ * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
+ * around the const-pointer mismatch situation with using them.
+ *
+ * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
+ * while these are happy with just 'struct attribute **ptr'
+ */
+static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
+{
+	int err = 0;
+	int i;
+
+	for (i = 0; ptr[i] && !err; i++)
+		err = sysfs_create_file(kobj, ptr[i]);
+	if (err)
+		while (--i >= 0)
+			sysfs_remove_file(kobj, ptr[i]);
+	return err;
+}
+
+static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
+{
+	int i;
+
+	for (i = 0; ptr[i]; i++)
+		sysfs_remove_file(kobj, ptr[i]);
+}
+
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev)
 {
@@ -1282,12 +1375,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
 		       sizeof(struct attribute *) * attrcount);
 
-	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
+	buffer->buffer_attrs = attr;
 
-	buffer->buffer_group.name = "buffer";
-	buffer->buffer_group.attrs = attr;
+	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
+				   &indio_dev->dev.kobj, "buffer");
+	if (ret)
+		goto error_buffer_free_attrs;
 
-	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
+	ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
+	if (ret)
+		goto error_buffer_kobject_put;
 
 	attrcount = 0;
 	INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
@@ -1317,28 +1414,42 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 		}
 	}
 
-	buffer->scan_el_group.name = iio_scan_elements_group_name;
-
-	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
-					      sizeof(buffer->scan_el_group.attrs[0]),
-					      GFP_KERNEL);
-	if (buffer->scan_el_group.attrs == NULL) {
+	buffer->scan_el_attrs = kcalloc(attrcount + 1,
+					sizeof(buffer->scan_el_attrs[0]),
+					GFP_KERNEL);
+	if (buffer->scan_el_attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_free_scan_mask;
 	}
-	attrn = 0;
 
+	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
+				   &indio_dev->dev.kobj, "scan_elements");
+	if (ret)
+		goto error_free_scan_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;
-	indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
+		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
+
+	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
+	if (ret)
+		goto error_scan_kobject_put;
 
 	return 0;
 
+error_scan_kobject_put:
+	kobject_put(&buffer->scan_el_dir);
+error_free_scan_attrs:
+	kfree(buffer->scan_el_attrs);
 error_free_scan_mask:
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
+	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
-	kfree(buffer->buffer_group.attrs);
+error_buffer_kobject_put:
+	kobject_put(&buffer->buffer_dir);
+error_buffer_free_attrs:
+	kfree(buffer->buffer_attrs);
 
 	return ret;
 }
@@ -1366,10 +1477,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 
 static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
 {
+	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
+	kobject_put(&buffer->scan_el_dir);
+	kfree(buffer->scan_el_attrs);
 	bitmap_free(buffer->scan_mask);
-	kfree(buffer->buffer_group.attrs);
-	kfree(buffer->scan_el_group.attrs);
+	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
 	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	kobject_put(&buffer->buffer_dir);
+	kfree(buffer->buffer_attrs);
 }
 
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index ca8b11541477..f389d8feacb0 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1819,18 +1819,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 
 	iio_device_register_debugfs(indio_dev);
 
-	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
-	if (ret) {
-		dev_err(indio_dev->dev.parent,
-			"Failed to create buffer sysfs interfaces\n");
-		goto error_unreg_debugfs;
-	}
-
 	ret = iio_device_register_sysfs(indio_dev);
 	if (ret) {
 		dev_err(indio_dev->dev.parent,
 			"Failed to register sysfs interfaces\n");
-		goto error_buffer_free_sysfs;
+		goto error_unreg_debugfs;
 	}
 	ret = iio_device_register_eventset(indio_dev);
 	if (ret) {
@@ -1859,14 +1852,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
 	if (ret < 0)
 		goto error_unreg_eventset;
 
+	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
+	if (ret) {
+		dev_err(indio_dev->dev.parent,
+			"Failed to create buffer sysfs interfaces\n");
+		goto error_device_del;
+	}
+
 	return 0;
 
+error_device_del:
+	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
 error_unreg_eventset:
 	iio_device_unregister_eventset(indio_dev);
 error_free_sysfs:
 	iio_device_unregister_sysfs(indio_dev);
-error_buffer_free_sysfs:
-	iio_buffer_free_sysfs_and_mask(indio_dev);
 error_unreg_debugfs:
 	iio_device_unregister_debugfs(indio_dev);
 	return ret;
@@ -1882,6 +1882,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_ioctl_handler *h, *t;
 
+	iio_buffer_free_sysfs_and_mask(indio_dev);
+
 	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
 
 	mutex_lock(&indio_dev->info_exist_lock);
@@ -1899,8 +1901,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
 	iio_buffer_wakeup_poll(indio_dev);
 
 	mutex_unlock(&indio_dev->info_exist_lock);
-
-	iio_buffer_free_sysfs_and_mask(indio_dev);
 }
 EXPORT_SYMBOL(iio_device_unregister);
 
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 67d73d465e02..77e169e51434 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -103,14 +103,20 @@ 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;
+	/* @buffer_dir: kobject for the 'buffer' directory of this buffer */
+	struct kobject buffer_dir;
+
+	/* @buffer_attrs: Attributes of the buffer group. */
+	struct attribute **buffer_attrs;
+
+	/* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
+	struct kobject scan_el_dir;
 
 	/*
-	 * @scan_el_group: Attribute group for those attributes not
+	 * @scan_el_attrs: Array of attributes for those attributes not
 	 * created from the iio_chan_info array.
 	 */
-	struct attribute_group scan_el_group;
+	struct attribute **scan_el_attrs;
 
 	/* @attrs: Standard attributes of the buffer. */
 	const struct attribute **attrs;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 9a3cf4815148..2ea185340a3a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -556,7 +556,7 @@ struct iio_dev {
 	struct mutex			info_exist_lock;
 	const struct iio_buffer_setup_ops	*setup_ops;
 	struct cdev			chrdev;
-#define IIO_MAX_GROUPS 6
+#define IIO_MAX_GROUPS 4
 	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
 	int				groupcounter;
 
-- 
2.17.1

[RFC PATCH 04/12] iio: buffer: add index to the first IIO buffer dir and symlink it back

[Alexandru Ardelean]

This change makes it so that the first buffer directory is named 'buffer0'
and moves the 'scan_elements' under it.

For backwards compatibility these folders are symlinked back to the
original folders.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 38 +++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 8b31faf049a5..62c8bd6b67cd 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1346,7 +1346,8 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
 }
 
 static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
-					     struct iio_dev *indio_dev)
+					     struct iio_dev *indio_dev,
+					     unsigned int idx)
 {
 	struct iio_dev_attr *p;
 	struct attribute **attr;
@@ -1378,7 +1379,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	buffer->buffer_attrs = attr;
 
 	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
-				   &indio_dev->dev.kobj, "buffer");
+				   &indio_dev->dev.kobj, "buffer%u", idx);
 	if (ret)
 		goto error_buffer_free_attrs;
 
@@ -1423,7 +1424,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	}
 
 	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
-				   &indio_dev->dev.kobj, "scan_elements");
+				   &buffer->buffer_dir, "scan_elements");
 	if (ret)
 		goto error_free_scan_attrs;
 
@@ -1454,11 +1455,13 @@ 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);
+
 int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 {
 	struct iio_buffer *buffer = indio_dev->buffer;
 	const struct iio_chan_spec *channels;
-	int i;
+	int i, ret;
 
 	channels = indio_dev->channels;
 	if (channels) {
@@ -1472,7 +1475,29 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return 0;
 
-	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
+	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_link(&indio_dev->dev.kobj,
+				&indio_dev->buffer->buffer_dir,
+				"buffer");
+	if (ret)
+		goto error_free_sysfs_and_mask;
+
+	ret = sysfs_create_link(&indio_dev->dev.kobj,
+				&indio_dev->buffer->scan_el_dir,
+				"scan_elements");
+	if (ret)
+		goto error_remove_buffer_dir_link;
+
+	return 0;
+
+error_remove_buffer_dir_link:
+	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+error_free_sysfs_and_mask:
+	__iio_buffer_free_sysfs_and_mask(buffer);
+	return ret;
 }
 
 static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
@@ -1494,6 +1519,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return;
 
+	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
+	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
+
 	__iio_buffer_free_sysfs_and_mask(buffer);
 }
 
-- 
2.17.1

[RFC PATCH 05/12] iio: core: split __iio_device_attr_init() to init only the attr object

[Alexandru Ardelean]

The __iio_device_attr_init() function initializes a device_attribute
object, but mostly it just does a lot of name creation logic.

We will want to re-use this logic for name-creation, so this change
re-purposes the __iio_device_attr_init() to be a __iio_attr_init() function
which just handles the creation of the attribute name.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 43 +++++++++++++--------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f389d8feacb0..e9aa84f5b05a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -973,22 +973,15 @@ static ssize_t iio_write_channel_info(struct device *dev,
 }
 
 static
-int __iio_device_attr_init(struct device_attribute *dev_attr,
-			   const char *postfix,
-			   struct iio_chan_spec const *chan,
-			   ssize_t (*readfunc)(struct device *dev,
-					       struct device_attribute *attr,
-					       char *buf),
-			   ssize_t (*writefunc)(struct device *dev,
-						struct device_attribute *attr,
-						const char *buf,
-						size_t len),
-			   enum iio_shared_by shared_by)
+int iio_attr_init(struct attribute *attr,
+		  const char *postfix,
+		  struct iio_chan_spec const *chan,
+		  enum iio_shared_by shared_by)
 {
 	int ret = 0;
 	char *name = NULL;
 	char *full_postfix;
-	sysfs_attr_init(&dev_attr->attr);
+	sysfs_attr_init(attr);
 
 	/* Build up postfix of <extend_name>_<modifier>_postfix */
 	if (chan->modified && (shared_by == IIO_SEPARATE)) {
@@ -1084,17 +1077,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 		ret = -ENOMEM;
 		goto error_free_full_postfix;
 	}
-	dev_attr->attr.name = name;
-
-	if (readfunc) {
-		dev_attr->attr.mode |= S_IRUGO;
-		dev_attr->show = readfunc;
-	}
-
-	if (writefunc) {
-		dev_attr->attr.mode |= S_IWUSR;
-		dev_attr->store = writefunc;
-	}
+	attr->name = name;
 
 error_free_full_postfix:
 	kfree(full_postfix);
@@ -1127,9 +1110,7 @@ int __iio_add_chan_devattr(const char *postfix,
 	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
 	if (iio_attr == NULL)
 		return -ENOMEM;
-	ret = __iio_device_attr_init(&iio_attr->dev_attr,
-				     postfix, chan,
-				     readfunc, writefunc, shared_by);
+	ret = iio_attr_init(&iio_attr->dev_attr.attr, postfix, chan, shared_by);
 	if (ret)
 		goto error_iio_dev_attr_free;
 	iio_attr->c = chan;
@@ -1145,6 +1126,16 @@ int __iio_add_chan_devattr(const char *postfix,
 		}
 	list_add(&iio_attr->l, attr_list);
 
+	if (readfunc) {
+		iio_attr->dev_attr.attr.mode |= S_IRUGO;
+		iio_attr->dev_attr.show = readfunc;
+	}
+
+	if (writefunc) {
+		iio_attr->dev_attr.attr.mode |= S_IWUSR;
+		iio_attr->dev_attr.store = writefunc;
+	}
+
 	return 0;
 
 error_device_attr_deinit:
-- 
2.17.1

[RFC PATCH 06/12] iio: buffer: re-route scan_elements via it's kobj_type

[Alexandru Ardelean]

The scan_elements attributes are solely located inside
'industrialio-buffer-sysfs.c'. In order to support more than one buffer per
IIO device, we need to expand scan_elements attributes directly to IIO
buffer object, and not the IIO device.

This also requires that a new 'iio_buffer_attr' type be added which is
mostly a copy of 'iio_dev_attr', but this expands to an IIO buffer object.

The 'iio_dev_attr' type could have been re-used here, but managing 'device'
objects is a bit more tricky (than it looks at first). A 'device' object
needs to be initialized & managed and we only need to the 'kobj' to expand
from the 'bufferX' directory back to an IIO buffer.
kobjects are simpler to manage.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/iio_core.h            |   5 +
 drivers/iio/industrialio-buffer.c | 162 +++++++++++++++++++++++-------
 drivers/iio/industrialio-core.c   |   1 -
 3 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index fced02cadcc3..43d44ec92781 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -31,6 +31,11 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
 				       struct iio_ioctl_handler *h);
 void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
 
+int iio_attr_init(struct attribute *attr,
+		  const char *postfix,
+		  struct iio_chan_spec const *chan,
+		  enum iio_shared_by shared_by);
+
 int __iio_add_chan_devattr(const char *postfix,
 			   struct iio_chan_spec const *chan,
 			   ssize_t (*func)(struct device *dev,
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 62c8bd6b67cd..445709ef245c 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -26,6 +26,26 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 
+/**
+ * struct iio_buf_attr - iio buffer specific attribute
+ * @attr:	underlying attribute
+ * @address:	associated register address
+ * @l:		list head for maintaining list of dynamically created attrs
+ * @c:		specification for the underlying channel
+ * @show:	sysfs show hook for this attribute
+ * @store:	sysfs store hook for this attribute
+ */
+struct iio_buf_attr {
+	struct attribute attr;
+	u64 address;
+	struct list_head l;
+	struct iio_chan_spec const *c;
+	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			char *buf);
+	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			 const char *buf, size_t count);
+};
+
 static const char * const iio_endian_prefix[] = {
 	[IIO_BE] = "be",
 	[IIO_LE] = "le",
@@ -210,18 +230,17 @@ void iio_buffer_init(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL(iio_buffer_init);
 
-static ssize_t iio_show_scan_index(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_show_scan_index(struct iio_buffer *buffer,
+				   struct iio_buf_attr *attr,
 				   char *buf)
 {
-	return sprintf(buf, "%u\n", to_iio_dev_attr(attr)->c->scan_index);
+	return sprintf(buf, "%u\n", attr->c->scan_index);
 }
 
-static ssize_t iio_show_fixed_type(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_show_fixed_type(struct iio_buffer *buffer,
+				   struct iio_buf_attr *this_attr,
 				   char *buf)
 {
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 	u8 type = this_attr->c->scan_type.endianness;
 
 	if (type == IIO_CPU) {
@@ -248,17 +267,14 @@ static ssize_t iio_show_fixed_type(struct device *dev,
 		       this_attr->c->scan_type.shift);
 }
 
-static ssize_t iio_scan_el_show(struct device *dev,
-				struct device_attribute *attr,
+static ssize_t iio_scan_el_show(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
 				char *buf)
 {
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
 
 	/* Ensure ret is 0 or 1. */
-	ret = !!test_bit(to_iio_dev_attr(attr)->address,
-		       buffer->scan_mask);
+	ret = !!test_bit(attr->address, buffer->scan_mask);
 
 	return sprintf(buf, "%d\n", ret);
 }
@@ -359,16 +375,14 @@ static int iio_scan_mask_query(struct iio_dev *indio_dev,
 	return !!test_bit(bit, buffer->scan_mask);
 };
 
-static ssize_t iio_scan_el_store(struct device *dev,
-				 struct device_attribute *attr,
+static ssize_t iio_scan_el_store(struct iio_buffer *buffer,
+				 struct iio_buf_attr *this_attr,
 				 const char *buf,
 				 size_t len)
 {
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	int ret;
 	bool state;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
 
 	ret = strtobool(buf, &state);
 	if (ret < 0)
@@ -398,24 +412,20 @@ static ssize_t iio_scan_el_store(struct device *dev,
 
 }
 
-static ssize_t iio_scan_el_ts_show(struct device *dev,
-				   struct device_attribute *attr,
+static ssize_t iio_scan_el_ts_show(struct iio_buffer *buffer,
+				   struct iio_buf_attr *attr,
 				   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", buffer->scan_timestamp);
 }
 
-static ssize_t iio_scan_el_ts_store(struct device *dev,
-				    struct device_attribute *attr,
+static ssize_t iio_scan_el_ts_store(struct iio_buffer *buffer,
+				    struct iio_buf_attr *attr,
 				    const char *buf,
 				    size_t len)
 {
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	int ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
 	bool state;
 
 	ret = strtobool(buf, &state);
@@ -434,13 +444,88 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
 	return ret ? ret : len;
 }
 
+static int __iio_add_chan_bufattr(const char *postfix,
+				  struct iio_chan_spec const *chan,
+				  ssize_t (*readfunc)(struct iio_buffer *buffer,
+						      struct iio_buf_attr *attr,
+						      char *buf),
+				  ssize_t (*writefunc)(struct iio_buffer *buffer,
+						       struct iio_buf_attr *attr,
+						       const char *buf,
+						       size_t len),
+				  u64 mask,
+				  enum iio_shared_by shared_by,
+				  struct device *dev,
+				  struct list_head *attr_list)
+{
+	struct iio_buf_attr *iio_attr, *t;
+	int ret;
+
+	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
+	if (iio_attr == NULL)
+		return -ENOMEM;
+
+	ret = iio_attr_init(&iio_attr->attr, postfix, chan, shared_by);
+	if (ret)
+		goto error_iio_buf_attr_free;
+
+	iio_attr->c = chan;
+	iio_attr->address = mask;
+	list_for_each_entry(t, attr_list, l) {
+		if (strcmp(t->attr.name, iio_attr->attr.name) == 0) {
+			if (shared_by == IIO_SEPARATE)
+				dev_err(dev, "tried to double register : %s\n",
+					t->attr.name);
+			ret = -EBUSY;
+			goto error_iio_buf_attr_deinit;
+		}
+	}
+	list_add(&iio_attr->l, attr_list);
+
+	if (readfunc) {
+		iio_attr->attr.mode |= S_IRUGO;
+		iio_attr->show = readfunc;
+	}
+
+	if (writefunc) {
+		iio_attr->attr.mode |= S_IWUSR;
+		iio_attr->store = writefunc;
+	}
+
+	return 0;
+
+error_iio_buf_attr_deinit:
+	kfree(iio_attr->attr.name);
+error_iio_buf_attr_free:
+	kfree(iio_attr);
+	return ret;
+}
+
+/**
+ * iio_free_chan_bufattr_list() - Free a list of IIO buffer attributes
+ * @attr_list: List of IIO buffer attributes
+ *
+ * This function frees the memory allocated for each of the IIO buffer
+ * attributes in the list.
+ */
+static void iio_free_chan_bufattr_list(struct list_head *attr_list)
+{
+	struct iio_buf_attr *p, *n;
+
+	list_for_each_entry_safe(p, n, attr_list, l) {
+		kfree(p->attr.name);
+		list_del(&p->l);
+		kfree(p);
+	}
+}
+
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					struct iio_buffer *buffer,
 					const struct iio_chan_spec *chan)
 {
 	int ret, attrcount = 0;
 
-	ret = __iio_add_chan_devattr("index",
+	ret = __iio_add_chan_bufattr("index",
 				     chan,
 				     &iio_show_scan_index,
 				     NULL,
@@ -451,7 +536,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	if (ret)
 		return ret;
 	attrcount++;
-	ret = __iio_add_chan_devattr("type",
+	ret = __iio_add_chan_bufattr("type",
 				     chan,
 				     &iio_show_fixed_type,
 				     NULL,
@@ -463,7 +548,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 		return ret;
 	attrcount++;
 	if (chan->type != IIO_TIMESTAMP)
-		ret = __iio_add_chan_devattr("en",
+		ret = __iio_add_chan_bufattr("en",
 					     chan,
 					     &iio_scan_el_show,
 					     &iio_scan_el_store,
@@ -472,7 +557,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 					     &indio_dev->dev,
 					     &buffer->scan_el_dev_attr_list);
 	else
-		ret = __iio_add_chan_devattr("en",
+		ret = __iio_add_chan_bufattr("en",
 					     chan,
 					     &iio_scan_el_ts_show,
 					     &iio_scan_el_ts_store,
@@ -1251,6 +1336,7 @@ static struct attribute *iio_buffer_attrs[] = {
 };
 
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
+#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
 
 static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 					struct attribute *attr,
@@ -1291,9 +1377,9 @@ static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
 					 char *buf)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
-	struct device_attribute *dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+	return battr->show(buffer, battr, buf);
 }
 
 static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
@@ -1302,9 +1388,9 @@ static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
 					  size_t len)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
-	struct device_attribute *dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+	return battr->store(buffer, battr, buf, len);
 }
 
 static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
@@ -1349,7 +1435,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 					     struct iio_dev *indio_dev,
 					     unsigned int idx)
 {
-	struct iio_dev_attr *p;
+	struct iio_buf_attr *p;
 	struct attribute **attr;
 	int ret, i, attrn, attrcount;
 	const struct iio_chan_spec *channels;
@@ -1430,7 +1516,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 
 	attrn = 0;
 	list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
-		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
+		buffer->scan_el_attrs[attrn++] = &p->attr;
 
 	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
 	if (ret)
@@ -1446,7 +1532,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 	bitmap_free(buffer->scan_mask);
 error_cleanup_dynamic:
 	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
-	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
 error_buffer_kobject_put:
 	kobject_put(&buffer->buffer_dir);
 error_buffer_free_attrs:
@@ -1507,7 +1593,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
 	kfree(buffer->scan_el_attrs);
 	bitmap_free(buffer->scan_mask);
 	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
-	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
+	iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
 	kobject_put(&buffer->buffer_dir);
 	kfree(buffer->buffer_attrs);
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e9aa84f5b05a..28830e87e8cb 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -972,7 +972,6 @@ static ssize_t iio_write_channel_info(struct device *dev,
 	return len;
 }
 
-static
 int iio_attr_init(struct attribute *attr,
 		  const char *postfix,
 		  struct iio_chan_spec const *chan,
-- 
2.17.1

[RFC PATCH 07/12] iio: buffer: re-route core buffer attributes via it's new kobj_type

[Alexandru Ardelean]

For the buffer attributes that are present inside the IIO core buffer logic
we can re-route them to expand the attribute into iio_buffer objects.

The rest, will still expand to device_attributes.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 113 +++++++++++++++++-------------
 1 file changed, 64 insertions(+), 49 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 445709ef245c..8b4b7baf21da 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -572,22 +572,18 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static ssize_t iio_buffer_read_length(struct device *dev,
-				      struct device_attribute *attr,
+static ssize_t iio_buffer_read_length(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr,
 				      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", buffer->length);
 }
 
-static ssize_t iio_buffer_write_length(struct device *dev,
-				       struct device_attribute *attr,
+static ssize_t iio_buffer_write_length(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr,
 				       const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	unsigned int val;
 	int ret;
 
@@ -615,13 +611,10 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t iio_buffer_show_enable(struct device *dev,
-				      struct device_attribute *attr,
+static ssize_t iio_buffer_show_enable(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr,
 				      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%d\n", iio_buffer_is_active(buffer));
 }
 
@@ -1227,15 +1220,14 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 	iio_buffer_deactivate_all(indio_dev);
 }
 
-static ssize_t iio_buffer_store_enable(struct device *dev,
-				       struct device_attribute *attr,
+static ssize_t iio_buffer_store_enable(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr,
 				       const char *buf,
 				       size_t len)
 {
 	int ret;
 	bool requested_state;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	bool inlist;
 
 	ret = strtobool(buf, &requested_state);
@@ -1260,23 +1252,19 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 	return (ret < 0) ? ret : len;
 }
 
-static ssize_t iio_buffer_show_watermark(struct device *dev,
-					 struct device_attribute *attr,
+static ssize_t iio_buffer_show_watermark(struct iio_buffer *buffer,
+					 struct iio_buf_attr *attr,
 					 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%u\n", buffer->watermark);
 }
 
-static ssize_t iio_buffer_store_watermark(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t iio_buffer_store_watermark(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  const char *buf,
 					  size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
+	struct iio_dev *indio_dev = buffer->indio_dev;
 	unsigned int val;
 	int ret;
 
@@ -1305,36 +1293,51 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t iio_dma_show_data_available(struct device *dev,
-						struct device_attribute *attr,
-						char *buf)
+static ssize_t iio_dma_show_data_available(struct iio_buffer *buffer,
+					   struct iio_buf_attr *attr,
+					   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct iio_buffer *buffer = indio_dev->buffer;
-
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
-static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
-		   iio_buffer_write_length);
-static struct device_attribute dev_attr_length_ro = __ATTR(length,
+#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
+	struct iio_buf_attr iio_buf_attr_##_name =	\
+		__ATTR(_name, _mode, _show, _store)
+
+static IIO_BUF_ATTR(length, S_IRUGO | S_IWUSR,
+		    iio_buffer_read_length, iio_buffer_write_length);
+static struct iio_buf_attr buf_attr_length_ro = __ATTR(length,
 	S_IRUGO, iio_buffer_read_length, NULL);
-static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_enable, iio_buffer_store_enable);
-static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_watermark, iio_buffer_store_watermark);
-static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
+static IIO_BUF_ATTR(enable, S_IRUGO | S_IWUSR,
+		    iio_buffer_show_enable, iio_buffer_store_enable);
+static IIO_BUF_ATTR(watermark, S_IRUGO | S_IWUSR,
+		    iio_buffer_show_watermark, iio_buffer_store_watermark);
+static struct iio_buf_attr buf_attr_watermark_ro = __ATTR(watermark,
 	S_IRUGO, iio_buffer_show_watermark, NULL);
-static DEVICE_ATTR(data_available, S_IRUGO,
-		iio_dma_show_data_available, NULL);
+static IIO_BUF_ATTR(data_available, S_IRUGO,
+		    iio_dma_show_data_available, NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
-	&dev_attr_length.attr,
-	&dev_attr_enable.attr,
-	&dev_attr_watermark.attr,
-	&dev_attr_data_available.attr,
+	&iio_buf_attr_length.attr,
+	&iio_buf_attr_enable.attr,
+	&iio_buf_attr_watermark.attr,
+	&iio_buf_attr_data_available.attr,
 };
 
+static bool iio_buffer_attr_is_core(struct attribute *attr)
+{
+	struct attribute *a;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(iio_buffer_attrs); i++) {
+		a = iio_buffer_attrs[i];
+		if (!strcmp(attr->name, a->name))
+			return true;
+	}
+
+	return false;
+}
+
 #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
 #define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
 
@@ -1344,6 +1347,12 @@ static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
 	struct device_attribute *dattr;
+	struct iio_buf_attr *battr;
+
+	if (iio_buffer_attr_is_core(attr)) {
+		battr = to_iio_buf_attr(attr);
+		return battr->show(buffer, battr, buf);
+	}
 
 	dattr = to_dev_attr(attr);
 
@@ -1357,6 +1366,12 @@ static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
 	struct device_attribute *dattr;
+	struct iio_buf_attr *battr;
+
+	if (iio_buffer_attr_is_core(attr)) {
+		battr = to_iio_buf_attr(attr);
+		return battr->store(buffer, battr, buf, len);
+	}
 
 	dattr = to_dev_attr(attr);
 
@@ -1453,10 +1468,10 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
 
 	memcpy(attr, iio_buffer_attrs, sizeof(iio_buffer_attrs));
 	if (!buffer->access->set_length)
-		attr[0] = &dev_attr_length_ro.attr;
+		attr[0] = &buf_attr_length_ro.attr;
 
 	if (buffer->access->flags & INDIO_BUFFER_FLAG_FIXED_WATERMARK)
-		attr[2] = &dev_attr_watermark_ro.attr;
+		attr[2] = &buf_attr_watermark_ro.attr;
 
 	if (buffer->attrs)
 		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
-- 
2.17.1

[RFC PATCH 08/12] iio: buffer: add helper to get the IIO device to which a buffer belongs

[Alexandru Ardelean]

This is useful for drivers that may have a reference to an IIO buffer, to
be able to get a reference back to the IIO device.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 14 ++++++++++++++
 include/linux/iio/buffer.h        |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 8b4b7baf21da..342e5432f7d9 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1736,6 +1736,20 @@ void iio_buffer_put(struct iio_buffer *buffer)
 }
 EXPORT_SYMBOL_GPL(iio_buffer_put);
 
+/**
+ * iio_buffer_get_iio_dev - Get the IIO device to which this buffer belongs to
+ * @buffer: The buffer for which to retrieve the IIO device
+ *
+ * This function retrieves the IIO device to which this IIO buffer is attached
+ * to. Given that an IIO device may have multiple IIO buffers, it's useful
+ * for some drivers to obtain a reference back to the IIO device.
+ */
+struct iio_dev *iio_buffer_get_iio_dev(struct iio_buffer *buffer)
+{
+	return buffer->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
+
 /**
  * iio_device_attach_buffer - Attach a buffer to a IIO device
  * @indio_dev: The device the buffer should be attached to
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 8febc23f5f26..b27d8c81f32c 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -11,6 +11,8 @@
 
 struct iio_buffer;
 
+struct iio_dev *iio_buffer_get_iio_dev(struct iio_buffer *buffer);
+
 int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data);
 
 /**
-- 
2.17.1

[RFC PATCH 09/12] iio: re-route all buffer attributes through new buffer kobj_type

[Alexandru Ardelean]

Now that the iio_buffer_set_attrs() has been removed, we can be sure that
no accidents can happen with drivers that try to provide extra buffer
attributes that expand to iio_dev objects.

So, we can convert all remaining buffer attributes to expand to iio_buffer
objects.
These will look a bit weird at first, as most of them will just pass back
their reference to the IIO device.
But this can also allow for newer (maybe more interesting) uses.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adxl372.c                   | 36 ++++-----
 drivers/iio/accel/bmc150-accel-core.c         | 34 ++++-----
 drivers/iio/adc/at91-sama5d2_adc.c            | 30 ++++----
 .../buffer/industrialio-buffer-dmaengine.c    | 13 ++--
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 30 ++++----
 .../common/hid-sensors/hid-sensor-trigger.c   | 32 ++++----
 drivers/iio/industrialio-buffer.c             | 73 +++----------------
 include/linux/iio/sysfs.h                     | 50 +++++++++++++
 8 files changed, 148 insertions(+), 150 deletions(-)

diff --git a/drivers/iio/accel/adxl372.c b/drivers/iio/accel/adxl372.c
index 8ba1453b8dbf..a90aaa518816 100644
--- a/drivers/iio/accel/adxl372.c
+++ b/drivers/iio/accel/adxl372.c
@@ -978,39 +978,39 @@ static ssize_t adxl372_show_filter_freq_avail(struct device *dev,
 	return len;
 }
 
-static ssize_t adxl372_get_fifo_enabled(struct device *dev,
-					  struct device_attribute *attr,
-					  char *buf)
+static ssize_t adxl372_get_fifo_enabled(struct iio_buffer *buffer,
+					struct iio_buf_attr *attr,
+					char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct adxl372_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->fifo_mode);
 }
 
-static ssize_t adxl372_get_fifo_watermark(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t adxl372_get_fifo_watermark(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct adxl372_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->watermark);
 }
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(ADXL372_FIFO_SIZE));
-static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
-		       adxl372_get_fifo_watermark, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       adxl372_get_fifo_enabled, NULL, 0);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max,
+			  __stringify(ADXL372_FIFO_SIZE));
+static IIO_BUF_ATTR(hwfifo_watermark, 0444,
+		    adxl372_get_fifo_watermark, NULL);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444,
+		    adxl372_get_fifo_enabled, NULL);
 
 static const struct attribute *adxl372_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index c641ee552038..bddda8f28fb8 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -767,11 +767,11 @@ static int bmc150_accel_validate_trigger(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
-					       struct device_attribute *attr,
+static ssize_t bmc150_accel_get_fifo_watermark(struct iio_buffer *buffer,
+					       struct iio_buf_attr *attr,
 					       char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int wm;
 
@@ -782,11 +782,11 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
 	return sprintf(buf, "%d\n", wm);
 }
 
-static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
-					   struct device_attribute *attr,
+static ssize_t bmc150_accel_get_fifo_state(struct iio_buffer *buffer,
+					   struct iio_buf_attr *attr,
 					   char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	bool state;
 
@@ -811,19 +811,19 @@ static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = {
 	{ }
 };
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
-static IIO_CONST_ATTR(hwfifo_watermark_max,
-		      __stringify(BMC150_ACCEL_FIFO_LENGTH));
-static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
-		       bmc150_accel_get_fifo_state, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
-		       bmc150_accel_get_fifo_watermark, NULL, 0);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max,
+			  __stringify(BMC150_ACCEL_FIFO_LENGTH));
+static IIO_BUF_ATTR(hwfifo_enabled, S_IRUGO,
+		    bmc150_accel_get_fifo_state, NULL);
+static IIO_BUF_ATTR(hwfifo_watermark, S_IRUGO,
+		    bmc150_accel_get_fifo_watermark, NULL);
 
 static const struct attribute *bmc150_accel_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 6edcc99009d1..f86ee1b0b051 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1612,31 +1612,29 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
 	at91_adc_config_emr(st);
 }
 
-static ssize_t at91_adc_get_fifo_state(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t at91_adc_get_fifo_state(struct iio_buffer *buffer,
+				       struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
 }
 
-static ssize_t at91_adc_get_watermark(struct device *dev,
-				      struct device_attribute *attr, char *buf)
+static ssize_t at91_adc_get_watermark(struct iio_buffer *buffer,
+				      struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct at91_adc_state *st = iio_priv(indio_dev);
 
 	return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       at91_adc_get_fifo_state, NULL, 0);
-static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
-		       at91_adc_get_watermark, NULL, 0);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444, at91_adc_get_fifo_state, NULL);
+static IIO_BUF_ATTR(hwfifo_watermark, 0444, at91_adc_get_watermark, NULL);
 
-static IIO_CONST_ATTR(hwfifo_watermark_min, "2");
-static IIO_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_min, "2");
+static IIO_BUF_CONST_ATTR(hwfifo_watermark_max, AT91_HWFIFO_MAX_SIZE_STR);
 
 static IIO_CONST_ATTR(oversampling_ratio_available,
 		      __stringify(AT91_OSR_1SAMPLES) " "
@@ -1653,10 +1651,10 @@ static const struct attribute_group at91_adc_attribute_group = {
 };
 
 static const struct attribute *at91_adc_fifo_attributes[] = {
-	&iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
-	&iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_min.buf_attr.attr,
+	&iio_buf_const_attr_hwfifo_watermark_max.buf_attr.attr,
+	&iio_buf_attr_hwfifo_watermark.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/buffer/industrialio-buffer-dmaengine.c b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
index b0cb9a35f5cd..2cd5fd3fe191 100644
--- a/drivers/iio/buffer/industrialio-buffer-dmaengine.c
+++ b/drivers/iio/buffer/industrialio-buffer-dmaengine.c
@@ -129,21 +129,20 @@ static const struct iio_dma_buffer_ops iio_dmaengine_default_ops = {
 	.abort = iio_dmaengine_buffer_abort,
 };
 
-static ssize_t iio_dmaengine_buffer_get_length_align(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static ssize_t iio_dmaengine_buffer_get_length_align(struct iio_buffer *buffer,
+	struct iio_buf_attr *attr, char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct dmaengine_buffer *dmaengine_buffer =
-		iio_buffer_to_dmaengine_buffer(indio_dev->buffer);
+		iio_buffer_to_dmaengine_buffer(buffer);
 
 	return sprintf(buf, "%zu\n", dmaengine_buffer->align);
 }
 
-static IIO_DEVICE_ATTR(length_align_bytes, 0444,
-		       iio_dmaengine_buffer_get_length_align, NULL, 0);
+static IIO_BUF_ATTR(length_align_bytes, 0444,
+		    iio_dmaengine_buffer_get_length_align, NULL);
 
 static const struct attribute *iio_dmaengine_buffer_attrs[] = {
-	&iio_dev_attr_length_align_bytes.dev_attr.attr,
+	&iio_buf_attr_length_align_bytes.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index 1eafcf04ad69..5c6c4e6fec9b 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -116,11 +116,11 @@ static int cros_ec_sensor_set_ec_rate(struct cros_ec_sensors_core_state *st,
 	return ret;
 }
 
-static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
-						 struct device_attribute *attr,
+static ssize_t cros_ec_sensor_set_report_latency(struct iio_buffer *buffer,
+						 struct iio_buf_attr *attr,
 						 const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int integer, fract, ret;
 	int latency;
@@ -138,11 +138,11 @@ static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
 	return len;
 }
 
-static ssize_t cros_ec_sensor_get_report_latency(struct device *dev,
-						 struct device_attribute *attr,
+static ssize_t cros_ec_sensor_get_report_latency(struct iio_buffer *buffer,
+						 struct iio_buf_attr *attr,
 						 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 	int latency, ret;
 
@@ -161,25 +161,25 @@ static ssize_t cros_ec_sensor_get_report_latency(struct device *dev,
 		       (latency % 1000) * 1000);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
-		       cros_ec_sensor_get_report_latency,
-		       cros_ec_sensor_set_report_latency, 0);
+static IIO_BUF_ATTR(hwfifo_timeout, 0644,
+		    cros_ec_sensor_get_report_latency,
+		    cros_ec_sensor_set_report_latency);
 
-static ssize_t hwfifo_watermark_max_show(struct device *dev,
-					 struct device_attribute *attr,
+static ssize_t hwfifo_watermark_max_show(struct iio_buffer *buffer,
+					 struct iio_buf_attr *attr,
 					 char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
 
 	return sprintf(buf, "%d\n", st->fifo_max_event_count);
 }
 
-static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
+static IIO_BUF_ATTR(hwfifo_watermark_max, 0444, hwfifo_watermark_max_show, NULL);
 
 static const struct attribute *cros_ec_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
+	&iio_buf_attr_hwfifo_timeout.attr,
+	&iio_buf_attr_hwfifo_watermark_max.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index 064c32bec9c7..c04dca7a457b 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -19,11 +19,11 @@
 #include <linux/iio/sysfs.h>
 #include "hid-sensor-trigger.h"
 
-static ssize_t _hid_sensor_set_report_latency(struct device *dev,
-					      struct device_attribute *attr,
+static ssize_t _hid_sensor_set_report_latency(struct iio_buffer *buffer,
+					      struct iio_buf_attr *attr,
 					      const char *buf, size_t len)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int integer, fract, ret;
 	int latency;
@@ -42,11 +42,11 @@ static ssize_t _hid_sensor_set_report_latency(struct device *dev,
 	return len;
 }
 
-static ssize_t _hid_sensor_get_report_latency(struct device *dev,
-					      struct device_attribute *attr,
+static ssize_t _hid_sensor_get_report_latency(struct iio_buffer *buffer,
+					      struct iio_buf_attr *attr,
 					      char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int latency;
 
@@ -57,11 +57,11 @@ static ssize_t _hid_sensor_get_report_latency(struct device *dev,
 	return sprintf(buf, "%d.%06u\n", latency / 1000, (latency % 1000) * 1000);
 }
 
-static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
-					  struct device_attribute *attr,
+static ssize_t _hid_sensor_get_fifo_state(struct iio_buffer *buffer,
+					  struct iio_buf_attr *attr,
 					  char *buf)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct iio_dev *indio_dev = iio_buffer_get_iio_dev(buffer);
 	struct hid_sensor_common *attrb = iio_device_get_drvdata(indio_dev);
 	int latency;
 
@@ -72,15 +72,15 @@ static ssize_t _hid_sensor_get_fifo_state(struct device *dev,
 	return sprintf(buf, "%d\n", !!latency);
 }
 
-static IIO_DEVICE_ATTR(hwfifo_timeout, 0644,
-		       _hid_sensor_get_report_latency,
-		       _hid_sensor_set_report_latency, 0);
-static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
-		       _hid_sensor_get_fifo_state, NULL, 0);
+static IIO_BUF_ATTR(hwfifo_timeout, 0644,
+		    _hid_sensor_get_report_latency,
+		    _hid_sensor_set_report_latency);
+static IIO_BUF_ATTR(hwfifo_enabled, 0444,
+		    _hid_sensor_get_fifo_state, NULL);
 
 static const struct attribute *hid_sensor_fifo_attributes[] = {
-	&iio_dev_attr_hwfifo_timeout.dev_attr.attr,
-	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	&iio_buf_attr_hwfifo_timeout.attr,
+	&iio_buf_attr_hwfifo_enabled.attr,
 	NULL,
 };
 
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 342e5432f7d9..6307ea84bbc7 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -26,31 +26,19 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/buffer_impl.h>
 
-/**
- * struct iio_buf_attr - iio buffer specific attribute
- * @attr:	underlying attribute
- * @address:	associated register address
- * @l:		list head for maintaining list of dynamically created attrs
- * @c:		specification for the underlying channel
- * @show:	sysfs show hook for this attribute
- * @store:	sysfs store hook for this attribute
- */
-struct iio_buf_attr {
-	struct attribute attr;
-	u64 address;
-	struct list_head l;
-	struct iio_chan_spec const *c;
-	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
-			char *buf);
-	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
-			 const char *buf, size_t count);
-};
-
 static const char * const iio_endian_prefix[] = {
 	[IIO_BE] = "be",
 	[IIO_LE] = "le",
 };
 
+ssize_t iio_read_buf_const_attr(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
+				char *buf)
+{
+	return sprintf(buf, "%s\n", to_iio_buf_const_attr(attr)->string);
+}
+EXPORT_SYMBOL(iio_read_buf_const_attr);
+
 static bool iio_buffer_is_active(struct iio_buffer *buf)
 {
 	return !list_empty(&buf->buffer_list);
@@ -1300,10 +1288,6 @@ static ssize_t iio_dma_show_data_available(struct iio_buffer *buffer,
 	return sprintf(buf, "%zu\n", iio_buffer_data_available(buffer));
 }
 
-#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
-	struct iio_buf_attr iio_buf_attr_##_name =	\
-		__ATTR(_name, _mode, _show, _store)
-
 static IIO_BUF_ATTR(length, S_IRUGO | S_IWUSR,
 		    iio_buffer_read_length, iio_buffer_write_length);
 static struct iio_buf_attr buf_attr_length_ro = __ATTR(length,
@@ -1324,39 +1308,14 @@ static struct attribute *iio_buffer_attrs[] = {
 	&iio_buf_attr_data_available.attr,
 };
 
-static bool iio_buffer_attr_is_core(struct attribute *attr)
-{
-	struct attribute *a;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(iio_buffer_attrs); i++) {
-		a = iio_buffer_attrs[i];
-		if (!strcmp(attr->name, a->name))
-			return true;
-	}
-
-	return false;
-}
-
-#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
-#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
-
 static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
 					struct attribute *attr,
 					char *buf)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
-	struct device_attribute *dattr;
-	struct iio_buf_attr *battr;
-
-	if (iio_buffer_attr_is_core(attr)) {
-		battr = to_iio_buf_attr(attr);
-		return battr->show(buffer, battr, buf);
-	}
-
-	dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
+	return battr->show(buffer, battr, buf);
 }
 
 static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
@@ -1365,17 +1324,9 @@ static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
 					 size_t len)
 {
 	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
-	struct device_attribute *dattr;
-	struct iio_buf_attr *battr;
-
-	if (iio_buffer_attr_is_core(attr)) {
-		battr = to_iio_buf_attr(attr);
-		return battr->store(buffer, battr, buf, len);
-	}
-
-	dattr = to_dev_attr(attr);
+	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
 
-	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
+	return battr->store(buffer, battr, buf, len);
 }
 
 static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index b532c875bc24..d25b47971c09 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;
 
 /**
@@ -28,6 +29,55 @@ struct iio_dev_attr {
 #define to_iio_dev_attr(_dev_attr)				\
 	container_of(_dev_attr, struct iio_dev_attr, dev_attr)
 
+/**
+ * struct iio_buf_attr - iio buffer specific attribute
+ * @attr:	underlying attribute
+ * @address:	associated register address
+ * @l:		list head for maintaining list of dynamically created attrs
+ * @c:		specification for the underlying channel
+ * @show:	sysfs show hook for this attribute
+ * @store:	sysfs store hook for this attribute
+ */
+struct iio_buf_attr {
+	struct attribute attr;
+	u64 address;
+	struct list_head l;
+	struct iio_chan_spec const *c;
+	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			char *buf);
+	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
+			 const char *buf, size_t count);
+};
+
+/**
+ * struct iio_buf_const_attr - constant buffer specific attribute
+ *                         often used for things constant parameters of buffers
+ * @string:	attribute string
+ * @buf_attr:	underlying buffer attribute
+ */
+struct iio_buf_const_attr {
+	const char *string;
+	struct iio_buf_attr buf_attr;
+};
+
+#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
+
+#define IIO_BUF_ATTR(_name, _mode, _show, _store)	\
+	struct iio_buf_attr iio_buf_attr_##_name =	\
+		__ATTR(_name, _mode, _show, _store)
+
+#define IIO_BUF_CONST_ATTR(_name, _string)				\
+	struct iio_buf_const_attr iio_buf_const_attr_##_name		\
+	= { .string = _string,						\
+	    .buf_attr = __ATTR(_name, 0444, iio_read_buf_const_attr, NULL)}
+
+#define to_iio_buf_const_attr(_buf_attr) \
+	container_of(_buf_attr, struct iio_buf_const_attr, buf_attr)
+
+ssize_t iio_read_buf_const_attr(struct iio_buffer *buffer,
+				struct iio_buf_attr *attr,
+				char *len);
+
 ssize_t iio_read_const_attr(struct device *dev,
 			    struct device_attribute *attr,
 			    char *len);
-- 
2.17.1

[RFC PATCH 10/12] iio: core: wrap iio device & buffer into struct for character devices

[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            |  6 ++++++
 drivers/iio/industrialio-buffer.c | 10 ++++++----
 drivers/iio/industrialio-core.c   | 18 ++++++++++++++++--
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index 43d44ec92781..ad6716fe1a93 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h
@@ -12,11 +12,17 @@
 #include <linux/kernel.h>
 #include <linux/device.h>
 
+struct iio_buffer;
 struct iio_chan_spec;
 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 6307ea84bbc7..c83cec89eddf 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -112,8 +112,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;
@@ -178,8 +179,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 28830e87e8cb..9e7a76723f00 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1668,13 +1668,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;
 }
@@ -1688,10 +1699,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;
 }
@@ -1711,7 +1724,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

[RFC PATCH 11/12] iio: buffer: introduce support for attaching more IIO buffers

[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.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 58 +++++++++++++++++++++++++------
 include/linux/iio/buffer_impl.h   |  3 ++
 include/linux/iio/iio-opaque.h    |  4 +++
 3 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index c83cec89eddf..daa68822cea7 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1513,6 +1513,7 @@ 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_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = indio_dev->buffer;
 	const struct iio_chan_spec *channels;
 	int i, ret;
@@ -1529,15 +1530,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return 0;
 
-	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
-	if (ret)
-		return ret;
+	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)
+			goto error_unwind_sysfs_and_mask;
+	}
 
 	ret = sysfs_create_link(&indio_dev->dev.kobj,
 				&indio_dev->buffer->buffer_dir,
 				"buffer");
 	if (ret)
-		goto error_free_sysfs_and_mask;
+		goto error_unwind_sysfs_and_mask;
 
 	ret = sysfs_create_link(&indio_dev->dev.kobj,
 				&indio_dev->buffer->scan_el_dir,
@@ -1549,8 +1553,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 
 error_remove_buffer_dir_link:
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
-error_free_sysfs_and_mask:
-	__iio_buffer_free_sysfs_and_mask(buffer);
+	i = iio_dev_opaque->attached_buffers_cnt - 1;
+error_unwind_sysfs_and_mask:
+	for (; i >= 0; i--) {
+		buffer = iio_dev_opaque->attached_buffers[i];
+		__iio_buffer_free_sysfs_and_mask(buffer);
+	}
+	kfree(iio_dev_opaque->attached_buffers);
+	iio_dev_opaque->attached_buffers = NULL;
 	return ret;
 }
 
@@ -1568,7 +1578,9 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
 
 void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer = indio_dev->buffer;
+	int i;
 
 	if (!buffer)
 		return;
@@ -1576,7 +1588,13 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 
-	__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);
+	iio_dev_opaque->attached_buffers = NULL;
 }
 
 /**
@@ -1709,14 +1727,32 @@ EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
  * @buffer: The buffer to attach to the device
  *
  * 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)
 {
-	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) {
+		kfree(old);
+		return;
+	}
+	iio_dev_opaque->attached_buffers = new;
+
+	/* first buffer is legacy; attach it to the IIO device directly */
+	if (!indio_dev->buffer)
+		indio_dev->buffer = iio_buffer_get(buffer);
+
+	buffer->indio_dev = indio_dev;
 
-	indio_dev->buffer->indio_dev = indio_dev;
+	iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
+	iio_dev_opaque->attached_buffers_cnt = cnt;
 }
 EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index 77e169e51434..e25d26a7f601 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -124,6 +124,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 07c5a8e52ca8..1db0ea09520e 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
@@ -20,6 +22,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

[RFC PATCH 12/12] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Alexandru Ardelean]

With this change, an ioctl() call is added to open a character device for a
buffer.
The ioctl() will not work for the first buffer, as that buffer is already
opened/reserved for the IIO device's character device.
This is also to preserve backwards compatibility.

For any other extra buffer (after the first buffer) this ioctl() will be
required.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 111 ++++++++++++++++++++++++++++++
 drivers/iio/industrialio-core.c   |   8 +++
 include/linux/iio/buffer_impl.h   |   5 ++
 include/linux/iio/iio-opaque.h    |   2 +
 include/uapi/linux/iio/buffer.h   |  16 +++++
 5 files changed, 142 insertions(+)
 create mode 100644 include/uapi/linux/iio/buffer.h

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index daa68822cea7..77f02870cd18 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>
@@ -1399,6 +1400,99 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
 		sysfs_remove_file(kobj, ptr[i]);
 }
 
+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;
+
+	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_outer_addr,
+	.poll = iio_buffer_poll_addr,
+	.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 -ENOENT;
+
+	fd = mutex_lock_interruptible(&indio_dev->mlock);
+	if (fd)
+		return fd;
+
+	buffer = iio_dev_opaque->attached_buffers[idx];
+
+	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
+		fd = -EBUSY;
+		goto error_unlock;
+	}
+
+	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;
+	}
+
+	mutex_unlock(&indio_dev->mlock);
+	return fd;
+
+error_free_ib:
+	kfree(ib);
+error_iio_dev_put:
+	iio_device_put(indio_dev);
+	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+error_unlock:
+	mutex_unlock(&indio_dev->mlock);
+	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,
 					     unsigned int idx)
@@ -1549,8 +1643,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (ret)
 		goto error_remove_buffer_dir_link;
 
+	i = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
+	iio_dev_opaque->buffer_ioctl_handler = kzalloc(i, GFP_KERNEL);
+	if (!iio_dev_opaque->buffer_ioctl_handler) {
+		ret = -ENOMEM;
+		goto error_remove_scan_el_dir;
+	}
+
+	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;
 
+error_remove_scan_el_dir:
+	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 error_remove_buffer_dir_link:
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 	i = iio_dev_opaque->attached_buffers_cnt - 1;
@@ -1585,6 +1692,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
 	if (!buffer)
 		return;
 
+	iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
+	kfree(iio_dev_opaque->buffer_ioctl_handler);
+	iio_dev_opaque->buffer_ioctl_handler = NULL;
+
 	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
 	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
 
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 9e7a76723f00..b4f7dd75bef5 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1685,6 +1685,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
 	ib->indio_dev = indio_dev;
 	ib->buffer = indio_dev->buffer;
 
+	if (indio_dev->buffer)
+		test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
+
 	filp->private_data = ib;
 
 	return 0;
@@ -1702,6 +1705,11 @@ 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);
+	struct iio_buffer *buffer = ib->buffer;
+
+	if (buffer)
+		clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
+
 	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 e25d26a7f601..78da590b5607 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;
 
@@ -75,6 +77,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 1db0ea09520e..d0429b13afa8 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
@@ -24,6 +25,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..3794eca78dad
--- /dev/null
+++ b/include/uapi/linux/iio/buffer.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* industrial I/O buffer definitions needed both in and out of kernel
+ *
+ * Copyright (c) 2020 Alexandru Ardelean
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#ifndef _UAPI_IIO_BUFFER_H_
+#define _UAPI_IIO_BUFFER_H_
+
+#define IIO_BUFFER_GET_FD_IOCTL		_IOWR('i', 0xa0, int)
+
+#endif /* _UAPI_IIO_BUFFER_H_ */
-- 
2.17.1

Re: [RFC PATCH 01/12] iio: core: register chardev only if needed

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:29 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> 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().
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I'll be honest. I have no idea why I didn't do this in first place!

I 'think' we are safe dropping this but I suppose it's possible some
odd code checks for the chrdev presence?

Hopefully not though.

Jonathan
 
> ---
>  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 419d6f8acc13..ca8b11541477 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = {
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> +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;
> @@ -1790,6 +1799,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)
> @@ -1807,9 +1817,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);
> @@ -1838,9 +1845,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)

Re: [RFC PATCH 03/12] iio: buffer: rework buffer & scan_elements dir creation

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:31 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> When adding more than one IIO buffer per IIO device, we will need to create
> a buffer & scan_elements directory for each buffer.
> We also want to move the 'scan_elements' to be a sub-directory of the
> 'buffer' folder.
> 
> The format we want to reach is, for a iio:device0 folder, for 2 buffers
> [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> it's own 'scan_elements' subfolder.
> 
> So, for example:
>    iio:device0/buffer0
>       scan_elements/
> 
>    iio:device0/buffer1
>       scan_elements/
> 
> The other attributes under 'bufferX' would remain unchanged.
> 
> However, we would also need to symlink back to the old 'buffer' &
> 'scan_elements' folders, to keep backwards compatibility.
> 
> Doing all these, require that we maintain the kobjects for each 'bufferX'
> and 'scan_elements' so that we can symlink them back. We also need to
> implement the sysfs_ops for these folders.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hmm. This ended up a bit nasty.  It could do with a few more comments
in the code to make it clear what is going on. 

> ---
>  drivers/iio/industrialio-buffer.c | 151 ++++++++++++++++++++++++++----
>  drivers/iio/industrialio-core.c   |  24 ++---
>  include/linux/iio/buffer_impl.h   |  14 ++-
>  include/linux/iio/iio.h           |   2 +-
>  4 files changed, 156 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 08aa8e0782ce..8b31faf049a5 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,6 +1250,101 @@ static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_data_available.attr,
>  };
>  
> +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> +
> +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> +					struct attribute *attr,
> +					char *buf)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> +	struct device_attribute *dattr;
> +
> +	dattr = to_dev_attr(attr);
> +
> +	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> +}
> +
> +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
> +					 struct attribute *attr,
> +					 const char *buf,
> +					 size_t len)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> +	struct device_attribute *dattr;
> +
> +	dattr = to_dev_attr(attr);
> +
> +	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> +}
> +
> +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
> +	.show = iio_buffer_dir_attr_show,
> +	.store = iio_buffer_dir_attr_store,
> +};
> +
> +static struct kobj_type iio_buffer_dir_ktype = {
> +	.sysfs_ops = &iio_buffer_dir_sysfs_ops,
> +};
> +
> +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> +					 struct attribute *attr,
> +					 char *buf)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> +	struct device_attribute *dattr = to_dev_attr(attr);
> +
> +	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> +}
> +
> +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> +					  struct attribute *attr,
> +					  const char *buf,
> +					  size_t len)
> +{
> +	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> +	struct device_attribute *dattr = to_dev_attr(attr);
> +
> +	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> +}
> +
> +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> +	.show = iio_scan_el_dir_attr_show,
> +	.store = iio_scan_el_dir_attr_store,
> +};
> +
> +static struct kobj_type iio_scan_el_dir_ktype = {
> +	.sysfs_ops = &iio_scan_el_dir_sysfs_ops,
> +};
> +
> +/*
> + * This iio_sysfs_{add,del}_attrs() are essentially re-implementations of
> + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
> + * around the const-pointer mismatch situation with using them.
> + *
> + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
> + * while these are happy with just 'struct attribute **ptr'

Ouch.  This definitely doesn't feel like a great thing to do. 

> + */
> +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
> +{
> +	int err = 0;
> +	int i;
> +
> +	for (i = 0; ptr[i] && !err; i++)
> +		err = sysfs_create_file(kobj, ptr[i]);
> +	if (err)
> +		while (--i >= 0)
> +			sysfs_remove_file(kobj, ptr[i]);
> +	return err;
> +}
> +
> +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> +{
> +	int i;
> +
> +	for (i = 0; ptr[i]; i++)
> +		sysfs_remove_file(kobj, ptr[i]);
> +}
> +
>  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,

Definitely add some docs to this to say why we have this complexity..

>  					     struct iio_dev *indio_dev)
>  {
> @@ -1282,12 +1375,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  		memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
>  		       sizeof(struct attribute *) * attrcount);
>  
> -	attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> +	buffer->buffer_attrs = attr;
>  
> -	buffer->buffer_group.name = "buffer";
> -	buffer->buffer_group.attrs = attr;
> +	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> +				   &indio_dev->dev.kobj, "buffer");
> +	if (ret)
> +		goto error_buffer_free_attrs;
>  
> -	indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
> +	ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> +	if (ret)
> +		goto error_buffer_kobject_put;
>  
>  	attrcount = 0;
>  	INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> @@ -1317,28 +1414,42 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  		}
>  	}
>  
> -	buffer->scan_el_group.name = iio_scan_elements_group_name;
> -
> -	buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
> -					      sizeof(buffer->scan_el_group.attrs[0]),
> -					      GFP_KERNEL);
> -	if (buffer->scan_el_group.attrs == NULL) {
> +	buffer->scan_el_attrs = kcalloc(attrcount + 1,
> +					sizeof(buffer->scan_el_attrs[0]),
> +					GFP_KERNEL);
> +	if (buffer->scan_el_attrs == NULL) {
>  		ret = -ENOMEM;
>  		goto error_free_scan_mask;
>  	}
> -	attrn = 0;
>  
> +	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
> +				   &indio_dev->dev.kobj, "scan_elements");
> +	if (ret)
> +		goto error_free_scan_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;
> -	indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
> +		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> +
> +	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> +	if (ret)
> +		goto error_scan_kobject_put;
>  
>  	return 0;
>  
> +error_scan_kobject_put:
> +	kobject_put(&buffer->scan_el_dir);
> +error_free_scan_attrs:
> +	kfree(buffer->scan_el_attrs);
>  error_free_scan_mask:
>  	bitmap_free(buffer->scan_mask);
>  error_cleanup_dynamic:
> +	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> -	kfree(buffer->buffer_group.attrs);
> +error_buffer_kobject_put:
> +	kobject_put(&buffer->buffer_dir);
> +error_buffer_free_attrs:
> +	kfree(buffer->buffer_attrs);
>  
>  	return ret;
>  }
> @@ -1366,10 +1477,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  
>  static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  {
> +	iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> +	kobject_put(&buffer->scan_el_dir);
> +	kfree(buffer->scan_el_attrs);
>  	bitmap_free(buffer->scan_mask);
> -	kfree(buffer->buffer_group.attrs);
> -	kfree(buffer->scan_el_group.attrs);
> +	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
>  	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> +	kobject_put(&buffer->buffer_dir);
> +	kfree(buffer->buffer_attrs);
>  }
>  
>  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index ca8b11541477..f389d8feacb0 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1819,18 +1819,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  
>  	iio_device_register_debugfs(indio_dev);
>  
> -	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> -	if (ret) {
> -		dev_err(indio_dev->dev.parent,
> -			"Failed to create buffer sysfs interfaces\n");
> -		goto error_unreg_debugfs;
> -	}
> -
>  	ret = iio_device_register_sysfs(indio_dev);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
>  			"Failed to register sysfs interfaces\n");
> -		goto error_buffer_free_sysfs;
> +		goto error_unreg_debugfs;
>  	}
>  	ret = iio_device_register_eventset(indio_dev);
>  	if (ret) {
> @@ -1859,14 +1852,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
>  	if (ret < 0)
>  		goto error_unreg_eventset;
>  
> +	ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);

There are some races around late creation of sysfs files (IIRC) but
I'm not sure what else could be done here.
Looking at device_add it is probably to do with the various notifiers being
called before we have put everything in place.

> +	if (ret) {
> +		dev_err(indio_dev->dev.parent,
> +			"Failed to create buffer sysfs interfaces\n");
> +		goto error_device_del;
> +	}
> +
>  	return 0;
>  
> +error_device_del:
> +	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
>  error_unreg_eventset:
>  	iio_device_unregister_eventset(indio_dev);
>  error_free_sysfs:
>  	iio_device_unregister_sysfs(indio_dev);
> -error_buffer_free_sysfs:
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  error_unreg_debugfs:
>  	iio_device_unregister_debugfs(indio_dev);
>  	return ret;
> @@ -1882,6 +1882,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_ioctl_handler *h, *t;
>  
> +	iio_buffer_free_sysfs_and_mask(indio_dev);
> +
>  	cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
>  
>  	mutex_lock(&indio_dev->info_exist_lock);
> @@ -1899,8 +1901,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
>  	iio_buffer_wakeup_poll(indio_dev);
>  
>  	mutex_unlock(&indio_dev->info_exist_lock);
> -
> -	iio_buffer_free_sysfs_and_mask(indio_dev);
>  }
>  EXPORT_SYMBOL(iio_device_unregister);
>  
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 67d73d465e02..77e169e51434 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -103,14 +103,20 @@ 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;
> +	/* @buffer_dir: kobject for the 'buffer' directory of this buffer */
> +	struct kobject buffer_dir;
> +
> +	/* @buffer_attrs: Attributes of the buffer group. */
> +	struct attribute **buffer_attrs;
> +
> +	/* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
> +	struct kobject scan_el_dir;
>  
>  	/*
> -	 * @scan_el_group: Attribute group for those attributes not
> +	 * @scan_el_attrs: Array of attributes for those attributes not
>  	 * created from the iio_chan_info array.
>  	 */
> -	struct attribute_group scan_el_group;
> +	struct attribute **scan_el_attrs;
>  
>  	/* @attrs: Standard attributes of the buffer. */
>  	const struct attribute **attrs;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 9a3cf4815148..2ea185340a3a 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -556,7 +556,7 @@ struct iio_dev {
>  	struct mutex			info_exist_lock;
>  	const struct iio_buffer_setup_ops	*setup_ops;
>  	struct cdev			chrdev;
> -#define IIO_MAX_GROUPS 6
> +#define IIO_MAX_GROUPS 4
>  	const struct attribute_group	*groups[IIO_MAX_GROUPS + 1];
>  	int				groupcounter;
>  

Re: [RFC PATCH 04/12] iio: buffer: add index to the first IIO buffer dir and symlink it back

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:32 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change makes it so that the first buffer directory is named 'buffer0'
> and moves the 'scan_elements' under it.
> 
> For backwards compatibility these folders are symlinked back to the
> original folders.
Well done on your patch breakdown here.  Makes the actual switch nice
and simple in this patch.

Looks good to me.

Thanks,

Jonathan

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 38 +++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 8b31faf049a5..62c8bd6b67cd 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1346,7 +1346,8 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
>  }
>  
>  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> -					     struct iio_dev *indio_dev)
> +					     struct iio_dev *indio_dev,
> +					     unsigned int idx)
>  {
>  	struct iio_dev_attr *p;
>  	struct attribute **attr;
> @@ -1378,7 +1379,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  	buffer->buffer_attrs = attr;
>  
>  	ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> -				   &indio_dev->dev.kobj, "buffer");
> +				   &indio_dev->dev.kobj, "buffer%u", idx);
>  	if (ret)
>  		goto error_buffer_free_attrs;
>  
> @@ -1423,7 +1424,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  	}
>  
>  	ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
> -				   &indio_dev->dev.kobj, "scan_elements");
> +				   &buffer->buffer_dir, "scan_elements");
>  	if (ret)
>  		goto error_free_scan_attrs;
>  
> @@ -1454,11 +1455,13 @@ 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);
> +
>  int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
>  	struct iio_buffer *buffer = indio_dev->buffer;
>  	const struct iio_chan_spec *channels;
> -	int i;
> +	int i, ret;
>  
>  	channels = indio_dev->channels;
>  	if (channels) {
> @@ -1472,7 +1475,29 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return 0;
>  
> -	return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev);
> +	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_link(&indio_dev->dev.kobj,
> +				&indio_dev->buffer->buffer_dir,
> +				"buffer");
> +	if (ret)
> +		goto error_free_sysfs_and_mask;
> +
> +	ret = sysfs_create_link(&indio_dev->dev.kobj,
> +				&indio_dev->buffer->scan_el_dir,
> +				"scan_elements");
> +	if (ret)
> +		goto error_remove_buffer_dir_link;
> +
> +	return 0;
> +
> +error_remove_buffer_dir_link:
> +	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> +error_free_sysfs_and_mask:
> +	__iio_buffer_free_sysfs_and_mask(buffer);
> +	return ret;
>  }
>  
>  static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> @@ -1494,6 +1519,9 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return;
>  
> +	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> +	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> +
>  	__iio_buffer_free_sysfs_and_mask(buffer);
>  }
>  

Re: [RFC PATCH 06/12] iio: buffer: re-route scan_elements via it's kobj_type

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:34 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> The scan_elements attributes are solely located inside
> 'industrialio-buffer-sysfs.c'. In order to support more than one buffer per
> IIO device, we need to expand scan_elements attributes directly to IIO
> buffer object, and not the IIO device.
> 
> This also requires that a new 'iio_buffer_attr' type be added which is
> mostly a copy of 'iio_dev_attr', but this expands to an IIO buffer object.
> 
> The 'iio_dev_attr' type could have been re-used here, but managing 'device'
> objects is a bit more tricky (than it looks at first). A 'device' object
> needs to be initialized & managed and we only need to the 'kobj' to expand
> from the 'bufferX' directory back to an IIO buffer.
> kobjects are simpler to manage.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
One trivial thing inline but otherwise looks fine to me.

Jonathan

> ---
>  drivers/iio/iio_core.h            |   5 +
>  drivers/iio/industrialio-buffer.c | 162 +++++++++++++++++++++++-------
>  drivers/iio/industrialio-core.c   |   1 -
>  3 files changed, 129 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index fced02cadcc3..43d44ec92781 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -31,6 +31,11 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
>  				       struct iio_ioctl_handler *h);
>  void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
>  
> +int iio_attr_init(struct attribute *attr,
> +		  const char *postfix,
> +		  struct iio_chan_spec const *chan,
> +		  enum iio_shared_by shared_by);
> +
>  int __iio_add_chan_devattr(const char *postfix,
>  			   struct iio_chan_spec const *chan,
>  			   ssize_t (*func)(struct device *dev,
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 62c8bd6b67cd..445709ef245c 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -26,6 +26,26 @@
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/buffer_impl.h>
>  
> +/**
> + * struct iio_buf_attr - iio buffer specific attribute
> + * @attr:	underlying attribute
> + * @address:	associated register address
> + * @l:		list head for maintaining list of dynamically created attrs
> + * @c:		specification for the underlying channel
> + * @show:	sysfs show hook for this attribute
> + * @store:	sysfs store hook for this attribute
> + */
> +struct iio_buf_attr {
> +	struct attribute attr;
> +	u64 address;
> +	struct list_head l;
> +	struct iio_chan_spec const *c;
> +	ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> +			char *buf);
> +	ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> +			 const char *buf, size_t count);
> +};
> +
>  static const char * const iio_endian_prefix[] = {
>  	[IIO_BE] = "be",
>  	[IIO_LE] = "le",
> @@ -210,18 +230,17 @@ void iio_buffer_init(struct iio_buffer *buffer)
>  }
>  EXPORT_SYMBOL(iio_buffer_init);
>  
> -static ssize_t iio_show_scan_index(struct device *dev,
> -				   struct device_attribute *attr,
> +static ssize_t iio_show_scan_index(struct iio_buffer *buffer,
> +				   struct iio_buf_attr *attr,
>  				   char *buf)
>  {
> -	return sprintf(buf, "%u\n", to_iio_dev_attr(attr)->c->scan_index);
> +	return sprintf(buf, "%u\n", attr->c->scan_index);
>  }
>  
> -static ssize_t iio_show_fixed_type(struct device *dev,
> -				   struct device_attribute *attr,
> +static ssize_t iio_show_fixed_type(struct iio_buffer *buffer,
> +				   struct iio_buf_attr *this_attr,
>  				   char *buf)
>  {
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  	u8 type = this_attr->c->scan_type.endianness;
>  
>  	if (type == IIO_CPU) {
> @@ -248,17 +267,14 @@ static ssize_t iio_show_fixed_type(struct device *dev,
>  		       this_attr->c->scan_type.shift);
>  }
>  
> -static ssize_t iio_scan_el_show(struct device *dev,
> -				struct device_attribute *attr,
> +static ssize_t iio_scan_el_show(struct iio_buffer *buffer,
> +				struct iio_buf_attr *attr,
>  				char *buf)
>  {
>  	int ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct iio_buffer *buffer = indio_dev->buffer;
>  
>  	/* Ensure ret is 0 or 1. */
> -	ret = !!test_bit(to_iio_dev_attr(attr)->address,
> -		       buffer->scan_mask);
> +	ret = !!test_bit(attr->address, buffer->scan_mask);
>  
>  	return sprintf(buf, "%d\n", ret);
>  }
> @@ -359,16 +375,14 @@ static int iio_scan_mask_query(struct iio_dev *indio_dev,
>  	return !!test_bit(bit, buffer->scan_mask);
>  };
>  
> -static ssize_t iio_scan_el_store(struct device *dev,
> -				 struct device_attribute *attr,
> +static ssize_t iio_scan_el_store(struct iio_buffer *buffer,
> +				 struct iio_buf_attr *this_attr,
>  				 const char *buf,
>  				 size_t len)
>  {
> +	struct iio_dev *indio_dev = buffer->indio_dev;
>  	int ret;
>  	bool state;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct iio_buffer *buffer = indio_dev->buffer;
> -	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>  
>  	ret = strtobool(buf, &state);
>  	if (ret < 0)
> @@ -398,24 +412,20 @@ static ssize_t iio_scan_el_store(struct device *dev,
>  
>  }
>  
> -static ssize_t iio_scan_el_ts_show(struct device *dev,
> -				   struct device_attribute *attr,
> +static ssize_t iio_scan_el_ts_show(struct iio_buffer *buffer,
> +				   struct iio_buf_attr *attr,
>  				   char *buf)
>  {
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct iio_buffer *buffer = indio_dev->buffer;
> -
>  	return sprintf(buf, "%d\n", buffer->scan_timestamp);
>  }
>  
> -static ssize_t iio_scan_el_ts_store(struct device *dev,
> -				    struct device_attribute *attr,
> +static ssize_t iio_scan_el_ts_store(struct iio_buffer *buffer,
> +				    struct iio_buf_attr *attr,
>  				    const char *buf,
>  				    size_t len)
>  {
> +	struct iio_dev *indio_dev = buffer->indio_dev;
>  	int ret;
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct iio_buffer *buffer = indio_dev->buffer;
>  	bool state;
>  
>  	ret = strtobool(buf, &state);
> @@ -434,13 +444,88 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> +static int __iio_add_chan_bufattr(const char *postfix,
> +				  struct iio_chan_spec const *chan,
> +				  ssize_t (*readfunc)(struct iio_buffer *buffer,
> +						      struct iio_buf_attr *attr,
> +						      char *buf),
> +				  ssize_t (*writefunc)(struct iio_buffer *buffer,
> +						       struct iio_buf_attr *attr,
> +						       const char *buf,
> +						       size_t len),
> +				  u64 mask,
> +				  enum iio_shared_by shared_by,
> +				  struct device *dev,
> +				  struct list_head *attr_list)
> +{
> +	struct iio_buf_attr *iio_attr, *t;
> +	int ret;
> +
> +	iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
> +	if (iio_attr == NULL)
> +		return -ENOMEM;
> +
> +	ret = iio_attr_init(&iio_attr->attr, postfix, chan, shared_by);
> +	if (ret)
> +		goto error_iio_buf_attr_free;
> +
> +	iio_attr->c = chan;
> +	iio_attr->address = mask;
> +	list_for_each_entry(t, attr_list, l) {
> +		if (strcmp(t->attr.name, iio_attr->attr.name) == 0) {
> +			if (shared_by == IIO_SEPARATE)
> +				dev_err(dev, "tried to double register : %s\n",
> +					t->attr.name);
> +			ret = -EBUSY;
> +			goto error_iio_buf_attr_deinit;
> +		}
> +	}
> +	list_add(&iio_attr->l, attr_list);
> +
> +	if (readfunc) {
> +		iio_attr->attr.mode |= S_IRUGO;
> +		iio_attr->show = readfunc;
> +	}
> +
> +	if (writefunc) {
> +		iio_attr->attr.mode |= S_IWUSR;
> +		iio_attr->store = writefunc;
> +	}
> +
> +	return 0;
> +
> +error_iio_buf_attr_deinit:
> +	kfree(iio_attr->attr.name);
> +error_iio_buf_attr_free:
> +	kfree(iio_attr);
> +	return ret;
> +}
> +
> +/**
> + * iio_free_chan_bufattr_list() - Free a list of IIO buffer attributes
> + * @attr_list: List of IIO buffer attributes
> + *
> + * This function frees the memory allocated for each of the IIO buffer
> + * attributes in the list.
> + */
> +static void iio_free_chan_bufattr_list(struct list_head *attr_list)
> +{
> +	struct iio_buf_attr *p, *n;
> +
> +	list_for_each_entry_safe(p, n, attr_list, l) {
> +		kfree(p->attr.name);

Trivial but ordering in here seems a bit odd.  Take it off
list before doing the name free makes more sense to me.

> +		list_del(&p->l);
> +		kfree(p);
> +	}
> +}
> +
>  static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  					struct iio_buffer *buffer,
>  					const struct iio_chan_spec *chan)
>  {
>  	int ret, attrcount = 0;
>  
> -	ret = __iio_add_chan_devattr("index",
> +	ret = __iio_add_chan_bufattr("index",
>  				     chan,
>  				     &iio_show_scan_index,
>  				     NULL,
> @@ -451,7 +536,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  	attrcount++;
> -	ret = __iio_add_chan_devattr("type",
> +	ret = __iio_add_chan_bufattr("type",
>  				     chan,
>  				     &iio_show_fixed_type,
>  				     NULL,
> @@ -463,7 +548,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  		return ret;
>  	attrcount++;
>  	if (chan->type != IIO_TIMESTAMP)
> -		ret = __iio_add_chan_devattr("en",
> +		ret = __iio_add_chan_bufattr("en",
>  					     chan,
>  					     &iio_scan_el_show,
>  					     &iio_scan_el_store,
> @@ -472,7 +557,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  					     &indio_dev->dev,
>  					     &buffer->scan_el_dev_attr_list);
>  	else
> -		ret = __iio_add_chan_devattr("en",
> +		ret = __iio_add_chan_bufattr("en",
>  					     chan,
>  					     &iio_scan_el_ts_show,
>  					     &iio_scan_el_ts_store,
> @@ -1251,6 +1336,7 @@ static struct attribute *iio_buffer_attrs[] = {
>  };
>  
>  #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> +#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
>  
>  static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
>  					struct attribute *attr,
> @@ -1291,9 +1377,9 @@ static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
>  					 char *buf)
>  {
>  	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> -	struct device_attribute *dattr = to_dev_attr(attr);
> +	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
>  
> -	return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> +	return battr->show(buffer, battr, buf);
>  }
>  
>  static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> @@ -1302,9 +1388,9 @@ static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
>  					  size_t len)
>  {
>  	struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> -	struct device_attribute *dattr = to_dev_attr(attr);
> +	struct iio_buf_attr *battr = to_iio_buf_attr(attr);
>  
> -	return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> +	return battr->store(buffer, battr, buf, len);
>  }
>  
>  static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> @@ -1349,7 +1435,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  					     struct iio_dev *indio_dev,
>  					     unsigned int idx)
>  {
> -	struct iio_dev_attr *p;
> +	struct iio_buf_attr *p;
>  	struct attribute **attr;
>  	int ret, i, attrn, attrcount;
>  	const struct iio_chan_spec *channels;
> @@ -1430,7 +1516,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  
>  	attrn = 0;
>  	list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> -		buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> +		buffer->scan_el_attrs[attrn++] = &p->attr;
>  
>  	ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
>  	if (ret)
> @@ -1446,7 +1532,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>  	bitmap_free(buffer->scan_mask);
>  error_cleanup_dynamic:
>  	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> -	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> +	iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
>  error_buffer_kobject_put:
>  	kobject_put(&buffer->buffer_dir);
>  error_buffer_free_attrs:
> @@ -1507,7 +1593,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  	kfree(buffer->scan_el_attrs);
>  	bitmap_free(buffer->scan_mask);
>  	iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> -	iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> +	iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
>  	kobject_put(&buffer->buffer_dir);
>  	kfree(buffer->buffer_attrs);
>  }
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index e9aa84f5b05a..28830e87e8cb 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -972,7 +972,6 @@ static ssize_t iio_write_channel_info(struct device *dev,
>  	return len;
>  }
>  
> -static
>  int iio_attr_init(struct attribute *attr,
>  		  const char *postfix,
>  		  struct iio_chan_spec const *chan,

Re: [RFC PATCH 11/12] iio: buffer: introduce support for attaching more IIO buffers

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:39 +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.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
A couple of minor things in here..

Jonathan

> ---
>  drivers/iio/industrialio-buffer.c | 58 +++++++++++++++++++++++++------
>  include/linux/iio/buffer_impl.h   |  3 ++
>  include/linux/iio/iio-opaque.h    |  4 +++
>  3 files changed, 54 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index c83cec89eddf..daa68822cea7 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1513,6 +1513,7 @@ 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_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_buffer *buffer = indio_dev->buffer;
>  	const struct iio_chan_spec *channels;
>  	int i, ret;
> @@ -1529,15 +1530,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return 0;
>  
> -	ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> -	if (ret)
> -		return ret;
> +	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)
> +			goto error_unwind_sysfs_and_mask;
> +	}
>  
>  	ret = sysfs_create_link(&indio_dev->dev.kobj,
>  				&indio_dev->buffer->buffer_dir,
>  				"buffer");
>  	if (ret)
> -		goto error_free_sysfs_and_mask;
> +		goto error_unwind_sysfs_and_mask;
>  
>  	ret = sysfs_create_link(&indio_dev->dev.kobj,
>  				&indio_dev->buffer->scan_el_dir,
> @@ -1549,8 +1553,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  
>  error_remove_buffer_dir_link:
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> -error_free_sysfs_and_mask:
> -	__iio_buffer_free_sysfs_and_mask(buffer);
> +	i = iio_dev_opaque->attached_buffers_cnt - 1;

Perhaps just use a counter variable that is only for this then you won't need
to set it again in this error path.

> +error_unwind_sysfs_and_mask:
> +	for (; i >= 0; i--) {
> +		buffer = iio_dev_opaque->attached_buffers[i];
> +		__iio_buffer_free_sysfs_and_mask(buffer);
> +	}
> +	kfree(iio_dev_opaque->attached_buffers);
> +	iio_dev_opaque->attached_buffers = NULL;
>  	return ret;
>  }
>  
> @@ -1568,7 +1578,9 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
>  
>  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_buffer *buffer = indio_dev->buffer;
> +	int i;
>  
>  	if (!buffer)
>  		return;
> @@ -1576,7 +1588,13 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
>  
> -	__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);
> +	iio_dev_opaque->attached_buffers = NULL;
>  }
>  
>  /**
> @@ -1709,14 +1727,32 @@ EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
>   * @buffer: The buffer to attach to the device
>   *
>   * 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)
>  {
> -	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) {
> +		kfree(old);

Need a comment on why freeing old makes sense.

> +		return;
> +	}
> +	iio_dev_opaque->attached_buffers = new;
> +
> +	/* first buffer is legacy; attach it to the IIO device directly */
> +	if (!indio_dev->buffer)
> +		indio_dev->buffer = iio_buffer_get(buffer);
> +
> +	buffer->indio_dev = indio_dev;
>  
> -	indio_dev->buffer->indio_dev = indio_dev;
> +	iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> +	iio_dev_opaque->attached_buffers_cnt = cnt;
>  }
>  EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index 77e169e51434..e25d26a7f601 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -124,6 +124,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 07c5a8e52ca8..1db0ea09520e 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
> @@ -20,6 +22,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;

Re: [RFC PATCH 12/12] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:40 +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() will not work for the first buffer, as that buffer is already
> opened/reserved for the IIO device's character device.
> This is also to preserve backwards compatibility.
> 
> For any other extra buffer (after the first buffer) this ioctl() will be
> required.

Silly question, but can we have the ioctl just return the file handle for
the buffer itself if called on index 0? 

Would make for slightly more natural userspace code even though it
doesn't do anything...

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-buffer.c | 111 ++++++++++++++++++++++++++++++
>  drivers/iio/industrialio-core.c   |   8 +++
>  include/linux/iio/buffer_impl.h   |   5 ++
>  include/linux/iio/iio-opaque.h    |   2 +
>  include/uapi/linux/iio/buffer.h   |  16 +++++
>  5 files changed, 142 insertions(+)
>  create mode 100644 include/uapi/linux/iio/buffer.h
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index daa68822cea7..77f02870cd18 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>
> @@ -1399,6 +1400,99 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
>  		sysfs_remove_file(kobj, ptr[i]);
>  }
>  
> +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;
> +
> +	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_outer_addr,
> +	.poll = iio_buffer_poll_addr,
> +	.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 -ENOENT;
> +
> +	fd = mutex_lock_interruptible(&indio_dev->mlock);
> +	if (fd)
> +		return fd;
> +
> +	buffer = iio_dev_opaque->attached_buffers[idx];
> +
> +	if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> +		fd = -EBUSY;
> +		goto error_unlock;
> +	}
> +
> +	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;
> +	}
> +
> +	mutex_unlock(&indio_dev->mlock);
> +	return fd;
> +
> +error_free_ib:
> +	kfree(ib);
> +error_iio_dev_put:
> +	iio_device_put(indio_dev);
> +	clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +error_unlock:
> +	mutex_unlock(&indio_dev->mlock);
> +	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,
>  					     unsigned int idx)
> @@ -1549,8 +1643,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto error_remove_buffer_dir_link;
>  
> +	i = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));

using i for that isn't particularly readable.  Add a sz or similar local
variable for it.

> +	iio_dev_opaque->buffer_ioctl_handler = kzalloc(i, GFP_KERNEL);
> +	if (!iio_dev_opaque->buffer_ioctl_handler) {
> +		ret = -ENOMEM;
> +		goto error_remove_scan_el_dir;
> +	}
> +
> +	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;
>  
> +error_remove_scan_el_dir:
> +	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
>  error_remove_buffer_dir_link:
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
>  	i = iio_dev_opaque->attached_buffers_cnt - 1;
> @@ -1585,6 +1692,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
>  	if (!buffer)
>  		return;
>  
> +	iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> +	kfree(iio_dev_opaque->buffer_ioctl_handler);
> +	iio_dev_opaque->buffer_ioctl_handler = NULL;
> +
>  	sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
>  	sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
>  
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 9e7a76723f00..b4f7dd75bef5 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1685,6 +1685,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
>  	ib->indio_dev = indio_dev;
>  	ib->buffer = indio_dev->buffer;
>  
> +	if (indio_dev->buffer)
> +		test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
> +
>  	filp->private_data = ib;
>  
>  	return 0;
> @@ -1702,6 +1705,11 @@ 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);
> +	struct iio_buffer *buffer = ib->buffer;
> +
> +	if (buffer)
> +		clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> +
>  	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 e25d26a7f601..78da590b5607 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;
>  
> @@ -75,6 +77,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 1db0ea09520e..d0429b13afa8 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
> @@ -24,6 +25,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..3794eca78dad
> --- /dev/null
> +++ b/include/uapi/linux/iio/buffer.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* industrial I/O buffer definitions needed both in and out of kernel
> + *
> + * Copyright (c) 2020 Alexandru Ardelean
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#ifndef _UAPI_IIO_BUFFER_H_
> +#define _UAPI_IIO_BUFFER_H_
> +
> +#define IIO_BUFFER_GET_FD_IOCTL		_IOWR('i', 0xa0, int)
Is it more sensible to just have that alongside the other IOCTLs rather than
defining a new header for it?

If not available I guess we'd just get an -EINVAL response, so harmless.

> +
> +#endif /* _UAPI_IIO_BUFFER_H_ */

Re: [RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per IIO device

[Jonathan Cameron]

On Tue, 17 Nov 2020 18:23:28 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Continuing from:
>   https://lore.kernel.org/linux-iio/20200517144023.6c5cb169@archlinux/
> 
> This is a V2 of the initial attempt in the discussion above.
> But it did not occur to me that I should mark it as V2 when I generated
> the patches.
> I've only tested [so far] that the current IIO buffer mechnism still works.
> And decided to show this sketch patchset.
> 
> This requires the ioctl() centralization mechanism, for which I sent a
> fix earlier.
>   https://lore.kernel.org/linux-iio/CA+U=Dsqf3UgyM666Gg9EmehpWiucDx2P0bmsC9JR--JJDT_eWQ@mail.gmail.com/T/#t
>   https://lore.kernel.org/linux-iio/20201117095154.7189-1-alexandru.ardelean@analog.com/T/#u
> 
> The gist of this is that now, the first IIO buffer should work as
> before, but all extra buffers should go through the anon inodes
> mechanism.
> I'd need to find a device or a way or a chip to test these extra buffers
> stuff. But I'm confident that this current form should eventually work
> with multiple IIO buffers per 1 IIO device and with anon inodes.
> 
> Maybe I'll take some of the patches in this set separately and send them
> individually. The problem with patchsets like this that tackle changes
> in a framework (like IIO) is that I become unsure after the 5th-7th patch,
> that the approach is correct. And I get even more unsure after that.
> 
> I'll create some userspace code to test this a bit, but I thought I'd
> send an RFC in the meantime.

From a first read, with all the warnings you give above, this looks pretty
good to me.   The kobj stuff is a little nasty and needs more docs
but other than that it all looks quite pleasant and readable and was
roughly what I was expecting from earlier discussions (which is great!).

Good work on this, looking forward to next steps.

Jonathan

> 
> Alexandru Ardelean (12):
>   iio: core: register chardev only if needed
>   iio: buffer: add back-ref from iio_buffer to iio_dev
>   iio: buffer: rework buffer & scan_elements dir creation
>   iio: buffer: add index to the first IIO buffer dir and symlink it back
>   iio: core: split __iio_device_attr_init() to init only the attr object
>   iio: buffer: re-route scan_elements via it's kobj_type
>   iio: buffer: re-route core buffer attributes via it's new kobj_type
>   iio: buffer: add helper to get the IIO device to which a buffer
>     belongs
>   iio: re-route all buffer attributes through new buffer kobj_type
>   iio: core: wrap iio device & buffer into struct for character devices
>   iio: buffer: introduce support for attaching more IIO buffers
>   iio: buffer: add ioctl() to support opening extra buffers for IIO
>     device
> 
>  drivers/iio/accel/adxl372.c                   |  36 +-
>  drivers/iio/accel/bmc150-accel-core.c         |  34 +-
>  drivers/iio/adc/at91-sama5d2_adc.c            |  30 +-
>  .../buffer/industrialio-buffer-dmaengine.c    |  13 +-
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |  30 +-
>  .../common/hid-sensors/hid-sensor-trigger.c   |  32 +-
>  drivers/iio/iio_core.h                        |  11 +
>  drivers/iio/industrialio-buffer.c             | 582 ++++++++++++++----
>  drivers/iio/industrialio-core.c               | 117 ++--
>  include/linux/iio/buffer.h                    |   2 +
>  include/linux/iio/buffer_impl.h               |  25 +-
>  include/linux/iio/iio-opaque.h                |   6 +
>  include/linux/iio/iio.h                       |   2 +-
>  include/linux/iio/sysfs.h                     |  50 ++
>  include/uapi/linux/iio/buffer.h               |  16 +
>  15 files changed, 735 insertions(+), 251 deletions(-)
>  create mode 100644 include/uapi/linux/iio/buffer.h
> 

Re: [RFC PATCH 01/12] iio: core: register chardev only if needed

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:05 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:29 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > 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().
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> I'll be honest. I have no idea why I didn't do this in first place!
>
> I 'think' we are safe dropping this but I suppose it's possible some
> odd code checks for the chrdev presence?

So, libiio at least doesn't rely on this being there for any odd things.
But yeah, who knows what else is out there that might.
I was also thinking of sending this separately to have this earlier
out there in case it bothers other people.
I guess I got a little mixed by other patches and re-ordered things a
few times and this remained in this series.

>
> Hopefully not though.
>
> Jonathan
>
> > ---
> >  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 419d6f8acc13..ca8b11541477 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1763,6 +1763,15 @@ static const struct file_operations iio_buffer_fileops = {
> >       .compat_ioctl = compat_ptr_ioctl,
> >  };
> >
> > +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;
> > @@ -1790,6 +1799,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)
> > @@ -1807,9 +1817,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);
> > @@ -1838,9 +1845,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)
>

Re: [RFC PATCH 03/12] iio: buffer: rework buffer & scan_elements dir creation

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:25 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:31 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > When adding more than one IIO buffer per IIO device, we will need to create
> > a buffer & scan_elements directory for each buffer.
> > We also want to move the 'scan_elements' to be a sub-directory of the
> > 'buffer' folder.
> >
> > The format we want to reach is, for a iio:device0 folder, for 2 buffers
> > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with
> > it's own 'scan_elements' subfolder.
> >
> > So, for example:
> >    iio:device0/buffer0
> >       scan_elements/
> >
> >    iio:device0/buffer1
> >       scan_elements/
> >
> > The other attributes under 'bufferX' would remain unchanged.
> >
> > However, we would also need to symlink back to the old 'buffer' &
> > 'scan_elements' folders, to keep backwards compatibility.
> >
> > Doing all these, require that we maintain the kobjects for each 'bufferX'
> > and 'scan_elements' so that we can symlink them back. We also need to
> > implement the sysfs_ops for these folders.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Hmm. This ended up a bit nasty.  It could do with a few more comments
> in the code to make it clear what is going on.

I'll take a look at these comments.

>
> > ---
> >  drivers/iio/industrialio-buffer.c | 151 ++++++++++++++++++++++++++----
> >  drivers/iio/industrialio-core.c   |  24 ++---
> >  include/linux/iio/buffer_impl.h   |  14 ++-
> >  include/linux/iio/iio.h           |   2 +-
> >  4 files changed, 156 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 08aa8e0782ce..8b31faf049a5 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,6 +1250,101 @@ static struct attribute *iio_buffer_attrs[] = {
> >       &dev_attr_data_available.attr,
> >  };
> >
> > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > +
> > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> > +                                     struct attribute *attr,
> > +                                     char *buf)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > +     struct device_attribute *dattr;
> > +
> > +     dattr = to_dev_attr(attr);
> > +
> > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +}
> > +
> > +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj,
> > +                                      struct attribute *attr,
> > +                                      const char *buf,
> > +                                      size_t len)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir);
> > +     struct device_attribute *dattr;
> > +
> > +     dattr = to_dev_attr(attr);
> > +
> > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > +}
> > +
> > +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = {
> > +     .show = iio_buffer_dir_attr_show,
> > +     .store = iio_buffer_dir_attr_store,
> > +};
> > +
> > +static struct kobj_type iio_buffer_dir_ktype = {
> > +     .sysfs_ops = &iio_buffer_dir_sysfs_ops,
> > +};
> > +
> > +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> > +                                      struct attribute *attr,
> > +                                      char *buf)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > +     struct device_attribute *dattr = to_dev_attr(attr);
> > +
> > +     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +}
> > +
> > +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> > +                                       struct attribute *attr,
> > +                                       const char *buf,
> > +                                       size_t len)
> > +{
> > +     struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > +     struct device_attribute *dattr = to_dev_attr(attr);
> > +
> > +     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > +}
> > +
> > +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> > +     .show = iio_scan_el_dir_attr_show,
> > +     .store = iio_scan_el_dir_attr_store,
> > +};
> > +
> > +static struct kobj_type iio_scan_el_dir_ktype = {
> > +     .sysfs_ops = &iio_scan_el_dir_sysfs_ops,
> > +};
> > +
> > +/*
> > + * This iio_sysfs_{add,del}_attrs() are essentially re-implementations of
> > + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get
> > + * around the const-pointer mismatch situation with using them.
> > + *
> > + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr',
> > + * while these are happy with just 'struct attribute **ptr'
>
> Ouch.  This definitely doesn't feel like a great thing to do.

Yep.
I'm still not 100% sure that this is needed.
But it may be that this is the best option.

>
> > + */
> > +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr)
> > +{
> > +     int err = 0;
> > +     int i;
> > +
> > +     for (i = 0; ptr[i] && !err; i++)
> > +             err = sysfs_create_file(kobj, ptr[i]);
> > +     if (err)
> > +             while (--i >= 0)
> > +                     sysfs_remove_file(kobj, ptr[i]);
> > +     return err;
> > +}
> > +
> > +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; ptr[i]; i++)
> > +             sysfs_remove_file(kobj, ptr[i]);
> > +}
> > +
> >  static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
>
> Definitely add some docs to this to say why we have this complexity..

Ack.

>
> >                                            struct iio_dev *indio_dev)
> >  {
> > @@ -1282,12 +1375,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >               memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs,
> >                      sizeof(struct attribute *) * attrcount);
> >
> > -     attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL;
> > +     buffer->buffer_attrs = attr;
> >
> > -     buffer->buffer_group.name = "buffer";
> > -     buffer->buffer_group.attrs = attr;
> > +     ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype,
> > +                                &indio_dev->dev.kobj, "buffer");
> > +     if (ret)
> > +             goto error_buffer_free_attrs;
> >
> > -     indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group;
> > +     ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> > +     if (ret)
> > +             goto error_buffer_kobject_put;
> >
> >       attrcount = 0;
> >       INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list);
> > @@ -1317,28 +1414,42 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >               }
> >       }
> >
> > -     buffer->scan_el_group.name = iio_scan_elements_group_name;
> > -
> > -     buffer->scan_el_group.attrs = kcalloc(attrcount + 1,
> > -                                           sizeof(buffer->scan_el_group.attrs[0]),
> > -                                           GFP_KERNEL);
> > -     if (buffer->scan_el_group.attrs == NULL) {
> > +     buffer->scan_el_attrs = kcalloc(attrcount + 1,
> > +                                     sizeof(buffer->scan_el_attrs[0]),
> > +                                     GFP_KERNEL);
> > +     if (buffer->scan_el_attrs == NULL) {
> >               ret = -ENOMEM;
> >               goto error_free_scan_mask;
> >       }
> > -     attrn = 0;
> >
> > +     ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype,
> > +                                &indio_dev->dev.kobj, "scan_elements");
> > +     if (ret)
> > +             goto error_free_scan_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;
> > -     indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group;
> > +             buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> > +
> > +     ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> > +     if (ret)
> > +             goto error_scan_kobject_put;
> >
> >       return 0;
> >
> > +error_scan_kobject_put:
> > +     kobject_put(&buffer->scan_el_dir);
> > +error_free_scan_attrs:
> > +     kfree(buffer->scan_el_attrs);
> >  error_free_scan_mask:
> >       bitmap_free(buffer->scan_mask);
> >  error_cleanup_dynamic:
> > +     iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > -     kfree(buffer->buffer_group.attrs);
> > +error_buffer_kobject_put:
> > +     kobject_put(&buffer->buffer_dir);
> > +error_buffer_free_attrs:
> > +     kfree(buffer->buffer_attrs);
> >
> >       return ret;
> >  }
> > @@ -1366,10 +1477,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >
> >  static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >  {
> > +     iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> > +     kobject_put(&buffer->scan_el_dir);
> > +     kfree(buffer->scan_el_attrs);
> >       bitmap_free(buffer->scan_mask);
> > -     kfree(buffer->buffer_group.attrs);
> > -     kfree(buffer->scan_el_group.attrs);
> > +     iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> >       iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > +     kobject_put(&buffer->buffer_dir);
> > +     kfree(buffer->buffer_attrs);
> >  }
> >
> >  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index ca8b11541477..f389d8feacb0 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1819,18 +1819,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >
> >       iio_device_register_debugfs(indio_dev);
> >
> > -     ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
> > -     if (ret) {
> > -             dev_err(indio_dev->dev.parent,
> > -                     "Failed to create buffer sysfs interfaces\n");
> > -             goto error_unreg_debugfs;
> > -     }
> > -
> >       ret = iio_device_register_sysfs(indio_dev);
> >       if (ret) {
> >               dev_err(indio_dev->dev.parent,
> >                       "Failed to register sysfs interfaces\n");
> > -             goto error_buffer_free_sysfs;
> > +             goto error_unreg_debugfs;
> >       }
> >       ret = iio_device_register_eventset(indio_dev);
> >       if (ret) {
> > @@ -1859,14 +1852,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod)
> >       if (ret < 0)
> >               goto error_unreg_eventset;
> >
> > +     ret = iio_buffer_alloc_sysfs_and_mask(indio_dev);
>
> There are some races around late creation of sysfs files (IIRC) but
> I'm not sure what else could be done here.

Yep, I was also thinking about these potential races a bit.
I'll need to think a bit more about handling them somehow.

Maybe there's a split I can try to do somewhere. Maybe the device
doesn't need to be added, but rather initialized somehow before it can
be referenced to add these directories dynamically.

Otherwise, we may try to temporarily set the 'indio_dev->info' to
NULL, and re-initialize it after the sysfs is completely initialized.
Under lock of course.

> Looking at device_add it is probably to do with the various notifiers being
> called before we have put everything in place.
>
> > +     if (ret) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "Failed to create buffer sysfs interfaces\n");
> > +             goto error_device_del;
> > +     }
> > +
> >       return 0;
> >
> > +error_device_del:
> > +     cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> >  error_unreg_eventset:
> >       iio_device_unregister_eventset(indio_dev);
> >  error_free_sysfs:
> >       iio_device_unregister_sysfs(indio_dev);
> > -error_buffer_free_sysfs:
> > -     iio_buffer_free_sysfs_and_mask(indio_dev);
> >  error_unreg_debugfs:
> >       iio_device_unregister_debugfs(indio_dev);
> >       return ret;
> > @@ -1882,6 +1882,8 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> >       struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >       struct iio_ioctl_handler *h, *t;
> >
> > +     iio_buffer_free_sysfs_and_mask(indio_dev);
> > +
> >       cdev_device_del(&indio_dev->chrdev, &indio_dev->dev);
> >
> >       mutex_lock(&indio_dev->info_exist_lock);
> > @@ -1899,8 +1901,6 @@ void iio_device_unregister(struct iio_dev *indio_dev)
> >       iio_buffer_wakeup_poll(indio_dev);
> >
> >       mutex_unlock(&indio_dev->info_exist_lock);
> > -
> > -     iio_buffer_free_sysfs_and_mask(indio_dev);
> >  }
> >  EXPORT_SYMBOL(iio_device_unregister);
> >
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index 67d73d465e02..77e169e51434 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -103,14 +103,20 @@ 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;
> > +     /* @buffer_dir: kobject for the 'buffer' directory of this buffer */
> > +     struct kobject buffer_dir;
> > +
> > +     /* @buffer_attrs: Attributes of the buffer group. */
> > +     struct attribute **buffer_attrs;
> > +
> > +     /* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */
> > +     struct kobject scan_el_dir;
> >
> >       /*
> > -      * @scan_el_group: Attribute group for those attributes not
> > +      * @scan_el_attrs: Array of attributes for those attributes not
> >        * created from the iio_chan_info array.
> >        */
> > -     struct attribute_group scan_el_group;
> > +     struct attribute **scan_el_attrs;
> >
> >       /* @attrs: Standard attributes of the buffer. */
> >       const struct attribute **attrs;
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 9a3cf4815148..2ea185340a3a 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -556,7 +556,7 @@ struct iio_dev {
> >       struct mutex                    info_exist_lock;
> >       const struct iio_buffer_setup_ops       *setup_ops;
> >       struct cdev                     chrdev;
> > -#define IIO_MAX_GROUPS 6
> > +#define IIO_MAX_GROUPS 4
> >       const struct attribute_group    *groups[IIO_MAX_GROUPS + 1];
> >       int                             groupcounter;
> >
>

Re: [RFC PATCH 06/12] iio: buffer: re-route scan_elements via it's kobj_type

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:34 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:34 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > The scan_elements attributes are solely located inside
> > 'industrialio-buffer-sysfs.c'. In order to support more than one buffer per
> > IIO device, we need to expand scan_elements attributes directly to IIO
> > buffer object, and not the IIO device.
> >
> > This also requires that a new 'iio_buffer_attr' type be added which is
> > mostly a copy of 'iio_dev_attr', but this expands to an IIO buffer object.
> >
> > The 'iio_dev_attr' type could have been re-used here, but managing 'device'
> > objects is a bit more tricky (than it looks at first). A 'device' object
> > needs to be initialized & managed and we only need to the 'kobj' to expand
> > from the 'bufferX' directory back to an IIO buffer.
> > kobjects are simpler to manage.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> One trivial thing inline but otherwise looks fine to me.
>
> Jonathan
>
> > ---
> >  drivers/iio/iio_core.h            |   5 +
> >  drivers/iio/industrialio-buffer.c | 162 +++++++++++++++++++++++-------
> >  drivers/iio/industrialio-core.c   |   1 -
> >  3 files changed, 129 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> > index fced02cadcc3..43d44ec92781 100644
> > --- a/drivers/iio/iio_core.h
> > +++ b/drivers/iio/iio_core.h
> > @@ -31,6 +31,11 @@ void iio_device_ioctl_handler_register(struct iio_dev *indio_dev,
> >                                      struct iio_ioctl_handler *h);
> >  void iio_device_ioctl_handler_unregister(struct iio_ioctl_handler *h);
> >
> > +int iio_attr_init(struct attribute *attr,
> > +               const char *postfix,
> > +               struct iio_chan_spec const *chan,
> > +               enum iio_shared_by shared_by);
> > +
> >  int __iio_add_chan_devattr(const char *postfix,
> >                          struct iio_chan_spec const *chan,
> >                          ssize_t (*func)(struct device *dev,
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 62c8bd6b67cd..445709ef245c 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -26,6 +26,26 @@
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/buffer_impl.h>
> >
> > +/**
> > + * struct iio_buf_attr - iio buffer specific attribute
> > + * @attr:    underlying attribute
> > + * @address: associated register address
> > + * @l:               list head for maintaining list of dynamically created attrs
> > + * @c:               specification for the underlying channel
> > + * @show:    sysfs show hook for this attribute
> > + * @store:   sysfs store hook for this attribute
> > + */
> > +struct iio_buf_attr {
> > +     struct attribute attr;
> > +     u64 address;
> > +     struct list_head l;
> > +     struct iio_chan_spec const *c;
> > +     ssize_t (*show)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> > +                     char *buf);
> > +     ssize_t (*store)(struct iio_buffer *buffer, struct iio_buf_attr *attr,
> > +                      const char *buf, size_t count);
> > +};
> > +
> >  static const char * const iio_endian_prefix[] = {
> >       [IIO_BE] = "be",
> >       [IIO_LE] = "le",
> > @@ -210,18 +230,17 @@ void iio_buffer_init(struct iio_buffer *buffer)
> >  }
> >  EXPORT_SYMBOL(iio_buffer_init);
> >
> > -static ssize_t iio_show_scan_index(struct device *dev,
> > -                                struct device_attribute *attr,
> > +static ssize_t iio_show_scan_index(struct iio_buffer *buffer,
> > +                                struct iio_buf_attr *attr,
> >                                  char *buf)
> >  {
> > -     return sprintf(buf, "%u\n", to_iio_dev_attr(attr)->c->scan_index);
> > +     return sprintf(buf, "%u\n", attr->c->scan_index);
> >  }
> >
> > -static ssize_t iio_show_fixed_type(struct device *dev,
> > -                                struct device_attribute *attr,
> > +static ssize_t iio_show_fixed_type(struct iio_buffer *buffer,
> > +                                struct iio_buf_attr *this_attr,
> >                                  char *buf)
> >  {
> > -     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >       u8 type = this_attr->c->scan_type.endianness;
> >
> >       if (type == IIO_CPU) {
> > @@ -248,17 +267,14 @@ static ssize_t iio_show_fixed_type(struct device *dev,
> >                      this_attr->c->scan_type.shift);
> >  }
> >
> > -static ssize_t iio_scan_el_show(struct device *dev,
> > -                             struct device_attribute *attr,
> > +static ssize_t iio_scan_el_show(struct iio_buffer *buffer,
> > +                             struct iio_buf_attr *attr,
> >                               char *buf)
> >  {
> >       int ret;
> > -     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -     struct iio_buffer *buffer = indio_dev->buffer;
> >
> >       /* Ensure ret is 0 or 1. */
> > -     ret = !!test_bit(to_iio_dev_attr(attr)->address,
> > -                    buffer->scan_mask);
> > +     ret = !!test_bit(attr->address, buffer->scan_mask);
> >
> >       return sprintf(buf, "%d\n", ret);
> >  }
> > @@ -359,16 +375,14 @@ static int iio_scan_mask_query(struct iio_dev *indio_dev,
> >       return !!test_bit(bit, buffer->scan_mask);
> >  };
> >
> > -static ssize_t iio_scan_el_store(struct device *dev,
> > -                              struct device_attribute *attr,
> > +static ssize_t iio_scan_el_store(struct iio_buffer *buffer,
> > +                              struct iio_buf_attr *this_attr,
> >                                const char *buf,
> >                                size_t len)
> >  {
> > +     struct iio_dev *indio_dev = buffer->indio_dev;
> >       int ret;
> >       bool state;
> > -     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -     struct iio_buffer *buffer = indio_dev->buffer;
> > -     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> >
> >       ret = strtobool(buf, &state);
> >       if (ret < 0)
> > @@ -398,24 +412,20 @@ static ssize_t iio_scan_el_store(struct device *dev,
> >
> >  }
> >
> > -static ssize_t iio_scan_el_ts_show(struct device *dev,
> > -                                struct device_attribute *attr,
> > +static ssize_t iio_scan_el_ts_show(struct iio_buffer *buffer,
> > +                                struct iio_buf_attr *attr,
> >                                  char *buf)
> >  {
> > -     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -     struct iio_buffer *buffer = indio_dev->buffer;
> > -
> >       return sprintf(buf, "%d\n", buffer->scan_timestamp);
> >  }
> >
> > -static ssize_t iio_scan_el_ts_store(struct device *dev,
> > -                                 struct device_attribute *attr,
> > +static ssize_t iio_scan_el_ts_store(struct iio_buffer *buffer,
> > +                                 struct iio_buf_attr *attr,
> >                                   const char *buf,
> >                                   size_t len)
> >  {
> > +     struct iio_dev *indio_dev = buffer->indio_dev;
> >       int ret;
> > -     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -     struct iio_buffer *buffer = indio_dev->buffer;
> >       bool state;
> >
> >       ret = strtobool(buf, &state);
> > @@ -434,13 +444,88 @@ static ssize_t iio_scan_el_ts_store(struct device *dev,
> >       return ret ? ret : len;
> >  }
> >
> > +static int __iio_add_chan_bufattr(const char *postfix,
> > +                               struct iio_chan_spec const *chan,
> > +                               ssize_t (*readfunc)(struct iio_buffer *buffer,
> > +                                                   struct iio_buf_attr *attr,
> > +                                                   char *buf),
> > +                               ssize_t (*writefunc)(struct iio_buffer *buffer,
> > +                                                    struct iio_buf_attr *attr,
> > +                                                    const char *buf,
> > +                                                    size_t len),
> > +                               u64 mask,
> > +                               enum iio_shared_by shared_by,
> > +                               struct device *dev,
> > +                               struct list_head *attr_list)
> > +{
> > +     struct iio_buf_attr *iio_attr, *t;
> > +     int ret;
> > +
> > +     iio_attr = kzalloc(sizeof(*iio_attr), GFP_KERNEL);
> > +     if (iio_attr == NULL)
> > +             return -ENOMEM;
> > +
> > +     ret = iio_attr_init(&iio_attr->attr, postfix, chan, shared_by);
> > +     if (ret)
> > +             goto error_iio_buf_attr_free;
> > +
> > +     iio_attr->c = chan;
> > +     iio_attr->address = mask;
> > +     list_for_each_entry(t, attr_list, l) {
> > +             if (strcmp(t->attr.name, iio_attr->attr.name) == 0) {
> > +                     if (shared_by == IIO_SEPARATE)
> > +                             dev_err(dev, "tried to double register : %s\n",
> > +                                     t->attr.name);
> > +                     ret = -EBUSY;
> > +                     goto error_iio_buf_attr_deinit;
> > +             }
> > +     }
> > +     list_add(&iio_attr->l, attr_list);
> > +
> > +     if (readfunc) {
> > +             iio_attr->attr.mode |= S_IRUGO;
> > +             iio_attr->show = readfunc;
> > +     }
> > +
> > +     if (writefunc) {
> > +             iio_attr->attr.mode |= S_IWUSR;
> > +             iio_attr->store = writefunc;
> > +     }
> > +
> > +     return 0;
> > +
> > +error_iio_buf_attr_deinit:
> > +     kfree(iio_attr->attr.name);
> > +error_iio_buf_attr_free:
> > +     kfree(iio_attr);
> > +     return ret;
> > +}
> > +
> > +/**
> > + * iio_free_chan_bufattr_list() - Free a list of IIO buffer attributes
> > + * @attr_list: List of IIO buffer attributes
> > + *
> > + * This function frees the memory allocated for each of the IIO buffer
> > + * attributes in the list.
> > + */
> > +static void iio_free_chan_bufattr_list(struct list_head *attr_list)
> > +{
> > +     struct iio_buf_attr *p, *n;
> > +
> > +     list_for_each_entry_safe(p, n, attr_list, l) {
> > +             kfree(p->attr.name);
>
> Trivial but ordering in here seems a bit odd.  Take it off
> list before doing the name free makes more sense to me.

Ack.

>
> > +             list_del(&p->l);
> > +             kfree(p);
> > +     }
> > +}
> > +
> >  static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> >                                       struct iio_buffer *buffer,
> >                                       const struct iio_chan_spec *chan)
> >  {
> >       int ret, attrcount = 0;
> >
> > -     ret = __iio_add_chan_devattr("index",
> > +     ret = __iio_add_chan_bufattr("index",
> >                                    chan,
> >                                    &iio_show_scan_index,
> >                                    NULL,
> > @@ -451,7 +536,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> >       if (ret)
> >               return ret;
> >       attrcount++;
> > -     ret = __iio_add_chan_devattr("type",
> > +     ret = __iio_add_chan_bufattr("type",
> >                                    chan,
> >                                    &iio_show_fixed_type,
> >                                    NULL,
> > @@ -463,7 +548,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> >               return ret;
> >       attrcount++;
> >       if (chan->type != IIO_TIMESTAMP)
> > -             ret = __iio_add_chan_devattr("en",
> > +             ret = __iio_add_chan_bufattr("en",
> >                                            chan,
> >                                            &iio_scan_el_show,
> >                                            &iio_scan_el_store,
> > @@ -472,7 +557,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> >                                            &indio_dev->dev,
> >                                            &buffer->scan_el_dev_attr_list);
> >       else
> > -             ret = __iio_add_chan_devattr("en",
> > +             ret = __iio_add_chan_bufattr("en",
> >                                            chan,
> >                                            &iio_scan_el_ts_show,
> >                                            &iio_scan_el_ts_store,
> > @@ -1251,6 +1336,7 @@ static struct attribute *iio_buffer_attrs[] = {
> >  };
> >
> >  #define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr)
> > +#define to_iio_buf_attr(_attr) container_of(_attr, struct iio_buf_attr, attr)
> >
> >  static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj,
> >                                       struct attribute *attr,
> > @@ -1291,9 +1377,9 @@ static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj,
> >                                        char *buf)
> >  {
> >       struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > -     struct device_attribute *dattr = to_dev_attr(attr);
> > +     struct iio_buf_attr *battr = to_iio_buf_attr(attr);
> >
> > -     return dattr->show(&buffer->indio_dev->dev, dattr, buf);
> > +     return battr->show(buffer, battr, buf);
> >  }
> >
> >  static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> > @@ -1302,9 +1388,9 @@ static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj,
> >                                         size_t len)
> >  {
> >       struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir);
> > -     struct device_attribute *dattr = to_dev_attr(attr);
> > +     struct iio_buf_attr *battr = to_iio_buf_attr(attr);
> >
> > -     return dattr->store(&buffer->indio_dev->dev, dattr, buf, len);
> > +     return battr->store(buffer, battr, buf, len);
> >  }
> >
> >  static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = {
> > @@ -1349,7 +1435,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >                                            struct iio_dev *indio_dev,
> >                                            unsigned int idx)
> >  {
> > -     struct iio_dev_attr *p;
> > +     struct iio_buf_attr *p;
> >       struct attribute **attr;
> >       int ret, i, attrn, attrcount;
> >       const struct iio_chan_spec *channels;
> > @@ -1430,7 +1516,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >
> >       attrn = 0;
> >       list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l)
> > -             buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr;
> > +             buffer->scan_el_attrs[attrn++] = &p->attr;
> >
> >       ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs);
> >       if (ret)
> > @@ -1446,7 +1532,7 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer,
> >       bitmap_free(buffer->scan_mask);
> >  error_cleanup_dynamic:
> >       iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> > -     iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > +     iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
> >  error_buffer_kobject_put:
> >       kobject_put(&buffer->buffer_dir);
> >  error_buffer_free_attrs:
> > @@ -1507,7 +1593,7 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >       kfree(buffer->scan_el_attrs);
> >       bitmap_free(buffer->scan_mask);
> >       iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs);
> > -     iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list);
> > +     iio_free_chan_bufattr_list(&buffer->scan_el_dev_attr_list);
> >       kobject_put(&buffer->buffer_dir);
> >       kfree(buffer->buffer_attrs);
> >  }
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index e9aa84f5b05a..28830e87e8cb 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -972,7 +972,6 @@ static ssize_t iio_write_channel_info(struct device *dev,
> >       return len;
> >  }
> >
> > -static
> >  int iio_attr_init(struct attribute *attr,
> >                 const char *postfix,
> >                 struct iio_chan_spec const *chan,
>

Re: [RFC PATCH 11/12] iio: buffer: introduce support for attaching more IIO buffers

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:46 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:39 +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.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> A couple of minor things in here..
>
> Jonathan
>
> > ---
> >  drivers/iio/industrialio-buffer.c | 58 +++++++++++++++++++++++++------
> >  include/linux/iio/buffer_impl.h   |  3 ++
> >  include/linux/iio/iio-opaque.h    |  4 +++
> >  3 files changed, 54 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index c83cec89eddf..daa68822cea7 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -1513,6 +1513,7 @@ 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_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >       struct iio_buffer *buffer = indio_dev->buffer;
> >       const struct iio_chan_spec *channels;
> >       int i, ret;
> > @@ -1529,15 +1530,18 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       if (!buffer)
> >               return 0;
> >
> > -     ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, 0);
> > -     if (ret)
> > -             return ret;
> > +     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)
> > +                     goto error_unwind_sysfs_and_mask;
> > +     }
> >
> >       ret = sysfs_create_link(&indio_dev->dev.kobj,
> >                               &indio_dev->buffer->buffer_dir,
> >                               "buffer");
> >       if (ret)
> > -             goto error_free_sysfs_and_mask;
> > +             goto error_unwind_sysfs_and_mask;
> >
> >       ret = sysfs_create_link(&indio_dev->dev.kobj,
> >                               &indio_dev->buffer->scan_el_dir,
> > @@ -1549,8 +1553,14 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >
> >  error_remove_buffer_dir_link:
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> > -error_free_sysfs_and_mask:
> > -     __iio_buffer_free_sysfs_and_mask(buffer);
> > +     i = iio_dev_opaque->attached_buffers_cnt - 1;
>
> Perhaps just use a counter variable that is only for this then you won't need
> to set it again in this error path.

Ack.

>
> > +error_unwind_sysfs_and_mask:
> > +     for (; i >= 0; i--) {
> > +             buffer = iio_dev_opaque->attached_buffers[i];
> > +             __iio_buffer_free_sysfs_and_mask(buffer);
> > +     }
> > +     kfree(iio_dev_opaque->attached_buffers);
> > +     iio_dev_opaque->attached_buffers = NULL;
> >       return ret;
> >  }
> >
> > @@ -1568,7 +1578,9 @@ static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer)
> >
> >  void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >  {
> > +     struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> >       struct iio_buffer *buffer = indio_dev->buffer;
> > +     int i;
> >
> >       if (!buffer)
> >               return;
> > @@ -1576,7 +1588,13 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >       sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> >
> > -     __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);
> > +     iio_dev_opaque->attached_buffers = NULL;
> >  }
> >
> >  /**
> > @@ -1709,14 +1727,32 @@ EXPORT_SYMBOL_GPL(iio_buffer_get_iio_dev);
> >   * @buffer: The buffer to attach to the device
> >   *
> >   * 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)
> >  {
> > -     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) {
> > +             kfree(old);
>
> Need a comment on why freeing old makes sense.

Hmm, maybe here I'd need to change this a bit.
I'm seeing a few potential issues.

>
> > +             return;
> > +     }
> > +     iio_dev_opaque->attached_buffers = new;
> > +
> > +     /* first buffer is legacy; attach it to the IIO device directly */
> > +     if (!indio_dev->buffer)
> > +             indio_dev->buffer = iio_buffer_get(buffer);
> > +
> > +     buffer->indio_dev = indio_dev;
> >
> > -     indio_dev->buffer->indio_dev = indio_dev;
> > +     iio_dev_opaque->attached_buffers[cnt - 1] = buffer;
> > +     iio_dev_opaque->attached_buffers_cnt = cnt;
> >  }
> >  EXPORT_SYMBOL_GPL(iio_device_attach_buffer);
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index 77e169e51434..e25d26a7f601 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -124,6 +124,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 07c5a8e52ca8..1db0ea09520e 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
> > @@ -20,6 +22,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;
>

Re: [RFC PATCH 12/12] iio: buffer: add ioctl() to support opening extra buffers for IIO device

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:40 +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() will not work for the first buffer, as that buffer is already
> > opened/reserved for the IIO device's character device.
> > This is also to preserve backwards compatibility.
> >
> > For any other extra buffer (after the first buffer) this ioctl() will be
> > required.
>
> Silly question, but can we have the ioctl just return the file handle for
> the buffer itself if called on index 0?

I also was thinking about this initially, but I'm not sure yet how to do it.
I'd need to dig into it.
Or find out where to dig this out from the 'struct file' type or wherever.
I initially started digging through anon_inode_getfd()  help, but it
didn't quite help.
I ended up in 'fs/anon_inodes.c',' 'fs/file.c' and
'include/linux/file.h', and the only thing you can seem to be able do
(at first glance) is to request and unused FD, and assigin a 'struct
file' object to it.

I may have missed something, but if that is the case, this ioctl()
would give you another FD for the same buffer zero.
First FD is the one used for the ioctl() (which can also do buffer
transfers), and second one is the one returned by this ioctl(), which
sounds like it could allow some mess in userspace code.

In case this isn't possible in a sane way, the only thing that can be
done is to indicate to the user that "hey, you already have an FD for
buffer 0, so use it".

>
> Would make for slightly more natural userspace code even though it
> doesn't do anything...
>
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-buffer.c | 111 ++++++++++++++++++++++++++++++
> >  drivers/iio/industrialio-core.c   |   8 +++
> >  include/linux/iio/buffer_impl.h   |   5 ++
> >  include/linux/iio/iio-opaque.h    |   2 +
> >  include/uapi/linux/iio/buffer.h   |  16 +++++
> >  5 files changed, 142 insertions(+)
> >  create mode 100644 include/uapi/linux/iio/buffer.h
> >
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index daa68822cea7..77f02870cd18 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>
> > @@ -1399,6 +1400,99 @@ static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr)
> >               sysfs_remove_file(kobj, ptr[i]);
> >  }
> >
> > +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;
> > +
> > +     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_outer_addr,
> > +     .poll = iio_buffer_poll_addr,
> > +     .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 -ENOENT;
> > +
> > +     fd = mutex_lock_interruptible(&indio_dev->mlock);
> > +     if (fd)
> > +             return fd;
> > +
> > +     buffer = iio_dev_opaque->attached_buffers[idx];
> > +
> > +     if (test_and_set_bit(IIO_BUSY_BIT_POS, &buffer->flags)) {
> > +             fd = -EBUSY;
> > +             goto error_unlock;
> > +     }
> > +
> > +     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;
> > +     }
> > +
> > +     mutex_unlock(&indio_dev->mlock);
> > +     return fd;
> > +
> > +error_free_ib:
> > +     kfree(ib);
> > +error_iio_dev_put:
> > +     iio_device_put(indio_dev);
> > +     clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +error_unlock:
> > +     mutex_unlock(&indio_dev->mlock);
> > +     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,
> >                                            unsigned int idx)
> > @@ -1549,8 +1643,21 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> >       if (ret)
> >               goto error_remove_buffer_dir_link;
> >
> > +     i = sizeof(*(iio_dev_opaque->buffer_ioctl_handler));
>
> using i for that isn't particularly readable.  Add a sz or similar local
> variable for it.

ack

>
> > +     iio_dev_opaque->buffer_ioctl_handler = kzalloc(i, GFP_KERNEL);
> > +     if (!iio_dev_opaque->buffer_ioctl_handler) {
> > +             ret = -ENOMEM;
> > +             goto error_remove_scan_el_dir;
> > +     }
> > +
> > +     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;
> >
> > +error_remove_scan_el_dir:
> > +     sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> >  error_remove_buffer_dir_link:
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> >       i = iio_dev_opaque->attached_buffers_cnt - 1;
> > @@ -1585,6 +1692,10 @@ void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev)
> >       if (!buffer)
> >               return;
> >
> > +     iio_device_ioctl_handler_unregister(iio_dev_opaque->buffer_ioctl_handler);
> > +     kfree(iio_dev_opaque->buffer_ioctl_handler);
> > +     iio_dev_opaque->buffer_ioctl_handler = NULL;
> > +
> >       sysfs_remove_link(&indio_dev->dev.kobj, "scan_elements");
> >       sysfs_remove_link(&indio_dev->dev.kobj, "buffer");
> >
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index 9e7a76723f00..b4f7dd75bef5 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1685,6 +1685,9 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
> >       ib->indio_dev = indio_dev;
> >       ib->buffer = indio_dev->buffer;
> >
> > +     if (indio_dev->buffer)
> > +             test_and_set_bit(IIO_BUSY_BIT_POS, &indio_dev->buffer->flags);
> > +
> >       filp->private_data = ib;
> >
> >       return 0;
> > @@ -1702,6 +1705,11 @@ 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);
> > +     struct iio_buffer *buffer = ib->buffer;
> > +
> > +     if (buffer)
> > +             clear_bit(IIO_BUSY_BIT_POS, &buffer->flags);
> > +
> >       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 e25d26a7f601..78da590b5607 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;
> >
> > @@ -75,6 +77,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 1db0ea09520e..d0429b13afa8 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
> > @@ -24,6 +25,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..3794eca78dad
> > --- /dev/null
> > +++ b/include/uapi/linux/iio/buffer.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/* industrial I/O buffer definitions needed both in and out of kernel
> > + *
> > + * Copyright (c) 2020 Alexandru Ardelean
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#ifndef _UAPI_IIO_BUFFER_H_
> > +#define _UAPI_IIO_BUFFER_H_
> > +
> > +#define IIO_BUFFER_GET_FD_IOCTL              _IOWR('i', 0xa0, int)
> Is it more sensible to just have that alongside the other IOCTLs rather than
> defining a new header for it?

If I didn't miss anything, the only other ioctl() is in
'include/uapi/linux/iio/events.h'
Which didn't seem logical to extend with this.

I can move it into events.h, but there will be more buffer ioctl()
calls anyway coming.
The ones we have now are here [Linux and libiio]:
https://github.com/analogdevicesinc/linux/blob/master/include/linux/iio/buffer_impl.h#L12
https://github.com/analogdevicesinc/libiio/blob/master/local.c#L51

I'm still trying to see how to make these backwards compatible with
our old ioctl() calls.
Maybe, one idea is that this gets moved to index 0xa5.
Or we just go to 0xb0 directly?


>
> If not available I guess we'd just get an -EINVAL response, so harmless.
>
> > +
> > +#endif /* _UAPI_IIO_BUFFER_H_ */
>

Re: [RFC PATCH 00/12] iio: core,buffer: add support for multiple IIO buffers per IIO device

[Alexandru Ardelean]

On Sat, Nov 21, 2020 at 8:53 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 17 Nov 2020 18:23:28 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > Continuing from:
> >   https://lore.kernel.org/linux-iio/20200517144023.6c5cb169@archlinux/
> >
> > This is a V2 of the initial attempt in the discussion above.
> > But it did not occur to me that I should mark it as V2 when I generated
> > the patches.
> > I've only tested [so far] that the current IIO buffer mechnism still works.
> > And decided to show this sketch patchset.
> >
> > This requires the ioctl() centralization mechanism, for which I sent a
> > fix earlier.
> >   https://lore.kernel.org/linux-iio/CA+U=Dsqf3UgyM666Gg9EmehpWiucDx2P0bmsC9JR--JJDT_eWQ@mail.gmail.com/T/#t
> >   https://lore.kernel.org/linux-iio/20201117095154.7189-1-alexandru.ardelean@analog.com/T/#u
> >
> > The gist of this is that now, the first IIO buffer should work as
> > before, but all extra buffers should go through the anon inodes
> > mechanism.
> > I'd need to find a device or a way or a chip to test these extra buffers
> > stuff. But I'm confident that this current form should eventually work
> > with multiple IIO buffers per 1 IIO device and with anon inodes.
> >
> > Maybe I'll take some of the patches in this set separately and send them
> > individually. The problem with patchsets like this that tackle changes
> > in a framework (like IIO) is that I become unsure after the 5th-7th patch,
> > that the approach is correct. And I get even more unsure after that.
> >
> > I'll create some userspace code to test this a bit, but I thought I'd
> > send an RFC in the meantime.
>
> From a first read, with all the warnings you give above, this looks pretty
> good to me.   The kobj stuff is a little nasty and needs more docs
> but other than that it all looks quite pleasant and readable and was
> roughly what I was expecting from earlier discussions (which is great!).
>
> Good work on this, looking forward to next steps.

Thanks.
I'll see about re-spinning this.
With the iio_buffer_set_attrs() change merged, this  patchset has a
new context that I need to take a look at.

>
> Jonathan
>
> >
> > Alexandru Ardelean (12):
> >   iio: core: register chardev only if needed
> >   iio: buffer: add back-ref from iio_buffer to iio_dev
> >   iio: buffer: rework buffer & scan_elements dir creation
> >   iio: buffer: add index to the first IIO buffer dir and symlink it back
> >   iio: core: split __iio_device_attr_init() to init only the attr object
> >   iio: buffer: re-route scan_elements via it's kobj_type
> >   iio: buffer: re-route core buffer attributes via it's new kobj_type
> >   iio: buffer: add helper to get the IIO device to which a buffer
> >     belongs
> >   iio: re-route all buffer attributes through new buffer kobj_type
> >   iio: core: wrap iio device & buffer into struct for character devices
> >   iio: buffer: introduce support for attaching more IIO buffers
> >   iio: buffer: add ioctl() to support opening extra buffers for IIO
> >     device
> >
> >  drivers/iio/accel/adxl372.c                   |  36 +-
> >  drivers/iio/accel/bmc150-accel-core.c         |  34 +-
> >  drivers/iio/adc/at91-sama5d2_adc.c            |  30 +-
> >  .../buffer/industrialio-buffer-dmaengine.c    |  13 +-
> >  .../cros_ec_sensors/cros_ec_sensors_core.c    |  30 +-
> >  .../common/hid-sensors/hid-sensor-trigger.c   |  32 +-
> >  drivers/iio/iio_core.h                        |  11 +
> >  drivers/iio/industrialio-buffer.c             | 582 ++++++++++++++----
> >  drivers/iio/industrialio-core.c               | 117 ++--
> >  include/linux/iio/buffer.h                    |   2 +
> >  include/linux/iio/buffer_impl.h               |  25 +-
> >  include/linux/iio/iio-opaque.h                |   6 +
> >  include/linux/iio/iio.h                       |   2 +-
> >  include/linux/iio/sysfs.h                     |  50 ++
> >  include/uapi/linux/iio/buffer.h               |  16 +
> >  15 files changed, 735 insertions(+), 251 deletions(-)
> >  create mode 100644 include/uapi/linux/iio/buffer.h
> >
>