Skip to contentSkip to conversation

iio: core: A few code cleanups and documentation fixes

PATCH Superseded Andy Shevchenko 2 participants 19 messages Jul 20, 2023 – Jul 21, 2023 IIO

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover iio: core: A few code cleanups and documentation fixes

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.

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(-)
Signature · 1 line
2.40.0.1.gaa8946217a0b
#1 lore raw

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

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 <[email protected]>
---
 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
#2 lore raw

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

In reply to Andy Shevchenko

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..90e59223b178 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 sprintf(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
#3 lore raw

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

Quoted 69 lines · Andy Shevchenko

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 +0 -0
index 6cca86fd0ef9..90e59223b178 100644
@@ -1400,50 +1400,32 @@ static ssize_t label_show(struct device *dev, struct
 device_attribute *attr,


 static DEVICE_ATTR_RO(label);

drivers/iio/industrialio-core.c +15 -0
+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;

Quoted 8 lines

-       default:
+       if (clk < 0 && clk >= ARRAY_SIZE(clock_names)) BUG();
-       }

-       memcpy(buf, name, sz);
-       return sz;
+       return sprintf(buf, "%s\n", clock_names[clk]);

syfs_emit()...

- Nuno Sá

#4 lore raw

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

In reply to 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 +7 -13
index 90e59223b178..be154879983e 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
#5 lore raw

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

Quoted 31 lines · 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 +0 -0
index 90e59223b178..be154879983e 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;

drivers/iio/industrialio-core.c +1 -1
@@ -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;

Quoted 8 lines

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

not really related with the change... maybe just mention it in the commit?

Quoted 8 lines

        }
        attrcount = attrcount_orig;

drivers/iio/industrialio-core.c +0 -0
@@ -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

Quoted 8 lines

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

since you're here and you also already did some style cleanups above, maybe change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?

- Nuno Sá

#6 lore raw

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

Quoted 1 line · Nuno Sá

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

Quoted 13 lines
Quoted 4 lines

+       struct attribute **attrs, **attr, *clk = NULL; struct iio_dev_attr *p;
-       struct attribute **attr, *clk = NULL;

Quoted 8 lines

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

Quoted 1 line

not really related with the change... maybe just mention it in the commit?

Hmm... It's related to make krealloc_array() to work as expected.

Quoted 1 line
Quoted 1 line

        }

...

Quoted 10 lines
Quoted 7 lines

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

since you're here and you also already did some style cleanups above, maybe change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?

I don't think it's related (but you can tell that this check related to the allocator, and since we touch it, we may touch this), if Jonathan wants this, I definitely do.

...

Thank you for the review!

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#7 lore raw

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

Quoted 19 lines · Andy Shevchenko
Quoted 1 line · Nuno Sá

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

Quoted 13 lines
Quoted 13 lines

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

Quoted 4 lines
Quoted 1 line

not really related with the change... maybe just mention it in the commit?

Hmm... It's related to make krealloc_array() to work as expected.

Hmm, I think it's arguable :). while() -> for() it's not really needed unless I'm missing something. You could even initialize 'attrs' to NULL at declaration and keep the above diff minimum.

That said, I actually prefer this style (even though some people don't like much the ternary operator).

Quoted 12 lines
Quoted 1 line
Quoted 1 line

        }

...

Quoted 8 lines
Quoted 7 lines

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

Quoted 6 lines
Quoted 2 lines

since you're here and you also already did some style cleanups above, maybe change it to 'if (!iio_dev_opaque->chan_attr_group.attrs)'?

I don't think it's related (but you can tell that this check related to the allocator, and since we touch it, we may touch this), if Jonathan wants this, I definitely do.

Fair enough...

- Nuno Sá

#8 lore raw

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

Quoted 3 lines · Nuno Sá
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Nuno Sá

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
Quoted 13 lines

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

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

not really related with the change... maybe just mention it in the commit?

Hmm... It's related to make krealloc_array() to work as expected.

Hmm, I think it's arguable :). while() -> for() it's not really needed unless I'm missing something. You could even initialize 'attrs' to NULL at declaration and keep the above diff minimum.

I'm not a fan of the assignments in the declarations when it potentially can be disrupted by a chunk of code and reading the code itself may be harder due to an interruption for checking the initial value. Hence, having

+ attr = attrs;
while (... != NULL)

seems enough to be replaced with one liner for-loop.

Quoted 2 lines

That said, I actually prefer this style (even though some people don't like much the ternary operator).

Thanks!

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

        }

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#9 lore raw

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

Quoted 4 lines · Andy Shevchenko
Quoted 3 lines · Nuno Sá
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Nuno Sá

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

Quoted 30 lines
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
Quoted 13 lines

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

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

not really related with the change... maybe just mention it in the commit?

Hmm... It's related to make krealloc_array() to work as expected.

Hmm, I think it's arguable :). while() -> for() it's not really needed unless I'm missing something. You could even initialize 'attrs' to NULL at declaration and keep the above diff minimum.

I'm not a fan of the assignments in the declarations when it potentially can be disrupted by a chunk of code and reading the code itself may be harder due to an interruption for checking the initial value. Hence, having

(quoted diff) +1 -0
+	attr = attrs;
 	while (... != NULL)
Quoted 1 line

seems enough to be replaced with one liner for-loop.

Note that attrs is reused later, so the above assignment makes it cleaner that some value is assigned to it. With the original code it's not so obvious.

Quoted 6 lines
Quoted 2 lines

That said, I actually prefer this style (even though some people don't like much the ternary operator).

Thanks!

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

        }

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#10 lore raw

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

In reply to 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().

---
 drivers/iio/industrialio-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/industrialio-core.c +1 -1
index be154879983e..9753f6552db4 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
#11 lore raw

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

In reply to Andy Shevchenko

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 9753f6552db4..ab9c6db69625 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
#12 lore raw

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

In reply to 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 +37 -20
index ab9c6db69625..e29772940886 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
#13 lore raw

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

In reply to Andy Shevchenko

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 e29772940886..202a1a67ba98 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
#14 lore raw

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

In reply to Andy Shevchenko

Improve an indentation in a few places to increase readability.

---
 drivers/iio/industrialio-core.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
drivers/iio/industrialio-core.c +10 -13
index 202a1a67ba98..1918da2a70ad 100644
@@ -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)
Signature · 1 line
2.40.0.1.gaa8946217a0b
Reviewed-by Nuno Sa on the v1 series
#15 lore raw
In reply to Andy Shevchenko
Quoted 16 lines · 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.

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

Quoted 3 lines

 drivers/iio/industrialio-core.c | 226 ++++++++++++++++----------------

 1 file changed, 115 insertions(+), 111 deletions(-)

Neat series... Just a few comments on my side and on patch 3 is up to you to take the comments or not.

With my comment addressed on patch 2:

#16 lore raw
Quoted 17 lines · Nuno Sá
Quoted 16 lines · 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.

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

Quoted 8 lines
Quoted 3 lines

 drivers/iio/industrialio-core.c | 226 ++++++++++++++++----------------

 1 file changed, 115 insertions(+), 111 deletions(-)

Neat series... Just a few comments on my side and on patch 3 is up to you to take the comments or not.

With my comment addressed on patch 2:

Agree, I won't change patch 3 for now, I replied there why.

Quoted 1 line

Thank you! I'll embed this into v2.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#18 lore raw
Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Nuno Sá

On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:

...

Quoted 3 lines
Quoted 1 line

Btw, is it deliberately you don't put accent on "a"?

So far in the upstream it had been started from f1caa90085ef ("iio: dac:
set variable max5522_channels storage-class-specifier to static").

Signature · 2 lines
With Best Regards,
Andy Shevchenko