linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
@ 2018-05-31 14:01 Bastian Germann
  2018-05-31 14:01 ` [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management Bastian Germann
  2018-05-31 14:27 ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
  0 siblings, 2 replies; 16+ messages in thread
From: Bastian Germann @ 2018-05-31 14:01 UTC (permalink / raw)
  To: Luca Tettamanti
  Cc: Bastian Germann, Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel

Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
of the deprecated hwmon_device_register.
Construct the expected attribute_group array from the sensor list which
contains all needed attributes.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 71 +++++++++++++++++++++++++++++-------
 1 file changed, 58 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 975c43d446f8..8a35fb27ff50 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -125,6 +125,7 @@ struct atk_data {
 	int temperature_count;
 	int fan_count;
 	struct list_head sensor_list;
+	const struct attribute_group **attr_groups;
 
 	struct {
 		struct dentry *root;
@@ -1242,27 +1243,67 @@ static void atk_free_sensors(struct atk_data *data)
 	}
 }
 
+static int atk_init_attribute_groups(struct atk_data *data)
+{
+	struct atk_sensor_data *s;
+	struct attribute **attrs;
+	struct attribute_group *group;
+	const struct attribute_group **groups;
+	int i = 0;
+	int len = (data->voltage_count + data->temperature_count
+			+ data->fan_count) * 4 + 1;
+
+	attrs = kcalloc(len, sizeof(struct attribute *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
+
+	list_for_each_entry(s, &data->sensor_list, list) {
+		attrs[i++] = &s->input_attr.attr;
+		attrs[i++] = &s->label_attr.attr;
+		attrs[i++] = &s->limit1_attr.attr;
+		attrs[i++] = &s->limit2_attr.attr;
+	}
+	attrs[i] = NULL;
+
+	group = kzalloc(sizeof(*group), GFP_KERNEL);
+	if (!group)
+		goto cleanup;
+	group->attrs = attrs;
+
+	groups = kcalloc(2, sizeof(struct attribute_group *), GFP_KERNEL);
+	if (!groups) {
+		kfree(group);
+		goto cleanup;
+	}
+	groups[0] = group;
+	groups[1] = NULL;
+	data->attr_groups = groups;
+
+	return 0;
+cleanup:
+	kfree(attrs);
+	return -ENOMEM;
+}
+
+static void atk_free_attribute_groups(struct atk_data *data)
+{
+	kfree(data->attr_groups[0]->attrs);
+	kfree(data->attr_groups[0]);
+	kfree(data->attr_groups);
+}
+
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
-	int err;
 
 	dev_dbg(dev, "registering hwmon device\n");
-	data->hwmon_dev = hwmon_device_register(dev);
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
+							    data,
+							    data->attr_groups);
 	if (IS_ERR(data->hwmon_dev))
 		return PTR_ERR(data->hwmon_dev);
 
-	dev_dbg(dev, "populating sysfs directory\n");
-	err = atk_create_files(data);
-	if (err)
-		goto remove;
-
 	return 0;
-remove:
-	/* Cleanup the registered files */
-	atk_remove_files(data);
-	hwmon_device_unregister(data->hwmon_dev);
-	return err;
 }
 
 static int atk_probe_if(struct atk_data *data)
@@ -1397,6 +1438,9 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	}
 
+	err = atk_init_attribute_groups(data);
+	if (err)
+		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
 		goto cleanup;
@@ -1407,6 +1451,7 @@ static int atk_add(struct acpi_device *device)
 	return 0;
 cleanup:
 	atk_free_sensors(data);
+	atk_free_attribute_groups(data);
 out:
 	if (data->disable_ec)
 		atk_ec_ctl(data, 0);
@@ -1423,9 +1468,9 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_remove_files(data);
 	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
+	atk_free_attribute_groups(data);
 
 	if (data->disable_ec) {
 		if (atk_ec_ctl(data, 0))
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management
  2018-05-31 14:01 [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
@ 2018-05-31 14:01 ` Bastian Germann
  2018-05-31 14:28   ` Guenter Roeck
  2018-05-31 14:27 ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
  1 sibling, 1 reply; 16+ messages in thread
From: Bastian Germann @ 2018-05-31 14:01 UTC (permalink / raw)
  To: Luca Tettamanti
  Cc: Bastian Germann, Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel

Remove the manual sysfs file creation and deletion that are now taken care
of by the (un)register calls via an attribute_group array.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 46 ------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 8a35fb27ff50..2f4955ec8437 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -263,14 +263,6 @@ static ssize_t atk_limit2_show(struct device *dev,
 	return sprintf(buf, "%lld\n", value);
 }
 
-static ssize_t atk_name_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "atk0110\n");
-}
-static struct device_attribute atk_name_attr =
-		__ATTR(name, 0444, atk_name_show, NULL);
-
 static void atk_init_attribute(struct device_attribute *attr, char *name,
 		sysfs_show_func show)
 {
@@ -1194,44 +1186,6 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
 	return err;
 }
 
-static int atk_create_files(struct atk_data *data)
-{
-	struct atk_sensor_data *s;
-	int err;
-
-	list_for_each_entry(s, &data->sensor_list, list) {
-		err = device_create_file(data->hwmon_dev, &s->input_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->label_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
-		if (err)
-			return err;
-	}
-
-	err = device_create_file(data->hwmon_dev, &atk_name_attr);
-
-	return err;
-}
-
-static void atk_remove_files(struct atk_data *data)
-{
-	struct atk_sensor_data *s;
-
-	list_for_each_entry(s, &data->sensor_list, list) {
-		device_remove_file(data->hwmon_dev, &s->input_attr);
-		device_remove_file(data->hwmon_dev, &s->label_attr);
-		device_remove_file(data->hwmon_dev, &s->limit1_attr);
-		device_remove_file(data->hwmon_dev, &s->limit2_attr);
-	}
-	device_remove_file(data->hwmon_dev, &atk_name_attr);
-}
-
 static void atk_free_sensors(struct atk_data *data)
 {
 	struct list_head *head = &data->sensor_list;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-05-31 14:01 [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
  2018-05-31 14:01 ` [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management Bastian Germann
@ 2018-05-31 14:27 ` Guenter Roeck
  2018-05-31 22:56   ` Bastian Germann
  1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2018-05-31 14:27 UTC (permalink / raw)
  To: Bastian Germann, Luca Tettamanti; +Cc: Jean Delvare, linux-hwmon, linux-kernel

Hi Bastian,

On 05/31/2018 07:01 AM, Bastian Germann wrote:
> Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
> of the deprecated hwmon_device_register.
> Construct the expected attribute_group array from the sensor list which
> contains all needed attributes.
> 
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
> ---
>   drivers/hwmon/asus_atk0110.c | 71 +++++++++++++++++++++++++++++-------
>   1 file changed, 58 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 975c43d446f8..8a35fb27ff50 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -125,6 +125,7 @@ struct atk_data {
>   	int temperature_count;
>   	int fan_count;
>   	struct list_head sensor_list;
> +	const struct attribute_group **attr_groups;
>   
>   	struct {
>   		struct dentry *root;
> @@ -1242,27 +1243,67 @@ static void atk_free_sensors(struct atk_data *data)
>   	}
>   }
>   
> +static int atk_init_attribute_groups(struct atk_data *data)
> +{
> +	struct atk_sensor_data *s;
> +	struct attribute **attrs;
> +	struct attribute_group *group;
> +	const struct attribute_group **groups;
> +	int i = 0;
> +	int len = (data->voltage_count + data->temperature_count
> +			+ data->fan_count) * 4 + 1;
> +
> +	attrs = kcalloc(len, sizeof(struct attribute *), GFP_KERNEL);

Please use devm_kcalloc().

> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	list_for_each_entry(s, &data->sensor_list, list) {
> +		attrs[i++] = &s->input_attr.attr;
> +		attrs[i++] = &s->label_attr.attr;
> +		attrs[i++] = &s->limit1_attr.attr;
> +		attrs[i++] = &s->limit2_attr.attr;
> +	}
> +	attrs[i] = NULL;
> +
> +	group = kzalloc(sizeof(*group), GFP_KERNEL);

devm_kzalloc()

> +	if (!group)
> +		goto cleanup;
> +	group->attrs = attrs;
> +
> +	groups = kcalloc(2, sizeof(struct attribute_group *), GFP_KERNEL);

devm_kcalloc()

> +	if (!groups) {
> +		kfree(group);
> +		goto cleanup;
> +	}
> +	groups[0] = group;
> +	groups[1] = NULL;

kcalloc() clears the allocated memory, so this is unnecessary. However,
I would suggest to make group and groups part of struct atk_sensor_data.
The size is static, and the fields will always be needed, so dynamic
allocation doesn't buy anything except overhead.

> +	data->attr_groups = groups;
> + > +	return 0;
> +cleanup:
> +	kfree(attrs);

then you won't need the kfree() calls.

The same should be done with the existing calls of kzalloc(),
but that should be a separate patch.

> +	return -ENOMEM;
> +}
> +
> +static void atk_free_attribute_groups(struct atk_data *data)
> +{
> +	kfree(data->attr_groups[0]->attrs);
> +	kfree(data->attr_groups[0]);
> +	kfree(data->attr_groups);
> +}
> +
>   static int atk_register_hwmon(struct atk_data *data)
>   {
>   	struct device *dev = &data->acpi_dev->dev;
> -	int err;
>   
>   	dev_dbg(dev, "registering hwmon device\n");
> -	data->hwmon_dev = hwmon_device_register(dev);
> +	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
> +							    data,
> +							    data->attr_groups);
>   	if (IS_ERR(data->hwmon_dev))
>   		return PTR_ERR(data->hwmon_dev);
>   
> -	dev_dbg(dev, "populating sysfs directory\n");
> -	err = atk_create_files(data);
> -	if (err)
> -		goto remove;
> -
>   	return 0;
> -remove:
> -	/* Cleanup the registered files */
> -	atk_remove_files(data);
> -	hwmon_device_unregister(data->hwmon_dev);
> -	return err;
>   }
>   
>   static int atk_probe_if(struct atk_data *data)
> @@ -1397,6 +1438,9 @@ static int atk_add(struct acpi_device *device)
>   		goto out;
>   	}
>   
> +	err = atk_init_attribute_groups(data);
> +	if (err)
> +		goto out;
>   	err = atk_register_hwmon(data);
>   	if (err)
>   		goto cleanup;
> @@ -1407,6 +1451,7 @@ static int atk_add(struct acpi_device *device)
>   	return 0;
>   cleanup:
>   	atk_free_sensors(data);
> +	atk_free_attribute_groups(data);
>   out:
>   	if (data->disable_ec)
>   		atk_ec_ctl(data, 0);
> @@ -1423,9 +1468,9 @@ static int atk_remove(struct acpi_device *device)
>   
>   	atk_debugfs_cleanup(data);
>   
> -	atk_remove_files(data);

You should also remove the no longer needed functions as well as the
name attribute.

>   	atk_free_sensors(data);
>   	hwmon_device_unregister(data->hwmon_dev);
> +	atk_free_attribute_groups(data);
>   
>   	if (data->disable_ec) {
>   		if (atk_ec_ctl(data, 0))
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management
  2018-05-31 14:01 ` [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management Bastian Germann
@ 2018-05-31 14:28   ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2018-05-31 14:28 UTC (permalink / raw)
  To: Bastian Germann, Luca Tettamanti; +Cc: Jean Delvare, linux-hwmon, linux-kernel

On 05/31/2018 07:01 AM, Bastian Germann wrote:
> Remove the manual sysfs file creation and deletion that are now taken care
> of by the (un)register calls via an attribute_group array.
> 
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>

Ah, there is the removal. That needs to be part of patch #1.

Thanks,
Guenter

> ---
>   drivers/hwmon/asus_atk0110.c | 46 ------------------------------------
>   1 file changed, 46 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 8a35fb27ff50..2f4955ec8437 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -263,14 +263,6 @@ static ssize_t atk_limit2_show(struct device *dev,
>   	return sprintf(buf, "%lld\n", value);
>   }
>   
> -static ssize_t atk_name_show(struct device *dev,
> -				struct device_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "atk0110\n");
> -}
> -static struct device_attribute atk_name_attr =
> -		__ATTR(name, 0444, atk_name_show, NULL);
> -
>   static void atk_init_attribute(struct device_attribute *attr, char *name,
>   		sysfs_show_func show)
>   {
> @@ -1194,44 +1186,6 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
>   	return err;
>   }
>   
> -static int atk_create_files(struct atk_data *data)
> -{
> -	struct atk_sensor_data *s;
> -	int err;
> -
> -	list_for_each_entry(s, &data->sensor_list, list) {
> -		err = device_create_file(data->hwmon_dev, &s->input_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->label_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
> -		if (err)
> -			return err;
> -	}
> -
> -	err = device_create_file(data->hwmon_dev, &atk_name_attr);
> -
> -	return err;
> -}
> -
> -static void atk_remove_files(struct atk_data *data)
> -{
> -	struct atk_sensor_data *s;
> -
> -	list_for_each_entry(s, &data->sensor_list, list) {
> -		device_remove_file(data->hwmon_dev, &s->input_attr);
> -		device_remove_file(data->hwmon_dev, &s->label_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit1_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit2_attr);
> -	}
> -	device_remove_file(data->hwmon_dev, &atk_name_attr);
> -}
> -
>   static void atk_free_sensors(struct atk_data *data)
>   {
>   	struct list_head *head = &data->sensor_list;
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-05-31 14:27 ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
@ 2018-05-31 22:56   ` Bastian Germann
  2018-05-31 22:57     ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
  0 siblings, 1 reply; 16+ messages in thread
From: Bastian Germann @ 2018-05-31 22:56 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, linux-hwmon,
	linux-kernel

Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
of the deprecated hwmon_device_register.
Construct the expected attribute_group array from the sensor list which
contains all needed attributes.
Remove the manual sysfs file creation and deletion that are now taken care
of by the (un)register calls via the attribute_group array.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
 1 file changed, 25 insertions(+), 50 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 975c43d446f8..33748cc07acc 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -125,6 +125,8 @@ struct atk_data {
 	int temperature_count;
 	int fan_count;
 	struct list_head sensor_list;
+	struct attribute_group attr_group[2];
+	const struct attribute_group *attr_groups[2];
 
 	struct {
 		struct dentry *root;
@@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
 	return sprintf(buf, "%lld\n", value);
 }
 
-static ssize_t atk_name_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "atk0110\n");
-}
-static struct device_attribute atk_name_attr =
-		__ATTR(name, 0444, atk_name_show, NULL);
-
 static void atk_init_attribute(struct device_attribute *attr, char *name,
 		sysfs_show_func show)
 {
@@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
 	return err;
 }
 
-static int atk_create_files(struct atk_data *data)
+static int atk_init_attribute_groups(struct atk_data *data)
 {
+	struct device *dev = &data->acpi_dev->dev;
 	struct atk_sensor_data *s;
-	int err;
+	struct attribute **attrs;
+	int i = 0;
+	int len = (data->voltage_count + data->temperature_count
+			+ data->fan_count) * 4 + 1;
+
+	attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
 
 	list_for_each_entry(s, &data->sensor_list, list) {
-		err = device_create_file(data->hwmon_dev, &s->input_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->label_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
-		if (err)
-			return err;
+		attrs[i++] = &s->input_attr.attr;
+		attrs[i++] = &s->label_attr.attr;
+		attrs[i++] = &s->limit1_attr.attr;
+		attrs[i++] = &s->limit2_attr.attr;
 	}
 
-	err = device_create_file(data->hwmon_dev, &atk_name_attr);
+	data->attr_group[0].attrs = attrs;
+	data->attr_groups[0] = data->attr_group;
 
-	return err;
-}
-
-static void atk_remove_files(struct atk_data *data)
-{
-	struct atk_sensor_data *s;
-
-	list_for_each_entry(s, &data->sensor_list, list) {
-		device_remove_file(data->hwmon_dev, &s->input_attr);
-		device_remove_file(data->hwmon_dev, &s->label_attr);
-		device_remove_file(data->hwmon_dev, &s->limit1_attr);
-		device_remove_file(data->hwmon_dev, &s->limit2_attr);
-	}
-	device_remove_file(data->hwmon_dev, &atk_name_attr);
+	return 0;
 }
 
 static void atk_free_sensors(struct atk_data *data)
@@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
-	int err;
 
 	dev_dbg(dev, "registering hwmon device\n");
-	data->hwmon_dev = hwmon_device_register(dev);
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
+							    data,
+							    data->attr_groups);
 	if (IS_ERR(data->hwmon_dev))
 		return PTR_ERR(data->hwmon_dev);
 
-	dev_dbg(dev, "populating sysfs directory\n");
-	err = atk_create_files(data);
-	if (err)
-		goto remove;
-
 	return 0;
-remove:
-	/* Cleanup the registered files */
-	atk_remove_files(data);
-	hwmon_device_unregister(data->hwmon_dev);
-	return err;
 }
 
 static int atk_probe_if(struct atk_data *data)
@@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	}
 
+	err = atk_init_attribute_groups(data);
+	if (err)
+		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
 		goto cleanup;
@@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_remove_files(data);
 	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory
  2018-05-31 22:56   ` Bastian Germann
@ 2018-05-31 22:57     ` Bastian Germann
  2018-06-01  9:54       ` Andy Shevchenko
  0 siblings, 1 reply; 16+ messages in thread
From: Bastian Germann @ 2018-05-31 22:57 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, linux-hwmon,
	linux-kernel

Use devm_* variants of kstrdup and kzalloc. Get rid of the kfree cleanups.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 54 +++++++-----------------------------
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 33748cc07acc..8bf097bf6f24 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -190,7 +190,6 @@ static int atk_add(struct acpi_device *device);
 static int atk_remove(struct acpi_device *device);
 static void atk_print_sensor(struct atk_data *data, union acpi_object *obj);
 static int atk_read_value(struct atk_sensor_data *sensor, u64 *value);
-static void atk_free_sensors(struct atk_data *data);
 
 static struct acpi_driver atk_driver = {
 	.name	= ATK_HID,
@@ -722,6 +721,7 @@ static void atk_pack_print(char *buf, size_t sz, union acpi_object *pack)
 static int atk_debugfs_ggrp_open(struct inode *inode, struct file *file)
 {
 	struct atk_data *data = inode->i_private;
+	struct device *dev = &data->acpi_dev->dev;
 	char *buf = NULL;
 	union acpi_object *ret;
 	u8 cls;
@@ -748,7 +748,7 @@ static int atk_debugfs_ggrp_open(struct inode *inode, struct file *file)
 		id = &pack->package.elements[0];
 		if (id->integer.value == data->debugfs.id) {
 			/* Print the package */
-			buf = kzalloc(512, GFP_KERNEL);
+			buf = devm_kzalloc(dev, 512, GFP_KERNEL);
 			if (!buf) {
 				ACPI_FREE(ret);
 				return -ENOMEM;
@@ -776,16 +776,9 @@ static ssize_t atk_debugfs_ggrp_read(struct file *file, char __user *buf,
 	return simple_read_from_buffer(buf, count, pos, str, len);
 }
 
-static int atk_debugfs_ggrp_release(struct inode *inode, struct file *file)
-{
-	kfree(file->private_data);
-	return 0;
-}
-
 static const struct file_operations atk_debugfs_ggrp_fops = {
 	.read		= atk_debugfs_ggrp_read,
 	.open		= atk_debugfs_ggrp_open,
-	.release	= atk_debugfs_ggrp_release,
 	.llseek		= no_llseek,
 };
 
@@ -906,15 +899,13 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	limit1 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
 	limit2 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
 
-	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
 		return -ENOMEM;
 
-	sensor->acpi_name = kstrdup(name->string.pointer, GFP_KERNEL);
-	if (!sensor->acpi_name) {
-		err = -ENOMEM;
-		goto out;
-	}
+	sensor->acpi_name = devm_kstrdup(dev, name->string.pointer, GFP_KERNEL);
+	if (!sensor->acpi_name)
+		return -ENOMEM;
 
 	INIT_LIST_HEAD(&sensor->list);
 	sensor->type = type;
@@ -955,9 +946,6 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	(*num)++;
 
 	return 1;
-out:
-	kfree(sensor);
-	return err;
 }
 
 static int atk_enumerate_old_hwmon(struct atk_data *data)
@@ -998,8 +986,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_TMP ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1020,8 +1007,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_FAN ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1035,9 +1021,6 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 	ACPI_FREE(buf.pointer);
 
 	return count;
-cleanup:
-	atk_free_sensors(data);
-	return ret;
 }
 
 static int atk_ec_present(struct atk_data *data)
@@ -1213,17 +1196,6 @@ static int atk_init_attribute_groups(struct atk_data *data)
 	return 0;
 }
 
-static void atk_free_sensors(struct atk_data *data)
-{
-	struct list_head *head = &data->sensor_list;
-	struct atk_sensor_data *s, *tmp;
-
-	list_for_each_entry_safe(s, tmp, head, list) {
-		kfree(s->acpi_name);
-		kfree(s);
-	}
-}
-
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
@@ -1323,7 +1295,7 @@ static int atk_add(struct acpi_device *device)
 
 	dev_dbg(&device->dev, "adding...\n");
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&device->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -1375,18 +1347,15 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
-		goto cleanup;
+		goto out;
 
 	atk_debugfs_init(data);
 
 	device->driver_data = data;
 	return 0;
-cleanup:
-	atk_free_sensors(data);
 out:
 	if (data->disable_ec)
 		atk_ec_ctl(data, 0);
-	kfree(data);
 	return err;
 }
 
@@ -1399,7 +1368,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
 	if (data->disable_ec) {
@@ -1407,8 +1375,6 @@ static int atk_remove(struct acpi_device *device)
 			dev_err(&device->dev, "Failed to disable EC\n");
 	}
 
-	kfree(data);
-
 	return 0;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory
  2018-05-31 22:57     ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
@ 2018-06-01  9:54       ` Andy Shevchenko
  2018-06-01 11:07         ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
  0 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2018-06-01  9:54 UTC (permalink / raw)
  To: Bastian Germann
  Cc: Guenter Roeck, Luca Tettamanti, Jean Delvare, linux-hwmon,
	Linux Kernel Mailing List

On Fri, Jun 1, 2018 at 1:57 AM, Bastian Germann
<bastiangermann@fishpost.de> wrote:
> Use devm_* variants of kstrdup and kzalloc. Get rid of the kfree cleanups.

>  static int atk_debugfs_ggrp_open(struct inode *inode, struct file *file)
>  {
>         struct atk_data *data = inode->i_private;
> +       struct device *dev = &data->acpi_dev->dev;
>         char *buf = NULL;
>         union acpi_object *ret;
>         u8 cls;
> @@ -748,7 +748,7 @@ static int atk_debugfs_ggrp_open(struct inode *inode, struct file *file)
>                 id = &pack->package.elements[0];
>                 if (id->integer.value == data->debugfs.id) {
>                         /* Print the package */
> -                       buf = kzalloc(512, GFP_KERNEL);
> +                       buf = devm_kzalloc(dev, 512, GFP_KERNEL);
>                         if (!buf) {
>                                 ACPI_FREE(ret);
>                                 return -ENOMEM;

Looking to the function name, it feels like you are creating a memory
leak and devm_ is inappropriate here.

> @@ -776,16 +776,9 @@ static ssize_t atk_debugfs_ggrp_read(struct file *file, char __user *buf,
>         return simple_read_from_buffer(buf, count, pos, str, len);
>  }
>
> -static int atk_debugfs_ggrp_release(struct inode *inode, struct file *file)
> -{
> -       kfree(file->private_data);
> -       return 0;
> -}
> -
>  static const struct file_operations atk_debugfs_ggrp_fops = {
>         .read           = atk_debugfs_ggrp_read,
>         .open           = atk_debugfs_ggrp_open,
> -       .release        = atk_debugfs_ggrp_release,
>         .llseek         = no_llseek,
>  };

So do these.

Also the question of time to leave of the objects used for debugfs
communication. Please double check that it's not affected by the
change.

Otherwise looks good.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-06-01  9:54       ` Andy Shevchenko
@ 2018-06-01 11:07         ` Bastian Germann
  2018-06-01 11:07           ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
  2018-06-01 13:28           ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
  0 siblings, 2 replies; 16+ messages in thread
From: Bastian Germann @ 2018-06-01 11:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, Guenter Roeck,
	linux-hwmon, linux-kernel

Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
of the deprecated hwmon_device_register.
Construct the expected attribute_group array from the sensor list which
contains all needed attributes.
Remove the manual sysfs file creation and deletion that are now taken care
of by the (un)register calls via the attribute_group array.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
 1 file changed, 25 insertions(+), 50 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 975c43d446f8..33748cc07acc 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -125,6 +125,8 @@ struct atk_data {
 	int temperature_count;
 	int fan_count;
 	struct list_head sensor_list;
+	struct attribute_group attr_group[2];
+	const struct attribute_group *attr_groups[2];
 
 	struct {
 		struct dentry *root;
@@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
 	return sprintf(buf, "%lld\n", value);
 }
 
-static ssize_t atk_name_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "atk0110\n");
-}
-static struct device_attribute atk_name_attr =
-		__ATTR(name, 0444, atk_name_show, NULL);
-
 static void atk_init_attribute(struct device_attribute *attr, char *name,
 		sysfs_show_func show)
 {
@@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
 	return err;
 }
 
-static int atk_create_files(struct atk_data *data)
+static int atk_init_attribute_groups(struct atk_data *data)
 {
+	struct device *dev = &data->acpi_dev->dev;
 	struct atk_sensor_data *s;
-	int err;
+	struct attribute **attrs;
+	int i = 0;
+	int len = (data->voltage_count + data->temperature_count
+			+ data->fan_count) * 4 + 1;
+
+	attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
 
 	list_for_each_entry(s, &data->sensor_list, list) {
-		err = device_create_file(data->hwmon_dev, &s->input_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->label_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
-		if (err)
-			return err;
+		attrs[i++] = &s->input_attr.attr;
+		attrs[i++] = &s->label_attr.attr;
+		attrs[i++] = &s->limit1_attr.attr;
+		attrs[i++] = &s->limit2_attr.attr;
 	}
 
-	err = device_create_file(data->hwmon_dev, &atk_name_attr);
+	data->attr_group[0].attrs = attrs;
+	data->attr_groups[0] = data->attr_group;
 
-	return err;
-}
-
-static void atk_remove_files(struct atk_data *data)
-{
-	struct atk_sensor_data *s;
-
-	list_for_each_entry(s, &data->sensor_list, list) {
-		device_remove_file(data->hwmon_dev, &s->input_attr);
-		device_remove_file(data->hwmon_dev, &s->label_attr);
-		device_remove_file(data->hwmon_dev, &s->limit1_attr);
-		device_remove_file(data->hwmon_dev, &s->limit2_attr);
-	}
-	device_remove_file(data->hwmon_dev, &atk_name_attr);
+	return 0;
 }
 
 static void atk_free_sensors(struct atk_data *data)
@@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
-	int err;
 
 	dev_dbg(dev, "registering hwmon device\n");
-	data->hwmon_dev = hwmon_device_register(dev);
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
+							    data,
+							    data->attr_groups);
 	if (IS_ERR(data->hwmon_dev))
 		return PTR_ERR(data->hwmon_dev);
 
-	dev_dbg(dev, "populating sysfs directory\n");
-	err = atk_create_files(data);
-	if (err)
-		goto remove;
-
 	return 0;
-remove:
-	/* Cleanup the registered files */
-	atk_remove_files(data);
-	hwmon_device_unregister(data->hwmon_dev);
-	return err;
 }
 
 static int atk_probe_if(struct atk_data *data)
@@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	}
 
+	err = atk_init_attribute_groups(data);
+	if (err)
+		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
 		goto cleanup;
@@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_remove_files(data);
 	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory
  2018-06-01 11:07         ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
@ 2018-06-01 11:07           ` Bastian Germann
  2018-06-01 13:28           ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
  1 sibling, 0 replies; 16+ messages in thread
From: Bastian Germann @ 2018-06-01 11:07 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, Guenter Roeck,
	linux-hwmon, linux-kernel

Use devm_* variants of kstrdup and kzalloc. Get rid of kfree cleanups.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 44 +++++++-----------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 33748cc07acc..91af065d0e4d 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -190,7 +190,6 @@ static int atk_add(struct acpi_device *device);
 static int atk_remove(struct acpi_device *device);
 static void atk_print_sensor(struct atk_data *data, union acpi_object *obj);
 static int atk_read_value(struct atk_sensor_data *sensor, u64 *value);
-static void atk_free_sensors(struct atk_data *data);
 
 static struct acpi_driver atk_driver = {
 	.name	= ATK_HID,
@@ -906,15 +905,13 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	limit1 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
 	limit2 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
 
-	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
 		return -ENOMEM;
 
-	sensor->acpi_name = kstrdup(name->string.pointer, GFP_KERNEL);
-	if (!sensor->acpi_name) {
-		err = -ENOMEM;
-		goto out;
-	}
+	sensor->acpi_name = devm_kstrdup(dev, name->string.pointer, GFP_KERNEL);
+	if (!sensor->acpi_name)
+		return -ENOMEM;
 
 	INIT_LIST_HEAD(&sensor->list);
 	sensor->type = type;
@@ -955,9 +952,6 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	(*num)++;
 
 	return 1;
-out:
-	kfree(sensor);
-	return err;
 }
 
 static int atk_enumerate_old_hwmon(struct atk_data *data)
@@ -998,8 +992,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_TMP ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1020,8 +1013,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_FAN ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1035,9 +1027,6 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 	ACPI_FREE(buf.pointer);
 
 	return count;
-cleanup:
-	atk_free_sensors(data);
-	return ret;
 }
 
 static int atk_ec_present(struct atk_data *data)
@@ -1213,17 +1202,6 @@ static int atk_init_attribute_groups(struct atk_data *data)
 	return 0;
 }
 
-static void atk_free_sensors(struct atk_data *data)
-{
-	struct list_head *head = &data->sensor_list;
-	struct atk_sensor_data *s, *tmp;
-
-	list_for_each_entry_safe(s, tmp, head, list) {
-		kfree(s->acpi_name);
-		kfree(s);
-	}
-}
-
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
@@ -1323,7 +1301,7 @@ static int atk_add(struct acpi_device *device)
 
 	dev_dbg(&device->dev, "adding...\n");
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&device->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -1375,18 +1353,15 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
-		goto cleanup;
+		goto out;
 
 	atk_debugfs_init(data);
 
 	device->driver_data = data;
 	return 0;
-cleanup:
-	atk_free_sensors(data);
 out:
 	if (data->disable_ec)
 		atk_ec_ctl(data, 0);
-	kfree(data);
 	return err;
 }
 
@@ -1399,7 +1374,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
 	if (data->disable_ec) {
@@ -1407,8 +1381,6 @@ static int atk_remove(struct acpi_device *device)
 			dev_err(&device->dev, "Failed to disable EC\n");
 	}
 
-	kfree(data);
-
 	return 0;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-06-01 11:07         ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
  2018-06-01 11:07           ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
@ 2018-06-01 13:28           ` Guenter Roeck
  2018-06-01 15:14             ` asus_atk0110: Use non-deprecated registration and managed memory Bastian Germann
  1 sibling, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2018-06-01 13:28 UTC (permalink / raw)
  To: Bastian Germann, Andy Shevchenko
  Cc: Luca Tettamanti, Jean Delvare, linux-hwmon, linux-kernel

On 06/01/2018 04:07 AM, Bastian Germann wrote:
> Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
> of the deprecated hwmon_device_register.
> Construct the expected attribute_group array from the sensor list which
> contains all needed attributes.
> Remove the manual sysfs file creation and deletion that are now taken care
> of by the (un)register calls via the attribute_group array.
> 
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>

Please version your patches, and provide a change log.

You are forcing me to spend time finding the latest version, and I have
to compare the various versions line by line to find out what changed
and if you addressed all comments.

Thanks,
Guenter

> ---
>   drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
>   1 file changed, 25 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 975c43d446f8..33748cc07acc 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -125,6 +125,8 @@ struct atk_data {
>   	int temperature_count;
>   	int fan_count;
>   	struct list_head sensor_list;
> +	struct attribute_group attr_group[2];
> +	const struct attribute_group *attr_groups[2];
>   
>   	struct {
>   		struct dentry *root;
> @@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
>   	return sprintf(buf, "%lld\n", value);
>   }
>   
> -static ssize_t atk_name_show(struct device *dev,
> -				struct device_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "atk0110\n");
> -}
> -static struct device_attribute atk_name_attr =
> -		__ATTR(name, 0444, atk_name_show, NULL);
> -
>   static void atk_init_attribute(struct device_attribute *attr, char *name,
>   		sysfs_show_func show)
>   {
> @@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
>   	return err;
>   }
>   
> -static int atk_create_files(struct atk_data *data)
> +static int atk_init_attribute_groups(struct atk_data *data)
>   {
> +	struct device *dev = &data->acpi_dev->dev;
>   	struct atk_sensor_data *s;
> -	int err;
> +	struct attribute **attrs;
> +	int i = 0;
> +	int len = (data->voltage_count + data->temperature_count
> +			+ data->fan_count) * 4 + 1;
> +
> +	attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
>   
>   	list_for_each_entry(s, &data->sensor_list, list) {
> -		err = device_create_file(data->hwmon_dev, &s->input_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->label_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
> -		if (err)
> -			return err;
> +		attrs[i++] = &s->input_attr.attr;
> +		attrs[i++] = &s->label_attr.attr;
> +		attrs[i++] = &s->limit1_attr.attr;
> +		attrs[i++] = &s->limit2_attr.attr;
>   	}
>   
> -	err = device_create_file(data->hwmon_dev, &atk_name_attr);
> +	data->attr_group[0].attrs = attrs;
> +	data->attr_groups[0] = data->attr_group;
>   
> -	return err;
> -}
> -
> -static void atk_remove_files(struct atk_data *data)
> -{
> -	struct atk_sensor_data *s;
> -
> -	list_for_each_entry(s, &data->sensor_list, list) {
> -		device_remove_file(data->hwmon_dev, &s->input_attr);
> -		device_remove_file(data->hwmon_dev, &s->label_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit1_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit2_attr);
> -	}
> -	device_remove_file(data->hwmon_dev, &atk_name_attr);
> +	return 0;
>   }
>   
>   static void atk_free_sensors(struct atk_data *data)
> @@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
>   static int atk_register_hwmon(struct atk_data *data)
>   {
>   	struct device *dev = &data->acpi_dev->dev;
> -	int err;
>   
>   	dev_dbg(dev, "registering hwmon device\n");
> -	data->hwmon_dev = hwmon_device_register(dev);
> +	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
> +							    data,
> +							    data->attr_groups);
>   	if (IS_ERR(data->hwmon_dev))
>   		return PTR_ERR(data->hwmon_dev);
>   
> -	dev_dbg(dev, "populating sysfs directory\n");
> -	err = atk_create_files(data);
> -	if (err)
> -		goto remove;
> -
>   	return 0;
> -remove:
> -	/* Cleanup the registered files */
> -	atk_remove_files(data);
> -	hwmon_device_unregister(data->hwmon_dev);
> -	return err;
>   }
>   
>   static int atk_probe_if(struct atk_data *data)
> @@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
>   		goto out;
>   	}
>   
> +	err = atk_init_attribute_groups(data);
> +	if (err)
> +		goto out;
>   	err = atk_register_hwmon(data);
>   	if (err)
>   		goto cleanup;
> @@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
>   
>   	atk_debugfs_cleanup(data);
>   
> -	atk_remove_files(data);
>   	atk_free_sensors(data);
>   	hwmon_device_unregister(data->hwmon_dev);
>   
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* asus_atk0110: Use non-deprecated registration and managed memory
  2018-06-01 13:28           ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
@ 2018-06-01 15:14             ` Bastian Germann
  2018-06-01 15:14               ` [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
  2018-06-01 15:14               ` [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
  0 siblings, 2 replies; 16+ messages in thread
From: Bastian Germann @ 2018-06-01 15:14 UTC (permalink / raw)
  To: Guenter Roeck; +Cc: Luca Tettamanti, Jean Delvare, linux-hwmon, linux-kernel

This is version 4 of my previously unversioned patch.

Version 2 addresses the issues brought up by Guenter.
The two v1 patches are joined in v2 1/2.
The unnecessary dynamic allocations are now part of the atk_data struct.
devm_kcalloc is used for the attribute array that is created from sensors.
v2 2/2 replaces all dynamic allocations in the driver with the device
managed devm_* versions. All corresponding kfree calls are removed.

Version 3 addresses the issue brought up by Andy.
v3 1/2 is unchanged. v3 2/2 reverses one of the devm_kzalloc changes of v2
because it introduces a memory leak.

v4 1/2 changes the attr_group field to only contain one item.
v4 2/2 is unchanged.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-06-01 15:14             ` asus_atk0110: Use non-deprecated registration and managed memory Bastian Germann
@ 2018-06-01 15:14               ` Bastian Germann
  2018-06-01 16:35                 ` Guenter Roeck
  2018-06-04 20:35                 ` Luca Tettamanti
  2018-06-01 15:14               ` [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
  1 sibling, 2 replies; 16+ messages in thread
From: Bastian Germann @ 2018-06-01 15:14 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, linux-hwmon,
	linux-kernel

Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
of the deprecated hwmon_device_register.
Construct the expected attribute_group array from the sensor list which
contains all needed attributes.
Remove the manual sysfs file creation and deletion that are now taken care
of by the (un)register calls via the attribute_group array.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
 1 file changed, 25 insertions(+), 50 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 975c43d446f8..33748cc07acc 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -125,6 +125,8 @@ struct atk_data {
 	int temperature_count;
 	int fan_count;
 	struct list_head sensor_list;
+	struct attribute_group attr_group;
+	const struct attribute_group *attr_groups[2];
 
 	struct {
 		struct dentry *root;
@@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
 	return sprintf(buf, "%lld\n", value);
 }
 
-static ssize_t atk_name_show(struct device *dev,
-				struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "atk0110\n");
-}
-static struct device_attribute atk_name_attr =
-		__ATTR(name, 0444, atk_name_show, NULL);
-
 static void atk_init_attribute(struct device_attribute *attr, char *name,
 		sysfs_show_func show)
 {
@@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
 	return err;
 }
 
-static int atk_create_files(struct atk_data *data)
+static int atk_init_attribute_groups(struct atk_data *data)
 {
+	struct device *dev = &data->acpi_dev->dev;
 	struct atk_sensor_data *s;
-	int err;
+	struct attribute **attrs;
+	int i = 0;
+	int len = (data->voltage_count + data->temperature_count
+			+ data->fan_count) * 4 + 1;
+
+	attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
+	if (!attrs)
+		return -ENOMEM;
 
 	list_for_each_entry(s, &data->sensor_list, list) {
-		err = device_create_file(data->hwmon_dev, &s->input_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->label_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
-		if (err)
-			return err;
-		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
-		if (err)
-			return err;
+		attrs[i++] = &s->input_attr.attr;
+		attrs[i++] = &s->label_attr.attr;
+		attrs[i++] = &s->limit1_attr.attr;
+		attrs[i++] = &s->limit2_attr.attr;
 	}
 
-	err = device_create_file(data->hwmon_dev, &atk_name_attr);
+	data->attr_group.attrs = attrs;
+	data->attr_groups[0] = &data->attr_group;
 
-	return err;
-}
-
-static void atk_remove_files(struct atk_data *data)
-{
-	struct atk_sensor_data *s;
-
-	list_for_each_entry(s, &data->sensor_list, list) {
-		device_remove_file(data->hwmon_dev, &s->input_attr);
-		device_remove_file(data->hwmon_dev, &s->label_attr);
-		device_remove_file(data->hwmon_dev, &s->limit1_attr);
-		device_remove_file(data->hwmon_dev, &s->limit2_attr);
-	}
-	device_remove_file(data->hwmon_dev, &atk_name_attr);
+	return 0;
 }
 
 static void atk_free_sensors(struct atk_data *data)
@@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
-	int err;
 
 	dev_dbg(dev, "registering hwmon device\n");
-	data->hwmon_dev = hwmon_device_register(dev);
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
+							    data,
+							    data->attr_groups);
 	if (IS_ERR(data->hwmon_dev))
 		return PTR_ERR(data->hwmon_dev);
 
-	dev_dbg(dev, "populating sysfs directory\n");
-	err = atk_create_files(data);
-	if (err)
-		goto remove;
-
 	return 0;
-remove:
-	/* Cleanup the registered files */
-	atk_remove_files(data);
-	hwmon_device_unregister(data->hwmon_dev);
-	return err;
 }
 
 static int atk_probe_if(struct atk_data *data)
@@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	}
 
+	err = atk_init_attribute_groups(data);
+	if (err)
+		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
 		goto cleanup;
@@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_remove_files(data);
 	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory
  2018-06-01 15:14             ` asus_atk0110: Use non-deprecated registration and managed memory Bastian Germann
  2018-06-01 15:14               ` [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
@ 2018-06-01 15:14               ` Bastian Germann
  2018-06-01 16:40                 ` Guenter Roeck
  1 sibling, 1 reply; 16+ messages in thread
From: Bastian Germann @ 2018-06-01 15:14 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Bastian Germann, Luca Tettamanti, Jean Delvare, linux-hwmon,
	linux-kernel

Use devm_* variants of kstrdup and kzalloc. Get rid of kfree cleanups.

Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>
---
 drivers/hwmon/asus_atk0110.c | 44 +++++++-----------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
index 33748cc07acc..91af065d0e4d 100644
--- a/drivers/hwmon/asus_atk0110.c
+++ b/drivers/hwmon/asus_atk0110.c
@@ -190,7 +190,6 @@ static int atk_add(struct acpi_device *device);
 static int atk_remove(struct acpi_device *device);
 static void atk_print_sensor(struct atk_data *data, union acpi_object *obj);
 static int atk_read_value(struct atk_sensor_data *sensor, u64 *value);
-static void atk_free_sensors(struct atk_data *data);
 
 static struct acpi_driver atk_driver = {
 	.name	= ATK_HID,
@@ -906,15 +905,13 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	limit1 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT1);
 	limit2 = atk_get_pack_member(data, obj, HWMON_PACK_LIMIT2);
 
-	sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
 		return -ENOMEM;
 
-	sensor->acpi_name = kstrdup(name->string.pointer, GFP_KERNEL);
-	if (!sensor->acpi_name) {
-		err = -ENOMEM;
-		goto out;
-	}
+	sensor->acpi_name = devm_kstrdup(dev, name->string.pointer, GFP_KERNEL);
+	if (!sensor->acpi_name)
+		return -ENOMEM;
 
 	INIT_LIST_HEAD(&sensor->list);
 	sensor->type = type;
@@ -955,9 +952,6 @@ static int atk_add_sensor(struct atk_data *data, union acpi_object *obj)
 	(*num)++;
 
 	return 1;
-out:
-	kfree(sensor);
-	return err;
 }
 
 static int atk_enumerate_old_hwmon(struct atk_data *data)
@@ -998,8 +992,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_TMP ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1020,8 +1013,7 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 		dev_warn(dev, METHOD_OLD_ENUM_FAN ": ACPI exception: %s\n",
 				acpi_format_exception(status));
 
-		ret = -ENODEV;
-		goto cleanup;
+		return -ENODEV;
 	}
 
 	pack = buf.pointer;
@@ -1035,9 +1027,6 @@ static int atk_enumerate_old_hwmon(struct atk_data *data)
 	ACPI_FREE(buf.pointer);
 
 	return count;
-cleanup:
-	atk_free_sensors(data);
-	return ret;
 }
 
 static int atk_ec_present(struct atk_data *data)
@@ -1213,17 +1202,6 @@ static int atk_init_attribute_groups(struct atk_data *data)
 	return 0;
 }
 
-static void atk_free_sensors(struct atk_data *data)
-{
-	struct list_head *head = &data->sensor_list;
-	struct atk_sensor_data *s, *tmp;
-
-	list_for_each_entry_safe(s, tmp, head, list) {
-		kfree(s->acpi_name);
-		kfree(s);
-	}
-}
-
 static int atk_register_hwmon(struct atk_data *data)
 {
 	struct device *dev = &data->acpi_dev->dev;
@@ -1323,7 +1301,7 @@ static int atk_add(struct acpi_device *device)
 
 	dev_dbg(&device->dev, "adding...\n");
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&device->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -1375,18 +1353,15 @@ static int atk_add(struct acpi_device *device)
 		goto out;
 	err = atk_register_hwmon(data);
 	if (err)
-		goto cleanup;
+		goto out;
 
 	atk_debugfs_init(data);
 
 	device->driver_data = data;
 	return 0;
-cleanup:
-	atk_free_sensors(data);
 out:
 	if (data->disable_ec)
 		atk_ec_ctl(data, 0);
-	kfree(data);
 	return err;
 }
 
@@ -1399,7 +1374,6 @@ static int atk_remove(struct acpi_device *device)
 
 	atk_debugfs_cleanup(data);
 
-	atk_free_sensors(data);
 	hwmon_device_unregister(data->hwmon_dev);
 
 	if (data->disable_ec) {
@@ -1407,8 +1381,6 @@ static int atk_remove(struct acpi_device *device)
 			dev_err(&device->dev, "Failed to disable EC\n");
 	}
 
-	kfree(data);
-
 	return 0;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-06-01 15:14               ` [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
@ 2018-06-01 16:35                 ` Guenter Roeck
  2018-06-04 20:35                 ` Luca Tettamanti
  1 sibling, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2018-06-01 16:35 UTC (permalink / raw)
  To: Bastian Germann; +Cc: Luca Tettamanti, Jean Delvare, linux-hwmon, linux-kernel

On Fri, Jun 01, 2018 at 05:14:18PM +0200, Bastian Germann wrote:
> Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
> of the deprecated hwmon_device_register.
> Construct the expected attribute_group array from the sensor list which
> contains all needed attributes.
> Remove the manual sysfs file creation and deletion that are now taken care
> of by the (un)register calls via the attribute_group array.
> 
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>

Nicely done.

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
>  1 file changed, 25 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 975c43d446f8..33748cc07acc 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -125,6 +125,8 @@ struct atk_data {
>  	int temperature_count;
>  	int fan_count;
>  	struct list_head sensor_list;
> +	struct attribute_group attr_group;
> +	const struct attribute_group *attr_groups[2];
>  
>  	struct {
>  		struct dentry *root;
> @@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
>  	return sprintf(buf, "%lld\n", value);
>  }
>  
> -static ssize_t atk_name_show(struct device *dev,
> -				struct device_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "atk0110\n");
> -}
> -static struct device_attribute atk_name_attr =
> -		__ATTR(name, 0444, atk_name_show, NULL);
> -
>  static void atk_init_attribute(struct device_attribute *attr, char *name,
>  		sysfs_show_func show)
>  {
> @@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
>  	return err;
>  }
>  
> -static int atk_create_files(struct atk_data *data)
> +static int atk_init_attribute_groups(struct atk_data *data)
>  {
> +	struct device *dev = &data->acpi_dev->dev;
>  	struct atk_sensor_data *s;
> -	int err;
> +	struct attribute **attrs;
> +	int i = 0;
> +	int len = (data->voltage_count + data->temperature_count
> +			+ data->fan_count) * 4 + 1;
> +
> +	attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
>  
>  	list_for_each_entry(s, &data->sensor_list, list) {
> -		err = device_create_file(data->hwmon_dev, &s->input_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->label_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit1_attr);
> -		if (err)
> -			return err;
> -		err = device_create_file(data->hwmon_dev, &s->limit2_attr);
> -		if (err)
> -			return err;
> +		attrs[i++] = &s->input_attr.attr;
> +		attrs[i++] = &s->label_attr.attr;
> +		attrs[i++] = &s->limit1_attr.attr;
> +		attrs[i++] = &s->limit2_attr.attr;
>  	}
>  
> -	err = device_create_file(data->hwmon_dev, &atk_name_attr);
> +	data->attr_group.attrs = attrs;
> +	data->attr_groups[0] = &data->attr_group;
>  
> -	return err;
> -}
> -
> -static void atk_remove_files(struct atk_data *data)
> -{
> -	struct atk_sensor_data *s;
> -
> -	list_for_each_entry(s, &data->sensor_list, list) {
> -		device_remove_file(data->hwmon_dev, &s->input_attr);
> -		device_remove_file(data->hwmon_dev, &s->label_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit1_attr);
> -		device_remove_file(data->hwmon_dev, &s->limit2_attr);
> -	}
> -	device_remove_file(data->hwmon_dev, &atk_name_attr);
> +	return 0;
>  }
>  
>  static void atk_free_sensors(struct atk_data *data)
> @@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
>  static int atk_register_hwmon(struct atk_data *data)
>  {
>  	struct device *dev = &data->acpi_dev->dev;
> -	int err;
>  
>  	dev_dbg(dev, "registering hwmon device\n");
> -	data->hwmon_dev = hwmon_device_register(dev);
> +	data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
> +							    data,
> +							    data->attr_groups);
>  	if (IS_ERR(data->hwmon_dev))
>  		return PTR_ERR(data->hwmon_dev);
>  
> -	dev_dbg(dev, "populating sysfs directory\n");
> -	err = atk_create_files(data);
> -	if (err)
> -		goto remove;
> -
>  	return 0;
> -remove:
> -	/* Cleanup the registered files */
> -	atk_remove_files(data);
> -	hwmon_device_unregister(data->hwmon_dev);
> -	return err;
>  }
>  
>  static int atk_probe_if(struct atk_data *data)
> @@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
>  		goto out;
>  	}
>  
> +	err = atk_init_attribute_groups(data);
> +	if (err)
> +		goto out;
>  	err = atk_register_hwmon(data);
>  	if (err)
>  		goto cleanup;
> @@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
>  
>  	atk_debugfs_cleanup(data);
>  
> -	atk_remove_files(data);
>  	atk_free_sensors(data);
>  	hwmon_device_unregister(data->hwmon_dev);
>  
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory
  2018-06-01 15:14               ` [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
@ 2018-06-01 16:40                 ` Guenter Roeck
  0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2018-06-01 16:40 UTC (permalink / raw)
  To: Bastian Germann; +Cc: Luca Tettamanti, Jean Delvare, linux-hwmon, linux-kernel

On Fri, Jun 01, 2018 at 05:14:19PM +0200, Bastian Germann wrote:
> Use devm_* variants of kstrdup and kzalloc. Get rid of kfree cleanups.
> 
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>

Thanks a lot for the cleanup and fast turnaround.

Applied to hwmon-next.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call
  2018-06-01 15:14               ` [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
  2018-06-01 16:35                 ` Guenter Roeck
@ 2018-06-04 20:35                 ` Luca Tettamanti
  1 sibling, 0 replies; 16+ messages in thread
From: Luca Tettamanti @ 2018-06-04 20:35 UTC (permalink / raw)
  To: Bastian Germann; +Cc: Guenter Roeck, Jean Delvare, linux-hwmon, LKML

On 1 June 2018 at 16:14, Bastian Germann <bastiangermann@fishpost.de> wrote:
> Make the asus_atk0110 driver use hwmon_device_register_with_groups instead
> of the deprecated hwmon_device_register.
> Construct the expected attribute_group array from the sensor list which
> contains all needed attributes.
> Remove the manual sysfs file creation and deletion that are now taken care
> of by the (un)register calls via the attribute_group array.
>
> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de>

Thank you for cleaning this up!

L.

> ---
>  drivers/hwmon/asus_atk0110.c | 75 ++++++++++++------------------------
>  1 file changed, 25 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/hwmon/asus_atk0110.c b/drivers/hwmon/asus_atk0110.c
> index 975c43d446f8..33748cc07acc 100644
> --- a/drivers/hwmon/asus_atk0110.c
> +++ b/drivers/hwmon/asus_atk0110.c
> @@ -125,6 +125,8 @@ struct atk_data {
>         int temperature_count;
>         int fan_count;
>         struct list_head sensor_list;
> +       struct attribute_group attr_group;
> +       const struct attribute_group *attr_groups[2];
>
>         struct {
>                 struct dentry *root;
> @@ -262,14 +264,6 @@ static ssize_t atk_limit2_show(struct device *dev,
>         return sprintf(buf, "%lld\n", value);
>  }
>
> -static ssize_t atk_name_show(struct device *dev,
> -                               struct device_attribute *attr, char *buf)
> -{
> -       return sprintf(buf, "atk0110\n");
> -}
> -static struct device_attribute atk_name_attr =
> -               __ATTR(name, 0444, atk_name_show, NULL);
> -
>  static void atk_init_attribute(struct device_attribute *attr, char *name,
>                 sysfs_show_func show)
>  {
> @@ -1193,42 +1187,30 @@ static int atk_enumerate_new_hwmon(struct atk_data *data)
>         return err;
>  }
>
> -static int atk_create_files(struct atk_data *data)
> +static int atk_init_attribute_groups(struct atk_data *data)
>  {
> +       struct device *dev = &data->acpi_dev->dev;
>         struct atk_sensor_data *s;
> -       int err;
> +       struct attribute **attrs;
> +       int i = 0;
> +       int len = (data->voltage_count + data->temperature_count
> +                       + data->fan_count) * 4 + 1;
> +
> +       attrs = devm_kcalloc(dev, len, sizeof(struct attribute *), GFP_KERNEL);
> +       if (!attrs)
> +               return -ENOMEM;
>
>         list_for_each_entry(s, &data->sensor_list, list) {
> -               err = device_create_file(data->hwmon_dev, &s->input_attr);
> -               if (err)
> -                       return err;
> -               err = device_create_file(data->hwmon_dev, &s->label_attr);
> -               if (err)
> -                       return err;
> -               err = device_create_file(data->hwmon_dev, &s->limit1_attr);
> -               if (err)
> -                       return err;
> -               err = device_create_file(data->hwmon_dev, &s->limit2_attr);
> -               if (err)
> -                       return err;
> +               attrs[i++] = &s->input_attr.attr;
> +               attrs[i++] = &s->label_attr.attr;
> +               attrs[i++] = &s->limit1_attr.attr;
> +               attrs[i++] = &s->limit2_attr.attr;
>         }
>
> -       err = device_create_file(data->hwmon_dev, &atk_name_attr);
> +       data->attr_group.attrs = attrs;
> +       data->attr_groups[0] = &data->attr_group;
>
> -       return err;
> -}
> -
> -static void atk_remove_files(struct atk_data *data)
> -{
> -       struct atk_sensor_data *s;
> -
> -       list_for_each_entry(s, &data->sensor_list, list) {
> -               device_remove_file(data->hwmon_dev, &s->input_attr);
> -               device_remove_file(data->hwmon_dev, &s->label_attr);
> -               device_remove_file(data->hwmon_dev, &s->limit1_attr);
> -               device_remove_file(data->hwmon_dev, &s->limit2_attr);
> -       }
> -       device_remove_file(data->hwmon_dev, &atk_name_attr);
> +       return 0;
>  }
>
>  static void atk_free_sensors(struct atk_data *data)
> @@ -1245,24 +1227,15 @@ static void atk_free_sensors(struct atk_data *data)
>  static int atk_register_hwmon(struct atk_data *data)
>  {
>         struct device *dev = &data->acpi_dev->dev;
> -       int err;
>
>         dev_dbg(dev, "registering hwmon device\n");
> -       data->hwmon_dev = hwmon_device_register(dev);
> +       data->hwmon_dev = hwmon_device_register_with_groups(dev, "atk0110",
> +                                                           data,
> +                                                           data->attr_groups);
>         if (IS_ERR(data->hwmon_dev))
>                 return PTR_ERR(data->hwmon_dev);
>
> -       dev_dbg(dev, "populating sysfs directory\n");
> -       err = atk_create_files(data);
> -       if (err)
> -               goto remove;
> -
>         return 0;
> -remove:
> -       /* Cleanup the registered files */
> -       atk_remove_files(data);
> -       hwmon_device_unregister(data->hwmon_dev);
> -       return err;
>  }
>
>  static int atk_probe_if(struct atk_data *data)
> @@ -1397,6 +1370,9 @@ static int atk_add(struct acpi_device *device)
>                 goto out;
>         }
>
> +       err = atk_init_attribute_groups(data);
> +       if (err)
> +               goto out;
>         err = atk_register_hwmon(data);
>         if (err)
>                 goto cleanup;
> @@ -1423,7 +1399,6 @@ static int atk_remove(struct acpi_device *device)
>
>         atk_debugfs_cleanup(data);
>
> -       atk_remove_files(data);
>         atk_free_sensors(data);
>         hwmon_device_unregister(data->hwmon_dev);
>
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-06-04 20:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 14:01 [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
2018-05-31 14:01 ` [PATCH 2/2] hwmon: (asus_atk0110) Remove unused manual sysfs file management Bastian Germann
2018-05-31 14:28   ` Guenter Roeck
2018-05-31 14:27 ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
2018-05-31 22:56   ` Bastian Germann
2018-05-31 22:57     ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
2018-06-01  9:54       ` Andy Shevchenko
2018-06-01 11:07         ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
2018-06-01 11:07           ` [PATCH 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
2018-06-01 13:28           ` [PATCH 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Guenter Roeck
2018-06-01 15:14             ` asus_atk0110: Use non-deprecated registration and managed memory Bastian Germann
2018-06-01 15:14               ` [PATCH v4 1/2] hwmon: (asus_atk0110) Replace deprecated device register call Bastian Germann
2018-06-01 16:35                 ` Guenter Roeck
2018-06-04 20:35                 ` Luca Tettamanti
2018-06-01 15:14               ` [PATCH v4 2/2] hwmon: (asus_atk0110) Make use of device managed memory Bastian Germann
2018-06-01 16:40                 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).