Re: [PATCH] Request driver inclusion - acer aspire one fan control

[Borislav Petkov]

Hi,

On Thu, Jun 18, 2009 at 1:49 PM, Peter Feuerer<peter@xxxxxxxx> wrote:
>>>>> Actually I think pre_suspend_kernelmode is needed, so it won't be
>>>>> dropped.
>>>>
>>>> and it is needed, because...?
>>>
>>> It's needed because we do now a clean revert to bios mode before we
>>> suspend.
>>> And after resume we have to switch to kernelmode again, if the driver was
>>> in
>>> kernelmode before suspend. So we need to keep track of in what state the
>>> driver was before suspending. That's what's this variable is for.
>>
>> You've got that state in the 'kernelmode' variable. See full comment:
>> http://marc.info/?l=linux-kernel&m=124482114200865
>
> We are talking about patch 0.5.9 and not 0.5.8, are we?
> http://patchwork.kernel.org/patch/30733/mbox/
>
> have a look at at line 543:
> +       /* remember previous setting */
> +       pre_suspend_kernelmode = kernelmode;
> +
> +       if (kernelmode) {
> +               acerhdf_revert_to_bios_mode();
> +               if (acerhdf_thz_dev)
> +                       thermal_zone_device_update(acerhdf_thz_dev);
> +       }

ok, this starts to look quite a bit overengineered for no reason. First,
acerhdf_revert_to_bios_mode() sets the fan to auto. Then, you've added
a thermal_zone_device_update() call in there which does set the fan to
auto indirectly _again_. And we end up with _three_ variables which
represent only _one_ state. Here's what it should do:

suspend:
 - set fan to auto

resume:
 - the thermal layer figures out what to do based on the 'kernelmode' and
 current temperature.

That's it, everything else is too much.

I'll have a deeper look during the weekend.

-- 
Regards/Gruss,
Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/