On 2025-05-06 10:49:37+0800, Sung-Chi Li wrote:
> 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>
<snip>
> > > 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`.
Agreed.
> > > +
> > > + 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.
Sounds good in general.
It should match what a potential future HWMON_C_REGISTER_TCD would do.
Which in turn should mirror HWMON_C_REGISTER_TZ, if that has a specific
naming scheme.
> > Error handling for devm_kasprintf() is missing.
> >
>
> Thank you for catching this, I will skip registering that device if the
> devm_kasprintf() fails.
This should also mirror what HWMON_C_REGISTER_TZ is doing on errors.
If I read the code correctly, probing is aborted there.
>
> > > + &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?
Same as above.
<snip>
