[lm-sensors] [RFC PATCH] Allow the configuration register to be

[lm-sensors] [RFC PATCH] Allow the configuration register to be

[Shubhrajyoti D]

The patch intends to allow the configuration of resolution
polarity etc.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/hwmon/lm75.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
index 2a5b12a..abf3135 100644
--- a/drivers/hwmon/lm75.c
+++ b/drivers/hwmon/lm75.c
@@ -111,7 +111,41 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
 	mutex_unlock(&data->update_lock);
 	return count;
 }
+static ssize_t set_configuration(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	s32 status;
+	struct i2c_client *client = to_i2c_client(dev);
+	struct lm75_data *data = i2c_get_clientdata(client);
+	unsigned long temp;
+	int error;
+	error = strict_strtol(buf, 10, &temp);
+	if (error)
+		return error;
+
+	data->orig_conf = (u8)temp;
+	/* I2C write to the configuration register */
+	status = lm75_write_value(client, LM75_REG_CONF,
+			data->orig_conf);
+	return count;
+}
+
+static ssize_t show_configuration(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int status = lm75_read_value(client, LM75_REG_CONF);
+	if (status < 0) {
+		dev_dbg(&client->dev, "Can't read config? %d\n", status);
+		return status;
+	}
+
+	return sprintf(buf, "%u\n", status);
+}
 
+static SENSOR_DEVICE_ATTR(configuration, S_IWUSR | S_IRUGO,
+			show_configuration, set_configuration, 0);
 static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
 			show_temp, set_temp, 1);
 static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
@@ -122,6 +156,7 @@ static struct attribute *lm75_attributes[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_max.dev_attr.attr,
 	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
+	&sensor_dev_attr_configuration.dev_attr.attr,
 
 	NULL
 };
-- 
1.7.0.4


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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Jean Delvare]

On Mon, 13 Sep 2010 19:37:47 +0530, Shubhrajyoti D wrote:
> The patch intends to allow the configuration of resolution
> polarity etc.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/hwmon/lm75.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> index 2a5b12a..abf3135 100644
> --- a/drivers/hwmon/lm75.c
> +++ b/drivers/hwmon/lm75.c
> @@ -111,7 +111,41 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *da,
>  	mutex_unlock(&data->update_lock);
>  	return count;
>  }
> +static ssize_t set_configuration(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	s32 status;
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct lm75_data *data = i2c_get_clientdata(client);
> +	unsigned long temp;
> +	int error;
> +	error = strict_strtol(buf, 10, &temp);
> +	if (error)
> +		return error;
> +
> +	data->orig_conf = (u8)temp;
> +	/* I2C write to the configuration register */
> +	status = lm75_write_value(client, LM75_REG_CONF,
> +			data->orig_conf);
> +	return count;
> +}
> +
> +static ssize_t show_configuration(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	int status = lm75_read_value(client, LM75_REG_CONF);
> +	if (status < 0) {
> +		dev_dbg(&client->dev, "Can't read config? %d\n", status);
> +		return status;
> +	}
> +
> +	return sprintf(buf, "%u\n", status);
> +}
>  
> +static SENSOR_DEVICE_ATTR(configuration, S_IWUSR | S_IRUGO,
> +			show_configuration, set_configuration, 0);
>  static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
>  			show_temp, set_temp, 1);
>  static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
> @@ -122,6 +156,7 @@ static struct attribute *lm75_attributes[] = {
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_temp1_max.dev_attr.attr,
>  	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> +	&sensor_dev_attr_configuration.dev_attr.attr,
>  
>  	NULL
>  };

Massive NACK. You are exposing a raw register interface to user-space,
in a way which can't be standardized. This goes against the philosophy
of our standard sysfs interface.

If you have an LM75-style device in an embedded device, configuration
should be provided as platform data. For other cases, either the
BIOS/firmware should configure the device properly, or you can do it
with i2c-dev + i2cset from user-space.

If any properly really needs to be exposed though sysfs in your
opinion, it must be standardized first and stick to the one file, one
feature philosophy.


-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Datta, Shubhrajyoti]

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: Monday, September 13, 2010 7:58 PM
> To: Datta, Shubhrajyoti
> Cc: lm-sensors@lm-sensors.org
> Subject: Re: [lm-sensors] [RFC PATCH] Allow the configuration register to
> be written
> 
> On Mon, 13 Sep 2010 19:37:47 +0530, Shubhrajyoti D wrote:
> > The patch intends to allow the configuration of resolution
> > polarity etc.
> >
> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> > ---
> >  drivers/hwmon/lm75.c |   35 +++++++++++++++++++++++++++++++++++
> >  1 files changed, 35 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c
> > index 2a5b12a..abf3135 100644
> > --- a/drivers/hwmon/lm75.c
> > +++ b/drivers/hwmon/lm75.c
> > @@ -111,7 +111,41 @@ static ssize_t set_temp(struct device *dev, struct
> device_attribute *da,
> >  	mutex_unlock(&data->update_lock);
> >  	return count;
> >  }
> > +static ssize_t set_configuration(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	s32 status;
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct lm75_data *data = i2c_get_clientdata(client);
> > +	unsigned long temp;
> > +	int error;
> > +	error = strict_strtol(buf, 10, &temp);
> > +	if (error)
> > +		return error;
> > +
> > +	data->orig_conf = (u8)temp;
> > +	/* I2C write to the configuration register */
> > +	status = lm75_write_value(client, LM75_REG_CONF,
> > +			data->orig_conf);
> > +	return count;
> > +}
> > +
> > +static ssize_t show_configuration(struct device *dev,
> > +				struct device_attribute *attr, char *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	int status = lm75_read_value(client, LM75_REG_CONF);
> > +	if (status < 0) {
> > +		dev_dbg(&client->dev, "Can't read config? %d\n", status);
> > +		return status;
> > +	}
> > +
> > +	return sprintf(buf, "%u\n", status);
> > +}
> >
> > +static SENSOR_DEVICE_ATTR(configuration, S_IWUSR | S_IRUGO,
> > +			show_configuration, set_configuration, 0);
> >  static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO,
> >  			show_temp, set_temp, 1);
> >  static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
> > @@ -122,6 +156,7 @@ static struct attribute *lm75_attributes[] = {
> >  	&sensor_dev_attr_temp1_input.dev_attr.attr,
> >  	&sensor_dev_attr_temp1_max.dev_attr.attr,
> >  	&sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> > +	&sensor_dev_attr_configuration.dev_attr.attr,
> >
> >  	NULL
> >  };
> 
> Massive NACK. You are exposing a raw register interface to user-space,
> in a way which can't be standardized. This goes against the philosophy
> of our standard sysfs interface.
> 
> If you have an LM75-style device in an embedded device, configuration
> should be provided as platform data. For other cases, either the
> BIOS/firmware should configure the device properly, or you can do it
> with i2c-dev + i2cset from user-space.

Ok makes sense. 
> 
> If any properly really needs to be exposed though sysfs in your
> opinion, it must be standardized first and stick to the one file, one
> feature philosophy.
I chose it as a sysfs because ideally the resolution/time is a tradeoff applications are better suited and not board dependent.
However agree to your "one file, one feature" comment. Was wondering if any other driver has similar interface or sysfs entry(resolution).

> 
> 
> --
> Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Jean Delvare]

On Mon, 13 Sep 2010 20:13:00 +0530, Datta, Shubhrajyoti wrote:
> > From: Jean Delvare [mailto:khali@linux-fr.org]
> > Massive NACK. You are exposing a raw register interface to user-space,
> > in a way which can't be standardized. This goes against the philosophy
> > of our standard sysfs interface.
> > 
> > If you have an LM75-style device in an embedded device, configuration
> > should be provided as platform data. For other cases, either the
> > BIOS/firmware should configure the device properly, or you can do it
> > with i2c-dev + i2cset from user-space.
> 
> Ok makes sense. 
> > 
> > If any properly really needs to be exposed though sysfs in your
> > opinion, it must be standardized first and stick to the one file, one
> > feature philosophy.
> I chose it as a sysfs because ideally the resolution/time is a tradeoff applications are better suited and not board dependent.

Polarity OTOH is quite board-specific.

> However agree to your "one file, one feature" comment. Was wondering if any other driver has similar interface or sysfs entry(resolution).

Not that I know of. At least there's no standard sysfs attribute for
that. We have one for update rate, but it's fairly recent. Having one
for resolution would certainly make sense for simple temperature
sensors.

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Guenter Roeck]

On Mon, Sep 13, 2010 at 11:33:56AM -0400, Jean Delvare wrote:
> On Mon, 13 Sep 2010 20:13:00 +0530, Datta, Shubhrajyoti wrote:
> > > From: Jean Delvare [mailto:khali@linux-fr.org]
> > > Massive NACK. You are exposing a raw register interface to user-space,
> > > in a way which can't be standardized. This goes against the philosophy
> > > of our standard sysfs interface.
> > > 
> > > If you have an LM75-style device in an embedded device, configuration
> > > should be provided as platform data. For other cases, either the
> > > BIOS/firmware should configure the device properly, or you can do it
> > > with i2c-dev + i2cset from user-space.
> > 
> > Ok makes sense. 
> > > 
> > > If any properly really needs to be exposed though sysfs in your
> > > opinion, it must be standardized first and stick to the one file, one
> > > feature philosophy.
> > I chose it as a sysfs because ideally the resolution/time is a tradeoff applications are better suited and not board dependent.
> 
> Polarity OTOH is quite board-specific.
> 
> > However agree to your "one file, one feature" comment. Was wondering if any other driver has similar interface or sysfs entry(resolution).
> 
> Not that I know of. At least there's no standard sysfs attribute for
> that. We have one for update rate, but it's fairly recent. Having one
> for resolution would certainly make sense for simple temperature
> sensors.

I found the following attributes used for the update inverval.

adt7470.c	auto_update_interval
lm95241.c	rate
adm1031.c	update_rate

Not sure about adt7470.c, ince it reflects an automatic interval, but would it make sense
to update lm95241.c to use the standard attribute ?

On a side note, update_rate (or rate) doesn't really reflect its use. A "rate"
would be measured in updates/time, not in absolute time. Or, in other words,
it reflects a frequency, not an interval. So we are really talking about intervals.
Not sure if that is worth bitching about, but since the attribute is quite new
it might make sense to think about it.

Resolution would have to be sensor dependent, since each sensor can have its own resolution.
Would be nice to have an attribute for that.

Guenter

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Jean Delvare]

On Mon, 13 Sep 2010 09:51:09 -0700, Guenter Roeck wrote:
> On Mon, Sep 13, 2010 at 11:33:56AM -0400, Jean Delvare wrote:
> > Not that I know of. At least there's no standard sysfs attribute for
> > that. We have one for update rate, but it's fairly recent. Having one
> > for resolution would certainly make sense for simple temperature
> > sensors.
> 
> I found the following attributes used for the update inverval.
> 
> adt7470.c	auto_update_interval
> lm95241.c	rate
> adm1031.c	update_rate
> 
> Not sure about adt7470.c, ince it reflects an automatic interval, but would it make sense
> to update lm95241.c to use the standard attribute ?

Yes it would.

> On a side note, update_rate (or rate) doesn't really reflect its use. A "rate"
> would be measured in updates/time, not in absolute time. Or, in other words,
> it reflects a frequency, not an interval. So we are really talking about intervals.
> Not sure if that is worth bitching about, but since the attribute is quite new
> it might make sense to think about it.

Jonathan Cameron (Cc'd) had a similar concern:
http://lists.lm-sensors.org/pipermail/lm-sensors/2010-May/028596.html

Nobody seemed to care back then. I can't disagree with both of you, but
OTOH I don't feel too strongly about it either. So if someone submits a
patch making things better, that's fine with me, but I won't spend my
own time on it.

> Resolution would have to be sensor dependent, since each sensor can have its own resolution.
> Would be nice to have an attribute for that.

Not too sure about that. I had the same reaction at first, but do we
actually know of devices where the resolution isn't global?

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Guenter Roeck]

On Mon, Sep 13, 2010 at 01:41:21PM -0400, Jean Delvare wrote:
> On Mon, 13 Sep 2010 09:51:09 -0700, Guenter Roeck wrote:
> > On Mon, Sep 13, 2010 at 11:33:56AM -0400, Jean Delvare wrote:
> > > Not that I know of. At least there's no standard sysfs attribute for
> > > that. We have one for update rate, but it's fairly recent. Having one
> > > for resolution would certainly make sense for simple temperature
> > > sensors.
> > 
> > I found the following attributes used for the update inverval.
> > 
> > adt7470.c	auto_update_interval
> > lm95241.c	rate
> > adm1031.c	update_rate
> > 
> > Not sure about adt7470.c, ince it reflects an automatic interval, but would it make sense
> > to update lm95241.c to use the standard attribute ?
> 
> Yes it would.
> 
> > On a side note, update_rate (or rate) doesn't really reflect its use. A "rate"
> > would be measured in updates/time, not in absolute time. Or, in other words,
> > it reflects a frequency, not an interval. So we are really talking about intervals.
> > Not sure if that is worth bitching about, but since the attribute is quite new
> > it might make sense to think about it.
> 
> Jonathan Cameron (Cc'd) had a similar concern:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2010-May/028596.html
> 
> Nobody seemed to care back then. I can't disagree with both of you, but
> OTOH I don't feel too strongly about it either. So if someone submits a
> patch making things better, that's fine with me, but I won't spend my
> own time on it.
> 
I'll submit a set of patches. update_interval ok ? It reflects the time between
events, while period tends to reflect a duration.

> > Resolution would have to be sensor dependent, since each sensor can have its own resolution.
> > Would be nice to have an attribute for that.
> 
> Not too sure about that. I had the same reaction at first, but do we
> actually know of devices where the resolution isn't global?
> 
Yes, in the lm90 driver. Only max6657/58/59 and max6646 have extended local
temperatures, all other chips have only 8 bit resolution for the local temperature.
Remote temperatures are all extended, ie have higher resoution.

Guenter


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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Jean Delvare]

On Mon, 13 Sep 2010 11:09:06 -0700, Guenter Roeck wrote:
> On Mon, Sep 13, 2010 at 01:41:21PM -0400, Jean Delvare wrote:
> > On Mon, 13 Sep 2010 09:51:09 -0700, Guenter Roeck wrote:
> > > I found the following attributes used for the update inverval.
> > > 
> > > adt7470.c	auto_update_interval
> > > lm95241.c	rate
> > > adm1031.c	update_rate
> > > 
> > > Not sure about adt7470.c, ince it reflects an automatic interval, but would it make sense
> > > to update lm95241.c to use the standard attribute ?
> > 
> > Yes it would.
> > 
> > > On a side note, update_rate (or rate) doesn't really reflect its use. A "rate"
> > > would be measured in updates/time, not in absolute time. Or, in other words,
> > > it reflects a frequency, not an interval. So we are really talking about intervals.
> > > Not sure if that is worth bitching about, but since the attribute is quite new
> > > it might make sense to think about it.
> > 
> > Jonathan Cameron (Cc'd) had a similar concern:
> > http://lists.lm-sensors.org/pipermail/lm-sensors/2010-May/028596.html
> > 
> > Nobody seemed to care back then. I can't disagree with both of you, but
> > OTOH I don't feel too strongly about it either. So if someone submits a
> > patch making things better, that's fine with me, but I won't spend my
> > own time on it.
>
> I'll submit a set of patches. update_interval ok ? It reflects the time between
> events, while period tends to reflect a duration.

Yes, I would be fine with update_interval. Jonathan, OK with you too?

> > > Resolution would have to be sensor dependent, since each sensor can have its own resolution.
> > > Would be nice to have an attribute for that.
> > 
> > Not too sure about that. I had the same reaction at first, but do we
> > actually know of devices where the resolution isn't global?
>
> Yes, in the lm90 driver. Only max6657/58/59 and max6646 have extended local
> temperatures, all other chips have only 8 bit resolution for the local temperature.
> Remote temperatures are all extended, ie have higher resoution.

But resolution can't be changed on these chips, can it? I think the
main purpose of these files is to let the user change the resolution
for chips which support this (usually this is an update speed /
resolution trade-off). Or do you believe that having read-only
attributes exposing fixed resolutions would be valuable?

I'm a little worried because resolution as a bit number isn't too
informative and doesn't quite work as a device-independent value. If
you really want the information to be exposed to user-space for all
devices, then we'd rather use actual sensor units (mV, m°C, whatever)
for resolution, and possibly add other attributes for the range. But
this means adding a whole lot of attribute files for some devices (if
we do it on a per-channel basis), so we first have to determine whether
it is really worth it. I don't think it is, but you can try and
convince me.

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Guenter Roeck]

Hi Jean,

On Tue, Sep 14, 2010 at 04:08:33AM -0400, Jean Delvare wrote:
> On Mon, 13 Sep 2010 11:09:06 -0700, Guenter Roeck wrote:
> > On Mon, Sep 13, 2010 at 01:41:21PM -0400, Jean Delvare wrote:
> > > On Mon, 13 Sep 2010 09:51:09 -0700, Guenter Roeck wrote:
> > > > I found the following attributes used for the update inverval.
> > > > 
> > > > adt7470.c	auto_update_interval
> > > > lm95241.c	rate
> > > > adm1031.c	update_rate
> > > > 
> > > > Not sure about adt7470.c, ince it reflects an automatic interval, but would it make sense
> > > > to update lm95241.c to use the standard attribute ?
> > > 
> > > Yes it would.
> > > 
> > > > On a side note, update_rate (or rate) doesn't really reflect its use. A "rate"
> > > > would be measured in updates/time, not in absolute time. Or, in other words,
> > > > it reflects a frequency, not an interval. So we are really talking about intervals.
> > > > Not sure if that is worth bitching about, but since the attribute is quite new
> > > > it might make sense to think about it.
> > > 
> > > Jonathan Cameron (Cc'd) had a similar concern:
> > > http://lists.lm-sensors.org/pipermail/lm-sensors/2010-May/028596.html
> > > 
> > > Nobody seemed to care back then. I can't disagree with both of you, but
> > > OTOH I don't feel too strongly about it either. So if someone submits a
> > > patch making things better, that's fine with me, but I won't spend my
> > > own time on it.
> >
> > I'll submit a set of patches. update_interval ok ? It reflects the time between
> > events, while period tends to reflect a duration.
> 
> Yes, I would be fine with update_interval. Jonathan, OK with you too?
> 
> > > > Resolution would have to be sensor dependent, since each sensor can have its own resolution.
> > > > Would be nice to have an attribute for that.
> > > 
> > > Not too sure about that. I had the same reaction at first, but do we
> > > actually know of devices where the resolution isn't global?
> >
> > Yes, in the lm90 driver. Only max6657/58/59 and max6646 have extended local
> > temperatures, all other chips have only 8 bit resolution for the local temperature.
> > Remote temperatures are all extended, ie have higher resoution.
> 
> But resolution can't be changed on these chips, can it? I think the

Correct.

> main purpose of these files is to let the user change the resolution
> for chips which support this (usually this is an update speed /
> resolution trade-off). Or do you believe that having read-only
> attributes exposing fixed resolutions would be valuable?
> 
Good point. Probably not really. More like nice to have.

> I'm a little worried because resolution as a bit number isn't too
> informative and doesn't quite work as a device-independent value. If
> you really want the information to be exposed to user-space for all
> devices, then we'd rather use actual sensor units (mV, m°C, whatever)
> for resolution, and possibly add other attributes for the range. But

I assumed it would be in units. Not sure how to express resolution 
as bit number in the first place.

> this means adding a whole lot of attribute files for some devices (if
> we do it on a per-channel basis), so we first have to determine whether
> it is really worth it. I don't think it is, but you can try and
> convince me.
> 
Thinking about it, I'd rather keep it in mind in case someone else
sees the need to it and convinces both of us...

Another question if both attributes (rate and resolution) would make sense 
as module parameters instead. Are those attributes expected to vary across
multiple instances of the same driver ?

Guenter

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Jean Delvare]

On Tue, 14 Sep 2010 05:01:28 -0700, Guenter Roeck wrote:
> Hi Jean,
> 
> On Tue, Sep 14, 2010 at 04:08:33AM -0400, Jean Delvare wrote:
> > On Mon, 13 Sep 2010 11:09:06 -0700, Guenter Roeck wrote:
> > > On Mon, Sep 13, 2010 at 01:41:21PM -0400, Jean Delvare wrote:
> > > > Not too sure about that. I had the same reaction at first, but do we
> > > > actually know of devices where the resolution isn't global?
> > >
> > > Yes, in the lm90 driver. Only max6657/58/59 and max6646 have extended local
> > > temperatures, all other chips have only 8 bit resolution for the local temperature.
> > > Remote temperatures are all extended, ie have higher resoution.
> > 
> > But resolution can't be changed on these chips, can it? I think the
> 
> Correct.
> 
> > main purpose of these files is to let the user change the resolution
> > for chips which support this (usually this is an update speed /
> > resolution trade-off). Or do you believe that having read-only
> > attributes exposing fixed resolutions would be valuable?
> > 
> Good point. Probably not really. More like nice to have.
> 
> > I'm a little worried because resolution as a bit number isn't too
> > informative and doesn't quite work as a device-independent value. If
> > you really want the information to be exposed to user-space for all
> > devices, then we'd rather use actual sensor units (mV, m°C, whatever)
> > for resolution, and possibly add other attributes for the range. But
> 
> I assumed it would be in units. Not sure how to express resolution 
> as bit number in the first place.

This is how datasheets usually refer to the future. The configuration
bits let the user select between 9-, 10, 11 and 12-bit resolution ADC
(with increasing integration time.)

As most temperature sensors we deal with agree that 9-bit resolution
means LSB weight of 0.5°C, it could almost be used as a standard. But
of course there could be a device doing things differently someday,
which is why we probably don't want to use the ADC bit notation.

> > this means adding a whole lot of attribute files for some devices (if
> > we do it on a per-channel basis), so we first have to determine whether
> > it is really worth it. I don't think it is, but you can try and
> > convince me.
>
> Thinking about it, I'd rather keep it in mind in case someone else
> sees the need to it and convinces both of us...
> 
> Another question if both attributes (rate and resolution) would make sense 
> as module parameters instead. Are those attributes expected to vary across
> multiple instances of the same driver ?

I see no reason to use module parameters for these. It does make sense
to use different settings on different chips in the same family (e.g.
one is on the motherboard and the other on the graphics adapter), plus
sysfs attribute values are easier to change at run-time (these
attributes could even be added to libsensors.)

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC PATCH] Allow the configuration register to be

[Guenter Roeck]

On Tue, Sep 14, 2010 at 10:57:19AM -0400, Jean Delvare wrote:
> On Tue, 14 Sep 2010 05:01:28 -0700, Guenter Roeck wrote:
> > Hi Jean,
> > 
> > On Tue, Sep 14, 2010 at 04:08:33AM -0400, Jean Delvare wrote:
> > > On Mon, 13 Sep 2010 11:09:06 -0700, Guenter Roeck wrote:
> > > > On Mon, Sep 13, 2010 at 01:41:21PM -0400, Jean Delvare wrote:
> > > > > Not too sure about that. I had the same reaction at first, but do we
> > > > > actually know of devices where the resolution isn't global?
> > > >
> > > > Yes, in the lm90 driver. Only max6657/58/59 and max6646 have extended local
> > > > temperatures, all other chips have only 8 bit resolution for the local temperature.
> > > > Remote temperatures are all extended, ie have higher resoution.
> > > 
> > > But resolution can't be changed on these chips, can it? I think the
> > 
> > Correct.
> > 
> > > main purpose of these files is to let the user change the resolution
> > > for chips which support this (usually this is an update speed /
> > > resolution trade-off). Or do you believe that having read-only
> > > attributes exposing fixed resolutions would be valuable?
> > > 
> > Good point. Probably not really. More like nice to have.
> > 
> > > I'm a little worried because resolution as a bit number isn't too
> > > informative and doesn't quite work as a device-independent value. If
> > > you really want the information to be exposed to user-space for all
> > > devices, then we'd rather use actual sensor units (mV, m°C, whatever)
> > > for resolution, and possibly add other attributes for the range. But
> > 
> > I assumed it would be in units. Not sure how to express resolution 
> > as bit number in the first place.
> 
> This is how datasheets usually refer to the future. The configuration
> bits let the user select between 9-, 10, 11 and 12-bit resolution ADC
> (with increasing integration time.)
> 
> As most temperature sensors we deal with agree that 9-bit resolution
> means LSB weight of 0.5°C, it could almost be used as a standard. But
> of course there could be a device doing things differently someday,
> which is why we probably don't want to use the ADC bit notation.
> 
Agreed.

> > > this means adding a whole lot of attribute files for some devices (if
> > > we do it on a per-channel basis), so we first have to determine whether
> > > it is really worth it. I don't think it is, but you can try and
> > > convince me.
> >
> > Thinking about it, I'd rather keep it in mind in case someone else
> > sees the need to it and convinces both of us...
> > 
> > Another question if both attributes (rate and resolution) would make sense 
> > as module parameters instead. Are those attributes expected to vary across
> > multiple instances of the same driver ?
> 
> I see no reason to use module parameters for these. It does make sense
> to use different settings on different chips in the same family (e.g.
> one is on the motherboard and the other on the graphics adapter), plus
> sysfs attribute values are easier to change at run-time (these
> attributes could even be added to libsensors.)
> 
Makes sense.

Guenter

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