Re: [PATCH] hwmon: (cros_ec) register thermal sensors to thermal framework

["Thomas Weißschuh" <thomas@weissschuh.net>]

Hi!

On 2024-11-11 15:49:04+0800, Sung-Chi, Li wrote:
> cros_ec hwmon driver probes available thermal sensors when probing the
> driver.  Register these thermal sensors to the thermal framework, such
> that thermal framework can adopt these sensors as well.
> 
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
>  drivers/hwmon/cros_ec_hwmon.c | 69 +++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 5514cf780b8b..4b1ea431e3d2 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -7,20 +7,31 @@
>  
>  #include <linux/device.h>
>  #include <linux/hwmon.h>
> +#include <linux/list.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #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>
>  #include <linux/types.h>
>  #include <linux/units.h>
>  
> -#define DRV_NAME	"cros-ec-hwmon"
> +#define DRV_NAME		"cros-ec-hwmon"
> +#define THERMAL_VAL_OFFSET	200
>  
>  struct cros_ec_hwmon_priv {
>  	struct cros_ec_device *cros_ec;
>  	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>  	u8 usable_fans;
> +	struct list_head sensors;
> +};
> +
> +struct cros_ec_sensor_data {

cros_ec_hwmon_thermal_zone_data.

> +	struct cros_ec_device *cros_ec;
> +	struct thermal_zone_device *tz_dev;
> +	int addr;

This is not an address, but an index.

> +	struct list_head node;
>  };
>  
>  static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> @@ -185,11 +196,32 @@ static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
>  	.info = cros_ec_hwmon_info,
>  };
>  
> -static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv,
> +static int cros_ec_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> +	struct cros_ec_sensor_data *data = thermal_zone_device_priv(tz);
> +	int ret;
> +	u8 val;
> +
> +	ret = cros_ec_hwmon_read_temp(data->cros_ec, data->addr, &val);
> +	if (ret)
> +		return -ENODATA;
> +	*temp = (val + THERMAL_VAL_OFFSET - 273) * 1000;

Use cros_ec_hwmon_temp_to_millicelsius() and cros_ec_hwmon_is_error_temp().

> +	return 0;
> +}
> +
> +static const struct thermal_zone_device_ops thermal_ops = {
> +	.get_temp = cros_ec_thermal_get_temp,
> +};

Use the cros_ec_hwmon symbol prefix.

> +
> +static void cros_ec_hwmon_probe_temp_sensors(struct cros_ec_device *cros_ec,
> +					     struct device *dev,
> +					     struct cros_ec_hwmon_priv *priv,
> +					     struct list_head *head,
>  					     u8 thermal_version)
>  {
>  	struct ec_params_temp_sensor_get_info req = {};
>  	struct ec_response_temp_sensor_get_info resp;
> +	struct cros_ec_sensor_data *data;
>  	size_t candidates, i, sensor_name_size;
>  	int ret;
>  	u8 temp;
> @@ -216,6 +248,23 @@ static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_
>  		priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
>  							    (int)sensor_name_size,
>  							    resp.sensor_name);
> +
> +		data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +		if (!data)
> +			continue;
> +
> +		data->addr = i;
> +		data->cros_ec = cros_ec;
> +		data->tz_dev = devm_thermal_of_zone_register(
> +			cros_ec->dev, i, data, &thermal_ops);

I'm a bit confused. The uses thermal configuration from OF,
but this driver has no specific OF support.
Can you provide a usage example in the commit message?

Does it not need a binding documentation update?

> +		if (IS_ERR_VALUE(data->tz_dev)) {
> +			dev_err(dev,
> +				"failed to register %zu thermal sensor, err = %ld",
> +				i, PTR_ERR(data->tz_dev));

Use %pe.

> +			continue;
> +		}
> +
> +		list_add(&data->node, head);
>  	}
>  }
>  
> @@ -255,8 +304,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	priv->cros_ec = cros_ec;
> +	INIT_LIST_HEAD(&priv->sensors);
>  
> -	cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version);
> +	cros_ec_hwmon_probe_temp_sensors(cros_ec, dev, priv, &priv->sensors,
> +					 thermal_version);

cros_ec is already passed as priv->cros_ec.

>  	cros_ec_hwmon_probe_fans(priv);
>  
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv,
> @@ -265,6 +316,17 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev)
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>  
> +static void cros_ec_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +	struct cros_ec_sensor_data *iter;
> +
> +	list_for_each_entry(iter, &priv->sensors, node) {
> +		devm_thermal_of_zone_unregister(dev, iter->tz_dev);

This shouldn't be needed, the zone will be unregistered automatically by
the devm framework.
This also means that .remove and priv->sensors can go away.

> +	}
> +}
> +
>  static const struct platform_device_id cros_ec_hwmon_id[] = {
>  	{ DRV_NAME, 0 },
>  	{}
> @@ -273,6 +335,7 @@ static const struct platform_device_id cros_ec_hwmon_id[] = {
>  static struct platform_driver cros_ec_hwmon_driver = {
>  	.driver.name	= DRV_NAME,
>  	.probe		= cros_ec_hwmon_probe,
> +	.remove		= cros_ec_hwmon_remove,
>  	.id_table	= cros_ec_hwmon_id,
>  };
>  module_platform_driver(cros_ec_hwmon_driver);
> -- 
> 2.47.0.277.g8800431eea-goog
>