[PATCH v2 0/7] hwmon: Add support for MAX6695/6696 to lm90 driver

[PATCH v2 0/7] hwmon: Add support for MAX6695/6696 to lm90 driver

[Guenter Roeck]

This patchset cleans up the lm90 driver and adds support for
Maxim MAX6695/6696 temperature sensors.

In detail,
- Add support for tempX_emergency_alarm ABI attribute
- Introduce device feature bits to reduce runtime overhead
- Introduce function to delete sysfs files
- Simplify calculations in set_temp11()
- Introduce 3rd set of upper temperature limits (named 'emergency' limits)
- Add max6659 support for those limits, and add support for max6659 address 0x4e
- Add max6695/96 support

The patchset depends on the previously submitted lm90 driver cleanup patch.

v2 changes:
- Split patch into multiple parts
- Add support for max6659 address 0x4e and extra features
- Improved max6695/96 device detection code
- max6695/96 does not support extended external limits, so don't try to
  read them for the 3rd sensor
- Other changes to address review feedback
  - Updated Documentation/hwmon/lm90 and drivers/hwmon/Kconfig.
  - Fixed alert_alarms comment
  - remote offset register not supported on max6658 (fixed comment)
  - rewrote set_temp11()
  - Added tempX_emergency_alarm
  - Some code alignment fixes

[PATCH v2 1/7] hwmon: Add tempX_emergency_alarm attribute to sysfs ABI

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 Documentation/hwmon/sysfs-interface |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
index 101300b..22843cb 100644
--- a/Documentation/hwmon/sysfs-interface
+++ b/Documentation/hwmon/sysfs-interface
@@ -513,6 +513,7 @@ fan[1-*]_max_alarm
 temp[1-*]_min_alarm
 temp[1-*]_max_alarm
 temp[1-*]_crit_alarm
+temp[1-*]_emergency_alarm
 		Limit alarm
 		0: no alarm
 		1: alarm
-- 
1.7.0.87.g0901d

Re: [PATCH v2 1/7] hwmon: Add tempX_emergency_alarm attribute to sysfs ABI

[Jean Delvare]

On Thu, 9 Sep 2010 06:25:44 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  Documentation/hwmon/sysfs-interface |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> index 101300b..22843cb 100644
> --- a/Documentation/hwmon/sysfs-interface
> +++ b/Documentation/hwmon/sysfs-interface
> @@ -513,6 +513,7 @@ fan[1-*]_max_alarm
>  temp[1-*]_min_alarm
>  temp[1-*]_max_alarm
>  temp[1-*]_crit_alarm
> +temp[1-*]_emergency_alarm
>  		Limit alarm
>  		0: no alarm
>  		1: alarm

Looks good, I'll merge this into the following patch:
ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/hwmon-add-tempx_emergency-attribute-to-sysfs-abi.patch

as they make more sense together than separated.

-- 
Jean Delvare

Re: [PATCH v2 1/7] hwmon: Add tempX_emergency_alarm attribute to sysfs ABI

[Guenter Roeck]

On Mon, Sep 13, 2010 at 12:17:18PM -0400, Jean Delvare wrote:
> On Thu, 9 Sep 2010 06:25:44 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  Documentation/hwmon/sysfs-interface |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/sysfs-interface b/Documentation/hwmon/sysfs-interface
> > index 101300b..22843cb 100644
> > --- a/Documentation/hwmon/sysfs-interface
> > +++ b/Documentation/hwmon/sysfs-interface
> > @@ -513,6 +513,7 @@ fan[1-*]_max_alarm
> >  temp[1-*]_min_alarm
> >  temp[1-*]_max_alarm
> >  temp[1-*]_crit_alarm
> > +temp[1-*]_emergency_alarm
> >  		Limit alarm
> >  		0: no alarm
> >  		1: alarm
> 
> Looks good, I'll merge this into the following patch:
> ftp://ftp.kernel.org/pub/linux/kernel/people/jdelvare/linux-2.6/jdelvare-hwmon/hwmon-add-tempx_emergency-attribute-to-sysfs-abi.patch
> 
> as they make more sense together than separated.
> 
Agreed.

Guenter

[PATCH v2 2/7] hwmon: (lm90) Introduce device feature bits

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/lm90.c |   38 +++++++++++++++++++++++++-------------
 1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index aafed28..a81a053 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -142,7 +142,11 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
 /*
  * Device flags
  */
-#define LM90_FLAG_ADT7461_EXT		0x01	/* ADT7461 extended mode */
+#define LM90_FLAG_ADT7461_EXT	0x01	/* ADT7461 extended mode	*/
+/* Device features */
+#define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
+#define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
+#define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
 
 /*
  * Functions declaration
@@ -462,17 +466,16 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
 		data->temp11[nr] = temp_to_u16_adt7461(data, val);
-	else if (data->kind == max6657 || data->kind == max6680)
-		data->temp11[nr] = temp_to_s8(val) << 8;
 	else if (data->kind == max6646)
 		data->temp11[nr] = temp_to_u8(val) << 8;
+	else if (!(data->flags & LM90_HAVE_REM_LIMIT_EXT))
+		data->temp11[nr] = temp_to_s8(val) << 8;
 	else
 		data->temp11[nr] = temp_to_s16(val);
 
 	i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
 				  data->temp11[nr] >> 8);
-	if (data->kind != max6657 && data->kind != max6680
-	    && data->kind != max6646)
+	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
 		i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
 					  data->temp11[nr] & 0xff);
 	mutex_unlock(&data->update_lock);
@@ -847,6 +850,17 @@ static int lm90_probe(struct i2c_client *new_client,
 		break;
 	}
 
+	/* Set chip capabilities */
+	if (data->kind != max6657 && data->kind != max6646)
+		data->flags |= LM90_HAVE_OFFSET;
+
+	if (data->kind == max6657 || data->kind == max6646)
+		data->flags |= LM90_HAVE_LOCAL_EXT;
+
+	if (data->kind != max6657 && data->kind != max6646
+	    && data->kind != max6680)
+		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
+
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
 
@@ -859,7 +873,7 @@ static int lm90_probe(struct i2c_client *new_client,
 		if (err)
 			goto exit_remove_files;
 	}
-	if (data->kind != max6657 && data->kind != max6646) {
+	if (data->flags & LM90_HAVE_OFFSET) {
 		err = device_create_file(&new_client->dev,
 					&sensor_dev_attr_temp2_offset.dev_attr);
 		if (err)
@@ -925,7 +939,7 @@ static int lm90_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm90_group);
 	device_remove_file(&client->dev, &dev_attr_pec);
-	if (data->kind != max6657 && data->kind != max6646)
+	if (data->flags & LM90_HAVE_OFFSET)
 		device_remove_file(&client->dev,
 				   &sensor_dev_attr_temp2_offset.dev_attr);
 
@@ -1019,7 +1033,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
 		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
 
-		if (data->kind == max6657 || data->kind == max6646) {
+		if (data->flags & LM90_HAVE_LOCAL_EXT) {
 			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
 				    MAX6657_REG_R_LOCAL_TEMPL,
 				    &data->temp11[4]);
@@ -1033,22 +1047,20 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
 			data->temp11[1] = h << 8;
-			if (data->kind != max6657 && data->kind != max6680
-			 && data->kind != max6646
+			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
 					  &l) == 0)
 				data->temp11[1] |= l;
 		}
 		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
 			data->temp11[2] = h << 8;
-			if (data->kind != max6657 && data->kind != max6680
-			 && data->kind != max6646
+			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
 					  &l) == 0)
 				data->temp11[2] |= l;
 		}
 
-		if (data->kind != max6657 && data->kind != max6646) {
+		if (data->flags & LM90_HAVE_OFFSET) {
 			if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
 					  &h) == 0
 			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
-- 
1.7.0.87.g0901d

Re: [PATCH v2 2/7] hwmon: (lm90) Introduce device feature bits

[Jean Delvare]

On Thu, 9 Sep 2010 06:25:45 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/lm90.c |   38 +++++++++++++++++++++++++-------------
>  1 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index aafed28..a81a053 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -142,7 +142,11 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
>  /*
>   * Device flags
>   */
> -#define LM90_FLAG_ADT7461_EXT		0x01	/* ADT7461 extended mode */
> +#define LM90_FLAG_ADT7461_EXT	0x01	/* ADT7461 extended mode	*/
> +/* Device features */
> +#define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
> +#define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
> +#define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/

Please always use a space, not a tab, between #define and the symbol
name.

When defining bit values, I suggest using the following notation:

#define LM90_HAVE_OFFSET	(1 << 2)	/* temperature offset register	*/
#define LM90_HAVE_LOCAL_EXT	(1 << 3)	/* extended local temperature	*/

It is more immediately obvious that you have a bit vector.

>  
>  /*
>   * Functions declaration
> @@ -462,17 +466,16 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  	mutex_lock(&data->update_lock);
>  	if (data->kind == adt7461)
>  		data->temp11[nr] = temp_to_u16_adt7461(data, val);
> -	else if (data->kind == max6657 || data->kind == max6680)
> -		data->temp11[nr] = temp_to_s8(val) << 8;
>  	else if (data->kind == max6646)
>  		data->temp11[nr] = temp_to_u8(val) << 8;
> +	else if (!(data->flags & LM90_HAVE_REM_LIMIT_EXT))

It would be more efficient to swap the last two statements to avoid the
negation.

> +		data->temp11[nr] = temp_to_s8(val) << 8;
>  	else
>  		data->temp11[nr] = temp_to_s16(val);
>  
>  	i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
>  				  data->temp11[nr] >> 8);
> -	if (data->kind != max6657 && data->kind != max6680
> -	    && data->kind != max6646)
> +	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  		i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
>  					  data->temp11[nr] & 0xff);
>  	mutex_unlock(&data->update_lock);
> @@ -847,6 +850,17 @@ static int lm90_probe(struct i2c_client *new_client,
>  		break;
>  	}
>  
> +	/* Set chip capabilities */
> +	if (data->kind != max6657 && data->kind != max6646)
> +		data->flags |= LM90_HAVE_OFFSET;
> +
> +	if (data->kind == max6657 || data->kind == max6646)
> +		data->flags |= LM90_HAVE_LOCAL_EXT;
> +
> +	if (data->kind != max6657 && data->kind != max6646
> +	    && data->kind != max6680)
> +		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
> +
>  	/* Initialize the LM90 chip */
>  	lm90_init_client(new_client);
>  
> @@ -859,7 +873,7 @@ static int lm90_probe(struct i2c_client *new_client,
>  		if (err)
>  			goto exit_remove_files;
>  	}
> -	if (data->kind != max6657 && data->kind != max6646) {
> +	if (data->flags & LM90_HAVE_OFFSET) {
>  		err = device_create_file(&new_client->dev,
>  					&sensor_dev_attr_temp2_offset.dev_attr);
>  		if (err)
> @@ -925,7 +939,7 @@ static int lm90_remove(struct i2c_client *client)
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&client->dev.kobj, &lm90_group);
>  	device_remove_file(&client->dev, &dev_attr_pec);
> -	if (data->kind != max6657 && data->kind != max6646)
> +	if (data->flags & LM90_HAVE_OFFSET)
>  		device_remove_file(&client->dev,
>  				   &sensor_dev_attr_temp2_offset.dev_attr);
>  
> @@ -1019,7 +1033,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
>  		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
>  
> -		if (data->kind == max6657 || data->kind == max6646) {
> +		if (data->flags & LM90_HAVE_LOCAL_EXT) {
>  			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
>  				    MAX6657_REG_R_LOCAL_TEMPL,
>  				    &data->temp11[4]);
> @@ -1033,22 +1047,20 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  
>  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
>  			data->temp11[1] = h << 8;
> -			if (data->kind != max6657 && data->kind != max6680
> -			 && data->kind != max6646
> +			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
>  					  &l) == 0)
>  				data->temp11[1] |= l;
>  		}
>  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
>  			data->temp11[2] = h << 8;
> -			if (data->kind != max6657 && data->kind != max6680
> -			 && data->kind != max6646
> +			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
>  					  &l) == 0)
>  				data->temp11[2] |= l;
>  		}
>  
> -		if (data->kind != max6657 && data->kind != max6646) {
> +		if (data->flags & LM90_HAVE_OFFSET) {
>  			if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
>  					  &h) == 0
>  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,

Other than these minor implementation details, the changes look good, I
like them.

-- 
Jean Delvare

Re: [PATCH v2 2/7] hwmon: (lm90) Introduce device feature bits

[Guenter Roeck]

Hi Jean,

On Mon, Sep 13, 2010 at 03:30:30PM -0400, Jean Delvare wrote:
> On Thu, 9 Sep 2010 06:25:45 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/lm90.c |   38 +++++++++++++++++++++++++-------------
> >  1 files changed, 25 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index aafed28..a81a053 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -142,7 +142,11 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> >  /*
> >   * Device flags
> >   */
> > -#define LM90_FLAG_ADT7461_EXT		0x01	/* ADT7461 extended mode */
> > +#define LM90_FLAG_ADT7461_EXT	0x01	/* ADT7461 extended mode	*/
> > +/* Device features */
> > +#define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
> > +#define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
> > +#define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
> 
> Please always use a space, not a tab, between #define and the symbol
> name.
> 
Ok.

> When defining bit values, I suggest using the following notation:
> 
> #define LM90_HAVE_OFFSET	(1 << 2)	/* temperature offset register	*/
> #define LM90_HAVE_LOCAL_EXT	(1 << 3)	/* extended local temperature	*/
> 
> It is more immediately obvious that you have a bit vector.
> 
Makes sense. I often do, but didn't here for formatting reasons (getting close to 80 columns).
You'll see the impact in v3 of the patch.

> >  
> >  /*
> >   * Functions declaration
> > @@ -462,17 +466,16 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
> >  	mutex_lock(&data->update_lock);
> >  	if (data->kind == adt7461)
> >  		data->temp11[nr] = temp_to_u16_adt7461(data, val);
> > -	else if (data->kind == max6657 || data->kind == max6680)
> > -		data->temp11[nr] = temp_to_s8(val) << 8;
> >  	else if (data->kind == max6646)
> >  		data->temp11[nr] = temp_to_u8(val) << 8;
> > +	else if (!(data->flags & LM90_HAVE_REM_LIMIT_EXT))
> 
> It would be more efficient to swap the last two statements to avoid the
> negation.
> 
Not sure about efficiency, but the code looks better that way. Done.

> > +		data->temp11[nr] = temp_to_s8(val) << 8;
> >  	else
> >  		data->temp11[nr] = temp_to_s16(val);
> >  
> >  	i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> >  				  data->temp11[nr] >> 8);
> > -	if (data->kind != max6657 && data->kind != max6680
> > -	    && data->kind != max6646)
> > +	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
> >  		i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> >  					  data->temp11[nr] & 0xff);
> >  	mutex_unlock(&data->update_lock);
> > @@ -847,6 +850,17 @@ static int lm90_probe(struct i2c_client *new_client,
> >  		break;
> >  	}
> >  
> > +	/* Set chip capabilities */
> > +	if (data->kind != max6657 && data->kind != max6646)
> > +		data->flags |= LM90_HAVE_OFFSET;
> > +
> > +	if (data->kind == max6657 || data->kind == max6646)
> > +		data->flags |= LM90_HAVE_LOCAL_EXT;
> > +
> > +	if (data->kind != max6657 && data->kind != max6646
> > +	    && data->kind != max6680)
> > +		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
> > +
> >  	/* Initialize the LM90 chip */
> >  	lm90_init_client(new_client);
> >  
> > @@ -859,7 +873,7 @@ static int lm90_probe(struct i2c_client *new_client,
> >  		if (err)
> >  			goto exit_remove_files;
> >  	}
> > -	if (data->kind != max6657 && data->kind != max6646) {
> > +	if (data->flags & LM90_HAVE_OFFSET) {
> >  		err = device_create_file(&new_client->dev,
> >  					&sensor_dev_attr_temp2_offset.dev_attr);
> >  		if (err)
> > @@ -925,7 +939,7 @@ static int lm90_remove(struct i2c_client *client)
> >  	hwmon_device_unregister(data->hwmon_dev);
> >  	sysfs_remove_group(&client->dev.kobj, &lm90_group);
> >  	device_remove_file(&client->dev, &dev_attr_pec);
> > -	if (data->kind != max6657 && data->kind != max6646)
> > +	if (data->flags & LM90_HAVE_OFFSET)
> >  		device_remove_file(&client->dev,
> >  				   &sensor_dev_attr_temp2_offset.dev_attr);
> >  
> > @@ -1019,7 +1033,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >  		lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT, &data->temp8[3]);
> >  		lm90_read_reg(client, LM90_REG_R_TCRIT_HYST, &data->temp_hyst);
> >  
> > -		if (data->kind == max6657 || data->kind == max6646) {
> > +		if (data->flags & LM90_HAVE_LOCAL_EXT) {
> >  			lm90_read16(client, LM90_REG_R_LOCAL_TEMP,
> >  				    MAX6657_REG_R_LOCAL_TEMPL,
> >  				    &data->temp11[4]);
> > @@ -1033,22 +1047,20 @@ static struct lm90_data *lm90_update_device(struct device *dev)
> >  
> >  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h) == 0) {
> >  			data->temp11[1] = h << 8;
> > -			if (data->kind != max6657 && data->kind != max6680
> > -			 && data->kind != max6646
> > +			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
> >  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_LOWL,
> >  					  &l) == 0)
> >  				data->temp11[1] |= l;
> >  		}
> >  		if (lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h) == 0) {
> >  			data->temp11[2] = h << 8;
> > -			if (data->kind != max6657 && data->kind != max6680
> > -			 && data->kind != max6646
> > +			if ((data->flags & LM90_HAVE_REM_LIMIT_EXT)
> >  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHL,
> >  					  &l) == 0)
> >  				data->temp11[2] |= l;
> >  		}
> >  
> > -		if (data->kind != max6657 && data->kind != max6646) {
> > +		if (data->flags & LM90_HAVE_OFFSET) {
> >  			if (lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSH,
> >  					  &h) == 0
> >  			 && lm90_read_reg(client, LM90_REG_R_REMOTE_OFFSL,
> 
> Other than these minor implementation details, the changes look good, I
> like them.
> 
As always, excellent feedback. Thanks a lot!

Guenter

[PATCH v2 3/7] hwmon: (lm90) Introduce function to delete sysfs files

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/lm90.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index a81a053..f21dde5 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -815,6 +815,15 @@ static int lm90_detect(struct i2c_client *new_client,
 	return 0;
 }
 
+static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
+{
+	if (data->flags & LM90_HAVE_OFFSET)
+		device_remove_file(&client->dev,
+				   &sensor_dev_attr_temp2_offset.dev_attr);
+	device_remove_file(&client->dev, &dev_attr_pec);
+	sysfs_remove_group(&client->dev.kobj, &lm90_group);
+}
+
 static int lm90_probe(struct i2c_client *new_client,
 		      const struct i2c_device_id *id)
 {
@@ -889,8 +898,7 @@ static int lm90_probe(struct i2c_client *new_client,
 	return 0;
 
 exit_remove_files:
-	sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
-	device_remove_file(&new_client->dev, &dev_attr_pec);
+	lm90_remove_files(new_client, data);
 exit_free:
 	kfree(data);
 exit:
@@ -937,11 +945,7 @@ static int lm90_remove(struct i2c_client *client)
 	struct lm90_data *data = i2c_get_clientdata(client);
 
 	hwmon_device_unregister(data->hwmon_dev);
-	sysfs_remove_group(&client->dev.kobj, &lm90_group);
-	device_remove_file(&client->dev, &dev_attr_pec);
-	if (data->flags & LM90_HAVE_OFFSET)
-		device_remove_file(&client->dev,
-				   &sensor_dev_attr_temp2_offset.dev_attr);
+	lm90_remove_files(client, data);
 
 	/* Restore initial configuration */
 	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
-- 
1.7.0.87.g0901d

Re: [PATCH v2 3/7] hwmon: (lm90) Introduce function to delete sysfs files

[Jean Delvare]

On Thu, 9 Sep 2010 06:25:46 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/lm90.c |   18 +++++++++++-------
>  1 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index a81a053..f21dde5 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -815,6 +815,15 @@ static int lm90_detect(struct i2c_client *new_client,
>  	return 0;
>  }
>  
> +static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
> +{
> +	if (data->flags & LM90_HAVE_OFFSET)
> +		device_remove_file(&client->dev,
> +				   &sensor_dev_attr_temp2_offset.dev_attr);
> +	device_remove_file(&client->dev, &dev_attr_pec);
> +	sysfs_remove_group(&client->dev.kobj, &lm90_group);
> +}
> +
>  static int lm90_probe(struct i2c_client *new_client,
>  		      const struct i2c_device_id *id)
>  {
> @@ -889,8 +898,7 @@ static int lm90_probe(struct i2c_client *new_client,
>  	return 0;
>  
>  exit_remove_files:
> -	sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
> -	device_remove_file(&new_client->dev, &dev_attr_pec);
> +	lm90_remove_files(new_client, data);
>  exit_free:
>  	kfree(data);
>  exit:
> @@ -937,11 +945,7 @@ static int lm90_remove(struct i2c_client *client)
>  	struct lm90_data *data = i2c_get_clientdata(client);
>  
>  	hwmon_device_unregister(data->hwmon_dev);
> -	sysfs_remove_group(&client->dev.kobj, &lm90_group);
> -	device_remove_file(&client->dev, &dev_attr_pec);
> -	if (data->flags & LM90_HAVE_OFFSET)
> -		device_remove_file(&client->dev,
> -				   &sensor_dev_attr_temp2_offset.dev_attr);
> +	lm90_remove_files(client, data);
>  
>  	/* Restore initial configuration */
>  	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,

Looks good. Applied, thanks!

-- 
Jean Delvare

[PATCH v2 4/7] hwmon: (lm90) Simplify set_temp11 register calculations

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/lm90.c |   55 +++++++++++++++++++++++++------------------------
 1 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index f21dde5..11b5701 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -418,7 +418,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
 			   char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct lm90_data *data = lm90_update_device(dev);
 	int temp;
 
@@ -439,19 +439,20 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
-	static const u8 reg[6] = {
-		LM90_REG_W_REMOTE_LOWH,
-		LM90_REG_W_REMOTE_LOWL,
-		LM90_REG_W_REMOTE_HIGHH,
-		LM90_REG_W_REMOTE_HIGHL,
-		LM90_REG_W_REMOTE_OFFSH,
-		LM90_REG_W_REMOTE_OFFSL,
+	struct {
+		u8 high;
+		u8 low;
+	} reg[3] = {
+		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
+		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL },
+		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL }
 	};
 
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
 	struct i2c_client *client = to_i2c_client(dev);
 	struct lm90_data *data = i2c_get_clientdata(client);
-	int nr = attr->index;
+	int nr = attr->nr;
+	int index = attr->index;
 	long val;
 	int err;
 
@@ -460,24 +461,24 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 		return err;
 
 	/* +16 degrees offset for temp2 for the LM99 */
-	if (data->kind == lm99 && attr->index <= 2)
+	if (data->kind == lm99 && index <= 2)
 		val -= 16000;
 
 	mutex_lock(&data->update_lock);
 	if (data->kind == adt7461)
-		data->temp11[nr] = temp_to_u16_adt7461(data, val);
+		data->temp11[index] = temp_to_u16_adt7461(data, val);
 	else if (data->kind == max6646)
-		data->temp11[nr] = temp_to_u8(val) << 8;
+		data->temp11[index] = temp_to_u8(val) << 8;
 	else if (!(data->flags & LM90_HAVE_REM_LIMIT_EXT))
-		data->temp11[nr] = temp_to_s8(val) << 8;
+		data->temp11[index] = temp_to_s8(val) << 8;
 	else
-		data->temp11[nr] = temp_to_s16(val);
+		data->temp11[index] = temp_to_s16(val);
 
-	i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
-				  data->temp11[nr] >> 8);
+	i2c_smbus_write_byte_data(client, reg[nr].high,
+				  data->temp11[index] >> 8);
 	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
-		i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
-					  data->temp11[nr] & 0xff);
+		i2c_smbus_write_byte_data(client, reg[nr].low,
+					  data->temp11[index] & 0xff);
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -549,16 +550,16 @@ static ssize_t show_alarm(struct device *dev, struct device_attribute
 	return sprintf(buf, "%d\n", (data->alarms >> bitnr) & 1);
 }
 
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp11, NULL, 4);
-static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp11, NULL, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
+static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
 static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
 	set_temp8, 0);
-static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
+	set_temp11, 0, 1);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
 	set_temp8, 1);
-static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 2);
+static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
+	set_temp11, 1, 2);
 static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
 	set_temp8, 2);
 static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
@@ -566,8 +567,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
 static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
 	set_temphyst, 2);
 static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
-static SENSOR_DEVICE_ATTR(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
-	set_temp11, 3);
+static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
+	set_temp11, 2, 3);
 
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);
-- 
1.7.0.87.g0901d

Re: [PATCH v2 4/7] hwmon: (lm90) Simplify set_temp11 register calculations

[Jean Delvare]

On Thu, 9 Sep 2010 06:25:47 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/lm90.c |   55 +++++++++++++++++++++++++------------------------
>  1 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index f21dde5..11b5701 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -418,7 +418,7 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>  static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>  			   char *buf)
>  {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>  	struct lm90_data *data = lm90_update_device(dev);
>  	int temp;
>  
> @@ -439,19 +439,20 @@ static ssize_t show_temp11(struct device *dev, struct device_attribute *devattr,
>  static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  			  const char *buf, size_t count)
>  {
> -	static const u8 reg[6] = {
> -		LM90_REG_W_REMOTE_LOWH,
> -		LM90_REG_W_REMOTE_LOWL,
> -		LM90_REG_W_REMOTE_HIGHH,
> -		LM90_REG_W_REMOTE_HIGHL,
> -		LM90_REG_W_REMOTE_OFFSH,
> -		LM90_REG_W_REMOTE_OFFSL,
> +	struct {
> +		u8 high;
> +		u8 low;
> +	} reg[3] = {
> +		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
> +		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL },
> +		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL }
>  	};
>  
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct lm90_data *data = i2c_get_clientdata(client);
> -	int nr = attr->index;
> +	int nr = attr->nr;
> +	int index = attr->index;
>  	long val;
>  	int err;
>  
> @@ -460,24 +461,24 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  		return err;
>  
>  	/* +16 degrees offset for temp2 for the LM99 */
> -	if (data->kind == lm99 && attr->index <= 2)
> +	if (data->kind == lm99 && index <= 2)
>  		val -= 16000;
>  
>  	mutex_lock(&data->update_lock);
>  	if (data->kind == adt7461)
> -		data->temp11[nr] = temp_to_u16_adt7461(data, val);
> +		data->temp11[index] = temp_to_u16_adt7461(data, val);
>  	else if (data->kind == max6646)
> -		data->temp11[nr] = temp_to_u8(val) << 8;
> +		data->temp11[index] = temp_to_u8(val) << 8;
>  	else if (!(data->flags & LM90_HAVE_REM_LIMIT_EXT))
> -		data->temp11[nr] = temp_to_s8(val) << 8;
> +		data->temp11[index] = temp_to_s8(val) << 8;
>  	else
> -		data->temp11[nr] = temp_to_s16(val);
> +		data->temp11[index] = temp_to_s16(val);
>  
> -	i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2],
> -				  data->temp11[nr] >> 8);
> +	i2c_smbus_write_byte_data(client, reg[nr].high,
> +				  data->temp11[index] >> 8);
>  	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
> -		i2c_smbus_write_byte_data(client, reg[(nr - 1) * 2 + 1],
> -					  data->temp11[nr] & 0xff);
> +		i2c_smbus_write_byte_data(client, reg[nr].low,
> +					  data->temp11[index] & 0xff);
>  	mutex_unlock(&data->update_lock);
>  	return count;
>  }
> @@ -549,16 +550,16 @@ static ssize_t show_alarm(struct device *dev, struct device_attribute
>  	return sprintf(buf, "%d\n", (data->alarms >> bitnr) & 1);
>  }
>  
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp11, NULL, 4);
> -static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp11, NULL, 0);
> +static SENSOR_DEVICE_ATTR_2(temp1_input, S_IRUGO, show_temp11, NULL, 0, 4);
> +static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp11, NULL, 0, 0);
>  static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_temp8,
>  	set_temp8, 0);
> -static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 1);
> +static SENSOR_DEVICE_ATTR_2(temp2_min, S_IWUSR | S_IRUGO, show_temp11,
> +	set_temp11, 0, 1);
>  static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_temp8,
>  	set_temp8, 1);
> -static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 2);
> +static SENSOR_DEVICE_ATTR_2(temp2_max, S_IWUSR | S_IRUGO, show_temp11,
> +	set_temp11, 1, 2);
>  static SENSOR_DEVICE_ATTR(temp1_crit, S_IWUSR | S_IRUGO, show_temp8,
>  	set_temp8, 2);
>  static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
> @@ -566,8 +567,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IWUSR | S_IRUGO, show_temp8,
>  static SENSOR_DEVICE_ATTR(temp1_crit_hyst, S_IWUSR | S_IRUGO, show_temphyst,
>  	set_temphyst, 2);
>  static SENSOR_DEVICE_ATTR(temp2_crit_hyst, S_IRUGO, show_temphyst, NULL, 3);
> -static SENSOR_DEVICE_ATTR(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> -	set_temp11, 3);
> +static SENSOR_DEVICE_ATTR_2(temp2_offset, S_IWUSR | S_IRUGO, show_temp11,
> +	set_temp11, 2, 3);
>  
>  /* Individual alarm files */
>  static SENSOR_DEVICE_ATTR(temp1_crit_alarm, S_IRUGO, show_alarm, NULL, 0);

Looks good, applied, thanks.

-- 
Jean Delvare

[PATCH v2 5/7] hwmon: (lm90) Introduce 3rd set of upper temperature limits

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/hwmon/lm90.c |   34 ++++++++++++++++++++++++++++++++--
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 11b5701..d2bcb47 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -147,6 +147,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
 #define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
 #define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
 #define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
+#define	LM90_HAVE_EMERGENCY	0x10	/* 3rd upper (emergency) limit	*/
 
 /*
  * Functions declaration
@@ -213,10 +214,12 @@ struct lm90_data {
 	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
 
 	/* registers values */
-	s8 temp8[4];	/* 0: local low limit
+	s8 temp8[6];	/* 0: local low limit
 			   1: local high limit
 			   2: local critical limit
-			   3: remote critical limit */
+			   3: remote critical limit
+			   4: local emergency limit
+			   5: remote emergency limit */
 	s16 temp11[5];	/* 0: remote input
 			   1: remote low limit
 			   2: remote high limit
@@ -608,6 +611,24 @@ static const struct attribute_group lm90_group = {
 	.attrs = lm90_attributes,
 };
 
+/*
+ * Additional attributes for devices with emergency sensors
+ */
+static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
+	set_temp8, 4);
+static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
+	set_temp8, 5);
+
+static struct attribute *lm90_emergency_attributes[] = {
+	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
+	&sensor_dev_attr_temp2_emergency.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lm90_emergency_group = {
+	.attrs = lm90_emergency_attributes,
+};
+
 /* pec used for ADM1032 only */
 static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
 			char *buf)
@@ -818,6 +839,9 @@ static int lm90_detect(struct i2c_client *new_client,
 
 static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
 {
+	if (data->flags & LM90_HAVE_EMERGENCY)
+		sysfs_remove_group(&client->dev.kobj,
+				   &lm90_emergency_group);
 	if (data->flags & LM90_HAVE_OFFSET)
 		device_remove_file(&client->dev,
 				   &sensor_dev_attr_temp2_offset.dev_attr);
@@ -889,6 +913,12 @@ static int lm90_probe(struct i2c_client *new_client,
 		if (err)
 			goto exit_remove_files;
 	}
+	if (data->flags & LM90_HAVE_EMERGENCY) {
+		err = sysfs_create_group(&new_client->dev.kobj,
+					 &lm90_emergency_group);
+		if (err)
+			goto exit_remove_files;
+	}
 
 	data->hwmon_dev = hwmon_device_register(&new_client->dev);
 	if (IS_ERR(data->hwmon_dev)) {
-- 
1.7.0.87.g0901d

Re: [PATCH v2 5/7] hwmon: (lm90) Introduce 3rd set of upper temperature limits

[Jean Delvare]

Hi Guenter,

On Thu, 9 Sep 2010 06:25:48 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/hwmon/lm90.c |   34 ++++++++++++++++++++++++++++++++--
>  1 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 11b5701..d2bcb47 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -147,6 +147,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
>  #define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
>  #define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
>  #define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
> +#define	LM90_HAVE_EMERGENCY	0x10	/* 3rd upper (emergency) limit	*/
>  
>  /*
>   * Functions declaration
> @@ -213,10 +214,12 @@ struct lm90_data {
>  	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
>  
>  	/* registers values */
> -	s8 temp8[4];	/* 0: local low limit
> +	s8 temp8[6];	/* 0: local low limit
>  			   1: local high limit
>  			   2: local critical limit
> -			   3: remote critical limit */
> +			   3: remote critical limit
> +			   4: local emergency limit
> +			   5: remote emergency limit */
>  	s16 temp11[5];	/* 0: remote input
>  			   1: remote low limit
>  			   2: remote high limit
> @@ -608,6 +611,24 @@ static const struct attribute_group lm90_group = {
>  	.attrs = lm90_attributes,
>  };
>  
> +/*
> + * Additional attributes for devices with emergency sensors
> + */
> +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> +	set_temp8, 4);
> +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> +	set_temp8, 5);
> +
> +static struct attribute *lm90_emergency_attributes[] = {
> +	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
> +	&sensor_dev_attr_temp2_emergency.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lm90_emergency_group = {
> +	.attrs = lm90_emergency_attributes,
> +};
> +
>  /* pec used for ADM1032 only */
>  static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
>  			char *buf)
> @@ -818,6 +839,9 @@ static int lm90_detect(struct i2c_client *new_client,
>  
>  static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
>  {
> +	if (data->flags & LM90_HAVE_EMERGENCY)
> +		sysfs_remove_group(&client->dev.kobj,
> +				   &lm90_emergency_group);
>  	if (data->flags & LM90_HAVE_OFFSET)
>  		device_remove_file(&client->dev,
>  				   &sensor_dev_attr_temp2_offset.dev_attr);

But this flag is never set?

> @@ -889,6 +913,12 @@ static int lm90_probe(struct i2c_client *new_client,
>  		if (err)
>  			goto exit_remove_files;
>  	}
> +	if (data->flags & LM90_HAVE_EMERGENCY) {
> +		err = sysfs_create_group(&new_client->dev.kobj,
> +					 &lm90_emergency_group);
> +		if (err)
> +			goto exit_remove_files;
> +	}
>  
>  	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>  	if (IS_ERR(data->hwmon_dev)) {

-- 
Jean Delvare

Re: [PATCH v2 5/7] hwmon: (lm90) Introduce 3rd set of upper temperature limits

[Guenter Roeck]

Hi Jean,

On Tue, Sep 14, 2010 at 06:51:37AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Thu, 9 Sep 2010 06:25:48 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/hwmon/lm90.c |   34 ++++++++++++++++++++++++++++++++--
> >  1 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 11b5701..d2bcb47 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -147,6 +147,7 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> >  #define	LM90_HAVE_OFFSET	0x02	/* temperature offset register	*/
> >  #define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
> >  #define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
> > +#define	LM90_HAVE_EMERGENCY	0x10	/* 3rd upper (emergency) limit	*/
> >  
> >  /*
> >   * Functions declaration
> > @@ -213,10 +214,12 @@ struct lm90_data {
> >  	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
> >  
> >  	/* registers values */
> > -	s8 temp8[4];	/* 0: local low limit
> > +	s8 temp8[6];	/* 0: local low limit
> >  			   1: local high limit
> >  			   2: local critical limit
> > -			   3: remote critical limit */
> > +			   3: remote critical limit
> > +			   4: local emergency limit
> > +			   5: remote emergency limit */
> >  	s16 temp11[5];	/* 0: remote input
> >  			   1: remote low limit
> >  			   2: remote high limit
> > @@ -608,6 +611,24 @@ static const struct attribute_group lm90_group = {
> >  	.attrs = lm90_attributes,
> >  };
> >  
> > +/*
> > + * Additional attributes for devices with emergency sensors
> > + */
> > +static SENSOR_DEVICE_ATTR(temp1_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > +	set_temp8, 4);
> > +static SENSOR_DEVICE_ATTR(temp2_emergency, S_IWUSR | S_IRUGO, show_temp8,
> > +	set_temp8, 5);
> > +
> > +static struct attribute *lm90_emergency_attributes[] = {
> > +	&sensor_dev_attr_temp1_emergency.dev_attr.attr,
> > +	&sensor_dev_attr_temp2_emergency.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group lm90_emergency_group = {
> > +	.attrs = lm90_emergency_attributes,
> > +};
> > +
> >  /* pec used for ADM1032 only */
> >  static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
> >  			char *buf)
> > @@ -818,6 +839,9 @@ static int lm90_detect(struct i2c_client *new_client,
> >  
> >  static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
> >  {
> > +	if (data->flags & LM90_HAVE_EMERGENCY)
> > +		sysfs_remove_group(&client->dev.kobj,
> > +				   &lm90_emergency_group);
> >  	if (data->flags & LM90_HAVE_OFFSET)
> >  		device_remove_file(&client->dev,
> >  				   &sensor_dev_attr_temp2_offset.dev_attr);
> 
> But this flag is never set?
> 
Assuming you mean LM90_HAVE_EMERGENCY, will be set with patch #6.
Feel free to merge the two if you prefer.

Guenter

> > @@ -889,6 +913,12 @@ static int lm90_probe(struct i2c_client *new_client,
> >  		if (err)
> >  			goto exit_remove_files;
> >  	}
> > +	if (data->flags & LM90_HAVE_EMERGENCY) {
> > +		err = sysfs_create_group(&new_client->dev.kobj,
> > +					 &lm90_emergency_group);
> > +		if (err)
> > +			goto exit_remove_files;
> > +	}
> >  
> >  	data->hwmon_dev = hwmon_device_register(&new_client->dev);
> >  	if (IS_ERR(data->hwmon_dev)) {
> 
> -- 
> Jean Delvare

Re: [lm-sensors] [PATCH v2 5/7] hwmon: (lm90) Introduce 3rd set of upper temperature limits

[Jean Delvare]

On Tue, 14 Sep 2010 12:51:37 +0200, Jean Delvare wrote:
> On Thu, 9 Sep 2010 06:25:48 -0700, Guenter Roeck wrote:
> > @@ -818,6 +839,9 @@ static int lm90_detect(struct i2c_client *new_client,
> >  
> >  static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
> >  {
> > +	if (data->flags & LM90_HAVE_EMERGENCY)
> > +		sysfs_remove_group(&client->dev.kobj,
> > +				   &lm90_emergency_group);
> >  	if (data->flags & LM90_HAVE_OFFSET)
> >  		device_remove_file(&client->dev,
> >  				   &sensor_dev_attr_temp2_offset.dev_attr);
> 
> But this flag is never set?

Oh, I get it now, it's set in the next patch. That's not OK, each patch
should do something useful in its own right. This suggests that you
have to swap patches 5/7 and 6/7 in the series, first adding a separate
type for the MAX6659, then adding support for the emergency limits.

Or if this is too much work for you, you may decide to merge both
patches (hint: "quilt fold" is quite helpful for this, if you're using
quilt).

-- 
Jean Delvare

Re: [lm-sensors] [PATCH v2 5/7] hwmon: (lm90) Introduce 3rd set of upper temperature limits

[Guenter Roeck]

Hi Jean,

On Tue, Sep 14, 2010 at 07:46:59AM -0400, Jean Delvare wrote:
> On Tue, 14 Sep 2010 12:51:37 +0200, Jean Delvare wrote:
> > On Thu, 9 Sep 2010 06:25:48 -0700, Guenter Roeck wrote:
> > > @@ -818,6 +839,9 @@ static int lm90_detect(struct i2c_client *new_client,
> > >  
> > >  static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
> > >  {
> > > +	if (data->flags & LM90_HAVE_EMERGENCY)
> > > +		sysfs_remove_group(&client->dev.kobj,
> > > +				   &lm90_emergency_group);
> > >  	if (data->flags & LM90_HAVE_OFFSET)
> > >  		device_remove_file(&client->dev,
> > >  				   &sensor_dev_attr_temp2_offset.dev_attr);
> > 
> > But this flag is never set?
> 
> Oh, I get it now, it's set in the next patch. That's not OK, each patch
> should do something useful in its own right. This suggests that you
> have to swap patches 5/7 and 6/7 in the series, first adding a separate
> type for the MAX6659, then adding support for the emergency limits.
> 
Ok.

> Or if this is too much work for you, you may decide to merge both
> patches (hint: "quilt fold" is quite helpful for this, if you're using
> quilt).
> 
I typically use git rebase -i. That lets me do the same, including merging
and reordering patches.

Guenter

[PATCH v2 6/7] hwmon: (lm90) Add support for additional features of max6659

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 Documentation/hwmon/lm90 |   13 ++++++-----
 drivers/hwmon/lm90.c     |   51 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
index 6a03dd4..bc2c2b4 100644
--- a/Documentation/hwmon/lm90
+++ b/Documentation/hwmon/lm90
@@ -63,8 +63,8 @@ Supported chips:
     Datasheet: Publicly available at the Maxim website
                http://www.maxim-ic.com/quick_view2.cfm/qv_pk/2578
   * Maxim MAX6659
-    Prefix: 'max6657'
-    Addresses scanned: I2C 0x4c, 0x4d (unsupported 0x4e)
+    Prefix: 'max6659'
+    Addresses scanned: I2C 0x4c, 0x4d, 0x4e
     Datasheet: Publicly available at the Maxim website
                http://www.maxim-ic.com/quick_view2.cfm/qv_pk/2578
   * Maxim MAX6680
@@ -101,10 +101,11 @@ well as the temperature of up to one external diode. It is compatible
 with many other devices, many of which are supported by this driver.
 
 Note that there is no easy way to differentiate between the MAX6657,
-MAX6658 and MAX6659 variants. The extra address and features of the
-MAX6659 are not supported by this driver. The MAX6680 and MAX6681 only
-differ in their pinout, therefore they obviously can't (and don't need to)
-be distinguished.
+MAX6658 and MAX6659 variants. The extra features of the MAX6659 are only
+supported by this driver if the chip is located at address 0x4d or 0x4e,
+or if the chip type is explicitly selected as max6659.
+The MAX6680 and MAX6681 only differ in their pinout, therefore they obviously
+can't (and don't need to) be distinguished.
 
 The specificity of this family of chipsets over the ADM1021/LM84
 family is that it features critical limits with hysteresis, and an
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index d2bcb47..690949b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -85,7 +85,7 @@
  * and MAX6658 have address 0x4c.
  * ADM1032-2, ADT7461-2, LM89-1, LM99-1 and MAX6646 have address 0x4d.
  * MAX6647 has address 0x4e.
- * MAX6659 can have address 0x4c, 0x4d or 0x4e (unsupported).
+ * MAX6659 can have address 0x4c, 0x4d or 0x4e.
  * MAX6680 and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
  * 0x4c, 0x4d or 0x4e.
  */
@@ -94,7 +94,7 @@ static const unsigned short normal_i2c[] = {
 	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
 
 enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
-	w83l771 };
+	w83l771, max6659 };
 
 /*
  * The LM90 registers
@@ -138,6 +138,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
 /* MAX6646/6647/6649/6657/6658/6659 registers */
 
 #define MAX6657_REG_R_LOCAL_TEMPL	0x11
+#define MAX6659_REG_R_REMOTE_EMERG	0x16
+#define MAX6659_REG_W_REMOTE_EMERG	0x16
+#define MAX6659_REG_R_LOCAL_EMERG	0x17
+#define MAX6659_REG_W_LOCAL_EMERG	0x17
 
 /*
  * Device flags
@@ -177,7 +181,7 @@ static const struct i2c_device_id lm90_id[] = {
 	{ "max6649", max6646 },
 	{ "max6657", max6657 },
 	{ "max6658", max6657 },
-	{ "max6659", max6657 },
+	{ "max6659", max6659 },
 	{ "max6680", max6680 },
 	{ "max6681", max6680 },
 	{ "w83l771", w83l771 },
@@ -218,12 +222,12 @@ struct lm90_data {
 			   1: local high limit
 			   2: local critical limit
 			   3: remote critical limit
-			   4: local emergency limit
-			   5: remote emergency limit */
+			   4: local emergency limit (max6659 only)
+			   5: remote emergency limit (max6659 only) */
 	s16 temp11[5];	/* 0: remote input
 			   1: remote low limit
 			   2: remote high limit
-			   3: remote offset (except max6646 and max6657)
+			   3: remote offset (except max6646 and max6657/58/59)
 			   4: local input */
 	u8 temp_hyst;
 	u8 alarms; /* bitvector */
@@ -384,11 +388,13 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
-	static const u8 reg[4] = {
+	static const u8 reg[6] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
 		LM90_REG_W_REMOTE_CRIT,
+		MAX6659_REG_W_LOCAL_EMERG,
+		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -787,12 +793,20 @@ static int lm90_detect(struct i2c_client *new_client,
 		 * register. Likewise, the config1 register seems to lack a
 		 * low nibble, so the value will be those of the previous
 		 * read, so in our case those of the man_id register.
+		 * MAX6659 has a third set of upper temperature limit registers.
+		 * Those registers also return values on MAX6657 and MAX6658,
+		 * thus the only way to detect MAX6659 is by its address.
+		 * For this reason it will be mis-detected as MAX6657 if its
+		 * address is 0x4C.
 		 */
 		if (chip_id == man_id
-		 && (address == 0x4C || address == 0x4D)
+		 && (address == 0x4C || address == 0x4D || address == 0x4E)
 		 && (reg_config1 & 0x1F) == (man_id & 0x0F)
 		 && reg_convrate <= 0x09) {
-			name = "max6657";
+			if (address == 0x4C)
+				name = "max6657";
+			else
+				name = "max6659";
 		} else
 		/*
 		 * The chip_id register of the MAX6680 and MAX6681 holds the
@@ -885,16 +899,21 @@ static int lm90_probe(struct i2c_client *new_client,
 	}
 
 	/* Set chip capabilities */
-	if (data->kind != max6657 && data->kind != max6646)
+	if (data->kind != max6657 && data->kind != max6659
+	    && data->kind != max6646)
 		data->flags |= LM90_HAVE_OFFSET;
 
-	if (data->kind == max6657 || data->kind == max6646)
+	if (data->kind == max6657 || data->kind == max6659
+	    || data->kind == max6646)
 		data->flags |= LM90_HAVE_LOCAL_EXT;
 
-	if (data->kind != max6657 && data->kind != max6646
-	    && data->kind != max6680)
+	if (data->kind != max6657 && data->kind != max6659
+	    && data->kind != max6646 && data->kind != max6680)
 		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
 
+	if (data->kind == max6659)
+		data->flags |= LM90_HAVE_EMERGENCY;
+
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
 
@@ -1102,6 +1121,12 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 					  &l) == 0)
 				data->temp11[3] = (h << 8) | l;
 		}
+		if (data->flags & LM90_HAVE_EMERGENCY) {
+			lm90_read_reg(client, MAX6659_REG_R_LOCAL_EMERG,
+				      &data->temp8[4]);
+			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
+				      &data->temp8[5]);
+		}
 		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
 
 		/* Re-enable ALERT# output if it was originally enabled and
-- 
1.7.0.87.g0901d

Re: [PATCH v2 6/7] hwmon: (lm90) Add support for additional features of max6659

[Jean Delvare]

Hi Guenter,

On Thu, 9 Sep 2010 06:25:49 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  Documentation/hwmon/lm90 |   13 ++++++-----
>  drivers/hwmon/lm90.c     |   51 ++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 45 insertions(+), 19 deletions(-)
> (...)
> @@ -94,7 +94,7 @@ static const unsigned short normal_i2c[] = {
>  	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
>  
>  enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> -	w83l771 };
> +	w83l771, max6659 };

As the numbers are internal only, it might make sense to group max6657
and max6659 together for better readability.

No other comment, other than the patch ordering (or merging) issue
already pointed out in patch 5/7.

-- 
Jean Delvare

Re: [PATCH v2 6/7] hwmon: (lm90) Add support for additional features of max6659

[Guenter Roeck]

Hi Jean,

On Tue, Sep 14, 2010 at 07:52:53AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Thu, 9 Sep 2010 06:25:49 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  Documentation/hwmon/lm90 |   13 ++++++-----
> >  drivers/hwmon/lm90.c     |   51 ++++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 45 insertions(+), 19 deletions(-)
> > (...)
> > @@ -94,7 +94,7 @@ static const unsigned short normal_i2c[] = {
> >  	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
> >  
> >  enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > -	w83l771 };
> > +	w83l771, max6659 };
> 
> As the numbers are internal only, it might make sense to group max6657
> and max6659 together for better readability.
> 
> No other comment, other than the patch ordering (or merging) issue
> already pointed out in patch 5/7.
> 
I made all changes as suggested, and tested the resulting code. Looks good so far.

Once I get your comments for patch #7, I'll send you another set of patches.
It will be the complete set for consistency; also I had a couple of conflicts
due to the changes in #2.

Thanks,
Guenter

[PATCH v2 7/7] hwmon: (lm90) Add support for max6695 and max6696

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 Documentation/hwmon/lm90 |   17 ++++
 drivers/hwmon/Kconfig    |    4 +-
 drivers/hwmon/lm90.c     |  240 +++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 237 insertions(+), 24 deletions(-)

diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
index bc2c2b4..5554bea 100644
--- a/Documentation/hwmon/lm90
+++ b/Documentation/hwmon/lm90
@@ -84,6 +84,17 @@ Supported chips:
     Addresses scanned: I2C 0x4c
     Datasheet: Publicly available at the Maxim website
                http://www.maxim-ic.com/quick_view2.cfm/qv_pk/3500
+  * Maxim MAX6695
+    Prefix: 'max6695'
+    Addresses scanned: I2C 0x18
+    Datasheet: Publicly available at the Maxim website
+               http://www.maxim-ic.com/datasheet/index.mvp/id/4199
+  * Maxim MAX6696
+    Prefix: 'max6695'
+    Addresses scanned: I2C 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
+                           0x4c, 0x4d and 0x4e
+    Datasheet: Publicly available at the Maxim website
+               http://www.maxim-ic.com/datasheet/index.mvp/id/4199
   * Winbond/Nuvoton W83L771AWG/ASG
     Prefix: 'w83l771'
     Addresses scanned: I2C 0x4c
@@ -152,6 +163,12 @@ MAX6680 and MAX6681:
   * Selectable address
   * Remote sensor type selection
 
+MAX6695 and MAX6696:
+  * Better local resolution
+  * Selectable address
+  * Second critical temperature limit
+  * Two remote sensors
+
 W83L771AWG/ASG
   * The AWG and ASG variants only differ in package format.
   * Filter and alert configuration register at 0xBF
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 4d4d09b..17c719b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -605,8 +605,8 @@ config SENSORS_LM90
 	  If you say yes here you get support for National Semiconductor LM90,
 	  LM86, LM89 and LM99, Analog Devices ADM1032 and ADT7461, Maxim
 	  MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
-	  MAX6680, MAX6681 and MAX6692, and Winbond/Nuvoton W83L771AWG/ASG
-	  sensor chips.
+	  MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, and Winbond/Nuvoton
+	  W83L771AWG/ASG sensor chips.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called lm90.
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 690949b..61256e9 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -42,6 +42,11 @@
  * chips. The MAX6680 and MAX6681 only differ in the pinout so they can
  * be treated identically.
  *
+ * This driver also supports the MAX6695 and MAX6696, two other sensor
+ * chips made by Maxim. These are also quite similar to other Maxim
+ * chips, but support three temperature sensors instead of two. MAX6695
+ * and MAX6696 only differ in the pinout so they can be treated identically.
+ *
  * This driver also supports the ADT7461 chip from Analog Devices.
  * It's supported in both compatibility and extended mode. It is mostly
  * compatible with LM90 except for a data format difference for the
@@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
 	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
 
 enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
-	w83l771, max6659 };
+	w83l771, max6659, max6695 };
 
 /*
  * The LM90 registers
@@ -135,9 +140,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
 #define LM90_REG_R_TCRIT_HYST		0x21
 #define LM90_REG_W_TCRIT_HYST		0x21
 
-/* MAX6646/6647/6649/6657/6658/6659 registers */
+/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
 
 #define MAX6657_REG_R_LOCAL_TEMPL	0x11
+#define MAX6695_REG_R_STATUS2		0x12
 #define MAX6659_REG_R_REMOTE_EMERG	0x16
 #define MAX6659_REG_W_REMOTE_EMERG	0x16
 #define MAX6659_REG_R_LOCAL_EMERG	0x17
@@ -152,6 +158,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
 #define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
 #define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
 #define	LM90_HAVE_EMERGENCY	0x10	/* 3rd upper (emergency) limit	*/
+#define	LM90_HAVE_EMERGENCY_ALARM 0x20	/* emergency alarm		*/
+#define	LM90_HAVE_TEMP3		0x40	/* 3rd temperature sensor	*/
 
 /*
  * Functions declaration
@@ -164,6 +172,10 @@ static void lm90_init_client(struct i2c_client *client);
 static void lm90_alert(struct i2c_client *client, unsigned int flag);
 static int lm90_remove(struct i2c_client *client);
 static struct lm90_data *lm90_update_device(struct device *dev);
+static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
+static inline void lm90_select_remote_channel(struct i2c_client *client,
+					      struct lm90_data *data,
+					      int channel);
 
 /*
  * Driver data (common to all clients)
@@ -184,6 +196,8 @@ static const struct i2c_device_id lm90_id[] = {
 	{ "max6659", max6659 },
 	{ "max6680", max6680 },
 	{ "max6681", max6680 },
+	{ "max6695", max6695 },
+	{ "max6696", max6695 },
 	{ "w83l771", w83l771 },
 	{ }
 };
@@ -215,22 +229,29 @@ struct lm90_data {
 	int flags;
 
 	u8 config_orig;		/* Original configuration register value */
-	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
+	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
+				/* Upper 8 bits for max6695/96 */
 
 	/* registers values */
-	s8 temp8[6];	/* 0: local low limit
+	s8 temp8[8];	/* 0: local low limit
 			   1: local high limit
 			   2: local critical limit
 			   3: remote critical limit
-			   4: local emergency limit (max6659 only)
-			   5: remote emergency limit (max6659 only) */
-	s16 temp11[5];	/* 0: remote input
+			   4: local emergency limit (max6659 and max6695/96)
+			   5: remote emergency limit (max6659 and max6695/96)
+			   6: remote 2 critical limit (max6695/96 only)
+			   7: remote 2 emergency limit (max6695/96 only) */
+	s16 temp11[8];	/* 0: remote input
 			   1: remote low limit
 			   2: remote high limit
-			   3: remote offset (except max6646 and max6657/58/59)
-			   4: local input */
+			   3: remote offset (except max6646, max6657/58/59,
+					     and max6695/96)
+			   4: local input
+			   5: remote 2 input (max6695/96 only)
+			   6: remote 2 low limit (max6695/96 only)
+			   7: remote 2 high limit (ma6695/96 only) */
 	u8 temp_hyst;
-	u8 alarms; /* bitvector */
+	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
 };
 
 /*
@@ -388,13 +409,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
 static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 			 const char *buf, size_t count)
 {
-	static const u8 reg[6] = {
+	static const u8 reg[8] = {
 		LM90_REG_W_LOCAL_LOW,
 		LM90_REG_W_LOCAL_HIGH,
 		LM90_REG_W_LOCAL_CRIT,
 		LM90_REG_W_REMOTE_CRIT,
 		MAX6659_REG_W_LOCAL_EMERG,
 		MAX6659_REG_W_REMOTE_EMERG,
+		LM90_REG_W_REMOTE_CRIT,
+		MAX6659_REG_W_REMOTE_EMERG,
 	};
 
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
@@ -419,7 +442,11 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
 		data->temp8[nr] = temp_to_u8(val);
 	else
 		data->temp8[nr] = temp_to_s8(val);
+
+	lm90_select_remote_channel(client, data, nr >= 6);
 	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
+	lm90_select_remote_channel(client, data, 0);
+
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -451,10 +478,13 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	struct {
 		u8 high;
 		u8 low;
-	} reg[3] = {
-		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
-		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL },
-		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL }
+		int channel;
+	} reg[5] = {
+		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
+		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
+		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
+		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 1 },
+		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
 	};
 
 	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
@@ -483,11 +513,14 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
 	else
 		data->temp11[index] = temp_to_s16(val);
 
+	lm90_select_remote_channel(client, data, reg[nr].channel);
 	i2c_smbus_write_byte_data(client, reg[nr].high,
 				  data->temp11[index] >> 8);
 	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
 		i2c_smbus_write_byte_data(client, reg[nr].low,
 					  data->temp11[index] & 0xff);
+	lm90_select_remote_channel(client, data, 0);
+
 	mutex_unlock(&data->update_lock);
 	return count;
 }
@@ -635,6 +668,59 @@ static const struct attribute_group lm90_emergency_group = {
 	.attrs = lm90_emergency_attributes,
 };
 
+static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO, show_alarm, NULL, 15);
+static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO, show_alarm, NULL, 13);
+
+static struct attribute *lm90_emergency_alarm_attributes[] = {
+	&sensor_dev_attr_temp1_emergency_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp2_emergency_alarm.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lm90_emergency_alarm_group = {
+	.attrs = lm90_emergency_alarm_attributes,
+};
+
+/*
+ * Additional attributes for devices with 3 temperature sensors
+ */
+static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
+static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
+	set_temp11, 3, 6);
+static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
+	set_temp11, 4, 7);
+static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
+	set_temp8, 6);
+static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
+static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
+	set_temp8, 7);
+
+static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
+static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
+static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
+static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
+static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO, show_alarm, NULL, 14);
+
+static struct attribute *lm90_temp3_attributes[] = {
+	&sensor_dev_attr_temp3_input.dev_attr.attr,
+	&sensor_dev_attr_temp3_min.dev_attr.attr,
+	&sensor_dev_attr_temp3_max.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp3_emergency.dev_attr.attr,
+
+	&sensor_dev_attr_temp3_fault.dev_attr.attr,
+	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
+	&sensor_dev_attr_temp3_emergency_alarm.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lm90_temp3_group = {
+	.attrs = lm90_temp3_attributes,
+};
+
 /* pec used for ADM1032 only */
 static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
 			char *buf)
@@ -674,6 +760,22 @@ static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
  * Real code
  */
 
+static inline void lm90_select_remote_channel(struct i2c_client *client,
+					      struct lm90_data *data,
+					      int channel)
+{
+	u8 config;
+
+	if (data->kind == max6695) {
+		lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
+		config &= ~0x08;
+		if (channel)
+			config |= 0x08;
+		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
+					  config);
+	}
+}
+
 /*
  * The ADM1032 supports PEC but not on write byte transactions, so we need
  * to explicitly ask for a transaction without PEC.
@@ -786,6 +888,23 @@ static int lm90_detect(struct i2c_client *new_client,
 		}
 	} else
 	if (man_id == 0x4D) { /* Maxim */
+		int reg_emerg, reg_emerg2, reg_status2;
+
+		/*
+		 * We read MAX6659_REG_R_REMOTE_EMERG twice, and re-read
+		 * LM90_REG_R_MAN_ID in between. If MAX6659_REG_R_REMOTE_EMERG
+		 * exists, both readings will reflect the same value. Otherwise,
+		 * the readings will be different.
+		 */
+		if ((reg_emerg = i2c_smbus_read_byte_data(new_client,
+						MAX6659_REG_R_REMOTE_EMERG)) < 0
+		 || i2c_smbus_read_byte_data(new_client, LM90_REG_R_MAN_ID) < 0
+		 || (reg_emerg2 = i2c_smbus_read_byte_data(new_client,
+						MAX6659_REG_R_REMOTE_EMERG)) < 0
+		 || (reg_status2 = i2c_smbus_read_byte_data(new_client,
+						MAX6695_REG_R_STATUS2)) < 0)
+			return -ENODEV;
+
 		/*
 		 * The MAX6657, MAX6658 and MAX6659 do NOT have a chip_id
 		 * register. Reading from that address will return the last
@@ -809,6 +928,24 @@ static int lm90_detect(struct i2c_client *new_client,
 				name = "max6659";
 		} else
 		/*
+		 * Even though MAX6695 and MAX6696 do not have a chip ID
+		 * register, reading it returns 0x01. Bit 4 of the config1
+		 * register is unused and should return zero when read. Bit 0 of
+		 * the status2 register is unused and should return zero when
+		 * read.
+		 *
+		 * MAX6695 and MAX6696 have an additional set of temperature
+		 * limit registers. We can detect those chips by checking if
+		 * one of those registers exists.
+		 */
+		if (chip_id == 0x01
+		 && (reg_config1 & 0x10) == 0x00
+		 && (reg_status2 & 0x01) == 0x00
+		 && reg_emerg == reg_emerg2
+		 && reg_convrate <= 0x07) {
+			name = "max6695";
+		} else
+		/*
 		 * The chip_id register of the MAX6680 and MAX6681 holds the
 		 * revision of the chip. The lowest bit of the config1 register
 		 * is unused and should return zero when read, so should the
@@ -853,6 +990,11 @@ static int lm90_detect(struct i2c_client *new_client,
 
 static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
 {
+	if (data->flags & LM90_HAVE_TEMP3)
+		sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
+	if (data->flags & LM90_HAVE_EMERGENCY_ALARM)
+		sysfs_remove_group(&client->dev.kobj,
+				   &lm90_emergency_alarm_group);
 	if (data->flags & LM90_HAVE_EMERGENCY)
 		sysfs_remove_group(&client->dev.kobj,
 				   &lm90_emergency_group);
@@ -893,6 +1035,9 @@ static int lm90_probe(struct i2c_client *new_client,
 	case lm86:
 		data->alert_alarms = 0x7b;
 		break;
+	case max6695:
+		data->alert_alarms = 0x187c;
+		break;
 	default:
 		data->alert_alarms = 0x7c;
 		break;
@@ -900,20 +1045,24 @@ static int lm90_probe(struct i2c_client *new_client,
 
 	/* Set chip capabilities */
 	if (data->kind != max6657 && data->kind != max6659
-	    && data->kind != max6646)
+	    && data->kind != max6646 && data->kind != max6695)
 		data->flags |= LM90_HAVE_OFFSET;
 
 	if (data->kind == max6657 || data->kind == max6659
-	    || data->kind == max6646)
+	    || data->kind == max6646 || data->kind == max6695)
 		data->flags |= LM90_HAVE_LOCAL_EXT;
 
 	if (data->kind != max6657 && data->kind != max6659
-	    && data->kind != max6646 && data->kind != max6680)
+	    && data->kind != max6646 && data->kind != max6680
+	    && data->kind != max6695)
 		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
 
-	if (data->kind == max6659)
+	if (data->kind == max6659 || data->kind == max6695)
 		data->flags |= LM90_HAVE_EMERGENCY;
 
+	if (data->kind == max6695)
+		data->flags |= LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3;
+
 	/* Initialize the LM90 chip */
 	lm90_init_client(new_client);
 
@@ -938,6 +1087,18 @@ static int lm90_probe(struct i2c_client *new_client,
 		if (err)
 			goto exit_remove_files;
 	}
+	if (data->flags & LM90_HAVE_EMERGENCY_ALARM) {
+		err = sysfs_create_group(&new_client->dev.kobj,
+					 &lm90_emergency_alarm_group);
+		if (err)
+			goto exit_remove_files;
+	}
+	if (data->flags & LM90_HAVE_TEMP3) {
+		err = sysfs_create_group(&new_client->dev.kobj,
+					 &lm90_temp3_group);
+		if (err)
+			goto exit_remove_files;
+	}
 
 	data->hwmon_dev = hwmon_device_register(&new_client->dev);
 	if (IS_ERR(data->hwmon_dev)) {
@@ -985,6 +1146,12 @@ static void lm90_init_client(struct i2c_client *client)
 	if (data->kind == max6680)
 		config |= 0x18;
 
+	/*
+	 * Select external channel 1 for max6695
+	 */
+	if (data->kind == max6695)
+		config &= ~0x08;
+
 	config &= 0xBF;	/* run */
 	if (config != data->config_orig) /* Only write if changed */
 		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
@@ -1008,10 +1175,14 @@ static int lm90_remove(struct i2c_client *client)
 static void lm90_alert(struct i2c_client *client, unsigned int flag)
 {
 	struct lm90_data *data = i2c_get_clientdata(client);
-	u8 config, alarms;
+	u8 config, alarms, alarms2 = 0;
 
 	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
-	if ((alarms & 0x7f) == 0) {
+
+	if (data->kind == max6695)
+		lm90_read_reg(client, MAX6695_REG_R_STATUS2, &alarms2);
+
+	if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
 		dev_info(&client->dev, "Everything OK\n");
 	} else {
 		if (alarms & 0x61)
@@ -1024,6 +1195,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
 			dev_warn(&client->dev,
 				 "temp%d diode open, please check!\n", 2);
 
+		if (alarms2 & 0x18)
+			dev_warn(&client->dev,
+				 "temp%d out of range, please check!\n", 3);
+
 		/* Disable ALERT# output, because these chips don't implement
 		  SMBus alert correctly; they should only hold the alert line
 		  low briefly. */
@@ -1079,6 +1254,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 	if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
 	 || !data->valid) {
 		u8 h, l;
+		u8 alarms;
 
 		dev_dbg(&client->dev, "Updating lm90 data.\n");
 		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
@@ -1127,7 +1303,27 @@ static struct lm90_data *lm90_update_device(struct device *dev)
 			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
 				      &data->temp8[5]);
 		}
-		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
+		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
+		data->alarms = alarms;	/* save as 16 bit value */
+
+		if (data->kind == max6695) {
+			lm90_select_remote_channel(client, data, 1);
+			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
+				      &data->temp8[6]);
+			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
+				      &data->temp8[7]);
+			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
+				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
+			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
+				data->temp11[6] = h << 8;
+			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
+				data->temp11[7] = h << 8;
+			lm90_select_remote_channel(client, data, 0);
+
+			if (!lm90_read_reg(client, MAX6695_REG_R_STATUS2,
+					   &alarms))
+				data->alarms |= alarms << 8;
+		}
 
 		/* Re-enable ALERT# output if it was originally enabled and
 		 * relevant alarms are all clear */
-- 
1.7.0.87.g0901d

Re: [PATCH v2 7/7] hwmon: (lm90) Add support for max6695 and max6696

[Jean Delvare]

Hi Guenter,

Sorry for the slow review, I am in the process of reinstalling my
workstation entirely, and this keeps be busy.

On Thu, 9 Sep 2010 06:25:50 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  Documentation/hwmon/lm90 |   17 ++++
>  drivers/hwmon/Kconfig    |    4 +-
>  drivers/hwmon/lm90.c     |  240 +++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 237 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/hwmon/lm90 b/Documentation/hwmon/lm90
> index bc2c2b4..5554bea 100644
> --- a/Documentation/hwmon/lm90
> +++ b/Documentation/hwmon/lm90
> @@ -84,6 +84,17 @@ Supported chips:
>      Addresses scanned: I2C 0x4c
>      Datasheet: Publicly available at the Maxim website
>                 http://www.maxim-ic.com/quick_view2.cfm/qv_pk/3500
> +  * Maxim MAX6695
> +    Prefix: 'max6695'
> +    Addresses scanned: I2C 0x18
> +    Datasheet: Publicly available at the Maxim website
> +               http://www.maxim-ic.com/datasheet/index.mvp/id/4199
> +  * Maxim MAX6696
> +    Prefix: 'max6695'
> +    Addresses scanned: I2C 0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b,
> +                           0x4c, 0x4d and 0x4e
> +    Datasheet: Publicly available at the Maxim website
> +               http://www.maxim-ic.com/datasheet/index.mvp/id/4199
>    * Winbond/Nuvoton W83L771AWG/ASG
>      Prefix: 'w83l771'
>      Addresses scanned: I2C 0x4c
> @@ -152,6 +163,12 @@ MAX6680 and MAX6681:
>    * Selectable address
>    * Remote sensor type selection
>  
> +MAX6695 and MAX6696:
> +  * Better local resolution
> +  * Selectable address

MAX6696 only?

> +  * Second critical temperature limit
> +  * Two remote sensors
> +
>  W83L771AWG/ASG
>    * The AWG and ASG variants only differ in package format.
>    * Filter and alert configuration register at 0xBF
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 4d4d09b..17c719b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -605,8 +605,8 @@ config SENSORS_LM90
>  	  If you say yes here you get support for National Semiconductor LM90,
>  	  LM86, LM89 and LM99, Analog Devices ADM1032 and ADT7461, Maxim
>  	  MAX6646, MAX6647, MAX6648, MAX6649, MAX6657, MAX6658, MAX6659,
> -	  MAX6680, MAX6681 and MAX6692, and Winbond/Nuvoton W83L771AWG/ASG
> -	  sensor chips.
> +	  MAX6680, MAX6681, MAX6692, MAX6695, MAX6696, and Winbond/Nuvoton
> +	  W83L771AWG/ASG sensor chips.
>  
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm90.
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 690949b..61256e9 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -42,6 +42,11 @@
>   * chips. The MAX6680 and MAX6681 only differ in the pinout so they can
>   * be treated identically.
>   *
> + * This driver also supports the MAX6695 and MAX6696, two other sensor
> + * chips made by Maxim. These are also quite similar to other Maxim
> + * chips, but support three temperature sensors instead of two. MAX6695
> + * and MAX6696 only differ in the pinout so they can be treated identically.
> + *
>   * This driver also supports the ADT7461 chip from Analog Devices.
>   * It's supported in both compatibility and extended mode. It is mostly
>   * compatible with LM90 except for a data format difference for the
> @@ -94,7 +99,7 @@ static const unsigned short normal_i2c[] = {
>  	0x18, 0x19, 0x1a, 0x29, 0x2a, 0x2b, 0x4c, 0x4d, 0x4e, I2C_CLIENT_END };
>  
>  enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> -	w83l771, max6659 };
> +	w83l771, max6659, max6695 };

How much time before we mix max6659 and max6695? ;) It might make sense
to decide to go with max6696 for the latter, to work around this
potential issue.

>  
>  /*
>   * The LM90 registers
> @@ -135,9 +140,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
>  #define LM90_REG_R_TCRIT_HYST		0x21
>  #define LM90_REG_W_TCRIT_HYST		0x21
>  
> -/* MAX6646/6647/6649/6657/6658/6659 registers */
> +/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
>  
>  #define MAX6657_REG_R_LOCAL_TEMPL	0x11
> +#define MAX6695_REG_R_STATUS2		0x12
>  #define MAX6659_REG_R_REMOTE_EMERG	0x16
>  #define MAX6659_REG_W_REMOTE_EMERG	0x16
>  #define MAX6659_REG_R_LOCAL_EMERG	0x17
> @@ -152,6 +158,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
>  #define	LM90_HAVE_LOCAL_EXT	0x04	/* extended local temperature	*/
>  #define	LM90_HAVE_REM_LIMIT_EXT	0x08	/* extended remote limit	*/
>  #define	LM90_HAVE_EMERGENCY	0x10	/* 3rd upper (emergency) limit	*/
> +#define	LM90_HAVE_EMERGENCY_ALARM 0x20	/* emergency alarm		*/
> +#define	LM90_HAVE_TEMP3		0x40	/* 3rd temperature sensor	*/

Use one space after #define.

>  
>  /*
>   * Functions declaration
> @@ -164,6 +172,10 @@ static void lm90_init_client(struct i2c_client *client);
>  static void lm90_alert(struct i2c_client *client, unsigned int flag);
>  static int lm90_remove(struct i2c_client *client);
>  static struct lm90_data *lm90_update_device(struct device *dev);
> +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
> +static inline void lm90_select_remote_channel(struct i2c_client *client,
> +					      struct lm90_data *data,
> +					      int channel);

Would it be possible to simply place these functions at the right
location so that forward declarations are not needed? Ideally I would
like to get rid of all these forward declarations.

>  
>  /*
>   * Driver data (common to all clients)
> @@ -184,6 +196,8 @@ static const struct i2c_device_id lm90_id[] = {
>  	{ "max6659", max6659 },
>  	{ "max6680", max6680 },
>  	{ "max6681", max6680 },
> +	{ "max6695", max6695 },
> +	{ "max6696", max6695 },
>  	{ "w83l771", w83l771 },
>  	{ }
>  };
> @@ -215,22 +229,29 @@ struct lm90_data {
>  	int flags;
>  
>  	u8 config_orig;		/* Original configuration register value */
> -	u8 alert_alarms;	/* Which alarm bits trigger ALERT# */
> +	u16 alert_alarms;	/* Which alarm bits trigger ALERT# */
> +				/* Upper 8 bits for max6695/96 */
>  
>  	/* registers values */
> -	s8 temp8[6];	/* 0: local low limit
> +	s8 temp8[8];	/* 0: local low limit
>  			   1: local high limit
>  			   2: local critical limit
>  			   3: remote critical limit
> -			   4: local emergency limit (max6659 only)
> -			   5: remote emergency limit (max6659 only) */
> -	s16 temp11[5];	/* 0: remote input
> +			   4: local emergency limit (max6659 and max6695/96)
> +			   5: remote emergency limit (max6659 and max6695/96)
> +			   6: remote 2 critical limit (max6695/96 only)
> +			   7: remote 2 emergency limit (max6695/96 only) */
> +	s16 temp11[8];	/* 0: remote input
>  			   1: remote low limit
>  			   2: remote high limit
> -			   3: remote offset (except max6646 and max6657/58/59)
> -			   4: local input */
> +			   3: remote offset (except max6646, max6657/58/59,
> +					     and max6695/96)
> +			   4: local input
> +			   5: remote 2 input (max6695/96 only)
> +			   6: remote 2 low limit (max6695/96 only)
> +			   7: remote 2 high limit (ma6695/96 only) */
>  	u8 temp_hyst;
> -	u8 alarms; /* bitvector */
> +	u16 alarms; /* bitvector (upper 8 bits for max6695/96) */
>  };
>  
>  /*
> @@ -388,13 +409,15 @@ static ssize_t show_temp8(struct device *dev, struct device_attribute *devattr,
>  static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>  			 const char *buf, size_t count)
>  {
> -	static const u8 reg[6] = {
> +	static const u8 reg[8] = {
>  		LM90_REG_W_LOCAL_LOW,
>  		LM90_REG_W_LOCAL_HIGH,
>  		LM90_REG_W_LOCAL_CRIT,
>  		LM90_REG_W_REMOTE_CRIT,
>  		MAX6659_REG_W_LOCAL_EMERG,
>  		MAX6659_REG_W_REMOTE_EMERG,
> +		LM90_REG_W_REMOTE_CRIT,
> +		MAX6659_REG_W_REMOTE_EMERG,
>  	};
>  
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> @@ -419,7 +442,11 @@ static ssize_t set_temp8(struct device *dev, struct device_attribute *devattr,
>  		data->temp8[nr] = temp_to_u8(val);
>  	else
>  		data->temp8[nr] = temp_to_s8(val);
> +
> +	lm90_select_remote_channel(client, data, nr >= 6);
>  	i2c_smbus_write_byte_data(client, reg[nr], data->temp8[nr]);
> +	lm90_select_remote_channel(client, data, 0);
> +
>  	mutex_unlock(&data->update_lock);
>  	return count;
>  }
> @@ -451,10 +478,13 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  	struct {
>  		u8 high;
>  		u8 low;
> -	} reg[3] = {
> -		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL },
> -		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL },
> -		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL }
> +		int channel;
> +	} reg[5] = {
> +		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 0 },
> +		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 0 },
> +		{ LM90_REG_W_REMOTE_OFFSH, LM90_REG_W_REMOTE_OFFSL, 0 },
> +		{ LM90_REG_W_REMOTE_LOWH, LM90_REG_W_REMOTE_LOWL, 1 },
> +		{ LM90_REG_W_REMOTE_HIGHH, LM90_REG_W_REMOTE_HIGHL, 1 }
>  	};
>  
>  	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> @@ -483,11 +513,14 @@ static ssize_t set_temp11(struct device *dev, struct device_attribute *devattr,
>  	else
>  		data->temp11[index] = temp_to_s16(val);
>  
> +	lm90_select_remote_channel(client, data, reg[nr].channel);
>  	i2c_smbus_write_byte_data(client, reg[nr].high,
>  				  data->temp11[index] >> 8);
>  	if (data->flags & LM90_HAVE_REM_LIMIT_EXT)
>  		i2c_smbus_write_byte_data(client, reg[nr].low,
>  					  data->temp11[index] & 0xff);
> +	lm90_select_remote_channel(client, data, 0);
> +
>  	mutex_unlock(&data->update_lock);
>  	return count;
>  }
> @@ -635,6 +668,59 @@ static const struct attribute_group lm90_emergency_group = {
>  	.attrs = lm90_emergency_attributes,
>  };
>  
> +static SENSOR_DEVICE_ATTR(temp1_emergency_alarm, S_IRUGO, show_alarm, NULL, 15);
> +static SENSOR_DEVICE_ATTR(temp2_emergency_alarm, S_IRUGO, show_alarm, NULL, 13);
> +
> +static struct attribute *lm90_emergency_alarm_attributes[] = {
> +	&sensor_dev_attr_temp1_emergency_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp2_emergency_alarm.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lm90_emergency_alarm_group = {
> +	.attrs = lm90_emergency_alarm_attributes,
> +};
> +
> +/*
> + * Additional attributes for devices with 3 temperature sensors
> + */
> +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
> +static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> +	set_temp11, 3, 6);
> +static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> +	set_temp11, 4, 7);
> +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> +	set_temp8, 6);
> +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);

4, really? Remote 2 critical limit is 6.

(That's what we get for using bare numbers instead of defining proper
symbols, sigh.)

Also it seems that the hysteresis register applies to both critical and
emergency limits, so it would probably make sense to create (read-only)
temp[1-3]_emergency_hyst attributes.

> +static SENSOR_DEVICE_ATTR(temp3_emergency, S_IWUSR | S_IRUGO, show_temp8,
> +	set_temp8, 7);
> +
> +static SENSOR_DEVICE_ATTR(temp3_crit_alarm, S_IRUGO, show_alarm, NULL, 9);
> +static SENSOR_DEVICE_ATTR(temp3_fault, S_IRUGO, show_alarm, NULL, 10);
> +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_alarm, NULL, 11);
> +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_alarm, NULL, 12);
> +static SENSOR_DEVICE_ATTR(temp3_emergency_alarm, S_IRUGO, show_alarm, NULL, 14);
> +
> +static struct attribute *lm90_temp3_attributes[] = {
> +	&sensor_dev_attr_temp3_input.dev_attr.attr,
> +	&sensor_dev_attr_temp3_min.dev_attr.attr,
> +	&sensor_dev_attr_temp3_max.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp3_emergency.dev_attr.attr,
> +
> +	&sensor_dev_attr_temp3_fault.dev_attr.attr,
> +	&sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_crit_alarm.dev_attr.attr,
> +	&sensor_dev_attr_temp3_emergency_alarm.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group lm90_temp3_group = {
> +	.attrs = lm90_temp3_attributes,
> +};
> +
>  /* pec used for ADM1032 only */
>  static ssize_t show_pec(struct device *dev, struct device_attribute *dummy,
>  			char *buf)
> @@ -674,6 +760,22 @@ static DEVICE_ATTR(pec, S_IWUSR | S_IRUGO, show_pec, set_pec);
>   * Real code
>   */
>  

Please document the fact that the following function must be called
with data->update_lock held.

> +static inline void lm90_select_remote_channel(struct i2c_client *client,
> +					      struct lm90_data *data,
> +					      int channel)
> +{
> +	u8 config;
> +
> +	if (data->kind == max6695) {
> +		lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> +		config &= ~0x08;
> +		if (channel)
> +			config |= 0x08;
> +		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
> +					  config);
> +	}
> +}
> +
>  /*
>   * The ADM1032 supports PEC but not on write byte transactions, so we need
>   * to explicitly ask for a transaction without PEC.
> @@ -786,6 +888,23 @@ static int lm90_detect(struct i2c_client *new_client,
>  		}
>  	} else
>  	if (man_id == 0x4D) { /* Maxim */
> +		int reg_emerg, reg_emerg2, reg_status2;
> +
> +		/*
> +		 * We read MAX6659_REG_R_REMOTE_EMERG twice, and re-read
> +		 * LM90_REG_R_MAN_ID in between. If MAX6659_REG_R_REMOTE_EMERG
> +		 * exists, both readings will reflect the same value. Otherwise,
> +		 * the readings will be different.
> +		 */
> +		if ((reg_emerg = i2c_smbus_read_byte_data(new_client,
> +						MAX6659_REG_R_REMOTE_EMERG)) < 0
> +		 || i2c_smbus_read_byte_data(new_client, LM90_REG_R_MAN_ID) < 0
> +		 || (reg_emerg2 = i2c_smbus_read_byte_data(new_client,
> +						MAX6659_REG_R_REMOTE_EMERG)) < 0
> +		 || (reg_status2 = i2c_smbus_read_byte_data(new_client,
> +						MAX6695_REG_R_STATUS2)) < 0)
> +			return -ENODEV;
> +
>  		/*
>  		 * The MAX6657, MAX6658 and MAX6659 do NOT have a chip_id
>  		 * register. Reading from that address will return the last
> @@ -809,6 +928,24 @@ static int lm90_detect(struct i2c_client *new_client,
>  				name = "max6659";
>  		} else
>  		/*
> +		 * Even though MAX6695 and MAX6696 do not have a chip ID
> +		 * register, reading it returns 0x01. Bit 4 of the config1
> +		 * register is unused and should return zero when read. Bit 0 of
> +		 * the status2 register is unused and should return zero when
> +		 * read.
> +		 *
> +		 * MAX6695 and MAX6696 have an additional set of temperature
> +		 * limit registers. We can detect those chips by checking if
> +		 * one of those registers exists.
> +		 */
> +		if (chip_id == 0x01
> +		 && (reg_config1 & 0x10) == 0x00
> +		 && (reg_status2 & 0x01) == 0x00
> +		 && reg_emerg == reg_emerg2
> +		 && reg_convrate <= 0x07) {
> +			name = "max6695";
> +		} else
> +		/*
>  		 * The chip_id register of the MAX6680 and MAX6681 holds the
>  		 * revision of the chip. The lowest bit of the config1 register
>  		 * is unused and should return zero when read, so should the
> @@ -853,6 +990,11 @@ static int lm90_detect(struct i2c_client *new_client,
>  
>  static void lm90_remove_files(struct i2c_client *client, struct lm90_data *data)
>  {
> +	if (data->flags & LM90_HAVE_TEMP3)
> +		sysfs_remove_group(&client->dev.kobj, &lm90_temp3_group);
> +	if (data->flags & LM90_HAVE_EMERGENCY_ALARM)
> +		sysfs_remove_group(&client->dev.kobj,
> +				   &lm90_emergency_alarm_group);
>  	if (data->flags & LM90_HAVE_EMERGENCY)
>  		sysfs_remove_group(&client->dev.kobj,
>  				   &lm90_emergency_group);
> @@ -893,6 +1035,9 @@ static int lm90_probe(struct i2c_client *new_client,
>  	case lm86:
>  		data->alert_alarms = 0x7b;
>  		break;
> +	case max6695:
> +		data->alert_alarms = 0x187c;
> +		break;
>  	default:
>  		data->alert_alarms = 0x7c;
>  		break;
> @@ -900,20 +1045,24 @@ static int lm90_probe(struct i2c_client *new_client,
>  
>  	/* Set chip capabilities */
>  	if (data->kind != max6657 && data->kind != max6659
> -	    && data->kind != max6646)
> +	    && data->kind != max6646 && data->kind != max6695)
>  		data->flags |= LM90_HAVE_OFFSET;
>  
>  	if (data->kind == max6657 || data->kind == max6659
> -	    || data->kind == max6646)
> +	    || data->kind == max6646 || data->kind == max6695)
>  		data->flags |= LM90_HAVE_LOCAL_EXT;
>  
>  	if (data->kind != max6657 && data->kind != max6659
> -	    && data->kind != max6646 && data->kind != max6680)
> +	    && data->kind != max6646 && data->kind != max6680
> +	    && data->kind != max6695)
>  		data->flags |= LM90_HAVE_REM_LIMIT_EXT;
>  
> -	if (data->kind == max6659)
> +	if (data->kind == max6659 || data->kind == max6695)
>  		data->flags |= LM90_HAVE_EMERGENCY;
>  
> +	if (data->kind == max6695)
> +		data->flags |= LM90_HAVE_EMERGENCY_ALARM | LM90_HAVE_TEMP3;
> +
>  	/* Initialize the LM90 chip */
>  	lm90_init_client(new_client);
>  
> @@ -938,6 +1087,18 @@ static int lm90_probe(struct i2c_client *new_client,
>  		if (err)
>  			goto exit_remove_files;
>  	}
> +	if (data->flags & LM90_HAVE_EMERGENCY_ALARM) {
> +		err = sysfs_create_group(&new_client->dev.kobj,
> +					 &lm90_emergency_alarm_group);
> +		if (err)
> +			goto exit_remove_files;
> +	}
> +	if (data->flags & LM90_HAVE_TEMP3) {
> +		err = sysfs_create_group(&new_client->dev.kobj,
> +					 &lm90_temp3_group);
> +		if (err)
> +			goto exit_remove_files;
> +	}
>  
>  	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>  	if (IS_ERR(data->hwmon_dev)) {
> @@ -985,6 +1146,12 @@ static void lm90_init_client(struct i2c_client *client)
>  	if (data->kind == max6680)
>  		config |= 0x18;
>  
> +	/*
> +	 * Select external channel 1 for max6695
> +	 */
> +	if (data->kind == max6695)
> +		config &= ~0x08;
> +
>  	config &= 0xBF;	/* run */
>  	if (config != data->config_orig) /* Only write if changed */
>  		i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1, config);
> @@ -1008,10 +1175,14 @@ static int lm90_remove(struct i2c_client *client)
>  static void lm90_alert(struct i2c_client *client, unsigned int flag)
>  {
>  	struct lm90_data *data = i2c_get_clientdata(client);
> -	u8 config, alarms;
> +	u8 config, alarms, alarms2 = 0;
>  
>  	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> -	if ((alarms & 0x7f) == 0) {
> +
> +	if (data->kind == max6695)
> +		lm90_read_reg(client, MAX6695_REG_R_STATUS2, &alarms2);
> +
> +	if ((alarms & 0x7f) == 0 && (alarms2 & 0xfe) == 0) {
>  		dev_info(&client->dev, "Everything OK\n");
>  	} else {
>  		if (alarms & 0x61)
> @@ -1024,6 +1195,10 @@ static void lm90_alert(struct i2c_client *client, unsigned int flag)
>  			dev_warn(&client->dev,
>  				 "temp%d diode open, please check!\n", 2);
>  
> +		if (alarms2 & 0x18)
> +			dev_warn(&client->dev,
> +				 "temp%d out of range, please check!\n", 3);
> +
>  		/* Disable ALERT# output, because these chips don't implement
>  		  SMBus alert correctly; they should only hold the alert line
>  		  low briefly. */
> @@ -1079,6 +1254,7 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  	if (time_after(jiffies, data->last_updated + HZ / 2 + HZ / 10)
>  	 || !data->valid) {
>  		u8 h, l;
> +		u8 alarms;
>  
>  		dev_dbg(&client->dev, "Updating lm90 data.\n");
>  		lm90_read_reg(client, LM90_REG_R_LOCAL_LOW, &data->temp8[0]);
> @@ -1127,7 +1303,27 @@ static struct lm90_data *lm90_update_device(struct device *dev)
>  			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
>  				      &data->temp8[5]);
>  		}
> -		lm90_read_reg(client, LM90_REG_R_STATUS, &data->alarms);
> +		lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);
> +		data->alarms = alarms;	/* save as 16 bit value */
> +
> +		if (data->kind == max6695) {
> +			lm90_select_remote_channel(client, data, 1);
> +			lm90_read_reg(client, LM90_REG_R_REMOTE_CRIT,
> +				      &data->temp8[6]);
> +			lm90_read_reg(client, MAX6659_REG_R_REMOTE_EMERG,
> +				      &data->temp8[7]);
> +			lm90_read16(client, LM90_REG_R_REMOTE_TEMPH,
> +				    LM90_REG_R_REMOTE_TEMPL, &data->temp11[5]);
> +			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_LOWH, &h))
> +				data->temp11[6] = h << 8;
> +			if (!lm90_read_reg(client, LM90_REG_R_REMOTE_HIGHH, &h))
> +				data->temp11[7] = h << 8;
> +			lm90_select_remote_channel(client, data, 0);
> +
> +			if (!lm90_read_reg(client, MAX6695_REG_R_STATUS2,
> +					   &alarms))
> +				data->alarms |= alarms << 8;
> +		}
>  
>  		/* Re-enable ALERT# output if it was originally enabled and
>  		 * relevant alarms are all clear */

Code looks overall pretty clean. Great job!

-- 
Jean Delvare

Re: [PATCH v2 7/7] hwmon: (lm90) Add support for max6695 and max6696

[Guenter Roeck]

Hi Jean,

On Wed, Sep 15, 2010 at 07:20:29AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> Sorry for the slow review, I am in the process of reinstalling my
> workstation entirely, and this keeps be busy.
> 
No problem. I appreciate your time.

> On Thu, 9 Sep 2010 06:25:50 -0700, Guenter Roeck wrote:
[...]
> > +MAX6695 and MAX6696:
> > +  * Better local resolution
> > +  * Selectable address
> 
> MAX6696 only?
> 
yes

[...]
> >  enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> > -     w83l771, max6659 };
> > +     w83l771, max6659, max6695 };
> 
> How much time before we mix max6659 and max6695? ;) It might make sense
> to decide to go with max6696 for the latter, to work around this
> potential issue.
> 
ok with me. 

> >
> >  /*
> >   * The LM90 registers
> > @@ -135,9 +140,10 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> >  #define LM90_REG_R_TCRIT_HYST                0x21
> >  #define LM90_REG_W_TCRIT_HYST                0x21
> >
> > -/* MAX6646/6647/6649/6657/6658/6659 registers */
> > +/* MAX6646/6647/6649/6657/6658/6659/6695/6696 registers */
> >
> >  #define MAX6657_REG_R_LOCAL_TEMPL    0x11
> > +#define MAX6695_REG_R_STATUS2                0x12
> >  #define MAX6659_REG_R_REMOTE_EMERG   0x16
> >  #define MAX6659_REG_W_REMOTE_EMERG   0x16
> >  #define MAX6659_REG_R_LOCAL_EMERG    0x17
> > @@ -152,6 +158,8 @@ enum chips { lm90, adm1032, lm99, lm86, max6657, adt7461, max6680, max6646,
> >  #define      LM90_HAVE_LOCAL_EXT     0x04    /* extended local temperature   */
> >  #define      LM90_HAVE_REM_LIMIT_EXT 0x08    /* extended remote limit        */
> >  #define      LM90_HAVE_EMERGENCY     0x10    /* 3rd upper (emergency) limit  */
> > +#define      LM90_HAVE_EMERGENCY_ALARM 0x20  /* emergency alarm              */
> > +#define      LM90_HAVE_TEMP3         0x40    /* 3rd temperature sensor       */
> 
> Use one space after #define.
> 
Did that already after the comment in the earlier patch of the series.

> >
> >  /*
> >   * Functions declaration
> > @@ -164,6 +172,10 @@ static void lm90_init_client(struct i2c_client *client);
> >  static void lm90_alert(struct i2c_client *client, unsigned int flag);
> >  static int lm90_remove(struct i2c_client *client);
> >  static struct lm90_data *lm90_update_device(struct device *dev);
> > +static int lm90_read_reg(struct i2c_client *client, u8 reg, u8 *value);
> > +static inline void lm90_select_remote_channel(struct i2c_client *client,
> > +                                           struct lm90_data *data,
> > +                                           int channel);
> 
> Would it be possible to simply place these functions at the right
> location so that forward declarations are not needed? Ideally I would
> like to get rid of all these forward declarations.
> 
I moved lm90_select_remote_channel() so the lm90_read_reg() declaration
is no longer needed.

Code structure is such that sensor show/set functions come first, followed by
"real" code. Unfortunately, that implies that local functions called from
those functions have to be forward declarated.

We can move that around to avoid forward declarations, but I would prefer
to do that in a separate patch if we decide to go that way.

[...]
> > +/*
> > + * Additional attributes for devices with 3 temperature sensors
> > + */
> > +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
> > +static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> > +     set_temp11, 3, 6);
> > +static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> > +     set_temp11, 4, 7);
> > +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> > +     set_temp8, 6);
> > +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
> 
> 4, really? Remote 2 critical limit is 6.
> 
I am impressed. Good catch.

> (That's what we get for using bare numbers instead of defining proper
> symbols, sigh.)
> 
> Also it seems that the hysteresis register applies to both critical and
> emergency limits, so it would probably make sense to create (read-only)
> temp[1-3]_emergency_hyst attributes.
> 
Makes sense. That will be in patch #6 (previously patch #5), though,
with additional code here, and yet another attribute in the ABI.

[...]
> 
> Please document the fact that the following function must be called
> with data->update_lock held.
> 
Done.

[...]

> 
> Code looks overall pretty clean. Great job!
> 
Thanks!

Guenter

Re: [PATCH v2 7/7] hwmon: (lm90) Add support for max6695 and max6696

[Jean Delvare]

On Wed, 15 Sep 2010 07:29:27 -0700, Guenter Roeck wrote:
> On Wed, Sep 15, 2010 at 07:20:29AM -0400, Jean Delvare wrote:
> > Would it be possible to simply place these functions at the right
> > location so that forward declarations are not needed? Ideally I would
> > like to get rid of all these forward declarations.
> > 
> I moved lm90_select_remote_channel() so the lm90_read_reg() declaration
> is no longer needed.
> 
> Code structure is such that sensor show/set functions come first, followed by
> "real" code. Unfortunately, that implies that local functions called from
> those functions have to be forward declarated.
> 
> We can move that around to avoid forward declarations, but I would prefer
> to do that in a separate patch if we decide to go that way.

Sure, fine with me.

> [...]
> > > +/*
> > > + * Additional attributes for devices with 3 temperature sensors
> > > + */
> > > +static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp11, NULL, 0, 5);
> > > +static SENSOR_DEVICE_ATTR_2(temp3_min, S_IWUSR | S_IRUGO, show_temp11,
> > > +     set_temp11, 3, 6);
> > > +static SENSOR_DEVICE_ATTR_2(temp3_max, S_IWUSR | S_IRUGO, show_temp11,
> > > +     set_temp11, 4, 7);
> > > +static SENSOR_DEVICE_ATTR(temp3_crit, S_IWUSR | S_IRUGO, show_temp8,
> > > +     set_temp8, 6);
> > > +static SENSOR_DEVICE_ATTR(temp3_crit_hyst, S_IRUGO, show_temphyst, NULL, 4);
> > 
> > 4, really? Remote 2 critical limit is 6.
> > 
> I am impressed. Good catch.

Now you understand why it takes me so long to review patches. I
actually read them ;)

-- 
Jean Delvare