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

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

Changelog v3 -> v4:
* added ifdef guard for iio_get_debugfs_dentry();
  Reported-by: kernel test robot <lkp@intel.com>

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   | 120 +++++++++++++++++++-----------
 drivers/iio/industrialio-event.c  |  68 ++++++++++-------
 include/linux/iio/iio-opaque.h    |  36 +++++++++
 include/linux/iio/iio.h           |  35 ++-------
 5 files changed, 182 insertions(+), 115 deletions(-)
 create mode 100644 include/linux/iio/iio-opaque.h

-- 
2.17.1

[PATCH v4 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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 v4 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.

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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 v4 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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 v4 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.

The iio_get_debugfs_dentry() function (which is moved in
industrialio-core.c) needs to also be guarded against the CONFIG_DEBUG_FS
symbol, when it isn't defined. We do want to keep the inline definition in
the iio.h header, so that the compiler can better infer when to compile out
debugfs code that is related to the IIO debugfs directory.

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

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 27005ba4d09c..64174052641a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -165,6 +165,19 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
 };
 
+#if !defined(CONFIG_DEBUG_FS)
+/**
+ * There's also a CONFIG_DEBUG_FS guard in include/linux/iio/iio.h for
+ * iio_get_debugfs_dentry() to make it inline if CONFIG_DEBUG_FS is undefined
+ */
+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);
+#endif
+
 /**
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
@@ -308,35 +321,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 +366,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 +393,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 v4 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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 64174052641a..21fbb187ee79 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1137,6 +1137,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) {
@@ -1149,7 +1150,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)
@@ -1165,6 +1166,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;
 
@@ -1184,7 +1186,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;
@@ -1199,6 +1201,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;
 
@@ -1274,7 +1277,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;
@@ -1409,6 +1412,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;
@@ -1448,47 +1452,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)
@@ -1543,7 +1549,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 v4 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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 21fbb187ee79..461a4e7f48d7 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1559,7 +1559,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 v4 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>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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 461a4e7f48d7..a0ad152c82a7 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -211,7 +211,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)
@@ -1442,7 +1443,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 v4 4/7] iio: core: move debugfs data on the private iio dev info

[Ardelean, Alexandru]

On Tue, 2020-06-30 at 07:57 +0300, Alexandru Ardelean wrote:
> 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.
> 
> The iio_get_debugfs_dentry() function (which is moved in
> industrialio-core.c) needs to also be guarded against the CONFIG_DEBUG_FS
> symbol, when it isn't defined. We do want to keep the inline definition
> in
> the iio.h header, so that the compiler can better infer when to compile
> out
> debugfs code that is related to the IIO debugfs directory.
> 

Well, pretty much only this patch changed since V3.
I thought about maybe re-doing just this patch, then I thought maybe I'd
get a minor complaint that I should re-send the series.

Either way, I prefer a complaint on this V4 series-re-send than if I were
to have re-sent just this patch.


> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 46 +++++++++++++++++++++++----------
>  include/linux/iio/iio-opaque.h  | 10 +++++++
>  include/linux/iio/iio.h         | 13 +---------
>  3 files changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> core.c
> index 27005ba4d09c..64174052641a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -165,6 +165,19 @@ static const char * const iio_chan_info_postfix[] =
> {
>  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
>  };
>  
> +#if !defined(CONFIG_DEBUG_FS)
> +/**
> + * There's also a CONFIG_DEBUG_FS guard in include/linux/iio/iio.h for
> + * iio_get_debugfs_dentry() to make it inline if CONFIG_DEBUG_FS is
> undefined
> + */
> +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);
> +#endif
> +
>  /**
>   * iio_find_channel_from_si() - get channel from its scan index
>   * @indio_dev:		device
> @@ -308,35 +321,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 +366,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 +393,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)
>  {

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

[Jonathan Cameron]

On Tue, 30 Jun 2020 04:58:06 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Tue, 2020-06-30 at 07:57 +0300, Alexandru Ardelean wrote:
> > 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.
> > 
> > The iio_get_debugfs_dentry() function (which is moved in
> > industrialio-core.c) needs to also be guarded against the CONFIG_DEBUG_FS
> > symbol, when it isn't defined. We do want to keep the inline definition
> > in
> > the iio.h header, so that the compiler can better infer when to compile
> > out
> > debugfs code that is related to the IIO debugfs directory.
> >   
> 
> Well, pretty much only this patch changed since V3.
> I thought about maybe re-doing just this patch, then I thought maybe I'd
> get a minor complaint that I should re-send the series.
> 
> Either way, I prefer a complaint on this V4 series-re-send than if I were
> to have re-sent just this patch.

Either way worked.

However this doesn't pass my basic build test. Config condition
is reversed. 

Fixed up and pushed out as testing.


Jonathan

> 
> 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > ---
> >  drivers/iio/industrialio-core.c | 46 +++++++++++++++++++++++----------
> >  include/linux/iio/iio-opaque.h  | 10 +++++++
> >  include/linux/iio/iio.h         | 13 +---------
> >  3 files changed, 44 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-
> > core.c
> > index 27005ba4d09c..64174052641a 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -165,6 +165,19 @@ static const char * const iio_chan_info_postfix[] =
> > {
> >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> >  };
> >  
> > +#if !defined(CONFIG_DEBUG_FS)

Don't we want this if it is defined.

> > +/**
> > + * There's also a CONFIG_DEBUG_FS guard in include/linux/iio/iio.h for
> > + * iio_get_debugfs_dentry() to make it inline if CONFIG_DEBUG_FS is
> > undefined
> > + */
> > +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);
> > +#endif
> > +
> >  /**
> >   * iio_find_channel_from_si() - get channel from its scan index
> >   * @indio_dev:		device
> > @@ -308,35 +321,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 +366,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 +393,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)
> >  {  

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

[Ardelean, Alexandru]

On Wed, 2020-07-01 at 19:42 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Tue, 30 Jun 2020 04:58:06 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> 
> > On Tue, 2020-06-30 at 07:57 +0300, Alexandru Ardelean wrote:
> > > 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.
> > > 
> > > The iio_get_debugfs_dentry() function (which is moved in
> > > industrialio-core.c) needs to also be guarded against the
> > > CONFIG_DEBUG_FS
> > > symbol, when it isn't defined. We do want to keep the inline
> > > definition
> > > in
> > > the iio.h header, so that the compiler can better infer when to
> > > compile
> > > out
> > > debugfs code that is related to the IIO debugfs directory.
> > >   
> > 
> > Well, pretty much only this patch changed since V3.
> > I thought about maybe re-doing just this patch, then I thought maybe
> > I'd
> > get a minor complaint that I should re-send the series.
> > 
> > Either way, I prefer a complaint on this V4 series-re-send than if I
> > were
> > to have re-sent just this patch.
> 
> Either way worked.
> 
> However this doesn't pass my basic build test. Config condition
> is reversed. 
> 
> Fixed up and pushed out as testing.

Sorry for the goof.
I tested with make allmodconfig.
Maybe I didn't pay attention somewhere.


> 
> 
> Jonathan
> 
> > 
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > >  drivers/iio/industrialio-core.c | 46 +++++++++++++++++++++++------
> > > ----
> > >  include/linux/iio/iio-opaque.h  | 10 +++++++
> > >  include/linux/iio/iio.h         | 13 +---------
> > >  3 files changed, 44 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/iio/industrialio-core.c
> > > b/drivers/iio/industrialio-
> > > core.c
> > > index 27005ba4d09c..64174052641a 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -165,6 +165,19 @@ static const char * const
> > > iio_chan_info_postfix[] =
> > > {
> > >  	[IIO_CHAN_INFO_THERMOCOUPLE_TYPE] = "thermocouple_type",
> > >  };
> > >  
> > > +#if !defined(CONFIG_DEBUG_FS)
> 
> Don't we want this if it is defined.
> 
> > > +/**
> > > + * There's also a CONFIG_DEBUG_FS guard in include/linux/iio/iio.h
> > > for
> > > + * iio_get_debugfs_dentry() to make it inline if CONFIG_DEBUG_FS is
> > > undefined
> > > + */
> > > +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);
> > > +#endif
> > > +
> > >  /**
> > >   * iio_find_channel_from_si() - get channel from its scan index
> > >   * @indio_dev:		device
> > > @@ -308,35 +321,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 +366,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 +393,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)
> > >  {  

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

[Dmitry Baryshkov]

Hello,

On Tue, Jun 30, 2020 at 07:57:03AM +0300, Alexandru Ardelean 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.

[skipped]

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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

[skipped]

> @@ -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;

This chunk (together with 8cb631ccbb1952b6422917f2ed16f760d84a762e and
d3be83244c7dfe686d23f1c0bac75915587fc044) break devicetree bindings of
IIO clients. After these changes there are no links between devicetree
nodes and corresponding IIO channels. I'd kindly ask to restore this
dev->dev.parent assignment (which restores of node binding).

> +	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;

-- 
With best wishes
Dmitry

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

[Alexandru Ardelean]

On Tue, Jul 21, 2020 at 12:47 PM Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
> Hello,
>
> On Tue, Jun 30, 2020 at 07:57:03AM +0300, Alexandru Ardelean 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.
>
> [skipped]
>
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.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
>
> [skipped]
>
> > @@ -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;
>
> This chunk (together with 8cb631ccbb1952b6422917f2ed16f760d84a762e and
> d3be83244c7dfe686d23f1c0bac75915587fc044) break devicetree bindings of
> IIO clients. After these changes there are no links between devicetree
> nodes and corresponding IIO channels. I'd kindly ask to restore this
> dev->dev.parent assignment (which restores of node binding).
>

Ack.
Will send a fix.
Thanks for catching this.
Apologies for the breakage.
It seems that I was tripping quite a bit with too many patchsets.


> > +     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;
>
> --
> With best wishes
> Dmitry