[lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[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.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
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(-)

diff --git a/drivers/hwmon/w83l786ng.c b/drivers/hwmon/w83l786ng.c
index 6ed76ce..8c0e370 100644
--- a/drivers/hwmon/w83l786ng.c
+++ b/drivers/hwmon/w83l786ng.c
@@ -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;
 
-- 
1.8.3.2




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Guenter Roeck]

On 06/24/2014 04:27 AM, Axel Lin wrote:
> 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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Axel Lin]

2014-06-24 22:38 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
> On 06/24/2014 04:27 AM, Axel Lin wrote:
>>
>> 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.
I thought the 'name' attribute was missing, but since it's created by the i2c
subsystem, that would be fine.

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

I just sent a patch to fix this.

> 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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Kevin Lo]

On Wed, Jun 25, 2014 at 09:06:17AM +0800, Axel Lin wrote:
> 
> 2014-06-24 22:38 GMT+08:00 Guenter Roeck <linux@roeck-us.net>:
> > On 06/24/2014 04:27 AM, Axel Lin wrote:
> >>
> >> 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.
> I thought the 'name' attribute was missing, but since it's created by the i2c
> subsystem, that would be fine.
> 
> >
> > The driver is buggy, though. It shows the current fan speed as minimum fan
> > speed.
> 
> I just sent a patch to fix this.
> 
> > 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,

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.

> Axel

	Kevin

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Jean Delvare]

Hi Guenter,

On Tue, 24 Jun 2014 07:38:46 -0700, Guenter Roeck wrote:
> 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.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Guenter Roeck]

On 06/24/2014 10:49 PM, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 24 Jun 2014 07:38:46 -0700, Guenter Roeck wrote:
>> 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.
>

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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Jean Delvare]

On Tue, 24 Jun 2014 23:26:48 -0700, Guenter Roeck wrote:
> 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.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Guenter Roeck]

On 06/24/2014 11:44 PM, Jean Delvare wrote:
> On Tue, 24 Jun 2014 23:26:48 -0700, Guenter Roeck wrote:
>> 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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Jean Delvare]

Hi Guenter,

On Tue, 24 Jun 2014 23:26:48 -0700, Guenter Roeck wrote:
> 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

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [RFT][PATCH] hwmon: (w83l786ng) Convert to devm_hwmon_device_register_with_groups

[Guenter Roeck]

On 06/25/2014 03:29 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 24 Jun 2014 23:26:48 -0700, Guenter Roeck wrote:
>> 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
>

Thanks a lot!

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors