Re: [PATCH 0/4] acerhdf/thermal: adding new models and appropriate governor

Peter Feuerer
2014-04-27T23:13:25Z

Hi,

Andreas Mohr writes:

> On Sun, Apr 27, 2014 at 03:23:31AM +0200, Peter Feuerer wrote:
>> This patch series is intended to:
>>
>> * Introduce "manual mode" support (Patch 1 & 2), which is needed to control
>> the fan of a few new models. Unfortunately this extends lines defining
>> the bios table over 80 characters, but all other methods make the code
>> really ugly and hard to read. So I hope for the reason of readability it
>> is ok to break this rule.
>
> Hmm... got an idea there. Possibly it's time to do away with direct
> "device name" <-> open-coded config data mappings.
> After all a specific device name is not really all too meaningful,
> can (and will) be invented out of thin air, with its reg config being
> identical to (read: painfully duplicated)
> several other names/BIOS versions in the series.
> So perhaps one should have a helper struct defined,
> with instances then named as particular base samples of a model series
> (ideally named after the precise internal development code name of the series),
> to then be referenced by all model/BIOS names which match.
>
> struct {
> struct reg_feat_1;
> struct reg_feat_2;
> } aao_reg_map;
>
> static const aao_reg_map aao_reg_map_AOAxxx_Acer_orig_version;
>
>
>
>
> { "Acer", "AOA1....", &aao_reg_map_AOAxxx_Acer_orig_version },
>
> Of course you then have the indirection of device name <-> specific
> register values (quote: "really ugly and hard to read"?),
> but IMHO that's ok since normally you wouldn't be too focused
> on looking up register values (...right!?).
>
> And if the next interface-breaking config change came along,
> you'd otherwise have to add yet another register index pair...
> (at which point some 100+ char line monsters
> would be breathing down our neck...)

I think we have been discussing this solution a year ago or something and
seems like it is really time to implement it. As I wrote in the other mail
to Boris, I'd like to just do a minor modification for now and then when
those 4 patches have been applied concentrate on implementing the splitted
structs.

> Model additions:
> Ain't there one MODULE_ALIAS missing?? (7 new models <-> 6 entries!?!?)
> "Aspire One 753"? But perhaps that's already implicitly covered by
> another existing entry? [if so, the commit log did not mention it ;)]

You are right, alias for 753 is missing, will add it for the next patch set.

>> * Add an appropriate thermal governor (Patch 3 & 4). Manipulating and
>> fiddling around with the step-wise governor has been a very fragile thing
>> in the past and as it broke again, I used the opportunity to add a two
>> point thermal governor which implements the actual fan handling required by
>> acerhdf and puts from my point of view things straight.
>
> I'm afraid I don't have the full picture,
> but so far it seems that this factoring out of common handling
> is a very good idea.

ok.

> - depends on THERMAL && ACPI
> + depends on THERMAL && ACPI && THERMAL_GOV_BANG_BANG
> Do we actively depend on THERMAL (code-wise, I mean?) Or is it now an
> implicit dependency given that we request THERMAL_GOV_BANG_BANG? If
> implicit, then THERMAL probably ought to be removed. But if we use
> generic thermal APIs (which we probably do), then of course we do have
> that dependency....

There's an implicit dependency due to the request of THERMAL_GOV_BANG_BANG, so
yes, we could remove THERMAL here.

> "bang_bang_throttle - throttles devices asscciated with the given zone"
>
> Typo ;)

c != o, got it.

> "used to force thermal" --> misleading ("we used to do this, but it's
> bad so we better do that").
>
> "intended to"? "established to"? "added to"? or some simpler wording?

What do you think about this wording:
/*
* this struct is used to instruct thermal layer to use bang_bang instead of
* default governor for acerhdf
*/

> pr_err("Thermal governor %s is not compiled into thermal subsystem\n"
> --> you are lying here... (the only thing we can reliably indicate
> is that we did not get the expected name -
> so we should perhaps indicate something like we "didn't get bang-bang,
> since perhaps not compiled into thermal subsystem").

Fixed in next submit.

>> Please test/review the patches and send me your comments.
>
> -ENODATA (my crappy JMicron JMF601 SSD had managed to break again,
> this time with fatal firmware corruption, so I had to reflash
> firmware to resurrect it, but I haven't restored my environment yet,
> but I'll obviously report back immediately if something comes up)

Ok, good luck with this.

Thanks for all the good input.

--
kind regards,
--peter;