[lm-sensors] [patch] hwmon: w83781d - check return code of

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jim Cromie]

add 2 attr-file groups (for base and model-specific attrs respectively),
create the base group with single call to sysfs_create_group,
check the return code on individual calls to device_create_file for each
of the model-specific attr-files.

Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

This is compile-tested only, needs validation on hardware, or
very thorough inspection.

$ diffstat diff.w83781d-checkrc.txt
 w83781d.c |  192 
++++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 119 insertions(+), 73 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff.w83781d-checkrc.txt
Type: plain/text
Size: 10002 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060831/1f312e73/attachment.bin 

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jean Delvare]

Hi Jim,

> add 2 attr-file groups (for base and model-specific attrs respectively),
> create the base group with single call to sysfs_create_group,
> check the return code on individual calls to device_create_file for each
> of the model-specific attr-files.
> 
> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
> 
> This is compile-tested only, needs validation on hardware, or
> very thorough inspection.

Thanks for working on this. I don't have a compatible chip either, but
I'll look for a tester (unless someone on the list can test?)

Can you set the mime-type of the attachement to text/plain rather than
plain/text next time? ;)

> diff -ruNp -X dontdiff -X exclude-diffs w83-1/drivers/hwmon/w83781d.c w83-rconly/drivers/hwmon/w83781d.c
> --- w83-1/drivers/hwmon/w83781d.c	2006-08-07 15:37:19.000000000 -0600
> +++ w83-rconly/drivers/hwmon/w83781d.c	2006-08-31 07:47:08.000000000 -0600
> @@ -40,6 +40,8 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-isa.h>
>  #include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/sysfs.h>
>  #include <linux/hwmon-vid.h>
>  #include <linux/err.h>
>  #include <linux/mutex.h>
> @@ -360,11 +362,9 @@ sysfs_in_offsets(7);
>  sysfs_in_offsets(8);
>  
>  #define device_create_file_in(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_in##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_in##offset##_min); \
> -device_create_file(&client->dev, &dev_attr_in##offset##_max); \
> -} while (0)
> +	device_create_file(&client->dev, &dev_attr_in##offset##_input)	\
> +	|| device_create_file(&client->dev, &dev_attr_in##offset##_min) \
> +	|| device_create_file(&client->dev, &dev_attr_in##offset##_max) \

Can you please get rid of all these macros while you're here? I didn't
like them much before, but now it's even worse as they are never
functions not constants but random code exerpts, which is quite
misleading and could cause a lot of confusion later. This would also
let you return the error values properly.

>  
>  #define show_fan_reg(reg) \
>  static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> @@ -421,10 +421,8 @@ sysfs_fan_offset(3);
>  sysfs_fan_min_offset(3);
>  
>  #define device_create_file_fan(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_min); \
> -} while (0)
> +	device_create_file(&client->dev, &dev_attr_fan##offset##_input) \
> +	|| device_create_file(&client->dev, &dev_attr_fan##offset##_min)
>  
>  #define show_temp_reg(reg) \
>  static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
> @@ -497,11 +495,9 @@ sysfs_temp_offsets(2);
>  sysfs_temp_offsets(3);
>  
>  #define device_create_file_temp(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_max); \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \
> -} while (0)
> +	device_create_file(&client->dev, &dev_attr_temp##offset##_input) \
> +	|| device_create_file(&client->dev, &dev_attr_temp##offset##_max) \
> +	|| device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst)
>  
>  static ssize_t
>  show_vid_reg(struct device *dev, struct device_attribute *attr, char *buf)
> @@ -513,7 +509,7 @@ show_vid_reg(struct device *dev, struct 
>  static
>  DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid_reg, NULL);
>  #define device_create_file_vid(client) \
> -device_create_file(&client->dev, &dev_attr_cpu0_vid);
> +device_create_file(&client->dev, &dev_attr_cpu0_vid)
>  static ssize_t
>  show_vrm_reg(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -537,7 +533,7 @@ store_vrm_reg(struct device *dev, struct
>  static
>  DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
>  #define device_create_file_vrm(client) \
> -device_create_file(&client->dev, &dev_attr_vrm);
> +device_create_file(&client->dev, &dev_attr_vrm)
>  static ssize_t
>  show_alarms_reg(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -548,7 +544,7 @@ show_alarms_reg(struct device *dev, stru
>  static
>  DEVICE_ATTR(alarms, S_IRUGO, show_alarms_reg, NULL);
>  #define device_create_file_alarms(client) \
> -device_create_file(&client->dev, &dev_attr_alarms);
> +device_create_file(&client->dev, &dev_attr_alarms)
>  static ssize_t show_beep_mask (struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct w83781d_data *data = w83781d_update_device(dev);
> @@ -615,10 +611,8 @@ sysfs_beep(ENABLE, enable);
>  sysfs_beep(MASK, mask);
>  
>  #define device_create_file_beep(client) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_beep_enable); \
> -device_create_file(&client->dev, &dev_attr_beep_mask); \
> -} while (0)
> +	device_create_file(&client->dev, &dev_attr_beep_enable) \
> +	|| device_create_file(&client->dev, &dev_attr_beep_mask)
>  
>  static ssize_t
>  show_fan_div_reg(struct device *dev, char *buf, int nr)
> @@ -686,9 +680,7 @@ sysfs_fan_div(2);
>  sysfs_fan_div(3);
>  
>  #define device_create_file_fan_div(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
> -} while (0)
> +	device_create_file(&client->dev, &dev_attr_fan##offset##_div)
>  
>  static ssize_t
>  show_pwm_reg(struct device *dev, char *buf, int nr)
> @@ -787,14 +779,10 @@ sysfs_pwm(3);
>  sysfs_pwm(4);
>  
>  #define device_create_file_pwm(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_pwm##offset); \
> -} while (0)
> +device_create_file(&client->dev, &dev_attr_pwm##offset)
>  
>  #define device_create_file_pwmenable(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_pwm##offset##_enable); \
> -} while (0)
> +device_create_file(&client->dev, &dev_attr_pwm##offset##_enable)
>  
>  static ssize_t
>  show_sensor_reg(struct device *dev, char *buf, int nr)
> @@ -865,9 +853,7 @@ sysfs_sensor(2);
>  sysfs_sensor(3);
>  
>  #define device_create_file_sensor(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_type); \
> -} while (0)
> +device_create_file(&client->dev, &dev_attr_temp##offset##_type)
>  
>  /* This function is called when:
>       * w83781d_driver is inserted (when this module is loaded), for each
> @@ -993,6 +979,70 @@ ERROR_SC_0:
>  	return err;
>  }
>  
> +#define IN_UNIT_ATTRS(X)			\
> +	&dev_attr_in##X##_input.attr,		\
> +	&dev_attr_in##X##_min.attr,		\
> +	&dev_attr_in##X##_max.attr
> +
> +#define FAN_UNIT_ATTRS(X)			\
> +	&dev_attr_fan##X##_input.attr,		\
> +	&dev_attr_fan##X##_min.attr,		\
> +	&dev_attr_fan##X##_div.attr
> +
> +#define TEMP_UNIT_ATTRS(X)			\
> +	&dev_attr_temp##X##_input.attr,		\
> +	&dev_attr_temp##X##_max.attr,		\
> +	&dev_attr_temp##X##_max_hyst.attr
> +
> +static struct attribute *w83781d_attributes[] = {
> +

No blank line here please.

> +	IN_UNIT_ATTRS(0),
> +	IN_UNIT_ATTRS(2),
> +	IN_UNIT_ATTRS(3),
> +	IN_UNIT_ATTRS(4),
> +	IN_UNIT_ATTRS(5),
> +	IN_UNIT_ATTRS(6),
> +	
> +	FAN_UNIT_ATTRS(1),
> +	FAN_UNIT_ATTRS(2),
> +	FAN_UNIT_ATTRS(3),
> +	
> +	TEMP_UNIT_ATTRS(1),
> +	TEMP_UNIT_ATTRS(2),
> +	
> +	&dev_attr_cpu0_vid.attr,
> +	&dev_attr_vrm.attr,
> +	&dev_attr_alarms.attr,
> +	&dev_attr_beep_mask.attr,
> +	&dev_attr_beep_enable.attr,
> +	

Nor here.

> +	NULL,

And no comma here.
> +};
> +static struct attribute_group w83781d_group = {
> +	.attrs = w83781d_attributes,
> +};
> +
> +static struct attribute *w83781d_opt_attributes[] = {
> +	IN_UNIT_ATTRS(1),
> +	IN_UNIT_ATTRS(7),
> +	IN_UNIT_ATTRS(8),
> +	TEMP_UNIT_ATTRS(3),
> +
> +	&dev_attr_pwm1.attr,
> +	&dev_attr_pwm2.attr,
> +	&dev_attr_pwm2_enable.attr,
> +	&dev_attr_pwm3.attr,
> +	&dev_attr_pwm4.attr,
> +
> +	&dev_attr_temp1_type.attr,	/* for sensor */
> +	&dev_attr_temp2_type.attr,
> +	&dev_attr_temp3_type.attr,
> +	NULL,
> +};
> +static struct attribute_group w83781d_opt_group = {
> +	.attrs = w83781d_opt_attributes,
> +};

Mark and I went for _group_opt and _attributes_opt suffixes. It's of
course arbitrary, but maybe you can do the same for consistency?

> +
>  static int
>  w83781d_detect(struct i2c_adapter *adapter, int address, int kind)
>  {
> @@ -1211,64 +1261,57 @@ w83781d_detect(struct i2c_adapter *adapt
>  			data->pwmenable[i] = 1;
>  
>  	/* Register sysfs hooks */
> -	data->class_dev = hwmon_device_register(&new_client->dev);
> -	if (IS_ERR(data->class_dev)) {
> -		err = PTR_ERR(data->class_dev);
> -		goto ERROR4;
> -	}
> +	if ((err = sysfs_create_group(&new_client->dev.kobj,
> +				      &w83781d_group)))
> +		goto ERROR5;
>  
> -	device_create_file_in(new_client, 0);
>  	if (kind != w83783s)
> -		device_create_file_in(new_client, 1);
> -	device_create_file_in(new_client, 2);
> -	device_create_file_in(new_client, 3);
> -	device_create_file_in(new_client, 4);
> -	device_create_file_in(new_client, 5);
> -	device_create_file_in(new_client, 6);
> +		if (device_create_file_in(new_client, 1))
> +			goto ERROR5;
> +
>  	if (kind != as99127f && kind != w83781d && kind != w83783s) {
> -		device_create_file_in(new_client, 7);
> -		device_create_file_in(new_client, 8);
> +		if (device_create_file_in(new_client, 7)
> +		    || device_create_file_in(new_client, 8))
> +			goto ERROR5;
>  	}
>  
> -	device_create_file_fan(new_client, 1);
> -	device_create_file_fan(new_client, 2);
> -	device_create_file_fan(new_client, 3);
> -
> -	device_create_file_temp(new_client, 1);
> -	device_create_file_temp(new_client, 2);
>  	if (kind != w83783s)
> -		device_create_file_temp(new_client, 3);
> -
> -	device_create_file_vid(new_client);
> -	device_create_file_vrm(new_client);
> -
> -	device_create_file_fan_div(new_client, 1);
> -	device_create_file_fan_div(new_client, 2);
> -	device_create_file_fan_div(new_client, 3);
> -
> -	device_create_file_alarms(new_client);
> -
> -	device_create_file_beep(new_client);
> +		if (device_create_file_temp(new_client, 3))
> +			goto ERROR5;
>  
>  	if (kind != w83781d && kind != as99127f) {
> -		device_create_file_pwm(new_client, 1);
> -		device_create_file_pwm(new_client, 2);
> -		device_create_file_pwmenable(new_client, 2);
> +		if (device_create_file_pwm(new_client, 1)
> +		    || device_create_file_pwm(new_client, 2)
> +		    || device_create_file_pwmenable(new_client, 2))
> +			goto ERROR5;
>  	}
>  	if (kind = w83782d && !is_isa) {
> -		device_create_file_pwm(new_client, 3);
> -		device_create_file_pwm(new_client, 4);
> +		if (device_create_file_pwm(new_client, 3)
> +		    || device_create_file_pwm(new_client, 4))
> +			goto ERROR5;
>  	}
>  
>  	if (kind != as99127f && kind != w83781d) {
> -		device_create_file_sensor(new_client, 1);
> -		device_create_file_sensor(new_client, 2);
> +		if (device_create_file_sensor(new_client, 1)
> +		    || device_create_file_sensor(new_client, 2))
> +			goto ERROR5;
>  		if (kind != w83783s)
> -			device_create_file_sensor(new_client, 3);
> +			if (device_create_file_sensor(new_client, 3))
> +				goto ERROR5;
> +	}
> +
> +	data->class_dev = hwmon_device_register(&new_client->dev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto ERROR5;
>  	}
>  
>  	return 0;
>  
> +ERROR5:
> +	sysfs_remove_group(&new_client->dev.kobj, &w83781d_group);
> +	sysfs_remove_group(&new_client->dev.kobj, &w83781d_opt_group);
> +
>  ERROR4:
>  	if (data->lm75[1]) {
>  		i2c_detach_client(data->lm75[1]);
> @@ -1299,6 +1342,9 @@ w83781d_detach_client(struct i2c_client 
>  	if (data)
>  		hwmon_device_unregister(data->class_dev);
>  
> +	sysfs_remove_group(&client->dev.kobj, &w83781d_group);
> +	sysfs_remove_group(&client->dev.kobj, &w83781d_opt_group);
> +

This should be inside the if (data) conditional - subclients have no
sysfs files.

>  	if (i2c_is_isa_client(client))
>  		release_region(client->addr, W83781D_EXTENT);
>  
> 

Care to respin a patch? I'd like to have all these fixes in -mm soon.

Thanks,
-- 
Jean Delvare

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jim Cromie]

Add 2 attr-file groups (for base and model-specific attrs respectively),
create the base group with single call to sysfs_create_group,
check the return code on individual calls to device_create_file for each
of the model-specific attr-files.

Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

---

This is compile-tested only, needs validation on hardware, or
very thorough inspection.

diffstat hwmon-unchecked-return-status-fixes-w83781d.patch
 w83781d.c |  292 
++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 155 insertions(+), 137 deletions(-)

> Can you please get rid of all these macros while you're here? I didn't
> like them much before, but now it's even worse as they are never
> functions not constants but random code exerpts, which is quite
> misleading and could cause a lot of confusion later. This would also
> let you return the error values properly.
>
>   

theyre gone. (the file_<foo> macros)

There are several distasteful macro-function-decl-expansions still there :

show_in_reg(in);
show_in_reg(in_min);
show_in_reg(in_max);

store_in_reg(MIN, min);
store_in_reg(MAX, max);

and also a compound one:  This one in particular is crying for a 
2D-callback implementation.
Ill do it (its my axe, Ill grind it some more ;-), IFF you find a tester 
for it.
Anyway, thats not for 19.

#define sysfs_in_offsets(offset) \
sysfs_in_offset(offset); \
sysfs_in_reg_offset(min, offset); \
sysfs_in_reg_offset(max, offset);

sysfs_in_offsets(0);
sysfs_in_offsets(1);
sysfs_in_offsets(2);
sysfs_in_offsets(3);
sysfs_in_offsets(4);
sysfs_in_offsets(5);
sysfs_in_offsets(6);
sysfs_in_offsets(7);
sysfs_in_offsets(8);



These all addressed :
> No blank line here please.
>
>   
> Nor here.
>
>   
> And no comma here.
>   
>
> Mark and I went for _group_opt and _attributes_opt suffixes. It's of
> course arbitrary, but maybe you can do the same for consistency?
>   

> This should be inside the if (data) conditional - subclients have no
> sysfs files.
>
>   
>
> Care to respin a patch? I'd like to have all these fixes in -mm soon.
>
> Thanks,
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-unchecked-return-status-fixes-w83781d.patch
Type: text/x-patch
Size: 15978 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060921/4f41ee3c/attachment-0001.bin 

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jean Delvare]

Hi Jim,

> Add 2 attr-file groups (for base and model-specific attrs respectively),
> create the base group with single call to sysfs_create_group,
> check the return code on individual calls to device_create_file for each
> of the model-specific attr-files.
> 
> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>
> 
> ---
> 
> This is compile-tested only, needs validation on hardware, or
> very thorough inspection.

I have some more comments, see below.

> There are several distasteful macro-function-decl-expansions still there :
> 
> show_in_reg(in);
> show_in_reg(in_min);
> show_in_reg(in_max);
> 
> store_in_reg(MIN, min);
> store_in_reg(MAX, max);
> 
> and also a compound one:  This one in particular is crying for a 
> 2D-callback implementation.
> Ill do it (its my axe, Ill grind it some more ;-), IFF you find a tester 
> for it.
> Anyway, thats not for 19.

Yes, there is much room for cleanups to this driver using at least 1D
callbacks, and possibly 2D callbacks in some cases. If you want to
provide a patch doing this, this is welcome - just don't be overzealous
and cleanup the code without rewriting too much of it please. And as
you noticed, this is 2.6.20 material at this point.

> diff -ruNp -X dontdiff -X exclude-diffs 6rlocks-1/drivers/hwmon/w83781d.c 6rlocks-w83/drivers/hwmon/w83781d.c
> --- 6rlocks-1/drivers/hwmon/w83781d.c	2006-09-19 23:58:37.000000000 -0600
> +++ 6rlocks-w83/drivers/hwmon/w83781d.c	2006-09-21 22:12:06.000000000 -0600
> @@ -40,6 +40,8 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-isa.h>
>  #include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

You do not appear to actually need this one yet.

> +#include <linux/sysfs.h>
>  #include <linux/hwmon-vid.h>
>  #include <linux/err.h>
>  #include <linux/mutex.h>
> @@ -359,13 +361,6 @@ sysfs_in_offsets(6);
>  sysfs_in_offsets(7);
>  sysfs_in_offsets(8);
>  
> -#define device_create_file_in(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_in##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_in##offset##_min); \
> -device_create_file(&client->dev, &dev_attr_in##offset##_max); \
> -} while (0)
> -
>  #define show_fan_reg(reg) \
>  static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
>  { \
> @@ -420,12 +415,6 @@ sysfs_fan_min_offset(2);
>  sysfs_fan_offset(3);
>  sysfs_fan_min_offset(3);
>  
> -#define device_create_file_fan(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_min); \
> -} while (0)
> -
>  #define show_temp_reg(reg) \
>  static ssize_t show_##reg (struct device *dev, char *buf, int nr) \
>  { \
> @@ -496,13 +485,6 @@ sysfs_temp_offsets(1);
>  sysfs_temp_offsets(2);
>  sysfs_temp_offsets(3);
>  
> -#define device_create_file_temp(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_input); \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_max); \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_max_hyst); \
> -} while (0)
> -
>  static ssize_t
>  show_vid_reg(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -510,10 +492,8 @@ show_vid_reg(struct device *dev, struct 
>  	return sprintf(buf, "%ld\n", (long) vid_from_reg(data->vid, data->vrm));
>  }
>  
> -static
> -DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid_reg, NULL);
> -#define device_create_file_vid(client) \
> -device_create_file(&client->dev, &dev_attr_cpu0_vid);
> +static DEVICE_ATTR(cpu0_vid, S_IRUGO, show_vid_reg, NULL);
> +
>  static ssize_t
>  show_vrm_reg(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -533,11 +513,8 @@ store_vrm_reg(struct device *dev, struct
>  
>  	return count;
>  }
> +static DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
>  
> -static
> -DEVICE_ATTR(vrm, S_IRUGO | S_IWUSR, show_vrm_reg, store_vrm_reg);
> -#define device_create_file_vrm(client) \
> -device_create_file(&client->dev, &dev_attr_vrm);
>  static ssize_t
>  show_alarms_reg(struct device *dev, struct device_attribute *attr, char *buf)
>  {
> @@ -545,17 +522,16 @@ show_alarms_reg(struct device *dev, stru
>  	return sprintf(buf, "%u\n", data->alarms);
>  }
>  
> -static
> -DEVICE_ATTR(alarms, S_IRUGO, show_alarms_reg, NULL);
> -#define device_create_file_alarms(client) \
> -device_create_file(&client->dev, &dev_attr_alarms);
> -static ssize_t show_beep_mask (struct device *dev, struct device_attribute *attr, char *buf)
> +static DEVICE_ATTR(alarms, S_IRUGO, show_alarms_reg, NULL);
> +
> +static ssize_t
> +show_beep_mask(struct device *dev, struct device_attribute *attr, char *buf)

Please back out this one cleanup - it doesn't belong to this patch.

>  {
>  	struct w83781d_data *data = w83781d_update_device(dev);
>  	return sprintf(buf, "%ld\n",
>  		       (long)BEEP_MASK_FROM_REG(data->beep_mask, data->type));
>  }
> -static ssize_t show_beep_enable (struct device *dev, struct device_attribute *attr, char *buf)
> +static ssize_t show_beep_enable(struct device *dev, struct device_attribute *attr, char *buf)

Same here.

>  {
>  	struct w83781d_data *data = w83781d_update_device(dev);
>  	return sprintf(buf, "%ld\n",
> @@ -614,12 +590,6 @@ static DEVICE_ATTR(beep_##reg, S_IRUGO |
>  sysfs_beep(ENABLE, enable);
>  sysfs_beep(MASK, mask);
>  
> -#define device_create_file_beep(client) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_beep_enable); \
> -device_create_file(&client->dev, &dev_attr_beep_mask); \
> -} while (0)
> -
>  static ssize_t
>  show_fan_div_reg(struct device *dev, char *buf, int nr)
>  {
> @@ -685,11 +655,6 @@ sysfs_fan_div(1);
>  sysfs_fan_div(2);
>  sysfs_fan_div(3);
>  
> -#define device_create_file_fan_div(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_fan##offset##_div); \
> -} while (0)
> -
>  static ssize_t
>  show_pwm_reg(struct device *dev, char *buf, int nr)
>  {
> @@ -786,16 +751,6 @@ sysfs_pwmenable(2);		/* only PWM2 can be
>  sysfs_pwm(3);
>  sysfs_pwm(4);
>  
> -#define device_create_file_pwm(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_pwm##offset); \
> -} while (0)
> -
> -#define device_create_file_pwmenable(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_pwm##offset##_enable); \
> -} while (0)
> -
>  static ssize_t
>  show_sensor_reg(struct device *dev, char *buf, int nr)
>  {
> @@ -864,11 +819,6 @@ sysfs_sensor(1);
>  sysfs_sensor(2);
>  sysfs_sensor(3);
>  
> -#define device_create_file_sensor(client, offset) \
> -do { \
> -device_create_file(&client->dev, &dev_attr_temp##offset##_type); \
> -} while (0)
> -
>  /* This function is called when:
>       * w83781d_driver is inserted (when this module is loaded), for each
>         available adapter
> @@ -993,11 +943,69 @@ ERROR_SC_0:
>  	return err;
>  }
>  
> +#define IN_UNIT_ATTRS(X)			\
> +	&dev_attr_in##X##_input.attr,		\
> +	&dev_attr_in##X##_min.attr,		\
> +	&dev_attr_in##X##_max.attr
> +
> +#define FAN_UNIT_ATTRS(X)			\
> +	&dev_attr_fan##X##_input.attr,		\
> +	&dev_attr_fan##X##_min.attr,		\
> +	&dev_attr_fan##X##_div.attr
> +
> +#define TEMP_UNIT_ATTRS(X)			\
> +	&dev_attr_temp##X##_input.attr,		\
> +	&dev_attr_temp##X##_max.attr,		\
> +	&dev_attr_temp##X##_max_hyst.attr
> +
> +static struct attribute *w83781d_attributes[] = {
> +	IN_UNIT_ATTRS(0),
> +	IN_UNIT_ATTRS(2),
> +	IN_UNIT_ATTRS(3),
> +	IN_UNIT_ATTRS(4),
> +	IN_UNIT_ATTRS(5),
> +	IN_UNIT_ATTRS(6),
> +	FAN_UNIT_ATTRS(1),
> +	FAN_UNIT_ATTRS(2),
> +	FAN_UNIT_ATTRS(3),
> +	TEMP_UNIT_ATTRS(1),
> +	TEMP_UNIT_ATTRS(2),
> +	&dev_attr_cpu0_vid.attr,
> +	&dev_attr_vrm.attr,
> +	&dev_attr_alarms.attr,
> +	&dev_attr_beep_mask.attr,
> +	&dev_attr_beep_enable.attr,
> +	NULL
> +};
> +static struct attribute_group w83781d_group = {
> +	.attrs = w83781d_attributes,
> +};

This one can be declared const.

> +
> +static struct attribute *w83781d_attributes_opt[] = {
> +	IN_UNIT_ATTRS(1),
> +	IN_UNIT_ATTRS(7),
> +	IN_UNIT_ATTRS(8),
> +	TEMP_UNIT_ATTRS(3),
> +	&dev_attr_pwm1.attr,
> +	&dev_attr_pwm2.attr,
> +	&dev_attr_pwm2_enable.attr,
> +	&dev_attr_pwm3.attr,
> +	&dev_attr_pwm4.attr,
> +	&dev_attr_temp1_type.attr,	/* for sensor */
> +	&dev_attr_temp2_type.attr,
> +	&dev_attr_temp3_type.attr,
> +	NULL
> +};
> +static struct attribute_group w83781d_group_opt = {
> +	.attrs = w83781d_attributes_opt,
> +};

const

> +
>  static int
>  w83781d_detect(struct i2c_adapter *adapter, int address, int kind)
>  {
>  	int i = 0, val1 = 0, val2;
> -	struct i2c_client *new_client;
> +	struct i2c_client *client;
> +	struct device *clidev;

I would have gone with just "dev" as there is no other device involved,
so no room for confusion. But the choice is yours.

>  	struct w83781d_data *data;
>  	int err;
>  	const char *client_name = "";
> @@ -1074,13 +1082,14 @@ w83781d_detect(struct i2c_adapter *adapt
>  		goto ERROR1;
>  	}
>  
> -	new_client = &data->client;
> -	i2c_set_clientdata(new_client, data);
> -	new_client->addr = address;
> +	client = &data->client;
> +	i2c_set_clientdata(client, data);
> +	client->addr = address;
>  	mutex_init(&data->lock);
> -	new_client->adapter = adapter;
> -	new_client->driver = is_isa ? &w83781d_isa_driver : &w83781d_driver;
> -	new_client->flags = 0;
> +	client->adapter = adapter;
> +	client->driver = is_isa ? &w83781d_isa_driver : &w83781d_driver;
> +	client->flags = 0;
> +	clidev = &client->dev;
>  
>  	/* Now, we do the remaining detection. */
>  
> @@ -1089,20 +1098,18 @@ w83781d_detect(struct i2c_adapter *adapt
>  	   force_*=... parameter, and the Winbond will be reset to the right
>  	   bank. */
>  	if (kind < 0) {
> -		if (w83781d_read_value(new_client, W83781D_REG_CONFIG) & 0x80) {
> -			dev_dbg(&new_client->dev, "Detection failed at step "
> -				"3\n");
> +		if (w83781d_read_value(client, W83781D_REG_CONFIG) & 0x80) {
> +			dev_dbg(&client->dev, "Detection failed at step 3\n");

Why don't you use clidev now that you have defined it?

>  			err = -ENODEV;
>  			goto ERROR2;
>  		}
> -		val1 = w83781d_read_value(new_client, W83781D_REG_BANK);
> -		val2 = w83781d_read_value(new_client, W83781D_REG_CHIPMAN);
> +		val1 = w83781d_read_value(client, W83781D_REG_BANK);
> +		val2 = w83781d_read_value(client, W83781D_REG_CHIPMAN);
>  		/* Check for Winbond or Asus ID if in bank 0 */
>  		if ((!(val1 & 0x07)) &&
>  		    (((!(val1 & 0x80)) && (val2 != 0xa3) && (val2 != 0xc3))
>  		     || ((val1 & 0x80) && (val2 != 0x5c) && (val2 != 0x12)))) {
> -			dev_dbg(&new_client->dev, "Detection failed at step "
> -				"4\n");
> +			dev_dbg(&client->dev, "Detection failed at step 4\n");

Same here (and in several other places in this function.)

>  			err = -ENODEV;
>  			goto ERROR2;
>  		}
> @@ -1111,8 +1118,8 @@ w83781d_detect(struct i2c_adapter *adapt
>  		if ((!is_isa) && (((!(val1 & 0x80)) && (val2 = 0xa3)) ||
>  				  ((val1 & 0x80) && (val2 = 0x5c)))) {
>  			if (w83781d_read_value
> -			    (new_client, W83781D_REG_I2C_ADDR) != address) {
> -				dev_dbg(&new_client->dev, "Detection failed "
> +			    (client, W83781D_REG_I2C_ADDR) != address) {
> +				dev_dbg(&client->dev, "Detection failed "
>  					"at step 5\n");
>  				err = -ENODEV;
>  				goto ERROR2;
> @@ -1122,27 +1129,27 @@ w83781d_detect(struct i2c_adapter *adapt
>  
>  	/* We have either had a force parameter, or we have already detected the
>  	   Winbond. Put it now into bank 0 and Vendor ID High Byte */
> -	w83781d_write_value(new_client, W83781D_REG_BANK,
> -			    (w83781d_read_value(new_client,
> -						W83781D_REG_BANK) & 0x78) |
> -			    0x80);
> +	w83781d_write_value(client, W83781D_REG_BANK,
> +			    (w83781d_read_value(client,
> +						W83781D_REG_BANK)

Seems to fit on the same line?

> +			     & 0x78) | 0x80);
>  
>  	/* Determine the chip type. */
>  	if (kind <= 0) {
>  		/* get vendor ID */
> -		val2 = w83781d_read_value(new_client, W83781D_REG_CHIPMAN);
> +		val2 = w83781d_read_value(client, W83781D_REG_CHIPMAN);
>  		if (val2 = 0x5c)
>  			vendid = winbond;
>  		else if (val2 = 0x12)
>  			vendid = asus;
>  		else {
> -			dev_dbg(&new_client->dev, "Chip was made by neither "
> +			dev_dbg(&client->dev, "Chip was made by neither "
>  				"Winbond nor Asus?\n");
>  			err = -ENODEV;
>  			goto ERROR2;
>  		}
>  
> -		val1 = w83781d_read_value(new_client, W83781D_REG_WCHIPID);
> +		val1 = w83781d_read_value(client, W83781D_REG_WCHIPID);
>  		if ((val1 = 0x10 || val1 = 0x11) && vendid = winbond)
>  			kind = w83781d;
>  		else if (val1 = 0x30 && vendid = winbond)
> @@ -1156,7 +1163,7 @@ w83781d_detect(struct i2c_adapter *adapt
>  			kind = as99127f;
>  		else {
>  			if (kind = 0)
> -				dev_warn(&new_client->dev, "Ignoring 'force' "
> +				dev_warn(&client->dev, "Ignoring 'force' "
>  					 "parameter for unknown chip at "
>  					 "adapter %d, address 0x%02x\n",
>  					 i2c_adapter_id(adapter), address);
> @@ -1178,20 +1185,20 @@ w83781d_detect(struct i2c_adapter *adapt
>  	}
>  
>  	/* Fill in the remaining client fields and put into the global list */
> -	strlcpy(new_client->name, client_name, I2C_NAME_SIZE);
> +	strlcpy(client->name, client_name, I2C_NAME_SIZE);
>  	data->type = kind;
>  
>  	data->valid = 0;
>  	mutex_init(&data->update_lock);
>  
>  	/* Tell the I2C layer a new client has arrived */
> -	if ((err = i2c_attach_client(new_client)))
> +	if ((err = i2c_attach_client(client)))
>  		goto ERROR2;
>  
>  	/* attach secondary i2c lm75-like clients */
>  	if (!is_isa) {
>  		if ((err = w83781d_detect_subclients(adapter, address,
> -				kind, new_client)))
> +				kind, client)))
>  			goto ERROR3;
>  	} else {
>  		data->lm75[0] = NULL;
> @@ -1199,11 +1206,11 @@ w83781d_detect(struct i2c_adapter *adapt
>  	}
>  
>  	/* Initialize the chip */
> -	w83781d_init_client(new_client);
> +	w83781d_init_client(client);
>  
>  	/* A few vars need to be filled upon startup */
>  	for (i = 1; i <= 3; i++) {
> -		data->fan_min[i - 1] = w83781d_read_value(new_client,
> +		data->fan_min[i - 1] = w83781d_read_value(client,
>  					W83781D_REG_FAN_MIN(i));
>  	}
>  	if (kind != w83781d && kind != as99127f)
> @@ -1211,65 +1218,74 @@ w83781d_detect(struct i2c_adapter *adapt
>  			data->pwmenable[i] = 1;
>  
>  	/* Register sysfs hooks */
> -	data->class_dev = hwmon_device_register(&new_client->dev);
> -	if (IS_ERR(data->class_dev)) {
> -		err = PTR_ERR(data->class_dev);
> -		goto ERROR4;
> -	}
> +	if ((err = sysfs_create_group(&client->dev.kobj, &w83781d_group)))
> +		goto ERROR5;
>  
> -	device_create_file_in(new_client, 0);
> -	if (kind != w83783s)
> -		device_create_file_in(new_client, 1);
> -	device_create_file_in(new_client, 2);
> -	device_create_file_in(new_client, 3);
> -	device_create_file_in(new_client, 4);
> -	device_create_file_in(new_client, 5);
> -	device_create_file_in(new_client, 6);
> +	if (kind != w83783s) {
> +		err = device_create_file(clidev, &dev_attr_in1_input)
> +			|| device_create_file(clidev, &dev_attr_in1_min)
> +			|| device_create_file(clidev, &dev_attr_in1_max);
> +		if (err)
> +			goto ERROR5;
> +	}

Not correct, sorry. device_create_file() returns a real error value,
not just a boolean status, so using boolean logic directly loses the
error value. You need to do something like:

	if (kind != w83783s) {
		if ((err = device_create_file(clidev, &dev_attr_in1_input))
		 || (err = device_create_file(clidev, &dev_attr_in1_min))
		 || (err = device_create_file(clidev, &dev_attr_in1_max)))
			goto ERROR5;
	}

That way the value of "err" is properly returned. Same for all other
cases below.

>  	if (kind != as99127f && kind != w83781d && kind != w83783s) {
> -		device_create_file_in(new_client, 7);
> -		device_create_file_in(new_client, 8);
> +		err = device_create_file(clidev, &dev_attr_in7_input)
> +			|| device_create_file(clidev, &dev_attr_in7_min)
> +			|| device_create_file(clidev, &dev_attr_in7_max)
> +			|| device_create_file(clidev, &dev_attr_in8_input)
> +			|| device_create_file(clidev, &dev_attr_in8_min)
> +			|| device_create_file(clidev, &dev_attr_in8_max);
> +		if (err)
> +			goto ERROR5;
> +	}
> +	if (kind != w83783s) {
> +		err = device_create_file(clidev, &dev_attr_temp3_input)
> +			|| device_create_file(clidev, &dev_attr_temp3_max)
> +			|| device_create_file(clidev, &dev_attr_temp3_max_hyst);
> +			if (err)
> +				goto ERROR5;
>  	}
> -
> -	device_create_file_fan(new_client, 1);
> -	device_create_file_fan(new_client, 2);
> -	device_create_file_fan(new_client, 3);
> -
> -	device_create_file_temp(new_client, 1);
> -	device_create_file_temp(new_client, 2);
> -	if (kind != w83783s)
> -		device_create_file_temp(new_client, 3);
> -
> -	device_create_file_vid(new_client);
> -	device_create_file_vrm(new_client);
> -
> -	device_create_file_fan_div(new_client, 1);
> -	device_create_file_fan_div(new_client, 2);
> -	device_create_file_fan_div(new_client, 3);
> -
> -	device_create_file_alarms(new_client);
> -
> -	device_create_file_beep(new_client);
>  
>  	if (kind != w83781d && kind != as99127f) {
> -		device_create_file_pwm(new_client, 1);
> -		device_create_file_pwm(new_client, 2);
> -		device_create_file_pwmenable(new_client, 2);
> +		err = device_create_file(clidev, &dev_attr_pwm1)
> +			|| device_create_file(clidev, &dev_attr_pwm2)
> +			|| device_create_file(clidev, &dev_attr_pwm2_enable);
> +					      
> +		if (err)
> +			goto ERROR5;
>  	}
>  	if (kind = w83782d && !is_isa) {
> -		device_create_file_pwm(new_client, 3);
> -		device_create_file_pwm(new_client, 4);
> +		err = device_create_file(clidev, &dev_attr_pwm3)
> +			|| device_create_file(clidev, &dev_attr_pwm4);
> +		if (err)
> +			goto ERROR5;
>  	}
>  
>  	if (kind != as99127f && kind != w83781d) {
> -		device_create_file_sensor(new_client, 1);
> -		device_create_file_sensor(new_client, 2);
> -		if (kind != w83783s)
> -			device_create_file_sensor(new_client, 3);
> +		err = device_create_file(clidev, &dev_attr_temp1_type)
> +			|| device_create_file(clidev, &dev_attr_temp2_type);
> +		if (err)
> +			goto ERROR5;
> +		if (kind != w83783s) {
> +			err = device_create_file(clidev, &dev_attr_temp3_type);
> +			if (err)
> +				goto ERROR5;
> +		}
> +	}
> +
> +	data->class_dev = hwmon_device_register(clidev);
> +	if (IS_ERR(data->class_dev)) {
> +		err = PTR_ERR(data->class_dev);
> +		goto ERROR5;
>  	}
>  
>  	return 0;
>  
> -ERROR4:
> +ERROR5:
> +	sysfs_remove_group(&client->dev.kobj, &w83781d_group);
> +	sysfs_remove_group(&client->dev.kobj, &w83781d_group_opt);
> +
> +/* ERROR4: */

You could use ERROR4 for the first sysfs group creation. If not, then
kill the label completely rather than commenting it out.

>  	if (data->lm75[1]) {
>  		i2c_detach_client(data->lm75[1]);
>  		kfree(data->lm75[1]);
> @@ -1279,7 +1295,7 @@ ERROR4:
>  		kfree(data->lm75[0]);
>  	}
>  ERROR3:
> -	i2c_detach_client(new_client);
> +	i2c_detach_client(client);
>  ERROR2:
>  	kfree(data);
>  ERROR1:
> @@ -1296,9 +1312,11 @@ w83781d_detach_client(struct i2c_client 
>  	int err;
>  
>  	/* main client */
> -	if (data)
> +	if (data) {
>  		hwmon_device_unregister(data->class_dev);
> -
> +		sysfs_remove_group(&client->dev.kobj, &w83781d_group);
> +		sysfs_remove_group(&client->dev.kobj, &w83781d_group_opt);
> +	}
>  	if (i2c_is_isa_client(client))
>  		release_region(client->addr, W83781D_EXTENT);
>  

Care to repin a patch addressing the few objections above? Thanks.

-- 
Jean Delvare

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jim Cromie]

Add 2 attr-file groups (for base and model-specific attrs respectively),
create the base group with single call to sysfs_create_group, and
check the return code on individual calls to device_create_file for each
of the model-specific attr-files.

Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

---

all suggestions incorporated.

[jimc at harpo hwmon-stuff]$ diffstat hwmon-unchecked-return-status-fixes-w83781d.patch

 w83781d.c |  265 
++++++++++++++++++++++++++++++++------------------------------
 1 files changed, 141 insertions(+), 124 deletions(-)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-unchecked-return-status-fixes-w83781d.patch
Type: text/x-patch
Size: 14593 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060922/9f119b79/attachment.bin 

[lm-sensors] [patch] hwmon: w83781d - check return code of

[Jean Delvare]

> Add 2 attr-file groups (for base and model-specific attrs respectively),
> create the base group with single call to sysfs_create_group, and
> check the return code on individual calls to device_create_file for each
> of the model-specific attr-files.
> 
> Signed-off-by:  Jim Cromie <jim.cromie at gmail.com>

Applied with some additions (you forgot to delete some macros and to
replace some occurrences of &client->dev by dev).

Thanks,
-- 
Jean Delvare