Re: [PATCH v2 3/3] hwmon: (cros_ec) register fans into thermal framework cooling devices

[Thomas Weißschuh]

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>