[lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Andre Prendel]

Hello Jean, Hans,

here is my first work at TI's 401/411 temperatur sensor. The patch(es)
aren't intented to be applied. I just want some comments and maybe
real testing by Hans.

The first patch is the original one from Hans sent to me.

The second patch contains some work of Hans' students. I've brought
it in shape. The driver now has multichip support in the usual way.

TODO:
Update Kconfig
Writing documentation

There are a few remaining questions.

1. Hans' students implemented two additional sysfs entries
(tempX_[lowest|highest]).

The TMP411 chip has some additional registers for minimum and maximum
temperatures measured since power-on and chip-reset.

AFAIK there are no standard sysfs entries for these values. I don't
remove the support so far. Should it be removed? Are there other chips
with such features?

2. There is another command to reset the chip. Is this a useful feature?

3. Indentation

What is the right way to indent arguments in function declaration?

a)
static int tmp401_probe(struct i2c_client *client,
                        const struct i2c_device_id *id)

b)
static int tmp401_probe(struct i2c_client *client,
       const struct i2c_device_id *id)

My emacs prefered the first one, but sometimes this doesn't look very
well.

4. May I add a copyright note of me in the header?

Again, thank you very much for your help. It's a pleasure to work with
you.

Thanks,
Andre

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Hans de Goede]

Hi Andre,

On 05/14/2009 10:06 AM, Andre Prendel wrote:
> Hello Jean, Hans,
>
> here is my first work at TI's 401/411 temperatur sensor. The patch(es)
> aren't intented to be applied. I just want some comments and maybe
> real testing by Hans.
>
> The first patch is the original one from Hans sent to me.
>
> The second patch contains some work of Hans' students. I've brought
> it in shape. The driver now has multichip support in the usual way.
>
> TODO:
> Update Kconfig
> Writing documentation
>

Great, thanks for working on this!

> There are a few remaining questions.
>
> 1. Hans' students implemented two additional sysfs entries
> (tempX_[lowest|highest]).
>
> The TMP411 chip has some additional registers for minimum and maximum
> temperatures measured since power-on and chip-reset.
>
> AFAIK there are no standard sysfs entries for these values. I don't
> remove the support so far. Should it be removed? Are there other chips
> with such features?
>

Ah, good point I forgot about that, I told my student to name the new
tempX_[lowest|highest] based on the existing and documented
(Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
sysfs attributes, which do the same (tracking min / max over time) for
powermeter IC's. So unless someone objects, I think these should stay
and get documented in Documentation/hwmon/sysfs-interface.

> 2. There is another command to reset the chip. Is this a useful feature?
>

An other very good point! My first response was no, but actually it is,
as this can be used to reset the min / max temp tracking. Modelling
after the powermeter API again, you should add a temp_reset_history, notice
no number there as it resets the history for all temps at once (AFAIK).

Again this needs to be documented, in 2 variants: temp_reset_history and
tempX_reset_history

> 3. Indentation
>
> What is the right way to indent arguments in function declaration?
>
> a)
> static int tmp401_probe(struct i2c_client *client,
>                          const struct i2c_device_id *id)
>
> b)
> static int tmp401_probe(struct i2c_client *client,
>         const struct i2c_device_id *id)
>
> My emacs prefered the first one, but sometimes this doesn't look very
> well.
>

Erm, I think both are allowed, I myself usually use b, but I don't think
I'm consistent with that.

> 4. May I add a copyright note of me in the header?
>

Sure.


> Again, thank you very much for your help. It's a pleasure to work with
> you.

And it is a pleasure to work with you sir!


Regards,

Hans


p.s.

I somehow am missing patch 2/2, or am I just lacking patience ?


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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Andre Prendel]

On Thu, May 14, 2009 at 11:02:40AM +0200, Hans de Goede wrote:
> Hi Andre,
>
> On 05/14/2009 10:06 AM, Andre Prendel wrote:
>> Hello Jean, Hans,
>>
>> here is my first work at TI's 401/411 temperatur sensor. The patch(es)
>> aren't intented to be applied. I just want some comments and maybe
>> real testing by Hans.
>>
>> The first patch is the original one from Hans sent to me.
>>
>> The second patch contains some work of Hans' students. I've brought
>> it in shape. The driver now has multichip support in the usual way.
>>
>> TODO:
>> Update Kconfig
>> Writing documentation
>>
>
> Great, thanks for working on this!
>
>> There are a few remaining questions.
>>
>> 1. Hans' students implemented two additional sysfs entries
>> (tempX_[lowest|highest]).
>>
>> The TMP411 chip has some additional registers for minimum and maximum
>> temperatures measured since power-on and chip-reset.
>>
>> AFAIK there are no standard sysfs entries for these values. I don't
>> remove the support so far. Should it be removed? Are there other chips
>> with such features?
>>
>
> Ah, good point I forgot about that, I told my student to name the new
> tempX_[lowest|highest] based on the existing and documented
> (Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
> sysfs attributes, which do the same (tracking min / max over time) for
> powermeter IC's. So unless someone objects, I think these should stay
> and get documented in Documentation/hwmon/sysfs-interface.
>
>> 2. There is another command to reset the chip. Is this a useful feature?
>>
>
> An other very good point! My first response was no, but actually it is,
> as this can be used to reset the min / max temp tracking. Modelling
> after the powermeter API again, you should add a temp_reset_history, notice
> no number there as it resets the history for all temps at once (AFAIK).
>
> Again this needs to be documented, in 2 variants: temp_reset_history and
> tempX_reset_history
>
>> 3. Indentation
>>
>> What is the right way to indent arguments in function declaration?
>>
>> a)
>> static int tmp401_probe(struct i2c_client *client,
>>                          const struct i2c_device_id *id)
>>
>> b)
>> static int tmp401_probe(struct i2c_client *client,
>>         const struct i2c_device_id *id)
>>
>> My emacs prefered the first one, but sometimes this doesn't look very
>> well.
>>
>
> Erm, I think both are allowed, I myself usually use b, but I don't think
> I'm consistent with that.
>
>> 4. May I add a copyright note of me in the header?
>>
>
> Sure.
>
>
>> Again, thank you very much for your help. It's a pleasure to work with
>> you.
>
> And it is a pleasure to work with you sir!
>
>
> Regards,
>
> Hans
>
>
> p.s.
>
> I somehow am missing patch 2/2, or am I just lacking patience ?

I sent the patches one by one. I don't know what's happend. No
delivery failure so far. I will just send the last patch again.

Andre

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Jean Delvare]

On Thu, 14 May 2009 11:02:40 +0200, Hans de Goede wrote:
> Hi Andre,
> 
> On 05/14/2009 10:06 AM, Andre Prendel wrote:
> > Hello Jean, Hans,
> >
> > here is my first work at TI's 401/411 temperatur sensor. The patch(es)
> > aren't intented to be applied. I just want some comments and maybe
> > real testing by Hans.
> >
> > The first patch is the original one from Hans sent to me.
> >
> > The second patch contains some work of Hans' students. I've brought
> > it in shape. The driver now has multichip support in the usual way.
> >
> > TODO:
> > Update Kconfig
> > Writing documentation
> >
> 
> Great, thanks for working on this!
> 
> > There are a few remaining questions.
> >
> > 1. Hans' students implemented two additional sysfs entries
> > (tempX_[lowest|highest]).
> >
> > The TMP411 chip has some additional registers for minimum and maximum
> > temperatures measured since power-on and chip-reset.
> >
> > AFAIK there are no standard sysfs entries for these values. I don't
> > remove the support so far. Should it be removed? Are there other chips
> > with such features?
> 
> Ah, good point I forgot about that, I told my student to name the new
> tempX_[lowest|highest] based on the existing and documented
> (Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
> sysfs attributes, which do the same (tracking min / max over time) for
> powermeter IC's. So unless someone objects, I think these should stay
> and get documented in Documentation/hwmon/sysfs-interface.

I agree. This is a very useful feature IMHO and I am surprised more
sensor chips don't implement it.

> > 2. There is another command to reset the chip. Is this a useful feature?
> 
> An other very good point! My first response was no, but actually it is,
> as this can be used to reset the min / max temp tracking. Modelling
> after the powermeter API again, you should add a temp_reset_history, notice
> no number there as it resets the history for all temps at once (AFAIK).
> 
> Again this needs to be documented, in 2 variants: temp_reset_history and
> tempX_reset_history

Agreed.

> > 3. Indentation
> >
> > What is the right way to indent arguments in function declaration?
> >
> > a)
> > static int tmp401_probe(struct i2c_client *client,
> >                          const struct i2c_device_id *id)
> >
> > b)
> > static int tmp401_probe(struct i2c_client *client,
> >         const struct i2c_device_id *id)
> >
> > My emacs prefered the first one, but sometimes this doesn't look very
> > well.
> >
> 
> Erm, I think both are allowed, I myself usually use b, but I don't think
> I'm consistent with that.

Both are OK, just try to be consistent within a given driver.

> > 4. May I add a copyright note of me in the header?
> 
> Sure.

+1 :)

> > Again, thank you very much for your help. It's a pleasure to work with
> > you.
> 
> And it is a pleasure to work with you sir!

Same here :)

What a useful reply from me this was, wasn't it? :D

-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Andre Prendel]

On Thu, May 14, 2009 at 01:20:00PM +0200, Jean Delvare wrote:
> On Thu, 14 May 2009 11:02:40 +0200, Hans de Goede wrote:
> > Hi Andre,
> > 
> > On 05/14/2009 10:06 AM, Andre Prendel wrote:
> > > Hello Jean, Hans,
> > >
> > > here is my first work at TI's 401/411 temperatur sensor. The patch(es)
> > > aren't intented to be applied. I just want some comments and maybe
> > > real testing by Hans.
> > >
> > > The first patch is the original one from Hans sent to me.
> > >
> > > The second patch contains some work of Hans' students. I've brought
> > > it in shape. The driver now has multichip support in the usual way.
> > >
> > > TODO:
> > > Update Kconfig
> > > Writing documentation
> > >
> > 
> > Great, thanks for working on this!
> > 
> > > There are a few remaining questions.
> > >
> > > 1. Hans' students implemented two additional sysfs entries
> > > (tempX_[lowest|highest]).
> > >
> > > The TMP411 chip has some additional registers for minimum and maximum
> > > temperatures measured since power-on and chip-reset.
> > >
> > > AFAIK there are no standard sysfs entries for these values. I don't
> > > remove the support so far. Should it be removed? Are there other chips
> > > with such features?
> > 
> > Ah, good point I forgot about that, I told my student to name the new
> > tempX_[lowest|highest] based on the existing and documented
> > (Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
> > sysfs attributes, which do the same (tracking min / max over time) for
> > powermeter IC's. So unless someone objects, I think these should stay
> > and get documented in Documentation/hwmon/sysfs-interface.
> 
> I agree. This is a very useful feature IMHO and I am surprised more
> sensor chips don't implement it.
> 
> > > 2. There is another command to reset the chip. Is this a useful feature?
> > 
> > An other very good point! My first response was no, but actually it is,
> > as this can be used to reset the min / max temp tracking. Modelling
> > after the powermeter API again, you should add a temp_reset_history, notice
> > no number there as it resets the history for all temps at once (AFAIK).
> > 
> > Again this needs to be documented, in 2 variants: temp_reset_history and
> > tempX_reset_history
> 
> Agreed.

Ok, I will add support for it to the driver.

> 
> > > 3. Indentation
> > >
> > > What is the right way to indent arguments in function declaration?
> > >
> > > a)
> > > static int tmp401_probe(struct i2c_client *client,
> > >                          const struct i2c_device_id *id)
> > >
> > > b)
> > > static int tmp401_probe(struct i2c_client *client,
> > >         const struct i2c_device_id *id)
> > >
> > > My emacs prefered the first one, but sometimes this doesn't look very
> > > well.
> > >
> > 
> > Erm, I think both are allowed, I myself usually use b, but I don't think
> > I'm consistent with that.
> 
> Both are OK, just try to be consistent within a given driver.
> 
> > > 4. May I add a copyright note of me in the header?
> > 
> > Sure.

Nice, so I will do it :)

> 
> +1 :)
> 
> > > Again, thank you very much for your help. It's a pleasure to work with
> > > you.
> > 
> > And it is a pleasure to work with you sir!
> 
> Same here :)
> 
> What a useful reply from me this was, wasn't it? :D

That's right :)

Andre

> -- 
> Jean Delvare

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Andre Prendel]

On Thu, May 14, 2009 at 11:02:40AM +0200, Hans de Goede wrote:
> Hi Andre,
>
> On 05/14/2009 10:06 AM, Andre Prendel wrote:
>> Hello Jean, Hans,
>>
>> here is my first work at TI's 401/411 temperatur sensor. The patch(es)
>> aren't intented to be applied. I just want some comments and maybe
>> real testing by Hans.
>>
>> The first patch is the original one from Hans sent to me.
>>
>> The second patch contains some work of Hans' students. I've brought
>> it in shape. The driver now has multichip support in the usual way.
>>
>> TODO:
>> Update Kconfig
>> Writing documentation
>>
>
> Great, thanks for working on this!
>
>> There are a few remaining questions.
>>
>> 1. Hans' students implemented two additional sysfs entries
>> (tempX_[lowest|highest]).
>>
>> The TMP411 chip has some additional registers for minimum and maximum
>> temperatures measured since power-on and chip-reset.
>>
>> AFAIK there are no standard sysfs entries for these values. I don't
>> remove the support so far. Should it be removed? Are there other chips
>> with such features?
>>
>
> Ah, good point I forgot about that, I told my student to name the new
> tempX_[lowest|highest] based on the existing and documented
> (Documentation/hwmon/sysfs-interface) powerX_average_[lowest|highest]
> sysfs attributes, which do the same (tracking min / max over time) for
> powermeter IC's. So unless someone objects, I think these should stay
> and get documented in Documentation/hwmon/sysfs-interface.
>
>> 2. There is another command to reset the chip. Is this a useful feature?
>>
>
> An other very good point! My first response was no, but actually it is,
> as this can be used to reset the min / max temp tracking. Modelling
> after the powermeter API again, you should add a temp_reset_history, notice
> no number there as it resets the history for all temps at once
(AFAIK).

Having a closer look at the datasheet

http://focus.ti.com/docs/prod/folders/print/tmp411.html

, I've seen two ways to reset the history.

1. Writing any value to the history registers (0x30-0x37). That should
reset all these registers. I've attached a patch (delta) doing this. Hans,
could you please test this one. echo "1"> .../temp_reset_history
should reset the history for tempX_lowest (0xFFF0) and tempX_highest (0x0000).

2. SOFTWARE RESET

Writing the first e-mail I meant this command in my question. You can
find it in datasheet under SOFTWARE RESET. This command restores the
power-on values to all registers (including limits etc.).

I'd prefer 1. for reseting the history. That's what I'd expect from
temp_reset_history.

IMHO if we want to use the SOFTWARE RESET we should use another
attribute (e.g. temp_reset).

BTW, what about write-only attributes? I've seen a reset functionality
in the fscpos driver (so I did it like that). show_temp_reset always
returns 1. Using WO attributes could make more sense, but in
sysfs-interface I can only see RO and RW.

>
> Again this needs to be documented, in 2 variants: temp_reset_history and
> tempX_reset_history
>

[...]

> Regards,
>
> Hans

Andre
---

--- /drivers/hwmon/tmp401.c	2009-05-17 17:37:03.000000000 +0200
+++ drivers/hwmon/tmp401.c	2009-05-17 17:38:00.000000000 +0200
@@ -1,9 +1,9 @@
 /* tmp401.c
  *
  * Copyright (C) 2007,2008 Hans de Goede <hdegoede@redhat.com>
- * Gabriel Konat
- * Sander Leget
- * Wouter Willems
+ * Preliminary tmp411 support by:
+ * Gabriel Konat, Sander Leget, Wouter Willems
+ * Copyright (C) 2009 Andre Prendel <andre.prendel@gmx.de>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -285,6 +285,12 @@
 		return sprintf(buf, "0\n");
 }
 
+static ssize_t show_temp_history(struct device *dev,
+	struct device_attribute *devattr, char *buf)
+{
+	return sprintf(buf, "1\n");
+}
+
 static ssize_t store_temp_min(struct device *dev,
 	struct device_attribute	*devattr, const char *buf, size_t count)
 {
@@ -396,6 +402,30 @@
 	return count;
 }
 
+/*
+ * Resets the historical measurements of minimum and maximum temperatures.
+ * This is done by writing any value to any of the minimum/maximum registers
+ * (0x30-0x37).
+ */
+static ssize_t reset_temp_history(struct device *dev,
+	struct device_attribute	*devattr, const char *buf, size_t count)
+{
+	long val;
+
+	if (strict_strtol(buf, 10, &val))
+		return -EINVAL;
+
+	if (val != 1) {
+		dev_err(dev, "temp_reset_history value %ld not"
+			" supported. Use 1 to reset the history!\n", val);
+		return -EINVAL;
+	}
+	i2c_smbus_write_byte_data(to_i2c_client(dev),
+		TMP411_TEMP_LOWEST_MSB[0], val);
+
+	return count;
+}
+
 static struct sensor_device_attribute tmp401_attr[] = {
 	SENSOR_ATTR(temp1_input, 0444, show_temp_value, NULL, 0),
 	SENSOR_ATTR(temp1_min, 0644, show_temp_min, store_temp_min, 0),
@@ -436,6 +466,8 @@
 	SENSOR_ATTR(temp1_lowest, 0444, show_temp_lowest, NULL, 0),
 	SENSOR_ATTR(temp2_highest, 0444, show_temp_highest, NULL, 1),
 	SENSOR_ATTR(temp2_lowest, 0444, show_temp_lowest, NULL, 1),
+	SENSOR_ATTR(temp_reset_history, 0644, show_temp_history,
+		    reset_temp_history, 0),
 };
 
 /*
@@ -606,7 +638,7 @@
 		data->temp_crit[i] = i2c_smbus_read_byte_data(client,
 			TMP401_TEMP_CRIT_LIMIT[i]);
 
-		if(data->kind = TMP411_DEVICE_ID) {
+		if (data->kind = tmp411) {
 			data->temp_lowest[i] = i2c_smbus_read_byte_data(client,
 				TMP411_TEMP_LOWEST_MSB[i]) << 8;
 			data->temp_lowest[i] |= i2c_smbus_read_byte_data(

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Hans de Goede]

On 05/18/2009 11:37 AM, Andre Prendel wrote:
> Having a closer look at the datasheet
>
> http://focus.ti.com/docs/prod/folders/print/tmp411.html
>
> , I've seen two ways to reset the history.
>
> 1. Writing any value to the history registers (0x30-0x37). That should
> reset all these registers. I've attached a patch (delta) doing this. Hans,
> could you please test this one. echo "1">  .../temp_reset_history
> should reset the history for tempX_lowest (0xFFF0) and tempX_highest (0x0000).
>
> 2. SOFTWARE RESET
>
> Writing the first e-mail I meant this command in my question. You can
> find it in datasheet under SOFTWARE RESET. This command restores the
> power-on values to all registers (including limits etc.).
>
> I'd prefer 1. for reseting the history. That's what I'd expect from
> temp_reset_history.
>

Ack, I think 1 is much better too.

> BTW, what about write-only attributes? I've seen a reset functionality
> in the fscpos driver (so I did it like that). show_temp_reset always
> returns 1. Using WO attributes could make more sense, but in
> sysfs-interface I can only see RO and RW.
>

The fscpos driver is an old obsolete driver, so not the best place to look
for inspiration. I think that using 0200 as rights for something like
temp_reset_history makes perfect sense.

I've tested you're latest patch and temp_reset_history works as advertised.

If you fix the mode for the temp_reset_history, and do a new patch adding
docs, then I think this is good to go to Jean's tree for merging into 2.6.31.
Jean do you agree?

Regards,

Hans

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

Re: [lm-sensors] [PATCH RFC 0/2] tmp401: Add support for Texas

[Jean Delvare]

On Mon, 18 May 2009 12:18:10 +0200, Hans de Goede wrote:
> I've tested you're latest patch and temp_reset_history works as advertised.
> 
> If you fix the mode for the temp_reset_history, and do a new patch adding
> docs, then I think this is good to go to Jean's tree for merging into 2.6.31.
> Jean do you agree?

Yes, I agree.

-- 
Jean Delvare

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