[PATCH v3 0/7] iio: core: wrap IIO device into an iio_dev_opaque object

[PATCH v3 0/7] iio: core: wrap IIO device into an iio_dev_opaque object

[Alexandru Ardelean]

This change starts to hide some internal fields of the IIO device into
the framework.

This patchset assumes that all drivers have been addressed with respect
to the use of iio_priv_to_dev(), so the first patch in the series (now)
is to remove it, so that we don't need to move it.

V2 series:
   https://patchwork.kernel.org/patch/11548709/
   https://lore.kernel.org/linux-iio/20200514131710.84201-1-alexandru.ardelean@analog.com/
Referencing them, since it took a bit longer to get to V3

Changelog v2 -> v3:
- no driver should use iio_priv_to_dev() now; all drivers that use it
  should have been taken care by now; and as a result, all patches from
  v2 are no longer here
- added patch that just removes iio_priv_to_dev()
- for patch 'iio: core: wrap IIO device into an iio_dev_opaque object'
  added comment about 'priv' field that it must be accessed via
  iio_priv() helper
- patch 'iio: core: simplify alloc alignment code' removed
  added 'iio: core: remove padding from private information' instead
- v2 did not address too many movements from iio_dev -> iio_dev_opaque
  this patchset introduces a few more, the ones that seemed easier;
  v2 only had 'iio: core: move debugfs data on the private iio dev info'
  anything after that was added in v3

Changelog v1 -> v2:
- add pre-work patches that remove some calls to iio_priv_to_dev() from
  interrupt handlers
- renamed iio_dev_priv -> iio_dev_opaque
- moved the iio_dev_opaque to 'include/linux/iio/iio-opaque.h' this way
  it should be usable for debugging
- the iio_priv() call, is still an inline function that returns an
  'indio_dev->priv' reference; this field is added to 'struct iio_dev';
  the reference is computed in iio_device_alloc() and should be
  cacheline aligned
- the to_iio_dev_opaque() container is in the
  'include/linux/iio/iio-opaque.h' header; it's still implemented with
  some pointer arithmetic; one idea was to do it via an
  'indio_dev->opaque' field; that may still be an optionif there are
  some opinions in that direction

Alexandru Ardelean (7):
  iio: core: remove iio_priv_to_dev() helper
  iio: core: wrap IIO device into an iio_dev_opaque object
  iio: core: remove padding from private information
  iio: core: move debugfs data on the private iio dev info
  iio: core: move channel list & group to private iio device object
  iio: core: move iio_dev's buffer_list to the private iio device object
  iio: core: move event interface on the opaque struct

 drivers/iio/industrialio-buffer.c |  38 ++++++----
 drivers/iio/industrialio-core.c   | 114 ++++++++++++++++++------------
 drivers/iio/industrialio-event.c  |  68 ++++++++++--------
 include/linux/iio/iio-opaque.h    |  36 ++++++++++
 include/linux/iio/iio.h           |  35 ++-------
 5 files changed, 176 insertions(+), 115 deletions(-)
 create mode 100644 include/linux/iio/iio-opaque.h

-- 
2.17.1

[PATCH v3 1/7] iio: core: remove iio_priv_to_dev() helper

[Alexandru Ardelean]

All users of this helper have been updated to not use it.
Remove it now, so that we don't need to move it when creating the
iio_dev_opaque structure.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 include/linux/iio/iio.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 1c1d02107722..10a6d97a8e3e 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -703,12 +703,6 @@ static inline void *iio_priv(const struct iio_dev *indio_dev)
 	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
 }
 
-static inline struct iio_dev *iio_priv_to_dev(void *priv)
-{
-	return (struct iio_dev *)((char *)priv -
-				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
-}
-
 void iio_device_free(struct iio_dev *indio_dev);
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
 struct iio_trigger *devm_iio_trigger_alloc(struct device *dev,
-- 
2.17.1

[PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object

[Alexandru Ardelean]

There are plenty of bad designs we want to discourage or not have to review
manually usually about accessing private (marked as [INTERN]) fields of
'struct iio_dev'.

Sometimes users copy drivers that are not always the best examples.

A better idea is to hide those fields into the framework.
For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
'struct iio_dev' object.

In the next series, some fields will be moved to this new struct, each with
it's own rework.

This rework will not be complete-able for a while, as many fields need some
drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').

But some fields can already be moved, and in time, all of them may get
there (in the 'struct iio_dev_opaque' object).

Since a lot of drivers also call 'iio_priv()', in order to preserve
fast-paths (where this matters), the public iio_dev object will have a
'priv' field that will have the pointer to the private information already
computed. The reference returned by this field should be guaranteed to be
cacheline aligned.

As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
aren't many users of this helper, and arguably drivers shouldn't need to
use it in any fast-paths, as they can maintain a reference to the IIO
device.

The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
header. Should the hidden information be required for some debugging or
some special needs, it can be made available via this header.
Otherwise, only the IIO core files should include this file.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 19 +++++++++++++------
 include/linux/iio/iio-opaque.h  | 17 +++++++++++++++++
 include/linux/iio/iio.h         |  6 +++++-
 3 files changed, 35 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/iio/iio-opaque.h

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 75661661aaba..33e2953cf021 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
 #include <linux/debugfs.h>
 #include <linux/mutex.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/iio-opaque.h>
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include <linux/iio/sysfs.h>
@@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 static void iio_dev_release(struct device *device)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(device);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
 	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
 		iio_device_unregister_trigger_consumer(indio_dev);
 	iio_device_unregister_eventset(indio_dev);
@@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device *device)
 	iio_buffer_put(indio_dev->buffer);
 
 	ida_simple_remove(&iio_ida, indio_dev->id);
-	kfree(indio_dev);
+	kfree(iio_dev_opaque);
 }
 
 struct device_type iio_device_type = {
@@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
  **/
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 {
+	struct iio_dev_opaque *iio_dev_opaque;
 	struct iio_dev *dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev);
+	alloc_size = sizeof(struct iio_dev_opaque);
 	if (sizeof_priv) {
 		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
 		alloc_size += sizeof_priv;
@@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	/* ensure 32-byte alignment of whole construct ? */
 	alloc_size += IIO_ALIGN - 1;
 
-	dev = kzalloc(alloc_size, GFP_KERNEL);
-	if (!dev)
+	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
+	if (!iio_dev_opaque)
 		return NULL;
 
-	dev->dev.parent = parent;
+	dev = &iio_dev_opaque->indio_dev;
+	dev->priv = (char *)iio_dev_opaque +
+		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
+
 	dev->dev.groups = dev->groups;
 	dev->dev.type = &iio_device_type;
 	dev->dev.bus = &iio_bus_type;
@@ -1524,7 +1531,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	if (dev->id < 0) {
 		/* cannot use a dev_err as the name isn't available */
 		pr_err("failed to get device id\n");
-		kfree(dev);
+		kfree(iio_dev_opaque);
 		return NULL;
 	}
 	dev_set_name(&dev->dev, "iio:device%d", dev->id);
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
new file mode 100644
index 000000000000..1375674f14cd
--- /dev/null
+++ b/include/linux/iio/iio-opaque.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _INDUSTRIAL_IO_OPAQUE_H_
+#define _INDUSTRIAL_IO_OPAQUE_H_
+
+/**
+ * struct iio_dev_opaque - industrial I/O device opaque information
+ * @indio_dev:			public industrial I/O device information
+ */
+struct iio_dev_opaque {
+	struct iio_dev			indio_dev;
+};
+
+#define to_iio_dev_opaque(indio_dev)		\
+	container_of(indio_dev, struct iio_dev_opaque, indio_dev)
+
+#endif
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 10a6d97a8e3e..86112e35ae5f 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -522,6 +522,8 @@ struct iio_buffer_setup_ops {
  * @flags:		[INTERN] file ops related flags including busy flag.
  * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
  * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
+ * @priv:		[DRIVER] reference to driver's private information
+ *			**MUST** be accessed **ONLY** via iio_priv() helper
  */
 struct iio_dev {
 	int				id;
@@ -571,6 +573,7 @@ struct iio_dev {
 	char				read_buf[20];
 	unsigned int			read_buf_len;
 #endif
+	void				*priv;
 };
 
 const struct iio_chan_spec
@@ -698,9 +701,10 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
 #define IIO_ALIGN L1_CACHE_BYTES
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
 
+/* The information at the returned address is guaranteed to be cacheline aligned */
 static inline void *iio_priv(const struct iio_dev *indio_dev)
 {
-	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
+	return indio_dev->priv;
 }
 
 void iio_device_free(struct iio_dev *indio_dev);
-- 
2.17.1

[PATCH v3 3/7] iio: core: remove padding from private information

[Alexandru Ardelean]

There was a recent discussion about this code:
  https://lore.kernel.org/linux-iio/20200322165317.0b1f0674@archlinux/

This looks like a good time to removed this, since any issues about it
should pop-up under testing, because the iio_dev is having a bit of an
overhaul and stuff being moved to iio_dev_opaque.

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

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 33e2953cf021..27005ba4d09c 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1507,8 +1507,6 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
 		alloc_size += sizeof_priv;
 	}
-	/* ensure 32-byte alignment of whole construct ? */
-	alloc_size += IIO_ALIGN - 1;
 
 	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_opaque)
-- 
2.17.1

[PATCH v3 4/7] iio: core: move debugfs data on the private iio dev info

[Alexandru Ardelean]

This change moves all iio_dev debugfs fields to the iio_dev_priv object.
It's not the biggest advantage yet (to the whole thing of abstractization)
but it's a start.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 40 ++++++++++++++++++++++-----------
 include/linux/iio/iio-opaque.h  | 10 +++++++++
 include/linux/iio/iio.h         | 13 +----------
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 27005ba4d09c..05175cf80c98 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -165,6 +165,13 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
 };
 
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
+{
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	return iio_dev_opaque->debugfs_dentry;
+}
+EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -308,35 +315,37 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
 			      size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	unsigned val = 0;
 	int ret;
 
 	if (*ppos > 0)
 		return simple_read_from_buffer(userbuf, count, ppos,
-					       indio_dev->read_buf,
-					       indio_dev->read_buf_len);
+					       iio_dev_opaque->read_buf,
+					       iio_dev_opaque->read_buf_len);
 
 	ret = indio_dev->info->debugfs_reg_access(indio_dev,
-						  indio_dev->cached_reg_addr,
+						  iio_dev_opaque->cached_reg_addr,
 						  0, &val);
 	if (ret) {
 		dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__);
 		return ret;
 	}
 
-	indio_dev->read_buf_len = snprintf(indio_dev->read_buf,
-					   sizeof(indio_dev->read_buf),
-					   "0x%X\n", val);
+	iio_dev_opaque->read_buf_len = snprintf(iio_dev_opaque->read_buf,
+					      sizeof(iio_dev_opaque->read_buf),
+					      "0x%X\n", val);
 
 	return simple_read_from_buffer(userbuf, count, ppos,
-				       indio_dev->read_buf,
-				       indio_dev->read_buf_len);
+				       iio_dev_opaque->read_buf,
+				       iio_dev_opaque->read_buf_len);
 }
 
 static ssize_t iio_debugfs_write_reg(struct file *file,
 		     const char __user *userbuf, size_t count, loff_t *ppos)
 {
 	struct iio_dev *indio_dev = file->private_data;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	unsigned reg, val;
 	char buf[80];
 	int ret;
@@ -351,10 +360,10 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
 
 	switch (ret) {
 	case 1:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_opaque->cached_reg_addr = reg;
 		break;
 	case 2:
-		indio_dev->cached_reg_addr = reg;
+		iio_dev_opaque->cached_reg_addr = reg;
 		ret = indio_dev->info->debugfs_reg_access(indio_dev, reg,
 							  val, NULL);
 		if (ret) {
@@ -378,23 +387,28 @@ static const struct file_operations iio_debugfs_reg_fops = {
 
 static void iio_device_unregister_debugfs(struct iio_dev *indio_dev)
 {
-	debugfs_remove_recursive(indio_dev->debugfs_dentry);
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	debugfs_remove_recursive(iio_dev_opaque->debugfs_dentry);
 }
 
 static void iio_device_register_debugfs(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque;
+
 	if (indio_dev->info->debugfs_reg_access == NULL)
 		return;
 
 	if (!iio_debugfs_dentry)
 		return;
 
-	indio_dev->debugfs_dentry =
+	iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	iio_dev_opaque->debugfs_dentry =
 		debugfs_create_dir(dev_name(&indio_dev->dev),
 				   iio_debugfs_dentry);
 
 	debugfs_create_file("direct_reg_access", 0644,
-			    indio_dev->debugfs_dentry, indio_dev,
+			    iio_dev_opaque->debugfs_dentry, indio_dev,
 			    &iio_debugfs_reg_fops);
 }
 #else
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 1375674f14cd..b3f234b4c1e9 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -6,9 +6,19 @@
 /**
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
+ * @debugfs_dentry:		device specific debugfs dentry
+ * @cached_reg_addr:		cached register address for debugfs reads
+ * @read_buf:			read buffer to be used for the initial reg read
+ * @read_buf_len:		data length in @read_buf
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+#if defined(CONFIG_DEBUG_FS)
+	struct dentry			*debugfs_dentry;
+	unsigned			cached_reg_addr;
+	char				read_buf[20];
+	unsigned int			read_buf_len;
+#endif
 };
 
 #define to_iio_dev_opaque(indio_dev)		\
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 86112e35ae5f..bb0aae11a111 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -520,8 +520,6 @@ struct iio_buffer_setup_ops {
  * @groups:		[INTERN] attribute groups
  * @groupcounter:	[INTERN] index of next attribute group
  * @flags:		[INTERN] file ops related flags including busy flag.
- * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
- * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
  * @priv:		[DRIVER] reference to driver's private information
  *			**MUST** be accessed **ONLY** via iio_priv() helper
  */
@@ -567,12 +565,6 @@ struct iio_dev {
 	int				groupcounter;
 
 	unsigned long			flags;
-#if defined(CONFIG_DEBUG_FS)
-	struct dentry			*debugfs_dentry;
-	unsigned			cached_reg_addr;
-	char				read_buf[20];
-	unsigned int			read_buf_len;
-#endif
 	void				*priv;
 };
 
@@ -727,10 +719,7 @@ static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
  * @indio_dev:		IIO device structure for device
  **/
 #if defined(CONFIG_DEBUG_FS)
-static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
-{
-	return indio_dev->debugfs_dentry;
-}
+struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev);
 #else
 static inline struct dentry *iio_get_debugfs_dentry(struct iio_dev *indio_dev)
 {
-- 
2.17.1

[PATCH v3 5/7] iio: core: move channel list & group to private iio device object

[Alexandru Ardelean]

This change bit straightforward and simple, since the
'channel_attr_list' & 'chan_attr_group' fields are only used in
'industrialio-core.c'.

This change moves to the private IIO device object

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c | 46 +++++++++++++++++++--------------
 include/linux/iio/iio-opaque.h  |  5 ++++
 include/linux/iio/iio.h         |  5 ----
 3 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 05175cf80c98..67b8c7eb8b46 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1131,6 +1131,7 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
 					 enum iio_shared_by shared_by,
 					 const long *infomask)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int i, ret, attrcount = 0;
 
 	for_each_set_bit(i, infomask, sizeof(*infomask)*8) {
@@ -1143,7 +1144,7 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
 					     i,
 					     shared_by,
 					     &indio_dev->dev,
-					     &indio_dev->channel_attr_list);
+					     &iio_dev_opaque->channel_attr_list);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
 		else if (ret < 0)
@@ -1159,6 +1160,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 					       enum iio_shared_by shared_by,
 					       const long *infomask)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int i, ret, attrcount = 0;
 	char *avail_postfix;
 
@@ -1178,7 +1180,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 					     i,
 					     shared_by,
 					     &indio_dev->dev,
-					     &indio_dev->channel_attr_list);
+					     &iio_dev_opaque->channel_attr_list);
 		kfree(avail_postfix);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
@@ -1193,6 +1195,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
 					struct iio_chan_spec const *chan)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int ret, attrcount = 0;
 	const struct iio_chan_spec_ext_info *ext_info;
 
@@ -1268,7 +1271,7 @@ static int iio_device_add_channel_sysfs(struct iio_dev *indio_dev,
 					i,
 					ext_info->shared,
 					&indio_dev->dev,
-					&indio_dev->channel_attr_list);
+					&iio_dev_opaque->channel_attr_list);
 			i++;
 			if (ret == -EBUSY && ext_info->shared)
 				continue;
@@ -1403,6 +1406,7 @@ static DEVICE_ATTR(current_timestamp_clock, S_IRUGO | S_IWUSR,
 
 static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int i, ret = 0, attrcount, attrn, attrcount_orig = 0;
 	struct iio_dev_attr *p;
 	struct attribute **attr, *clk = NULL;
@@ -1442,47 +1446,49 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 	if (clk)
 		attrcount++;
 
-	indio_dev->chan_attr_group.attrs = kcalloc(attrcount + 1,
-						   sizeof(indio_dev->chan_attr_group.attrs[0]),
-						   GFP_KERNEL);
-	if (indio_dev->chan_attr_group.attrs == NULL) {
+	iio_dev_opaque->chan_attr_group.attrs =
+		kcalloc(attrcount + 1,
+			sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
+			GFP_KERNEL);
+	if (iio_dev_opaque->chan_attr_group.attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_clear_attrs;
 	}
 	/* Copy across original attributes */
 	if (indio_dev->info->attrs)
-		memcpy(indio_dev->chan_attr_group.attrs,
+		memcpy(iio_dev_opaque->chan_attr_group.attrs,
 		       indio_dev->info->attrs->attrs,
-		       sizeof(indio_dev->chan_attr_group.attrs[0])
+		       sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
 		       *attrcount_orig);
 	attrn = attrcount_orig;
 	/* Add all elements from the list. */
-	list_for_each_entry(p, &indio_dev->channel_attr_list, l)
-		indio_dev->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
+	list_for_each_entry(p, &iio_dev_opaque->channel_attr_list, l)
+		iio_dev_opaque->chan_attr_group.attrs[attrn++] = &p->dev_attr.attr;
 	if (indio_dev->name)
-		indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
+		iio_dev_opaque->chan_attr_group.attrs[attrn++] = &dev_attr_name.attr;
 	if (indio_dev->label)
-		indio_dev->chan_attr_group.attrs[attrn++] = &dev_attr_label.attr;
+		iio_dev_opaque->chan_attr_group.attrs[attrn++] = &dev_attr_label.attr;
 	if (clk)
-		indio_dev->chan_attr_group.attrs[attrn++] = clk;
+		iio_dev_opaque->chan_attr_group.attrs[attrn++] = clk;
 
 	indio_dev->groups[indio_dev->groupcounter++] =
-		&indio_dev->chan_attr_group;
+		&iio_dev_opaque->chan_attr_group;
 
 	return 0;
 
 error_clear_attrs:
-	iio_free_chan_devattr_list(&indio_dev->channel_attr_list);
+	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
 
 	return ret;
 }
 
 static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 
-	iio_free_chan_devattr_list(&indio_dev->channel_attr_list);
-	kfree(indio_dev->chan_attr_group.attrs);
-	indio_dev->chan_attr_group.attrs = NULL;
+	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
+	kfree(iio_dev_opaque->chan_attr_group.attrs);
+	iio_dev_opaque->chan_attr_group.attrs = NULL;
 }
 
 static void iio_dev_release(struct device *device)
@@ -1537,7 +1543,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	dev_set_drvdata(&dev->dev, (void *)dev);
 	mutex_init(&dev->mlock);
 	mutex_init(&dev->info_exist_lock);
-	INIT_LIST_HEAD(&dev->channel_attr_list);
+	INIT_LIST_HEAD(&iio_dev_opaque->channel_attr_list);
 
 	dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL);
 	if (dev->id < 0) {
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index b3f234b4c1e9..9419a05c698d 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -6,6 +6,9 @@
 /**
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
+ * @channel_attr_list:		keep track of automatically created channel
+ *				attributes
+ * @chan_attr_group:		group for all attrs in base directory
  * @debugfs_dentry:		device specific debugfs dentry
  * @cached_reg_addr:		cached register address for debugfs reads
  * @read_buf:			read buffer to be used for the initial reg read
@@ -13,6 +16,8 @@
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+	struct list_head		channel_attr_list;
+	struct attribute_group		chan_attr_group;
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry			*debugfs_dentry;
 	unsigned			cached_reg_addr;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index bb0aae11a111..30e6fc1506ea 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -506,9 +506,6 @@ struct iio_buffer_setup_ops {
  * @pollfunc_event:	[DRIVER] function run on events trigger being received
  * @channels:		[DRIVER] channel specification structure table
  * @num_channels:	[DRIVER] number of channels specified in @channels.
- * @channel_attr_list:	[INTERN] keep track of automatically created channel
- *			attributes
- * @chan_attr_group:	[INTERN] group for all attrs in base directory
  * @name:		[DRIVER] name of the device.
  * @label:              [DRIVER] unique name to identify which device this is
  * @info:		[DRIVER] callbacks and constant info from driver
@@ -551,8 +548,6 @@ struct iio_dev {
 	struct iio_chan_spec const	*channels;
 	int				num_channels;
 
-	struct list_head		channel_attr_list;
-	struct attribute_group		chan_attr_group;
 	const char			*name;
 	const char			*label;
 	const struct iio_info		*info;
-- 
2.17.1

[PATCH v3 6/7] iio: core: move iio_dev's buffer_list to the private iio device object

[Alexandru Ardelean]

This change moves the 'buffer_list' away from the public IIO device object
into the private part.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-buffer.c | 38 +++++++++++++++++++------------
 drivers/iio/industrialio-core.c   |  2 +-
 include/linux/iio/iio-opaque.h    |  2 ++
 include/linux/iio/iio.h           |  2 --
 4 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 329dd4d6757a..2aec8b85f40d 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -19,6 +19,7 @@
 #include <linux/sched/signal.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/iio-opaque.h>
 #include "iio_core.h"
 #include "iio_core_trigger.h"
 #include <linux/iio/sysfs.h>
@@ -599,8 +600,10 @@ static int iio_compute_scan_bytes(struct iio_dev *indio_dev,
 static void iio_buffer_activate(struct iio_dev *indio_dev,
 	struct iio_buffer *buffer)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
 	iio_buffer_get(buffer);
-	list_add(&buffer->buffer_list, &indio_dev->buffer_list);
+	list_add(&buffer->buffer_list, &iio_dev_opaque->buffer_list);
 }
 
 static void iio_buffer_deactivate(struct iio_buffer *buffer)
@@ -612,10 +615,11 @@ static void iio_buffer_deactivate(struct iio_buffer *buffer)
 
 static void iio_buffer_deactivate_all(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer, *_buffer;
 
 	list_for_each_entry_safe(buffer, _buffer,
-			&indio_dev->buffer_list, buffer_list)
+			&iio_dev_opaque->buffer_list, buffer_list)
 		iio_buffer_deactivate(buffer);
 }
 
@@ -688,6 +692,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	struct iio_buffer *insert_buffer, struct iio_buffer *remove_buffer,
 	struct iio_device_config *config)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	unsigned long *compound_mask;
 	const unsigned long *scan_mask;
 	bool strict_scanmask = false;
@@ -710,12 +715,12 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 	 * to verify.
 	 */
 	if (remove_buffer && !insert_buffer &&
-		list_is_singular(&indio_dev->buffer_list))
+		list_is_singular(&iio_dev_opaque->buffer_list))
 			return 0;
 
 	modes = indio_dev->modes;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
 		if (buffer == remove_buffer)
 			continue;
 		modes &= buffer->access->modes;
@@ -736,7 +741,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 		 * Keep things simple for now and only allow a single buffer to
 		 * be connected in hardware mode.
 		 */
-		if (insert_buffer && !list_empty(&indio_dev->buffer_list))
+		if (insert_buffer && !list_empty(&iio_dev_opaque->buffer_list))
 			return -EINVAL;
 		config->mode = INDIO_BUFFER_HARDWARE;
 		strict_scanmask = true;
@@ -756,7 +761,7 @@ static int iio_verify_update(struct iio_dev *indio_dev,
 
 	scan_timestamp = false;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
 		if (buffer == remove_buffer)
 			continue;
 		bitmap_or(compound_mask, compound_mask, buffer->scan_mask,
@@ -902,10 +907,11 @@ static int iio_buffer_update_demux(struct iio_dev *indio_dev,
 
 static int iio_update_demux(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer;
 	int ret;
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
 		ret = iio_buffer_update_demux(indio_dev, buffer);
 		if (ret < 0)
 			goto error_clear_mux_table;
@@ -913,7 +919,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
 	return 0;
 
 error_clear_mux_table:
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list)
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list)
 		iio_buffer_demux_free(buffer);
 
 	return ret;
@@ -922,6 +928,7 @@ static int iio_update_demux(struct iio_dev *indio_dev)
 static int iio_enable_buffers(struct iio_dev *indio_dev,
 	struct iio_device_config *config)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer;
 	int ret;
 
@@ -958,7 +965,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 		indio_dev->info->hwfifo_set_watermark(indio_dev,
 			config->watermark);
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
 		ret = iio_buffer_enable(buffer, indio_dev);
 		if (ret)
 			goto err_disable_buffers;
@@ -983,7 +990,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	return 0;
 
 err_disable_buffers:
-	list_for_each_entry_continue_reverse(buffer, &indio_dev->buffer_list,
+	list_for_each_entry_continue_reverse(buffer, &iio_dev_opaque->buffer_list,
 					     buffer_list)
 		iio_buffer_disable(buffer, indio_dev);
 err_run_postdisable:
@@ -998,12 +1005,13 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 
 static int iio_disable_buffers(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_buffer *buffer;
 	int ret = 0;
 	int ret2;
 
 	/* Wind down existing buffers - iff there are any */
-	if (list_empty(&indio_dev->buffer_list))
+	if (list_empty(&iio_dev_opaque->buffer_list))
 		return 0;
 
 	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
@@ -1024,7 +1032,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 			ret = ret2;
 	}
 
-	list_for_each_entry(buffer, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buffer, &iio_dev_opaque->buffer_list, buffer_list) {
 		ret2 = iio_buffer_disable(buffer, indio_dev);
 		if (ret2 && !ret)
 			ret = ret2;
@@ -1047,6 +1055,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
 		       struct iio_buffer *insert_buffer,
 		       struct iio_buffer *remove_buffer)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_device_config new_config;
 	int ret;
 
@@ -1071,7 +1080,7 @@ static int __iio_update_buffers(struct iio_dev *indio_dev,
 		iio_buffer_activate(indio_dev, insert_buffer);
 
 	/* If no buffers in list, we are done */
-	if (list_empty(&indio_dev->buffer_list))
+	if (list_empty(&iio_dev_opaque->buffer_list))
 		return 0;
 
 	ret = iio_enable_buffers(indio_dev, &new_config);
@@ -1420,10 +1429,11 @@ static int iio_push_to_buffer(struct iio_buffer *buffer, const void *data)
  */
 int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	int ret;
 	struct iio_buffer *buf;
 
-	list_for_each_entry(buf, &indio_dev->buffer_list, buffer_list) {
+	list_for_each_entry(buf, &iio_dev_opaque->buffer_list, buffer_list) {
 		ret = iio_push_to_buffer(buf, data);
 		if (ret < 0)
 			return ret;
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 67b8c7eb8b46..051cb05a8da7 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1553,7 +1553,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 		return NULL;
 	}
 	dev_set_name(&dev->dev, "iio:device%d", dev->id);
-	INIT_LIST_HEAD(&dev->buffer_list);
+	INIT_LIST_HEAD(&iio_dev_opaque->buffer_list);
 
 	return dev;
 }
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 9419a05c698d..af6c69a40169 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -6,6 +6,7 @@
 /**
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
+ * @buffer_list:		list of all buffers currently attached
  * @channel_attr_list:		keep track of automatically created channel
  *				attributes
  * @chan_attr_group:		group for all attrs in base directory
@@ -16,6 +17,7 @@
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+	struct list_head		buffer_list;
 	struct list_head		channel_attr_list;
 	struct attribute_group		chan_attr_group;
 #if defined(CONFIG_DEBUG_FS)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 30e6fc1506ea..219847f6f5c6 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -490,7 +490,6 @@ struct iio_buffer_setup_ops {
  *			and owner
  * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
  * @buffer:		[DRIVER] any buffer present
- * @buffer_list:	[INTERN] list of all buffers currently attached
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
  * @mlock:		[INTERN] lock used to prevent simultaneous device state
  *			changes
@@ -531,7 +530,6 @@ struct iio_dev {
 	struct iio_event_interface	*event_interface;
 
 	struct iio_buffer		*buffer;
-	struct list_head		buffer_list;
 	int				scan_bytes;
 	struct mutex			mlock;
 
-- 
2.17.1

[PATCH v3 7/7] iio: core: move event interface on the opaque struct

[Alexandru Ardelean]

Same as with other private fields, this moves the event interface reference
to the opaque IIO device object, to be invisible to drivers.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/industrialio-core.c  |  5 ++-
 drivers/iio/industrialio-event.c | 68 +++++++++++++++++++-------------
 include/linux/iio/iio-opaque.h   |  2 +
 include/linux/iio/iio.h          |  3 --
 4 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 051cb05a8da7..12550f59caa4 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -205,7 +205,8 @@ EXPORT_SYMBOL(iio_read_const_attr);
 int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 {
 	int ret;
-	const struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	const struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 
 	ret = mutex_lock_interruptible(&indio_dev->mlock);
 	if (ret)
@@ -1436,7 +1437,7 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 			attrcount += ret;
 		}
 
-	if (indio_dev->event_interface)
+	if (iio_dev_opaque->event_interface)
 		clk = &dev_attr_current_timestamp_clock.attr;
 
 	if (indio_dev->name)
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 5b17c92d3b50..2ab4d4c44427 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -18,6 +18,7 @@
 #include <linux/uaccess.h>
 #include <linux/wait.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/iio-opaque.h>
 #include "iio_core.h"
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
@@ -62,7 +63,8 @@ bool iio_event_enabled(const struct iio_event_interface *ev_int)
  **/
 int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
 {
-	struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 	struct iio_event_data ev;
 	int copied;
 
@@ -96,7 +98,8 @@ static __poll_t iio_event_poll(struct file *filep,
 			     struct poll_table_struct *wait)
 {
 	struct iio_dev *indio_dev = filep->private_data;
-	struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 	__poll_t events = 0;
 
 	if (!indio_dev->info)
@@ -116,7 +119,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
 				     loff_t *f_ps)
 {
 	struct iio_dev *indio_dev = filep->private_data;
-	struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 	unsigned int copied;
 	int ret;
 
@@ -165,7 +169,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
 static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
 {
 	struct iio_dev *indio_dev = filep->private_data;
-	struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 
 	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
 
@@ -184,7 +189,8 @@ static const struct file_operations iio_event_chrdev_fileops = {
 
 int iio_event_getfd(struct iio_dev *indio_dev)
 {
-	struct iio_event_interface *ev_int = indio_dev->event_interface;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
 	int fd;
 
 	if (ev_int == NULL)
@@ -343,6 +349,7 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
 	enum iio_event_type type, enum iio_event_direction dir,
 	enum iio_shared_by shared_by, const unsigned long *mask)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	ssize_t (*show)(struct device *, struct device_attribute *, char *);
 	ssize_t (*store)(struct device *, struct device_attribute *,
 		const char *, size_t);
@@ -376,7 +383,7 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
 
 		ret = __iio_add_chan_devattr(postfix, chan, show, store,
 			 (i << 16) | spec_index, shared_by, &indio_dev->dev,
-			&indio_dev->event_interface->dev_attr_list);
+			&iio_dev_opaque->event_interface->dev_attr_list);
 		kfree(postfix);
 
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
@@ -469,6 +476,7 @@ static void iio_setup_ev_int(struct iio_event_interface *ev_int)
 static const char *iio_event_group_name = "events";
 int iio_device_register_eventset(struct iio_dev *indio_dev)
 {
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 	struct iio_dev_attr *p;
 	int ret = 0, attrcount_orig = 0, attrcount, attrn;
 	struct attribute **attr;
@@ -477,14 +485,14 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
 	      iio_check_for_dynamic_events(indio_dev)))
 		return 0;
 
-	indio_dev->event_interface =
+	iio_dev_opaque->event_interface =
 		kzalloc(sizeof(struct iio_event_interface), GFP_KERNEL);
-	if (indio_dev->event_interface == NULL)
+	if (iio_dev_opaque->event_interface == NULL)
 		return -ENOMEM;
 
-	INIT_LIST_HEAD(&indio_dev->event_interface->dev_attr_list);
+	INIT_LIST_HEAD(&iio_dev_opaque->event_interface->dev_attr_list);
 
-	iio_setup_ev_int(indio_dev->event_interface);
+	iio_setup_ev_int(iio_dev_opaque->event_interface);
 	if (indio_dev->info->event_attrs != NULL) {
 		attr = indio_dev->info->event_attrs->attrs;
 		while (*attr++ != NULL)
@@ -498,35 +506,35 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
 		attrcount += ret;
 	}
 
-	indio_dev->event_interface->group.name = iio_event_group_name;
-	indio_dev->event_interface->group.attrs = kcalloc(attrcount + 1,
-							  sizeof(indio_dev->event_interface->group.attrs[0]),
+	iio_dev_opaque->event_interface->group.name = iio_event_group_name;
+	iio_dev_opaque->event_interface->group.attrs = kcalloc(attrcount + 1,
+							  sizeof(iio_dev_opaque->event_interface->group.attrs[0]),
 							  GFP_KERNEL);
-	if (indio_dev->event_interface->group.attrs == NULL) {
+	if (iio_dev_opaque->event_interface->group.attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_free_setup_event_lines;
 	}
 	if (indio_dev->info->event_attrs)
-		memcpy(indio_dev->event_interface->group.attrs,
+		memcpy(iio_dev_opaque->event_interface->group.attrs,
 		       indio_dev->info->event_attrs->attrs,
-		       sizeof(indio_dev->event_interface->group.attrs[0])
+		       sizeof(iio_dev_opaque->event_interface->group.attrs[0])
 		       *attrcount_orig);
 	attrn = attrcount_orig;
 	/* Add all elements from the list. */
 	list_for_each_entry(p,
-			    &indio_dev->event_interface->dev_attr_list,
+			    &iio_dev_opaque->event_interface->dev_attr_list,
 			    l)
-		indio_dev->event_interface->group.attrs[attrn++] =
+		iio_dev_opaque->event_interface->group.attrs[attrn++] =
 			&p->dev_attr.attr;
 	indio_dev->groups[indio_dev->groupcounter++] =
-		&indio_dev->event_interface->group;
+		&iio_dev_opaque->event_interface->group;
 
 	return 0;
 
 error_free_setup_event_lines:
-	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
-	kfree(indio_dev->event_interface);
-	indio_dev->event_interface = NULL;
+	iio_free_chan_devattr_list(&iio_dev_opaque->event_interface->dev_attr_list);
+	kfree(iio_dev_opaque->event_interface);
+	iio_dev_opaque->event_interface = NULL;
 	return ret;
 }
 
@@ -539,16 +547,20 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
  */
 void iio_device_wakeup_eventset(struct iio_dev *indio_dev)
 {
-	if (indio_dev->event_interface == NULL)
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	if (iio_dev_opaque->event_interface == NULL)
 		return;
-	wake_up(&indio_dev->event_interface->wait);
+	wake_up(&iio_dev_opaque->event_interface->wait);
 }
 
 void iio_device_unregister_eventset(struct iio_dev *indio_dev)
 {
-	if (indio_dev->event_interface == NULL)
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	if (iio_dev_opaque->event_interface == NULL)
 		return;
-	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
-	kfree(indio_dev->event_interface->group.attrs);
-	kfree(indio_dev->event_interface);
+	iio_free_chan_devattr_list(&iio_dev_opaque->event_interface->dev_attr_list);
+	kfree(iio_dev_opaque->event_interface->group.attrs);
+	kfree(iio_dev_opaque->event_interface);
 }
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index af6c69a40169..f2e94196d31f 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -6,6 +6,7 @@
 /**
  * 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
  * @buffer_list:		list of all buffers currently attached
  * @channel_attr_list:		keep track of automatically created channel
  *				attributes
@@ -17,6 +18,7 @@
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+	struct iio_event_interface	*event_interface;
 	struct list_head		buffer_list;
 	struct list_head		channel_attr_list;
 	struct attribute_group		chan_attr_group;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 219847f6f5c6..e2df67a3b9ab 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -488,7 +488,6 @@ struct iio_buffer_setup_ops {
  * @currentmode:	[DRIVER] current operating mode
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
- * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
  * @buffer:		[DRIVER] any buffer present
  * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
  * @mlock:		[INTERN] lock used to prevent simultaneous device state
@@ -527,8 +526,6 @@ struct iio_dev {
 	int				currentmode;
 	struct device			dev;
 
-	struct iio_event_interface	*event_interface;
-
 	struct iio_buffer		*buffer;
 	int				scan_bytes;
 	struct mutex			mlock;
-- 
2.17.1

Re: [PATCH v3 1/7] iio: core: remove iio_priv_to_dev() helper

[Jonathan Cameron]

On Sun, 21 Jun 2020 15:33:39 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> All users of this helper have been updated to not use it.
> Remove it now, so that we don't need to move it when creating the
> iio_dev_opaque structure.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied to the togreg branch of iio.git and pushed out as testing
(note I discovered I'd not applied the at91-sama5d patch when I said I had
so noticed when doing local tests on this).

Thanks,

Jonathan

> ---
>  include/linux/iio/iio.h | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 1c1d02107722..10a6d97a8e3e 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -703,12 +703,6 @@ static inline void *iio_priv(const struct iio_dev *indio_dev)
>  	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
>  }
>  
> -static inline struct iio_dev *iio_priv_to_dev(void *priv)
> -{
> -	return (struct iio_dev *)((char *)priv -
> -				  ALIGN(sizeof(struct iio_dev), IIO_ALIGN));
> -}
> -
>  void iio_device_free(struct iio_dev *indio_dev);
>  struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
>  struct iio_trigger *devm_iio_trigger_alloc(struct device *dev,

Re: [PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object

[Jonathan Cameron]

On Sun, 21 Jun 2020 15:33:40 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> There are plenty of bad designs we want to discourage or not have to review
> manually usually about accessing private (marked as [INTERN]) fields of
> 'struct iio_dev'.
> 
> Sometimes users copy drivers that are not always the best examples.
> 
> A better idea is to hide those fields into the framework.
> For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a public
> 'struct iio_dev' object.
> 
> In the next series, some fields will be moved to this new struct, each with
> it's own rework.
> 
> This rework will not be complete-able for a while, as many fields need some
> drivers to be reworked in order to finalize them (e.g. 'indio_dev->mlock').
> 
> But some fields can already be moved, and in time, all of them may get
> there (in the 'struct iio_dev_opaque' object).
> 
> Since a lot of drivers also call 'iio_priv()', in order to preserve
> fast-paths (where this matters), the public iio_dev object will have a
> 'priv' field that will have the pointer to the private information already
> computed. The reference returned by this field should be guaranteed to be
> cacheline aligned.
> 
> As for the 'iio_priv_to_dev()' helper, this needs to be hidden away. There
> aren't many users of this helper, and arguably drivers shouldn't need to
> use it in any fast-paths, as they can maintain a reference to the IIO
> device.
Dropped this bit as previous patch deleted iio_priv_to_dev :)
> 
> The opaque parts will be moved into the 'include/linux/iio/iio-opaque.h'
> header. Should the hidden information be required for some debugging or
> some special needs, it can be made available via this header.
> Otherwise, only the IIO core files should include this file.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

I've applied this as it stands, but I wonder if we should combine
this with the existing iio-core.h header.

Can do that later if it makes sense.

Jonathan



> ---
>  drivers/iio/industrialio-core.c | 19 +++++++++++++------
>  include/linux/iio/iio-opaque.h  | 17 +++++++++++++++++
>  include/linux/iio/iio.h         |  6 +++++-
>  3 files changed, 35 insertions(+), 7 deletions(-)
>  create mode 100644 include/linux/iio/iio-opaque.h
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 75661661aaba..33e2953cf021 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/mutex.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/iio-opaque.h>
>  #include "iio_core.h"
>  #include "iio_core_trigger.h"
>  #include <linux/iio/sysfs.h>
> @@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct iio_dev *indio_dev)
>  static void iio_dev_release(struct device *device)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(device);
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
>  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
>  		iio_device_unregister_trigger_consumer(indio_dev);
>  	iio_device_unregister_eventset(indio_dev);
> @@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device *device)
>  	iio_buffer_put(indio_dev->buffer);
>  
>  	ida_simple_remove(&iio_ida, indio_dev->id);
> -	kfree(indio_dev);
> +	kfree(iio_dev_opaque);
>  }
>  
>  struct device_type iio_device_type = {
> @@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
>   **/
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque;
>  	struct iio_dev *dev;
>  	size_t alloc_size;
>  
> -	alloc_size = sizeof(struct iio_dev);
> +	alloc_size = sizeof(struct iio_dev_opaque);
>  	if (sizeof_priv) {
>  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
>  		alloc_size += sizeof_priv;
> @@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  	/* ensure 32-byte alignment of whole construct ? */
>  	alloc_size += IIO_ALIGN - 1;
>  
> -	dev = kzalloc(alloc_size, GFP_KERNEL);
> -	if (!dev)
> +	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
> +	if (!iio_dev_opaque)
>  		return NULL;
>  
> -	dev->dev.parent = parent;
> +	dev = &iio_dev_opaque->indio_dev;
> +	dev->priv = (char *)iio_dev_opaque +
> +		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> +
>  	dev->dev.groups = dev->groups;
>  	dev->dev.type = &iio_device_type;
>  	dev->dev.bus = &iio_bus_type;
> @@ -1524,7 +1531,7 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  	if (dev->id < 0) {
>  		/* cannot use a dev_err as the name isn't available */
>  		pr_err("failed to get device id\n");
> -		kfree(dev);
> +		kfree(iio_dev_opaque);
>  		return NULL;
>  	}
>  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> new file mode 100644
> index 000000000000..1375674f14cd
> --- /dev/null
> +++ b/include/linux/iio/iio-opaque.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _INDUSTRIAL_IO_OPAQUE_H_
> +#define _INDUSTRIAL_IO_OPAQUE_H_
> +
> +/**
> + * struct iio_dev_opaque - industrial I/O device opaque information
> + * @indio_dev:			public industrial I/O device information
> + */
> +struct iio_dev_opaque {
> +	struct iio_dev			indio_dev;
> +};
> +
> +#define to_iio_dev_opaque(indio_dev)		\
> +	container_of(indio_dev, struct iio_dev_opaque, indio_dev)
> +
> +#endif
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 10a6d97a8e3e..86112e35ae5f 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -522,6 +522,8 @@ struct iio_buffer_setup_ops {
>   * @flags:		[INTERN] file ops related flags including busy flag.
>   * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
>   * @cached_reg_addr:	[INTERN] cached register address for debugfs reads.
> + * @priv:		[DRIVER] reference to driver's private information
> + *			**MUST** be accessed **ONLY** via iio_priv() helper
>   */
>  struct iio_dev {
>  	int				id;
> @@ -571,6 +573,7 @@ struct iio_dev {
>  	char				read_buf[20];
>  	unsigned int			read_buf_len;
>  #endif
> +	void				*priv;
>  };
>  
>  const struct iio_chan_spec
> @@ -698,9 +701,10 @@ static inline void *iio_device_get_drvdata(const struct iio_dev *indio_dev)
>  #define IIO_ALIGN L1_CACHE_BYTES
>  struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv);
>  
> +/* The information at the returned address is guaranteed to be cacheline aligned */
>  static inline void *iio_priv(const struct iio_dev *indio_dev)
>  {
> -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev), IIO_ALIGN);
> +	return indio_dev->priv;
>  }
>  
>  void iio_device_free(struct iio_dev *indio_dev);

Re: [PATCH v3 3/7] iio: core: remove padding from private information

[Jonathan Cameron]

On Sun, 21 Jun 2020 15:33:41 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> There was a recent discussion about this code:
>   https://lore.kernel.org/linux-iio/20200322165317.0b1f0674@archlinux/
> 
> This looks like a good time to removed this, since any issues about it
> should pop-up under testing, because the iio_dev is having a bit of an
> overhaul and stuff being moved to iio_dev_opaque.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
*crosses fingers*

Applied to the togreg branch of iio.git and pushed out as testing.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 33e2953cf021..27005ba4d09c 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1507,8 +1507,6 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
>  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
>  		alloc_size += sizeof_priv;
>  	}
> -	/* ensure 32-byte alignment of whole construct ? */
> -	alloc_size += IIO_ALIGN - 1;
>  
>  	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
>  	if (!iio_dev_opaque)

Re: [PATCH v3 7/7] iio: core: move event interface on the opaque struct

[Jonathan Cameron]

On Sun, 21 Jun 2020 15:33:45 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Same as with other private fields, this moves the event interface reference
> to the opaque IIO device object, to be invisible to drivers.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Whole series applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it and see what we missed.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c  |  5 ++-
>  drivers/iio/industrialio-event.c | 68 +++++++++++++++++++-------------
>  include/linux/iio/iio-opaque.h   |  2 +
>  include/linux/iio/iio.h          |  3 --
>  4 files changed, 45 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 051cb05a8da7..12550f59caa4 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -205,7 +205,8 @@ EXPORT_SYMBOL(iio_read_const_attr);
>  int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
>  {
>  	int ret;
> -	const struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	const struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  
>  	ret = mutex_lock_interruptible(&indio_dev->mlock);
>  	if (ret)
> @@ -1436,7 +1437,7 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  			attrcount += ret;
>  		}
>  
> -	if (indio_dev->event_interface)
> +	if (iio_dev_opaque->event_interface)
>  		clk = &dev_attr_current_timestamp_clock.attr;
>  
>  	if (indio_dev->name)
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 5b17c92d3b50..2ab4d4c44427 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -18,6 +18,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/wait.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/iio-opaque.h>
>  #include "iio_core.h"
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/events.h>
> @@ -62,7 +63,8 @@ bool iio_event_enabled(const struct iio_event_interface *ev_int)
>   **/
>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp)
>  {
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  	struct iio_event_data ev;
>  	int copied;
>  
> @@ -96,7 +98,8 @@ static __poll_t iio_event_poll(struct file *filep,
>  			     struct poll_table_struct *wait)
>  {
>  	struct iio_dev *indio_dev = filep->private_data;
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  	__poll_t events = 0;
>  
>  	if (!indio_dev->info)
> @@ -116,7 +119,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  				     loff_t *f_ps)
>  {
>  	struct iio_dev *indio_dev = filep->private_data;
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  	unsigned int copied;
>  	int ret;
>  
> @@ -165,7 +169,8 @@ static ssize_t iio_event_chrdev_read(struct file *filep,
>  static int iio_event_chrdev_release(struct inode *inode, struct file *filep)
>  {
>  	struct iio_dev *indio_dev = filep->private_data;
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  
>  	clear_bit(IIO_BUSY_BIT_POS, &ev_int->flags);
>  
> @@ -184,7 +189,8 @@ static const struct file_operations iio_event_chrdev_fileops = {
>  
>  int iio_event_getfd(struct iio_dev *indio_dev)
>  {
> -	struct iio_event_interface *ev_int = indio_dev->event_interface;
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +	struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
>  	int fd;
>  
>  	if (ev_int == NULL)
> @@ -343,6 +349,7 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
>  	enum iio_event_type type, enum iio_event_direction dir,
>  	enum iio_shared_by shared_by, const unsigned long *mask)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	ssize_t (*show)(struct device *, struct device_attribute *, char *);
>  	ssize_t (*store)(struct device *, struct device_attribute *,
>  		const char *, size_t);
> @@ -376,7 +383,7 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
>  
>  		ret = __iio_add_chan_devattr(postfix, chan, show, store,
>  			 (i << 16) | spec_index, shared_by, &indio_dev->dev,
> -			&indio_dev->event_interface->dev_attr_list);
> +			&iio_dev_opaque->event_interface->dev_attr_list);
>  		kfree(postfix);
>  
>  		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
> @@ -469,6 +476,7 @@ static void iio_setup_ev_int(struct iio_event_interface *ev_int)
>  static const char *iio_event_group_name = "events";
>  int iio_device_register_eventset(struct iio_dev *indio_dev)
>  {
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  	struct iio_dev_attr *p;
>  	int ret = 0, attrcount_orig = 0, attrcount, attrn;
>  	struct attribute **attr;
> @@ -477,14 +485,14 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
>  	      iio_check_for_dynamic_events(indio_dev)))
>  		return 0;
>  
> -	indio_dev->event_interface =
> +	iio_dev_opaque->event_interface =
>  		kzalloc(sizeof(struct iio_event_interface), GFP_KERNEL);
> -	if (indio_dev->event_interface == NULL)
> +	if (iio_dev_opaque->event_interface == NULL)
>  		return -ENOMEM;
>  
> -	INIT_LIST_HEAD(&indio_dev->event_interface->dev_attr_list);
> +	INIT_LIST_HEAD(&iio_dev_opaque->event_interface->dev_attr_list);
>  
> -	iio_setup_ev_int(indio_dev->event_interface);
> +	iio_setup_ev_int(iio_dev_opaque->event_interface);
>  	if (indio_dev->info->event_attrs != NULL) {
>  		attr = indio_dev->info->event_attrs->attrs;
>  		while (*attr++ != NULL)
> @@ -498,35 +506,35 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
>  		attrcount += ret;
>  	}
>  
> -	indio_dev->event_interface->group.name = iio_event_group_name;
> -	indio_dev->event_interface->group.attrs = kcalloc(attrcount + 1,
> -							  sizeof(indio_dev->event_interface->group.attrs[0]),
> +	iio_dev_opaque->event_interface->group.name = iio_event_group_name;
> +	iio_dev_opaque->event_interface->group.attrs = kcalloc(attrcount + 1,
> +							  sizeof(iio_dev_opaque->event_interface->group.attrs[0]),
>  							  GFP_KERNEL);
> -	if (indio_dev->event_interface->group.attrs == NULL) {
> +	if (iio_dev_opaque->event_interface->group.attrs == NULL) {
>  		ret = -ENOMEM;
>  		goto error_free_setup_event_lines;
>  	}
>  	if (indio_dev->info->event_attrs)
> -		memcpy(indio_dev->event_interface->group.attrs,
> +		memcpy(iio_dev_opaque->event_interface->group.attrs,
>  		       indio_dev->info->event_attrs->attrs,
> -		       sizeof(indio_dev->event_interface->group.attrs[0])
> +		       sizeof(iio_dev_opaque->event_interface->group.attrs[0])
>  		       *attrcount_orig);
>  	attrn = attrcount_orig;
>  	/* Add all elements from the list. */
>  	list_for_each_entry(p,
> -			    &indio_dev->event_interface->dev_attr_list,
> +			    &iio_dev_opaque->event_interface->dev_attr_list,
>  			    l)
> -		indio_dev->event_interface->group.attrs[attrn++] =
> +		iio_dev_opaque->event_interface->group.attrs[attrn++] =
>  			&p->dev_attr.attr;
>  	indio_dev->groups[indio_dev->groupcounter++] =
> -		&indio_dev->event_interface->group;
> +		&iio_dev_opaque->event_interface->group;
>  
>  	return 0;
>  
>  error_free_setup_event_lines:
> -	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
> -	kfree(indio_dev->event_interface);
> -	indio_dev->event_interface = NULL;
> +	iio_free_chan_devattr_list(&iio_dev_opaque->event_interface->dev_attr_list);
> +	kfree(iio_dev_opaque->event_interface);
> +	iio_dev_opaque->event_interface = NULL;
>  	return ret;
>  }
>  
> @@ -539,16 +547,20 @@ int iio_device_register_eventset(struct iio_dev *indio_dev)
>   */
>  void iio_device_wakeup_eventset(struct iio_dev *indio_dev)
>  {
> -	if (indio_dev->event_interface == NULL)
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +	if (iio_dev_opaque->event_interface == NULL)
>  		return;
> -	wake_up(&indio_dev->event_interface->wait);
> +	wake_up(&iio_dev_opaque->event_interface->wait);
>  }
>  
>  void iio_device_unregister_eventset(struct iio_dev *indio_dev)
>  {
> -	if (indio_dev->event_interface == NULL)
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> +
> +	if (iio_dev_opaque->event_interface == NULL)
>  		return;
> -	iio_free_chan_devattr_list(&indio_dev->event_interface->dev_attr_list);
> -	kfree(indio_dev->event_interface->group.attrs);
> -	kfree(indio_dev->event_interface);
> +	iio_free_chan_devattr_list(&iio_dev_opaque->event_interface->dev_attr_list);
> +	kfree(iio_dev_opaque->event_interface->group.attrs);
> +	kfree(iio_dev_opaque->event_interface);
>  }
> diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
> index af6c69a40169..f2e94196d31f 100644
> --- a/include/linux/iio/iio-opaque.h
> +++ b/include/linux/iio/iio-opaque.h
> @@ -6,6 +6,7 @@
>  /**
>   * 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
>   * @buffer_list:		list of all buffers currently attached
>   * @channel_attr_list:		keep track of automatically created channel
>   *				attributes
> @@ -17,6 +18,7 @@
>   */
>  struct iio_dev_opaque {
>  	struct iio_dev			indio_dev;
> +	struct iio_event_interface	*event_interface;
>  	struct list_head		buffer_list;
>  	struct list_head		channel_attr_list;
>  	struct attribute_group		chan_attr_group;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 219847f6f5c6..e2df67a3b9ab 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -488,7 +488,6 @@ struct iio_buffer_setup_ops {
>   * @currentmode:	[DRIVER] current operating mode
>   * @dev:		[DRIVER] device structure, should be assigned a parent
>   *			and owner
> - * @event_interface:	[INTERN] event chrdevs associated with interrupt lines
>   * @buffer:		[DRIVER] any buffer present
>   * @scan_bytes:		[INTERN] num bytes captured to be fed to buffer demux
>   * @mlock:		[INTERN] lock used to prevent simultaneous device state
> @@ -527,8 +526,6 @@ struct iio_dev {
>  	int				currentmode;
>  	struct device			dev;
>  
> -	struct iio_event_interface	*event_interface;
> -
>  	struct iio_buffer		*buffer;
>  	int				scan_bytes;
>  	struct mutex			mlock;

Re: [PATCH v3 2/7] iio: core: wrap IIO device into an iio_dev_opaque object

[Ardelean, Alexandru]

On Sat, 2020-06-27 at 17:40 +0100, Jonathan Cameron wrote:
> On Sun, 21 Jun 2020 15:33:40 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > There are plenty of bad designs we want to discourage or not have to
> > review
> > manually usually about accessing private (marked as [INTERN]) fields of
> > 'struct iio_dev'.
> > 
> > Sometimes users copy drivers that are not always the best examples.
> > 
> > A better idea is to hide those fields into the framework.
> > For 'struct iio_dev' this is a 'struct iio_dev_opaque' which wraps a
> > public
> > 'struct iio_dev' object.
> > 
> > In the next series, some fields will be moved to this new struct, each
> > with
> > it's own rework.
> > 
> > This rework will not be complete-able for a while, as many fields need
> > some
> > drivers to be reworked in order to finalize them (e.g. 'indio_dev-
> > >mlock').
> > 
> > But some fields can already be moved, and in time, all of them may get
> > there (in the 'struct iio_dev_opaque' object).
> > 
> > Since a lot of drivers also call 'iio_priv()', in order to preserve
> > fast-paths (where this matters), the public iio_dev object will have a
> > 'priv' field that will have the pointer to the private information
> > already
> > computed. The reference returned by this field should be guaranteed to
> > be
> > cacheline aligned.
> > 
> > As for the 'iio_priv_to_dev()' helper, this needs to be hidden away.
> > There
> > aren't many users of this helper, and arguably drivers shouldn't need
> > to
> > use it in any fast-paths, as they can maintain a reference to the IIO
> > device.
> Dropped this bit as previous patch deleted iio_priv_to_dev :)

Right.
My bad.

> > The opaque parts will be moved into the 'include/linux/iio/iio-
> > opaque.h'
> > header. Should the hidden information be required for some debugging or
> > some special needs, it can be made available via this header.
> > Otherwise, only the IIO core files should include this file.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> I've applied this as it stands, but I wonder if we should combine
> this with the existing iio-core.h  header.
> 
> Can do that later if it makes sense.

Initially, I had thought about putting all of the iio-opaque.h code in iio-
core.h, and limit the include range in drivers/iio, but then thought maybe
leave it in 'include/linux/iio/iio-opaque.h'. That way, it's probably a bit
easier for some debugging.
I am still not sure which direction is best.
But, we can move iio-core.h into iio-opaque.h (or the other way around).

> 
> Jonathan
> 
> 
> 
> > ---
> >  drivers/iio/industrialio-core.c | 19 +++++++++++++------
> >  include/linux/iio/iio-opaque.h  | 17 +++++++++++++++++
> >  include/linux/iio/iio.h         |  6 +++++-
> >  3 files changed, 35 insertions(+), 7 deletions(-)
> >  create mode 100644 include/linux/iio/iio-opaque.h
> > 
> > diff --git a/drivers/iio/industrialio-core.c
> > b/drivers/iio/industrialio-core.c
> > index 75661661aaba..33e2953cf021 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/mutex.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/iio-opaque.h>
> >  #include "iio_core.h"
> >  #include "iio_core_trigger.h"
> >  #include <linux/iio/sysfs.h>
> > @@ -1473,6 +1474,8 @@ static void iio_device_unregister_sysfs(struct
> > iio_dev *indio_dev)
> >  static void iio_dev_release(struct device *device)
> >  {
> >  	struct iio_dev *indio_dev = dev_to_iio_dev(device);
> > +	struct iio_dev_opaque *iio_dev_opaque =
> > to_iio_dev_opaque(indio_dev);
> > +
> >  	if (indio_dev->modes & INDIO_ALL_TRIGGERED_MODES)
> >  		iio_device_unregister_trigger_consumer(indio_dev);
> >  	iio_device_unregister_eventset(indio_dev);
> > @@ -1481,7 +1484,7 @@ static void iio_dev_release(struct device
> > *device)
> >  	iio_buffer_put(indio_dev->buffer);
> >  
> >  	ida_simple_remove(&iio_ida, indio_dev->id);
> > -	kfree(indio_dev);
> > +	kfree(iio_dev_opaque);
> >  }
> >  
> >  struct device_type iio_device_type = {
> > @@ -1495,10 +1498,11 @@ struct device_type iio_device_type = {
> >   **/
> >  struct iio_dev *iio_device_alloc(struct device *parent, int
> > sizeof_priv)
> >  {
> > +	struct iio_dev_opaque *iio_dev_opaque;
> >  	struct iio_dev *dev;
> >  	size_t alloc_size;
> >  
> > -	alloc_size = sizeof(struct iio_dev);
> > +	alloc_size = sizeof(struct iio_dev_opaque);
> >  	if (sizeof_priv) {
> >  		alloc_size = ALIGN(alloc_size, IIO_ALIGN);
> >  		alloc_size += sizeof_priv;
> > @@ -1506,11 +1510,14 @@ struct iio_dev *iio_device_alloc(struct device
> > *parent, int sizeof_priv)
> >  	/* ensure 32-byte alignment of whole construct ? */
> >  	alloc_size += IIO_ALIGN - 1;
> >  
> > -	dev = kzalloc(alloc_size, GFP_KERNEL);
> > -	if (!dev)
> > +	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
> > +	if (!iio_dev_opaque)
> >  		return NULL;
> >  
> > -	dev->dev.parent = parent;
> > +	dev = &iio_dev_opaque->indio_dev;
> > +	dev->priv = (char *)iio_dev_opaque +
> > +		ALIGN(sizeof(struct iio_dev_opaque), IIO_ALIGN);
> > +
> >  	dev->dev.groups = dev->groups;
> >  	dev->dev.type = &iio_device_type;
> >  	dev->dev.bus = &iio_bus_type;
> > @@ -1524,7 +1531,7 @@ struct iio_dev *iio_device_alloc(struct device
> > *parent, int sizeof_priv)
> >  	if (dev->id < 0) {
> >  		/* cannot use a dev_err as the name isn't available */
> >  		pr_err("failed to get device id\n");
> > -		kfree(dev);
> > +		kfree(iio_dev_opaque);
> >  		return NULL;
> >  	}
> >  	dev_set_name(&dev->dev, "iio:device%d", dev->id);
> > diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-
> > opaque.h
> > new file mode 100644
> > index 000000000000..1375674f14cd
> > --- /dev/null
> > +++ b/include/linux/iio/iio-opaque.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _INDUSTRIAL_IO_OPAQUE_H_
> > +#define _INDUSTRIAL_IO_OPAQUE_H_
> > +
> > +/**
> > + * struct iio_dev_opaque - industrial I/O device opaque information
> > + * @indio_dev:			public industrial I/O device
> > information
> > + */
> > +struct iio_dev_opaque {
> > +	struct iio_dev			indio_dev;
> > +};
> > +
> > +#define to_iio_dev_opaque(indio_dev)		\
> > +	container_of(indio_dev, struct iio_dev_opaque, indio_dev)
> > +
> > +#endif
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 10a6d97a8e3e..86112e35ae5f 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -522,6 +522,8 @@ struct iio_buffer_setup_ops {
> >   * @flags:		[INTERN] file ops related flags including busy
> > flag.
> >   * @debugfs_dentry:	[INTERN] device specific debugfs dentry.
> >   * @cached_reg_addr:	[INTERN] cached register address for
> > debugfs reads.
> > + * @priv:		[DRIVER] reference to driver's private information
> > + *			**MUST** be accessed **ONLY** via iio_priv() helper
> >   */
> >  struct iio_dev {
> >  	int				id;
> > @@ -571,6 +573,7 @@ struct iio_dev {
> >  	char				read_buf[20];
> >  	unsigned int			read_buf_len;
> >  #endif
> > +	void				*priv;
> >  };
> >  
> >  const struct iio_chan_spec
> > @@ -698,9 +701,10 @@ static inline void *iio_device_get_drvdata(const
> > struct iio_dev *indio_dev)
> >  #define IIO_ALIGN L1_CACHE_BYTES
> >  struct iio_dev *iio_device_alloc(struct device *parent, int
> > sizeof_priv);
> >  
> > +/* The information at the returned address is guaranteed to be
> > cacheline aligned */
> >  static inline void *iio_priv(const struct iio_dev *indio_dev)
> >  {
> > -	return (char *)indio_dev + ALIGN(sizeof(struct iio_dev),
> > IIO_ALIGN);
> > +	return indio_dev->priv;
> >  }
> >  
> >  void iio_device_free(struct iio_dev *indio_dev);