On Sat, May 03, 2025 at 09:27:18AM +0200, Thomas Weißschuh wrote:
> On 2025-05-02 13:34:47+0800, Sung-Chi Li via B4 Relay wrote:
> > From: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> >
> > Register fans connected under EC as thermal cooling devices as well, so
> > these fans can then work with the thermal framework.
> >
> > During the driver probing phase, we will also try to register each fan
> > as a thermal cooling device based on previous probe result (whether the
> > there are fans connected on that channel, and whether EC supports fan
> > control). The basic get max state, get current state, and set current
> > state methods are then implemented as well.
> >
> > Signed-off-by: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> > ---
> > Documentation/hwmon/cros_ec_hwmon.rst | 2 ++
> > drivers/hwmon/cros_ec_hwmon.c | 66 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 68 insertions(+)
> >
> > diff --git a/Documentation/hwmon/cros_ec_hwmon.rst b/Documentation/hwmon/cros_ec_hwmon.rst
> > index 5b802be120438732529c3d25b1afa8b4ee353305..82c75bdaf912a116eaafa3149dc1252b3f7007d2 100644
> > --- a/Documentation/hwmon/cros_ec_hwmon.rst
> > +++ b/Documentation/hwmon/cros_ec_hwmon.rst
> > @@ -27,3 +27,5 @@ Fan and temperature readings are supported. PWM fan control is also supported if
> > the EC also supports setting fan PWM values and fan mode. Note that EC will
> > switch fan control mode back to auto when suspended. This driver will restore
> > the fan state before suspended.
> > +If a fan is controllable, this driver will register that fan as a cooling device
> > +in the thermal framework as well.
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > index c5e42e2a03a0c8c68d3f8afbb2bb45b93a58b955..abfcf44fb7505189124e78c651b0eb1e0533b4e8 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -13,6 +13,7 @@
> > #include <linux/platform_device.h>
> > #include <linux/platform_data/cros_ec_commands.h>
> > #include <linux/platform_data/cros_ec_proto.h>
> > +#include <linux/thermal.h>
>
> Needs a dependency on CONFIG_THERMAL.
>
I think adding the `if (!IS_ENABLED(CONFIG_THERMAL))` you suggested is
sufficient, and turning on or off CONFIG_THERMAL both can compile, so I'll only
add the guarding statement in the `cros_ec_hwmon_register_fan_cooling_devices`.
> > +
> > + if (!priv->fan_control_supported)
> > + return;
> > +
> > + for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > + if (!(priv->usable_fans & BIT(i)))
> > + continue;
> > +
> > + cpriv = devm_kzalloc(dev, sizeof(*cpriv), GFP_KERNEL);
> > + if (!cpriv)
> > + return;
> > +
> > + cpriv->hwmon_priv = priv;
> > + cpriv->index = i;
> > + devm_thermal_of_cooling_device_register(
> > + dev, NULL, devm_kasprintf(dev, GFP_KERNEL, "cros-ec-fan%zu", i), cpriv,
>
> What happens for multiple/chained ECs? If both provide sensors the
> thermal device names will collide.
>
How about changing the "cros-ec-fan%zu" to "%s-fan%zu", which prefixes the
`dev_name()`? Here is an example from a device: cros-ec-hwmon.12.auto-fan0.
> Error handling for devm_kasprintf() is missing.
>
Thank you for catching this, I will skip registering that device if the
devm_kasprintf() fails.
> > + &cros_ec_thermal_cooling_ops);
>
> Error handling for devm_thermal_of_cooling_device_register() is missing.
>
I think we should continue registering other fans, so maybe we add a warning
here if the registration fails?
> > + }
> > +}
> > +
> > static int cros_ec_hwmon_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -402,6 +467,7 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
> > cros_ec_hwmon_probe_fans(priv);
> > priv->fan_control_supported =
> > cros_ec_hwmon_probe_fan_control_supported(priv->cros_ec);
> > + cros_ec_hwmon_register_fan_cooling_devices(dev, priv);
> >
> > hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> > &cros_ec_hwmon_chip_info, NULL);
> >
> > --
> > 2.49.0.906.g1f30a19c02-goog
> >
> >
