[lm-sensors] Driver for MAX6696 temperature sensor

[lm-sensors] Driver for MAX6696 temperature sensor

[Guenter Roeck]

Hi all,

one of the hwmon drivers I'll have to write is for MAX6696. This chip is similar to lm90 / max6680,
only it supports three temperature sensors instead of two. The two external sensors share one
set of registers.

What is the better approach - write a new driver based on the lm90 driver,
or modify the lm90 driver to support max6696 and thus optionally three channels ?
Any thoughts ?

Guenter

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Jean Delvare]

Hi Guenter,

On Wed, 30 Jun 2010 18:18:31 -0700, Guenter Roeck wrote:
> one of the hwmon drivers I'll have to write is for MAX6696. This chip is similar to lm90 / max6680,
> only it supports three temperature sensors instead of two. The two external sensors share one
> set of registers.

How does it work exactly?

> What is the better approach - write a new driver based on the lm90 driver,
> or modify the lm90 driver to support max6696 and thus optionally three channels ?
> Any thoughts ?

Both are acceptable. Best choice depends on how intrusive it is to add
support to the lm90 driver. I suspect there won't be too much to add? I
would recommend trying it, and if we finally decide it's too intrusive,
copy the lm90 driver to a new one and clean up them both.

-- 
Jean Delvare

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Jean Delvare]

On Thu, 1 Jul 2010 07:52:44 -0700, Guenter Roeck wrote:
> On Thu, Jul 01, 2010 at 02:33:11AM -0400, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Wed, 30 Jun 2010 18:18:31 -0700, Guenter Roeck wrote:
> > > one of the hwmon drivers I'll have to write is for MAX6696. This chip is similar to lm90 / max6680,
> > > only it supports three temperature sensors instead of two. The two external sensors share one
> > > set of registers.
> > 
> > How does it work exactly?
> > 
> Setting Command register bit 3 switches the limit and temperature registers to the
> second external channel, and there is a second status register for its status bits.

I see. strange approach, I wonder why they did it that way.

> > > What is the better approach - write a new driver based on the lm90 driver,
> > > or modify the lm90 driver to support max6696 and thus optionally three channels ?
> > > Any thoughts ?
> > 
> > Both are acceptable. Best choice depends on how intrusive it is to add
> > support to the lm90 driver. I suspect there won't be too much to add? I
> > would recommend trying it, and if we finally decide it's too intrusive,
> > copy the lm90 driver to a new one and clean up them both.
> > 
> I implemented a prototype last night. It adds about 130 lines of code, and changes about 20.
> A few things like chip detection are still missing, so it will probably end up adding
> maybe 150 lines of code.

Seems reasonable. And maybe another pair of eyes who know the lm90
driver well will have suggestions to make it even smaller :)

-- 
Jean Delvare

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Jean Delvare]

Hi Guenter,

On Thu, 1 Jul 2010 18:17:24 -0700, Guenter Roeck wrote:
> On Thu, Jul 01, 2010 at 11:05:01AM -0400, Jean Delvare wrote:
> > On Thu, 1 Jul 2010 07:52:44 -0700, Guenter Roeck wrote:
> > > I implemented a prototype last night. It adds about 130 lines of code, and changes about 20.
> > > A few things like chip detection are still missing, so it will probably end up adding
> > > maybe 150 lines of code.
> > 
> > Seems reasonable. And maybe another pair of eyes who know the lm90
> > driver well will have suggestions to make it even smaller :)
> 
> Agreed.
> 
> Followup question: The chip supports three limits per sensor - ALERT, OT1, and OT2.
> Default settings are along the line of 70, 90, and 120 degrees C. OT2 typically causes
> a board shutdown.
> 
> Current API only allows for two limits, so I am using ALERT for min/max, OT1 for crit,
> and ignore OT2. Any idea if/how we could report OT2 as well ?

I have no objection adding another limit to the sysfs-interface. I seem
to recall that a few other thermal sensors would benefit from it.

Could take the form of temp[1-*]_warn or temp[1-*]_crit2 (or any other
suggestion you may have). The w83795 driver I'm currently working on
has temp5_warn and temp6_warn. The values are lower than the ones in
temp5_max and temp6_max, respectively, so it seems different from your
own case. I'll check the register descriptions.

Note that our decision might change the pertinence of adding support
for the MAX6696 to the lm90 driver.

-- 
Jean Delvare

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Jean Delvare]

Hi Guenter,

On Fri, 2 Jul 2010 01:38:30 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 03:52:04AM -0400, Jean Delvare wrote:
> > Hi Guenter,
> > 
> > On Thu, 1 Jul 2010 18:17:24 -0700, Guenter Roeck wrote:
> > > On Thu, Jul 01, 2010 at 11:05:01AM -0400, Jean Delvare wrote:
> > > > On Thu, 1 Jul 2010 07:52:44 -0700, Guenter Roeck wrote:
> > > > > I implemented a prototype last night. It adds about 130 lines of code, and changes about 20.
> > > > > A few things like chip detection are still missing, so it will probably end up adding
> > > > > maybe 150 lines of code.
> > > > 
> > > > Seems reasonable. And maybe another pair of eyes who know the lm90
> > > > driver well will have suggestions to make it even smaller :)
> > > 
> > > Agreed.
> > > 
> > > Followup question: The chip supports three limits per sensor - ALERT, OT1, and OT2.
> > > Default settings are along the line of 70, 90, and 120 degrees C. OT2 typically causes
> > > a board shutdown.
> > > 
> > > Current API only allows for two limits, so I am using ALERT for min/max, OT1 for crit,
> > > and ignore OT2. Any idea if/how we could report OT2 as well ?
> > 
> > I have no objection adding another limit to the sysfs-interface. I seem
> > to recall that a few other thermal sensors would benefit from it.
> > 
> > Could take the form of temp[1-*]_warn or temp[1-*]_crit2 (or any other
> > suggestion you may have). The w83795 driver I'm currently working on
> 
> temp[1-*]_crit2 would probably be a better fit. "warn" doesn't seem right
> for a temperature causing a board reset.

Of course it really depends on what each limit does. "warn" would
probably be the lowest high limit, only warning the user / admin that
some action will be taken later if the temperature keeps raising.

Maybe temp[1-*]_emergency would be better than temp[1-*]_crit2?

> > has temp5_warn and temp6_warn. The values are lower than the ones in
> > temp5_max and temp6_max, respectively, so it seems different from your
> > own case. I'll check the register descriptions.
> > 
> > Note that our decision might change the pertinence of adding support
> > for the MAX6696 to the lm90 driver.
> 
> Code would not have to change much, though. All I have to add is some code to 
> read the additional registers, and add three more sysfs attributes for the _crit2
> temperatures. Since I already added several attributes for the third sensor, 
> this is not much additional change.

OK, alright then.

-- 
Jean Delvare

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Guenter Roeck]

On Fri, Jul 02, 2010 at 05:53:45AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri, 2 Jul 2010 01:38:30 -0700, Guenter Roeck wrote:
> > On Fri, Jul 02, 2010 at 03:52:04AM -0400, Jean Delvare wrote:
> > > Hi Guenter,
> > > 
> > > On Thu, 1 Jul 2010 18:17:24 -0700, Guenter Roeck wrote:
> > > > On Thu, Jul 01, 2010 at 11:05:01AM -0400, Jean Delvare wrote:
> > > > > On Thu, 1 Jul 2010 07:52:44 -0700, Guenter Roeck wrote:
> > > > > > I implemented a prototype last night. It adds about 130 lines of code, and changes about 20.
> > > > > > A few things like chip detection are still missing, so it will probably end up adding
> > > > > > maybe 150 lines of code.
> > > > > 
> > > > > Seems reasonable. And maybe another pair of eyes who know the lm90
> > > > > driver well will have suggestions to make it even smaller :)
> > > > 
> > > > Agreed.
> > > > 
> > > > Followup question: The chip supports three limits per sensor - ALERT, OT1, and OT2.
> > > > Default settings are along the line of 70, 90, and 120 degrees C. OT2 typically causes
> > > > a board shutdown.
> > > > 
> > > > Current API only allows for two limits, so I am using ALERT for min/max, OT1 for crit,
> > > > and ignore OT2. Any idea if/how we could report OT2 as well ?
> > > 
> > > I have no objection adding another limit to the sysfs-interface. I seem
> > > to recall that a few other thermal sensors would benefit from it.
> > > 
> > > Could take the form of temp[1-*]_warn or temp[1-*]_crit2 (or any other
> > > suggestion you may have). The w83795 driver I'm currently working on
> > 
> > temp[1-*]_crit2 would probably be a better fit. "warn" doesn't seem right
> > for a temperature causing a board reset.
> 
> Of course it really depends on what each limit does. "warn" would
> probably be the lowest high limit, only warning the user / admin that
> some action will be taken later if the temperature keeps raising.
> 
> Maybe temp[1-*]_emergency would be better than temp[1-*]_crit2?
> 
Since it would be the highest limit, temp[1-*]_emergency sounds really good. 

Guenter

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

Re: [lm-sensors] Driver for MAX6696 temperature sensor

[Jean Delvare]

On Fri, 2 Jul 2010 07:14:06 -0700, Guenter Roeck wrote:
> On Fri, Jul 02, 2010 at 05:53:45AM -0400, Jean Delvare wrote:
> > Of course it really depends on what each limit does. "warn" would
> > probably be the lowest high limit, only warning the user / admin that
> > some action will be taken later if the temperature keeps raising.
> > 
> > Maybe temp[1-*]_emergency would be better than temp[1-*]_crit2?
> > 
> Since it would be the highest limit, temp[1-*]_emergency sounds really good. 

Feel free to submit a documentation patch adding this then.

-- 
Jean Delvare

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