[lm-sensors] tempX_source sysfs attribute needed ?

[lm-sensors] tempX_source sysfs attribute needed ?

[Guenter Roeck]

Hi all,

in the recent months, I have seen two instances where a sysfs attribute 
identifying the source associated with a temperature sensor would have been 
helpful.

One is max6639. For this chip, the source of the second temperature channel
can be configured to be local or remote.

The others are W83677HG-B, NCT6775F, and NCT6776F. Those chips have four to six
temperature channels with a configurable source.

Current approach is to stick with whatever is configured by the BIOS for the Nuvoton
chips, and to use the local source for the max6639 driver. For the Nuvoton chips,
my prototype driver for NCT6775F and NCT6776F reports the temperature source 
in tempX_label.

Possible solutions might be:
- Stick with the current situation, ie do nothing.
- Use module parameters. Doesn't really work well for max6639 since it affects 
  all instances of the driver, and if the driver is built into the kernel.
- Use platform data. Might work for max6639, but not for the Nuvoton chips.
- Define a new sysfs attribute to make the temperature source configurable,
  and to report the current selection.

Any thoughts/comments ?

Thanks,
Guenter

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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Ian Dobson]

--------------------------------------------------
From: "Guenter Roeck" <guenter.roeck@ericsson.com>
Sent: Tuesday, February 08, 2011 6:10 PM
To: <lm-sensors@lm-sensors.org>
Subject: [lm-sensors] tempX_source sysfs attribute needed ?

> Hi all,
>
> in the recent months, I have seen two instances where a sysfs attribute
> identifying the source associated with a temperature sensor would have 
> been
> helpful.
>
> One is max6639. For this chip, the source of the second temperature 
> channel
> can be configured to be local or remote.
>
> The others are W83677HG-B, NCT6775F, and NCT6776F. Those chips have four 
> to six
> temperature channels with a configurable source.
>
> Current approach is to stick with whatever is configured by the BIOS for 
> the Nuvoton
> chips, and to use the local source for the max6639 driver. For the Nuvoton 
> chips,
> my prototype driver for NCT6775F and NCT6776F reports the temperature 
> source
> in tempX_label.
>
> Possible solutions might be:
> - Stick with the current situation, ie do nothing.
> - Use module parameters. Doesn't really work well for max6639 since it 
> affects
>  all instances of the driver, and if the driver is built into the kernel.
> - Use platform data. Might work for max6639, but not for the Nuvoton 
> chips.
> - Define a new sysfs attribute to make the temperature source 
> configurable,
>  and to report the current selection.
>
> Any thoughts/comments ?
>
> Thanks,
> Guenter
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Hi Guenter,

I actually think it's a good idea (tempX_label), I've already seen that 
sensors can use this information if you don't have a label defined in 
sensors3.conf. It actually makes is alot easier to configure the 
sensors3.conf file correctly. I'm not sure if _label is the correct name, it 
really describes the source of the signal.

Also with the nuv chips what happens if you change the source for a 
temperature input that's used by smartfan? The BIOS should know what's best 
and playing with the fan control inputs could cause all sorts of problems. 
During my hacking in the nct6776f driver I managed to screw up the smartfan 
control so much that the temperature setpoint for one of the fans was 90°c 
and the fan stopped.

Regards
Ian Dobson
 


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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Guenter Roeck]

On Tue, 2011-02-08 at 13:03 -0500, Ian Dobson wrote:
> 
> --------------------------------------------------
> From: "Guenter Roeck" <guenter.roeck@ericsson.com>
> Sent: Tuesday, February 08, 2011 6:10 PM
> To: <lm-sensors@lm-sensors.org>
> Subject: [lm-sensors] tempX_source sysfs attribute needed ?
> 
> > Hi all,
> >
> > in the recent months, I have seen two instances where a sysfs attribute
> > identifying the source associated with a temperature sensor would have 
> > been
> > helpful.
> >
> > One is max6639. For this chip, the source of the second temperature 
> > channel
> > can be configured to be local or remote.
> >
> > The others are W83677HG-B, NCT6775F, and NCT6776F. Those chips have four 
> > to six
> > temperature channels with a configurable source.
> >
> > Current approach is to stick with whatever is configured by the BIOS for 
> > the Nuvoton
> > chips, and to use the local source for the max6639 driver. For the Nuvoton 
> > chips,
> > my prototype driver for NCT6775F and NCT6776F reports the temperature 
> > source
> > in tempX_label.
> >
> > Possible solutions might be:
> > - Stick with the current situation, ie do nothing.
> > - Use module parameters. Doesn't really work well for max6639 since it 
> > affects
> >  all instances of the driver, and if the driver is built into the kernel.
> > - Use platform data. Might work for max6639, but not for the Nuvoton 
> > chips.
> > - Define a new sysfs attribute to make the temperature source 
> > configurable,
> >  and to report the current selection.
> >
> > Any thoughts/comments ?
> >
> > Thanks,
> > Guenter
> >
> > _______________________________________________
> > lm-sensors mailing list
> > lm-sensors@lm-sensors.org
> > http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
> 
> Hi Guenter,
> 
> I actually think it's a good idea (tempX_label), I've already seen that 
> sensors can use this information if you don't have a label defined in 
> sensors3.conf. It actually makes is alot easier to configure the 
> sensors3.conf file correctly. I'm not sure if _label is the correct name, it 
> really describes the source of the signal.
> 
tempX_label is a textual description, so I think it does make sense for
this driver. 

> Also with the nuv chips what happens if you change the source for a 
> temperature input that's used by smartfan? The BIOS should know what's best 
> and playing with the fan control inputs could cause all sorts of problems. 
> During my hacking in the nct6776f driver I managed to screw up the smartfan 
> control so much that the temperature setpoint for one of the fans was 90°c 
> and the fan stopped.
> 
Yes, that is always a risk. 

I don't really know if tempX_source adds any real value - that is why I
was asking for feedback. Ultimately the information is very board
specific, so providing tempX_label for the Nuvoton chips and using - if
ever needed - platform data for the max6639 may well be good enough.

Thanks,
Guenter



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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Jean Delvare]

Hi Guenter,

Sorry for the late reply.

On Tue, 8 Feb 2011 09:10:26 -0800, Guenter Roeck wrote:
> in the recent months, I have seen two instances where a sysfs attribute 
> identifying the source associated with a temperature sensor would have been 
> helpful.
> 
> One is max6639. For this chip, the source of the second temperature channel
> can be configured to be local or remote.
> 
> The others are W83677HG-B, NCT6775F, and NCT6776F. Those chips have four to six
> temperature channels with a configurable source.
> 
> Current approach is to stick with whatever is configured by the BIOS for the Nuvoton
> chips,

Definitely a good approach. For PC boards, we always assumed that the
BIOS had configured that kind of things properly, and it is almost
always the case.

> and to use the local source for the max6639 driver.

I guess that the local source is always available while the remote
source isn't necessarily, and that dictated the decision? It makes some
sense, but given that the hardware default is local, I'd rather just
read the value from the configuration register, and trust it. It's
either the hardware default and it is safe, or it was changed by the
BIOS/firmware and can certainly be trusted.

> For the Nuvoton chips,
> my prototype driver for NCT6775F and NCT6776F reports the temperature source 
> in tempX_label.

I presume this can't hurt, as we usually don't have anything else to
report here.

> Possible solutions might be:
> - Stick with the current situation, ie do nothing.
> - Use module parameters. Doesn't really work well for max6639 since it affects 
>   all instances of the driver, and if the driver is built into the kernel.

Array parameters exist, and passing parameters to built-in modules is
supported for years, so you don't really have an excuse if you want to
go this way. That being said, we purposely got rid of this in i2c
drivers are wide not so long ago, so I wouldn't encourage you to do
that unless you really have to.

You may still look at driver thmc50 for a counter-example. Module
parameter adm1022_temp3 is there for historical reasons, I don't think
we would accept it if the driver was submitted for inclusion today.

> - Use platform data. Might work for max6639, but not for the Nuvoton chips.

For max6639 this would definitely be the right thing to do, if the
kernel (not the BIOS/firmware) has to configure the device.

> - Define a new sysfs attribute to make the temperature source configurable,
>   and to report the current selection.

This seems very difficult and dangerous to me. Difficult because I'd be
very surprised if you can find a stable standard for source
identification. Each chip, each board will have its own possibilities.
So it would be up to each driver to list what possible source values
are valid. This doesn't seem to fit in the current sysfs interface /
libsensors implementation. Not to say it can't be done, but it's not
particularly appealing.

Dangerous because in many cases, only one configuration will be valid
for the given hardware, so letting the user change it may result in
an invalid setup, in turn leading to wrong temperature values, in turn
leading to a confused user at best and a confused automatic fan speed
control (with the potential to fry the hardware) in the worst case.

This is a generic problem, BTW, and the approach I have been advocating
so far is to leave things unchanged, don't let the user change them,
and simply report the chip-specific settings in the kernel logs if they
are relevant. See the lm63 driver for example. This may sound limited,
but apparently it worked well enough in practice and complaints have
been few.

Back to the specific problem of temperature sources, we may not get
away with it. I recall think along the same lines you're doing now when
I started reviewing the fan control part of the w83795 driver. The chip
doesn't map fans to real temperature channels, but to virtual ones,
which are in turn mapped to real ones via a kind of multiplexer. It
doesn't fit in our sysfs interface. Thankfully it only affected the
automatic fan speed control tweaking, which I consider an optional
feature.

I'll definitely have to look into it again someday, and maybe after
reading your code for the NCT6775F and NCT6776F, I'll have a clearer
view of what can be done. But don't hold your breath, it's nowhere near
the top of my to-do list currently.

-- 
Jean Delvare

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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Guenter Roeck]

Hi Jean,

On Mon, Mar 07, 2011 at 11:54:24AM -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> Sorry for the late reply.
> 
No problem. We are all busy ...

> On Tue, 8 Feb 2011 09:10:26 -0800, Guenter Roeck wrote:
> > in the recent months, I have seen two instances where a sysfs attribute 
> > identifying the source associated with a temperature sensor would have been 
> > helpful.
> > 
> > One is max6639. For this chip, the source of the second temperature channel
> > can be configured to be local or remote.
> > 
> > The others are W83677HG-B, NCT6775F, and NCT6776F. Those chips have four to six
> > temperature channels with a configurable source.
> > 
> > Current approach is to stick with whatever is configured by the BIOS for the Nuvoton
> > chips,
> 
> Definitely a good approach. For PC boards, we always assumed that the
> BIOS had configured that kind of things properly, and it is almost
> always the case.
> 
> > and to use the local source for the max6639 driver.
> 
> I guess that the local source is always available while the remote
> source isn't necessarily, and that dictated the decision? It makes some
> sense, but given that the hardware default is local, I'd rather just
> read the value from the configuration register, and trust it. It's
> either the hardware default and it is safe, or it was changed by the
> BIOS/firmware and can certainly be trusted.
> 
I'd have to ask the driver developer, but I guess that is what they use
on their board.

> > For the Nuvoton chips,
> > my prototype driver for NCT6775F and NCT6776F reports the temperature source 
> > in tempX_label.
> 
> I presume this can't hurt, as we usually don't have anything else to
> report here.
> 
> > Possible solutions might be:
> > - Stick with the current situation, ie do nothing.
> > - Use module parameters. Doesn't really work well for max6639 since it affects 
> >   all instances of the driver, and if the driver is built into the kernel.
> 
> Array parameters exist, and passing parameters to built-in modules is
> supported for years, so you don't really have an excuse if you want to
> go this way. That being said, we purposely got rid of this in i2c
> drivers are wide not so long ago, so I wouldn't encourage you to do
> that unless you really have to.
> 
> You may still look at driver thmc50 for a counter-example. Module
> parameter adm1022_temp3 is there for historical reasons, I don't think
> we would accept it if the driver was submitted for inclusion today.
> 
> > - Use platform data. Might work for max6639, but not for the Nuvoton chips.
> 
> For max6639 this would definitely be the right thing to do, if the
> kernel (not the BIOS/firmware) has to configure the device.
> 
Ok, I think I'll continue along that path. The parameter isn't there right now
in the max6639 driver, but it can be added anytime if needed. Plus, nowadays there
is also the option of using the device tree.

> > - Define a new sysfs attribute to make the temperature source configurable,
> >   and to report the current selection.
> 
> This seems very difficult and dangerous to me. Difficult because I'd be
> very surprised if you can find a stable standard for source
> identification. Each chip, each board will have its own possibilities.
> So it would be up to each driver to list what possible source values
> are valid. This doesn't seem to fit in the current sysfs interface /
> libsensors implementation. Not to say it can't be done, but it's not
> particularly appealing.
> 
> Dangerous because in many cases, only one configuration will be valid
> for the given hardware, so letting the user change it may result in
> an invalid setup, in turn leading to wrong temperature values, in turn
> leading to a confused user at best and a confused automatic fan speed
> control (with the potential to fry the hardware) in the worst case.
> 
Yes, I agree. This is too board specific to let the user configure it
during runtime.

> This is a generic problem, BTW, and the approach I have been advocating
> so far is to leave things unchanged, don't let the user change them,
> and simply report the chip-specific settings in the kernel logs if they
> are relevant. See the lm63 driver for example. This may sound limited,
> but apparently it worked well enough in practice and complaints have
> been few.
> 
> Back to the specific problem of temperature sources, we may not get
> away with it. I recall think along the same lines you're doing now when
> I started reviewing the fan control part of the w83795 driver. The chip
> doesn't map fans to real temperature channels, but to virtual ones,
> which are in turn mapped to real ones via a kind of multiplexer. It
> doesn't fit in our sysfs interface. Thankfully it only affected the
> automatic fan speed control tweaking, which I consider an optional
> feature.
> 
> I'll definitely have to look into it again someday, and maybe after
> reading your code for the NCT6775F and NCT6776F, I'll have a clearer
> view of what can be done. But don't hold your breath, it's nowhere near
> the top of my to-do list currently.
> 
Mine not either. Frankly, I pretty much forgot about it. Just too much to do.

Speaking about NCT6775F/NCT6776F - did you follow the fan debounce discussion ?
I advocated using a module parameter there, but your response above seems 
to suggest that this may not be the way to go (I still prefer it, though).
Other options would be a non-standard sysfs attribute or to always enable it.
It would help to get your input on this.

Thanks,
Guenter

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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Jean Delvare]

On Mon, 7 Mar 2011 09:58:51 -0800, Guenter Roeck wrote:
> Speaking about NCT6775F/NCT6776F - did you follow the fan debounce discussion ?

No, I did not.

> I advocated using a module parameter there, but your response above seems 
> to suggest that this may not be the way to go (I still prefer it, though).
> Other options would be a non-standard sysfs attribute or to always enable it.
> It would help to get your input on this.

This isn't an I2C device/driver, and you can assume a single instance
on a given system, so module parameters are just fine if they solve
your problem.

-- 
Jean Delvare

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

Re: [lm-sensors] tempX_source sysfs attribute needed ?

[Guenter Roeck]

On Mon, Mar 07, 2011 at 01:19:37PM -0500, Jean Delvare wrote:
> On Mon, 7 Mar 2011 09:58:51 -0800, Guenter Roeck wrote:
> > Speaking about NCT6775F/NCT6776F - did you follow the fan debounce discussion ?
> 
> No, I did not.
> 
> > I advocated using a module parameter there, but your response above seems 
> > to suggest that this may not be the way to go (I still prefer it, though).
> > Other options would be a non-standard sysfs attribute or to always enable it.
> > It would help to get your input on this.
> 
> This isn't an I2C device/driver, and you can assume a single instance
> on a given system, so module parameters are just fine if they solve
> your problem.
> 
Sounds good.

Thanks,
Guenter

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