* [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes
@ 2023-07-20 20:53 Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 1/8] iio: core: Add opaque_struct_size() helper and use it Andy Shevchenko
` (8 more replies)
0 siblings, 9 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
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(-)
--
2.40.0.1.gaa8946217a0b
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v1 1/8] iio: core: Add opaque_struct_size() helper and use it
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper Andy Shevchenko
` (7 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen, Uwe Kleine-König
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>
---
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 1/8] iio: core: Add opaque_struct_size() helper and use it Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-21 8:03 ` Nuno Sá
2023-07-20 20:53 ` [PATCH v1 3/8] iio: core: Switch to krealloc_array() Andy Shevchenko
` (6 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
Use sysfs_match_string() helper instead of open coded variant.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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..90e59223b178 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 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)
--
2.40.0.1.gaa8946217a0b
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 1/8] iio: core: Add opaque_struct_size() helper and use it Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-21 7:59 ` Nuno Sá
2023-07-20 20:53 ` [PATCH v1 4/8] iio: core: Use min() instead of min_t() to make code more robust Andy Shevchenko
` (5 subsequent siblings)
8 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
Let the krealloc_array() copy the original data and
check for a multiplication overflow.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 90e59223b178..be154879983e 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 4/8] iio: core: Use min() instead of min_t() to make code more robust
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (2 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 3/8] iio: core: Switch to krealloc_array() Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 5/8] iio: core: Get rid of redundant 'else' Andy Shevchenko
` (4 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
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>
---
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 be154879983e..9753f6552db4 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 5/8] iio: core: Get rid of redundant 'else'
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (3 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 4/8] iio: core: Use min() instead of min_t() to make code more robust Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 6/8] iio: core: Fix issues and style of the comments Andy Shevchenko
` (3 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
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>
---
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 9753f6552db4..ab9c6db69625 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 6/8] iio: core: Fix issues and style of the comments
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (4 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 5/8] iio: core: Get rid of redundant 'else' Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 7/8] iio: core: Move initcalls closer to the respective calls Andy Shevchenko
` (2 subsequent siblings)
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
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>
---
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 ab9c6db69625..e29772940886 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 7/8] iio: core: Move initcalls closer to the respective calls
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (5 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 6/8] iio: core: Fix issues and style of the comments Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 8/8] iio: core: Improve indentation in a few places Andy Shevchenko
2023-07-21 8:06 ` [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Nuno Sá
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
Move subsys_initcall() and module_exit() closer to the respective calls.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 e29772940886..202a1a67ba98 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v1 8/8] iio: core: Improve indentation in a few places
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (6 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 7/8] iio: core: Move initcalls closer to the respective calls Andy Shevchenko
@ 2023-07-20 20:53 ` Andy Shevchenko
2023-07-21 8:06 ` [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Nuno Sá
8 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-20 20:53 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
Improve an indentation in a few places to increase readability.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 202a1a67ba98..1918da2a70ad 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-20 20:53 ` [PATCH v1 3/8] iio: core: Switch to krealloc_array() Andy Shevchenko
@ 2023-07-21 7:59 ` Nuno Sá
2023-07-21 10:14 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-07-21 7:59 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko 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>
> ---
> 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 90e59223b178..be154879983e 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++;
not really related with the change... maybe just mention it in the commit?
> }
> 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) {
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á
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper
2023-07-20 20:53 ` [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper Andy Shevchenko
@ 2023-07-21 8:03 ` Nuno Sá
0 siblings, 0 replies; 19+ messages in thread
From: Nuno Sá @ 2023-07-21 8:03 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
> Use sysfs_match_string() helper instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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..90e59223b178 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 sprintf(buf, "%s\n", clock_names[clk]);
syfs_emit()...
- Nuno Sá
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
` (7 preceding siblings ...)
2023-07-20 20:53 ` [PATCH v1 8/8] iio: core: Improve indentation in a few places Andy Shevchenko
@ 2023-07-21 8:06 ` Nuno Sá
2023-07-21 10:15 ` Andy Shevchenko
2023-07-21 16:55 ` Andy Shevchenko
8 siblings, 2 replies; 19+ messages in thread
From: Nuno Sá @ 2023-07-21 8:06 UTC (permalink / raw)
To: Andy Shevchenko, linux-iio, linux-kernel
Cc: Jonathan Cameron, Lars-Peter Clausen
On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
> 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(-)
>
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:
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-21 7:59 ` Nuno Sá
@ 2023-07-21 10:14 ` Andy Shevchenko
2023-07-21 10:53 ` Nuno Sá
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 10:14 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> > + 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++;
> 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.
> > }
...
> > 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!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes
2023-07-21 8:06 ` [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Nuno Sá
@ 2023-07-21 10:15 ` Andy Shevchenko
2023-07-21 16:55 ` Andy Shevchenko
1 sibling, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 10:15 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 10:06:05AM +0200, Nuno Sá wrote:
> On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
> > 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(-)
> >
>
> 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.
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Thank you! I'll embed this into v2.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-21 10:14 ` Andy Shevchenko
@ 2023-07-21 10:53 ` Nuno Sá
2023-07-21 11:28 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Nuno Sá @ 2023-07-21 10:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
>
> ...
>
> > > + 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++;
>
> > 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).
> > > }
>
> ...
>
> > > 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.
Fair enough...
- Nuno Sá
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-21 10:53 ` Nuno Sá
@ 2023-07-21 11:28 ` Andy Shevchenko
2023-07-21 11:31 ` Andy Shevchenko
0 siblings, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 11:28 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote:
> On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> > > > + 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++;
> >
> > > 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.
> That said, I actually prefer this style (even though some people don't like much
> the ternary operator).
Thanks!
> > > > }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 3/8] iio: core: Switch to krealloc_array()
2023-07-21 11:28 ` Andy Shevchenko
@ 2023-07-21 11:31 ` Andy Shevchenko
0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 11:31 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 02:28:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote:
> > On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> > > > > + 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++;
> > >
> > > > 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.
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.
> > That said, I actually prefer this style (even though some people don't like much
> > the ternary operator).
>
> Thanks!
>
> > > > > }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes
2023-07-21 8:06 ` [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Nuno Sá
2023-07-21 10:15 ` Andy Shevchenko
@ 2023-07-21 16:55 ` Andy Shevchenko
2023-07-21 16:57 ` Andy Shevchenko
1 sibling, 1 reply; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 16:55 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 10:06:05AM +0200, Nuno Sá wrote:
> On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Btw, is it deliberately you don't put accent on "a"?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes
2023-07-21 16:55 ` Andy Shevchenko
@ 2023-07-21 16:57 ` Andy Shevchenko
0 siblings, 0 replies; 19+ messages in thread
From: Andy Shevchenko @ 2023-07-21 16:57 UTC (permalink / raw)
To: Nuno Sá
Cc: linux-iio, linux-kernel, Jonathan Cameron, Lars-Peter Clausen
On Fri, Jul 21, 2023 at 07:55:22PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 10:06:05AM +0200, Nuno Sá wrote:
> > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
> 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").
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-07-21 16:58 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-20 20:53 [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 1/8] iio: core: Add opaque_struct_size() helper and use it Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 2/8] iio: core: Use sysfs_match_string() helper Andy Shevchenko
2023-07-21 8:03 ` Nuno Sá
2023-07-20 20:53 ` [PATCH v1 3/8] iio: core: Switch to krealloc_array() Andy Shevchenko
2023-07-21 7:59 ` Nuno Sá
2023-07-21 10:14 ` Andy Shevchenko
2023-07-21 10:53 ` Nuno Sá
2023-07-21 11:28 ` Andy Shevchenko
2023-07-21 11:31 ` Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 4/8] iio: core: Use min() instead of min_t() to make code more robust Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 5/8] iio: core: Get rid of redundant 'else' Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 6/8] iio: core: Fix issues and style of the comments Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 7/8] iio: core: Move initcalls closer to the respective calls Andy Shevchenko
2023-07-20 20:53 ` [PATCH v1 8/8] iio: core: Improve indentation in a few places Andy Shevchenko
2023-07-21 8:06 ` [PATCH v1 0/8] iio: core: A few code cleanups and documentation fixes Nuno Sá
2023-07-21 10:15 ` Andy Shevchenko
2023-07-21 16:55 ` Andy Shevchenko
2023-07-21 16:57 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.