CONFIG_THERMAL_HWMON in thermal_sys.c

CONFIG_THERMAL_HWMON in thermal_sys.c

[R, Durgadoss]

[-- Attachment #1.1: Type: text/plain, Size: 302 bytes --]

Hi Jean/Guenter,

In thermal_sys.c we have a CONFIG_THERMAL_HWMON.
If enabled, this creates sysfs nodes for thermal zones inside
hwmon. Do we need this functionality inside thermal_sys.c ?
Or is it Ok to remove this ?

I wanted to know your opinion before submitting a patch.

Thanks,
Durga

[-- Attachment #1.2: Type: text/html, Size: 21679 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Guenter Roeck]

On Tue, Sep 11, 2012 at 05:07:29AM +0000, R, Durgadoss wrote:
> Hi Jean/Guenter,
> 
> In thermal_sys.c we have a CONFIG_THERMAL_HWMON.
> If enabled, this creates sysfs nodes for thermal zones inside
> hwmon. Do we need this functionality inside thermal_sys.c ?
> Or is it Ok to remove this ?
> 
> I wanted to know your opinion before submitting a patch.
> 
Hi Durga,

question is really what would replace it. From the plumbers conference,
I understand the idea was that drivers would have to register to both hwmon
and the thermal subsystem. Is that going to happen ?

Second question is why you want to remove it in the first place. It only
matters for drivers which _do_ register with both hwmon and thermal - in
other words, none today. This case could be handled with a new thermal API,
which would specifically exclude hwmon registration.

My concern at this point with the thermal plans is that you might end up
duplicating a bunch of temperature sensor drivers in the thermal subsystem.
And if drivers are to support both subsystems, you would end up duplicating
a lot of common code in drivers which support both the thermal subsystem
and hwmon.

I personally don't think that is a good idea, and would prefer a more unified
approach where it would be possible to register a hwmon temperature sensor driver
with the thermal subsystem. Since that would be quite limited in scope, it might
not be too difficult to define an API for it which should both simplify hwmon
temperature sensor drivers and provide an interface to the thermal subsystem.

Thanks,
Guenter

RE: CONFIG_THERMAL_HWMON in thermal_sys.c

[R, Durgadoss]

Hi Guenter,

Thanks for taking the time to reply.

> -----Original Message-----
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> owner@vger.kernel.org] On Behalf Of Guenter Roeck
> Sent: Tuesday, September 11, 2012 11:00 AM
> To: R, Durgadoss
> Cc: Guenter Roeck (guenter.roeck@ericsson.com); Jean Delvare
> (khali@linux-fr.org); Zhang, Rui; lenb@kernel.org; lm-sensors (lm-
> sensors@lm-sensors.org); linux-acpi@vger.kernel.org
> Subject: Re: CONFIG_THERMAL_HWMON in thermal_sys.c
> 
> On Tue, Sep 11, 2012 at 05:07:29AM +0000, R, Durgadoss wrote:
> > Hi Jean/Guenter,
> >
> > In thermal_sys.c we have a CONFIG_THERMAL_HWMON.
> > If enabled, this creates sysfs nodes for thermal zones inside
> > hwmon. Do we need this functionality inside thermal_sys.c ?
> > Or is it Ok to remove this ?
> >
> > I wanted to know your opinion before submitting a patch.
> >
> Hi Durga,
> 
> question is really what would replace it. From the plumbers conference,
> I understand the idea was that drivers would have to register to both hwmon
> and the thermal subsystem. Is that going to happen ?

The thermal subsystem provides a standard sysfs interface today, and more
drivers are starting to use it. The idea is, if a thermal sensor driver wants to
participate in the platform Thermal management, (not just reporting
temperature from hardware, but taking actions based on that) it should
register with Thermal subsystem, and expose sysfs under /sys/class/thermal/
as a thermal_zone. For this registration there is already an EXPORT_SYMBOL
API, that can be used.

If the driver is a hwmon driver, and wants to participate in Thermal management,
it can
a) Expose its temperature through hwmon sysfs (as it exists today)
b) register with thermal subsystem through the provided API. (optional)

If the driver is a thermal driver (placed inside drivers/thermal/) it can directly
use the thermal subsystem APIs to expose/monitor temperature. In this case,
I don't see a need for this kind of driver to use CONFIG_HWMON.

> 
> Second question is why you want to remove it in the first place. It only

We are (kind of) re-implementing the Thermal framework to have more
robust thermal monitoring. So, thought we can make this implementation
also better.

> matters for drivers which _do_ register with both hwmon and thermal - in
> other words, none today. This case could be handled with a new thermal API,
> which would specifically exclude hwmon registration.
> 
> My concern at this point with the thermal plans is that you might end up
> duplicating a bunch of temperature sensor drivers in the thermal subsystem.
> And if drivers are to support both subsystems, you would end up duplicating
> a lot of common code in drivers which support both the thermal subsystem
> and hwmon.

I agree with both of your points. Please see below.

> 
> I personally don't think that is a good idea, and would prefer a more unified
> approach where it would be possible to register a hwmon temperature
> sensor driver
> with the thermal subsystem. Since that would be quite limited in scope, it
> might
> not be too difficult to define an API for it which should both simplify hwmon
> temperature sensor drivers and provide an interface to the thermal
> subsystem.

Yes, an API exists. The drivers in hwmon (if they want to) can register with
Thermal subsystem using thermal_zone_device_register() API.

My thought of starting this discussion was for the other set of drivers,
which are inside drivers/thermal/

Thanks,
Durga

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Jean Delvare]

Hi Durga,

On Tue, 11 Sep 2012 05:07:29 +0000, R, Durgadoss wrote:
> In thermal_sys.c we have a CONFIG_THERMAL_HWMON.
> If enabled, this creates sysfs nodes for thermal zones inside
> hwmon. Do we need this functionality inside thermal_sys.c ?
> Or is it Ok to remove this ?

It isn't OK to remove it unless you offer an alternative implementation
doing the same.

The idea is that thermal zones and in particular ACPI thermal zones
show up in the "sensors" command, and are thus available to monitoring
applications through libsensors. If you want to remove the bridge code
from thermal zones to hwmon, you must either:
* Register every thermal device as a hwmon device explicitly. I don't
  see any point in doing that, the generic code we have today is
  certainly easier to maintain.
* Move the generic code somewhere else. I know there have been a lot of
  discussions about thermal devices lately, I admit I did not follow
  them. If there's a better place for the generic code now, that's fine
  with me.
* Extend libsensors to be able to deal with thermal drivers directly.
  If thermal devices are presented with a reasonable sysfs interface,
  then why not.

But I'm not sure I understand what problem you are trying to solve.
What's wrong with the current code?

-- 
Jean Delvare

RE: CONFIG_THERMAL_HWMON in thermal_sys.c

[R, Durgadoss]

Hi Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: Tuesday, September 11, 2012 12:42 PM
> To: R, Durgadoss
> Cc: Guenter Roeck; Zhang, Rui; Len Brown; lm-sensors@lm-sensors.org;
> linux-acpi@vger.kernel.org
> Subject: Re: CONFIG_THERMAL_HWMON in thermal_sys.c
> 
> Hi Durga,
> 
> On Tue, 11 Sep 2012 05:07:29 +0000, R, Durgadoss wrote:
> > In thermal_sys.c we have a CONFIG_THERMAL_HWMON.
> > If enabled, this creates sysfs nodes for thermal zones inside
> > hwmon. Do we need this functionality inside thermal_sys.c ?
> > Or is it Ok to remove this ?
> 
> It isn't OK to remove it unless you offer an alternative implementation
> doing the same.
> 
> The idea is that thermal zones and in particular ACPI thermal zones
> show up in the "sensors" command, and are thus available to monitoring
> applications through libsensors. If you want to remove the bridge code

Oh .. I did not know this. Thanks for educating me !!

> from thermal zones to hwmon, you must either:
> * Register every thermal device as a hwmon device explicitly. I don't
>   see any point in doing that, the generic code we have today is
>   certainly easier to maintain.

Completely agree with you. I don't want to do this.

> * Move the generic code somewhere else. I know there have been a lot of
>   discussions about thermal devices lately, I admit I did not follow
>   them. If there's a better place for the generic code now, that's fine
>   with me.
> * Extend libsensors to be able to deal with thermal drivers directly.
>   If thermal devices are presented with a reasonable sysfs interface,
>   then why not.

Yes, The thermal subsystem presents all sysfs under /sys/class/thermal/
as thermal_zoneX and all cooling devices under /sys/class/thermal/
as cooling_deviceY. The framework does some monitoring to manage
the platform thermals.

> 
> But I'm not sure I understand what problem you are trying to solve.
> What's wrong with the current code?

Nothing wrong with the current code. My thought was (before your
mail educated me):
1. Certainly hwmon drivers don't need this #define inside thermal_sys.c
2. Drivers inside thermal subsystem are not gaining anything using this
#define.

Now that I know about sensors/libsensors, if the purpose of using
this #define is to expose the thermal zones to 'sensors' command,
then I think it's better we go with your third option (above), since
we have the sysfs interfaces standardized (/sys/class/thermal/)

Thanks,
Durga

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Jean Delvare]

On Tue, 11 Sep 2012 08:55:34 +0000, R, Durgadoss wrote:
> > -----Original Message-----
> > From: Jean Delvare [mailto:khali@linux-fr.org]
> > * Extend libsensors to be able to deal with thermal drivers directly.
> >   If thermal devices are presented with a reasonable sysfs interface,
> >   then why not.
> (...)
> Now that I know about sensors/libsensors, if the purpose of using
> this #define is to expose the thermal zones to 'sensors' command,
> then I think it's better we go with your third option (above), since
> we have the sysfs interfaces standardized (/sys/class/thermal/)

Is there a document describing this sysfs interface?

Will you write and submit the required libsensors patches, or do you
expect me to take care of that?

Note: if the same device can now be registered as both hwmon and
thermal, we will have to find a way to detect that to avoid duplicate
entries in libsensors.

-- 
Jean Delvare

RE: CONFIG_THERMAL_HWMON in thermal_sys.c

[R, Durgadoss]

Hi Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: Tuesday, September 11, 2012 2:34 PM
> To: R, Durgadoss
> Cc: Guenter Roeck; Zhang, Rui; Len Brown; lm-sensors@lm-sensors.org;
> linux-acpi@vger.kernel.org
> Subject: Re: CONFIG_THERMAL_HWMON in thermal_sys.c
> 
> On Tue, 11 Sep 2012 08:55:34 +0000, R, Durgadoss wrote:
> > > -----Original Message-----
> > > From: Jean Delvare [mailto:khali@linux-fr.org]
> > > * Extend libsensors to be able to deal with thermal drivers directly.
> > >   If thermal devices are presented with a reasonable sysfs interface,
> > >   then why not.
> > (...)
> > Now that I know about sensors/libsensors, if the purpose of using
> > this #define is to expose the thermal zones to 'sensors' command,
> > then I think it's better we go with your third option (above), since
> > we have the sysfs interfaces standardized (/sys/class/thermal/)
> 
> Is there a document describing this sysfs interface?

It is in Documentation/thermal/sysfs-api.txt

> 
> Will you write and submit the required libsensors patches, or do you
> expect me to take care of that?

I have not tried anything with libsensors until now..so it will take
some time for me. If you can wait, I can happily submit the required
patches :-)

But, we are changing the thermal framework quite a bit these days.
In some time, things should become saner. Until then, we will not
remove this #define and keep it as it is. Once that happens, we can
take care of removing this #defined code and also patching
libsensors.

We can do this now also. I know, we are changing things on
the thermal side (sysfs attribute names etc..).  If we patch 
libsensors now, then we might have to re-visit again, soon.
That's why, I would like to defer this until things settle down on the
Thermal side.

Let me know what you think.

> 
> Note: if the same device can now be registered as both hwmon and
> thermal, we will have to find a way to detect that to avoid duplicate
> entries in libsensors.
 
The hwmon sysfs has a 'name' field. We can pass the same string as the
first argument of the thermal_zone_device_register() call. This will
show up as 'type' attribute under /sys/class/thermal/thermal_zoneX.
So, from user space, we can compare these interfaces and thus avoid
the duplicates.

Thank you Jean,
Durga

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Jean Delvare]

On Tue, 11 Sep 2012 09:24:27 +0000, R, Durgadoss wrote:
> I have not tried anything with libsensors until now..so it will take
> some time for me. If you can wait, I can happily submit the required
> patches :-)
> 
> But, we are changing the thermal framework quite a bit these days.
> In some time, things should become saner. Until then, we will not
> remove this #define and keep it as it is. Once that happens, we can
> take care of removing this #defined code and also patching
> libsensors.
> 
> We can do this now also. I know, we are changing things on
> the thermal side (sysfs attribute names etc..).  If we patch 
> libsensors now, then we might have to re-visit again, soon.
> That's why, I would like to defer this until things settle down on the
> Thermal side.
> 
> Let me know what you think.

I agree, let's wait for the interface to stabilize, there's no point in
updating libsensors publicly now if the interface is going to change in
a near future.

That being said you may also want to anticipate a bit in private, to
make sure that the interface as it exists today is enough for
libsensors.

> > Note: if the same device can now be registered as both hwmon and
> > thermal, we will have to find a way to detect that to avoid duplicate
> > entries in libsensors.
>  
> The hwmon sysfs has a 'name' field. We can pass the same string as the
> first argument of the thermal_zone_device_register() call. This will
> show up as 'type' attribute under /sys/class/thermal/thermal_zoneX.
> So, from user space, we can compare these interfaces and thus avoid
> the duplicates.

This is a start, yes. However the name isn't necessarily unique, so
more tricks may be needed.

-- 
Jean Delvare

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Guenter Roeck]

On Tue, Sep 11, 2012 at 11:49:21AM +0200, Jean Delvare wrote:
> On Tue, 11 Sep 2012 09:24:27 +0000, R, Durgadoss wrote:
> > I have not tried anything with libsensors until now..so it will take
> > some time for me. If you can wait, I can happily submit the required
> > patches :-)
> > 
> > But, we are changing the thermal framework quite a bit these days.
> > In some time, things should become saner. Until then, we will not
> > remove this #define and keep it as it is. Once that happens, we can
> > take care of removing this #defined code and also patching
> > libsensors.
> > 
> > We can do this now also. I know, we are changing things on
> > the thermal side (sysfs attribute names etc..).  If we patch 
> > libsensors now, then we might have to re-visit again, soon.
> > That's why, I would like to defer this until things settle down on the
> > Thermal side.
> > 
> > Let me know what you think.
> 
> I agree, let's wait for the interface to stabilize, there's no point in
> updating libsensors publicly now if the interface is going to change in
> a near future.
> 
> That being said you may also want to anticipate a bit in private, to
> make sure that the interface as it exists today is enough for
> libsensors.
> 
> > > Note: if the same device can now be registered as both hwmon and
> > > thermal, we will have to find a way to detect that to avoid duplicate
> > > entries in libsensors.
> >  
> > The hwmon sysfs has a 'name' field. We can pass the same string as the
> > first argument of the thermal_zone_device_register() call. This will
> > show up as 'type' attribute under /sys/class/thermal/thermal_zoneX.
> > So, from user space, we can compare these interfaces and thus avoid
> > the duplicates.
> 
> This is a start, yes. However the name isn't necessarily unique, so
> more tricks may be needed.
> 
I do wonder what is going to happen with existing hwmon temperature sensor
drivers which are needed as thermal drivers. Re-write/copy ?

Guenter

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Jean Delvare]

On Tue, 11 Sep 2012 06:45:05 -0700, Guenter Roeck wrote:
> I do wonder what is going to happen with existing hwmon temperature sensor
> drivers which are needed as thermal drivers. Re-write/copy ?

Have them register as thermal devices and leave their hwmon interface
unchanged?

For simple temperature sensor devices, the current situation (expose
thermal devices as hwmon devices automatically) works well, but if a
more complex devices (with, say, voltage sensors) needs to be exposed
as a thermal device then it doesn't work. If we want to handle this
case, then I see no alternative to having each driver implement both
interfaces.

-- 
Jean Delvare

Re: CONFIG_THERMAL_HWMON in thermal_sys.c

[Guenter Roeck]

On Tue, Sep 11, 2012 at 04:05:38PM +0200, Jean Delvare wrote:
> On Tue, 11 Sep 2012 06:45:05 -0700, Guenter Roeck wrote:
> > I do wonder what is going to happen with existing hwmon temperature sensor
> > drivers which are needed as thermal drivers. Re-write/copy ?
> 
> Have them register as thermal devices and leave their hwmon interface
> unchanged?
> 
> For simple temperature sensor devices, the current situation (expose
> thermal devices as hwmon devices automatically) works well, but if a
> more complex devices (with, say, voltage sensors) needs to be exposed
> as a thermal device then it doesn't work. If we want to handle this
> case, then I see no alternative to having each driver implement both
> interfaces.
> 
Ok, makes sense.

Guenter