Re: [PATCH] arm64: dts: sdm845: Add PSCI cpuidle low power states

[Raju P L S S S N]

On 10/24/2018 12:06 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Oct 15, 2018 at 10:02 AM Raju P.L.S.S.S.N
> <rplsssn@xxxxxxxxxxxxxx> wrote:
> > +               idle-states {
> > +                       entry-method = "psci";
> > +
> > +                       C0_CPU_SPC: c0_spc {
>
> nit: all these nodes should have dashes instead of spaces in the node
> names (labels can still have spaces).  AKA:
>
> C0_CPU_SPC: c0-spc {

Sure. I will change this. (However, from device tree specification v0.2, 
I see that Table 2.1 mentions underscore as valid character for node. 
Please correct me if I'm missing something)

> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param = <0x40000003>;
> > +                               entry-latency-us = <350>;
> > +                               exit-latency-us = <461>;
> > +                               min-residency-us = <1890>;
> > +                               local-timer-stop;
> > +                               idle-state-name = "pc";
>
> It seems weird that the idle state with the node name "spc" has the
> name "pc" and the idle state with the node name "pc" has the name
> "rail pc".  Can this be more consistent or is there a reason why they
> need to be mismatched?

Agree. They are confusing. Will change this.

> Also: AAHTUFSWDKWTM (acronyms are hard to understand for someone who
> doesn't know what they mean).  If you really need to use an the
> acronyms "PC" and "SPC" please document them somewhere.  In the very
> least the commit message, but having a comment in the file is good
> too.  ...or (even better) don't use the acronym and spell out what
> you're talking about.  Please correct me if I'm wrong, but I don't
> think it's obvious what the "PC" and "SPC" idle states mean.

Sure. Will change this.


Thanks for feedback Doug.

> -Doug