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

[Andreas Mohr <andi@lisas.de>]

Hi,

On Mon, Apr 28, 2014 at 01:13:19AM +0200, Peter Feuerer wrote:
> Hi,
> 
> Andreas Mohr writes:
> 
> > { "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.

Yup, to get things out of the way now :)


[[BTW your mail environment seems configured to have trailing blanks -
might want to "optimize" that away...]]


> > -       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.

ok. Does not seem too risky, and reduces the need for future updates.

> > "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
>  */

Nicely detailed.


About the _t typedef: it's said that use of "_t" is undesirable anyway
due to being reserved for POSIX.
https://en.wikipedia.org/wiki/Typedef
http://stackoverflow.com/questions/1186072/naming-scheme-for-typedefs
As an alternative using a full "_type" is ok right?
(that's what I've been doing...)

Andreas