lkml.org 
[lkml]   [2014]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 0/4] acerhdf/thermal: adding new models and appropriate governor
    Date
    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;


    \
     
     \ /
      Last update: 2014-04-28 01:21    [W:4.022 / U:0.132 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site