Received-SPF: pass (google.com: domain of linux-kernel+bounces-46156-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240130-rk-dts-additions-v2-0-c6222c4c78df@gmail.com>
 <20240130-rk-dts-additions-v2-2-c6222c4c78df@gmail.com> <8115ab382115f66aa01dd3d00aaae474@manjaro.org>
In-Reply-To: <8115ab382115f66aa01dd3d00aaae474@manjaro.org>
From: Alexey Charkov <alchark@gmail.com>
Date: Wed, 31 Jan 2024 13:43:19 +0400
Message-ID: <CABjd4YyP0trjBi+Djau-n69vg6-s2SdPWwmWK9fT626sKST8EA@mail.gmail.com>
Subject: Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven
 fan control on Rock 5B
To: Dragan Simic <dsimic@manjaro.org>
Cc: Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Heiko Stuebner <heiko@sntech.de>, Daniel Lezcano <daniel.lezcano@linaro.org>, 
	Viresh Kumar <viresh.kumar@linaro.org>, devicetree@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 9:08=E2=80=AFAM Dragan Simic <dsimic@manjaro.org> w=
rote:
>
> Hello Alexey,
>
> Some notes below, please have a look.
>
> On 2024-01-30 19:21, Alexey Charkov wrote:
> > This enables thermal monitoring on Radxa Rock 5B and links the PWM
> > fan as an active cooling device managed automatically by the thermal
> > subsystem, with a target SoC temperature of 65C and a minimum-spin
> > interval from 55C to 65C to ensure airflow when the system gets warm
>
> I'd suggest that you replace "temperature driven fan control" with
> "active cooling" in the patch subject.  More concise and reads better.

Agreed, thanks!

> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
> > ++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index a0e303c3a1dc..b485edeef876 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -52,7 +52,7 @@ led_rgb_b {
> >
> >       fan: pwm-fan {
> >               compatible =3D "pwm-fan";
> > -             cooling-levels =3D <0 95 145 195 255>;
> > +             cooling-levels =3D <0 120 150 180 210 240 255>;
> >               fan-supply =3D <&vcc5v0_sys>;
> >               pwms =3D <&pwm1 0 50000 0>;
> >               #cooling-cells =3D <2>;
> > @@ -173,6 +173,34 @@ &cpu_l3 {
> >       cpu-supply =3D <&vdd_cpu_lit_s0>;
> >  };
> >
> > +&package_thermal {
> > +     polling-delay =3D <1000>;
> > +
> > +     trips {
> > +             package_fan0: package-fan0 {
> > +                     temperature =3D <55000>;
> > +                     hysteresis =3D <2000>;
> > +                     type =3D "active";
> > +             };
> > +             package_fan1: package-fan1 {
> > +                     temperature =3D <65000>;
> > +                     hysteresis =3D <2000>;
> > +                     type =3D "active";
> > +             };
> > +     };
> > +
> > +     cooling-maps {
> > +             map0 {
>
> Should be "map1" instead of "map0".  There's already "map0"
> defined for "package_thermal" in the RK3588(s) dtsi file.

Indeed. I got overzealous renaming everything to be zero-based.

> > +                     trip =3D <&package_fan0>;
> > +                     cooling-device =3D <&fan THERMAL_NO_LIMIT 1>;
> > +             };
> > +             map1 {
>
> Should be "map2" instead of "map1".

Noted, thanks!

> > +                     trip =3D <&package_fan1>;
> > +                     cooling-device =3D <&fan 1 THERMAL_NO_LIMIT>;
>
> Should be "cooling-device =3D <&fan 2 THERMAL_NO_LIMIT>;"
> (i.e., "2 THERMAL_NO_LIMIT" instead of "1 THERMAL_NO_LIMIT").
>
> The first fan speed is already covered by the first cooling map.
> The second cooling map takes over from the second fan speed.

Makes sense, will adjust, thank you!

Best regards,
Alexey