[lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

Hi,

I am working on adding automatic fan control configuration to the
f75375s driver.  I have code that is almost complete and working well,
but I wonder what is the best way to expose this in the sysfs.

The F75387 fan control chip configuration can be fully switched between
open loop mode, where it is programmed with raw PWM values, and closed
loop mode, where one specifies desired RPM fan speeds instead.

However, both modes use the same configuration input registers.

The lm-sensors standard sysfs interface defines that RPM target values
should be exposed using filenames fan*_target, and PWM values via pwm*.
For automatic, temperature-bounded fan control values, only
pwm*_auto_point*_pwm files are defined.

Now I wonder, what is the best way to expose the configuration for the
F75387 in sysfs?

Trying to match the standard sysfs interface makes me think I need to
expose different files in sysfs, depending on the mode.  That is, pwm*
and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
sure what to put into $TYPE...  Any suggestions for $TYPE?

But then, if the user switches between open and closed loop mode, all
these mode-specific filenames in sysfs need be switched over, since it
would make no sense to keep the files for the inactive mode around,
right?  This makes me feel slightly unhappy.  Thoughts?

Of course, one could inhibit the mode switching and just stick with the
mode that was enabled by the BIOS.  But I don't see why the user should
not be empowered to switch modes; and I have tested this, and it works
like a charm.

I would very much appreciate your thoughts on this.

Thanks,
Nikolaus

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Guenter Roeck]

On Thu, 2012-02-09 at 17:23 -0500, Nikolaus Schulz wrote:
> Hi,
> 
> I am working on adding automatic fan control configuration to the
> f75375s driver.  I have code that is almost complete and working well,
> but I wonder what is the best way to expose this in the sysfs.
> 
> The F75387 fan control chip configuration can be fully switched between
> open loop mode, where it is programmed with raw PWM values, and closed
> loop mode, where one specifies desired RPM fan speeds instead.
> 
> However, both modes use the same configuration input registers.
> 
> The lm-sensors standard sysfs interface defines that RPM target values
> should be exposed using filenames fan*_target, and PWM values via pwm*.
> For automatic, temperature-bounded fan control values, only
> pwm*_auto_point*_pwm files are defined.
> 
> Now I wonder, what is the best way to expose the configuration for the
> F75387 in sysfs?
> 
> Trying to match the standard sysfs interface makes me think I need to
> expose different files in sysfs, depending on the mode.  That is, pwm*
> and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
> fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
> sure what to put into $TYPE...  Any suggestions for $TYPE?
> 

Maybe Jean has an idea here. There are various drivers with slightly
different (and sometimes undocumented) pwm attributes around. Maybe it
is time to clean up the ABI and define a complete (or at least more
complete) set of attributes.

> But then, if the user switches between open and closed loop mode, all
> these mode-specific filenames in sysfs need be switched over, since it
> would make no sense to keep the files for the inactive mode around,
> right?  This makes me feel slightly unhappy.  Thoughts?
> 
You should be able to keep all attributes around, even if some may not
be active at a given time. Otherwise, you would not be able to program
auto mode parameters until you enabled auto mode ... which would not be
a good idea.

> Of course, one could inhibit the mode switching and just stick with the
> mode that was enabled by the BIOS.  But I don't see why the user should
> not be empowered to switch modes; and I have tested this, and it works
> like a charm.
> 
More a matter of avoiding risk, I would guess. If you let the user
program auto mode, you probably want to have some kind of validation
before the mode is actually enabled, to at least try to avoid board
fatalities. This is really highly driver specific, and has to be very
well tested.

I have been thinking about doing the same for the w83627ehf driver.
Pretty much the same problem - some attributes are missing in the ABI,
and others only apply to certain modes. So I am definitely interested in
a generic set of attibutes.

Guenter



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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

On Thu, Feb 09, 2012 at 02:34:54PM -0800, Guenter Roeck wrote:
> On Thu, 2012-02-09 at 17:23 -0500, Nikolaus Schulz wrote:
> > I am working on adding automatic fan control configuration to the
> > f75375s driver.  I have code that is almost complete and working well,
> > but I wonder what is the best way to expose this in the sysfs.
> > 
> > The F75387 fan control chip configuration can be fully switched between
> > open loop mode, where it is programmed with raw PWM values, and closed
> > loop mode, where one specifies desired RPM fan speeds instead.
> > 
> > However, both modes use the same configuration input registers.
> > 
> > The lm-sensors standard sysfs interface defines that RPM target values
> > should be exposed using filenames fan*_target, and PWM values via pwm*.
> > For automatic, temperature-bounded fan control values, only
> > pwm*_auto_point*_pwm files are defined.
> > 
> > Now I wonder, what is the best way to expose the configuration for the
> > F75387 in sysfs?
> > 
> > Trying to match the standard sysfs interface makes me think I need to
> > expose different files in sysfs, depending on the mode.  That is, pwm*
> > and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
> > fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
> > sure what to put into $TYPE...  Any suggestions for $TYPE?
> 
> Maybe Jean has an idea here. There are various drivers with slightly
> different (and sometimes undocumented) pwm attributes around. Maybe it
> is time to clean up the ABI and define a complete (or at least more
> complete) set of attributes.

Yes, it looks like the current sysfs interface standard is lacking.

> > But then, if the user switches between open and closed loop mode, all
> > these mode-specific filenames in sysfs need be switched over, since it
> > would make no sense to keep the files for the inactive mode around,
> > right?  This makes me feel slightly unhappy.  Thoughts?
> > 
> You should be able to keep all attributes around, even if some may not
> be active at a given time. Otherwise, you would not be able to program
> auto mode parameters until you enabled auto mode ... which would not be
> a good idea.

Unfortunately, the F75387 uses the very same registers for both modes.
The values are only interpreted differently, depending on the open vs
closed loop mode, either as RPM values, or raw PWM.

> > Of course, one could inhibit the mode switching and just stick with the
> > mode that was enabled by the BIOS.  But I don't see why the user should
> > not be empowered to switch modes; and I have tested this, and it works
> > like a charm.
> > 
> More a matter of avoiding risk, I would guess. If you let the user
> program auto mode, you probably want to have some kind of validation
> before the mode is actually enabled, to at least try to avoid board
> fatalities.

Not possible for the F75387, for the reason above.

> This is really highly driver specific, and has to be very
> well tested.

Agreed

> I have been thinking about doing the same for the w83627ehf driver.
> Pretty much the same problem - some attributes are missing in the ABI,
> and others only apply to certain modes. So I am definitely interested in
> a generic set of attibutes.
> 
> Guenter

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Guenter Roeck]

On Thu, Feb 09, 2012 at 05:54:52PM -0500, Nikolaus Schulz wrote:
> On Thu, Feb 09, 2012 at 02:34:54PM -0800, Guenter Roeck wrote:
> > On Thu, 2012-02-09 at 17:23 -0500, Nikolaus Schulz wrote:
> > > I am working on adding automatic fan control configuration to the
> > > f75375s driver.  I have code that is almost complete and working well,
> > > but I wonder what is the best way to expose this in the sysfs.
> > > 
> > > The F75387 fan control chip configuration can be fully switched between
> > > open loop mode, where it is programmed with raw PWM values, and closed
> > > loop mode, where one specifies desired RPM fan speeds instead.
> > > 
> > > However, both modes use the same configuration input registers.
> > > 
> > > The lm-sensors standard sysfs interface defines that RPM target values
> > > should be exposed using filenames fan*_target, and PWM values via pwm*.
> > > For automatic, temperature-bounded fan control values, only
> > > pwm*_auto_point*_pwm files are defined.
> > > 
> > > Now I wonder, what is the best way to expose the configuration for the
> > > F75387 in sysfs?
> > > 
> > > Trying to match the standard sysfs interface makes me think I need to
> > > expose different files in sysfs, depending on the mode.  That is, pwm*
> > > and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
> > > fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
> > > sure what to put into $TYPE...  Any suggestions for $TYPE?
> > 
> > Maybe Jean has an idea here. There are various drivers with slightly
> > different (and sometimes undocumented) pwm attributes around. Maybe it
> > is time to clean up the ABI and define a complete (or at least more
> > complete) set of attributes.
> 
> Yes, it looks like the current sysfs interface standard is lacking.
> 
> > > But then, if the user switches between open and closed loop mode, all
> > > these mode-specific filenames in sysfs need be switched over, since it
> > > would make no sense to keep the files for the inactive mode around,
> > > right?  This makes me feel slightly unhappy.  Thoughts?
> > > 
> > You should be able to keep all attributes around, even if some may not
> > be active at a given time. Otherwise, you would not be able to program
> > auto mode parameters until you enabled auto mode ... which would not be
> > a good idea.
> 
> Unfortunately, the F75387 uses the very same registers for both modes.
> The values are only interpreted differently, depending on the open vs
> closed loop mode, either as RPM values, or raw PWM.
> 
Other chips do the same, or something similar. For example, the NCT6775F
uses the same register for the target speed in one mode, and for the target
temperature in another.

In this example, we have to be able to set both. The w83627ehf driver "solves"
that problem by implementing pwmX_target, which is specified in the driver
documentation as target temperature. Depending on the mode, it can also be
interpreted by the chip as target speed. Not the most elegant solution,
but at least it works (though maybe we should document it ;).
Unless Jean has a better idea, I would suggest to do the same.

Guenter

> > > Of course, one could inhibit the mode switching and just stick with the
> > > mode that was enabled by the BIOS.  But I don't see why the user should
> > > not be empowered to switch modes; and I have tested this, and it works
> > > like a charm.
> > > 
> > More a matter of avoiding risk, I would guess. If you let the user
> > program auto mode, you probably want to have some kind of validation
> > before the mode is actually enabled, to at least try to avoid board
> > fatalities.
> 
> Not possible for the F75387, for the reason above.
> 
> > This is really highly driver specific, and has to be very
> > well tested.
> 
> Agreed
> 
> > I have been thinking about doing the same for the w83627ehf driver.
> > Pretty much the same problem - some attributes are missing in the ABI,
> > and others only apply to certain modes. So I am definitely interested in
> > a generic set of attibutes.
> > 
> > Guenter
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Jean Delvare]

On Thu, 9 Feb 2012 18:13:35 -0800, Guenter Roeck wrote:
> On Thu, Feb 09, 2012 at 05:54:52PM -0500, Nikolaus Schulz wrote:
> > Unfortunately, the F75387 uses the very same registers for both modes.
> > The values are only interpreted differently, depending on the open vs
> > closed loop mode, either as RPM values, or raw PWM.
>
> Other chips do the same, or something similar. For example, the NCT6775F
> uses the same register for the target speed in one mode, and for the target
> temperature in another.
> 
> In this example, we have to be able to set both. The w83627ehf driver "solves"
> that problem by implementing pwmX_target, which is specified in the driver
> documentation as target temperature. Depending on the mode, it can also be
> interpreted by the chip as target speed. Not the most elegant solution,
> but at least it works (though maybe we should document it ;).
> Unless Jean has a better idea, I would suggest to do the same.

In this specific case, what you want to do is decouple the register
cache from the actual hardware registers. In your per-device private
data structure, have room to store all the settings for all mode. This
means that a given hardware register can have 2 or more corresponding
members in the data structure, one for each meaning it can have
depending on the mode.

You can see an example of this in driver it87:

struct it87_data {
	(...)
	/* The following 3 arrays correspond to the same registers up to
	 * the IT8720F. The meaning of bits 6-0 depends on the value of bit
	 * 7, and we want to preserve settings on mode changes, so we have
	 * to track all values separately.
	 * Starting with the IT8721F, the manual PWM duty cycles are stored
	 * in separate registers (8-bit values), so the separate tracking
	 * is no longer needed, but it is still done to keep the driver
	 * simple. */
	u8 pwm_ctrl[3];		/* Register value */
	u8 pwm_duty[3];		/* Manual PWM value set by user */
	u8 pwm_temp_map[3];	/* PWM to temp. chan. mapping (bits 1-0) */

When reading the register value from the chip, you write them to the
right struct member for the given mode, and leave the other untouched.
When values are changed by the user through sysfs, you store them in
the data structure. If they relate to the currently selected mode, you
write them to the chip immediately. If not, you keep them for later.

When the user changes the mode through sysfs, you write that mode
change to the chip, and then automatically program the new mode using
the settings from the data structure.

So, to make it clear, it is perfectly possible (and desirable) to
handle such devices without coming up with a custom and obscure sysfs
interface. The shared registers can and must be abstracted by the
driver and the user shouldn't notice.

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Jean Delvare]

On Thu, 9 Feb 2012 23:54:52 +0100, Nikolaus Schulz wrote:
> On Thu, Feb 09, 2012 at 02:34:54PM -0800, Guenter Roeck wrote:
> > On Thu, 2012-02-09 at 17:23 -0500, Nikolaus Schulz wrote:
> > > I am working on adding automatic fan control configuration to the
> > > f75375s driver.  I have code that is almost complete and working well,
> > > but I wonder what is the best way to expose this in the sysfs.
> > > 
> > > The F75387 fan control chip configuration can be fully switched between
> > > open loop mode, where it is programmed with raw PWM values, and closed
> > > loop mode, where one specifies desired RPM fan speeds instead.
> > > 
> > > However, both modes use the same configuration input registers.
> > > 
> > > The lm-sensors standard sysfs interface defines that RPM target values
> > > should be exposed using filenames fan*_target, and PWM values via pwm*.
> > > For automatic, temperature-bounded fan control values, only
> > > pwm*_auto_point*_pwm files are defined.
> > > 
> > > Now I wonder, what is the best way to expose the configuration for the
> > > F75387 in sysfs?
> > > 
> > > Trying to match the standard sysfs interface makes me think I need to
> > > expose different files in sysfs, depending on the mode.  That is, pwm*
> > > and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
> > > fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
> > > sure what to put into $TYPE...  Any suggestions for $TYPE?

I'm not sure what exactly you want to put in this file, so I can't
answer. The mapping between PWM output and fan input? If so that would
be pwm[1-*]_auto_channels_fan (which isn't yet defined in out standard
sysfs interface but could be added.)

Or, if what your chip does is multiple RPM targets based on the
temperature, i.e. a mix of temperature trip points mode and fan speed
target mode, then indeed our sysfs interface is lacking standard file
names for this (I can't remember another chip supporting this.)
Following the other attribute names, the proper name could be
pwm[1-*]_auto_point[1-*]_fan_target.

I hope I replied to your question.

> > Maybe Jean has an idea here. There are various drivers with slightly
> > different (and sometimes undocumented) pwm attributes around. Maybe it
> > is time to clean up the ABI and define a complete (or at least more
> > complete) set of attributes.
> 
> Yes, it looks like the current sysfs interface standard is lacking.

Blame it on my, I designed it, years ago. I remember a number of people
argued about it, but I did not listen, in part because I am stubborn,
in part because they did not propose any alternative. This is possibly
the worse memory I have since I am involved in the lm-sensors project.

One (now) very obvious mistake I made back then is that I did not
abstract the fan speed controller units from PWM outputs. If we design
a new API then we really want to start with controllers, and then
associate one or more PWM (or DC) outputs, zero or more temperature
channels, and zero or more fan speed inputs to the controller. Note
that depending on the mode, temperature or fan input may be irrelevant.
Also note that for some (most?) chips, some of these settings will be
hard-coded and not changeable by the user. Lastly, note that in some
cases we will NOT know the associations, in particular for the fan
inputs, the chip only needs to know in speed target mode, so if the
chip doesn't support that, we just don't know.

I do not exactly have the amount of spare time it would take to work on
this right now. If someone wants to work on this, go ahead and I'll be
happy to review the proposal. If not, I'm putting this on my to-do list.

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Guenter Roeck]

On Fri, Feb 10, 2012 at 02:38:40AM -0500, Jean Delvare wrote:
> On Thu, 9 Feb 2012 18:13:35 -0800, Guenter Roeck wrote:
> > On Thu, Feb 09, 2012 at 05:54:52PM -0500, Nikolaus Schulz wrote:
> > > Unfortunately, the F75387 uses the very same registers for both modes.
> > > The values are only interpreted differently, depending on the open vs
> > > closed loop mode, either as RPM values, or raw PWM.
> >
> > Other chips do the same, or something similar. For example, the NCT6775F
> > uses the same register for the target speed in one mode, and for the target
> > temperature in another.
> > 
> > In this example, we have to be able to set both. The w83627ehf driver "solves"
> > that problem by implementing pwmX_target, which is specified in the driver
> > documentation as target temperature. Depending on the mode, it can also be
> > interpreted by the chip as target speed. Not the most elegant solution,
> > but at least it works (though maybe we should document it ;).
> > Unless Jean has a better idea, I would suggest to do the same.
> 
> In this specific case, what you want to do is decouple the register
> cache from the actual hardware registers. In your per-device private
> data structure, have room to store all the settings for all mode. This
> means that a given hardware register can have 2 or more corresponding
> members in the data structure, one for each meaning it can have
> depending on the mode.
> 
> You can see an example of this in driver it87:
> 
> struct it87_data {
> 	(...)
> 	/* The following 3 arrays correspond to the same registers up to
> 	 * the IT8720F. The meaning of bits 6-0 depends on the value of bit
> 	 * 7, and we want to preserve settings on mode changes, so we have
> 	 * to track all values separately.
> 	 * Starting with the IT8721F, the manual PWM duty cycles are stored
> 	 * in separate registers (8-bit values), so the separate tracking
> 	 * is no longer needed, but it is still done to keep the driver
> 	 * simple. */
> 	u8 pwm_ctrl[3];		/* Register value */
> 	u8 pwm_duty[3];		/* Manual PWM value set by user */
> 	u8 pwm_temp_map[3];	/* PWM to temp. chan. mapping (bits 1-0) */
> 
> When reading the register value from the chip, you write them to the
> right struct member for the given mode, and leave the other untouched.
> When values are changed by the user through sysfs, you store them in
> the data structure. If they relate to the currently selected mode, you
> write them to the chip immediately. If not, you keep them for later.
> 
> When the user changes the mode through sysfs, you write that mode
> change to the chip, and then automatically program the new mode using
> the settings from the data structure.
> 
> So, to make it clear, it is perfectly possible (and desirable) to
> handle such devices without coming up with a custom and obscure sysfs
> interface. The shared registers can and must be abstracted by the
> driver and the user shouldn't notice.
> 
Excellent idea. I think I'll do the same for the nct6775f if/when I have time.

Guenter

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

On Fri, Feb 10, 2012 at 08:38:40AM +0100, Jean Delvare wrote:
> On Thu, 9 Feb 2012 18:13:35 -0800, Guenter Roeck wrote:
> > On Thu, Feb 09, 2012 at 05:54:52PM -0500, Nikolaus Schulz wrote:
> > > Unfortunately, the F75387 uses the very same registers for both modes.
> > > The values are only interpreted differently, depending on the open vs
> > > closed loop mode, either as RPM values, or raw PWM.
> >
> > Other chips do the same, or something similar. For example, the NCT6775F
> > uses the same register for the target speed in one mode, and for the target
> > temperature in another.

[...]

> In this specific case, what you want to do is decouple the register
> cache from the actual hardware registers. In your per-device private
> data structure, have room to store all the settings for all mode. This
> means that a given hardware register can have 2 or more corresponding
> members in the data structure, one for each meaning it can have
> depending on the mode.
[...]
> When reading the register value from the chip, you write them to the
> right struct member for the given mode, and leave the other untouched.
> When values are changed by the user through sysfs, you store them in
> the data structure. If they relate to the currently selected mode, you
> write them to the chip immediately. If not, you keep them for later.
> 
> When the user changes the mode through sysfs, you write that mode
> change to the chip, and then automatically program the new mode using
> the settings from the data structure.

Very elegant and convincing.  My thinking obviously has been too tied to
the device specific details, thanks for your guidance. :)

I will apply this to the f75375 driver.

Nikolaus

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

On Fri, Feb 10, 2012 at 09:05:19AM +0100, Jean Delvare wrote:
> On Thu, 9 Feb 2012 23:54:52 +0100, Nikolaus Schulz wrote:
> > On Thu, Feb 09, 2012 at 02:34:54PM -0800, Guenter Roeck wrote:
> > > On Thu, 2012-02-09 at 17:23 -0500, Nikolaus Schulz wrote:
> > > > The F75387 fan control chip configuration can be fully switched between
> > > > open loop mode, where it is programmed with raw PWM values, and closed
> > > > loop mode, where one specifies desired RPM fan speeds instead.
> > > > 
> > > > However, both modes use the same configuration input registers.
> > > > 
> > > > The lm-sensors standard sysfs interface defines that RPM target values
> > > > should be exposed using filenames fan*_target, and PWM values via pwm*.
> > > > For automatic, temperature-bounded fan control values, only
> > > > pwm*_auto_point*_pwm files are defined.
> > > > 
> > > > Now I wonder, what is the best way to expose the configuration for the
> > > > F75387 in sysfs?
> > > > 
> > > > Trying to match the standard sysfs interface makes me think I need to
> > > > expose different files in sysfs, depending on the mode.  That is, pwm*
> > > > and pwm*_auto_point*_pwm for open loop mode, and for closed loop mode
> > > > fan*_target and, hmm, I guess ${TYPE}*_auto_point*_fan where I'm not
> > > > sure what to put into $TYPE...  Any suggestions for $TYPE?

[...]

> [...] if what your chip does is multiple RPM targets based on the
> temperature, i.e. a mix of temperature trip points mode and fan speed
> target mode, then indeed our sysfs interface is lacking standard file
> names for this

Yes, this is what the F75387 does in closed loop mode.  All fan target
values are then specified in RPM, not PWM, including the
temperature-bounded for automatic fan control.

> (I can't remember another chip supporting this.)

Apparently the F71805F does this, and it is supported by the hwmon
driver.  This driver uses the sysfs filenames pwm*_auto_point*_fan.

> Following the other attribute names, the proper name could be
> pwm[1-*]_auto_point[1-*]_fan_target.

That sounds better and more consistent than the name chosen for the
f71805f.

I am not very happy about using the pwm* prefix, since the fan speed
need not be driven using PWM at all.  This issue alone probably doesn't
warrant coming up with a more abstract naming scheme; unfortunately
there are more.  But still, until such a scheme exists, I'll stick to
your proposal.

> I hope I replied to your question.

You did, at least the part about the file naming, thanks. :)

> > > Maybe Jean has an idea here. There are various drivers with slightly
> > > different (and sometimes undocumented) pwm attributes around. Maybe it
> > > is time to clean up the ABI and define a complete (or at least more
> > > complete) set of attributes.
> > 
> > Yes, it looks like the current sysfs interface standard is lacking.
[...]
> 
> One (now) very obvious mistake I made back then is that I did not
> abstract the fan speed controller units from PWM outputs.

Right, that's exactly my remaining unhappiness above was about.

Let me add two more things.

First, I doubt that it's a good idea to put the fan*_target value into
the fan domain.  After all, this value is about fan *control*, not about
fan state, which I think suggests it might better reside in the pwm
domain (that is, the domain of the fan controller).  This is emphasized
by having fan*_target in the fan domain, and pwm*_auto_point*_fan_target
in the pwm domain, as you suggested above.  One domain should win here,
and I think it should be the latter.

Second, the F75387 draws a distinction between the actual (PWM or DAC)
output value and the configured target value.  The current definition of
pwm*, on the other hand, not only encodes the steering method (PWM), but
also mixes target value and actual value.  Does it make sense to keep
these values separate in sysfs?

> If we design a new API then we really want to start with controllers,
> and then associate one or more PWM (or DC) outputs, zero or more
> temperature channels, and zero or more fan speed inputs to the
> controller. Note that depending on the mode, temperature or fan input
> may be irrelevant.  Also note that for some (most?) chips, some of
> these settings will be hard-coded and not changeable by the user.
> Lastly, note that in some cases we will NOT know the associations, in
> particular for the fan inputs, the chip only needs to know in speed
> target mode, so if the chip doesn't support that, we just don't know.
> 
> I do not exactly have the amount of spare time it would take to work
> on this right now. If someone wants to work on this, go ahead and I'll
> be happy to review the proposal. If not, I'm putting this on my to-do
> list.

I doubt I will make it, due to lack of time, and lack of experience in
the sensors area. :/ 

Nikolaus

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Jean Delvare]

On Fri, 10 Feb 2012 23:56:07 +0100, Nikolaus Schulz wrote:
> On Fri, Feb 10, 2012 at 09:05:19AM +0100, Jean Delvare wrote:
> > [...] if what your chip does is multiple RPM targets based on the
> > temperature, i.e. a mix of temperature trip points mode and fan speed
> > target mode, then indeed our sysfs interface is lacking standard file
> > names for this
> 
> Yes, this is what the F75387 does in closed loop mode.  All fan target
> values are then specified in RPM, not PWM, including the
> temperature-bounded for automatic fan control.
> 
> > (I can't remember another chip supporting this.)
> 
> Apparently the F71805F does this, and it is supported by the hwmon
> driver.  This driver uses the sysfs filenames pwm*_auto_point*_fan.

Wow. This is my driver, I didn't write this part of the code but I
signed than patch so I must have reviewed it, and I did not remember it
at all.

> > Following the other attribute names, the proper name could be
> > pwm[1-*]_auto_point[1-*]_fan_target.
> 
> That sounds better and more consistent than the name chosen for the
> f71805f.

What is bad is that the name used in the f71805f driver isn't part of
the standard. Changing it now would be worse. Using a different name in
your driver would be even worse. So you may not like it, but using it
would be preferred. And documenting it, this time.

> I am not very happy about using the pwm* prefix, since the fan speed
> need not be driven using PWM at all.  This issue alone probably doesn't
> warrant coming up with a more abstract naming scheme; unfortunately
> there are more.  But still, until such a scheme exists, I'll stick to
> your proposal.

"pwm" in all attribute names really mean "fan control". I agree it can
be confusing and was a design mistake in the first place. At least I'm
not to blame this time.

> > (...)
> > One (now) very obvious mistake I made back then is that I did not
> > abstract the fan speed controller units from PWM outputs.
> 
> Right, that's exactly my remaining unhappiness above was about.
> 
> Let me add two more things.
> 
> First, I doubt that it's a good idea to put the fan*_target value into
> the fan domain.  After all, this value is about fan *control*, not about
> fan state, which I think suggests it might better reside in the pwm
> domain (that is, the domain of the fan controller).  This is emphasized
> by having fan*_target in the fan domain, and pwm*_auto_point*_fan_target
> in the pwm domain, as you suggested above.  One domain should win here,
> and I think it should be the latter.

You're right. fan[1-*]_target was my idea (if I remember correctly) and
shows that I did not notice that "target speed control mode" was a
degenerated case of "closed-loop temperature trip point driven control
mode". OTOH it isn't always clear if we want to represent degenerated
cases differently to present a more simple interface to the user, or
not to keep things as consistent as possible. No doubt that this is
something we will have to revisit if/when designing a brand new
interface.

> Second, the F75387 draws a distinction between the actual (PWM or DAC)
> output value and the configured target value.  The current definition of
> pwm*, on the other hand, not only encodes the steering method (PWM), but
> also mixes target value and actual value.  Does it make sense to keep
> these values separate in sysfs?

How could the actual PWM duty cycle ever differ from the desired one? I
don't get it.

-- 
Jean Delvare

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

On Sat, Feb 11, 2012 at 01:12:08AM +0100, Jean Delvare wrote:
> On Fri, 10 Feb 2012 23:56:07 +0100, Nikolaus Schulz wrote:
> > On Fri, Feb 10, 2012 at 09:05:19AM +0100, Jean Delvare wrote:
> > > [...] if what your chip does is multiple RPM targets based on the
> > > temperature, i.e. a mix of temperature trip points mode and fan speed
> > > target mode, then indeed our sysfs interface is lacking standard file
> > > names for this
> > 
> > Yes, this is what the F75387 does in closed loop mode.  All fan target
> > values are then specified in RPM, not PWM, including the
> > temperature-bounded for automatic fan control.
> > 
> > > (I can't remember another chip supporting this.)
> > 
> > Apparently the F71805F does this, and it is supported by the hwmon
> > driver.  This driver uses the sysfs filenames pwm*_auto_point*_fan.
> 
> Wow. This is my driver, I didn't write this part of the code but I
> signed than patch so I must have reviewed it, and I did not remember it
> at all.
> 
> > > Following the other attribute names, the proper name could be
> > > pwm[1-*]_auto_point[1-*]_fan_target.
> > 
> > That sounds better and more consistent than the name chosen for the
> > f71805f.
> 
> What is bad is that the name used in the f71805f driver isn't part of
> the standard. Changing it now would be worse. Using a different name in
> your driver would be even worse. So you may not like it, but using it
> would be preferred. And documenting it, this time.

Okay.

This should then go into both the sysfs interface doc and the driver
specific doc.  I'm willing to try that.

[...]

> > Second, the F75387 draws a distinction between the actual (PWM or DAC)
> > output value and the configured target value.  The current definition of
> > pwm*, on the other hand, not only encodes the steering method (PWM), but
> > also mixes target value and actual value.  Does it make sense to keep
> > these values separate in sysfs?
> 
> How could the actual PWM duty cycle ever differ from the desired one? I
> don't get it.

At least in theory it can, since the desired PWM duty cycle is bound to
the open loop mode.  If the fan controller runs in closed loop mode,
that setting might be ignored.

One can consider this an example of your (smart) suggestion to expose
r/w sysfs configuration files for all modes, including inactive ones, so
that the user can use the sysfs files to compose a complete
configuration for a mode that is currently inactive, and activate the
mode when he is done, thereby activating all corresponding settings in
one go.

Nikolaus

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

Re: [lm-sensors] [RFC] (F75387) How expose automatic fan control in sysfs?

[Nikolaus Schulz]

On Sat, Feb 11, 2012 at 07:28:24PM +0100, Nikolaus Schulz wrote:
> > > Second, the F75387 draws a distinction between the actual (PWM or DAC)
> > > output value and the configured target value.  The current definition of
> > > pwm*, on the other hand, not only encodes the steering method (PWM), but
> > > also mixes target value and actual value.  Does it make sense to keep
> > > these values separate in sysfs?
> > 
> > How could the actual PWM duty cycle ever differ from the desired one? I
> > don't get it.
> 
> At least in theory it can, since the desired PWM duty cycle is bound to
> the open loop mode.  If the fan controller runs in closed loop mode,
> that setting might be ignored.
> 
> One can consider this an example of your (smart) suggestion to expose
> r/w sysfs configuration files for all modes, including inactive ones, so
> that the user can use the sysfs files to compose a complete
> configuration for a mode that is currently inactive, and activate the
> mode when he is done, thereby activating all corresponding settings in
> one go.

Here actually lies another problem when programming the F78387.

This chip exposes the current duty cycle in a register that is always
r/o.  The fan is steered using a second register, which is used in both
open loop mode, where it is interpreted as a PWM value, and closed loop
mode, where it is interpreted as a RPM value.  (In automatic mode, this
steering register is automatically set, depending on the temperature.)

This is similar to the problem with the automatic mode of the F75387 I
mentioned in my initial post: the configuration register is reused, thus
a switch between manual open loop mode and manual closed loop mode is a
dangerous operation.

To support such a mode switch safely, there should be a way to set the
values for the new mode before the switch.

For automatic mode, you proposed to always expose separate r/w sysfs
files for both open and closed loop mode, and handle this in the driver.

For manual mode, switching from open to closed loop can be done by
writing to fan*_target before the switch, and again handle this in the
driver.  However, the other direction doesn't work, since the pwm* file
is overloaded, providing measurement *and* configuration.  What can be
done here?  Introduce a new file pwm*_target?

I have to say I am not particularly excited about this.  Better ideas
are welcome.

Nikolaus

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