[PATCH v2 0/8] iio: core: A few code cleanups and documentation fixes

[PATCH v2 0/8] iio: core: A few code cleanups and documentation fixes

[Andy Shevchenko]

Just set of almost unrelated to each other cleanups against IIO
core implementation.

The positive LoCs diffstat due to first patch that adds a lot of
documentation for the new added macro.

v2:
- sprintf() --> sysfs_emit() (Nuno)
- added tag (Nuno)

Andy Shevchenko (8):
  iio: core: Add opaque_struct_size() helper and use it
  iio: core: Use sysfs_match_string() helper
  iio: core: Switch to krealloc_array()
  iio: core: Use min() instead of min_t() to make code more robust
  iio: core: Get rid of redundant 'else'
  iio: core: Fix issues and style of the comments
  iio: core: Move initcalls closer to the respective calls
  iio: core: Improve indentation in a few places

 drivers/iio/industrialio-core.c | 226 ++++++++++++++++----------------
 1 file changed, 115 insertions(+), 111 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

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

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c117f50d0cf3..6cca86fd0ef9 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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;
-- 
2.40.0.1.gaa8946217a0b

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

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> Introduce opaque_struct_size() helper, which may be moved
> to overflow.h in the future, and use it in the IIO core.

...

> +#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?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > Introduce opaque_struct_size() helper, which may be moved
> > to overflow.h in the future, and use it in the IIO core.

...

> > +#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?

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 08:25:12PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > > Introduce opaque_struct_size() helper, which may be moved
> > > to overflow.h in the future, and use it in the IIO core.

...

> > > +#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?
> 
> 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

Hello Andy,

On Fri, Jul 21, 2023 at 08:25:12PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 08:22:35PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 08:00:15PM +0300, Andy Shevchenko wrote:
> > > Introduce opaque_struct_size() helper, which may be moved
> > > to overflow.h in the future, and use it in the IIO core.
> 
> ...
> 
> > > +#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.

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

Still I like this better.

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

> #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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andy Shevchenko]

Use sysfs_match_string() helper instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 81 +++++++++++++--------------------
 1 file changed, 31 insertions(+), 50 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 6cca86fd0ef9..4e45740331ee 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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)
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:16 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Use sysfs_match_string() helper instead of open coded variant.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>

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

> ---
>  drivers/iio/industrialio-core.c | 81 +++++++++++++--------------------
>  1 file changed, 31 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 6cca86fd0ef9..4e45740331ee 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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();

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

> -	}
>  
> -	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)

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 4e45740331ee..6e28c2a3d223 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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 =
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:17 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Let the krealloc_array() copy the original data and
> check for a multiplication overflow.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 4e45740331ee..6e28c2a3d223 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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);


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.



>  	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 =

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

[Andy Shevchenko]

On Sat, Jul 22, 2023 at 06:28:20PM +0100, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 20:00:17 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

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

-- 
With Best Regards,
Andy Shevchenko

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

[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 <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 6e28c2a3d223..78cc67efa490 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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;
 
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:18 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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 <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
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

> ---
>  drivers/iio/industrialio-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 6e28c2a3d223..78cc67efa490 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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;
>  

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

[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 <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 78cc67efa490..66cea23df7e0 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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++;
 	}
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:19 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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 <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied.

Thanks,

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 78cc67efa490..66cea23df7e0 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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++;
>  	}

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

[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 <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 66cea23df7e0..a9b9804097ab 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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)
 {
-- 
2.40.0.1.gaa8946217a0b

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

[Randy Dunlap]

On 7/21/23 10:00, Andy Shevchenko wrote:
> 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 <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 66cea23df7e0..a9b9804097ab 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c

> @@ -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)
>  {

> @@ -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 <rdunlap@infradead.org>

Thanks.
-- 
~Randy

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

[andy.shevchenko]

Fri, Jul 21, 2023 at 01:48:34PM -0700, Randy Dunlap kirjoitti:
> On 7/21/23 10:00, Andy Shevchenko wrote:

...

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

Plural allowed for some section(s):
'\s*(\@[.\w]+|\@\.\.\.|description|context|returns?|notes?|examples?)\s*:([^:].*)?$'

> Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:20 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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 <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>

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

> ---
>  drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 66cea23df7e0..a9b9804097ab 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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)
>  {

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

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index a9b9804097ab..5c9c68d69fc6 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -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 <jic23@kernel.org>");
 MODULE_DESCRIPTION("Industrial I/O core");
 MODULE_LICENSE("GPL");
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:21 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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


> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/industrialio-core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index a9b9804097ab..5c9c68d69fc6 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -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 <jic23@kernel.org>");
>  MODULE_DESCRIPTION("Industrial I/O core");
>  MODULE_LICENSE("GPL");

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

[Andy Shevchenko]

On Sun, Jul 23, 2023 at 10:23:35AM +0100, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 20:00:21 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > 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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Improve an indentation in a few places to increase readability.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-core.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 5c9c68d69fc6..e1293fdbc0ef 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -206,9 +206,9 @@ bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
 
-	return iio_dev_opaque->currentmode
-		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
-		   INDIO_BUFFER_SOFTWARE);
+	return iio_dev_opaque->currentmode &
+	       (INDIO_BUFFER_HARDWARE | INDIO_BUFFER_SOFTWARE |
+		INDIO_BUFFER_TRIGGERED);
 }
 EXPORT_SYMBOL_GPL(iio_buffer_enabled);
 
@@ -388,8 +388,8 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
 	}
 
 	iio_dev_opaque->read_buf_len = snprintf(iio_dev_opaque->read_buf,
-					      sizeof(iio_dev_opaque->read_buf),
-					      "0x%X\n", val);
+						sizeof(iio_dev_opaque->read_buf),
+						"0x%X\n", val);
 
 	return simple_read_from_buffer(userbuf, count, ppos,
 				       iio_dev_opaque->read_buf,
@@ -492,8 +492,7 @@ static ssize_t iio_read_channel_ext_info(struct device *dev,
 
 static ssize_t iio_write_channel_ext_info(struct device *dev,
 				     struct device_attribute *attr,
-				     const char *buf,
-					 size_t len)
+				     const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
@@ -585,9 +584,9 @@ static int iio_setup_mount_idmatrix(const struct device *dev,
 ssize_t iio_show_mount_matrix(struct iio_dev *indio_dev, uintptr_t priv,
 			      const struct iio_chan_spec *chan, char *buf)
 {
-	const struct iio_mount_matrix *mtx = ((iio_get_mount_matrix_t *)
-					      priv)(indio_dev, chan);
+	const struct iio_mount_matrix *mtx;
 
+	mtx = ((iio_get_mount_matrix_t *)priv)(indio_dev, chan);
 	if (IS_ERR(mtx))
 		return PTR_ERR(mtx);
 
@@ -1025,14 +1024,12 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 	if (chan->modified && (shared_by == IIO_SEPARATE)) {
 		if (chan->extend_name)
 			full_postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
-						 iio_modifier_names[chan
-								    ->channel2],
+						 iio_modifier_names[chan->channel2],
 						 chan->extend_name,
 						 postfix);
 		else
 			full_postfix = kasprintf(GFP_KERNEL, "%s_%s",
-						 iio_modifier_names[chan
-								    ->channel2],
+						 iio_modifier_names[chan->channel2],
 						 postfix);
 	} else {
 		if (chan->extend_name == NULL || shared_by != IIO_SEPARATE)
-- 
2.40.0.1.gaa8946217a0b

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 20:00:22 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Improve an indentation in a few places to increase readability.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing
for 0-day to not notice any change (hopefully ;)

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 5c9c68d69fc6..e1293fdbc0ef 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -206,9 +206,9 @@ bool iio_buffer_enabled(struct iio_dev *indio_dev)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
>  
> -	return iio_dev_opaque->currentmode
> -		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
> -		   INDIO_BUFFER_SOFTWARE);
> +	return iio_dev_opaque->currentmode &
> +	       (INDIO_BUFFER_HARDWARE | INDIO_BUFFER_SOFTWARE |
> +		INDIO_BUFFER_TRIGGERED);
>  }
>  EXPORT_SYMBOL_GPL(iio_buffer_enabled);
>  
> @@ -388,8 +388,8 @@ static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
>  	}
>  
>  	iio_dev_opaque->read_buf_len = snprintf(iio_dev_opaque->read_buf,
> -					      sizeof(iio_dev_opaque->read_buf),
> -					      "0x%X\n", val);
> +						sizeof(iio_dev_opaque->read_buf),
> +						"0x%X\n", val);
>  
>  	return simple_read_from_buffer(userbuf, count, ppos,
>  				       iio_dev_opaque->read_buf,
> @@ -492,8 +492,7 @@ static ssize_t iio_read_channel_ext_info(struct device *dev,
>  
>  static ssize_t iio_write_channel_ext_info(struct device *dev,
>  				     struct device_attribute *attr,
> -				     const char *buf,
> -					 size_t len)
> +				     const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> @@ -585,9 +584,9 @@ static int iio_setup_mount_idmatrix(const struct device *dev,
>  ssize_t iio_show_mount_matrix(struct iio_dev *indio_dev, uintptr_t priv,
>  			      const struct iio_chan_spec *chan, char *buf)
>  {
> -	const struct iio_mount_matrix *mtx = ((iio_get_mount_matrix_t *)
> -					      priv)(indio_dev, chan);
> +	const struct iio_mount_matrix *mtx;
>  
> +	mtx = ((iio_get_mount_matrix_t *)priv)(indio_dev, chan);
>  	if (IS_ERR(mtx))
>  		return PTR_ERR(mtx);
>  
> @@ -1025,14 +1024,12 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>  	if (chan->modified && (shared_by == IIO_SEPARATE)) {
>  		if (chan->extend_name)
>  			full_postfix = kasprintf(GFP_KERNEL, "%s_%s_%s",
> -						 iio_modifier_names[chan
> -								    ->channel2],  
> +						 iio_modifier_names[chan->channel2],
>  						 chan->extend_name,
>  						 postfix);
>  		else
>  			full_postfix = kasprintf(GFP_KERNEL, "%s_%s",
> -						 iio_modifier_names[chan
> -								    ->channel2],  
> +						 iio_modifier_names[chan->channel2],
>  						 postfix);
>  	} else {
>  		if (chan->extend_name == NULL || shared_by != IIO_SEPARATE)