Re: [lm-sensors] [PATCH 1/2] hwmon: add generic GPIO fan driver

[Simon Guinot <simon@sequanux.org>]

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

Hi Guenter,

On Mon, Oct 18, 2010 at 11:52:21PM -0700, Guenter Roeck wrote:
> On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
> [ ... ]
> > > I don't really understand the value of supporting pwm attributes,
> > > since you have to convert those to rpm anyway. Why not just stick
> > > with fan1_input and fan1_target ? This would simplify the code a lot.
> > 
> > I don't know very well the hwmon API. I have simply been fooled by the
> > sysfs-interface document which claim that fan[1-*]_target only make
> > sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> > compliant with the fancontrol shell script...
> > 
> > But anyway, you are right. I just don't want the pwm interface.
> > 
> Thinking more about this, another option might be to keep using
> the pwm interface (for fancontrol), but simplify your code.
> Your transitions are currently pwm -> rpm -> ctrl and vice versa.
> However, direct conversion pwm -> ctrl should also be possible
> and would be much simpler than the two-stage conversion.
> 
> To do that, you could map num_speed directly to the pwm range of (0..255).
> Something like
> 
> 	pwm = DIV_ROUND_CLOSEST(speed_index * 255, num_speed - 1);
> and
> 	speed_index = pwm * (num_speed - 1) / 255;
> 
> Then use speed_index to get control value and rpm from the speed table.
> 
> Would that make sense ? You could then also provide fan1_target
> for direct fan speed control.

Unfortunately, I believe this approximation is not possible. Take a look
at the Network Space Max fan speed array:

/* Designed for fan 40x40x16: ADDA AD0412LB-D50 6000rpm@12v */
static struct gpio_fan_speed netspace_max_v2_fan_speed[] = {
        {    0,  0 },
        { 1500, 15 },
        { 1700, 14 },
        { 1800, 13 },
        { 2100, 12 },
        { 3100, 11 },
        { 3300, 10 },
        { 4300,  9 },
        { 5500,  8 },
};

The function rpm_speed(index) is really not linear. I am not sure about
the resulting behaviour when this will be used by a userland fan control
process...

If the pwm interface must be exported, I think we have to respect the
fan speed array. But as you said, maybe the work is rather on the
fancontrol script side.

As a first step, we could keep the heavy pwm interface. Once the
fancontrol script (or something else) will be able to handle the rpm
attributes, we will drop the pwm compatibility.
Simply remove the gpio-fan pwm interface seems a good option too...

Both make sense for me. Let me know your favourite way.

Thanks,

Simon

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 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