[lm-sensors] [PATCH] Add MAX6650 support

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

This patch adds support for the MAX6650 fan controller. I'm not the author of 
this module, just tested it on an i386 embedded system and cleaned up a bit.
The patch is against 2.6.20-rc3.

Signed-off-by: Hans J. Koch <hjk at linutronix.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 19558 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070117/14ab4371/attachment.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

On Wed, 17 Jan 2007 13:59:46 +0100, Hans-J?rgen Koch wrote:
> This patch adds support for the MAX6650 fan controller. I'm not the author of 
> this module, just tested it on an i386 embedded system and cleaned up a bit.
> The patch is against 2.6.20-rc3.
> 
> Signed-off-by: Hans J. Koch <hjk at linutronix.de>

Anybody up for a driver review? It's a simple I2C device, the review
shouldn't be too hard. Things to pay attention too:
* Respect of the standard sysfs interface specifications
* Driver must be safe to use on any system
* Coding style

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[corentin.labbe]

Hello

i saw some problems in your code:

On line 141 you excess the 80 characters width
+ static ssize_t get_fan(struct device *dev, struct device_attribute *devattr, char *buf, int nr)


On the comment you have written "MAX6650 also" instead of "MAX6551 also"
  * This module has only been tested with the MAX6650 chip. It should
  * work with the MAX6650 also, though with reduced functionality. It
  * does not distinguish max6650 and max6651 chips.


You must use dev_dbg instead of #ifdef debug


On line 535 the while(0) is useless
erase the max6650_init_client function
+static void max6650_init_client(struct i2c_client *client)
+{
+	/* Nothing to do here - assume the BIOS has initialized the chip */
+	while(0)
+	{
+		;
+	}
+}


Don't use down() up()
replace with mutex

On line 553
	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
	MAX6650_REG_TACH2, MAX6650_REG_TACH3
	one per line
	"," at the end
like
static const u8 tach_reg[] {
	MAX6650_REG_TACH0,
	MAX6650_REG_TACH1,
	MAX6650_REG_TACH2,
	MAX6650_REG_TACH3,
};

	
Why use max6650_read_value and max6650_write_value?
replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data directly

Check error return code of i2c_detach_client(client);
like
if ((err = i2c_detach_client(client)))
	return err;


Don't use many device_create_file
use sysfs_create_group

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:
> Hello
>
> i saw some problems in your code:
>
> On line 141 you excess the 80 characters width
> + static ssize_t get_fan(struct device *dev, struct
> device_attribute *devattr, char *buf, int nr)
>
>
> On the comment you have written "MAX6650 also" instead of "MAX6551
> also" * This module has only been tested with the MAX6650 chip. It
> should * work with the MAX6650 also, though with reduced
> functionality. It * does not distinguish max6650 and max6651 chips.
>
>
> You must use dev_dbg instead of #ifdef debug
>
>
> On line 535 the while(0) is useless
> erase the max6650_init_client function
> +static void max6650_init_client(struct i2c_client *client)
> +{
> +	/* Nothing to do here - assume the BIOS has initialized the chip
> */ +	while(0)
> +	{
> +		;
> +	}
> +}
>
>
> Don't use down() up()
> replace with mutex
>
> On line 553
> 	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
> 	MAX6650_REG_TACH2, MAX6650_REG_TACH3
> 	one per line
> 	"," at the end
> like
> static const u8 tach_reg[] > {
> 	MAX6650_REG_TACH0,
> 	MAX6650_REG_TACH1,
> 	MAX6650_REG_TACH2,
> 	MAX6650_REG_TACH3,
> };
>
>
> Why use max6650_read_value and max6650_write_value?
> replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data
> directly
>
> Check error return code of i2c_detach_client(client);
> like
> if ((err = i2c_detach_client(client)))
> 	return err;
>
>
> Don't use many device_create_file
> use sysfs_create_group

Thanks for your review, I'll look at the code again and post an 
updated patch soon.

Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Sonntag, 11. Februar 2007 17:22 schrieb Hans-J?rgen Koch:
> Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:
> > Hello
> >
> > i saw some problems in your code:
> >
> > On line 141 you excess the 80 characters width
> > + static ssize_t get_fan(struct device *dev, struct
> > device_attribute *devattr, char *buf, int nr)
> >
> >
> > On the comment you have written "MAX6650 also" instead of "MAX6551
> > also" * This module has only been tested with the MAX6650 chip. It
> > should * work with the MAX6650 also, though with reduced
> > functionality. It * does not distinguish max6650 and max6651 chips.
> >
> >
> > You must use dev_dbg instead of #ifdef debug
> >
> >
> > On line 535 the while(0) is useless
> > erase the max6650_init_client function
> > +static void max6650_init_client(struct i2c_client *client)
> > +{
> > +	/* Nothing to do here - assume the BIOS has initialized the chip
> > */ +	while(0)
> > +	{
> > +		;
> > +	}
> > +}
> >
> >
> > Don't use down() up()
> > replace with mutex
> >
> > On line 553
> > 	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
> > 	MAX6650_REG_TACH2, MAX6650_REG_TACH3
> > 	one per line
> > 	"," at the end
> > like
> > static const u8 tach_reg[] > > {
> > 	MAX6650_REG_TACH0,
> > 	MAX6650_REG_TACH1,
> > 	MAX6650_REG_TACH2,
> > 	MAX6650_REG_TACH3,
> > };
> >
> >
> > Why use max6650_read_value and max6650_write_value?
> > replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data
> > directly
> >
> > Check error return code of i2c_detach_client(client);
> > like
> > if ((err = i2c_detach_client(client)))
> > 	return err;
> >
> >
> > Don't use many device_create_file
> > use sysfs_create_group
>
> Thanks for your review, I'll look at the code again and post an
> updated patch soon.
>

Hi,
here's the promised patch with these issues hopefully solved. I tested the 
module on a Kontron CPX board, with a vanilla 2.6.20 kernel, it seems to work 
fine.

Thanks again for the review,

Hans
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 18702 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070212/7ca36436/attachment.bin 

[lm-sensors] Re: [PATCH] Add MAX6650 support

[Matej Kenda]

Hi Hans,

I have tried the driver with on kernel 2.6.20.1 for platform ARM (processor 
PXA255) to get fan speeds from the chip max6651.

It seems that the driver works, because it exports the values in /sys file 
system.

I have few small comments on the implementation:

* use mutex instead of semaphore
* define i2c driver ID in the include/i2c-id.h

However I have noticed the following issue, which might be related to the 
driver of libsensors. Whenever I issue the command sensors to display values 
of the fans, I get an error:

$ sensors
max1617a-i2c-0-18
Adapter: pxa2xx-i2c
Main PCB:    +40 C  (low  =    +0 C, high =   +50 C)

max1617a-i2c-0-29
Adapter: pxa2xx-i2c
Main PCB:    +38 C  (low  =    +0 C, high =   +50 C)

max6650-i2c-0-48
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

max6650-i2c-0-4b
Adapter: pxa2xx-i2c
ERROR: Can't get FAN1 data!
ERROR: Can't get FAN2 data!
ERROR: Can't get FAN3 data!
ERROR: Can't get FAN4 data!

$ sensors --version
sensors version 2.10.2 with libsensors version 2.10.2


I tried to rename entries that the driver creates from fan0-fan3 to fan1-fan4 
as used by the libsensors and sensors, but that didn't help.

Did you have to modify the (lib)sensors in any way to display the values?

Matej

-- 
Matej Kenda, Senior Software Engineer
HERMES SoftLab d.d. (www.hermes-softlab.com), Slovenia

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

On Tue, 27 Feb 2007 16:21:17 +0100, Matej Kenda wrote:
> Hi Hans,
> 
> I have tried the driver with on kernel 2.6.20.1 for platform ARM (processor 
> PXA255) to get fan speeds from the chip max6651.
> 
> It seems that the driver works, because it exports the values in /sys file 
> system.
> 
> I have few small comments on the implementation:
> 
> * use mutex instead of semaphore

This was fixed in the second version of the driver (posted by
Hans-J?rgen on February 12th.)

> * define i2c driver ID in the include/i2c-id.h

Even better, don't set an ID if you don't need it.

> However I have noticed the following issue, which might be related to the 
> driver of libsensors. Whenever I issue the command sensors to display values 
> of the fans, I get an error:
> 
> $ sensors
> max1617a-i2c-0-18
> Adapter: pxa2xx-i2c
> Main PCB:    +40 C  (low  =    +0 C, high =   +50 C)
> 
> max1617a-i2c-0-29
> Adapter: pxa2xx-i2c
> Main PCB:    +38 C  (low  =    +0 C, high =   +50 C)
> 
> max6650-i2c-0-48
> Adapter: pxa2xx-i2c
> ERROR: Can't get FAN1 data!
> ERROR: Can't get FAN2 data!
> ERROR: Can't get FAN3 data!
> ERROR: Can't get FAN4 data!
> 
> max6650-i2c-0-4b
> Adapter: pxa2xx-i2c
> ERROR: Can't get FAN1 data!
> ERROR: Can't get FAN2 data!
> ERROR: Can't get FAN3 data!
> ERROR: Can't get FAN4 data!
> 
> $ sensors --version
> sensors version 2.10.2 with libsensors version 2.10.2
> 
> I tried to rename entries that the driver creates from fan0-fan3 to fan1-fan4 
> as used by the libsensors and sensors, but that didn't help.

The files should actually be named fan1_input to fan4_input (for
measured values.)

> Did you have to modify the (lib)sensors in any way to display the values?

You shouldn't need to.

-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Dienstag, 27. Februar 2007 16:35 schrieb Jean Delvare:
> On Tue, 27 Feb 2007 16:21:17 +0100, Matej Kenda wrote:
> > Hi Hans,
> >
> > I have tried the driver with on kernel 2.6.20.1 for platform ARM
> > (processor PXA255) to get fan speeds from the chip max6651.
> >
> > It seems that the driver works, because it exports the values in /sys
> > file system.
> >
> > I have few small comments on the implementation:
> >
> > * use mutex instead of semaphore
>
> This was fixed in the second version of the driver (posted by
> Hans-J?rgen on February 12th.)
>
> > * define i2c driver ID in the include/i2c-id.h
>
> Even better, don't set an ID if you don't need it.

All right, I threw that out. Actually, I didn't know that you can simply omit 
that .id 

>
> > However I have noticed the following issue, which might be related to the
> > driver of libsensors. Whenever I issue the command sensors to display
> > values of the fans, I get an error:
> >
> > $ sensors
> > max1617a-i2c-0-18
> > Adapter: pxa2xx-i2c
> > Main PCB:    +40 C  (low  =    +0 C, high =   +50 C)
> >
> > max1617a-i2c-0-29
> > Adapter: pxa2xx-i2c
> > Main PCB:    +38 C  (low  =    +0 C, high =   +50 C)
> >
> > max6650-i2c-0-48
> > Adapter: pxa2xx-i2c
> > ERROR: Can't get FAN1 data!
> > ERROR: Can't get FAN2 data!
> > ERROR: Can't get FAN3 data!
> > ERROR: Can't get FAN4 data!
> >
> > max6650-i2c-0-4b
> > Adapter: pxa2xx-i2c
> > ERROR: Can't get FAN1 data!
> > ERROR: Can't get FAN2 data!
> > ERROR: Can't get FAN3 data!
> > ERROR: Can't get FAN4 data!
> >
> > $ sensors --version
> > sensors version 2.10.2 with libsensors version 2.10.2
> >
> > I tried to rename entries that the driver creates from fan0-fan3 to
> > fan1-fan4 as used by the libsensors and sensors, but that didn't help.
>
> The files should actually be named fan1_input to fan4_input (for
> measured values.)

I renamed the files accordingly.

>
> > Did you have to modify the (lib)sensors in any way to display the values?
>
> You shouldn't need to.

I attached the current version of the patch, please tell us if this works for 
you.

Thanks,
Hans

-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 18673 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070227/0ef1fcd6/attachment.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Matej Kenda]

2007/2/27, Jean Delvare <khali at linux-fr.org>:
> > * define i2c driver ID in the include/i2c-id.h
>
> Even better, don't set an ID if you don't need it.

Most of the hwmon drivers set the id. Were they set by inertia or do
some of the drivers actually need it?

> The files should actually be named fan1_input to fan4_input (for
> measured values.)

I've modified the names like this:

static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan1, NULL);
static DEVICE_ATTR(fan2_input, S_IRUGO, get_fan2, NULL);
static DEVICE_ATTR(fan3_input, S_IRUGO, get_fan3, NULL);
static DEVICE_ATTR(fan4_input, S_IRUGO, get_fan4, NULL);
static DEVICE_ATTR(count, S_IRUGO, get_count, NULL);
static DEVICE_ATTR(config, S_IRUGO, get_config, NULL);
static DEVICE_ATTR(speed, S_IWUSR | S_IRUGO, get_speed, set_speed);

Other driver usually have _min and _div files.

Do the count, speed and config also have to be modified to follow a convention?

Matej

-- 
Matej Kenda, Senior Software Engineer
HERMES SoftLab d.d. (www.hermes-softlab.com), Slovenia

[lm-sensors] [PATCH] Add MAX6650 support

[Matej Kenda]

2007/2/27, Hans-J?rgen Koch <hjk at linutronix.de>:
> > > I tried to rename entries that the driver creates from fan0-fan3 to
> > > fan1-fan4 as used by the libsensors and sensors, but that didn't help.
> >
> > The files should actually be named fan1_input to fan4_input (for
> > measured values.)
>
> I renamed the files accordingly.
>
> I attached the current version of the patch, please tell us if this works for
> you.
>

I have just tried your patch with kernel 2.6.20.1. Now I get values
with sensors:

$ sensors
max6650-i2c-0-48
Adapter: pxa2xx-i2c
fan4:       30 RPM

max6650-i2c-0-4b
Adapter: pxa2xx-i2c
fan4:       30 RPM

However, the values seem very low to me. With kernel 2.4 I got values
in the range around 4000 RPM. I'll check sensors.conf as well.

Matej

-- 
Matej Kenda, Senior Software Engineer
HERMES SoftLab d.d. (www.hermes-softlab.com), Slovenia

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Dienstag, 27. Februar 2007 18:03 schrieb Matej Kenda:

>
> I have just tried your patch with kernel 2.6.20.1. Now I get values
> with sensors:
>
> $ sensors
> max6650-i2c-0-48
> Adapter: pxa2xx-i2c
> fan4:       30 RPM
>
> max6650-i2c-0-4b
> Adapter: pxa2xx-i2c
> fan4:       30 RPM
>
> However, the values seem very low to me. With kernel 2.4 I got values
> in the range around 4000 RPM. I'll check sensors.conf as well.

30 is the value I get with no fan connected. I hope there's no bug in 
max6650_update_device() that makes it always return 30. I'll check.

Bene, could you check on your board?

Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

On Tue, 27 Feb 2007 17:29:59 +0100, Matej Kenda wrote:
> 2007/2/27, Jean Delvare <khali at linux-fr.org>:
> > > * define i2c driver ID in the include/i2c-id.h
> >
> > Even better, don't set an ID if you don't need it.
> 
> Most of the hwmon drivers set the id. Were they set by inertia or do
> some of the drivers actually need it?

Inertia, mostly.

> > The files should actually be named fan1_input to fan4_input (for
> > measured values.)
> 
> I've modified the names like this:
> 
> static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan1, NULL);
> static DEVICE_ATTR(fan2_input, S_IRUGO, get_fan2, NULL);
> static DEVICE_ATTR(fan3_input, S_IRUGO, get_fan3, NULL);
> static DEVICE_ATTR(fan4_input, S_IRUGO, get_fan4, NULL);
> static DEVICE_ATTR(count, S_IRUGO, get_count, NULL);
> static DEVICE_ATTR(config, S_IRUGO, get_config, NULL);
> static DEVICE_ATTR(speed, S_IWUSR | S_IRUGO, get_speed, set_speed);
> 
> Other driver usually have _min and _div files.

It only makes sense if the chip actually supports it!

> Do the count, speed and config also have to be modified to follow a convention?

The count and config files are non-standard files so they don't have to
follow any specific convention. In fact I would even remove these
files, in particular the config file infringes the fundamental sysfs
rule. And the count file isn't particularly useful.

The speed file, what is it doing? If it is used to set the desired fan
speed, then the right name would be fan1_target. Does it only apply to
one fan or all? Is it always active?

-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Dienstag, 27. Februar 2007 19:07 schrieb Jean Delvare:

>
> > Do the count, speed and config also have to be modified to follow a
> > convention?
>
> The count and config files are non-standard files so they don't have to
> follow any specific convention. In fact I would even remove these
> files, in particular the config file infringes the fundamental sysfs
> rule. And the count file isn't particularly useful.

Well, the current implementation assumes that the MAX6650 is initialized by 
the BIOS. On the other hand, the 5V/12V setting is done by the driver, the 
value is hardcoded in the source. This doesn't look like a clean solution.

I suggest to add module parameters to allow configuration of 5V/12V, 
prescaler, count and mode. Then we _know_ the settings and can drop the 
config and the count file. We also get rid of the hardcoded 5V/12V setting 
and don't need to rely on a BIOS that might or might not do what we want.

Any objections?

>
> The speed file, what is it doing? If it is used to set the desired fan
> speed, then the right name would be fan1_target. Does it only apply to
> one fan or all? Is it always active?

It sets the speed of the single fan for a MAX6650. For a MAX6651, it sets a 
speed that is valid for all connected fans, if I understand the data sheet 
correctly. In any case, this works only if mode is set to "closed loop".

Thanks,
Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Dienstag, 27. Februar 2007 20:58 schrieb Hans-J?rgen Koch:
> Am Dienstag, 27. Februar 2007 19:07 schrieb Jean Delvare:
> > > Do the count, speed and config also have to be modified to follow a
> > > convention?
> >
> > The count and config files are non-standard files so they don't have to
> > follow any specific convention. In fact I would even remove these
> > files, in particular the config file infringes the fundamental sysfs
> > rule. And the count file isn't particularly useful.
>
> Well, the current implementation assumes that the MAX6650 is initialized by
> the BIOS. On the other hand, the 5V/12V setting is done by the driver, the
> value is hardcoded in the source. This doesn't look like a clean solution.
>
> I suggest to add module parameters to allow configuration of 5V/12V,
> prescaler, count and mode. Then we _know_ the settings and can drop the
> config and the count file. We also get rid of the hardcoded 5V/12V setting
> and don't need to rely on a BIOS that might or might not do what we want.
>

I added these changes. If you modprobe max6650 without any parameters, the 
driver makes no changes to the chip's configuration. With one or more 
parameter, you can change whatever you like. I think this is a good solution 
because it doesn't assume there's a BIOS that does what you want.

Please test, review, comment...

Note: Documentation is not updatet yet.

Thanks,
Hans
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 19199 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070228/a276eee1/attachment.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Matej Kenda]

2007/2/28, Hans-J?rgen Koch <hjk at linutronix.de>:
> I added these changes. If you modprobe max6650 without any parameters, the
> driver makes no changes to the chip's configuration. With one or more
> parameter, you can change whatever you like. I think this is a good solution
> because it doesn't assume there's a BIOS that does what you want.
>
> Please test, review, comment...

Well done, Hans!

This version of the driver works well on my ARM-based system.

Regards,

Matej

-- 
Matej Kenda, Senior Software Engineer
HERMES SoftLab d.d. (www.hermes-softlab.com), Slovenia

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Mittwoch, 28. Februar 2007 16:57 schrieb Matej Kenda:
> 2007/2/28, Hans-J?rgen Koch <hjk at linutronix.de>:
> > I added these changes. If you modprobe max6650 without any parameters,
> > the driver makes no changes to the chip's configuration. With one or more
> > parameter, you can change whatever you like. I think this is a good
> > solution because it doesn't assume there's a BIOS that does what you
> > want.
> >
> > Please test, review, comment...
>
> Well done, Hans!
>
> This version of the driver works well on my ARM-based system.
>

Thanks a lot for testing, Matej! Jean, what do you say to that? If you think 
you can apply this I'll update the documentation file.

Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Tue, 27 Feb 2007 20:58:26 +0100, Hans-J?rgen Koch wrote:
> Well, the current implementation assumes that the MAX6650 is initialized by 
> the BIOS. On the other hand, the 5V/12V setting is done by the driver, the 
> value is hardcoded in the source. This doesn't look like a clean solution.

Indeed it wasn't acceptable. Hardware monitoring drivers should, in
general, leave the chip configuration untouched by default.

> I suggest to add module parameters to allow configuration of 5V/12V, 
> prescaler, count and mode. Then we _know_ the settings and can drop the 
> config and the count file. We also get rid of the hardcoded 5V/12V setting 
> and don't need to rely on a BIOS that might or might not do what we want.
> 
> Any objections?

I agree with the general direction. Details may need to be discussed
though.

> > The speed file, what is it doing? If it is used to set the desired fan
> > speed, then the right name would be fan1_target. Does it only apply to
> > one fan or all? Is it always active?
> 
> It sets the speed of the single fan for a MAX6650. For a MAX6651, it sets a 
> speed that is valid for all connected fans, if I understand the data sheet 
> correctly. In any case, this works only if mode is set to "closed loop".

This is what I don't understand, in the case of the MAX6651. I don't
quite see how you can implement a closed loop with 4 possibly different
inputs and only one output.

-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Wed, 28 Feb 2007 17:08:16 +0100, Hans-J?rgen Koch wrote:
> Thanks a lot for testing, Matej! Jean, what do you say to that? If you think 
> you can apply this I'll update the documentation file.

Thanks for all the work you've been doing on the driver lately. I am
waiting for Corentin Labbe to comment before I do the final review and
merge the driver - I know he found some more things that could be fixed
or improved since his first review, some of which I think you addressed
yesterday, some not.

I look briefly at the new options you implemented. They are definitely
better as module options than hardcoded in the driver code, however I'm
not sure we really want all of them to be module options.

General remark: the module parameters are driver-wide and not
device-specific, so they must me checked at module load time (in
sensors_max6650_init). Also, for all settings that are not forced by
the user, it might be convenient to print in the logs the value that
was found for each chip.

voltage_12V: Must indeed be an option, although the convention you
chose is a bit strange IMHO. What about a parameter named "fan_voltage"
with possible values 0 (default, no change), 5 and 12?

(BTW, is this an output voltage? input voltage? both?)

prescaler: What does it do? It smells like a fan clock divider to me.
In that case it would be better implemented as a fan1_div sysfs file,
which the user can adjust at runtime.

clock: OK.

mode: This smells like fan speed control parameters, which we typically
implement as the pwm1_enable sysfs file. Please take a look at the
description in Documentation/hwmon/sysfs-interface and tell me what
you think.

counttime: This one too seems to be related to a fan clock divider? How
does it functionally differ from prescaler?

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Donnerstag 01 M?rz 2007 08:34 schrieb Jean Delvare:
> Hi Hans-J?rgen,
>
> On Wed, 28 Feb 2007 17:08:16 +0100, Hans-J?rgen Koch wrote:
> > Thanks a lot for testing, Matej! Jean, what do you say to that?
> > If you think you can apply this I'll update the documentation
> > file.
>
> Thanks for all the work you've been doing on the driver lately. I
> am waiting for Corentin Labbe to comment before I do the final
> review and merge the driver - I know he found some more things that
> could be fixed or improved since his first review, some of which I
> think you addressed yesterday, some not.

All right, I'm looking forward to his review.

>
> I look briefly at the new options you implemented. They are
> definitely better as module options than hardcoded in the driver
> code, however I'm not sure we really want all of them to be module
> options.

The problem is that all these parameters have to fit together and 
match the fan used. If you have a BIOS that configures the chip for 
one special fan, and you have to replace that fan with a different 
type, you have to calculate new values for prescaler and counttime. 
Or, you might notice your board becomes too hot in a certain 
application and decide to change the mode from "closed loop" to "full 
on". All these things happen frequently in industrial applications.

Having all the values as module parameters doesn't harm as you can 
safely ignore them if the BIOS values work for you. But to have them 
as sysfs files seems to be an advantage.

>
> General remark: the module parameters are driver-wide and not
> device-specific, so they must me checked at module load time (in
> sensors_max6650_init). 

I'll have a look at that.

> Also, for all settings that are not forced 
> by the user, it might be convenient to print in the logs the value
> that was found for each chip.

Agreed, that would be nice to have. I'll add that.

>
> voltage_12V: Must indeed be an option, although the convention you
> chose is a bit strange IMHO. What about a parameter named
> "fan_voltage" with possible values 0 (default, no change), 5 and
> 12?

OK, I agree, that looks better.

>
> (BTW, is this an output voltage? input voltage? both?)

Neither nor. It's simply an information for the chip. It measures the 
voltage at the low side of the fan. The parameter tells it whether 
there are 5V or 12V at the high side. If it measures e.g. 5V at the 
low side, a 5V fan is standing still while a 12V fan still has 7V 
across it and is running at almost half speed. 
If you accidentally set 12V where you really have a 5V fan, this 
doesn't damage the fan. The fan will probably always run with full 
speed, because the chip desperately tries to bring the voltage up.

>
> prescaler: What does it do? It smells like a fan clock divider to
> me. In that case it would be better implemented as a fan1_div sysfs
> file, which the user can adjust at runtime.

The chip generates a reference frequency which is determined by the 
speed register, the prescaler, and the internal clock (nominal value 
254kHz). The regulator then tries to make the tacho frequency equal 
to this reference frequency. 
As clock is fixed and speed has a well defined meaning, the prescaler 
is the only way to adapt to different tacho generators. The data 
sheet gives hints about how to select a prescaler value to achieve 
optimal range and resolution. If you get that wrong, the fan might 
not reach it's full speed, switch between two speed values only or 
similar effects.

I read Documentation/hwmon/sysfs-interface, but I have to admit that I 
don't really understand if fan[1-*]_div has this meaning. If it has, 
we could possibly make prescaler such a sysfs file. 

>
> clock: OK.

This is the only one where I think it could be omitted. The internal 
254kHz oscillator has a tolerance of +/-10%, so fan speed can also be 
off +/-10%. This parameter is just there to compensate for that. I 
doubt it will ever be used in practical applications.

>
> mode: This smells like fan speed control parameters, which we
> typically implement as the pwm1_enable sysfs file. Please take a
> look at the description in Documentation/hwmon/sysfs-interface and
> tell me what you think.

That looks good. We can implement pwm1_enable that way. Problem is 
that we can have up to four fan speeds (fan[1-4]_input), but the 
control values are there only once (pwm1, pwm1_enable). Is that 
possible?

>
> counttime: This one too seems to be related to a fan clock divider?
> How does it functionally differ from prescaler?

counttime is the time interval during which tacho pulses are counted.
It determines the resolution of the measured speed values. It should 
be set to the longest time that still ensures the 8-bit counter 
doesn't overflow under worst case conditions. 

Thanks,
Hans

[lm-sensors] [PATCH] Add MAX6650 support

[corentin.labbe]

Hans-J?rgen Koch a ?crit :
> Am Sonntag, 11. Februar 2007 17:22 schrieb Hans-J?rgen Koch:
>> Am Sonntag 11 Februar 2007 16:44 schrieb corentin.labbe:
>>> Hello
>>>
>>> i saw some problems in your code:
>>>
>>> On line 141 you excess the 80 characters width
>>> + static ssize_t get_fan(struct device *dev, struct
>>> device_attribute *devattr, char *buf, int nr)
>>>
>>>
>>> On the comment you have written "MAX6650 also" instead of "MAX6551
>>> also" * This module has only been tested with the MAX6650 chip. It
>>> should * work with the MAX6650 also, though with reduced
>>> functionality. It * does not distinguish max6650 and max6651 chips.
>>>
>>>
>>> You must use dev_dbg instead of #ifdef debug
>>>
>>>
>>> On line 535 the while(0) is useless
>>> erase the max6650_init_client function
>>> +static void max6650_init_client(struct i2c_client *client)
>>> +{
>>> +	/* Nothing to do here - assume the BIOS has initialized the chip
>>> */ +	while(0)
>>> +	{
>>> +		;
>>> +	}
>>> +}
>>>
>>>
>>> Don't use down() up()
>>> replace with mutex
>>>
>>> On line 553
>>> 	MAX6650_REG_TACH0, MAX6650_REG_TACH1,
>>> 	MAX6650_REG_TACH2, MAX6650_REG_TACH3
>>> 	one per line
>>> 	"," at the end
>>> like
>>> static const u8 tach_reg[] >>> {
>>> 	MAX6650_REG_TACH0,
>>> 	MAX6650_REG_TACH1,
>>> 	MAX6650_REG_TACH2,
>>> 	MAX6650_REG_TACH3,
>>> };
>>>
>>>
>>> Why use max6650_read_value and max6650_write_value?
>>> replace with i2c_smbus_write_byte_data and i2c_smbus_read_byte_data
>>> directly
>>>
>>> Check error return code of i2c_detach_client(client);
>>> like
>>> if ((err = i2c_detach_client(client)))
>>> 	return err;
>>>
>>>
>>> Don't use many device_create_file
>>> use sysfs_create_group
>> Thanks for your review, I'll look at the code again and post an
>> updated patch soon.
>>
> 
> Hi,
> here's the promised patch with these issues hopefully solved. I tested the 
> module on a Kontron CPX board, with a vanilla 2.6.20 kernel, it seems to work 
> fine.
> 
> Thanks again for the review,
> 
> Hans
>  
> 
> 
> ------------------------------------------------------------------------
> 
> Index: linux-2.6.20/Documentation/hwmon/max6650
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/Documentation/hwmon/max6650	2007-02-12 14:40:26.000000000 +0100
> @@ -0,0 +1,33 @@
> +Kernel driver max6650
> +==========> +
> +Supported chips:
> +  * Maxim 6650 / 6651
> +    Prefix: 'w83792d'
> +    Addresses scanned: I2C 0x1b, 0x1f, 0x48, 0x4b
> +    Datasheet: http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
> +
> +Authors:
> +    John Morris <john.morris at spirentcom.com>
> +    Claus Gindhart <claus.gindhart at kontron.com>
> +    Hans J. Koch <hjk at linutronix.de>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Maxim 6650/6651
> +
> +The 2 devices are very similar, bit the Maxim 6550 has a reduced feature
> +set, e.g. only one fan-input, instead of 4 for the 6651.
> +
> +The driver is not able to distinguish between the 2 devices.
> +
> +The driver provides the following sensor accesses
> +
> +fan0   ro     fan tachometer speed in RPM
> +fan1   ro     "
> +fan2   ro     "
> +fan3   ro     "
> +config ro     contents of the config register (for debugging)
> +count  ro     tachometer count time in milliseconds
> +speed  rw     fan speed adjustment value
> Index: linux-2.6.20/drivers/hwmon/Kconfig
> =================================> --- linux-2.6.20.orig/drivers/hwmon/Kconfig	2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.20/drivers/hwmon/Kconfig	2007-02-12 09:01:25.000000000 +0100
> @@ -364,6 +364,16 @@
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called max1619.
>  
> +config SENSORS_MAX6650
> +	tristate "Maxim MAX6650 sensor chip"
> +	depends on HWMON && I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the MAX6650 / MAX6651
> +	  sensor chips.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max6650.
> +
>  config SENSORS_PC87360
>  	tristate "National Semiconductor PC87360 family"
>  	depends on HWMON && I2C && EXPERIMENTAL
> Index: linux-2.6.20/drivers/hwmon/Makefile
> =================================> --- linux-2.6.20.orig/drivers/hwmon/Makefile	2007-02-04 19:44:54.000000000 +0100
> +++ linux-2.6.20/drivers/hwmon/Makefile	2007-02-12 09:01:25.000000000 +0100
> @@ -42,6 +42,7 @@
>  obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
>  obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
>  obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
> +obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_SIS5595)	+= sis5595.o
> Index: linux-2.6.20/drivers/hwmon/max6650.c
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20/drivers/hwmon/max6650.c	2007-02-12 14:34:50.000000000 +0100
> @@ -0,0 +1,558 @@
> +/*
> + * max6650.c - Part of lm_sensors, Linux kernel modules for hardware
> + *             monitoring.
> + *
> + * Author: John Morris <john.morris at spirentcom.com>
> + * Copyright (c) 2003 Spirent Communications
> + *
> + * This module has only been tested with the MAX6650 chip. It should
> + * also work with the MAX6651, though with reduced functionality. It
> + * does not distinguish max6650 and max6651 chips.
> + *
> + * Tha datasheet was last seen at:
> + *
> + *        http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
> + *
> + * Ported to Linux 2.6 by Claus Gindhart <claus.gindhart at kontron.com>
> + * Some cleanup done by Hans J. Koch <hjk at linutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +
> +#ifndef I2C_DRIVERID_MAX6650
> +#define I2C_DRIVERID_MAX6650	1044
> +#endif
> +
> +/* HW-related: Set this to 1 for 12V and 0 for 5V configuration */
> +static const int voltage_12V=0;
> +
> +/*
> + * Addresses to scan. There are four disjoint possibilities, by pin config.
> + */
> +
> +static unsigned short normal_i2c[] = {0x1b, 0x1f, 0x48, 0x4b, I2C_CLIENT_END};
> +
> +/*
> + * Insmod parameters
> + */
> +
> +I2C_CLIENT_INSMOD_1(max6650);
> +
> +/*
> + * MAX 6650/6651 registers
> + */
> +
> +#define MAX6650_REG_SPEED       0x00
> +#define MAX6650_REG_CONFIG      0x02
> +#define MAX6650_REG_GPIO_DEF    0x04
> +#define MAX6650_REG_DAC         0x06
> +#define MAX6650_REG_ALARM_EN    0x08
> +#define MAX6650_REG_ALARM       0x0A
> +#define MAX6650_REG_TACH0       0x0C
> +#define MAX6650_REG_TACH1       0x0E
> +#define MAX6650_REG_TACH2       0x10
> +#define MAX6650_REG_TACH3       0x12
> +#define MAX6650_REG_GPIO_STAT   0x14
> +#define MAX6650_REG_COUNT       0x16
> +
> +
> +/*
> + * Config register bits
> + */
> +
> +#define MAX6650_CFG_V12           	0x08
> +#define MAX6650_CFG_MODE_MASK           0x30
> +#define MAX6650_CFG_MODE_ON             0x00
> +#define MAX6650_CFG_MODE_OFF            0x10
> +#define MAX6650_CFG_MODE_CLOSED_LOOP    0x20
> +#define MAX6650_CFG_MODE_OPEN_LOOP      0x30
> +
> +#define MAX6650_INT_CLK 254000  /* Default clock speed - 254 kHz */
> +
> +/* Minimum and maximum values of the FAN-RPM */
> +#define FAN_RPM_MIN 240
> +#define FAN_RPM_MAX 30000
> +
> +#define DIV_FROM_REG(reg) (1 << (reg & 7))
> +
> +static int max6650_attach_adapter(struct i2c_adapter *adapter);
> +static int max6650_detect(struct i2c_adapter *adapter, int address, int kind);
> +static void max6650_init_client(struct i2c_client *client);
> +static int max6650_detach_client(struct i2c_client *client);
> +static struct max6650_data *max6650_update_device(struct device *dev);
> +
> +/*
> + * Driver data (common to all clients)
> + */
> +
> +static struct i2c_driver max6650_driver = {
> +	.driver = {
> +		.name	= "max6650",
> +	},
> +	.id             = I2C_DRIVERID_MAX6650,
> +	.attach_adapter = max6650_attach_adapter,
> +	.detach_client  = max6650_detach_client
> +};
> +
> +/*
> + * Client data (each client gets its own)
> + */
> +
> +struct max6650_data
> +{
> +	struct i2c_client client;
> +	struct class_device *class_dev;
> +	struct mutex update_lock;
> +	char valid; /* zero until following fields are valid */
> +	unsigned long last_updated; /* in jiffies */
> +
> +	/* register values */
> +	u8 speed;
> +	u8 config;
> +	u8 tach[4];
> +	u8 count;
> +};
> +
> +static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
> +		       char *buf, int nr)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +
> +	/*
> +	* Calculation details:
> +	*
> +	* Each tachometer counts over an interval given by the "count"
> +	* register (0.25, 0.5, 1 or 2 seconds). This module assumes
> +	* that the fans produce two pulses per revolution (this seems
> +	* to be the most common).
> +	*/
> +
> +	int rpm = ((data->tach[nr] * 120) / DIV_FROM_REG(data->count));
> +	return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t get_fan0(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,0);
> +}
> +
> +static ssize_t get_fan1(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,1);
> +}
> +
> +static ssize_t get_fan2(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,2);
> +}
> +
> +static ssize_t get_fan3(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,3);
> +}
> +
> +/* Show values of the config register for debugging purposes
> + */
> +
> +static ssize_t get_config(struct device *dev, struct device_attribute *devattr,
> +			  char *buf)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +
> +	return sprintf(buf, "mode: %s%s%s%s, voltage:%s prescale: %d\n",
> +		((data->config & MAX6650_CFG_MODE_MASK) =
> +		  MAX6650_CFG_MODE_ON) ? "On" : "",
> +		((data->config & MAX6650_CFG_MODE_MASK) =
> +		  MAX6650_CFG_MODE_OFF) ? "Off" : "",
> +		((data->config & MAX6650_CFG_MODE_MASK) =
> +		  MAX6650_CFG_MODE_CLOSED_LOOP) ? "closed loop" : "",
> +		((data->config & MAX6650_CFG_MODE_MASK) =
> +		  MAX6650_CFG_MODE_OPEN_LOOP) ? "open Loop" : "",
> +		(data->config & MAX6650_CFG_V12) ? "12V" : "5V",
> +		DIV_FROM_REG(data->config)
> +		);
> +}
> +
> +/* Returns count time in ms */
> +static ssize_t get_count(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +
> +	return sprintf(buf, "%d\n", DIV_FROM_REG(data->count) * 250 );
> +}
> +
> +/*
> + * Set the fan speed to the specified RPM (or read back the RPM setting).
> + *
> + * The MAX6650/1 will automatically control fan speed when in closed loop
> + * mode.
> + *
> + * Assumptions:
> + *
> + * 1) The MAX6650/1 is running from its internal 254kHz clock (perhaps
> + *    this should be made a module parameter).
> + *
> + * 2) The prescaler (low three bits of the config register) has already
> + *    been set to an appropriate value.
> + *
> + * The relevant equations are given on pages 21 and 22 of the datasheet.
> + *
> + * From the datasheet, the relevant equation when in regulation is:
> + *
> + *    [fCLK / (128 x (KTACH + 1))] = 2 x FanSpeed / KSCALE
> + *
> + * where:
> + *
> + *    fCLK is the oscillator frequency (either the 254kHz internal
> + *         oscillator or the externally applied clock)
> + *
> + *    KTACH is the value in the speed register
> + *
> + *    FanSpeed is the speed of the fan in rps
> + *
> + *    KSCALE is the prescaler value (1, 2, 4, 8, or 16)
> + *
> + * When reading, we need to solve for FanSpeed. When writing, we need to
> + * solve for KTACH.
> + *
> + * Note: this tachometer is completely separate from the tachometers
> + * used to measure the fan speeds. Only one fan's speed (fan1) is
> + * controlled.
> + */
> +
> +static ssize_t get_speed(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +	int kscale, ktach, fclk, rpm;
> +
> +	/*
> +	* Use the datasheet equation:
> +	*
> +	*    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> +	*
> +	* then multiply by 60 to give rpm.
> +	*/
> +
> +	kscale = DIV_FROM_REG(data->config);
> +	ktach  = data->speed;
> +	fclk   = MAX6650_INT_CLK;
> +	rpm    = 60 * kscale * fclk / (256 * (ktach + 1));
> +	return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t set_speed(struct device *dev, struct device_attribute *devattr,
> +			 const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int rpm = simple_strtoul(buf, NULL, 10);
> +	int kscale, ktach, fclk;
> +
> +	dev_dbg(dev, "%s called, rpm=%d\n", __FUNCTION__, rpm);
> +
> +	mutex_lock(&data->update_lock);
> +	if (rpm = 0)
> +	{
> +		/* Switch totally off */
> +		data->speed  = 0;
> +		data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
> +				| MAX6650_CFG_MODE_OFF;
> +	}
> +	else
> +	{
> +		rpm = SENSORS_LIMIT(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
> +
> +		/*
> +		* Divide the required speed by 60 to get from rpm to rps, then
> +		* use the datasheet equation:
> +		*
> +		*     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
> +		*/
> +
> +		kscale = DIV_FROM_REG(data->config);
> +		fclk   = MAX6650_INT_CLK;
> +		ktach  = ((fclk * kscale) / (256 * rpm / 60)) - 1;
> +
> +		data->speed  = ktach;
> +		data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
> +				| MAX6650_CFG_MODE_CLOSED_LOOP;
> +	}
> +	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
> +	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED,  data->speed);
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(fan0, S_IRUGO, get_fan0, NULL);
> +static DEVICE_ATTR(fan1, S_IRUGO, get_fan1, NULL);
> +static DEVICE_ATTR(fan2, S_IRUGO, get_fan2, NULL);
> +static DEVICE_ATTR(fan3, S_IRUGO, get_fan3, NULL);
> +static DEVICE_ATTR(count, S_IRUGO, get_count, NULL);
> +static DEVICE_ATTR(config, S_IRUGO, get_config, NULL);
> +static DEVICE_ATTR(speed, S_IWUSR | S_IRUGO, get_speed, set_speed);
> +
> +static struct attribute *max6650_attrs[] = {
> +	&dev_attr_fan0.attr,
> +	&dev_attr_fan1.attr,
> +	&dev_attr_fan2.attr,
> +	&dev_attr_fan3.attr,
> +	&dev_attr_count.attr,
> +	&dev_attr_config.attr,
> +	&dev_attr_speed.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group max6650_attr_grp = {
> +	.attrs = max6650_attrs,
> +};
> +
> +/*
> + * Real code
> + */
> +
> +static int max6650_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	dev_dbg(adapter->dev, "max6650_attach_adapter called for %s\n",
> +		(char*)&adapter->name);
> +
> +	if (!(adapter->class & I2C_CLASS_HWMON)) {
> +		dev_dbg(adapter->dev,
> +			"FATAL: max6650_attach_adapter class HWMON not set\n");
> +		return 0;
> +	}
> +
> +	return i2c_probe(adapter, &addr_data, max6650_detect);
> +}
> +
> +/*
> + * The following function does more than just detection. If detection
> + * succeeds, it also registers the new chip.
> + */
> +
> +static int max6650_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *new_client;
> +	struct max6650_data *data;
> +	int err = -ENODEV;
> +	const char *name = "";
> +
> +	dev_dbg(adapter->dev, "max6650_detect called, kind = %d\n",kind);
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_dbg(adapter->dev, "MAX6650: I2C bus doesn't support byte"
> +			"read mode, skipping.\n");
> +		return 0;
> +	}
> +
> +	if (!(data = kzalloc(sizeof(struct max6650_data), GFP_KERNEL))) {
> +		printk("MAX6650: Out of memory in max6650_detect "
> +			"(new_client).\n");
> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * The common I2C client data is placed right before the
> +	 * max6650-specific data. The max6650-specific data is pointed to by
> +	 * the data field from the I2C client data.
> +	 */
> +
> +	new_client = &data->client;
> +	i2c_set_clientdata(new_client, data);
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &max6650_driver;
> +	new_client->flags = 0;
> +
> +	/*
> +	 * Now we do the remaining detection. A negative kind means that
> +	 * the driver was loaded with no force parameter (default), so we
> +	 * must both detect and identify the chip (actually there is only
> +	 * one possible kind of chip for now, max6650). A zero kind means that
> +	 * the driver was loaded with the force parameter, the detection
> +	 * step shall be skipped. A positive kind means that the driver
> +	 * was loaded with the force parameter and a given kind of chip is
> +	 * requested, so both the detection and the identification steps
> +	 * are skipped.
> +	 *
> +	 * Currently I can find no way to distinguish between a MAX6650 and
> +	 * a MAX6651. This driver has only been tried on the latter.
> +	 */
> +
> +	if ((kind < 0)&&
> +	   (  (i2c_smbus_read_byte_data(new_client,MAX6650_REG_CONFIG) & 0xC0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_GPIO_STAT)& 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM_EN) & 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM) & 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_COUNT) & 0xFC)))
> +	{
> +		dev_dbg(adapter->dev, "max6650.o: max6650 detection failed"
> +			"at 0x%02x.\n", address);
> +
> +		goto err_free;
> +        }
> +
> +	/* Configure HW-related voltage setting */
> +	if (voltage_12V) {
> +		i2c_smbus_write_byte_data(new_client, MAX6650_REG_CONFIG,
> +			i2c_smbus_read_byte_data(new_client, MAX6650_REG_CONFIG)
> +			| MAX6650_CFG_V12);
> +	}
> +	else {
> +		i2c_smbus_write_byte_data(new_client, MAX6650_REG_CONFIG,
> +			i2c_smbus_read_byte_data(new_client, MAX6650_REG_CONFIG)
> +			& ~MAX6650_CFG_V12);
> +	}
> +
> +	if (kind <= 0) {
> +		kind = max6650;
> +	}
> +
> +	if (kind = max6650) {
> +		name = "max6650";
> +		printk("MAX6650: Chip found at 0x%02x.\n", address);
> +	}
> +	else {
> +		printk("MAX6650: Unsupported chip.\n");
> +		goto err_free;
> +	}
> +
> +	/*
> +	 * OK, we got a valid chip so we can fill in the remaining client
> +	 * fields.
> +	 */
> +
> +	strlcpy(new_client->name, name, I2C_NAME_SIZE);
> +	data->valid = 0;
> +	mutex_init(&data->update_lock);
> +
> +	/*
> +	 * Tell the I2C layer a new client has arrived.
> +	 */
> +
> +	if ((err = i2c_attach_client(new_client))) {
> +		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
> +		goto err_free;
> +	}
> +
> +	/*
> +	 * Initialize the max6650 chip
> +	 */
> +	max6650_init_client(new_client);
> +
> +	/* 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 err_detach;
> +	}
> +
> +	err = sysfs_create_group(&new_client->dev.kobj, &max6650_attr_grp);
> +	if (!err)
> +		return 0;
> +
> +	dev_err(&new_client->dev, "error creating sysfs files (%d)\n", err);
> +	hwmon_device_unregister(data->class_dev);
> +err_detach:
> +	i2c_detach_client(new_client);
> +err_free:
> +	kfree(data);
> +	return err;
> +}
> +
> +static int max6650_detach_client(struct i2c_client *client)
> +{
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int err;
> +	hwmon_device_unregister(data->class_dev);
> +	err = i2c_detach_client(client);
> +	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
> +	kfree(data);
> +	return err;
> +}
> +
> +static void max6650_init_client(struct i2c_client *client)
> +{
> +	/* Nothing to do here - assume the BIOS has initialized the chip */
> +}
> +
> +static const u8 tach_reg[] > +{
> +	MAX6650_REG_TACH0,
> +	MAX6650_REG_TACH1,
> +	MAX6650_REG_TACH2,
> +	MAX6650_REG_TACH3,
> +};
> +
> +static struct max6650_data *max6650_update_device(struct device *dev)
> +{
> +	int i;
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> +		data->speed = i2c_smbus_read_byte_data(client,
> +			 			       MAX6650_REG_SPEED);
> +		data->config = i2c_smbus_read_byte_data(client,
> +							MAX6650_REG_CONFIG);
> +		for (i = 0; i < 4; i++) {
> +			data->tach[i] = i2c_smbus_read_byte_data(client,
> +								 tach_reg[i]);
> +		}
> +		data->count = i2c_smbus_read_byte_data (client,
> +							MAX6650_REG_COUNT);
> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static int __init sensors_max6650_init(void)
> +{
> +	return i2c_add_driver(&max6650_driver);
> +}
> +
> +static void __exit sensors_max6650_exit(void)
> +{
> +	i2c_del_driver(&max6650_driver);
> +}
> +
> +MODULE_AUTHOR("john.morris at spirentcom.com");
> +MODULE_DESCRIPTION("max6650 sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_max6650_init);
> +module_exit(sensors_max6650_exit);
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors


hello

Instead of declaring four static DEVICE_ATTR(fan1, S_IRUGO, get_fan1, NULL);
I'm suggest you to use
static SENSOR_DEVICE_ATTR(fan1, S_IRUGO, get_fan, NULL,1);
static SENSOR_DEVICE_ATTR(fan2, S_IRUGO, get_fan, NULL,2);
etc....

secondly in /Documentation/hwmon/max6650 you have a wrong prefix
+Supported chips:
 > +  * Maxim 6650 / 6651
 > +    Prefix: 'w83792d'

and 2 typos errors
(but and not bit)
-> bit the Maxim 6550 has a reduced feature

6550 not 6650
 > +  * Maxim 6650 / 6651
 > +    Prefix: 'w83792d'

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

On Thu, 01 Mar 2007 18:34:54 +0100, corentin.labbe wrote:
> Instead of declaring four static DEVICE_ATTR(fan1, S_IRUGO, get_fan1, NULL);
> I'm suggest you to use
> static SENSOR_DEVICE_ATTR(fan1, S_IRUGO, get_fan, NULL,1);
> static SENSOR_DEVICE_ATTR(fan2, S_IRUGO, get_fan, NULL,2);
> etc....
> 
> secondly in /Documentation/hwmon/max6650 you have a wrong prefix
> +Supported chips:
>  > +  * Maxim 6650 / 6651
>  > +    Prefix: 'w83792d'
> 
> and 2 typos errors
> (but and not bit)
> -> bit the Maxim 6550 has a reduced feature
> 
> 6550 not 6650
>  > +  * Maxim 6650 / 6651
>  > +    Prefix: 'w83792d'

Actually, the other way around: 6650 is correct and 6550 is the typo.

Thanks for this second review Corentin.

Hans-J?rgen, care to post an updated version of your driver? Hopefully
we're not too far from a final version now.

-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Hi Jean,
I updated the driver once again. Basically, I made these changes:

* "mode" is now not only a module parameter, but also a sysfs file. This is 
needed to simply switch the fan fully on/off. Depending on the other 
settings, this might not be possible with the "speed" attribute alone.

* Setting "speed" really only sets the speed value. The original 
implementation changed "mode" between "off" and "closed loop" when speed was 
0 or >0, respectively. I consider this bad style, a driver should do what 
it's told to do and not make guesses about what the user might wish.

* Updated documentation.

I left the other values as module parameters. I think it's unlikely that 
somebody wants to set these to different values if he has more than one chip. 
Of course we could make all of them sysfs files, too, but I think this is not 
necessary. Let's wait and see if somebody actually wants that.

Thanks,
Hans

Signed-off-by: Hans J. Koch <hjk at linutronix.de>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 22299 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070305/63bbcc3e/attachment.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans,

On Mon, 5 Mar 2007 12:34:21 +0100, Hans-J?rgen Koch wrote:
> Hi Jean,
> I updated the driver once again. Basically, I made these changes:
> 
> * "mode" is now not only a module parameter, but also a sysfs file. This is 
> needed to simply switch the fan fully on/off. Depending on the other 
> settings, this might not be possible with the "speed" attribute alone.
> 
> * Setting "speed" really only sets the speed value. The original 
> implementation changed "mode" between "off" and "closed loop" when speed was 
> 0 or >0, respectively. I consider this bad style, a driver should do what 
> it's told to do and not make guesses about what the user might wish.
> 
> * Updated documentation.
> 
> I left the other values as module parameters. I think it's unlikely that 
> somebody wants to set these to different values if he has more than one chip. 
> Of course we could make all of them sysfs files, too, but I think this is not 
> necessary. Let's wait and see if somebody actually wants that.

I can't get this version to compile:

  CC [M]  drivers/hwmon/max6650.o
drivers/hwmon/max6650.c: In function `max6650_attach_adapter':
drivers/hwmon/max6650.c:350: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:350: error: invalid type argument of `->'
drivers/hwmon/max6650.c:354: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:354: error: invalid type argument of `->'
drivers/hwmon/max6650.c: In function `max6650_detect':
drivers/hwmon/max6650.c:374: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:374: error: invalid type argument of `->'
drivers/hwmon/max6650.c:377: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:377: error: invalid type argument of `->'
drivers/hwmon/max6650.c:423: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:423: error: invalid type argument of `->'
drivers/hwmon/max6650.c:455: error: incompatible type for argument 1 of `dev_driver_string'
drivers/hwmon/max6650.c:455: error: invalid type argument of `->'
make[2]: *** [drivers/hwmon/max6650.o] Error 1
make[1]: *** [drivers/hwmon] Error 2
make: *** [drivers] Error 2

I guess you didn't try with debug enabled...

Also, Corentin Labbe made interesting comments here:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019017.html
(at the end of the post)
Please fix the problems he mentioned.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans,

Sorry for the delay, I've been busy with large i2c-core changes.

On Thu, 1 Mar 2007 11:11:12 +0100, Hans-J?rgen Koch wrote:
> Am Donnerstag 01 M?rz 2007 08:34 schrieb Jean Delvare:
> > prescaler: What does it do? It smells like a fan clock divider to
> > me. In that case it would be better implemented as a fan1_div sysfs
> > file, which the user can adjust at runtime.
> 
> The chip generates a reference frequency which is determined by the 
> speed register, the prescaler, and the internal clock (nominal value 
> 254kHz). The regulator then tries to make the tacho frequency equal 
> to this reference frequency. 
> As clock is fixed and speed has a well defined meaning, the prescaler 
> is the only way to adapt to different tacho generators. The data 
> sheet gives hints about how to select a prescaler value to achieve 
> optimal range and resolution. If you get that wrong, the fan might 
> not reach it's full speed, switch between two speed values only or 
> similar effects.
> 
> I read Documentation/hwmon/sysfs-interface, but I have to admit that I 
> don't really understand if fan[1-*]_div has this meaning. If it has, 
> we could possibly make prescaler such a sysfs file. 

I'm not certain myself. fan1_div is usually related to the monitoring
only and not to the output.

Typical chips monitor fan speeds by gating a well defined frequency
with the tachometer pulses. The result is stored in an 8-bit register,
so basically you can only measure fan speeds down to F/255, speeds
below this limit overflow the register. So the chips usually let the
user divide F by a power of two to be able to monitor slower fans - at
the price of resolution.

So the concept is similar to your "prescaler" in that it's a tradeoff
between resolution and range. But the fan speed control method of the
MAX6650 is completely different from what the other chips do, so other
chips don't need any kind of divider for fan speed control.

> > clock: OK.
> 
> This is the only one where I think it could be omitted. The internal 
> 254kHz oscillator has a tolerance of +/-10%, so fan speed can also be 
> off +/-10%. This parameter is just there to compensate for that. I 
> doubt it will ever be used in practical applications.

Agreed, it probably makes sense to get rid of this one.

> > mode: This smells like fan speed control parameters, which we
> > typically implement as the pwm1_enable sysfs file. Please take a
> > look at the description in Documentation/hwmon/sysfs-interface and
> > tell me what you think.
> 
> That looks good. We can implement pwm1_enable that way. Problem is 
> that we can have up to four fan speeds (fan[1-4]_input), but the 
> control values are there only once (pwm1, pwm1_enable). Is that 
> possible?

Yes, this isn't a problem. I am not a specialist but I don't think it's
possible to have a closed loop fan control working with 4 inputs and 1
output, so I believe the output is really related to fan1 only. A
comment in the original driver seems to confirm that ("Only one fan's
speed (fan1) is controlled.")

I don't think the MAX6650 implements anything like pwm1, but more like
pwm1_enable and fan1_target. "speed" is expressed in RPM, isn't it?

> > counttime: This one too seems to be related to a fan clock divider?
> > How does it functionally differ from prescaler?
> 
> counttime is the time interval during which tacho pulses are counted.
> It determines the resolution of the measured speed values. It should 
> be set to the longest time that still ensures the 8-bit counter 
> doesn't overflow under worst case conditions. 

_This_ really sounds like fan1_div. It changes the period of time
rather than the clock frequency, but from a functional point of view
the result is the same: preventing 8-bit register overflow for certain
speed fans. The only difference is that typical chips overflow for slow
fans, while the MAX6650 is more likely to overflow when the fan is too
fast, if I understood correctly.

So please implement it that way. fan1_div = 1 should correspond to the
smaller period of time (faster fan.)

Aren't the count time and precaler values somewhat related? Won't the
user have to change both the same way if he/she gets a new fan running
at a different speed? Maybe it would make sense to link fan1_div to
both the prescaler and the count time after all.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Sonntag 11 M?rz 2007 16:21 schrieb Jean Delvare:
> Hi Hans,
>
> Sorry for the delay, I've been busy with large i2c-core changes.

I noticed :-) 
No problem.

> >
> > I read Documentation/hwmon/sysfs-interface, but I have to admit that I
> > don't really understand if fan[1-*]_div has this meaning. If it has,
> > we could possibly make prescaler such a sysfs file.
>
> I'm not certain myself. fan1_div is usually related to the monitoring
> only and not to the output.
>
> Typical chips monitor fan speeds by gating a well defined frequency
> with the tachometer pulses. The result is stored in an 8-bit register,
> so basically you can only measure fan speeds down to F/255, speeds
> below this limit overflow the register. So the chips usually let the
> user divide F by a power of two to be able to monitor slower fans - at
> the price of resolution.
>
> So the concept is similar to your "prescaler" in that it's a tradeoff
> between resolution and range. But the fan speed control method of the
> MAX6650 is completely different from what the other chips do, so other
> chips don't need any kind of divider for fan speed control.

Obviously, we've reached some limits of the current concept. IMHO, the 'pwm' 
and 'divider' sysfs files are much too hardware dependent. We should have 
more abstraction, e.g. like this:

fan1_input: (ro) Current speed in RPM
fan1_speed: (rw) Desired speed in RPM
fan1_min_speed,
fan1_max_speed: (rw) Planned/defined operating range in RPM
fan1_tacho: (rw) Pulses per round of the tacho generator
fan1_mode: (rw) on, off, closed_loop, open_loop, ...

From this, _every_ driver could calculate the necessary register values it 
needs, without bothering the user with chip internal details. Why should a 
user care if the fan regulator chip uses PWM or not? The values I mentioned 
above are the ones he/she knows and can easily set without having the data 
sheet in one hand and the pocket calculator in the other.

Of course, implementation is slightly more complicated, as the values depend 
on each other, so setting one of them might invalidate the setup until an 
other is set. The driver needs to deal with that in a reasonable way. But I 
think I could easily do it for the max6650. It shouldn't be difficult for 
other drivers, too.

[...]

>
> Yes, this isn't a problem. I am not a specialist but I don't think it's
> possible to have a closed loop fan control working with 4 inputs and 1
> output, so I believe the output is really related to fan1 only. A
> comment in the original driver seems to confirm that ("Only one fan's
> speed (fan1) is controlled.")

That comment, as well as the data sheet, are a bit misleading. In fact, the 
MAX6651 is NOT able to drive four independent fans. It just has four tacho 
inputs. The first one can be used to close the loop if you want closed loop 
operation. To the other three, you can connect whatever you like. The 
intended use is to connect four identical fans in parallel to the output. The 
tacho signal of one of them is used for closed loop regulation. If the fans 
are _really_ identical, all of them should have the same speed. You can check 
their actual speeds by looking at the other tacho inputs.

> >
> > counttime is the time interval during which tacho pulses are counted.
> > It determines the resolution of the measured speed values. It should
> > be set to the longest time that still ensures the 8-bit counter
> > doesn't overflow under worst case conditions.
>
> _This_ really sounds like fan1_div. It changes the period of time
> rather than the clock frequency, but from a functional point of view
> the result is the same: preventing 8-bit register overflow for certain
> speed fans. The only difference is that typical chips overflow for slow
> fans, while the MAX6650 is more likely to overflow when the fan is too
> fast, if I understood correctly.
>
> So please implement it that way. fan1_div = 1 should correspond to the
> smaller period of time (faster fan.)

As I said above, I really believe we shouldn't do that but be less hardware 
dependent.

>
> Aren't the count time and precaler values somewhat related? Won't the
> user have to change both the same way if he/she gets a new fan running
> at a different speed? Maybe it would make sense to link fan1_div to
> both the prescaler and the count time after all.

If I did that, confusion would be complete. A user would not only need data 
sheet in one hand, pocket calculator in other hand, but also driver source 
code in the third hand to understand what's going on :-)

Initially, I didn't want to make a lot of changes to that driver, just have it 
in the mainline kernel. But meanwhile, I feel if we do it, we should do it 
properly. That means, either we accept the driver as it is now (with special 
sysfs files no other driver has), or we change the specification in 
Documentation/hwmon/sysfs-interface to be more general so that all fan 
drivers fit in.

What's your opinion?

Thanks,
Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Sun, 11 Mar 2007 22:08:01 +0100, Hans-J?rgen Koch wrote:
> Obviously, we've reached some limits of the current concept. IMHO, the 'pwm' 
> and 'divider' sysfs files are much too hardware dependent. We should have 
> more abstraction, e.g. like this:

Forget about it right away, this interface is here to stay and we're
not changing it.

> fan1_input: (ro) Current speed in RPM
> fan1_speed: (rw) Desired speed in RPM

We have fan1_input and fan1_target for this. Note though that many fan
speed controllers do not work by setting a desired fan speed, but by
setting a specific output voltage (DC) or duty cycle (PWM), or a target
temperature.

> fan1_min_speed,
> fan1_max_speed: (rw) Planned/defined operating range in RPM

We have fan1_min for the former. We could have fan1_max for the latter,
but no known chip actually supports upper limit for fan speeds.

> fan1_tacho: (rw) Pulses per round of the tacho generator

99% of the fans out there emit two pulses per revolution, so we didn't
bother, although a couple driver do expose a non-standard sysfs file
for that. The fact is that this can be addressed easily in user-space,
so the interest in implementing it in every driver is questionable.

> fan1_mode: (rw) on, off, closed_loop, open_loop, ...

This is exactly what pwm1_enable does. The name could have been chosen
better, granted.

So as you can see, all the concepts you want are already in place, just
not using the names you'd like.

> From this, _every_ driver could calculate the necessary register values it 
> needs, without bothering the user with chip internal details. Why should a 
> user care if the fan regulator chip uses PWM or not? The values I mentioned 
> above are the ones he/she knows and can easily set without having the data 
> sheet in one hand and the pocket calculator in the other.

We chose the interface we have now because this is how the vast
majority of chips work.

> Of course, implementation is slightly more complicated, as the values depend 
> on each other, so setting one of them might invalidate the setup until an 
> other is set. The driver needs to deal with that in a reasonable way. But I 
> think I could easily do it for the max6650. It shouldn't be difficult for 
> other drivers, too.

Again this isn't going to happen. We have a standard interface, it was
hard enough to put in place, we're not changing it again. If you want a
different interface, you get to build a timeback machine first, get
back 3 years in the past when we designed this interface, and inflence
the discussions ;)

The drivers must match the standard interface, and not the other way
around. The MAX6650 is quite different from the other chips, so
things look a bit odd from your point of view, and I'm sorry about
that, but in general the interface we have works well enough, and we
should be able to make the max6650 driver fit in it too.

> > Yes, this isn't a problem. I am not a specialist but I don't think it's
> > possible to have a closed loop fan control working with 4 inputs and 1
> > output, so I believe the output is really related to fan1 only. A
> > comment in the original driver seems to confirm that ("Only one fan's
> > speed (fan1) is controlled.")
> 
> That comment, as well as the data sheet, are a bit misleading. In fact, the 
> MAX6651 is NOT able to drive four independent fans. It just has four tacho 
> inputs. The first one can be used to close the loop if you want closed loop 
> operation. To the other three, you can connect whatever you like. The 
> intended use is to connect four identical fans in parallel to the output. The 
> tacho signal of one of them is used for closed loop regulation. If the fans 
> are _really_ identical, all of them should have the same speed. You can check 
> their actual speeds by looking at the other tacho inputs.

Correct. I don't see anything confusing here, BTW.

> > > counttime is the time interval during which tacho pulses are counted.
> > > It determines the resolution of the measured speed values. It should
> > > be set to the longest time that still ensures the 8-bit counter
> > > doesn't overflow under worst case conditions.
> >
> > _This_ really sounds like fan1_div. It changes the period of time
> > rather than the clock frequency, but from a functional point of view
> > the result is the same: preventing 8-bit register overflow for certain
> > speed fans. The only difference is that typical chips overflow for slow
> > fans, while the MAX6650 is more likely to overflow when the fan is too
> > fast, if I understood correctly.
> >
> > So please implement it that way. fan1_div = 1 should correspond to the
> > smaller period of time (faster fan.)
> 
> As I said above, I really believe we shouldn't do that but be less hardware 
> dependent.

It's just giving a different name to something to make it fit in the
standard. It's rather _less_ hardware dependent than what you propose.

> > Aren't the count time and precaler values somewhat related? Won't the
> > user have to change both the same way if he/she gets a new fan running
> > at a different speed? Maybe it would make sense to link fan1_div to
> > both the prescaler and the count time after all.
> 
> If I did that, confusion would be complete. A user would not only need data 
> sheet in one hand, pocket calculator in other hand, but also driver source 
> code in the third hand to understand what's going on :-)

I'm not insisting on this particular point then. This whole prescaler
thing is above my head anyway.

> Initially, I didn't want to make a lot of changes to that driver, just have it 
> in the mainline kernel. But meanwhile, I feel if we do it, we should do it 
> properly. That means, either we accept the driver as it is now (with special 
> sysfs files no other driver has), or we change the specification in 
> Documentation/hwmon/sysfs-interface to be more general so that all fan 
> drivers fit in.
> 
> What's your opinion?

That either the max6650 driver complies with the standard interface as
much as possible and I'll be pleased to merge it in mainline, or it uses
it's own sysfs files no other driver uses and no user-space application
supports, that makes it essentially useless and I can't take it in
mainline.

I'm not asking that you remove features from the driver because they
just don't fit in the interface (as seems to be the case of the
prescaler thing), just that you use the standard names where they
exist, even if the name itself doesn't match the physical reality.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Montag, 12. M?rz 2007 14:50 schrieb Jean Delvare:
> Hi Hans-J?rgen,
>
> On Sun, 11 Mar 2007 22:08:01 +0100, Hans-J?rgen Koch wrote:
> > Obviously, we've reached some limits of the current concept. IMHO, the
> > 'pwm' and 'divider' sysfs files are much too hardware dependent. We
> > should have more abstraction, e.g. like this:
>
> Forget about it right away, this interface is here to stay and we're
> not changing it.

All right, I accept that and won't argue.

>
> > fan1_input: (ro) Current speed in RPM
> > fan1_speed: (rw) Desired speed in RPM
>
> We have fan1_input and fan1_target for this. 

Documentation/hwmon/sysfs-interface doesn't mention fan1_target. I tried to 
follow that specification. I'll rename "speed" accordingly.

[...]
>
> > fan1_min_speed,
> > fan1_max_speed: (rw) Planned/defined operating range in RPM
>
> We have fan1_min for the former. We could have fan1_max for the latter,
> but no known chip actually supports upper limit for fan speeds.

My intention was not to set upper/lower limits but to allow the driver to 
calculate things like "divider" or "counttime". If I make "divider" 
or "counttime" (or whatever algorithm a chip uses) sysfs attributes, then a 
user needs data sheet and calculator to find appropriate values for them. 
Min/max values are either known or can simply be found by experimenting and 
the rest is calculated by the driver, not by the user.
But as I said, I won't argue, I'll implement it your way.

>
> > fan1_tacho: (rw) Pulses per round of the tacho generator
>
> 99% of the fans out there emit two pulses per revolution, so we didn't
> bother, although a couple driver do expose a non-standard sysfs file
> for that. The fact is that this can be addressed easily in user-space,
> so the interest in implementing it in every driver is questionable.

Again, I won't argue.

>
> > fan1_mode: (rw) on, off, closed_loop, open_loop, ...
>
> This is exactly what pwm1_enable does. The name could have been chosen
> better, granted.

I don't understand Documentation/hwmon/sysfs-interface in this point. Which 
values for pwm1_enable correspond to on, off, closed_loop, and open_loop? Can 
I define new ones? The text mentions "2+"...

[...]
>
> > > > counttime is the time interval during which tacho pulses are counted.
> > > > It determines the resolution of the measured speed values. It should
> > > > be set to the longest time that still ensures the 8-bit counter
> > > > doesn't overflow under worst case conditions.
> > >
> > > _This_ really sounds like fan1_div. It changes the period of time
> > > rather than the clock frequency, but from a functional point of view
> > > the result is the same: preventing 8-bit register overflow for certain
> > > speed fans. The only difference is that typical chips overflow for slow
> > > fans, while the MAX6650 is more likely to overflow when the fan is too
> > > fast, if I understood correctly.
> > >
> > > So please implement it that way. fan1_div = 1 should correspond to the
> > > smaller period of time (faster fan.)

I'll do that if possible. After all, I have two registers (prescaler and 
countime) that are affected. As I'm limited to powers of two by the 
specification, it might not fit. If that is the case, I'll leave that 
attribute away and we have to live with a fixed value given by a module 
parameter.

> >
> > As I said above, I really believe we shouldn't do that but be less
> > hardware dependent.
>
> It's just giving a different name to something to make it fit in the
> standard. It's rather _less_ hardware dependent than what you propose.
>
> > > Aren't the count time and precaler values somewhat related? Won't the
> > > user have to change both the same way if he/she gets a new fan running
> > > at a different speed? Maybe it would make sense to link fan1_div to
> > > both the prescaler and the count time after all.
> >
> > If I did that, confusion would be complete. A user would not only need
> > data sheet in one hand, pocket calculator in other hand, but also driver
> > source code in the third hand to understand what's going on :-)
>
> I'm not insisting on this particular point then. This whole prescaler
> thing is above my head anyway.

OK.

>
> > Initially, I didn't want to make a lot of changes to that driver, just
> > have it in the mainline kernel. But meanwhile, I feel if we do it, we
> > should do it properly. That means, either we accept the driver as it is
> > now (with special sysfs files no other driver has), or we change the
> > specification in Documentation/hwmon/sysfs-interface to be more general
> > so that all fan drivers fit in.
> >
> > What's your opinion?
>
> That either the max6650 driver complies with the standard interface as
> much as possible and I'll be pleased to merge it in mainline, or it uses
> it's own sysfs files no other driver uses and no user-space application
> supports, that makes it essentially useless and I can't take it in
> mainline.

All right. New version will follow soon.

>
> I'm not asking that you remove features from the driver because they
> just don't fit in the interface (as seems to be the case of the
> prescaler thing), just that you use the standard names where they
> exist, even if the name itself doesn't match the physical reality.

I remove the feature of being able to adapt to a special fan through sysfs 
files, but I don't really care. To get the driver merged is more important to 
me.

Thanks,
Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Mon, 12 Mar 2007 15:44:19 +0100, Hans-J?rgen Koch wrote:
> Am Montag, 12. M?rz 2007 14:50 schrieb Jean Delvare:
> > We have fan1_input and fan1_target for this. 
> 
> Documentation/hwmon/sysfs-interface doesn't mention fan1_target. I tried to 
> follow that specification. I'll rename "speed" accordingly.

Sorry about that, it is relatively new, only one driver supports that
at the moment (f71805f) and I forgot to update the documentation
accordingly. Patch attached, comments welcome.

> > > fan1_min_speed,
> > > fan1_max_speed: (rw) Planned/defined operating range in RPM
> >
> > We have fan1_min for the former. We could have fan1_max for the latter,
> > but no known chip actually supports upper limit for fan speeds.
> 
> My intention was not to set upper/lower limits but to allow the driver to 
> calculate things like "divider" or "counttime". If I make "divider" 
> or "counttime" (or whatever algorithm a chip uses) sysfs attributes, then a 
> user needs data sheet and calculator to find appropriate values for them. 
> Min/max values are either known or can simply be found by experimenting and 
> the rest is calculated by the driver, not by the user.

Ah, I get the idea now, interesting.

I proposed some times ago that drivers could adjust these parameters by
themselves, by detecting overflows and underflows, but my proposal
received a mitigated response, and only a few drivers are implementing
this at the moment (w83627ehf, adm9240 and pc87360 IIRC). Choosing the
fan clock divider (or whatever it is) based on expected speed range is
more or less the same. For most chips, fast fans are not the problem,
slow fans are, and setting fan1_min had the effect of pinning down the
clock divider to the correct clock divider value in my model. Pretty
much what you are proposing...

> > > fan1_mode: (rw) on, off, closed_loop, open_loop, ...
> >
> > This is exactly what pwm1_enable does. The name could have been chosen
> > better, granted.
> 
> I don't understand Documentation/hwmon/sysfs-interface in this point. Which 
> values for pwm1_enable correspond to on, off, closed_loop, and open_loop? Can 
> I define new ones? The text mentions "2+"...

pwm1_enable = 0 means no control at all, i.e. fan on at full speed.
pwm1_enable = 1 means manual control, from fan down (pwm1 = 0) to full
speed (pwm1 = 255.) This is your open loop as I understand it.
pwm1_enable = 2 or more is any form of closed loop. Some chips
implement the feedback based on temperature, some on fan speed. Some
chips implement both (e.g. Fintek F71805F.)

> I'll do that if possible. After all, I have two registers (prescaler and 
> countime) that are affected. As I'm limited to powers of two by the 
> specification, it might not fit. If that is the case, I'll leave that 
> attribute away and we have to live with a fixed value given by a module 
> parameter.

Notice that the counttime is also only expressed as powers of 2.

> All right. New version will follow soon.

Great, thanks.

-- 
Jean Delvare
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: hwmon-sysfs-interface-add-fan-target.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070312/c90e122d/attachment-0001.pl 

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Montag, 12. M?rz 2007 17:49 schrieb Jean Delvare:

>
> Notice that the counttime is also only expressed as powers of 2.
>
> > All right. New version will follow soon.
>
> Great, thanks.

Here it is: The driver creates only standard sysfs files now. I left the 
module insmod parameters in place, though. This allows users to initialize 
ALL registers if their BIOS doesn't do so. I hope you do not object.

sysfs files follow the specification as close as possible. I used 
pwm1_enable=3 for "fan off". There is no other possibility to switch the fan 
off, setting pwm1_enable to 1 and fan1_target to 0 doesn't turn it off 
completely.

You mentioned problems compiling my code. Sorry, I can't reproduce this here. 
It compiles without any warnings. If you still notice problems, please tell 
me.

Signed-off-by: Hans J. Koch <hjk at linutronix.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 24520 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070313/084653de/attachment.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Tue, 13 Mar 2007 14:25:10 +0100, Hans-J?rgen Koch wrote:
> Here it is: The driver creates only standard sysfs files now. I left the 
> module insmod parameters in place, though. This allows users to initialize 
> ALL registers if their BIOS doesn't do so. I hope you do not object.

Well, if the same can be achieved in a standard way using the sysfs
files, I don't quite see the point. It adds to confusion more than it
helps, given that they don't follow the same conventions. And it makes
the driver bigger at that. So please get rid of mode and counttime.

About module parameters: please don't initialize them to -1 if you
don't need to. The compiler can optimize static globals which default
to zero nicely.

> sysfs files follow the specification as close as possible. I used 
> pwm1_enable=3 for "fan off". There is no other possibility to switch the fan 
> off, setting pwm1_enable to 1 and fan1_target to 0 doesn't turn it off 
> completely.

I don't understand. For one thing, if you know how to turn the fan off,
then you could do it when fan1_target is set to 0, it's just a matter
of adding a few more lines of code. For another, fan1_target is related
to the closed loop mode (pwm1_enable=2) while the fan off mode would
generally be implemented as part of the open loop mode: pwm1_enable=1
and pwm1=0. But I see that you don't have a pwm1 file, so... How does
the open loop mode work at all?

> You mentioned problems compiling my code. Sorry, I can't reproduce this here. 
> It compiles without any warnings. If you still notice problems, please tell 
> me.

The problem is still there, and I explained why. Quoting myself:

> > I guess you didn't try with debug enabled...

The driver compiles fine with CONFIG_HWMON_DEBUG_CHIP=n, but fails to
compile with CONFIG_HWMON_DEBUG_CHIP=y. Try it yourself. It's probably
trivial to fix, too.

I see that you still did not fix the problems Corentin Labbe reported
after his second review [1]. Please do. When someone takes the time to
review your code - and it's not exactly a fun job - the least you can
do is take his/her findings into account for the next iteration of the
driver.

[1] http://lists.lm-sensors.org/pipermail/lm-sensors/2007-March/019017.html

Code review:

> Index: linux-2.6.20-cpx/Documentation/hwmon/max6650
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20-cpx/Documentation/hwmon/max6650	2007-03-13 12:46:07.000000000 +0100
> ...)
> +voltage_12V: 0=5V fan, 1\x12V fan, -1=don't change

This is no longer true as you changed the convention.

> Index: linux-2.6.20-cpx/drivers/hwmon/max6650.c
> =================================> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.20-cpx/drivers/hwmon/max6650.c	2007-03-13 12:36:44.000000000 +0100
> @@ -0,0 +1,746 @@
> +/*
> + * max6650.c - Part of lm_sensors, Linux kernel modules for hardware
> + *             monitoring.
> + *
> + * Author: John Morris <john.morris at spirentcom.com>
> + * Copyright (c) 2003 Spirent Communications
> + *
> + * This module has only been tested with the MAX6650 chip. It should
> + * also work with the MAX6651, though with reduced functionality. It
> + * does not distinguish max6650 and max6651 chips.
> + *
> + * Tha datasheet was last seen at:
> + *
> + *        http://pdfserv.maxim-ic.com/en/ds/MAX6650-MAX6651.pdf
> + *
> + * Ported to Linux 2.6 by Claus Gindhart <claus.gindhart at kontron.com>
> + * Modifications (C)2007 by Hans J. Koch <hjk at linutronix.de>:
> + *   - added insmod parameters
> + *   - made sysfs interface compatible with hwmon specifications
> + *   - general cleanup
> + *   - updated documentation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +
> +/*
> + * Addresses to scan. There are four disjoint possibilities, by pin config.
> + */
> +
> +static unsigned short normal_i2c[] = {0x1b, 0x1f, 0x48, 0x4b, I2C_CLIENT_END};
> +
> +/*
> + * Insmod parameters
> + */
> +
> +/* fan_voltage: 5=5V fan, 12\x12V fan, -1=don't change */
> +static int fan_voltage = -1;
> +/* prescaler: Possible values are 1,2,4,8,16, or -1 for don't change */
> +static int prescaler = -1;
> +/* clock: The clock frequency of the chip the driver should assume */
> +static int clock = 254000;
> +/* mode: 0=on, 1=off, 2=closed loop, 3=open loop, -1=don't change */
> +static int mode = -1;
> +/* counttime: 0=0.25s, 1=0.5s, 2=1.0s, 3=2.0s, -1=don't change */
> +static int counttime = -1;
> +
> +module_param(fan_voltage, int, S_IRUGO);
> +module_param(prescaler, int, S_IRUGO);
> +module_param(clock, int, S_IRUGO);
> +module_param(mode, int, S_IRUGO);
> +module_param(counttime, int, S_IRUGO);
> +
> +I2C_CLIENT_INSMOD_1(max6650);
> +
> +/*
> + * MAX 6650/6651 registers
> + */
> +
> +#define MAX6650_REG_SPEED       0x00
> +#define MAX6650_REG_CONFIG      0x02
> +#define MAX6650_REG_GPIO_DEF    0x04
> +#define MAX6650_REG_DAC         0x06
> +#define MAX6650_REG_ALARM_EN    0x08
> +#define MAX6650_REG_ALARM       0x0A
> +#define MAX6650_REG_TACH0       0x0C
> +#define MAX6650_REG_TACH1       0x0E
> +#define MAX6650_REG_TACH2       0x10
> +#define MAX6650_REG_TACH3       0x12
> +#define MAX6650_REG_GPIO_STAT   0x14
> +#define MAX6650_REG_COUNT       0x16
> +
> +/*
> + * Config register bits
> + */
> +
> +#define MAX6650_CFG_V12           	0x08
> +#define MAX6650_CFG_PRESCALER_MASK	0x07
> +#define MAX6650_CFG_PRESCALER_2		0x01
> +#define MAX6650_CFG_PRESCALER_4		0x02
> +#define MAX6650_CFG_PRESCALER_8		0x03
> +#define MAX6650_CFG_PRESCALER_16	0x04
> +#define MAX6650_CFG_MODE_MASK           0x30
> +#define MAX6650_CFG_MODE_ON             0x00
> +#define MAX6650_CFG_MODE_OFF            0x10
> +#define MAX6650_CFG_MODE_CLOSED_LOOP    0x20
> +#define MAX6650_CFG_MODE_OPEN_LOOP      0x30
> +#define MAX6650_COUNT_MASK		0x03
> +
> +/* Minimum and maximum values of the FAN-RPM */
> +#define FAN_RPM_MIN 240
> +#define FAN_RPM_MAX 30000
> +
> +#define DIV_FROM_REG(reg) (1 << (reg & 7))
> +
> +static int max6650_attach_adapter(struct i2c_adapter *adapter);
> +static int max6650_detect(struct i2c_adapter *adapter, int address, int kind);
> +static int max6650_init_client(struct i2c_client *client);
> +static int max6650_detach_client(struct i2c_client *client);
> +static struct max6650_data *max6650_update_device(struct device *dev);
> +
> +/*
> + * Driver data (common to all clients)
> + */
> +
> +static struct i2c_driver max6650_driver = {
> +	.driver = {
> +		.name	= "max6650",
> +	},
> +	.attach_adapter = max6650_attach_adapter,
> +	.detach_client  = max6650_detach_client

Please add a comma at the end of this line.

> +};
> +
> +/*
> + * Client data (each client gets its own)
> + */
> +
> +struct max6650_data
> +{
> +	struct i2c_client client;
> +	struct class_device *class_dev;
> +	struct mutex update_lock;
> +	char valid; /* zero until following fields are valid */
> +	unsigned long last_updated; /* in jiffies */
> +
> +	/* register values */
> +	u8 speed;
> +	u8 config;
> +	u8 tach[4];
> +	u8 count;
> +};
> +
> +static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
> +		       char *buf, int nr)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +
> +	/*
> +	* Calculation details:
> +	*
> +	* Each tachometer counts over an interval given by the "count"
> +	* register (0.25, 0.5, 1 or 2 seconds). This module assumes
> +	* that the fans produce two pulses per revolution (this seems
> +	* to be the most common).
> +	*/
> +
> +	int rpm = ((data->tach[nr] * 120) / DIV_FROM_REG(data->count));
> +	return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t get_fan1(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,0);
> +}
> +
> +static ssize_t get_fan2(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,1);
> +}
> +
> +static ssize_t get_fan3(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,2);
> +}
> +
> +static ssize_t get_fan4(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	return get_fan(dev,devattr,buf,3);
> +}
> +
> +/*
> + * Set the fan speed to the specified RPM (or read back the RPM setting).
> + *
> + * The MAX6650/1 will automatically control fan speed when in closed loop
> + * mode.
> + *
> + * Assumptions:
> + *
> + * 1) The MAX6650/1 is running from its internal 254kHz clock (perhaps
> + *    this should be made a module parameter).

Well, it is now.

> + *
> + * 2) The prescaler (low three bits of the config register) has already
> + *    been set to an appropriate value.
> + *
> + * The relevant equations are given on pages 21 and 22 of the datasheet.
> + *
> + * From the datasheet, the relevant equation when in regulation is:
> + *
> + *    [fCLK / (128 x (KTACH + 1))] = 2 x FanSpeed / KSCALE
> + *
> + * where:
> + *
> + *    fCLK is the oscillator frequency (either the 254kHz internal
> + *         oscillator or the externally applied clock)
> + *
> + *    KTACH is the value in the speed register
> + *
> + *    FanSpeed is the speed of the fan in rps
> + *
> + *    KSCALE is the prescaler value (1, 2, 4, 8, or 16)
> + *
> + * When reading, we need to solve for FanSpeed. When writing, we need to
> + * solve for KTACH.
> + *
> + * Note: this tachometer is completely separate from the tachometers
> + * used to measure the fan speeds. Only one fan's speed (fan1) is
> + * controlled.
> + */
> +
> +static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct max6650_data *data = max6650_update_device(dev);
> +	int kscale, ktach, rpm;
> +
> +	/*
> +	* Use the datasheet equation:
> +	*
> +	*    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
> +	*
> +	* then multiply by 60 to give rpm.
> +	*/
> +
> +	kscale = DIV_FROM_REG(data->config);
> +	ktach  = data->speed;
> +	rpm    = 60 * kscale * clock / (256 * (ktach + 1));
> +	return sprintf(buf, "%d\n", rpm);
> +}
> +
> +static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
> +			 const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int rpm = simple_strtoul(buf, NULL, 10);
> +	int kscale, ktach;
> +
> +	dev_dbg(dev, "%s called, rpm=%d\n", __FUNCTION__, rpm);

I read lately that the standard notation is __func__ not __FUNCTION.
Not that this debug line looks particularly interesting anyway.

> +
> +	mutex_lock(&data->update_lock);
> +
> +	rpm = SENSORS_LIMIT(rpm, FAN_RPM_MIN, FAN_RPM_MAX);

You don't need to hold the lock for this instruction.

> +
> +	/*
> +	* Divide the required speed by 60 to get from rpm to rps, then
> +	* use the datasheet equation:
> +	*
> +	*     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
> +	*/
> +
> +	kscale = DIV_FROM_REG(data->config);
> +	ktach  = ((clock * kscale) / (256 * rpm / 60)) - 1;
> +	if (ktach < 0)
> +		ktach = 0;
> +	if (ktach > 255)
> +		ktach = 255;
> +	data->speed  = ktach;

Doubled space.

> +
> +	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED,  data->speed);

Doubled space.

> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
> +/*
> + * Get/Set controller mode:
> + * Possible values:
> + * 0 = Fan always full on
> + * 1 = Open loop, Voltage is set according to speed, not regulated.
> + * 2 = Closed loop, RPM for all fans regulated by fan1 tachometer
> + * 3 = Fan always off
> + */
> +
> +static ssize_t get_enable(struct device *dev, struct device_attribute *devattr,
> +			 char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);

You are supposed to call the update function here, otherwise
data->config may not be valid.

> +	int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
> +
> +	switch (mode) {
> +	case 1:
> +		mode = 3;
> +		break;
> +	case 3:
> +		mode = 1;
> +	}
> +
> +	return sprintf(buf, "%d\n", mode);
> +}
> +
> +static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int mode = simple_strtoul(buf, NULL, 10);
> +
> +	if ((mode < 0)||(mode > 3))
> +		return count;
> +
> +	switch (mode) {
> +	case 1:
> +		mode = 3;
> +		break;
> +	case 3:
> +		mode = 1;
> +	}
> +
> +	data->config = (  (data->config & ~MAX6650_CFG_MODE_MASK)
> +			| (mode << 4) );
> +
> +	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);

You shouldn't touch data->config without holding data->update_lock.
Also, you should read the value of MAX6650_REG_CONFIG from the device
before changing it, as you are not writing all the bits.

> +
> +	return count;
> +}
> +
> +/*
> + * Read/write functions for fan1_div sysfs file. The MAX6650 has no such
> + * divider. We handle this by converting between divider and counttime:
> + *
> + * (counttime = k) <=> (divider = 2^k), k = 0, 1, 2, or 3
> + *
> + * Lower values of k allow to connect a faster fan without the risk of
> + * counter overflow. The price is lower resolution. You can also set counttime
> + * using the module parameter. Note that the module parameter "prescaler" also
> + * influences the behaviour. Unfortunately, there's no sysfs attribute
> + * defined for that. See the data sheet for details.
> + */
> +
> +static ssize_t get_div(struct device *dev, struct device_attribute *devattr,
> +			char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);

Here again you forgot to call the update function.

> +
> +	return sprintf(buf, "%d\n", 1 << data->count);

Why don't you use DIV_FROM_REG?

> +}
> +
> +static ssize_t set_div(struct device *dev, struct device_attribute *devattr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int div = simple_strtoul(buf, NULL, 10);
> +
> +	switch (div) {
> +	case 1:
> +		data->count = 0;
> +		break;
> +	case 2:
> +		data->count = 1;
> +		break;
> +	case 4:
> +		data->count = 2;
> +		break;
> +	case 8:
> +		data->count = 3;
> +		break;
> +	default:
> +		return count;

This returns with success, while you were not able to set the divider
the user requested! Better return an error (-EINVAL) possibly with a
message in the logs.

> +	}
> +
> +	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);

Again the while section should be protected by taking the
data->update_lock mutex.

> +	return count;
> +}
> +
> +static DEVICE_ATTR(fan1_input, S_IRUGO, get_fan1, NULL);
> +static DEVICE_ATTR(fan2_input, S_IRUGO, get_fan2, NULL);
> +static DEVICE_ATTR(fan3_input, S_IRUGO, get_fan3, NULL);
> +static DEVICE_ATTR(fan4_input, S_IRUGO, get_fan4, NULL);
> +static DEVICE_ATTR(fan1_target, S_IWUSR | S_IRUGO, get_target, set_target);
> +static DEVICE_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, get_enable, set_enable);
> +static DEVICE_ATTR(fan1_div, S_IWUSR | S_IRUGO, get_div, set_div);
> +
> +static struct attribute *max6650_attrs[] = {
> +	&dev_attr_fan1_input.attr,
> +	&dev_attr_fan2_input.attr,
> +	&dev_attr_fan3_input.attr,
> +	&dev_attr_fan4_input.attr,
> +	&dev_attr_fan1_target.attr,
> +	&dev_attr_pwm1_enable.attr,
> +	&dev_attr_fan1_div.attr,
> +	NULL,

No comma at the end of this one - it cannot be extended by definition.

> +};
> +
> +static struct attribute_group max6650_attr_grp = {
> +	.attrs = max6650_attrs,
> +};
> +
> +/*
> + * Real code
> + */
> +
> +static int max6650_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	dev_dbg(adapter->dev, "max6650_attach_adapter called for %s\n",
> +		(char*)&adapter->name);

This is bogus.

> +
> +	if (!(adapter->class & I2C_CLASS_HWMON)) {
> +		dev_dbg(adapter->dev,
> +			"FATAL: max6650_attach_adapter class HWMON not set\n");
> +		return 0;
> +	}
> +
> +	return i2c_probe(adapter, &addr_data, max6650_detect);
> +}
> +
> +/*
> + * The following function does more than just detection. If detection
> + * succeeds, it also registers the new chip.
> + */
> +
> +static int max6650_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *new_client;

Please rename this to just "client". I know many other drivers do that,
but it sucks. It's pretty obvious that the client is new.

> +	struct max6650_data *data;
> +	int err = -ENODEV;
> +	const char *name = "";
> +
> +	dev_dbg(adapter->dev, "max6650_detect called, kind = %d\n",kind);

Missing space after comma.

> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> +		dev_dbg(adapter->dev, "MAX6650: I2C bus doesn't support byte"

Missing space at the end of the first part of the string.

> +			"read mode, skipping.\n");
> +		return 0;
> +	}
> +
> +	if (!(data = kzalloc(sizeof(struct max6650_data), GFP_KERNEL))) {
> +		printk("MAX6650: Out of memory in max6650_detect "

Please either switch to dev_err(&adapter->dev, ...) or at least add
KERN_ERR before the string.

> +			"(new_client).\n");

You are actually trying to allocate "data", not "new_client". But given
that there's a single allocation in the whole driver anyway...

> +		return -ENOMEM;
> +	}
> +
> +	/*
> +	 * The common I2C client data is placed right before the
> +	 * max6650-specific data. The max6650-specific data is pointed to by
> +	 * the data field from the I2C client data.
> +	 */

Please rip off this useless comment.

> +
> +	new_client = &data->client;
> +	i2c_set_clientdata(new_client, data);
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &max6650_driver;
> +	new_client->flags = 0;

You can omit this one, the kzalloc above already set the flags to 0.

> +
> +	/*
> +	 * Now we do the remaining detection. A negative kind means that
> +	 * the driver was loaded with no force parameter (default), so we
> +	 * must both detect and identify the chip (actually there is only
> +	 * one possible kind of chip for now, max6650). A zero kind means that
> +	 * the driver was loaded with the force parameter, the detection
> +	 * step shall be skipped. A positive kind means that the driver
> +	 * was loaded with the force parameter and a given kind of chip is
> +	 * requested, so both the detection and the identification steps
> +	 * are skipped.
> +	 *
> +	 * Currently I can find no way to distinguish between a MAX6650 and
> +	 * a MAX6651. This driver has only been tried on the latter.
> +	 */

I thought it was on the former?

> +
> +	if ((kind < 0)&&
> +	   (  (i2c_smbus_read_byte_data(new_client,MAX6650_REG_CONFIG) & 0xC0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_GPIO_STAT)& 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM_EN) & 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM) & 0xE0)
> +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_COUNT) & 0xFC)))
> +	{
> +		dev_dbg(adapter->dev, "max6650.o: max6650 detection failed"

Missing space at the end of the first half, and this is not max6650.o.
Please format all your debug messages the same way, it's confusing
otherwise.

> +			"at 0x%02x.\n", address);
> +
> +		goto err_free;
> +        }

Indentation problem, the line above uses spaces instead of a tabulation.

> +
> +	if (kind <= 0)
> +		kind = max6650;
> +
> +	if (kind = max6650) {
> +		name = "max6650";
> +		printk("MAX6650: Chip found at 0x%02x.\n", address);

Missing log level.

> +	}
> +	else {
> +		printk("MAX6650: Unsupported chip.\n");

Missing log level.

> +		goto err_free;
> +	}

Given that this just cannot happen, and your driver supports only one
kind anyway, you could simply hardcode the kind and name.

> +
> +	/*
> +	 * OK, we got a valid chip so we can fill in the remaining client
> +	 * fields.
> +	 */
> +
> +	strlcpy(new_client->name, name, I2C_NAME_SIZE);
> +	data->valid = 0;

This one too can be omitted.

> +	mutex_init(&data->update_lock);
> +
> +	/*
> +	 * Tell the I2C layer a new client has arrived.
> +	 */
> +
> +	if ((err = i2c_attach_client(new_client))) {
> +		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
> +		goto err_free;
> +	}
> +
> +	/*
> +	 * Initialize the max6650 chip
> +	 */
> +	if (max6650_init_client(new_client))
> +		goto err_detach;
> +
> +	/* 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 err_detach;
> +	}

Please create the files first, and register the class only after that.

> +
> +	err = sysfs_create_group(&new_client->dev.kobj, &max6650_attr_grp);
> +	if (!err)
> +		return 0;
> +
> +	dev_err(&new_client->dev, "error creating sysfs files (%d)\n", err);
> +	hwmon_device_unregister(data->class_dev);
> +err_detach:
> +	i2c_detach_client(new_client);
> +err_free:
> +	kfree(data);
> +	return err;
> +}
> +
> +static int max6650_detach_client(struct i2c_client *client)
> +{
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	int err;
> +
> +	hwmon_device_unregister(data->class_dev);
> +	err = i2c_detach_client(client);
> +	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);

You should remove the files before detaching the client.

> +	kfree(data);

Do not free the memory if you failed to detach the client!

> +	return err;
> +}
> +
> +static int max6650_init_client(struct i2c_client *client)
> +{
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +	const char* mode_strings[4] = {"full-on", "always-off",
> +					"closed-loop", "open-loop"};
> +	int config, countreg;
> +	int err = -ENODEV;

The only errors that can happen here are I/O errors, so that would be
-EIO.

> +
> +	mutex_lock(&data->update_lock);

You don't really need to take the mutex here - there's nobody else who
can access your data at this point.

> +	config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> +
> +	if (config < 0) {
> +		dev_err(&client->dev, "Error reading config, aborting.\n");
> +		goto out;
> +	}
> +
> +	switch (fan_voltage) {
> +		case -1:
> +			break;
> +		case  5:
> +			config &= ~MAX6650_CFG_V12;
> +			break;
> +		case 12:
> +			config |= MAX6650_CFG_V12;
> +			break;
> +		default:
> +			dev_err(&client->dev,
> +				"illegal value for fan_voltage (%d)\n",
> +				fan_voltage);
> +	}
> +
> +	dev_info(&client->dev, "Fan voltage is set to %dV.\n",
> +		 (config & MAX6650_CFG_V12) ? 12 : 5);
> +
> +	switch (prescaler) {
> +		case -1:
> +			break;
> +		case  1:
> +			config &= ~MAX6650_CFG_PRESCALER_MASK;
> +			break;
> +		case  2:
> +			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
> +				  | MAX6650_CFG_PRESCALER_2 );
> +			break;
> +		case  4:
> +			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
> +				  | MAX6650_CFG_PRESCALER_4 );
> +			break;
> +		case  8:
> +			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
> +				  | MAX6650_CFG_PRESCALER_8 );
> +			break;
> +		case 16:
> +			config = ( (config & ~MAX6650_CFG_PRESCALER_MASK)
> +				  | MAX6650_CFG_PRESCALER_16 );
> +			break;
> +		default:
> +			dev_err(&client->dev,
> +				"illegal value for prescaler (%d)\n",
> +				prescaler);
> +	}
> +
> +	dev_info(&client->dev, "Prescaler is set to %d.\n",
> +		 DIV_FROM_REG(config) );

This is confusing, you're using DIV_FROM_REG for both the prescaler and
the counttime. It works more or less by accident...

> +
> +	switch (mode) {
> +		case -1:
> +			break;
> +		case  0:
> +		case  1:
> +		case  2:
> +		case  3:
> +			config = (  (config & ~MAX6650_CFG_MODE_MASK)
> +				  | (mode << 4) );
> +			break;
> +		default:
> +			dev_err(&client->dev,
> +				 "illegal value for mode (%d)\n", mode);
> +	}
> +
> +	dev_info(&client->dev, "Mode is set to %s.\n",
> +		mode_strings[(config & MAX6650_CFG_MODE_MASK) >> 4]);
> +
> +	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
> +		dev_err(&client->dev, "Error writing config, aborting.\n");
> +		goto out;
> +	}
> +
> +	data->config = config;
> +
> +	countreg = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
> +

No blank line between instruction and test.

> +	if (countreg < 0) {
> +		dev_err(&client->dev, "Error reading count time, aborting.\n");
> +		goto out;
> +	}
> +
> +	countreg &= MAX6650_COUNT_MASK;
> +	data->count = countreg;
> +
> +	if ((counttime >= 0) && (counttime <= 3)) {
> +		if (i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT,
> +					      counttime)) {
> +			dev_err(&client->dev,
> +				"Error writing counttime, aborting.\n");
> +			goto out;
> +		}
> +		data->count = counttime;
> +	}
> +	else {
> +		if (counttime != -1) {
> +			dev_err(&client->dev,
> +				"illegal value for counttime (%d)\n",
> +				counttime);
> +			goto out;
> +		}
> +	}
> +
> +	err = 0;
> +	dev_info(&client->dev, "Count time is set to %d (%d msec).\n",
> +		 data->count, (1 << data->count) * 250);
> +out:
> +	mutex_unlock(&data->update_lock);
> +	return err;
> +}
> +
> +static const u8 tach_reg[] > +{
> +	MAX6650_REG_TACH0,
> +	MAX6650_REG_TACH1,
> +	MAX6650_REG_TACH2,
> +	MAX6650_REG_TACH3,
> +};
> +
> +static struct max6650_data *max6650_update_device(struct device *dev)
> +{
> +	int i;
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max6650_data *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->update_lock);
> +
> +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> +		data->speed = i2c_smbus_read_byte_data(client,
> +			 			       MAX6650_REG_SPEED);
> +		data->config = i2c_smbus_read_byte_data(client,
> +							MAX6650_REG_CONFIG);
> +		for (i = 0; i < 4; i++) {
> +			data->tach[i] = i2c_smbus_read_byte_data(client,
> +								 tach_reg[i]);
> +		}
> +		data->count = i2c_smbus_read_byte_data (client,
> +							MAX6650_REG_COUNT);

Extra space before opening parenthesis.

> +		data->last_updated = jiffies;
> +		data->valid = 1;
> +	}
> +
> +	mutex_unlock(&data->update_lock);
> +
> +	return data;
> +}
> +
> +static int __init sensors_max6650_init(void)
> +{
> +	return i2c_add_driver(&max6650_driver);
> +}
> +
> +static void __exit sensors_max6650_exit(void)
> +{
> +	i2c_del_driver(&max6650_driver);
> +}
> +
> +MODULE_AUTHOR("john.morris at spirentcom.com");

This is more or less your driver now... BTW (I can't remember if I told
you already) feel free to add yourself to MAINTAINERS as the maintainer
for this new driver.

> +MODULE_DESCRIPTION("max6650 sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_max6650_init);
> +module_exit(sensors_max6650_exit);

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Donnerstag, 15. M?rz 2007 21:10 schrieb Jean Delvare:


Jean,
Thank you for your detailed review! Here's the next iteration.

> So please get rid of mode and counttime.

Done.

>
> About module parameters: please don't initialize them to -1 if you
> don't need to. The compiler can optimize static globals which default
> to zero nicely.

Done.

>
> > sysfs files follow the specification as close as possible. I used
> > pwm1_enable=3 for "fan off". There is no other possibility to switch the
> > fan off, setting pwm1_enable to 1 and fan1_target to 0 doesn't turn it
> > off completely.
>
> I don't understand. For one thing, if you know how to turn the fan off,
> then you could do it when fan1_target is set to 0, it's just a matter
> of adding a few more lines of code. For another, fan1_target is related
> to the closed loop mode (pwm1_enable=2) while the fan off mode would
> generally be implemented as part of the open loop mode: pwm1_enable=1
> and pwm1=0. But I see that you don't have a pwm1 file, so... How does
> the open loop mode work at all?

Open loop mode just sets the ouput voltage according to a DAC value (0..255).
If someone needs a defined speed, he's supposed to do the regulator in 
software. It is not influenced by the speed register used in closed loop 
mode. I added a pwm1 file to read/write that DAC value. After doing that, I 
noticed the fan can be brought to full/zero speed by setting pwm1 to 0 or 
255, respectively. That's OK for me.
For pwm1_enable, I only use the well defined values now: 0=off, 1=open loop, 
2=closed loop. If I find the chip in "full on" mode at load time, I change 
mode to open loop and set it to full speed. I hope this always works and 
won't set somebodies board on fire :-)

BTW, meanwhile I'm a bit confused about what pwm1_enable=0 means. Is it "fan 
off" or "PWM off, fan full on"? At the moment, I implemented "fan off", but I 
can easily change that. The other way round would be slightly simpler.

>
> > You mentioned problems compiling my code. Sorry, I can't reproduce this
> > here. It compiles without any warnings. If you still notice problems,
> > please tell me.
>
> The problem is still there, and I explained why. Quoting myself:
> > > I guess you didn't try with debug enabled...
>
> The driver compiles fine with CONFIG_HWMON_DEBUG_CHIP=n, but fails to
> compile with CONFIG_HWMON_DEBUG_CHIP=y. Try it yourself. It's probably
> trivial to fix, too.

It was and it's fixed now.

>
> I see that you still did not fix the problems Corentin Labbe reported
> after his second review [1]. Please do. When someone takes the time to
> review your code - and it's not exactly a fun job - the least you can
> do is take his/her findings into account for the next iteration of the
> driver.

I agree. I started adding his ideas in one tree, then did something else for a 
few days, and started again in a new tree. His fixes got lost on the way. 
Sorry. I added them now.


> > +voltage_12V: 0=5V fan, 1\x12V fan, -1=don't change
>
> This is no longer true as you changed the convention.

fixed.

> > +static struct i2c_driver max6650_driver = {
> > +	.driver = {
> > +		.name	= "max6650",
> > +	},
> > +	.attach_adapter = max6650_attach_adapter,
> > +	.detach_client  = max6650_detach_client
>
> Please add a comma at the end of this line.

fixed.

> > + *
> > + * Assumptions:
> > + *
> > + * 1) The MAX6650/1 is running from its internal 254kHz clock (perhaps
> > + *    this should be made a module parameter).
>
> Well, it is now.

fixed.

> > +
> > +	dev_dbg(dev, "%s called, rpm=%d\n", __FUNCTION__, rpm);
>
> I read lately that the standard notation is __func__ not __FUNCTION.
> Not that this debug line looks particularly interesting anyway.

True. I removed it.

>
> > +
> > +	mutex_lock(&data->update_lock);
> > +
> > +	rpm = SENSORS_LIMIT(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
>
> You don't need to hold the lock for this instruction.

fixed.

> > +	data->speed  = ktach;
>
> Doubled space.

fixed.

>
> > +
> > +	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED,  data->speed);
>
> Doubled space.

fixed.


> > +static ssize_t get_enable(struct device *dev, struct device_attribute
> > *devattr, +			 char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct max6650_data *data = i2c_get_clientdata(client);
>
> You are supposed to call the update function here, otherwise
> data->config may not be valid.

Very true. Fixed.

> > +
> > +	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
>
> You shouldn't touch data->config without holding data->update_lock.
> Also, you should read the value of MAX6650_REG_CONFIG from the device
> before changing it, as you are not writing all the bits.

fixed.

> > +static ssize_t get_div(struct device *dev, struct device_attribute
> > *devattr, +			char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct max6650_data *data = i2c_get_clientdata(client);
>
> Here again you forgot to call the update function.

fixed.

>
> > +
> > +	return sprintf(buf, "%d\n", 1 << data->count);
>
> Why don't you use DIV_FROM_REG?

fixed.

>
> > +}
> > +
> > +static ssize_t set_div(struct device *dev, struct device_attribute
> > *devattr, +			const char *buf, size_t count)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct max6650_data *data = i2c_get_clientdata(client);
> > +	int div = simple_strtoul(buf, NULL, 10);
> > +
> > +	switch (div) {
> > +	case 1:
> > +		data->count = 0;
> > +		break;
> > +	case 2:
> > +		data->count = 1;
> > +		break;
> > +	case 4:
> > +		data->count = 2;
> > +		break;
> > +	case 8:
> > +		data->count = 3;
> > +		break;
> > +	default:
> > +		return count;
>
> This returns with success, while you were not able to set the divider
> the user requested! Better return an error (-EINVAL) possibly with a
> message in the logs.

fixed.

>
> > +	}
> > +
> > +	i2c_smbus_write_byte_data(client, MAX6650_REG_COUNT, data->count);
>
> Again the while section should be protected by taking the
> data->update_lock mutex.

I guess you meant the switch section. Fixed.

> > +static struct attribute *max6650_attrs[] = {
> > +	&dev_attr_fan1_input.attr,
> > +	&dev_attr_fan2_input.attr,
> > +	&dev_attr_fan3_input.attr,
> > +	&dev_attr_fan4_input.attr,
> > +	&dev_attr_fan1_target.attr,
> > +	&dev_attr_pwm1_enable.attr,
> > +	&dev_attr_fan1_div.attr,
> > +	NULL,
>
> No comma at the end of this one - it cannot be extended by definition.

True. Fixed.

> > +
> > +static int max6650_attach_adapter(struct i2c_adapter *adapter)
> > +{
> > +	dev_dbg(adapter->dev, "max6650_attach_adapter called for %s\n",
> > +		(char*)&adapter->name);
>
> This is bogus.

Removed.

> > +static int max6650_detect(struct i2c_adapter *adapter, int address, int
> > kind) +{
> > +	struct i2c_client *new_client;
>
> Please rename this to just "client". I know many other drivers do that,
> but it sucks. It's pretty obvious that the client is new.

Done.

>
> > +	struct max6650_data *data;
> > +	int err = -ENODEV;
> > +	const char *name = "";
> > +
> > +	dev_dbg(adapter->dev, "max6650_detect called, kind = %d\n",kind);
>
> Missing space after comma.

Fixed.

>
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > +		dev_dbg(adapter->dev, "MAX6650: I2C bus doesn't support byte"
>
> Missing space at the end of the first part of the string.

Fixed.

>
> > +			"read mode, skipping.\n");
> > +		return 0;
> > +	}
> > +
> > +	if (!(data = kzalloc(sizeof(struct max6650_data), GFP_KERNEL))) {
> > +		printk("MAX6650: Out of memory in max6650_detect "
>
> Please either switch to dev_err(&adapter->dev, ...) or at least add
> KERN_ERR before the string.

Used dev_err() instead.

>
> > +			"(new_client).\n");
>
> You are actually trying to allocate "data", not "new_client". But given
> that there's a single allocation in the whole driver anyway...

Simplified the message.

>
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/*
> > +	 * The common I2C client data is placed right before the
> > +	 * max6650-specific data. The max6650-specific data is pointed to by
> > +	 * the data field from the I2C client data.
> > +	 */
>
> Please rip off this useless comment.

Done.

>
> > +
> > +	new_client = &data->client;
> > +	i2c_set_clientdata(new_client, data);
> > +	new_client->addr = address;
> > +	new_client->adapter = adapter;
> > +	new_client->driver = &max6650_driver;
> > +	new_client->flags = 0;
>
> You can omit this one, the kzalloc above already set the flags to 0.

Done.

>
> > +
> > +	/*
> > +	 * Now we do the remaining detection. A negative kind means that
> > +	 * the driver was loaded with no force parameter (default), so we
> > +	 * must both detect and identify the chip (actually there is only
> > +	 * one possible kind of chip for now, max6650). A zero kind means that
> > +	 * the driver was loaded with the force parameter, the detection
> > +	 * step shall be skipped. A positive kind means that the driver
> > +	 * was loaded with the force parameter and a given kind of chip is
> > +	 * requested, so both the detection and the identification steps
> > +	 * are skipped.
> > +	 *
> > +	 * Currently I can find no way to distinguish between a MAX6650 and
> > +	 * a MAX6651. This driver has only been tried on the latter.
> > +	 */
>
> I thought it was on the former?

Sure. Fixed.

>
> > +
> > +	if ((kind < 0)&&
> > +	   (  (i2c_smbus_read_byte_data(new_client,MAX6650_REG_CONFIG) & 0xC0)
> > +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_GPIO_STAT)&
> > 0xE0) +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM_EN)
> > & 0xE0) +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_ALARM) &
> > 0xE0) +	    ||(i2c_smbus_read_byte_data(new_client,MAX6650_REG_COUNT) &
> > 0xFC))) +	{
> > +		dev_dbg(adapter->dev, "max6650.o: max6650 detection failed"
>
> Missing space at the end of the first half, and this is not max6650.o.
> Please format all your debug messages the same way, it's confusing
> otherwise.

Done.

>
> > +			"at 0x%02x.\n", address);
> > +
> > +		goto err_free;
> > +        }
>
> Indentation problem, the line above uses spaces instead of a tabulation.

Fixed.

>
> > +
> > +	if (kind <= 0)
> > +		kind = max6650;
> > +
> > +	if (kind = max6650) {
> > +		name = "max6650";
> > +		printk("MAX6650: Chip found at 0x%02x.\n", address);
>
> Missing log level.

Used dev_info() instead.

>
> > +	}
> > +	else {
> > +		printk("MAX6650: Unsupported chip.\n");
>
> Missing log level.

Used dev_info() instead.

>
> > +		goto err_free;
> > +	}
>
> Given that this just cannot happen, and your driver supports only one
> kind anyway, you could simply hardcode the kind and name.

Done.

>
> > +
> > +	/*
> > +	 * OK, we got a valid chip so we can fill in the remaining client
> > +	 * fields.
> > +	 */
> > +
> > +	strlcpy(new_client->name, name, I2C_NAME_SIZE);
> > +	data->valid = 0;
>
> This one too can be omitted.

Done.

>
> > +	mutex_init(&data->update_lock);
> > +
> > +	/*
> > +	 * Tell the I2C layer a new client has arrived.
> > +	 */
> > +
> > +	if ((err = i2c_attach_client(new_client))) {
> > +		dev_dbg(adapter->dev, "max6650.o: Failed attaching client.\n");
> > +		goto err_free;
> > +	}
> > +
> > +	/*
> > +	 * Initialize the max6650 chip
> > +	 */
> > +	if (max6650_init_client(new_client))
> > +		goto err_detach;
> > +
> > +	/* 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 err_detach;
> > +	}
>
> Please create the files first, and register the class only after that.

Done.

> > +
> > +	hwmon_device_unregister(data->class_dev);
> > +	err = i2c_detach_client(client);
> > +	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
>
> You should remove the files before detaching the client.

Fixed.

>
> > +	kfree(data);
>
> Do not free the memory if you failed to detach the client!

Fixed.

>
> > +	return err;
> > +}
> > +
> > +static int max6650_init_client(struct i2c_client *client)
> > +{
> > +	struct max6650_data *data = i2c_get_clientdata(client);
> > +	const char* mode_strings[4] = {"full-on", "always-off",
> > +					"closed-loop", "open-loop"};
> > +	int config, countreg;
> > +	int err = -ENODEV;
>
> The only errors that can happen here are I/O errors, so that would be
> -EIO.

Fixed.

>
> > +
> > +	mutex_lock(&data->update_lock);
>
> You don't really need to take the mutex here - there's nobody else who
> can access your data at this point.

Fixed.


> > +
> > +	dev_info(&client->dev, "Prescaler is set to %d.\n",
> > +		 DIV_FROM_REG(config) );
>
> This is confusing, you're using DIV_FROM_REG for both the prescaler and
> the counttime. It works more or less by accident...

Right. Fixed.


> > +static struct max6650_data *max6650_update_device(struct device *dev)
> > +{
> > +	int i;
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct max6650_data *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->update_lock);
> > +
> > +	if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> > +		data->speed = i2c_smbus_read_byte_data(client,
> > +			 			       MAX6650_REG_SPEED);
> > +		data->config = i2c_smbus_read_byte_data(client,
> > +							MAX6650_REG_CONFIG);
> > +		for (i = 0; i < 4; i++) {
> > +			data->tach[i] = i2c_smbus_read_byte_data(client,
> > +								 tach_reg[i]);
> > +		}
> > +		data->count = i2c_smbus_read_byte_data (client,
> > +							MAX6650_REG_COUNT);
>
> Extra space before opening parenthesis.

Fixed.

> > +}
> > +
> > +MODULE_AUTHOR("john.morris at spirentcom.com");
>
> This is more or less your driver now... 

True. There's not much left of the original code. I made myself MODULE_AUTHOR 
and gave credit to the original authors in the heading.

> BTW (I can't remember if I told 
> you already) feel free to add yourself to MAINTAINERS as the maintainer
> for this new driver.

Thank you. Done.

BTW, the patch is against 2.6.21-rc3 now.

Thanks,
Hans

Signed-off-by: Hans J. Koch <hjk at linutronix.de>



-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 24400 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070316/d60b3806/attachment-0001.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

Thanks for the quick update.

On Fri, 16 Mar 2007 17:44:32 +0100, Hans-J?rgen Koch wrote:
> > I don't understand. For one thing, if you know how to turn the fan off,
> > then you could do it when fan1_target is set to 0, it's just a matter
> > of adding a few more lines of code. For another, fan1_target is related
> > to the closed loop mode (pwm1_enable=2) while the fan off mode would
> > generally be implemented as part of the open loop mode: pwm1_enable=1
> > and pwm1=0. But I see that you don't have a pwm1 file, so... How does
> > the open loop mode work at all?
> 
> Open loop mode just sets the ouput voltage according to a DAC value (0..255).
> If someone needs a defined speed, he's supposed to do the regulator in 
> software. It is not influenced by the speed register used in closed loop 
> mode. I added a pwm1 file to read/write that DAC value. After doing that, I 
> noticed the fan can be brought to full/zero speed by setting pwm1 to 0 or 
> 255, respectively. That's OK for me.
> For pwm1_enable, I only use the well defined values now: 0=off, 1=open loop, 
> 2=closed loop. If I find the chip in "full on" mode at load time, I change 
> mode to open loop and set it to full speed. I hope this always works and 
> won't set somebodies board on fire :-)
> 
> BTW, meanwhile I'm a bit confused about what pwm1_enable=0 means. Is it "fan 
> off" or "PWM off, fan full on"? At the moment, I implemented "fan off", but I 
> can easily change that. The other way round would be slightly simpler.

pwm1_enable=0 means fan full on, so you'll have to update your code and
documentation a bit, sorry.

You should not need to change anything at module load time.

> /* fan_voltage: 5=5V fan, 12\x12V fan, 0=don't change */
> static int fan_voltage = 0;
> /* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */
> static int prescaler = 0;

The trick is to _not_ initialize these variables at all. The compiler
will put them in a special section and will zero them all at once
automatically.

> static int max6650_attach_adapter(struct i2c_adapter *adapter)
> {
> 	if (!(adapter->class & I2C_CLASS_HWMON)) {
> 		dev_err(&adapter->dev,
> 			"FATAL: max6650_attach_adapter class HWMON not set\n");
> 		return 0;
> 	}
> 
> 	return i2c_probe(adapter, &addr_data, max6650_detect);
> }

Please make it a dev_dbg(). There are more than one I2C bus on most
systems, and usually only one with I2C_CLASS_HWMON set, on purpose. You
don't want to generate an error message in the logs for every other I2C
bus.

> 	if ((kind < 0)&&
> 	   (  (i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG) & 0xC0)
> 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_GPIO_STAT) & 0xE0)
> 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN) & 0xE0)
> 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM) & 0xE0)
> 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT) & 0xFC)))
> 	{
> 		dev_dbg(&adapter->dev,
> 			"max6650: detection failed at 0x%02x.\n", address);
> 		goto err_free;
> 	}

Opening curly brace is misplaced.

> static const u8 tach_reg[] > {
> 	MAX6650_REG_TACH0,
> 	MAX6650_REG_TACH1,
> 	MAX6650_REG_TACH2,
> 	MAX6650_REG_TACH3,
> };

And same here.

Other than that I am very happy with this latest version. Great job!
Thanks for your patience, I know it's always a bit frustrating when
your code works well enough for yourself and you are still told to make
many changes before it is acceptable upstream.

Can you please test the user-space support now? I suspect that we need
to update libsensors at least to match the name change of the "speed"
file. We're about to release lm_sensors 2.10.3 (in a few days now) and
I'd like it to support your new driver properly.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Freitag, 16. M?rz 2007 20:17 schrieb Jean Delvare:
>
> pwm1_enable=0 means fan full on, so you'll have to update your code and
> documentation a bit, sorry.

No problem, wasn't much work.

>
> You should not need to change anything at module load time.

Well, MAX6650 knows 4 modes, we know 3, so there's always one mode the MAX6650 
could be in that we have to map to one of our known modes. In that case I 
map "full off" to "open loop" and set the output to zero (dac%5).

>
> > /* fan_voltage: 5=5V fan, 12\x12V fan, 0=don't change */
> > static int fan_voltage = 0;
> > /* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */
> > static int prescaler = 0;
>
> The trick is to _not_ initialize these variables at all. The compiler
> will put them in a special section and will zero them all at once
> automatically.

Sounds dangerous. But I trust you :-)

>
> > static int max6650_attach_adapter(struct i2c_adapter *adapter)
> > {
> > 	if (!(adapter->class & I2C_CLASS_HWMON)) {
> > 		dev_err(&adapter->dev,
> > 			"FATAL: max6650_attach_adapter class HWMON not set\n");
> > 		return 0;
> > 	}
> >
> > 	return i2c_probe(adapter, &addr_data, max6650_detect);
> > }
>
> Please make it a dev_dbg(). There are more than one I2C bus on most
> systems, and usually only one with I2C_CLASS_HWMON set, on purpose. You
> don't want to generate an error message in the logs for every other I2C
> bus.

Already noticed that, but didn't know the explanation. Fixed.

>
> > 	if ((kind < 0)&&
> > 	   (  (i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG) & 0xC0)
> >
> > 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_GPIO_STAT) & 0xE0)
> > 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM_EN) & 0xE0)
> > 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_ALARM) & 0xE0)
> > 	    ||(i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT) & 0xFC)))
> >
> > 	{
> > 		dev_dbg(&adapter->dev,
> > 			"max6650: detection failed at 0x%02x.\n", address);
> > 		goto err_free;
> > 	}
>
> Opening curly brace is misplaced.

I know, I know. IMHO, with the curly brace placed correctly, it looks ugly. 
But for the sake of coding style, I fixed it. I hoped you wouldn't notice ;-)

>
> > static const u8 tach_reg[] > > {
> > 	MAX6650_REG_TACH0,
> > 	MAX6650_REG_TACH1,
> > 	MAX6650_REG_TACH2,
> > 	MAX6650_REG_TACH3,
> > };
>
> And same here.

OK, I missed that one.

>
> Other than that I am very happy with this latest version. Great job!
> Thanks for your patience, I know it's always a bit frustrating when
> your code works well enough for yourself and you are still told to make
> many changes before it is acceptable upstream.

Well, I really appreciate good code quality. If this is the price, I'm willing 
to pay it. Actually, I thank you for helping me so much.

>
> Can you please test the user-space support now? I suspect that we need
> to update libsensors at least to match the name change of the "speed"
> file. We're about to release lm_sensors 2.10.3 (in a few days now) and
> I'd like it to support your new driver properly.

Yes, but not so soon, sorry. I'm not at home the whole next week and will not 
have any sensors hardware available. After that, I might find some time to 
have a look. I have an LM93 driver almost ready, so that'll be the next thing 
I'll keep you busy with :-) In the course of that development I can have a 
short look at user space stuff, too.

Thanks,

Hans

-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6650.patch
Type: text/x-diff
Size: 24447 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070316/74f5c1dd/attachment-0001.bin 

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Freitag, 16. M?rz 2007 22:19 schrieb Thomas Gleixner:
> On Fri, 2007-03-16 at 22:07 +0100, Hans-J?rgen Koch wrote:
> > > > /* fan_voltage: 5=5V fan, 12\x12V fan, 0=don't change */
> > > > static int fan_voltage = 0;
> > > > /* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change
> > > > */ static int prescaler = 0;
> > >
> > > The trick is to _not_ initialize these variables at all. The compiler
> > > will put them in a special section and will zero them all at once
> > > automatically.
> >
> > Sounds dangerous. But I trust you :-)
>
> It's one of the things which actually work reliable in gcc :)
>
> 	tglx

Fine, I'll remember that. But I think readability is better if you explicitly 
initialize to zero. Is that optimization really worth it?

Hans

[lm-sensors] [PATCH] Add MAX6650 support

[Thomas Gleixner]

On Fri, 2007-03-16 at 22:07 +0100, Hans-J?rgen Koch wrote:
> > > /* fan_voltage: 5=5V fan, 12\x12V fan, 0=don't change */
> > > static int fan_voltage = 0;
> > > /* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change */
> > > static int prescaler = 0;
> >
> > The trick is to _not_ initialize these variables at all. The compiler
> > will put them in a special section and will zero them all at once
> > automatically.
> 
> Sounds dangerous. But I trust you :-)

It's one of the things which actually work reliable in gcc :)

	tglx

[lm-sensors] [PATCH] Add MAX6650 support

[Thomas Gleixner]

On Fri, 2007-03-16 at 22:19 +0100, Hans-J?rgen Koch wrote:
> Am Freitag, 16. M?rz 2007 22:19 schrieb Thomas Gleixner:
> > On Fri, 2007-03-16 at 22:07 +0100, Hans-J?rgen Koch wrote:
> > > > > /* fan_voltage: 5=5V fan, 12\x12V fan, 0=don't change */
> > > > > static int fan_voltage = 0;
> > > > > /* prescaler: Possible values are 1,2,4,8,16, or 0 for don't change
> > > > > */ static int prescaler = 0;
> > > >
> > > > The trick is to _not_ initialize these variables at all. The compiler
> > > > will put them in a special section and will zero them all at once
> > > > automatically.
> > >
> > > Sounds dangerous. But I trust you :-)
> >
> > It's one of the things which actually work reliable in gcc :)
> >
> > 	tglx
> 
> Fine, I'll remember that. But I think readability is better if you explicitly 
> initialize to zero. Is that optimization really worth it?

The zeroed segment is set with memset() in one go, while all explicit
initializations carry additional code. gcc might be clever enough now to
move a "=0" initialized variable to the zeroed segment, but it is common
style since ever. And it removes _three_ characters per variable :)

	tglx

[lm-sensors] [PATCH] Add MAX6650 support

[Jean Delvare]

Hi Hans-J?rgen,

On Fri, 16 Mar 2007 22:07:52 +0100, Hans-J?rgen Koch wrote:
> Well, I really appreciate good code quality. If this is the price, I'm willing 
> to pay it. Actually, I thank you for helping me so much.

Patch accepted, will be in next -mm, and from there will go to 2.6.22.
Thanks :)

> > Can you please test the user-space support now? I suspect that we need
> > to update libsensors at least to match the name change of the "speed"
> > file. We're about to release lm_sensors 2.10.3 (in a few days now) and
> > I'd like it to support your new driver properly.
> 
> Yes, but not so soon, sorry. I'm not at home the whole next week and will not 
> have any sensors hardware available. After that, I might find some time to 
> have a look. I have an LM93 driver almost ready, so that'll be the next thing 
> I'll keep you busy with :-) In the course of that development I can have a 
> short look at user space stuff, too.

I've made the minimum fix for the 2.10.3 release, so that at least no
error is displayed. Feel free to propose further improvements later.
I've also updated the Devices on our wiki to reflect the current state
of the MAX6650/MAX6651 support.

If you start working on the lm93 driver, make sure to search the
mailing list archives first. I remember that the driver was already
ported to Linux 2.6, but never reviewed. This was some times ago
though, many things changed in the kernel meanwhile, so it's up to you
whether you want to start from an old port or from scratch. Beware, the
lm93 driver is a fat beast.

-- 
Jean Delvare

[lm-sensors] [PATCH] Add MAX6650 support

[Hans-Jürgen Koch]

Am Samstag, 17. M?rz 2007 14:39 schrieb Jean Delvare:
>
> Patch accepted, will be in next -mm, and from there will go to 2.6.22.
> Thanks :)

I thank you!

>
> If you start working on the lm93 driver, make sure to search the
> mailing list archives first. I remember that the driver was already
> ported to Linux 2.6, but never reviewed. This was some times ago
> though, many things changed in the kernel meanwhile, so it's up to you
> whether you want to start from an old port or from scratch. 

I won't start from scratch, I have an at least partly working driver. Somebody 
made it apply to current kernels, but after a first quick glance at the code 
I'd say it needs quite a bit of work to make it acceptable for mainline.

> Beware, the 
> lm93 driver is a fat beast.

I already noticed :-)

Thanks,
Hans