[PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[Guenter Roeck]

This patch series introduces new hwmon API functions
hwmon_device_register_with_groups() and devm_hwmon_device_register_with_groups().

hwmon_device_register_with_groups() lets callers register hwmon devices
as well as associated sysfs attributes with a single call. This simplifies
hwmon device registration and avoids potential race conditions seen
if sysfs attributes are created after the initial hwmon device was
registered.

devm_hwmon_device_register_with_groups() is the managed version of the same
function.

The rationale for the new API is that sysfs attributes should be created
synchronously with device creation to avoid race conditions, as outlined in
http://www.linuxfoundation.org/news-media/blogs/browse/2013/06/how-create-sysfs-file-correctly.

The first patch of the series introduces hwmon_device_register_with_groups().

Patches 2 to 6 convert some hwmon drivers to use the new API to show and
test its use. Only the ds1621 and nct6775 patches have been tested at this time;
the others are informational to show the potential of the new functions.

Patch 7 introduces devm_hwmon_device_register_with_groups() and
the matching function devm_hwmon_device_unregister(). Patch 8 and 9
convert the nct6775 and ds1621 drivers drivers to use this function.

hwmon attributes

The old hwmon API attaches hwmon attributes to the parent device, not to the
hwmon device itself. With the new API, hwmon attributes are attached to the
hwmon device itself.

Handling the 'name' attribute for hwmon drivers

Since sysfs attributes are no longer attached to the parent device, the
mandatory 'name' attribute has to be created explicitly. For some drivers,
this was already the case. I2c drivers, however, create the 'name' attribute
in the i2c core, and it was used by the hwmon driver.

To simplify name attribute handling, the value of the 'name' attribute is
passed as parameter to hwmon_device_register_with_groups(). The hwmon core
creates and manages the attribute. If a NULL pointer is passed instead of
a name, the attribute is not created.

Unfortunately, since we need a variable to store the pointer to 'name',
it was necessary to create an internal 'struct hwmon_device'. For this
reason, device_create(), or rather device_create_with_groups() can no
longer be used to create the actual device. device_register() is used
instead, and the device data structures are managed locally. 
[ If there is a better solution for this problem, I am listening. ]

The series is based on my hwmon-next branch and should apply to the top of
linux-next.

---------

v2: Rename API functions
	device_create_groups -> device_create_with_groups
    	hwmon_device_register_groups -> hwmon_device_register_with_groups
    Additional parameter 'void *drvdata' for hwmon_device_register_groups.
    Additional parameter 'name' for hwmon_device_register_groups.
    Introduce devm_hwmon_device_register_with_groups().
    Various fixes in hwmon driver conversions.

[PATCH v2 1/9] hwmon: Introduce hwmon_device_register_with_groups

[Guenter Roeck]

hwmon_device_register_with_groups() lets callers register a hwmon device
together with all sysfs attributes in a single call.

When using hwmon_device_register_with_groups(), hwmon attributes are attached
to the hwmon device directly and no longer with its parent device.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c |  121 ++++++++++++++++++++++++++++++++++++++++++-------
 include/linux/hwmon.h |    5 ++
 2 files changed, 110 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 646314f..69323d3 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -25,35 +25,122 @@
 #define HWMON_ID_PREFIX "hwmon"
 #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
 
-static struct class *hwmon_class;
+struct hwmon_device {
+	const char *name;
+	struct device dev;
+};
+#define to_hwmon_device(d) container_of(d, struct hwmon_device, dev)
+
+static ssize_t
+show_name(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%s\n", to_hwmon_device(dev)->name);
+}
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
+static struct attribute *hwmon_dev_attrs[] = {
+	&dev_attr_name.attr,
+	NULL
+};
+
+static umode_t hwmon_dev_name_is_visible(struct kobject *kobj,
+					 struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+
+	if (to_hwmon_device(dev)->name == NULL)
+		return 0;
+
+	return attr->mode;
+}
+
+static struct attribute_group hwmon_dev_attr_group = {
+	.attrs		= hwmon_dev_attrs,
+	.is_visible	= hwmon_dev_name_is_visible,
+};
+
+static const struct attribute_group *hwmon_dev_attr_groups[] = {
+	&hwmon_dev_attr_group,
+	NULL
+};
+
+static void hwmon_dev_release(struct device *dev)
+{
+	kfree(to_hwmon_device(dev));
+}
+
+static struct class hwmon_class = {
+	.name = "hwmon",
+	.owner = THIS_MODULE,
+	.dev_groups = hwmon_dev_attr_groups,
+	.dev_release = hwmon_dev_release,
+};
 
 static DEFINE_IDA(hwmon_ida);
 
 /**
- * hwmon_device_register - register w/ hwmon
- * @dev: the device to register
+ * hwmon_device_register_with_groups - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @groups: List of attribute groups to create
  *
  * hwmon_device_unregister() must be called when the device is no
  * longer needed.
  *
  * Returns the pointer to the new device.
  */
-struct device *hwmon_device_register(struct device *dev)
+struct device *
+hwmon_device_register_with_groups(struct device *dev, const char *name,
+				  void *drvdata,
+				  const struct attribute_group **groups)
 {
-	struct device *hwdev;
-	int id;
+	struct hwmon_device *hwdev;
+	int err, id;
 
 	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
 	if (id < 0)
 		return ERR_PTR(id);
 
-	hwdev = device_create(hwmon_class, dev, MKDEV(0, 0), NULL,
-			      HWMON_ID_FORMAT, id);
+	hwdev = kzalloc(sizeof(*hwdev), GFP_KERNEL);
+	if (hwdev == NULL) {
+		err = -ENOMEM;
+		goto ida_remove;
+	}
 
-	if (IS_ERR(hwdev))
-		ida_simple_remove(&hwmon_ida, id);
+	hwdev->name = name;
+	hwdev->dev.class = &hwmon_class;
+	hwdev->dev.parent = dev;
+	hwdev->dev.groups = groups;
+	hwdev->dev.of_node = dev ? dev->of_node : NULL;
+	dev_set_drvdata(&hwdev->dev, drvdata);
+	dev_set_name(&hwdev->dev, HWMON_ID_FORMAT, id);
+	err = device_register(&hwdev->dev);
+	if (err)
+		goto free;
+
+	return &hwdev->dev;
+
+free:
+	kfree(hwdev);
+ida_remove:
+	ida_simple_remove(&hwmon_ida, id);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(hwmon_device_register_with_groups);
 
-	return hwdev;
+/**
+ * hwmon_device_register - register w/ hwmon
+ * @dev: the device to register
+ *
+ * hwmon_device_unregister() must be called when the device is no
+ * longer needed.
+ *
+ * Returns the pointer to the new device.
+ */
+struct device *hwmon_device_register(struct device *dev)
+{
+	return hwmon_device_register_with_groups(dev, NULL, NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(hwmon_device_register);
 
@@ -105,19 +192,21 @@ static void __init hwmon_pci_quirks(void)
 
 static int __init hwmon_init(void)
 {
+	int err;
+
 	hwmon_pci_quirks();
 
-	hwmon_class = class_create(THIS_MODULE, "hwmon");
-	if (IS_ERR(hwmon_class)) {
-		pr_err("couldn't create sysfs class\n");
-		return PTR_ERR(hwmon_class);
+	err = class_register(&hwmon_class);
+	if (err) {
+		pr_err("couldn't register hwmon sysfs class\n");
+		return err;
 	}
 	return 0;
 }
 
 static void __exit hwmon_exit(void)
 {
-	class_destroy(hwmon_class);
+	class_unregister(&hwmon_class);
 }
 
 subsys_initcall(hwmon_init);
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index b2514f7..6d02ff7 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -15,8 +15,13 @@
 #define _HWMON_H_
 
 struct device;
+struct attribute_group;
 
 struct device *hwmon_device_register(struct device *dev);
+struct device *
+hwmon_device_register_with_groups(struct device *dev, const char *name,
+				  void *drvdata,
+				  const struct attribute_group **groups);
 
 void hwmon_device_unregister(struct device *dev);
 
-- 
1.7.9.7

Re: [PATCH v2 1/9] hwmon: Introduce hwmon_device_register_with_groups

[Greg Kroah-Hartman]

This looks good, just one minor question about the
non-driver-core-related code:

> +struct device *
> +hwmon_device_register_with_groups(struct device *dev, const char *name,
> +				  void *drvdata,
> +				  const struct attribute_group **groups)
>  {
> -	struct device *hwdev;
> -	int id;
> +	struct hwmon_device *hwdev;
> +	int err, id;
>  
>  	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
>  	if (id < 0)
>  		return ERR_PTR(id);

Don't you need a lock around the ida_simple_get call to ensure hwmon_ida
is not being used at the same time twice?  Or does the ida framework
handle that ok?

thanks,

greg k-h

Re: [PATCH v2 1/9] hwmon: Introduce hwmon_device_register_with_groups

[Guenter Roeck]

On 09/01/2013 09:24 AM, Greg Kroah-Hartman wrote:
> This looks good, just one minor question about the
> non-driver-core-related code:
>
>> +struct device *
>> +hwmon_device_register_with_groups(struct device *dev, const char *name,
>> +				  void *drvdata,
>> +				  const struct attribute_group **groups)
>>   {
>> -	struct device *hwdev;
>> -	int id;
>> +	struct hwmon_device *hwdev;
>> +	int err, id;
>>
>>   	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
>>   	if (id < 0)
>>   		return ERR_PTR(id);
>
> Don't you need a lock around the ida_simple_get call to ensure hwmon_ida
> is not being used at the same time twice?  Or does the ida framework
> handle that ok?
>

ida_simple_get handles the locking as far as I can see. Other callers don't use
local locks either, so I guess it must be working.

Guenter

Re: [PATCH v2 1/9] hwmon: Introduce hwmon_device_register_with_groups

[Greg Kroah-Hartman]

On Sun, Sep 01, 2013 at 10:55:34AM -0700, Guenter Roeck wrote:
> On 09/01/2013 09:24 AM, Greg Kroah-Hartman wrote:
> >This looks good, just one minor question about the
> >non-driver-core-related code:
> >
> >>+struct device *
> >>+hwmon_device_register_with_groups(struct device *dev, const char *name,
> >>+				  void *drvdata,
> >>+				  const struct attribute_group **groups)
> >>  {
> >>-	struct device *hwdev;
> >>-	int id;
> >>+	struct hwmon_device *hwdev;
> >>+	int err, id;
> >>
> >>  	id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
> >>  	if (id < 0)
> >>  		return ERR_PTR(id);
> >
> >Don't you need a lock around the ida_simple_get call to ensure hwmon_ida
> >is not being used at the same time twice?  Or does the ida framework
> >handle that ok?
> >
> 
> ida_simple_get handles the locking as far as I can see. Other callers don't use
> local locks either, so I guess it must be working.

Ok, nevermind then, nice work.

greg k-h

[PATCH v2 2/9] hwmon: (ds1621) Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ds1621.c |   60 ++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index a26ba7a..595f4ef 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -120,7 +120,7 @@ static const u8 DS1621_REG_TEMP[3] = {
 
 /* Each client has this additional data */
 struct ds1621_data {
-	struct device *hwmon_dev;
+	struct i2c_client *client;
 	struct mutex update_lock;
 	char valid;			/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
@@ -151,10 +151,10 @@ static inline u16 DS1621_TEMP_TO_REG(long temp, u8 zbits)
 	return temp;
 }
 
-static void ds1621_init_client(struct i2c_client *client)
+static void ds1621_init_client(struct ds1621_data *data,
+			       struct i2c_client *client)
 {
 	u8 conf, new_conf, sreg, resol;
-	struct ds1621_data *data = i2c_get_clientdata(client);
 
 	new_conf = conf = i2c_smbus_read_byte_data(client, DS1621_REG_CONF);
 	/* switch to continuous conversion mode */
@@ -197,8 +197,8 @@ static void ds1621_init_client(struct i2c_client *client)
 
 static struct ds1621_data *ds1621_update_client(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct ds1621_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	u8 new_conf;
 
 	mutex_lock(&data->update_lock);
@@ -247,8 +247,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
 			const char *buf, size_t count)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct ds1621_data *data = dev_get_drvdata(dev);
 	long val;
 	int err;
 
@@ -258,7 +257,7 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
 
 	mutex_lock(&data->update_lock);
 	data->temp[attr->index] = DS1621_TEMP_TO_REG(val, data->zbits);
-	i2c_smbus_write_word_swapped(client, DS1621_REG_TEMP[attr->index],
+	i2c_smbus_write_word_swapped(data->client, DS1621_REG_TEMP[attr->index],
 				     data->temp[attr->index]);
 	mutex_unlock(&data->update_lock);
 	return count;
@@ -282,16 +281,15 @@ static ssize_t show_alarm(struct device *dev, struct device_attribute *da,
 static ssize_t show_convrate(struct device *dev, struct device_attribute *da,
 			  char *buf)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct ds1621_data *data = dev_get_drvdata(dev);
 	return scnprintf(buf, PAGE_SIZE, "%hu\n", data->update_interval);
 }
 
 static ssize_t set_convrate(struct device *dev, struct device_attribute *da,
 			    const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct ds1621_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	unsigned long convrate;
 	s32 err;
 	int resol = 0;
@@ -343,8 +341,7 @@ static umode_t ds1621_attribute_visible(struct kobject *kobj,
 					struct attribute *attr, int index)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct ds1621_data *data = dev_get_drvdata(dev);
 
 	if (attr == &dev_attr_update_interval.attr)
 		if (data->kind == ds1621 || data->kind == ds1625)
@@ -358,49 +355,46 @@ static const struct attribute_group ds1621_group = {
 	.is_visible = ds1621_attribute_visible
 };
 
+static const struct attribute_group *ds1621_groups[] = {
+	&ds1621_group,
+	NULL
+};
+
 static int ds1621_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct ds1621_data *data;
-	int err;
+	struct device *hwmon_dev;
 
 	data = devm_kzalloc(&client->dev, sizeof(struct ds1621_data),
 			    GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
 	data->kind = id->driver_data;
+	data->client = client;
 
 	/* Initialize the DS1621 chip */
-	ds1621_init_client(client);
+	ds1621_init_client(data, client);
 
-	/* Register sysfs hooks */
-	err = sysfs_create_group(&client->dev.kobj, &ds1621_group);
-	if (err)
-		return err;
+	hwmon_dev = hwmon_device_register_with_groups(&client->dev,
+						      client->name, data,
+						      ds1621_groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
 
-	data->hwmon_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove_files;
-	}
+	i2c_set_clientdata(client, hwmon_dev);
 
 	return 0;
-
- exit_remove_files:
-	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
-	return err;
 }
 
 static int ds1621_remove(struct i2c_client *client)
 {
-	struct ds1621_data *data = i2c_get_clientdata(client);
+	struct device *hwmon_dev = i2c_get_clientdata(client);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
+	hwmon_device_unregister(hwmon_dev);
 
 	return 0;
 }
-- 
1.7.9.7

[PATCH v2 3/9] hwmon: (gpio-fan) Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/gpio-fan.c |   41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index b7d6a57..05c2220 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -309,12 +309,6 @@ exit_unlock:
 	return ret;
 }
 
-static ssize_t show_name(struct device *dev,
-			 struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "gpio-fan\n");
-}
-
 static DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm);
 static DEVICE_ATTR(pwm1_enable, S_IRUGO | S_IWUSR,
 		   show_pwm_enable, set_pwm_enable);
@@ -324,26 +318,23 @@ static DEVICE_ATTR(fan1_max, S_IRUGO, show_rpm_max, NULL);
 static DEVICE_ATTR(fan1_input, S_IRUGO, show_rpm, NULL);
 static DEVICE_ATTR(fan1_target, S_IRUGO | S_IWUSR, show_rpm, set_rpm);
 
-static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
-
 static umode_t gpio_fan_is_visible(struct kobject *kobj,
 				   struct attribute *attr, int index)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct gpio_fan_data *data = dev_get_drvdata(dev);
 
-	if (index == 1 && !data->alarm)
+	if (index == 0 && !data->alarm)
 		return 0;
-	if (index > 1 && !data->ctrl)
+	if (index > 0 && !data->ctrl)
 		return 0;
 
 	return attr->mode;
 }
 
 static struct attribute *gpio_fan_attributes[] = {
-	&dev_attr_name.attr,
-	&dev_attr_fan1_alarm.attr,		/* 1 */
-	&dev_attr_pwm1.attr,			/* 2 */
+	&dev_attr_fan1_alarm.attr,		/* 0 */
+	&dev_attr_pwm1.attr,			/* 1 */
 	&dev_attr_pwm1_enable.attr,
 	&dev_attr_pwm1_mode.attr,
 	&dev_attr_fan1_input.attr,
@@ -358,6 +349,11 @@ static const struct attribute_group gpio_fan_group = {
 	.is_visible = gpio_fan_is_visible,
 };
 
+static const struct attribute_group *gpio_fan_groups[] = {
+	&gpio_fan_group,
+	NULL
+};
+
 static int fan_ctrl_init(struct gpio_fan_data *fan_data,
 			 struct gpio_fan_platform_data *pdata)
 {
@@ -539,24 +535,16 @@ static int gpio_fan_probe(struct platform_device *pdev)
 			return err;
 	}
 
-	err = sysfs_create_group(&pdev->dev.kobj, &gpio_fan_group);
-	if (err)
-		return err;
-
 	/* Make this driver part of hwmon class. */
-	fan_data->hwmon_dev = hwmon_device_register(&pdev->dev);
-	if (IS_ERR(fan_data->hwmon_dev)) {
-		err = PTR_ERR(fan_data->hwmon_dev);
-		goto err_remove;
-	}
+	fan_data->hwmon_dev = hwmon_device_register_with_groups(&pdev->dev,
+						"gpio-fan", fan_data,
+						gpio_fan_groups);
+	if (IS_ERR(fan_data->hwmon_dev))
+		return PTR_ERR(fan_data->hwmon_dev);
 
 	dev_info(&pdev->dev, "GPIO fan initialized\n");
 
 	return 0;
-
-err_remove:
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_group);
-	return err;
 }
 
 static int gpio_fan_remove(struct platform_device *pdev)
@@ -564,7 +552,6 @@ static int gpio_fan_remove(struct platform_device *pdev)
 	struct gpio_fan_data *fan_data = platform_get_drvdata(pdev);
 
 	hwmon_device_unregister(fan_data->hwmon_dev);
-	sysfs_remove_group(&pdev->dev.kobj, &gpio_fan_group);
 
 	return 0;
 }
-- 
1.7.9.7

[PATCH v2 4/9] hwmon: (ltc4245) Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ltc4245.c |   78 +++++++++++++++--------------------------------
 1 file changed, 24 insertions(+), 54 deletions(-)

diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
index cdc1ecc..d4172933 100644
--- a/drivers/hwmon/ltc4245.c
+++ b/drivers/hwmon/ltc4245.c
@@ -51,7 +51,9 @@ enum ltc4245_cmd {
 };
 
 struct ltc4245_data {
-	struct device *hwmon_dev;
+	struct i2c_client *client;
+
+	const struct attribute_group *groups[3];
 
 	struct mutex update_lock;
 	bool valid;
@@ -77,8 +79,8 @@ struct ltc4245_data {
  */
 static void ltc4245_update_gpios(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ltc4245_data *data = i2c_get_clientdata(client);
+	struct ltc4245_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	u8 gpio_curr, gpio_next, gpio_reg;
 	int i;
 
@@ -130,8 +132,8 @@ static void ltc4245_update_gpios(struct device *dev)
 
 static struct ltc4245_data *ltc4245_update_device(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct ltc4245_data *data = i2c_get_clientdata(client);
+	struct ltc4245_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	s32 val;
 	int i;
 
@@ -455,41 +457,14 @@ static const struct attribute_group ltc4245_gpio_group = {
 	.attrs = ltc4245_gpio_attributes,
 };
 
-static int ltc4245_sysfs_create_groups(struct i2c_client *client)
+static void ltc4245_sysfs_add_groups(struct ltc4245_data *data)
 {
-	struct ltc4245_data *data = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-	int ret;
-
-	/* register the standard sysfs attributes */
-	ret = sysfs_create_group(&dev->kobj, &ltc4245_std_group);
-	if (ret) {
-		dev_err(dev, "unable to register standard attributes\n");
-		return ret;
-	}
+	/* standard sysfs attributes */
+	data->groups[0] = &ltc4245_std_group;
 
 	/* if we're using the extra gpio support, register it's attributes */
-	if (data->use_extra_gpios) {
-		ret = sysfs_create_group(&dev->kobj, &ltc4245_gpio_group);
-		if (ret) {
-			dev_err(dev, "unable to register gpio attributes\n");
-			sysfs_remove_group(&dev->kobj, &ltc4245_std_group);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
-static void ltc4245_sysfs_remove_groups(struct i2c_client *client)
-{
-	struct ltc4245_data *data = i2c_get_clientdata(client);
-	struct device *dev = &client->dev;
-
 	if (data->use_extra_gpios)
-		sysfs_remove_group(&dev->kobj, &ltc4245_gpio_group);
-
-	sysfs_remove_group(&dev->kobj, &ltc4245_std_group);
+		data->groups[1] = &ltc4245_gpio_group;
 }
 
 static bool ltc4245_use_extra_gpios(struct i2c_client *client)
@@ -517,7 +492,7 @@ static int ltc4245_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = client->adapter;
 	struct ltc4245_data *data;
-	int ret;
+	struct device *hwmon_dev;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -ENODEV;
@@ -526,7 +501,7 @@ static int ltc4245_probe(struct i2c_client *client,
 	if (!data)
 		return -ENOMEM;
 
-	i2c_set_clientdata(client, data);
+	data->client = client;
 	mutex_init(&data->update_lock);
 	data->use_extra_gpios = ltc4245_use_extra_gpios(client);
 
@@ -534,30 +509,25 @@ static int ltc4245_probe(struct i2c_client *client,
 	i2c_smbus_write_byte_data(client, LTC4245_FAULT1, 0x00);
 	i2c_smbus_write_byte_data(client, LTC4245_FAULT2, 0x00);
 
-	/* Register sysfs hooks */
-	ret = ltc4245_sysfs_create_groups(client);
-	if (ret)
-		return ret;
+	/* Add sysfs hooks */
+	ltc4245_sysfs_add_groups(data);
 
-	data->hwmon_dev = hwmon_device_register(&client->dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		ret = PTR_ERR(data->hwmon_dev);
-		goto out_hwmon_device_register;
-	}
+	hwmon_dev = hwmon_device_register_with_groups(&client->dev,
+						      client->name, data,
+						      data->groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
 
-	return 0;
+	i2c_set_clientdata(client, hwmon_dev);
 
-out_hwmon_device_register:
-	ltc4245_sysfs_remove_groups(client);
-	return ret;
+	return 0;
 }
 
 static int ltc4245_remove(struct i2c_client *client)
 {
-	struct ltc4245_data *data = i2c_get_clientdata(client);
+	struct device *hwmon_dev = i2c_get_clientdata(client);
 
-	hwmon_device_unregister(data->hwmon_dev);
-	ltc4245_sysfs_remove_groups(client);
+	hwmon_device_unregister(hwmon_dev);
 
 	return 0;
 }
-- 
1.7.9.7

[PATCH v2 5/9] hwmon: (pmbus) Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/pmbus/pmbus_core.c |   20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 9319fcf..e2c34a5 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -97,6 +97,7 @@ struct pmbus_data {
 	int max_attributes;
 	int num_attributes;
 	struct attribute_group group;
+	const struct attribute_group *groups[2];
 
 	struct pmbus_sensor *sensors;
 
@@ -348,7 +349,7 @@ static struct _pmbus_status {
 
 static struct pmbus_data *pmbus_update_device(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	const struct pmbus_driver_info *info = data->info;
 	struct pmbus_sensor *sensor;
@@ -733,7 +734,7 @@ static ssize_t pmbus_set_sensor(struct device *dev,
 				struct device_attribute *devattr,
 				const char *buf, size_t count)
 {
-	struct i2c_client *client = to_i2c_client(dev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
 	ssize_t rv = count;
@@ -1768,22 +1769,16 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		goto out_kfree;
 	}
 
-	/* Register sysfs hooks */
-	ret = sysfs_create_group(&dev->kobj, &data->group);
-	if (ret) {
-		dev_err(dev, "Failed to create sysfs entries\n");
-		goto out_kfree;
-	}
-	data->hwmon_dev = hwmon_device_register(dev);
+	data->groups[0] = &data->group;
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
+							    data, data->groups);
 	if (IS_ERR(data->hwmon_dev)) {
 		ret = PTR_ERR(data->hwmon_dev);
 		dev_err(dev, "Failed to register hwmon device\n");
-		goto out_hwmon_device_register;
+		goto out_kfree;
 	}
 	return 0;
 
-out_hwmon_device_register:
-	sysfs_remove_group(&dev->kobj, &data->group);
 out_kfree:
 	kfree(data->group.attrs);
 	return ret;
@@ -1794,7 +1789,6 @@ int pmbus_do_remove(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &data->group);
 	kfree(data->group.attrs);
 	return 0;
 }
-- 
1.7.9.7

[PATCH v2 6/9] hwmon: (nct6775) Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nct6775.c |  122 ++++++++++++++---------------------------------
 1 file changed, 36 insertions(+), 86 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index 6eb03ce..bf0c35f 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -725,10 +725,9 @@ struct nct6775_data {
 	const char *name;
 
 	struct device *hwmon_dev;
-	struct attribute_group *group_in;
-	struct attribute_group *group_fan;
-	struct attribute_group *group_temp;
-	struct attribute_group *group_pwm;
+
+	int num_attr_groups;
+	const struct attribute_group *groups[6];
 
 	u16 reg_temp[5][NUM_TEMP]; /* 0=temp, 1=temp_over, 2=temp_hyst,
 				    * 3=temp_crit, 4=temp_lcrit
@@ -942,7 +941,7 @@ nct6775_create_attr_group(struct device *dev, struct sensor_template_group *tg,
 	struct sensor_device_attribute_2 *a2;
 	struct attribute **attrs;
 	struct sensor_device_template **t;
-	int err, i, j, count;
+	int i, j, count;
 
 	if (repeat <= 0)
 		return ERR_PTR(-EINVAL);
@@ -1002,10 +1001,6 @@ nct6775_create_attr_group(struct device *dev, struct sensor_template_group *tg,
 		}
 	}
 
-	err = sysfs_create_group(&dev->kobj, group);
-	if (err)
-		return ERR_PTR(-ENOMEM);
-
 	return group;
 }
 
@@ -2726,16 +2721,6 @@ store_fan_time(struct device *dev, struct device_attribute *attr,
 }
 
 static ssize_t
-show_name(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct nct6775_data *data = dev_get_drvdata(dev);
-
-	return sprintf(buf, "%s\n", data->name);
-}
-
-static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
-
-static ssize_t
 show_auto_pwm(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct nct6775_data *data = nct6775_update_device(dev);
@@ -3061,16 +3046,16 @@ static umode_t nct6775_other_is_visible(struct kobject *kobj,
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct nct6775_data *data = dev_get_drvdata(dev);
 
-	if (index == 1 && !data->have_vid)
+	if (index == 0 && !data->have_vid)
 		return 0;
 
-	if (index == 2 || index == 3) {
-		if (data->ALARM_BITS[INTRUSION_ALARM_BASE + index - 2] < 0)
+	if (index == 1 || index == 2) {
+		if (data->ALARM_BITS[INTRUSION_ALARM_BASE + index - 1] < 0)
 			return 0;
 	}
 
-	if (index == 4 || index == 5) {
-		if (data->BEEP_BITS[INTRUSION_ALARM_BASE + index - 4] < 0)
+	if (index == 3 || index == 4) {
+		if (data->BEEP_BITS[INTRUSION_ALARM_BASE + index - 3] < 0)
 			return 0;
 	}
 
@@ -3083,13 +3068,12 @@ static umode_t nct6775_other_is_visible(struct kobject *kobj,
  * Any change in order or content must be matched.
  */
 static struct attribute *nct6775_attributes_other[] = {
-	&dev_attr_name.attr,
-	&dev_attr_cpu0_vid.attr,				/* 1 */
-	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,	/* 2 */
-	&sensor_dev_attr_intrusion1_alarm.dev_attr.attr,	/* 3 */
-	&sensor_dev_attr_intrusion0_beep.dev_attr.attr,		/* 4 */
-	&sensor_dev_attr_intrusion1_beep.dev_attr.attr,		/* 5 */
-	&sensor_dev_attr_beep_enable.dev_attr.attr,		/* 6 */
+	&dev_attr_cpu0_vid.attr,				/* 0 */
+	&sensor_dev_attr_intrusion0_alarm.dev_attr.attr,	/* 1 */
+	&sensor_dev_attr_intrusion1_alarm.dev_attr.attr,	/* 2 */
+	&sensor_dev_attr_intrusion0_beep.dev_attr.attr,		/* 3 */
+	&sensor_dev_attr_intrusion1_beep.dev_attr.attr,		/* 4 */
+	&sensor_dev_attr_beep_enable.dev_attr.attr,		/* 5 */
 
 	NULL
 };
@@ -3099,27 +3083,6 @@ static const struct attribute_group nct6775_group_other = {
 	.is_visible = nct6775_other_is_visible,
 };
 
-/*
- * Driver and device management
- */
-
-static void nct6775_device_remove_files(struct device *dev)
-{
-	struct nct6775_data *data = dev_get_drvdata(dev);
-
-	if (data->group_pwm)
-		sysfs_remove_group(&dev->kobj, data->group_pwm);
-	if (data->group_in)
-		sysfs_remove_group(&dev->kobj, data->group_in);
-	if (data->group_fan)
-		sysfs_remove_group(&dev->kobj, data->group_fan);
-	if (data->group_temp)
-		sysfs_remove_group(&dev->kobj, data->group_temp);
-
-	sysfs_remove_group(&dev->kobj, &nct6775_group_other);
-}
-
-/* Get the monitoring functions started */
 static inline void nct6775_init_device(struct nct6775_data *data)
 {
 	int i;
@@ -3870,51 +3833,39 @@ static int nct6775_probe(struct platform_device *pdev)
 	/* Register sysfs hooks */
 	group = nct6775_create_attr_group(dev, &nct6775_pwm_template_group,
 					  data->pwm_num);
-	if (IS_ERR(group)) {
-		err = PTR_ERR(group);
-		goto exit_remove;
-	}
-	data->group_pwm = group;
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	data->groups[data->num_attr_groups++] = group;
 
 	group = nct6775_create_attr_group(dev, &nct6775_in_template_group,
 					  fls(data->have_in));
-	if (IS_ERR(group)) {
-		err = PTR_ERR(group);
-		goto exit_remove;
-	}
-	data->group_in = group;
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	data->groups[data->num_attr_groups++] = group;
 
 	group = nct6775_create_attr_group(dev, &nct6775_fan_template_group,
 					  fls(data->has_fan));
-	if (IS_ERR(group)) {
-		err = PTR_ERR(group);
-		goto exit_remove;
-	}
-	data->group_fan = group;
+	if (IS_ERR(group))
+		return PTR_ERR(group);
+
+	data->groups[data->num_attr_groups++] = group;
 
 	group = nct6775_create_attr_group(dev, &nct6775_temp_template_group,
 					  fls(data->have_temp));
-	if (IS_ERR(group)) {
-		err = PTR_ERR(group);
-		goto exit_remove;
-	}
-	data->group_temp = group;
+	if (IS_ERR(group))
+		return PTR_ERR(group);
 
-	err = sysfs_create_group(&dev->kobj, &nct6775_group_other);
-	if (err)
-		goto exit_remove;
+	data->groups[data->num_attr_groups++] = group;
+	data->groups[data->num_attr_groups++] = &nct6775_group_other;
 
-	data->hwmon_dev = hwmon_device_register(dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove;
-	}
+	data->hwmon_dev = hwmon_device_register_with_groups(dev, data->name,
+							    data, data->groups);
+	if (IS_ERR(data->hwmon_dev))
+		return PTR_ERR(data->hwmon_dev);
 
 	return 0;
-
-exit_remove:
-	nct6775_device_remove_files(dev);
-	return err;
 }
 
 static int nct6775_remove(struct platform_device *pdev)
@@ -3922,7 +3873,6 @@ static int nct6775_remove(struct platform_device *pdev)
 	struct nct6775_data *data = platform_get_drvdata(pdev);
 
 	hwmon_device_unregister(data->hwmon_dev);
-	nct6775_device_remove_files(&pdev->dev);
 
 	return 0;
 }
@@ -4101,7 +4051,7 @@ static int __init nct6775_find(int sioaddr, struct nct6775_sio_data *sio_data)
 /*
  * when Super-I/O functions move to a separate file, the Super-I/O
  * bus will manage the lifetime of the device and this module will only keep
- * track of the nct6775 driver. But since we platform_device_alloc(), we
+ * track of the nct6775 driver. But since we use platform_device_alloc(), we
  * must keep track of the device
  */
 static struct platform_device *pdev[2];
-- 
1.7.9.7

[PATCH v2 7/9] hwmon: Provide managed hwmon registration

[Guenter Roeck]

Drivers using the new hwmon_device_register_with_groups API often have a
remove function which consists solely of a call hwmon_device_unregister().

Provide support for devm_hwmon_device_register_with_groups and
devm_hwmon_device_unregister to allow this repeated code to be removed
and help eliminate error handling code.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hwmon.h |    5 ++++
 2 files changed, 68 insertions(+)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 69323d3..90e539e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -162,6 +162,69 @@ void hwmon_device_unregister(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(hwmon_device_unregister);
 
+static void devm_hwmon_release(struct device *dev, void *res)
+{
+	struct device *hwdev = *(struct device **)res;
+
+	hwmon_device_unregister(hwdev);
+}
+
+/**
+ * devm_hwmon_device_register_with_groups - register w/ hwmon
+ * @dev: the parent device
+ * @name: hwmon name attribute
+ * @drvdata: driver data to attach to created device
+ * @groups: List of attribute groups to create
+ *
+ * Returns the pointer to the new device. The new device is automatically
+ * unregistered with the parent device.
+ */
+struct device *
+devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
+				       void *drvdata,
+				       const struct attribute_group **groups)
+{
+	struct device **ptr, *hwdev;
+
+	if (!dev)
+		return ERR_PTR(-EINVAL);
+
+	ptr = devres_alloc(devm_hwmon_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	hwdev = hwmon_device_register_with_groups(dev, name, drvdata, groups);
+	if (IS_ERR(hwdev))
+		goto error;
+
+	*ptr = hwdev;
+	devres_add(dev, ptr);
+	return hwdev;
+
+error:
+	devres_free(ptr);
+	return hwdev;
+}
+EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
+
+static int devm_hwmon_match(struct device *dev, void *res, void *data)
+{
+	struct device **hwdev = res;
+
+	return *hwdev == data;
+}
+
+/**
+ * devm_hwmon_device_unregister - removes a previously registered hwmon device
+ *
+ * @dev: the parent device of the device to unregister
+ */
+void devm_hwmon_device_unregister(struct device *dev)
+{
+	WARN_ON(devres_release(dev, devm_hwmon_release, devm_hwmon_match, dev));
+}
+EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister);
+
 static void __init hwmon_pci_quirks(void)
 {
 #if defined CONFIG_X86 && defined CONFIG_PCI
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 6d02ff7..09354f6 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -22,7 +22,12 @@ struct device *
 hwmon_device_register_with_groups(struct device *dev, const char *name,
 				  void *drvdata,
 				  const struct attribute_group **groups);
+struct device *
+devm_hwmon_device_register_with_groups(struct device *dev, const char *name,
+				       void *drvdata,
+				       const struct attribute_group **groups);
 
 void hwmon_device_unregister(struct device *dev);
+void devm_hwmon_device_unregister(struct device *dev);
 
 #endif
-- 
1.7.9.7

[PATCH v2 8/9] hwmon: (nct6775) Convert to use devm_hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/nct6775.c |   21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/nct6775.c b/drivers/hwmon/nct6775.c
index bf0c35f..1b22871 100644
--- a/drivers/hwmon/nct6775.c
+++ b/drivers/hwmon/nct6775.c
@@ -724,8 +724,6 @@ struct nct6775_data {
 	enum kinds kind;
 	const char *name;
 
-	struct device *hwmon_dev;
-
 	int num_attr_groups;
 	const struct attribute_group *groups[6];
 
@@ -3259,6 +3257,7 @@ static int nct6775_probe(struct platform_device *pdev)
 	int num_reg_temp;
 	u8 cr2a;
 	struct attribute_group *group;
+	struct device *hwmon_dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	if (!devm_request_region(&pdev->dev, res->start, IOREGION_LENGTH,
@@ -3860,19 +3859,10 @@ static int nct6775_probe(struct platform_device *pdev)
 	data->groups[data->num_attr_groups++] = group;
 	data->groups[data->num_attr_groups++] = &nct6775_group_other;
 
-	data->hwmon_dev = hwmon_device_register_with_groups(dev, data->name,
-							    data, data->groups);
-	if (IS_ERR(data->hwmon_dev))
-		return PTR_ERR(data->hwmon_dev);
-
-	return 0;
-}
-
-static int nct6775_remove(struct platform_device *pdev)
-{
-	struct nct6775_data *data = platform_get_drvdata(pdev);
-
-	hwmon_device_unregister(data->hwmon_dev);
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, data->name,
+							   data, data->groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
 
 	return 0;
 }
@@ -3963,7 +3953,6 @@ static struct platform_driver nct6775_driver = {
 		.pm	= NCT6775_DEV_PM_OPS,
 	},
 	.probe		= nct6775_probe,
-	.remove		= nct6775_remove,
 };
 
 static const char * const nct6775_sio_names[] __initconst = {
-- 
1.7.9.7

[PATCH v2 9/9] hwmon: (ds1621) Convert to use devm_hwmon_device_register_with_groups

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/ds1621.c |   18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index 595f4ef..51a6113 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -379,23 +379,12 @@ static int ds1621_probe(struct i2c_client *client,
 	/* Initialize the DS1621 chip */
 	ds1621_init_client(data, client);
 
-	hwmon_dev = hwmon_device_register_with_groups(&client->dev,
-						      client->name, data,
-						      ds1621_groups);
+	hwmon_dev = devm_hwmon_device_register_with_groups(&client->dev,
+							   client->name, data,
+							   ds1621_groups);
 	if (IS_ERR(hwmon_dev))
 		return PTR_ERR(hwmon_dev);
 
-	i2c_set_clientdata(client, hwmon_dev);
-
-	return 0;
-}
-
-static int ds1621_remove(struct i2c_client *client)
-{
-	struct device *hwmon_dev = i2c_get_clientdata(client);
-
-	hwmon_device_unregister(hwmon_dev);
-
 	return 0;
 }
 
@@ -416,7 +405,6 @@ static struct i2c_driver ds1621_driver = {
 		.name	= "ds1621",
 	},
 	.probe		= ds1621_probe,
-	.remove		= ds1621_remove,
 	.id_table	= ds1621_id,
 };
 
-- 
1.7.9.7

Re: [PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[Guenter Roeck]

On 08/31/2013 07:48 PM, Guenter Roeck wrote:
> This patch series introduces new hwmon API functions
> hwmon_device_register_with_groups() and devm_hwmon_device_register_with_groups().
>
> hwmon_device_register_with_groups() lets callers register hwmon devices
> as well as associated sysfs attributes with a single call. This simplifies
> hwmon device registration and avoids potential race conditions seen
> if sysfs attributes are created after the initial hwmon device was
> registered.
>
> devm_hwmon_device_register_with_groups() is the managed version of the same
> function.
>
Jean, any comments on this patch series ?

If you are ok with it, I would like to push the new API (patches 1 and 7 of the series)
into 3.12, and keep the rest for 3.13.

Thanks,
Guenter

> The rationale for the new API is that sysfs attributes should be created
> synchronously with device creation to avoid race conditions, as outlined in
> http://www.linuxfoundation.org/news-media/blogs/browse/2013/06/how-create-sysfs-file-correctly.
>
> The first patch of the series introduces hwmon_device_register_with_groups().
>
> Patches 2 to 6 convert some hwmon drivers to use the new API to show and
> test its use. Only the ds1621 and nct6775 patches have been tested at this time;
> the others are informational to show the potential of the new functions.
>
> Patch 7 introduces devm_hwmon_device_register_with_groups() and
> the matching function devm_hwmon_device_unregister(). Patch 8 and 9
> convert the nct6775 and ds1621 drivers drivers to use this function.
>
> hwmon attributes
>
> The old hwmon API attaches hwmon attributes to the parent device, not to the
> hwmon device itself. With the new API, hwmon attributes are attached to the
> hwmon device itself.
>
> Handling the 'name' attribute for hwmon drivers
>
> Since sysfs attributes are no longer attached to the parent device, the
> mandatory 'name' attribute has to be created explicitly. For some drivers,
> this was already the case. I2c drivers, however, create the 'name' attribute
> in the i2c core, and it was used by the hwmon driver.
>
> To simplify name attribute handling, the value of the 'name' attribute is
> passed as parameter to hwmon_device_register_with_groups(). The hwmon core
> creates and manages the attribute. If a NULL pointer is passed instead of
> a name, the attribute is not created.
>
> Unfortunately, since we need a variable to store the pointer to 'name',
> it was necessary to create an internal 'struct hwmon_device'. For this
> reason, device_create(), or rather device_create_with_groups() can no
> longer be used to create the actual device. device_register() is used
> instead, and the device data structures are managed locally.
> [ If there is a better solution for this problem, I am listening. ]
>
> The series is based on my hwmon-next branch and should apply to the top of
> linux-next.
>
> ---------
>
> v2: Rename API functions
> 	device_create_groups -> device_create_with_groups
>      	hwmon_device_register_groups -> hwmon_device_register_with_groups
>      Additional parameter 'void *drvdata' for hwmon_device_register_groups.
>      Additional parameter 'name' for hwmon_device_register_groups.
>      Introduce devm_hwmon_device_register_with_groups().
>      Various fixes in hwmon driver conversions.
>
>

Re: [PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[Guenter Roeck]

On Mon, Sep 02, 2013 at 12:25:35PM -0700, Guenter Roeck wrote:
> On 08/31/2013 07:48 PM, Guenter Roeck wrote:
> >This patch series introduces new hwmon API functions
> >hwmon_device_register_with_groups() and devm_hwmon_device_register_with_groups().
> >
> >hwmon_device_register_with_groups() lets callers register hwmon devices
> >as well as associated sysfs attributes with a single call. This simplifies
> >hwmon device registration and avoids potential race conditions seen
> >if sysfs attributes are created after the initial hwmon device was
> >registered.
> >
> >devm_hwmon_device_register_with_groups() is the managed version of the same
> >function.
> >
> Jean, any comments on this patch series ?
> 
> If you are ok with it, I would like to push the new API (patches 1 and 7 of the series)
> into 3.12, and keep the rest for 3.13.
> 
The commit window opened up, so this will have to wait for 3.13.

Guenter

Re: [PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[Jean Delvare]

Hi Guenter,

On Mon, 02 Sep 2013 12:25:35 -0700, Guenter Roeck wrote:
> On 08/31/2013 07:48 PM, Guenter Roeck wrote:
> > This patch series introduces new hwmon API functions
> > hwmon_device_register_with_groups() and devm_hwmon_device_register_with_groups().
> >
> > hwmon_device_register_with_groups() lets callers register hwmon devices
> > as well as associated sysfs attributes with a single call. This simplifies
> > hwmon device registration and avoids potential race conditions seen
> > if sysfs attributes are created after the initial hwmon device was
> > registered.
> >
> > devm_hwmon_device_register_with_groups() is the managed version of the same
> > function.
> >
> Jean, any comments on this patch series ?
> 
> If you are ok with it, I would like to push the new API (patches 1 and 7 of the series)
> into 3.12, and keep the rest for 3.13.

I'm afraid I won't have the time to review all these patches. The
concept looks good to me and I trust that you implemented things right
so feel free to push the patches upstream even without my review.

-- 
Jean Delvare

Re: [PATCH v2 0/9] Introduce hwmon_device_register_with_groups and

[Guenter Roeck]

On 09/14/2013 11:31 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Mon, 02 Sep 2013 12:25:35 -0700, Guenter Roeck wrote:
>> On 08/31/2013 07:48 PM, Guenter Roeck wrote:
>>> This patch series introduces new hwmon API functions
>>> hwmon_device_register_with_groups() and devm_hwmon_device_register_with_groups().
>>>
>>> hwmon_device_register_with_groups() lets callers register hwmon devices
>>> as well as associated sysfs attributes with a single call. This simplifies
>>> hwmon device registration and avoids potential race conditions seen
>>> if sysfs attributes are created after the initial hwmon device was
>>> registered.
>>>
>>> devm_hwmon_device_register_with_groups() is the managed version of the same
>>> function.
>>>
>> Jean, any comments on this patch series ?
>>
>> If you are ok with it, I would like to push the new API (patches 1 and 7 of the series)
>> into 3.12, and keep the rest for 3.13.
>
> I'm afraid I won't have the time to review all these patches. The
> concept looks good to me and I trust that you implemented things right
> so feel free to push the patches upstream even without my review.
>

No problem. I'll add the entire series to -next after the commit window closes.

Thanks,
Guenter