iio: core: A few code cleanups and documentation fixes
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
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.
--- drivers/iio/industrialio-core.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)
drivers/iio/industrialio-core.c +24 -8
@@ -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
PATCH 2/8 iio: core: Use sysfs_match_string() helper
In reply to Andy ShevchenkoUse 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
@@ -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
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.Signed-off-by Andy Shevchenko <[email protected]>---
drivers/iio/industrialio-core.c | 81 +++++++++++++--------------------1 file changed, 31 insertions(+), 50 deletions(-)drivers/iio/industrialio-core.c +0
-0@@ -1400,50 +1400,32 @@ static ssize_t label_show(struct device *dev, struct device_attribute *attr,
staticDEVICE_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_tcurrent_timestamp_clock_show(struct device *dev, struct device_attribute *attr, char *buf)
{
const structiio_dev*indio_dev=dev_to_iio_dev(dev); constclockid_tclk =iio_device_get_clock(indio_dev); - const char *name;
- ssize_t sz;
- switch (clk) {
- caseCLOCK_REALTIME:
- name = "realtime\n";
- sz =sizeof("realtime\n");
- break;
- caseCLOCK_MONOTONIC:
- name = "monotonic\n";
- sz =sizeof("monotonic\n");
- break;
- caseCLOCK_MONOTONIC_RAW:
- name = "monotonic_raw\n";
- sz =sizeof("monotonic_raw\n");
- break;
- caseCLOCK_REALTIME_COARSE:
- name = "realtime_coarse\n";
- sz =sizeof("realtime_coarse\n");
- break;
- caseCLOCK_MONOTONIC_COARSE:
- name = "monotonic_coarse\n";
- sz =sizeof("monotonic_coarse\n");
- break;
- caseCLOCK_BOOTTIME:
- name = "boottime\n";
- sz =sizeof("boottime\n");
- break;
- caseCLOCK_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;
+ returnsprintf(buf, "%s\n", clock_names[clk]);
syfs_emit()...
- Nuno Sá
PATCH 3/8 iio: core: Switch to krealloc_array()
In reply to Andy ShevchenkoLet 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
@@ -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
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.Signed-off-by Andy Shevchenko <[email protected]>---
drivers/iio/industrialio-core.c | 20 +++++++-------------1 file changed, 7 insertions(+), 13 deletions(-)drivers/iio/industrialio-core.c +0
-0@@ -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á
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
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á
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
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
PATCH 4/8 iio: core: Use min() instead of min_t() to make code more robust
In reply to Andy Shevchenkomin() 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
@@ -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
PATCH 5/8 iio: core: Get rid of redundant 'else'
In reply to Andy ShevchenkoIn 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
@@ -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
PATCH 6/8 iio: core: Fix issues and style of the comments
In reply to Andy ShevchenkoThe 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
@@ -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
PATCH 7/8 iio: core: Move initcalls closer to the respective calls
In reply to Andy ShevchenkoMove 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
@@ -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
PATCH 8/8 iio: core: Improve indentation in a few places
In reply to Andy ShevchenkoImprove 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
@@ -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
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: Addopaque_struct_size()helper and use it
iio: core: Usesysfs_match_string()helper
iio: core: Switch tokrealloc_array()
iio: core: Usemin()instead ofmin_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:
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: Addopaque_struct_size()helper and use it
iio: core: Usesysfs_match_string()helper
iio: core: Switch tokrealloc_array()
iio: core: Usemin()instead ofmin_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
Reviewed-by Nuno Sa <[email protected]>
Thank you! I'll embed this into v2.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 1 line · Nuno Sá
On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
Quoted 1 line
Reviewed-by Nuno Sa <[email protected]>
Btw, is it deliberately you don't put accent on "a"?
Signature · 2 lines
With Best Regards, Andy Shevchenko
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
Reviewed-by Nuno Sa <[email protected]>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