Re: [PATCH v2 06/14] arm64: dts: rockchip: Remove #cooling-cells from fan on Theobroma boards

[Dragan Simic <dsimic@manjaro.org>]

Hello Quentin,

On 2024-10-14 17:39, Quentin Schulz wrote:
> On 10/9/24 9:16 AM, Dragan Simic wrote:
>> On 2024-10-08 22:39, Heiko Stuebner wrote:
>>> All Theobroma boards use a ti,amc6821 as fan controller.
>>> It normally runs in an automatically controlled way and while it may 
>>> be
>>> possible to use it as part of a dt-based thermal management, this is
>>> not yet specified in the binding, nor implemented in any kernel.
>>> 
>>> Newer boards already don't contain that #cooling-cells property, but
>>> older ones do. So remove them for now, they can be re-added if 
>>> thermal
>>> integration gets implemented in the future.
>>> 
>>> Fixes: c484cf93f61b ("arm64: dts: rockchip: add PX30-µQ7 (Ringneck)
>>> SoM with Haikou baseboard")
>>> Fixes: d99a02bcfa81 ("arm64: dts: rockchip: add RK3368-uQ7 (Lion) 
>>> SoM")
>>> Fixes: 2c66fc34e945 ("arm64: dts: rockchip: add RK3399-Q7 (Puma) 
>>> SoM")
>>> Cc: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>> Cc: Klaus Goger <klaus.goger@theobroma-systems.com>
>>> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>>> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>> 
>> Looking good to me, thanks for the patch.  In addition to the amc6821
>> driver currently not supporting full integration into the thermal
>> framework, the "fan" DT node also isn't referenced in any cooling map,
>> so having it define the "cooling-cells" property is of no use.
>> 
>> By the way, it would be nice to see the amc6821 driver supporting fan
>> speed regulation, and test it to check who does a better job when it
>> comes to cooling and fan speed regulation, the thermal framework or
>> the chip's built-in logic. :)
> 
> Wasn't this feature added this summer by Guenter?
> 
> c.f. 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hwmon/amc6821.c?id=becbd16ed2f8f427239ffda66b5d894008bc56af
> 
> Mode 4 is
> https://elixir.bootlin.com/linux/v6.11.3/source/drivers/hwmon/amc6821.c#L367
> ([FDRC1:FDRC0] = [01] -> Software-RPM Control Mode (Fan Speed
> Regulator) according to the datasheet).

Ah, SENSOR_DEVICE_ATTR_RW(fan1_target, fan, IDX_FAN1_TARGET)...
How did I miss that?  Hmm...  Maybe I was looking at some older
local branch, which happened not to include that commit.

Anywyay, good to know, thanks.

> In any case, we cannot compare those for our products as we do not
> have a genuine AMC6821 but a handmade simulation of the IP we run in
> an MCU.

I seem to remember your MCU that performs a few tasks, back from
some related discussions.  I wonder what was the reason to implement
it in software, instead of using actual fan controller chip?

> But that'd be an interesting data point indeed :)

I'm glad that you agree. :)