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

Andreas Mohr
2014-04-28T04:58:13Z

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