Skip to contentSkip to conversation

hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

PATCH In review Axel Lin 4 participants 10 messages Jun 24, 2014 – Jun 25, 2014 Hardware monitoring

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

Use ATTRIBUTE_GROUPS macro and devm_hwmon_device_register_with_groups() to simplify the code a bit.

The 'sensors' command never worked with this driver, fixed with this conversion.

Signed-off-by Axel Lin <[email protected]>

---
Hi,
I think the 'sensors' command never worked with this driver, but I don't have the h/w to test. I'd appreciate if someone can test it.
Thanks,
Axel

 drivers/hwmon/w83l786ng.c | 87 ++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 58 deletions(-)
drivers/hwmon/w83l786ng.c +29 -58
index 6ed76ce..8c0e370 100644
@@ -124,7 +124,7 @@ DIV_TO_REG(long val)
 }
 
 struct w83l786ng_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 */

@@ -152,7 +152,6 @@ static int w83l786ng_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id);
 static int w83l786ng_detect(struct i2c_client *client,
 			    struct i2c_board_info *info);
-static int w83l786ng_remove(struct i2c_client *client);
 static void w83l786ng_init_client(struct i2c_client *client);
 static struct w83l786ng_data *w83l786ng_update_device(struct device *dev);
 

@@ -168,7 +167,6 @@ static struct i2c_driver w83l786ng_driver = {
 		   .name = "w83l786ng",
 	},
 	.probe		= w83l786ng_probe,
-	.remove		= w83l786ng_remove,
 	.id_table	= w83l786ng_id,
 	.detect		= w83l786ng_detect,
 	.address_list	= normal_i2c,

@@ -207,8 +205,8 @@ store_in_##reg(struct device *dev, struct device_attribute *attr, \
 	       const char *buf, size_t count) \
 { \
 	int nr = to_sensor_dev_attr(attr)->index; \
-	struct i2c_client *client = to_i2c_client(dev); \
-	struct w83l786ng_data *data = i2c_get_clientdata(client); \
+	struct w83l786ng_data *data = dev_get_drvdata(dev); \
+	struct i2c_client *client = data->client; \
 	unsigned long val; \
 	int err = kstrtoul(buf, 10, &val); \
 	if (err) \

@@ -260,8 +258,8 @@ store_fan_min(struct device *dev, struct device_attribute *attr,
 	      const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	unsigned long val;
 	int err;
 

@@ -298,8 +296,8 @@ store_fan_div(struct device *dev, struct device_attribute *attr,
 	      const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 
 	unsigned long min;
 	u8 tmp_fan_div;

@@ -389,8 +387,8 @@ store_temp(struct device *dev, struct device_attribute *attr,
 	    to_sensor_dev_attr_2(attr);
 	int nr = sensor_attr->nr;
 	int index = sensor_attr->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	long val;
 	int err;
 

@@ -444,8 +442,8 @@ store_pwm_mode(struct device *dev, struct device_attribute *attr,
 	       const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	u8 reg;
 	unsigned long val;
 	int err;

@@ -472,8 +470,8 @@ store_pwm(struct device *dev, struct device_attribute *attr,
 	  const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	unsigned long val;
 	int err;
 

@@ -496,8 +494,8 @@ store_pwm_enable(struct device *dev, struct device_attribute *attr,
 		 const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	u8 reg;
 	unsigned long val;
 	int err;

@@ -552,8 +550,8 @@ store_tolerance(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
 	int nr = to_sensor_dev_attr(attr)->index;
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	u8 tol_tmp, tol_mask;
 	unsigned long val;
 	int err;

@@ -608,7 +606,7 @@ static struct sensor_device_attribute sda_tolerance[] = {
 #define TOLERANCE_UNIT_ATTRS(X)	\
 	&sda_tolerance[X].dev_attr.attr
 
-static struct attribute *w83l786ng_attributes[] = {
+static struct attribute *w83l786ng_attrs[] = {
 	IN_UNIT_ATTRS(0),
 	IN_UNIT_ATTRS(1),
 	IN_UNIT_ATTRS(2),

@@ -623,9 +621,7 @@ static struct attribute *w83l786ng_attributes[] = {
 	NULL
 };
 
-static const struct attribute_group w83l786ng_group = {
-	.attrs = w83l786ng_attributes,
-};
+ATTRIBUTE_GROUPS(w83l786ng);
 
 static int
 w83l786ng_detect(struct i2c_client *client, struct i2c_board_info *info)

@@ -667,15 +663,15 @@ w83l786ng_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
 	struct w83l786ng_data *data;
-	int i, err = 0;
+	struct device *hwmon_dev;
+	int i;
 	u8 reg_tmp;
 
-	data = devm_kzalloc(&client->dev, sizeof(struct w83l786ng_data),
-			    GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(struct w83l786ng_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	i2c_set_clientdata(client, data);
+	data->client = client;
 	mutex_init(&data->update_lock);
 
 	/* Initialize the chip */

@@ -692,35 +688,10 @@ w83l786ng_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	data->fan_div[0] = reg_tmp & 0x07;
 	data->fan_div[1] = (reg_tmp >> 4) & 0x07;
 
-	/* Register sysfs hooks */
-	err = sysfs_create_group(&client->dev.kobj, &w83l786ng_group);
-	if (err)
-		goto exit_remove;
-
-	data->hwmon_dev = hwmon_device_register(dev);
-	if (IS_ERR(data->hwmon_dev)) {
-		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove;
-	}
-
-	return 0;
-
-	/* Unregister sysfs hooks */
-
-exit_remove:
-	sysfs_remove_group(&client->dev.kobj, &w83l786ng_group);
-	return err;
-}
-
-static int
-w83l786ng_remove(struct i2c_client *client)
-{
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
-
-	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &w83l786ng_group);
-
-	return 0;
+	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+							   data,
+							   w83l786ng_groups);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
 static void

@@ -739,8 +710,8 @@ w83l786ng_init_client(struct i2c_client *client)
 
 static struct w83l786ng_data *w83l786ng_update_device(struct device *dev)
 {
-	struct i2c_client *client = to_i2c_client(dev);
-	struct w83l786ng_data *data = i2c_get_clientdata(client);
+	struct w83l786ng_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
 	int i, j;
 	u8 reg_tmp, pwmcfg;
Signature · 9 lines
1.8.3.2




_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#1 lore raw
Quoted 5 lines · Axel Lin

Use ATTRIBUTE_GROUPS macro and devm_hwmon_device_register_with_groups() to simplify the code a bit.

The 'sensors' command never worked with this driver, fixed with this conversion.

Hi Axel,

You'd have to be a bit more specific here. Why ? It is not obvious to me. If you mean the 'name' attribute, that is created by the i2c subsystem.

The driver is buggy, though. It shows the current fan speed as minimum fan speed. Typical bug that can happen easily with function macros. But that is a different problem.

The driver could also benefit from some cleanup. It wastes code space by using function macros instead of SENSOR_ATTR_2, and functions should be reordered to get rid of forward declarations. Difficult to do without HW or at least simulation, though. Jean, do you have a register dump, by any chance ?

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#2 lore raw
In reply to Axel Lin

2014-06-24 22:38 GMT+08:00 Guenter Roeck <[email protected]>:

Quoted 5 lines · Axel Lin
Quoted 4 lines

Use ATTRIBUTE_GROUPS macro and devm_hwmon_device_register_with_groups() to simplify the code a bit.

Quoted 7 lines
Quoted 3 lines

The 'sensors' command never worked with this driver, fixed with this conversion.

Hi Axel,

You'd have to be a bit more specific here. Why ? It is not obvious to me. If you mean the 'name' attribute, that is created by the i2c subsystem.

I thought the 'name' attribute was missing, but since it's created by the i2c subsystem, that would be fine.

Quoted 3 lines

The driver is buggy, though. It shows the current fan speed as minimum fan speed.

I just sent a patch to fix this.

Quoted 8 lines

Typical bug that can happen easily with function macros. But that is a different
problem.

The driver could also benefit from some cleanup. It wastes code space by using
function macros instead of SENSOR_ATTR_2, and functions should be reordered to get rid of forward declarations.

ok. I'll address these issues.
Thanks for your review,
Axel

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#3 lore raw
In reply to Axel Lin
Quoted 27 lines · Axel Lin

2014-06-24 22:38 GMT+08:00 Guenter Roeck <[email protected]>:

Quoted 5 lines · Axel Lin
Quoted 4 lines

Use ATTRIBUTE_GROUPS macro and devm_hwmon_device_register_with_groups() to simplify the code a bit.

Quoted 7 lines
Quoted 3 lines

The 'sensors' command never worked with this driver, fixed with this conversion.

Hi Axel,

You'd have to be a bit more specific here. Why ? It is not obvious to me. If you mean the 'name' attribute, that is created by the i2c subsystem.

I thought the 'name' attribute was missing, but since it's created by the i2c subsystem, that would be fine.

Quoted 3 lines

The driver is buggy, though. It shows the current fan speed as minimum fan speed.

I just sent a patch to fix this.

Quoted 4 lines

Typical bug that can happen easily with function macros. But that is a different
problem.

Quoted 7 lines
Quoted 4 lines

The driver could also benefit from some cleanup. It wastes code space by using
function macros instead of SENSOR_ATTR_2, and functions should be reordered to get rid of forward declarations.

ok. I'll address these issues.
Thanks for your review,

Thank you for taking the time to fix issues.
Since I've had no w83l786ng chips available for testing, I couldn't verify that your patch works or not.

Quoted 1 line

Axel

Kevin

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#4 lore raw
In reply to Axel Lin

Hi Guenter,

Quoted 4 lines · Guenter Roeck

The driver could also benefit from some cleanup. It wastes code space by using function macros instead of SENSOR_ATTR_2, and functions should be reordered to get rid of forward declarations. Difficult to do without HW or at least simulation, though. Jean, do you have a register dump, by any chance ?

I have the following:

0 1 2 3 4 5 6 7 8 9 a b c d e f
20: c0 c1 c2 XX XX 25 26 XX 88 89 XX ff 00 ff 00 ff 30: 00 XX XX XX XX 50 4b 50 4b XX XX ff ff f0 f0 XX 40: 01 00 00 de fe 00 00 00 XX 00 00 00 a3 5c 80 XX 50: XX 00 04 41 11 XX XX XX XX XX XX XX XX XX XX XX 80: 3c ff ff 00 00 00 00 ff ff 00 00 00 00 33 XX 22 90: 00 14 0a 00 00 00 19 19 XX XX XX XX XX XX XX XX

It was stored under the name w83l786ng-powerup.dump, so it is likely that I generated it based on the default register values in the datasheet and this isn't an real chip dump. Hope that helps anyway.

Signature · 7 lines
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#5 lore raw
In reply to Axel Lin
Quoted 18 lines · Jean Delvare

Hi Guenter,

Quoted 4 lines · Guenter Roeck

The driver could also benefit from some cleanup. It wastes code space by using function macros instead of SENSOR_ATTR_2, and functions should be reordered to get rid of forward declarations. Difficult to do without HW or at least simulation, though. Jean, do you have a register dump, by any chance ?

I have the following:

0 1 2 3 4 5 6 7 8 9 a b c d e f
20: c0 c1 c2 XX XX 25 26 XX 88 89 XX ff 00 ff 00 ff 30: 00 XX XX XX XX 50 4b 50 4b XX XX ff ff f0 f0 XX 40: 01 00 00 de fe 00 00 00 XX 00 00 00 a3 5c 80 XX 50: XX 00 04 41 11 XX XX XX XX XX XX XX XX XX XX XX 80: 3c ff ff 00 00 00 00 ff ff 00 00 00 00 33 XX 22 90: 00 14 0a 00 00 00 19 19 XX XX XX XX XX XX XX XX

Quoted 4 lines

It was stored under the name w83l786ng-powerup.dump, so it is likely that I generated it based on the default register values in the datasheet and this isn't an real chip dump. Hope that helps anyway.

Seems to be working. This is what I get after applying the patches:

w83l786ng-i2c-7-2e

Adapter: SMBus stub driver
in0:          +1.54 V  (min =  +0.00 V, max =  +2.04 V)
in1:          +1.54 V  (min =  +0.00 V, max =  +2.04 V)
in2:          +1.55 V  (min =  +0.00 V, max =  +2.04 V)
fan1:        4963 RPM  (min =    0 RPM, div = 2)
fan2:        4927 RPM  (min =    0 RPM, div = 2)
temp1:        +37.0°C  (high = +80.0°C, hyst = +75.0°C)
temp2:        +38.0°C  (high = +80.0°C, hyst = +75.0°C)

Do you have all those register dumps online, by any chance ? That would be quite valuable.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#6 lore raw
In reply to Axel Lin
Quoted 2 lines · Guenter Roeck

Do you have all those register dumps online, by any chance ? That would be quite valuable.

I do not, but I can certainly setup something. Just give me a moment to clean up my collection into something I can share publicly.

Signature · 7 lines
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#7 lore raw
In reply to Axel Lin
Quoted 7 lines · Jean Delvare
Quoted 2 lines · Guenter Roeck

Do you have all those register dumps online, by any chance ? That would be quite valuable.

I do not, but I can certainly setup something. Just give me a moment to clean up my collection into something I can share publicly.

That would be great - thanks a lot!

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#8 lore raw
In reply to Axel Lin

Hi Guenter,

Quoted 2 lines · Guenter Roeck

Do you have all those register dumps online, by any chance ? That would be quite valuable.

I'm done cleaning up my 200 hwmon dump file collection, you can find it all online at:
http://jdelvare.nerim.net/devel/lm-sensors/dumps/

Note: some I got from you ;-)

I've also packed them for easier download:
http://jdelvare.nerim.net/devel/lm-sensors/dumps/hwmon-dumps.tar.gz

Signature · 7 lines
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
maintainer
#9 lore raw
In reply to Axel Lin
Quoted 6 lines · Jean Delvare

Hi Guenter,

Quoted 2 lines · Guenter Roeck

Do you have all those register dumps online, by any chance ? That would be quite valuable.

Quoted 9 lines

I'm done cleaning up my 200 hwmon dump file collection, you can find it all online at:
http://jdelvare.nerim.net/devel/lm-sensors/dumps/

Note: some I got from you ;-)

I've also packed them for easier download:
http://jdelvare.nerim.net/devel/lm-sensors/dumps/hwmon-dumps.tar.gz

Thanks a lot!

Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors