[PATCH 1/2] iio: Don't include extended name in shared attributes

[PATCH 1/2] iio: Don't include extended name in shared attributes

[Lars-Peter Clausen]

The extended name is channel specific and should not be included in shared
attributes.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
It's a fix, but there are no drivers yet, which have channels with extended
names and shared attributes, so no need to apply it to the fixes branch.
---
 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 acc911a..fd96e8b 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -558,7 +558,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 								    ->channel2],
 						 postfix);
 	} else {
-		if (chan->extend_name == NULL)
+		if (chan->extend_name == NULL || shared_by != IIO_SEPARATE)
 			full_postfix = kstrdup(postfix, GFP_KERNEL);
 		else
 			full_postfix = kasprintf(GFP_KERNEL,
-- 
1.8.0

[PATCH 2/2] iio: Avoid unnecessary kasprintf

[Lars-Peter Clausen]

name_format already contains the final name and no format characters. So the
code basically reads:

	dev_attr->attr.name = kstrdup(GFP_KERNEL, name_format);
	if (dev_attr->attr.name == NULL)
		...
	kfree(name_format);

Which means we can save one alloc and free pair per attribute name if we
directly assign name_format to dev_attr->attr.name.

The patch also renames name_format to name to denote that this is indeed the
final name and has no format characters in it.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
I went back through the log and it looks like it has always been like this.
name_format never actually contained a format string.
---
 drivers/iio/industrialio-core.c | 39 +++++++++++++--------------------------
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index fd96e8b..85a2a07 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -540,7 +540,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 			   enum iio_shared_by shared_by)
 {
 	int ret = 0;
-	char *name_format = NULL;
+	char *name = NULL;
 	char *full_postfix;
 	sysfs_attr_init(&dev_attr->attr);
 
@@ -572,16 +572,15 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 	if (chan->differential) { /* Differential can not have modifier */
 		switch (shared_by) {
 		case IIO_SHARED_BY_ALL:
-			name_format = kasprintf(GFP_KERNEL, "%s", full_postfix);
+			name = kasprintf(GFP_KERNEL, "%s", full_postfix);
 			break;
 		case IIO_SHARED_BY_DIR:
-			name_format = kasprintf(GFP_KERNEL, "%s_%s",
+			name = kasprintf(GFP_KERNEL, "%s_%s",
 						iio_direction[chan->output],
 						full_postfix);
 			break;
 		case IIO_SHARED_BY_TYPE:
-			name_format
-				= kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
+			name = kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
 					    iio_chan_type_name_spec[chan->type],
@@ -593,8 +592,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 				ret = -EINVAL;
 				goto error_free_full_postfix;
 			}
-			name_format
-				= kasprintf(GFP_KERNEL,
+			name = kasprintf(GFP_KERNEL,
 					    "%s_%s%d-%s%d_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
@@ -607,16 +605,15 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 	} else { /* Single ended */
 		switch (shared_by) {
 		case IIO_SHARED_BY_ALL:
-			name_format = kasprintf(GFP_KERNEL, "%s", full_postfix);
+			name = kasprintf(GFP_KERNEL, "%s", full_postfix);
 			break;
 		case IIO_SHARED_BY_DIR:
-			name_format = kasprintf(GFP_KERNEL, "%s_%s",
+			name = kasprintf(GFP_KERNEL, "%s_%s",
 						iio_direction[chan->output],
 						full_postfix);
 			break;
 		case IIO_SHARED_BY_TYPE:
-			name_format
-				= kasprintf(GFP_KERNEL, "%s_%s_%s",
+			name = kasprintf(GFP_KERNEL, "%s_%s_%s",
 					    iio_direction[chan->output],
 					    iio_chan_type_name_spec[chan->type],
 					    full_postfix);
@@ -624,33 +621,24 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 
 		case IIO_SEPARATE:
 			if (chan->indexed)
-				name_format
-					= kasprintf(GFP_KERNEL, "%s_%s%d_%s",
+				name = kasprintf(GFP_KERNEL, "%s_%s%d_%s",
 						    iio_direction[chan->output],
 						    iio_chan_type_name_spec[chan->type],
 						    chan->channel,
 						    full_postfix);
 			else
-				name_format
-					= kasprintf(GFP_KERNEL, "%s_%s_%s",
+				name = kasprintf(GFP_KERNEL, "%s_%s_%s",
 						    iio_direction[chan->output],
 						    iio_chan_type_name_spec[chan->type],
 						    full_postfix);
 			break;
 		}
 	}
-	if (name_format == NULL) {
+	if (name == NULL) {
 		ret = -ENOMEM;
 		goto error_free_full_postfix;
 	}
-	dev_attr->attr.name = kasprintf(GFP_KERNEL,
-					name_format,
-					chan->channel,
-					chan->channel2);
-	if (dev_attr->attr.name == NULL) {
-		ret = -ENOMEM;
-		goto error_free_name_format;
-	}
+	dev_attr->attr.name = name;
 
 	if (readfunc) {
 		dev_attr->attr.mode |= S_IRUGO;
@@ -661,8 +649,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
 		dev_attr->attr.mode |= S_IWUSR;
 		dev_attr->store = writefunc;
 	}
-error_free_name_format:
-	kfree(name_format);
+
 error_free_full_postfix:
 	kfree(full_postfix);
 
-- 
1.8.0

Re: [PATCH 1/2] iio: Don't include extended name in shared attributes

[Jonathan Cameron]

On 14/02/14 14:19, Lars-Peter Clausen wrote:
> The extended name is channel specific and should not be included in shared
> attributes.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Applied to the togreg branch of iio.git

> ---
> It's a fix, but there are no drivers yet, which have channels with extended
> names and shared attributes, so no need to apply it to the fixes branch.
Thanks for making this clear - saves me checking ;)
> ---
>   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 acc911a..fd96e8b 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -558,7 +558,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   								    ->channel2],
>   						 postfix);
>   	} else {
> -		if (chan->extend_name == NULL)
> +		if (chan->extend_name == NULL || shared_by != IIO_SEPARATE)
>   			full_postfix = kstrdup(postfix, GFP_KERNEL);
>   		else
>   			full_postfix = kasprintf(GFP_KERNEL,
>

Re: [PATCH 2/2] iio: Avoid unnecessary kasprintf

[Jonathan Cameron]

On 14/02/14 14:19, Lars-Peter Clausen wrote:
> name_format already contains the final name and no format characters. So the
> code basically reads:
>
> 	dev_attr->attr.name = kstrdup(GFP_KERNEL, name_format);
> 	if (dev_attr->attr.name == NULL)
> 		...
> 	kfree(name_format);
>
> Which means we can save one alloc and free pair per attribute name if we
> directly assign name_format to dev_attr->attr.name.
>
> The patch also renames name_format to name to denote that this is indeed the
> final name and has no format characters in it.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> I went back through the log and it looks like it has always been like this.
> name_format never actually contained a format string.
oops ;)

It did in my mind at some point.

Good spot.  Applied to the togreg branch of iio.git.
> ---
>   drivers/iio/industrialio-core.c | 39 +++++++++++++--------------------------
>   1 file changed, 13 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index fd96e8b..85a2a07 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -540,7 +540,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   			   enum iio_shared_by shared_by)
>   {
>   	int ret = 0;
> -	char *name_format = NULL;
> +	char *name = NULL;
>   	char *full_postfix;
>   	sysfs_attr_init(&dev_attr->attr);
>
> @@ -572,16 +572,15 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   	if (chan->differential) { /* Differential can not have modifier */
>   		switch (shared_by) {
>   		case IIO_SHARED_BY_ALL:
> -			name_format = kasprintf(GFP_KERNEL, "%s", full_postfix);
> +			name = kasprintf(GFP_KERNEL, "%s", full_postfix);
>   			break;
>   		case IIO_SHARED_BY_DIR:
> -			name_format = kasprintf(GFP_KERNEL, "%s_%s",
> +			name = kasprintf(GFP_KERNEL, "%s_%s",
>   						iio_direction[chan->output],
>   						full_postfix);
>   			break;
>   		case IIO_SHARED_BY_TYPE:
> -			name_format
> -				= kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
> +			name = kasprintf(GFP_KERNEL, "%s_%s-%s_%s",
>   					    iio_direction[chan->output],
>   					    iio_chan_type_name_spec[chan->type],
>   					    iio_chan_type_name_spec[chan->type],
> @@ -593,8 +592,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   				ret = -EINVAL;
>   				goto error_free_full_postfix;
>   			}
> -			name_format
> -				= kasprintf(GFP_KERNEL,
> +			name = kasprintf(GFP_KERNEL,
>   					    "%s_%s%d-%s%d_%s",
>   					    iio_direction[chan->output],
>   					    iio_chan_type_name_spec[chan->type],
> @@ -607,16 +605,15 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   	} else { /* Single ended */
>   		switch (shared_by) {
>   		case IIO_SHARED_BY_ALL:
> -			name_format = kasprintf(GFP_KERNEL, "%s", full_postfix);
> +			name = kasprintf(GFP_KERNEL, "%s", full_postfix);
>   			break;
>   		case IIO_SHARED_BY_DIR:
> -			name_format = kasprintf(GFP_KERNEL, "%s_%s",
> +			name = kasprintf(GFP_KERNEL, "%s_%s",
>   						iio_direction[chan->output],
>   						full_postfix);
>   			break;
>   		case IIO_SHARED_BY_TYPE:
> -			name_format
> -				= kasprintf(GFP_KERNEL, "%s_%s_%s",
> +			name = kasprintf(GFP_KERNEL, "%s_%s_%s",
>   					    iio_direction[chan->output],
>   					    iio_chan_type_name_spec[chan->type],
>   					    full_postfix);
> @@ -624,33 +621,24 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>
>   		case IIO_SEPARATE:
>   			if (chan->indexed)
> -				name_format
> -					= kasprintf(GFP_KERNEL, "%s_%s%d_%s",
> +				name = kasprintf(GFP_KERNEL, "%s_%s%d_%s",
>   						    iio_direction[chan->output],
>   						    iio_chan_type_name_spec[chan->type],
>   						    chan->channel,
>   						    full_postfix);
>   			else
> -				name_format
> -					= kasprintf(GFP_KERNEL, "%s_%s_%s",
> +				name = kasprintf(GFP_KERNEL, "%s_%s_%s",
>   						    iio_direction[chan->output],
>   						    iio_chan_type_name_spec[chan->type],
>   						    full_postfix);
>   			break;
>   		}
>   	}
> -	if (name_format == NULL) {
> +	if (name == NULL) {
>   		ret = -ENOMEM;
>   		goto error_free_full_postfix;
>   	}
> -	dev_attr->attr.name = kasprintf(GFP_KERNEL,
> -					name_format,
> -					chan->channel,
> -					chan->channel2);
> -	if (dev_attr->attr.name == NULL) {
> -		ret = -ENOMEM;
> -		goto error_free_name_format;
> -	}
> +	dev_attr->attr.name = name;
>
>   	if (readfunc) {
>   		dev_attr->attr.mode |= S_IRUGO;
> @@ -661,8 +649,7 @@ int __iio_device_attr_init(struct device_attribute *dev_attr,
>   		dev_attr->attr.mode |= S_IWUSR;
>   		dev_attr->store = writefunc;
>   	}
> -error_free_name_format:
> -	kfree(name_format);
> +
>   error_free_full_postfix:
>   	kfree(full_postfix);
>
>