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

[Peter Feuerer <peter@piie.net>]

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;