On 2/5/22 13:05, Christian Lamparter wrote:
On 05/02/2022 19:23, Guenter Roeck wrote:
On Thu, Feb 03, 2022 at 10:28:53PM +0100, Christian Lamparter wrote:
Adds thermal_cooling device support to the tc654/tc655
driver. This make it possible to integrate it into a
device-tree supported thermal-zone node as a
cooling device.
I have been using this patch as part of the Netgear WNDR4700
Centria NAS Router support within OpenWrt since 2016.
Note: Driver uses imply THERMAL. The driver previously worked fine with
just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the
devm_thermal_of_cooling_device_register() will be a
"return ERR_PTR(-ENODEV)" stub.
What good does this do ? THERMAL is bool, so it is either there
or it isn't, and the IS_ENABLED() in the code handles that.
According to Kconfig language, "imply THERMAL" means that
THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would,
if it were supported, be clearly wrong in that case.
Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the
module Kconfig's option"), THERMAL was tristate, and you would have
needed something like "depends on THERMAL || THERMAL=n". This
is, however, no longer needed.
Given this, I really don't understand what the imply is expected
to do. The above text does not explain that. Please either drop
the imply or provide a better explanation why it is needed.
Oh, okay. Yes, you are right. A lot happend since 2016, I have to admit,
I was updating the driver code, but haven't kept track with the THERMAL
change.
So, I'm planning to address your comments in the following way:
- Drop imply THERMAL (no replacement)
- add __maybe_unused to the new functions+struct
In the !THERMAL case, compilers will spot the static declarations.
If you prefer, I could instead add a #ifdef CONFIG_THERMAL [...] #endif
around the new cooling functions and the tc654_fan_cool_ops struct.
- Changed in tc654_probe() that IS_ENABLED(CONFIG_THERMAL) to
IS_BUILTIN(CONFIG_THERMAL)
Please no #ifdef, and __maybe_unused should not be necessary unless you use #ifdef.
I don't understand why IS_BUILTIN() instead of IS_ENABLED() would be necessary.
/*
* IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
* otherwise. For boolean options, this is equivalent to
* IS_ENABLED(CONFIG_FOO).
*/
Thanks,
Guenter
