iio: core: A few code cleanups and documentation fixes

---
 drivers/iio/industrialio-core.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

drivers/iio/industrialio-core.c +24 -8

index c117f50d0cf3..6cca86fd0ef9 100644
@@ -1617,6 +1617,28 @@ const struct device_type iio_device_type = {
 	.release = iio_dev_release,
 };
 
+/**
+ * opaque_struct_size() - Calculate size of opaque structure with trailing aligned data.
+ * @p: Pointer to the opaque structure.
+ * @a: Alignment in bytes before trailing data.
+ * @s: Data size in bytes (can be 0).
+ *
+ * Calculates size of memory needed for structure of @p followed by the aligned data
+ * of size @s. When @s is 0, the alignment @a is not taken into account and the result
+ * is an equivalent to sizeof(*(@p)).
+ *
+ * Returns: Number of bytes needed or SIZE_MAX on overflow.
+ */
+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
+
 /**
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.

@@ -1628,19 +1650,13 @@ struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 	struct iio_dev *indio_dev;
 	size_t alloc_size;
 
-	alloc_size = sizeof(struct iio_dev_opaque);
-	if (sizeof_priv) {
-		alloc_size = ALIGN(alloc_size, IIO_DMA_MINALIGN);
-		alloc_size += sizeof_priv;
-	}
-
+	alloc_size = opaque_struct_size(iio_dev_opaque, IIO_DMA_MINALIGN, sizeof_priv);
 	iio_dev_opaque = kzalloc(alloc_size, GFP_KERNEL);
 	if (!iio_dev_opaque)
 		return NULL;
 
 	indio_dev = &iio_dev_opaque->indio_dev;
-	indio_dev->priv = (char *)iio_dev_opaque +
-		ALIGN(sizeof(struct iio_dev_opaque), IIO_DMA_MINALIGN);
+	indio_dev->priv = opaque_struct_data(iio_dev_opaque, IIO_DMA_MINALIGN);
 
 	indio_dev->dev.parent = parent;
 	indio_dev->dev.type = &iio_device_type;

Signature · 1 line

2.40.0.1.gaa8946217a0b

Andy Shevchenko

author

2023-07-21 17:22 UTC #2 lore raw

Re: [PATCH v2 1/8] iio: core: Add opaque_struct_size() helper and use it

Quoted 2 lines · Andy Shevchenko

Introduce opaque_struct_size() helper, which may be moved to overflow.h in the future, and use it in the IIO core.

...

drivers/iio/industrialio-core.c const struct device_type iio_device_type = { · line 1640

Quoted 9 lines

drivers/iio/industrialio-core.c +9 -0

+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))

Hmm... This can potentially evaluate p twice.

Perhaps this variant is better:

#define opaque_struct_data(p, a) ALIGN((p) + 1, (a)))

Besides, I don't think we should worry about @s evaluation in the main macro which may be simplified to

#define opaque_struct_size(p, a, s)					\
	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))

Thoughts?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2023-07-21 17:26 UTC #3 lore raw

Re: [PATCH v2 1/8] iio: core: Add opaque_struct_size() helper and use it

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Andy Shevchenko
Introduce opaque_struct_size() helper, which may be moved to overflow.h in the future, and use it in the IIO core.

...

Quoted 16 lines

drivers/iio/industrialio-core.c const struct device_type iio_device_type = { · line 1640
Quoted 9 lines
drivers/iio/industrialio-core.c +9 -0
+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
Hmm... This can potentially evaluate p twice.
Perhaps this variant is better:
#define opaque_struct_data(p, a) ALIGN((p) + 1, (a)))

Quoted 7 lines

Besides, I don't think we should worry about @s evaluation in the main macro which may be simplified to
#define opaque_struct_size(p, a, s)					\
	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))
Thoughts?

Also we may leave the struct always be aligned which makes the above even simpler (but might waste bytes if @s = 0).

#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s))

(with the respective documentation update).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2023-07-21 17:27 UTC #4 lore raw

Re: [PATCH v2 1/8] iio: core: Add opaque_struct_size() helper and use it

Quoted 4 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Introduce opaque_struct_size() helper, which may be moved to overflow.h in the future, and use it in the IIO core.

...

Quoted 22 lines

Quoted 22 lines
drivers/iio/industrialio-core.c const struct device_type iio_device_type = { · line 1640
Quoted 9 lines
drivers/iio/industrialio-core.c +9 -0
+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
Hmm... This can potentially evaluate p twice.
Perhaps this variant is better:
#define opaque_struct_data(p, a) ALIGN((p) + 1, (a)))
Besides, I don't think we should worry about @s evaluation in the main macro which may be simplified to
#define opaque_struct_size(p, a, s)					\
	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))

Quoted 8 lines

Quoted 1 line
Thoughts?
Also we may leave the struct always be aligned which makes the above even simpler (but might waste bytes if @s = 0).
#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s))
(with the respective documentation update).

Jonathan, this patch can be skipped and if you are okay with the rest, the rest may be (clearly, I have just checked) applied without as there is no dependency.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Uwe Kleine-König

2023-07-21 18:13 UTC #5 lore raw

Re: [PATCH v2 1/8] iio: core: Add opaque_struct_size() helper and use it

Hello Andy,

Quoted 7 lines · Andy Shevchenko

Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Andy Shevchenko
Introduce opaque_struct_size() helper, which may be moved to overflow.h in the future, and use it in the IIO core.
...

Quoted 11 lines

Quoted 11 lines
drivers/iio/industrialio-core.c const struct device_type iio_device_type = { · line 1640
Quoted 9 lines
drivers/iio/industrialio-core.c +9 -0
+#define opaque_struct_size(p, a, s)	({		\
+	size_t _psize = sizeof(*(p));			\
+	size_t _asize = ALIGN(_psize, (a));		\
+	size_t _ssize = s;				\
+	_ssize ? size_add(_asize, _ssize) : _psize;	\
+})
+
+#define opaque_struct_data(p, a)			\
+	((void *)(p) + ALIGN(sizeof(*(p)), (a)))
Hmm... This can potentially evaluate p twice.

I don't think sizeof evaluates its parameter? It should only defer its type and then calculate the respective size.

Quoted 3 lines

Quoted 3 lines
Perhaps this variant is better:
#define opaque_struct_data(p, a) ALIGN((p) + 1, (a)))

Still I like this better.

Quoted 10 lines

Quoted 7 lines
Besides, I don't think we should worry about @s evaluation in the main macro which may be simplified to
#define opaque_struct_size(p, a, s)					\
	((s) ? size_add(ALIGN(sizeof(*(p)), (a)), (s)) : sizeof(*(p)))
Thoughts?
Also we may leave the struct always be aligned which makes the above even simpler (but might waste bytes if @s = 0).

Depending on the value of a, the bytes "wasted" here might not be saved anyhow because the memory allocator will round up the allocation size to a multiple of some chunk size anyhow.

Quoted 3 lines

#define opaque_struct_size(p, a, s) size_add(ALIGN(sizeof(*(p)), (a)), (s))
(with the respective documentation update).

That would be my favourite.

Possible users of this helper include:

__spi_alloc_controller() in drivers/spi/spi.c alloc_netdev_mqs in net/core/dev.c

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Andy Shevchenko

author

2023-07-21 17:00 UTC #6 lore raw

PATCH v2 2/8 iio: core: Use sysfs_match_string() helper

Use sysfs_match_string() helper instead of open coded variant.

---
 drivers/iio/industrialio-core.c | 81 +++++++++++++--------------------
 1 file changed, 31 insertions(+), 50 deletions(-)

drivers/iio/industrialio-core.c +31 ~~-50~~

index 6cca86fd0ef9..4e45740331ee 100644
@@ -1400,50 +1400,32 @@ static ssize_t label_show(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(label);
 
+static const char * const clock_names[] = {
+	[CLOCK_REALTIME]	 	= "realtime",
+	[CLOCK_MONOTONIC]	 	= "monotonic",
+	[CLOCK_PROCESS_CPUTIME_ID]	= "process_cputime_id",
+	[CLOCK_THREAD_CPUTIME_ID]	= "thread_cputime_id",
+	[CLOCK_MONOTONIC_RAW]	 	= "monotonic_raw",
+	[CLOCK_REALTIME_COARSE]	 	= "realtime_coarse",
+	[CLOCK_MONOTONIC_COARSE] 	= "monotonic_coarse",
+	[CLOCK_BOOTTIME]	 	= "boottime",
+	[CLOCK_REALTIME_ALARM]		= "realtime_alarm",
+	[CLOCK_BOOTTIME_ALARM]		= "boottime_alarm",
+	[CLOCK_SGI_CYCLE]		= "sgi_cycle",
+	[CLOCK_TAI]		 	= "tai",
+};
+
 static ssize_t current_timestamp_clock_show(struct device *dev,
 					    struct device_attribute *attr,
 					    char *buf)
 {
 	const struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	const clockid_t clk = iio_device_get_clock(indio_dev);
-	const char *name;
-	ssize_t sz;
 
-	switch (clk) {
-	case CLOCK_REALTIME:
-		name = "realtime\n";
-		sz = sizeof("realtime\n");
-		break;
-	case CLOCK_MONOTONIC:
-		name = "monotonic\n";
-		sz = sizeof("monotonic\n");
-		break;
-	case CLOCK_MONOTONIC_RAW:
-		name = "monotonic_raw\n";
-		sz = sizeof("monotonic_raw\n");
-		break;
-	case CLOCK_REALTIME_COARSE:
-		name = "realtime_coarse\n";
-		sz = sizeof("realtime_coarse\n");
-		break;
-	case CLOCK_MONOTONIC_COARSE:
-		name = "monotonic_coarse\n";
-		sz = sizeof("monotonic_coarse\n");
-		break;
-	case CLOCK_BOOTTIME:
-		name = "boottime\n";
-		sz = sizeof("boottime\n");
-		break;
-	case CLOCK_TAI:
-		name = "tai\n";
-		sz = sizeof("tai\n");
-		break;
-	default:
+	if (clk < 0 && clk >= ARRAY_SIZE(clock_names))
 		BUG();
-	}
 
-	memcpy(buf, name, sz);
-	return sz;
+	return sysfs_emit(buf, "%s\n", clock_names[clk]);
 }
 
 static ssize_t current_timestamp_clock_store(struct device *dev,

@@ -1453,22 +1435,21 @@ static ssize_t current_timestamp_clock_store(struct device *dev,
 	clockid_t clk;
 	int ret;
 
-	if (sysfs_streq(buf, "realtime"))
-		clk = CLOCK_REALTIME;
-	else if (sysfs_streq(buf, "monotonic"))
-		clk = CLOCK_MONOTONIC;
-	else if (sysfs_streq(buf, "monotonic_raw"))
-		clk = CLOCK_MONOTONIC_RAW;
-	else if (sysfs_streq(buf, "realtime_coarse"))
-		clk = CLOCK_REALTIME_COARSE;
-	else if (sysfs_streq(buf, "monotonic_coarse"))
-		clk = CLOCK_MONOTONIC_COARSE;
-	else if (sysfs_streq(buf, "boottime"))
-		clk = CLOCK_BOOTTIME;
-	else if (sysfs_streq(buf, "tai"))
-		clk = CLOCK_TAI;
-	else
+	ret = sysfs_match_string(clock_names, buf);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case CLOCK_PROCESS_CPUTIME_ID:
+	case CLOCK_THREAD_CPUTIME_ID:
+	case CLOCK_REALTIME_ALARM:
+	case CLOCK_BOOTTIME_ALARM:
+	case CLOCK_SGI_CYCLE:
 		return -EINVAL;
+	default:
+		clk = ret;
+		break;
+	}
 
 	ret = iio_device_set_clock(dev_to_iio_dev(dev), clk);
 	if (ret)

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer

2023-07-22 17:11 UTC #7 lore raw

Re: [PATCH v2 2/8] iio: core: Use sysfs_match_string() helper

Quoted 4 lines · On Fri, 21 Jul 2023 20:00:16 +0300 Andy Shevchenko…

Use sysfs_match_string() helper instead of open coded variant.
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

It 'should' be impossible to end up with one of the clocks that we reject for the store..

So I wonder if it would be cleaner to only add the clocks we actually use to the clock_names[] array and use the failure to match (-EINVAL) to call BUG() which then matches the existing behaviour.

If the clock matching was a generic function I'd be fine with doing what you have here, but it's local to IIO so we don't have to play nice with clocks we don't understand.

Jonathan

Quoted 58 lines

---
 drivers/iio/industrialio-core.c | 81 +++++++++++++--------------------
 1 file changed, 31 insertions(+), 50 deletions(-)

drivers/iio/industrialio-core.c +15 ~~-24~~

index 6cca86fd0ef9..4e45740331ee 100644
@@ -1400,50 +1400,32 @@ static ssize_t label_show(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR_RO(label);
 
+static const char * const clock_names[] = {
+	[CLOCK_REALTIME]	 	= "realtime",
+	[CLOCK_MONOTONIC]	 	= "monotonic",
+	[CLOCK_PROCESS_CPUTIME_ID]	= "process_cputime_id",
+	[CLOCK_THREAD_CPUTIME_ID]	= "thread_cputime_id",
+	[CLOCK_MONOTONIC_RAW]	 	= "monotonic_raw",
+	[CLOCK_REALTIME_COARSE]	 	= "realtime_coarse",
+	[CLOCK_MONOTONIC_COARSE] 	= "monotonic_coarse",
+	[CLOCK_BOOTTIME]	 	= "boottime",
+	[CLOCK_REALTIME_ALARM]		= "realtime_alarm",
+	[CLOCK_BOOTTIME_ALARM]		= "boottime_alarm",
+	[CLOCK_SGI_CYCLE]		= "sgi_cycle",
+	[CLOCK_TAI]		 	= "tai",
+};
+
 static ssize_t current_timestamp_clock_show(struct device *dev,
 					    struct device_attribute *attr,
 					    char *buf)
 {
 	const struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	const clockid_t clk = iio_device_get_clock(indio_dev);
-	const char *name;
-	ssize_t sz;
 
-	switch (clk) {
-	case CLOCK_REALTIME:
-		name = "realtime\n";
-		sz = sizeof("realtime\n");
-		break;
-	case CLOCK_MONOTONIC:
-		name = "monotonic\n";
-		sz = sizeof("monotonic\n");
-		break;
-	case CLOCK_MONOTONIC_RAW:
-		name = "monotonic_raw\n";
-		sz = sizeof("monotonic_raw\n");
-		break;
-	case CLOCK_REALTIME_COARSE:
-		name = "realtime_coarse\n";
-		sz = sizeof("realtime_coarse\n");
-		break;
-	case CLOCK_MONOTONIC_COARSE:
-		name = "monotonic_coarse\n";
-		sz = sizeof("monotonic_coarse\n");
-		break;
-	case CLOCK_BOOTTIME:

drivers/iio/industrialio-core.c static ssize_t label_show(struct device *dev, struct device_attribute *attr, · line 1426

Quoted 10 lines

drivers/iio/industrialio-core.c +1 -8

-		name = "boottime\n";
-		sz = sizeof("boottime\n");
-		break;
-	case CLOCK_TAI:
-		name = "tai\n";
-		sz = sizeof("tai\n");
-		break;
-	default:
+	if (clk < 0 && clk >= ARRAY_SIZE(clock_names))
 		BUG();

This is a functional change as more clocks are allowed without things going boom.

Quoted 45 lines

drivers/iio/industrialio-core.c +15 ~~-18~~

-	}
 
-	memcpy(buf, name, sz);
-	return sz;
+	return sysfs_emit(buf, "%s\n", clock_names[clk]);
 }
 
 static ssize_t current_timestamp_clock_store(struct device *dev,

@@ -1453,22 +1435,21 @@ static ssize_t current_timestamp_clock_store(struct device *dev,
 	clockid_t clk;
 	int ret;
 
-	if (sysfs_streq(buf, "realtime"))
-		clk = CLOCK_REALTIME;
-	else if (sysfs_streq(buf, "monotonic"))
-		clk = CLOCK_MONOTONIC;
-	else if (sysfs_streq(buf, "monotonic_raw"))
-		clk = CLOCK_MONOTONIC_RAW;
-	else if (sysfs_streq(buf, "realtime_coarse"))
-		clk = CLOCK_REALTIME_COARSE;
-	else if (sysfs_streq(buf, "monotonic_coarse"))
-		clk = CLOCK_MONOTONIC_COARSE;
-	else if (sysfs_streq(buf, "boottime"))
-		clk = CLOCK_BOOTTIME;
-	else if (sysfs_streq(buf, "tai"))
-		clk = CLOCK_TAI;
-	else
+	ret = sysfs_match_string(clock_names, buf);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case CLOCK_PROCESS_CPUTIME_ID:
+	case CLOCK_THREAD_CPUTIME_ID:
+	case CLOCK_REALTIME_ALARM:
+	case CLOCK_BOOTTIME_ALARM:
+	case CLOCK_SGI_CYCLE:
 		return -EINVAL;
+	default:
+		clk = ret;
+		break;
+	}
 
 	ret = iio_device_set_clock(dev_to_iio_dev(dev), clk);
 	if (ret)

Andy Shevchenko

author

2023-07-21 17:00 UTC #8 lore raw

PATCH v2 3/8 iio: core: Switch to krealloc_array()

Let the krealloc_array() copy the original data and check for a multiplication overflow.

---
 drivers/iio/industrialio-core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/iio/industrialio-core.c +7 ~~-13~~

index 4e45740331ee..6e28c2a3d223 100644
@@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
 	const struct attribute_group **new, **old = iio_dev_opaque->groups;
 	unsigned int cnt = iio_dev_opaque->groupcounter;
 
-	new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
+	new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;

@@ -1483,13 +1483,13 @@ 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 attribute **attrs, **attr, *clk = NULL;
 	struct iio_dev_attr *p;
-	struct attribute **attr, *clk = NULL;
 
 	/* First count elements in any existing group */
-	if (indio_dev->info->attrs) {
-		attr = indio_dev->info->attrs->attrs;
-		while (*attr++ != NULL)
+	attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL;
+	if (attrs) {
+		for (attr = attrs; *attr; attr++)
 			attrcount_orig++;
 	}
 	attrcount = attrcount_orig;

@@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 	if (clk)
 		attrcount++;
 
+	/* Copy across original attributes, and point to original binary attributes */
 	iio_dev_opaque->chan_attr_group.attrs =
-		kcalloc(attrcount + 1,
-			sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
-			GFP_KERNEL);
+		krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL);
 	if (iio_dev_opaque->chan_attr_group.attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_clear_attrs;
 	}
-	/* Copy across original attributes, and point to original binary attributes */
 	if (indio_dev->info->attrs) {
-		memcpy(iio_dev_opaque->chan_attr_group.attrs,
-		       indio_dev->info->attrs->attrs,
-		       sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
-		       *attrcount_orig);
 		iio_dev_opaque->chan_attr_group.is_visible =
 			indio_dev->info->attrs->is_visible;
 		iio_dev_opaque->chan_attr_group.bin_attrs =

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer

2023-07-22 17:28 UTC #9 lore raw

Re: [PATCH v2 3/8] iio: core: Switch to krealloc_array()

Quoted 41 lines · On Fri, 21 Jul 2023 20:00:17 +0300 Andy Shevchenko…

Let the krealloc_array() copy the original data and check for a multiplication overflow.

---
 drivers/iio/industrialio-core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

drivers/iio/industrialio-core.c +5 -5

index 4e45740331ee..6e28c2a3d223 100644
@@ -1465,7 +1465,7 @@ int iio_device_register_sysfs_group(struct iio_dev *indio_dev,
 	const struct attribute_group **new, **old = iio_dev_opaque->groups;
 	unsigned int cnt = iio_dev_opaque->groupcounter;
 
-	new = krealloc(old, sizeof(*new) * (cnt + 2), GFP_KERNEL);
+	new = krealloc_array(old, cnt + 2, sizeof(*new), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;

@@ -1483,13 +1483,13 @@ 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 attribute **attrs, **attr, *clk = NULL;
 	struct iio_dev_attr *p;
-	struct attribute **attr, *clk = NULL;
 
 	/* First count elements in any existing group */
-	if (indio_dev->info->attrs) {
-		attr = indio_dev->info->attrs->attrs;
-		while (*attr++ != NULL)
+	attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs : NULL;
+	if (attrs) {
+		for (attr = attrs; *attr; attr++)
 			attrcount_orig++;
 	}
 	attrcount = attrcount_orig;

drivers/iio/industrialio-core.c static int iio_device_register_sysfs(struct iio_dev *indio_dev) · line 1526

Quoted 9 lines

drivers/iio/industrialio-core.c +2 -3

@@ -1521,20 +1521,14 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 	if (clk)
 		attrcount++;
 
+	/* Copy across original attributes, and point to original binary attributes */
 	iio_dev_opaque->chan_attr_group.attrs =
-		kcalloc(attrcount + 1,
-			sizeof(iio_dev_opaque->chan_attr_group.attrs[0]),
-			GFP_KERNEL);
+		krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL);

I'm a little lost, but isn't this realloc()ing attrs, which should be provided by drivers as constant if it is set to indio_dev->info->attrs->attrs?

That seems unlikely to work correctly, but I may well have lost track of the flow and attrs points somewhere else at this point. I guess it might work as the realloc code will detect it can't resize that array.

Feels wrong approach however.

Quoted 13 lines

drivers/iio/industrialio-core.c +0 -5

 	if (iio_dev_opaque->chan_attr_group.attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_clear_attrs;
 	}
-	/* Copy across original attributes, and point to original binary attributes */
 	if (indio_dev->info->attrs) {
-		memcpy(iio_dev_opaque->chan_attr_group.attrs,
-		       indio_dev->info->attrs->attrs,
-		       sizeof(iio_dev_opaque->chan_attr_group.attrs[0])
-		       *attrcount_orig);
 		iio_dev_opaque->chan_attr_group.is_visible =
 			indio_dev->info->attrs->is_visible;
 		iio_dev_opaque->chan_attr_group.bin_attrs =

Andy Shevchenko

author

2023-07-24 10:38 UTC #10 lore raw

Re: [PATCH v2 3/8] iio: core: Switch to krealloc_array()

Quoted 2 lines · Jonathan Cameron

On Fri, 21 Jul 2023 20:00:17 +0300
Andy Shevchenko <[email protected]> wrote:

...

Quoted 9 lines

drivers/iio/industrialio-core.c static int iio_device_register_sysfs(struct iio_dev *indio_dev) · line 1526
Quoted 1 line
drivers/iio/industrialio-core.c +1 -0
+		krealloc_array(attrs, attrcount + 1, sizeof(*attrs), GFP_KERNEL);
I'm a little lost, but isn't this realloc()ing attrs, which should be provided by drivers as constant if it is set to indio_dev->info->attrs->attrs?
That seems unlikely to work correctly, but I may well have lost track of the flow and attrs points somewhere else at this point. I guess it might work as the realloc code will detect it can't resize that array.

Argh!

The attrs are defined without const. So, basically to prevent code like this we have to make sure our local variables are defined as const.

I will drop this hunk from the next version, need to think if it makes sense to refactor and if so, in which way.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2023-07-21 17:00 UTC #11 lore raw

PATCH v2 4/8 iio: core: Use min() instead of min_t() to make code more robust

min() has strict type checking and preferred over min_t() for unsigned types to avoid overflow. Here it's unclear why min_t() was chosen since both variables are of the same type. In any case update to use min().

---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/industrialio-core.c +1 -1

index 6e28c2a3d223..78cc67efa490 100644
@@ -389,7 +389,7 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
 	char buf[80];
 	int ret;
 
-	count = min_t(size_t, count, (sizeof(buf)-1));
+	count = min(count, sizeof(buf) - 1);
 	if (copy_from_user(buf, userbuf, count))
 		return -EFAULT;

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer

2023-07-23 09:14 UTC #12 lore raw

Re: [PATCH v2 4/8] iio: core: Use min() instead of min_t() to make code more robust

Quoted 7 lines · On Fri, 21 Jul 2023 20:00:18 +0300 Andy Shevchenko…

min() has strict type checking and preferred over min_t() for unsigned types to avoid overflow. Here it's unclear why min_t() was chosen since both variables are of the same type. In any case update to use min().
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

Applied to the togreg branch of iio.git and pushed out as testing for 0-day to give it a quick sanity check.

Thanks,

Jonathan

Quoted 17 lines

---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iio/industrialio-core.c +1 -1

index 6e28c2a3d223..78cc67efa490 100644
@@ -389,7 +389,7 @@ static ssize_t iio_debugfs_write_reg(struct file *file,
 	char buf[80];
 	int ret;
 
-	count = min_t(size_t, count, (sizeof(buf)-1));
+	count = min(count, sizeof(buf) - 1);
 	if (copy_from_user(buf, userbuf, count))
 		return -EFAULT;

Andy Shevchenko

author

2023-07-21 17:00 UTC #13 lore raw

PATCH v2 5/8 iio: core: Get rid of redundant 'else'

In the snippets like the following

	if (...)
		return / goto / break / continue ...;
	else
		...

the 'else' is redundant. Get rid of it.

---
 drivers/iio/industrialio-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/industrialio-core.c +3 -3

index 78cc67efa490..66cea23df7e0 100644
@@ -524,7 +524,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev,
 	i = e->get(indio_dev, chan);
 	if (i < 0)
 		return i;
-	else if (i >= e->num_items || !e->items[i])
+	if (i >= e->num_items || !e->items[i])
 		return -EINVAL;
 
 	return sysfs_emit(buf, "%s\n", e->items[i]);

@@ -1217,7 +1217,7 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
 					     &iio_dev_opaque->channel_attr_list);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
-		else if (ret < 0)
+		if (ret < 0)
 			return ret;
 		attrcount++;
 	}

@@ -1255,7 +1255,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 		kfree(avail_postfix);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
-		else if (ret < 0)
+		if (ret < 0)
 			return ret;
 		attrcount++;
 	}

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer Applied

2023-07-23 09:16 UTC #14 lore raw

Re: [PATCH v2 5/8] iio: core: Get rid of redundant 'else'

Quoted 11 lines · On Fri, 21 Jul 2023 20:00:19 +0300 Andy Shevchenko…

In the snippets like the following
	if (...)
		return / goto / break / continue ...;
	else
		...
the 'else' is redundant. Get rid of it.
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

Applied.

Thanks,

Jonathan

Quoted 35 lines

---
 drivers/iio/industrialio-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iio/industrialio-core.c +3 -3

index 78cc67efa490..66cea23df7e0 100644
@@ -524,7 +524,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev,
 	i = e->get(indio_dev, chan);
 	if (i < 0)
 		return i;
-	else if (i >= e->num_items || !e->items[i])
+	if (i >= e->num_items || !e->items[i])
 		return -EINVAL;
 
 	return sysfs_emit(buf, "%s\n", e->items[i]);

@@ -1217,7 +1217,7 @@ static int iio_device_add_info_mask_type(struct iio_dev *indio_dev,
 					     &iio_dev_opaque->channel_attr_list);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
-		else if (ret < 0)
+		if (ret < 0)
 			return ret;
 		attrcount++;
 	}

@@ -1255,7 +1255,7 @@ static int iio_device_add_info_mask_type_avail(struct iio_dev *indio_dev,
 		kfree(avail_postfix);
 		if ((ret == -EBUSY) && (shared_by != IIO_SEPARATE))
 			continue;
-		else if (ret < 0)
+		if (ret < 0)
 			return ret;
 		attrcount++;
 	}

Andy Shevchenko

author

2023-07-21 17:00 UTC #15 lore raw

PATCH v2 6/8 iio: core: Fix issues and style of the comments

The scripts/kernel-doc -v -none -Wall reports several issues with the kernel doc in IIO core C file. Update the comments accordingly.

---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

drivers/iio/industrialio-core.c +37 ~~-20~~

index 66cea23df7e0..a9b9804097ab 100644
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* The industrial I/O core
+/*
+ * The industrial I/O core
  *
  * Copyright (c) 2008 Jonathan Cameron
  *

@@ -183,7 +184,9 @@ static const char * const iio_chan_info_postfix[] = {
  * @indio_dev:		Device structure whose ID is being queried
  *
  * The IIO device ID is a unique index used for example for the naming
- * of the character device /dev/iio\:device[ID]
+ * of the character device /dev/iio\:device[ID].
+ *
+ * Returns: Unique ID for the device.
  */
 int iio_device_id(struct iio_dev *indio_dev)
 {

@@ -196,6 +199,8 @@ EXPORT_SYMBOL_GPL(iio_device_id);
 /**
  * iio_buffer_enabled() - helper function to test if the buffer is enabled
  * @indio_dev:		IIO device structure for device
+ *
+ * Returns: True, if the buffer is enabled.
  */
 bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {

@@ -225,6 +230,9 @@ EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
  * @si:			scan index to match
+ *
+ * Returns:
+ * Constant pointer to iio_chan_spec, if scan index matches, NULL on failure.
  */
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)

@@ -249,7 +257,9 @@ EXPORT_SYMBOL(iio_read_const_attr);
 /**
  * iio_device_set_clock() - Set current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
- * @clock_id: timestamping clock posix identifier to set.
+ * @clock_id: timestamping clock POSIX identifier to set.
+ *
+ * Returns: 0 on success, or a negative error code.
  */
 int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 {

@@ -275,6 +285,8 @@ EXPORT_SYMBOL(iio_device_set_clock);
 /**
  * iio_device_get_clock() - Retrieve current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
+ *
+ * Returns: Clock ID of the current timestamping clock for the device.
  */
 clockid_t iio_device_get_clock(const struct iio_dev *indio_dev)
 {

@@ -287,6 +299,8 @@ EXPORT_SYMBOL(iio_device_get_clock);
 /**
  * iio_get_time_ns() - utility function to get a time stamp for events etc
  * @indio_dev: device
+ *
+ * Returns: Timestamp of the event in nanoseconds.
  */
 s64 iio_get_time_ns(const struct iio_dev *indio_dev)
 {

@@ -594,7 +608,7 @@ EXPORT_SYMBOL_GPL(iio_show_mount_matrix);
  * If device is assigned no mounting matrix property, a default 3x3 identity
  * matrix will be filled in.
  *
- * Return: 0 if success, or a negative error code on failure.
+ * Returns: 0 if success, or a negative error code on failure.
  */
 int iio_read_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix)
 {

@@ -692,9 +706,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
  * @vals:	Pointer to the values, exact meaning depends on the
  *		type parameter.
  *
- * Return: 0 by default, a negative number on failure or the
- *	   total number of characters written for a type that belongs
- *	   to the IIO_VAL_* constant.
+ * Returns:
+ * 0 by default, a negative number on failure or the total number of characters
+ * written for a type that belongs to the IIO_VAL_* constant.
  */
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
 {

@@ -847,8 +861,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
  * @fract: The fractional part of the number
  * @scale_db: True if this should parse as dB
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 static int __iio_str_to_fixpoint(const char *str, int fract_mult,
 				 int *integer, int *fract, bool scale_db)

@@ -917,8 +931,8 @@ static int __iio_str_to_fixpoint(const char *str, int fract_mult,
  * @integer: The integer part of the number
  * @fract: The fractional part of the number
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 int iio_str_to_fixpoint(const char *str, int fract_mult,
 			int *integer, int *fract)

@@ -1618,7 +1632,10 @@ const struct device_type iio_device_type = {
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.
  * @sizeof_priv:	Space to allocate for private structure.
- **/
+ *
+ * Returns:
+ * Pointer to allocated iio_dev on success, NULL on failure.
+ */
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 {
 	struct iio_dev_opaque *iio_dev_opaque;

@@ -1668,7 +1685,7 @@ EXPORT_SYMBOL(iio_device_alloc);
 /**
  * iio_device_free() - free an iio_dev from a driver
  * @dev:		the iio_dev associated with the device
- **/
+ */
 void iio_device_free(struct iio_dev *dev)
 {
 	if (dev)

@@ -1689,7 +1706,7 @@ static void devm_iio_device_release(void *iio_dev)
  * Managed iio_device_alloc. iio_dev allocated with this function is
  * automatically freed on driver detach.
  *
- * RETURNS:
+ * Returns:
  * Pointer to allocated iio_dev on success, NULL on failure.
  */
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)

@@ -1716,8 +1733,8 @@ EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
  * @filp:	File structure for iio device used to keep and later access
  *		private data
  *
- * Return: 0 on success or -EBUSY if the device is already opened
- **/
+ * Returns: 0 on success or -EBUSY if the device is already opened
+ */
 static int iio_chrdev_open(struct inode *inode, struct file *filp)
 {
 	struct iio_dev_opaque *iio_dev_opaque =

@@ -1750,7 +1767,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
  * @inode:	Inode structure pointer for the char device
  * @filp:	File structure pointer for the char device
  *
- * Return: 0 for successful release
+ * Returns: 0 for successful release.
  */
 static int iio_chrdev_release(struct inode *inode, struct file *filp)
 {

@@ -1789,7 +1806,7 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
 
-	/**
+	/*
 	 * The NULL check here is required to prevent crashing when a device
 	 * is being removed while userspace would still have open file handles
 	 * to try to access this device.

@@ -1966,7 +1983,7 @@ EXPORT_SYMBOL(__iio_device_register);
 /**
  * iio_device_unregister() - unregister a device from the IIO subsystem
  * @indio_dev:		Device structure representing the device.
- **/
+ */
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);

@@ -2017,7 +2034,7 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
  *
  * Use with iio_device_release_direct_mode()
  *
- * Returns: 0 on success, -EBUSY on failure
+ * Returns: 0 on success, -EBUSY on failure.
  */
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
 {

Signature · 1 line

2.40.0.1.gaa8946217a0b

Randy Dunlap

Reviewed-by on v2 6/8

2023-07-21 20:48 UTC #16 lore raw

Re: [PATCH v2 6/8] iio: core: Fix issues and style of the comments

Quoted 26 lines · Andy Shevchenko

The scripts/kernel-doc -v -none -Wall reports several issues with the kernel doc in IIO core C file. Update the comments accordingly.

---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

drivers/iio/industrialio-core.c +1 -1

index 66cea23df7e0..a9b9804097ab 100644
@@ -594,7 +608,7 @@ EXPORT_SYMBOL_GPL(iio_show_mount_matrix);
  * If device is assigned no mounting matrix property, a default 3x3 identity
  * matrix will be filled in.
  *
- * Return: 0 if success, or a negative error code on failure.
+ * Returns: 0 if success, or a negative error code on failure.
  */
 int iio_read_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix)
 {

drivers/iio/industrialio-core.c static int iio_chrdev_open(struct inode *inode, struct file *filp) · line 1771

Quoted 6 lines

drivers/iio/industrialio-core.c +1 -1

@@ -1750,7 +1767,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
  * @inode:	Inode structure pointer for the char device
  * @filp:	File structure pointer for the char device
  *
- * Return: 0 for successful release
+ * Returns: 0 for successful release.
  */

As documented in Documentation/doc-guide/kernel-doc.rst:
The return value, if any, should be described in a dedicated section named ``Return``.

However, as you (and I) have found, "Returns:" also works as a section name.

Reviewed-by Randy Dunlap <[email protected]>

Thanks.

Signature · 1 line

~Randy

[email protected]

2023-07-21 21:37 UTC #17 lore raw

Re: [PATCH v2 6/8] iio: core: Fix issues and style of the comments

Fri, Jul 21, 2023 at 01:48:34PM -0700, Randy Dunlap kirjoitti:

Quoted 1 line · Andy Shevchenko

...

Quoted 8 lines

Quoted 2 lines
- * Return: 0 for successful release
+ * Returns: 0 for successful release.
As documented in Documentation/doc-guide/kernel-doc.rst:
The return value, if any, should be described in a dedicated section named ``Return``.
However, as you (and I) have found, "Returns:" also works as a section name.

Quoted 1 line

Reviewed-by Randy Dunlap <[email protected]>

Thank you!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jonathan Cameron

maintainer

2023-07-23 09:21 UTC #18 lore raw

Re: [PATCH v2 6/8] iio: core: Fix issues and style of the comments

Quoted 6 lines · On Fri, 21 Jul 2023 20:00:20 +0300 Andy Shevchenko…

The scripts/kernel-doc -v -none -Wall reports several issues with the kernel doc in IIO core C file. Update the comments accordingly.
Signed-off-by Andy Shevchenko <[email protected]>
Reviewed-by Nuno Sa <[email protected]>

Looks good to me, but given I didn't apply the earlier clock setting related patch there will be a bunch of churn. Hence I'll pick this one up once that's resolved.

Thanks,

Jonathan

Quoted 196 lines

---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

drivers/iio/industrialio-core.c +37 ~~-20~~

index 66cea23df7e0..a9b9804097ab 100644
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* The industrial I/O core
+/*
+ * The industrial I/O core
  *
  * Copyright (c) 2008 Jonathan Cameron
  *

@@ -183,7 +184,9 @@ static const char * const iio_chan_info_postfix[] = {
  * @indio_dev:		Device structure whose ID is being queried
  *
  * The IIO device ID is a unique index used for example for the naming
- * of the character device /dev/iio\:device[ID]
+ * of the character device /dev/iio\:device[ID].
+ *
+ * Returns: Unique ID for the device.
  */
 int iio_device_id(struct iio_dev *indio_dev)
 {

@@ -196,6 +199,8 @@ EXPORT_SYMBOL_GPL(iio_device_id);
 /**
  * iio_buffer_enabled() - helper function to test if the buffer is enabled
  * @indio_dev:		IIO device structure for device
+ *
+ * Returns: True, if the buffer is enabled.
  */
 bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {

@@ -225,6 +230,9 @@ EXPORT_SYMBOL_GPL(iio_get_debugfs_dentry);
  * iio_find_channel_from_si() - get channel from its scan index
  * @indio_dev:		device
  * @si:			scan index to match
+ *
+ * Returns:
+ * Constant pointer to iio_chan_spec, if scan index matches, NULL on failure.
  */
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si)

@@ -249,7 +257,9 @@ EXPORT_SYMBOL(iio_read_const_attr);
 /**
  * iio_device_set_clock() - Set current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
- * @clock_id: timestamping clock posix identifier to set.
+ * @clock_id: timestamping clock POSIX identifier to set.
+ *
+ * Returns: 0 on success, or a negative error code.
  */
 int iio_device_set_clock(struct iio_dev *indio_dev, clockid_t clock_id)
 {

@@ -275,6 +285,8 @@ EXPORT_SYMBOL(iio_device_set_clock);
 /**
  * iio_device_get_clock() - Retrieve current timestamping clock for the device
  * @indio_dev: IIO device structure containing the device
+ *
+ * Returns: Clock ID of the current timestamping clock for the device.
  */
 clockid_t iio_device_get_clock(const struct iio_dev *indio_dev)
 {

@@ -287,6 +299,8 @@ EXPORT_SYMBOL(iio_device_get_clock);
 /**
  * iio_get_time_ns() - utility function to get a time stamp for events etc
  * @indio_dev: device
+ *
+ * Returns: Timestamp of the event in nanoseconds.
  */
 s64 iio_get_time_ns(const struct iio_dev *indio_dev)
 {

@@ -594,7 +608,7 @@ EXPORT_SYMBOL_GPL(iio_show_mount_matrix);
  * If device is assigned no mounting matrix property, a default 3x3 identity
  * matrix will be filled in.
  *
- * Return: 0 if success, or a negative error code on failure.
+ * Returns: 0 if success, or a negative error code on failure.
  */
 int iio_read_mount_matrix(struct device *dev, struct iio_mount_matrix *matrix)
 {

@@ -692,9 +706,9 @@ static ssize_t __iio_format_value(char *buf, size_t offset, unsigned int type,
  * @vals:	Pointer to the values, exact meaning depends on the
  *		type parameter.
  *
- * Return: 0 by default, a negative number on failure or the
- *	   total number of characters written for a type that belongs
- *	   to the IIO_VAL_* constant.
+ * Returns:
+ * 0 by default, a negative number on failure or the total number of characters
+ * written for a type that belongs to the IIO_VAL_* constant.
  */
 ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
 {

@@ -847,8 +861,8 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
  * @fract: The fractional part of the number
  * @scale_db: True if this should parse as dB
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 static int __iio_str_to_fixpoint(const char *str, int fract_mult,
 				 int *integer, int *fract, bool scale_db)

@@ -917,8 +931,8 @@ static int __iio_str_to_fixpoint(const char *str, int fract_mult,
  * @integer: The integer part of the number
  * @fract: The fractional part of the number
  *
- * Returns 0 on success, or a negative error code if the string could not be
- * parsed.
+ * Returns:
+ * 0 on success, or a negative error code if the string could not be parsed.
  */
 int iio_str_to_fixpoint(const char *str, int fract_mult,
 			int *integer, int *fract)

@@ -1618,7 +1632,10 @@ const struct device_type iio_device_type = {
  * iio_device_alloc() - allocate an iio_dev from a driver
  * @parent:		Parent device.
  * @sizeof_priv:	Space to allocate for private structure.
- **/
+ *
+ * Returns:
+ * Pointer to allocated iio_dev on success, NULL on failure.
+ */
 struct iio_dev *iio_device_alloc(struct device *parent, int sizeof_priv)
 {
 	struct iio_dev_opaque *iio_dev_opaque;

@@ -1668,7 +1685,7 @@ EXPORT_SYMBOL(iio_device_alloc);
 /**
  * iio_device_free() - free an iio_dev from a driver
  * @dev:		the iio_dev associated with the device
- **/
+ */
 void iio_device_free(struct iio_dev *dev)
 {
 	if (dev)

@@ -1689,7 +1706,7 @@ static void devm_iio_device_release(void *iio_dev)
  * Managed iio_device_alloc. iio_dev allocated with this function is
  * automatically freed on driver detach.
  *
- * RETURNS:
+ * Returns:
  * Pointer to allocated iio_dev on success, NULL on failure.
  */
 struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv)

@@ -1716,8 +1733,8 @@ EXPORT_SYMBOL_GPL(devm_iio_device_alloc);
  * @filp:	File structure for iio device used to keep and later access
  *		private data
  *
- * Return: 0 on success or -EBUSY if the device is already opened
- **/
+ * Returns: 0 on success or -EBUSY if the device is already opened
+ */
 static int iio_chrdev_open(struct inode *inode, struct file *filp)
 {
 	struct iio_dev_opaque *iio_dev_opaque =

@@ -1750,7 +1767,7 @@ static int iio_chrdev_open(struct inode *inode, struct file *filp)
  * @inode:	Inode structure pointer for the char device
  * @filp:	File structure pointer for the char device
  *
- * Return: 0 for successful release
+ * Returns: 0 for successful release.
  */
 static int iio_chrdev_release(struct inode *inode, struct file *filp)
 {

@@ -1789,7 +1806,7 @@ static long iio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 	mutex_lock(&iio_dev_opaque->info_exist_lock);
 
-	/**
+	/*
 	 * The NULL check here is required to prevent crashing when a device
 	 * is being removed while userspace would still have open file handles
 	 * to try to access this device.

@@ -1966,7 +1983,7 @@ EXPORT_SYMBOL(__iio_device_register);
 /**
  * iio_device_unregister() - unregister a device from the IIO subsystem
  * @indio_dev:		Device structure representing the device.
- **/
+ */
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);

@@ -2017,7 +2034,7 @@ EXPORT_SYMBOL_GPL(__devm_iio_device_register);
  *
  * Use with iio_device_release_direct_mode()
  *
- * Returns: 0 on success, -EBUSY on failure
+ * Returns: 0 on success, -EBUSY on failure.
  */
 int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
 {

Andy Shevchenko

author

2023-07-21 17:00 UTC #19 lore raw

PATCH v2 7/8 iio: core: Move initcalls closer to the respective calls

Move subsys_initcall() and module_exit() closer to the respective calls.

---
 drivers/iio/industrialio-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/industrialio-core.c +2 -3

index a9b9804097ab..5c9c68d69fc6 100644
@@ -354,6 +354,7 @@ static int __init iio_init(void)
 error_nothing:
 	return ret;
 }
+subsys_initcall(iio_init);
 
 static void __exit iio_exit(void)
 {

@@ -362,6 +363,7 @@ static void __exit iio_exit(void)
 	bus_unregister(&iio_bus_type);
 	debugfs_remove(iio_debugfs_dentry);
 }
+module_exit(iio_exit);
 
 #if defined(CONFIG_DEBUG_FS)
 static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,

@@ -2118,9 +2120,6 @@ int iio_device_get_current_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
 
-subsys_initcall(iio_init);
-module_exit(iio_exit);
-
 MODULE_AUTHOR("Jonathan Cameron <[email protected]>");
 MODULE_DESCRIPTION("Industrial I/O core");
 MODULE_LICENSE("GPL");

Signature · 1 line

2.40.0.1.gaa8946217a0b

Jonathan Cameron

maintainer

2023-07-23 09:23 UTC #20 lore raw

Re: [PATCH v2 7/8] iio: core: Move initcalls closer to the respective calls

Quoted 1 line · On Fri, 21 Jul 2023 20:00:21 +0300 Andy Shevchenko…

Move subsys_initcall() and module_exit() closer to the respective calls.

Why? For this particular set of macros I can see advantages to them being near the code and to them being in a fairly predictable location (end of file).

I think the patch description should make the why argument.

Jonathan

Quoted 37 lines

---
 drivers/iio/industrialio-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/iio/industrialio-core.c +2 -3

index a9b9804097ab..5c9c68d69fc6 100644
@@ -354,6 +354,7 @@ static int __init iio_init(void)
 error_nothing:
 	return ret;
 }
+subsys_initcall(iio_init);
 
 static void __exit iio_exit(void)
 {

@@ -362,6 +363,7 @@ static void __exit iio_exit(void)
 	bus_unregister(&iio_bus_type);
 	debugfs_remove(iio_debugfs_dentry);
 }
+module_exit(iio_exit);
 
 #if defined(CONFIG_DEBUG_FS)
 static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,

@@ -2118,9 +2120,6 @@ int iio_device_get_current_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
 
-subsys_initcall(iio_init);
-module_exit(iio_exit);
-
 MODULE_AUTHOR("Jonathan Cameron <[email protected]>");
 MODULE_DESCRIPTION("Industrial I/O core");
 MODULE_LICENSE("GPL");

Andy Shevchenko

author

2023-07-24 10:53 UTC #21 lore raw

Re: [PATCH v2 7/8] iio: core: Move initcalls closer to the respective calls

Quoted 10 lines · Jonathan Cameron

Quoted 1 line · On Fri, 21 Jul 2023 20:00:21 +0300 Andy Shevchenko…
Move subsys_initcall() and module_exit() closer to the respective calls.
Why? For this particular set of macros I can see advantages to them being near the code and to them being in a fairly predictable location (end of file).
I think the patch description should make the why argument.

The documented case is about exported symbols.
Now I looked for the module_*() stuff and there is no consensus between subsystems. Both styles are being documented and used.

I think I may drop this. Someone also can investigate the trends, but I won't spend my time on this.

Thank you for the review.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2023-07-21 17:00 UTC #22 lore raw

PATCH v2 8/8 iio: core: Improve indentation in a few places

Improve an indentation in a few places to increase readability.