Re: [PATCH v1] thermal/of: Introduce k-po, k-pu and k-i for a thermal zone

[Lukasz Luba]

Hi Rob,

On 8/25/20 12:09 AM, Rob Herring wrote:
> On Tue, Aug 11, 2020 at 08:31:15PM +0800, Finley Xiao wrote:
> > The default value for k_pu is:
> >      2 * sustainable_power / (desired_temperature - switch_on_temp)
> > The default value for k_po is:
> >      sustainable_power / (desired_temperature - switch_on_temp)
> > The default value for k_i is 10.
> >
> > Even though these parameters of the PID controller can be changed
> > by the following sysfs files:
> >      /sys/class/thermal/thermal_zoneX/k_pu
> >      /sys/class/thermal/thermal_zoneX/k_po
> >      /sys/class/thermal/thermal_zoneX/k_i
> >
> > But it's still more convenient to change the default values by devicetree,
> > so introduce these three optional properties. If provided these properties,
> > they will be parsed and associated with the thermal zone via the thermal
> > zone parameters.
> >
> > Signed-off-by: Finley Xiao <finley.xiao@xxxxxxxxxxxxxx>
> > ---
> >   Documentation/devicetree/bindings/thermal/thermal.txt | 14 ++++++++++++++
>
> Bindings should be a separate file and this one is a DT schema now.
>
> >   drivers/thermal/thermal_of.c                          |  7 +++++++
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> > index f78bec19ca35..ebe936b57ded 100644
> > --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> > +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> > @@ -165,6 +165,20 @@ Optional property:
> >   			2000mW, while on a 10'' tablet is around
> >   			4500mW.
> >   
> > +- k-po:			Proportional parameter of the PID controller when
> > +			current temperature is above the target.
> > +  Type: signed
> > +  Size: one cell
> > +
> > +- k-pu:			Proportional parameter of the PID controller when
> > +			current temperature is below the target.
> > +  Type: signed
> > +  Size: one cell
> > +
> > +- k-i:			Integral parameter of the PID controller.
> > +  Type: signed
> > +  Size: one cell
>
> What's PID?
>
> I know nothing about the sysfs params, but the binding needs to stand on
> it's own and needs enough detail to educate me.

Sorry for the delay, I missed that patch.
These parameters are the coefficients for the
Proportional-Integral-Derivative (PID) controller [1], which is the
core of the Intelligent Power Allocation (IPA) thermal governor.

Only IPA uses them, thus I don't think the governors parameters:
k-po, k-pu, k-i
should be part of the DeviceTree. I haven't seen such governors
tunnables in the DT, please point me if they exist somewhere.

Do you think Rob they might be specified in the DT?

Regards,
Lukasz

[1] https://en.wikipedia.org/wiki/PID_controller