[lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for

[lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for

[Guenter Roeck]

On LM96163, the external temperature sensor type is configurable to
either a thermal diode or a 3904 transistor. The chip reports a wrong
temperature if misconfigured. Add writable attribute to support sensor type
configuration.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/lm63.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index 368db01..f8f4739 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -93,6 +93,7 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
 #define LM63_REG_MAN_ID			0xFE
 #define LM63_REG_CHIP_ID		0xFF
 
+#define LM96163_REG_TRUTHERM		0x30
 #define LM96163_REG_REMOTE_TEMP_U_MSB	0x31
 #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
 #define LM96163_REG_CONFIG_ENHANCED	0x45
@@ -214,6 +215,7 @@ struct lm63_data {
 	u8 alarms;
 	bool pwm_highres;
 	bool remote_unsigned; /* true if unsigned remote upper limits */
+	bool trutherm;
 };
 
 static inline int temp8_from_reg(struct lm63_data *data, int nr)
@@ -529,6 +531,38 @@ static ssize_t set_update_interval(struct device *dev,
 	return count;
 }
 
+static ssize_t show_type(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm63_data *data = i2c_get_clientdata(client);
+
+	return snprintf(buf, PAGE_SIZE - 1,
+		data->trutherm ? "3\n" : "2\n");
+}
+
+static ssize_t set_type(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm63_data *data = i2c_get_clientdata(client);
+	unsigned long val;
+
+	if (kstrtoul(buf, 10, &val) < 0)
+		return -EINVAL;
+	if (val != 2 && val != 3)
+		return -EINVAL;
+
+	mutex_lock(&data->update_lock);
+	data->trutherm = val = 3;
+	i2c_smbus_write_byte_data(client, LM96163_REG_TRUTHERM,
+				  data->trutherm ? 0x02 : 0x00);
+	data->valid = 0;
+	mutex_unlock(&data->update_lock);
+
+	return count;
+}
+
 static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
 			   char *buf)
 {
@@ -569,6 +603,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
 static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
 	set_temp2_crit_hyst);
 
+static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
+
 /* Individual alarm files */
 static SENSOR_DEVICE_ATTR(fan1_min_alarm, S_IRUGO, show_alarm, NULL, 0);
 static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
@@ -728,6 +764,12 @@ static int lm63_probe(struct i2c_client *new_client,
 		if (err)
 			goto exit_remove_files;
 	}
+	if (data->kind = lm96163) {
+		err = device_create_file(&new_client->dev,
+					 &dev_attr_temp2_type);
+		if (err)
+			goto exit_remove_files;
+	}
 
 	data->hwmon_dev = hwmon_device_register(&new_client->dev);
 	if (IS_ERR(data->hwmon_dev)) {
@@ -738,6 +780,7 @@ static int lm63_probe(struct i2c_client *new_client,
 	return 0;
 
 exit_remove_files:
+	device_remove_file(&new_client->dev, &dev_attr_temp2_type);
 	sysfs_remove_group(&new_client->dev.kobj, &lm63_group);
 	sysfs_remove_group(&new_client->dev.kobj, &lm63_group_fan1);
 exit_free:
@@ -779,6 +822,9 @@ static void lm63_init_client(struct i2c_client *client)
 		break;
 	case lm96163:
 		data->max_convrate_hz = LM96163_MAX_CONVRATE_HZ;
+		data->trutherm
+		  = i2c_smbus_read_byte_data(client,
+					     LM96163_REG_TRUTHERM) = 0x02;
 		break;
 	default:
 		BUG();
@@ -822,6 +868,7 @@ static int lm63_remove(struct i2c_client *client)
 	struct lm63_data *data = i2c_get_clientdata(client);
 
 	hwmon_device_unregister(data->hwmon_dev);
+	device_remove_file(&client->dev, &dev_attr_temp2_type);
 	sysfs_remove_group(&client->dev.kobj, &lm63_group);
 	sysfs_remove_group(&client->dev.kobj, &lm63_group_fan1);
 
-- 
1.7.5.4


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

Re: [lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for external sensor on LM96163

[Jean Delvare]

Hi Guenter,

On Tue, 10 Jan 2012 09:14:19 -0800, Guenter Roeck wrote:
> On LM96163, the external temperature sensor type is configurable to
> either a thermal diode or a 3904 transistor. The chip reports a wrong
> temperature if misconfigured. Add writable attribute to support sensor type
> configuration.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/lm63.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 47 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> index 368db01..f8f4739 100644
> --- a/drivers/hwmon/lm63.c
> +++ b/drivers/hwmon/lm63.c
> @@ -93,6 +93,7 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
>  #define LM63_REG_MAN_ID			0xFE
>  #define LM63_REG_CHIP_ID		0xFF
>  
> +#define LM96163_REG_TRUTHERM		0x30
>  #define LM96163_REG_REMOTE_TEMP_U_MSB	0x31
>  #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
>  #define LM96163_REG_CONFIG_ENHANCED	0x45
> @@ -214,6 +215,7 @@ struct lm63_data {
>  	u8 alarms;
>  	bool pwm_highres;
>  	bool remote_unsigned; /* true if unsigned remote upper limits */
> +	bool trutherm;
>  };
>  
>  static inline int temp8_from_reg(struct lm63_data *data, int nr)
> @@ -529,6 +531,38 @@ static ssize_t set_update_interval(struct device *dev,
>  	return count;
>  }
>  
> +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm63_data *data = i2c_get_clientdata(client);
> +
> +	return snprintf(buf, PAGE_SIZE - 1,

This is a little inconsistent, we're using sprintf() everywhere else in
this driver.

> +		data->trutherm ? "3\n" : "2\n");

It's questionable whether you want 3 or rather 1 for TruTherm beta
compensation mode. While Documentation/hwmon/sysfs-interface says 1 is
PII/Celeron Diode, I think this is an historical glitch and what it
really means is "CPU embedded diode". It might be the right time to fix
that. BTW "sensors" says just "diode" for type 1, with no reference to
PII/Celeron.

> +}
> +
> +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm63_data *data = i2c_get_clientdata(client);
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 10, &val) < 0)
> +		return -EINVAL;

Here again this is inconsistent with the rest of the driver where the
error value returned by kstrto*l is passed down to the caller.

> +	if (val != 2 && val != 3)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->update_lock);
> +	data->trutherm = val = 3;
> +	i2c_smbus_write_byte_data(client, LM96163_REG_TRUTHERM,
> +				  data->trutherm ? 0x02 : 0x00);

I'd rather do a read-modify-write cycle even though the datasheet says
other bits are unused. Datasheets aren't always accurate and future
chips might add something in this register.

It might make sense to return the error value if the write fails?

> +	data->valid = 0;
> +	mutex_unlock(&data->update_lock);
> +
> +	return count;
> +}
> +
>  static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
>  			   char *buf)
>  {
> @@ -569,6 +603,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
>  static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
>  	set_temp2_crit_hyst);
>  
> +static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
> +
>  /* Individual alarm files */
>  static SENSOR_DEVICE_ATTR(fan1_min_alarm, S_IRUGO, show_alarm, NULL, 0);
>  static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
> @@ -728,6 +764,12 @@ static int lm63_probe(struct i2c_client *new_client,
>  		if (err)
>  			goto exit_remove_files;
>  	}
> +	if (data->kind = lm96163) {
> +		err = device_create_file(&new_client->dev,
> +					 &dev_attr_temp2_type);
> +		if (err)
> +			goto exit_remove_files;
> +	}
>  
>  	data->hwmon_dev = hwmon_device_register(&new_client->dev);
>  	if (IS_ERR(data->hwmon_dev)) {
> @@ -738,6 +780,7 @@ static int lm63_probe(struct i2c_client *new_client,
>  	return 0;
>  
>  exit_remove_files:
> +	device_remove_file(&new_client->dev, &dev_attr_temp2_type);
>  	sysfs_remove_group(&new_client->dev.kobj, &lm63_group);
>  	sysfs_remove_group(&new_client->dev.kobj, &lm63_group_fan1);
>  exit_free:
> @@ -779,6 +822,9 @@ static void lm63_init_client(struct i2c_client *client)
>  		break;
>  	case lm96163:
>  		data->max_convrate_hz = LM96163_MAX_CONVRATE_HZ;
> +		data->trutherm
> +		  = i2c_smbus_read_byte_data(client,
> +					     LM96163_REG_TRUTHERM) = 0x02;

Here too I'd play it safe and check the single bit, not the whole
register value.

>  		break;
>  	default:
>  		BUG();
> @@ -822,6 +868,7 @@ static int lm63_remove(struct i2c_client *client)
>  	struct lm63_data *data = i2c_get_clientdata(client);
>  
>  	hwmon_device_unregister(data->hwmon_dev);
> +	device_remove_file(&client->dev, &dev_attr_temp2_type);
>  	sysfs_remove_group(&client->dev.kobj, &lm63_group);
>  	sysfs_remove_group(&client->dev.kobj, &lm63_group_fan1);
>  

Maybe you could also mention this specific feature at the end of
Documentation/hwmon/lm63 (where the LM96163 differences are listed.)

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for external sensor on LM96163

[Guenter Roeck]

Hi Jean,

On Thu, 2012-01-12 at 16:51 -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Tue, 10 Jan 2012 09:14:19 -0800, Guenter Roeck wrote:
> > On LM96163, the external temperature sensor type is configurable to
> > either a thermal diode or a 3904 transistor. The chip reports a wrong
> > temperature if misconfigured. Add writable attribute to support sensor type
> > configuration.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/hwmon/lm63.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 47 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
> > index 368db01..f8f4739 100644
> > --- a/drivers/hwmon/lm63.c
> > +++ b/drivers/hwmon/lm63.c
> > @@ -93,6 +93,7 @@ static const unsigned short normal_i2c[] = { 0x18, 0x4c, 0x4e, I2C_CLIENT_END };
> >  #define LM63_REG_MAN_ID			0xFE
> >  #define LM63_REG_CHIP_ID		0xFF
> >  
> > +#define LM96163_REG_TRUTHERM		0x30
> >  #define LM96163_REG_REMOTE_TEMP_U_MSB	0x31
> >  #define LM96163_REG_REMOTE_TEMP_U_LSB	0x32
> >  #define LM96163_REG_CONFIG_ENHANCED	0x45
> > @@ -214,6 +215,7 @@ struct lm63_data {
> >  	u8 alarms;
> >  	bool pwm_highres;
> >  	bool remote_unsigned; /* true if unsigned remote upper limits */
> > +	bool trutherm;
> >  };
> >  
> >  static inline int temp8_from_reg(struct lm63_data *data, int nr)
> > @@ -529,6 +531,38 @@ static ssize_t set_update_interval(struct device *dev,
> >  	return count;
> >  }
> >  
> > +static ssize_t show_type(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct lm63_data *data = i2c_get_clientdata(client);
> > +
> > +	return snprintf(buf, PAGE_SIZE - 1,
> 
> This is a little inconsistent, we're using sprintf() everywhere else in
> this driver.
> 
ok

> > +		data->trutherm ? "3\n" : "2\n");
> 
> It's questionable whether you want 3 or rather 1 for TruTherm beta
> compensation mode. While Documentation/hwmon/sysfs-interface says 1 is
> PII/Celeron Diode, I think this is an historical glitch and what it
> really means is "CPU embedded diode". It might be the right time to fix
> that. BTW "sensors" says just "diode" for type 1, with no reference to
> PII/Celeron.
> 
Ok with me. Should we change the documentation as well (separate
patch) ?

> > +}
> > +
> > +static ssize_t set_type(struct device *dev, struct device_attribute *attr,
> > +			const char *buf, size_t count)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct lm63_data *data = i2c_get_clientdata(client);
> > +	unsigned long val;
> > +
> > +	if (kstrtoul(buf, 10, &val) < 0)
> > +		return -EINVAL;
> 
> Here again this is inconsistent with the rest of the driver where the
> error value returned by kstrto*l is passed down to the caller.
> 
ok. Just being lazy.

> > +	if (val != 2 && val != 3)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&data->update_lock);
> > +	data->trutherm = val = 3;
> > +	i2c_smbus_write_byte_data(client, LM96163_REG_TRUTHERM,
> > +				  data->trutherm ? 0x02 : 0x00);
> 
> I'd rather do a read-modify-write cycle even though the datasheet says
> other bits are unused. Datasheets aren't always accurate and future
> chips might add something in this register.
> 
ok

> It might make sense to return the error value if the write fails?
> 
We could do that, but it should be a separate patch to add error
handling to the entire driver.

> > +	data->valid = 0;
> > +	mutex_unlock(&data->update_lock);
> > +
> > +	return count;
> > +}
> > +
> >  static ssize_t show_alarms(struct device *dev, struct device_attribute *dummy,
> >  			   char *buf)
> >  {
> > @@ -569,6 +603,8 @@ static SENSOR_DEVICE_ATTR(temp2_crit, S_IRUGO, show_remote_temp8,
> >  static DEVICE_ATTR(temp2_crit_hyst, S_IWUSR | S_IRUGO, show_temp2_crit_hyst,
> >  	set_temp2_crit_hyst);
> >  
> > +static DEVICE_ATTR(temp2_type, S_IWUSR | S_IRUGO, show_type, set_type);
> > +
> >  /* Individual alarm files */
> >  static SENSOR_DEVICE_ATTR(fan1_min_alarm, S_IRUGO, show_alarm, NULL, 0);
> >  static SENSOR_DEVICE_ATTR(temp2_crit_alarm, S_IRUGO, show_alarm, NULL, 1);
> > @@ -728,6 +764,12 @@ static int lm63_probe(struct i2c_client *new_client,
> >  		if (err)
> >  			goto exit_remove_files;
> >  	}
> > +	if (data->kind = lm96163) {
> > +		err = device_create_file(&new_client->dev,
> > +					 &dev_attr_temp2_type);
> > +		if (err)
> > +			goto exit_remove_files;
> > +	}
> >  
> >  	data->hwmon_dev = hwmon_device_register(&new_client->dev);
> >  	if (IS_ERR(data->hwmon_dev)) {
> > @@ -738,6 +780,7 @@ static int lm63_probe(struct i2c_client *new_client,
> >  	return 0;
> >  
> >  exit_remove_files:
> > +	device_remove_file(&new_client->dev, &dev_attr_temp2_type);
> >  	sysfs_remove_group(&new_client->dev.kobj, &lm63_group);
> >  	sysfs_remove_group(&new_client->dev.kobj, &lm63_group_fan1);
> >  exit_free:
> > @@ -779,6 +822,9 @@ static void lm63_init_client(struct i2c_client *client)
> >  		break;
> >  	case lm96163:
> >  		data->max_convrate_hz = LM96163_MAX_CONVRATE_HZ;
> > +		data->trutherm
> > +		  = i2c_smbus_read_byte_data(client,
> > +					     LM96163_REG_TRUTHERM) = 0x02;
> 
> Here too I'd play it safe and check the single bit, not the whole
> register value.
> 
ok

> >  		break;
> >  	default:
> >  		BUG();
> > @@ -822,6 +868,7 @@ static int lm63_remove(struct i2c_client *client)
> >  	struct lm63_data *data = i2c_get_clientdata(client);
> >  
> >  	hwmon_device_unregister(data->hwmon_dev);
> > +	device_remove_file(&client->dev, &dev_attr_temp2_type);
> >  	sysfs_remove_group(&client->dev.kobj, &lm63_group);
> >  	sysfs_remove_group(&client->dev.kobj, &lm63_group_fan1);
> >  
> 
> Maybe you could also mention this specific feature at the end of
> Documentation/hwmon/lm63 (where the LM96163 differences are listed.)
> 
Ok, will do.

Thanks,
Guenter



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

Re: [lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for external sensor on LM96163

[Jean Delvare]

On Thu, 12 Jan 2012 14:14:45 -0800, Guenter Roeck wrote:
> On Thu, 2012-01-12 at 16:51 -0500, Jean Delvare wrote:
> > It's questionable whether you want 3 or rather 1 for TruTherm beta
> > compensation mode. While Documentation/hwmon/sysfs-interface says 1 is
> > PII/Celeron Diode, I think this is an historical glitch and what it
> > really means is "CPU embedded diode". It might be the right time to fix
> > that. BTW "sensors" says just "diode" for type 1, with no reference to
> > PII/Celeron.
> > 
> Ok with me. Should we change the documentation as well (separate
> patch) ?

I think so, yes. Either you do it, or I'll do if you prefer.

> > (...)
> > It might make sense to return the error value if the write fails?
> > 
> We could do that, but it should be a separate patch to add error
> handling to the entire driver.

Good point, I agree.

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (lm63) Add sensor type attribute for external sensor on LM96163

[Guenter Roeck]

On Fri, Jan 13, 2012 at 01:54:15AM -0500, Jean Delvare wrote:
> On Thu, 12 Jan 2012 14:14:45 -0800, Guenter Roeck wrote:
> > On Thu, 2012-01-12 at 16:51 -0500, Jean Delvare wrote:
> > > It's questionable whether you want 3 or rather 1 for TruTherm beta
> > > compensation mode. While Documentation/hwmon/sysfs-interface says 1 is
> > > PII/Celeron Diode, I think this is an historical glitch and what it
> > > really means is "CPU embedded diode". It might be the right time to fix
> > > that. BTW "sensors" says just "diode" for type 1, with no reference to
> > > PII/Celeron.
> > > 
> > Ok with me. Should we change the documentation as well (separate
> > patch) ?
> 
> I think so, yes. Either you do it, or I'll do if you prefer.
> 
I'll do it, no problem.

Guenter

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