On Wed, Jul 15, 2015 at 05:12:41AM +0300, Paul Irofti wrote:
> I am not familiar with all the fan hack specifics so please keep that in mind
> if my questions and comments seem trivial.
>
> > This is an attempt to solve the problem slightly differently.
> > - Hook into acpitz and only speed the fan up when it is requesting active
> > cooling
> > - Never set the fan to a mode that would endanger the hardware should we
> > crash
>
> Your diff applies to all Thinkpad models. Is that okay?
It applies to all Thinkpads that have a sensible value in
THINKPAD_ECOFFSET_FANLEVEL, as you noticed. There is no flag or
documentation indicating the existence of this register as far as I
know. It's somewhat of a tradition, every TP I've owned had it.
My hope is if they drop support or move the offset, the value will
change and we won't do any damage.
I'm thinking this can be improved further by querying ACPI for _AC, so
it doesn't ever interfere with acpitz_setfan(). I'll look into this.
>
> >
> > PS: It would be nice if there was a function to add cooling methods to
> > acpitz eg: acpitz_add(void (*fn)(struct acpitz_softc *, void *), void *arg)
> > I tried but getting struct acpitz_softc into a header is a bit messy.
>
> If needed you can move the softc in the acpivar.h header file.
> Nothing messy about it. What other model drivers would need this though?
So far none, but I imagine we will see more software controlled power
management in the future. I guess I just don't like global variables and
extern declarations :)
>
> thinkpad_hotkey, sc, ACPIDEV_POLL);
> > +
> > + /* Make sure fan is in auto mode, otherwise we're not sure of support */
> > + acpiec_read(acpi_softc->sc_ec, THINKPAD_ECOFFSET_FANLEVEL, 1, &level);
> > + if (level == THINKPAD_ECFANLEVEL_AUTO)
> > + acpitz_activecool = thinkpad_activecool;
>
> Why is there no support if the fan is not in auto mode?
Trying not to break future models.
>
> > + else if (tmp > psv-50)
> > + /* EC firmware fan control is too slow in some models. When
> > + * we're getting within 5C of active cooling mode, turn the
> > + * fan to MAX. Helps with oscillation between blast and auto */
>
> This comment does not follow KNF :-)
Heh, I'll fix that to make you happy ;)
>
> > + nlevel = THINKPAD_ECFANLEVEL_MAX;
> > + else
> > + nlevel = THINKPAD_ECFANLEVEL_AUTO;
> > +
> > + if (nlevel != level) {
> > + acpiec_write(acpi_softc->sc_ec, THINKPAD_ECOFFSET_FANLEVEL, 1,
> > + &nlevel);
> > + level = nlevel;
> > + }
> > }
> > Index: acpitz.c
> > ===================================================================
> > RCS file: /home/vcs/cvs/openbsd/src/sys/dev/acpi/acpitz.c,v
> > retrieving revision 1.49
> > diff -u -p -r1.49 acpitz.c
> > --- acpitz.c 6 May 2015 01:41:55 -0000 1.49
> > +++ acpitz.c 14 Jul 2015 23:52:14 -0000
> > @@ -86,6 +86,7 @@ int acpitz_setfan(struct acpitz_softc *,
> > void acpitz_init(struct acpitz_softc *, int);
> >
> > void (*acpitz_cpu_setperf)(int);
> > +void (*acpitz_activecool)(int, int) = NULL;
> > int acpitz_perflevel = -1;
> > extern void (*cpu_setperf)(int);
> > extern int perflevel;
> > @@ -427,6 +428,11 @@ acpitz_refresh(void *arg)
> > acpitz_setfan(sc, i, "_OFF");
> > }
> > }
> > +
> > + /* active cooling hook */
> > + if (acpitz_activecool)
> > + acpitz_activecool(sc->sc_tmp, sc->sc_psv);
> > +
>
> I'm not sure, but is it ever possible for acpitz to attach BEFORE
> acpithinkpad and render this check false?
> Would you need to somehow tie the two together?
The check evaluating to false is not a problem, it will become true in
time, it's a slow running loop. I didn't see any locking around
acpitz_cpu_setperf(), so I'm guessing it's fine
